[PATCH] D108081: [ORC] Add Platform and runtime support for ELF-based platforms

[Lang Hames via Phabricator via llvm-commits]

lhames added a comment.

This looks great. Thanks Peter!

I've tested this out in a Linux container and verified that it correctly handles static constructors and destructors in both in-process and out-of-process mode. This is really exciting -- until now only Darwin users have been able to use these features. :)

Please see inline comments -- there are some vestigial TLV functions / members in the ORC runtime, which I think could be removed relatively easily. There are also some in the platform, but they would be tricker to take out -- I would be fine with them staying in in the initial commit.

Side note: This implementation replicates some of MachOPlatform's current warts (e.g. fuzzy operation ordering, JIT-side tracking of init data). This isn't a problem at all, we'll just need to be careful that any fixes made to MachOPlatform are also applied to ELFNix and vice-versa.

I know that you mentioned on discord that you do not have commit access: Would you mind if I removed the walkEHFrame and TLV functions from the runtime, then committed on your behalf?



================
Comment at: compiler-rt/lib/orc/elfnix_platform.cpp:39-59
+template <typename HandleFDEFn>
+void walkEHFrameSection(span<const char> EHFrameSection,
+                        HandleFDEFn HandleFDE) {
+  const char *CurCFIRecord = EHFrameSection.data();
+  uint64_t Size = *reinterpret_cast<const uint32_t *>(CurCFIRecord);
+
+  while (CurCFIRecord != EHFrameSection.end() && Size != 0) {
----------------
This is kind-of-sort-of out of place, but the situation is complex:

On Darwin the default unwinder is LLVM libunwind, and LLVM libunwind's `__register_frame` and `__deregister_frame` functions expect to be called on individual FDEs.

On Linux the default unwinder is libgcc_s, and libgcc_s's `__register_frame` and `__deregister_frame` functions expect to be called on whole eh-frame //sections//.

In LLVM at the moment we just assume that Darwin implies LLVM libunwind, and anything else implies libgcc_s, but this has also caused problems when people try to use LLVM libunwind on other platforms. See http://llvm.org/PR44074.

I think the best thing to do for now is to stay consistent with the current not-quite-right behavior of LLVM, and assume that ELFNix implies libgcc_s for unwinding: the `walkEHFrameSection` utility should be removed, and `registerObjectSections` and `deregisterObjectSections` should call `__register_frame` and `__deregister_frame` directly on the section ranges.

Separate from this review, we can revisit this as part of http://llvm.org/PR44074 -- I will update that bug with notes and cite this review.


================
Comment at: compiler-rt/lib/orc/elfnix_platform.cpp:164-165
+
+  std::mutex ThreadDataSectionsMutex;
+  std::map<const char *, size_t> ThreadDataSections;
+};
----------------
These are unused and can be removed for now.


================
Comment at: compiler-rt/lib/orc/elfnix_platform.cpp:191-195
+  if (POSR.ThreadDataSection.StartAddress) {
+    if (auto Err = registerThreadDataSection(
+            POSR.ThreadDataSection.toSpan<const char>()))
+      return Err;
+  }
----------------
This can be removed and re-introduced in a future TLV patch.


================
Comment at: compiler-rt/lib/orc/elfnix_platform.cpp:275-286
+Expected<std::pair<const char *, size_t>>
+ELFNixPlatformRuntimeState::getThreadDataSectionFor(const char *ThreadData) {
+  std::lock_guard<std::mutex> Lock(ThreadDataSectionsMutex);
+  auto I = ThreadDataSections.upper_bound(ThreadData);
+  // Check that we have a valid entry covering this address.
+  if (I == ThreadDataSections.begin())
+    return make_error<StringError>("No thread local data section for key");
----------------
I don't think this is called yet, so the whole function can be removed for now. It can be re-added in future a ELFNix TLV support patch.


================
Comment at: compiler-rt/lib/orc/elfnix_platform.cpp:327-339
+Error ELFNixPlatformRuntimeState::registerThreadDataSection(
+    span<const char> ThreadDataSection) {
+  std::lock_guard<std::mutex> Lock(ThreadDataSectionsMutex);
+  auto I = ThreadDataSections.upper_bound(ThreadDataSection.data());
+  if (I != ThreadDataSections.begin()) {
+    auto J = std::prev(I);
+    if (J->first + J->second > ThreadDataSection.data())
----------------
This function is only used to put data into currently unused members, so it can be removed for now (and re-added in a future TLV support patch).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108081