Page MenuHomePhabricator

[X86] Correct the number of bits in a fixup
AcceptedPublic

Authored by dougk on Oct 17 2016, 12:09 PM.

Details

Reviewers
niravd
Summary

The intended behavior on a too-large relative branch is an assembler crash.
We don't need to do the right thing, because the only branch that can't
be relaxed is J[ER]CXZ, and only hand-written code uses that.

Event Timeline

dougk updated this revision to Diff 74884.Oct 17 2016, 12:09 PM
dougk retitled this revision from to [X86] Correct the number of bits in a fixup.
dougk updated this object.
dougk added a reviewer: niravd.
dougk added a subscriber: llvm-commits.
niravd accepted this revision.Oct 17 2016, 12:12 PM
niravd edited edge metadata.
This revision is now accepted and ready to land.Oct 17 2016, 12:12 PM
joerg added a subscriber: joerg.Oct 17 2016, 12:15 PM

Are you sure about the summary? An assert on user-input is never the intended behavior.

We can change it from an assert to report_fatal_error, but we can't actually distinguish at this point whether it was in fact a user error versus whether it was an internal bug.
I guess I agree that dumping core is a bit sad for the user error case. Would you prefer it always to say this instead?

clang -cc1as: fatal error: error in backend: Value does not fit in fixup

Yeah, that's better. Can we easily get at least the section as well? I really hate having to debug instances like this by running gdb :)

dougk added a comment.Oct 17 2016, 1:01 PM

it looks like we need an MCAssembler on which to call getContext() and then Ctx->reportError(Fixup.getLoc(), "mumble").
Some of the other AsmBackends have started to have this plumbing added in.
However most of them actually guard the emission of the message with "if (Ctx)" because we usually don't have the context apparently, so you really can't get a nice source location anyway.
Without this change, we'll silently emit bogus code, so I think we should land it as-is, but do change it from assert to report_fatal_error. Are you ok with that?

joerg added a comment.Oct 17 2016, 1:34 PM

Sure, including the sector would be just a QoI issue.

dougk added a comment.Oct 19 2016, 4:38 PM

this changes harms the .byte,.short, etc assembler directives. niravd will take a look