137434 2014-10-05 07:09:36 -0700 [JSC] Segfault on ARM64 with gcc in elf_dynamic_do_Rela 2014-10-09 08:59:16 -0700 1 1 1 Unclassified WebKit JavaScriptCore 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 108645 1 akiss webkit-unassigned commit-queue dbates fpizlo msaboff oliver ossy oldest_to_newest 1039593 0 akiss 2014-10-05 07:09:36 -0700 When building a debug jsc for ARM64/Linux/GTK with gcc (4.8.3, as shipped with Ubuntu 14.04.1), the resulting binary is completely unusable. The execution of the app stops with segfault very early, as revealed by gdb: Program received signal SIGSEGV, Segmentation fault. elf_dynamic_do_Rela (skip_ifunc=<optimized out>, lazy=0, nrelative=<optimized out>, relsize=<optimized out>, reladdr=<optimized out>, map=0x7fb7ffb770) at do-rel.h:112 112 do-rel.h: No such file or directory. This is still the dynamic linking phase of jsc to its dependencies. After digging deeper, it turned out that some dynamic relocations in libjavascriptcoregtk.so cause the problem. Further analysis revealed that some symbols in the object file compiled from jit/ThunkGenerators.cpp are in the wrong section, in .text instead of .data. The following is an excerpt from the output of objdump run on ThunkGenerators.cpp.o, i.e., from the symbol table: 0000000000000000 l O .data.rel 0000000000000008 _ZN3JSCL14jsRoundWrapperE 0000000000001e68 l O .text 0000000000000008 _ZN3JSCL10expWrapperE 0000000000001e78 l O .text 0000000000000008 _ZN3JSCL10logWrapperE 0000000000001e88 l O .text 0000000000000008 _ZN3JSCL12floorWrapperE 0000000000001e98 l O .text 0000000000000008 _ZN3JSCL11ceilWrapperE The following lines are the relevant part of the source code: #elif CPU(ARM64) #define defineUnaryDoubleOpWrapper(function) \ asm( \ ".text\n" \ ".align 2\n" \ ".globl " SYMBOL_STRING(function##Thunk) "\n" \ HIDE_SYMBOL(function##Thunk) "\n" \ SYMBOL_STRING(function##Thunk) ":" "\n" \ "b " GLOBAL_REFERENCE(function) "\n" \ ); \ extern "C" { \ MathThunkCallingConvention function##Thunk(MathThunkCallingConvention); \ } \ static MathThunk UnaryDoubleOpWrapper(function) = &function##Thunk; #else #define defineUnaryDoubleOpWrapper(function) \ static MathThunk UnaryDoubleOpWrapper(function) = 0 #endif defineUnaryDoubleOpWrapper(jsRound); defineUnaryDoubleOpWrapper(exp); defineUnaryDoubleOpWrapper(log); defineUnaryDoubleOpWrapper(floor); defineUnaryDoubleOpWrapper(ceil); And now comes the revelation: this bug has nothing to do with the GTK port but it's a bug in gcc. The following minimized example also exhibits the problem: #include <math.h> #define ASM_AND_VAR(fn) \ asm(".text"); \ static double (*var##fn)(double) = &fn; ASM_AND_VAR(exp); ASM_AND_VAR(log); The assembly output is: .cpu cortex-a53+fp+simd .file "asmtext.cpp" #APP .text #NO_APP .data .align 3 .type _ZL6varexp, %object .size _ZL6varexp, 8 _ZL6varexp: .xword exp #APP .text #NO_APP .align 3 .type _ZL6varlog, %object .size _ZL6varlog, 8 _ZL6varlog: .xword log After the second asm(".text"); (or rather: before the second static double variable) gcc does not insert a .data assembler directive. So, it seems GTK is only suffering because it puts the JS engine into a shared lib instead of linking it statically (the static linker gets over this issue). 1039595 1 239299 akiss 2014-10-05 07:20:39 -0700 Created attachment 239299 Proposed patch. 1039675 2 akiss 2014-10-06 01:22:28 -0700 Update: According to conversations with gcc devs (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63461), it seems that this is not a bug in gcc but expected (alternatively: intentionally undefined) behaviour. The solution proposed at gcc bugzilla is to use .pushsection/.popsection, however, I'd still propose for the current .text/.data approach. .text is apropriately translated to .section __TEXT,__text on Mac and to .section .text on Linux (.data similarly), while .pushsection ".text" would not be so portable. If I got it right. 1040107 3 msaboff 2014-10-07 13:07:11 -0700 (In reply to comment #2) > Update: According to conversations with gcc devs (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63461), it seems that this is not a bug in gcc but expected (alternatively: intentionally undefined) behaviour. > > The solution proposed at gcc bugzilla is to use .pushsection/.popsection, however, I'd still propose for the current .text/.data approach. .text is apropriately translated to .section __TEXT,__text on Mac and to .section .text on Linux (.data similarly), while .pushsection ".text" would not be so portable. If I got it right. What about using .previous instead of .data? That should be more portable. 1040522 4 239527 akiss 2014-10-09 04:47:11 -0700 Created attachment 239527 Proposed patch, v2 You're right, .previous works -- on my ARM64/Linux at least. (On that box, I've been experimenting with some corner cases to check that different compilers -- gcc & clang -- behave as expected, and found no issues, fortunately. I've also tried out some .pushsection/.popsection-based approaches but all turned out to need more and potentially harder to maintain changes.) I could not validate the patch for iOS. Interestingly, all the other defines use the same .text-based approach for defining the thunks with no .data/.previous/.pushsection-.popsection around and noone ran into this problem before. Still I guess that it would be safer to have .previous in those macros as well, but following the "if it's not broken, don't fix it" line, I've made no changes there. 1040548 5 239527 msaboff 2014-10-09 08:22:54 -0700 Comment on attachment 239527 Proposed patch, v2 r=me 1040557 6 239527 commit-queue 2014-10-09 08:59:13 -0700 Comment on attachment 239527 Proposed patch, v2 Clearing flags on attachment: 239527 Committed r174503: <http://trac.webkit.org/changeset/174503> 1040558 7 commit-queue 2014-10-09 08:59:16 -0700 All reviewed patches have been landed. Closing bug. 239299 2014-10-05 07:20:39 -0700 2014-10-09 04:47:11 -0700 Proposed patch. thunk-data-v1.patch text/plain 1937 akiss ZGlmZiAtLWdpdCBhL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cgYi9Tb3VyY2UvSmF2 YVNjcmlwdENvcmUvQ2hhbmdlTG9nCmluZGV4IGQ0NDdkM2MuLmM3NjkxMTggMTAwNjQ0Ci0tLSBh L1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKKysrIGIvU291cmNlL0phdmFTY3JpcHRD b3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDI1IEBACisyMDE0LTEwLTA1ICBBa29zIEtpc3MgIDxh a2lzc0BpbmYudS1zemVnZWQuaHU+CisKKyAgICAgICAgV29yayBhcm91bmQgQVJNNjQtR0NDIGJ1 ZyBpbiBkZWZpbmVVbmFyeURvdWJsZU9wV3JhcHBlcgorICAgICAgICBodHRwczovL2J1Z3Mud2Vi a2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTM3NDM0CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9C T0RZIChPT1BTISkuCisKKyAgICAgICAgVGhlIEFSTTY0IHZlcnNpb24gb2YgdGhlIGRlZmluZVVu YXJ5RG91YmxlT3BXcmFwcGVyIG1hY3JvIGluCisgICAgICAgIFRodW5rR2VuZXJhdG9ycy5jcHAg Y29udGFpbnMgaW5saW5lIGFzc2VtYmx5IHdpdGggLnRleHQgYXNzZW1ibGVyCisgICAgICAgIGRp cmVjdGl2ZSBmb2xsb3dlZCBieSBhIHN0YXRpYyB2YXJpYWJsZSBkZWNsYXJhdGlvbi4gVGhpcyBt YWNybyBnZXRzCisgICAgICAgIGV4cGFuZGVkIHNldmVyYWwgdGltZXMgYWZ0ZXJ3YXJkcywgaG93 ZXZlciwgb25seSBkdXJpbmcgdGhlIGNvbXBpbGF0aW9uCisgICAgICAgIG9mIHRoZSBmaXJzdCBl eHBhbnNpb24gZG9lcyBnY2MgaW5zZXJ0IGEgLmRhdGEgYXNzZW1ibGVyIGRpcmVjdGl2ZQorICAg ICAgICBiZWZvcmUgdGhlIGFzc2VtYmxlZCB2ZXJzaW9uIG9mIHRoZSBzdGF0aWMgdmFyaWFibGUu IFRodXMsIG9ubHkgdGhlCisgICAgICAgIGZpcnN0IHZhcmlhYmxlIGdldHMgYWxsb2NhdGVkIGlu IHRoZSAuZGF0YSBzZWN0aW9uLCBhbGwgdGhlIG90aGVycworICAgICAgICByZW1haW4gaW4gLnRl eHQuIElmIEphdmFTY3JpcHRDb3JlIGlzIGJ1aWx0IGFzIGEgc2hhcmVkIGxpYnJhcnkgdGhlbgor ICAgICAgICB0aGlzIGNhdXNlcyBhIHNlZ21lbnRhdGlvbiBmYXVsdCBkdXJpbmcgZHluYW1pYyBs aW5raW5nLgorCisgICAgICAgIFRoaXMgcGF0Y2ggcHV0cyBhbiBleHBsaWNpdCAuZGF0YSBkaXJl Y3RpdmUgYXQgdGhlIGVuZCBvZiB0aGUgaW5saW5lCisgICAgICAgIGFzc2VtYmx5IHRvIGVuc3Vy ZSB0aGF0IHRoZSBmb2xsb3dpbmcgdmFyaWFibGUgZ29lcyB0byB0aGUgcmlnaHQgcGxhY2UuCisK KyAgICAgICAgKiBqaXQvVGh1bmtHZW5lcmF0b3JzLmNwcDoKKwogMjAxNC0xMC0wMiAgRmlsaXAg UGl6bG8gIDxmcGl6bG9AYXBwbGUuY29tPgogCiAgICAgICAgIEZUTCBzaG91bGQgc2luayBQdXRM b2NhbHMKZGlmZiAtLWdpdCBhL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9qaXQvVGh1bmtHZW5lcmF0 b3JzLmNwcCBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9qaXQvVGh1bmtHZW5lcmF0b3JzLmNwcApp bmRleCBmMmE1Nzk0Mi4uMTc3NjcwOSAxMDA2NDQKLS0tIGEvU291cmNlL0phdmFTY3JpcHRDb3Jl L2ppdC9UaHVua0dlbmVyYXRvcnMuY3BwCisrKyBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9qaXQv VGh1bmtHZW5lcmF0b3JzLmNwcApAQCAtODAxLDYgKzgwMSw3IEBAIGRvdWJsZSBqc1JvdW5kKGRv dWJsZSBkKQogICAgICAgICBISURFX1NZTUJPTChmdW5jdGlvbiMjVGh1bmspICJcbiIgXAogICAg ICAgICBTWU1CT0xfU1RSSU5HKGZ1bmN0aW9uIyNUaHVuaykgIjoiICJcbiIgXAogICAgICAgICAi YiAiIEdMT0JBTF9SRUZFUkVOQ0UoZnVuY3Rpb24pICJcbiIgXAorICAgICAgICAiLmRhdGEiIFwK ICAgICApOyBcCiAgICAgZXh0ZXJuICJDIiB7IFwKICAgICAgICAgTWF0aFRodW5rQ2FsbGluZ0Nv bnZlbnRpb24gZnVuY3Rpb24jI1RodW5rKE1hdGhUaHVua0NhbGxpbmdDb252ZW50aW9uKTsgXAo= 239527 2014-10-09 04:47:11 -0700 2014-10-09 08:59:13 -0700 Proposed patch, v2 thunk-data-v3.patch text/plain 2063 akiss ZGlmZiAtLWdpdCBhL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cgYi9Tb3VyY2UvSmF2 YVNjcmlwdENvcmUvQ2hhbmdlTG9nCmluZGV4IGQ0NDdkM2MuLjg2N2UzNDggMTAwNjQ0Ci0tLSBh L1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKKysrIGIvU291cmNlL0phdmFTY3JpcHRD b3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDI2IEBACisyMDE0LTEwLTA5ICBBa29zIEtpc3MgIDxh a2lzc0BpbmYudS1zemVnZWQuaHU+CisKKyAgICAgICAgRW5zdXJlIHRoYXQgaW5saW5lIGFzc2Vt Ymx5IFRodW5rIGZ1bmN0aW9ucyBkb24ndCBjb25mbGljdCB3aXRoIHRoZSBzZWN0aW9uIGRlc2ln bmF0aW9ucyBvZiB0aGUgY29tcGlsZXIKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcv c2hvd19idWcuY2dpP2lkPTEzNzQzNAorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09Q UyEpLgorCisgICAgICAgIFRoZSBBUk02NCB2ZXJzaW9uIG9mIHRoZSBkZWZpbmVVbmFyeURvdWJs ZU9wV3JhcHBlciBtYWNybyBpbgorICAgICAgICBUaHVua0dlbmVyYXRvcnMuY3BwIGNvbnRhaW5z IGlubGluZSBhc3NlbWJseSB3aXRoIC50ZXh0IGFzc2VtYmxlcgorICAgICAgICBkaXJlY3RpdmUg Zm9sbG93ZWQgYnkgYSBzdGF0aWMgdmFyaWFibGUgZGVjbGFyYXRpb24uIFRoaXMgbWFjcm8gZ2V0 cworICAgICAgICBleHBhbmRlZCBzZXZlcmFsIHRpbWVzIGFmdGVyd2FyZHMsIGhvd2V2ZXIsIG9u bHkgZHVyaW5nIHRoZSBjb21waWxhdGlvbgorICAgICAgICBvZiB0aGUgZmlyc3QgZXhwYW5zaW9u IGRvZXMgZ2NjIGluc2VydCBhIC5kYXRhIGFzc2VtYmxlciBkaXJlY3RpdmUKKyAgICAgICAgYmVm b3JlIHRoZSBhc3NlbWJsZWQgdmVyc2lvbiBvZiB0aGUgc3RhdGljIHZhcmlhYmxlLiBUaHVzLCBv bmx5IHRoZQorICAgICAgICBmaXJzdCB2YXJpYWJsZSBnZXRzIGFsbG9jYXRlZCBpbiB0aGUgLmRh dGEgc2VjdGlvbiwgYWxsIHRoZSBvdGhlcnMKKyAgICAgICAgcmVtYWluIGluIC50ZXh0LiBJZiBK YXZhU2NyaXB0Q29yZSBpcyBidWlsdCBhcyBhIHNoYXJlZCBsaWJyYXJ5IHRoZW4KKyAgICAgICAg dGhpcyBjYXVzZXMgYSBzZWdtZW50YXRpb24gZmF1bHQgZHVyaW5nIGR5bmFtaWMgbGlua2luZy4K KworICAgICAgICBUaGlzIHBhdGNoIHB1dHMgYSAucHJldmlvdXMgZGlyZWN0aXZlIGF0IHRoZSBl bmQgb2YgdGhlIGlubGluZSBhc3NlbWJseQorICAgICAgICB0byBlbnN1cmUgdGhhdCB0aGUgYXNz dW1wdGlvbnMgb2YgdGhlIGNvbXBpbGVyIGFib3V0IHRoZSBzZWN0aW9ucyBhcmUKKyAgICAgICAg bm90IGJyb2tlbiBhbmQgdGhlIGZvbGxvd2luZyB2YXJpYWJsZSBnb2VzIHRvIHRoZSByaWdodCBw bGFjZS4KKworICAgICAgICAqIGppdC9UaHVua0dlbmVyYXRvcnMuY3BwOgorCiAyMDE0LTEwLTAy ICBGaWxpcCBQaXpsbyAgPGZwaXpsb0BhcHBsZS5jb20+CiAKICAgICAgICAgRlRMIHNob3VsZCBz aW5rIFB1dExvY2FscwpkaWZmIC0tZ2l0IGEvU291cmNlL0phdmFTY3JpcHRDb3JlL2ppdC9UaHVu a0dlbmVyYXRvcnMuY3BwIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL2ppdC9UaHVua0dlbmVyYXRv cnMuY3BwCmluZGV4IGYyYTU3OTQyLi41ZmJhYjY2IDEwMDY0NAotLS0gYS9Tb3VyY2UvSmF2YVNj cmlwdENvcmUvaml0L1RodW5rR2VuZXJhdG9ycy5jcHAKKysrIGIvU291cmNlL0phdmFTY3JpcHRD b3JlL2ppdC9UaHVua0dlbmVyYXRvcnMuY3BwCkBAIC04MDEsNiArODAxLDcgQEAgZG91YmxlIGpz Um91bmQoZG91YmxlIGQpCiAgICAgICAgIEhJREVfU1lNQk9MKGZ1bmN0aW9uIyNUaHVuaykgIlxu IiBcCiAgICAgICAgIFNZTUJPTF9TVFJJTkcoZnVuY3Rpb24jI1RodW5rKSAiOiIgIlxuIiBcCiAg ICAgICAgICJiICIgR0xPQkFMX1JFRkVSRU5DRShmdW5jdGlvbikgIlxuIiBcCisgICAgICAgICIu cHJldmlvdXMiIFwKICAgICApOyBcCiAgICAgZXh0ZXJuICJDIiB7IFwKICAgICAgICAgTWF0aFRo dW5rQ2FsbGluZ0NvbnZlbnRpb24gZnVuY3Rpb24jI1RodW5rKE1hdGhUaHVua0NhbGxpbmdDb252 ZW50aW9uKTsgXAo=