PPC: Verify that branch fixups fit within the range.

[iteratee]

Summary

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

[iteratee]

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

[echristo]

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

lib/Target/PowerPC/MCTargetDesc/PPCAsmBackend.cpp

39

Assert instead?

[inouehrs]

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]

Tidy up comparisons.

[iteratee]

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]

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.

[echristo]

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]

All the checks use unreachable now.

[echristo]

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]

LGTM

[asb]

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.

[hfinkel]

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]

In D33574#850019, @hfinkel wrote:

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:

lib/Target/PowerPC/MCTargetDesc/PPCAsmBackend.cpp

default:
     llvm_unreachable("Unknown fixup kind!");
   case FK_Data_1:
   case FK_Data_2:
   case FK_Data_4:
   case FK_Data_8:
   case PPC::fixup_ppc_nofixup:
     return Value;
   case PPC::fixup_ppc_brcond14:
+    if (static_cast<int64_t>(Value) >= 0 && Value > 0x7fff)
+      llvm_unreachable("Cond branch target overflow.");
+    else if (static_cast<int64_t>(Value) < 0 && -Value > 0x8000)
+      llvm_unreachable("Cond branch target underflow.");
+    return Value & 0xfffc;
   case PPC::fixup_ppc_brcond14abs:
+    assert(Value <= 0xffff && "Cond branch absolute target overflow.");
     return Value & 0xfffc;
   case PPC::fixup_ppc_br24:
+    if (static_cast<int64_t>(Value) >= 0 && Value > 0x1ffffff)
+      llvm_unreachable("Branch target overflow.");
+    else if (static_cast<int64_t>(Value) < 0 && -Value > 0x2000000)
+      llvm_unreachable("Branch target underflow.");
+    return Value & 0x3fffffc;
   case PPC::fixup_ppc_br24abs:
+    assert(Value <= 0x3ffffff && "Branch absolute target overflow.");
     return Value & 0x3fffffc;
   case PPC::fixup_ppc_half16:
     return Value & 0xffff;
   case PPC::fixup_ppc_half16ds:
     return Value & 0xfffc;
   }
 }