[1/2] Fix "list ambiguous_variable"
Commit Message
The "list" command allows specifying the name of variables as
argument, not just functions, so that users can type "list
a_global_variable".
That support is a broken when it comes to ambiguous locations though.
If there's more than one such global variable in the program, e.g.,
static globals in different compilation units, GDB ends up listing the
source of the first variable it finds, only.
linespec.c does find both symbol and minsym locations for all the
globals. The problem is that it ends up merging all the resulting
sals into one, because they all have address, zero. I.e., all sals
end up with sal.pc == 0, so maybe_add_address returns false for all
but the first.
The zero addresses appear because:
- in the minsyms case, linespec.c:minsym_found incorrectly treats all
minsyms as if they were function/text symbols. In list mode we can
end up with data symbols there, and we shouldn't be using
find_pc_sect_line on data symbols.
- in the debug symbols case, symbol_to_sal misses recording an address
(sal.pc) for non-block, non-label symbols.
gdb/ChangeLog:
yyyy-mm-dd Pedro Alves <palves@redhat.com>
* linespec.c (minsym_found): Handle non-text minsyms.
(symbol_to_sal): Record a sal.pc for non-block, non-label symbols.
gdb/testsuite/ChangeLog:
yyyy-mm-dd Pedro Alves <palves@redhat.com>
* gdb.base/list-ambiguous.exp (test_list_ambiguous_function):
Rename to ...
(test_list_ambiguous_symbol): ... this and add a symbol name
parameter. Adjust.
(test_list_ambiguous_function): Reimplement on top of
test_list_ambiguous_symbol and also test listing ambiguous
variables.
* gdb.base/list-ambiguous0.c (ambiguous): Rename to ...
(ambiguous_fun): ... this.
(ambiguous_var): New.
* gdb.base/list-ambiguous1.c (ambiguous): Rename to ...
(ambiguous_fun): ... this.
(ambiguous_var): New.
---
gdb/linespec.c | 61 ++++++++++++++++++-------------
gdb/testsuite/gdb.base/list-ambiguous.exp | 37 +++++++++++--------
gdb/testsuite/gdb.base/list-ambiguous0.c | 5 ++-
gdb/testsuite/gdb.base/list-ambiguous1.c | 5 ++-
4 files changed, 63 insertions(+), 45 deletions(-)
Comments
On 09/04/2017 11:47 AM, Pedro Alves wrote:
> The "list" command allows specifying the name of variables as
> argument, not just functions, so that users can type "list
> a_global_variable".
>
> That support is a broken when it comes to ambiguous locations though.
Very nice!
FWIW, I have only one trivial nit (below).
Keith
PS. Out of curiosity, I hacked up a test program with multiple symbols named "ambiguous," both functions and variables. Well done!
(gdb) set listsize 3
(gdb) list ambiguous
file: "amb1.c", line number: 5, symbol: "ambiguous"
4
5 static int ambiguous = 0;
6
file: "amb2.c", line number: 3, symbol: "ambiguous"
2 ambiguous (void)
3 {
4 return 0;
file: "amb3.c", line number: 3, symbol: "ambiguous"
2 ambiguous (void)
3 {
4 return 0;
file: "amb4.c", line number: 1, symbol: "ambiguous"
1 static int ambiguous = 0;
2
3 int
> diff --git a/gdb/linespec.c b/gdb/linespec.c
> index 4801808..d72d19d 100644
> --- a/gdb/linespec.c
> +++ b/gdb/linespec.c
> @@ -4318,35 +4318,45 @@ minsym_found (struct linespec_state *self, struct objfile *objfile,
> CORE_ADDR pc;
> struct symtab_and_line sal;
>
> - sal = find_pc_sect_line (MSYMBOL_VALUE_ADDRESS (objfile, msymbol),
> - (struct obj_section *) 0, 0);
> - sal.section = MSYMBOL_OBJ_SECTION (objfile, msymbol);
> + if (msymbol_is_text (msymbol))
> + {
> + sal = find_pc_sect_line (MSYMBOL_VALUE_ADDRESS (objfile, msymbol),
> + (struct obj_section *) 0, 0);
> + sal.section = MSYMBOL_OBJ_SECTION (objfile, msymbol);
>
> - /* 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, sal.pc, ¤t_target);
> - if (pc != sal.pc)
> - sal = find_pc_sect_line (pc, NULL, 0);
> + /* 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, sal.pc, ¤t_target);
This line exceeds the 80-char line length limit.
> + if (pc != sal.pc)
> + sal = find_pc_sect_line (pc, NULL, 0);
>
> - if (self->funfirstline)
> - {
> - if (sal.symtab != NULL
> - && (COMPUNIT_LOCATIONS_VALID (SYMTAB_COMPUNIT (sal.symtab))
> - || SYMTAB_LANGUAGE (sal.symtab) == language_asm))
> + if (self->funfirstline)
> {
> - /* If gdbarch_convert_from_func_ptr_addr does not apply then
> - sal.SECTION, sal.LINE&co. will stay correct from above.
> - If gdbarch_convert_from_func_ptr_addr applies then
> - sal.SECTION is cleared from above and sal.LINE&co. will
> - stay correct from the last find_pc_sect_line above. */
> - sal.pc = MSYMBOL_VALUE_ADDRESS (objfile, msymbol);
> - sal.pc = gdbarch_convert_from_func_ptr_addr (gdbarch, sal.pc,
> - ¤t_target);
> - if (gdbarch_skip_entrypoint_p (gdbarch))
> - sal.pc = gdbarch_skip_entrypoint (gdbarch, sal.pc);
> + if (sal.symtab != NULL
> + && (COMPUNIT_LOCATIONS_VALID (SYMTAB_COMPUNIT (sal.symtab))
> + || SYMTAB_LANGUAGE (sal.symtab) == language_asm))
> + {
> + /* If gdbarch_convert_from_func_ptr_addr does not apply then
> + sal.SECTION, sal.LINE&co. will stay correct from above.
> + If gdbarch_convert_from_func_ptr_addr applies then
> + sal.SECTION is cleared from above and sal.LINE&co. will
> + stay correct from the last find_pc_sect_line above. */
> + sal.pc = MSYMBOL_VALUE_ADDRESS (objfile, msymbol);
> + sal.pc = gdbarch_convert_from_func_ptr_addr (gdbarch, sal.pc,
> + ¤t_target);
> + if (gdbarch_skip_entrypoint_p (gdbarch))
> + sal.pc = gdbarch_skip_entrypoint (gdbarch, sal.pc);
> + }
> + else
> + skip_prologue_sal (&sal);
> }
> - else
> - skip_prologue_sal (&sal);
> + }
> + else
> + {
> + sal.objfile = objfile;
> + sal.pc = MSYMBOL_VALUE_ADDRESS (objfile, msymbol);
> + sal.pspace = current_program_space;
> + sal.section = MSYMBOL_OBJ_SECTION (objfile, msymbol);
> }
>
> if (maybe_add_address (self->addr_set, objfile->pspace, sal.pc))
On 09/06/2017 07:41 PM, Keith Seitz wrote:
> On 09/04/2017 11:47 AM, Pedro Alves wrote:
>> The "list" command allows specifying the name of variables as
>> argument, not just functions, so that users can type "list
>> a_global_variable".
>>
>> That support is a broken when it comes to ambiguous locations though.
>
> Very nice!
>
> FWIW, I have only one trivial nit (below).
>
> Keith
>
> PS. Out of curiosity, I hacked up a test program with multiple symbols named "ambiguous," both functions and variables. Well done!
Awesome. :-)
>
> (gdb) set listsize 3
> (gdb) list ambiguous
> file: "amb1.c", line number: 5, symbol: "ambiguous"
> 4
> 5 static int ambiguous = 0;
> 6
> file: "amb2.c", line number: 3, symbol: "ambiguous"
> 2 ambiguous (void)
> 3 {
> 4 return 0;
> file: "amb3.c", line number: 3, symbol: "ambiguous"
> 2 ambiguous (void)
> 3 {
> 4 return 0;
> file: "amb4.c", line number: 1, symbol: "ambiguous"
> 1 static int ambiguous = 0;
> 2
> 3 int
>
>> - /* 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, sal.pc, ¤t_target);
>> - if (pc != sal.pc)
>> - sal = find_pc_sect_line (pc, NULL, 0);
>> + /* 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, sal.pc, ¤t_target);
>
> This line exceeds the 80-char line length limit.
Indeed it does. Fixed before pushing (both patches).
Thanks for the review!
On 2017-09-20 11:25 AM, Pedro Alves wrote:
> On 09/06/2017 07:41 PM, Keith Seitz wrote:
>> On 09/04/2017 11:47 AM, Pedro Alves wrote:
>>> The "list" command allows specifying the name of variables as
>>> argument, not just functions, so that users can type "list
>>> a_global_variable".
>>>
>>> That support is a broken when it comes to ambiguous locations though.
>>
>> Very nice!
>>
>> FWIW, I have only one trivial nit (below).
>>
>> Keith
>>
>> PS. Out of curiosity, I hacked up a test program with multiple symbols named "ambiguous," both functions and variables. Well done!
>
> Awesome. :-)
>
>>
>> (gdb) set listsize 3
>> (gdb) list ambiguous
>> file: "amb1.c", line number: 5, symbol: "ambiguous"
>> 4
>> 5 static int ambiguous = 0;
>> 6
>> file: "amb2.c", line number: 3, symbol: "ambiguous"
>> 2 ambiguous (void)
>> 3 {
>> 4 return 0;
>> file: "amb3.c", line number: 3, symbol: "ambiguous"
>> 2 ambiguous (void)
>> 3 {
>> 4 return 0;
>> file: "amb4.c", line number: 1, symbol: "ambiguous"
>> 1 static int ambiguous = 0;
>> 2
>> 3 int
>>
>
>>> - /* 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, sal.pc, ¤t_target);
>>> - if (pc != sal.pc)
>>> - sal = find_pc_sect_line (pc, NULL, 0);
>>> + /* 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, sal.pc, ¤t_target);
>>
>> This line exceeds the 80-char line length limit.
>
> Indeed it does. Fixed before pushing (both patches).
>
> Thanks for the review!
>
Hi Pedro,
The buildbot shows some failures on ppc64be:
PASS -> FAIL: gdb.base/dbx.exp: whereis my_list
PASS -> FAIL: gdb.mi/gdb669.exp: -thread-list-ids
PASS -> FAIL: gdb.mi/gdb669.exp: finding MI result string
PASS -> FAIL: gdb.mi/gdb669.exp: finding number of threads in MI output
I tested on gcc110, and bisect points to this patch here (for both tests).
A symptom of the problem is that "break main" generates two locations.
Before (at e5f25bc5^):
(gdb) b main
Breakpoint 1 at 0x10000560: file test.c, line 3.
(gdb) info breakpoints
Num Type Disp Enb Address What
1 breakpoint keep y 0x0000000010000560 in main at test.c:3
After (at e5f25bc5)
(gdb) b main
Breakpoint 1 at 0x10000560: main. (2 locations)
(gdb) info breakpoints
Num Type Disp Enb Address What
1 breakpoint keep y <MULTIPLE>
1.1 y 0x0000000010000560 in main at test.c:3
1.2 y 0x0000000010020078 <main>
Simon
On Mon, 16 Oct 2017 17:03:18 +0200, Simon Marchi wrote:
> The buildbot shows some failures on ppc64be:
>
> PASS -> FAIL: gdb.base/dbx.exp: whereis my_list
> PASS -> FAIL: gdb.mi/gdb669.exp: -thread-list-ids
> PASS -> FAIL: gdb.mi/gdb669.exp: finding MI result string
> PASS -> FAIL: gdb.mi/gdb669.exp: finding number of threads in MI output
>
> I tested on gcc110, and bisect points to this patch here (for both tests).
> A symptom of the problem is that "break main" generates two locations.
>
> Before (at e5f25bc5^):
>
> (gdb) b main
> Breakpoint 1 at 0x10000560: file test.c, line 3.
> (gdb) info breakpoints
> Num Type Disp Enb Address What
> 1 breakpoint keep y 0x0000000010000560 in main at test.c:3
>
> After (at e5f25bc5)
>
> (gdb) b main
> Breakpoint 1 at 0x10000560: main. (2 locations)
> (gdb) info breakpoints
> Num Type Disp Enb Address What
> 1 breakpoint keep y <MULTIPLE>
> 1.1 y 0x0000000010000560 in main at test.c:3
> 1.2 y 0x0000000010020078 <main>
I have also hit this regression now on RHEL-7.5pre ppc64:
e5f25bc5d6dba5a5c4dd36e08afd57e918c63dea is the first bad commit
commit e5f25bc5d6dba5a5c4dd36e08afd57e918c63dea
Author: Pedro Alves <palves@redhat.com>
Date: Wed Sep 20 16:12:54 2017 +0100
Fix "list ambiguous_variable"
echo 'main(){}'|gcc -g -x c -;./gdb -batch ./a.out -ex start
Temporary breakpoint 1 at 0x10000560: file <stdin>, line 1.
Temporary breakpoint 1, main () at <stdin>:1
1 <stdin>: No such file or directory.
->
Temporary breakpoint 1 at 0x10000560: main. (2 locations)
Program received signal SIGSEGV, Segmentation fault.
0x7d82100810000554 in ?? ()
Jan
@@ -4318,35 +4318,45 @@ minsym_found (struct linespec_state *self, struct objfile *objfile,
CORE_ADDR pc;
struct symtab_and_line sal;
- sal = find_pc_sect_line (MSYMBOL_VALUE_ADDRESS (objfile, msymbol),
- (struct obj_section *) 0, 0);
- sal.section = MSYMBOL_OBJ_SECTION (objfile, msymbol);
+ if (msymbol_is_text (msymbol))
+ {
+ sal = find_pc_sect_line (MSYMBOL_VALUE_ADDRESS (objfile, msymbol),
+ (struct obj_section *) 0, 0);
+ sal.section = MSYMBOL_OBJ_SECTION (objfile, msymbol);
- /* 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, sal.pc, ¤t_target);
- if (pc != sal.pc)
- sal = find_pc_sect_line (pc, NULL, 0);
+ /* 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, sal.pc, ¤t_target);
+ if (pc != sal.pc)
+ sal = find_pc_sect_line (pc, NULL, 0);
- if (self->funfirstline)
- {
- if (sal.symtab != NULL
- && (COMPUNIT_LOCATIONS_VALID (SYMTAB_COMPUNIT (sal.symtab))
- || SYMTAB_LANGUAGE (sal.symtab) == language_asm))
+ if (self->funfirstline)
{
- /* If gdbarch_convert_from_func_ptr_addr does not apply then
- sal.SECTION, sal.LINE&co. will stay correct from above.
- If gdbarch_convert_from_func_ptr_addr applies then
- sal.SECTION is cleared from above and sal.LINE&co. will
- stay correct from the last find_pc_sect_line above. */
- sal.pc = MSYMBOL_VALUE_ADDRESS (objfile, msymbol);
- sal.pc = gdbarch_convert_from_func_ptr_addr (gdbarch, sal.pc,
- ¤t_target);
- if (gdbarch_skip_entrypoint_p (gdbarch))
- sal.pc = gdbarch_skip_entrypoint (gdbarch, sal.pc);
+ if (sal.symtab != NULL
+ && (COMPUNIT_LOCATIONS_VALID (SYMTAB_COMPUNIT (sal.symtab))
+ || SYMTAB_LANGUAGE (sal.symtab) == language_asm))
+ {
+ /* If gdbarch_convert_from_func_ptr_addr does not apply then
+ sal.SECTION, sal.LINE&co. will stay correct from above.
+ If gdbarch_convert_from_func_ptr_addr applies then
+ sal.SECTION is cleared from above and sal.LINE&co. will
+ stay correct from the last find_pc_sect_line above. */
+ sal.pc = MSYMBOL_VALUE_ADDRESS (objfile, msymbol);
+ sal.pc = gdbarch_convert_from_func_ptr_addr (gdbarch, sal.pc,
+ ¤t_target);
+ if (gdbarch_skip_entrypoint_p (gdbarch))
+ sal.pc = gdbarch_skip_entrypoint (gdbarch, sal.pc);
+ }
+ else
+ skip_prologue_sal (&sal);
}
- else
- skip_prologue_sal (&sal);
+ }
+ else
+ {
+ sal.objfile = objfile;
+ sal.pc = MSYMBOL_VALUE_ADDRESS (objfile, msymbol);
+ sal.pspace = current_program_space;
+ sal.section = MSYMBOL_OBJ_SECTION (objfile, msymbol);
}
if (maybe_add_address (self->addr_set, objfile->pspace, sal.pc))
@@ -4633,6 +4643,7 @@ symbol_to_sal (struct symtab_and_line *result,
*result = {};
result->symtab = symbol_symtab (sym);
result->line = SYMBOL_LINE (sym);
+ result->pc = SYMBOL_VALUE_ADDRESS (sym);
result->pspace = SYMTAB_PSPACE (result->symtab);
return 1;
}
@@ -39,36 +39,41 @@ proc line_range_pattern { range_start range_end } {
# Test the "list" command with linespecs that expand to multiple
# locations.
-proc test_list_ambiguous_function {} {
+proc test_list_ambiguous_symbol {symbol_line symbol} {
global srcfile srcfile2
- set lineno0 [gdb_get_line_number "ambiguous (void)" $srcfile]
- set lineno1 [gdb_get_line_number "ambiguous (void)" $srcfile2]
+ set lineno0 [gdb_get_line_number $symbol_line $srcfile]
+ set lineno1 [gdb_get_line_number $symbol_line $srcfile2]
set lines0_re [line_range_pattern [expr $lineno0 - 5] [expr $lineno0 + 4]]
set lines1_re [line_range_pattern [expr $lineno1 - 5] [expr $lineno1 + 4]]
set any "\[^\r\n\]*"
set h0_re "file: \"${any}list-ambiguous0.c\", line number: $lineno0"
set h1_re "file: \"${any}list-ambiguous1.c\", line number: $lineno1"
- gdb_test "list ambiguous" "${h0_re}${lines0_re}\r\n${h1_re}${lines1_re}"
-
- gdb_test "list main,ambiguous" \
- "Specified last line 'ambiguous' is ambiguous:\r\n${h0_re}\r\n${h1_re}"
- gdb_test "list ,ambiguous" \
- "Specified last line 'ambiguous' is ambiguous:\r\n${h0_re}\r\n${h1_re}"
- gdb_test "list ambiguous,main" \
- "Specified first line 'ambiguous' is ambiguous:\r\n${h0_re}\r\n${h1_re}"
- gdb_test "list ambiguous,ambiguous" \
- "Specified first line 'ambiguous' is ambiguous:\r\n${h0_re}\r\n${h1_re}"
- gdb_test "list ambiguous," \
- "Specified first line 'ambiguous' is ambiguous:\r\n${h0_re}\r\n${h1_re}"
+ gdb_test "list $symbol" "${h0_re}${lines0_re}\r\n${h1_re}${lines1_re}"
+
+ gdb_test "list main,$symbol" \
+ "Specified last line '$symbol' is ambiguous:\r\n${h0_re}\r\n${h1_re}"
+ gdb_test "list ,$symbol" \
+ "Specified last line '$symbol' is ambiguous:\r\n${h0_re}\r\n${h1_re}"
+ gdb_test "list $symbol,main" \
+ "Specified first line '$symbol' is ambiguous:\r\n${h0_re}\r\n${h1_re}"
+ gdb_test "list $symbol,$symbol" \
+ "Specified first line '$symbol' is ambiguous:\r\n${h0_re}\r\n${h1_re}"
+ gdb_test "list $symbol," \
+ "Specified first line '$symbol' is ambiguous:\r\n${h0_re}\r\n${h1_re}"
# While at it, test the "edit" command as well, since it shares
# code with "list".
- gdb_test "edit ambiguous" \
+ gdb_test "edit $symbol" \
"Specified line is ambiguous:\r\n${h0_re}\r\n${h1_re}"
}
+proc test_list_ambiguous_function {} {
+ test_list_ambiguous_symbol "ambiguous_fun (void)" "ambiguous_fun"
+ test_list_ambiguous_symbol "ambiguous_var" "ambiguous_var"
+}
+
gdb_test_no_output "set listsize 10"
test_list_ambiguous_function
@@ -19,12 +19,13 @@
-/* This function is defined in both
+/* These symbols are defined in both
list-ambiguous0.c/list-ambiguous1.c files, in order to test
"list"'s behavior with ambiguous linespecs. */
-static void __attribute__ ((used)) ambiguous (void) {}
+static void __attribute__ ((used)) ambiguous_fun (void) {}
+static int ambiguous_var;
@@ -23,11 +23,12 @@
-/* This function is defined in both
+/* These symbols are defined in both
list-ambiguous0.c/list-ambiguous1.c files, in order to test
"list"'s behavior with ambiguous linespecs. */
-static void __attribute__ ((used)) ambiguous (void) {}
+static void __attribute__ ((used)) ambiguous_fun (void) {}
+static int ambiguous_var;