Skip to content

[lld][ELF] Allow data.rel.ro.hot and .data.rel.ro.unlikely to be RELRO #148920

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Jul 23, 2025
Merged

[lld][ELF] Allow data.rel.ro.hot and .data.rel.ro.unlikely to be RELRO #148920

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
00b0ddb
allow data.rel.ro.unlikely as an relro section
mingmingl-llvm Jul 15, 2025
9cdf86d
regard both .hot and .unlikely as section suffix if lld option keep-d…
mingmingl-llvm Jul 15, 2025
27778e5
resolve comments
mingmingl-llvm Jul 16, 2025
eb439b1
omit ByName as suggested
mingmingl-llvm Jul 17, 2025
3b23fde
apply suggestions
mingmingl-llvm Jul 18, 2025
0f42025
resolve comments
mingmingl-llvm Jul 21, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions lld/ELF/Config.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -405,6 +405,7 @@ struct Config {
bool zInitfirst;
bool zInterpose;
bool zKeepTextSectionPrefix;
bool zKeepDataSectionPrefix;
bool zLrodataAfterBss;
bool zNoBtCfi;
bool zNodefaultlib;
Expand Down
2 changes: 2 additions & 0 deletions lld/ELF/Driver.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1627,6 +1627,8 @@ static void readConfigs(Ctx &ctx, opt::InputArgList &args) {
ctx.arg.zInterpose = hasZOption(args, "interpose");
ctx.arg.zKeepTextSectionPrefix = getZFlag(
args, "keep-text-section-prefix", "nokeep-text-section-prefix", false);
ctx.arg.zKeepDataSectionPrefix = getZFlag(
args, "keep-data-section-prefix", "nokeep-data-section-prefix", false);
ctx.arg.zLrodataAfterBss =
getZFlag(args, "lrodata-after-bss", "nolrodata-after-bss", false);
ctx.arg.zNoBtCfi = hasZOption(args, "nobtcfi");
Expand Down
18 changes: 17 additions & 1 deletion lld/ELF/Writer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -553,6 +553,22 @@ template <class ELFT> void Writer<ELFT>::addSectionSymbols() {
}
}

// Returns true if the section is a data section that's read only and
// relocatable.
static bool isRelRoDataSection(Ctx &ctx, StringRef SectionName) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

secName

if (!secName.consume_front(".data.rel.ro")) return false; then check .hot or .unlikely

This is more efficient.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

// If -z keep-data-section-prefix is given, '<section>.hot' and
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

<section> in the comment seems misleading, since it only applies to .data.rel.ro. Just spell out .data.rel.ro.hot to make it easy for users to grep the code.

Copy link

@snehasish snehasish Jul 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the function name I expected this logic to use the sh_type and sh_flags to determine if it was relocatable and read only. Would that be more robust than relying on the name? If we choose to keep this logic based on the name, maybe note that in the function comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed this helper function to isRelRoDataSectionByName and added a comment.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of .data,.rodata,.data.rel.ro use SHT_PROGBITS. They don't have distinguishing flags, either.

I'd omit ByName from the function name. isDataRelRoSection to be consistent with ctx.in.bssRelRo

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated the function name.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the informative discussion.

// '<section>.unlikely' is considered a split of '<section>' based on hotness.
// They should share the same section attributes.
if (ctx.arg.zKeepDataSectionPrefix) {
if (SectionName.ends_with(".hot"))
SectionName = SectionName.drop_back(4);
else if (SectionName.ends_with(".unlikely"))
SectionName = SectionName.drop_back(9);
}

return SectionName == ".data.rel.ro";
}

// Today's loaders have a feature to make segments read-only after
// processing dynamic relocations to enhance security. PT_GNU_RELRO
// is defined for that.
Expand Down Expand Up @@ -629,7 +645,7 @@ static bool isRelroSection(Ctx &ctx, const OutputSection *sec) {
// magic section names.
StringRef s = sec->name;

bool abiAgnostic = s == ".data.rel.ro" || s == ".bss.rel.ro" ||
bool abiAgnostic = isRelRoDataSection(ctx, s) || s == ".bss.rel.ro" ||
s == ".ctors" || s == ".dtors" || s == ".jcr" ||
s == ".eh_frame" || s == ".fini_array" ||
s == ".init_array" || s == ".preinit_array";
Expand Down
50 changes: 50 additions & 0 deletions lld/test/ELF/relro-data-unlikely.s
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
# REQUIRES: x86
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

keep-text-section-prefix.s

(Make the filename more generic in case this feature applies to .data.unlikely in the future)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

# RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %s -o %t.o

# RUN: echo "SECTIONS { \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are quite a few extra files. We prefer split-file for such tests. Many tests use a.o b.o out as relocatable and executable file names. Consider -o a.o ld.lld -z keep-data-section-prefix -T a.lds a.o -o out

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

# RUN: .data.rel.ro : { .data.rel.ro } \
# RUN: .data.rel.ro.unlikely : { *(.data.rel.ro.unlikely) } \
# RUN: } INSERT AFTER .text " > %t.script

# RUN: ld.lld -z keep-data-section-prefix --script=%t.script %t.o -o %t
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using a linker script (most people specify -T instead of the long form --script) changes input-section-to-output-section mapping behavior, e.g. no default .text.* => `.text.

We need another ld.lld invocation without a linker script.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done by using -T linker-script.lds and adding three more invocation lines (to cover 4 combinations of {keep-data-section-prefix on/off, linker-script or not}

# RUN: llvm-readelf -l %t | FileCheck --check-prefix=SEG %s
# RUN: llvm-readelf -S %t | FileCheck %s

# There are 2 RW PT_LOAD segments. p_offset p_vaddr p_paddr p_filesz of the first
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We use ## for non-RUN-non-CHECK comments to make them stand out.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done. I'll keep this in mind for future changes.

# should match PT_GNU_RELRO.
# The .data.rel.ro.unlikely section is in PT_GNU_RELRO segment.

# Type Offset VirtAddr PhysAddr FileSiz MemSiz Flg Align
# SEG: LOAD 0x0001c8 0x00000000002011c8 0x00000000002011c8 0x000001 0x000001 R E 0x1000
# SEG-NEXT: LOAD 0x0001c9 0x00000000002021c9 0x00000000002021c9 0x001001 0x001e37 RW 0x1000
# SEG-NEXT: LOAD 0x0011ca 0x00000000002041ca 0x00000000002041ca 0x000001 0x000002 RW 0x1000
# SEG-NEXT: GNU_RELRO 0x0001c9 0x00000000002021c9 0x00000000002021c9 0x001001 0x001e37 R 0x1
# SEG-NEXT: GNU_STACK 0x000000 0x0000000000000000 0x0000000000000000 0x000000 0x000000 RW 0x0

# SEG: .text
# SEG-NEXT: .data.rel.ro .data.rel.ro.unlikely .relro_padding
# SEG-NEXT: .data .bss

# [Nr] Name Type Address Off Size
# CHECK: .data.rel.ro PROGBITS 00000000002021c9 0001c9 000001
# CHECK-NEXT: .data.rel.ro.unlikely PROGBITS 00000000002021ca 0001ca 001000
# CHECK-NEXT: .relro_padding NOBITS 00000000002031ca 0011ca 000e36
# CHECK-NEXT: .data PROGBITS 00000000002041ca 0011ca 000001
# CHECK-NEXT: .bss NOBITS 00000000002041cb 0011cb 000001

.globl _start
_start:
ret


.section .data.rel.ro, "aw"
.space 1

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add .data.rel.ro.hot ``.data.rel.ro.split` to demonstrate the behavior

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

.section .data.rel.ro.unlikely, "aw"
.space 4096

.section .data, "aw"
.space 1

.section .bss, "aw"
.space 1