Get longjmp target check in breakpoint.c - is it necessary?

Get longjmp target check in breakpoint.c - is it necessary?

[Tiago Stürmer Daitx]

We have in gdb/breakpoint.c the following condition checking:

 3222       if (!gdbarch_get_longjmp_target_p (gdbarch))¬
 3223 »       continue;¬

Which is forcing me to implement an (unnecessary?) arch specific
get_longjmp_target while having a longjmp user probe in glibc should be
enough. Removing that if/continue statement showed no regressions - it
actually fixed 3 longjmp failures (assuming proper support in glibc for
a longjmp probe is in place). 

I would be glad to hear your thoughts on any possible side effects of
removing that statement.

$ diff -u gdb/testsuite/gdb.sum
gdb/testsuite/gdb-no-get-longjmp-target-check.sum

[snip]
-FAIL: gdb.base/longjmp.exp: next over longjmp(1)
+PASS: gdb.base/longjmp.exp: next over longjmp(1)
+PASS: gdb.base/longjmp.exp: next into else block (1)
+PASS: gdb.base/longjmp.exp: next into safety net (1)
 PASS: gdb.base/longjmp.exp: breakpoint at pattern 2 start
 PASS: gdb.base/longjmp.exp: continue to breakpoint at pattern 2 start
 PASS: gdb.base/longjmp.exp: breakpoint at miss_step_2
 PASS: gdb.base/longjmp.exp: next over setjmp (2)
-FAIL: gdb.base/longjmp.exp: next over call_longjmp (2)
+PASS: gdb.base/longjmp.exp: next over call_longjmp (2)
+PASS: gdb.base/longjmp.exp: next into else block (2)
+PASS: gdb.base/longjmp.exp: next into safety net (2)
[snip]
-XFAIL: gdb.base/stale-infcall.exp: test system longjmp tracking support
-UNTESTED: gdb.base/stale-infcall.exp: System lacks support for tracking
longjmps
+PASS: gdb.base/stale-infcall.exp: test system longjmp tracking support
+PASS: gdb.base/stale-infcall.exp: delete $test_fail_bpnum
+PASS: gdb.base/stale-infcall.exp: continue to breakpoint: break-run1
+PASS: gdb.base/stale-infcall.exp: print infcall ()
+PASS: gdb.base/stale-infcall.exp: stack corrupted
+PASS: gdb.base/stale-infcall.exp: bt
+PASS: gdb.base/stale-infcall.exp: maintenance print dummy-frames
+PASS: gdb.base/stale-infcall.exp: maintenance info breakpoints


Everything was tested on both PPC and PPC64. The glibc user probes have
been implemented but are not yet upstream.

Cheers!
-tiago

-- 
Tiago Stürmer Daitx
tdaitx@linux.vnet.ibm.com
IBM - Linux Technology Center

Re: Get longjmp target check in breakpoint.c - is it necessary?

[Edjunior Barbosa Machado]

Hi Tiago,

On 10/31/2013 12:36 AM, Tiago Stürmer Daitx wrote:
> We have in gdb/breakpoint.c the following condition checking:
> 
>  3222       if (!gdbarch_get_longjmp_target_p (gdbarch))¬
>  3223 »       continue;¬
> 
> Which is forcing me to implement an (unnecessary?) arch specific
> get_longjmp_target while having a longjmp user probe in glibc should be
> enough. Removing that if/continue statement showed no regressions - it
> actually fixed 3 longjmp failures (assuming proper support in glibc for
> a longjmp probe is in place). 

If I understood correctly, there are different longjmps symbols search
strategies on this create_longjmp_master_breakpoint() function: the
recent libc probe search and the per-objfile cache lookup.

It seems this 'if' works like an optimization to avoid searching for
symbols if the arch didn't provide support for get_longjmp_target().
However, <arch>_get_longjmp_target() function became unnecessary in
targets that provide libc probe support, as you noticed with the
addition of probes on ppc64 glibc.

What if you try to move this 'if' to a bit below in the function (right
before "for (i = 0; i < NUM_LONGJMP_NAMES; i++)" loop)?

Thanks and regards,
-- 
Edjunior

Re: Get longjmp target check in breakpoint.c - is it necessary?

[Tiago Stürmer Daitx]

On Thu, 2013-10-31 at 11:27 -0200, Edjunior Barbosa Machado wrote:

> What if you try to move this 'if' to a bit below in the function (right
> before "for (i = 0; i < NUM_LONGJMP_NAMES; i++)" loop)?

Good catch, I didn't notice the 'if' is kind of an optimization for the
scenarios where <arch>_get_longjmp_target is not provided. Moving it
down before that 'for NUM_LONGJMP_NAMES' loop would work nicely as
probes would work. 

The no changes were detected in the testsuite by moving the 'if' before
the 'for' compared to removing it altogether.

Regards,
Tiago


-- 
Tiago Stürmer Daitx
tdaitx@linux.vnet.ibm.com
IBM - Linux Technology Center

Re: Get longjmp target check in breakpoint.c - is it necessary?

[Sergio Durigan Junior]

On Thursday, October 31 2013, Edjunior Barbosa Machado wrote:

> Hi Tiago,

Hey :-)

> On 10/31/2013 12:36 AM, Tiago Stürmer Daitx wrote:
>> We have in gdb/breakpoint.c the following condition checking:
>> 
>>  3222       if (!gdbarch_get_longjmp_target_p (gdbarch))¬
>>  3223 »       continue;¬
>> 
>> Which is forcing me to implement an (unnecessary?) arch specific
>> get_longjmp_target while having a longjmp user probe in glibc should be
>> enough. Removing that if/continue statement showed no regressions - it
>> actually fixed 3 longjmp failures (assuming proper support in glibc for
>> a longjmp probe is in place). 
>
> If I understood correctly, there are different longjmps symbols search
> strategies on this create_longjmp_master_breakpoint() function: the
> recent libc probe search and the per-objfile cache lookup.
>
> It seems this 'if' works like an optimization to avoid searching for
> symbols if the arch didn't provide support for get_longjmp_target().
> However, <arch>_get_longjmp_target() function became unnecessary in
> targets that provide libc probe support, as you noticed with the
> addition of probes on ppc64 glibc.
>
> What if you try to move this 'if' to a bit below in the function (right
> before "for (i = 0; i < NUM_LONGJMP_NAMES; i++)" loop)?

Edjunior is right, Tiago.  This is a bug, actually.  This check should
be moved to the place Edjunior mentioned.  Could you post a patch to fix
that?

Thanks a lot for looking into this :-).

-- 
Sergio