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

Kevin Buettner
2006-06-26T23:55:42Z

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