[PATCH] D97550: [LLD][ELF][ARM] Refactor inBranchRange to use addend for PC Bias

[Peter Smith via Phabricator via llvm-commits]

peter.smith marked an inline comment as done.
peter.smith added a comment.

Thanks, I'll upload another patch.

I can write a test case for thunk reuse. The linux kernel problem is complicated as it needs several passes to populate the table in such a way that an error message will be produced:
pass 0:

- Add thunks round 1 (all thunks are added with rel.addend + getPCBias(rel.type) = 0

pass N:

- normalizeExistingThunk invalidates some calls to thunks, their rel.addend is set to 0
- Thunks are added again with rel.addend + getPCBias(rel.type) = -8. By definition no match with any existing thunk as key addend is -8 and all previous thunks will have key addend 0.

pass N + M:

- normalizeExistingThunk invalidates some calls to thunks, their rel.addend is set to 0
- One of the invalidated calls matches an existing thunk as we now have existing thunks added with key addend -8 (from normalizeExistingThunks), we match one of these existing thunks yet we can be up to 8 bytes out of range due to the rel.addend being 0 in the range check.



================
Comment at: lld/ELF/Relocations.cpp:1937
+  // Compensate for the different ARM/Thumb PC bias so we can reuse more thunks.
+  int64_t keyAddend = rel.addend + getPCBias(rel.type);
 
----------------
MaskRay wrote:
> MaskRay wrote:
> > This probably needs a test (similar to D70637) to test sharing, even if it is difficult to construct a test exercising the range limit.
> Perhaps the comment can say that `keyAddend` is usually 0, even on ARM.
I've made a specific test case that tests this line. There is an existing test that fails if the getPCBias(rel.type) is removed but it is in the middle of a larger test that is more difficult to diagnose the problem.



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

https://reviews.llvm.org/D97550