Page MenuHomePhabricator

[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
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
33

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
33

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
1727

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
21

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

34

when the "m" extension

llvm/unittests/Support/ELFAttributeParserTest.cpp
9

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

18

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
1727

I have added a comment for it.

llvm/unittests/Support/ELFAttributeParserTest.cpp
18

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
22

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

28

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
35–36

Same comment as elsewhere.

llvm/lib/Support/ARMBuildAttrs.cpp
13–14

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
35–36

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

llvm/lib/Support/ARMBuildAttrs.cpp
13–14

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
28

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
17

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–68

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–68

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";

86

Does String need to be escaped?

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

TS can be inlined.

195

TS can be inlined.

llvm/test/CodeGen/RISCV/attributes.ll
26

Delete nounwind (unneeded detail)

30

Delete empty line

llvm/test/MC/RISCV/attribute-arch.s
39

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
1689

No space before [

1723

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
86

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.