[PATCH] D58625: [llvm-objcopy] - Check for invalidated relocations when removing a section.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: test/tools/llvm-objcopy/ELF/strip-section-err.test:7
+
+# CHECK: error: Section .data cannot be removed because it is referenced by the relocation section .rela.text.
+ 
----------------
grimar wrote:
> jhenderson wrote:
> > The relocations don't really reference the section, but rather a symbol in that section. I think the error message should reflect that somehow. It would also be great if the index of the relocation could be mentioned in the error message.
> What do you think about the updated version (I am printing the relocation name and index)?
Thanks, that looks much better, although I don't think there's much benefit in the type of relocation. I think we should probably be quoting section names too, but be consistent with what we do in other error messages.


================
Comment at: tools/llvm-objcopy/ELF/Object.cpp:1377
+  // from symbol table that belongs to removed section) or trigger an error if
+  // alive section critically depends on a section being removed somehow
+  // (e.g.removed section is referenced by a relocation).
----------------
jhenderson wrote:
> alive -> a live (here only, not in the first line).
You've lost the "a" here. Full text should be "if a live section critically"


================
Comment at: tools/llvm-objcopy/ELF/Object.cpp:563
+    StringRef RelName = getELFRelocationTypeName(Obj.Machine, R.Type);
+    const std::string &SymSecName = R.RelocSymbol->DefinedIn->Name;
+
----------------
`StringRef`?


================
Comment at: tools/llvm-objcopy/ELF/Object.cpp:570
+        SymSecName.data(), R.RelocSymbol->Name.c_str(), RelName.str().c_str(),
+        I + 1, this->Name.data());
+  }
----------------
`I + 1`? Shouldn't this just be `I`? There's no null relocation to ignore...

Do you need the `this->`?


================
Comment at: tools/llvm-objcopy/ELF/Object.cpp:1385
+  // live section critically depends on a section being removed somehow
+  // (e.g. removed section is referenced by a relocation).
+  for (auto &KeepSec : make_range(std::begin(Sections), Iter)) {
----------------
One more nit: e.g. removed -> e.g. the removed


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

https://reviews.llvm.org/D58625