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
Unit Tests
Event Timeline
lld/test/ELF/Inputs/riscv-attributes1.s | ||
---|---|---|
1 ↗ | (On Diff #246382) | Comment refers to SHT_ARM_ATTRIBUTES instead of SHT_RISCV_ATTRIBUTES |
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 | ||
69 ↗ | (On Diff #245670) | I create an assembly version in test/tools/llvm-readobj/ELF/RISCV/attribute.s. I just keep the binary format for the same test. |
llvm/lib/Object/ELFObjectFile.cpp | ||
---|---|---|
317 | I will add a test for it later. |
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. |
lld/test/ELF/riscv-attributes.s | ||
---|---|---|
43 ↗ | (On Diff #246982) | I want to call another function func located in another file riscv-attributes1.s. |
Use ELFObjectFileBase::getARMFeatures() as reference, return empty feature set when encountering errors and ignore unknown features.d
llvm/lib/Object/ELFObjectFile.cpp | ||
---|---|---|
317 | 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. |
llvm/test/tools/llvm-readobj/ELF/RISCV/section-types.test | ||
---|---|---|
12 | You don't need this line. | |
24 | 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 | ||
5 | process the following | |
7 | 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." | |
23–28 | Are these bits relevant to the thing under test? | |
31 | Maybe change this prefix to just DISASM: | |
43 | 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). |
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 | ||
298 | is -> are | |
301 | from -> in | |
332 | To take care -> Handle | |
llvm/lib/Support/ARMBuildAttrs.cpp | ||
58 | The formatting changes here seem unrelated so shouldn't be made with this change, right? | |
llvm/lib/Support/ELFAttributes.cpp | ||
31 | Perhaps this should return an Optional<T> to avoid the -1 magic number? | |
33 | Range-based for loop? | |
llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp | ||
121 | Nit: missing trailing full stop. | |
llvm/test/MC/RISCV/invalid-attribute.s | ||
2 | Negative tests: | |
3–5 | Full stop at end of each line. |
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 | ||
58 | 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. |
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 | ||
33 | 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 | ||
58 | In general, you should only format the code you are working on. Unrelated code should not be changed usually. |
llvm/include/llvm/Support/ELFAttributes.h | ||
---|---|---|
33 | 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. |
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 | ||
---|---|---|
26 | 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 | ||
183 | The variable can be inlined | |
192 | excess braces |
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 | ||
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, 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? |
@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 | ||
15 | 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 | ||
---|---|---|
37 | I prefer to keep the formatting in this file. We could create another NFC patch for it. | |
llvm/lib/Support/ARMBuildAttrs.cpp | ||
15 | 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'. |
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 |
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 | ||
---|---|---|
64 | clang-format appears to be complaining here? |
llvm/lib/Support/ARMBuildAttrs.cpp | ||
---|---|---|
64 | 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"; | |
93 | 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 | ||
---|---|---|
93 | There is no need to escape the string. The string will be generated in RISCVTargetStreamer::emitTargetAttributes(). |
clang-format: please reformat the code