Re: [PATCH v2] BaseTools/GenFw: Add X64 GOTPCREL Support to GenFw

["Shi, Steven" <steven.shi@intel.com>]

Hi Zenith,
I like your patch. Sorry for the late response. I see your patch  has no impact to current supported relocation types, and I also hope we can add more relocation types in our GenFw tool. 
Below are some code style issues in your patch:

 
> +STATIC
> +VOID
> +FindElfGOTSectionFromGOTEntryElfRva (
> +  Elf64_Addr GOTEntryElfRva
> +  )
> +{
> +  UINT32 i;
> +  if (mGOTShdr != NULL) {
> +    if (GOTEntryElfRva >= mGOTShdr->sh_addr &&
> +        GOTEntryElfRva <  mGOTShdr->sh_addr + mGOTShdr->sh_size)
> +      return;

We usually use the {} to brace the  if branch statements, even it has only one statement,  like this:
    if (GOTEntryElfRva >= mGOTShdr->sh_addr &&
        GOTEntryElfRva <  mGOTShdr->sh_addr + mGOTShdr->sh_size){
        return;
    }


> +STATIC
> +BOOLEAN
> +AccumulateCoffGOTEntries (
> +  UINT32 GOTCoffEntry
> +  )
> +{
> +  UINT32 i;
> +  if (mGOTCoffEntries != NULL) {
> +    for (i = 0; i < mGOTNumCoffEntries; i++)
> +      if (mGOTCoffEntries[i] == GOTCoffEntry)
> +        return FALSE;
Same issue as above, we usually always use {} to include if true branch statement like this:
      if (mGOTCoffEntries[i] == GOTCoffEntry){
        return FALSE;
      }

> +STATIC
> +int
> +__comparator (
> +  const void* lhs,
> +  const void* rhs
> +  )
> +{
> +  if (*(const UINT32*)lhs < *(const UINT32*)rhs)
> +    return -1;
> +  return *(const UINT32*)lhs > *(const UINT32*)rhs;
> +}
We usually don't use '__' as prefix for a function, could you enhance the comparator() name and add {} to include if branch statement?


> +STATIC
> +VOID
> +EmitGOTRelocations (
> +  VOID
> +  )
> +{
> +  UINT32 i;
> +  if (mGOTCoffEntries == NULL)
> +    return;
Please add {} to include if branch statement


> +        if (mEhdr->e_machine == EM_X86_64 && RelShdr->sh_info ==
> mGOTShindex) {
> +          //
> +          // Tack relocations for GOT entries after other relocations for
> +          //   the section the GOT is in, as it's usually found at the end
> +          //   of the section.
> +          //
> +          EmitGOTRelocations();
> +        }
>        }
>      }
>    }
> 
> +  if (mEhdr->e_machine == EM_X86_64) {
> +    //
> +    // Just in case GOT is in a section with no other relocations
> +    //
> +    EmitGOTRelocations();
> +  }
Could we combine the two EmitGOTRelocations() invokes and only keep the second one?

Thanks
Steven