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

[Aiden Grossman via Phabricator via llvm-commits]

aidengrossman marked 3 inline comments as done.
aidengrossman added inline comments.


================
Comment at: llvm/include/llvm/Object/ELF.h:397
+  Error getSectionAndRelocations(
+      std::function<Expected<bool>(const Elf_Shdr &)> IsMatch,
+      llvm::MapVector<const Elf_Shdr *, const Elf_Shdr *> &SecToRelocMap) const;
----------------
rahmanl wrote:
> rahmanl wrote:
> > Let's not add complexity here. IsMatch should be a simple function which returns true or false.
> If adopting the approach I mentioned below, we can keep the return value as `Expected<bool>>` here and just capture the error and store that in the map.
See my comment below on the other approach. I think that returning an `Expected<bool>` here doesn't add too much complexity, and it allows for errors to propagate up through the library code so that callers can handle errors depending upon their specific requirements. Otherwise we either have to `assert` (might not be a bad design decision in the only caller of this function returning errors through the matcher though) or report an error/warning within the library code, which I'd prefer to avoid.


================
Comment at: llvm/lib/Object/ELF.cpp:727-735
+    if (!RelSecOrErr) {
+      IsSuccess = joinErrors(
+          std::move(IsSuccess),
+          make_error<StringError>(std::error_code(),
+                                  describe(*this, Sec) +
+                                      ": failed to get a relocated section: " +
+              toString(RelSecOrErr.takeError())));
----------------
jhenderson wrote:
> rahmanl wrote:
> > How about reporting warnings here and changing `SecToRelcMap` type to `llvm::MapVector<const Elf_Shdr *, Expected<const Elf_Shdr *>>`  so you can put the errors there? This way you can just let the clients do the error handling on their own.
> This is a library, so we shouldn't be reporting warnings directly here (see also my lightning talk :) https://www.youtube.com/watch?v=YSEY4pg1YB0). Or do you mean something else?
Changing to `llvm::MapVector<const Elf_Shdr *, Expected<const Elf_Shdr *>>` wouldn't really capture the failure modes that are present here. We either can't get the section linked to by the Rel/Rela's `SH_INFO` which means we can't associate it with any specific relocated section, or the `IsMatch` call fails which also means we likely can't associate it with any content section within the map.


================
Comment at: llvm/lib/Object/ELF.cpp:739
+    if(!DoesRelTargetMatch)
+      return DoesRelTargetMatch.takeError();
+    if (*DoesRelTargetMatch)
----------------
jhenderson wrote:
> This will cause a failure in certain configurations, because `IsSuccess` is not being consumed in any way.
> 
> Is there a reason this and the other `return` statement in the loop don't simply use `joinErrors` like everything else?
Thank you for catching this. I've changed to `joinErrors` (not sure why I didn't use it here).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144783