[RFC][ELF] Refactor relocation processing

[MaskRay]

Summary

Current relocation processing does:

if (the relocation expr belongs to category A)
  do something;
if (the relocation expr belongs to category B)
  do something;
if (the relocation expr belongs to category C)
  do something;

This refactor flattens it into

switch (expr) {
case R_PC:
case R_...:
  do something;
case R_...:
  do something;
...
}

which may potentially improve performance.
In addition, if some architectures have strange logic, we can add a new RelExpr member and make it interrupt less to the generic code.

[peter.smith]

Will take a look later this week, hopefully tomorrow. Just got back into the office and have a bit of a backlog of stuff to go through.

[peter.smith]

On a first look I like the way this is going. There is more information in a single place. I've made a few suggestions but none are particularly important for an RFC. If I get time I'll keep thinking to see if I can think of any other suggestions.

lld/ELF/Relocations.cpp

214–215

I'm guessing that some of the entries have been taken out as the relocations were not CUSTOM so they would never expect to be called?

Perhaps worth changing the comment
// The following expressions always compute a constant. The list does not contain expressions marked RELOCATE in scanReloc.

214–215

Is it worth moving closer to the only caller processRelocAux. Perhaps worth emphasisng that this only handles relocations that are expected in processRelocaAux. Could do with a comment or perhaps renaming to something like customRelocIsStaticLinkTimeConstant

If there were a way to know if a particular expr code could be CUSTOM outside of scanReloc then we could assert that which would make it clear, but I'm not sure if there is an easily maintainable way to do that.

890

suggest cantUseRelocationAgainstLocal or invalidRelocToLocal

1168–1169

Perhaps worth renaming to handleTlsNonIE

1372

This is also in handleTlsRelocation, worth making an inline function? Something like toExecRelax()

1445

Would be a shame to lose the information in the comment, even if it had to be moved/split up.

[MaskRay]

Thanks for the comments. Applies changes.

I put up this patch because I think our relocation processing still has some problems
wand want explore how we should organize relocations.

lld/ELF/Relocations.cpp

214–215

Thanks for the suggestion. I just pushed the isStaticLinkTimeConstant move and comment adding separately.

890

Used invalidRelocAgainstSym.

I was thinking of extracting this out as a separate function to be shared, but currently it is only used once. I may inline it again.

[stefanp]

I feel like there is a lot going on in this patch. It may be easier if it was just done little by little.
For example: toExecRelax is now a static function. This can be separated out into a small NFC patch where the only thing that is done is the refactoring of that one static function.
Also, handleTlsRelocation returns a slightly different value (what should be skipped vs what was processed). This can also be done as a separate mini patch.

Anyway, those are my 2 cents.

lld/ELF/Relocations.cpp

1269–1271

Do we need this } and the return 0; ?
The if has been changed to an assert so now we just return return target->getTlsGdRelaxSkip(type) - 1;.

You can also remove the starting }.

[MaskRay]

In D113228#3122697, @stefanp wrote:

I feel like there is a lot going on in this patch. It may be easier if it was just done little by little.
For example: toExecRelax is now a static function. This can be separated out into a small NFC patch where the only thing that is done is the refactoring of that one static function.
Also, handleTlsRelocation returns a slightly different value (what should be skipped vs what was processed). This can also be done as a separate mini patch.

Anyway, those are my 2 cents.

Structurally it's difficult to do it piecemeal. This patch explores an alternative way handling relocations.
If we consider the current state and the exploration local optima, some changes (like moving toExecRelax outside) are actually strictly inferior (if we don't split handleTlsRelocations).

For minor details, some changes can be split and I've pushed such commits separately (e.g. moving isStaticLinkTimeConstant).

I have spent much time on this patch (some architectures like Mips and PPC64 add significant complexity) but not entirely decided this is the way to go.

[pkasting]

It's about a year later; I'm curious if you've gained any insight into whether this is a good direction or not.

[MaskRay]

In D113228#3869752, @pkasting wrote:

It's about a year later; I'm curious if you've gained any insight into whether this is a good direction or not.

I think there is value in optimizing relocation scanning. For some workload lld's pass takes 2.x time as mold's.
InputSectionBase::relocations takes some time but other checks contribute to the other slowdown.

We probably need to be more aggressive and possibly moving more relocation scanning into arch-specific ELF/Arch/*.cpp.