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

[jozefl]

Summary

The current implementation of printAttributes makes it fiddly to extend
attribute support for new targets.

By refactoring the code so all target specific variables are
initialized in a switch/case statement, it becomes simpler to extend
attribute support for new targets.

[jozefl]

I don't have commit access, so if this patch is OK, I would appreciate if some would apply it for me.
Thanks.

[asl]

In general, this looks reasonable refactoring to me. @jhenderson ?

[jhenderson]

llvm/tools/llvm-readobj/ELFDumper.cpp

2592

I'm mildly uncomfortable that there is an overlap between this switch statement and that contained in printArchSpecificInfo - you'd need to update both switch statements to access this code. I wonder if you could change printAttributes to take the parser and required section type as an input argument and create it in the appropriate place in printArchSpecificInfo instead?

[jozefl]

Thanks @jhenderson, good point. Fixed in the updated diff.

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.

In my updated diff, I removed these warnings, so we have some consistency.

Thanks,
Jozef

[jhenderson]

In 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).

llvm/test/tools/llvm-readobj/ELF/RISCV/validate-attr-section.test

6 ↗

(On Diff #366636)

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.

[jozefl]

Ah yes that makes sense. My mistake was assuming there were other targets with
unimplemented ELF attribute support when in fact, MSP430 is the only one.

Updated the diff with the warnings added back.

I don't see any reference to endinanness in the build attributes section of the
ARM ABI[1], so at the moment it is only coincidental that both ARM and RISCV
don't have attribute support for big-endian machines. This means we need the
duplicated warnings when big-endian mode is used.

Thanks,
Jozef

[1] https://github.com/ARM-software/abi-aa/blob/2bcab1e3b22d55170c563c3c7940134089176746/aaelf32/aaelf32.rst#build-attributes

[jhenderson]

In D107968#2948883, @jozefl wrote:

Ah yes that makes sense. My mistake was assuming there were other targets with
unimplemented ELF attribute support when in fact, MSP430 is the only one.

I wouldn't assume that there aren't others, but at this point, we can't know which machine kinds might still be missing an implementation, so we can't really warn for them.

llvm/tools/llvm-readobj/ELFDumper.cpp

2565

This might be a good chance to change the stdout message to a stderr warning, by using printUniqueWarning(). Also a good chance to change the style to match the current LLVM warning styles (i.e. no leading capital letter and no trailing full stop - if using printUniqueWarning, you can also omit the \n).

You could also change the text to say "big-endian ARM" (and RISCV below). I'd also change "modes" to "objects". Approximate full suggestion inline.

[jozefl]

In D107968#2948901, @jhenderson wrote:

In D107968#2948883, @jozefl wrote:

Ah yes that makes sense. My mistake was assuming there were other targets with
unimplemented ELF attribute support when in fact, MSP430 is the only one.

I wouldn't assume that there aren't others, but at this point, we can't know which machine kinds might still be missing an implementation, so we can't really warn for them.

I checked the source of binutils-readelf, and for the targets that LLVM supports, llvm-readelf has parity with it in terms of the targets that it will dump a target-specific attribute section for.
However, binutils-readelf also handles SHT_GNU_ATTRIBUTES, which can be used with any target.

[jozefl]

Fixed in the attached.

There's lots of warnings in ELFDumper.cpp that use the W.startLine() approach
of printing warnings. As I understand from the LLVM Coding Standards, the
community does not want patches that performs large-scale refactoring, such as
changing the W.startLine to reportUniqueWarning, and changing the text to match
current LLVM style. Given this, I assume a bug report that details this
inconsistencies is not required, and that they will just get updated when the
surrounding code next gets updated?

[jhenderson]

In D107968#2948996, @jozefl wrote:

Fixed in the attached.

There's lots of warnings in ELFDumper.cpp that use the W.startLine() approach
of printing warnings. As I understand from the LLVM Coding Standards, the
community does not want patches that performs large-scale refactoring, such as
changing the W.startLine to reportUniqueWarning, and changing the text to match
current LLVM style. Given this, I assume a bug report that details this
inconsistencies is not required, and that they will just get updated when the
surrounding code next gets updated?

Yes, exactly.

llvm/test/tools/llvm-readobj/ELF/RISCV/validate-attr-section.test

1 ↗

(On Diff #366851)

Just realised that with the way you've refactored the code, we need an equivalent test for ARM too.

[jozefl]

Added a readobj test for ARM.

[jhenderson]

Looks good!

[jozefl]

Thanks for the reviews!

I don't have commit access, so would appreciate if someone would commit the patch for me, using the name and email: "Jozef Lawrynowicz <[e-mail]>".

Thanks,
Jozef

[MaskRay]

(You used arc diff to upload the patch, so arc patch D107968 pulled commit has the correct author line.)

[jozefl]

@MaskRay thanks!

llvm/tools/llvm-readobj/ELFDumper.cpp

 template <class ELFT> void ELFDumper<ELFT>::printArchSpecificInfo() {
   switch (Obj.getHeader().e_machine) {
   case EM_ARM:
   case EM_RISCV:
     printAttributes();
     break;
   case EM_MIPS: {
     printMipsABIFlags();
     printMipsOptions();
     printMipsReginfo();
     MipsGOTParser<ELFT> Parser(*this);
     if (Error E = Parser.findGOT(dynamic_table(), dynamic_symbols()))
       reportUniqueWarning(std::move(E));
     else if (!Parser.isGotEmpty())
       printMipsGOT(Parser);
 
     if (Error E = Parser.findPLT(dynamic_table()))
       reportUniqueWarning(std::move(E));
     else if (!Parser.isPltEmpty())
       printMipsPLT(Parser);
     break;
   }
   default:
     break;
   }
 }
 
 template <class ELFT> void ELFDumper<ELFT>::printAttributes() {
   if (!Obj.isLE()) {
     W.startLine() << "Attributes not implemented.\n";
     return;
   }
 
-  const unsigned Machine = Obj.getHeader().e_machine;
-  assert((Machine == EM_ARM || Machine == EM_RISCV) &&
-         "Attributes not implemented.");
+  auto AttrShType = ELF::SHT_NULL;
+  std::unique_ptr<ELFAttributeParser> AttrParser;
+  switch (Obj.getHeader().e_machine) {
+  case EM_ARM:
+    AttrShType = ELF::SHT_ARM_ATTRIBUTES;
+    AttrParser = std::make_unique<ARMAttributeParser>(&W);
+    break;
+  case EM_RISCV:
+    AttrShType = ELF::SHT_RISCV_ATTRIBUTES;
+    AttrParser = std::make_unique<RISCVAttributeParser>(&W);
+    break;
+  default:
+    llvm_unreachable("ELF attributes not implemented for target");
+    return;
+  }
+  assert((AttrShType != ELF::SHT_NULL && AttrParser != nullptr) &&
+         "Incomplete ELF attribute implementation");
 
   DictScope BA(W, "BuildAttributes");
   for (const Elf_Shdr &Sec : cantFail(Obj.sections())) {
-    if (Sec.sh_type != ELF::SHT_ARM_ATTRIBUTES &&
-        Sec.sh_type != ELF::SHT_RISCV_ATTRIBUTES)
+    if (Sec.sh_type != AttrShType)
       continue;
 
     ArrayRef<uint8_t> Contents;
     if (Expected<ArrayRef<uint8_t>> ContentOrErr =
             Obj.getSectionContents(Sec)) {
       Contents = *ContentOrErr;
       if (Contents.empty()) {
         reportUniqueWarning("the " + describe(Sec) + " is empty");
         continue;
       }
     } else {
       reportUniqueWarning("unable to read the content of the " + describe(Sec) +
                           ": " + toString(ContentOrErr.takeError()));
       continue;
     }
 
     W.printHex("FormatVersion", Contents[0]);
 
-    auto ParseAttrubutes = [&]() {
-      if (Machine == EM_ARM)
-        return ARMAttributeParser(&W).parse(Contents, support::little);
-      return RISCVAttributeParser(&W).parse(Contents, support::little);
-    };
-
-    if (Error E = ParseAttrubutes())
+    if (Error E = AttrParser->parse(Contents, support::little))
       reportUniqueWarning("unable to dump attributes from the " +
                           describe(Sec) + ": " + toString(std::move(E)));
   }
 }
 
 namespace {
 
 template <class ELFT> class MipsGOTParser {