1388338 - /js/src/wasm/WasmSignalHandlers.cpp:452:39: error: use of undeclared identifier 'R14_sig'

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Igor Denys]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/60.0.3112.90 Safari/537.36

Steps to reproduce:

After applying patch from https://bugzilla.mozilla.org/show_bug.cgi?id=1388005
I do:

CXXFLAGS+=-std=gnu++11 ../configure --target=armv7-apple-darwin --with-ios-sdk=iphoneos10.0 --enable-optimize=-O0 --disable-ion --enable-warnings-as-errors
make



Actual results:

/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/clang++ -arch armv7 -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS10.3.sdk -miphoneos-version-min=8.0 -o Unified_cpp_js_src39.o -c  -fvisibility=hidden -fvisibility-inlines-hidden -DNDEBUG=1 -DTRIMMED=1 -DENABLE_BINARYDATA -DENABLE_SIMD -DJS_CACHEIR_SPEW -DENABLE_SHARED_ARRAY_BUFFER -DEXPORT_JS_API -DMOZ_HAS_MOZGLUE -I/mozilla-central/js/src -I/mozilla-central/js/src/build_ios_OPT.OBJ/js/src  -I/mozilla-central/js/src/build_ios_OPT.OBJ/dist/include          -fPIC  -DMOZILLA_CLIENT -include /mozilla-central/js/src/build_ios_OPT.OBJ/js/src/js-confdefs.h -MD -MP -MF .deps/Unified_cpp_js_src39.o.pp -Qunused-arguments  -Qunused-arguments -Wall -Wc++11-compat -Wempty-body -Wignored-qualifiers -Woverloaded-virtual -Wpointer-arith -Wsign-compare -Wtype-limits -Wunreachable-code -Wwrite-strings -Wno-invalid-offsetof -Wclass-varargs -Wloop-analysis -Wc++11-compat-pedantic -Wc++14-compat -Wc++14-compat-pedantic -Wc++1z-compat -Wcomma -Wimplicit-fallthrough -Werror=non-literal-null-conversion -Wstring-conversion -Wno-inline-new-delete -Wno-error=deprecated-declarations -Wno-error=array-bounds -Wformat -Wno-gnu-zero-variadic-macro-arguments -Wformat-security -Wno-unknown-warning-option -Wno-return-type-c-linkage -std=gnu++11 -fno-common -stdlib=libc++ -fno-rtti -fno-exceptions -fno-math-errno -pthread -pipe  -g -O0  -Werror -Wno-shadow -Werror=format -fno-strict-aliasing  /mozilla-central/js/src/build_ios_OPT.OBJ/js/src/Unified_cpp_js_src39.cpp
In file included from /mozilla-central/js/src/build_ios_OPT.OBJ/js/src/Unified_cpp_js_src39.cpp:38:
/mozilla-central/js/src/wasm/WasmSignalHandlers.cpp:452:39: error: use of undeclared identifier 'R14_sig'
    return reinterpret_cast<uint8_t*>(LR_sig(context));
                                      ^
/mozilla-central/js/src/wasm/WasmSignalHandlers.cpp:408:20: note: expanded from macro 'LR_sig'
# define LR_sig(p) R14_sig(p)
                   ^
/mozilla-central/js/src/wasm/WasmSignalHandlers.cpp:486:38: error: no member named '__fp' in '__darwin_arm_thread_state'; did you mean '__sp'?
    return (uint8_t*)context->thread.__fp;
                                     ^~~~
                                     __sp
/Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS10.3.sdk/usr/include/mach/arm/_structs.h:51:13: note: '__sp' declared here
        __uint32_t      __sp;           /* Stack pointer r13 */
                        ^
2 errors generated.

Comment 1

[Benjamin Bouvier [:bbouvier] (inactive)]

Thank you for opening a follow-up bug for this!

Assuming https://github.com/opensource-apple/cctools/blob/master/include/mach/arm/_structs.h#L26 is describing the correct structure, can you try replacing __fp by __r[11] and see if it compiles? If so, feel free to make a patch, or I can do so.

Comment 2

[Igor Denys]

Replacing __fp with __r[11] fixes second error.

Also I added definition of R14_sig under XP_DARWIN branch

# define R14_sig(p) ((p)->uc_mcontext->__ss.__lr)

and I got warning:

In file included from /mozilla-central/js/src/build_ios_OPT.OBJ/js/src/Unified_cpp_js_src39.cpp:38:
/mozilla-central/js/src/wasm/WasmSignalHandlers.cpp:451:1: warning: unused function 'ContextToLR' [-Wunused-function]
ContextToLR(CONTEXT* context)
^

So I removed option --enable-warnings-as-errors and now it compiles with some extra warning:

In file included from /mozilla-central/js/src/build_ios_OPT.OBJ/memory/mozalloc/Unified_cpp_memory_mozalloc0.cpp:11:
/mozilla-central/memory/mozalloc/mozalloc_abort.cpp:72:1: warning: all paths through this function will call itself [-Winfinite-recursion]
{
^
1 warning generated.

/mozilla-central/js/src/shell/OSObject.cpp:809:18: warning: 'system' is deprecated: first deprecated in iOS 8.0 - Use posix_spawn APIs instead. [-Wdeprecated-declarations]
    int result = system(command.ptr());
                 ^
/Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS10.3.sdk/usr/include/stdlib.h:187:6: note: 'system' has been explicitly marked deprecated here
int      system(const char *) __DARWIN_ALIAS_C(system);
         ^
1 warning generated.

Comment 3

[Igor Denys]

Attachment: fix_build_without_ion_2.zip

Comment 4

[Igor Denys]

Attachment: fix_build_without_ion_2.diff

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8895411 [details] [diff] [review]
fix_build_without_ion_2.diff

Review of attachment 8895411 [details] [diff] [review]:
-----------------------------------------------------------------

Stealing, since I introduced this change. Thanks for the patch! It's almost perfect.

In ToRegisterState(EMULATOR_CONTEXT* context), can you replace the comment saying

   // no ARM on Darwin => don't fill state.lr.

By an #if defined(__arm__) etc similar to the on in ToRegisterState(CONTEXT* context), please?

This should make use of the ContextToLR function, avoiding us surprises in a future where we'd compile wasm under ios, and will get rid of the unused error you encountered during compilation.

Comment 6

[Igor Denys]

I replaced comment with

#if defined(__arm__) || defined(__aarch64__)
    state.lr = ContextToLR(context);
#endif

and got error:

/mozilla-central/js/src/wasm/WasmSignalHandlers.cpp:515:16: error: no matching function for call to 'ContextToLR'
    state.lr = ContextToLR(context);
               ^~~~~~~~~~~
/mozilla-central/js/src/wasm/WasmSignalHandlers.cpp:451:1: note: candidate function not viable: no known conversion from 'macos_arm_context *' to 'ucontext_t *' (aka '__darwin_ucontext *') for 1st argument
ContextToLR(CONTEXT* context)
^
1 error generated.

Comment 7

[Benjamin Bouvier [:bbouvier] (inactive)]

Ha, there's something subtle with the difference between CONTEXT and EMULATOR_CONTEXT. I'll take a closer look at it tomorrow.

Comment 8

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 2.darwin-arm.patch

Igor, can you apply this patch atop of the patch you've submitted before (without the additional changes I asked for), and let me know if it compiles, please?

Comment 9

[Igor Denys]

It compiles with your patch.

Comment 10

[Benjamin Bouvier [:bbouvier] (inactive)]

Great, let's try to land this quickly. Thanks for testing!

Comment 11

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8895411 [details] [diff] [review]
fix_build_without_ion_2.diff

Can you export your patch following the commit conventions, please: https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#Commit_Message_Conventions and once it's done, submit again for review?

(you can put r=bbouvier at the end)

Comment 12

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8895764 [details] [diff] [review]
2.darwin-arm.patch

Review of attachment 8895764 [details] [diff] [review]:
-----------------------------------------------------------------

(Context: 1. fixes none codegen, 2. implements ContextToLR on MacOS X cross compiling to ARM/ARM64)

Comment 13

[Igor Denys]

Attachment: darwin_arm_final_fix.diff

Cumulative patch with all above fixes.

Comment 14

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8895787 [details] [diff] [review]
darwin_arm_final_fix.diff

Review of attachment 8895787 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry, will have to make another round:

- please include a commit message as indicated in the previous link I sent
- please keep your changes to the initial patch you submitted

Let me know if you struggle with this! (I'm on irc :bbouvier on #jsapi on irc.mozilla.org, or reachable via email too)
Thanks!

Comment 15

[Igor Denys]

Attachment: fix_build_arm_ion.diff

Comment 16

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8895844 [details] [diff] [review]
fix_build_arm_ion.diff

Review of attachment 8895844 [details] [diff] [review]:
-----------------------------------------------------------------

Perfect, thanks!

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a92cd58d54a2
fixed undeclared identifier R14_sig and _fp on MacOS X cross compiling to ARM/ARM64. r=bbouvier
https://hg.mozilla.org/integration/mozilla-inbound/rev/6c588d8119a4
Add ContextToLR on Darwin ARM to wasm signal handling; r=luke

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a92cd58d54a2
https://hg.mozilla.org/mozilla-central/rev/6c588d8119a4