Re: PATCH: Turn on -fomit-frame-pointer by default for 32bit Linux/x86

Re: PATCH: Turn on -fomit-frame-pointer by default for 32bit Linux/x86

[Paul Pluzhnikov]

I do not have an opinion of whether -O2 should or should not omit frame
pointers, but ...

On Mon, 2 Aug 2010 10:54:59 -0700, H.J. Lu <hjl.tools@gmail.com> wrote:

> We definitely should help our users deal with this change.
> I only propose this for Linux/x86 where backstrace in glibc
> supports unwind table. We can show them how to use
> backstrace in glibc

Unfortunately, backtrace() in glibc is not currently (glibc-2.11) usable
for getting stack traces from places that developers at Google care about;
namely from within malloc and from signal handlers.

If this patch was to go in, we'll just have to add -fno-omit-frame-pointer
for our 32-bit compiles (we already do that for 64-bit ones).

I suspect that above two "use cases" (heap operations and signal handlers)
cover vast majority of users who record stack traces, and the "just use glibc
backtrace" advice will "just not work" for them.

I can provide details, but the executive summary is that in order to allow
backtraces from malloc and from signal handlers, we use libunwind, and
even then we've had to patch glibc in some unpleasant ways, which will
not be acceptable upstream, and which (I suspect) most other users can't
afford to do.  And even after all that patching our solution is not
(yet) robust enough for general use across Google fleet, so we are using
frame-based unwinder for now.

Cheers,
-- 
Paul Pluzhnikov

Re: PATCH: Turn on -fomit-frame-pointer by default for 32bit Linux/x86

[H.J. Lu]

On Wed, Aug 4, 2010 at 11:51 AM, Paul Pluzhnikov <ppluzhnikov@google.com> wrote:
> I do not have an opinion of whether -O2 should or should not omit frame
> pointers, but ...
>
> On Mon, 2 Aug 2010 10:54:59 -0700, H.J. Lu <hjl.tools@gmail.com> wrote:
>
>> We definitely should help our users deal with this change.
>> I only propose this for Linux/x86 where backstrace in glibc
>> supports unwind table. We can show them how to use
>> backstrace in glibc
>
> Unfortunately, backtrace() in glibc is not currently (glibc-2.11) usable
> for getting stack traces from places that developers at Google care about;
> namely from within malloc and from signal handlers.
>
> If this patch was to go in, we'll just have to add -fno-omit-frame-pointer
> for our 32-bit compiles (we already do that for 64-bit ones).
>
> I suspect that above two "use cases" (heap operations and signal handlers)
> cover vast majority of users who record stack traces, and the "just use glibc
> backtrace" advice will "just not work" for them.
>
> I can provide details, but the executive summary is that in order to allow
> backtraces from malloc and from signal handlers, we use libunwind, and
> even then we've had to patch glibc in some unpleasant ways, which will
> not be acceptable upstream, and which (I suspect) most other users can't
> afford to do.  And even after all that patching our solution is not
> (yet) robust enough for general use across Google fleet, so we are using
> frame-based unwinder for now.
>

I am not sure about single handler. For malloc, are you referring to

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=24724

If it can't be fixed and is very important to you, you have to keep
frame pointer.
It isn't a new problem.


-- 
H.J.

Re: PATCH: Turn on -fomit-frame-pointer by default for 32bit Linux/x86

[Paul Pluzhnikov]

On Wed, Aug 4, 2010 at 12:13 PM, H.J. Lu <hjl.tools@gmail.com> wrote:

> For malloc, are you referring to
>
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=24724

Thanks for the pointer. This is one example, but there are many more.

> It isn't a new problem.

I didn't claim it is.

I am only claiming that glibc backtrace() is currently not a viable solution
for our use cases.

Cheers,
-- 
Paul Pluzhnikov

Re: PATCH: Turn on -fomit-frame-pointer by default for 32bit Linux/x86

[Xinliang David Li]

Paul just points out the use case in google that unfortunately
requires frame pointer (for now) at the cost of some performance.
There is no objection to the default switching as it is easy to throw
in a build option to override it.

Thanks,

David

On Wed, Aug 4, 2010 at 12:13 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Wed, Aug 4, 2010 at 11:51 AM, Paul Pluzhnikov <ppluzhnikov@google.com> wrote:
>> I do not have an opinion of whether -O2 should or should not omit frame
>> pointers, but ...
>>
>> On Mon, 2 Aug 2010 10:54:59 -0700, H.J. Lu <hjl.tools@gmail.com> wrote:
>>
>>> We definitely should help our users deal with this change.
>>> I only propose this for Linux/x86 where backstrace in glibc
>>> supports unwind table. We can show them how to use
>>> backstrace in glibc
>>
>> Unfortunately, backtrace() in glibc is not currently (glibc-2.11) usable
>> for getting stack traces from places that developers at Google care about;
>> namely from within malloc and from signal handlers.
>>
>> If this patch was to go in, we'll just have to add -fno-omit-frame-pointer
>> for our 32-bit compiles (we already do that for 64-bit ones).
>>
>> I suspect that above two "use cases" (heap operations and signal handlers)
>> cover vast majority of users who record stack traces, and the "just use glibc
>> backtrace" advice will "just not work" for them.
>>
>> I can provide details, but the executive summary is that in order to allow
>> backtraces from malloc and from signal handlers, we use libunwind, and
>> even then we've had to patch glibc in some unpleasant ways, which will
>> not be acceptable upstream, and which (I suspect) most other users can't
>> afford to do.  And even after all that patching our solution is not
>> (yet) robust enough for general use across Google fleet, so we are using
>> frame-based unwinder for now.
>>
>
> I am not sure about single handler. For malloc, are you referring to
>
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=24724
>
> If it can't be fixed and is very important to you, you have to keep
> frame pointer.
> It isn't a new problem.
>
>
> --
> H.J.
>

Re: PATCH: Turn on -fomit-frame-pointer by default for 32bit Linux/x86

[Andi Kleen]

"H.J. Lu" <hjl.tools@gmail.com> writes:

> I am not sure about single handler. For malloc, are you referring to

In Linux the kernel provides unwind information for the signal
handler. I just tested it and it works here with a frame pointer
less 32bit executable using gdb.

-Andi
-- 
ak@linux.intel.com -- Speaking for myself only.

Re: PATCH: Turn on -fomit-frame-pointer by default for 32bit Linux/x86

[David Daney]

On 08/04/2010 01:39 PM, Andi Kleen wrote:
> "H.J. Lu"<hjl.tools@gmail.com>  writes:
>
>> I am not sure about single handler. For malloc, are you referring to
>
> In Linux the kernel provides unwind information for the signal
> handler. I just tested it and it works here with a frame pointer
> less 32bit executable using gdb.

Not that I doubt your claim, but the ability of gdb to unwind signal 
frames is not proof of the existence of kernel unwind information.

gdb can often generate a backtrace (even through signal frames) in the 
absence of any debugging meta-data.

David Daney

Re: PATCH: Turn on -fomit-frame-pointer by default for 32bit Linux/x86

[Paul Pluzhnikov]

On Wed, Aug 4, 2010 at 1:57 PM, David Daney <ddaney@caviumnetworks.com> wrote:
> On 08/04/2010 01:39 PM, Andi Kleen wrote:
>>
>> "H.J. Lu"<hjl.tools@gmail.com>  writes:
>>
>>> I am not sure about single handler. For malloc, are you referring to
>>
>> In Linux the kernel provides unwind information for the signal
>> handler. I just tested it and it works here with a frame pointer
>> less 32bit executable using gdb.
>
> Not that I doubt your claim, but the ability of gdb to unwind signal frames
> is not proof of the existence of kernel unwind information.

The kernel VDSO page does in fact provide correct unwind info, but that
is completely irrelevant.

The issue here is not that the unwind info is missing; it's that glibc
backtrace() is not async-signal safe (which, given that it calls malloc
shouldn't be a big surprise :-).

Cheers,
-- 
Paul Pluzhnikov

Re: PATCH: Turn on -fomit-frame-pointer by default for 32bit Linux/x86

[Andi Kleen]

> The issue here is not that the unwind info is missing; it's that glibc
> backtrace() is not async-signal safe (which, given that it calls malloc
> shouldn't be a big surprise :-).

Are you sure? AFAIK it only calls malloc when you register frames,
but not during unwinding itself.

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only.

Re: PATCH: Turn on -fomit-frame-pointer by default for 32bit Linux/x86

[Paul Pluzhnikov]

On Wed, Aug 4, 2010 at 2:16 PM, Andi Kleen <andi@firstfloor.org> wrote:

>> The issue here is not that the unwind info is missing; it's that glibc
>> backtrace() is not async-signal safe (which, given that it calls malloc
>> shouldn't be a big surprise :-).
>
> Are you sure? AFAIK it only calls malloc when you register frames,
> but not during unwinding itself.

Yes, I double-checked this morning.

The self-deadlock sequence observed with glibc-2.11.1 when calling
backtrace() from inside TCMalloc malloc() was the same as in
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=24724:

backtrace() (first time around) calls dlopen("libgcc_s.so.1"), which
(recursively) calls malloc(), which self-deadlocks.

I am sure this particular problem could be resolved, but I am also sure
there are great many other ways in which backtrace() is not async-signal
safe.

For example, libgcc_s.so.1 _Unwind_Backtrace (which is what glibc
backtrace on x86 dispatches to) calls back into dl_iterate_phdr, which
locks loader lock => kaboom!

-- 
Paul Pluzhnikov

Re: PATCH: Turn on -fomit-frame-pointer by default for 32bit Linux/x86

[Andi Kleen]

> Yes, I double-checked this morning.

Yes I found it after the post.

> 
> The self-deadlock sequence observed with glibc-2.11.1 when calling
> backtrace() from inside TCMalloc malloc() was the same as in
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=24724:

I think it's only doing  this to avoid the sorting on initialization
time. The easy way to avoid it would be to call init_object() 
for everything early, at the cost of some start up time.

> For example, libgcc_s.so.1 _Unwind_Backtrace (which is what glibc
> backtrace on x86 dispatches to) calls back into dl_iterate_phdr, which
> locks loader lock => kaboom!

Well still -- this stuff should be all fixable and it would seem
odd to leave 4% performance on the table just to avoid fixing a few bugs
in the infrastructure.

People often go to large jumps for much smaller performance improvements.

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only.

Re: PATCH: Turn on -fomit-frame-pointer by default for 32bit Linux/x86

[David Daney]

On 08/04/2010 03:07 PM, Paul Pluzhnikov wrote:
> On Wed, Aug 4, 2010 at 2:16 PM, Andi Kleen<andi@firstfloor.org>  wrote:
>
>>> The issue here is not that the unwind info is missing; it's that glibc
>>> backtrace() is not async-signal safe (which, given that it calls malloc
>>> shouldn't be a big surprise :-).
>>
>> Are you sure? AFAIK it only calls malloc when you register frames,
>> but not during unwinding itself.
>
> Yes, I double-checked this morning.
>
> The self-deadlock sequence observed with glibc-2.11.1 when calling
> backtrace() from inside TCMalloc malloc() was the same as in
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=24724:
>
> backtrace() (first time around) calls dlopen("libgcc_s.so.1"), which
> (recursively) calls malloc(), which self-deadlocks.
>
> I am sure this particular problem could be resolved, but I am also sure
> there are great many other ways in which backtrace() is not async-signal
> safe.

Most DWARF unwinding data is not async-signal safe (i.e. it is not 
generated with -fasynchronous-unwind-tables).  So using it to generate a 
backtrace from an asynchronous signal handler wouldn't be reliable.

David Daney

Re: PATCH: Turn on -fomit-frame-pointer by default for 32bit Linux/x86

[Andi Kleen]

> Most DWARF unwinding data is not async-signal safe (i.e. it is not
> generated with -fasynchronous-unwind-tables).  So using it to
> generate a backtrace from an asynchronous signal handler wouldn't be
> reliable.

x86-64 defaults to -fasynchronous-unwind-tables. i386 when this
change was done would need too.

-Andi
-- 
ak@linux.intel.com -- Speaking for myself only.

Re: PATCH: Turn on -fomit-frame-pointer by default for 32bit Linux/x86 II

[Andi Kleen]

Never mind I see now what you mean. Looks more like a simple 
glibc bug to me.  I cannot imagine it being that difficult to fix.

-Andi
-- 
ak@linux.intel.com -- Speaking for myself only.

Re: PATCH: Turn on -fomit-frame-pointer by default for 32bit Linux/x86

[Richard Henderson]

On 08/04/2010 02:08 PM, Paul Pluzhnikov wrote:
> The issue here is not that the unwind info is missing; it's that glibc
> backtrace() is not async-signal safe (which, given that it calls malloc
> shouldn't be a big surprise :-).

It will only ever call malloc if you are missing PT_GNU_EH_FRAME headers
in your binary.  Which really ought never be true given a modern tool chain.


r~

Re: PATCH: Turn on -fomit-frame-pointer by default for 32bit Linux/x86

[Andi Kleen]

On Wed, Aug 04, 2010 at 04:11:43PM -0700, Richard Henderson wrote:
> On 08/04/2010 02:08 PM, Paul Pluzhnikov wrote:
> > The issue here is not that the unwind info is missing; it's that glibc
> > backtrace() is not async-signal safe (which, given that it calls malloc
> > shouldn't be a big surprise :-).
> 
> It will only ever call malloc if you are missing PT_GNU_EH_FRAME headers
> in your binary.  Which really ought never be true given a modern tool chain.

It looks like it happened in Paul's case. Maybe we can find a way
to avoid that.  Also there's the loader lock issue Paul pointed out.

Still I'm optimistic that can be all fixed.

-Andi
-- 
ak@linux.intel.com -- Speaking for myself only.

Re: PATCH: Turn on -fomit-frame-pointer by default for 32bit Linux/x86

[Andi Kleen]

> Not that I doubt your claim, but the ability of gdb to unwind signal
> frames is not proof of the existence of kernel unwind information.
> 
> gdb can often generate a backtrace (even through signal frames) in
> the absence of any debugging meta-data.

Fair enough. I verified it works using glibc backtrace() too.

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only.