[PATCH] D136344: [ELF][RISCV] Merge `riscv.attributes` sections from all input files
Fangrui Song via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Nov 17 19:14:17 PST 2022
MaskRay added a comment.
Sorry for the delay. I think this still requires some changes.
================
Comment at: lld/ELF/Driver.cpp:2821
+
+ // Collect all `.riscv.attributes` sections.
+ SmallVector<InputSectionBase *, 0> attributesSections;
----------------
"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
================
Comment at: lld/ELF/Driver.cpp:2826
+ if (!attributesSections.empty()) {
+ // Need to merge different attributes.
+ InputSectionBase *resultAttributesSection =
----------------
This comment does not say beyond what the code explains. Remove it.
================
Comment at: lld/ELF/Driver.cpp:2832
+ // Replace all existing attributes sections with merged one.
+ assert(
+ !ctx.inputSections.empty() &&
----------------
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).
================
Comment at: lld/ELF/InputFiles.cpp:605
+ if (!parseResult) {
+ error(std::string("Invalid arch name: ") + arch.str());
+ }
----------------
no braces around single line simple statements.
================
Comment at: lld/ELF/InputFiles.cpp:607
+ }
+ auto &isaInfo = *parseResult;
+ if (!isaInfo->hasExtension("c")) {
----------------
avoid the used-once variable
================
Comment at: lld/ELF/InputFiles.cpp:609
+ if (!isaInfo->hasExtension("c")) {
+ error(toString(this) +
+ " has set set RISCV_RVC flag, but RISCV_ARCH tag from " +
----------------
errorOrWarn so that users can ignore the error with --noinhibit-exec.
================
Comment at: lld/ELF/SyntheticSections.cpp:3684
+// Parse `.riscv.attributes` sections from input files.
+static void parseInputRISCVAttributesSections(
----------------
Delete comments when the function name is self-explanatory.
There are many places where a redundant comment is added. Please drop.
================
Comment at: lld/ELF/SyntheticSections.cpp:3705
+ error(toString(s) + ": " + llvm::toString(std::move(e)));
+ }
+ for (const auto ¤tTag : attributesTags) {
----------------
MaskRay wrote:
> drop braces
not done
================
Comment at: lld/ELF/SyntheticSections.h:1197
+ llvm::DenseMap<unsigned, std::string> mergedStringAttributes;
+ // Current vendor.
+ llvm::StringRef currentVendor = "riscv";
----------------
The comment is not useful.
================
Comment at: lld/ELF/SyntheticSections.h:1198
+ // Current vendor.
+ llvm::StringRef currentVendor = "riscv";
+
----------------
This variable can be a const. `constexpr`
================
Comment at: lld/ELF/SyntheticSections.h:1308
std::unique_ptr<InputSection> attributes;
+ std::unique_ptr<RISCVMergedAttributeSection> riscvAttributes;
std::unique_ptr<BssSection> bss;
----------------
This must be reset in `reset()`
================
Comment at: lld/test/ELF/riscv-merge-attributes.s:40
.attribute 5, "rv32i2p0_m2p0_a2p0_f2p0_d2p0"
+
----------------
no trailing blank line
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D136344/new/
https://reviews.llvm.org/D136344
More information about the llvm-commits
mailing list