[PATCH] D127056: [ORC][ORC_RT] Handle ELF .init_array with non-default priority

[Peter S. Housel via Phabricator via llvm-commits]

housel added inline comments.


================
Comment at: compiler-rt/lib/orc/elfnix_platform.cpp:379
+  if (pos == name.size() - 1)
+    return 65535;
+  else
----------------
MaskRay wrote:
> 65536 is better.
> 
> See lld/ELF/OutputSections.cpp::576
I saw that, but it wasn't clear what advantage that would have (as opposed to reversing the mapping in llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp:1060)


================
Comment at: compiler-rt/lib/orc/elfnix_platform.cpp:391
+  using SectionList = std::vector<ExecutorAddrRange>;
+  std::sort(MOJDIs.InitSections.begin(), MOJDIs.InitSections.end(),
+            [](const std::pair<std::string, SectionList> &LHS,
----------------
MaskRay wrote:
> lhames wrote:
> > MaskRay wrote:
> > > stable_sort
> > Does ELF make any guarantees about order-of-initialization beyond priority ordering?
> > 
> > We don't currently number `MaterializationUnit`s, so initialization order is already at the mercy of the dependence graph (likely opaque to clients), and the scheduler (if concurrent compilation is enabled). I'm not opposed to stable_sort (I like that it should make run-to-run behavior of `llvm-jitlink` more stable), but it's probably worth a comment that order of initialization is still not guaranteed in general.
> For determinism (using different standard library implementations should give the same result), stable_sort should be used. Relying on the order is user code issue, but that's unrelated to the general determinism guarantee provided by the toolchain (https://maskray.me/blog/2021-11-07-init-ctors-init-array)
At the point where this occurs, `ELFNixPlatform` has already collected all initializers of each priority level into a single entry, so this sort by priority level is already going to be completely deterministic. I can change this to `stable_sort` but it will never have any effect on the result.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D127056/new/

https://reviews.llvm.org/D127056