[PATCH] D146085: [BOLT] Add .relr.dyn section support

[Vladislav Khmelevsky via Phabricator via llvm-commits]

yota9 marked 5 inline comments as done.
yota9 added inline comments.


================
Comment at: bolt/lib/Rewrite/RewriteInstance.cpp:2421
+  uint64_t Offset = 0, Address = 0;
+  uint64_t RelrCount = DynamicRelrSize / DynamicRelrEntrySize;
+  while (RelrCount--) {
----------------
rafauler wrote:
> assert that DynamicRelrSize is a multiple of DynamicRelrEntrySize?
Would add check on dynamic reading


================
Comment at: bolt/lib/Rewrite/RewriteInstance.cpp:2423-2439
+    assert(DE.isValidOffset(Offset));
+    uint64_t Entry = DE.getUnsigned(&Offset, DynamicRelrEntrySize);
+    if ((Entry & 1) == 0) {
+      AddRelocation(Entry);
+      Address = Entry + PSize;
+    } else {
+      const uint64_t StartAddress = Address;
----------------
rafauler wrote:
> This has untested code because our testcase currently is very simple and doesn't have entries with bitmaps. Can we come up with a slightly more complicated testcase, just for relr, that exercises this decoding mechanism?  (I'm not saying the code is wrong, it's just nice to have a testcase in case we ever change this and need to verify that it still works as intended).
Extended the testcase a bit


================
Comment at: bolt/lib/Rewrite/RewriteInstance.cpp:5463
+  // to be emitted first.
+  if (!DynamicRelrAddress)
+    writeRelocations(/* PatchRelative */ true);
----------------
rafauler wrote:
> Is it possible to have some relocations in input RELA and some others in RELR? e.g. does the RELR extension allow for this?
I was thinking about this too, but for now I don't see a reason a linker to make such a strange thing. Theoretically it is possible, but in practice - I don't think so. Even if I don't think we can currently handle such situations since we're not preserving input section information for each relocation. The same problem might appear when patching RELA and JMPREL relocations, currently we're saving only which type goes where, but theoretically there might be a problem e.g. different linkers might place some TLS relocations in any of these sections. When and if we would support rewriting binary and we would be able to remove old relocation section and create new we would be able to handle such cases, but for now I think the logic would stay as-is..


================
Comment at: bolt/lib/Rewrite/RewriteInstance.cpp:5692
+
+  if (!DynamicRelrAddress || !DynamicRelrSize) {
+    DynamicRelrAddress.reset();
----------------
rafauler wrote:
> I guess you also rely on DynamicRelrEntrySize being non-zero for this patch to work, so maybe check here too, in addition to DynamicRelrAddress and DynamicRelrSize.
Would add error check


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146085