Page MenuHomePhabricator

PPC: Verify that branch fixups fit within the range.
AcceptedPublic

Authored by iteratee on May 25 2017, 4:04 PM.

Details

Reviewers
kbarton
Summary

Verify that conditional and unconditional branch offsets fit within the
maximal range available in the instruction. Catches the bug that needed
rL303904

Diff Detail

Event Timeline

iteratee created this revision.May 25 2017, 4:04 PM

I'm fairly certain these are right, but I wanted to get another set of eyes on them before I committed them.

Haven't checked the numbers yet, but perhaps assert?

lib/Target/PowerPC/MCTargetDesc/PPCAsmBackend.cpp
39

Assert instead?

inouehrs added inline comments.
lib/Target/PowerPC/MCTargetDesc/PPCAsmBackend.cpp
40

You need suffix UL or ULL for a 64-bit literal?
Also I feel signed comparison is easier to read than unsigned comparison with a very large literal.

44

You need a cast here; 'Value >= 0' is always true.

53

Ditto. 'Value >= 0' is always true.

iteratee updated this revision to Diff 100730.May 30 2017, 10:21 AM

Tidy up comparisons.

iteratee added inline comments.May 30 2017, 10:22 AM
lib/Target/PowerPC/MCTargetDesc/PPCAsmBackend.cpp
40

Thanks for the suggestion. Negating one of the operands make the comparison symmetric and much clearer.

44

Or I just don't need it. ;-)

kbarton added inline comments.Jun 5 2017, 10:35 AM
lib/Target/PowerPC/MCTargetDesc/PPCAsmBackend.cpp
44

Why an assert here vs llvm_unreachable?
If we build without asserts, this won't fail and we'll most likely generate incorrect code.

There are two different perspectives here of assert vs unreachable vs other things.

In general you want unreachable if the code is unreachable and not a bad value - the latter you want asserts.

In this case one thing we can also do is report a fatal error if we get to this point as well with invalid assembly. There's not a lot of that in MC right now, but it's probably the right direction. Better errors would be good, but would involve some reworking of the MC infrastructure to pass around an error context and might not be worth it at the moment.

iteratee updated this revision to Diff 104951.Jun 30 2017, 3:59 PM
iteratee set the repository for this revision to rL LLVM.

All the checks use unreachable now.

iteratee marked an inline comment as done.Jun 30 2017, 3:59 PM

Not quite what I meant, but if you want to ensure that this crashes the compiler rather than emitting bad code then this is fine. Can you also add a TODO with "this should return an error" or some such?

Thanks!

-eric

kbarton accepted this revision.Aug 22 2017, 1:40 PM

LGTM

This revision is now accepted and ready to land.Aug 22 2017, 1:40 PM
asb added a subscriber: asb.Aug 22 2017, 11:09 PM

Hi folks, I think llvm_unreachable is really the wrong error handling strategy here. It _won't_ necessarily cause the process to exit on a release build. This is actually one of the cases where it is possible to report a nice error without too much hassle. In rL299529 I added an MCContext parameter to MCAsmBackend::applyFixup in order to make it easy to use MCContext::reportError for effort reporting in helper functions like adjustFixupValue. Due to later upstream changes, you now get hold of your MCContext via the MCAssembler argument. You can see an example of this approach in action in D23568.

In D33574#849770, @asb wrote:

Hi folks, I think llvm_unreachable is really the wrong error handling strategy here. It _won't_ necessarily cause the process to exit on a release build. This is actually one of the cases where it is possible to report a nice error without too much hassle. In rL299529 I added an MCContext parameter to MCAsmBackend::applyFixup in order to make it easy to use MCContext::reportError for effort reporting in helper functions like adjustFixupValue. Due to later upstream changes, you now get hold of your MCContext via the MCAssembler argument. You can see an example of this approach in action in D23568.

I think that the test here is: is this something that a user could hit, (including with inline assembly, etc.), or does this indicate an internal problem only? For internal errors, we have lots of checks that we optimize out for release builds (and there's always the option of building release+asserts, and I deploy those builds on more experimental platforms as a general rule). If it is something that a user could hit, then it should be an error that the user will always get.

lib/Target/PowerPC/MCTargetDesc/PPCAsmBackend.cpp
44

Space after if.

asb added a comment.Aug 23 2017, 5:53 AM

I think that the test here is: is this something that a user could hit, (including with inline assembly, etc.), or does this indicate an internal problem only? For internal errors, we have lots of checks that we optimize out for release builds (and there's always the option of building release+asserts, and I deploy those builds on more experimental platforms as a general rule). If it is something that a user could hit, then it should be an error that the user will always get.

That matches my understanding of when to use report_fatal_error or mechanisms like MCContext::reportError. Unless I'm missing in the PowerPC backend, I believe invalid fixup values can be easily triggered by user input, e.g.

b distant
.space 1<<27
distant: