This is an archive of the discontinued LLVM Phabricator instance.

[ELF][RISCV] Merge `.riscv.attributes` sections
AbandonedPublic

Authored by eklepilkina on Oct 20 2022, 5:50 AM.

Details

Summary

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.

Diff Detail

Event Timeline

eklepilkina created this revision.Oct 20 2022, 5:50 AM
Herald added a project: Restricted Project. · View Herald Transcript
eklepilkina requested review of this revision.Oct 20 2022, 5:50 AM
eklepilkina edited the summary of this revision. (Show Details)Oct 20 2022, 5:53 AM
kito-cheng added inline comments.Oct 31 2022, 6:36 AM
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

  • Fix merge rules for attributes based on documentation
eklepilkina added inline comments.Oct 31 2022, 8:13 AM
lld/ELF/SyntheticSections.cpp
3758

Yes, it's true I can remove completely.

3781

Thank you for link to documentation with merge rules. Fix merging attributes with these rules.

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.

MaskRay requested changes to this revision.Nov 5 2022, 1:35 PM
This revision now requires changes to proceed.Nov 5 2022, 1:35 PM
eklepilkina edited the summary of this revision. (Show Details)Nov 6 2022, 11:34 PM
eklepilkina edited the summary of this revision. (Show Details)
eklepilkina edited the summary of this revision. (Show Details)
eklepilkina marked 7 inline comments as done.
  • Review fixes

@MaskRay thank you for review. I fixed all comments, could you have a look again?

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
605

no braces around single line simple statements.

607

avoid the used-once variable

609

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

1309

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.

eklepilkina marked 11 inline comments as done.
  • Review fixes
  • Added test for lto case
eklepilkina added inline comments.Nov 21 2022, 5:46 AM
lld/ELF/Driver.cpp
2821

Replaced to copy_if, this comment was connected with creating container with all riscv attributes sections.

eklepilkina retitled this revision from [ELF][RISCV] Merge `riscv.attributes` sections from all input files to [ELF][RISCV] Merge `.riscv.attributes` sections.Nov 21 2022, 5:47 AM

Some variable names are overly longer and are unlike the rest of lld/ELF.

lld/ELF/Arch/RISCV.cpp
824 ↗(On Diff #476867)

The name is verbose and isn't like other lld code. Just sections is fine since we are in the context of combineRISCVAttributesSections

826 ↗(On Diff #476867)

This with erase_if can be simplified as: llvm::erase_if(ctx.inputSections, [&](...) { if (!riscvattribute) return false; sections.push_back(...); return true; })

828 ↗(On Diff #476867)

early return is more common

834 ↗(On Diff #476867)

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) + "...")

eklepilkina marked 8 inline comments as done.
  • Review fixes
eklepilkina marked an inline comment as done.Nov 22 2022, 12:00 AM
MaskRay added a comment.EditedNov 22 2022, 7:47 PM

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
597

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

MaskRay added inline comments.Nov 22 2022, 8:32 PM
lld/ELF/InputFiles.cpp
596

config->eflags hasn't been computed at this point, so everything in this body won't fire.
I am rewriting this a la armExidx.

eklepilkina marked 7 inline comments as done.
  • Review fixes

Alternative with more tests: D138550

eklepilkina abandoned this revision.Jan 19 2023, 5:17 AM

Abandoned in favor of D138550