When a fixup that can be resolved by the assembler is out of range, we should report an error in the source, rather than crashing.
adjustFixupValue will always be called twice, first from processFixupValue and then from applyFixup. The first one will report the errors (because the MCContext is available there), so the second one does not have to. The ARM version of this code works the exact same way.
Sorry, this fell out of my radar. How can you guarantee the fixup and the value are the exact same? What if some process (even if later on), does change it?
I think if the ARM one works in the same way, then both are wrong...
I'll defer to Tim and James to accept this, as I'm not comfortable with the idea.
Reporting an error is better than crashing, but crashing is better than silent codegen errors.
I think this is a good idea. MachO has been doing similar things for a while and I don't think we've had any complaints about misplaced diagnostics. We certainly used to get complaints about ones with missing locations though!
Hi all. Sorry for digging up an old review, but I've been looking at reporting fixup overflow errors for RISC-V, and this seems the most appropriate place to discuss how it's currently handled in AArch64. I agree with @rengolin's concerns with this approach - or at least, I think we can do this better. Right now, adjustFixupValue is always called twice. The second time it's called, the MCContext argument is null and the code relies on the fact that any errors should have been caught the first time round, and so this null pointer will never be dereferenced [EDIT: sorry, that's not true - it does consistently check that Ctx is non-null]. I like having error checking alongside the value adjustments, but I propose that instead we achieve this by:
- Adding an MCContext argument to MCAsmBackend::applyFixup, and document that this function should report any errors in applying the fixup (should as the value being out of range).. This just requires a simple update to MCAssembler.cpp so it passes the context argument, and of course modifying FootgtAsmBackend for all targets to reflect the new argument
- For backends that report errors using adjustFixupValue, stop calling it from processFixupValue and instead just modify the applyFixup implementation to pass an MCContext rather than a nullptr
I just thought I'd quickly check in case I'm missing something obviously wrong with this approach.