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

[Aiden Grossman via Phabricator via llvm-commits]

aidengrossman 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:
> 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?
Returning an `Expected<llvm::MapVector<const Elf_Shdr*, const Elf_Shdr*>>` should work pretty well and would definitely make the function easier to understand. Having the value of the `MapVector` also be an `Expected` would make things slightly messy though I would think since we would only be creating errors when adding content sections, and then destroying the errors when associating a relocation section (which also means the value of the error has to be checked). Returning a `std::optional` as the value seems like it should work well though since it captures exactly the information that we want (address to section or the fact that there's no section). And we should be able to just continue propagating all of the errors as done currently.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144783