* [PATCH v2 15/15] Fix resolving GNU ifunc bp locations when inferior runs resolver
2018-03-25 19:19 [PATCH v2 00/15] Fixing GNU ifunc support Pedro Alves
@ 2018-03-25 19:19 ` Pedro Alves
2018-03-25 19:19 ` [PATCH v2 08/15] Eliminate find_pc_partial_function_gnu_ifunc Pedro Alves
` (14 subsequent siblings)
15 siblings, 0 replies; 33+ messages in thread
From: Pedro Alves @ 2018-03-25 19:19 UTC (permalink / raw)
To: gdb-patches
I noticed that if you set a breakpoint on an ifunc before the ifunc is
resolved, and then let the program call the ifunc, thus resolving it,
GDB end up with a location for that original breakpoint that is
pointing to the ifunc target, but it is left pointing to the first
address of the function, instead of after its prologue. After
prologue is what you get if you create a new breakpoint at that point.
1) With no debug info for the target function:
1.a) Set before resolving, and then program continued passed resolving:
Num Type Disp Enb Address What
1 breakpoint keep y 0x0000000000400753 <final>
1.b) Breakpoint set after inferior resolved ifunc:
Num Type Disp Enb Address What
2 breakpoint keep y 0x0000000000400757 <final+4>
2) With debug info for the target function:
1.a) Set before resolving, and then program continued passed resolving:
Num Type Disp Enb Address What
1 breakpoint keep y 0x0000000000400753 in final at gdb/testsuite/gdb.base/gnu-ifunc-final.c:20
1.b) Breakpoint set after inferior resolved ifunc:
Num Type Disp Enb Address What
2 breakpoint keep y 0x000000000040075a in final at gdb/testsuite/gdb.base/gnu-ifunc-final.c:21
The problem is that elf_gnu_ifunc_resolver_return_stop (called by the
internal breakpoint that traps the resolver returning) does not agree
with linespec.c:minsym_found. It does not skip to the function's
start line (i.e., past the prologue). We can now use the
find_function_start_sal overload added by the previous commmit to fix
this.
New tests included, which fail before the patch, and pass afterwards.
gdb/ChangeLog:
yyyy-mm-dd Pedro Alves <palves@redhat.com>
* elfread.c (elf_gnu_ifunc_resolver_return_stop): Use
find_function_start_sal instead of find_pc_line.
gdb/testsuite/ChangeLog:
yyyy-mm-dd Pedro Alves <palves@redhat.com>
* gdb.base/gnu-ifunc.exp (set-break): Test that GDB resolves
ifunc breakpoint locations correctly of ifunc breakpoints set
while the program resolves the ifunc.
---
gdb/elfread.c | 3 ++-
gdb/testsuite/gdb.base/gnu-ifunc.exp | 18 ++++++++++++++----
2 files changed, 16 insertions(+), 5 deletions(-)
diff --git a/gdb/elfread.c b/gdb/elfread.c
index 2a876e1bcc4..b50a283bea1 100644
--- a/gdb/elfread.c
+++ b/gdb/elfread.c
@@ -1033,7 +1033,8 @@ elf_gnu_ifunc_resolver_return_stop (struct breakpoint *b)
b->type = bp_breakpoint;
update_breakpoint_locations (b, current_program_space,
- find_pc_line (resolved_pc, 0), {});
+ find_function_start_sal (resolved_pc, NULL, true),
+ {});
}
/* A helper function for elf_symfile_read that reads the minimal
diff --git a/gdb/testsuite/gdb.base/gnu-ifunc.exp b/gdb/testsuite/gdb.base/gnu-ifunc.exp
index 827ac1202d2..d6ec6988a7d 100644
--- a/gdb/testsuite/gdb.base/gnu-ifunc.exp
+++ b/gdb/testsuite/gdb.base/gnu-ifunc.exp
@@ -106,6 +106,9 @@ proc_with_prefix set-break {resolver_attr resolver_debug final_debug} {
return 1
}
+ gdb_breakpoint [gdb_get_line_number "break-at-call"]
+ gdb_continue_to_breakpoint "break-at-call" ".*break-at-call.*"
+
set ws "\[ \t\]+"
set dot "\\.?"
@@ -131,19 +134,21 @@ proc_with_prefix set-break {resolver_attr resolver_debug final_debug} {
"Breakpoint $decimal at gnu-indirect-function resolver at $hex"
gdb_test "info breakpoints" \
"$decimal${ws}STT_GNU_IFUNC resolver${ws}keep${ws}y${ws}$hex <${gnu_ifunc_resolver}>"
+
+ # Make the breakpoint conditional on a condition that always
+ # fails. This is so that when the ifunc-resolver breakpoint
+ # triggers, GDB resumes the program immediately.
+ gdb_test_no_output "condition \$bpnum 0"
}
global final_src
with_test_prefix "resolve" {
- delete_breakpoints
gdb_breakpoint [gdb_get_line_number "break-at-exit"]
gdb_continue_to_breakpoint "break-at-exit" ".*break-at-exit.*"
}
with_test_prefix "after resolving" {
- delete_breakpoints
-
if {!$final_debug} {
# Set a breakpoint both at the ifunc, and at the ifunc's
# target. GDB should resolve both to the same address.
@@ -176,7 +181,12 @@ proc_with_prefix set-break {resolver_attr resolver_debug final_debug} {
gdb_test "break gnu_ifunc" "Breakpoint .* at $hex: file .*$final_src, line $lineno\\."
set location "$decimal${ws}breakpoint${ws}keep${ws}y${ws}$hex in final at .*$final_src:$lineno"
}
- gdb_test "info breakpoints" "$location\r\n$location"
+
+ # The first location here is for the breakpoint that was set
+ # before the ifunc was resolved. It should be resolved by
+ # now, and it should have the exact same address/line as the
+ # other two locations.
+ gdb_test "info breakpoints" "$location\r\n.*$location\r\n$location"
}
}
--
2.14.3
^ permalink raw reply [flat|nested] 33+ messages in thread* [PATCH v2 08/15] Eliminate find_pc_partial_function_gnu_ifunc
2018-03-25 19:19 [PATCH v2 00/15] Fixing GNU ifunc support Pedro Alves
2018-03-25 19:19 ` [PATCH v2 15/15] Fix resolving GNU ifunc bp locations when inferior runs resolver Pedro Alves
@ 2018-03-25 19:19 ` Pedro Alves
2018-03-25 19:19 ` [PATCH v2 03/15] Calling ifunc functions when target has no debug info but resolver has Pedro Alves
` (13 subsequent siblings)
15 siblings, 0 replies; 33+ messages in thread
From: Pedro Alves @ 2018-03-25 19:19 UTC (permalink / raw)
To: gdb-patches
Not used anywhere any longer.
If this is ever reinstated, note that this case:
cache_pc_function_is_gnu_ifunc = TYPE_GNU_IFUNC (SYMBOL_TYPE (f));
was incorrect in that regular symbols never have type marked as GNU
ifunc type, only minimal symbols. At some point I had some fix that
checking the matching minsym here. But in the end I ended up just
eliminating need for this function, so that fix was not necessary.
gdb/ChangeLog:
yyyy-mm-dd Pedro Alves <palves@redhat.com>
* blockframe.c (cache_pc_function_is_gnu_ifunc): Delete. Remove
all references.
(find_pc_partial_function_gnu_ifunc): Rename to ...
(find_pc_partial_function): ... this, and remove references to
'is_gnu_ifunc_p'.
(find_pc_partial_function): Delete old implementation.
* symtab.h (find_pc_partial_function_gnu_ifunc): Delete.
---
gdb/blockframe.c | 36 +++++++-----------------------------
gdb/symtab.h | 5 -----
2 files changed, 7 insertions(+), 34 deletions(-)
diff --git a/gdb/blockframe.c b/gdb/blockframe.c
index db02b35742d..1a28b3c8b5f 100644
--- a/gdb/blockframe.c
+++ b/gdb/blockframe.c
@@ -159,7 +159,6 @@ static CORE_ADDR cache_pc_function_low = 0;
static CORE_ADDR cache_pc_function_high = 0;
static const char *cache_pc_function_name = 0;
static struct obj_section *cache_pc_function_section = NULL;
-static int cache_pc_function_is_gnu_ifunc = 0;
/* Clear cache, e.g. when symbol table is discarded. */
@@ -170,7 +169,6 @@ clear_pc_function_cache (void)
cache_pc_function_high = 0;
cache_pc_function_name = (char *) 0;
cache_pc_function_section = NULL;
- cache_pc_function_is_gnu_ifunc = 0;
}
/* Finds the "function" (text symbol) that is smaller than PC but
@@ -178,19 +176,17 @@ clear_pc_function_cache (void)
*NAME and/or *ADDRESS conditionally if that pointer is non-null.
If ENDADDR is non-null, then set *ENDADDR to be the end of the
function (exclusive), but passing ENDADDR as non-null means that
- the function might cause symbols to be read. If IS_GNU_IFUNC_P is provided
- *IS_GNU_IFUNC_P is set to 1 on return if the function is STT_GNU_IFUNC.
- This function either succeeds or fails (not halfway succeeds). If it
- succeeds, it sets *NAME, *ADDRESS, and *ENDADDR to real information and
- returns 1. If it fails, it sets *NAME, *ADDRESS, *ENDADDR and
- *IS_GNU_IFUNC_P to zero and returns 0. */
+ the function might cause symbols to be read. This function either
+ succeeds or fails (not halfway succeeds). If it succeeds, it sets
+ *NAME, *ADDRESS, and *ENDADDR to real information and returns 1.
+ If it fails, it sets *NAME, *ADDRESS and *ENDADDR to zero and
+ returns 0. */
/* Backward compatibility, no section argument. */
int
-find_pc_partial_function_gnu_ifunc (CORE_ADDR pc, const char **name,
- CORE_ADDR *address, CORE_ADDR *endaddr,
- int *is_gnu_ifunc_p)
+find_pc_partial_function (CORE_ADDR pc, const char **name, CORE_ADDR *address,
+ CORE_ADDR *endaddr)
{
struct obj_section *section;
struct symbol *f;
@@ -243,7 +239,6 @@ find_pc_partial_function_gnu_ifunc (CORE_ADDR pc, const char **name,
cache_pc_function_high = BLOCK_END (SYMBOL_BLOCK_VALUE (f));
cache_pc_function_name = SYMBOL_LINKAGE_NAME (f);
cache_pc_function_section = section;
- cache_pc_function_is_gnu_ifunc = TYPE_GNU_IFUNC (SYMBOL_TYPE (f));
goto return_cached_value;
}
}
@@ -266,16 +261,12 @@ find_pc_partial_function_gnu_ifunc (CORE_ADDR pc, const char **name,
*address = 0;
if (endaddr != NULL)
*endaddr = 0;
- if (is_gnu_ifunc_p != NULL)
- *is_gnu_ifunc_p = 0;
return 0;
}
cache_pc_function_low = BMSYMBOL_VALUE_ADDRESS (msymbol);
cache_pc_function_name = MSYMBOL_LINKAGE_NAME (msymbol.minsym);
cache_pc_function_section = section;
- cache_pc_function_is_gnu_ifunc = (MSYMBOL_TYPE (msymbol.minsym)
- == mst_text_gnu_ifunc);
cache_pc_function_high = minimal_symbol_upper_bound (msymbol);
return_cached_value:
@@ -307,22 +298,9 @@ find_pc_partial_function_gnu_ifunc (CORE_ADDR pc, const char **name,
*endaddr = cache_pc_function_high;
}
- if (is_gnu_ifunc_p)
- *is_gnu_ifunc_p = cache_pc_function_is_gnu_ifunc;
-
return 1;
}
-/* See find_pc_partial_function_gnu_ifunc, only the IS_GNU_IFUNC_P parameter
- is omitted here for backward API compatibility. */
-
-int
-find_pc_partial_function (CORE_ADDR pc, const char **name, CORE_ADDR *address,
- CORE_ADDR *endaddr)
-{
- return find_pc_partial_function_gnu_ifunc (pc, name, address, endaddr, NULL);
-}
-
/* See symtab.h. */
struct type *
diff --git a/gdb/symtab.h b/gdb/symtab.h
index c6c643a93bf..20808777467 100644
--- a/gdb/symtab.h
+++ b/gdb/symtab.h
@@ -1665,11 +1665,6 @@ extern struct symbol *find_pc_sect_function (CORE_ADDR, struct obj_section *);
extern struct symbol *find_symbol_at_address (CORE_ADDR);
-extern int find_pc_partial_function_gnu_ifunc (CORE_ADDR pc, const char **name,
- CORE_ADDR *address,
- CORE_ADDR *endaddr,
- int *is_gnu_ifunc_p);
-
/* lookup function from address, return name, start addr and end addr. */
extern int find_pc_partial_function (CORE_ADDR, const char **, CORE_ADDR *,
--
2.14.3
^ permalink raw reply [flat|nested] 33+ messages in thread* [PATCH v2 03/15] Calling ifunc functions when target has no debug info but resolver has
2018-03-25 19:19 [PATCH v2 00/15] Fixing GNU ifunc support Pedro Alves
2018-03-25 19:19 ` [PATCH v2 15/15] Fix resolving GNU ifunc bp locations when inferior runs resolver Pedro Alves
2018-03-25 19:19 ` [PATCH v2 08/15] Eliminate find_pc_partial_function_gnu_ifunc Pedro Alves
@ 2018-03-25 19:19 ` Pedro Alves
2018-04-01 4:22 ` Simon Marchi
2018-03-25 19:19 ` [PATCH v2 01/15] Fix breakpoints in ifunc after inferior resolved it (@got.plt symbol creation) Pedro Alves
` (12 subsequent siblings)
15 siblings, 1 reply; 33+ messages in thread
From: Pedro Alves @ 2018-03-25 19:19 UTC (permalink / raw)
To: gdb-patches
In v2:
- Added testsuite tweak.
After the previous patch, on Fedora 27 (glibc 2.26), if you try
calling strlen in the inferior, you now get:
(top-gdb) p strlen ("hello")
'__strlen_avx2' has unknown return type; cast the call to its declared return type
This is correct, because __strlen_avx2 is written in assembly.
We can improve on this though -- if the final ifunc resolved/target
function has no debug info, but the ifunc _resolver_ does have debug
info, we can try extracting the final function's type from the type
that the resolver returns. E.g.,:
typedef size_t (*strlen_t) (const char*);
size_t my_strlen (const char *) { /* some implementation */ }
strlen_t strlen_resolver (unsigned long hwcap) { return my_strlen; }
extern size_t strlen (const char *s);
__typeof (strlen) strlen __attribute__ ((ifunc ("strlen_resolver")));
In the strlen example above, the resolver returns strlen_t, which is a
typedef for pointer to a function that returns size_t. "strlen_t" is
the type of both the user-visible "strlen", and of the the target
function that implements it.
This patch teaches GDB to extract that type.
This is done for actual inferior function calls (in infcall.c), and
for ptype (in eval_call). By the time we get to either of these
places, we've already lost the original symbol/minsym, and only have
values and types to work with. Hence the changes to c-exp.y and
evaluate_var_msym_value, to ensure that we propagate the ifunc
minsymbol's info.
The change to make ifunc symbols have no/unknown return type exposes a
latent problem -- gdb.compile/compile-ifunc.exp calls a no-debug-info
function, but we did not warn about it. The test is fixed by this
commit too.
gdb/ChangeLog:
yyyy-mm-dd Pedro Alves <palves@redhat.com>
* blockframe.c (gnu_ifunc_target_type): New function.
* eval.c (evaluate_var_msym_value): For GNU ifunc types, always
return a value with a memory address.
(eval_call): For calls to GNU ifunc functions, try to find the
type of the target function from the type that the resolver
returns.
* gdbtypes.c (objfile_type): Don't install a return type for ifunc
symbols.
* infcall.c (find_function_return_type): Rename to ...
(find_function_type): ... this, and return the function's type
instead of the function's return type.
(find_function_addr): Add 'function_type' parameter. For calls to
GNU ifunc functions, try to find the type of the target function
from the type that the resolver returns, and return it via
FUNCTION_TYPE.
(call_function_by_hand_dummy): Adjust to use the function type
returned by find_function_addr.
(find_function_addr): Add 'function_type' parameter and move
description here.
* symtab.h (find_gnu_ifunc_target_type): New declaration.
gdb/testsuite/ChangeLog:
yyyy-mm-dd Pedro Alves <palves@redhat.com>
* gdb.compile/compile-ifunc.exp: Also expect "function has unknown
return type" warnings.
---
gdb/blockframe.c | 34 +++++++++++++++++
gdb/eval.c | 25 ++++++++-----
gdb/gdbtypes.c | 4 --
gdb/infcall.c | 58 +++++++++++++++++++----------
gdb/infcall.h | 9 ++++-
gdb/symtab.h | 6 +++
gdb/testsuite/gdb.compile/compile-ifunc.exp | 9 +++--
7 files changed, 106 insertions(+), 39 deletions(-)
diff --git a/gdb/blockframe.c b/gdb/blockframe.c
index 9be8871f756..db02b35742d 100644
--- a/gdb/blockframe.c
+++ b/gdb/blockframe.c
@@ -323,6 +323,40 @@ find_pc_partial_function (CORE_ADDR pc, const char **name, CORE_ADDR *address,
return find_pc_partial_function_gnu_ifunc (pc, name, address, endaddr, NULL);
}
+/* See symtab.h. */
+
+struct type *
+find_gnu_ifunc_target_type (CORE_ADDR resolver_funaddr)
+{
+ /* See if we can figure out the function's return type from the type
+ that the resolver returns. */
+ symbol *sym = find_pc_function (resolver_funaddr);
+ if (sym != NULL
+ && SYMBOL_CLASS (sym) == LOC_BLOCK
+ && BLOCK_START (SYMBOL_BLOCK_VALUE (sym)) == resolver_funaddr)
+ {
+ struct type *resolver_type = SYMBOL_TYPE (sym);
+ if (TYPE_CODE (resolver_type) == TYPE_CODE_FUNC)
+ {
+ /* Get the return type of the resolver. */
+ struct type *resolver_ret_type
+ = check_typedef (TYPE_TARGET_TYPE (resolver_type));
+
+ /* If we found a pointer to function, then the resolved type
+ is the type of the pointed-to function. */
+ if (TYPE_CODE (resolver_ret_type) == TYPE_CODE_PTR)
+ {
+ struct type *resolved_type
+ = TYPE_TARGET_TYPE (resolver_ret_type);
+ if (TYPE_CODE (check_typedef (resolved_type)) == TYPE_CODE_FUNC)
+ return resolved_type;
+ }
+ }
+ }
+
+ return NULL;
+}
+
/* Return the innermost stack frame that is executing inside of BLOCK and is
at least as old as the selected frame. Return NULL if there is no
such frame. If BLOCK is NULL, just return NULL. */
diff --git a/gdb/eval.c b/gdb/eval.c
index 021503e9dd9..90f8c7d8b32 100644
--- a/gdb/eval.c
+++ b/gdb/eval.c
@@ -735,17 +735,13 @@ value *
evaluate_var_msym_value (enum noside noside,
struct objfile *objfile, minimal_symbol *msymbol)
{
- if (noside == EVAL_AVOID_SIDE_EFFECTS)
- {
- type *the_type = find_minsym_type_and_address (msymbol, objfile, NULL);
- return value_zero (the_type, not_lval);
- }
+ CORE_ADDR address;
+ type *the_type = find_minsym_type_and_address (msymbol, objfile, &address);
+
+ if (noside == EVAL_AVOID_SIDE_EFFECTS && !TYPE_GNU_IFUNC (the_type))
+ return value_zero (the_type, not_lval);
else
- {
- CORE_ADDR address;
- type *the_type = find_minsym_type_and_address (msymbol, objfile, &address);
- return value_at_lazy (the_type, address);
- }
+ return value_at_lazy (the_type, address);
}
/* Helper for returning a value when handling EVAL_SKIP. */
@@ -798,6 +794,15 @@ eval_call (expression *exp, enum noside noside,
else if (TYPE_CODE (ftype) == TYPE_CODE_FUNC
|| TYPE_CODE (ftype) == TYPE_CODE_METHOD)
{
+ if (TYPE_GNU_IFUNC (ftype))
+ {
+ CORE_ADDR address = value_address (argvec[0]);
+ type *resolved_type = find_gnu_ifunc_target_type (address);
+
+ if (resolved_type != NULL)
+ ftype = resolved_type;
+ }
+
type *return_type = TYPE_TARGET_TYPE (ftype);
if (return_type == NULL)
diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
index b3a037971e0..2efd1264ff8 100644
--- a/gdb/gdbtypes.c
+++ b/gdb/gdbtypes.c
@@ -5443,10 +5443,6 @@ objfile_type (struct objfile *objfile)
objfile_type->nodebug_text_gnu_ifunc_symbol
= init_type (objfile, TYPE_CODE_FUNC, TARGET_CHAR_BIT,
"<text gnu-indirect-function variable, no debug info>");
- /* Ifunc resolvers return a function address. */
- TYPE_TARGET_TYPE (objfile_type->nodebug_text_gnu_ifunc_symbol)
- = init_integer_type (objfile, gdbarch_addr_bit (gdbarch), 1,
- "__IFUNC_RESOLVER_RET");
TYPE_GNU_IFUNC (objfile_type->nodebug_text_gnu_ifunc_symbol) = 1;
objfile_type->nodebug_got_plt_symbol
= init_pointer_type (objfile, gdbarch_addr_bit (gdbarch),
diff --git a/gdb/infcall.c b/gdb/infcall.c
index 9f02674bdf2..d89d8ca7e8c 100644
--- a/gdb/infcall.c
+++ b/gdb/infcall.c
@@ -229,26 +229,26 @@ value_arg_coerce (struct gdbarch *gdbarch, struct value *arg,
return value_cast (type, arg);
}
-/* Return the return type of a function with its first instruction exactly at
+/* Return the type of a function with its first instruction exactly at
the PC address. Return NULL otherwise. */
static struct type *
-find_function_return_type (CORE_ADDR pc)
+find_function_type (CORE_ADDR pc)
{
struct symbol *sym = find_pc_function (pc);
- if (sym != NULL && BLOCK_START (SYMBOL_BLOCK_VALUE (sym)) == pc
- && SYMBOL_TYPE (sym) != NULL)
- return TYPE_TARGET_TYPE (SYMBOL_TYPE (sym));
+ if (sym != NULL && BLOCK_START (SYMBOL_BLOCK_VALUE (sym)) == pc)
+ return SYMBOL_TYPE (sym);
return NULL;
}
-/* Determine a function's address and its return type from its value.
- Calls error() if the function is not valid for calling. */
+/* See infcall.h. */
CORE_ADDR
-find_function_addr (struct value *function, struct type **retval_type)
+find_function_addr (struct value *function,
+ struct type **retval_type,
+ struct type **function_type)
{
struct type *ftype = check_typedef (value_type (function));
struct gdbarch *gdbarch = get_type_arch (ftype);
@@ -275,17 +275,33 @@ find_function_addr (struct value *function, struct type **retval_type)
if (TYPE_CODE (ftype) == TYPE_CODE_FUNC
|| TYPE_CODE (ftype) == TYPE_CODE_METHOD)
{
- value_type = TYPE_TARGET_TYPE (ftype);
-
if (TYPE_GNU_IFUNC (ftype))
{
- funaddr = gnu_ifunc_resolve_addr (gdbarch, funaddr);
+ CORE_ADDR resolver_addr = funaddr;
- /* Skip querying the function symbol if no RETVAL_TYPE has been
- asked for. */
- if (retval_type)
- value_type = find_function_return_type (funaddr);
+ /* Resolve the ifunc. Note this may call the resolver
+ function in the inferior. */
+ funaddr = gnu_ifunc_resolve_addr (gdbarch, resolver_addr);
+
+ /* Skip querying the function symbol if no RETVAL_TYPE or
+ FUNCTION_TYPE have been asked for. */
+ if (retval_type != NULL || function_type != NULL)
+ {
+ type *target_ftype = find_function_type (funaddr);
+ /* If we don't have debug info for the target function,
+ see if we can instead extract the target function's
+ type from the type that the resolver returns. */
+ if (target_ftype == NULL)
+ target_ftype = find_gnu_ifunc_target_type (resolver_addr);
+ if (target_ftype != NULL)
+ {
+ value_type = TYPE_TARGET_TYPE (check_typedef (target_ftype));
+ ftype = target_ftype;
+ }
+ }
}
+ else
+ value_type = TYPE_TARGET_TYPE (ftype);
}
else if (TYPE_CODE (ftype) == TYPE_CODE_INT)
{
@@ -320,6 +336,8 @@ find_function_addr (struct value *function, struct type **retval_type)
if (retval_type != NULL)
*retval_type = value_type;
+ if (function_type != NULL)
+ *function_type = ftype;
return funaddr + gdbarch_deprecated_function_start_offset (gdbarch);
}
@@ -727,7 +745,6 @@ call_function_by_hand_dummy (struct value *function,
struct infcall_suspend_state *caller_state;
CORE_ADDR funaddr;
CORE_ADDR real_pc;
- struct type *ftype = check_typedef (value_type (function));
CORE_ADDR bp_addr;
struct frame_id dummy_id;
struct frame_info *frame;
@@ -738,9 +755,6 @@ call_function_by_hand_dummy (struct value *function,
char name_buf[RAW_FUNCTION_ADDRESS_SIZE];
bool stack_temporaries = thread_stack_temporaries_enabled_p (inferior_ptid);
- if (TYPE_CODE (ftype) == TYPE_CODE_PTR)
- ftype = check_typedef (TYPE_TARGET_TYPE (ftype));
-
if (!target_has_execution)
noprocess ();
@@ -864,7 +878,11 @@ call_function_by_hand_dummy (struct value *function,
}
}
- funaddr = find_function_addr (function, &values_type);
+ struct type *ftype = check_typedef (value_type (function));
+ if (TYPE_CODE (ftype) == TYPE_CODE_PTR)
+ ftype = check_typedef (TYPE_TARGET_TYPE (ftype));
+
+ funaddr = find_function_addr (function, &values_type, &ftype);
if (values_type == NULL)
values_type = default_return_type;
if (values_type == NULL)
diff --git a/gdb/infcall.h b/gdb/infcall.h
index a3861fb1bf3..bea1494b50d 100644
--- a/gdb/infcall.h
+++ b/gdb/infcall.h
@@ -25,8 +25,15 @@
struct value;
struct type;
+/* Determine a function's address and its return type from its value.
+ If the function is a GNU ifunc, then return the address of the
+ target function, and set *FUNCTION_TYPE to the target function's
+ type, and *RETVAL_TYPE to the target function's return type..
+ Calls error() if the function is not valid for calling. */
+
extern CORE_ADDR find_function_addr (struct value *function,
- struct type **retval_type);
+ struct type **retval_type,
+ struct type **function_type = NULL);
/* Perform a function call in the inferior.
diff --git a/gdb/symtab.h b/gdb/symtab.h
index f9d52e76979..22b52019ee3 100644
--- a/gdb/symtab.h
+++ b/gdb/symtab.h
@@ -1675,6 +1675,12 @@ extern int find_pc_partial_function_gnu_ifunc (CORE_ADDR pc, const char **name,
extern int find_pc_partial_function (CORE_ADDR, const char **, CORE_ADDR *,
CORE_ADDR *);
+/* See if we can figure out the function's actual type from the type
+ that the resolver returns. RESOLVER_FUNADDR is the address of the
+ ifunc resolver. */
+
+extern struct type *find_gnu_ifunc_target_type (CORE_ADDR resolver_funaddr);
+
extern void clear_pc_function_cache (void);
/* Expand symtab containing PC, SECTION if not already expanded. */
diff --git a/gdb/testsuite/gdb.compile/compile-ifunc.exp b/gdb/testsuite/gdb.compile/compile-ifunc.exp
index ed700e416bb..979e39147e7 100644
--- a/gdb/testsuite/gdb.compile/compile-ifunc.exp
+++ b/gdb/testsuite/gdb.compile/compile-ifunc.exp
@@ -37,7 +37,9 @@ with_test_prefix "nodebug" {
}
gdb_test "compile code resultvar = gnu_ifunc (10);" \
- "warning: variable has unknown type; assuming int"
+ [multi_line \
+ "warning: variable has unknown type; assuming int" \
+ "warning: function has unknown return type; assuming int"]
gdb_test "p (int) resultvar" " = 11"
@@ -52,10 +54,9 @@ with_test_prefix "debug" {
if ![runto_main] {
return -1
}
-
# gnu_ifunc (10): error: too many arguments to function 'gnu_ifunc'
- gdb_test_no_output "compile code resultvar = gnu_ifunc_alias (10);"
-
+ gdb_test "compile code resultvar = gnu_ifunc_alias (10);" \
+ "warning: function has unknown return type; assuming int"
gdb_test "p resultvar" " = 11"
}
--
2.14.3
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH v2 03/15] Calling ifunc functions when target has no debug info but resolver has
2018-03-25 19:19 ` [PATCH v2 03/15] Calling ifunc functions when target has no debug info but resolver has Pedro Alves
@ 2018-04-01 4:22 ` Simon Marchi
2018-04-10 21:48 ` Pedro Alves
0 siblings, 1 reply; 33+ messages in thread
From: Simon Marchi @ 2018-04-01 4:22 UTC (permalink / raw)
To: Pedro Alves, gdb-patches
On 2018-03-25 03:19 PM, Pedro Alves wrote:
> In v2:
> - Added testsuite tweak.
>
> After the previous patch, on Fedora 27 (glibc 2.26), if you try
> calling strlen in the inferior, you now get:
>
> (top-gdb) p strlen ("hello")
> '__strlen_avx2' has unknown return type; cast the call to its declared return type
>
> This is correct, because __strlen_avx2 is written in assembly.
>
> We can improve on this though -- if the final ifunc resolved/target
> function has no debug info, but the ifunc _resolver_ does have debug
> info, we can try extracting the final function's type from the type
> that the resolver returns. E.g.,:
>
> typedef size_t (*strlen_t) (const char*);
>
> size_t my_strlen (const char *) { /* some implementation */ }
> strlen_t strlen_resolver (unsigned long hwcap) { return my_strlen; }
>
> extern size_t strlen (const char *s);
> __typeof (strlen) strlen __attribute__ ((ifunc ("strlen_resolver")));
>
> In the strlen example above, the resolver returns strlen_t, which is a
> typedef for pointer to a function that returns size_t. "strlen_t" is
> the type of both the user-visible "strlen", and of the the target
> function that implements it.
>
> This patch teaches GDB to extract that type.
>
> This is done for actual inferior function calls (in infcall.c), and
> for ptype (in eval_call). By the time we get to either of these
> places, we've already lost the original symbol/minsym, and only have
> values and types to work with. Hence the changes to c-exp.y and
> evaluate_var_msym_value, to ensure that we propagate the ifunc
> minsymbol's info.
>
> The change to make ifunc symbols have no/unknown return type exposes a
> latent problem -- gdb.compile/compile-ifunc.exp calls a no-debug-info
> function, but we did not warn about it. The test is fixed by this
> commit too.
As usual, what you did seems to make sense, but I'm a bit lost. I noted
some random comments.
> diff --git a/gdb/blockframe.c b/gdb/blockframe.c
> index 9be8871f756..db02b35742d 100644
> --- a/gdb/blockframe.c
> +++ b/gdb/blockframe.c
> @@ -323,6 +323,40 @@ find_pc_partial_function (CORE_ADDR pc, const char **name, CORE_ADDR *address,
> return find_pc_partial_function_gnu_ifunc (pc, name, address, endaddr, NULL);
> }
>
> +/* See symtab.h. */
> +
> +struct type *
> +find_gnu_ifunc_target_type (CORE_ADDR resolver_funaddr)
> +{
> + /* See if we can figure out the function's return type from the type
> + that the resolver returns. */
> + symbol *sym = find_pc_function (resolver_funaddr);
> + if (sym != NULL
> + && SYMBOL_CLASS (sym) == LOC_BLOCK
> + && BLOCK_START (SYMBOL_BLOCK_VALUE (sym)) == resolver_funaddr)
> + {
This looks a lot like the "find_function_type" function. Maybe it should use it?
> @@ -864,7 +878,11 @@ call_function_by_hand_dummy (struct value *function,
> }
> }
>
> - funaddr = find_function_addr (function, &values_type);
> + struct type *ftype = check_typedef (value_type (function));
> + if (TYPE_CODE (ftype) == TYPE_CODE_PTR)
> + ftype = check_typedef (TYPE_TARGET_TYPE (ftype));
Are these last operations necessary to do here? It seems to me like find_function_addr
will do pretty much the same work and ignore the input value of ftype anyway.
> +
> + funaddr = find_function_addr (function, &values_type, &ftype);
> if (values_type == NULL)
> values_type = default_return_type;
> if (values_type == NULL)
> diff --git a/gdb/infcall.h b/gdb/infcall.h
> index a3861fb1bf3..bea1494b50d 100644
> --- a/gdb/infcall.h
> +++ b/gdb/infcall.h
> @@ -25,8 +25,15 @@
> struct value;
> struct type;
>
> +/* Determine a function's address and its return type from its value.
> + If the function is a GNU ifunc, then return the address of the
> + target function, and set *FUNCTION_TYPE to the target function's
> + type, and *RETVAL_TYPE to the target function's return type..
> + Calls error() if the function is not valid for calling. */
> +
> extern CORE_ADDR find_function_addr (struct value *function,
> - struct type **retval_type);
> + struct type **retval_type,
> + struct type **function_type = NULL);
Isn't the function's return value type always the target type of the function's
type? If so, it seems a bit redundant to have both retval_type and function_type.
The callers easily call TYPE_TARGET_TYPE on *function_type. Or maybe do you see
some situations where we're able to determine the reval_type but not the
function_type, in which case the retval_type would still be relevant?
Simon
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH v2 03/15] Calling ifunc functions when target has no debug info but resolver has
2018-04-01 4:22 ` Simon Marchi
@ 2018-04-10 21:48 ` Pedro Alves
2018-04-10 21:54 ` Pedro Alves
0 siblings, 1 reply; 33+ messages in thread
From: Pedro Alves @ 2018-04-10 21:48 UTC (permalink / raw)
To: Simon Marchi, gdb-patches
On 04/01/2018 05:22 AM, Simon Marchi wrote:
>> +/* See symtab.h. */
>> +
>> +struct type *
>> +find_gnu_ifunc_target_type (CORE_ADDR resolver_funaddr)
>> +{
>> + /* See if we can figure out the function's return type from the type
>> + that the resolver returns. */
>> + symbol *sym = find_pc_function (resolver_funaddr);
>> + if (sym != NULL
>> + && SYMBOL_CLASS (sym) == LOC_BLOCK
>> + && BLOCK_START (SYMBOL_BLOCK_VALUE (sym)) == resolver_funaddr)
>> + {
>
> This looks a lot like the "find_function_type" function. Maybe it should use it?
Good idea. That lives in infcall.c currently, but I moved it along.
>
>> @@ -864,7 +878,11 @@ call_function_by_hand_dummy (struct value *function,
>> }
>> }
>>
>> - funaddr = find_function_addr (function, &values_type);
>> + struct type *ftype = check_typedef (value_type (function));
>> + if (TYPE_CODE (ftype) == TYPE_CODE_PTR)
>> + ftype = check_typedef (TYPE_TARGET_TYPE (ftype));
>
> Are these last operations necessary to do here? It seems to me like find_function_addr
> will do pretty much the same work and ignore the input value of ftype anyway.
You're right, I did this.
>
>> +
>> + funaddr = find_function_addr (function, &values_type, &ftype);
>> if (values_type == NULL)
>> values_type = default_return_type;
>> if (values_type == NULL)
>> diff --git a/gdb/infcall.h b/gdb/infcall.h
>> index a3861fb1bf3..bea1494b50d 100644
>> --- a/gdb/infcall.h
>> +++ b/gdb/infcall.h
>> @@ -25,8 +25,15 @@
>> struct value;
>> struct type;
>>
>> +/* Determine a function's address and its return type from its value.
>> + If the function is a GNU ifunc, then return the address of the
>> + target function, and set *FUNCTION_TYPE to the target function's
>> + type, and *RETVAL_TYPE to the target function's return type..
>> + Calls error() if the function is not valid for calling. */
>> +
>> extern CORE_ADDR find_function_addr (struct value *function,
>> - struct type **retval_type);
>> + struct type **retval_type,
>> + struct type **function_type = NULL);
>
> Isn't the function's return value type always the target type of the function's
> type? If so, it seems a bit redundant to have both retval_type and function_type.
> The callers easily call TYPE_TARGET_TYPE on *function_type. Or maybe do you see
> some situations where we're able to determine the reval_type but not the
> function_type, in which case the retval_type would still be relevant?
Yeah, find_function_addr handles a case where the function's type is not
really a function, but an integer:
else if (TYPE_CODE (ftype) == TYPE_CODE_INT)
{
This is reached when you call a function by address, like
(gdb) print 0x1()
In this case, the caller wouldn't be able to use
TYPE_TARGET_TYPE on function_type. Instead it gets a
NULL retval_type.
I think it may be possible to handle that case by making
it return the type we use for non-debug functions in function_type:
objfile_type->nodebug_text_symbol
= init_type (objfile, TYPE_CODE_FUNC, TARGET_CHAR_BIT,
"<text variable, no debug info>");
but that type is currently tied to an objfile, seemingly for no
good reason. But I'm not sure. It'd needs experimentation
and reworking nodebug_text_symbol, which I'd prefer to leave
for another day.
Here's what I'm squashing into the patch locally.
From 2e89a1ef27e7eac203092346da942d16bc5a606c Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Tue, 10 Apr 2018 22:46:49 +0100
Subject: [PATCH] find_function_type
---
gdb/blockframe.c | 47 ++++++++++++++++++++++++++---------------------
gdb/infcall.c | 24 ++++--------------------
gdb/infcall.h | 2 +-
gdb/symtab.h | 5 +++++
4 files changed, 36 insertions(+), 42 deletions(-)
diff --git a/gdb/blockframe.c b/gdb/blockframe.c
index db02b35742d..e6938a341ae 100644
--- a/gdb/blockframe.c
+++ b/gdb/blockframe.c
@@ -325,32 +325,37 @@ find_pc_partial_function (CORE_ADDR pc, const char **name, CORE_ADDR *address,
/* See symtab.h. */
+struct type *
+find_function_type (CORE_ADDR pc)
+{
+ struct symbol *sym = find_pc_function (pc);
+
+ if (sym != NULL && BLOCK_START (SYMBOL_BLOCK_VALUE (sym)) == pc)
+ return SYMBOL_TYPE (sym);
+
+ return NULL;
+}
+
+/* See symtab.h. */
+
struct type *
find_gnu_ifunc_target_type (CORE_ADDR resolver_funaddr)
{
- /* See if we can figure out the function's return type from the type
- that the resolver returns. */
- symbol *sym = find_pc_function (resolver_funaddr);
- if (sym != NULL
- && SYMBOL_CLASS (sym) == LOC_BLOCK
- && BLOCK_START (SYMBOL_BLOCK_VALUE (sym)) == resolver_funaddr)
+ struct type *resolver_type = find_function_type (resolver_funaddr);
+ if (resolver_type != NULL)
{
- struct type *resolver_type = SYMBOL_TYPE (sym);
- if (TYPE_CODE (resolver_type) == TYPE_CODE_FUNC)
+ /* Get the return type of the resolver. */
+ struct type *resolver_ret_type
+ = check_typedef (TYPE_TARGET_TYPE (resolver_type));
+
+ /* If we found a pointer to function, then the resolved type
+ is the type of the pointed-to function. */
+ if (TYPE_CODE (resolver_ret_type) == TYPE_CODE_PTR)
{
- /* Get the return type of the resolver. */
- struct type *resolver_ret_type
- = check_typedef (TYPE_TARGET_TYPE (resolver_type));
-
- /* If we found a pointer to function, then the resolved type
- is the type of the pointed-to function. */
- if (TYPE_CODE (resolver_ret_type) == TYPE_CODE_PTR)
- {
- struct type *resolved_type
- = TYPE_TARGET_TYPE (resolver_ret_type);
- if (TYPE_CODE (check_typedef (resolved_type)) == TYPE_CODE_FUNC)
- return resolved_type;
- }
+ struct type *resolved_type
+ = TYPE_TARGET_TYPE (resolver_ret_type);
+ if (TYPE_CODE (check_typedef (resolved_type)) == TYPE_CODE_FUNC)
+ return resolved_type;
}
}
diff --git a/gdb/infcall.c b/gdb/infcall.c
index d89d8ca7e8c..b233e369f27 100644
--- a/gdb/infcall.c
+++ b/gdb/infcall.c
@@ -229,20 +229,6 @@ value_arg_coerce (struct gdbarch *gdbarch, struct value *arg,
return value_cast (type, arg);
}
-/* Return the type of a function with its first instruction exactly at
- the PC address. Return NULL otherwise. */
-
-static struct type *
-find_function_type (CORE_ADDR pc)
-{
- struct symbol *sym = find_pc_function (pc);
-
- if (sym != NULL && BLOCK_START (SYMBOL_BLOCK_VALUE (sym)) == pc)
- return SYMBOL_TYPE (sym);
-
- return NULL;
-}
-
/* See infcall.h. */
CORE_ADDR
@@ -737,13 +723,12 @@ call_function_by_hand_dummy (struct value *function,
void *dummy_dtor_data)
{
CORE_ADDR sp;
- struct type *values_type, *target_values_type;
+ struct type *target_values_type;
unsigned char struct_return = 0, hidden_first_param_p = 0;
CORE_ADDR struct_addr = 0;
struct infcall_control_state *inf_status;
struct cleanup *inf_status_cleanup;
struct infcall_suspend_state *caller_state;
- CORE_ADDR funaddr;
CORE_ADDR real_pc;
CORE_ADDR bp_addr;
struct frame_id dummy_id;
@@ -878,11 +863,10 @@ call_function_by_hand_dummy (struct value *function,
}
}
- struct type *ftype = check_typedef (value_type (function));
- if (TYPE_CODE (ftype) == TYPE_CODE_PTR)
- ftype = check_typedef (TYPE_TARGET_TYPE (ftype));
+ type *ftype;
+ type *values_type;
+ CORE_ADDR funaddr = find_function_addr (function, &values_type, &ftype);
- funaddr = find_function_addr (function, &values_type, &ftype);
if (values_type == NULL)
values_type = default_return_type;
if (values_type == NULL)
diff --git a/gdb/infcall.h b/gdb/infcall.h
index bea1494b50d..8b2195019c9 100644
--- a/gdb/infcall.h
+++ b/gdb/infcall.h
@@ -28,7 +28,7 @@ struct type;
/* Determine a function's address and its return type from its value.
If the function is a GNU ifunc, then return the address of the
target function, and set *FUNCTION_TYPE to the target function's
- type, and *RETVAL_TYPE to the target function's return type..
+ type, and *RETVAL_TYPE to the target function's return type.
Calls error() if the function is not valid for calling. */
extern CORE_ADDR find_function_addr (struct value *function,
diff --git a/gdb/symtab.h b/gdb/symtab.h
index 22b52019ee3..83ff6f226d8 100644
--- a/gdb/symtab.h
+++ b/gdb/symtab.h
@@ -1675,6 +1675,11 @@ extern int find_pc_partial_function_gnu_ifunc (CORE_ADDR pc, const char **name,
extern int find_pc_partial_function (CORE_ADDR, const char **, CORE_ADDR *,
CORE_ADDR *);
+/* Return the type of a function with its first instruction exactly at
+ the PC address. Return NULL otherwise. */
+
+extern struct type *find_function_type (CORE_ADDR pc);
+
/* See if we can figure out the function's actual type from the type
that the resolver returns. RESOLVER_FUNADDR is the address of the
ifunc resolver. */
--
2.14.3
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH v2 03/15] Calling ifunc functions when target has no debug info but resolver has
2018-04-10 21:48 ` Pedro Alves
@ 2018-04-10 21:54 ` Pedro Alves
0 siblings, 0 replies; 33+ messages in thread
From: Pedro Alves @ 2018-04-10 21:54 UTC (permalink / raw)
To: Simon Marchi, gdb-patches
On 04/10/2018 10:48 PM, Pedro Alves wrote:
>
> Here's what I'm squashing into the patch locally.
And here's the result.
I've also pushed the refreshed patches to users/palves/ifunc.
From 7a6a9fdd7504b3098e9a7019dff945e579bde371 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Tue, 10 Apr 2018 22:48:46 +0100
Subject: [PATCH] Calling ifunc functions when target has no debug info but
resolver has
After the previous patch, on Fedora 27 (glibc 2.26), if you try
calling strlen in the inferior, you now get:
(top-gdb) p strlen ("hello")
'__strlen_avx2' has unknown return type; cast the call to its declared return type
This is correct, because __strlen_avx2 is written in assembly.
We can improve on this though -- if the final ifunc resolved/target
function has no debug info, but the ifunc _resolver_ does have debug
info, we can try extracting the final function's type from the type
that the resolver returns. E.g.,:
typedef size_t (*strlen_t) (const char*);
size_t my_strlen (const char *) { /* some implementation */ }
strlen_t strlen_resolver (unsigned long hwcap) { return my_strlen; }
extern size_t strlen (const char *s);
__typeof (strlen) strlen __attribute__ ((ifunc ("strlen_resolver")));
In the strlen example above, the resolver returns strlen_t, which is a
typedef for pointer to a function that returns size_t. "strlen_t" is
the type of both the user-visible "strlen", and of the the target
function that implements it.
This patch teaches GDB to extract that type.
This is done for actual inferior function calls (in infcall.c), and
for ptype (in eval_call). By the time we get to either of these
places, we've already lost the original symbol/minsym, and only have
values and types to work with. Hence the changes to c-exp.y and
evaluate_var_msym_value, to ensure that we propagate the ifunc
minsymbol's info.
The change to make ifunc symbols have no/unknown return type exposes a
latent problem -- gdb.compile/compile-ifunc.exp calls a no-debug-info
function, but we did not warn about it. The test is fixed by this
commit too.
gdb/ChangeLog:
yyyy-mm-dd Pedro Alves <palves@redhat.com>
* blockframe.c (find_gnu_ifunc_target_type): New function.
(find_function_type): New.
* eval.c (evaluate_var_msym_value): For GNU ifunc types, always
return a value with a memory address.
(eval_call): For calls to GNU ifunc functions, try to find the
type of the target function from the type that the resolver
returns.
* gdbtypes.c (objfile_type): Don't install a return type for ifunc
symbols.
* infcall.c (find_function_return_type): Delete.
(find_function_addr): Add 'function_type' parameter. For calls to
GNU ifunc functions, try to find the type of the target function
from the type that the resolver returns, and return it via
FUNCTION_TYPE.
(call_function_by_hand_dummy): Adjust to use the function type
returned by find_function_addr.
(find_function_addr): Add 'function_type' parameter and move
description here.
* symtab.h (find_function_type, find_gnu_ifunc_target_type): New
declarations.
gdb/testsuite/ChangeLog:
yyyy-mm-dd Pedro Alves <palves@redhat.com>
* gdb.compile/compile-ifunc.exp: Also expect "function has unknown
return type" warnings.
---
gdb/blockframe.c | 39 +++++++++++++++++
gdb/eval.c | 25 ++++++-----
gdb/gdbtypes.c | 4 --
gdb/infcall.c | 66 +++++++++++++++--------------
gdb/infcall.h | 9 +++-
gdb/symtab.h | 11 +++++
gdb/testsuite/gdb.compile/compile-ifunc.exp | 9 ++--
7 files changed, 112 insertions(+), 51 deletions(-)
diff --git a/gdb/blockframe.c b/gdb/blockframe.c
index 9be8871f756..e6938a341ae 100644
--- a/gdb/blockframe.c
+++ b/gdb/blockframe.c
@@ -323,6 +323,45 @@ find_pc_partial_function (CORE_ADDR pc, const char **name, CORE_ADDR *address,
return find_pc_partial_function_gnu_ifunc (pc, name, address, endaddr, NULL);
}
+/* See symtab.h. */
+
+struct type *
+find_function_type (CORE_ADDR pc)
+{
+ struct symbol *sym = find_pc_function (pc);
+
+ if (sym != NULL && BLOCK_START (SYMBOL_BLOCK_VALUE (sym)) == pc)
+ return SYMBOL_TYPE (sym);
+
+ return NULL;
+}
+
+/* See symtab.h. */
+
+struct type *
+find_gnu_ifunc_target_type (CORE_ADDR resolver_funaddr)
+{
+ struct type *resolver_type = find_function_type (resolver_funaddr);
+ if (resolver_type != NULL)
+ {
+ /* Get the return type of the resolver. */
+ struct type *resolver_ret_type
+ = check_typedef (TYPE_TARGET_TYPE (resolver_type));
+
+ /* If we found a pointer to function, then the resolved type
+ is the type of the pointed-to function. */
+ if (TYPE_CODE (resolver_ret_type) == TYPE_CODE_PTR)
+ {
+ struct type *resolved_type
+ = TYPE_TARGET_TYPE (resolver_ret_type);
+ if (TYPE_CODE (check_typedef (resolved_type)) == TYPE_CODE_FUNC)
+ return resolved_type;
+ }
+ }
+
+ return NULL;
+}
+
/* Return the innermost stack frame that is executing inside of BLOCK and is
at least as old as the selected frame. Return NULL if there is no
such frame. If BLOCK is NULL, just return NULL. */
diff --git a/gdb/eval.c b/gdb/eval.c
index b6fbfcf6c95..ad66f7c32c1 100644
--- a/gdb/eval.c
+++ b/gdb/eval.c
@@ -734,17 +734,13 @@ value *
evaluate_var_msym_value (enum noside noside,
struct objfile *objfile, minimal_symbol *msymbol)
{
- if (noside == EVAL_AVOID_SIDE_EFFECTS)
- {
- type *the_type = find_minsym_type_and_address (msymbol, objfile, NULL);
- return value_zero (the_type, not_lval);
- }
+ CORE_ADDR address;
+ type *the_type = find_minsym_type_and_address (msymbol, objfile, &address);
+
+ if (noside == EVAL_AVOID_SIDE_EFFECTS && !TYPE_GNU_IFUNC (the_type))
+ return value_zero (the_type, not_lval);
else
- {
- CORE_ADDR address;
- type *the_type = find_minsym_type_and_address (msymbol, objfile, &address);
- return value_at_lazy (the_type, address);
- }
+ return value_at_lazy (the_type, address);
}
/* Helper for returning a value when handling EVAL_SKIP. */
@@ -797,6 +793,15 @@ eval_call (expression *exp, enum noside noside,
else if (TYPE_CODE (ftype) == TYPE_CODE_FUNC
|| TYPE_CODE (ftype) == TYPE_CODE_METHOD)
{
+ if (TYPE_GNU_IFUNC (ftype))
+ {
+ CORE_ADDR address = value_address (argvec[0]);
+ type *resolved_type = find_gnu_ifunc_target_type (address);
+
+ if (resolved_type != NULL)
+ ftype = resolved_type;
+ }
+
type *return_type = TYPE_TARGET_TYPE (ftype);
if (return_type == NULL)
diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
index b3a037971e0..2efd1264ff8 100644
--- a/gdb/gdbtypes.c
+++ b/gdb/gdbtypes.c
@@ -5443,10 +5443,6 @@ objfile_type (struct objfile *objfile)
objfile_type->nodebug_text_gnu_ifunc_symbol
= init_type (objfile, TYPE_CODE_FUNC, TARGET_CHAR_BIT,
"<text gnu-indirect-function variable, no debug info>");
- /* Ifunc resolvers return a function address. */
- TYPE_TARGET_TYPE (objfile_type->nodebug_text_gnu_ifunc_symbol)
- = init_integer_type (objfile, gdbarch_addr_bit (gdbarch), 1,
- "__IFUNC_RESOLVER_RET");
TYPE_GNU_IFUNC (objfile_type->nodebug_text_gnu_ifunc_symbol) = 1;
objfile_type->nodebug_got_plt_symbol
= init_pointer_type (objfile, gdbarch_addr_bit (gdbarch),
diff --git a/gdb/infcall.c b/gdb/infcall.c
index 9f02674bdf2..b233e369f27 100644
--- a/gdb/infcall.c
+++ b/gdb/infcall.c
@@ -229,26 +229,12 @@ value_arg_coerce (struct gdbarch *gdbarch, struct value *arg,
return value_cast (type, arg);
}
-/* Return the return type of a function with its first instruction exactly at
- the PC address. Return NULL otherwise. */
-
-static struct type *
-find_function_return_type (CORE_ADDR pc)
-{
- struct symbol *sym = find_pc_function (pc);
-
- if (sym != NULL && BLOCK_START (SYMBOL_BLOCK_VALUE (sym)) == pc
- && SYMBOL_TYPE (sym) != NULL)
- return TYPE_TARGET_TYPE (SYMBOL_TYPE (sym));
-
- return NULL;
-}
-
-/* Determine a function's address and its return type from its value.
- Calls error() if the function is not valid for calling. */
+/* See infcall.h. */
CORE_ADDR
-find_function_addr (struct value *function, struct type **retval_type)
+find_function_addr (struct value *function,
+ struct type **retval_type,
+ struct type **function_type)
{
struct type *ftype = check_typedef (value_type (function));
struct gdbarch *gdbarch = get_type_arch (ftype);
@@ -275,17 +261,33 @@ find_function_addr (struct value *function, struct type **retval_type)
if (TYPE_CODE (ftype) == TYPE_CODE_FUNC
|| TYPE_CODE (ftype) == TYPE_CODE_METHOD)
{
- value_type = TYPE_TARGET_TYPE (ftype);
-
if (TYPE_GNU_IFUNC (ftype))
{
- funaddr = gnu_ifunc_resolve_addr (gdbarch, funaddr);
+ CORE_ADDR resolver_addr = funaddr;
- /* Skip querying the function symbol if no RETVAL_TYPE has been
- asked for. */
- if (retval_type)
- value_type = find_function_return_type (funaddr);
+ /* Resolve the ifunc. Note this may call the resolver
+ function in the inferior. */
+ funaddr = gnu_ifunc_resolve_addr (gdbarch, resolver_addr);
+
+ /* Skip querying the function symbol if no RETVAL_TYPE or
+ FUNCTION_TYPE have been asked for. */
+ if (retval_type != NULL || function_type != NULL)
+ {
+ type *target_ftype = find_function_type (funaddr);
+ /* If we don't have debug info for the target function,
+ see if we can instead extract the target function's
+ type from the type that the resolver returns. */
+ if (target_ftype == NULL)
+ target_ftype = find_gnu_ifunc_target_type (resolver_addr);
+ if (target_ftype != NULL)
+ {
+ value_type = TYPE_TARGET_TYPE (check_typedef (target_ftype));
+ ftype = target_ftype;
+ }
+ }
}
+ else
+ value_type = TYPE_TARGET_TYPE (ftype);
}
else if (TYPE_CODE (ftype) == TYPE_CODE_INT)
{
@@ -320,6 +322,8 @@ find_function_addr (struct value *function, struct type **retval_type)
if (retval_type != NULL)
*retval_type = value_type;
+ if (function_type != NULL)
+ *function_type = ftype;
return funaddr + gdbarch_deprecated_function_start_offset (gdbarch);
}
@@ -719,15 +723,13 @@ call_function_by_hand_dummy (struct value *function,
void *dummy_dtor_data)
{
CORE_ADDR sp;
- struct type *values_type, *target_values_type;
+ struct type *target_values_type;
unsigned char struct_return = 0, hidden_first_param_p = 0;
CORE_ADDR struct_addr = 0;
struct infcall_control_state *inf_status;
struct cleanup *inf_status_cleanup;
struct infcall_suspend_state *caller_state;
- CORE_ADDR funaddr;
CORE_ADDR real_pc;
- struct type *ftype = check_typedef (value_type (function));
CORE_ADDR bp_addr;
struct frame_id dummy_id;
struct frame_info *frame;
@@ -738,9 +740,6 @@ call_function_by_hand_dummy (struct value *function,
char name_buf[RAW_FUNCTION_ADDRESS_SIZE];
bool stack_temporaries = thread_stack_temporaries_enabled_p (inferior_ptid);
- if (TYPE_CODE (ftype) == TYPE_CODE_PTR)
- ftype = check_typedef (TYPE_TARGET_TYPE (ftype));
-
if (!target_has_execution)
noprocess ();
@@ -864,7 +863,10 @@ call_function_by_hand_dummy (struct value *function,
}
}
- funaddr = find_function_addr (function, &values_type);
+ type *ftype;
+ type *values_type;
+ CORE_ADDR funaddr = find_function_addr (function, &values_type, &ftype);
+
if (values_type == NULL)
values_type = default_return_type;
if (values_type == NULL)
diff --git a/gdb/infcall.h b/gdb/infcall.h
index a3861fb1bf3..8b2195019c9 100644
--- a/gdb/infcall.h
+++ b/gdb/infcall.h
@@ -25,8 +25,15 @@
struct value;
struct type;
+/* Determine a function's address and its return type from its value.
+ If the function is a GNU ifunc, then return the address of the
+ target function, and set *FUNCTION_TYPE to the target function's
+ type, and *RETVAL_TYPE to the target function's return type.
+ Calls error() if the function is not valid for calling. */
+
extern CORE_ADDR find_function_addr (struct value *function,
- struct type **retval_type);
+ struct type **retval_type,
+ struct type **function_type = NULL);
/* Perform a function call in the inferior.
diff --git a/gdb/symtab.h b/gdb/symtab.h
index f9d52e76979..83ff6f226d8 100644
--- a/gdb/symtab.h
+++ b/gdb/symtab.h
@@ -1675,6 +1675,17 @@ extern int find_pc_partial_function_gnu_ifunc (CORE_ADDR pc, const char **name,
extern int find_pc_partial_function (CORE_ADDR, const char **, CORE_ADDR *,
CORE_ADDR *);
+/* Return the type of a function with its first instruction exactly at
+ the PC address. Return NULL otherwise. */
+
+extern struct type *find_function_type (CORE_ADDR pc);
+
+/* See if we can figure out the function's actual type from the type
+ that the resolver returns. RESOLVER_FUNADDR is the address of the
+ ifunc resolver. */
+
+extern struct type *find_gnu_ifunc_target_type (CORE_ADDR resolver_funaddr);
+
extern void clear_pc_function_cache (void);
/* Expand symtab containing PC, SECTION if not already expanded. */
diff --git a/gdb/testsuite/gdb.compile/compile-ifunc.exp b/gdb/testsuite/gdb.compile/compile-ifunc.exp
index ed700e416bb..979e39147e7 100644
--- a/gdb/testsuite/gdb.compile/compile-ifunc.exp
+++ b/gdb/testsuite/gdb.compile/compile-ifunc.exp
@@ -37,7 +37,9 @@ with_test_prefix "nodebug" {
}
gdb_test "compile code resultvar = gnu_ifunc (10);" \
- "warning: variable has unknown type; assuming int"
+ [multi_line \
+ "warning: variable has unknown type; assuming int" \
+ "warning: function has unknown return type; assuming int"]
gdb_test "p (int) resultvar" " = 11"
@@ -52,10 +54,9 @@ with_test_prefix "debug" {
if ![runto_main] {
return -1
}
-
# gnu_ifunc (10): error: too many arguments to function 'gnu_ifunc'
- gdb_test_no_output "compile code resultvar = gnu_ifunc_alias (10);"
-
+ gdb_test "compile code resultvar = gnu_ifunc_alias (10);" \
+ "warning: function has unknown return type; assuming int"
gdb_test "p resultvar" " = 11"
}
--
2.14.3
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH v2 01/15] Fix breakpoints in ifunc after inferior resolved it (@got.plt symbol creation)
2018-03-25 19:19 [PATCH v2 00/15] Fixing GNU ifunc support Pedro Alves
` (2 preceding siblings ...)
2018-03-25 19:19 ` [PATCH v2 03/15] Calling ifunc functions when target has no debug info but resolver has Pedro Alves
@ 2018-03-25 19:19 ` Pedro Alves
2018-04-01 3:35 ` Simon Marchi
2018-03-25 19:19 ` [PATCH v2 02/15] Fix calling ifunc functions when resolver has debug info and different name Pedro Alves
` (11 subsequent siblings)
15 siblings, 1 reply; 33+ messages in thread
From: Pedro Alves @ 2018-03-25 19:19 UTC (permalink / raw)
To: gdb-patches
In v2: - reworked to keep supporting rel.plt sections for .plt, and
to handle jump slot offsets pointing to .plt.
Setting a breakpoint on an ifunc symbol after the ifunc has already
been resolved by the inferior should result in creating a breakpoint
location at the ifunc target. However, that's not what happens on
current Fedora:
(gdb) n
53 i = gnu_ifunc (1); /* break-at-call */
(gdb)
54 assert (i == 2);
(gdb) b gnu_ifunc
Breakpoint 2 at gnu-indirect-function resolver at 0x7ffff7bd36ee
(gdb) info breakpoints
Num Type Disp Enb Address What
2 STT_GNU_IFUNC resolver keep y 0x00007ffff7bd36ee <gnu_ifunc+4>
The problem is that elf_gnu_ifunc_resolve_by_got never manages to
resolve an ifunc target. The reason is that GDB never actually
creates the internal got.plt symbols:
(gdb) p 'gnu_ifunc@got.plt'
No symbol "gnu_ifunc@got.plt" in current context.
and this is because GDB expects that rela.plt has relocations for
.plt, while it actually has relocations for .got.plt:
Relocation section [10] '.rela.plt' for section [22] '.got.plt' at offset 0x570 contains 2 entries:
Offset Type Value Addend Name
0x0000000000601018 X86_64_JUMP_SLOT 000000000000000000 +0 __assert_fail
0x0000000000601020 X86_64_JUMP_SLOT 000000000000000000 +0 gnu_ifunc
Using an older system on the GCC compile farm (machine gcc15, an
x86-64 running Debian 6.0.8, with GNU ld 2.20.1), we see that it used
to be that we'd get a .rela.plt section for .plt:
Relocation section [ 9] '.rela.plt' for section [11] '.plt' at offset 0x578 contains 3 entries:
Offset Type Value Addend Name
0x0000000000600cc0 X86_64_JUMP_SLOT 000000000000000000 +0 __assert_fail
0x0000000000600cc8 X86_64_JUMP_SLOT 000000000000000000 +0 __libc_start_main
0x0000000000600cd0 X86_64_JUMP_SLOT 000000000000000000 +0 gnu_ifunc
Those offsets did point into .got.plt, as seen with objdump -h:
20 .got.plt 00000030 0000000000600ca8 0000000000600ca8 00000ca8 2**3
CONTENTS, ALLOC, LOAD, DATA
I also tested on gcc110 on the compile farm (PPC64 running CentOS
7.4.1708, with GNU ld 2.25.1), and there we see instead:
Relocation section [ 9] '.rela.plt' for section [23] '.plt' at offset 0x5d0 contains 4 entries:
Offset Type Value Addend Name
0x0000000010020148 PPC64_JMP_SLOT 000000000000000000 +0 __libc_start_main
0x0000000010020160 PPC64_JMP_SLOT 000000000000000000 +0 __gmon_start__
0x0000000010020178 PPC64_JMP_SLOT 000000000000000000 +0 __assert_fail
0x0000000010020190 PPC64_JMP_SLOT 000000000000000000 +0 gnu_ifunc
But note that those offsets point into .plt, not .got.plt, as seen
with objdump -h:
22 .plt 00000078 0000000010020130 0000000010020130 00010130 2**3
ALLOC
This commit makes us support all the different combinations above.
With that addressed, we now get:
(gdb) p 'gnu_ifunc@got.plt'
$1 = (<text from jump slot in .got.plt, no debug info>) 0x400753 <final>
And setting a breakpoint on the ifunc finds the ifunc target:
(gdb) b gnu_ifunc
Breakpoint 2 at 0x400753
(gdb) info breakpoints
Num Type Disp Enb Address What
2 breakpoint keep y 0x0000000000400753 <final>
gdb/ChangeLog:
yyyy-mm-dd Pedro Alves <palves@redhat.com>
* elfread.c (elf_rel_plt_read): Look for relocations for .got.plt too.
not .plt.
---
gdb/elfread.c | 64 +++++++++++++++++++++++++++++++++++++++++++----------------
1 file changed, 47 insertions(+), 17 deletions(-)
diff --git a/gdb/elfread.c b/gdb/elfread.c
index 260789062d0..9ffbf99db6e 100644
--- a/gdb/elfread.c
+++ b/gdb/elfread.c
@@ -535,8 +535,7 @@ elf_rel_plt_read (minimal_symbol_reader &reader,
{
bfd *obfd = objfile->obfd;
const struct elf_backend_data *bed = get_elf_backend_data (obfd);
- asection *plt, *relplt, *got_plt;
- int plt_elf_idx;
+ asection *relplt, *got_plt;
bfd_size_type reloc_count, reloc;
struct gdbarch *gdbarch = get_objfile_arch (objfile);
struct type *ptr_type = builtin_type (gdbarch)->builtin_data_ptr;
@@ -545,11 +544,6 @@ elf_rel_plt_read (minimal_symbol_reader &reader,
if (objfile->separate_debug_objfile_backlink)
return;
- plt = bfd_get_section_by_name (obfd, ".plt");
- if (plt == NULL)
- return;
- plt_elf_idx = elf_section_data (plt)->this_idx;
-
got_plt = bfd_get_section_by_name (obfd, ".got.plt");
if (got_plt == NULL)
{
@@ -559,12 +553,31 @@ elf_rel_plt_read (minimal_symbol_reader &reader,
return;
}
+ /* Depending on system, we may find jump slots in a relocation
+ section for either .got.plt or .plt. */
+ asection *plt = bfd_get_section_by_name (obfd, ".plt");
+ int plt_elf_idx = (plt != NULL) ? elf_section_data (plt)->this_idx : -1;
+
+ int got_plt_elf_idx = elf_section_data (got_plt)->this_idx;
+
/* This search algorithm is from _bfd_elf_canonicalize_dynamic_reloc. */
for (relplt = obfd->sections; relplt != NULL; relplt = relplt->next)
- if (elf_section_data (relplt)->this_hdr.sh_info == plt_elf_idx
- && (elf_section_data (relplt)->this_hdr.sh_type == SHT_REL
- || elf_section_data (relplt)->this_hdr.sh_type == SHT_RELA))
- break;
+ {
+ const auto &this_hdr = elf_section_data (relplt)->this_hdr;
+
+ if (this_hdr.sh_type == SHT_REL || this_hdr.sh_type == SHT_RELA)
+ {
+ asection *target_section = NULL;
+
+ if (this_hdr.sh_info == plt_elf_idx)
+ target_section = plt;
+ else if (this_hdr.sh_info == got_plt_elf_idx)
+ target_section = got_plt;
+
+ if (target_section != NULL)
+ break;
+ }
+ }
if (relplt == NULL)
return;
@@ -573,6 +586,18 @@ elf_rel_plt_read (minimal_symbol_reader &reader,
std::string string_buffer;
+ /* Does ADDRESS reside in SECTION of OBFD? */
+ auto within_section = [obfd] (asection *section, CORE_ADDR address)
+ {
+ if (section == NULL)
+ return false;
+
+ /* Does the pointer reside in the .got.plt section? */
+ return (bfd_get_section_vma (obfd, section) <= address
+ && (address < bfd_get_section_vma (obfd, section)
+ + bfd_get_section_size (section)));
+ };
+
reloc_count = relplt->size / elf_section_data (relplt)->this_hdr.sh_entsize;
for (reloc = 0; reloc < reloc_count; reloc++)
{
@@ -585,10 +610,15 @@ elf_rel_plt_read (minimal_symbol_reader &reader,
name = bfd_asymbol_name (*relplt->relocation[reloc].sym_ptr_ptr);
address = relplt->relocation[reloc].address;
- /* Does the pointer reside in the .got.plt section? */
- if (!(bfd_get_section_vma (obfd, got_plt) <= address
- && address < bfd_get_section_vma (obfd, got_plt)
- + bfd_get_section_size (got_plt)))
+ asection *msym_section;
+
+ /* Does the pointer reside in either the .got.plt or .plt
+ sections? */
+ if (within_section (got_plt, address))
+ msym_section = got_plt;
+ else if (within_section (plt, address))
+ msym_section = plt;
+ else
continue;
/* We cannot check if NAME is a reference to mst_text_gnu_ifunc as in
@@ -600,8 +630,8 @@ elf_rel_plt_read (minimal_symbol_reader &reader,
msym = record_minimal_symbol (reader, string_buffer.c_str (),
string_buffer.size (),
- true, address, mst_slot_got_plt, got_plt,
- objfile);
+ true, address, mst_slot_got_plt,
+ msym_section, objfile);
if (msym)
SET_MSYMBOL_SIZE (msym, ptr_size);
}
--
2.14.3
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH v2 01/15] Fix breakpoints in ifunc after inferior resolved it (@got.plt symbol creation)
2018-03-25 19:19 ` [PATCH v2 01/15] Fix breakpoints in ifunc after inferior resolved it (@got.plt symbol creation) Pedro Alves
@ 2018-04-01 3:35 ` Simon Marchi
2018-04-10 21:20 ` Pedro Alves
0 siblings, 1 reply; 33+ messages in thread
From: Simon Marchi @ 2018-04-01 3:35 UTC (permalink / raw)
To: Pedro Alves, gdb-patches
On 2018-03-25 03:19 PM, Pedro Alves wrote:
> In v2: - reworked to keep supporting rel.plt sections for .plt, and
> to handle jump slot offsets pointing to .plt.
>
> Setting a breakpoint on an ifunc symbol after the ifunc has already
> been resolved by the inferior should result in creating a breakpoint
> location at the ifunc target. However, that's not what happens on
> current Fedora:
>
> (gdb) n
> 53 i = gnu_ifunc (1); /* break-at-call */
> (gdb)
> 54 assert (i == 2);
> (gdb) b gnu_ifunc
> Breakpoint 2 at gnu-indirect-function resolver at 0x7ffff7bd36ee
> (gdb) info breakpoints
> Num Type Disp Enb Address What
> 2 STT_GNU_IFUNC resolver keep y 0x00007ffff7bd36ee <gnu_ifunc+4>
>
> The problem is that elf_gnu_ifunc_resolve_by_got never manages to
> resolve an ifunc target. The reason is that GDB never actually
> creates the internal got.plt symbols:
>
> (gdb) p 'gnu_ifunc@got.plt'
> No symbol "gnu_ifunc@got.plt" in current context.
>
> and this is because GDB expects that rela.plt has relocations for
> .plt, while it actually has relocations for .got.plt:
>
> Relocation section [10] '.rela.plt' for section [22] '.got.plt' at offset 0x570 contains 2 entries:
> Offset Type Value Addend Name
> 0x0000000000601018 X86_64_JUMP_SLOT 000000000000000000 +0 __assert_fail
> 0x0000000000601020 X86_64_JUMP_SLOT 000000000000000000 +0 gnu_ifunc
>
>
> Using an older system on the GCC compile farm (machine gcc15, an
> x86-64 running Debian 6.0.8, with GNU ld 2.20.1), we see that it used
> to be that we'd get a .rela.plt section for .plt:
>
> Relocation section [ 9] '.rela.plt' for section [11] '.plt' at offset 0x578 contains 3 entries:
> Offset Type Value Addend Name
> 0x0000000000600cc0 X86_64_JUMP_SLOT 000000000000000000 +0 __assert_fail
> 0x0000000000600cc8 X86_64_JUMP_SLOT 000000000000000000 +0 __libc_start_main
> 0x0000000000600cd0 X86_64_JUMP_SLOT 000000000000000000 +0 gnu_ifunc
>
> Those offsets did point into .got.plt, as seen with objdump -h:
>
> 20 .got.plt 00000030 0000000000600ca8 0000000000600ca8 00000ca8 2**3
> CONTENTS, ALLOC, LOAD, DATA
>
> I also tested on gcc110 on the compile farm (PPC64 running CentOS
> 7.4.1708, with GNU ld 2.25.1), and there we see instead:
>
> Relocation section [ 9] '.rela.plt' for section [23] '.plt' at offset 0x5d0 contains 4 entries:
> Offset Type Value Addend Name
> 0x0000000010020148 PPC64_JMP_SLOT 000000000000000000 +0 __libc_start_main
> 0x0000000010020160 PPC64_JMP_SLOT 000000000000000000 +0 __gmon_start__
> 0x0000000010020178 PPC64_JMP_SLOT 000000000000000000 +0 __assert_fail
> 0x0000000010020190 PPC64_JMP_SLOT 000000000000000000 +0 gnu_ifunc
>
> But note that those offsets point into .plt, not .got.plt, as seen
> with objdump -h:
>
> 22 .plt 00000078 0000000010020130 0000000010020130 00010130 2**3
> ALLOC
>
> This commit makes us support all the different combinations above.
>
> With that addressed, we now get:
>
> (gdb) p 'gnu_ifunc@got.plt'
> $1 = (<text from jump slot in .got.plt, no debug info>) 0x400753 <final>
>
> And setting a breakpoint on the ifunc finds the ifunc target:
>
> (gdb) b gnu_ifunc
> Breakpoint 2 at 0x400753
> (gdb) info breakpoints
> Num Type Disp Enb Address What
> 2 breakpoint keep y 0x0000000000400753 <final>
>
> gdb/ChangeLog:
> yyyy-mm-dd Pedro Alves <palves@redhat.com>
>
> * elfread.c (elf_rel_plt_read): Look for relocations for .got.plt too.
> not .plt.
> ---
> gdb/elfread.c | 64 +++++++++++++++++++++++++++++++++++++++++++----------------
> 1 file changed, 47 insertions(+), 17 deletions(-)
>
> diff --git a/gdb/elfread.c b/gdb/elfread.c
> index 260789062d0..9ffbf99db6e 100644
> --- a/gdb/elfread.c
> +++ b/gdb/elfread.c
> @@ -535,8 +535,7 @@ elf_rel_plt_read (minimal_symbol_reader &reader,
> {
> bfd *obfd = objfile->obfd;
> const struct elf_backend_data *bed = get_elf_backend_data (obfd);
> - asection *plt, *relplt, *got_plt;
> - int plt_elf_idx;
> + asection *relplt, *got_plt;
> bfd_size_type reloc_count, reloc;
> struct gdbarch *gdbarch = get_objfile_arch (objfile);
> struct type *ptr_type = builtin_type (gdbarch)->builtin_data_ptr;
> @@ -545,11 +544,6 @@ elf_rel_plt_read (minimal_symbol_reader &reader,
> if (objfile->separate_debug_objfile_backlink)
> return;
>
> - plt = bfd_get_section_by_name (obfd, ".plt");
> - if (plt == NULL)
> - return;
> - plt_elf_idx = elf_section_data (plt)->this_idx;
> -
> got_plt = bfd_get_section_by_name (obfd, ".got.plt");
> if (got_plt == NULL)
> {
> @@ -559,12 +553,31 @@ elf_rel_plt_read (minimal_symbol_reader &reader,
> return;
> }
>
> + /* Depending on system, we may find jump slots in a relocation
> + section for either .got.plt or .plt. */
> + asection *plt = bfd_get_section_by_name (obfd, ".plt");
> + int plt_elf_idx = (plt != NULL) ? elf_section_data (plt)->this_idx : -1;
> +
> + int got_plt_elf_idx = elf_section_data (got_plt)->this_idx;
> +
> /* This search algorithm is from _bfd_elf_canonicalize_dynamic_reloc. */
> for (relplt = obfd->sections; relplt != NULL; relplt = relplt->next)
> - if (elf_section_data (relplt)->this_hdr.sh_info == plt_elf_idx
> - && (elf_section_data (relplt)->this_hdr.sh_type == SHT_REL
> - || elf_section_data (relplt)->this_hdr.sh_type == SHT_RELA))
> - break;
> + {
> + const auto &this_hdr = elf_section_data (relplt)->this_hdr;
> +
> + if (this_hdr.sh_type == SHT_REL || this_hdr.sh_type == SHT_RELA)
> + {
> + asection *target_section = NULL;
> +
> + if (this_hdr.sh_info == plt_elf_idx)
> + target_section = plt;
> + else if (this_hdr.sh_info == got_plt_elf_idx)
> + target_section = got_plt;
> +
> + if (target_section != NULL)
> + break;
Is it really useful to have/set target_section? Couldn't we just break out of the
loop like this?
if (this_hdr.sh_info == plt_elf_idx
|| this_hdr.sh_info == got_plt_elf_idx)
break;
> + }
> + }
> if (relplt == NULL)
> return;
>
> @@ -573,6 +586,18 @@ elf_rel_plt_read (minimal_symbol_reader &reader,
>
> std::string string_buffer;
>
> + /* Does ADDRESS reside in SECTION of OBFD? */
> + auto within_section = [obfd] (asection *section, CORE_ADDR address)
> + {
> + if (section == NULL)
> + return false;
> +
> + /* Does the pointer reside in the .got.plt section? */
That comment should change, since it's not stricly .got.plt.
> + return (bfd_get_section_vma (obfd, section) <= address
> + && (address < bfd_get_section_vma (obfd, section)
> + + bfd_get_section_size (section)));
> + };
> +
> reloc_count = relplt->size / elf_section_data (relplt)->this_hdr.sh_entsize;
> for (reloc = 0; reloc < reloc_count; reloc++)
> {
> @@ -585,10 +610,15 @@ elf_rel_plt_read (minimal_symbol_reader &reader,
> name = bfd_asymbol_name (*relplt->relocation[reloc].sym_ptr_ptr);
> address = relplt->relocation[reloc].address;
>
> - /* Does the pointer reside in the .got.plt section? */
> - if (!(bfd_get_section_vma (obfd, got_plt) <= address
> - && address < bfd_get_section_vma (obfd, got_plt)
> - + bfd_get_section_size (got_plt)))
> + asection *msym_section;
> +
> + /* Does the pointer reside in either the .got.plt or .plt
> + sections? */
> + if (within_section (got_plt, address))
> + msym_section = got_plt;
> + else if (within_section (plt, address))
> + msym_section = plt;
> + else
> continue;
Or maybe you intended to use target_section here at some point? Is there a
relationship between the section that matched in the for loop above and the
section that will contain the address? In other words, could we save the
target_section from above and do
if (!within_section (target_section, address))
continue;
>
> /* We cannot check if NAME is a reference to mst_text_gnu_ifunc as in
> @@ -600,8 +630,8 @@ elf_rel_plt_read (minimal_symbol_reader &reader,
>
> msym = record_minimal_symbol (reader, string_buffer.c_str (),
> string_buffer.size (),
> - true, address, mst_slot_got_plt, got_plt,
> - objfile);
> + true, address, mst_slot_got_plt,
> + msym_section, objfile);
> if (msym)
> SET_MSYMBOL_SIZE (msym, ptr_size);
> }
>
Simon
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH v2 01/15] Fix breakpoints in ifunc after inferior resolved it (@got.plt symbol creation)
2018-04-01 3:35 ` Simon Marchi
@ 2018-04-10 21:20 ` Pedro Alves
2018-04-14 16:36 ` Simon Marchi
0 siblings, 1 reply; 33+ messages in thread
From: Pedro Alves @ 2018-04-10 21:20 UTC (permalink / raw)
To: Simon Marchi, gdb-patches
On 04/01/2018 04:35 AM, Simon Marchi wrote:
> On 2018-03-25 03:19 PM, Pedro Alves wrote:
>> /* This search algorithm is from _bfd_elf_canonicalize_dynamic_reloc. */
>> for (relplt = obfd->sections; relplt != NULL; relplt = relplt->next)
>> - if (elf_section_data (relplt)->this_hdr.sh_info == plt_elf_idx
>> - && (elf_section_data (relplt)->this_hdr.sh_type == SHT_REL
>> - || elf_section_data (relplt)->this_hdr.sh_type == SHT_RELA))
>> - break;
>> + {
>> + const auto &this_hdr = elf_section_data (relplt)->this_hdr;
>> +
>> + if (this_hdr.sh_type == SHT_REL || this_hdr.sh_type == SHT_RELA)
>> + {
>> + asection *target_section = NULL;
>> +
>> + if (this_hdr.sh_info == plt_elf_idx)
>> + target_section = plt;
>> + else if (this_hdr.sh_info == got_plt_elf_idx)
>> + target_section = got_plt;
>> +
>> + if (target_section != NULL)
>> + break;
>
> Is it really useful to have/set target_section? Couldn't we just break out of the
> loop like this?
>
> if (this_hdr.sh_info == plt_elf_idx
> || this_hdr.sh_info == got_plt_elf_idx)
> break;
>
Hmm, the original intention was to use target_section in the other
loop, but that didn't work, so I reverted it, but somehow not that
part. :-P
>>
>> @@ -573,6 +586,18 @@ elf_rel_plt_read (minimal_symbol_reader &reader,
>>
>> std::string string_buffer;
>>
>> + /* Does ADDRESS reside in SECTION of OBFD? */
>> + auto within_section = [obfd] (asection *section, CORE_ADDR address)
>> + {
>> + if (section == NULL)
>> + return false;
>> +
>> + /* Does the pointer reside in the .got.plt section? */
>
> That comment should change, since it's not stricly .got.plt.
>
I've removed it, since the intro comment already says it all.
> Or maybe you intended to use target_section here at some point? Is there a
> relationship between the section that matched in the for loop above and the
> section that will contain the address? In other words, could we save the
> target_section from above and do
Yeah, in an earlier version I tried doing that, but then testing on the
different systems found out that there's no relation between the
two sections.
Here's the updated patch. WDYT?
From 0e91b0c40141326243dbd1dd735ca1e1fe5c78ce Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Tue, 10 Apr 2018 18:11:27 +0100
Subject: [PATCH] Fix breakpoints in ifunc after inferior resolved it (@got.plt
symbol creation)
Setting a breakpoint on an ifunc symbol after the ifunc has already
been resolved by the inferior should result in creating a breakpoint
location at the ifunc target. However, that's not what happens on
current Fedora:
(gdb) n
53 i = gnu_ifunc (1); /* break-at-call */
(gdb)
54 assert (i == 2);
(gdb) b gnu_ifunc
Breakpoint 2 at gnu-indirect-function resolver at 0x7ffff7bd36ee
(gdb) info breakpoints
Num Type Disp Enb Address What
2 STT_GNU_IFUNC resolver keep y 0x00007ffff7bd36ee <gnu_ifunc+4>
The problem is that elf_gnu_ifunc_resolve_by_got never manages to
resolve an ifunc target. The reason is that GDB never actually
creates the internal got.plt symbols:
(gdb) p 'gnu_ifunc@got.plt'
No symbol "gnu_ifunc@got.plt" in current context.
and this is because GDB expects that rela.plt has relocations for
.plt, while it actually has relocations for .got.plt:
Relocation section [10] '.rela.plt' for section [22] '.got.plt' at offset 0x570 contains 2 entries:
Offset Type Value Addend Name
0x0000000000601018 X86_64_JUMP_SLOT 000000000000000000 +0 __assert_fail
0x0000000000601020 X86_64_JUMP_SLOT 000000000000000000 +0 gnu_ifunc
Using an older system on the GCC compile farm (machine gcc15, an
x86-64 running Debian 6.0.8, with GNU ld 2.20.1), we see that it used
to be that we'd get a .rela.plt section for .plt:
Relocation section [ 9] '.rela.plt' for section [11] '.plt' at offset 0x578 contains 3 entries:
Offset Type Value Addend Name
0x0000000000600cc0 X86_64_JUMP_SLOT 000000000000000000 +0 __assert_fail
0x0000000000600cc8 X86_64_JUMP_SLOT 000000000000000000 +0 __libc_start_main
0x0000000000600cd0 X86_64_JUMP_SLOT 000000000000000000 +0 gnu_ifunc
Those offsets did point into .got.plt, as seen with objdump -h:
20 .got.plt 00000030 0000000000600ca8 0000000000600ca8 00000ca8 2**3
CONTENTS, ALLOC, LOAD, DATA
I also tested on gcc110 on the compile farm (PPC64 running CentOS
7.4.1708, with GNU ld 2.25.1), and there we see instead:
Relocation section [ 9] '.rela.plt' for section [23] '.plt' at offset 0x5d0 contains 4 entries:
Offset Type Value Addend Name
0x0000000010020148 PPC64_JMP_SLOT 000000000000000000 +0 __libc_start_main
0x0000000010020160 PPC64_JMP_SLOT 000000000000000000 +0 __gmon_start__
0x0000000010020178 PPC64_JMP_SLOT 000000000000000000 +0 __assert_fail
0x0000000010020190 PPC64_JMP_SLOT 000000000000000000 +0 gnu_ifunc
But note that those offsets point into .plt, not .got.plt, as seen
with objdump -h:
22 .plt 00000078 0000000010020130 0000000010020130 00010130 2**3
ALLOC
This commit makes us support all the different combinations above.
With that addressed, we now get:
(gdb) p 'gnu_ifunc@got.plt'
$1 = (<text from jump slot in .got.plt, no debug info>) 0x400753 <final>
And setting a breakpoint on the ifunc finds the ifunc target:
(gdb) b gnu_ifunc
Breakpoint 2 at 0x400753
(gdb) info breakpoints
Num Type Disp Enb Address What
2 breakpoint keep y 0x0000000000400753 <final>
gdb/ChangeLog:
yyyy-mm-dd Pedro Alves <palves@redhat.com>
* elfread.c (elf_rel_plt_read): Look for relocations for .got.plt too.
not .plt.
---
gdb/elfread.c | 57 ++++++++++++++++++++++++++++++++++++++++-----------------
1 file changed, 40 insertions(+), 17 deletions(-)
diff --git a/gdb/elfread.c b/gdb/elfread.c
index 260789062d0..16a692d3713 100644
--- a/gdb/elfread.c
+++ b/gdb/elfread.c
@@ -535,8 +535,7 @@ elf_rel_plt_read (minimal_symbol_reader &reader,
{
bfd *obfd = objfile->obfd;
const struct elf_backend_data *bed = get_elf_backend_data (obfd);
- asection *plt, *relplt, *got_plt;
- int plt_elf_idx;
+ asection *relplt, *got_plt;
bfd_size_type reloc_count, reloc;
struct gdbarch *gdbarch = get_objfile_arch (objfile);
struct type *ptr_type = builtin_type (gdbarch)->builtin_data_ptr;
@@ -545,11 +544,6 @@ elf_rel_plt_read (minimal_symbol_reader &reader,
if (objfile->separate_debug_objfile_backlink)
return;
- plt = bfd_get_section_by_name (obfd, ".plt");
- if (plt == NULL)
- return;
- plt_elf_idx = elf_section_data (plt)->this_idx;
-
got_plt = bfd_get_section_by_name (obfd, ".got.plt");
if (got_plt == NULL)
{
@@ -559,12 +553,25 @@ elf_rel_plt_read (minimal_symbol_reader &reader,
return;
}
+ /* Depending on system, we may find jump slots in a relocation
+ section for either .got.plt or .plt. */
+ asection *plt = bfd_get_section_by_name (obfd, ".plt");
+ int plt_elf_idx = (plt != NULL) ? elf_section_data (plt)->this_idx : -1;
+
+ int got_plt_elf_idx = elf_section_data (got_plt)->this_idx;
+
/* This search algorithm is from _bfd_elf_canonicalize_dynamic_reloc. */
for (relplt = obfd->sections; relplt != NULL; relplt = relplt->next)
- if (elf_section_data (relplt)->this_hdr.sh_info == plt_elf_idx
- && (elf_section_data (relplt)->this_hdr.sh_type == SHT_REL
- || elf_section_data (relplt)->this_hdr.sh_type == SHT_RELA))
- break;
+ {
+ const auto &this_hdr = elf_section_data (relplt)->this_hdr;
+
+ if (this_hdr.sh_type == SHT_REL || this_hdr.sh_type == SHT_RELA)
+ {
+ if (this_hdr.sh_info == plt_elf_idx
+ || this_hdr.sh_info == got_plt_elf_idx)
+ break;
+ }
+ }
if (relplt == NULL)
return;
@@ -573,6 +580,17 @@ elf_rel_plt_read (minimal_symbol_reader &reader,
std::string string_buffer;
+ /* Does ADDRESS reside in SECTION of OBFD? */
+ auto within_section = [obfd] (asection *section, CORE_ADDR address)
+ {
+ if (section == NULL)
+ return false;
+
+ return (bfd_get_section_vma (obfd, section) <= address
+ && (address < bfd_get_section_vma (obfd, section)
+ + bfd_get_section_size (section)));
+ };
+
reloc_count = relplt->size / elf_section_data (relplt)->this_hdr.sh_entsize;
for (reloc = 0; reloc < reloc_count; reloc++)
{
@@ -585,10 +603,15 @@ elf_rel_plt_read (minimal_symbol_reader &reader,
name = bfd_asymbol_name (*relplt->relocation[reloc].sym_ptr_ptr);
address = relplt->relocation[reloc].address;
- /* Does the pointer reside in the .got.plt section? */
- if (!(bfd_get_section_vma (obfd, got_plt) <= address
- && address < bfd_get_section_vma (obfd, got_plt)
- + bfd_get_section_size (got_plt)))
+ asection *msym_section;
+
+ /* Does the pointer reside in either the .got.plt or .plt
+ sections? */
+ if (within_section (got_plt, address))
+ msym_section = got_plt;
+ else if (within_section (plt, address))
+ msym_section = plt;
+ else
continue;
/* We cannot check if NAME is a reference to mst_text_gnu_ifunc as in
@@ -600,8 +623,8 @@ elf_rel_plt_read (minimal_symbol_reader &reader,
msym = record_minimal_symbol (reader, string_buffer.c_str (),
string_buffer.size (),
- true, address, mst_slot_got_plt, got_plt,
- objfile);
+ true, address, mst_slot_got_plt,
+ msym_section, objfile);
if (msym)
SET_MSYMBOL_SIZE (msym, ptr_size);
}
--
2.14.3
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH v2 01/15] Fix breakpoints in ifunc after inferior resolved it (@got.plt symbol creation)
2018-04-10 21:20 ` Pedro Alves
@ 2018-04-14 16:36 ` Simon Marchi
0 siblings, 0 replies; 33+ messages in thread
From: Simon Marchi @ 2018-04-14 16:36 UTC (permalink / raw)
To: Pedro Alves; +Cc: gdb-patches
On 2018-04-10 17:19, Pedro Alves wrote:
> On 04/01/2018 04:35 AM, Simon Marchi wrote:
>> On 2018-03-25 03:19 PM, Pedro Alves wrote:
>
>>> /* This search algorithm is from
>>> _bfd_elf_canonicalize_dynamic_reloc. */
>>> for (relplt = obfd->sections; relplt != NULL; relplt =
>>> relplt->next)
>>> - if (elf_section_data (relplt)->this_hdr.sh_info == plt_elf_idx
>>> - && (elf_section_data (relplt)->this_hdr.sh_type == SHT_REL
>>> - || elf_section_data (relplt)->this_hdr.sh_type == SHT_RELA))
>>> - break;
>>> + {
>>> + const auto &this_hdr = elf_section_data (relplt)->this_hdr;
>>> +
>>> + if (this_hdr.sh_type == SHT_REL || this_hdr.sh_type ==
>>> SHT_RELA)
>>> + {
>>> + asection *target_section = NULL;
>>> +
>>> + if (this_hdr.sh_info == plt_elf_idx)
>>> + target_section = plt;
>>> + else if (this_hdr.sh_info == got_plt_elf_idx)
>>> + target_section = got_plt;
>>> +
>>> + if (target_section != NULL)
>>> + break;
>>
>> Is it really useful to have/set target_section? Couldn't we just
>> break out of the
>> loop like this?
>>
>> if (this_hdr.sh_info == plt_elf_idx
>> || this_hdr.sh_info == got_plt_elf_idx)
>> break;
>>
>
> Hmm, the original intention was to use target_section in the other
> loop, but that didn't work, so I reverted it, but somehow not that
> part. :-P
>
>>>
>>> @@ -573,6 +586,18 @@ elf_rel_plt_read (minimal_symbol_reader &reader,
>>>
>>> std::string string_buffer;
>>>
>>> + /* Does ADDRESS reside in SECTION of OBFD? */
>>> + auto within_section = [obfd] (asection *section, CORE_ADDR
>>> address)
>>> + {
>>> + if (section == NULL)
>>> + return false;
>>> +
>>> + /* Does the pointer reside in the .got.plt section? */
>>
>> That comment should change, since it's not stricly .got.plt.
>>
>
> I've removed it, since the intro comment already says it all.
>
>> Or maybe you intended to use target_section here at some point? Is
>> there a
>> relationship between the section that matched in the for loop above
>> and the
>> section that will contain the address? In other words, could we save
>> the
>> target_section from above and do
>
> Yeah, in an earlier version I tried doing that, but then testing on the
> different systems found out that there's no relation between the
> two sections.
>
> Here's the updated patch. WDYT?
LGTM (though I trust the passing tests more than I trust myself).
Simon
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH v2 02/15] Fix calling ifunc functions when resolver has debug info and different name
2018-03-25 19:19 [PATCH v2 00/15] Fixing GNU ifunc support Pedro Alves
` (3 preceding siblings ...)
2018-03-25 19:19 ` [PATCH v2 01/15] Fix breakpoints in ifunc after inferior resolved it (@got.plt symbol creation) Pedro Alves
@ 2018-03-25 19:19 ` Pedro Alves
2018-04-01 3:44 ` Simon Marchi
2018-03-25 19:19 ` [PATCH v2 05/15] Fix elf_gnu_ifunc_resolve_by_got buglet Pedro Alves
` (10 subsequent siblings)
15 siblings, 1 reply; 33+ messages in thread
From: Pedro Alves @ 2018-03-25 19:19 UTC (permalink / raw)
To: gdb-patches
Currently, on Fedora 27 (glibc 2.26), if you try to call strlen in the
inferior you get:
(gdb) p strlen ("hello")
$1 = (size_t (*)(const char *)) 0x7ffff554aac0 <__strlen_avx2>
strlen is an ifunc function, and what we see above is the result of
calling the ifunc resolver in the inferior. That returns a pointer to
the actual target function that implements strlen on my machine. GDB
should have turned around and called the resolver automatically
without the user noticing.
This is was caused by commit:
commit bf223d3e808e6fec9ee165d3d48beb74837796de
Date: Mon Aug 21 11:34:32 2017 +0100
Handle function aliases better (PR gdb/19487, errno printing)
which added the find_function_alias_target call to c-exp.y, to try to
find an alias with debug info for a minsym. For ifunc symbols, that
finds the ifunc's resolver if it has debug info (in the example it's
called "strlen_ifunc"), with the result that GDB calls that as a
regular function.
After this commit, we get now get:
(top-gdb) p strlen ("hello")
'__strlen_avx2' has unknown return type; cast the call to its declared return type
Which is correct, because __strlen_avx2 is written in assembly.
That'll be improved in a following patch, though.
gdb/ChangeLog:
yyyy-mm-dd Pedro Alves <palves@redhat.com>
* c-exp.y (variable production): Skip finding an alias for ifunc
symbols.
---
gdb/c-exp.y | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/gdb/c-exp.y b/gdb/c-exp.y
index 8dc3c068a5e..e2ea07cd792 100644
--- a/gdb/c-exp.y
+++ b/gdb/c-exp.y
@@ -1081,7 +1081,9 @@ variable: name_not_typename
is important for example for "p
*__errno_location()". */
symbol *alias_target
- = find_function_alias_target (msymbol);
+ = (msymbol.minsym->type != mst_text_gnu_ifunc
+ ? find_function_alias_target (msymbol)
+ : NULL);
if (alias_target != NULL)
{
write_exp_elt_opcode (pstate, OP_VAR_VALUE);
--
2.14.3
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH v2 02/15] Fix calling ifunc functions when resolver has debug info and different name
2018-03-25 19:19 ` [PATCH v2 02/15] Fix calling ifunc functions when resolver has debug info and different name Pedro Alves
@ 2018-04-01 3:44 ` Simon Marchi
2018-04-10 21:20 ` Pedro Alves
0 siblings, 1 reply; 33+ messages in thread
From: Simon Marchi @ 2018-04-01 3:44 UTC (permalink / raw)
To: Pedro Alves, gdb-patches
On 2018-03-25 03:19 PM, Pedro Alves wrote:
> Currently, on Fedora 27 (glibc 2.26), if you try to call strlen in the
> inferior you get:
>
> (gdb) p strlen ("hello")
> $1 = (size_t (*)(const char *)) 0x7ffff554aac0 <__strlen_avx2>
>
> strlen is an ifunc function, and what we see above is the result of
> calling the ifunc resolver in the inferior. That returns a pointer to
> the actual target function that implements strlen on my machine. GDB
> should have turned around and called the resolver automatically
> without the user noticing.
>
> This is was caused by commit:
>
> commit bf223d3e808e6fec9ee165d3d48beb74837796de
> Date: Mon Aug 21 11:34:32 2017 +0100
>
> Handle function aliases better (PR gdb/19487, errno printing)
>
> which added the find_function_alias_target call to c-exp.y, to try to
> find an alias with debug info for a minsym. For ifunc symbols, that
> finds the ifunc's resolver if it has debug info (in the example it's
> called "strlen_ifunc"), with the result that GDB calls that as a
> regular function.
>
> After this commit, we get now get:
>
> (top-gdb) p strlen ("hello")
> '__strlen_avx2' has unknown return type; cast the call to its declared return type
>
> Which is correct, because __strlen_avx2 is written in assembly.
> That'll be improved in a following patch, though.
>
> gdb/ChangeLog:
> yyyy-mm-dd Pedro Alves <palves@redhat.com>
>
> * c-exp.y (variable production): Skip finding an alias for ifunc
> symbols.
> ---
> gdb/c-exp.y | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/gdb/c-exp.y b/gdb/c-exp.y
> index 8dc3c068a5e..e2ea07cd792 100644
> --- a/gdb/c-exp.y
> +++ b/gdb/c-exp.y
> @@ -1081,7 +1081,9 @@ variable: name_not_typename
> is important for example for "p
> *__errno_location()". */
> symbol *alias_target
> - = find_function_alias_target (msymbol);
> + = (msymbol.minsym->type != mst_text_gnu_ifunc
> + ? find_function_alias_target (msymbol)
> + : NULL);
> if (alias_target != NULL)
> {
> write_exp_elt_opcode (pstate, OP_VAR_VALUE);
>
Just one question, to which I really don't have a preconceived answer:
Would it make sense to add that filtering to find_function_alias_target or
another function deeper in the call chain instead? In other words, would
it ever make sense for find_function_alias_target to return an non-NULL
result for an mst_text_gnu_ifunc minimal symbol?
Simon
Simon
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH v2 02/15] Fix calling ifunc functions when resolver has debug info and different name
2018-04-01 3:44 ` Simon Marchi
@ 2018-04-10 21:20 ` Pedro Alves
0 siblings, 0 replies; 33+ messages in thread
From: Pedro Alves @ 2018-04-10 21:20 UTC (permalink / raw)
To: Simon Marchi, gdb-patches
Hi Simon,
On 04/01/2018 04:44 AM, Simon Marchi wrote:
>> diff --git a/gdb/c-exp.y b/gdb/c-exp.y
>> index 8dc3c068a5e..e2ea07cd792 100644
>> --- a/gdb/c-exp.y
>> +++ b/gdb/c-exp.y
>> @@ -1081,7 +1081,9 @@ variable: name_not_typename
>> is important for example for "p
>> *__errno_location()". */
>> symbol *alias_target
>> - = find_function_alias_target (msymbol);
>> + = (msymbol.minsym->type != mst_text_gnu_ifunc
>> + ? find_function_alias_target (msymbol)
>> + : NULL);
>> if (alias_target != NULL)
>> {
>> write_exp_elt_opcode (pstate, OP_VAR_VALUE);
>>
>
> Just one question, to which I really don't have a preconceived answer:
>
> Would it make sense to add that filtering to find_function_alias_target or
> another function deeper in the call chain instead? In other words, would
> it ever make sense for find_function_alias_target to return an non-NULL
> result for an mst_text_gnu_ifunc minimal symbol?
For ifuncs, find_function_alias_target will finds the debug symbol for
the resolver. If that has a different name from the ifunc (it will
if you use gcc's attribute ifunc, then it'll work the same as any other
minsym, in the sense that it'll find an alias.
So from that angle, the function works as intended, and it
wasn't clear that we'll always want to treat ifuncs differently.
Note this is the only caller of find_function_alias_target currently.
Another reason for leaving the check here is that patch #4 adds
another case of "do this for ifuncs" just above this code, but for
the "sym.symbol" case. It felt better to keep both of the
"for ifunc, do this" cases close together.
Thanks,
Pedro Alves
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH v2 05/15] Fix elf_gnu_ifunc_resolve_by_got buglet
2018-03-25 19:19 [PATCH v2 00/15] Fixing GNU ifunc support Pedro Alves
` (4 preceding siblings ...)
2018-03-25 19:19 ` [PATCH v2 02/15] Fix calling ifunc functions when resolver has debug info and different name Pedro Alves
@ 2018-03-25 19:19 ` Pedro Alves
2018-04-01 4:32 ` Simon Marchi
2018-03-25 19:19 ` [PATCH v2 11/15] Fix stepping past GNU ifunc resolvers (introduce lookup_msym_prefer) Pedro Alves
` (9 subsequent siblings)
15 siblings, 1 reply; 33+ messages in thread
From: Pedro Alves @ 2018-03-25 19:19 UTC (permalink / raw)
To: gdb-patches
The next patch will add a call to elf_gnu_ifunc_resolve_by_got that
trips on a latent buglet -- the function is writing to its output
parameter even if the address wasn't found, confusing the caller. The
function's intro comment says:
/* Try to find the target resolved function entry address of a STT_GNU_IFUNC
function NAME. If the address is found it is stored to *ADDR_P (if ADDR_P
is not NULL) and the function returns 1. It returns 0 otherwise.
So fix the function accordingly.
gdb/ChangeLog:
yyyy-mm-dd Pedro Alves <palves@redhat.com>
* elfread.c (elf_gnu_ifunc_resolve_by_got): Don't write to *ADDR_P
unless we actually resolved the ifunc.
---
gdb/elfread.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/gdb/elfread.c b/gdb/elfread.c
index 9ffbf99db6e..82ab3d891ce 100644
--- a/gdb/elfread.c
+++ b/gdb/elfread.c
@@ -840,10 +840,12 @@ elf_gnu_ifunc_resolve_by_got (const char *name, CORE_ADDR *addr_p)
¤t_target);
addr = gdbarch_addr_bits_remove (gdbarch, addr);
- if (addr_p)
- *addr_p = addr;
if (elf_gnu_ifunc_record_cache (name, addr))
- return 1;
+ {
+ if (addr_p)
+ *addr_p = addr;
+ return 1;
+ }
}
return 0;
--
2.14.3
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH v2 05/15] Fix elf_gnu_ifunc_resolve_by_got buglet
2018-03-25 19:19 ` [PATCH v2 05/15] Fix elf_gnu_ifunc_resolve_by_got buglet Pedro Alves
@ 2018-04-01 4:32 ` Simon Marchi
2018-04-10 21:52 ` Pedro Alves
0 siblings, 1 reply; 33+ messages in thread
From: Simon Marchi @ 2018-04-01 4:32 UTC (permalink / raw)
To: Pedro Alves, gdb-patches
On 2018-03-25 03:19 PM, Pedro Alves wrote:
> The next patch will add a call to elf_gnu_ifunc_resolve_by_got that
> trips on a latent buglet -- the function is writing to its output
> parameter even if the address wasn't found, confusing the caller. The
> function's intro comment says:
>
> /* Try to find the target resolved function entry address of a STT_GNU_IFUNC
> function NAME. If the address is found it is stored to *ADDR_P (if ADDR_P
> is not NULL) and the function returns 1. It returns 0 otherwise.
>
> So fix the function accordingly.
>
> gdb/ChangeLog:
> yyyy-mm-dd Pedro Alves <palves@redhat.com>
>
> * elfread.c (elf_gnu_ifunc_resolve_by_got): Don't write to *ADDR_P
> unless we actually resolved the ifunc.
> ---
> gdb/elfread.c | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/gdb/elfread.c b/gdb/elfread.c
> index 9ffbf99db6e..82ab3d891ce 100644
> --- a/gdb/elfread.c
> +++ b/gdb/elfread.c
> @@ -840,10 +840,12 @@ elf_gnu_ifunc_resolve_by_got (const char *name, CORE_ADDR *addr_p)
> ¤t_target);
> addr = gdbarch_addr_bits_remove (gdbarch, addr);
>
> - if (addr_p)
> - *addr_p = addr;
> if (elf_gnu_ifunc_record_cache (name, addr))
> - return 1;
> + {
> + if (addr_p)
addr_p != NULL ?
Simon
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH v2 05/15] Fix elf_gnu_ifunc_resolve_by_got buglet
2018-04-01 4:32 ` Simon Marchi
@ 2018-04-10 21:52 ` Pedro Alves
0 siblings, 0 replies; 33+ messages in thread
From: Pedro Alves @ 2018-04-10 21:52 UTC (permalink / raw)
To: Simon Marchi, gdb-patches
On 04/01/2018 05:32 AM, Simon Marchi wrote:
> On 2018-03-25 03:19 PM, Pedro Alves wrote:
>> --- a/gdb/elfread.c
>> +++ b/gdb/elfread.c
>> @@ -840,10 +840,12 @@ elf_gnu_ifunc_resolve_by_got (const char *name, CORE_ADDR *addr_p)
>> ¤t_target);
>> addr = gdbarch_addr_bits_remove (gdbarch, addr);
>>
>> - if (addr_p)
>> - *addr_p = addr;
>> if (elf_gnu_ifunc_record_cache (name, addr))
>> - return 1;
>> + {
>> + if (addr_p)
>
> addr_p != NULL ?
>
Might as well. I've done that locally.
Thanks,
Pedro Alves
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH v2 11/15] Fix stepping past GNU ifunc resolvers (introduce lookup_msym_prefer)
2018-03-25 19:19 [PATCH v2 00/15] Fixing GNU ifunc support Pedro Alves
` (5 preceding siblings ...)
2018-03-25 19:19 ` [PATCH v2 05/15] Fix elf_gnu_ifunc_resolve_by_got buglet Pedro Alves
@ 2018-03-25 19:19 ` Pedro Alves
2018-06-18 20:26 ` [PATCH] Silence GCC "uninitialized" warning on minsyms.c:lookup_minimal_symbol_by_pc_section Sergio Durigan Junior
2018-03-25 19:19 ` [PATCH v2 07/15] Breakpoints, don't skip prologue of ifunc resolvers with debug info Pedro Alves
` (8 subsequent siblings)
15 siblings, 1 reply; 33+ messages in thread
From: Pedro Alves @ 2018-03-25 19:19 UTC (permalink / raw)
To: gdb-patches
When we're stepping (with "step"), we want to skip trampoline-like
functions automatically, including GNU ifunc resolvers. That is done
by infrun.c calling into:
in_solib_dynsym_resolve_code
-> svr4_in_dynsym_resolve_code
-> in_gnu_ifunc_stub
A problem here is that if there's a regular text symbol at the same
address as the ifunc symbol, the minimal symbol lookup in
in_gnu_ifunc_stub may miss the GNU ifunc symbol:
(...)
41: 000000000000071a 53 FUNC GLOBAL DEFAULT 11 gnu_ifunc_resolver
(...)
50: 000000000000071a 53 IFUNC GLOBAL DEFAULT 11 gnu_ifunc
(...)
This causes this FAIL in the tests added later in the series:
(gdb) PASS: gdb.base/gnu-ifunc.exp: resolver_attr=1: resolver_debug=0: final_debug=0: resolver received HWCAP
set step-mode on
(gdb) PASS: gdb.base/gnu-ifunc.exp: resolver_attr=1: resolver_debug=0: final_debug=0: set step-mode on
step
0x00007ffff7bd371a in gnu_ifunc_resolver () from build/gdb/testsuite/outputs/gdb.base/gnu-ifunc/gnu-ifunc-lib-1-0-0.so
(gdb) FAIL: gdb.base/gnu-ifunc.exp: resolver_attr=1: resolver_debug=0: final_debug=0: step
Above, GDB simply thought that it stepped into a regular function, so
it stopped stepping, while it should have continued stepping past the
resolver.
The fix is to teach minimal symbol lookup to prefer GNU ifunc symbols
if desired.
gdb/ChangeLog:
yyyy-mm-dd Pedro Alves <palves@redhat.com>
* minsyms.c (lookup_minimal_symbol_by_pc_section_1): Rename to ...
(lookup_minimal_symbol_by_pc_section): ... this. Replace
'want_trampoline' parameter by a lookup_msym_prefer parameter.
Handle it.
(lookup_minimal_symbol_by_pc_section): Delete old implementation.
(lookup_minimal_symbol_by_pc): Adjust.
(in_gnu_ifunc_stub): Prefer GNU ifunc symbols.
(lookup_solib_trampoline_symbol_by_pc): Adjust.
* minsyms.h (lookup_msym_prefer): New enum.
(lookup_minimal_symbol_by_pc_section): Replace 'want_trampoline'
parameter by a lookup_msym_prefer parameter.
---
gdb/minsyms.c | 77 ++++++++++++++++++++++++-----------------------------------
gdb/minsyms.h | 26 +++++++++++++++++---
2 files changed, 54 insertions(+), 49 deletions(-)
diff --git a/gdb/minsyms.c b/gdb/minsyms.c
index c75c316c08d..f71e2db484e 100644
--- a/gdb/minsyms.c
+++ b/gdb/minsyms.c
@@ -686,10 +686,9 @@ frob_address (struct objfile *objfile, CORE_ADDR *pc)
there are text and trampoline symbols at the same address.
Otherwise prefer mst_text symbols. */
-static struct bound_minimal_symbol
-lookup_minimal_symbol_by_pc_section_1 (CORE_ADDR pc_in,
- struct obj_section *section,
- int want_trampoline)
+bound_minimal_symbol
+lookup_minimal_symbol_by_pc_section (CORE_ADDR pc_in, struct obj_section *section,
+ lookup_msym_prefer prefer)
{
int lo;
int hi;
@@ -699,10 +698,27 @@ lookup_minimal_symbol_by_pc_section_1 (CORE_ADDR pc_in,
struct minimal_symbol *best_symbol = NULL;
struct objfile *best_objfile = NULL;
struct bound_minimal_symbol result;
- enum minimal_symbol_type want_type, other_type;
+ enum minimal_symbol_type want_type;
- want_type = want_trampoline ? mst_solib_trampoline : mst_text;
- other_type = want_trampoline ? mst_text : mst_solib_trampoline;
+ if (section == NULL)
+ {
+ section = find_pc_section (pc_in);
+ if (section == NULL)
+ return {};
+ }
+
+ switch (prefer)
+ {
+ case lookup_msym_prefer::TEXT:
+ want_type = mst_text;
+ break;
+ case lookup_msym_prefer::TRAMPOLINE:
+ want_type = mst_solib_trampoline;
+ break;
+ case lookup_msym_prefer::GNU_IFUNC:
+ want_type = mst_text_gnu_ifunc;
+ break;
+ }
/* We can not require the symbol found to be in section, because
e.g. IRIX 6.5 mdebug relies on this code returning an absolute
@@ -821,7 +837,7 @@ lookup_minimal_symbol_by_pc_section_1 (CORE_ADDR pc_in,
preceding symbol too. If they are otherwise
identical prefer that one. */
if (hi > 0
- && MSYMBOL_TYPE (&msymbol[hi]) == other_type
+ && MSYMBOL_TYPE (&msymbol[hi]) != want_type
&& MSYMBOL_TYPE (&msymbol[hi - 1]) == want_type
&& (MSYMBOL_SIZE (&msymbol[hi])
== MSYMBOL_SIZE (&msymbol[hi - 1]))
@@ -918,41 +934,12 @@ lookup_minimal_symbol_by_pc_section_1 (CORE_ADDR pc_in,
return result;
}
-struct bound_minimal_symbol
-lookup_minimal_symbol_by_pc_section (CORE_ADDR pc, struct obj_section *section)
-{
- if (section == NULL)
- {
- /* NOTE: cagney/2004-01-27: This was using find_pc_mapped_section to
- force the section but that (well unless you're doing overlay
- debugging) always returns NULL making the call somewhat useless. */
- section = find_pc_section (pc);
- if (section == NULL)
- {
- struct bound_minimal_symbol result;
-
- memset (&result, 0, sizeof (result));
- return result;
- }
- }
- return lookup_minimal_symbol_by_pc_section_1 (pc, section, 0);
-}
-
/* See minsyms.h. */
struct bound_minimal_symbol
lookup_minimal_symbol_by_pc (CORE_ADDR pc)
{
- struct obj_section *section = find_pc_section (pc);
-
- if (section == NULL)
- {
- struct bound_minimal_symbol result;
-
- memset (&result, 0, sizeof (result));
- return result;
- }
- return lookup_minimal_symbol_by_pc_section_1 (pc, section, 0);
+ return lookup_minimal_symbol_by_pc_section (pc, NULL);
}
/* Return non-zero iff PC is in an STT_GNU_IFUNC function resolver. */
@@ -960,8 +947,9 @@ lookup_minimal_symbol_by_pc (CORE_ADDR pc)
int
in_gnu_ifunc_stub (CORE_ADDR pc)
{
- struct bound_minimal_symbol msymbol = lookup_minimal_symbol_by_pc (pc);
-
+ bound_minimal_symbol msymbol
+ = lookup_minimal_symbol_by_pc_section (pc, NULL,
+ lookup_msym_prefer::GNU_IFUNC);
return msymbol.minsym && MSYMBOL_TYPE (msymbol.minsym) == mst_text_gnu_ifunc;
}
@@ -1470,12 +1458,9 @@ terminate_minimal_symbol_table (struct objfile *objfile)
static struct minimal_symbol *
lookup_solib_trampoline_symbol_by_pc (CORE_ADDR pc)
{
- struct obj_section *section = find_pc_section (pc);
- struct bound_minimal_symbol msymbol;
-
- if (section == NULL)
- return NULL;
- msymbol = lookup_minimal_symbol_by_pc_section_1 (pc, section, 1);
+ bound_minimal_symbol msymbol
+ = lookup_minimal_symbol_by_pc_section (pc, NULL,
+ lookup_msym_prefer::TRAMPOLINE);
if (msymbol.minsym != NULL
&& MSYMBOL_TYPE (msymbol.minsym) == mst_solib_trampoline)
diff --git a/gdb/minsyms.h b/gdb/minsyms.h
index 6bd34a58058..0dab5874a18 100644
--- a/gdb/minsyms.h
+++ b/gdb/minsyms.h
@@ -240,6 +240,22 @@ struct bound_minimal_symbol lookup_minimal_symbol_solib_trampoline
struct minimal_symbol *lookup_minimal_symbol_by_pc_name
(CORE_ADDR, const char *, struct objfile *);
+enum class lookup_msym_prefer
+{
+ /* Prefer mst_text symbols. */
+ TEXT,
+
+ /* Prefer mst_solib_trampoline symbols when there are text and
+ trampoline symbols at the same address. Otherwise prefer
+ mst_text symbols. */
+ TRAMPOLINE,
+
+ /* Prefer mst_text_gnu_ifunc symbols when there are text and ifunc
+ symbols at the same address. Otherwise prefer mst_text
+ symbols. */
+ GNU_IFUNC,
+};
+
/* Search through the minimal symbol table for each objfile and find
the symbol whose address is the largest address that is still less
than or equal to PC, and which matches SECTION.
@@ -248,11 +264,15 @@ struct minimal_symbol *lookup_minimal_symbol_by_pc_name
instead.
The result has a non-NULL 'minsym' member if such a symbol is
- found, or NULL if PC is not in a suitable range. */
+ found, or NULL if PC is not in a suitable range.
+
+ See definition of lookup_msym_prefer for description of PREFER. By
+ default mst_text symbols are preferred. */
struct bound_minimal_symbol lookup_minimal_symbol_by_pc_section
- (CORE_ADDR,
- struct obj_section *);
+ (CORE_ADDR,
+ struct obj_section *,
+ lookup_msym_prefer prefer = lookup_msym_prefer::TEXT);
/* Backward compatibility: search through the minimal symbol table
for a matching PC (no section given).
--
2.14.3
^ permalink raw reply [flat|nested] 33+ messages in thread* [PATCH] Silence GCC "uninitialized" warning on minsyms.c:lookup_minimal_symbol_by_pc_section
2018-03-25 19:19 ` [PATCH v2 11/15] Fix stepping past GNU ifunc resolvers (introduce lookup_msym_prefer) Pedro Alves
@ 2018-06-18 20:26 ` Sergio Durigan Junior
2018-06-19 15:22 ` Pedro Alves
0 siblings, 1 reply; 33+ messages in thread
From: Sergio Durigan Junior @ 2018-06-18 20:26 UTC (permalink / raw)
To: GDB Patches; +Cc: Pedro Alves, Sergio Durigan Junior
Commit 20944a6e20324cd897bf6c4c5fd20ef7224dacaa ("Fix stepping past
GNU ifunc resolvers (introduce lookup_msym_prefer)") introduced a new
way to determine the 'want_type' variable on
minsyms.c:lookup_minimal_symbol_by_pc_section. Because the
'lookup_msym_prefer' has a default value, we know that 'want_type'
will always be initialized. However, GCC is complaining that the
variable can be used uninitialized in the function:
../../gdb/minsyms.c: In function 'bound_minimal_symbol lookup_minimal_symbol_by_pc_section(CORE_ADDR, obj_section*, lookup_msym_prefer)':
../../gdb/minsyms.c:825:40: warning: 'want_type' may be used uninitialized in this function [-Wmaybe-uninitialized]
&& MSYMBOL_TYPE (&msymbol[hi]) != want_type
(This is with gcc-8.1.1-1.fc29.x86_64).
This patch fixes it by initializing 'want_type' with 'mst_text', which
is the same default value that is passed in the 'lookup_msym_prefer'
variable.
Even though this can be considered an obvious patch, I'm sending it
for approval.
gdb/ChangeLog:
2018-06-18 Sergio Durigan Junior <sergiodj@redhat.com>
* minsyms.c (lookup_minimal_symbol_by_pc_section): Initialize
'want_type' to silence GCC warning.
---
gdb/ChangeLog | 5 +++++
gdb/minsyms.c | 2 +-
2 files changed, 6 insertions(+), 1 deletion(-)
diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 4fc6cf5446..5cf9da8e56 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,8 @@
+2018-06-18 Sergio Durigan Junior <sergiodj@redhat.com>
+
+ * minsyms.c (lookup_minimal_symbol_by_pc_section): Initialize
+ 'want_type' to silence GCC warning.
+
2018-06-18 Tom Tromey <tom@tromey.com>
* solib-aix.c (solib_aix_get_section_offsets): Return
diff --git a/gdb/minsyms.c b/gdb/minsyms.c
index 4882e58ee4..c9d89dc171 100644
--- a/gdb/minsyms.c
+++ b/gdb/minsyms.c
@@ -683,7 +683,7 @@ lookup_minimal_symbol_by_pc_section (CORE_ADDR pc_in, struct obj_section *sectio
struct minimal_symbol *best_symbol = NULL;
struct objfile *best_objfile = NULL;
struct bound_minimal_symbol result;
- enum minimal_symbol_type want_type;
+ enum minimal_symbol_type want_type = mst_text;
if (section == NULL)
{
--
2.14.3
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH] Silence GCC "uninitialized" warning on minsyms.c:lookup_minimal_symbol_by_pc_section
2018-06-18 20:26 ` [PATCH] Silence GCC "uninitialized" warning on minsyms.c:lookup_minimal_symbol_by_pc_section Sergio Durigan Junior
@ 2018-06-19 15:22 ` Pedro Alves
2018-06-19 16:55 ` Sergio Durigan Junior
2018-06-19 18:47 ` Tom Tromey
0 siblings, 2 replies; 33+ messages in thread
From: Pedro Alves @ 2018-06-19 15:22 UTC (permalink / raw)
To: Sergio Durigan Junior, GDB Patches
On 06/18/2018 09:26 PM, Sergio Durigan Junior wrote:
> Commit 20944a6e20324cd897bf6c4c5fd20ef7224dacaa ("Fix stepping past
> GNU ifunc resolvers (introduce lookup_msym_prefer)") introduced a new
> way to determine the 'want_type' variable on
> minsyms.c:lookup_minimal_symbol_by_pc_section. Because the
> 'lookup_msym_prefer' has a default value, we know that 'want_type'
> will always be initialized.
Sorry, but that doesn't follow. It's the 'prefer' parameter
that will always be initialized to something with the default
value, not the 'want_type' local.
struct bound_minimal_symbol lookup_minimal_symbol_by_pc_section
(CORE_ADDR,
struct obj_section *,
lookup_msym_prefer prefer = lookup_msym_prefer::TEXT);
vs:
enum minimal_symbol_type want_type;
But even that is not necessarily true, since the caller may well
pass down an explicit argument, which in turn was uninitialized,
like:
lookup_msym_prefer bogus;
lookup_minimal_symbol_by_pc_section (addr, NULL, bogus);
> However, GCC is complaining that the
> variable can be used uninitialized in the function:
>
> ../../gdb/minsyms.c: In function 'bound_minimal_symbol lookup_minimal_symbol_by_pc_section(CORE_ADDR, obj_section*, lookup_msym_prefer)':
> ../../gdb/minsyms.c:825:40: warning: 'want_type' may be used uninitialized in this function [-Wmaybe-uninitialized]
> && MSYMBOL_TYPE (&msymbol[hi]) != want_type
>
> (This is with gcc-8.1.1-1.fc29.x86_64).
>
> This patch fixes it by initializing 'want_type' with 'mst_text', which
> is the same default value that is passed in the 'lookup_msym_prefer'
> variable.
But it's not, as explained above.
The reason this warning is a false positive is that we know that 'want_type'
is always going to be initialized because the switch to converts
enum lookup_msym_prefer values to enum minimal_symbol_type values:
switch (prefer)
{
case lookup_msym_prefer::TEXT:
want_type = mst_text;
break;
case lookup_msym_prefer::TRAMPOLINE:
want_type = mst_solib_trampoline;
break;
case lookup_msym_prefer::GNU_IFUNC:
want_type = mst_text_gnu_ifunc;
break;
}
has a case for every possible enumerator.
The actual problem is that GCC assumes that enum variables may hold
values other than the named enumerators, like e.g.,
"lookup_msym_prefer prefer = (lookup_msym_prefer) 10;".
We know that this isn't something we want to support with these
enum types, so it's better to assert that we never see a value
not covered by the enumerators.
The simplest is to add a "default:" case with a gdb_assert, but
when I wrote that code, I had not done that on purpose,
thinking that I'd prefer it if we enabled "-Wswitch" in gdb, which
helps find switch/cases where we need to handle a new enumerator,
whenever we add one, like this:
CXX minsyms.o
src/gdb/minsyms.c: In function ‘bound_minimal_symbol lookup_minimal_symbol_by_pc_section(CORE_ADDR, obj_section*, lookup_msym_prefer)’:
src/gdb/minsyms.c:695:10: error: enumeration value ‘NEW_KIND’ not handled in switch [-Werror=switch]
switch (prefer)
^
I think that it's preferable, if reasonable, to rework the code a bit to
make it more explicit to the compiler that a variable is always
initialized instead of initializing variables to quiet -Wmaybe-uninitialized,
which is documented as spewing false positives. (IMO it'd be reasonable
if GCC moved that warning from -Wall to -Wextra.)
Thus I'd prefer this instead:
From d66e808818ece92d6d79bfdb4e0a421b30662dd3 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Mon, 18 Jun 2018 22:36:43 +0100
Subject: [PATCH] Silence -Wmaybe-uninitialized warning in
minsyms.c:lookup_minimal_symbol_by_pc_section
Compiling with GCC 8.1 shows this warning:
../../gdb/minsyms.c: In function 'bound_minimal_symbol lookup_minimal_symbol_by_pc_section(CORE_ADDR, obj_section*, lookup_msym_prefer)':
../../gdb/minsyms.c:825:40: warning: 'want_type' may be used uninitialized in this function [-Wmaybe-uninitialized]
&& MSYMBOL_TYPE (&msymbol[hi]) != want_type
That warning is a false positive, because the switch that converts
enum lookup_msym_prefer values to enum enum minimal_symbol_type values
has a case for every lookup_msym_prefer enumerator:
switch (prefer)
{
case lookup_msym_prefer::TEXT:
want_type = mst_text;
break;
case lookup_msym_prefer::TRAMPOLINE:
want_type = mst_solib_trampoline;
break;
case lookup_msym_prefer::GNU_IFUNC:
want_type = mst_text_gnu_ifunc;
break;
}
The problem is that GCC assumes that enum variables may hold values
other than the named enumerators (like e.g., "lookup_msym_prefer
prefer = (lookup_msym_prefer) 10;").
Rework the code a bit, adding a gdb_assert to make it explicit to the
compiler that want_type is initialized in all normal-return paths.
gdb/ChangeLog:
yyyy-mm-dd Pedro Alves <palves@redhat.com>
* minsyms.c (msym_prefer_to_msym_type): New, factored out from ...
(lookup_minimal_symbol_by_pc_section)... here.
---
gdb/minsyms.c | 35 ++++++++++++++++++++++-------------
1 file changed, 22 insertions(+), 13 deletions(-)
diff --git a/gdb/minsyms.c b/gdb/minsyms.c
index 4882e58ee4..4409e6f8b3 100644
--- a/gdb/minsyms.c
+++ b/gdb/minsyms.c
@@ -656,6 +656,27 @@ frob_address (struct objfile *objfile, CORE_ADDR *pc)
return 0;
}
+/* Helper for lookup_minimal_symbol_by_pc_section. Convert a
+ lookup_msym_prefer to a minimal_symbol_type. */
+
+static minimal_symbol_type
+msym_prefer_to_msym_type (lookup_msym_prefer prefer)
+{
+ switch (prefer)
+ {
+ case lookup_msym_prefer::TEXT:
+ return mst_text;
+ case lookup_msym_prefer::TRAMPOLINE:
+ return mst_solib_trampoline;
+ case lookup_msym_prefer::GNU_IFUNC:
+ return mst_text_gnu_ifunc;
+ }
+
+ /* Assert here instead of in a default switch case above so that
+ -Wswitch warns if a new enumerator is added. */
+ gdb_assert_not_reached ("unhandled lookup_msym_prefer");
+}
+
/* Search through the minimal symbol table for each objfile and find
the symbol whose address is the largest address that is still less
than or equal to PC, and matches SECTION (which is not NULL).
@@ -683,7 +704,6 @@ lookup_minimal_symbol_by_pc_section (CORE_ADDR pc_in, struct obj_section *sectio
struct minimal_symbol *best_symbol = NULL;
struct objfile *best_objfile = NULL;
struct bound_minimal_symbol result;
- enum minimal_symbol_type want_type;
if (section == NULL)
{
@@ -692,18 +712,7 @@ lookup_minimal_symbol_by_pc_section (CORE_ADDR pc_in, struct obj_section *sectio
return {};
}
- switch (prefer)
- {
- case lookup_msym_prefer::TEXT:
- want_type = mst_text;
- break;
- case lookup_msym_prefer::TRAMPOLINE:
- want_type = mst_solib_trampoline;
- break;
- case lookup_msym_prefer::GNU_IFUNC:
- want_type = mst_text_gnu_ifunc;
- break;
- }
+ minimal_symbol_type want_type = msym_prefer_to_msym_type (prefer);
/* We can not require the symbol found to be in section, because
e.g. IRIX 6.5 mdebug relies on this code returning an absolute
--
2.14.3
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH] Silence GCC "uninitialized" warning on minsyms.c:lookup_minimal_symbol_by_pc_section
2018-06-19 15:22 ` Pedro Alves
@ 2018-06-19 16:55 ` Sergio Durigan Junior
2018-06-19 18:47 ` Tom Tromey
1 sibling, 0 replies; 33+ messages in thread
From: Sergio Durigan Junior @ 2018-06-19 16:55 UTC (permalink / raw)
To: Pedro Alves; +Cc: GDB Patches
On Tuesday, June 19 2018, Pedro Alves wrote:
> On 06/18/2018 09:26 PM, Sergio Durigan Junior wrote:
>> Commit 20944a6e20324cd897bf6c4c5fd20ef7224dacaa ("Fix stepping past
>> GNU ifunc resolvers (introduce lookup_msym_prefer)") introduced a new
>> way to determine the 'want_type' variable on
>> minsyms.c:lookup_minimal_symbol_by_pc_section. Because the
>> 'lookup_msym_prefer' has a default value, we know that 'want_type'
>> will always be initialized.
>
> Sorry, but that doesn't follow. It's the 'prefer' parameter
> that will always be initialized to something with the default
> value, not the 'want_type' local.
>
> struct bound_minimal_symbol lookup_minimal_symbol_by_pc_section
> (CORE_ADDR,
> struct obj_section *,
> lookup_msym_prefer prefer = lookup_msym_prefer::TEXT);
>
> vs:
>
> enum minimal_symbol_type want_type;
That's correct, when I wrote "will always be initialized" I was thinking
that it will always have a value assigned to it, because of the switch.
> But even that is not necessarily true, since the caller may well
> pass down an explicit argument, which in turn was uninitialized,
> like:
>
> lookup_msym_prefer bogus;
> lookup_minimal_symbol_by_pc_section (addr, NULL, bogus);
That's true as well. And as you said below, we also have to account for
the fact that we may add more entries in the enum, and somehow forget to
update the switch statement. So saying that "it will always have a
value assigned to it" is also not good.
>> However, GCC is complaining that the
>> variable can be used uninitialized in the function:
>>
>> ../../gdb/minsyms.c: In function 'bound_minimal_symbol lookup_minimal_symbol_by_pc_section(CORE_ADDR, obj_section*, lookup_msym_prefer)':
>> ../../gdb/minsyms.c:825:40: warning: 'want_type' may be used uninitialized in this function [-Wmaybe-uninitialized]
>> && MSYMBOL_TYPE (&msymbol[hi]) != want_type
>>
>> (This is with gcc-8.1.1-1.fc29.x86_64).
>>
>> This patch fixes it by initializing 'want_type' with 'mst_text', which
>> is the same default value that is passed in the 'lookup_msym_prefer'
>> variable.
>
> But it's not, as explained above.
>
> The reason this warning is a false positive is that we know that 'want_type'
> is always going to be initialized because the switch to converts
> enum lookup_msym_prefer values to enum minimal_symbol_type values:
>
> switch (prefer)
> {
> case lookup_msym_prefer::TEXT:
> want_type = mst_text;
> break;
> case lookup_msym_prefer::TRAMPOLINE:
> want_type = mst_solib_trampoline;
> break;
> case lookup_msym_prefer::GNU_IFUNC:
> want_type = mst_text_gnu_ifunc;
> break;
> }
>
> has a case for every possible enumerator.
>
> The actual problem is that GCC assumes that enum variables may hold
> values other than the named enumerators, like e.g.,
> "lookup_msym_prefer prefer = (lookup_msym_prefer) 10;".
> We know that this isn't something we want to support with these
> enum types, so it's better to assert that we never see a value
> not covered by the enumerators.
>
> The simplest is to add a "default:" case with a gdb_assert, but
> when I wrote that code, I had not done that on purpose,
> thinking that I'd prefer it if we enabled "-Wswitch" in gdb, which
> helps find switch/cases where we need to handle a new enumerator,
> whenever we add one, like this:
>
> CXX minsyms.o
> src/gdb/minsyms.c: In function ‘bound_minimal_symbol lookup_minimal_symbol_by_pc_section(CORE_ADDR, obj_section*, lookup_msym_prefer)’:
> src/gdb/minsyms.c:695:10: error: enumeration value ‘NEW_KIND’ not handled in switch [-Werror=switch]
> switch (prefer)
> ^
>
> I think that it's preferable, if reasonable, to rework the code a bit to
> make it more explicit to the compiler that a variable is always
> initialized instead of initializing variables to quiet -Wmaybe-uninitialized,
> which is documented as spewing false positives. (IMO it'd be reasonable
> if GCC moved that warning from -Wall to -Wextra.)
>
> Thus I'd prefer this instead:
Fair enough. Thanks for taking a look at this.
> From d66e808818ece92d6d79bfdb4e0a421b30662dd3 Mon Sep 17 00:00:00 2001
> From: Pedro Alves <palves@redhat.com>
> Date: Mon, 18 Jun 2018 22:36:43 +0100
> Subject: [PATCH] Silence -Wmaybe-uninitialized warning in
> minsyms.c:lookup_minimal_symbol_by_pc_section
>
> Compiling with GCC 8.1 shows this warning:
>
> ../../gdb/minsyms.c: In function 'bound_minimal_symbol lookup_minimal_symbol_by_pc_section(CORE_ADDR, obj_section*, lookup_msym_prefer)':
> ../../gdb/minsyms.c:825:40: warning: 'want_type' may be used uninitialized in this function [-Wmaybe-uninitialized]
> && MSYMBOL_TYPE (&msymbol[hi]) != want_type
>
> That warning is a false positive, because the switch that converts
> enum lookup_msym_prefer values to enum enum minimal_symbol_type values
> has a case for every lookup_msym_prefer enumerator:
>
> switch (prefer)
> {
> case lookup_msym_prefer::TEXT:
> want_type = mst_text;
> break;
> case lookup_msym_prefer::TRAMPOLINE:
> want_type = mst_solib_trampoline;
> break;
> case lookup_msym_prefer::GNU_IFUNC:
> want_type = mst_text_gnu_ifunc;
> break;
> }
>
> The problem is that GCC assumes that enum variables may hold values
> other than the named enumerators (like e.g., "lookup_msym_prefer
> prefer = (lookup_msym_prefer) 10;").
>
> Rework the code a bit, adding a gdb_assert to make it explicit to the
> compiler that want_type is initialized in all normal-return paths.
>
> gdb/ChangeLog:
> yyyy-mm-dd Pedro Alves <palves@redhat.com>
>
> * minsyms.c (msym_prefer_to_msym_type): New, factored out from ...
> (lookup_minimal_symbol_by_pc_section)... here.
> ---
> gdb/minsyms.c | 35 ++++++++++++++++++++++-------------
> 1 file changed, 22 insertions(+), 13 deletions(-)
>
> diff --git a/gdb/minsyms.c b/gdb/minsyms.c
> index 4882e58ee4..4409e6f8b3 100644
> --- a/gdb/minsyms.c
> +++ b/gdb/minsyms.c
> @@ -656,6 +656,27 @@ frob_address (struct objfile *objfile, CORE_ADDR *pc)
> return 0;
> }
>
> +/* Helper for lookup_minimal_symbol_by_pc_section. Convert a
> + lookup_msym_prefer to a minimal_symbol_type. */
> +
> +static minimal_symbol_type
> +msym_prefer_to_msym_type (lookup_msym_prefer prefer)
> +{
> + switch (prefer)
> + {
> + case lookup_msym_prefer::TEXT:
> + return mst_text;
> + case lookup_msym_prefer::TRAMPOLINE:
> + return mst_solib_trampoline;
> + case lookup_msym_prefer::GNU_IFUNC:
> + return mst_text_gnu_ifunc;
> + }
> +
> + /* Assert here instead of in a default switch case above so that
> + -Wswitch warns if a new enumerator is added. */
> + gdb_assert_not_reached ("unhandled lookup_msym_prefer");
> +}
> +
> /* Search through the minimal symbol table for each objfile and find
> the symbol whose address is the largest address that is still less
> than or equal to PC, and matches SECTION (which is not NULL).
> @@ -683,7 +704,6 @@ lookup_minimal_symbol_by_pc_section (CORE_ADDR pc_in, struct obj_section *sectio
> struct minimal_symbol *best_symbol = NULL;
> struct objfile *best_objfile = NULL;
> struct bound_minimal_symbol result;
> - enum minimal_symbol_type want_type;
>
> if (section == NULL)
> {
> @@ -692,18 +712,7 @@ lookup_minimal_symbol_by_pc_section (CORE_ADDR pc_in, struct obj_section *sectio
> return {};
> }
>
> - switch (prefer)
> - {
> - case lookup_msym_prefer::TEXT:
> - want_type = mst_text;
> - break;
> - case lookup_msym_prefer::TRAMPOLINE:
> - want_type = mst_solib_trampoline;
> - break;
> - case lookup_msym_prefer::GNU_IFUNC:
> - want_type = mst_text_gnu_ifunc;
> - break;
> - }
> + minimal_symbol_type want_type = msym_prefer_to_msym_type (prefer);
>
> /* We can not require the symbol found to be in section, because
> e.g. IRIX 6.5 mdebug relies on this code returning an absolute
> --
> 2.14.3
--
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF 31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH] Silence GCC "uninitialized" warning on minsyms.c:lookup_minimal_symbol_by_pc_section
2018-06-19 15:22 ` Pedro Alves
2018-06-19 16:55 ` Sergio Durigan Junior
@ 2018-06-19 18:47 ` Tom Tromey
1 sibling, 0 replies; 33+ messages in thread
From: Tom Tromey @ 2018-06-19 18:47 UTC (permalink / raw)
To: Pedro Alves; +Cc: Sergio Durigan Junior, GDB Patches
>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
Pedro> The simplest is to add a "default:" case with a gdb_assert, but
Pedro> when I wrote that code, I had not done that on purpose,
Pedro> thinking that I'd prefer it if we enabled "-Wswitch" in gdb, which
Pedro> helps find switch/cases where we need to handle a new enumerator,
Pedro> whenever we add one, like this:
I looked at adding -Wswitch or -Wswitch-enum once. All I really
remember is thinking that there were cases where gdb would not want
this.
However, I think maybe it can be done on a case-by-case basis with
"#pragma GCC diagnostic push".
Tom
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH v2 07/15] Breakpoints, don't skip prologue of ifunc resolvers with debug info
2018-03-25 19:19 [PATCH v2 00/15] Fixing GNU ifunc support Pedro Alves
` (6 preceding siblings ...)
2018-03-25 19:19 ` [PATCH v2 11/15] Fix stepping past GNU ifunc resolvers (introduce lookup_msym_prefer) Pedro Alves
@ 2018-03-25 19:19 ` Pedro Alves
2018-03-25 19:19 ` [PATCH v2 12/15] For PPC64/ELFv1: Introduce mst_data_gnu_ifunc Pedro Alves
` (7 subsequent siblings)
15 siblings, 0 replies; 33+ messages in thread
From: Pedro Alves @ 2018-03-25 19:19 UTC (permalink / raw)
To: gdb-patches
In v2:
- The minsym/symbol matching in convert_linespec_to_sals is done
differently. The v1 bsearch method wouldn't work for PPC64, given
function descriptors. convert_linespec_to_sals will be touched
again later in the series, for PPC64.
- Moved declarations of locals closer to uses, because at some point
I managed to reuse 'i' in an inner loop messing the outer loop...
Without this patch, some of the tests added to gdb.base/gnu-ifunc.exp
by a following patch fail like so:
FAIL: gdb.base/gnu-ifunc.exp: resolver_attr=0: resolver_debug=1: resolved_debug=0: set-break: before resolving: break gnu_ifunc
FAIL: gdb.base/gnu-ifunc.exp: resolver_attr=0: resolver_debug=1: resolved_debug=0: set-break: before resolving: info breakpoints
FAIL: gdb.base/gnu-ifunc.exp: resolver_attr=0: resolver_debug=1: resolved_debug=0: set-break: after resolving: break gnu_ifunc
FAIL: gdb.base/gnu-ifunc.exp: resolver_attr=0: resolver_debug=1: resolved_debug=0: set-break: after resolving: info breakpoints
FAIL: gdb.base/gnu-ifunc.exp: resolver_attr=0: resolver_debug=1: resolved_debug=1: set-break: before resolving: break gnu_ifunc
FAIL: gdb.base/gnu-ifunc.exp: resolver_attr=0: resolver_debug=1: resolved_debug=1: set-break: before resolving: info breakpoints
FAIL: gdb.base/gnu-ifunc.exp: resolver_attr=0: resolver_debug=1: resolved_debug=1: set-break: after resolving: break gnu_ifunc
FAIL: gdb.base/gnu-ifunc.exp: resolver_attr=0: resolver_debug=1: resolved_debug=1: set-break: after resolving: info breakpoints
All of them trigger iff:
- you have debug info for the ifunc resolver.
- the resolver and the user-visible symbol have the same name.
If you have an ifunc that has a resolver with the same name as the
user visible symbol, debug info for the resolver masks out the ifunc
minsym. When you set a breakpoint by name on the user visible symbol,
GDB finds the DWARF symbol for the resolver, and thinking that it's a
regular function, sets a breakpoint location past its prologue.
Like so, location 1.2, before the ifunc is resolved by the inferior:
(gdb) break gnu_ifunc
Breakpoint 2 at 0x7ffff7bd36ea (2 locations)
(gdb) info breakpoints
Num Type Disp Enb Address What
1 breakpoint keep y <MULTIPLE>
1.1 y 0x00007ffff7bd36ea <gnu_ifunc>
1.2 y 0x00007ffff7bd36f2 in gnu_ifunc at src/gdb/testsuite/gdb.base/gnu-ifunc-lib.c:34
(gdb)
And like so, location 2.2, if you set the breakpoint after the ifunc
is resolved by the inferior (to "final"):
(gdb) break gnu_ifunc
Breakpoint 5 at 0x400757 (2 locations)
(gdb) info breakpoints
Num Type Disp Enb Address What
2 breakpoint keep y <MULTIPLE>
2.1 y 0x000000000040075a in final at src/gdb/testsuite/gdb.base/gnu-ifunc-resd.c:21
2.2 y 0x00007ffff7bd36f2 in gnu_ifunc at src/gdb/testsuite/gdb.base/gnu-ifunc-lib.c:34
(gdb)
I don't think this is right because when users set a breakpoint at an
ifunc, they don't care about debugging the resolver. Instead what you
should is a single location for the ifunc in the first case, and a
single location of the ifunc target in the second case.
gdb/ChangeLog:
yyyy-mm-dd Pedro Alves <palves@redhat.com>
* linespec.c (struct bound_minimal_symbol_search_key): New.
(convert_linespec_to_sals): Sort minimal symbols earlier. Don't
skip first line if we found a GNU ifunc minimal symbol by name.
(compare_msymbols): Change parameters to work with a destructured
lhs minsym.
(compare_msymbols_for_qsort, compare_msymbols_for_bsearch): New
functions.
---
gdb/linespec.c | 60 +++++++++++++++++++++++++++++++++++++++++++---------------
1 file changed, 45 insertions(+), 15 deletions(-)
diff --git a/gdb/linespec.c b/gdb/linespec.c
index c549ba09349..70f23187e02 100644
--- a/gdb/linespec.c
+++ b/gdb/linespec.c
@@ -2308,12 +2308,6 @@ convert_linespec_to_sals (struct linespec_state *state, linespec_p ls)
else if (ls->function_symbols != NULL || ls->minimal_symbols != NULL)
{
/* We have just a bunch of functions and/or methods. */
- int i;
- struct symtab_and_line sal;
- struct symbol *sym;
- bound_minimal_symbol_d *elem;
- struct program_space *pspace;
-
if (ls->function_symbols != NULL)
{
/* Sort symbols so that symbols with the same program space are next
@@ -2322,30 +2316,66 @@ convert_linespec_to_sals (struct linespec_state *state, linespec_p ls)
VEC_length (symbolp, ls->function_symbols),
sizeof (symbolp), compare_symbols);
- for (i = 0; VEC_iterate (symbolp, ls->function_symbols, i, sym); ++i)
+ struct symbol *sym;
+ for (int i = 0; VEC_iterate (symbolp, ls->function_symbols, i, sym); ++i)
{
- pspace = SYMTAB_PSPACE (symbol_symtab (sym));
+ program_space *pspace = SYMTAB_PSPACE (symbol_symtab (sym));
set_current_program_space (pspace);
- if (symbol_to_sal (&sal, state->funfirstline, sym)
- && maybe_add_address (state->addr_set, pspace, sal.pc))
- add_sal_to_sals (state, &sals, &sal,
- SYMBOL_NATURAL_NAME (sym), 0);
+
+ /* Don't skip to the first line of the function if we
+ had found an ifunc minimal symbol for this function,
+ because that means that this function is an ifunc
+ resolver with the same name as the ifunc itself. */
+ bool found_ifunc = false;
+
+ if (state->funfirstline
+ && ls->minimal_symbols != NULL
+ && SYMBOL_CLASS (sym) == LOC_BLOCK)
+ {
+ const CORE_ADDR addr
+ = BLOCK_START (SYMBOL_BLOCK_VALUE (sym));
+
+ bound_minimal_symbol_d *elem;
+ for (int m = 0;
+ VEC_iterate (bound_minimal_symbol_d, ls->minimal_symbols,
+ m, elem);
+ ++m)
+ {
+ if (MSYMBOL_TYPE (elem->minsym) == mst_text_gnu_ifunc
+ && BMSYMBOL_VALUE_ADDRESS (*elem) == addr)
+ {
+ found_ifunc = true;
+ break;
+ }
+ }
+ }
+
+ if (!found_ifunc)
+ {
+ symtab_and_line sal;
+ if (symbol_to_sal (&sal, state->funfirstline, sym)
+ && maybe_add_address (state->addr_set, pspace, sal.pc))
+ add_sal_to_sals (state, &sals, &sal,
+ SYMBOL_NATURAL_NAME (sym), 0);
+ }
}
}
if (ls->minimal_symbols != NULL)
{
- /* Sort minimal symbols by program space, too. */
+ /* Sort minimal symbols by program space, too */
qsort (VEC_address (bound_minimal_symbol_d, ls->minimal_symbols),
VEC_length (bound_minimal_symbol_d, ls->minimal_symbols),
sizeof (bound_minimal_symbol_d), compare_msymbols);
- for (i = 0;
+ bound_minimal_symbol_d *elem;
+
+ for (int i = 0;
VEC_iterate (bound_minimal_symbol_d, ls->minimal_symbols,
i, elem);
++i)
{
- pspace = elem->objfile->pspace;
+ program_space *pspace = elem->objfile->pspace;
set_current_program_space (pspace);
minsym_found (state, elem->objfile, elem->minsym, &sals);
}
--
2.14.3
^ permalink raw reply [flat|nested] 33+ messages in thread* [PATCH v2 12/15] For PPC64/ELFv1: Introduce mst_data_gnu_ifunc
2018-03-25 19:19 [PATCH v2 00/15] Fixing GNU ifunc support Pedro Alves
` (7 preceding siblings ...)
2018-03-25 19:19 ` [PATCH v2 07/15] Breakpoints, don't skip prologue of ifunc resolvers with debug info Pedro Alves
@ 2018-03-25 19:19 ` Pedro Alves
2018-03-25 19:25 ` [PATCH v2 04/15] Calling ifunc functions when resolver has debug info, user symbol same name Pedro Alves
` (6 subsequent siblings)
15 siblings, 0 replies; 33+ messages in thread
From: Pedro Alves @ 2018-03-25 19:19 UTC (permalink / raw)
To: gdb-patches
Running the new tests added later in the series on PPC64 (ELFv1)
revealed that the current ifunc support needs a bit of a design rework
to work properly on PPC64/ELFv1, as most of the new tests fail. The
ifunc support only kind of works today if the ifunc symbol and the
resolver have the same name, as is currently tested by the
gdb.base/gnu-ifunc.exp testcase, which is unlike how ifuncs are
written nowadays.
The crux of the problem is that ifunc symbols are really function
descriptors, not text symbols:
44: 0000000000020060 104 FUNC GLOBAL DEFAULT 18 gnu_ifunc_resolver
54: 0000000000020060 104 GNU_IFUNC GLOBAL DEFAULT 18 gnu_ifunc
But, currently GDB only knows about ifunc symbols that are text
symbols. GDB's support happens to work in practice for PPC64 when the
ifunc and resolver are one and only, like in the current
gdb.base/gnu-ifunc.exp testcase:
15: 0000000000020060 104 GNU_IFUNC GLOBAL DEFAULT 18 gnu_ifunc
because in that case, the synthetic ".gnu_ifunc" entry point text
symbol that bfd creates from the actual GNU ifunc "gnu_ifunc" function
(descriptor) symbol ends up with the the "is a gnu ifunc" flag set /
copied over:
(gdb) maint print msymbols
...
[ 8] i 0x9c4 .gnu_ifunc section .text <<< mst_text_gnu_ifunc
...
[29] D 0x20060 gnu_ifunc section .opd crtstuff.c <<< mst_data
But, if the resolver gets a distinct symbol/name from the ifunc
symbol, then we end up with this:
(gdb) maint print msymbols
[ 8] T 0x9e4 .gnu_ifunc_resolver section .text <<< mst_text
...
[29] D 0x20060 gnu_ifunc section .opd crtstuff.c <<< mst_data
[30] D 0x20060 gnu_ifunc_resolver section .opd crtstuff.c <<< mst_data
I have a follow up bfd patch that turns that into:
(gdb) maint print msymbols
+ [ 8] i 0x9e4 .gnu_ifunc section .text <<< mst_text_gnu_ifunc
[ 8] T 0x9e4 .gnu_ifunc_resolver section .text <<< mst_text
...
[29] D 0x20060 gnu_ifunc section .opd crtstuff.c
[30] D 0x20060 gnu_ifunc_resolver section .opd crtstuff.c
but that won't help everything. We still need this patch.
Specifically, when we do a symbol lookup by name, like e.g., to call a
function (see c-exp.y hunk), e.g., "p gnu_ifunc()", then we need to
know that the found "gnu_ifunc" minimal symbol is an ifunc in order to
do some special processing. But, on PPC, that lookup by name finds
the function descriptor symbol, which presently is just a mst_data
symbol, while at present, we look for mst_text_gnu_ifunc symbols to
decide whether to do special GNU ifunc processing. In most of those
places, we could try to resolve the function descriptor with
gdbarch_convert_from_func_ptr_addr, and then lookup the minimal symbol
at the resolved PC, see if that finds a minimal symbol of type
mst_text_gnu_ifunc. If so, then we could assume that the original
mst_dadta / function descriptor "gnu_ifunc" symbol was an ifunc. I
tried it, and it mostly works, even if it's not the most efficient.
However, there's one case that can't work with such a design -- it's
that of the user calling the ifunc resolver directly to debug it, like
"p gnu_ifunc_resolver(0)", expecting that to return the function
pointer of the final function (which is exercised by the new tests
added later). In this case, with the not-fully-working solution, we'd
resolve the function descriptor, find that there's an
mst_text_gnu_ifunc symbol for the resolved address, and proceed
calling the function as if we tried to call "gnu_ifunc", the
user-visible GNU ifunc symbol, instead of the resolver. I.e., it'd be
impossible to call the resolver directly as a normal function.
Introducing mst_data_gnu_ifunc eliminates the need for several
gdbarch_convert_from_func_ptr_addr calls, and, fixes the "call
resolver directly" use case mentioned above too. It's the cleanest
approach I could think of.
In sum, we make GNU ifunc function descriptor symbols get a new
"mst_data_gnu_ifunc" minimal symbol type instead of the bare mst_data
type. So when symbol lookup by name finds such a minimal symbol, we
know we found an ifunc symbol, without resolving the entry/text
symbol. If the user calls the the resolver symbol instead, like "p
gnu_ifunc_resolver(0)", then we'll find the regular mst_data symbol
for "gnu_ifunc_resolver", and we'll call the resolver function as just
another regular function.
With this, most of the GNU ifunc tests added by a later patch pass on
PPC64 too. The following bfd patch fixes the remaining issues.
gdb/ChangeLog:
yyyy-mm-dd Pedro Alves <palves@redhat.com>
* breakpoint.c (set_breakpoint_location_function): Handle
mst_data_gnu_ifunc.
* c-exp.y (variable production): Handle mst_data_gnu_ifunc.
* elfread.c (elf_symtab_read): Give data symbols with
BSF_GNU_INDIRECT_FUNCTION set mst_data_gnu_ifunc type.
(elf_rel_plt_read): Update comment.
* linespec.c (convert_linespec_to_sals): Handle
mst_data_gnu_ifunc.
(minsym_found): Handle mst_data_gnu_ifunc.
* minsyms.c (msymbol_is_function, minimal_symbol_reader::record)
(find_solib_trampoline_target): Handle mst_data_gnu_ifunc.
* parse.c (find_minsym_type_and_address): Handle
mst_data_gnu_ifunc.
* symmisc.c (dump_msymbols): Handle mst_data_gnu_ifunc.
* symtab.c (find_gnu_ifunc): Handle mst_data_gnu_ifunc.
* symtab.h (minimal_symbol_type) <mst_text_gnu_ifunc>: Update
comment.
<mst_data_gnu_ifunc>: New enumerator.
---
gdb/breakpoint.c | 3 ++-
gdb/c-exp.y | 3 ++-
gdb/elfread.c | 13 +++++++++----
gdb/linespec.c | 24 ++++++++++++++++++++----
gdb/minsyms.c | 23 +++++++++--------------
gdb/parse.c | 45 +++++++++++++++++++--------------------------
gdb/symmisc.c | 1 +
gdb/symtab.c | 17 ++++++++++++++---
gdb/symtab.h | 20 +++++++++++++++++++-
9 files changed, 95 insertions(+), 54 deletions(-)
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 6840a200183..43b60871f1e 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -7183,7 +7183,8 @@ set_breakpoint_location_function (struct bp_location *loc, int explicit_loc)
const char *function_name;
if (loc->msymbol != NULL
- && MSYMBOL_TYPE (loc->msymbol) == mst_text_gnu_ifunc
+ && (MSYMBOL_TYPE (loc->msymbol) == mst_text_gnu_ifunc
+ || MSYMBOL_TYPE (loc->msymbol) == mst_data_gnu_ifunc)
&& !explicit_loc)
{
struct breakpoint *b = loc->owner;
diff --git a/gdb/c-exp.y b/gdb/c-exp.y
index 723249c1f58..9e2f80889f1 100644
--- a/gdb/c-exp.y
+++ b/gdb/c-exp.y
@@ -1093,7 +1093,8 @@ variable: name_not_typename
is important for example for "p
*__errno_location()". */
symbol *alias_target
- = (msymbol.minsym->type != mst_text_gnu_ifunc
+ = ((msymbol.minsym->type != mst_text_gnu_ifunc
+ && msymbol.minsym->type != mst_data_gnu_ifunc)
? find_function_alias_target (msymbol)
: NULL);
if (alias_target != NULL)
diff --git a/gdb/elfread.c b/gdb/elfread.c
index 4e79b080e65..2a876e1bcc4 100644
--- a/gdb/elfread.c
+++ b/gdb/elfread.c
@@ -422,7 +422,11 @@ elf_symtab_read (minimal_symbol_reader &reader,
{
if (sym->flags & (BSF_GLOBAL | BSF_WEAK | BSF_GNU_UNIQUE))
{
- if (sym->section->flags & SEC_LOAD)
+ if (sym->flags & BSF_GNU_INDIRECT_FUNCTION)
+ {
+ ms_type = mst_data_gnu_ifunc;
+ }
+ else if (sym->section->flags & SEC_LOAD)
{
ms_type = mst_data;
}
@@ -621,9 +625,10 @@ elf_rel_plt_read (minimal_symbol_reader &reader,
else
continue;
- /* We cannot check if NAME is a reference to mst_text_gnu_ifunc as in
- OBJFILE the symbol is undefined and the objfile having NAME defined
- may not yet have been loaded. */
+ /* We cannot check if NAME is a reference to
+ mst_text_gnu_ifunc/mst_data_gnu_ifunc as in OBJFILE the
+ symbol is undefined and the objfile having NAME defined may
+ not yet have been loaded. */
string_buffer.assign (name);
string_buffer.append (got_suffix, got_suffix + got_suffix_len);
diff --git a/gdb/linespec.c b/gdb/linespec.c
index 74ddc9fe71d..277e6048d3a 100644
--- a/gdb/linespec.c
+++ b/gdb/linespec.c
@@ -2342,10 +2342,25 @@ convert_linespec_to_sals (struct linespec_state *state, linespec_p ls)
++m)
{
if (MSYMBOL_TYPE (elem->minsym) == mst_text_gnu_ifunc
- && BMSYMBOL_VALUE_ADDRESS (*elem) == addr)
+ || MSYMBOL_TYPE (elem->minsym) == mst_data_gnu_ifunc)
{
- found_ifunc = true;
- break;
+ CORE_ADDR msym_addr = BMSYMBOL_VALUE_ADDRESS (*elem);
+ if (MSYMBOL_TYPE (elem->minsym) == mst_data_gnu_ifunc)
+ {
+ struct gdbarch *gdbarch
+ = get_objfile_arch (elem->objfile);
+ msym_addr
+ = (gdbarch_convert_from_func_ptr_addr
+ (gdbarch,
+ msym_addr,
+ ¤t_target));
+ }
+
+ if (msym_addr == addr)
+ {
+ found_ifunc = true;
+ break;
+ }
}
}
}
@@ -4373,7 +4388,8 @@ minsym_found (struct linespec_state *self, struct objfile *objfile,
{
const char *msym_name = MSYMBOL_LINKAGE_NAME (msymbol);
- if (MSYMBOL_TYPE (msymbol) == mst_text_gnu_ifunc)
+ if (MSYMBOL_TYPE (msymbol) == mst_text_gnu_ifunc
+ || MSYMBOL_TYPE (msymbol) == mst_data_gnu_ifunc)
want_start_sal = gnu_ifunc_resolve_name (msym_name, &func_addr);
else
want_start_sal = true;
diff --git a/gdb/minsyms.c b/gdb/minsyms.c
index f71e2db484e..b6c3417bea9 100644
--- a/gdb/minsyms.c
+++ b/gdb/minsyms.c
@@ -70,6 +70,7 @@ msymbol_is_function (struct objfile *objfile, minimal_symbol *minsym,
case mst_abs:
case mst_file_data:
case mst_file_bss:
+ case mst_data_gnu_ifunc:
{
struct gdbarch *gdbarch = get_objfile_arch (objfile);
CORE_ADDR pc = gdbarch_convert_from_func_ptr_addr (gdbarch, msym_addr,
@@ -1089,6 +1090,7 @@ minimal_symbol_reader::record (const char *name, CORE_ADDR address,
section = SECT_OFF_TEXT (m_objfile);
break;
case mst_data:
+ case mst_data_gnu_ifunc:
case mst_file_data:
section = SECT_OFF_DATA (m_objfile);
break;
@@ -1489,26 +1491,19 @@ find_solib_trampoline_target (struct frame_info *frame, CORE_ADDR pc)
{
ALL_MSYMBOLS (objfile, msymbol)
{
- if ((MSYMBOL_TYPE (msymbol) == mst_text
- || MSYMBOL_TYPE (msymbol) == mst_text_gnu_ifunc)
- && strcmp (MSYMBOL_LINKAGE_NAME (msymbol),
- MSYMBOL_LINKAGE_NAME (tsymbol)) == 0)
- return MSYMBOL_VALUE_ADDRESS (objfile, msymbol);
-
/* Also handle minimal symbols pointing to function descriptors. */
- if (MSYMBOL_TYPE (msymbol) == mst_data
+ if ((MSYMBOL_TYPE (msymbol) == mst_text
+ || MSYMBOL_TYPE (msymbol) == mst_text_gnu_ifunc
+ || MSYMBOL_TYPE (msymbol) == mst_data
+ || MSYMBOL_TYPE (msymbol) == mst_data_gnu_ifunc)
&& strcmp (MSYMBOL_LINKAGE_NAME (msymbol),
MSYMBOL_LINKAGE_NAME (tsymbol)) == 0)
{
CORE_ADDR func;
- func = gdbarch_convert_from_func_ptr_addr
- (get_objfile_arch (objfile),
- MSYMBOL_VALUE_ADDRESS (objfile, msymbol),
- ¤t_target);
-
- /* Ignore data symbols that are not function descriptors. */
- if (func != MSYMBOL_VALUE_ADDRESS (objfile, msymbol))
+ /* Ignore data symbols that are not function
+ descriptors. */
+ if (msymbol_is_function (objfile, msymbol, &func))
return func;
}
}
diff --git a/gdb/parse.c b/gdb/parse.c
index 3abb9d42191..1d53b5aa1ac 100644
--- a/gdb/parse.c
+++ b/gdb/parse.c
@@ -461,42 +461,35 @@ find_minsym_type_and_address (minimal_symbol *msymbol,
struct gdbarch *gdbarch = get_objfile_arch (objfile);
struct obj_section *section = MSYMBOL_OBJ_SECTION (objfile, msymbol);
enum minimal_symbol_type type = MSYMBOL_TYPE (msymbol);
- CORE_ADDR pc;
bool is_tls = (section != NULL
&& section->the_bfd_section->flags & SEC_THREAD_LOCAL);
- /* Addresses of TLS symbols are really offsets into a
- per-objfile/per-thread storage block. */
- CORE_ADDR addr = (is_tls
- ? MSYMBOL_VALUE_RAW_ADDRESS (bound_msym.minsym)
- : BMSYMBOL_VALUE_ADDRESS (bound_msym));
-
/* The minimal symbol might point to a function descriptor;
resolve it to the actual code address instead. */
- pc = gdbarch_convert_from_func_ptr_addr (gdbarch, addr, ¤t_target);
- if (pc != addr)
+ CORE_ADDR addr;
+ if (is_tls)
{
- struct bound_minimal_symbol ifunc_msym = lookup_minimal_symbol_by_pc (pc);
-
- /* In this case, assume we have a code symbol instead of
- a data symbol. */
-
- if (ifunc_msym.minsym != NULL
- && MSYMBOL_TYPE (ifunc_msym.minsym) == mst_text_gnu_ifunc
- && BMSYMBOL_VALUE_ADDRESS (ifunc_msym) == pc)
+ /* Addresses of TLS symbols are really offsets into a
+ per-objfile/per-thread storage block. */
+ addr = MSYMBOL_VALUE_RAW_ADDRESS (bound_msym.minsym);
+ }
+ else if (msymbol_is_function (objfile, msymbol, &addr))
+ {
+ if (addr != BMSYMBOL_VALUE_ADDRESS (bound_msym))
{
- /* A function descriptor has been resolved but PC is still in the
- STT_GNU_IFUNC resolver body (such as because inferior does not
- run to be able to call it). */
-
- type = mst_text_gnu_ifunc;
+ /* This means we resolved a function descriptor, and we now
+ have an address for a code/text symbol instead of a data
+ symbol. */
+ if (MSYMBOL_TYPE (msymbol) == mst_data_gnu_ifunc)
+ type = mst_text_gnu_ifunc;
+ else
+ type = mst_text;
+ section = NULL;
}
- else
- type = mst_text;
- section = NULL;
- addr = pc;
}
+ else
+ addr = BMSYMBOL_VALUE_ADDRESS (bound_msym);
if (overlay_debugging)
addr = symbol_overlayed_address (addr, section);
diff --git a/gdb/symmisc.c b/gdb/symmisc.c
index 9adde044cdb..91ddc578a8d 100644
--- a/gdb/symmisc.c
+++ b/gdb/symmisc.c
@@ -209,6 +209,7 @@ dump_msymbols (struct objfile *objfile, struct ui_file *outfile)
ms_type = 'T';
break;
case mst_text_gnu_ifunc:
+ case mst_data_gnu_ifunc:
ms_type = 'i';
break;
case mst_solib_trampoline:
diff --git a/gdb/symtab.c b/gdb/symtab.c
index 8b6d26cd843..8a5694e4917 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -4969,10 +4969,21 @@ find_gnu_ifunc (const symbol *sym)
[&] (minimal_symbol *minsym)
{
if (MSYMBOL_TYPE (minsym) == mst_text_gnu_ifunc
- && MSYMBOL_VALUE_ADDRESS (objfile, minsym) == address)
+ || MSYMBOL_TYPE (minsym) == mst_data_gnu_ifunc)
{
- ifunc = minsym;
- return true;
+ CORE_ADDR msym_addr = MSYMBOL_VALUE_ADDRESS (objfile, minsym);
+ if (MSYMBOL_TYPE (minsym) == mst_data_gnu_ifunc)
+ {
+ struct gdbarch *gdbarch = get_objfile_arch (objfile);
+ msym_addr = gdbarch_convert_from_func_ptr_addr (gdbarch,
+ msym_addr,
+ ¤t_target);
+ }
+ if (msym_addr == address)
+ {
+ ifunc = minsym;
+ return true;
+ }
}
return false;
});
diff --git a/gdb/symtab.h b/gdb/symtab.h
index 7c56c5319c0..da63de36c6f 100644
--- a/gdb/symtab.h
+++ b/gdb/symtab.h
@@ -580,8 +580,26 @@ enum minimal_symbol_type
{
mst_unknown = 0, /* Unknown type, the default */
mst_text, /* Generally executable instructions */
- mst_text_gnu_ifunc, /* Executable code returning address
+
+ /* A GNU ifunc symbol, in the .text section. GDB uses to know
+ whether the user is setting a breakpoint on a GNU ifunc function,
+ and thus GDB needs to actually set the breakpoint on the target
+ function. It is also used to know whether the program stepped
+ into an ifunc resolver -- the resolver may get a separate
+ symbol/alias under a different name, but it'll have the same
+ address as the ifunc symbol. */
+ mst_text_gnu_ifunc, /* Executable code returning address
+ of executable code */
+
+ /* A GNU ifunc function descriptor symbol, in a data section
+ (typically ".opd"). Seen on architectures that use function
+ descriptors, like PPC64/ELFv1. In this case, this symbol's value
+ is the address of the descriptor. There'll be a corresponding
+ mst_text_gnu_ifunc synthetic symbol for the text/entry
+ address. */
+ mst_data_gnu_ifunc, /* Executable code returning address
of executable code */
+
mst_slot_got_plt, /* GOT entries for .plt sections */
mst_data, /* Generally initialized data */
mst_bss, /* Generally uninitialized data */
--
2.14.3
^ permalink raw reply [flat|nested] 33+ messages in thread* [PATCH v2 04/15] Calling ifunc functions when resolver has debug info, user symbol same name
2018-03-25 19:19 [PATCH v2 00/15] Fixing GNU ifunc support Pedro Alves
` (8 preceding siblings ...)
2018-03-25 19:19 ` [PATCH v2 12/15] For PPC64/ELFv1: Introduce mst_data_gnu_ifunc Pedro Alves
@ 2018-03-25 19:25 ` Pedro Alves
2018-03-25 19:25 ` [PATCH v2 09/15] Factor out minsym_found/find_function_start_sal overload Pedro Alves
` (5 subsequent siblings)
15 siblings, 0 replies; 33+ messages in thread
From: Pedro Alves @ 2018-03-25 19:25 UTC (permalink / raw)
To: gdb-patches
In v2:
- find_gnu_ifunc is now based on name search. Need exposed by
PP64 (see later patches in the series).
- Added iterate_over_minimal_symbols function_view overload for
that, and made it possible to stop the search if the callback says
so.
If the GNU ifunc resolver has the same name as the user visible
symbol, and the resolver has debug info, then the DWARF info for the
resolver masks the ifunc minsym. In that scenario, if you try calling
the ifunc from GDB, you call the resolver instead. With the
gnu-ifunc.exp testcase added in a following patch, you'd see:
(gdb) p gnu_ifunc (3)
$1 = (int (*)(int)) 0x400753 <final>
(gdb) FAIL: gdb.base/gnu-ifunc.exp: resolver_attr=0: resolver_debug=1: resolved_debug=0: p gnu_ifunc (3)
^^^^^^^^^^^^^^^^
That is, we called the ifunc resolver manually, which returned a
pointer to the ifunc target function ("final"). The "final" symbol is
the function that GDB should have called automatically,
~~~~~~~~~~~~
int
final (int arg)
{
return arg + 1;
}
~~~~~~~~~
which is what happens if you don't have debug info for the resolver:
(gdb) p gnu_ifunc (3)
$1 = 4
(gdb) PASS: gdb.base/gnu-ifunc.exp: resolver_attr=0: resolver_debug=0: resolved_debug=1: p gnu_ifunc (3)
^^^^^^^^^^^^^^^^
or if the resolver's symbol has a different name from the ifunc (as is
the case with modern uses of ifunc via __attribute__ ifunc, such as
glibc uses):
(gdb) p gnu_ifunc (3)
$1 = 4
(gdb) PASS: gdb.base/gnu-ifunc.exp: resolver_attr=1: resolver_debug=1: resolved_debug=0: p gnu_ifunc (3)
^^^^^^^^^^^^^^^
in which case after this patch, you can still call the resolver
directly if you want:
(gdb) p gnu_ifunc_resolver (3)
$1 = (int (*)(int)) 0x400753 <final>
gdb/ChangeLog:
yyyy-mm-dd Pedro Alves <palves@redhat.com>
* c-exp.y (variable production): Prefer ifunc minsyms over
regular function symbols.
* symtab.c (find_gnu_ifunc): New function.
* minsyms.h (lookup_msym_prefer): New enum.
(lookup_minimal_symbol_by_pc_section): Replace 'want_trampoline'
parameter by a lookup_msym_prefer parameter.
* symtab.h (find_gnu_ifunc): New declaration.
---
gdb/c-exp.y | 20 ++++++++++++++++----
gdb/linespec.c | 7 ++++---
gdb/minsyms.c | 30 +++++++++++++++++++++++-------
gdb/minsyms.h | 13 +++++++++++--
gdb/symtab.c | 32 ++++++++++++++++++++++++++++++++
gdb/symtab.h | 3 +++
6 files changed, 89 insertions(+), 16 deletions(-)
diff --git a/gdb/c-exp.y b/gdb/c-exp.y
index e2ea07cd792..723249c1f58 100644
--- a/gdb/c-exp.y
+++ b/gdb/c-exp.y
@@ -1041,10 +1041,22 @@ variable: name_not_typename
if (symbol_read_needs_frame (sym.symbol))
innermost_block.update (sym);
- write_exp_elt_opcode (pstate, OP_VAR_VALUE);
- write_exp_elt_block (pstate, sym.block);
- write_exp_elt_sym (pstate, sym.symbol);
- write_exp_elt_opcode (pstate, OP_VAR_VALUE);
+ /* If we found a function, see if it's
+ an ifunc resolver that has the same
+ address as the ifunc symbol itself.
+ If so, prefer the ifunc symbol. */
+
+ bound_minimal_symbol resolver
+ = find_gnu_ifunc (sym.symbol);
+ if (resolver.minsym != NULL)
+ write_exp_msymbol (pstate, resolver);
+ else
+ {
+ write_exp_elt_opcode (pstate, OP_VAR_VALUE);
+ write_exp_elt_block (pstate, sym.block);
+ write_exp_elt_sym (pstate, sym.symbol);
+ write_exp_elt_opcode (pstate, OP_VAR_VALUE);
+ }
}
else if ($1.is_a_field_of_this)
{
diff --git a/gdb/linespec.c b/gdb/linespec.c
index 1236b3f4754..7ff8dcf3a5b 100644
--- a/gdb/linespec.c
+++ b/gdb/linespec.c
@@ -4431,7 +4431,7 @@ compare_msyms (const void *a, const void *b)
/* Callback for iterate_over_minimal_symbols that adds the symbol to
the result. */
-static void
+static bool
add_minsym (struct minimal_symbol *minsym, void *d)
{
struct collect_minsyms *info = (struct collect_minsyms *) d;
@@ -4446,16 +4446,17 @@ add_minsym (struct minimal_symbol *minsym, void *d)
symtab_and_line sal = find_pc_sect_line (func_addr, NULL, 0);
if (info->symtab != sal.symtab)
- return;
+ return false;
}
}
/* Exclude data symbols when looking for breakpoint locations. */
if (!info->list_mode && !msymbol_is_function (info->objfile, minsym))
- return;
+ return false;
bound_minimal_symbol_d mo = {minsym, info->objfile};
VEC_safe_push (bound_minimal_symbol_d, info->msyms, &mo);
+ return false;
}
/* Search for minimal symbols called NAME. If SEARCH_PSPACE
diff --git a/gdb/minsyms.c b/gdb/minsyms.c
index 72969b77787..c75c316c08d 100644
--- a/gdb/minsyms.c
+++ b/gdb/minsyms.c
@@ -469,11 +469,10 @@ linkage_name_str (const lookup_name_info &lookup_name)
/* See minsyms.h. */
void
-iterate_over_minimal_symbols (struct objfile *objf,
- const lookup_name_info &lookup_name,
- void (*callback) (struct minimal_symbol *,
- void *),
- void *user_data)
+iterate_over_minimal_symbols
+ (struct objfile *objf,
+ const lookup_name_info &lookup_name,
+ gdb::function_view<bool (minimal_symbol *)> callback)
{
/* The first pass is over the ordinary hash table. */
@@ -490,7 +489,8 @@ iterate_over_minimal_symbols (struct objfile *objf,
iter = iter->hash_next)
{
if (mangled_cmp (MSYMBOL_LINKAGE_NAME (iter), name) == 0)
- (*callback) (iter, user_data);
+ if (callback (iter))
+ return;
}
}
@@ -509,12 +509,28 @@ iterate_over_minimal_symbols (struct objfile *objf,
iter != NULL;
iter = iter->demangled_hash_next)
if (name_match (MSYMBOL_SEARCH_NAME (iter), lookup_name, NULL))
- (*callback) (iter, user_data);
+ if (callback (iter))
+ return;
}
}
/* See minsyms.h. */
+void
+iterate_over_minimal_symbols (struct objfile *objf,
+ const lookup_name_info &lookup_name,
+ bool (*callback) (struct minimal_symbol *,
+ void *),
+ void *user_data)
+{
+ iterate_over_minimal_symbols (objf, lookup_name, [&] (minimal_symbol *iter)
+ {
+ return callback (iter, user_data);
+ });
+}
+
+/* See minsyms.h. */
+
struct bound_minimal_symbol
lookup_minimal_symbol_text (const char *name, struct objfile *objf)
{
diff --git a/gdb/minsyms.h b/gdb/minsyms.h
index 11a202025d3..6bd34a58058 100644
--- a/gdb/minsyms.h
+++ b/gdb/minsyms.h
@@ -20,6 +20,8 @@
#ifndef MINSYMS_H
#define MINSYMS_H
+#include "common/function-view.h"
+
struct type;
/* Several lookup functions return both a minimal symbol and the
@@ -266,14 +268,21 @@ struct bound_minimal_symbol lookup_minimal_symbol_by_pc (CORE_ADDR);
should that need to be done.
For each matching symbol, CALLBACK is called with the symbol and
- USER_DATA as arguments. */
+ USER_DATA as arguments. If CALLBACK returns true, the search
+ stops. */
void iterate_over_minimal_symbols (struct objfile *objf,
const lookup_name_info &name,
- void (*callback) (struct minimal_symbol *,
+ bool (*callback) (struct minimal_symbol *,
void *),
void *user_data);
+/* Same, but use a gdb::function_view as callback type. */
+void iterate_over_minimal_symbols
+ (struct objfile *objf,
+ const lookup_name_info &name,
+ gdb::function_view<bool (minimal_symbol *)> callback);
+
/* Compute the upper bound of MINSYM. The upper bound is the last
address thought to be part of the symbol. If the symbol has a
size, it is used. Otherwise use the lesser of the next minimal
diff --git a/gdb/symtab.c b/gdb/symtab.c
index 2b1f9558abe..9d9d447776e 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -4945,6 +4945,38 @@ symbol_is_function_or_method (minimal_symbol *msymbol)
}
}
+/* See symtab.h. */
+
+bound_minimal_symbol
+find_gnu_ifunc (const symbol *sym)
+{
+ if (SYMBOL_CLASS (sym) != LOC_BLOCK)
+ return {};
+
+ lookup_name_info lookup_name (SYMBOL_SEARCH_NAME (sym),
+ symbol_name_match_type::SEARCH_NAME);
+ struct objfile *objfile = symbol_objfile (sym);
+
+ CORE_ADDR address = BLOCK_START (SYMBOL_BLOCK_VALUE (sym));
+ minimal_symbol *ifunc = NULL;
+
+ iterate_over_minimal_symbols (objfile, lookup_name,
+ [&] (minimal_symbol *minsym)
+ {
+ if (MSYMBOL_TYPE (minsym) == mst_text_gnu_ifunc
+ && MSYMBOL_VALUE_ADDRESS (objfile, minsym) == address)
+ {
+ ifunc = minsym;
+ return true;
+ }
+ return false;
+ });
+
+ if (ifunc != NULL)
+ return {ifunc, objfile};
+ return {};
+}
+
/* Add matching symbols from SYMTAB to the current completion list. */
static void
diff --git a/gdb/symtab.h b/gdb/symtab.h
index 22b52019ee3..ddf4cb2e310 100644
--- a/gdb/symtab.h
+++ b/gdb/symtab.h
@@ -1681,6 +1681,9 @@ extern int find_pc_partial_function (CORE_ADDR, const char **, CORE_ADDR *,
extern struct type *find_gnu_ifunc_target_type (CORE_ADDR resolver_funaddr);
+/* Find the GNU ifunc minimal symbol that matches SYM. */
+extern bound_minimal_symbol find_gnu_ifunc (const symbol *sym);
+
extern void clear_pc_function_cache (void);
/* Expand symtab containing PC, SECTION if not already expanded. */
--
2.14.3
^ permalink raw reply [flat|nested] 33+ messages in thread* [PATCH v2 09/15] Factor out minsym_found/find_function_start_sal overload
2018-03-25 19:19 [PATCH v2 00/15] Fixing GNU ifunc support Pedro Alves
` (9 preceding siblings ...)
2018-03-25 19:25 ` [PATCH v2 04/15] Calling ifunc functions when resolver has debug info, user symbol same name Pedro Alves
@ 2018-03-25 19:25 ` Pedro Alves
2018-03-25 19:28 ` [PATCH v2 14/15] Extend GNU ifunc testcases Pedro Alves
` (4 subsequent siblings)
15 siblings, 0 replies; 33+ messages in thread
From: Pedro Alves @ 2018-03-25 19:25 UTC (permalink / raw)
To: gdb-patches
I need to make the ifunc resolving code in elfread.c skip the target
function's prologue like minsym_found does. I thought of factoring
that out to a separate function, but turns out there's already a
comment in find_function_start_sal that says that should agree with
minsym_found...
Instead of making sure the code agrees with a comment, factor out the
common code to a separate function and use it from both places.
Note that the current find_function_start_sal does a bit more than
minsym_found's equivalent (the "We always should ..." bit), though
that's probably a latent bug.
gdb/ChangeLog:
yyyy-mm-dd Pedro Alves <palves@redhat.com>
* linespec.c (minsym_found): Use find_function_start_sal CORE_ADDR
overload.
* symtab.c (find_function_start_sal(CORE_ADDR, obj_section *,bool)):
New, factored out from ...
(find_function_start_sal(symbol *, int)): ... this. Reimplement
and use bool.
* symtab.h (find_function_start_sal(CORE_ADDR, obj_section *,bool)):
New.
(find_function_start_sal(symbol *, int)): Change boolean parameter
type to bool.
---
gdb/linespec.c | 20 +-------------------
gdb/symtab.c | 45 +++++++++++++++++++++++++--------------------
gdb/symtab.h | 13 +++++++++++--
3 files changed, 37 insertions(+), 41 deletions(-)
diff --git a/gdb/linespec.c b/gdb/linespec.c
index 70f23187e02..74ddc9fe71d 100644
--- a/gdb/linespec.c
+++ b/gdb/linespec.c
@@ -4382,25 +4382,7 @@ minsym_found (struct linespec_state *self, struct objfile *objfile,
symtab_and_line sal;
if (is_function && want_start_sal)
- {
- sal = find_pc_sect_line (func_addr, NULL, 0);
-
- if (self->funfirstline)
- {
- if (sal.symtab != NULL
- && (COMPUNIT_LOCATIONS_VALID (SYMTAB_COMPUNIT (sal.symtab))
- || SYMTAB_LANGUAGE (sal.symtab) == language_asm))
- {
- struct gdbarch *gdbarch = get_objfile_arch (objfile);
-
- sal.pc = func_addr;
- if (gdbarch_skip_entrypoint_p (gdbarch))
- sal.pc = gdbarch_skip_entrypoint (gdbarch, sal.pc);
- }
- else
- skip_prologue_sal (&sal);
- }
- }
+ sal = find_function_start_sal (func_addr, NULL, self->funfirstline);
else
{
sal.objfile = objfile;
diff --git a/gdb/symtab.c b/gdb/symtab.c
index 9d9d447776e..8b6d26cd843 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -3575,46 +3575,36 @@ find_pc_line_pc_range (CORE_ADDR pc, CORE_ADDR *startptr, CORE_ADDR *endptr)
return sal.symtab != 0;
}
-/* Given a function symbol SYM, find the symtab and line for the start
- of the function.
- If the argument FUNFIRSTLINE is nonzero, we want the first line
- of real code inside the function.
- This function should return SALs matching those from minsym_found,
- otherwise false multiple-locations breakpoints could be placed. */
+/* See symtab.h. */
-struct symtab_and_line
-find_function_start_sal (struct symbol *sym, int funfirstline)
+symtab_and_line
+find_function_start_sal (CORE_ADDR func_addr, obj_section *section,
+ bool funfirstline)
{
- fixup_symbol_section (sym, NULL);
-
- obj_section *section = SYMBOL_OBJ_SECTION (symbol_objfile (sym), sym);
- symtab_and_line sal
- = find_pc_sect_line (BLOCK_START (SYMBOL_BLOCK_VALUE (sym)), section, 0);
- sal.symbol = sym;
+ symtab_and_line sal = find_pc_sect_line (func_addr, section, 0);
if (funfirstline && sal.symtab != NULL
&& (COMPUNIT_LOCATIONS_VALID (SYMTAB_COMPUNIT (sal.symtab))
|| SYMTAB_LANGUAGE (sal.symtab) == language_asm))
{
- struct gdbarch *gdbarch = symbol_arch (sym);
+ struct gdbarch *gdbarch = get_objfile_arch (SYMTAB_OBJFILE (sal.symtab));
- sal.pc = BLOCK_START (SYMBOL_BLOCK_VALUE (sym));
+ sal.pc = func_addr;
if (gdbarch_skip_entrypoint_p (gdbarch))
sal.pc = gdbarch_skip_entrypoint (gdbarch, sal.pc);
return sal;
}
/* We always should have a line for the function start address.
- If we don't, something is odd. Create a plain SAL refering
+ If we don't, something is odd. Create a plain SAL referring
just the PC and hope that skip_prologue_sal (if requested)
can find a line number for after the prologue. */
- if (sal.pc < BLOCK_START (SYMBOL_BLOCK_VALUE (sym)))
+ if (sal.pc < func_addr)
{
sal = {};
sal.pspace = current_program_space;
- sal.pc = BLOCK_START (SYMBOL_BLOCK_VALUE (sym));
+ sal.pc = func_addr;
sal.section = section;
- sal.symbol = sym;
}
if (funfirstline)
@@ -3623,6 +3613,21 @@ find_function_start_sal (struct symbol *sym, int funfirstline)
return sal;
}
+/* See symtab.h. */
+
+symtab_and_line
+find_function_start_sal (symbol *sym, bool funfirstline)
+{
+ fixup_symbol_section (sym, NULL);
+ symtab_and_line sal
+ = find_function_start_sal (BLOCK_START (SYMBOL_BLOCK_VALUE (sym)),
+ SYMBOL_OBJ_SECTION (symbol_objfile (sym), sym),
+ funfirstline);
+ sal.symbol = sym;
+ return sal;
+}
+
+
/* Given a function start address FUNC_ADDR and SYMTAB, find the first
address for that function that has an entry in SYMTAB's line info
table. If such an entry cannot be found, return FUNC_ADDR
diff --git a/gdb/symtab.h b/gdb/symtab.h
index 20808777467..7c56c5319c0 100644
--- a/gdb/symtab.h
+++ b/gdb/symtab.h
@@ -1894,8 +1894,17 @@ int matching_obj_sections (struct obj_section *, struct obj_section *);
extern struct symtab *find_line_symtab (struct symtab *, int, int *, int *);
-extern struct symtab_and_line find_function_start_sal (struct symbol *sym,
- int);
+/* Given a function symbol SYM, find the symtab and line for the start
+ of the function. If FUNFIRSTLINE is true, we want the first line
+ of real code inside the function. */
+extern symtab_and_line find_function_start_sal (symbol *sym, bool
+ funfirstline);
+
+/* Same, but start with a function address/section instead of a
+ symbol. */
+extern symtab_and_line find_function_start_sal (CORE_ADDR func_addr,
+ obj_section *section,
+ bool funfirstline);
extern void skip_prologue_sal (struct symtab_and_line *);
--
2.14.3
^ permalink raw reply [flat|nested] 33+ messages in thread* [PATCH v2 14/15] Extend GNU ifunc testcases
2018-03-25 19:19 [PATCH v2 00/15] Fixing GNU ifunc support Pedro Alves
` (10 preceding siblings ...)
2018-03-25 19:25 ` [PATCH v2 09/15] Factor out minsym_found/find_function_start_sal overload Pedro Alves
@ 2018-03-25 19:28 ` Pedro Alves
2018-03-25 19:29 ` [PATCH v2 06/15] Fix setting breakpoints on ifunc functions after they're already resolved Pedro Alves
` (3 subsequent siblings)
15 siblings, 0 replies; 33+ messages in thread
From: Pedro Alves @ 2018-03-25 19:28 UTC (permalink / raw)
To: gdb-patches
In v2:
- Renamed "resvd" -> "final", as I was constantly confusing myself.
- Tweaked tests for PPC64.
This patch extends/rewrites the gdb.base/gnu-ifunc.exp testcase to
cover the many different fixes in earlier patches. (This was actually
what encovered most of the problems.)
The current testcase uses an ifunc symbol with the same name as the
ifunc resolver symbol and makes sure to compile the ifunc resolver
without debug info. That does not model how ifuncs are implemented in
gcc/ifunc nowadays. Instead, what we have is that the glibc ifunc
resolvers nowadays are written in C and end up with debug info.
Also, in some cases the ifunc target is written in assembly, but in
other cases it's written in C. In the case of target function written
in C, if the target function has debug info, when we set a break on
the ifunc, we want to set it past the prologue of the target function.
Currently GDB gets that wrong.
To make sure we cover all the different scenarios, the testcase is
tweaked to cover all the different combinations of
- An ifunc resolver with the same name as the user-visible symbol vs
an ifunc resolver with a different name as the user-visible symbol.
- ifunc resolver compiled with and without debug info.
- ifunc target function compiled with and without debug info.
The testcase currently sets breakpoints on ifuncs, calls ifunc
functions, steps into ifunc functions, etc. After this series, this
all works and the testcase passes cleanly.
While working on this, I noticed that "b gnu_ifunc" before and after
the inferior resolved the ifunc would end up with a breakpoint with
different locations. That's now covered by new tests inside the new
"set-break" procedure.
It also tests other things like making sure we can't call an ifunc
without a return-type case if we don't know the type of the target.
And making sure that we pass enough arguments when we do know the
type.
gdb/testsuite/ChangeLog:
yyyy-mm-dd Pedro Alves <palves@redhat.com>
* gdb.base/gnu-ifunc-final.c: New file.
* gdb.base/gnu-ifunc.c (final): Delete, moved to gnu-ifunc-final.c.
* gdb.base/gnu-ifunc.exp (executable): Delete.
(staticexecutable): Adjust.
(lib_opts, exec_opts): Delete.
(make_binsuffix, build, set-break): New procedures.
(misc_tests): New, with tests factored out from the top level.
(top level): Test different combinations of ifunc resolver name,
resolver with and with debug info, and ifunc target with and
without debug info. Wrap static tests with with_target_prefix.
---
gdb/testsuite/gdb.base/gnu-ifunc-final.c | 22 ++
gdb/testsuite/gdb.base/gnu-ifunc-lib.c | 12 +-
gdb/testsuite/gdb.base/gnu-ifunc.c | 6 -
gdb/testsuite/gdb.base/gnu-ifunc.exp | 408 ++++++++++++++++++++++++-------
4 files changed, 353 insertions(+), 95 deletions(-)
create mode 100644 gdb/testsuite/gdb.base/gnu-ifunc-final.c
diff --git a/gdb/testsuite/gdb.base/gnu-ifunc-final.c b/gdb/testsuite/gdb.base/gnu-ifunc-final.c
new file mode 100644
index 00000000000..ef19fc98fab
--- /dev/null
+++ b/gdb/testsuite/gdb.base/gnu-ifunc-final.c
@@ -0,0 +1,22 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+ Copyright 2009-2018 Free Software Foundation, Inc.
+
+ This program is free software; you can redistribute it and/or modify
+ it under the terms of the GNU General Public License as published by
+ the Free Software Foundation; either version 3 of the License, or
+ (at your option) any later version.
+
+ This program is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ GNU General Public License for more details.
+
+ You should have received a copy of the GNU General Public License
+ along with this program. If not, see <http://www.gnu.org/licenses/>. */
+
+int
+final (int arg)
+{
+ return arg + 1;
+}
diff --git a/gdb/testsuite/gdb.base/gnu-ifunc-lib.c b/gdb/testsuite/gdb.base/gnu-ifunc-lib.c
index b9d446c92fa..7aac81faec6 100644
--- a/gdb/testsuite/gdb.base/gnu-ifunc-lib.c
+++ b/gdb/testsuite/gdb.base/gnu-ifunc-lib.c
@@ -22,10 +22,14 @@ extern int final (int arg);
typedef int (*final_t) (int arg);
+#ifndef IFUNC_RESOLVER_ATTR
asm (".type gnu_ifunc, %gnu_indirect_function");
-
final_t
gnu_ifunc (unsigned long hwcap)
+#else
+final_t
+gnu_ifunc_resolver (unsigned long hwcap)
+#endif
{
resolver_hwcap = hwcap;
if (! gnu_ifunc_initialized)
@@ -33,3 +37,9 @@ gnu_ifunc (unsigned long hwcap)
else
return final;
}
+
+#ifdef IFUNC_RESOLVER_ATTR
+extern int gnu_ifunc (int arg);
+
+__typeof (gnu_ifunc) gnu_ifunc __attribute__ ((ifunc ("gnu_ifunc_resolver")));
+#endif
diff --git a/gdb/testsuite/gdb.base/gnu-ifunc.c b/gdb/testsuite/gdb.base/gnu-ifunc.c
index 78fd30d9fd2..a4abacaccee 100644
--- a/gdb/testsuite/gdb.base/gnu-ifunc.c
+++ b/gdb/testsuite/gdb.base/gnu-ifunc.c
@@ -23,12 +23,6 @@ init_stub (int arg)
return 0;
}
-int
-final (int arg)
-{
- return arg + 1;
-}
-
/* Make differentiation of how the gnu_ifunc call resolves before and after
calling gnu_ifunc_pre. This ensures the resolved function address is not
being cached anywhere for the debugging purposes. */
diff --git a/gdb/testsuite/gdb.base/gnu-ifunc.exp b/gdb/testsuite/gdb.base/gnu-ifunc.exp
index 1d0d0401c37..827ac1202d2 100644
--- a/gdb/testsuite/gdb.base/gnu-ifunc.exp
+++ b/gdb/testsuite/gdb.base/gnu-ifunc.exp
@@ -18,139 +18,371 @@ if {[skip_shlib_tests]} {
}
standard_testfile .c
-set executable ${testfile}
-set staticexecutable ${executable}-static
+set staticexecutable ${testfile}-static
set staticbinfile [standard_output_file ${staticexecutable}]
set libfile "${testfile}-lib"
set libsrc ${libfile}.c
-set lib_so [standard_output_file ${libfile}.so]
-# $lib_o must not have {debug}, it would override the STT_GNU_IFUNC ELF markers.
-set lib_o [standard_output_file ${libfile}.o]
-# We need DWARF for the "final" function as we "step" into the function and GDB
-# would step-over the "final" function if there would be no line number debug
-# information (DWARF) available.
-#
-# We must not have DWARF for the "gnu_ifunc" function as DWARF has no way to
-# express the STT_GNU_IFUNC type and it would be considered as a regular
-# function due to DWARF by GDB. In ELF gnu-ifunc is expressed by the
-# STT_GNU_IFUNC type.
-#
-# Both functions need to be in the same shared library file but
-# gdb_compile_shlib has no way to specify source-specific compilation options.
-#
-# Therefore $libfile contains only the STT_GNU_IFUNC function with no DWARF
-# referencing all the other parts from the main executable with DWARF.
-
-set lib_opts {}
-set exec_opts [list debug shlib=$lib_so]
+set final_file "${testfile}-final"
+set final_src ${final_file}.c
if [get_compiler_info] {
return -1
}
-if { [gdb_compile_shlib ${srcdir}/${subdir}/$libsrc $lib_so $lib_opts] != ""
- || [gdb_compile ${srcdir}/${subdir}/$srcfile $binfile executable $exec_opts] != ""} {
- untested "failed to compile first testcase"
- return -1
+# Return the binary suffix appended to program and library names to
+# make each testcase variant unique.
+proc make_binsuffix {resolver_attr resolver_debug final_debug} {
+ return "$resolver_attr-$resolver_debug-$final_debug"
}
-# Start with a fresh gdb.
+# Compile the testcase. RESOLVER_ATTR is true if we're testing with
+# an ifunc resolver that has a different name from the user symbol,
+# specified with GCC's __attribute__ ifunc. RESOLVER_DEBUG is true
+# iff the resolver was compiled with debug info. FINAL_DEBUG is true
+# iff the target function was compiled with debug info.
+proc build {resolver_attr resolver_debug final_debug} {
+ global srcdir subdir srcfile binfile
+ global libsrc lib_so libfile
+ global exec_opts executable
+ global hex gdb_prompt
+ global final_file final_src
+
+ set suffix [make_binsuffix $resolver_attr $resolver_debug $final_debug]
+
+ set lib_so [standard_output_file ${libfile}-$suffix.so]
+ # $lib_o must not have {debug}, it would override the STT_GNU_IFUNC ELF markers.
+ set lib_o [standard_output_file ${libfile}-$suffix.o]
+
+ set exec_opts [list debug shlib=$lib_so]
+
+ set lib_opts {}
+ set final_opts {}
+
+ if {$resolver_attr} {
+ lappend lib_opts "additional_flags=-DIFUNC_RESOLVER_ATTR"
+ }
+
+ if {$resolver_debug} {
+ lappend lib_opts "debug"
+ }
+
+ if {$final_debug} {
+ lappend final_opts "debug"
+ }
-clean_restart $executable
-gdb_load_shlib ${lib_so}
+ set final_o $final_file-$suffix.o
+
+ if { [gdb_compile_shlib ${srcdir}/${subdir}/$libsrc \
+ $lib_so $lib_opts] != ""
+ || [gdb_compile ${srcdir}/${subdir}/$final_src \
+ $final_o object $final_opts] != ""
+ || [gdb_compile [list ${srcdir}/${subdir}/$srcfile $final_o] \
+ $binfile-$suffix executable $exec_opts] != ""} {
+ untested "failed to compile testcase"
+ return 0
+ }
-if ![runto_main] then {
- fail "can't run to main"
return 1
}
-# The "if" condition is artifical to test regression of a former patch.
-gdb_breakpoint "[gdb_get_line_number "break-at-nextcall"] if i && gnu_ifunc (i) != 42"
+# Test setting a breakpoint on a ifunc function before and after the
+# ifunc is resolved. For the description of RESOLVER_ATTR,
+# RESOLVER_DEBUG and FINAL_DEBUG, see the "build" procedure above.
+proc_with_prefix set-break {resolver_attr resolver_debug final_debug} {
+ global binfile libfile lib_so
+ global hex decimal
+ global gdb_prompt
+
+ set suffix [make_binsuffix $resolver_attr $resolver_debug $final_debug]
+
+ set lib_so [standard_output_file ${libfile}-$suffix.so]
+ clean_restart $binfile-$suffix
+ gdb_load_shlib ${lib_so}
+
+ if ![runto_main] then {
+ fail "can't run to main"
+ return 1
+ }
+
+ set ws "\[ \t\]+"
+ set dot "\\.?"
-gdb_breakpoint [gdb_get_line_number "break-at-call"]
-gdb_continue_to_breakpoint "break-at-call" ".*break-at-call.*"
+ if {$resolver_attr} {
+ set gnu_ifunc_resolver "gnu_ifunc_resolver"
+ } else {
+ set gnu_ifunc_resolver "gnu_ifunc"
+ }
-# Test GDB will automatically indirect the call.
+ if {!$resolver_debug} {
+ set gnu_ifunc_resolver "${dot}${gnu_ifunc_resolver}"
+ }
-gdb_test "p gnu_ifunc (3)" " = 4"
+ if {!$final_debug} {
+ set final "${dot}final"
+ } else {
+ set final "final"
+ }
-# Test that the resolver received its argument.
+ with_test_prefix "before resolving" {
+ delete_breakpoints
+ gdb_test "break gnu_ifunc" \
+ "Breakpoint $decimal at gnu-indirect-function resolver at $hex"
+ gdb_test "info breakpoints" \
+ "$decimal${ws}STT_GNU_IFUNC resolver${ws}keep${ws}y${ws}$hex <${gnu_ifunc_resolver}>"
+ }
-set actual_hwcap "0x0"
-set test "info auxv"
-gdb_test_multiple $test $test {
- -re "\r\n\\d+\\s+AT_HWCAP\[^\r\n\]+($hex)\r\n.*$gdb_prompt $" {
- set actual_hwcap $expect_out(1,string)
+ global final_src
+
+ with_test_prefix "resolve" {
+ delete_breakpoints
+ gdb_breakpoint [gdb_get_line_number "break-at-exit"]
+ gdb_continue_to_breakpoint "break-at-exit" ".*break-at-exit.*"
}
- -re ".*$gdb_prompt $" {
- pass "$test (no HWCAP)"
+
+ with_test_prefix "after resolving" {
+ delete_breakpoints
+
+ if {!$final_debug} {
+ # Set a breakpoint both at the ifunc, and at the ifunc's
+ # target. GDB should resolve both to the same address.
+ # Start with the ifunc's target.
+ set addr "-"
+ set test "break final"
+ # Extract the address without the leading "0x", because
+ # addresses in "info break" output include leading 0s
+ # (like "0x0000ADDR").
+ set hex_number {[0-9a-fA-F][0-9a-fA-F]*}
+ gdb_test_multiple $test $test {
+ -re "Breakpoint .* at 0x($hex_number)\r\n$gdb_prompt $" {
+ set addr $expect_out(1,string)
+ pass $test
+ }
+ }
+
+ # Now set a break at the ifunc.
+ gdb_test "break gnu_ifunc" "Breakpoint .* at 0x$addr"
+ set location "$decimal${ws}breakpoint${ws}keep${ws}y${ws}0x0*$addr${ws}<${final}\\+.*>"
+ } else {
+ set lineno -1
+ set test "break final"
+ gdb_test_multiple $test $test {
+ -re "Breakpoint .* at $hex: file .*$final_src, line ($decimal)\\.\r\n$gdb_prompt $" {
+ set lineno $expect_out(1,string)
+ pass $test
+ }
+ }
+ gdb_test "break gnu_ifunc" "Breakpoint .* at $hex: file .*$final_src, line $lineno\\."
+ set location "$decimal${ws}breakpoint${ws}keep${ws}y${ws}$hex in final at .*$final_src:$lineno"
+ }
+ gdb_test "info breakpoints" "$location\r\n$location"
}
}
-gdb_test "p/x resolver_hwcap" "= $actual_hwcap" "resolver received HWCAP"
+# Misc GNU ifunc tests. For the description of RESOLVER_ATTR,
+# RESOLVER_DEBUG and FINAL_DEBUG, see the "build" procedure above.
+proc misc_tests {resolver_attr resolver_debug final_debug} {
+ global srcdir subdir srcfile binfile
+ global libsrc lib_so libfile
+ global exec_opts executable
+ global hex gdb_prompt
+ global final_file final_src
+
+ set suffix [make_binsuffix $resolver_attr $resolver_debug $final_debug]
+
+ if {$resolver_attr} {
+ set gnu_ifunc_resolver "gnu_ifunc_resolver"
+ } else {
+ set gnu_ifunc_resolver "gnu_ifunc"
+ }
+
+ set dot "\\.?"
-# Test GDB will skip the gnu_ifunc resolver on first call.
+ if {!$resolver_debug} {
+ set gnu_ifunc_resolver "${dot}${gnu_ifunc_resolver}"
+ }
-gdb_test "step" "\r\nfinal .*"
+ if {!$final_debug} {
+ set final "${dot}final"
+ } else {
+ set final "final"
+ }
-# Test GDB will not break before the final chosen implementation.
+ # Start with a fresh gdb.
-# Also test a former patch regression:
-# Continuing.
-# Error in testing breakpoint condition:
-# Attempt to take address of value not located in memory.
-#
-# Breakpoint 2, main () at ./gdb.base/gnu-ifunc.c:33
+ clean_restart $binfile-$suffix
+ gdb_load_shlib ${lib_so}
-gdb_test "continue" "Continuing.\r\n\r\nBreakpoint .* (at|in) .*break-at-nextcall.*" \
- "continue to break-at-nextcall"
+ if ![runto_main] then {
+ fail "can't run to main"
+ return 1
+ }
-gdb_breakpoint "gnu_ifunc"
+ # The "if" condition is artifical to test regression of a former patch.
+ gdb_breakpoint "[gdb_get_line_number "break-at-nextcall"] if i && (int) gnu_ifunc (i) != 42"
-gdb_continue_to_breakpoint "nextcall gnu_ifunc"
+ gdb_breakpoint [gdb_get_line_number "break-at-call"]
+ gdb_continue_to_breakpoint "break-at-call" ".*break-at-call.*"
-gdb_test "frame" "#0 +(0x\[0-9a-f\]+ in +)?final \\(.*" "nextcall gnu_ifunc skipped"
+ # Test GDB will automatically indirect the call.
+ if {!$resolver_debug && !$final_debug} {
+ gdb_test "p gnu_ifunc()" \
+ "'${dot}final' has unknown return type; cast the call to its declared return type"
+ gdb_test "p gnu_ifunc (3)" \
+ "'${dot}final' has unknown return type; cast the call to its declared return type"
+ gdb_test "p (int) gnu_ifunc (3)" " = 4"
+ } else {
+ gdb_test "p gnu_ifunc()" "Too few arguments in function call\\."
+ gdb_test "p gnu_ifunc (3)" " = 4"
+ }
-# Check any commands not doing an inferior call access the address of the
-# STT_GNU_IFUNC resolver, not the target function.
+ # Test that the resolver received its argument.
+
+ set actual_hwcap "0x0"
+ set test "info auxv"
+ gdb_test_multiple $test $test {
+ -re "\r\n\\d+\\s+AT_HWCAP\[^\r\n\]+($hex)\r\n.*$gdb_prompt $" {
+ set actual_hwcap $expect_out(1,string)
+ }
+ -re ".*$gdb_prompt $" {
+ pass "$test (no HWCAP)"
+ }
+ }
-if {[istarget powerpc64-*] && [is_lp64_target]} {
- # With only minimal symbols GDB provides the function descriptors. With
- # full debug info the function code would be displayed.
- set func_prefix {\.}
-} else {
- set func_prefix {}
-}
+ gdb_test "p/x resolver_hwcap" "= $actual_hwcap" "resolver received HWCAP"
-gdb_test "p gnu_ifunc" " = {<text gnu-indirect-function variable, no debug info>} 0x\[0-9a-f\]+ <${func_prefix}gnu_ifunc>" "p gnu_ifunc executing"
-gdb_test "info sym gnu_ifunc" "gnu_ifunc in section .*" "info sym gnu_ifunc executing"
+ # Test GDB will skip the gnu_ifunc resolver on first call.
-set test "info addr gnu_ifunc"
-gdb_test_multiple $test $test {
- -re "Symbol \"gnu_ifunc\" is at (0x\[0-9a-f\]+) in .*$gdb_prompt $" {
- pass $test
+ # Even if the resolver has debug info, stepping into an ifunc call
+ # should skip the resolver.
+ if {!$final_debug} {
+ # Make GDB stop stepping even if it steps into a function with
+ # no debug info.
+ gdb_test_no_output "set step-mode on"
+ gdb_test "step" "$hex in ${dot}final \\\(\\\)"
+ } else {
+ gdb_test "step" "\r\nfinal .*"
+ }
+
+ # Test GDB will not break before the final chosen implementation.
+
+ # Also test a former patch regression:
+ # Continuing.
+ # Error in testing breakpoint condition:
+ # Attempt to take address of value not located in memory.
+ #
+ # Breakpoint 2, main () at ./gdb.base/gnu-ifunc.c:33
+
+ gdb_test "continue" \
+ "Continuing.\r\n\r\nBreakpoint .* (at|in) .*break-at-nextcall.*" \
+ "continue to break-at-nextcall"
+
+ gdb_breakpoint "gnu_ifunc"
+
+ gdb_continue_to_breakpoint "nextcall gnu_ifunc"
+
+ gdb_test "frame" \
+ "#0 +(0x\[0-9a-f\]+ in +)?${final} \\(.*" "nextcall gnu_ifunc skipped"
+
+ # Check any commands not doing an inferior call access the address of the
+ # STT_GNU_IFUNC resolver, not the target function.
+
+ if {[istarget powerpc64-*] && [is_lp64_target]} {
+ # With only minimal symbols GDB provides the function descriptors. With
+ # full debug info the function code would be displayed.
+ }
+
+ gdb_test "p gnu_ifunc" \
+ " = {<text gnu-indirect-function variable, no debug info>} 0x\[0-9a-f\]+ <${gnu_ifunc_resolver}>" \
+ "p gnu_ifunc executing"
+ gdb_test "info sym gnu_ifunc" \
+ "${gnu_ifunc_resolver} in section .*" \
+ "info sym gnu_ifunc executing"
+
+ set test "info addr gnu_ifunc"
+ if {!$resolver_attr && $resolver_debug} {
+ gdb_test_multiple $test $test {
+ -re "Symbol \"gnu_ifunc\" is a function at address (0x\[0-9a-f\]+).*$gdb_prompt $" {
+ pass $test
+ }
+ }
+ } else {
+ gdb_test_multiple $test $test {
+ -re "Symbol \"gnu_ifunc\" is at (0x\[0-9a-f\]+) in .*$gdb_prompt $" {
+ pass $test
+ }
+ }
+ }
+ gdb_test "info sym $expect_out(1,string)" \
+ "${gnu_ifunc_resolver} in section .*" \
+ "info sym <gnu_ifunc-address>"
+
+ # Test calling the resolver directly instead of the ifunc symbol.
+ # Can only do that if the ifunc and the ifunc resolver have
+ # different names.
+ if {$resolver_attr} {
+ if {$resolver_debug} {
+ if {[istarget powerpc64-*] && [is_lp64_target]} {
+ gdb_test "p gnu_ifunc_resolver(0)" \
+ " = \\(int \\(\\*\\)\\(int\\)\\) @$hex: $hex <${final}>"
+ } else {
+ gdb_test "p gnu_ifunc_resolver(0)" \
+ " = \\(int \\(\\*\\)\\(int\\)\\) $hex <final>"
+ }
+ } else {
+ gdb_test "p gnu_ifunc_resolver(0)" \
+ "'${gnu_ifunc_resolver}' has unknown return type; cast the call to its declared return type"
+ gdb_test "p (void *) gnu_ifunc_resolver(0)" \
+ " = \\(void \\*\\) $hex <${final}>"
+ }
}
}
-gdb_test "info sym $expect_out(1,string)" "gnu_ifunc in section .*" "info sym <gnu_ifunc-address>"
+# Test all the combinations of:
+#
+# - An ifunc resolver with the same name as the ifunc symbol vs an
+# ifunc resolver with a different name as the ifunc symbol.
+#
+# - ifunc resolver compiled with and without debug info. This ensures
+# that GDB understands that a function not a regular function by
+# looking at the STT_GNU_IFUNC type in the elf symbols. DWARF has
+# no way to express the STT_GNU_IFUNC type.
+#
+# - ifunc target function (resolved) compiled with and without debug
+# info.
+foreach_with_prefix resolver_attr {0 1} {
+ foreach_with_prefix resolver_debug {0 1} {
+ foreach_with_prefix final_debug {0 1} {
+ build $resolver_attr $resolver_debug $final_debug
+ misc_tests $resolver_attr $resolver_debug $final_debug
+ set-break $resolver_attr $resolver_debug $final_debug
+ }
+ }
+}
# Test statically linked ifunc resolving during inferior start.
# https://bugzilla.redhat.com/show_bug.cgi?id=624967
-# Compile $staticbinfile separately as it may exit on error (ld/12595).
-
-if { [gdb_compile ${srcdir}/${subdir}/$libsrc $lib_o object {}] != ""
- || [gdb_compile "${srcdir}/${subdir}/$srcfile $lib_o" $staticbinfile executable {debug}] != "" } {
- untested "failed to compile second testcase"
- return -1
-}
+with_test_prefix "static" {
+ # Compile $staticbinfile separately as it may exit on error
+ # (ld/12595).
+
+ set lib_o [standard_output_file ${libfile}.o]
+ set final_o [standard_output_file ${final_file}.o]
+ if { [gdb_compile ${srcdir}/${subdir}/$libsrc $lib_o object {}] != ""
+ || [gdb_compile ${srcdir}/${subdir}/$final_src $final_o object {}] != ""
+ || [gdb_compile "${srcdir}/${subdir}/$srcfile $lib_o $final_o" \
+ $staticbinfile executable {debug}] != "" } {
+ untested "failed to compile second testcase"
+ return -1
+ }
-clean_restart $staticexecutable
+ clean_restart $staticexecutable
-gdb_breakpoint "gnu_ifunc"
-gdb_breakpoint "main"
-gdb_run_cmd
-gdb_test "" "Breakpoint \[0-9\]*, main .*" "static gnu_ifunc"
+ gdb_breakpoint "gnu_ifunc"
+ gdb_breakpoint "main"
+ gdb_run_cmd
+ gdb_test "" "Breakpoint \[0-9\]*, main .*" "static gnu_ifunc"
+}
--
2.14.3
^ permalink raw reply [flat|nested] 33+ messages in thread* [PATCH v2 06/15] Fix setting breakpoints on ifunc functions after they're already resolved
2018-03-25 19:19 [PATCH v2 00/15] Fixing GNU ifunc support Pedro Alves
` (11 preceding siblings ...)
2018-03-25 19:28 ` [PATCH v2 14/15] Extend GNU ifunc testcases Pedro Alves
@ 2018-03-25 19:29 ` Pedro Alves
2018-03-25 19:29 ` [PATCH v2 10/15] For PPC64: elf_gnu_ifunc_record_cache: handle plt symbols in .text section Pedro Alves
` (2 subsequent siblings)
15 siblings, 0 replies; 33+ messages in thread
From: Pedro Alves @ 2018-03-25 19:29 UTC (permalink / raw)
To: gdb-patches
This fixes setting breakpoints on ifunc functions by name after the
ifunc has already been resolved.
In that case, if you have debug info for the ifunc resolver, without
the fix, then gdb puts a breakpoint past the prologue of the resolver,
instead of setting a breakpoint at the ifunc target:
break gnu_ifunc
Breakpoint 4 at 0x7ffff7bd36f2: file src/gdb/testsuite/gdb.base/gnu-ifunc-lib.c, line 34.
(gdb) continue
Continuing.
[Inferior 1 (process 13300) exited normally]
(gdb)
above we should have stopped at "final", but didn't because we never
resolved the ifunc to the final location.
If you don't have debug info for the resolver, GDB manages to resolve
the ifunc target, but, it should be setting a breakpoint after the
prologue of the final function, and instead what you get is that GDB
sets a breakpoint on the first address of the target function. With
the gnu-ifunc.exp tests added by a later patch, we get, without the
fix:
(gdb) break gnu_ifunc
Breakpoint 4 at 0x400753
(gdb) continue
Continuing.
Breakpoint 4, final (arg=1) at src/gdb/testsuite/gdb.base/gnu-ifunc-final.c:20
20 {
vs, fixed:
(gdb) break gnu_ifunc
Breakpoint 4 at 0x40075a: file src/gdb/testsuite/gdb.base/gnu-ifunc-final.c, line 21.
(gdb) continue
Continuing.
Breakpoint 4, final (arg=2) at src/gdb/testsuite/gdb.base/gnu-ifunc-final.c:21
21 return arg + 1;
(gdb)
Fix the problems above by moving the ifunc target resolving to
linespec.c, before we skip a function's prologue. We need to save
something in the sal, so that set_breakpoint_location_function knows
that it needs to create a bp_gnu_ifunc_resolver bp_location. Might as
well just save a pointer to the minsym.
gdb/ChangeLog:
yyyy-mm-dd Pedro Alves <palves@redhat.com>
* breakpoint.c (set_breakpoint_location_function): Don't resolve
ifunc targets here. Instead, if we have an ifunc minsym, use its
address/name.
(add_location_to_breakpoint): Store the minsym and the objfile in
the breakpoint location.
* breakpoint.h (bp_location) <msymbol, objfile>: New fields.
* linespec.c (minsym_found): Resolve GNU ifunc targets here.
Record the minsym in the sal.
* symtab.h (symtab_and_line) <msymbol>: New field.
---
gdb/breakpoint.c | 30 ++++++++++++------------------
gdb/breakpoint.h | 8 ++++++++
gdb/linespec.c | 26 +++++++++++++++++++++++---
gdb/symtab.h | 1 +
4 files changed, 44 insertions(+), 21 deletions(-)
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 9de0e6330c5..6840a200183 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -7180,37 +7180,29 @@ set_breakpoint_location_function (struct bp_location *loc, int explicit_loc)
|| loc->owner->type == bp_hardware_breakpoint
|| is_tracepoint (loc->owner))
{
- int is_gnu_ifunc;
const char *function_name;
- CORE_ADDR func_addr;
- find_pc_partial_function_gnu_ifunc (loc->address, &function_name,
- &func_addr, NULL, &is_gnu_ifunc);
-
- if (is_gnu_ifunc && !explicit_loc)
+ if (loc->msymbol != NULL
+ && MSYMBOL_TYPE (loc->msymbol) == mst_text_gnu_ifunc
+ && !explicit_loc)
{
struct breakpoint *b = loc->owner;
- gdb_assert (loc->pspace == current_program_space);
- if (gnu_ifunc_resolve_name (function_name,
- &loc->requested_address))
- {
- /* Recalculate ADDRESS based on new REQUESTED_ADDRESS. */
- loc->address = adjust_breakpoint_address (loc->gdbarch,
- loc->requested_address,
- b->type);
- }
- else if (b->type == bp_breakpoint && b->loc == loc
- && loc->next == NULL && b->related_breakpoint == b)
+ function_name = MSYMBOL_LINKAGE_NAME (loc->msymbol);
+
+ if (b->type == bp_breakpoint && b->loc == loc
+ && loc->next == NULL && b->related_breakpoint == b)
{
/* Create only the whole new breakpoint of this type but do not
mess more complicated breakpoints with multiple locations. */
b->type = bp_gnu_ifunc_resolver;
/* Remember the resolver's address for use by the return
breakpoint. */
- loc->related_address = func_addr;
+ loc->related_address = loc->address;
}
}
+ else
+ find_pc_partial_function (loc->address, &function_name, NULL, NULL);
if (function_name)
loc->function_name = xstrdup (function_name);
@@ -8674,6 +8666,8 @@ add_location_to_breakpoint (struct breakpoint *b,
loc->line_number = sal->line;
loc->symtab = sal->symtab;
loc->symbol = sal->symbol;
+ loc->msymbol = sal->msymbol;
+ loc->objfile = sal->objfile;
set_breakpoint_location_function (loc,
sal->explicit_pc || sal->explicit_line);
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index d8e2b73edc9..692a553cef1 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -491,6 +491,14 @@ public:
ascertain when an event location was set at a different location than
the one originally selected by parsing, e.g., inlined symbols. */
const struct symbol *symbol = NULL;
+
+ /* Similarly, the minimal symbol found by the location parser, if
+ any. This may be used to ascertain if the location was
+ originally set on a GNU ifunc symbol. */
+ const minimal_symbol *msymbol = NULL;
+
+ /* The objfile the symbol or minimal symbol were found in. */
+ const struct objfile *objfile = NULL;
};
/* The possible return values for print_bpstat, print_it_normal,
diff --git a/gdb/linespec.c b/gdb/linespec.c
index 7ff8dcf3a5b..c549ba09349 100644
--- a/gdb/linespec.c
+++ b/gdb/linespec.c
@@ -4334,10 +4334,24 @@ minsym_found (struct linespec_state *self, struct objfile *objfile,
struct minimal_symbol *msymbol,
std::vector<symtab_and_line> *result)
{
- struct symtab_and_line sal;
+ bool want_start_sal;
CORE_ADDR func_addr;
- if (msymbol_is_function (objfile, msymbol, &func_addr))
+ bool is_function = msymbol_is_function (objfile, msymbol, &func_addr);
+
+ if (is_function)
+ {
+ const char *msym_name = MSYMBOL_LINKAGE_NAME (msymbol);
+
+ if (MSYMBOL_TYPE (msymbol) == mst_text_gnu_ifunc)
+ want_start_sal = gnu_ifunc_resolve_name (msym_name, &func_addr);
+ else
+ want_start_sal = true;
+ }
+
+ symtab_and_line sal;
+
+ if (is_function && want_start_sal)
{
sal = find_pc_sect_line (func_addr, NULL, 0);
@@ -4360,7 +4374,13 @@ minsym_found (struct linespec_state *self, struct objfile *objfile,
else
{
sal.objfile = objfile;
- sal.pc = MSYMBOL_VALUE_ADDRESS (objfile, msymbol);
+ sal.msymbol = msymbol;
+ /* Store func_addr, not the minsym's address in case this was an
+ ifunc that hasn't been resolved yet. */
+ if (is_function)
+ sal.pc = func_addr;
+ else
+ sal.pc = MSYMBOL_VALUE_ADDRESS (objfile, msymbol);
sal.pspace = current_program_space;
}
diff --git a/gdb/symtab.h b/gdb/symtab.h
index ddf4cb2e310..c6c643a93bf 100644
--- a/gdb/symtab.h
+++ b/gdb/symtab.h
@@ -1760,6 +1760,7 @@ struct symtab_and_line
struct symtab *symtab = NULL;
struct symbol *symbol = NULL;
struct obj_section *section = NULL;
+ struct minimal_symbol *msymbol = NULL;
/* Line number. Line numbers start at 1 and proceed through symtab->nlines.
0 is never a valid line number; it is used to indicate that line number
information is not available. */
--
2.14.3
^ permalink raw reply [flat|nested] 33+ messages in thread* [PATCH v2 10/15] For PPC64: elf_gnu_ifunc_record_cache: handle plt symbols in .text section
2018-03-25 19:19 [PATCH v2 00/15] Fixing GNU ifunc support Pedro Alves
` (12 preceding siblings ...)
2018-03-25 19:29 ` [PATCH v2 06/15] Fix setting breakpoints on ifunc functions after they're already resolved Pedro Alves
@ 2018-03-25 19:29 ` Pedro Alves
2018-03-25 19:29 ` [PATCH v2 13/15] PPC64: always make synthetic .text symbols for GNU ifunc symbols Pedro Alves
2018-04-26 12:23 ` [PATCH v2 00/15] Fixing GNU ifunc support Pedro Alves
15 siblings, 0 replies; 33+ messages in thread
From: Pedro Alves @ 2018-03-25 19:29 UTC (permalink / raw)
To: gdb-patches
elf_gnu_ifunc_record_cache doesn't ever record anything on PPC64
(tested on gcc110 on the compile farm, CentOS 7.4, ELFv1), because
that expects to find PLT symbols in the .plt section, while there we
get:
(gdb) info symbol 'gnu_ifunc@plt'
gnu_ifunc@plt in section .text
^^^^^
I guess that may be related to the comment in ppc-linux-tdep.c that
says "For secure PLT, stub is in .text".
In any case, this commit fixes the issue by making the function look
at the symbol name instead of at the section.
gdb/ChangeLog:
yyyy-mm-dd Pedro Alves <palves@redhat.com>
* elfread.c (elf_gnu_ifunc_record_cache): Check if the symbol name
ends in "@plt" instead of looking at the symbol's section.
---
gdb/elfread.c | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/gdb/elfread.c b/gdb/elfread.c
index 82ab3d891ce..4e79b080e65 100644
--- a/gdb/elfread.c
+++ b/gdb/elfread.c
@@ -687,7 +687,6 @@ static int
elf_gnu_ifunc_record_cache (const char *name, CORE_ADDR addr)
{
struct bound_minimal_symbol msym;
- asection *sect;
struct objfile *objfile;
htab_t htab;
struct elf_gnu_ifunc_cache entry_local, *entry_p;
@@ -698,14 +697,17 @@ elf_gnu_ifunc_record_cache (const char *name, CORE_ADDR addr)
return 0;
if (BMSYMBOL_VALUE_ADDRESS (msym) != addr)
return 0;
- /* minimal symbols have always SYMBOL_OBJ_SECTION non-NULL. */
- sect = MSYMBOL_OBJ_SECTION (msym.objfile, msym.minsym)->the_bfd_section;
objfile = msym.objfile;
/* If .plt jumps back to .plt the symbol is still deferred for later
- resolution and it has no use for GDB. Besides ".text" this symbol can
- reside also in ".opd" for ppc64 function descriptor. */
- if (strcmp (bfd_get_section_name (objfile->obfd, sect), ".plt") == 0)
+ resolution and it has no use for GDB. */
+ const char *target_name = MSYMBOL_LINKAGE_NAME (msym.minsym);
+ size_t len = strlen (target_name);
+
+ /* Note we check the symbol's name instead of checking whether the
+ symbol is in the .plt section because some systems have @plt
+ symbols in the .text section. */
+ if (len > 4 && strcmp (target_name + len - 4, "@plt") == 0)
return 0;
htab = (htab_t) objfile_data (objfile, elf_objfile_gnu_ifunc_cache_data);
--
2.14.3
^ permalink raw reply [flat|nested] 33+ messages in thread* [PATCH v2 13/15] PPC64: always make synthetic .text symbols for GNU ifunc symbols
2018-03-25 19:19 [PATCH v2 00/15] Fixing GNU ifunc support Pedro Alves
` (13 preceding siblings ...)
2018-03-25 19:29 ` [PATCH v2 10/15] For PPC64: elf_gnu_ifunc_record_cache: handle plt symbols in .text section Pedro Alves
@ 2018-03-25 19:29 ` Pedro Alves
2018-03-25 19:33 ` Pedro Alves
2018-03-26 7:54 ` Alan Modra
2018-04-26 12:23 ` [PATCH v2 00/15] Fixing GNU ifunc support Pedro Alves
15 siblings, 2 replies; 33+ messages in thread
From: Pedro Alves @ 2018-03-25 19:29 UTC (permalink / raw)
To: gdb-patches; +Cc: Binutils
If you create an ifunc using GCC's __attribute__ ifunc, like:
extern int gnu_ifunc (int arg);
static int gnu_ifunc_target (int arg) { return 0; }
__typeof (gnu_ifunc) *gnu_ifunc_resolver (unsigned long hwcap) { return gnu_ifunc_target; }
__typeof (gnu_ifunc) gnu_ifunc __attribute__ ((ifunc ("gnu_ifunc_resolver")));
then you end up with two (function descriptor) symbols, one for the
ifunc itself, and another for the resolver:
(...)
12: 0000000000020060 104 FUNC GLOBAL DEFAULT 18 gnu_ifunc_resolver
(...)
16: 0000000000020060 104 GNU_IFUNC GLOBAL DEFAULT 18 gnu_ifunc
(...)
Both ifunc and resolver symbols have the same address/value, so
ppc64_elf_get_synthetic_symtab only creates a synthetic text symbol
for one of them. In the case above, it ends up being created for the
resolver, only:
(gdb) maint print msymbols
(...)
[ 7] t 0x980 .frame_dummy section .text
[ 8] T 0x9e4 .gnu_ifunc_resolver section .text
[ 9] T 0xa58 __glink_PLTresolve section .text
(...)
GDB needs to know when a program stepped into an ifunc resolver, so
that it can know whether to step past the resolver into the target
function without the user noticing. The way GDB does it is my
checking whether the current PC points to an ifunc symbol (since
resolver and ifunc have the same address by design).
The problem is then that ppc64_elf_get_synthetic_symtab never creates
the synchetic symbol for the ifunc, so GDB stops stepping at the
resolver (in a test added by the following patch):
(gdb) step
gnu_ifunc_resolver (hwcap=21) at gdb/testsuite/gdb.base/gnu-ifunc-lib.c:33
33 {
(gdb) FAIL: gdb.base/gnu-ifunc.exp: resolver_attr=1: resolver_debug=1: final_debug=0: step
After this commit, we get:
[ 8] i 0x9e4 .gnu_ifunc section .text
[ 9] T 0x9e4 .gnu_ifunc_resolver section .text
And stepping an ifunc call takes to the final function:
(gdb) step
0x00000000100009e8 in .final ()
(gdb) PASS: gdb.base/gnu-ifunc.exp: resolver_attr=1: resolver_debug=1: final_debug=0: step
An alternative to touching bfd I considered was for GDB to check
whether there's an ifunc data symbol / function descriptor that points
to the current PC, whenever the program stops, but discarded it
because we'd have to do a linear scan over .opd over an over to find a
matching function descriptor for the current PC. At that point I
considered caching that info, but quickly dismissed it as then that
has no advantage (memory or performance) over just creating the
synthetic ifunc text symbol in the first place.
I ran the binutils and ld testsuites on PPC64 ELFv1 (machine gcc110 on
the GCC compile farm), and saw no regressions. This commit is part of
a GDB patch series that includes GDB tests that fail without this fix.
OK to apply?
bfd/ChangeLog:
yyyy-mm-dd Pedro Alves <palves@redhat.com>
* elf64-ppc.c (ppc64_elf_get_synthetic_symtab): Don't consider
ifunc and non-ifunc symbols duplicates.
---
bfd/elf64-ppc.c | 22 ++++++++++++++++------
1 file changed, 16 insertions(+), 6 deletions(-)
diff --git a/bfd/elf64-ppc.c b/bfd/elf64-ppc.c
index 751ad778b26..791ec49b73d 100644
--- a/bfd/elf64-ppc.c
+++ b/bfd/elf64-ppc.c
@@ -3330,13 +3330,23 @@ ppc64_elf_get_synthetic_symtab (bfd *abfd,
if (!relocatable && symcount > 1)
{
- /* Trim duplicate syms, since we may have merged the normal and
- dynamic symbols. Actually, we only care about syms that have
- different values, so trim any with the same value. */
+ /* Trim duplicate syms, since we may have merged the normal
+ and dynamic symbols. Actually, we only care about syms
+ that have different values, so trim any with the same
+ value. Don't consider ifunc and ifunc resolver symbols
+ duplicates however, because GDB wants to know whether a
+ text symbol is an ifunc resolver. */
for (i = 1, j = 1; i < symcount; ++i)
- if (syms[i - 1]->value + syms[i - 1]->section->vma
- != syms[i]->value + syms[i]->section->vma)
- syms[j++] = syms[i];
+ {
+ const asymbol *s0 = syms[i - 1];
+ const asymbol *s1 = syms[i];
+
+ if ((s0->value + s0->section->vma
+ != s1->value + s1->section->vma)
+ || ((s0->flags & BSF_GNU_INDIRECT_FUNCTION)
+ != (s1->flags & BSF_GNU_INDIRECT_FUNCTION)))
+ syms[j++] = syms[i];
+ }
symcount = j;
}
--
2.14.3
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH v2 13/15] PPC64: always make synthetic .text symbols for GNU ifunc symbols
2018-03-25 19:29 ` [PATCH v2 13/15] PPC64: always make synthetic .text symbols for GNU ifunc symbols Pedro Alves
@ 2018-03-25 19:33 ` Pedro Alves
2018-03-26 7:54 ` Alan Modra
1 sibling, 0 replies; 33+ messages in thread
From: Pedro Alves @ 2018-03-25 19:33 UTC (permalink / raw)
To: gdb-patches; +Cc: Binutils
Dear binutils friends,
This is a bfd patch that is part of a larger gdb patch series,
whose cover letter is found here:
https://sourceware.org/ml/gdb-patches/2018-03/msg00504.html
Thanks,
Pedro Alves
On 03/25/2018 08:19 PM, Pedro Alves wrote:
> If you create an ifunc using GCC's __attribute__ ifunc, like:
>
> extern int gnu_ifunc (int arg);
> static int gnu_ifunc_target (int arg) { return 0; }
> __typeof (gnu_ifunc) *gnu_ifunc_resolver (unsigned long hwcap) { return gnu_ifunc_target; }
> __typeof (gnu_ifunc) gnu_ifunc __attribute__ ((ifunc ("gnu_ifunc_resolver")));
>
> then you end up with two (function descriptor) symbols, one for the
> ifunc itself, and another for the resolver:
>
> (...)
> 12: 0000000000020060 104 FUNC GLOBAL DEFAULT 18 gnu_ifunc_resolver
> (...)
> 16: 0000000000020060 104 GNU_IFUNC GLOBAL DEFAULT 18 gnu_ifunc
> (...)
>
> Both ifunc and resolver symbols have the same address/value, so
> ppc64_elf_get_synthetic_symtab only creates a synthetic text symbol
> for one of them. In the case above, it ends up being created for the
> resolver, only:
>
> (gdb) maint print msymbols
> (...)
> [ 7] t 0x980 .frame_dummy section .text
> [ 8] T 0x9e4 .gnu_ifunc_resolver section .text
> [ 9] T 0xa58 __glink_PLTresolve section .text
> (...)
>
> GDB needs to know when a program stepped into an ifunc resolver, so
> that it can know whether to step past the resolver into the target
> function without the user noticing. The way GDB does it is my
> checking whether the current PC points to an ifunc symbol (since
> resolver and ifunc have the same address by design).
>
> The problem is then that ppc64_elf_get_synthetic_symtab never creates
> the synchetic symbol for the ifunc, so GDB stops stepping at the
> resolver (in a test added by the following patch):
>
> (gdb) step
> gnu_ifunc_resolver (hwcap=21) at gdb/testsuite/gdb.base/gnu-ifunc-lib.c:33
> 33 {
> (gdb) FAIL: gdb.base/gnu-ifunc.exp: resolver_attr=1: resolver_debug=1: final_debug=0: step
>
> After this commit, we get:
>
> [ 8] i 0x9e4 .gnu_ifunc section .text
> [ 9] T 0x9e4 .gnu_ifunc_resolver section .text
>
> And stepping an ifunc call takes to the final function:
> (gdb) step
> 0x00000000100009e8 in .final ()
> (gdb) PASS: gdb.base/gnu-ifunc.exp: resolver_attr=1: resolver_debug=1: final_debug=0: step
>
> An alternative to touching bfd I considered was for GDB to check
> whether there's an ifunc data symbol / function descriptor that points
> to the current PC, whenever the program stops, but discarded it
> because we'd have to do a linear scan over .opd over an over to find a
> matching function descriptor for the current PC. At that point I
> considered caching that info, but quickly dismissed it as then that
> has no advantage (memory or performance) over just creating the
> synthetic ifunc text symbol in the first place.
>
> I ran the binutils and ld testsuites on PPC64 ELFv1 (machine gcc110 on
> the GCC compile farm), and saw no regressions. This commit is part of
> a GDB patch series that includes GDB tests that fail without this fix.
>
> OK to apply?
>
> bfd/ChangeLog:
> yyyy-mm-dd Pedro Alves <palves@redhat.com>
>
> * elf64-ppc.c (ppc64_elf_get_synthetic_symtab): Don't consider
> ifunc and non-ifunc symbols duplicates.
> ---
> bfd/elf64-ppc.c | 22 ++++++++++++++++------
> 1 file changed, 16 insertions(+), 6 deletions(-)
>
> diff --git a/bfd/elf64-ppc.c b/bfd/elf64-ppc.c
> index 751ad778b26..791ec49b73d 100644
> --- a/bfd/elf64-ppc.c
> +++ b/bfd/elf64-ppc.c
> @@ -3330,13 +3330,23 @@ ppc64_elf_get_synthetic_symtab (bfd *abfd,
>
> if (!relocatable && symcount > 1)
> {
> - /* Trim duplicate syms, since we may have merged the normal and
> - dynamic symbols. Actually, we only care about syms that have
> - different values, so trim any with the same value. */
> + /* Trim duplicate syms, since we may have merged the normal
> + and dynamic symbols. Actually, we only care about syms
> + that have different values, so trim any with the same
> + value. Don't consider ifunc and ifunc resolver symbols
> + duplicates however, because GDB wants to know whether a
> + text symbol is an ifunc resolver. */
> for (i = 1, j = 1; i < symcount; ++i)
> - if (syms[i - 1]->value + syms[i - 1]->section->vma
> - != syms[i]->value + syms[i]->section->vma)
> - syms[j++] = syms[i];
> + {
> + const asymbol *s0 = syms[i - 1];
> + const asymbol *s1 = syms[i];
> +
> + if ((s0->value + s0->section->vma
> + != s1->value + s1->section->vma)
> + || ((s0->flags & BSF_GNU_INDIRECT_FUNCTION)
> + != (s1->flags & BSF_GNU_INDIRECT_FUNCTION)))
> + syms[j++] = syms[i];
> + }
> symcount = j;
> }
>
>
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH v2 13/15] PPC64: always make synthetic .text symbols for GNU ifunc symbols
2018-03-25 19:29 ` [PATCH v2 13/15] PPC64: always make synthetic .text symbols for GNU ifunc symbols Pedro Alves
2018-03-25 19:33 ` Pedro Alves
@ 2018-03-26 7:54 ` Alan Modra
1 sibling, 0 replies; 33+ messages in thread
From: Alan Modra @ 2018-03-26 7:54 UTC (permalink / raw)
To: Pedro Alves; +Cc: gdb-patches, Binutils
On Sun, Mar 25, 2018 at 08:19:41PM +0100, Pedro Alves wrote:
> * elf64-ppc.c (ppc64_elf_get_synthetic_symtab): Don't consider
> ifunc and non-ifunc symbols duplicates.
OK, thanks!
--
Alan Modra
Australia Development Lab, IBM
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 00/15] Fixing GNU ifunc support
2018-03-25 19:19 [PATCH v2 00/15] Fixing GNU ifunc support Pedro Alves
` (14 preceding siblings ...)
2018-03-25 19:29 ` [PATCH v2 13/15] PPC64: always make synthetic .text symbols for GNU ifunc symbols Pedro Alves
@ 2018-04-26 12:23 ` Pedro Alves
15 siblings, 0 replies; 33+ messages in thread
From: Pedro Alves @ 2018-04-26 12:23 UTC (permalink / raw)
To: GDB Patches
Hello all,
FYI, I've pushed this in now.
Thanks,
Pedro Alves
^ permalink raw reply [flat|nested] 33+ messages in thread