Jordan, My mistake. I responded to the wrong email.
Additional responses below. Mike > -----Original Message----- > From: Justen, Jordan L > Sent: Wednesday, June 29, 2016 2:40 PM > To: Kinney, Michael D <michael.d.kin...@intel.com>; Shi, Steven > <steven....@intel.com>; > edk2-devel@lists.01.org; Gao, Liming <liming....@intel.com>; Kinney, Michael D > <michael.d.kin...@intel.com> > Cc: af...@apple.com > Subject: RE: [edk2] [PATCH 2/7] BaseTools: Add missing Elf relocation type > for LTO > build > > Why didn't you reply to my message where I made the related comments? > It kind of confuses the email threading this way... > > On 2016-06-29 14:04:49, Kinney, Michael D wrote: > > Jordan, > > > > Steven has been maintaining a public branch of this work at: > > > > https://github.com/shijunjing/edk2/tree > > > > The latest patch series appears to be in the following branch: > > > > https://github.com/shijunjing/edk2/tree/review4 > > > > Thanks. Maybe Steven can name them as something like linux-lto-v1, and > mention the branch in the cover letter in the future. These are not linux. They are GCC and CLANG. I agree the branch names need to be more descriptive, and that adding the branch name to the patch series summary is always a good idea if a public branch is available. > > > Why do you think we do not need a new tool chain tag for CLANG based static > > analysis? > > I think it is very valuable to have static analysis for EDK II builds. We > > do not > > want to run static analysis as part of a normal FW build, so we need a way > > to run > > that type of build. > > > > First, I don't think we should readily add new toolchains so long as > we cannot deprecate old useless toolchains. I'll admit that my > frustration about old toolchains is more of a concern to me than > adding a new toolchain. > > Second, didn't Andrew find that parsing tools_def.txt was taking a > significant amount of time during each build invocation? That was fixed. The issue was parsing that file more than once. > > One idea I have is: Can we get BaseTools to allow target.txt to have > $(TOOLCHAIN) in TOOL_CHAIN_CONF? Then we could set: > > TOOL_CHAIN_CONF = BaseTools/Conf/tools_def.$(TOOLCHAIN) > > This would let us split each toolchain into a separate file. I would > recommend we then add at the top of each file a comment with the > support status. Something like: > > # Support Status: Maintained > > # Support Status: Community > > # Support Status: Deprecated I agree there are many ways to reduce size of tools_def.txt. Removing tool chain tags that are no longer viable at all is a good idea. But it seems every time I see that type of discussion, someone jumps in and says they are still using it :) I do like the deprecated idea. Why can't we simply use the target.txt TOOL_CHAIN_CONF define in target.txt as it works today? You could split tools_def.txt into several versions, with the default one being the maintained one and developers can opt into community or deprecated ones. I do want to make sure we are focused on the right problem here 1) # lines in tools_def.txt 2) maintenance of tools_def.txt 3) build performance I think the build performance issues have been addressed. If someone is aware of build performance issues for the length of tools_def.txt please let us know and we can work on that. One aspect of tools_def.txt syntax that has always bothered me is that tool chains that are closely related require lots of almost duplicate lines. I have been considering the concept of being able to declare that one tool chain is related to another tool chain and we only need to describe the deltas. That would reduce lines and simplify maintenance and may reduce the need to split out tool chains into categories. > > -Jordan > > > > > > -----Original Message----- > > > From: Justen, Jordan L > > > Sent: Wednesday, June 29, 2016 1:57 PM > > > To: Shi, Steven <steven....@intel.com>; edk2-devel@lists.01.org; Gao, > > > Liming > > > <liming....@intel.com> > > > Cc: Kinney, Michael D <michael.d.kin...@intel.com>; af...@apple.com > > > Subject: Re: [edk2] [PATCH 2/7] BaseTools: Add missing Elf relocation > > > type for LTO > > > build > > > > > > The subject prefix should be: "BaseTools/GenFw" > > > > > > Should this patch move before 1/7? > > > > > > -Jordan > > > > > > On 2016-06-28 08:18:56, Shi, Steven wrote: > > > > Add support to convert missing Elf relocation types > > > > (R_X86_64_PLT32, R_X86_64_GOTPCREL, R_X86_64_REX_GOTPCRELX) > > > > to PeCoff, which are required by LTO image. > > > > > > > > Contributed-under: TianoCore Contribution Agreement 1.0 > > > > Signed-off-by: Steven Shi <steven....@intel.com> > > > > --- > > > > BaseTools/Source/C/GenFw/Elf64Convert.c | 98 > > > > ++++++++++++++++++++++++++++----- > > > > BaseTools/Source/C/GenFw/elf_common.h | 2 +- > > > > 2 files changed, 86 insertions(+), 14 deletions(-) > > > > mode change 100644 => 100755 BaseTools/Source/C/GenFw/Elf64Convert.c > > > > mode change 100644 => 100755 BaseTools/Source/C/GenFw/elf_common.h > > > > > > > > diff --git a/BaseTools/Source/C/GenFw/Elf64Convert.c > > > b/BaseTools/Source/C/GenFw/Elf64Convert.c > > > > old mode 100644 > > > > new mode 100755 > > > > index 024a2a0..205360c > > > > --- a/BaseTools/Source/C/GenFw/Elf64Convert.c > > > > +++ b/BaseTools/Source/C/GenFw/Elf64Convert.c > > > > @@ -75,7 +75,7 @@ CleanUp64 ( > > > > ); > > > > > > > > // > > > > -// Rename ELF32 strucutres to common names to help when porting to > > > > ELF64. > > > > +// Rename ELF32 structures to common names to help when porting to > > > > ELF64. > > > > // > > > > typedef Elf64_Shdr Elf_Shdr; > > > > typedef Elf64_Ehdr Elf_Ehdr; > > > > @@ -233,7 +233,7 @@ IsTextShdr ( > > > > Elf_Shdr *Shdr > > > > ) > > > > { > > > > - return (BOOLEAN) ((Shdr->sh_flags & (SHF_WRITE | SHF_ALLOC)) == > > > > SHF_ALLOC); > > > > + return (BOOLEAN) ((Shdr->sh_flags & (SHF_EXECINSTR | SHF_WRITE | > > > > SHF_ALLOC)) > == > > > (SHF_EXECINSTR | SHF_ALLOC)); > > > > } > > > > > > > > STATIC > > > > @@ -256,7 +256,7 @@ IsDataShdr ( > > > > if (IsHiiRsrcShdr(Shdr)) { > > > > return FALSE; > > > > } > > > > - return (BOOLEAN) (Shdr->sh_flags & (SHF_WRITE | SHF_ALLOC)) == > > > > (SHF_ALLOC | > > > SHF_WRITE); > > > > + return (BOOLEAN) (Shdr->sh_flags & (SHF_EXECINSTR | SHF_WRITE | > > > > SHF_ALLOC)) == > > > (SHF_ALLOC | SHF_WRITE); > > > > } > > > > > > > > STATIC > > > > @@ -661,9 +661,9 @@ WriteSections64 ( > > > > > > > > default: > > > > // > > > > - // Ignore for unkown section type. > > > > + // Ignore for unknown section type. > > > > // > > > > - VerboseMsg ("%s unknown section type %x. We directly copy this > > > > section > into > > > Coff file", mInImageName, (unsigned)Shdr->sh_type); > > > > + VerboseMsg ("%s unknown section type %x. We directly ignore > > > > this section > and > > > NOT copy into Coff file", mInImageName, (unsigned)Shdr->sh_type); > > > > break; > > > > } > > > > } > > > > @@ -763,24 +763,24 @@ WriteSections64 ( > > > > // Absolute relocation. > > > > // > > > > VerboseMsg ("R_X86_64_64"); > > > > - VerboseMsg ("Offset: 0x%08X, Addend: 0x%016LX", > > > > - (UINT32)(SecOffset + (Rel->r_offset - SecShdr->sh_addr)), > > > > + VerboseMsg ("Offset: 0x%08X, Addend: 0x%016LX", > > > > + (UINT32)(SecOffset + (Rel->r_offset - SecShdr->sh_addr)), > > > > *(UINT64 *)Targ); > > > > *(UINT64 *)Targ = *(UINT64 *)Targ - SymShdr->sh_addr + > > > mCoffSectionsOffset[Sym->st_shndx]; > > > > VerboseMsg ("Relocation: 0x%016LX", *(UINT64*)Targ); > > > > break; > > > > case R_X86_64_32: > > > > VerboseMsg ("R_X86_64_32"); > > > > - VerboseMsg ("Offset: 0x%08X, Addend: 0x%08X", > > > > - (UINT32)(SecOffset + (Rel->r_offset - SecShdr->sh_addr)), > > > > + VerboseMsg ("Offset: 0x%08X, Addend: 0x%08X", > > > > + (UINT32)(SecOffset + (Rel->r_offset - SecShdr->sh_addr)), > > > > *(UINT32 *)Targ); > > > > *(UINT32 *)Targ = (UINT32)((UINT64)(*(UINT32 *)Targ) - > > > > SymShdr- > >sh_addr > > > + mCoffSectionsOffset[Sym->st_shndx]); > > > > VerboseMsg ("Relocation: 0x%08X", *(UINT32*)Targ); > > > > break; > > > > case R_X86_64_32S: > > > > VerboseMsg ("R_X86_64_32S"); > > > > - VerboseMsg ("Offset: 0x%08X, Addend: 0x%08X", > > > > - (UINT32)(SecOffset + (Rel->r_offset - SecShdr->sh_addr)), > > > > + VerboseMsg ("Offset: 0x%08X, Addend: 0x%08X", > > > > + (UINT32)(SecOffset + (Rel->r_offset - SecShdr->sh_addr)), > > > > *(UINT32 *)Targ); > > > > *(INT32 *)Targ = (INT32)((INT64)(*(INT32 *)Targ) - > > > > SymShdr->sh_addr > + > > > mCoffSectionsOffset[Sym->st_shndx]); > > > > VerboseMsg ("Relocation: 0x%08X", *(UINT32*)Targ); > > > > @@ -790,14 +790,34 @@ WriteSections64 ( > > > > // Relative relocation: Symbol - Ip + Addend > > > > // > > > > VerboseMsg ("R_X86_64_PC32"); > > > > - VerboseMsg ("Offset: 0x%08X, Addend: 0x%08X", > > > > - (UINT32)(SecOffset + (Rel->r_offset - SecShdr->sh_addr)), > > > > + VerboseMsg ("Offset: 0x%08X, Addend: 0x%08X", > > > > + (UINT32)(SecOffset + (Rel->r_offset - SecShdr->sh_addr)), > > > > *(UINT32 *)Targ); > > > > *(UINT32 *)Targ = (UINT32) (*(UINT32 *)Targ > > > > + (mCoffSectionsOffset[Sym->st_shndx] - SymShdr->sh_addr) > > > > - (SecOffset - SecShdr->sh_addr)); > > > > VerboseMsg ("Relocation: 0x%08X", *(UINT32 *)Targ); > > > > break; > > > > + case R_X86_64_PLT32: > > > > + // > > > > + // Relative relocation: L + A - P > > > > + // > > > > + VerboseMsg ("R_X86_64_PLT32"); > > > > + VerboseMsg ("Offset: 0x%08X, Addend: 0x%08X", > > > > + (UINT32)(SecOffset + (Rel->r_offset - SecShdr->sh_addr)), > > > > + *(UINT32 *)Targ); > > > > + *(UINT32 *)Targ = (UINT32) (*(UINT32 *)Targ > > > > + + (mCoffSectionsOffset[Sym->st_shndx] - SymShdr->sh_addr) > > > > + - (SecOffset - SecShdr->sh_addr)); > > > > + VerboseMsg ("Relocation: 0x%08X", *(UINT32 *)Targ); > > > > + break; > > > > + case R_X86_64_GOTPCREL: > > > > + case R_X86_64_REX_GOTPCRELX: > > > > + // > > > > + // Relative relocation: G + GOT + A - P > > > > + // > > > > + VerboseMsg ("R_X86_64_GOTPCREL"); > > > > + break; > > > > default: > > > > Error (NULL, 0, 3000, "Invalid", "%s unsupported ELF > > > > EM_X86_64 > > > relocation 0x%x.", mInImageName, (unsigned) ELF_R_TYPE(Rel->r_info)); > > > > } > > > > @@ -887,6 +907,12 @@ WriteRelocations64 ( > > > > UINT32 Index; > > > > EFI_IMAGE_OPTIONAL_HEADER_UNION *NtHdr; > > > > EFI_IMAGE_DATA_DIRECTORY *Dir; > > > > + UINT64 GoTPcRelPtrOffset = 0; > > > > + UINT64 *RipDisplacementPtr; > > > > + UINT64 *ElfFileGoTPcRelPtr; > > > > + UINT64 *CoffFileGoTPcRelPtr; > > > > + Elf_Shdr *shdr; > > > > + UINT32 i; > > > > > > > > for (Index = 0; Index < mEhdr->e_shnum; Index++) { > > > > Elf_Shdr *RelShdr = GetShdrByIndex(Index); > > > > @@ -902,6 +928,52 @@ WriteRelocations64 ( > > > > switch (ELF_R_TYPE(Rel->r_info)) { > > > > case R_X86_64_NONE: > > > > case R_X86_64_PC32: > > > > + case R_X86_64_PLT32: > > > > + break; > > > > + case R_X86_64_GOTPCREL: > > > > + case R_X86_64_REX_GOTPCRELX: > > > > + // > > > > + // link script force .got and .got.* in .text section, > > > > so GoTPcRel > > > pointer must be in .text section > > > > + // but its value might point to .text or .data section > > > > + // > > > > + RipDisplacementPtr = (UINT64 *)((UINT8*)mEhdr + SecShdr- > >sh_offset + > > > Rel->r_offset - SecShdr->sh_addr); > > > > + GoTPcRelPtrOffset = Rel->r_offset + 4 + > (INT32)(*RipDisplacementPtr) > > > - SecShdr->sh_addr; > > > > + ElfFileGoTPcRelPtr = (UINT64 *)((UINT8*)mEhdr + SecShdr- > >sh_offset + > > > GoTPcRelPtrOffset); > > > > + CoffFileGoTPcRelPtr = (UINT64 *)(mCoffFile + > > > mCoffSectionsOffset[RelShdr->sh_info] + GoTPcRelPtrOffset); > > > > + // > > > > + // Check which section the GoTPcRel pointer point to, and > calculate > > > Elf to Coff sections displacement accordingly > > > > + // > > > > + shdr = NULL; > > > > + for (i = 0; i < mEhdr->e_shnum; i++) { > > > > + shdr = GetShdrByIndex(i); > > > > + if ((*ElfFileGoTPcRelPtr >= shdr->sh_addr) && > > > > + (*ElfFileGoTPcRelPtr < shdr->sh_addr + > > > > shdr->sh_size)) { > > > > + break; > > > > + } > > > > + } > > > > + // > > > > + // Fix the Elf to Coff sections displacement > > > > + // > > > > + if(IsDataShdr(shdr)) { > > > > + *CoffFileGoTPcRelPtr = *CoffFileGoTPcRelPtr + > > > > mDataOffset - > shdr- > > > >sh_addr; > > > > + }else if (IsTextShdr(SecShdr)){ > > > > + *CoffFileGoTPcRelPtr = *CoffFileGoTPcRelPtr + > > > > mTextOffset - > shdr- > > > >sh_addr; > > > > + }else { > > > > + // > > > > + // link script force to merge .rodata .rodata.*, .got > > > > and .got.* > in > > > .text section, > > > > + // not support GoTPcRel point to section other than > > > > .data or > .text > > > > + // > > > > + Error (NULL, 0, 3000, "Invalid", "Not support relocate > > > R_X86_64_GOTPCREL address in section other than .data or .text"); > > > > + assert (FALSE); > > > > + } > > > > + > > > > + VerboseMsg ("R_X86_64_GOTPCREL to > > > > EFI_IMAGE_REL_BASED_DIR64 > Offset: > > > 0x%08X", > > > > + mCoffSectionsOffset[RelShdr->sh_info] + > > > > GoTPcRelPtrOffset); > > > > + CoffAddFixup( > > > > + (UINT32)(UINTN)((UINT64) > > > > mCoffSectionsOffset[RelShdr->sh_info] + > > > GoTPcRelPtrOffset), > > > > + EFI_IMAGE_REL_BASED_DIR64); > > > > + VerboseMsg ("Relocation: 0x%08X", > > > > *(UINT64*)CoffFileGoTPcRelPtr); > > > > + > > > > break; > > > > case R_X86_64_64: > > > > VerboseMsg ("EFI_IMAGE_REL_BASED_DIR64 Offset: 0x%08X", > > > > diff --git a/BaseTools/Source/C/GenFw/elf_common.h > > > b/BaseTools/Source/C/GenFw/elf_common.h > > > > old mode 100644 > > > > new mode 100755 > > > > index 766d0e4..caaae23 > > > > --- a/BaseTools/Source/C/GenFw/elf_common.h > > > > +++ b/BaseTools/Source/C/GenFw/elf_common.h > > > > @@ -1052,6 +1052,6 @@ typedef struct { > > > > #define R_X86_64_DTPOFF32 21 /* Offset in TLS block > > > > */ > > > > #define R_X86_64_GOTTPOFF 22 /* PC relative offset > > > > to IE GOT > entry > > > */ > > > > #define R_X86_64_TPOFF32 23 /* Offset in static TLS > > > > block */ > > > > - > > > > +#define R_X86_64_REX_GOTPCRELX 0x2a > > > > > > > > #endif /* !_SYS_ELF_COMMON_H_ */ > > > > -- > > > > 2.7.4 > > > > > > > > _______________________________________________ > > > > edk2-devel mailing list > > > > edk2-devel@lists.01.org > > > > https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
