Now .riscv.attributes section from the first file is taken and placed to result elf file. This can cause problems with using GNU tools, e.g. GNU objdump looks at this section to get extensions and may fail to decode instructions from c extension if there is no c mentioned in riscv.attributes section.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
lld/ELF/SyntheticSections.cpp | ||
---|---|---|
3758 | I think this assertion should be removed or use error instead, but I realized one fact soon is: we'll check the version during RISCVISAInfo::parseArchString and it will error out if the version is unsupported, so the assertion should always true for this moment. Anyway I am OK with that for now, but I would prefer that let @MaskRay to make the final call for that. Could you add a TODO: to the comment, like TODO: Versions should be the same otherwise parser... | |
3781 | This should just error if the stack alignment is different, see psABI for that https://github.com/riscv-non-isa/riscv-elf-psabi-doc/blob/master/riscv-elf.adoc#tag_riscv_stack_align-4-uleb128value |
Can you add a link to the official ABI and the relevant chapter?
riscv.attributes sections
Change these to .riscv.attributes in the subject and description and comments.
lld/ELF/SyntheticSections.cpp | ||
---|---|---|
3689 | The extra memcpy is wasteful. | |
3702 | delete the blank line | |
3705 | drop braces | |
3768 | Does make_unique work? | |
3772 | drop braces around single line simple statements. | |
3846 | drop blank line. | |
3866 | drop blank line. |
Sorry for the delay. I think this still requires some changes.
lld/ELF/Driver.cpp | ||
---|---|---|
2821 | "Combine RISCV attributes." and this comment have the same meaning. Just place the comment before if. This block does not belong to generic code. Move it to Arch/RISCV.cpp | |
2826 | This comment does not say beyond what the code explains. Remove it. | |
2832 | ctx.inputSections[0] being an attribute section is possible, so the code should handle the case, instead of using an assert which would lead to a runtime crash (either assert or attributesSectionPlace). | |
lld/ELF/InputFiles.cpp | ||
606 | no braces around single line simple statements. | |
608 | avoid the used-once variable | |
610 | errorOrWarn so that users can ignore the error with --noinhibit-exec. | |
lld/ELF/SyntheticSections.cpp | ||
3684 | Delete comments when the function name is self-explanatory. There are many places where a redundant comment is added. Please drop. | |
3705 | not done | |
lld/ELF/SyntheticSections.h | ||
1197 | The comment is not useful. | |
1198 | This variable can be a const. constexpr | |
1305 | This must be reset in reset() | |
lld/test/ELF/riscv-merge-attributes.s | ||
40 | no trailing blank line |
[ELF][RISCV] Merge riscv.attributes sections from all input files
.riscv.attributes
You may drop from all input files since it is not precise: we only merge sections from relocatable object files, e.g. not from shared objects.
Ideally, add a lld/test/ELF/lto/ test that an attribute section from a bitcode file (compiled by compileBitcodeFiles) is handled as well.
lld/ELF/Driver.cpp | ||
---|---|---|
2821 | Replaced to copy_if, this comment was connected with creating container with all riscv attributes sections. |
Some variable names are overly longer and are unlike the rest of lld/ELF.
lld/ELF/Arch/RISCV.cpp | ||
---|---|---|
824 | The name is verbose and isn't like other lld code. Just sections is fine since we are in the context of combineRISCVAttributesSections | |
826 | This with erase_if can be simplified as: llvm::erase_if(ctx.inputSections, [&](...) { if (!riscvattribute) return false; sections.push_back(...); return true; }) | |
828 | early return is more common | |
834 | The name is verbose. Just place is fine. | |
lld/ELF/Driver.cpp | ||
2818 | drop braces | |
lld/ELF/SyntheticSections.cpp | ||
3691 | parser | |
3695 | Just avoid the variable? It isn't much shorter than currentTag.attr. You can use tag instead of currentTag | |
3721 | Prefer lower-case diagnostics. See the coding standard about error messages. | |
3736 | no trailing dot. avoid std::to_string. error(Twine(some_integer) + "...") |
Thank you for the patch and sorry for my previous belated response. I took a careful look today. I think a very large architectural refactoring is needed.
We probably want to use an armExidx style to merge sections.
Merging attributes as we see an input section is probably more convenient than the current approach maintaining two DenseMap with DenseSet as values (minor size concern) and iterating over all possible values.
I'll try an alternative implementation.
(I'll be out of town for about 1 week starting from Thursday, so sorry for my possible belated response. Hopefully I can get a draft out before that.)
lld/ELF/Driver.cpp | ||
---|---|---|
2815 | Prefer "RISC-V" in comments. | |
lld/ELF/InputFiles.cpp | ||
598 | C++17 allows if (Optional<StringRef> Attr = ...) | |
lld/ELF/SyntheticSections.cpp | ||
3684 | I'd use parseRISCVAttributesSections. "parse" implies input (we don't parse output). | |
3695 | if (Optional<unsigned> value = ...) | |
3712 | arch | |
3805 | Delete the assert. The call site has an emptiness check and the next line accesses [0]. |
lld/ELF/InputFiles.cpp | ||
---|---|---|
597 | config->eflags hasn't been computed at this point, so everything in this body won't fire. |
The name is verbose and isn't like other lld code. Just sections is fine since we are in the context of combineRISCVAttributesSections