-
Notifications
You must be signed in to change notification settings - Fork 14.9k
[lld][ELF] Introduce an option to keep data section prefix. #148985
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.
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
base: main
Are you sure you want to change the base?
[lld][ELF] Introduce an option to keep data section prefix. #148985
Conversation
6a35d1b
to
25c6149
Compare
@llvm/pr-subscribers-lld @llvm/pr-subscribers-lld-elf Author: Mingming Liu (mingmingl-llvm) Changeshttps://discourse.llvm.org/t/rfc-profile-guided-static-data-partitioning/83744 proposes to partition a static data section (like This change implements section grouping for {
Full diff: https://github.com/llvm/llvm-project/pull/148985.diff 5 Files Affected:
diff --git a/lld/ELF/Config.h b/lld/ELF/Config.h
index 03b3cd4771f49..d3f6103fde4b7 100644
--- a/lld/ELF/Config.h
+++ b/lld/ELF/Config.h
@@ -371,6 +371,7 @@ struct Config {
bool zIfuncNoplt;
bool zInitfirst;
bool zInterpose;
+ bool zKeepDataSectionPrefix;
bool zKeepTextSectionPrefix;
bool zLrodataAfterBss;
bool zNoBtCfi;
diff --git a/lld/ELF/Driver.cpp b/lld/ELF/Driver.cpp
index 0a220432333cc..b1b55247388a3 100644
--- a/lld/ELF/Driver.cpp
+++ b/lld/ELF/Driver.cpp
@@ -1564,6 +1564,8 @@ static void readConfigs(Ctx &ctx, opt::InputArgList &args) {
ctx.arg.zIfuncNoplt = hasZOption(args, "ifunc-noplt");
ctx.arg.zInitfirst = hasZOption(args, "initfirst");
ctx.arg.zInterpose = hasZOption(args, "interpose");
+ ctx.arg.zKeepDataSectionPrefix = getZFlag(
+ args, "keep-data-section-prefix", "nokeep-data-section-prefix", false);
ctx.arg.zKeepTextSectionPrefix = getZFlag(
args, "keep-text-section-prefix", "nokeep-text-section-prefix", false);
ctx.arg.zLrodataAfterBss =
diff --git a/lld/ELF/LinkerScript.cpp b/lld/ELF/LinkerScript.cpp
index e19823f2ea752..f2b20db7b0521 100644
--- a/lld/ELF/LinkerScript.cpp
+++ b/lld/ELF/LinkerScript.cpp
@@ -48,6 +48,11 @@ static bool isSectionPrefix(StringRef prefix, StringRef name) {
return name.consume_front(prefix) && (name.empty() || name[0] == '.');
}
+static bool isSectionSuffix(StringRef suffix, StringRef name) {
+ name.consume_back(".");
+ return name.ends_with(suffix);
+}
+
StringRef LinkerScript::getOutputSectionName(const InputSectionBase *s) const {
// This is for --emit-relocs and -r. If .text.foo is emitted as .text.bar, we
// want to emit .rela.text.foo as .rela.text.bar for consistency (this is not
@@ -105,13 +110,41 @@ StringRef LinkerScript::getOutputSectionName(const InputSectionBase *s) const {
return ".text";
}
- for (StringRef v : {".data.rel.ro", ".data", ".rodata",
- ".bss.rel.ro", ".bss", ".ldata",
- ".lrodata", ".lbss", ".gcc_except_table",
- ".init_array", ".fini_array", ".tbss",
- ".tdata", ".ARM.exidx", ".ARM.extab",
- ".ctors", ".dtors", ".sbss",
- ".sdata", ".srodata"})
+ // When zKeepDataSectionPrefix is true, keep .hot and .unlikely suffixes
+ // in data sections.
+ static constexpr StringRef dataSectionPrefixes[] = {
+ ".data.rel.ro", ".data", ".rodata", ".bss.rel.ro", ".bss",
+ };
+
+ for (auto [index, v] : llvm::enumerate(dataSectionPrefixes)) {
+ if (isSectionPrefix(v, s->name)) {
+ // The .bss.rel.ro section is considered rarely accessed. So this section
+ // is not partitioned and zKeepDataSectionPrefix is not applied to
+ // make the executable simpler with fewer elf sections.
+ if (ctx.arg.zKeepDataSectionPrefix && index != 3) {
+ if (isSectionPrefix(".hot", s->name.substr(v.size())))
+ return s->name.substr(0, v.size() + 4);
+ if (isSectionPrefix(".unlikely", s->name.substr(v.size())))
+ return s->name.substr(0, v.size() + 9);
+ // For .rodata, a section could be`.rodata.cst<N>.hot.` for constant
+ // pool or `rodata.str<N>.hot.` for string literals.
+ if (index == 2) {
+ if (isSectionSuffix(".hot", s->name)) {
+ return ".rodata.hot";
+ }
+ if (isSectionSuffix(".unlikely", s->name)) {
+ return ".rodata.unlikely";
+ }
+ }
+ }
+ return v;
+ }
+ }
+
+ for (StringRef v :
+ {".ldata", ".lrodata", ".lbss", ".gcc_except_table", ".init_array",
+ ".fini_array", ".tbss", ".tdata", ".ARM.exidx", ".ARM.extab", ".ctors",
+ ".dtors", ".sbss", ".sdata", ".srodata"})
if (isSectionPrefix(v, s->name))
return v;
diff --git a/lld/ELF/Writer.cpp b/lld/ELF/Writer.cpp
index 2cea6a44b391a..82e5ba6d9e6a8 100644
--- a/lld/ELF/Writer.cpp
+++ b/lld/ELF/Writer.cpp
@@ -555,6 +555,27 @@ template <class ELFT> void Writer<ELFT>::addSectionSymbols() {
}
}
+// Returns true if the section is a data section that's read only and
+// relocatable per its section name.
+static bool isRelRoDataSection(Ctx &ctx, StringRef secName) {
+ // The section name should start with ".data.rel.ro".
+ if (!secName.consume_front(".data.rel.ro"))
+ return false;
+
+ // If the section name is .data.rel.ro, it is a relocatable read-only data
+ // section.
+ if (secName.empty())
+ return true;
+ // If -z keep-data-section-prefix is given, '.data.rel.ro.hot' and
+ // '.data.rel.ro.unlikely' are considered a split of '.data.rel.ro' based on
+ // hotness.
+ if (ctx.arg.zKeepDataSectionPrefix) {
+ return secName == ".hot" || secName == ".unlikely";
+ }
+
+ return false;
+}
+
// 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.
@@ -631,7 +652,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";
diff --git a/lld/test/ELF/data-section-prefix.s b/lld/test/ELF/data-section-prefix.s
new file mode 100644
index 0000000000000..5012039e438a7
--- /dev/null
+++ b/lld/test/ELF/data-section-prefix.s
@@ -0,0 +1,127 @@
+# REQUIRES: x86
+## -z keep-data-section-prefix separates static data sections with prefix
+## .<section>.hot, .<section>.unlikely in the absence of a SECTIONS command.
+
+# RUN: rm -rf %t && split-file %s %t && cd %t
+
+# RUN: llvm-mc -filetype=obj -triple=x86_64 a.s -o a.o
+
+# RUN: ld.lld a.o -o out1
+# RUN: llvm-readelf -S out1 | FileCheck --check-prefix=BASIC %s
+# RUN: ld.lld -z nokeep-text-section-prefix a.o -o out2
+# RUN: cmp out1 out2
+
+# RUN: ld.lld -z keep-data-section-prefix a.o -o out3
+# RUN: llvm-readelf -S out3 | FileCheck --check-prefix=KEEP %s
+
+## With a SECTIONS command, orphan sections are created verbatim.
+## No grouping is performed for them.
+# RUN: ld.lld -T x.lds -z keep-data-section-prefix a.o -o out4
+# RUN: llvm-readelf -S out4 | FileCheck --check-prefix=SCRIPT %s
+
+# BASIC: [Nr] Name Type Address Off Size
+# BASIC: [ 1] .text
+# BASIC-NEXT: [ 2] .data.rel.ro PROGBITS 00000000002021c9 0001c9 00000f
+# BASIC-NEXT: [ 3] .bss.rel.ro NOBITS 00000000002021d8 0001d8 000008
+# BASIC-NEXT: [ 4] .relro_padding NOBITS 00000000002021e0 0001d8 000e20
+# BASIC-NEXT: [ 5] .rodata PROGBITS 00000000002031e0 0001e0 000006
+# BASIC-NEXT: [ 6] .data PROGBITS 00000000002031e6 0001e6 000004
+# BASIC-NEXT: [ 7] .bss NOBITS 00000000002031ea 0001ea 000004
+
+# KEEP: [Nr] Name Type Address Off Size
+# KEEP: [ 1] .text
+# KEEP-NEXT: [ 2] .data.rel.ro PROGBITS 00000000002021c9 0001c9 000009
+# KEEP-NEXT: [ 3] .data.rel.ro.hot PROGBITS 00000000002021d2 0001d2 000004
+# KEEP-NEXT: [ 4] .data.rel.ro.unlikely PROGBITS 00000000002021d6 0001d6 000002
+# KEEP-NEXT: [ 5] .bss.rel.ro NOBITS 00000000002021d8 0001d8 000008
+# KEEP-NEXT: [ 6] .relro_padding NOBITS 00000000002021e0 0001d8 000e20
+# KEEP-NEXT: [ 7] .rodata PROGBITS 00000000002031e0 0001e0 000002
+# KEEP-NEXT: [ 8] .rodata.hot PROGBITS 00000000002031e2 0001e2 000002
+# KEEP-NEXT: [ 9] .rodata.unlikely PROGBITS 00000000002031e4 0001e4 000002
+# KEEP-NEXT: [10] .data PROGBITS 00000000002031e6 0001e6 000002
+# KEEP-NEXT: [11] .data.hot PROGBITS 00000000002031e8 0001e8 000001
+# KEEP-NEXT: [12] .data.unlikely PROGBITS 00000000002031e9 0001e9 000001
+# KEEP-NEXT: [13] .bss NOBITS 00000000002031ea 0001ea 000002
+# KEEP-NEXT: [14] .bss.hot NOBITS 00000000002031ec 0001ea 000001
+# KEEP-NEXT: [15] .bss.unlikely NOBITS 00000000002031ed 0001ea 000001
+
+# SCRIPT: .text
+# SCRIPT-NEXT: .rodata.i
+# SCRIPT-NEXT: .rodata.hot.j
+# SCRIPT-NEXT: .rodata.unlikely.k
+# SCRIPT-NEXT: .rodata.split.l
+# SCRIPT-NEXT: .rodata.cst32.hot.
+# SCRIPT-NEXT: .rodata.str1.1.unlikely.
+# SCRIPT-NEXT: .data.m
+# SCRIPT-NEXT: .data.hot.n
+# SCRIPT-NEXT: .data.unlikely.o
+# SCRIPT-NEXT: .data.split.p
+# SCRIPT-NEXT: .data.rel.ro.q
+# SCRIPT-NEXT: .data.rel.ro.hot.r
+# SCRIPT-NEXT: .data.rel.ro.unlikely.s
+# SCRIPT-NEXT: .data.rel.ro.split.t
+# SCRIPT-NEXT: .bss.a
+# SCRIPT-NEXT: .bss.hot.b
+# SCRIPT-NEXT: .bss.unlikely.c
+# SCRIPT-NEXT: .bss.split.d
+# SCRIPT-NEXT: .bss.rel.ro.e
+# SCRIPT-NEXT: .bss.rel.ro.hot.f
+# SCRIPT-NEXT: .bss.rel.ro.unlikely.g
+# SCRIPT-NEXT: .bss.rel.ro.split.h
+
+#--- a.s
+.globl _start
+_start:
+ ret
+
+.section .bss.a,"aw"
+ .byte 0
+.section .bss.hot.b,"aw"
+ .byte 0
+.section .bss.unlikely.c,"aw"
+ .byte 0
+.section .bss.split.d,"aw"
+ .byte 0
+
+.section .bss.rel.ro.e, "aw"
+ .space 2
+.section .bss.rel.ro.hot.f, "aw"
+ .space 2
+.section .bss.rel.ro.unlikely.g, "aw"
+ .space 2
+.section .bss.rel.ro.split.h, "aw"
+ .space 2
+
+.section .rodata.i,"aw"
+ .byte 1
+.section .rodata.hot.j,"aw"
+ .byte 2
+.section .rodata.unlikely.k,"aw"
+ .byte 3
+.section .rodata.split.l,"aw"
+ .byte 4
+.section .rodata.cst32.hot.,"aw"
+ .byte 5
+.section .rodata.str1.1.unlikely.,"aw"
+ .byte 6
+
+.section .data.m,"aw"
+ .byte 5
+.section .data.hot.n,"aw"
+ .byte 6
+.section .data.unlikely.o,"aw"
+ .byte 7
+.section .data.split.p,"aw"
+ .byte 8
+
+.section .data.rel.ro.q,"aw"
+ .quad 0
+.section .data.rel.ro.hot.r,"aw"
+ .long 255
+.section .data.rel.ro.unlikely.s,"aw"
+ .word 1
+.section .data.rel.ro.split.t,"aw"
+ .byte 0
+
+#--- x.lds
+SECTIONS {}
|
lld/ELF/LinkerScript.cpp
Outdated
|
||
for (auto [index, v] : llvm::enumerate(dataSectionPrefixes)) { | ||
if (isSectionPrefix(v, s->name)) { | ||
// The .bss.rel.ro section is considered rarely accessed. So this section |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make if (isSectionPrefix(v, s->name)) {
early return.
.bss.rel.ro is currently only used by LLD's copy relocation feature, not generated by compilers. It's unnecessary to custom stuff with index != 3
. Just remove the condition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make if (isSectionPrefix(v, s->name)) { early return.
Just to confirm, is the early return primarily aimed at optimizing the path where zKeepDataSectionPrefix is false? I'll go ahead and implement it that way.
.bss.rel.ro is currently only used by LLD's copy relocation feature, not generated by compilers. It's unnecessary to custom stuff with index != 3. Just remove the condition.
Thanks for the information. Removed 'index = 3' stuff, and updated lld/test/ELF/data-section-prefix.s accordingly to have .bss.rel.ro without any prefix.
lld/ELF/LinkerScript.cpp
Outdated
// is not partitioned and zKeepDataSectionPrefix is not applied to | ||
// make the executable simpler with fewer elf sections. | ||
if (ctx.arg.zKeepDataSectionPrefix && index != 3) { | ||
if (isSectionPrefix(".hot", s->name.substr(v.size()))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If StringRef name = s->name
at the loop body beginning and use consume_front
, we can remove substr(v.size())
here and below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incorporate this by adding a helper function trimSectionPrefix
. Since this function is only called once, I'm open to remove trimSectionPrefix
and inline its content at the callsite.
lld/ELF/LinkerScript.cpp
Outdated
// pool or `rodata.str<N>.hot.` for string literals. | ||
if (index == 2) { | ||
if (isSectionSuffix(".hot", s->name)) { | ||
return ".rodata.hot"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
delete braces for single-line simple body. Is it necessary to make the feature specific to .rodata
? While I understand that it probably has not been extended to .data
, we are adding an option with a quite generic name, can we include these other sections as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
delete braces for single-line simple body.
done
Is it necessary to make the feature specific to
.rodata
? While I understand that it probably has not been extended to.data
, we are adding an option with a quite generic name, can we include these other sections as well?
This is a good question. The reason for if (index == 2) {
(the whole if-block in [1]) is to have simpler code by spelling out .rodata.hot
string literal when we cannot return a substring as a StringRef. I added a code comment for this.
To elaborate
- For majority cases, the section name has one of the three prefixes (
.<section>.hot
,.<section>.unlikely
or.<section>
). In those cases, the returned StringRef is a substring ofs->name
. This works well, without the need to separate if-else branches for different data sections. - The
isSectionPrefix
pattern above cannot be easily extended to return StringRef as a substring of.str<N>
or.cst<N>
come in between, and specifically we cannot getStringRef
from .rodata.str.hot.` .
[1]
if (index == 2) {
if (s->name.ends_with(".hot."))
return ".rodata.hot";
if (s->name.ends_with(".unlikely."))
return ".rodata.unlikely";
}
}
lld/ELF/LinkerScript.cpp
Outdated
// For .rodata, a section could be`.rodata.cst<N>.hot.` for constant | ||
// pool or `rodata.str<N>.hot.` for string literals. | ||
if (index == 2) { | ||
if (isSectionSuffix(".hot", s->name)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isSectionSuffix
supports both .rodata.hot
and .rodata.hot.
. I don't think we want to support .rodata.hot
(which can be used by a function named hot
). Supporting just .rodata.hot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't think we want to support .rodata.hot (which can be used by a function named hot). Supporting just .rodata.hot.
sure. I think it's sensible to support only rodata.hot.
(with trailing dot) to disambiguate functions and data.
lld/test/ELF/data-section-prefix.s
Outdated
|
||
.section .rodata.i,"aw" | ||
.byte 1 | ||
.section .rodata.hot.j,"aw" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can test .rodata.hot.
(without j
), for -fno-unique-section-names
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
lld/test/ELF/data-section-prefix.s
Outdated
@@ -0,0 +1,127 @@ | |||
# REQUIRES: x86 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From a patch organization perspective, it would be clearer to first introduce the option in a patch and then add RELRO semantics in a follow-up patch (#148920). While it's unusual that 148920 was merged first, the changes remain on the right track... I would rename this to keep-data-section-prefix.s, not introducing a separate test file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From a patch organization perspective, it would be clearer to first introduce the option in a patch and then add RELRO semantics in a follow-up patch (#148920). While it's unusual that 148920 was merged first, the changes remain on the right track...
I appreciate you bringing up the merge order and agree with it. I've opted to merge #148920 first because linker script is an alternative option to this patch, and #148920 is on the common path of either linker script or section grouping.
I'll also follow up on the reviews to get this one ready asap.
I would rename this to keep-data-section-prefix.s, not introducing a separate test file.
Done.
lld/test/ELF/data-section-prefix.s
Outdated
|
||
# BASIC: [Nr] Name Type Address Off Size | ||
# BASIC: [ 1] .text | ||
# BASIC-NEXT: [ 2] .data.rel.ro PROGBITS 00000000002021c9 0001c9 00000f |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The addresses are not useful. They could cause complexity when lld's address assignment changes. Suggest:
# BASIC-NEXT: [ 2] .data.rel.ro PROGBITS
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The addresses are not useful. They could cause complexity when lld's address assignment changes
Got it. I replaced Address
and Off
columns with {{.*}} and kept Size
column.
cc @yozhu for reviewing too as he is working on ordering the ELF data sections based on memprof profiles too. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for reviews! PTAL.
lld/ELF/LinkerScript.cpp
Outdated
|
||
for (auto [index, v] : llvm::enumerate(dataSectionPrefixes)) { | ||
if (isSectionPrefix(v, s->name)) { | ||
// The .bss.rel.ro section is considered rarely accessed. So this section |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make if (isSectionPrefix(v, s->name)) { early return.
Just to confirm, is the early return primarily aimed at optimizing the path where zKeepDataSectionPrefix is false? I'll go ahead and implement it that way.
.bss.rel.ro is currently only used by LLD's copy relocation feature, not generated by compilers. It's unnecessary to custom stuff with index != 3. Just remove the condition.
Thanks for the information. Removed 'index = 3' stuff, and updated lld/test/ELF/data-section-prefix.s accordingly to have .bss.rel.ro without any prefix.
lld/ELF/LinkerScript.cpp
Outdated
// For .rodata, a section could be`.rodata.cst<N>.hot.` for constant | ||
// pool or `rodata.str<N>.hot.` for string literals. | ||
if (index == 2) { | ||
if (isSectionSuffix(".hot", s->name)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't think we want to support .rodata.hot (which can be used by a function named hot). Supporting just .rodata.hot.
sure. I think it's sensible to support only rodata.hot.
(with trailing dot) to disambiguate functions and data.
lld/ELF/LinkerScript.cpp
Outdated
// pool or `rodata.str<N>.hot.` for string literals. | ||
if (index == 2) { | ||
if (isSectionSuffix(".hot", s->name)) { | ||
return ".rodata.hot"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
delete braces for single-line simple body.
done
Is it necessary to make the feature specific to
.rodata
? While I understand that it probably has not been extended to.data
, we are adding an option with a quite generic name, can we include these other sections as well?
This is a good question. The reason for if (index == 2) {
(the whole if-block in [1]) is to have simpler code by spelling out .rodata.hot
string literal when we cannot return a substring as a StringRef. I added a code comment for this.
To elaborate
- For majority cases, the section name has one of the three prefixes (
.<section>.hot
,.<section>.unlikely
or.<section>
). In those cases, the returned StringRef is a substring ofs->name
. This works well, without the need to separate if-else branches for different data sections. - The
isSectionPrefix
pattern above cannot be easily extended to return StringRef as a substring of.str<N>
or.cst<N>
come in between, and specifically we cannot getStringRef
from .rodata.str.hot.` .
[1]
if (index == 2) {
if (s->name.ends_with(".hot."))
return ".rodata.hot";
if (s->name.ends_with(".unlikely."))
return ".rodata.unlikely";
}
}
lld/test/ELF/data-section-prefix.s
Outdated
|
||
.section .rodata.i,"aw" | ||
.byte 1 | ||
.section .rodata.hot.j,"aw" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
lld/ELF/LinkerScript.cpp
Outdated
// is not partitioned and zKeepDataSectionPrefix is not applied to | ||
// make the executable simpler with fewer elf sections. | ||
if (ctx.arg.zKeepDataSectionPrefix && index != 3) { | ||
if (isSectionPrefix(".hot", s->name.substr(v.size()))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incorporate this by adding a helper function trimSectionPrefix
. Since this function is only called once, I'm open to remove trimSectionPrefix
and inline its content at the callsite.
lld/test/ELF/data-section-prefix.s
Outdated
@@ -0,0 +1,127 @@ | |||
# REQUIRES: x86 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From a patch organization perspective, it would be clearer to first introduce the option in a patch and then add RELRO semantics in a follow-up patch (#148920). While it's unusual that 148920 was merged first, the changes remain on the right track...
I appreciate you bringing up the merge order and agree with it. I've opted to merge #148920 first because linker script is an alternative option to this patch, and #148920 is on the common path of either linker script or section grouping.
I'll also follow up on the reviews to get this one ready asap.
I would rename this to keep-data-section-prefix.s, not introducing a separate test file.
Done.
lld/test/ELF/data-section-prefix.s
Outdated
|
||
# BASIC: [Nr] Name Type Address Off Size | ||
# BASIC: [ 1] .text | ||
# BASIC-NEXT: [ 2] .data.rel.ro PROGBITS 00000000002021c9 0001c9 00000f |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The addresses are not useful. They could cause complexity when lld's address assignment changes
Got it. I replaced Address
and Off
columns with {{.*}} and kept Size
column.
The merged change, approved by someone probably unfamiliar with LLD, was the exact case I mentioned to lenary the day before. I had to rename the incorrect file from
|
lld/ELF/LinkerScript.cpp
Outdated
|
||
for (auto [index, v] : llvm::enumerate(dataSectionPrefixes)) { | ||
StringRef secName = s->name; | ||
if (trimSectionPrefix(v, secName)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can use early return for trimSectionPrefix
. The helper is only called once. Inlining it is more readable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
inlined the helper function and used 'continue;' to reduce nested IFs inside this loop.
lld/ELF/LinkerScript.cpp
Outdated
// For .rodata, a section could be`.rodata.cst<N>.hot.` for constant | ||
// pool or `rodata.str<N>.hot.` for string literals. | ||
if (index == 2) { | ||
// The reason to specialize this path is to spell out .rodata.hot and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment is too verbose. I would specify:
// Place input .rodata.cst<N>.hot.
into .rodata.hot
.
and delete the previous comment
// For .rodata, a section could be
.rodata.cst<N>.hot.
for constant
// pool orrodata.str<N>.hot.
for string literals.
The code block has fewer than 20 lines, making it easy for users to understand its semantics without adding comments that redundantly explain the code.
Now I am unsure why we want to support .rodata.hot
while we don't support .rodata.cst4.hot
.
(The keep-text-data-sections
was added long ago when we supported both .text.hot
and .text.hot.
In clang, we now always emit .text.hot.
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment is too verbose ... making it easy for users to understand its semantics without adding comments that redundantly explain the code
done.
Now I am unsure why we want to support .rodata.hot while we don't support .rodata.cst4.hot.
If I understand correctly, the question is about why having [1] when .rodata.cst4.hot
doesn't end with a trailing dot.
Basically, [1] is meant to to group both string literals (with name pattern .rodata.str1.1.unlikely.
[2]) and constant pools (with name pattern .rodata.cst<N>.hot
[3]). As the test case examples show, string literal section names have a trailing dot but the constant pool section names don't.
Based on #148985 (comment), we need trailing dot to disambiguate between .rodata.<var-name>
and .rodata.<hotness>
when <var-name>
is hot
(something like https://gcc.godbolt.org/z/95f4oafcn). I prepared #150859 to add trailing dot for constant pools when there is .hot
or .unlikely
in the section names. Without PR 150859, only string literals will be grouped with [1].
[1]
if (index == 2) {
// Place input .rodata.str<N>.hot. or .rodata.cst<N>.hot. into the
// .rodata.hot section.
if (s->name.ends_with(".hot."))
return ".rodata.hot";
// Place input .rodata.str<N>.hot. or .rodata.cst<N>.unlikely. into the
// .rodata.unlikely section.
if (s->name.ends_with(".unlikely."))
return ".rodata.unlikely";
}
}
[2]
llvm-project/llvm/test/CodeGen/X86/global-variable-partition.ll
Lines 60 to 66 in 314e22b
; For @.str.2 | |
; COMMON: .type .L.str.2,@object | |
; SYM-NEXT: .section .rodata.str1.1.unlikely.,"aMS",@progbits,1 | |
; UNIQ-NEXT: .section .rodata.str1.1.unlikely.,"aMS",@progbits,1 | |
; AGG-NEXT: .section .rodata.str1.1.unlikely.,"aMS",@progbits,1 | |
; COMMON-NEXT: .L.str.2: | |
; COMMON-NEXT: "cold%d\t%d\t%d\n" |
[3]
llvm-project/llvm/test/CodeGen/X86/constant-pool-partition.ll
Lines 27 to 31 in 314e22b
; CHECK: .section .rodata.cst8.hot,"aM",@progbits,8 | |
; CHECK-NEXT: .p2align | |
; CHECK-NEXT: .LCPI0_0: | |
; CHECK-NEXT: .quad 0x3fe5c28f5c28f5c3 # double 0.68000000000000005 | |
; CHECK-NEXT: .section .rodata.cst8.unlikely,"aM",@progbits,8 |
|
||
.section .bss.rel.ro, "aw" | ||
.space 2 | ||
.section .bss.rel.ro, "aw" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are the 4 .bss.rel.ro
sections of the same name intended?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah the 4 .bss.rel.ro
sections used to have different names. Given the discussion in #148985 (comment), only one section without any prefix should be kept.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to rename the incorrect file from keep-text-section-prefix.s (when the option is actually keep-data-section-prefix), and now the tests require significant adjustments, which will clutter the commit history....
ah thanks for the renaming! Sorry for the mistaken filenames..
In the past, changes to areas I care about, like lld and MC, were sometimes approved and landed before their implications were fully thought through, requiring me to revise them later.
I can definitely understand that premature merges can be frustrating, leading to extra work for you. I'll make it a priority to get explicit sign-off especially when changes are requested.
lld/ELF/LinkerScript.cpp
Outdated
|
||
for (auto [index, v] : llvm::enumerate(dataSectionPrefixes)) { | ||
StringRef secName = s->name; | ||
if (trimSectionPrefix(v, secName)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
inlined the helper function and used 'continue;' to reduce nested IFs inside this loop.
|
||
.section .bss.rel.ro, "aw" | ||
.space 2 | ||
.section .bss.rel.ro, "aw" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah the 4 .bss.rel.ro
sections used to have different names. Given the discussion in #148985 (comment), only one section without any prefix should be kept.
lld/ELF/LinkerScript.cpp
Outdated
// For .rodata, a section could be`.rodata.cst<N>.hot.` for constant | ||
// pool or `rodata.str<N>.hot.` for string literals. | ||
if (index == 2) { | ||
// The reason to specialize this path is to spell out .rodata.hot and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment is too verbose ... making it easy for users to understand its semantics without adding comments that redundantly explain the code
done.
Now I am unsure why we want to support .rodata.hot while we don't support .rodata.cst4.hot.
If I understand correctly, the question is about why having [1] when .rodata.cst4.hot
doesn't end with a trailing dot.
Basically, [1] is meant to to group both string literals (with name pattern .rodata.str1.1.unlikely.
[2]) and constant pools (with name pattern .rodata.cst<N>.hot
[3]). As the test case examples show, string literal section names have a trailing dot but the constant pool section names don't.
Based on #148985 (comment), we need trailing dot to disambiguate between .rodata.<var-name>
and .rodata.<hotness>
when <var-name>
is hot
(something like https://gcc.godbolt.org/z/95f4oafcn). I prepared #150859 to add trailing dot for constant pools when there is .hot
or .unlikely
in the section names. Without PR 150859, only string literals will be grouped with [1].
[1]
if (index == 2) {
// Place input .rodata.str<N>.hot. or .rodata.cst<N>.hot. into the
// .rodata.hot section.
if (s->name.ends_with(".hot."))
return ".rodata.hot";
// Place input .rodata.str<N>.hot. or .rodata.cst<N>.unlikely. into the
// .rodata.unlikely section.
if (s->name.ends_with(".unlikely."))
return ".rodata.unlikely";
}
}
[2]
llvm-project/llvm/test/CodeGen/X86/global-variable-partition.ll
Lines 60 to 66 in 314e22b
; For @.str.2 | |
; COMMON: .type .L.str.2,@object | |
; SYM-NEXT: .section .rodata.str1.1.unlikely.,"aMS",@progbits,1 | |
; UNIQ-NEXT: .section .rodata.str1.1.unlikely.,"aMS",@progbits,1 | |
; AGG-NEXT: .section .rodata.str1.1.unlikely.,"aMS",@progbits,1 | |
; COMMON-NEXT: .L.str.2: | |
; COMMON-NEXT: "cold%d\t%d\t%d\n" |
[3]
llvm-project/llvm/test/CodeGen/X86/constant-pool-partition.ll
Lines 27 to 31 in 314e22b
; CHECK: .section .rodata.cst8.hot,"aM",@progbits,8 | |
; CHECK-NEXT: .p2align | |
; CHECK-NEXT: .LCPI0_0: | |
; CHECK-NEXT: .quad 0x3fe5c28f5c28f5c3 # double 0.68000000000000005 | |
; CHECK-NEXT: .section .rodata.cst8.unlikely,"aM",@progbits,8 |
PR llvm#148920 was merged before I could share my comments. * Fix the text filename. There are other minor suggestions, but can be done in llvm#148985 * Make `isRelRoDataSection` concise, to be consistent with the majority of helper functions.
… has a hotness prefix (#150859) Currently, `TargetLoweringObjectFileELF::getSectionForConstant` produce `.<section>.hot` or `.<section>.unlikely` for a constant with non-empty section prefix. This PR changes the implementation add trailing dot when section prefix is not empty, to disambiguate `.hot` as a hotness prefix from `.hot` as a (pure C) variable name. Relevant discussions are in #148985 (comment) and #148985 (comment) and
…ion when it has a hotness prefix (#150859) Currently, `TargetLoweringObjectFileELF::getSectionForConstant` produce `.<section>.hot` or `.<section>.unlikely` for a constant with non-empty section prefix. This PR changes the implementation add trailing dot when section prefix is not empty, to disambiguate `.hot` as a hotness prefix from `.hot` as a (pure C) variable name. Relevant discussions are in llvm/llvm-project#148985 (comment) and llvm/llvm-project#148985 (comment) and
lld/ELF/LinkerScript.cpp
Outdated
|
||
for (auto [index, v] : llvm::enumerate(dataSectionPrefixes)) { | ||
StringRef secName = s->name; | ||
if (!secName.consume_front(v)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the recent LLVM change that uses .data.rel.ro.hot.
(the .
after hot
is mandatory), can this be simplified?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The two ifs before if (!ctx.arg.zKeepDataSectionPrefix)
applies on the general cases, and there may not be a trailing dot to data sections. For instance, https://gcc.godbolt.org/z/G77P8n6qf compiles a simple source code with -O2 -ffunction-sections -fdata-sections
, and var section name is .section .bss.var
(without trailing dot).
lld/ELF/LinkerScript.cpp
Outdated
StringRef secName = s->name; | ||
if (!secName.consume_front(v)) | ||
continue; | ||
if (!secName.empty() && secName[0] != '.') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: drop braces for single-line simple body.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
lld/ELF/LinkerScript.cpp
Outdated
return s->name.substr(0, v.size() + 4); | ||
if (isSectionPrefix(".unlikely", secName)) | ||
return s->name.substr(0, v.size() + 9); | ||
if (index == 2) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need index == 2
? I think it's ok for linker to map input .bss.hot.
to .bss.hot
even if compilers don't generate .bss.hot.
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's ok for linker to map input .bss.hot. to .bss.hot even if compilers don't generate .bss.hot. .
yes, it's ok to map .bss.hot.
to .bss.hot
, in which case if (isSectionPrefix(".hot", secName))
at L127 will return .bss.hot
, and code at L131 (if index==2)
won't run.
As discussed in #148985 (comment) and #148985 (comment), patterns like .rodata.str1.1.unlikely.
or .rodata.cst4.hot.
are not mapped before if (index==2)
because of the strings between section name and hot/unlikely prefix, and if(index==2)
is added to map them. Meanwhile, we can use s->name.substr
to return a StringRef by referencing s->name
.
To get rid of if (index==2)
specialization, one implementation would be something like [1]. If this version is more readable, I can update the patch to it.
[1]
// records the hot prefixes in data array.
// Note `bss.rel.ro` doesn't have hot prefix.
static constexpr StringRef dataSectionHotPrefixes[] = {
".data.rel.ro.hot", ".data.hot", ".rodata.hot", ".bss.rel.ro", ".bss.hot",
};
// records the unlikely suffixes in data array
// Note `bss.rel.ro` doesn't have unlikely prefix.
static constexpr StringRef dataSectionUnlikelyPrefixes[] = {
".data.rel.ro.unlikely", ".data.unlikely", ".rodata.unlikely", ".bss.rel.ro", ".bss.unlikely",
};
for (auto [index, v] : llvm::enumerate(dataSectionPrefixes)) {
... // code before `ctx.arg.zKeepDataSectionPrefix`
if (!ctx.arg.zKeepDataSectionPrefix)
return v;
// We require the trailing dot after hot or unlikely to exist to map them
// to hot or unlikely variant.
if (s->name.ends_with(".hot.")
return dataSectionHotPrefixes[index];
if (s->name.ends_with(".unlikely.")
return dataSectionUnlikelyPrefixes[index];
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the explanation! I think the current index == 2
looks better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps
if (ctx.arg.zKeepDataSectionPrefix) {
// .rodata*.hot. => .rodata.hot
...
for (auto v : dataSectionPrefixes) {
... [1]
}
}
for (auto v : dataSectionPrefixes) {
... [2] slightly duplicates [1]
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I lifted the keep-data-section-prefix
path as suggested, and realized that [2] (the for (auto v : dataSectionPrefixes) {
) could be merged with the rest of the loop iterating {".ldata", ".lrodata" ...}
. So I went ahead to do that. Please take another look, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for reviews! PTAL.
lld/ELF/LinkerScript.cpp
Outdated
StringRef secName = s->name; | ||
if (!secName.consume_front(v)) | ||
continue; | ||
if (!secName.empty() && secName[0] != '.') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
lld/ELF/LinkerScript.cpp
Outdated
return s->name.substr(0, v.size() + 4); | ||
if (isSectionPrefix(".unlikely", secName)) | ||
return s->name.substr(0, v.size() + 9); | ||
if (index == 2) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's ok for linker to map input .bss.hot. to .bss.hot even if compilers don't generate .bss.hot. .
yes, it's ok to map .bss.hot.
to .bss.hot
, in which case if (isSectionPrefix(".hot", secName))
at L127 will return .bss.hot
, and code at L131 (if index==2)
won't run.
As discussed in #148985 (comment) and #148985 (comment), patterns like .rodata.str1.1.unlikely.
or .rodata.cst4.hot.
are not mapped before if (index==2)
because of the strings between section name and hot/unlikely prefix, and if(index==2)
is added to map them. Meanwhile, we can use s->name.substr
to return a StringRef by referencing s->name
.
To get rid of if (index==2)
specialization, one implementation would be something like [1]. If this version is more readable, I can update the patch to it.
[1]
// records the hot prefixes in data array.
// Note `bss.rel.ro` doesn't have hot prefix.
static constexpr StringRef dataSectionHotPrefixes[] = {
".data.rel.ro.hot", ".data.hot", ".rodata.hot", ".bss.rel.ro", ".bss.hot",
};
// records the unlikely suffixes in data array
// Note `bss.rel.ro` doesn't have unlikely prefix.
static constexpr StringRef dataSectionUnlikelyPrefixes[] = {
".data.rel.ro.unlikely", ".data.unlikely", ".rodata.unlikely", ".bss.rel.ro", ".bss.unlikely",
};
for (auto [index, v] : llvm::enumerate(dataSectionPrefixes)) {
... // code before `ctx.arg.zKeepDataSectionPrefix`
if (!ctx.arg.zKeepDataSectionPrefix)
return v;
// We require the trailing dot after hot or unlikely to exist to map them
// to hot or unlikely variant.
if (s->name.ends_with(".hot.")
return dataSectionHotPrefixes[index];
if (s->name.ends_with(".unlikely.")
return dataSectionUnlikelyPrefixes[index];
}
lld/ELF/LinkerScript.cpp
Outdated
|
||
for (auto [index, v] : llvm::enumerate(dataSectionPrefixes)) { | ||
StringRef secName = s->name; | ||
if (!secName.consume_front(v)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The two ifs before if (!ctx.arg.zKeepDataSectionPrefix)
applies on the general cases, and there may not be a trailing dot to data sections. For instance, https://gcc.godbolt.org/z/G77P8n6qf compiles a simple source code with -O2 -ffunction-sections -fdata-sections
, and var section name is .section .bss.var
(without trailing dot).
The description should be updated to describe the .rodata.cstN.hot special case. Ideally just list the literal mapping rules
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The description should be updated to describe the .rodata.cstN.hot special case. Ideally just list the literal mapping rules
This makes sense. Done in PR description and code comment. Please take another look, thanks!
lld/ELF/LinkerScript.cpp
Outdated
return s->name.substr(0, v.size() + 4); | ||
if (isSectionPrefix(".unlikely", secName)) | ||
return s->name.substr(0, v.size() + 9); | ||
if (index == 2) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I lifted the keep-data-section-prefix
path as suggested, and realized that [2] (the for (auto v : dataSectionPrefixes) {
) could be merged with the rest of the loop iterating {".ldata", ".lrodata" ...}
. So I went ahead to do that. Please take another look, thanks!
thanks for the reviews! To share some context and progress on merging this change, we rely on the default behavior configured in the linker script to map When |
On the linker side, the option should be generic, treating .hot and .unlikely uniformly. In LLVM CodeGen, a cl::opt option can be used to manage |
We want to support two use cases:
I agree that
To elaborate on 2 and 3 above, CodeGen is more prone to proliferate for the purpose of materializing various forms in-memory data representation into object files, especially when support for new/customized data are added. Taking #158172 as an example, |
https://discourse.llvm.org/t/rfc-profile-guided-static-data-partitioning/83744 proposes to partition a static data section (like
.data.rel.ro
) into two sections, one grouping the cold ones and the other grouping the rest.This change implements section grouping for {
.data.rel.ro
,.data
,.rodata
,.bss
} to group.hot
input sections to.<section>.hot
in the output and to group.unlikely
input sections to.<section>.unlikely
. The grouping functionality is gated under new lld option-z,keep-data-section-prefix
. The mapping for hot ones is illustrated below.[bar] is a placeholder to represent optional global variable name below
.data.rel.ro.hot.[bar]
=>.data.rel.ro.hot
.data.hot.[bar]
=>.data.hot
.rodata.hot.[bar]
,.rodata.str.*.hot.
,.rodata.cst*.hot.
} => .rodata.hot#148920 is a related change which also introduces lld option
-z,keep-data-section-prefix
. The option will be introduced once and shared between the two patches.