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

[Rahman Lavaee via Phabricator via llvm-commits]

rahmanl added inline comments.


================
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())));
----------------
rahmanl wrote:
> aidengrossman wrote:
> > 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.
> Makes sense. No need to report the warning here. My main suggestion is to avoid both returning by parameter and by return value. So I guess in this case we can return by `llvm::MapVector<const Elf_Shdr *, Expected<const Elf_Shdr *>>`. This way, the client will need to consume the error only when accessing the corresponding map entry. WDYT?
I see what you mean. My concern with the current logic is that the function is very hard to explain (because of double return by parameter and by return value) and use. Sections with `IsMatch=true` and with failed rela sections, will be mapped to `nullptr`, but their failure will be returned.

How about `Expected<llvm::MapVector<const Elf_Shdr *, Expected<const Elf_Shdr *>>>` ? If any of the `IsMatch` calls fail, then we outright return it. Otherwise, we fill in the map with all the matched sections. If any matched section's rela section cannot be captured, then the failure will be in the map. WDYT?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144783