[RISCV] ELF attribute section for RISC-V

[HsiangKai]

Summary

Prototyping ELF attribute section for RISC-V according https://github.com/riscv/riscv-elf-psabi-doc/pull/71/files

[jhenderson]

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]

llvm/unittests/Support/ELFAttributeParser.cpp

5 ↗

(On Diff #249024)

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

[HsiangKai]

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.

[HsiangKai]

• Specify underlying type for AttrType.
• Update unit tests.

[HsiangKai]

Fix unit test errors.

[jhenderson]

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]

Address @jhenderson's comments.

[HsiangKai]

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]

Replace std::find_if with llvm::find_if.

[jhenderson]

@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]

In D74023#1933427, @jhenderson wrote:

@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?

[jhenderson]

In D74023#1937220, @HsiangKai wrote:

In D74023#1933427, @jhenderson wrote:

@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]

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]

In D74023#1938583, @jhenderson wrote:

In D74023#1937220, @HsiangKai wrote:

In D74023#1933427, @jhenderson wrote:

@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]

Rebase on master.

[HsiangKai]

Fix Harbormaster test failures.

[HsiangKai]

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.

[MaskRay]

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

llvm/unittests/Support/ELFAttributeParserTest.cpp

17

static const

[HsiangKai]

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

[HsiangKai]

Rebase on D76819.

[jhenderson]

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?

[HsiangKai]

llvm/lib/Support/ARMBuildAttrs.cpp

66–68

I will rerun clang-format for it.

[HsiangKai]

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

[MaskRay]

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]

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

[HsiangKai]

In D74023#1948388, @MaskRay wrote:

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}/.

[jhenderson]

In D74023#1949261, @HsiangKai wrote:

In D74023#1948388, @MaskRay wrote:

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]

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

86

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