[PATCH] D65242: [ELF] More dynamic relocation packing

[Vic Yang via Phabricator via llvm-commits]

victoryang added a comment.

In D65242#1618221 <https://reviews.llvm.org/D65242#1618221>, @victoryang wrote:

> In D65242#1618217 <https://reviews.llvm.org/D65242#1618217>, @MaskRay wrote:
>
> > In D65242#1618214 <https://reviews.llvm.org/D65242#1618214>, @victoryang wrote:
> >
> > > In D65242#1618207 <https://reviews.llvm.org/D65242#1618207>, @MaskRay wrote:
> > >
> > > > In D65242#1618198 <https://reviews.llvm.org/D65242#1618198>, @victoryang wrote:
> > > >
> > > > > In D65242#1618191 <https://reviews.llvm.org/D65242#1618191>, @MaskRay wrote:
> > > > >
> > > > > > > Would it be simpler to only support an addend of 0 here? I reckon that non-zero addends are rare enough that we can just let them be emitted in ungroupedNonRelatives.
> > > > > >
> > > > > > I'd like @pcc's comment to be addressed. The three relocation types `R_*_JUMP_SLOT, R_*_GLOB_DAT, R_*_COPY` always have 0 r_addend. A symbolic relocation (`R_AARCH64_ABS64, R_X86_64_64`, etc) may have non-zero r_addend but they are rare. Non-relative relocation with non-zero r_addend can just be placed in `ungroupedNonRelatives`.
> > > > >
> > > > >
> > > > > I don't disagree that non-zero addends are rare, but I don't think it'd be any simpler if we only support addends of zero. As it is now, the grouping logic only needs to compare adjacent entries. If we want to only group entries with zero addends, it'd need to check the value of addends in addition to what's currently implemented. What you are asking essentially comes down to adding the following code block to before line 1716:
> > > > >
> > > > >   if (config->isRela && i->addend != 0) {
> > > > >     ungroupedNonRelatives.push_back(*i++);
> > > > >     continue; 
> > > > >   }
> > > > >
> > > >
> > > >
> > > > `if (j - i < 3 || i->addend != 0)` ( it could be `if (j - i < 3 || (config->isRela && i->addend != 0))` but probably unnecessary)
> > >
> > >
> > > Isn't that still not simpler? This is more code and (very) slightly less benefit.
> >
> >
> > There is no more line. It just adds 18 characters.
> >
> > > What are we trying to achieve by restricting addend==0?
> >
> > In return, you can delete these lines, and improve size savings:
> >
> >   if (config->isRela) {
> >     add(g[0].r_addend - addend);
> >     addend = g[0].r_addend;
> >   }
> >   
>
>
> I don't think these can be deleted. If I understand this correctly, we still need to encode the delta relative to the last relocation, so either we still need to keep these lines here or we need to special case handle the first group. Did I miss anything? Just to be clear, I'm not opposed to making the change as requested. I just don't see how that simplify this patch. I'd be very happy to be proven wrong.


Oh wait, I take that back. Relative relocations all have zero addend, so this indeed can be simplified. I'll put up a new diff.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65242/new/

https://reviews.llvm.org/D65242