Emit a trap instruction for IR 'unreachable'

[yaron.keren]

Summary

Patch by Vadim Chugunov.

I made a patch that fixes the "missing epilogue" problem by emitting a trap instruction for IR 'unreachable'.
A secondary use for it would be for anyone wanting to make double-sure that 'noreturn' functions, indeed, do not return.

[vadimcn]

Looks like this is missing my changes to include/llvm/Target/TargetOptions.h

[yaron.keren]

Ok, not sure why the TargetOptions.h patch didn't go in, but here it is.

[rnk]

LGTM

The patch isn't displaying correctly, but the combination of the diffs looks good. A good followup would be to wire this through Clang's CodeGenOptions.

[yaron.keren]

I didn't use Phabricator correctly, hence the diff was split into two parts.
Here is the complete diff, the combination of the preceding two.

[asl]

LGTM

[yaron.keren]

With test.

[asl]

test/CodeGen/X86/br-fold.ll

4

The test should include the explicit target triple then, right?

[vadimcn]

I would at least write it as:

; CHECK-NEXT: ud2
; CHECK-NEXT: %bb8.i329

to preserve the original intent of the test as well.

Also, I would expect to see ud2 only on x64 windows, not for other x64 OS'es. Hmm... Perhaps it would be better to change this test to use non-Windows target triple, and write another one that specifically checks 'unreachable' -> 'ud2' transform on x64 Windows?

[yaron.keren]

Updated for x64 : linux, windows, windows-gnu

[yaron.keren]

Anton & Vadim, test now three triples.

Vadim, there is no label on Windows after the patch, the output is for x64_86-pc-windows is:

.text
.def     foo;
.scl    2;
.type   32;
.endef
.globl  foo
.align  16, 0x90

foo: # @foo

BB#0: # %entry

leaq    _ZN11xercesc_2_56XMLUni16fgNotationStringE(%rip), %rax
leaq    _ZN11xercesc_2_513SchemaSymbols21fgURI_SCHEMAFORSCHEMAE(%rip), %rcx
orq     %rax, %rcx
ud2

should the label still be there?

[yaron.keren]

code was cut off, trying again:

`foo: # @foo

BB#0: # %entry

leaq    _ZN11xercesc_2_56XMLUni16fgNotationStringE(%rip), %rax
leaq    _ZN11xercesc_2_513SchemaSymbols21fgURI_SCHEMAFORSCHEMAE(%rip), %rcx
orq     %rax, %rcx
ud2

`

[yaron.keren]

foo:                                    # @foo
# BB#0:                                 # %entry
        leaq    _ZN11xercesc_2_56XMLUni16fgNotationStringE(%rip), %rax
        leaq    _ZN11xercesc_2_513SchemaSymbols21fgURI_SCHEMAFORSCHEMAE(%rip), %rcx
        orq     %rax, %rcx
        ud2

ok, this time looks good. Anyhow, ud2 is the last instruction.

[asl]

LGTM

[yaron.keren]

Added a test for x86_64-apple-darwin and committed in r206684.

[vadimcn]

Thanks for seeing this through!

include/llvm/Target/TargetOptions.h

           EnableFastISel(false), PositionIndependentExecutable(false),
           UseInitArray(false),
           DisableIntegratedAS(false), CompressDebugSections(false),
+          TrapUnreachable(false),
           TrapFuncName(""), FloatABIType(FloatABI::Default),
           AllowFPOpFusion(FPOpFusion::Standard) {}
 

     /// Compress DWARF debug sections.
     unsigned CompressDebugSections : 1;
 
+    /// Emit target-specific trap instruction for 'unreachable' IR instructions.
+    unsigned TrapUnreachable : 1;
+
     /// getTrapFunctionName - If this returns a non-empty string, this means
     /// isel should lower Intrinsic::trap to a call to the specified function
     /// name instead of an ISD::TRAP node.

     ARE_EQUAL(EnableFastISel) &&
     ARE_EQUAL(PositionIndependentExecutable) &&
     ARE_EQUAL(UseInitArray) &&
+    ARE_EQUAL(TrapUnreachable) &&
     ARE_EQUAL(TrapFuncName) &&
     ARE_EQUAL(FloatABIType) &&
     ARE_EQUAL(AllowFPOpFusion);

lib/CodeGen/SelectionDAG/FastISel.cpp

   }
 
   case Instruction::Unreachable:
-    // Nothing to emit.
-    return true;
+    if (TM.Options.TrapUnreachable)
+      return FastEmit_(MVT::Other, MVT::Other, ISD::TRAP) != 0;
+    else
+      return true;
 
   case Instruction::Alloca:
     // FunctionLowering has the static-sized case covered.

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.h

   void visitBr(const BranchInst &I);
   void visitSwitch(const SwitchInst &I);
   void visitIndirectBr(const IndirectBrInst &I);
-  void visitUnreachable(const UnreachableInst &I) { /* noop */ }
+  void visitUnreachable(const UnreachableInst &I);
 
   // Helpers for visitSwitch
   bool handleSmallSwitchRange(CaseRec& CR,

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

                           getValue(I.getAddress())));
 }
 
+void SelectionDAGBuilder::visitUnreachable(const UnreachableInst &I) {
+  if (DAG.getTarget().Options.TrapUnreachable)
+    DAG.setRoot(DAG.getNode(ISD::TRAP, getCurSDLoc(), MVT::Other, DAG.getRoot()));
+}
+
 void SelectionDAGBuilder::visitFSub(const User &I) {
   // -0.0 - X --> fneg
   Type *Ty = I.getType();

lib/Target/X86/X86TargetMachine.cpp

   if (Options.FloatABIType == FloatABI::Default)
     this->Options.FloatABIType = FloatABI::Hard;
 
+  // Windows stack unwinder gets confused when execution flow "falls through"
+  // after a call to 'noreturn' function.
+  // To prevent that, we emit a trap for 'unreachable' IR instructions.
+  // (which on X86, happens to be the 'ud2' instruction)
+  if (Subtarget.isTargetWin64())
+    this->Options.TrapUnreachable = true;
+
   initAsmInfo();
 }
 

test/CodeGen/X86/br-fold.ll

 ; RUN: llc -march=x86-64 < %s | FileCheck %s
 
 ; CHECK: orq
-; CHECK-NEXT: %bb8.i329
+; CHECK-NEXT: ud2
 
 @_ZN11xercesc_2_513SchemaSymbols21fgURI_SCHEMAFORSCHEMAE = external constant [33 x i16], align 32 ; <[33 x i16]*> [#uses=1]
 @_ZN11xercesc_2_56XMLUni16fgNotationStringE = external constant [9 x i16], align 16 ; <[9 x i16]*> [#uses=1]