[PATCH] D117612: [BOLT] Update dynamic relocations from section relocations

[Maksim Panchenko via Phabricator via llvm-commits]

maksfb added inline comments.


================
Comment at: bolt/include/bolt/Rewrite/RewriteInstance.h:442
+
+  /// Index of specified symbol in symbol table
+  std::map<MCSymbol *, uint32_t> SymbolIndex;
----------------
Could you put a more accurate description? I.e. "index in the dynamic symbol table". Also, not all symbols will be in the table, right? Only those referenced by relocations?


================
Comment at: bolt/include/bolt/Rewrite/RewriteInstance.h:443
+  /// Index of specified symbol in symbol table
+  std::map<MCSymbol *, uint32_t> SymbolIndex;
+
----------------
Perhaps `DenseMap` here as well?


================
Comment at: bolt/lib/Rewrite/RewriteInstance.cpp:2106
     const uint64_t Addend = getRelocationAddend(InputFile, Rel);
+    const uint32_t SymbolIdx = getRelocationSymbol(InputFile, Rel);
+    const uint64_t Type = Rel.getType();
----------------
nit: there's no need for the variable as there's only one usage.


================
Comment at: bolt/lib/Rewrite/RewriteInstance.cpp:2107
+    const uint32_t SymbolIdx = getRelocationSymbol(InputFile, Rel);
+    const uint64_t Type = Rel.getType();
 
----------------
nit: use `RType` defined above. You can make it `const`.


================
Comment at: bolt/lib/Rewrite/RewriteInstance.cpp:4798-4800
+          auto It = SymbolIndex.find(Symbol);
+          if (It != SymbolIndex.end())
+            SymbolIdx = It->second;
----------------
I'd rather put the code under `getOutputDynamicSymbolIndex(MCSymbol *)`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117612