Closed Bug 1388338 Opened 7 years ago Closed 7 years ago

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


(Core :: JavaScript Engine: JIT, defect)

57 Branch
Not set



Tracking Status
firefox57 --- fixed


(Reporter: black.balamut, Assigned: black.balamut)



(2 files, 3 obsolete files)

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

Actual results:

/Applications/ -arch armv7 -isysroot /Applications/ -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;
/Applications/ note: '__sp' declared here
        __uint32_t      __sp;           /* Stack pointer r13 */
2 errors generated.
Version: Other Branch → 57 Branch
Thank you for opening a follow-up bug for this!

Assuming 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.
Flags: needinfo?(black.balamut)
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/ note: 'system' has been explicitly marked deprecated here
int      system(const char *) __DARWIN_ALIAS_C(system);
1 warning generated.
Attached file (obsolete) —
Attached patch fix_build_without_ion_2.diff (obsolete) — Splinter Review
Attachment #8895411 - Flags: review?(nicolas.b.pierron)
Flags: needinfo?(black.balamut)
Attachment #8895411 - Flags: review?(nicolas.b.pierron) → review?(bbouvier)
Comment on attachment 8895411 [details] [diff] [review]

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

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.
Attachment #8895411 - Flags: review?(bbouvier) → feedback+
I replaced comment with

#if defined(__arm__) || defined(__aarch64__) = ContextToLR(context);

and got error:

/mozilla-central/js/src/wasm/WasmSignalHandlers.cpp:515:16: error: no matching function for call to 'ContextToLR' = 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.
Ha, there's something subtle with the difference between CONTEXT and EMULATOR_CONTEXT. I'll take a closer look at it tomorrow.
Flags: needinfo?(bbouvier)
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?
Flags: needinfo?(bbouvier) → needinfo?(black.balamut)
It compiles with your patch.
Great, let's try to land this quickly. Thanks for testing!
Flags: needinfo?(black.balamut)
Assignee: nobody → black.balamut
Ever confirmed: true
Comment on attachment 8895411 [details] [diff] [review]

Can you export your patch following the commit conventions, please: and once it's done, submit again for review?

(you can put r=bbouvier at the end)
Comment on attachment 8895764 [details] [diff] [review]

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

(Context: 1. fixes none codegen, 2. implements ContextToLR on MacOS X cross compiling to ARM/ARM64)
Attachment #8895764 - Flags: review?(luke)
Attached patch darwin_arm_final_fix.diff (obsolete) — Splinter Review
Cumulative patch with all above fixes.
Attachment #8895409 - Attachment is obsolete: true
Attachment #8895411 - Attachment is obsolete: true
Attachment #8895787 - Flags: review?(bbouvier)
Comment on attachment 8895787 [details] [diff] [review]

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, or reachable via email too)
Attachment #8895787 - Flags: review?(bbouvier)
Attachment #8895787 - Attachment is obsolete: true
Attachment #8895844 - Flags: review?(bbouvier)
Comment on attachment 8895844 [details] [diff] [review]

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

Perfect, thanks!
Attachment #8895844 - Flags: review?(bbouvier) → review+
Attachment #8895764 - Flags: review?(luke) → review+
Pushed by
fixed undeclared identifier R14_sig and _fp on MacOS X cross compiling to ARM/ARM64. r=bbouvier
Add ContextToLR on Darwin ARM to wasm signal handling; r=luke
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.