[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));
+
----------------
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.


================
Comment at: llvm/test/tools/llvm-objdump/X86/elf-bbaddrmap-symbolize-relocatable.yaml:11
+# RUN: llvm-objdump %t1 -d --symbolize-operands -M att --no-show-raw-insn --no-leading-addr | \
+# RUN:   FileCheck %s -DSYM=symbol --match-full-lines
+
----------------
jhenderson wrote:
> rahmanl wrote:
> > nit: unnecessary spaces.
> The spaces should be left there. The idea is to provide an indentation to show that the FileCheck line is a continuation of the previous line (without having to spot that the previous line ends with `\`. It's a common pattern in many newer tests in the tools (especially ones I've been involved with).
You're right. sorry for the noise.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143841