This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] ELF attribute section for RISC-V
ClosedPublic

Authored by HsiangKai on Feb 5 2020, 12:43 AM.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
jhenderson added inline comments.Feb 27 2020, 1:47 AM
llvm/test/tools/llvm-readobj/ELF/RISCV/attribute.s
15 ↗(On Diff #246382)

Same comments re. spacing.

llvm/test/tools/llvm-readobj/ELF/RISCV/attribute.test
70

Seems like this comment still hasn't been addressed?

apazos added inline comments.Feb 27 2020, 7:36 AM
lld/test/ELF/Inputs/riscv-attributes1.s
1 ↗(On Diff #246382)

Comment refers to SHT_ARM_ATTRIBUTES instead of SHT_RISCV_ATTRIBUTES

HsiangKai marked 3 inline comments as done.Feb 27 2020, 8:48 AM
HsiangKai added inline comments.
lld/test/ELF/riscv-attributes.s
43–44 ↗(On Diff #246382)

Yes, this test case will link with another file located in Inputs directory. I create this test case to ensure this patch will not broken lld. So, I link two object files together and decode the ELF attributes.

llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
160–162

I will follow current LLVM coding standards before there is a final decision about variable names.

llvm/test/tools/llvm-readobj/ELF/RISCV/attribute.test
70

I create an assembly version in test/tools/llvm-readobj/ELF/RISCV/attribute.s. I just keep the binary format for the same test.

HsiangKai updated this revision to Diff 246982.Feb 27 2020, 9:21 AM

Formatting.

HsiangKai marked an inline comment as done.Feb 27 2020, 9:22 AM
HsiangKai added inline comments.
llvm/lib/Object/ELFObjectFile.cpp
308

I will add a test for it later.

Hi @MaskRay, I know you have a patch to refactor ARMAttributeParser in D75015.
If you have no other comments in this patch, do you mind let this patch upstream first and refactor ARMAttributeParser based on it?

MaskRay added inline comments.Feb 27 2020, 9:28 AM
lld/test/ELF/riscv-attributes.s
1 ↗(On Diff #246982)

I suggest not making changes to lld/test/ELF for this patch. The changes can be made in a follow-up patch.

3 ↗(On Diff #246982)

Remove -unknown-elf

43 ↗(On Diff #246982)

To disable a missing entry warning:

.globl _start
_start:

is sufficient.

HsiangKai updated this revision to Diff 246988.Feb 27 2020, 9:34 AM

More formatting for test cases.

HsiangKai marked an inline comment as done.Feb 27 2020, 9:40 AM
HsiangKai added inline comments.
lld/test/ELF/riscv-attributes.s
1 ↗(On Diff #246982)

Do you mean there is no need to add a test case for lld in this patch? Indeed, I have no modification for lld. I just want to ensure the patch will not broken lld.

Hi @MaskRay, I know you have a patch to refactor ARMAttributeParser in D75015.
If you have no other comments in this patch, do you mind let this patch upstream first and refactor ARMAttributeParser based on it?

I created D75015 because I thought

  • it would be more likely accepted before D74023. I am of the belief that new code does not have to start using VariableName.
  • Doing D75015 before D4023 can minimize the changes on the RISC-V side. (No future void -> Error. Better error reporting.)
llvm/test/MC/RISCV/invalid-attribute.s
10 ↗(On Diff #246988)

Write the column number explicitly.

HsiangKai marked an inline comment as done.Feb 27 2020, 6:34 PM
HsiangKai added inline comments.
lld/test/ELF/riscv-attributes.s
43 ↗(On Diff #246982)

I want to call another function func located in another file riscv-attributes1.s.

HsiangKai updated this revision to Diff 247159.Feb 27 2020, 6:44 PM
HsiangKai updated this revision to Diff 247246.Feb 28 2020, 6:07 AM

Use ELFObjectFileBase::getARMFeatures() as reference, return empty feature set when encountering errors and ignore unknown features.d

HsiangKai updated this revision to Diff 247250.Feb 28 2020, 6:17 AM
Harbormaster completed remote builds in B47580: Diff 247246.
HsiangKai updated this revision to Diff 247445.Feb 29 2020, 7:33 AM
HsiangKai marked an inline comment as done.
HsiangKai added inline comments.
llvm/lib/Object/ELFObjectFile.cpp
308

Add a test case llvm/test/tools/llvm-readobj/ELF/RISCV/unknown-arch-attr.test.

I think I have addressed all the comments.
Is there anything I am not aware or any comments I missed out?

In lld, SHT_ARM_ATTRIBUTES sections are deduplicated, thus test/ELF/arm-attributes.s. No code deduplicate SHT_RISCV_ATTRIBUTES, thus I still suggest we delete the lld test change.

Landing D75015 first can reduce changes in this patch....

lld/test/ELF/Inputs/riscv-attributes1.s
2 ↗(On Diff #247445)

.text can be deleted

lld/test/ELF/riscv-attributes.s
43 ↗(On Diff #246982)

ld.lld %t1.rv32.o %t2.rv32.o links both object files. It is not necessary to add a call.

HsiangKai updated this revision to Diff 247473.Feb 29 2020, 7:30 PM

Remove lld test cases and rebase on master branch.

HsiangKai updated this revision to Diff 247475.Feb 29 2020, 7:39 PM

Use clang-format to tidy up.

jhenderson added inline comments.Mar 2 2020, 1:57 AM
llvm/test/tools/llvm-readobj/ELF/RISCV/section-types.test
11 ↗(On Diff #247475)

You don't need this line.

23 ↗(On Diff #247475)

You don't need the Content blob, as the contents of the section aren't important for printing the section header.

llvm/test/tools/llvm-readobj/ELF/RISCV/unknown-arch-attr.test
4 ↗(On Diff #247475)

process the following

6 ↗(On Diff #247475)

Write this in the third person, describing what is in the object file, i.e. something like "The object file has the "rv32i2p0_x1p0_m2p0_a2p0" arch feature."

22–27 ↗(On Diff #247475)

Are these bits relevant to the thing under test?

30 ↗(On Diff #247475)

Maybe change this prefix to just DISASM:

42 ↗(On Diff #247475)

I'm guessing this test can't be assembly because it needs to use an unsupported feature? If so, please comment this and the other content block below describing what the content represents. E.g. here you might put something like mov %rsp,3 (or whatever this actually is).

HsiangKai updated this revision to Diff 247789.Mar 2 2020, 10:28 PM

Update test cases.

There's a lot of this code that I don't have the knowledge to review, so I'm just focusing on the cosmetic aspects primarily.

Also, what do you think about splitting the changes up a bit. In particular, I think the section types testing for llvm-readobj could be done separately, along with the required code changes (e.g. the SHT_RISCV section type). There are probably other parts too.

llvm/include/llvm/Support/ELFAttributeParser.h
23–24

Do these need to be ordered, or can they be unordered_map (or an LLVM equivalent)?

llvm/include/llvm/Support/RISCVAttributeParser.h
28–29

These function names don't follow LLVM style (old or new). They should be unalignedAccess and stackAlign.

llvm/include/llvm/Support/RISCVAttributes.h
29

Rest of what?

30–35

These names are not in LLVM style. Are they matching part of the specification? If not, please change them.

llvm/lib/Object/ELFObjectFile.cpp
289

is -> are
feature -> features
ignore it -> ignore tehm

292

from -> in

323

To take care -> Handle

llvm/lib/Support/ARMBuildAttrs.cpp
11

The formatting changes here seem unrelated so shouldn't be made with this change, right?

llvm/lib/Support/ELFAttributes.cpp
30 ↗(On Diff #247789)

Perhaps this should return an Optional<T> to avoid the -1 magic number?

32 ↗(On Diff #247789)

Range-based for loop?

llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp
121

Nit: missing trailing full stop.

llvm/test/MC/RISCV/invalid-attribute.s
1 ↗(On Diff #247789)

Negative tests:

2–4 ↗(On Diff #247789)

Full stop at end of each line.

HsiangKai marked 3 inline comments as done.Mar 5 2020, 5:33 AM
HsiangKai added inline comments.
llvm/include/llvm/Support/RISCVAttributes.h
29

Originally, there are three common types before architecture-specific attributes, File, Section, Symbol. These three attribute types have been moved to ELFAttributes.h. I think it is indeed confusing here. I will modify this comment.

30–35

These are some definitions in RISC-V ELF specification. I looked up some ELF definitions in llvm/include/llvm/BinaryFormat/ELF.h. I think it should be fine to include underscore in the names. So, I will capitalize all these names and keep underscores.

llvm/lib/Support/ARMBuildAttrs.cpp
11

They are formatted by clang-format. Actually, I am a little bit confused whether to apply clang-format on unrelated code or not. I will restore them.

HsiangKai updated this revision to Diff 248460.Mar 5 2020, 6:05 AM
HsiangKai updated this revision to Diff 248520.Mar 5 2020, 9:43 AM
HsiangKai updated this revision to Diff 248523.Mar 5 2020, 9:50 AM
HsiangKai updated this revision to Diff 248660.Mar 6 2020, 1:40 AM

Rebase on D75015. @MaskRay, could you help me to review this update? Thanks.

jhenderson added inline comments.Mar 6 2020, 1:41 AM
llvm/include/llvm/Support/ELFAttributeParser.h
60

No need for else after a return:

if (I == Attributes.end())
  return None;
return I->second;
67

No need for else after return.

llvm/include/llvm/Support/ELFAttributes.h
32 ↗(On Diff #248523)

Can this be Optional<AttrType>?

llvm/include/llvm/Support/RISCVAttributes.h
30–35

Okay, cool. Since these names are part of the standard, they should match exactly what they are in the standard if possible (casing, underscores etc).

llvm/lib/Support/ARMBuildAttrs.cpp
11

In general, you should only format the code you are working on. Unrelated code should not be changed usually.

HsiangKai marked an inline comment as done.Mar 6 2020, 5:59 AM
HsiangKai added inline comments.
llvm/include/llvm/Support/ELFAttributes.h
32 ↗(On Diff #248523)

AttrType is the enum in ELFAttrs. There is also AttrType in ARMBuildAttributes and RISCVAttributes. They all could be implicitly converted to integer. So, I use integer as the common interface between different architecture attributes enum.

HsiangKai updated this revision to Diff 248713.Mar 6 2020, 6:00 AM

The change to ARMAttributeParser.cpp (Error (ARMAttributeParser::*routine)(AttrType); -> Error (ARMAttributeParser::*routine)(unsigned);) is not needed.

Some common parsing tests can be moved from unittests/Support/ARMAttributeParser.cpp to unittests/Object/ELFAttributeParser.cpp

I have checked whether it is easy to move Support/ARM{AttributeParser,BuildAttrs,TargetParser}.cpp to Object/. It is not, but I think the end goal is probably to have them under Object/.

The llvm-readobj changes and tests can be moved to a separate patch.

llvm/include/llvm/Support/ARMAttributeParser.h
19–20

It is not necessary to change AttrType to unsigned.

Reverting the change the reduce the diff in this file to a minimum.

llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp
167

The variable can be inlined

176

excess braces

HsiangKai updated this revision to Diff 249024.Mar 8 2020, 7:52 PM

Move llvm-readobj changes and tests to D75833.

Herald added a project: Restricted Project. · View Herald TranscriptMar 8 2020, 7:52 PM
jhenderson added inline comments.Mar 11 2020, 2:28 AM
llvm/include/llvm/Support/ELFAttributes.h
32 ↗(On Diff #248523)

Okay, seems reasonable. I wonder if AttrType needs to be specified as having unsigned underlying tap (in all cases) to be consistent with the TagNameItem::attr member.

llvm/unittests/Support/ELFAttributeParser.cpp
1 ↗(On Diff #249024)

Missing licence header.

Test files usually are called *Test.cpp, where * is the base file/class that is being tested. It seems like this file is only testing the ARMAttributeParser. Should it be called "ARMAttributeParserTest.cpp"

5 ↗(On Diff #249024)

I think it's normal to put a blank line between standard library headers and other includes.

jhenderson added inline comments.Mar 11 2020, 2:34 AM
llvm/unittests/Support/ELFAttributeParser.cpp
5 ↗(On Diff #249024)

Ignore this. I was thinking of a different code base.

HsiangKai marked 2 inline comments as done.Mar 12 2020, 12:01 AM
HsiangKai added inline comments.
llvm/include/llvm/Support/ELFAttributes.h
32 ↗(On Diff #248523)

OK, I will specify the underlying type explicitly.

llvm/unittests/Support/ELFAttributeParser.cpp
1 ↗(On Diff #249024)

This file tests the common part of the attribute section, i.e., attribute section header. However, since ELFAttributeParser is an abstract class, I need to use ARMAttributeParser or RISCVAttributeParser as the concrete object to run the test. I just pick ARMAttributeParser as the concrete object. This test is not used to test ARMAttributeParser.

You could refer to https://reviews.llvm.org/D74023#1911405. @MaskRay suggests to extract the common part out.

  • Specify underlying type for AttrType.
  • Update unit tests.
HsiangKai updated this revision to Diff 249849.Mar 12 2020, 1:00 AM
HsiangKai updated this revision to Diff 249854.Mar 12 2020, 1:46 AM

Fix unit test errors.

jhenderson added inline comments.Mar 16 2020, 2:57 AM
llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
1723

I don't understand why this line changed, but more importantly, the 2 looks like a magic number and it is unclear why that value is the correct one. Is there another way of writing this that would be more correct (e.g. bitwise & against a known flag value)?

llvm/test/tools/llvm-objdump/RISCV/unknown-arch-attr.test
20 ↗(On Diff #249854)

No need for the large amount of whitespace between DISASM: and mul. One space is sufficient.

33 ↗(On Diff #249854)

when the "m" extension

llvm/unittests/Support/ELFAttributeParserTest.cpp
8 ↗(On Diff #249854)

I think it's normal to have a blank line after the licence header.

17 ↗(On Diff #249854)

If these tests are for the generic parts of the attribute parser, you should probably define a concrete parser type that is neither ARM nor RISC-V, and use that in the tests, e.g. TestAttributeParser.

If that's not practical for whatever reason, you need to put a comment somewhere in this file indicating that the ARMAttributeParser is used here for generality. Alternatively, consider changing this function to take a template argument for the parser type, and call the test for all instantiated parser types, or simply duplicating the contents of this function, but for the other parser type.

HsiangKai updated this revision to Diff 251244.Mar 18 2020, 7:13 PM

Address @jhenderson's comments.

HsiangKai marked 2 inline comments as done.Mar 18 2020, 7:21 PM
HsiangKai added inline comments.
llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
1723

I have added a comment for it.

llvm/unittests/Support/ELFAttributeParserTest.cpp
17 ↗(On Diff #249854)

Thanks for your suggestions. It makes sense. I have added a concrete parser for testing.

HsiangKai updated this revision to Diff 251375.Mar 19 2020, 7:19 AM

Replace std::find_if with llvm::find_if.

@HsiangKai, have you noticed that there are some test failures according to the harbourmaster bot? They look like they could be related to this somehow.

llvm/unittests/Support/ELFAttributeParserTest.cpp
21 ↗(On Diff #251375)

I might suggest changing this comment to "Treat all attributes as handled."

27 ↗(On Diff #251375)

sw seems a bit of a random name. What does it mean?

@HsiangKai, have you noticed that there are some test failures according to the harbourmaster bot? They look like they could be related to this somehow.

@jhenderson, yes, I found test failures in harbormaster. The failures are occurred after I rebased my patch on master branch. After digging into error messages, I found the failures are triggered by find_if(). Maybe I misuse find_if() in this patch? Do you have any idea about this?
By the way, I also found some patch, D75015, landed even harbormaster is failed. I am curious about is it a necessary condition to pass harbormaster before landing?

@HsiangKai, have you noticed that there are some test failures according to the harbourmaster bot? They look like they could be related to this somehow.

@jhenderson, yes, I found test failures in harbormaster. The failures are occurred after I rebased my patch on master branch. After digging into error messages, I found the failures are triggered by find_if(). Maybe I misuse find_if() in this patch? Do you have any idea about this?
By the way, I also found some patch, D75015, landed even harbormaster is failed. I am curious about is it a necessary condition to pass harbormaster before landing?

I don't have much understanding of how Harbormaster works, and it may be that the failures are unrelated to anything you did, since I believe it just applies your patch on top of the current HEAD of master, which might not work for various reasons. Still, it's worth reviewing and locally checking the same tests to make sure they aren't failing locally. If you review the logs produced, you might spot an issue. If Harbormaster is failing for a reason related to your patch, your patch will almost certainly cause build bot failures, so in that case, it is necessary to fix the issues (but in other cases, if the issues are unrelated, it isn't).

As for why find_if isn't working, I don't know, and I'd suggest you debug it.

llvm/include/llvm/Support/ARMBuildAttributes.h
37

Same comment as elsewhere.

llvm/lib/Support/ARMBuildAttrs.cpp
10–11

By the way: this clang-format failure might be related to your changes, so it's probably worth checking to see if it is incorrect without your changes, and if not, reformat it as part of this patch.

HsiangKai marked 3 inline comments as done.Mar 24 2020, 1:58 AM
HsiangKai added inline comments.
llvm/include/llvm/Support/ARMBuildAttributes.h
37

I prefer to keep the formatting in this file. We could create another NFC patch for it.

llvm/lib/Support/ARMBuildAttrs.cpp
10–11

I didn't change the formatting in this file. I think it is not related to this patch. We could create another NFC patch to correct the formatting in ARMBuildAttrs.cpp.

llvm/unittests/Support/ELFAttributeParserTest.cpp
27 ↗(On Diff #251375)

I do not know either. I borrowed the name from ARMAttributeParser. I will change the variable name to 'printer'.

@HsiangKai, have you noticed that there are some test failures according to the harbourmaster bot? They look like they could be related to this somehow.

@jhenderson, yes, I found test failures in harbormaster. The failures are occurred after I rebased my patch on master branch. After digging into error messages, I found the failures are triggered by find_if(). Maybe I misuse find_if() in this patch? Do you have any idea about this?
By the way, I also found some patch, D75015, landed even harbormaster is failed. I am curious about is it a necessary condition to pass harbormaster before landing?

I don't have much understanding of how Harbormaster works, and it may be that the failures are unrelated to anything you did, since I believe it just applies your patch on top of the current HEAD of master, which might not work for various reasons. Still, it's worth reviewing and locally checking the same tests to make sure they aren't failing locally. If you review the logs produced, you might spot an issue. If Harbormaster is failing for a reason related to your patch, your patch will almost certainly cause build bot failures, so in that case, it is necessary to fix the issues (but in other cases, if the issues are unrelated, it isn't).

As for why find_if isn't working, I don't know, and I'd suggest you debug it.

Thanks for your reply. I will try to figure out why Harbormaster is failed.

HsiangKai updated this revision to Diff 252300.Mar 24 2020, 6:34 AM

Rebase on master.

HsiangKai updated this revision to Diff 252562.Mar 25 2020, 6:52 AM

Fix Harbormaster test failures.

HsiangKai updated this revision to Diff 252591.Mar 25 2020, 8:49 AM

Harbormaster result:

Unit tests pass. 64040 tests passed, 0 failed and 650 were skipped.
clang-tidy fail. clang-tidy found 1 errors and 18 warnings. 0 of them are added as review comments why?.
clang-format fail. Please format your changes with clang-format by running `git-clang-format HEAD^` or applying this patch.

These errors are caused by formatting in ELF.h, ARMBuildAttributes.h, and ARMBuildAttrs.cpp. Although they are located in the same files this patch modified, they are not related to this patch. These errors could be ignored.

I remember the patch caused many failures yesterday. The new diff is good.

llvm/unittests/Support/ELFAttributeParserTest.cpp
16 ↗(On Diff #251375)

static const

HsiangKai updated this revision to Diff 252710.Mar 25 2020, 5:14 PM

I create a patch, D76819, to apply clang-format to the ELF header file and ARM build attributes files.

jhenderson accepted this revision.Mar 26 2020, 1:21 AM

I haven't really reviewed the funcional parts of this change in the attribute parser stuff, but everything else LGTM. Please wait for somebody else to review the attribute parser bits.

llvm/lib/Support/ARMBuildAttrs.cpp
66–67

clang-format appears to be complaining here?

This revision is now accepted and ready to land.Mar 26 2020, 1:21 AM
HsiangKai marked an inline comment as done.Mar 26 2020, 2:06 AM
HsiangKai added inline comments.
llvm/lib/Support/ARMBuildAttrs.cpp
66–67

I will rerun clang-format for it.

HsiangKai updated this revision to Diff 252801.Mar 26 2020, 4:28 AM

@MaskRay, do you have any other comments about this patch?

MaskRay added inline comments.Mar 28 2020, 8:50 PM
llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp
102

emitInt8

113

emitInt32

115

emitInt8

llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp
42

These empty lines seem unneeded.

44

Just Arch += "_a2p0";

93

Does String need to be escaped?

llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp
170

TS can be inlined.

179

TS can be inlined.

llvm/test/CodeGen/RISCV/attributes.ll
25 ↗(On Diff #252801)

Delete nounwind (unneeded detail)

29 ↗(On Diff #252801)

Delete empty line

llvm/test/MC/RISCV/attribute-arch.s
38 ↗(On Diff #252801)

Delete empty line

MaskRay added a comment.EditedMar 28 2020, 9:13 PM

The code generally looks good. For unittests, I think we can either make llvm-readobj -A canonical or the unittests canonical. If we decide to place tests on one place, we should delete most tests on the other side.

My current preference is that we use more of unittests and leave the minimum to test/llvm-readobj/ELF/{ARM,RISCV}/

llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
1685

No space before [

1719

Delete = ""

The code generally looks good. For unittests, I think we can either make llvm-readobj -A canonical or the unittests canonical. If we decide to place tests on one place, we should delete most tests on the other side.

My current preference is that we use more of unittests and leave the minimum to test/llvm-readobj/ELF/{ARM,RISCV}/

Agree. I will remove redundant tests in test/tools/llvm-readobj/ELF/{ARM,RISCV}/.

The code generally looks good. For unittests, I think we can either make llvm-readobj -A canonical or the unittests canonical. If we decide to place tests on one place, we should delete most tests on the other side.

My current preference is that we use more of unittests and leave the minimum to test/llvm-readobj/ELF/{ARM,RISCV}/

Agree. I will remove redundant tests in test/tools/llvm-readobj/ELF/{ARM,RISCV}/.

I also agree with testing primarily through the unit-tests where possible. However, there are probably aspects that still need to be tested via lit, namely code behaviour in ELFDumper.cpp, so just be slightly wary. High-level examples that might or might not be relevant for this patch are making sure that the output is printed correctly, and making sure errors/warnings can be threaded up to the final output.

HsiangKai marked an inline comment as done.Mar 30 2020, 4:35 AM
HsiangKai added inline comments.
llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp
93

There is no need to escape the string. The string will be generated in RISCVTargetStreamer::emitTargetAttributes().

HsiangKai updated this revision to Diff 253553.Mar 30 2020, 4:41 AM
This revision was automatically updated to reflect the committed changes.