Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(329)

Issue 5630065: Implement the GET_CALLER_PC macro, stub the GET_CURRENT_FRAME macro (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 years, 2 months ago by timurrrr_at_google_com
Modified:
2 years, 1 month ago
Reviewers:
kcc1, glider, kcc
CC:
llvm-commits_cs.uiuc.edu
Visibility:
Public.

Description

Implement 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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+9 lines, -5 lines) Patch
M lib/asan/asan_internal.h View 1 2 3 1 chunk +9 lines, -5 lines 0 comments Download

Messages

Total messages: 9
timurrrr_at_google_com
Hi Kostya, Can you please review this small patch? Regarding the GET_CURRENT_FRAME - see the ...
2 years, 2 months ago #1
glider
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 ...
2 years, 2 months ago #2
kcc
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 ...
2 years, 2 months ago #3
timurrrr_at_google_com
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: > ...
2 years, 2 months ago #4
kcc1
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<http://codereview.appspot.com/5630065/diff/1/lib/asan/asan_internal.h> > File ...
2 years, 2 months ago #5
timurrrr_at_google_com
PTAL, though personally I don't like how it looks now. On Tue, Feb 7, 2012 ...
2 years, 2 months ago #6
kcc1
Much better. Now, how about using _AddressOfReturnAddress() for the frame? --kcc On Tue, Feb 7, ...
2 years, 2 months ago #7
timurrrr_at_google_com
On Tue, Feb 7, 2012 at 10:05 PM, Kostya Serebryany <kcc@google.com> wrote: > Much better. ...
2 years, 2 months ago #8
kcc1
2 years, 2 months ago #9
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.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 1278:e6ce13d99bf5