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

[James Henderson via Phabricator via llvm-commits]

jhenderson 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:
> 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).


================
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
+
----------------
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).


================
Comment at: llvm/test/tools/llvm-readobj/ELF/bb-addr-map-relocatable.test:167
+
+## Check that if we have a ET_DYN file with a .rela.dyn section, we don't get
+## a warning about a missing relocation section and can get the baddrmap data.
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143841