[PATCH] D107968: [llvm-readobj] Refactor ELFDumper::printAttributes()

Tue Aug 17 00:24:08 PDT 2021

jhenderson added a comment.

In D107968#2946973 <https://reviews.llvm.org/D107968#2946973>, @jozefl wrote:

> Also, is it ok to just remove all user-visible warnings when attributes
> aren't supported? If you try and dump attributes for for an unsupported
> target, nothing will happen, but there is a test for RISCV that checks
> for a warning when dumping from a big-endian encoded file.

I'm neither an ARM nor a RISCV developer, and so I don't know much about the attributes in detail. However, it seems to me like they'd be a generic feature for specific machines, not one tied specifically to the endianness of the host machine? In other words, a theoretical RISCV or ARM big endian machine might have attributes, right? In such a case, I think we should warn. If we don't, a user may get confused when the tool silently does nothing, even though they know there are attributes present. This is different to the case where a machine type doesn't have attributes - in such a case, there is no concept of them, so saying printing them isn't implemented doesn't really make sense (because there's nothing to implement).

================
Comment at: llvm/test/tools/llvm-readobj/ELF/RISCV/validate-attr-section.test:6

-# CHECK: Attributes not implemented.
+# CHECK-NOT: BuildAttributes {

----------------
A CHECK-NOT like this is fragile - if somebody changed the term "BuildAttributes" to e.g. simply "Attributes" this test would continue to pass, even if attributes were started to be printed.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107968