Re: [PATCH v2 2/7] BaseTools-GenFw:Add new x86_64 Elf relocation types for PIC/PIE code

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 31 July 2016 at 07:42, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> On 31 July 2016 at 05:08, Shi, Steven <steven.shi@intel.com> wrote:
>>> OK. Could we do that in a separate patch?
>> [Steven]: Yes. We could separate it in a new patch.
>>
>>> >> Is this necessary? I would expect the GOT entry itself to be already
>>> >> covered by a R_X86_64_64 relocation, so I don't think there is a need
>>> >> to emit a EFI_IMAGE_REL_BASED_DIR64 PE/COFF reloc here.
>>> > [Steven]: Do you ask whether it is still necessary to support the
>>> GOTPCREL/GOTPCRELX/REX_GOTPCRELX new relocation types? I think they
>>> are nice to have now, since new type support has no impact to old ones, we
>>> could just leave the code there for reference.
>>>
>>> No. The question is whether it is necessary to emit the
>>> EFI_IMAGE_REL_BASED_DIR64 fixup reloc here. The relocation place is
>>> the instruction, not the GOT entry, and I would expect the GOT entry
>>> to be covered by a R_X86_64_64 relocation already
>> [Steven]: Yes, it is necessary. Even the R_X86_64_64 relocation need to emit the EFI_IMAGE_REL_BASED_DIR64 fixup reloc like below in the original GenFw code. You can test it like this, remove the below CoffAddFixup() code and build the OVMF with GCC5, you will see the Ovmf boot failure with cpu exception.
>>
>>             case R_X86_64_64:
>>               VerboseMsg ("EFI_IMAGE_REL_BASED_DIR64 Offset: 0x%08X",
>>                 mCoffSectionsOffset[RelShdr->sh_info] + (Rel->r_offset - SecShdr->sh_addr));
>>               CoffAddFixup(
>>                 (UINT32) ((UINT64) mCoffSectionsOffset[RelShdr->sh_info]
>>                 + (Rel->r_offset - SecShdr->sh_addr)),
>>                 EFI_IMAGE_REL_BASED_DIR64);
>>
>
> That was not my point. With your code, how many
> EFI_IMAGE_REL_BASED_DIR64 fixups are added to the .reloc section for
> the GOT entry of 'n'?
>
> int n;
> int f () { return n; }
> int g () { return n; }
> int h () { return n; }

I am also concerned about the GOTPCRELX/REX_GOTPCRELX relocations.
Reading the x86_64 ABI docs, it appears that these may refer to
instructions that have been modified by the linker. In that case, how
do we deal with the relocation? Also, according to the doc, mov
instructions may be emitted by the linker in some cases that are only
valid in the lowest 2 GB of the address space.

All in all, I think supporting GOT based relocations is a can of
worms, and I would prefer to get rid of them completely if we can
(i.e., using hidden visibility even for LTO, I have a fix for that I
will sent out separately)

Thanks,
Ard.