[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())));
----------------
jhenderson wrote:
> aidengrossman wrote:
> > rahmanl wrote:
> > > 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.
> > The function loops through all of the sections and starts by seeing if the current section matches (ie it is a matching content section). If so, it inserts a pointer to the section into the map with the relocation section entered in as `nullptr` to be filled in later. If the section, doesn't match, but is a relocation section, the section linked to by `SH_INFO` is checked. That section will already be in the map if the relocation section follows the content section (which I believe is normally the case).
> > 
> > If the section is a rel/rela section, there are two failure cases. We either fail to get the relocated section and then can't even associate it with a content/relocated section to put it into the map, or the call to IsMatch with the linked to content section fails where I also don't think it would really be correct to put anything into the map, which suggests to me that the only error we'd be putting in an `Expected<const Elf_Shdr*>` map value would be a "not found" error, which has the consequences mentioned earlier.
> > 
> > We don't necessarily have to insert the content section pointer with a `nullptr` to the relocation section if we come across the content section first, but changing that around would mean that content sections that don't have relocation sections don't get put in.
> > 
> > Hopefully my logic makes sense here. If it doesn't, or I'm misunderstanding what you're trying to say, please let me know. Thank you for the feedback so far.
> I'd strongly advise against a Map that contains `Expected<T>` entries, because it means at some point, you have to iterate over all the entries to consume all the errors. Whilst you might do that already in the current usage, future users of the code might well just want to get an individual section that is in this vector, and will run into problems later on. I think it would be better to just bundle all errors into one with `joinErrors` that is then returned in the form of `Expected<MapVector<...>>` or similar.
This makes sense. Also I was also under the wrong impression that there are errors associated to every matching content section while errors are associated to the rela sections for which we can't get the content section (which is a serious error. Surprised to see that the initial code considers those warnings).  Matching sections which don't have a rela section simply get `nullptr`. Then we can adopt Jame's suggestion of just returning `Expected<MapVector<const Elf_Shdr *,  const Elf_Shdr *>>`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144783