Page MenuHomePhabricator

[lld/elf] support quote usage in section names
AcceptedPublic

Authored by royger on Apr 22 2022, 8:04 AM.

Details

Reviewers
ruiu
MaskRay
Summary

Section names used in ELF linker scripts can be quoted, but such
quotes must not be propagated to the binary ELF section names. As
such strip the quotes from the section names when processing them, and
also strip them from linker script functions that take section names
as parameters.

Diff Detail

Event Timeline

royger created this revision.Apr 22 2022, 8:04 AM
Herald added a project: Restricted Project. · View Herald Transcript
royger requested review of this revision.Apr 22 2022, 8:04 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 22 2022, 8:04 AM
MaskRay added inline comments.Apr 22 2022, 8:19 PM
lld/test/ELF/linkerscript/section-quotes.test
5

Use split-file to provide the linker script

20

Just keep ALIGNOF for .text and keep LOADADDR for .data? That provides sufficient coverage without being too verbose.

24

Unused. Delete

30

Unused. Delete

36

Unused. Delete

Thanks for the review, let me know if you agree with the reply. Will look into using split-file if that's desirable.

lld/test/ELF/linkerscript/section-quotes.test
20

The point is to test all possible combinations of quoted section name definition and usage of quotes section names in functions. Hence the tree combinations for .text .data and .bss:

  • text: quoted section name used for both definition and as function parameter
  • data: section definition not quoted, section parameter in functions quoted.
  • bss: section definition quoted, section parameter in functions not quoted.
24

It's unused, but the point (here and in usages below) is to test that the SIZEOF function can correctly deal with quoted and unquoted section names.

MaskRay added inline comments.Apr 23 2022, 10:23 AM
lld/test/ELF/linkerscript/section-quotes.test
20

One LONG (ALIGNOF("...")) and one LONG (LOADADDR(".text")) are sufficient. No need to test LONG (ALIGNOF("...")) in two output section descriptions.

24

OK, keeping text_size and data_text sounds good. But change llvm-readelf -S %t to llvm-readelf -S -s %t and additionally test the values of the two symbols.

royger updated this revision to Diff 424843.Apr 25 2022, 1:35 AM
  • Use split-file.
  • Remove .bss test case.
  • Test presence of {text,data}_size in the output.
  • Add comment about purpose of tests.
MaskRay accepted this revision.Apr 25 2022, 1:23 PM

[lld/elf] fix quote usage in section names

"Support " is more appropriate. We don't claim this was supported so we cannot consider this as a bug.

This revision is now accepted and ready to land.Apr 25 2022, 1:23 PM

[lld/elf] fix quote usage in section names

"Support " is more appropriate. We don't claim this was supported so we cannot consider this as a bug.

OK, I've used fix because GNU ld does support quoted section names, and lld claims to be compatible.

Can this be fixed at commit please? Are there any other steps I should do to get this committed?

Thanks

[lld/elf] fix quote usage in section names

"Support " is more appropriate. We don't claim this was supported so we cannot consider this as a bug.

OK, I've used fix because GNU ld does support quoted section names, and lld claims to be compatible.

Can this be fixed at commit please? Are there any other steps I should do to get this committed?

Thanks

You may change the subject of the differential.

There are number of comments for tests which haven't been marked as done.

royger retitled this revision from [lld/elf] fix quote usage in section names to [lld/elf] support quote usage in section names.EditedApr 27 2022, 1:19 AM
royger marked 7 inline comments as done.

Since you approved the change I take you are happy with the code as-is, and marked all the comments as done. Thanks.