Bug 30863 – [Qt] Fix linking on Linux 32-bit.

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Jocelyn Turcotte]

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:

[Jocelyn Turcotte]

Created attachment 42032 [details]
Patch

[Mark Rowe (bdash)]

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.

[Kenneth Rohde Christiansen]

Comment on attachment 42032 [details]
Patch

Changing to r- due to the above comments

[Thiago Macieira]

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.

[Jocelyn Turcotte]

Created attachment 42976 [details]
Patch v2

Updated patch that just keep the
+".text\n"
line.

[Kenneth Rohde Christiansen]

Comment on attachment 42976 [details]
Patch v2

ChangeLog entry should be at the top of the file.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

@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.

[Mark Rowe (bdash)]

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.

[Jocelyn Turcotte]

Created attachment 43044 [details]
Patch v3

Changelog entry moved to the top.

[WebKit Commit Bot]

Comment on attachment 43044 [details]
Patch v3

Clearing flags on attachment: 43044

Committed r50874: <http://trac.webkit.org/changeset/50874>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.