[PATCH] Fixes problem setting breakpoint in dynamic loader

[Thiago Jung Bauermann]

Hi folks,

This is a re-submission of:

http://sourceware.org/ml/gdb-patches/2006-07/msg00264.html

The patch above resolves all review comments which were made on previous
versions (I collected them in the attached .txt, if you wish to have a
look). The problem is that it is malformed (maybe it was hand edited, I
don't know) and doesn't fix a couple of typos in comments.

The attached patch fixes the typos and applies cleanly on current CVS
HEAD.

Regarding this comment from Kevin Buettner (found in
http://sources.redhat.com/ml/gdb-patches/2006-06/msg00382.html):

> > +	     When those function descriptors are in the code section, they
> > +	     contain executable code and we can set a breakpoint there. >
*/
> 
> Also, I don't mind that the comment was rearranged, but I would like
> to see information regarding the two linker symbols retained in some
> fashion.

The information regarding the two linker symbols is outdated. The "dot"
symbols pointing to the function entrypoint are no longer generated by
the toolchain (this is true at least for ppc, I don't know about other
architectures). Tools are expected to automatically generate those
symbols when needed.

Is this ok for inclusion?
-- 
[]'s
Thiago Jung Bauermann
Software Engineer
IBM Linux Technology Center
-------------- next part --------------
My comments in [].

--------------------------------------------------------------------------------
http://sourceware.org/ml/gdb-patches/2006-07/msg00183.html

> +	  if (sym_addr != 0)
> +	    /* Convert 'sym_addr' from a function pointer to an address. */
> +	    sym_addr = gdbarch_convert_from_func_ptr_addr (current_gdbarch,
> +							   sym_addr,
> +							   tmp_bfd_target);

Why do you call gdbarch_convert_from_func_ptr_addr here?  This is already
called by adjust_breakpoint_address via
ppc64_sysv_abi_adjust_breakpoint_address when setting the breakpoint.

[ that code is gone now ]

Andreas.
--------------------------------------------------------------------------------
http://sourceware.org/ml/gdb-patches/2006-07/msg00020.html

Formatting notes.

On Wed, Jul 05, 2006 at 04:57:23PM -0700, PAUL GILLIAM wrote:
> +	     contain executable code and we can set a breakpoint there. */

Two spaces after period, here and elsewhere.

[ that code is gone now ]

> +	  /* No symbol was found in a code section, so look elsewhere. */
> +	  for (bkpt_namep = solib_break_names; *bkpt_namep!=NULL; bkpt_namep++)

Line is too long; also related, spaces around operators.

[ that code is gone now ]

> +	      /* On ABI's that use function descriptors that are in the data
> +	         section, */

Lost a bit of this comment?

[ that code is gone now ]
-- 
Daniel Jacobowitz
CodeSourcery
--------------------------------------------------------------------------------
http://sources.redhat.com/ml/gdb-patches/2006-06/msg00382.html

> +	  /* What we're looking for here is the machine code entry point,
> +	     so we are only interested in symbols in code sections.
> +
> +	     On ABI's that use function descriptors, the linker symbol with
                ^^^^^
		ABIs
[ fixed ]

> +	     the same name as a C funtion points to that functions descriptor.
                                  ^^^^^^^                ^^^^^^^^^
				  function               function's

[ that code is gone now ]

> +	     When those function descriptors are in the code section, they
> +	     contain executable code and we can set a breakpoint there. */

Also, I don't mind that the comment was rearranged, but I would like
to see information regarding the two linker symbols retained in some
fashion.

[ see message ]

>  	  sym_addr = bfd_lookup_symbol (tmp_bfd, *bkpt_namep, SEC_CODE);
>  	  if (sym_addr != 0)
>  	    break;
>  	}
>  
> +      if (sym_addr == 0)
> +        {
> +	  CORE_ADDR sect_offset;
> +	  
> +	  /* No symbol was found in a code section, so look in the data
> +             sections.  This will only happen when the linker symbol points
> +	     to a function descriptor that is in a data section. */
> +	  for (bkpt_namep = solib_break_names; *bkpt_namep!=NULL; bkpt_namep++)
> +	    {
> +	      /* On ABI's that use function descriptors that are in the data
> +	         section, */
> +	      sym_addr = bfd_lookup_symbol (tmp_bfd, *bkpt_namep, SEC_DATA);
> +	      if (sym_addr != 0)
> +		break;
> +	    }

Starting from the line immediately below...
> +	  if (sym_addr == 0)
> +	    {
> +	      target_close (tmp_bfd_target, 0);
> +	      goto bkpt_at_symbol;
> +	    }
...through the line immediately above, could we delete those lines and
instead just say:

	  if (sym_addr != 0)

before the assignment (sym_addr = gdbarch_convert...) below?

(This gets rid of the goto and the extra call to target_close().)

[ that code is gone now ]

> +
> +	  /* Convert 'sym_addr' from a function pointer to an address. */
> +	  sym_addr = gdbarch_convert_from_func_ptr_addr (current_gdbarch,
> +							 sym_addr,
> +							 tmp_bfd_target);
> +        }
> +
>        /* We're done with both the temporary bfd and target.  Remember,
>           closing the target closes the underlying bfd.  */
>        target_close (tmp_bfd_target, 0);

With my suggested changes above, I think this is okay.  I'd like to
see another patch posted to this list though prior to committing...

Thanks,

Kevin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: loader-break-2007-04-27.diff
Type: text/x-patch
Size: 10096 bytes
Desc: not available
URL: <http://sourceware.org/pipermail/gdb-patches/attachments/20070427/aa6acd2d/attachment.bin>