[PATCH] D144783: [ELF] Move getSectionAndRelocations to ELF.cpp from ELFDumper.cpp

Mon Mar 6 00:52:16 PST 2023

aidengrossman added inline comments.

================
Comment at: llvm/include/llvm/Object/ELF.h:401
+  /// retrieve the content section of any relocation section.
+  Expected<llvm::MapVector<const Elf_Shdr *, const Elf_Shdr *>>
+  getSectionAndRelocations(
----------------
jhenderson wrote:
> Maybe a dumb question, but why are we qualifying `MapVector` with the `llvm` namespace here and elsewhere?
Not a dumb question. For some reason I completely forgot about the fact that we're either inside a `namspace llvm {}` block or `using namespace llvm;`. This should be fixed with the latest revision.

================
Comment at: llvm/test/tools/llvm-readobj/ELF/stack-sizes.test:874
-# INVALID-TARGET: warning: '[[FILE]]': SHT_RELA section with index 1: failed to get a relocated section: invalid section index: 255
-# INVALID-TARGET: warning: '[[FILE]]': SHT_RELA section with index 2: failed to get a relocated section: invalid section index: 255

----------------
jhenderson wrote:
> It would be nice to not lost the second warning, since it's for a different section, and therefore indicates that there's more than one problem. I think you'd have to use `joinErrors` in your loop, to gather all errors into a single one, and then handle them at the caller end, individually.
Fixed by moving back to using `joinErrors` within the `getSectionAndRelocations()` function. This also addresses a couple of your other comments since I forgot to remove the `continue;` statements left behind there.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144783