[PATCH] D147544: [BOLT] Move from RuntimeDyld to JITLink

Fri Apr 21 04:46:04 PDT 2023

jobnoorman added a comment.

In D147544#4285166 <https://reviews.llvm.org/D147544#4285166>, @rafauler wrote:

> We can't have non-determinism in BOLT

Agreed.

I dug a bit deeper into this and fortunately, the order of output sections is already controlled by BOLT here <https://github.com/llvm/llvm-project/blob/88da59682004f2649732be9dbe38340ef68058a5/bolt/lib/Rewrite/RewriteInstance.cpp#L3913-L3937>. Since `compareSections` does not implement a strict total order, the result of `stable_sort` depends on the initial order of sections which is ultimately determined here <https://github.com/llvm/llvm-project/blob/11eb8d88d829a86bfc252628f105c99b1ead73e6/bolt/include/bolt/Core/BinarySection.h#L215-L244>. The source of non-determinism comes from the last comparison between the `SectionNumber` of sections which is based on the creation order of sections (e.g., here <https://github.com/llvm/llvm-project/blob/11eb8d88d829a86bfc252628f105c99b1ead73e6/bolt/include/bolt/Core/BinarySection.h#L155>).

When using `RuntimeDyld`, the creation order of sections (that are created during `emitAndLink`) is out of BOLT's hands since `RuntimeDyld` calls `ExecutableFileMemoryManager::allocateSection` in some order. The way this order is determined is quite complex <https://github.com/llvm/llvm-project/blob/384a8dd10eb43b23301acc801855bdb0a0b6b35c/llvm/lib/ExecutionEngine/RuntimeDyld/RuntimeDyld.cpp#L181-L457> (e.g., it depends on symbol order) and I doubt any guarantees are made about it but at least it seems deterministic.

When using `JITLink`, whole `LinkGraph`s are allocated at once so BOLT has full control over the order in which sections are allocated. The current version of the patch simply uses the order of `LinkGraph::sections()` which is non-deterministic since a `DenseMap` is used to store sections. However, we can make it deterministic by first sorting the sections using any deterministic property (e.g., their names). Note that this doesn't guarantee the same order as BOLT currently produces but it //will// be deterministic.

The other way to make it deterministic would of course be to apply the patch I posted above. Since this also happens to produce the same section order as current BOLT, and hence simplifies verification of this patch, I would like to ask your opinion about this, @lhames. Would it make sense for `JITLink` to make the section order deterministic? The patch above replaces the `DenseMap` used to store sections with a `MapVector` preserving insertion order (which, when creating a link graph from an object, makes sure the sections are stored in the same order as in the object file).

> I saw that you provided a patch to make the section order deterministic. I used that, and also used a change to insert the null terminator. And then I got the binaries to match perfectly for some tests. But what's weird is that that happened only for some runs. Other runs would generate un-matching binaries, which is something that we can't have in BOLT. I didn't investigate why is that happening, but it is because of my change to add the prepass to add the null terminator. But assuming we don't have this change, if the section orders aren't deterministic, we also can't have that, even if the code is correct.

About the null terminator: when looking at `.eh_frame` in some test binaries, the terminator is already inserted by the current version of the patch. The difference with current BOLT is that some spurious null terminators in the middle of the section are not there anymore. So why are you trying to add the prepass to insert the terminator?

When you refer to "other runs", do you mean subsequent runs on the same binary? That would obviously be a serious bug that needs to be fixed. Or are you referring to runs on other binaries? The only binary differences I still see in the test cases are caused by the `.eh_frame` null terminator difference which, iiuc, is benign.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147544