We are upgrading our local LLVM and noticed a change in one of our tests. The change led us to commit 165321b3d27de5349520b5fdb7e08cbd238c880f which contains this interesting change:
diff --git a/llvm/lib/MC/MCContext.cpp b/llvm/lib/MC/MCContext.cpp
index 72171488e93f..906588a1a169 100644
--- a/llvm/lib/MC/MCContext.cpp
+++ b/llvm/lib/MC/MCContext.cpp
@@ -580,7 +580,7 @@ void MCContext::recordELFMergeableSectionInfo(StringRef SectionName,
unsigned Flags, unsigned UniqueID,
unsigned EntrySize) {
bool IsMergeable = Flags & ELF::SHF_MERGE;
- if (IsMergeable && (UniqueID == GenericSectionID))
+ if (UniqueID == GenericSectionID)
ELFSeenGenericMergeableSections.insert(SectionName);
// For mergeable sections or non-mergeable sections with a generic mergeable
I am not knowledgeable enough to know whether this change is correct, but the naming makes me suspicious. Why are we adding a section to a container containing “Mergeable” in its name without checking the IsMergeable flag?
If the change is correct, it seems like a rename might be helpful.
It’s been a while since I wrote that so I don’t remember the details. Probably the best way to proceed (if you think there is a bug) would be to open an issue with an example of the incorrect behaviour and tag me on that and I can take a look.
To be clear, I don’t think the change is functionally incorrect, but it was somewhat surprising to us. In our test we look for a set of relocations and the test assumes that the entries would appear next to each other (perhaps dubious). With the change one of the relocation sections is not merged with the others and llvm-objdump adds the header for the unmerged section, interrupting the expected sequence.
One strange thing about it is that the flags on the two sections are identical. So even with this change I would expect them to be merged (the previous behavior), yet they are not. The other odd thing about it is that the sections do not have the mergeable flag set, which contradicts our expectation of them to be merged (the previous behavior). But if I back out this change they are merged. I’m just about to trace through what happens and I’ll post an update if I learn anything significant.
Given these observations and the (to me) confusing logic & naming, I wanted to make sure this change is operating as intended.
All right, I think I know what is happening. The sections containing the data referenced by the relocations indeed have different flags, as one is executable code and the other is global data. So I guess since the sections are separate, the relocations for the symbols in those sections are in different sections. Is this an ELF requirement and/or expected behavior of the patch? The relocation sections themselves never pass through recordELFMergeableSectionInfo.
Not sure if this answers your question, but in ELF, each section usually has its own associated relocation section. I can imagine that if the content sections aren’t merged, their relocation sections also won’t be merged. Relocation sections are usually ordered according to the section offset at which the relocations should be applied. The symbols they refer to can be from any section in any order.