<div dir="ltr">Sorry about the style change. I'll separate them in future.<div><br></div><div>I think the use of unordered_set is reasonable because the number of segments are very small.</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Sun, Oct 11, 2015 at 6:58 AM, Rafael Espíndola <span dir="ltr"><<a href="mailto:rafael.espindola@gmail.com" target="_blank">rafael.espindola@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">The change to use a std::unordered_set is hilly questionable. You<br>
could have kept just a pair of Elf_Phdr and bool, no?<br>
<br>
On 11 October 2015 at 09:48, Rafael Espíndola<br>
<div class="HOEnZb"><div class="h5"><<a href="mailto:rafael.espindola@gmail.com">rafael.espindola@gmail.com</a>> wrote:<br>
> Functions names normally start with a verb. So it should probably be<br>
> copyPhdr and setPhdr.<br>
><br>
> On 10 October 2015 at 18:34, Rui Ueyama via llvm-commits<br>
> <<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>> wrote:<br>
>> Author: ruiu<br>
>> Date: Sat Oct 10 17:34:30 2015<br>
>> New Revision: 249955<br>
>><br>
>> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=249955&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=249955&view=rev</a><br>
>> Log:<br>
>> ELF2: Remove ProgramHeader class and use Elf_Phdr directly. NFC.<br>
>><br>
>> Modified:<br>
>> lld/trunk/ELF/Writer.cpp<br>
>><br>
>> Modified: lld/trunk/ELF/Writer.cpp<br>
>> URL: <a href="http://llvm.org/viewvc/llvm-project/lld/trunk/ELF/Writer.cpp?rev=249955&r1=249954&r2=249955&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/lld/trunk/ELF/Writer.cpp?rev=249955&r1=249954&r2=249955&view=diff</a><br>
>> ==============================================================================<br>
>> --- lld/trunk/ELF/Writer.cpp (original)<br>
>> +++ lld/trunk/ELF/Writer.cpp Sat Oct 10 17:34:30 2015<br>
>> @@ -14,6 +14,7 @@<br>
>> #include "Target.h"<br>
>><br>
>> #include "llvm/Support/FileOutputBuffer.h"<br>
>> +#include <unordered_set><br>
>><br>
>> using namespace llvm;<br>
>> using namespace llvm::ELF;<br>
>> @@ -24,7 +25,7 @@ using namespace lld::elf2;<br>
>><br>
>> namespace {<br>
>><br>
>> -static uint32_t toPHDRFlags(uint64_t Flags) {<br>
>> +static uint32_t toPhdrFlags(uint64_t Flags) {<br>
>> uint32_t Ret = PF_R;<br>
>> if (Flags & SHF_WRITE)<br>
>> Ret |= PF_W;<br>
>> @@ -33,34 +34,6 @@ static uint32_t toPHDRFlags(uint64_t Fla<br>
>> return Ret;<br>
>> }<br>
>><br>
>> -template <class ELFT> struct ProgramHeader {<br>
>> - typedef typename ELFFile<ELFT>::uintX_t uintX_t;<br>
>> - typedef typename ELFFile<ELFT>::Elf_Phdr Elf_Phdr;<br>
>> -<br>
>> - ProgramHeader(uintX_t Type, uintX_t Flags, uintX_t FileOff, uintX_t VA) {<br>
>> - std::memset(&Header, 0, sizeof(Elf_Phdr));<br>
>> - Header.p_type = Type;<br>
>> - Header.p_flags = Flags;<br>
>> - Header.p_align = Target->getPageSize();<br>
>> - Header.p_offset = FileOff;<br>
>> - Header.p_vaddr = VA;<br>
>> - Header.p_paddr = VA;<br>
>> - }<br>
>> -<br>
>> - void setValuesFromSection(OutputSectionBase<ELFT::Is64Bits> *Sec) {<br>
>> - Header.p_flags = toPHDRFlags(Sec->getFlags());<br>
>> - Header.p_offset = Sec->getFileOff();<br>
>> - Header.p_vaddr = Sec->getVA();<br>
>> - Header.p_paddr = Header.p_vaddr;<br>
>> - Header.p_filesz = Sec->getSize();<br>
>> - Header.p_memsz = Header.p_filesz;<br>
>> - Header.p_align = Sec->getAlign();<br>
>> - }<br>
>> -<br>
>> - Elf_Phdr Header;<br>
>> - bool Closed = false;<br>
>> -};<br>
>> -<br>
>> // The writer writes a SymbolTable result to a file.<br>
>> template <class ELFT> class Writer {<br>
>> public:<br>
>> @@ -100,13 +73,17 @@ private:<br>
>> std::vector<OutputSectionBase<ELFT::Is64Bits> *> OutputSections;<br>
>> unsigned getNumSections() const { return OutputSections.size() + 1; }<br>
>><br>
>> + void phdrSet(Elf_Phdr *PH, uint32_t Type, uint32_t Flags, uintX_t FileOff,<br>
>> + uintX_t VA, uintX_t Align);<br>
>> + void phdrCopy(Elf_Phdr *PH, OutputSectionBase<ELFT::Is64Bits> *From);<br>
>> +<br>
>> llvm::BumpPtrAllocator PAlloc;<br>
>> SymbolTable<ELFT> &Symtab;<br>
>> - std::vector<ProgramHeader<ELFT> *> PHDRs;<br>
>> - ProgramHeader<ELFT> PhdrPHDR{PT_PHDR, PF_R, 0, 0};<br>
>> - ProgramHeader<ELFT> FileHeaderPHDR{PT_LOAD, PF_R, 0, 0};<br>
>> - ProgramHeader<ELFT> InterpPHDR{PT_INTERP, 0, 0, 0};<br>
>> - ProgramHeader<ELFT> DynamicPHDR{PT_DYNAMIC, 0, 0, 0};<br>
>> + std::vector<Elf_Phdr *> Phdrs;<br>
>> + Elf_Phdr PhdrPhdr;<br>
>> + Elf_Phdr FileHeaderPhdr;<br>
>> + Elf_Phdr InterpPhdr;<br>
>> + Elf_Phdr DynamicPhdr;<br>
>><br>
>> uintX_t FileSize;<br>
>> uintX_t ProgramHeaderOff;<br>
>> @@ -473,7 +450,7 @@ template <class ELFT> void Writer<ELFT>:<br>
>> }<br>
>><br>
>> template <class ELFT><br>
>> -static bool needsPHDR(OutputSectionBase<ELFT::Is64Bits> *Sec) {<br>
>> +static bool needsPhdr(OutputSectionBase<ELFT::Is64Bits> *Sec) {<br>
>> return Sec->getFlags() & SHF_ALLOC;<br>
>> }<br>
>><br>
>> @@ -481,50 +458,44 @@ static bool needsPHDR(OutputSectionBase<<br>
>> // file offsets.<br>
>> template <class ELFT> void Writer<ELFT>::assignAddresses() {<br>
>> assert(!OutputSections.empty() && "No output sections to layout!");<br>
>> - uintX_t VA = getVAStart();<br>
>> - uintX_t FileOff = 0;<br>
>> -<br>
>> - FileOff += sizeof(Elf_Ehdr);<br>
>> - VA += sizeof(Elf_Ehdr);<br>
>> + uintX_t VA = getVAStart() + sizeof(Elf_Ehdr);<br>
>> + uintX_t FileOff = sizeof(Elf_Ehdr);<br>
>><br>
>> - // The first PHDR entry is PT_PHDR which describes the program header itself.<br>
>> - PHDRs.push_back(&PhdrPHDR);<br>
>> - PhdrPHDR.Header.p_align = 8;<br>
>> - PhdrPHDR.Header.p_offset = FileOff;<br>
>> - PhdrPHDR.Header.p_vaddr = VA;<br>
>> - PhdrPHDR.Header.p_paddr = VA;<br>
>> + // The first Phdr entry is PT_PHDR which describes the program header itself.<br>
>> + Phdrs.push_back(&PhdrPhdr);<br>
>> + phdrSet(&PhdrPhdr, PT_PHDR, PF_R, FileOff, VA, /*Align=*/8);<br>
>><br>
>> - // Reserve space for PHDRs.<br>
>> + // Reserve space for Phdrs.<br>
>> ProgramHeaderOff = FileOff;<br>
>> FileOff = RoundUpToAlignment(FileOff, Target->getPageSize());<br>
>> VA = RoundUpToAlignment(VA, Target->getPageSize());<br>
>><br>
>> if (needsInterpSection())<br>
>> - PHDRs.push_back(&InterpPHDR);<br>
>> + Phdrs.push_back(&InterpPhdr);<br>
>><br>
>> - // Create a PHDR for the file header.<br>
>> - PHDRs.push_back(&FileHeaderPHDR);<br>
>> - FileHeaderPHDR.Header.p_vaddr = getVAStart();<br>
>> - FileHeaderPHDR.Header.p_paddr = getVAStart();<br>
>> - FileHeaderPHDR.Header.p_align = Target->getPageSize();<br>
>> + // Create a Phdr for the file header.<br>
>> + Phdrs.push_back(&FileHeaderPhdr);<br>
>> + phdrSet(&FileHeaderPhdr, PT_LOAD, PF_R, 0, getVAStart(),<br>
>> + Target->getPageSize());<br>
>><br>
>> + std::unordered_set<Elf_Phdr *> Closed;<br>
>> for (OutputSectionBase<ELFT::Is64Bits> *Sec : OutputSections) {<br>
>> if (Sec->getSize()) {<br>
>> - uintX_t Flags = toPHDRFlags(Sec->getFlags());<br>
>> - ProgramHeader<ELFT> *Last = PHDRs.back();<br>
>> - if (Last->Header.p_flags != Flags || !needsPHDR<ELFT>(Sec)) {<br>
>> - // Flags changed. End current PHDR and potentially create a new one.<br>
>> - if (!Last->Closed) {<br>
>> - Last->Header.p_filesz = FileOff - Last->Header.p_offset;<br>
>> - Last->Header.p_memsz = VA - Last->Header.p_vaddr;<br>
>> - Last->Closed = true;<br>
>> + uintX_t Flags = toPhdrFlags(Sec->getFlags());<br>
>> + Elf_Phdr *Last = Phdrs.back();<br>
>> + if (Last->p_flags != Flags || !needsPhdr<ELFT>(Sec)) {<br>
>> + // Flags changed. End current Phdr and potentially create a new one.<br>
>> + if (Closed.insert(Last).second) {<br>
>> + Last->p_filesz = FileOff - Last->p_offset;<br>
>> + Last->p_memsz = VA - Last->p_vaddr;<br>
>> }<br>
>><br>
>> - if (needsPHDR<ELFT>(Sec)) {<br>
>> + if (needsPhdr<ELFT>(Sec)) {<br>
>> VA = RoundUpToAlignment(VA, Target->getPageSize());<br>
>> FileOff = RoundUpToAlignment(FileOff, Target->getPageSize());<br>
>> - PHDRs.push_back(new (PAlloc)<br>
>> - ProgramHeader<ELFT>(PT_LOAD, Flags, FileOff, VA));<br>
>> + auto *PH = new (PAlloc) Elf_Phdr;<br>
>> + phdrSet(PH, PT_LOAD, Flags, FileOff, VA, Target->getPageSize());<br>
>> + Phdrs.push_back(PH);<br>
>> }<br>
>> }<br>
>> }<br>
>> @@ -542,17 +513,22 @@ template <class ELFT> void Writer<ELFT>:<br>
>> FileOff += Size;<br>
>> }<br>
>><br>
>> - // Add a PHDR for the dynamic table.<br>
>> - if (needsDynamicSections())<br>
>> - PHDRs.push_back(&DynamicPHDR);<br>
>> -<br>
>> - FileOff += OffsetToAlignment(FileOff, ELFT::Is64Bits ? 8 : 4);<br>
>> + if (needsInterpSection()) {<br>
>> + InterpPhdr.p_type = PT_INTERP;<br>
>> + phdrCopy(&InterpPhdr, Out<ELFT>::Interp);<br>
>> + }<br>
>> + if (needsDynamicSections()) {<br>
>> + Phdrs.push_back(&DynamicPhdr);<br>
>> + DynamicPhdr.p_type = PT_DYNAMIC;<br>
>> + phdrCopy(&DynamicPhdr, Out<ELFT>::Dynamic);<br>
>> + }<br>
>><br>
>> // Fix up the first entry's size.<br>
>> - PhdrPHDR.Header.p_filesz = sizeof(Elf_Phdr) * PHDRs.size();<br>
>> - PhdrPHDR.Header.p_memsz = sizeof(Elf_Phdr) * PHDRs.size();<br>
>> + PhdrPhdr.p_filesz = sizeof(Elf_Phdr) * Phdrs.size();<br>
>> + PhdrPhdr.p_memsz = sizeof(Elf_Phdr) * Phdrs.size();<br>
>><br>
>> // Add space for section headers.<br>
>> + FileOff = RoundUpToAlignment(FileOff, ELFT::Is64Bits ? 8 : 4);<br>
>> SectionHeaderOff = FileOff;<br>
>> FileOff += getNumSections() * sizeof(Elf_Shdr);<br>
>> FileSize = FileOff;<br>
>> @@ -587,26 +563,21 @@ template <class ELFT> void Writer<ELFT>:<br>
>> EHdr->e_shoff = SectionHeaderOff;<br>
>> EHdr->e_ehsize = sizeof(Elf_Ehdr);<br>
>> EHdr->e_phentsize = sizeof(Elf_Phdr);<br>
>> - EHdr->e_phnum = PHDRs.size();<br>
>> + EHdr->e_phnum = Phdrs.size();<br>
>> EHdr->e_shentsize = sizeof(Elf_Shdr);<br>
>> EHdr->e_shnum = getNumSections();<br>
>> EHdr->e_shstrndx = Out<ELFT>::StrTab->getSectionIndex();<br>
>><br>
>> // If nothing was merged into the file header PT_LOAD, set the size correctly.<br>
>> - if (FileHeaderPHDR.Header.p_filesz == Target->getPageSize()) {<br>
>> - uint64_t Size = sizeof(Elf_Ehdr) + sizeof(Elf_Phdr) * PHDRs.size();<br>
>> - FileHeaderPHDR.Header.p_filesz = Size;<br>
>> - FileHeaderPHDR.Header.p_memsz = Size;<br>
>> + if (FileHeaderPhdr.p_filesz == Target->getPageSize()) {<br>
>> + uint64_t Size = sizeof(Elf_Ehdr) + sizeof(Elf_Phdr) * Phdrs.size();<br>
>> + FileHeaderPhdr.p_filesz = Size;<br>
>> + FileHeaderPhdr.p_memsz = Size;<br>
>> }<br>
>><br>
>> - if (needsInterpSection())<br>
>> - InterpPHDR.setValuesFromSection(Out<ELFT>::Interp);<br>
>> - if (needsDynamicSections())<br>
>> - DynamicPHDR.setValuesFromSection(Out<ELFT>::Dynamic);<br>
>> -<br>
>> auto PHdrs = reinterpret_cast<Elf_Phdr *>(Buf + EHdr->e_phoff);<br>
>> - for (ProgramHeader<ELFT> *PHDR : PHDRs)<br>
>> - *PHdrs++ = PHDR->Header;<br>
>> + for (Elf_Phdr *PH : Phdrs)<br>
>> + *PHdrs++ = *PH;<br>
>><br>
>> auto SHdrs = reinterpret_cast<Elf_Shdr *>(Buf + EHdr->e_shoff);<br>
>> // First entry is null.<br>
>> @@ -631,6 +602,29 @@ template <class ELFT> void Writer<ELFT>:<br>
>> Sec->writeTo(Buf + Sec->getFileOff());<br>
>> }<br>
>><br>
>> +template <class ELFT><br>
>> +void Writer<ELFT>::phdrSet(Elf_Phdr *PH, uint32_t Type, uint32_t Flags,<br>
>> + uintX_t FileOff, uintX_t VA, uintX_t Align) {<br>
>> + PH->p_type = Type;<br>
>> + PH->p_flags = Flags;<br>
>> + PH->p_offset = FileOff;<br>
>> + PH->p_vaddr = VA;<br>
>> + PH->p_paddr = VA;<br>
>> + PH->p_align = Align;<br>
>> +}<br>
>> +<br>
>> +template <class ELFT><br>
>> +void Writer<ELFT>::phdrCopy(Elf_Phdr *PH,<br>
>> + OutputSectionBase<ELFT::Is64Bits> *From) {<br>
>> + PH->p_flags = toPhdrFlags(From->getFlags());<br>
>> + PH->p_offset = From->getFileOff();<br>
>> + PH->p_vaddr = From->getVA();<br>
>> + PH->p_paddr = From->getVA();<br>
>> + PH->p_filesz = From->getSize();<br>
>> + PH->p_memsz = From->getSize();<br>
>> + PH->p_align = From->getAlign();<br>
>> +}<br>
>> +<br>
>> template void lld::elf2::writeResult<ELF32LE>(SymbolTable<ELF32LE> *Symtab);<br>
>> template void lld::elf2::writeResult<ELF32BE>(SymbolTable<ELF32BE> *Symtab);<br>
>> template void lld::elf2::writeResult<ELF64LE>(SymbolTable<ELF64LE> *Symtab);<br>
>><br>
>><br>
>> _______________________________________________<br>
>> llvm-commits mailing list<br>
>> <a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a><br>
>> <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</div></div></blockquote></div><br></div>