[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())));
----------------
aidengrossman wrote:
> 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.
> 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)

I am not following the logic. Why do we need to insert initial entries in the map? We insert an entry in the map iff `IsMatch(Sec) && (Sec.sh_type == ELF::SHT_RELA || Sec.sh_type == ELF::SHT_REL))` and the value of the entry will either contain the relocations section or Error if it can't be retrieved. These errors will of course need to be consumed by the caller (which is the intention of making this a library call). I can see that the current callers (`printCGProfile` and `printRelocatableStackSizes`) both already iterate over the entire map. So they can easily report warnings upon consuming errors and it will be the same for our case. We don't need to keep another wrapper around this function `ELFDumper<ELFT>::getSectionAndRelocations` that applies a specific error handling. We can simply let the callers choose what to do with the errors. 

Clients can also ignore the errors which will trigger assertion error if `-DLLVM_ENABLE_ABI_BREAKING_CHECKS=true` IIUC.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144783