Page MenuHomePhabricator

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

Authored by jozefl on Aug 12 2021, 8:39 AM.

Details

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.

Diff Detail

Event Timeline

jozefl created this revision.Aug 12 2021, 8:39 AM
jozefl requested review of this revision.Aug 12 2021, 8:39 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 12 2021, 8:39 AM

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

asl added a subscriber: asl.Aug 16 2021, 2:20 AM

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

jhenderson added inline comments.Aug 16 2021, 4:47 AM
llvm/tools/llvm-readobj/ELFDumper.cpp
2600

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 updated this revision to Diff 366636.Aug 16 2021, 8:12 AM

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

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

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 updated this revision to Diff 366841.Aug 17 2021, 2:12 AM
jozefl marked an inline comment as done.

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

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
2566

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.

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 updated this revision to Diff 366851.Aug 17 2021, 3:30 AM

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?

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

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

jozefl updated this revision to Diff 366863.Aug 17 2021, 4:49 AM
jozefl marked an inline comment as done.

Added a readobj test for ARM.

jhenderson accepted this revision.Aug 17 2021, 4:51 AM

Looks good!

This revision is now accepted and ready to land.Aug 17 2021, 4:51 AM

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 <jozefl.opensrc@gmail.com>".

Thanks,
Jozef

MaskRay accepted this revision.Aug 17 2021, 1:27 PM

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

This revision was automatically updated to reflect the committed changes.