Bug 30863

Summary: [Qt] Fix linking on Linux 32-bit.
Product: WebKit Reporter: Jocelyn Turcotte <jturcotte>
Component: Tools / TestsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: agl, commit-queue, eric, kenneth, laszlo.gombos, thiago.macieira
Priority: P1 Keywords: Qt
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Attachments:
Description Flags
Patch
kenneth: review-
Patch v2
kenneth: review-
Patch v3 none

Description Jocelyn Turcotte 2009-10-28 09:08:06 PDT
It was missing the ".text" directive at the top of the file, indicating that code would follow. Without it, the assembler created "NOTYPE" symbols, which would result in linker errors.

* jit/JITStubs.cpp:
Comment 1 Jocelyn Turcotte 2009-10-28 09:09:41 PDT
Created attachment 42032 [details]
Patch
Comment 2 Mark Rowe (bdash) 2009-10-28 14:36:23 PDT
Comment on attachment 42032 [details]
Patch

> diff --git a/JavaScriptCore/jit/JITStubs.cpp b/JavaScriptCore/jit/JITStubs.cpp
> index c999618..9fa898a 100644
> --- a/JavaScriptCore/jit/JITStubs.cpp
> +++ b/JavaScriptCore/jit/JITStubs.cpp
> @@ -75,7 +75,7 @@ namespace JSC {
>  #define THUMB_FUNC_PARAM(name)
>  #endif
>  
> -#if PLATFORM(LINUX) && PLATFORM(X86_64)
> +#if PLATFORM(LINUX) && (PLATFORM(X86_64) || PLATFORM(X86))
>  #define SYMBOL_STRING_RELOCATION(name) #name "@plt"
>  #else
>  #define SYMBOL_STRING_RELOCATION(name) SYMBOL_STRING(name)

It’s not clear why it’s necessary given that this code is well-tested on 32-bit Linux already.  There’s nothing in the ChangeLog to explain it either.
Comment 3 Kenneth Rohde Christiansen 2009-11-10 04:39:02 PST
Comment on attachment 42032 [details]
Patch

Changing to r- due to the above comments
Comment 4 Thiago Macieira 2009-11-10 05:02:11 PST
That change (PLT) is unrelated, but makes sense.

Feel free to remove it.

But in any case, the convention didn't change between 32- and 64-bit. So if one architecture requires it, both should.
Comment 5 Jocelyn Turcotte 2009-11-11 10:33:51 PST
Created attachment 42976 [details]
Patch v2

Updated patch that just keep the
+".text\n"
line.
Comment 6 Kenneth Rohde Christiansen 2009-11-11 11:05:40 PST
Comment on attachment 42976 [details]
Patch v2

ChangeLog entry should be at the top of the file.
Comment 7 Eric Seidel (no email) 2009-11-11 11:15:02 PST
Comment on attachment 42976 [details]
Patch v2

@kenneth:  Actually I think svn-apply should handle this patch correctly regardless.  svn-apply has code to move ChangeLog entries to the top automatically.
Comment 8 Eric Seidel (no email) 2009-11-11 11:16:15 PST
@kenneth:  Nope. you were right.  svn-apply did not move this entry, so your r- was correct.

I used:
curl "https://bug-30863-attachments.webkit.org/attachment.cgi?id=42976" | svn-apply
to test my theory.
Comment 9 Mark Rowe (bdash) 2009-11-11 11:17:05 PST
svn-apply will only do it if the entry in the patch was at the top of the file to begin with.  In this case it’s 1000+ lines down in the file.
Comment 10 Jocelyn Turcotte 2009-11-12 01:28:44 PST
Created attachment 43044 [details]
Patch v3

Changelog entry moved to the top.
Comment 11 WebKit Commit Bot 2009-11-12 04:45:08 PST
Comment on attachment 43044 [details]
Patch v3

Clearing flags on attachment: 43044

Committed r50874: <http://trac.webkit.org/changeset/50874>
Comment 12 WebKit Commit Bot 2009-11-12 04:45:14 PST
All reviewed patches have been landed.  Closing bug.