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

[Rafael Auler via Phabricator via llvm-commits]

rafauler added a comment.

Code looks good. A few comments.



================
Comment at: bolt/lib/Rewrite/RewriteInstance.cpp:2393
+    StringRef SectionName = Section.getName();
+    dbgs() << "BOLT-DEBUG: reading relocations for section " << SectionName
+           << ":\n";
----------------
for -> in


================
Comment at: bolt/lib/Rewrite/RewriteInstance.cpp:2421
+  uint64_t Offset = 0, Address = 0;
+  uint64_t RelrCount = DynamicRelrSize / DynamicRelrEntrySize;
+  while (RelrCount--) {
----------------
assert that DynamicRelrSize is a multiple of DynamicRelrEntrySize?


================
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;
----------------
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).


================
Comment at: bolt/lib/Rewrite/RewriteInstance.cpp:5463
+  // to be emitted first.
+  if (!DynamicRelrAddress)
+    writeRelocations(/* PatchRelative */ true);
----------------
Is it possible to have some relocations in input RELA and some others in RELR? e.g. does the RELR extension allow for this?


================
Comment at: bolt/lib/Rewrite/RewriteInstance.cpp:5692
+
+  if (!DynamicRelrAddress || !DynamicRelrSize) {
+    DynamicRelrAddress.reset();
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146085