Page MenuHomePhabricator

[AArch64] Better errors for out-of-range fixups
ClosedPublic

Authored by olista01 on Mar 23 2016, 9:32 AM.

Details

Summary

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.

Diff Detail

Repository
rL LLVM

Event Timeline

olista01 updated this revision to Diff 51434.Mar 23 2016, 9:32 AM
olista01 retitled this revision from to [AArch64] Better errors for out-of-range fixups.
olista01 updated this object.
olista01 added reviewers: t.p.northover, rengolin.
olista01 set the repository for this revision to rL LLVM.
olista01 added a subscriber: llvm-commits.
rengolin added inline comments.Mar 23 2016, 9:36 AM
lib/Target/AArch64/MCTargetDesc/AArch64AsmBackend.cpp
271 ↗(On Diff #51434)

As it is, in through this call, none of the errors will be caught or reported.

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.

cheers,
--renato

t.p.northover accepted this revision.Mar 31 2016, 12:56 PM
t.p.northover edited edge metadata.

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!

Tim.

This revision is now accepted and ready to land.Mar 31 2016, 12:56 PM
This revision was automatically updated to reflect the committed changes.
asb added a subscriber: asb.EditedFeb 22 2017, 7:46 AM

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.