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

[Job Noorman via Phabricator via llvm-commits]

jobnoorman added inline comments.


================
Comment at: bolt/lib/Rewrite/RewriteInstance.cpp:1661
+    auto *Sym = BC->getOrCreateGlobalSymbol(Value, "EHat");
+    RelocatedEHFrameSection->addRelocation(Offset, Sym, RelType, Value);
   };
----------------
rafauler wrote:
> Second thing I noticed was this. This just looks wrong because here we're adding a reloc against a new symbol Sym, but then we're adding its own Value as an addend too. That Value probably should be zero, instead:
> 
>   RelocatedEHFrameSection->addRelocation(Offset, Sym, RelType, 0);
> 
> But then I did that and .eh_frame still looks completely different (the addresses). I'm not sure what's happening.
> 
> To see that, you need to build a test case with exceptions with two versions of BOLT, with and without your change, and then compare the eh_frame sections in them. To dump the eh_frame contents you can use a utility such as  "eu-readelf -e binary".
> 
> Finally, to check that your JITLink enabled BOLT is generating the same output of the old BOLT and thus, proving that this diff is safe, I suggest using this utility: https://github.com/llvm/llvm-project/blob/main/bolt/utils/llvm-bolt-wrapper.py
> 
> This utility basically replaces llvm-bolt in your binary folder, and when you run "check-bolt", it will trigger two BOLT runs, with and without your cahnge, and then compare the binaries produced by both BOLTs.  That's essentially part of the testing that we do when we have to approve a diff on BOLT, we take a look if the diff is not unintentionally changing some parts of the binary that it shouldn't, otherwise it's not safe to go to production.
> 
> Essentially, your diff is blocked atm because it is generating different eh_frame contents in x86 in a way that doesn't look right.
> Second thing I noticed was this. This just looks wrong because here we're adding a reloc against a new symbol Sym, but then we're adding its own Value as an addend too. That Value probably should be zero, instead:
> 
>   RelocatedEHFrameSection->addRelocation(Offset, Sym, RelType, 0);
> 
> But then I did that and .eh_frame still looks completely different (the addresses). I'm not sure what's happening.

This was indeed the problem. However, when setting the addend to 0, the symbol will get patched with the address of the relocated function and not with the original value (which is what BOLT wants iiuc). Hence the different addresses. The "trick" I used to fix this is to create a symbol at address 0x0 and then set the addend to the value. As my comments explain, this is is necessary as JITLink crashes when adding an ABS symbol as was done before.

> To see that, you need to build a test case with exceptions with two versions of BOLT, with and without your change, and then compare the eh_frame sections in them. To dump the eh_frame contents you can use a utility such as  "eu-readelf -e binary".

TIL about `eu-readelf`, thanks! :)

> Finally, to check that your JITLink enabled BOLT is generating the same output of the old BOLT and thus, proving that this diff is safe, I suggest using this utility: https://github.com/llvm/llvm-project/blob/main/bolt/utils/llvm-bolt-wrapper.py

I've only manually tested using a simple binary for now and will try this asap.

> Essentially, your diff is blocked atm because it is generating different eh_frame contents in x86 in a way that doesn't look right.

The null-terminator is now the only difference I see on a test binary.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147544