[cfe-commits] [PATCH] Fix support for naked C/C++ functions writing to unexpected memory regions in debug builds

[Eli Friedman]

2012/11/8 Jeffrey Lim <jeff at lim.com.au>:
>
>
> Hi Eli (or others),
>
> If you could kindly have a look at this updated proposed patch to correct
> the errors.
>
> The current svn version (r167575) has an additional problem with naked
> functions than the version released with Xcode due to r165914 which inserts
> llvm.trap and unreachable annotations in debug builds in naked functions
> that need to return a value.
>
> test.cpp:
> class MyClass
> {
> public:
> int NakedTest(int value, int value2);
> };
>
> __attribute((noinline, naked)) int MyClass::NakedTest(int value, int value2)
> {
> asm("");
> }
>
> clang -S -flto test.cpp
> test.s:
>
> ; ModuleID = 'test.cpp'
> target datalayout =
> "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128-n8:16:32:64-S128"
> target triple = "x86_64-apple-macosx10.8.0"
>
> %class.MyClass = type { i8 }
>
> define i32 @_ZN7MyClass9NakedTestEii(%class.MyClass* %this, i32 %value, i32
> %value2) nounwind uwtable noinline ssp naked align 2 {
> entry:
>   %retval = alloca i32, align 4
>   %this.addr = alloca %class.MyClass*, align 8
>   %value.addr = alloca i32, align 4
>   %value2.addr = alloca i32, align 4
>   store %class.MyClass* %this, %class.MyClass** %this.addr, align 8
>   store i32 %value, i32* %value.addr, align 4
>   store i32 %value2, i32* %value2.addr, align 4
>   %this1 = load %class.MyClass** %this.addr
>   call void asm sideeffect "", "~{dirflag},~{fpsr},~{flags}"() nounwind,
> !srcloc !0
>   call void @llvm.trap()
>   unreachable
>
> return:                                           ; No predecessors!
>   %0 = load i32* %retval
>   ret i32 %0
> }
>
> declare void @llvm.trap() noreturn nounwind
>
> !0 = metadata !{i32 154}
>
>
> The bolded lines are new compare to the clang that shipped with Xcode 4.5.2
>
> I've added a separate path for naked functions in GenerateCode, and avoid
> the generation of trap/unreachable for undefined return paths for naked
> functions.
>
> I'm also not sure whether I'm doing the right thing with return types -- if
> the function is naked, then there can't be a value around for it to return,
> so I've forced it to be void. This then fails the Verifier for return types,
> so I've attached a llvm.naked.patch which bypasses the return type check for
> naked functions.
>
> I've left a bunch of commented out code in StartNakedFunction( ) for review
> by someone more knowledgeable than me because I'm not particularly confident
> my understanding of what should be there or not is adequate. Ideally all of
> this commented section would be stripped before submitting.
>
> I've also run "make clang-test" before submitting these patches - hopefully
> that means I haven't messed things up too bad :)

Please split the change to factor out
GenerateSubprogramDebugDescriptor and GenerateStartFunctionBlocks into
a separate patch.

I'm assuming you're leaving in the commented-out lines for review
purposes?  We wouldn't want to commit something like that.

+// Naked functions cannot be inline

Yes.

+// OpenCL doesn't support naked functions

Yes.

+// I'm guessing we shouldn't emit instrumentation functions when we
don't even know if we have a standard stack frame

Well, even if the function does have a standard stack frame, we would
be emitting this call before we built the stack frame.  Fine.

+// Naked functions have no prolog

Yes.

+// Class member naked functions also have no prolog, nor should they

Yes.

+// I don't think this is necessary for naked functions either
+//  // If any of the arguments have a variably modified type, make sure to
+//  // emit the type size.

Yes.

-  unsigned N = RI.getNumOperands();
-  if (F->getReturnType()->isVoidTy())
-    Assert2(N == 0,
-            "Found return instr that returns non-void in Function of void "
-            "return type!", &RI, F->getReturnType());
-  else
-    Assert2(N == 1 && F->getReturnType() == RI.getOperand(0)->getType(),
-            "Function return type does not match operand "
-            "type of return inst!", &RI, F->getReturnType());
+  if(!F->isNaked())
+  {
+    unsigned N = RI.getNumOperands();
+    if (F->getReturnType()->isVoidTy())
+      Assert2(N == 0,
+              "Found return instr that returns non-void in Function of void "
+              "return type!", &RI, F->getReturnType());
+    else
+      Assert2(N == 1 && F->getReturnType() == RI.getOperand(0)->getType(),
+              "Function return type does not match operand "
+              "type of return inst!", &RI, F->getReturnType());
+  }

I don't think this is acceptable; this is likely to confuse the
optimizers.  This really needs some design discussion; please bring it
up on llvmdev.

-Eli