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

Thu, 08 Nov 2012 06:48:50 -0800



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


Attachment: llvm.naked.patch
Description: Binary data

Attachment: clang.naked.patch
Description: Binary data



Thanks,
Jeff


On 7 Nov, 2012, at 8:18 AM, Eli Friedman <[email protected]> wrote:

2012/11/6 Jeffrey Lim <[email protected]>:

On 7 Nov, 2012, at 5:59 AM, Eli Friedman <[email protected]> wrote:

2012/11/6 Jeffrey Lim <[email protected]>:

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

_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

Reply via email to