Details
- Reviewers
• rafael
Diff Detail
Event Timeline
This doesn't build:
/home/espindola/llvm/llvm/tools/lld/ELF/Writer.cpp:289:9: error:
member initializer 'ProgramHeader' does not name a non-static data
member or base class
: ProgramHeader(PT_DYNAMIC, ^~~~~~~~~~~~~~~~~~~~~~~~~
/home/espindola/llvm/llvm/tools/lld/ELF/Writer.cpp:291:5: error: use
of undeclared identifier 'Header'
Header.p_offset = Sec->getFileOff(); ^
...
git-clang-format the patch
+ ProgramHeader(uintX_t p_type, uintX_t p_flags) : Closed(false) {
Used
bool Closed = false;
std::unique_ptr<llvm::FileOutputBuffer> Buffer;
+
llvm::SpecificBumpPtrAllocator<OutputSection<ELFT>> CAlloc;
Why?
+ void setFromSection(OutputSectionBase<Is64Bits> &Sec) {
Set what from section?
+ // Create a PHDR for the first output section which covers the file header.
+ PHDRs.push_back(new (PAlloc) ProgramHeader<ELFT::Is64Bits>(
+ PT_LOAD, convertSectionFlagsToPHDRFlags(OutputSections[0]->getFlags())));
This means that the file and program headers are now executable. While
that is the common default for elf linkers, it is undesirable in
security sensitive contexts. For now, just keep creating the first
PT_LOAD with just PF_R. We can change that (possible with an option)
on another patch.
+ if (Sec->getSize()) {
All sh_alloc segments come before all others, so it might be
possible/cleaner to write this with
if (outputSectionHasPHDR<ELFT>(Sec)) {
And just close the last program header out of the loop, no?
I noticed that this patch doesn't change compSec. This means that we
might alternate from, for example, PF_R to PF_R|PF_W back to PF_R and
create more segments than necessary. Changing this on another patch is
preferable to changing it on this one. I just wanted to point it out.
Cheers,
Rafael
std::unique_ptr<llvm::FileOutputBuffer> Buffer;+
llvm::SpecificBumpPtrAllocator<OutputSection<ELFT>> CAlloc;Why?
Separating out logically related members.
+ if (Sec->getSize()) {
All sh_alloc segments come before all others, so it might be
possible/cleaner to write this withif (outputSectionHasPHDR<ELFT>(Sec)) {
And just close the last program header out of the loop, no?
There are zero sized allocated sections (.data, .bss) in some tests that have different flags, and would create zero sized PHDRs. This ignores them.
Closing the last program header outside of the loop makes that PT_LOAD include all of the non SHF_ALLOC sections.
This now needs more tests.
The main thing this patch does is put different output sections into
the same program header. Add a new test that checks for just that:
The file has two sections that cannot possibly be in the section
output section (different names for example).
The sections have the same flags, so they end up in the same program header.
Cheers,
Rafael
ELF/Writer.cpp | ||
---|---|---|
325–332 | Can this just be *PHDR = Header? |
ELF/Writer.cpp | ||
---|---|---|
747 | This creates an output section even if the section is empty. | |
760–780 | This does not. We should to treat the first section specially. What I would suggest is to have a "ProgramHeader *LastHeader" variable and initialize it to the file header program header. In the loop you can check if the current section is compatible with the previous one. If not, create a new one and add to LastHeader. Note in particular that if the first section that needs a program header is read only, the original program header gets extended. | |
test/elf2/program-header-layout.s | ||
3 ↗ | (On Diff #34831) | It doesn't fit in 80 columns? |
This does not. We should to treat the first section specially. What I would suggest is to have a "ProgramHeader *LastHeader" variable and initialize it to the file header program header.
We should *not* treat ...
LGTM on the condition that one extra test is added showing a read only section being included in the initial PT_LOAD.
Can this just be *PHDR = Header?