[PATCH] D143748: [BOLT] Improve dynamic relocations support for CI

[Rafael Auler via Phabricator via llvm-commits]

rafauler added inline comments.


================
Comment at: bolt/include/bolt/Core/BinaryFunction.h:170
 
+    /// Set true if constant island contains dynamic relocations
+    bool HasDynamicRelocations{false};
----------------
Can you expand this comment to:

Set true if constant island contains dynamic relocations, which happens if binary is linked with -znotext


================
Comment at: bolt/include/bolt/Core/BinaryFunction.h:1944
 
+  void markIslandDynamicRelocationAtAddress(uint64_t Address) {
+    if (!isInConstantIsland(Address)) {
----------------
Add comment

Support dynamic relocations in constant islands, which happens if binary is linked with -znotext


================
Comment at: bolt/include/bolt/Core/BinaryFunction.h:1946
+    if (!isInConstantIsland(Address)) {
+      errs() << "BOLT-ERROR: Dynamic relocation found for text section at 0x"
+             << Twine::utohexstr(Address) << "\n";
----------------
nit:  Dynamic -> dynamic


================
Comment at: bolt/lib/Core/BinaryContext.cpp:394
     if (IslandIter != AddressToConstantIslandMap.end()) {
-      if (MCSymbol *IslandSym =
-              IslandIter->second->getOrCreateProxyIslandAccess(Address, BF)) {
+      // We would refer original CI if it has dynamic relocations in it
+      if (IslandIter->second->hasDynamicRelocationAtIsland()) {
----------------
Is this because we don't support cloning dynamic relocs? If yes, we can expand this comment to clarify:

// Fall-back to referencing the original constant island in the presence of dynamic relocs, as we currently do not support cloning them. Notice: we might fail to link because of this, if the original constant island is far away.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143748