Fix for crash during SjLj preparation step

[vhbit]

Summary

This crash was pretty common while compiling Rust for iOS (armv7). Reason - SjLj preparation step was lowering aggregate arguments as ExtractValue + InsertValue. ExtractValue has assertion which checks that there is some data in value, which is not true in case of empty (no fields) structures. Rust uses them quite extensively so this patch skips ExtractValue + InsertValue in case of empty struct or empty array.

[vhbit]

Fixes http://llvm.org/bugs/show_bug.cgi?id=19855

[void]

This is crying out for a testcase. Can you supply one?

lib/CodeGen/SjLjEHPrepare.cpp

255

This comment should go inside of the 'if (ST || AT) {' block. Also, no need to mention specific languages. Something like this:

An aggregate type with no fields (e.g., an empty struct) cannot have
extract/insert instructions applied to it. In this case, there is no need to
// transfer anything.

258

I'm worried about doing this. The purpose of this function is to lower the arguments so that we don't have to handle them with special code. If we do this, we are now allowing the empty aggregate to be live outside of the entry block. Actually, this whole code seems suspect to me. One way around both the original problem and your problem would be to use another instruction that would satisfy both. This one is off the top of my head:

define @foo([i8, i32] %a) {

%new_a = select i1 true, [i8, i32] %a, [i8, i32] null
...

}

Could you convert this block of code to do that and see if it still works?

[vhbit]

Added a test case and used 'select' as Bill proposed.

Still need advice if it is possible to unify both aggregate type branches into one, as I'm not sure if there is a combination of codegen/optimization flags which might not eliminate loading from null.

[void]

In D4256#10, @vhbit wrote:

Still need advice if it is possible to unify both aggregate type branches into one, as I'm not sure if there is a combination of codegen/optimization flags which might not eliminate loading from null.

If there is a combination of flags that will result in a load from 'null', then it's a bug and will need to be fixed. The best way to guard against something like this is to create tests to make sure that the expected ASM is emitted.

I would modify the test you added here so that it checks for the expected ARM ASM once it runs through llc. Also, expand the testcase a bit, adding optimizations, etc. You won't be able to check all combinations of flags, nor should you need to. But at least check at every llc '-O' level.

test/CodeGen/Thumb/sjljehprepare-lower-empty-struct.ll

1 ↗

(On Diff #11076)

Please put this in test/CodeGen/ARM. It's not thumb-specific.

5 ↗

(On Diff #11076)

I don't think you need the 'target *' lines here.

7 ↗

(On Diff #11076)

The arguments aren't used in the code. Could you modify the testcase so that they are used outside of the entry basic block?

[vhbit]

Cleaned up test file, moved to ARM, added checks for all optimization levels.

[void]

lib/CodeGen/SjLjEHPrepare.cpp

277

You don't need to do this. Just use the 'null' value directly. :-)

[void]

I think there's some misunderstanding of what I'd like to see here. (I confused things by using 'null' for the example.) This is the type of code I was thinking of:

def void @foo({} %c) {
    %fake_c = select i1 true, {} %c, {} undef
    ; subsequent uses of %fake_c in the code.

This should be valid for all types, whether they are aggregates or not. So you can replace the whole of that code with just this.

[vhbit]

Switched to using undef => code simplification and unification of 2 branches

[void]

lib/CodeGen/SjLjEHPrepare.cpp

255

Nit: You don't need the line break before this sentence. :-)

258

Watch out for 80-column violations here.

267

Now that we have the above code, I think we can use it for this situation as well. However, you can do that as a separate commit.

[vhbit]

Fixed too wide line.
Yep, I think that a complete unification should be done in a separate commit

[Eugene.Zelenko]

Committed in rL212922.

lib/CodeGen/SjLjEHPrepare.cpp

void SjLjEHPrepare::lowerIncomingArguments(Function &F) {
   while (isa<AllocaInst>(AfterAllocaInsPt) &&
          isa<ConstantInt>(cast<AllocaInst>(AfterAllocaInsPt)->getArraySize()))
     ++AfterAllocaInsPt;
 
   for (Function::arg_iterator AI = F.arg_begin(), AE = F.arg_end(); AI != AE;
        ++AI) {
     Type *Ty = AI->getType();
 
-    // Aggregate types can't be cast, but are legal argument types, so we have
-    // to handle them differently. We use an extract/insert pair as a
-    // lightweight method to achieve the same goal.
     if (isa<StructType>(Ty) || isa<ArrayType>(Ty)) {
-      Instruction *EI = ExtractValueInst::Create(AI, 0, "", AfterAllocaInsPt);
-      Instruction *NI = InsertValueInst::Create(AI, EI, 0);
-      NI->insertAfter(EI);
-      AI->replaceAllUsesWith(NI);
-
-      // Set the operand of the instructions back to the AllocaInst.
-      EI->setOperand(0, AI);
-      NI->setOperand(0, AI);
+      // Aggregate types can't be cast, but are legal argument types,
+      // so we have to handle them differently.  We use
+      // select i8 true, %arg, undef to achieve the same goal
+      Value *TrueValue = ConstantInt::getTrue(F.getContext());
+      Value *UndefValue = UndefValue::get(Ty);
+      Instruction *SI = SelectInst::Create(TrueValue, AI, UndefValue,
+                                           AI->getName() + ".tmp",
+                                           AfterAllocaInsPt);
+      AI->replaceAllUsesWith(SI);
+
+      SI->setOperand(1, AI);
     } else {
       // This is always a no-op cast because we're casting AI to AI->getType()
       // so src and destination types are identical. BitCast is the only
       // possibility.
       CastInst *NC = new BitCastInst(AI, AI->getType(), AI->getName() + ".tmp",
                                      AfterAllocaInsPt);
       AI->replaceAllUsesWith(NC);
 
       // Set the operand of the cast instruction back to the AllocaInst.
       // Normally it's forbidden to replace a CastInst's operand because it
       // could cause the opcode to reflect an illegal conversion. However, we're
       // replacing it here with the same value it was constructed with.  We do
       // this because the above replaceAllUsesWith() clobbered the operand, but
       // we want this one to remain.
       NC->setOperand(0, AI);
     }
   }
 }
 
 /// lowerAcrossUnwindEdges - Find all variables which are alive across an unwind
 /// edge and spill them.
 void SjLjEHPrepare::lowerAcrossUnwindEdges(Function &F,

test/CodeGen/ARM/sjljehprepare-lower-empty-struct.ll

+; RUN: llc -mtriple=armv7-apple-ios -O0 < %s | FileCheck %s
+; RUN: llc -mtriple=armv7-apple-ios -O1 < %s | FileCheck %s
+; RUN: llc -mtriple=armv7-apple-ios -O2 < %s | FileCheck %s
+; RUN: llc -mtriple=armv7-apple-ios -O3 < %s | FileCheck %s
+
+; SjLjEHPrepare shouldn't crash when lowering empty structs.
+;
+; Checks that between in case of empty structs used as arguments
+; nothing happens, i.e. there are no instructions between
+; __Unwind_SjLj_Register and actual @bar invocation
+
+
+define i8* @foo({} %c) {
+entry:
+; CHECK: bl __Unwind_SjLj_Register
+; CHECK-NEXT: {{[A-Z][a-zA-Z0-9]*}}:
+; CHECK-NEXT: bl _bar
+  invoke void @bar ()
+    to label %unreachable unwind label %handler
+
+unreachable:
+  unreachable
+
+handler:
+  %tmp = landingpad { i8*, i32 } personality i8* bitcast (i32 (...)* @baz to i8*)
+  cleanup
+  resume { i8*, i32 } undef
+}
+
+declare void @bar()
+declare i32 @baz(...)