Prevent GCC using FP registers for pointer storage on arm64 Linux #193
Conversation
…ds to use them to store pointers
Overall package sizeSelf size: 9.56 MB Dependency sizes| name | version | self size | total size | |------|---------|-----------|------------| | source-map | 0.7.4 | 226 kB | 226 kB | | pprof-format | 2.1.0 | 111.69 kB | 111.69 kB | | p-limit | 3.1.0 | 7.75 kB | 13.78 kB | | delay | 5.0.0 | 11.17 kB | 11.17 kB | | node-gyp-build | 3.9.0 | 8.81 kB | 8.81 kB |🤖 This report was automatically generated by heaviest-objects-in-the-universe |
BenchmarksBenchmark execution time: 2025-01-23 15:10:46 Comparing candidate commit 532062b in PR branch Found 2 performance improvements and 0 performance regressions! Performance is the same for 89 metrics, 29 unstable metrics. scenario:profiler-idle-no-wall-profiler-18
scenario:profiler-idle-with-wall-profiler-18
|
| array = Array::New(isolate); | ||
| contextsByNode[sample] = {array, 1}; | ||
| (*contextsByNode)[sample] = {array, 1}; |
There was a problem hiding this comment.
Is this clean enough, or is there a better idiom to call the [] operator on the target of a pointer? I can also replace it with
contextsByNode->emplace(sample, (struct NodeInfo){array, 1});
| @@ -24,7 +24,7 @@ namespace dd { | |||
| v8::Local<v8::Value> TranslateTimeProfile( | |||
| const v8::CpuProfile* profile, | |||
| bool includeLineInfo, | |||
| ContextsByNode* contextsByNode = nullptr, | |||
| std::shared_ptr<ContextsByNode> contextsByNode = nullptr, | |||
There was a problem hiding this comment.
nit: you could have kept a raw pointer for contextsByNode in translate-time-profile.cc/hh
There was a problem hiding this comment.
Yeah, but figured why not be consistent. If I already have a safe shared pointer around I might as well use it.
* Prohibit use of floating-point registers where gcc on arm64 Linux tends to use them to store pointers
* Prohibit use of floating-point registers where gcc on arm64 Linux tends to use them to store pointers
What does this PR do?:
Prohibits use of arm64 floating point (FP) registers in some specific functions. For some reason, GCC compiling for arm64 on Linux will sometimes use FP registers to store/load pointers.
Motivation:
We see evidence of customer crashes on arm64 Linux, and the only thing they have in common is that they always seem to happen in code that invoked a method by loading
thisof the invoked method inx0from some floating point register. We speculate that GCC uses that as an optimization (hey, more free registers!) – as long as it only stores/loads 64-bit values, it's fine. The crashes might be due to some further invoked functions not restoring these.We can't generally prohibit use of floating point registers in our code (as it'd completely disable FP arithmetic and that pops up in unusual places, like a float load factor for inlined std::map constructors etc.), but we identified all methods where this "FP reg as pointer storage" tactics were employed by GCC and marked them separately with
__attribute__((target("general-regs-only")))to prohibit it from using FP regs in them.Since some of these methods did in fact try to manipulate floating-point values either directly or indirectly, we also had to either move those parts out of those functions (such is
ContextsByNodecreation because of aforementioned inlined float load factor instd::mapconstructor, or creation ofv8::Numberinstances from ints), or convertedasync_idfrom double to long, as it will always be an integral value.How to test the change?:
We'll release a dev build from this PR, download it, and check whether the use of fp has gone. In local testing on a Linux arm64 VM GCC sadly doesn't reproduce the issue.
JIRA: PROF-11193