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

Jeffrey Lim jeff at lim.com.au
Thu Nov 8 06:46:40 PST 2012 


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 :)





Thanks,
Jeff


On 7 Nov, 2012, at 8:18 AM, Eli Friedman <eli.friedman at gmail.com> wrote:

> 2012/11/6 Jeffrey Lim <jeff at lim.com.au>:
>> 
>> On 7 Nov, 2012, at 5:59 AM, Eli Friedman <eli.friedman at gmail.com> wrote:
>> 
>>> 2012/11/6 Jeffrey Lim <jeff at lim.com.au>:
>>>> 
>>>> First time post to this list -- I think I posted to the wrong place before
>>>> (cfe-dev). Hopefully I'm right this time.
>>>> 
>>>> The problem I was noticing is that a naked function would cause very
>>>> unexpected behaviour. I simplified it to this test case:
>>>> 
>>>> test.c:
>>>> 
>>>> __attribute__((naked)) void NakedTest(int value, int value2)
>>>> {
>>>> asm("");
>>>> }
>>>> 
>>>> 
>>>> clang -S test.cpp
>>>> test.s:
>>>> 
>>>> .section __TEXT,__text,regular,pure_instructions
>>>> .globl __Z9NakedTestii
>>>> .align 4, 0x90
>>>> __Z9NakedTestii:                        ## @_Z9NakedTestii
>>>> .cfi_startproc
>>>> ## BB#0:
>>>> movl %edi, -4(%rbp)
>>>> movl %esi, -8(%rbp)
>>>> ## InlineAsm Start
>>>> ## InlineAsm End
>>>> ret
>>>> .cfi_endproc
>>>> 
>>>> 
>>>> clang -flto -S test.cpp
>>>> test.s:
>>>> 
>>>> define void @_Z9NakedTestii(i32 %value, i32 %value2) nounwind uwtable
>>>> noinline ssp naked {
>>>> entry:
>>>> %value.addr = alloca i32, align 4
>>>> %value2.addr = alloca i32, align 4
>>>> store i32 %value, i32* %value.addr, align 4
>>>> store i32 %value2, i32* %value2.addr, align 4
>>>> call void asm sideeffect "", "~{dirflag},~{fpsr},~{flags}"() nounwind,
>>>> !srcloc !0
>>>> ret void
>>>> }
>>>> 
>>>> The bolded instructions above are generated in debug builds, causing
>>>> problems. This happens in all architectures that I've tested on (ARM, x86,
>>>> x64)
>>>> 
>>>> 
>>>> I've attached a patch file which I fixes the problem. I don't know if I've
>>>> done it the right way (I only downloaded and looked at the source for clang
>>>> for the first time 2 hours ago), but here's the newer result:
>>>> 
>>>> clang -S test.cpp
>>>> test.s:
>>>> .section __TEXT,__text,regular,pure_instructions
>>>> .globl __Z9NakedTestii
>>>> .align 4, 0x90
>>>> __Z9NakedTestii:                        ## @_Z9NakedTestii
>>>> .cfi_startproc
>>>> ## BB#0:                                ## %entry
>>>> ## InlineAsm Start
>>>> ## InlineAsm End
>>>> ret
>>>> .cfi_endproc
>>>> 
>>>> 
>>>> clang -flto -S test.cpp
>>>> test.s:
>>>> 
>>>> define void @_Z9NakedTestii(i32, i32) nounwind uwtable noinline ssp naked {
>>>> entry:
>>>> call void asm sideeffect "", "~{dirflag},~{fpsr},~{flags}"() nounwind,
>>>> !srcloc !0
>>>> ret void
>>>> }
>>>> 
>>>> Any comments, or alternative fixes to this problem would be greatly
>>>> appreciated!
>>> 
>>> I would like to see a more principled approach here... scattering
>>> checks for the naked attribute all over the function emission code
>>> seems like a bad idea.  (clang with just your patch doesn't handle,
>>> for example, the case where the return type is not void consistently,
>>> the case where the function in question is a member function, etc.)
>>> 
>>> -Eli
>> 
>> Thanks for the feedback -- you're absolutely right about the fail cases.
>> 
>> Obviously I could add in even more checks in the function emission code, but do you have any thoughts on how the right way to address this might look like? I'd love to fix it in the proper way with a little guidance.
> 
> I would suggest adding a special case to CodeGenFunction::GenerateCode
> which does exactly what is necessary for naked functions.
> 
> -Eli

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20121108/e36fa408/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: llvm.naked.patch
Type: application/octet-stream
Size: 1819 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20121108/e36fa408/attachment.obj>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20121108/e36fa408/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: clang.naked.patch
Type: application/octet-stream
Size: 18231 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20121108/e36fa408/attachment-0001.obj>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20121108/e36fa408/attachment-0002.html>

More information about the cfe-commits mailing list