IR: Introduce !elf_section_properties for setting section properties. #149260
Conversation
Created using spr 1.3.6-beta.1
|
@llvm/pr-subscribers-backend-x86 @llvm/pr-subscribers-llvm-ir Author: Peter Collingbourne (pcc) ChangesThis new metadata type may be used to set sh_type and sh_entsize on a TODO:
Full diff: https://github.com/llvm/llvm-project/pull/149260.diff 2 Files Affected:
diff --git a/llvm/include/llvm/IR/FixedMetadataKinds.def b/llvm/include/llvm/IR/FixedMetadataKinds.def
index df572e8791e13..16fd9f4996176 100644
--- a/llvm/include/llvm/IR/FixedMetadataKinds.def
+++ b/llvm/include/llvm/IR/FixedMetadataKinds.def
@@ -53,3 +53,4 @@ LLVM_FIXED_MD_KIND(MD_DIAssignID, "DIAssignID", 38)
LLVM_FIXED_MD_KIND(MD_coro_outside_frame, "coro.outside.frame", 39)
LLVM_FIXED_MD_KIND(MD_mmra, "mmra", 40)
LLVM_FIXED_MD_KIND(MD_noalias_addrspace, "noalias.addrspace", 41)
+LLVM_FIXED_MD_KIND(MD_elf_section_properties, "elf_section_properties", 42)
diff --git a/llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp b/llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
index 5454cd475f5ed..2c523b58d0de4 100644
--- a/llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
+++ b/llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
@@ -653,10 +653,11 @@ static StringRef getSectionPrefixForGlobal(SectionKind Kind, bool IsLarge) {
static SmallString<128>
getELFSectionNameForGlobal(const GlobalObject *GO, SectionKind Kind,
Mangler &Mang, const TargetMachine &TM,
- unsigned EntrySize, bool UniqueSectionName,
+ bool UniqueSectionName,
const MachineJumpTableEntry *JTE) {
SmallString<128> Name =
getSectionPrefixForGlobal(Kind, TM.isLargeGlobalValue(GO));
+ unsigned EntrySize = getEntrySizeForKind(Kind);
if (Kind.isMergeableCString()) {
// We also need alignment here.
// FIXME: this is getting the alignment of the character, not the
@@ -790,8 +791,8 @@ calcUniqueIDUpdateFlagsAndSize(const GlobalObject *GO, StringRef SectionName,
// implicitly for this symbol e.g. .rodata.str1.1, then we don't need
// to unique the section as the entry size for this symbol will be
// compatible with implicitly created sections.
- SmallString<128> ImplicitSectionNameStem = getELFSectionNameForGlobal(
- GO, Kind, Mang, TM, EntrySize, false, /*MJTE=*/nullptr);
+ SmallString<128> ImplicitSectionNameStem =
+ getELFSectionNameForGlobal(GO, Kind, Mang, TM, false, /*MJTE=*/nullptr);
if (SymbolMergeable &&
Ctx.isELFImplicitMergeableSectionNamePrefix(SectionName) &&
SectionName.starts_with(ImplicitSectionNameStem))
@@ -802,8 +803,9 @@ calcUniqueIDUpdateFlagsAndSize(const GlobalObject *GO, StringRef SectionName,
return NextUniqueID++;
}
-static std::tuple<StringRef, bool, unsigned>
-getGlobalObjectInfo(const GlobalObject *GO, const TargetMachine &TM) {
+static std::tuple<StringRef, bool, unsigned, unsigned, unsigned>
+getGlobalObjectInfo(const GlobalObject *GO, const TargetMachine &TM,
+ StringRef SectionName, SectionKind Kind) {
StringRef Group = "";
bool IsComdat = false;
unsigned Flags = 0;
@@ -814,7 +816,23 @@ getGlobalObjectInfo(const GlobalObject *GO, const TargetMachine &TM) {
}
if (TM.isLargeGlobalValue(GO))
Flags |= ELF::SHF_X86_64_LARGE;
- return {Group, IsComdat, Flags};
+
+ unsigned Type, EntrySize;
+ if (MDNode *MD = GO->getMetadata(LLVMContext::MD_elf_section_properties)) {
+ Type = cast<ConstantAsMetadata>(MD->getOperand(0))
+ ->getValue()
+ ->getUniqueInteger()
+ .getZExtValue();
+ EntrySize = cast<ConstantAsMetadata>(MD->getOperand(1))
+ ->getValue()
+ ->getUniqueInteger()
+ .getZExtValue();
+ } else {
+ Type = getELFSectionType(SectionName, Kind);
+ EntrySize = getEntrySizeForKind(Kind);
+ }
+
+ return {Group, IsComdat, Flags, Type, EntrySize};
}
static StringRef handlePragmaClangSection(const GlobalObject *GO,
@@ -850,18 +868,18 @@ static MCSection *selectExplicitSectionGlobal(const GlobalObject *GO,
Kind = getELFKindForNamedSection(SectionName, Kind);
unsigned Flags = getELFSectionFlags(Kind, TM.getTargetTriple());
- auto [Group, IsComdat, ExtraFlags] = getGlobalObjectInfo(GO, TM);
+ auto [Group, IsComdat, ExtraFlags, Type, EntrySize] =
+ getGlobalObjectInfo(GO, TM, SectionName, Kind);
Flags |= ExtraFlags;
- unsigned EntrySize = getEntrySizeForKind(Kind);
const unsigned UniqueID = calcUniqueIDUpdateFlagsAndSize(
GO, SectionName, Kind, TM, Ctx, Mang, Flags, EntrySize, NextUniqueID,
Retain, ForceUnique);
const MCSymbolELF *LinkedToSym = getLinkedToSymbol(GO, TM);
- MCSectionELF *Section = Ctx.getELFSection(
- SectionName, getELFSectionType(SectionName, Kind), Flags, EntrySize,
- Group, IsComdat, UniqueID, LinkedToSym);
+ MCSectionELF *Section =
+ Ctx.getELFSection(SectionName, Type, Flags, EntrySize, Group, IsComdat,
+ UniqueID, LinkedToSym);
// Make sure that we did not get some other section with incompatible sh_link.
// This should not be possible due to UniqueID code above.
assert(Section->getLinkedToSymbol() == LinkedToSym &&
@@ -899,13 +917,6 @@ static MCSectionELF *selectELFSectionForGlobal(
const TargetMachine &TM, bool EmitUniqueSection, unsigned Flags,
unsigned *NextUniqueID, const MCSymbolELF *AssociatedSymbol,
const MachineJumpTableEntry *MJTE = nullptr) {
-
- auto [Group, IsComdat, ExtraFlags] = getGlobalObjectInfo(GO, TM);
- Flags |= ExtraFlags;
-
- // Get the section entry size based on the kind.
- unsigned EntrySize = getEntrySizeForKind(Kind);
-
bool UniqueSectionName = false;
unsigned UniqueID = MCSection::NonUniqueID;
if (EmitUniqueSection) {
@@ -916,15 +927,18 @@ static MCSectionELF *selectELFSectionForGlobal(
(*NextUniqueID)++;
}
}
- SmallString<128> Name = getELFSectionNameForGlobal(
- GO, Kind, Mang, TM, EntrySize, UniqueSectionName, MJTE);
+ SmallString<128> Name =
+ getELFSectionNameForGlobal(GO, Kind, Mang, TM, UniqueSectionName, MJTE);
+
+ auto [Group, IsComdat, ExtraFlags, Type, EntrySize] =
+ getGlobalObjectInfo(GO, TM, Name, Kind);
+ Flags |= ExtraFlags;
// Use 0 as the unique ID for execute-only text.
if (Kind.isExecuteOnly())
UniqueID = 0;
- return Ctx.getELFSection(Name, getELFSectionType(Name, Kind), Flags,
- EntrySize, Group, IsComdat, UniqueID,
- AssociatedSymbol);
+ return Ctx.getELFSection(Name, Type, Flags, EntrySize, Group, IsComdat,
+ UniqueID, AssociatedSymbol);
}
static MCSection *selectELFSectionForGlobal(
|
You can test this locally with the following command:git-clang-format --diff HEAD~1 HEAD --extensions cpp -- llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp llvm/lib/IR/Verifier.cppView the diff from clang-format here.diff --git a/llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp b/llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
index 292c06f53..207adb0e5 100644
--- a/llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
+++ b/llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
@@ -799,13 +799,13 @@ getGlobalObjectInfo(const GlobalObject *GO, const TargetMachine &TM,
unsigned Type, EntrySize;
if (MDNode *MD = GO->getMetadata(LLVMContext::MD_elf_section_properties)) {
Type = cast<ConstantAsMetadata>(MD->getOperand(0))
- ->getValue()
- ->getUniqueInteger()
- .getZExtValue();
+ ->getValue()
+ ->getUniqueInteger()
+ .getZExtValue();
EntrySize = cast<ConstantAsMetadata>(MD->getOperand(1))
- ->getValue()
- ->getUniqueInteger()
- .getZExtValue();
+ ->getValue()
+ ->getUniqueInteger()
+ .getZExtValue();
} else {
Type = getELFSectionType(SectionName, Kind);
EntrySize = getEntrySizeForKind(Kind);
diff --git a/llvm/lib/IR/Verifier.cpp b/llvm/lib/IR/Verifier.cpp
index 537b6dfcb..aae54edf5 100644
--- a/llvm/lib/IR/Verifier.cpp
+++ b/llvm/lib/IR/Verifier.cpp
@@ -766,7 +766,8 @@ void Verifier::visitGlobalValue(const GlobalValue &GV) {
if (auto *Props = GO->getMetadata(LLVMContext::MD_elf_section_properties)) {
Check(Props->getNumOperands() == 2,
- "elf_section_properties metadata must have two operands", GO, Props);
+ "elf_section_properties metadata must have two operands", GO,
+ Props);
if (Props->getNumOperands() == 2) {
auto *Type = dyn_cast<ConstantAsMetadata>(Props->getOperand(0));
Check(Type, "type field must be ConstantAsMetadata", GO, Props);
|
| @@ -53,3 +53,4 @@ LLVM_FIXED_MD_KIND(MD_DIAssignID, "DIAssignID", 38) | |||
| LLVM_FIXED_MD_KIND(MD_coro_outside_frame, "coro.outside.frame", 39) | |||
| LLVM_FIXED_MD_KIND(MD_mmra, "mmra", 40) | |||
| LLVM_FIXED_MD_KIND(MD_noalias_addrspace, "noalias.addrspace", 41) | |||
| LLVM_FIXED_MD_KIND(MD_elf_section_properties, "elf_section_properties", 42) | |||
There was a problem hiding this comment.
Add document to llvm/docs/LangRef.rst named metadata?
Created using spr 1.3.6-beta.1
|
Ping |
|
I am in favor of this change from the object file format's perspective, but it would be beneficial to broaden the pool of reviewers. The property, if we decide to introduce, does not need to be overly extensible. That is because within the section header type |
There was a problem hiding this comment.
The intended semantics without a corresponding "section" marking seem a little ambiguous... if we're not promising to put the global in a specific section, what section/sections are relevant? (If we require a section marking, we also inherit the optimization restrictions for globals with an explicit section, which seems useful here.)
What happens if there's a disagreement between different globals on the correct section attributes? Do we just generate multiple sections with the same name?
Missing Verifier checks for the new metadata.
I don't think we need to require a section marking. If the global does not have a section marking, the intent is that the section name will be the same as if it didn't have the metadata attachment, implying that the attachment is only appropriate if input sections with the same output section are allowed to have different types, which is usually accepted by ELF linkers [1]. This is what we want to happen with CFI jump tables because I think we want the jump table to end up in Of course, if another feature would benefit from the optimization restrictions that come from a custom section name, that feature would be free to set the section name. [1] LLD warns in case of a mismatch unless a change is made to canMergeToProgbits as I did in #147424, so a feature that uses this metadata attachment without an explicit section would likely need to modify LLD in a similar way.
Yes. I force unique section IDs for these sections to ensure that this works even without function sections.
Will add. |
|
There are various transforms which currently check hasSection() on globals; some of them you probably care about if you want to actually preserve the metadata. Like constant merging. |
|
At least for the use cases implemented so far, it would be valid to drop the metadata. For example, it could be dropped from CFI jump tables and the resulting binary would still be correct, it would just be less optimized because jump table relaxation could not be applied. If a user would benefit from a higher likelihood of the metadata being preserved, they have the option of modifying passes such as ConstantMerge to skip globals that have this metadata. |
Created using spr 1.3.6-beta.1
Done |
Is the opposite also true, that you can add it to non-cfi-jump-table globals and the resulting binary is still correct? (Unless there's some check, constant merging can merge either way, I think.) |
I would expect passes to refuse to merge constants with inconsistent metadata, as that is effectively the same as adding metadata to one of the sides, which is not guaranteed to result in correct behavior. I seem to recall a discussion about this a few years ago in the context of merging instructions, the outcome being that passes should not do this. Indeed, looking at ConstantMerge it does refuse to merge globals with non-debug metadata: |
This new metadata type may be used to set sh_type and sh_entsize on a
global's section. The intent is that it will be used to mark up
CFI jump table sections.