[PATCH] D15779: [ELF] - Implemented optimization for R_X86_64_GOTPCREL relocation.

[Rafael Espíndola via llvm-commits]

>> Could canBePreempted take care of the ifunc check?
> I would not do it.
> For example we have the next code currently:
>
> ```
> if (!CanBePreempted && Body && isGnuIFunc<ELFT>(*Body))
>       Reloc = Target->getIRelativeReloc();
> ```
>
> Currently we implemented ifunc for static linking case. But afaik gold/bfd supports dynamic case for which real CBP makes sence. And I am not sure we should cover such special type under clear for understanding canBePreempted() name.

Good point. Thanks.


> ================
> Comment at: ELF/Target.cpp:955
> @@ -906,1 +954,3 @@
>    case R_X86_64_GOTPCREL:
> +    if (S && canOptimizeGotPcRel(Type, *S, Loc, (uint64_t)-1))
> +      optimizeGotPcRel(Loc);
> ----------------
> rafael wrote:
>> You are passing -1 in here because the offset is not available, correct?
>> Is there any point in ever passing the offset if we cannot pass it here?
> I cannot pass it here, but I still do the check for that place inside canOptimizeGotPcRel().
> I just assume that there is not possible to have negative buffer overflow from here.
> My point was: since we dont have  R_X86_64_GOTPCRELX yet its better to do all possible additional checks. After implementation of R_X86_64_GOTPCRELX we probably can remove it (like we dont check overflows for TLS relaxations, assuming inputs are correct).

You would still get a buffer overflow. Say Off is 1. On the first call
you pass 1 and avoid the buffer access. On the second one you pass -1
and still access position -2.

The options I see are
* Save the result of the call.
* Propagate Off to RelocateOne
* Don't check it.

I would go with 3 for now.