Re: [PATCH 11/11] ELFv2 ABI: skip global entry point code

[Joel Brobecker <brobecker at adacore dot com>]

> Now, when setting a breakpoint on a function by name, you really want
> that breakpoint to trigger either way, no matter whether the function
> is called via its local or global entry point.  Since the global entry
> point will always fall through into the local entry point, the way to
> achieve that is to simply set the breakpoint at the local entry point.

I think ppc-aix has the same kind issue? Some inter-unit function calls
can get optimized, even by the linker. I can't remember the details,
but maybe the optimization is make possible when the jump distance
is not exceeding a certain value?

Anyway...

> Unfortunately, I don't really see any way to express the notion of
> local entry points with the current set of gdbarch callbacks.
> 
> Thus this patch adds a new callback, skip_entrypoint, that is
> somewhat analogous to skip_prologue, but is called every time
> GDB needs to determine a function start address, even in those
> cases where GDB decides to not call skip_prologue.

I don't feel very qualified to review this addition. However, I couldn't
help but notice that this new callback is very close to
deprecated_function_start_offset - which, btw, appears to be only used
in the case of VAX.

If this method was to be added, I am wondering whether we could
replace deprecated_function_start_offset by this new method.
The implementation in vax-tdep would simply return pc+2 instead
of plain 2.

> With this implemented, two test cases would still fail to set
> the breakpoint correctly, but that's because they use the construct:
> 
>  gdb_test "break *hello"
> 
> Now, using "*hello" explicitly instructs GDB to set the breakpoint
> at the numerical value of "hello" treated as function pointer, so
> it will by definition only hit the global entry point.
> 
> I think this behaviour is unavoidable, but acceptable -- most people
> do not use this construct, and if they do, they get what they
> asked for ...

Agreed. In fact, I'd argue that the behavior is as it should be.

> However, in those two test cases, use of this construct is really
> not appropriate.  I think this was added way back when as a means
> to work around prologue skipping problems on some platforms.  These
> days that shouldn't really be necessary any more ...

Agreed for one, but not sure about the second. More explanations below.

> ChangeLog:
> 
> 	* gdbarch.sh (skip_entrypoint): New callback.
> 	* gdbarch.c, gdbarch.h: Regenerate.
> 	* symtab.c (skip_prologue_sal): Call gdbarch_skip_entrypoint.
> 	* infrun.c (fill_in_stop_func): Likewise.
> 	* ppc-linux-tdep.c: Include "elf/ppc64.h".
> 	(ppc_elfv2_elf_make_msymbol_special): New function.
> 	(ppc_elfv2_skip_entrypoint): Likewise.
> 	(ppc_linux_init_abi): Install them for ELFv2.
> 
> testsuite/ChangeLog:
> 
> 	* gdb.base/sigbpt.exp: Do not use "*" when setting breakpoint
> 	on a function.
> 	* gdb.base/step-bt.exp: Likewise.
> 
> 
> Index: binutils-gdb/gdb/gdbarch.sh
> ===================================================================
> --- binutils-gdb.orig/gdb/gdbarch.sh
> +++ binutils-gdb/gdb/gdbarch.sh
> @@ -530,6 +530,7 @@ m:int:return_in_first_hidden_param_p:str
>  
>  m:CORE_ADDR:skip_prologue:CORE_ADDR ip:ip:0:0
>  M:CORE_ADDR:skip_main_prologue:CORE_ADDR ip:ip
> +M:CORE_ADDR:skip_entrypoint:CORE_ADDR ip:ip

Can you add a comment documenting precisely the purpose of this method,
what its parameters are, and what the return value is expected to be?
That's really going to help us out in the future.

> +/* If the ELF symbol has a local entry point, use it as SYMBOL_VALUE_ADDRESS
> +   for the msymbol.  This matches the DWARF function start if present.  */
> +
> +static void
> +ppc_elfv2_elf_make_msymbol_special (asymbol *sym, struct minimal_symbol *msym)

You probably meant this comment for the next function
(ppc_elfv2_skip_entrypoint)? Although, I would prefer it if
the comment just said the usual "Implement the skip_entry_point
gdbarch method". That way, when we make some changes to that
method, we can just change the code without worrying about
the function documentation. Feel free to add target-specific
comments if needed, of course, either at the end of the documentation,
or (my preference), inside the function bodies.

Can you add documentation for this function (same remark as above,
since this is also a gdbarch implementation)?

> +  elf_symbol_type *elf_sym = (elf_symbol_type *)sym;
> +  switch (PPC64_LOCAL_ENTRY_OFFSET (elf_sym->internal_elf_sym.st_other))

Empty line after local variable declaration.

> -gdb_test "advance *bowler" "bowler.*" "advance to the bowler"
> +gdb_test "advance bowler" "bowler.*" "advance to the bowler"
>  set test "stepping to fault"

Agreed on this one. I did some archeology as well as looking at
the test. There is no real reason to stop at the start of the function.
Despite stopping after the prologue, the code is written in a way
that we should still have multiple instructions to step before
causing the SEGV.

> -gdb_test "break *hello" \
> +gdb_test "break hello" \
>           "Breakpoint.*at.* file .*$srcfile, line .*" \
>           "breakpoint at first instruction of hello()"

I am not sure about this one, however. It seems to me that the purpose
of this test is to verify the behavior of the "bt" command while
still inside a function's prologue.  That is something useful that
I wouldn't change.

For ppc64 using ELFv2, my suggestion would be to break at *hello+8?

-- 
Joel