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

[George Rimar via llvm-commits]

grimar added a comment.



In http://reviews.llvm.org/D15779#330316, @rafael wrote:

> > > 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.


Yes, I know, as "overflow" I was mean that there would not be access violation in that place. 
(It is not possible to get it here I believe because of lot of other data in front of buffer before relocations).
So the worst thing could happen is a possible read of some side data at second check. But at least one correct check would still be performed (first one).

> 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.


I am agree with that. Now I think that having partial checks is no better than absence of them here.


http://reviews.llvm.org/D15779