[PATCH] D61477: [ELF] -z combreloc: sort dynamic relocations by (!is_relative,symbol_index,r_offset)

Tue May 7 00:03:13 PDT 2019

grimar added a comment.

In D61477#1492293 <https://reviews.llvm.org/D61477#1492293>, @mcgrathr wrote:

> Sorting R_*_RELATIVE relocs first and setting REL(A)COUNT is a format requirement, period.  It's not a debatable question of optimization.

I never said we should stop doing that. Sorting by an offset implemented in this patch is completely different story.

> The multi-level sorting behavior here makes lld consistent with the GNU linkers, which it is not today.

Above you wrote "The sorting behavior is entirely an optimization for locality at dynamic reloc time. Both locality of symtab indices and locality of reloc offsets are useful to that goal."
I pointed to Rafael's comment saying that sorting by offsets did not show any improvements during the testing and was not useful that time.

Now you're talking about consistency reasons. We are inconsistent with the GNU linkers in a many things and not always we want to fix it.
"consistency" by itself usually is not a reason for doing a change.

One reasonable argument to land this I saw in this thread was "Relocations sorted by r_offset would make a poor person's life easier when debugging with readelf -r :)"
It sounds useful.

In D61477#1492925 <https://reviews.llvm.org/D61477#1492925>, @ruiu wrote:

> Can you give me a pointer to the formal spec and also include that to the patch description?

FWIW here is the most detailed explanation of the feature I found were:
https://www.airs.com/blog/archives/186
http://people.redhat.com/jakub/prelink.pdf (p.2)

I says it worth sorting by `r_offset` though. But out tests showed that sorting did not improve anything (https://reviews.llvm.org/D19528#423828).
So this patch seems to be for a nice output only (i.e. not about the optimization).
I am not against doing this, but I think we should explicitly mention this in comments and/or patch description.

Repository:
  rLLD LLVM Linker

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

https://reviews.llvm.org/D61477