[PATCH] D44887: [RISCV] Add shouldForceRelocationWithApplyFixup MC AsmBackend Target Hook

[Alex Bradbury via Phabricator via llvm-commits]

asb added a comment.

Could you please expand rv32-relaxation.s and rv64-relaxation.s so the test includes the behaviour for branches to an unresolved symbol?

I'm a bit concerned about adding shouldForceRelocationWithApplyFixup. It adds complexity by adding another way of achieving something very similar to the existing shouldForceRelocation, and its use means that the return value of evaluateFixup no longer matches the description in its docstring (the value isn't 'fixed' by the definition in the docstring, but evaluateFixup still returns true).

One option is we could just say that it's not worth doing exactly what gcc does here, and leave the linker to relax a 32-bit jump. Alternatively, I think the hooks could be structured differently. As I understand it: we want fixupNeedsRelaxation to return false in cases where evaluateFixup returns false, but does so because a relocation was forced. Achieve this by:

- Modifying evaluateFixup so it takes a bool ref ('WasForced' or similar) , which indicates when the fixup was evaluated, but is not fixed due to the return value of shouldForceRelocation
- Modify the evaluateFixup doc string to document the behaviour being relied upon: the Value in the fixup is set to the correct value if WasForced is true, even if evaluateFixup returns false
- Modify the signature of fixupNeedsRelaxationAdvanced so it passes along the WasForced flag
- Implement fixupNeedsRelaxationAdvanced in the riscvbackend, so it returns false if !Resolved && WasForced

Would the above resolve the issue? If so, the question is whether it's worth implementing this behaviour or not.


Repository:
  rL LLVM

https://reviews.llvm.org/D44887