[llvm][yaml2obj] Modify section header overriding timing #130942
Conversation
Signed-off-by: Ruoyu Qiu <[email protected]>
|
@llvm/pr-subscribers-llvm-binary-utilities @llvm/pr-subscribers-objectyaml Author: Ruoyu Qiu (cabbaken) Changesyaml2obj should determine the program header offset (and other properties) based on the intended values rather than the final sh_offset of the section header. This change adjusts the timing of when the section header is overridden to ensure that the program headers are set correctly. More details here. Full diff: https://github.com/llvm/llvm-project/pull/130942.diff 1 Files Affected:
diff --git a/llvm/lib/ObjectYAML/ELFEmitter.cpp b/llvm/lib/ObjectYAML/ELFEmitter.cpp
index 9ae76a71ede5e..48d5d58beea86 100644
--- a/llvm/lib/ObjectYAML/ELFEmitter.cpp
+++ b/llvm/lib/ObjectYAML/ELFEmitter.cpp
@@ -206,6 +206,9 @@ template <class ELFT> class ELFState {
NameToIdxMap DynSymN2I;
ELFYAML::Object &Doc;
+ std::vector<std::pair<unsigned, ELFYAML::Section>>
+ SectionHeadersOverrideHelper;
+
StringSet<> ExcludedSectionHeaders;
uint64_t LocationCounter = 0;
@@ -226,6 +229,7 @@ template <class ELFT> class ELFState {
StringRef SecName, ELFYAML::Section *YAMLSec);
void initSectionHeaders(std::vector<Elf_Shdr> &SHeaders,
ContiguousBlobAccumulator &CBA);
+ void overrideSectionHeaders(std::vector<Elf_Shdr> &SHeaders);
void initSymtabSectionHeader(Elf_Shdr &SHeader, SymtabType STType,
ContiguousBlobAccumulator &CBA,
ELFYAML::Section *YAMLSec);
@@ -845,7 +849,7 @@ void ELFState<ELFT>::initSectionHeaders(std::vector<Elf_Shdr> &SHeaders,
}
LocationCounter += SHeader.sh_size;
- overrideFields<ELFT>(Sec, SHeader);
+ SectionHeadersOverrideHelper.push_back({SN2I.get(Sec->Name), *Sec});
continue;
}
@@ -899,12 +903,17 @@ void ELFState<ELFT>::initSectionHeaders(std::vector<Elf_Shdr> &SHeaders,
}
LocationCounter += SHeader.sh_size;
-
- // Override section fields if requested.
- overrideFields<ELFT>(Sec, SHeader);
+ SectionHeadersOverrideHelper.push_back({SN2I.get(Sec->Name), *Sec});
}
}
+template <class ELFT>
+void ELFState<ELFT>::overrideSectionHeaders(std::vector<Elf_Shdr> &SHeaders) {
+ for (std::pair<unsigned, ELFYAML::Section> &IndexAndSec :
+ SectionHeadersOverrideHelper)
+ overrideFields<ELFT>(&IndexAndSec.second, SHeaders[IndexAndSec.first]);
+}
+
template <class ELFT>
void ELFState<ELFT>::assignSectionAddress(Elf_Shdr &SHeader,
ELFYAML::Section *YAMLSec) {
@@ -2090,6 +2099,9 @@ bool ELFState<ELFT>::writeELF(raw_ostream &OS, ELFYAML::Object &Doc,
// Now we can decide segment offsets.
State.setProgramHeaderLayout(PHeaders, SHeaders);
+ // Override section fields if requested.
+ State.overrideSectionHeaders(SHeaders);
+
bool ReachedLimit = CBA.getOffset() > MaxSize;
if (Error E = CBA.takeLimitError()) {
// We report a custom error message instead below.
|
|
We can see the test of But the code here(FileSize and MemSize) this behavior is correct. Both the last two section of the ProgramHeader is And I also found another two failed tests
@jh7370 Do you have any ideas about this? Should these tests be correct? |
There was a problem hiding this comment.
Unfortunately, apart from maybe a couple of quick responses over the next couple of hours, I've run out of time for this today and am off work for the next couple of weeks, so it'll be ~3 weeks before I can continue looking at this. Hopefully @MaskRay might be able to review this PR further. I'm happy for it to go in (once the issues have been addressed) if he is.
We can see the test of
tools/yaml2obj/ELF/program-header-size-offset.yamlfailed because of the unmatchedFileSizeandMemSizeof the last PorgramHeader after delaying theoverrideFields().But the code here(FileSize and MemSize) this behavior is correct. Both the last two section of the ProgramHeader is
SHT_NOBITS, which won't occupy physical space in a file. Does this indicate that the current test has a mistake?
I think the test is just using ShOffset when it should be using Offset instead. I'm not 100% certain about this though and would like @MaskRay to take a look if he gets a chance. Does changing that help fix any issues?
(Additionally, the code only checks for the last
SHT_NOBITShere, rather than all of them.)
It's too long ago to remember whether this was by design or not. I'd avoid changing it unless you can see there's an obvious hole in the test coverage of the actual code. @MaskRay may have a different opinion and I'm happy to defer to that.
And I also found another two failed tests
* [`tools/llvm-readobj/ELF/malformed-pt-dynamic.test`](https://github.com/llvm/llvm-project/blob/main/llvm/test/tools/llvm-readobj/ELF/malformed-pt-dynamic.test#L43): Failed because it assumes `p_offset` is determined by `sh_offset`.
I think the fix here is to avoid this assumption somehow. The test itself is fine, it's just the change in yaml2obj behaviour means we need a different way of forcing the p_offset value. I believe you can fix this test by removing the ShOffset field and replacing it with an Offset field in the program header description.
* [`tools/obj2yaml/ELF/program-headers.yaml`](https://github.com/llvm/llvm-project/blob/main/llvm/test/tools/obj2yaml/ELF/program-headers.yaml#L437): Failed due to a mismatch between the overridden `sh_offset` and the corresponding `p_offset`.
The line number reference here didn't make sense to me for the description you've mentioned. Are you referring to the bar/zed sections at
and the check at line 533? If so, I'm not sure what the fix should be here. ChangingShOffset to Offset in the zed section description might work, but I'm not sure if it will or not (it might result in a different error). If that doesn't work, you might be able to use the SectionHeaderTable field in the YAML to reorder the section headers: put the zed section first in the Sections: list in the YAML, without any specific offset (but a non-zero size), followed by bar, but then in the SectionHeaderTable listing, specify the sections in the opposite order. You might need to break the individual test case into two parts, with two different YAMLs, each for one of the warning messages. Aside: this test case is in the wrong directory, as it's purely a yaml2obj test, not an obj2yaml one. I'm not sure it's worth moving though.
llvm/lib/ObjectYAML/ELFEmitter.cpp
Outdated
| @@ -845,7 +849,7 @@ void ELFState<ELFT>::initSectionHeaders(std::vector<Elf_Shdr> &SHeaders, | |||
| } | |||
|
|
|||
| LocationCounter += SHeader.sh_size; | |||
| overrideFields<ELFT>(Sec, SHeader); | |||
| SectionHeadersOverrideHelper.push_back({SN2I.get(Sec->Name), *Sec}); | |||
There was a problem hiding this comment.
I'm struggling to follow why you can't just store Sec and SHeader in the vector (SHeader as a pointer, obviously)?
llvm/lib/ObjectYAML/ELFEmitter.cpp
Outdated
| void ELFState<ELFT>::overrideSectionHeaders(std::vector<Elf_Shdr> &SHeaders) { | ||
| for (std::pair<unsigned, ELFYAML::Section> &IndexAndSec : | ||
| SectionHeadersOverrideHelper) | ||
| overrideFields<ELFT>(&IndexAndSec.second, SHeaders[IndexAndSec.first]); |
There was a problem hiding this comment.
I think it would just be simpler to update the overrideFields function to take the vector of pairs and iterate it over it inside the function, rather than have this additional function. If you want to rename it to overrideSectionHeaders for additional clarity, I'm not opposed to that.
There was a problem hiding this comment.
initImplicitHeader() will also use overrideFields(), so I defined a new function to call overrideFields().
llvm/lib/ObjectYAML/ELFEmitter.cpp
Outdated
| @@ -2090,6 +2099,9 @@ bool ELFState<ELFT>::writeELF(raw_ostream &OS, ELFYAML::Object &Doc, | |||
| // Now we can decide segment offsets. | |||
| State.setProgramHeaderLayout(PHeaders, SHeaders); | |||
|
|
|||
| // Override section fields if requested. | |||
There was a problem hiding this comment.
| // Override section fields if requested. | |
| // Override section fields, if requested. This needs to happen after program header layout happens, because otherwise the layout will use the new values. |
(and reflow to column width rules of course)
…emove unneed test Signed-off-by: Ruoyu Qiu <[email protected]>
Yes, this fixed this test.
I tried to fix this tests, but I got in malformed-pt-dynamic.test with --- a/llvm/test/tools/llvm-readobj/ELF/malformed-pt-dynamic.test
+++ b/llvm/test/tools/llvm-readobj/ELF/malformed-pt-dynamic.test
@@ -133,7 +133,6 @@ Sections:
Type: SHT_DYNAMIC
Address: 0x1000
Offset: 0x1000
- ShOffset: [[OFFSET=<none>]]
Entries:
- Tag: DT_NULL
Value: 0
@@ -142,5 +141,6 @@ Sections:
ProgramHeaders:
- Type: PT_DYNAMIC
FileSize: [[FILESIZE=<none>]]
+ Offset: [[OFFSET=<none>]]
FirstSec: .dynamic
LastSec: .dynamicThis occurs because there is a check when the program header offset is explicitly defined. It seems that
And about # NOBITS-NEXT: FirstSec: .bss
# NOBITS-NEXT: LastSec: .bssand it will continue to fail here: # UNSORTED-NEXT: error: sections in the program header with index 3 are not sorted by their file offset |
Signed-off-by: Ruoyu Qiu <[email protected]>
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
Signed-off-by: Ruoyu Qiu <[email protected]>
There was a problem hiding this comment.
My apologies for letting this one drop. I needed to work through a fairly busy period and now am on the other side of things. I've applied this patch locally and investigated the test failures and I think I have answers for all of them, so far.
malformed-pt-dynamic.test: I got this to work with the following patch:
diff --git a/llvm/test/tools/llvm-readobj/ELF/malformed-pt-dynamic.test b/llvm/test/tools/llvm-readobj/ELF/malformed-pt-dynamic.test
index 07b3d62ef576..2d9bb1cb4894 100644
--- a/llvm/test/tools/llvm-readobj/ELF/malformed-pt-dynamic.test
+++ b/llvm/test/tools/llvm-readobj/ELF/malformed-pt-dynamic.test
@@ -91,7 +91,7 @@
## Case D: the same as "Case C", but for a 32-bit object.
-# RUN: yaml2obj %s -DBITS=32 -DOFFSET=0xffffffff -o %t5
+# RUN: yaml2obj %s -DBITS=32 -DFILESIZE=8 -DOFFSET=0xffffffff -o %t5
# RUN: llvm-readobj %t5 --dynamic-table 2>&1 | FileCheck -DFILE=%t5 %s \
# RUN: --check-prefix=WARN5 --implicit-check-not=warning:
# RUN: llvm-readelf %t5 --dynamic-table 2>&1 | FileCheck -DFILE=%t5 %s \
@@ -101,7 +101,7 @@
# WARN5: warning: '[[FILE]]': unable to read the dynamic table from SHT_DYNAMIC section with index 1: offset (0xffffffff) + size (0x8) is greater than the file size (0x10c8)
# WARN5: warning: '[[FILE]]': no valid dynamic table was found
-# RUN: yaml2obj %s -DNOHEADERS=true -DBITS=32 -DOFFSET=0xffffffff -o %t5.noheaders
+# RUN: yaml2obj %s -DNOHEADERS=true -DBITS=32 -DFILESIZE=8 -DOFFSET=0xffffffff -o %t5.noheaders
# RUN: llvm-readobj %t5.noheaders --dynamic-table 2>&1 | \
# RUN: FileCheck -DFILE=%t5.noheaders %s --check-prefix=WARN5-NOHEADERS
# RUN: llvm-readelf %t5.noheaders --dynamic-table 2>&1 | \
@@ -133,6 +133,7 @@ Sections:
Type: SHT_DYNAMIC
Address: 0x1000
Offset: 0x1000
+ ShOffset: [[OFFSET=0x1000]]
Entries:
- Tag: DT_NULL
Value: 0
@@ -140,7 +141,5 @@ Sections:
NoHeaders: [[NOHEADERS=false]]
ProgramHeaders:
- Type: PT_DYNAMIC
- FileSize: [[FILESIZE=<none>]]
- Offset: [[OFFSET=<none>]]
- FirstSec: .dynamic
- LastSec: .dynamic
+ FileSize: [[FILESIZE=0x10]]
+ Offset: [[OFFSET=0x1000]]
I dug up the conversation from the original review related to this error message and the comments there helped remind me how to address this, namely to not explicitly mention the sections in the program header. Instead, we just have to always specify the file size and offset. By default, we simply want these to match that of the section, but when we want to do something different, to trigger the various error messages, we explicitly control both the final section offset and the program header offset.
The program-headers.yaml test is quite a bit more complicated. There are actually two parts to it that need modifying. Here's the full patch:
diff --git a/llvm/test/tools/obj2yaml/ELF/program-headers.yaml b/llvm/test/tools/obj2yaml/ELF/program-headers.yaml
index 1d620d54019b..fa607a87a185 100644
--- a/llvm/test/tools/obj2yaml/ELF/program-headers.yaml
+++ b/llvm/test/tools/obj2yaml/ELF/program-headers.yaml
@@ -436,28 +436,38 @@ Sections:
# NOBITS-NEXT: Flags: [ PF_W, PF_R ]
# NOBITS-NEXT: FirstSec: .bss
# NOBITS-NEXT: LastSec: .bss
-# NOBITS-NEXT: Offset: 0x159
+# NOBITS-NEXT: VAddr: 0x1001
+# NOBITS-NEXT: Offset: 0x191
# NOBITS-NEXT: - Type: PT_LOAD
# NOBITS-NEXT: Flags: [ PF_W, PF_R ]
# NOBITS-NEXT: FirstSec: .data.1
# NOBITS-NEXT: LastSec: .bss
-# NOBITS-NEXT: Offset: 0x158
+# NOBITS-NEXT: VAddr: 0x1000
+# NOBITS-NEXT: Offset: 0x190
# NOBITS-NEXT: - Type: PT_LOAD
# NOBITS-NEXT: Flags: [ PF_W, PF_R ]
# NOBITS-NEXT: FirstSec: .data.1
# NOBITS-NEXT: LastSec: .data.2
-# NOBITS-NEXT: Offset: 0x158
+# NOBITS-NEXT: VAddr: 0x1000
+# NOBITS-NEXT: Offset: 0x190
# NOBITS-NEXT: - Type: PT_LOAD
# NOBITS-NEXT: Flags: [ PF_W, PF_R ]
# NOBITS-NEXT: FirstSec: .bss
# NOBITS-NEXT: LastSec: .data.2
-# NOBITS-NEXT: Offset: 0x159
+# NOBITS-NEXT: VAddr: 0x1001
+# NOBITS-NEXT: Offset: 0x191
# NOBITS-NEXT: - Type: PT_LOAD
# NOBITS-NEXT: Flags: [ PF_W, PF_R ]
# NOBITS-NEXT: FirstSec: .foo.bss
# NOBITS-NEXT: LastSec: .bar.bss
# NOBITS-NEXT: VAddr: 0x200000000
-# NOBITS-NEXT: Offset: 0x15A
+# NOBITS-NEXT: Offset: 0x193
+# NOBITS-NEXT: - Type: PT_LOAD
+# NOBITS-NEXT: Flags: [ PF_W, PF_R ]
+# NOBITS-NEXT: FirstSec: .data.3
+# NOBITS-NEXT: LastSec: .bss.large
+# NOBITS-NEXT: VAddr: 0x200000030
+# NOBITS-NEXT: Offset: 0x193
# NOBITS-NEXT: Sections:
--- !ELF
@@ -471,27 +481,37 @@ ProgramHeaders:
Flags: [ PF_W, PF_R ]
FirstSec: .bss
LastSec: .bss
+ VAddr: 0x1001
## Case 2: the SHT_NOBITS section is the last section in the segment.
- Type: PT_LOAD
Flags: [ PF_W, PF_R ]
FirstSec: .data.1
LastSec: .bss
+ VAddr: 0x1000
## Case 3: the SHT_NOBITS section is in the middle of the segment.
- Type: PT_LOAD
Flags: [ PF_W, PF_R ]
FirstSec: .data.1
LastSec: .data.2
+ VAddr: 0x1000
## Case 4: the SHT_NOBITS section is the first section in the segment.
- Type: PT_LOAD
Flags: [ PF_W, PF_R ]
FirstSec: .bss
LastSec: .data.2
+ VAddr: 0x1001
## Case 5: another two SHT_NOBITS sections in a different segment.
- Type: PT_LOAD
Flags: [ PF_W, PF_R ]
FirstSec: .foo.bss
LastSec: .bar.bss
VAddr: 0x200000000
+## Case 6: a SHT_NOBITS section following a normal section, where the SHT_NOBITS size points past the end of the program header.
+ - Type: PT_LOAD
+ Flags: [ PF_W, PF_R ]
+ FirstSec: .data.3
+ LastSec: .bss.large
+ VAddr: 0x200000030
Sections:
- Name: .data.1
Type: SHT_PROGBITS
@@ -502,13 +522,13 @@ Sections:
- Name: .bss
Type: SHT_NOBITS
Flags: [ SHF_WRITE, SHF_ALLOC ]
-## Use a size that is larger than the file size.
- ShSize: 0x00000000FFFFFFFF
- - Name: .data.2
- Type: SHT_PROGBITS
- Flags: [ SHF_WRITE, SHF_ALLOC ]
+## Use an arbitrary non-zero size.
+ Size: 0x1
+ - Name: .data.2
+ Type: SHT_PROGBITS
+ Flags: [ SHF_WRITE, SHF_ALLOC ]
## Use an arbitrary size.
- Size: 0x1
+ Size: 0x1
- Name: .foo.bss
Type: SHT_NOBITS
Flags: [ SHF_WRITE, SHF_ALLOC ]
@@ -523,6 +543,19 @@ Sections:
## Use an arbitrary size that is different to the size of
## the previous section.
Size: 0x20
+ - Name: .data.3
+ Type: SHT_PROGBITS
+ Flags: [ SHF_WRITE, SHF_ALLOC ]
+# Use an abitrary size. Also use an address that is larger than the previous
+# section, because the tools expect segment addresses to be in order.
+ Size: 0x1
+ Address: 0x200000030
+ - Name: .bss.large
+ Type: SHT_NOBITS
+ Flags: [ SHF_WRITE, SHF_ALLOC ]
+## Use a size that is larger than the file size.
+ Size: 0x00000000FFFFFFFF
+ Address: 0x200000031
## Check that we require sections in a program header
## declaration to be sorted by their offsets.
@@ -530,7 +563,6 @@ Sections:
# RUN: FileCheck %s --check-prefix=UNSORTED --implicit-check-not="error:"
# UNSORTED: error: program header with index 0: the section index of .bar is greater than the index of .foo
-# UNSORTED-NEXT: error: sections in the program header with index 3 are not sorted by their file offset
--- !ELF
FileHeader:
@@ -552,15 +584,6 @@ ProgramHeaders:
FirstSec: .foo
LastSec: .bar
VAddr: 0x1000
-## Case 2: the .bar section is placed before the .zed section in the file,
-## but the sh_offset of .zed is less than the sh_offset of
-## the .bar section because of the "ShOffset" property.
-## Document we report an error for such a case.
- - Type: PT_LOAD
- Flags: [ PF_R ]
- FirstSec: .bar
- LastSec: .zed
- VAddr: 0x1001
Sections:
- Name: .foo
Type: SHT_PROGBITS
@@ -571,11 +594,6 @@ Sections:
Type: SHT_PROGBITS
Flags: [ SHF_ALLOC ]
Size: 0x1
- - Name: .zed
- Type: SHT_PROGBITS
- Flags: [ SHF_ALLOC ]
- Size: 0x1
- ShOffset: 0x0
## Check how we dump segments which contain empty sections.
# RUN: yaml2obj --docnum=7 %s -o %t7
For the first case, the existing testing method is no longer possible with your changes. The .bss section was being used for two different things. Firstly testing that a NOBITS section with a size greater than the size of the file doesn't trigger an error, because it should only impact the memory size of the containing segment. Secondly, it was being used for a range of program headers that included it and other sections. Prior to your change, the size of the segments was being determined (in part) by the ShSize of 0xffffffff, but the sections would be laid out much closer together (with an assumption that ,bss was empty). After your change, the segment sizes were no longer being impacted by the ShSize usage, which resulted in them not containing the .bss section in the object generated by yaml2obj. Changing ShSize to Size doesn't work, because then the .data.2 section would be placed at a very high offset, causing the object to be excessively bloated. We could try explicitly specifying the program header Offset and size fields, but I don't think that's particularly correct, as I think the attempt to fold the large size and section ranges are kind of deliberate aspects of the test. However, I cannot see any reason why these need to be the same section, so I've simply split the two behaviours into two different sections. Now .bss deals with the range of a program header, while .bss.large deals with the "past the end" size behaviour. I had to add an additional program header to the test to work with this. The additions of the address fields was necessary to try to ensure the program headers are seen to "contain" the expected SHT_NOBITS sections. There may be alternative ways to fix this aspect of the test, but I don't know what they are, nor do I fully understand without further investigation why similar changes weren't previously needed.
The second case is a lot simpler. This case was testing that yaml2obj was generating a secific error message. I've taken a look at the code and I don't think it's possible to hit that error case any longer, so the test is redundant and the error should probably be changed into an assert within yaml2obj. Specifically, the condition the error is checking for is that at the time of the call to setProgramHeaderLayout, the series of "fragments" (i.e. sections and filler blocks between sections) is sorted by their offset. Previously, this method was called after section header details were overridden, which meant that the override values were used in this comparison. With your change, this check now happens before the overriding. If you try to set the Offset of the sections to be out-of-order in the YAML sections list, a different error about offset going backwards will be hit. On the other hand, specifying the sections in the right order in the Sections list, but then reversing them in the program header's first section/last section list results in the other error being checked for in this test case.
That's a lot of words, but hopefully it should allow you to progress whilst also understand what is going on.
| @@ -179,32 +179,3 @@ ProgramHeaders: | |||
| # RUN: FileCheck %s --check-prefix=INVALID-OFFSET | |||
|
|
|||
| # INVALID-OFFSET: yaml2obj: error: 'Offset' for segment with index 1 must be less than or equal to the minimum file offset of all included sections (0x78) | |||
|
|
|||
| ## Document that the "Offset" value is checked after the section offset is overriden using "ShOffset". | |||
There was a problem hiding this comment.
Rather than outright delete this test case, we should update it to show that the opposite is true, i.e. that the offset check happens before ShOffset is applied.
|
I forgot what ShOffset did and I was confused when reading the commit message for the first time... Consider incorporating the following to the description.
|
|
I'm sorry that I just went through a few busy weeks changing jobs and didn't have time to pay attention to this PR. I will correct this code next week, thanks for your reply~ |
yaml2obj should determine the program header offset (and other properties) based on the intended values rather than the final sh_offset of the section header.
This change adjusts the timing of when the section header is overridden to ensure that the program headers are set correctly.
More details here.