Page MenuHomePhabricator

[llvm-readobj] Support dumping of MSP430 ELF attributes
ClosedPublic

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

Details

Summary

The MSP430 ABI supports build attributes for specifying
the ISA, code model, data model and enum size in ELF object files.

Diff Detail

Event Timeline

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

This patch relies on https://reviews.llvm.org/D107968, hence the patch application failure.
Is there a way to link these patches in some way? I've added that revision as a parent of this revision.
Thanks,
Jozef

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

asl accepted this revision.Aug 16 2021, 2:18 AM

LGTM, thanks

This revision is now accepted and ready to land.Aug 16 2021, 2:18 AM

We generally test new llvm-readelf features with dedicated llvm-readelf tests, usually with the support of yaml2obj or failing that, llvm-mc to generate the input files, where possible. I've no understanding of the attribute section myself, so don't know if either of these are practical options or not, but they would be preferred if so.

jozefl updated this revision to Diff 366930.Aug 17 2021, 9:50 AM

Added yaml2obj tests that create SHT_MSP430_ATTRIBUTES sections by
directly specifying the raw contents of the section.

I've kept the CodeGen tests to ensure that we have coverage that the different
values for -mcpu= each emit the expected build attributes.

The ELFDumper.cpp changes have been refactored to apply cleanly on top of the
changes in https://reviews.llvm.org/D107968.

Thanks,
Jozef

jhenderson accepted this revision.Aug 18 2021, 12:17 AM

Looks reasonable to me, barring some nits. It would be worth @asl or someone else with MSP430 knowledge to take a brief look at the test.

llvm/test/tools/llvm-readobj/ELF/MSP430/build-attributes.test
24

Small nit: use - rather than _ in prefix names: it's more common in other tests, and is easier to type on a typical English keyboard.

85

Nit: add a blank line immediately before the YAML blocks. This just makes it a little easier to spot the end of the test directives and start of the YAML for a given test case.

jozefl updated this revision to Diff 367148.Aug 18 2021, 1:59 AM
jozefl marked 2 inline comments as done.

Thanks, fixed the nits.

Ping.
If the added yaml2obj test is acceptable, I would appreciate it if someone would "arc land" the patch for me, as I do not have commit access.

Thanks,
Jozef

Ping.
If the added yaml2obj test is acceptable, I would appreciate it if someone would "arc land" the patch for me, as I do not have commit access.

Thanks,
Jozef

Hi @jozefl,

The general assumption in LLVM is that someone has commit access to land their own work after an LGTM on the patch has been received, unless they specifically say so. Also, I personally don't use "arc" for landing patches, so I'll need your preferred email address and name for the commit message if you want me (or someone else) to land it for you. Alternatively, if you've already contributed a few patches, you can request commit access for yourself.

Ping.
If the added yaml2obj test is acceptable, I would appreciate it if someone would "arc land" the patch for me, as I do not have commit access.

Thanks,
Jozef

Hi @jozefl,

The general assumption in LLVM is that someone has commit access to land their own work after an LGTM on the patch has been received, unless they specifically say so. Also, I personally don't use "arc" for landing patches, so I'll need your preferred email address and name for the commit message if you want me (or someone else) to land it for you. Alternatively, if you've already contributed a few patches, you can request commit access for yourself.

Hi @jhenderson,

It would be much appreciated if you could apply the patch for me, the name and email to use is "Jozef Lawrynowicz <jozefl.opensrc@gmail.com>".
This would only be my second commit to LLVM, but once it's applied I'll try applying for commit access.

Thanks!
Jozef

Actually, I'd still like @asl to take a look at the test, since they presumably have more understanding of this area than I do. Sorry about that.

Pinging for review of the MSP430-specific yaml2obj test.

Thanks,
Jozef

asl added a comment.Mon, Sep 20, 1:36 AM

Sorry, I was AFK for a moment. Will review today

This revision was automatically updated to reflect the committed changes.

Thanks for the review, and committing the patch for me.

Jozef