[PATCH] D143841: [Propeller] Make decoding BBAddrMaps trace through relocations

[Rahman Lavaee via Phabricator via llvm-commits]

rahmanl added inline comments.


================
Comment at: llvm/lib/Object/ELF.cpp:645
+  bool IsRelocatable = getHeader().e_type == ELF::ET_REL;
+  assert(!IsRelocatable || (IsRelocatable && RelaSec));
+
----------------
rahmanl wrote:
> jhenderson wrote:
> > rahmanl wrote:
> > > Callers may call this function on relocatable sections without providing `RelaSec`. If assertions are not enabled, we will still get zero addresses for all functions. I think it's better to report error here instead.
> > > 
> > > In general, assertions should be used to check the invariants of the code, not things like input validation.
> > Hmm, I think assertions are perfectly valid to use for input validation for function arguments. The question I use to decide on assertion versus error is "can the assertion ever be hit with any of our existing code paths" - in other words, if somebody passed in an arbitrary binary blob to one of our tools that uses this code, could it theoretically get here?
> > 
> > The LLVM coding standard's first example on assert usage actually covers this case. See https://llvm.org/docs/CodingStandards.html#assert-liberally. In that case, the index is passed into the function and validated via an assert.
> > 
> > As an aside, @aidengrossman, you should have a message explaining the assertion (see the coding standards document).
> Thanks for your clarification @jhenderson. Using assertions for input validation does make sense.
> 
> Let me rephrase my opinion. The input validation here is a complex one. It is already being done three times in this code (in the callers ELFObjectFile.cpp:711 , ELFDumper.cpp:7196,  and here). The callers already create errors in such cases. I think moving the error handling code here simplifies the code. We don't have to add a message explaining that this function can only be called when `RelaSec != std::nullopt || getHeader().e_type != ELF:ET_REL` when we already have error handling for the same case in the callers.
Actually, I take my argument back. This function should block calls with assertions. Errors are relevant only in the context of the callers.


================
Comment at: llvm/lib/Object/ELF.cpp:651
+  llvm::DenseMap<uint64_t, uint64_t> FunctionOffsetTranslations;
+  if (IsRelocatable && RelaSec) {
+    Expected<Elf_Rela_Range> Relas = this->relas(*RelaSec);
----------------
Here, add
`assert(RelaSec && "Trying to read a SHT_LLVM_BB_ADDR_MAP section in a relocatable object file without providing a relocation section.");`
And remove the assertion above.


================
Comment at: llvm/lib/Object/ELFObjectFile.cpp:711-716
+    if (IsRelocatable) {
+      if (!RelocSec) {
+        return createError("unable to get relocation section for " +
+                           describe(EF, *Sec));
+      }
+      RelaSec = *RelocSec;
----------------
rahmanl wrote:
> Moving this error handling to `decodeBBAddr` and passing `RelaSec` by `const Elf_Shdr *` will greatly simplify this code:
> 
> ```
> for (auto const &[Sec, RelocSec] : *SectionRelocMapOrErr) {
>     Expected<std::vector<BBAddrMap>> BBAddrMapOrErr =
>         EF.decodeBBAddrMap(*Sec, RelocSec);
> }
> ```

```
if (IsRelocatable && !RelocSec) return createError(....)
Expected<std::vector<BBAddrMap>> BBAddrMapOrErr =
        EF.decodeBBAddrMap(*Sec, RelocSec);
```



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143841