Issue 5630065: Implement the GET_CALLER_PC macro, stub the GET_CURRENT_FRAME macro (Closed)
|
Created:
13 years, 1 month ago by timurrrr_at_google_com Modified:
12 years, 11 months ago CC:
llvm-commits_cs.uiuc.edu Visibility:
Public. |
DescriptionImplement the GET_CALLER_PC macro, stub the GET_CURRENT_FRAME macro
Committed as http://llvm.org/viewvc/llvm-project?view=rev&revision=149994
Patch Set 1 #
Total comments: 8
Patch Set 2 : . #Patch Set 3 : avoided #include <intrin.h> #Patch Set 4 : # pragma instead of #pragma #MessagesTotal messages: 9
Hi Kostya, Can you please review this small patch? Regarding the GET_CURRENT_FRAME - see the other e-mail in your inbox; we can un-stub it later if we decide it's needed. Thanks, Timur http://codereview.appspot.com/5630065/diff/1/lib/asan/asan_internal.h File lib/asan/asan_internal.h (right): http://codereview.appspot.com/5630065/diff/1/lib/asan/asan_internal.h#newcode218 lib/asan/asan_internal.h:218: (uintptr_t)__builtin_extract_return_address(__builtin_return_address(0)) Looks like the __builtin_return_address macro was slightly misused See http://gcc.gnu.org/onlinedocs/gcc/Return-Address.html
Sign in to reply to this message.
http://codereview.appspot.com/5630065/diff/1/lib/asan/asan_internal.h File lib/asan/asan_internal.h (right): http://codereview.appspot.com/5630065/diff/1/lib/asan/asan_internal.h#newcode218 lib/asan/asan_internal.h:218: (uintptr_t)__builtin_extract_return_address(__builtin_return_address(0)) On 2012/02/06 13:49:02, timurrrr_at_google_com wrote: > Looks like the __builtin_return_address macro was slightly misused > > See http://gcc.gnu.org/onlinedocs/gcc/Return-Address.html This seems to be ok on x86 and ARM.
Sign in to reply to this message.
http://codereview.appspot.com/5630065/diff/1/lib/asan/asan_internal.h File lib/asan/asan_internal.h (right): http://codereview.appspot.com/5630065/diff/1/lib/asan/asan_internal.h#newcode24 lib/asan/asan_internal.h:24: # include <intrin.h> Can we avoid this include in the header? ( I am not sure we can, just asking) http://codereview.appspot.com/5630065/diff/1/lib/asan/asan_internal.h#newcode218 lib/asan/asan_internal.h:218: (uintptr_t)__builtin_extract_return_address(__builtin_return_address(0)) I'd keep this as as, it works. http://codereview.appspot.com/5630065/diff/1/lib/asan/asan_internal.h#newcode222 lib/asan/asan_internal.h:222: // TODO(timurrrr): We don't unwind on Windows - do we need this? What do you mean "we don't unwind"? Also, better use FIXME instead of TODO(user)
Sign in to reply to this message.
http://codereview.appspot.com/5630065/diff/1/lib/asan/asan_internal.h File lib/asan/asan_internal.h (right): http://codereview.appspot.com/5630065/diff/1/lib/asan/asan_internal.h#newcode24 lib/asan/asan_internal.h:24: # include <intrin.h> On 2012/02/06 19:18:31, kcc wrote: > Can we avoid this include in the header? > ( I am not sure we can, just asking) I doubt so. Or we'll need to #include it in every file which uses GET_CURRENT_PC or GET_BP_PC_SP. Following the "include what you use" rule, we have to include here. http://codereview.appspot.com/5630065/diff/1/lib/asan/asan_internal.h#newcode218 lib/asan/asan_internal.h:218: (uintptr_t)__builtin_extract_return_address(__builtin_return_address(0)) The other one works too, but up to you (reverted) On 2012/02/06 19:18:31, kcc wrote: > I'd keep this as as, it works. http://codereview.appspot.com/5630065/diff/1/lib/asan/asan_internal.h#newcode222 lib/asan/asan_internal.h:222: // TODO(timurrrr): We don't unwind on Windows - do we need this? On 2012/02/06 19:18:31, kcc wrote: > What do you mean "we don't unwind"? > Also, better use FIXME instead of TODO(user) Done.
Sign in to reply to this message.
On Tue, Feb 7, 2012 at 4:51 AM, <timurrrr@google.com> wrote: > > http://codereview.appspot.com/**5630065/diff/1/lib/asan/asan_**internal.h<htt... > File lib/asan/asan_internal.h (right): > > http://codereview.appspot.com/**5630065/diff/1/lib/asan/asan_** > internal.h#newcode24<http://codereview.appspot.com/5630065/diff/1/lib/asan/asan_internal.h#newcode24> > lib/asan/asan_internal.h:24: # include <intrin.h> > On 2012/02/06 19:18:31, kcc wrote: > >> Can we avoid this include in the header? >> ( I am not sure we can, just asking) >> > I doubt so. > Or we'll need to #include it in every file which uses GET_CURRENT_PC or > GET_BP_PC_SP. > Maybe you can find how this thing is defined in intrin.h and use similar definition? --kcc
Sign in to reply to this message.
PTAL, though personally I don't like how it looks now. On Tue, Feb 7, 2012 at 9:36 PM, Kostya Serebryany <kcc@google.com> wrote: > > > On Tue, Feb 7, 2012 at 4:51 AM, <timurrrr@google.com> wrote: >> >> >> http://codereview.appspot.com/5630065/diff/1/lib/asan/asan_internal.h >> File lib/asan/asan_internal.h (right): >> >> >> http://codereview.appspot.com/5630065/diff/1/lib/asan/asan_internal.h#newcode24 >> lib/asan/asan_internal.h:24: # include <intrin.h> >> On 2012/02/06 19:18:31, kcc wrote: >>> >>> Can we avoid this include in the header? >>> ( I am not sure we can, just asking) >> >> I doubt so. >> Or we'll need to #include it in every file which uses GET_CURRENT_PC or >> GET_BP_PC_SP. > > > Maybe you can find how this thing is defined in intrin.h and use similar > definition? > > > --kcc
Sign in to reply to this message.
Much better. Now, how about using _AddressOfReturnAddress() for the frame? --kcc On Tue, Feb 7, 2012 at 9:56 AM, Timur Iskhodzhanov <timurrrr@google.com>wrote: > PTAL, > though personally I don't like how it looks now. > > On Tue, Feb 7, 2012 at 9:36 PM, Kostya Serebryany <kcc@google.com> wrote: > > > > > > On Tue, Feb 7, 2012 at 4:51 AM, <timurrrr@google.com> wrote: > >> > >> > >> http://codereview.appspot.com/5630065/diff/1/lib/asan/asan_internal.h > >> File lib/asan/asan_internal.h (right): > >> > >> > >> > http://codereview.appspot.com/5630065/diff/1/lib/asan/asan_internal.h#newcode24 > >> lib/asan/asan_internal.h:24: # include <intrin.h> > >> On 2012/02/06 19:18:31, kcc wrote: > >>> > >>> Can we avoid this include in the header? > >>> ( I am not sure we can, just asking) > >> > >> I doubt so. > >> Or we'll need to #include it in every file which uses GET_CURRENT_PC or > >> GET_BP_PC_SP. > > > > > > Maybe you can find how this thing is defined in intrin.h and use similar > > definition? > > > > > > --kcc >
Sign in to reply to this message.
On Tue, Feb 7, 2012 at 10:05 PM, Kostya Serebryany <kcc@google.com> wrote: > Much better. > Now, how about using _AddressOfReturnAddress() for the frame? This will be a separate patch, see the reasoning below. On Windows, the value of GET_CURRENT_FRAME is only used when printing ASan reports and it is named "bp=". (we use CaptureStackBackTrace to get stack traces now, it doesn't require to know bp) _AddressOfReturnAddress returns "ebp+4" on x86, which isn't a problem; (but this might not be true for all the calling conventions? need to check) On x64 however rbp is not used making function calls (tested on helloworld-like app calling foo(), rbp==0!) and _AddressOfReturnAddress actually returns rsp+40 (which is likely to always stay true as there's only one calling convention on Win x64) Also, it's not clear if we need to print ebp at all on Windows in the ASan reports. > --kcc > > > On Tue, Feb 7, 2012 at 9:56 AM, Timur Iskhodzhanov <timurrrr@google.com> > wrote: >> >> PTAL, >> though personally I don't like how it looks now. >> >> On Tue, Feb 7, 2012 at 9:36 PM, Kostya Serebryany <kcc@google.com> wrote: >> > >> > >> > On Tue, Feb 7, 2012 at 4:51 AM, <timurrrr@google.com> wrote: >> >> >> >> >> >> http://codereview.appspot.com/5630065/diff/1/lib/asan/asan_internal.h >> >> File lib/asan/asan_internal.h (right): >> >> >> >> >> >> >> >> http://codereview.appspot.com/5630065/diff/1/lib/asan/asan_internal.h#newcode24 >> >> lib/asan/asan_internal.h:24: # include <intrin.h> >> >> On 2012/02/06 19:18:31, kcc wrote: >> >>> >> >>> Can we avoid this include in the header? >> >>> ( I am not sure we can, just asking) >> >> >> >> I doubt so. >> >> Or we'll need to #include it in every file which uses GET_CURRENT_PC or >> >> GET_BP_PC_SP. >> > >> > >> > Maybe you can find how this thing is defined in intrin.h and use similar >> > definition? >> > >> > >> > --kcc > >
Sign in to reply to this message.
Ok, r149994. thanks! On Tue, Feb 7, 2012 at 10:18 AM, Timur Iskhodzhanov <timurrrr@google.com>wrote: > On Tue, Feb 7, 2012 at 10:05 PM, Kostya Serebryany <kcc@google.com> wrote: > > Much better. > > Now, how about using _AddressOfReturnAddress() for the frame? > This will be a separate patch, see the reasoning below. > > On Windows, the value of GET_CURRENT_FRAME is only used when printing > ASan reports and it is named "bp=". > (we use CaptureStackBackTrace to get stack traces now, it doesn't > require to know bp) > CaptureStackBackTrace may appear to be prohibitively slow in the long run. But ok for now. > _AddressOfReturnAddress returns "ebp+4" on x86, which isn't a problem; > (but this might not be true for all the calling conventions? need to check) > > On x64 however rbp is not used making function calls (tested on > helloworld-like app calling foo(), rbp==0!) and _AddressOfReturnAddress > actually returns rsp+40 (which is likely to always stay true as > there's only one calling convention on Win x64) > > Also, it's not clear if we need to print ebp at all on Windows in the > ASan reports. > bp= is useful when you have weird stack-buffer-overflow reports that are hard to reason about. --kcc > > > --kcc > > > > > > On Tue, Feb 7, 2012 at 9:56 AM, Timur Iskhodzhanov <timurrrr@google.com> > > wrote: > >> > >> PTAL, > >> though personally I don't like how it looks now. > >> > >> On Tue, Feb 7, 2012 at 9:36 PM, Kostya Serebryany <kcc@google.com> > wrote: > >> > > >> > > >> > On Tue, Feb 7, 2012 at 4:51 AM, <timurrrr@google.com> wrote: > >> >> > >> >> > >> >> > http://codereview.appspot.com/5630065/diff/1/lib/asan/asan_internal.h > >> >> File lib/asan/asan_internal.h (right): > >> >> > >> >> > >> >> > >> >> > http://codereview.appspot.com/5630065/diff/1/lib/asan/asan_internal.h#newcode24 > >> >> lib/asan/asan_internal.h:24: # include <intrin.h> > >> >> On 2012/02/06 19:18:31, kcc wrote: > >> >>> > >> >>> Can we avoid this include in the header? > >> >>> ( I am not sure we can, just asking) > >> >> > >> >> I doubt so. > >> >> Or we'll need to #include it in every file which uses GET_CURRENT_PC > or > >> >> GET_BP_PC_SP. > >> > > >> > > >> > Maybe you can find how this thing is defined in intrin.h and use > similar > >> > definition? > >> > > >> > > >> > --kcc > > > > >
Sign in to reply to this message.
|
