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