[PATCH] create_internal_breakpoint: Apply gdbarch_skip_entrypoint.

[PATCH] create_internal_breakpoint: Apply gdbarch_skip_entrypoint.

[Doug Evans]

Hi.

In glibc, _dl_debug_state is usually defined like this:

void
_dl_debug_state (void)
{
}

and thus on powerpc64le-linux this function does not require a TOC register.

This is important because the toolchain will optimize intra-module calls
to skip the first two instructions that set up the TOC register.
And since gdb currently doesn't to "entry point skipping" for internal
breakpoints things work (in particular shlib event breakpoints).

But, if one happens to put something in _dl_debug_state that needs
the TOC register, its caller will enter the function at
_dl_debug_state + 8, but gdb will set its shlib breakpoint
at _dl_debug_state + 0, and shlib tracking stops working.

This patch fixes things by applying entry point skipping to
internal breakpoints.  Is this the best place to apply entry point
skipping for internal breakpoints?

Here's a hacky testcase to see the problem.
I haven't put any time into trying to turn it into something
that can be committed.

---snip---
int x;

void
_ovly_debug_event (void)
{
  x = 42;
}

int
main ()
{
  _ovly_debug_event ();
  return 0;
}
---snip---

With unpatched gdb:

(gdb) start
Temporary breakpoint 1 at 0x10000768: file ovly-debug-event.c, line 13.
Starting program: /home/dje/src/play/ovly-debug-event.x

Temporary breakpoint 1, main () at ovly-debug-event.c:13
13        _ovly_debug_event ();
(gdb) mt i br
Num     Type           Disp Enb Address            What
-3      shlib events   keep y   0x00003fffb7fd8e40 <__GI__dl_debug_state> inf 1
-5      overlay events keep n   0x0000000010000710 <_ovly_debug_event> inf 1
(gdb) disas _ovly_debug_event
Dump of assembler code for function _ovly_debug_event:
   0x0000000010000710 <+0>:     addis   r2,r12,2
   0x0000000010000714 <+4>:     addi    r2,r2,-31528
   0x0000000010000718 <+8>:     ...
   ...
(gdb) disas main
Dump of assembler code for function main:
   0x000000001000074c <+0>:     addis   r2,r12,2
   0x0000000010000750 <+4>:     addi    r2,r2,-31588
   ...
=> 0x0000000010000768 <+28>:    bl      0x10000718 <_ovly_debug_event+8>
   ...

Note that the "overlay events" breakpoint won't be hit because main calls
_ovly_debug_event after the two insns that set up the TOC register.

With a patched gdb:

(gdb) start
Temporary breakpoint 1 at 0x10000768: file ovly-debug-event.c, line 13.
Starting program: /home/dje/src/play/ovly-debug-event.x

Temporary breakpoint 1, main () at ovly-debug-event.c:13
13        _ovly_debug_event ();
(gdb) mt i br
Num     Type           Disp Enb Address            What
-3      shlib events   keep y   0x0000100000020e48 <__GI__dl_debug_state+8> inf 1
-6      overlay events keep n   0x0000000010000718 <_ovly_debug_event+8> inf 1

Note that the "overlay events" breakpoint is now at an instruction
that will get executed.


2014-11-07  Doug Evans  <dje@google.com>

	* breakpoint.c (create_internal_breakpoint): Apply
	gdbarch_skip_entrypoint if it's defined.

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index bd51f5d..1b5cf5f 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -3306,6 +3306,9 @@ create_internal_breakpoint (struct gdbarch
*gdbarch,

   init_sal (&sal);		/* Initialize to zeroes.  */

+  if (gdbarch_skip_entrypoint_p (gdbarch))
+    address = gdbarch_skip_entrypoint (gdbarch, address);
+
   sal.pc = address;
   sal.section = find_pc_overlay (sal.pc);
   sal.pspace = current_program_space;

Re: [PATCH] create_internal_breakpoint: Apply gdbarch_skip_entrypoint.

[Doug Evans]

Doug Evans writes:
 > Hi.
 > 
 > In glibc, _dl_debug_state is usually defined like this:
 > 
 > void
 > _dl_debug_state (void)
 > {
 > }
 > 
 > and thus on powerpc64le-linux this function does not require a TOC register.
 >
 > [...]
 > ...
 > -3      shlib events   keep y   0x00003fffb7fd8e40 <__GI__dl_debug_state> inf 1
 > ...
 > [...]
 > ...
 > -3      shlib events   keep y   0x0000100000020e48 <__GI__dl_debug_state+8> inf 1

For completeness sake,
You can also see the difference in the "shlib events" breakpoint,
but with _ovly_debug_state you can run it for yourself without
having to hack glibc.

There was a cut-n-paste error in the patch.
Fixed below.

2014-11-07  Doug Evans  <dje@google.com>

	* breakpoint.c (create_internal_breakpoint): Apply
	gdbarch_skip_entrypoint if it's defined.

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index bd51f5d..1b5cf5f 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -3306,6 +3306,9 @@ create_internal_breakpoint (struct gdbarch *gdbarch,
 
   init_sal (&sal);		/* Initialize to zeroes.  */
 
+  if (gdbarch_skip_entrypoint_p (gdbarch))
+    address = gdbarch_skip_entrypoint (gdbarch, address);
+
   sal.pc = address;
   sal.section = find_pc_overlay (sal.pc);
   sal.pspace = current_program_space;

Re: [PATCH] create_internal_breakpoint: Apply gdbarch_skip_entrypoint.

[Ulrich Weigand]

Doug Evans worte:

> This is important because the toolchain will optimize intra-module calls
> to skip the first two instructions that set up the TOC register.
> And since gdb currently doesn't to "entry point skipping" for internal
> breakpoints things work (in particular shlib event breakpoints).

Huh.  Yes, that's clearly a bug.

> This patch fixes things by applying entry point skipping to
> internal breakpoints.  Is this the best place to apply entry point
> skipping for internal breakpoints?

I think this is fine.  (The alternative would be push it up into the
callers, which would have the advantage that you could do it only
for those callers that get the address from a symbol, and not those
that e.g. get it from a probe.  However, since gdbarch_skip_entrypoint
is safe even if the address is not equal to a symbol address, it's
probably preferable to just do the skipping in one place.)

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com

Re: [PATCH] create_internal_breakpoint: Apply gdbarch_skip_entrypoint.

[Pedro Alves]

On 11/10/2014 12:58 PM, Ulrich Weigand wrote:
> Doug Evans worte:
> 
>> This is important because the toolchain will optimize intra-module calls
>> to skip the first two instructions that set up the TOC register.
>> And since gdb currently doesn't to "entry point skipping" for internal
>> breakpoints things work (in particular shlib event breakpoints).
> 
> Huh.  Yes, that's clearly a bug.
> 
>> This patch fixes things by applying entry point skipping to
>> internal breakpoints.  Is this the best place to apply entry point
>> skipping for internal breakpoints?
> 
> I think this is fine.  (The alternative would be push it up into the
> callers, which would have the advantage that you could do it only
> for those callers that get the address from a symbol, and not those
> that e.g. get it from a probe.  However, since gdbarch_skip_entrypoint
> is safe even if the address is not equal to a symbol address, it's
> probably preferable to just do the skipping in one place.)

Hmm, not sure.  Are there cases that do want a breakpoint
on the specific address that was passed down, even if it was
the entry point address?  I'm looking at the watchpoint scope
code:

 	  scope_breakpoint
	    = create_internal_breakpoint (frame_unwind_caller_arch (frame),
					  frame_unwind_caller_pc (frame),
					  bp_watchpoint_scope,
					  &momentary_breakpoint_ops);

and wondering about a signal coming in just while the mainline code
was going to execute the entry point address.

Should we add a create_internal_breakpoint_at_function method,
and adjust callers that are setting a breakpoint at a function,
rather than a specific address (like probes and the scope
watchpoint), to use that instead [1]?  (or the opposite; add a
new create_internal_breakpoint_at_address function?)

I'm particularly thinking of gdbarch_convert_from_func_ptr_addr, and
wondering why create_overlay_event_breakpoint doesn't need to call it.
The jit event breakpoint code doesn't call it either, it seems.
And neither the longjmp even breakpoint, when the breakpoint
it set by function name instead of by probe.  Are these just cases
of people not having stumbled on this yet?  Or are these event
locations/functions different somehow?  Why does the solib-svr4.c
need to call gdbarch_convert_from_func_ptr_addr for its event
functions, while others do not?

[1] something like:

static struct breakpoint *
create_internal_breakpoint_at_function (struct gdbarch *gdbarch,
			    CORE_ADDR address, enum bptype type,
			    const struct breakpoint_ops *ops)
{
  address = gdbarch_convert_from_func_ptr_addr (..., address, ...);
  address = gdbarch_skip_entrypoint (..., address, ...);
  return create_internal_breakpoint (..., address, ...)
}

(or instead move the gdbarch calls to a helper function that given
an address, returns the function's breakpoint address?)

Thanks,
Pedro Alves

Re: [PATCH] create_internal_breakpoint: Apply gdbarch_skip_entrypoint.

[Ulrich Weigand]

Pedro Alves wrote:

> Hmm, not sure.  Are there cases that do want a breakpoint
> on the specific address that was passed down, even if it was
> the entry point address?  I'm looking at the watchpoint scope
> code:
> 
>  	  scope_breakpoint
> 	    = create_internal_breakpoint (frame_unwind_caller_arch (frame),
> 					  frame_unwind_caller_pc (frame),
> 					  bp_watchpoint_scope,
> 					  &momentary_breakpoint_ops);
> 
> and wondering about a signal coming in just while the mainline code
> was going to execute the entry point address.

Hmm, OK.  It probably wouldn't matter in practice, but I agree that
in principle, this usage should not skip to the local entry point.

> I'm particularly thinking of gdbarch_convert_from_func_ptr_addr, and
> wondering why create_overlay_event_breakpoint doesn't need to call it.
> The jit event breakpoint code doesn't call it either, it seems.
> And neither the longjmp even breakpoint, when the breakpoint
> it set by function name instead of by probe.  Are these just cases
> of people not having stumbled on this yet?  Or are these event
> locations/functions different somehow?  Why does the solib-svr4.c
> need to call gdbarch_convert_from_func_ptr_addr for its event
> functions, while others do not?

gdbarch_convert_from_func_ptr_addr pretty much was added whereever
someone noticed that something didn't work on ppc64 ...

In particular, on Linux/ppc64 we don't use overlays; JIT didn't
work for other reasons anyway, same for non-probe longjmp.  The
non-svr4 solib routines aren't used on ppc64.

I agree it would be better to clean this up somehow.

> (or instead move the gdbarch calls to a helper function that given
> an address, returns the function's breakpoint address?)

This sounds reasonable.  Maybe start with an msymbol instead of an
address to make sure this is used only where we have symbol address
to start with?  This is what most users of create_internal_breakpoint
acually do anyway.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com