Closed
Bug 1238661
Opened 9 years ago
Closed 8 years ago
remove MOZ_SIGNAL_TRAMPOLINE use in the profiler's platform-linux.cc
Categories
(Core :: Gecko Profiler, defect)
Tracking
()
RESOLVED
FIXED
mozilla50
Tracking | Status | |
---|---|---|
firefox50 | --- | fixed |
People
(Reporter: froydnj, Assigned: froydnj)
References
Details
Attachments
(1 file)
2.55 KB,
patch
|
jchen
:
review+
|
Details | Diff | Splinter Review |
Ehsan and I were chatting about this, as this is one of the blockers to using clang to compile Fennec, due to this LLVM bug: https://llvm.org/bugs/show_bug.cgi?id=24556 Even if it were to be fixed upstream, the lag time for the fix to make it into the NDK might be significant. And since clang is become the default compiler in newer versions of the NDK (and GCC is being deprecated), it would be nice to prepare for that switchover sooner rather than later. The alternative to MOZ_SIGNAL_TRAMPOLINE is writing some ARM-specific assembly and groveling around with C++ mangled names.
Comment 1•9 years ago
|
||
Looks like it's only used in the profiler right now, and the only reasons it's a problem is because we run some profiler-related tests on Android 2.3 builds. If we disable those tests for 2.3 (e.g. testANRReporter.java), we should be able to get rid of it entirely.
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → nfroyd
Assignee | ||
Comment 3•8 years ago
|
||
The below is a lovely summary of the issue, but reading back through the bug, it sounds like we might not need this anymore, since we've almost certainly disabled Android 2.3 tests? So maybe all of this stuff is totally unnecessary now? mozilla::SignalTrampoline is designed to work around a bug in older ARM kernels; it constructs a trampoline function with a NOP slide and then calls a specified function. This feat is accomplished using inline assembly and naked functions, which is a GCC extension would you get to write the body of your function using GCC inline assembly. Unfortunately, the particular implementation that it uses requires the specified function's address to be loaded into a register. GCC permits this and we use input arguments to the assembly statement to ensure that GCC knows it shouldn't clobber the incoming argument registers when trying to load the function's address. clang, however, complains about the use of input parameters in naked functions. So we need to find something that will work on both GCC and clang. The trick is to realize that we're a) tail-calling the specified function and b) we don't have to worry about calling a fully-general function. We just have to worry about calling a function inside libxul, and we can therefore "assume" that the offset between the branch and the called function fits into the immediate field of a Thumb (or ARM) branch instruction. (This assumption is not strictly true; the branch range is +/-16MB or so and libxul is actually quite a bit bigger than that. But it works in practice, and the linker will insert branch stubs if necessary to make things work out OK.) The upshot is that we can use a "b" instruction instead of a "bx" instruction, and this makes clang much happier. As a small bonus, the stub gets ever-so-much-more efficient, which is probably the least-significant micro-optimization ever.
Attachment #8773392 -
Flags: review?(nchen)
Comment 4•8 years ago
|
||
Comment on attachment 8773392 [details] [diff] [review] fix mozilla::SignalTrampoline to work properly with clang Review of attachment 8773392 [details] [diff] [review]: ----------------------------------------------------------------- We stopped supporting Android 2.3, so removing it (and mfbt/LinuxSignal.h entirely) should be fine. We only support API 15+ now, and that was based on the 3.0.1 kernel, which doesn't have this bug.
Attachment #8773392 -
Flags: review?(nchen) → review+
Pushed by nfroyd@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/537f4d1d837a fix mozilla::SignalTrampoline to work properly with clang; r=darchons
Comment 6•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/537f4d1d837a
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox50:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in
before you can comment on or make changes to this bug.
Description
•