Re: [patch] Fixes problem setting breakpoint in dynamic loader

[Kevin Buettner <kevinb@redhat.com>]

On Mon, 26 Jun 2006 14:23:41 -0700
PAUL GILLIAM <pgilliam@us.ibm.com> wrote:

> Here is the new patch, with out deleting the 'dot' symbol.  I included a
> copy of the rs6000 patch as well, just for completeness.

I didn't see the rs6000 patch.

> OK to commit?

Not quite yet...

> Index: solib-svr4.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/solib-svr4.c,v
> retrieving revision 1.58
> diff -a -u -r1.58 solib-svr4.c
> --- solib-svr4.c	18 May 2006 20:38:56 -0000	1.58
> +++ solib-svr4.c	26 Jun 2006 22:08:43 -0000
> @@ -1043,20 +1043,45 @@
>        /* Now try to set a breakpoint in the dynamic linker.  */
>        for (bkpt_namep = solib_break_names; *bkpt_namep != NULL; bkpt_namep++)
>  	{
> -          /* On ABI's that use function descriptors, there are usually
> -             two linker symbols associated with each C function: one
> -             pointing at the actual entry point of the machine code,
> -             and one pointing at the function's descriptor.  The
> -             latter symbol has the same name as the C function.
> -
> -             What we're looking for here is the machine code entry
> -             point, so we are only interested in symbols in code
> -             sections.  */

I have a few suggestions regarding your rewrite of the comment:

> +	  /* 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
> +	     the same name as a C funtion points to that functions descriptor.
                                  ^^^^^^^                ^^^^^^^^^
				  function               function's
> +	     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.

>  	  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().)
> +
> +	  /* 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