Prototyping ELF attribute section for RISC-V according https://github.com/riscv/riscv-elf-psabi-doc/pull/71/files
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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. |
llvm/unittests/Support/ELFAttributeParser.cpp | ||
---|---|---|
5 ↗ | (On Diff #249024) | Ignore this. I was thinking of a different code base. |
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. |
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, 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? |
@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. |
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'. |
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 |
I create a patch, D76819, to apply clang-format to the ELF header file and ARM build attributes files.
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? |
llvm/lib/Support/ARMBuildAttrs.cpp | ||
---|---|---|
66–68 | I will rerun clang-format for it. |
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 |
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 = "" |
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.
llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp | ||
---|---|---|
86 | There is no need to escape the string. The string will be generated in RISCVTargetStreamer::emitTargetAttributes(). |