| Summary: | gdb sets breakpoint at cold clone | ||
|---|---|---|---|
| Product: | gdb | Reporter: | Tom de Vries <vries> |
| Component: | breakpoints | Assignee: | Not yet assigned to anyone <unassigned> |
| Status: | RESOLVED FIXED | ||
| Severity: | normal | CC: | bernd.edlinger |
| Priority: | P2 | ||
| Version: | HEAD | ||
| Target Milestone: | --- | ||
| Host: | Target: | ||
| Build: | Last reconfirmed: | ||
Tentative patch:
...
diff --git a/gdb/utils.c b/gdb/utils.c
index 102db28787..073d5b2e10 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -2652,7 +2652,18 @@ strncmp_iw_with_mode (const char *string1, const char *string2,
return 0;
}
else
- return (*string1 != '\0' && *string1 != '(');
+ {
+ if (*string1 == '\0')
+ return 0;
+
+ if (*string1 == '(')
+ {
+ const char *end_str1 = string1 + strlen (string1);
+ return end_str1[-1] != ')';
+ }
+
+ return 1;
+ }
}
else
return 1;
...
(In reply to Tom de Vries from comment #1) > Tentative patch: OK, that's too restrictive, we miss out on the '() const': ... (gdb) complete b const_overload_fn()^M b struct_with_const_overload::const_overload_fn()^M b struct_with_const_overload::const_overload_fn() const^M (gdb) PASS: gdb.linespec/cpcompletion.exp: const-overload: cmd complete "b const_overload_fn()" b const_overload_fn^M Breakpoint 36 at 0x400646: file /data/gdb_versions/devel/src/gdb/testsuite/gdb.linespec/cpls.cc, line 197.^M (gdb) info breakpoint $bpnum^M Num Type Disp Enb Address What^M 36 breakpoint keep y 0x0000000000400646 in struct_with_const_overload::const_overload_fn() at /data/gd\ b_versions/devel/src/gdb/testsuite/gdb.linespec/cpls.cc:197^M (gdb) FAIL: gdb.linespec/cpcompletion.exp: const-overload: compare "b const_overload_fn" completion list with bp \ location list: matches ... Let's try this: ... diff --git a/gdb/utils.c b/gdb/utils.c index 102db28787..8b20d6669e 100644 --- a/gdb/utils.c +++ b/gdb/utils.c @@ -2652,7 +2652,34 @@ strncmp_iw_with_mode (const char *string1, const char *string2, return 0; } else - return (*string1 != '\0' && *string1 != '('); + { + if (*string1 == '\0') + return 0; + + /* Skip over '(.*)'. */ + if (*string1 != '(') + return 1; + while (true) + { + string1++; + if (*string1 == '\0') + return 0; + if (*string1 == ')') + break; + } + + /* Return no-match if there's a "[clone .cold]" suffix. */ + while (true) + { + string1++; + if (*string1 == '\0') + return 0; + if (*string1 == '[') + return strcmp (string1, "[clone .cold]") == 0; + } + + return 1; + } } else return 1; ... Another thing I noticed with cold clones:
...
$ gdb -q -batch a.out -ex "maint print psymbols" | egrep "foo|bar"
`bar', function, 0x400455
`foo', function, 0x400450
...
The psymbol address of the function bar is the address of "bar() [clone .cold]".
Presumably we'd want the entry address here.
Looking at the corresponding DWARF, we have:
...
<1><990>: Abbrev Number: 35 (DW_TAG_subprogram)
<991> DW_AT_name : foo
<995> DW_AT_decl_file : 1
<996> DW_AT_decl_line : 8
<997> DW_AT_decl_column : 1
<998> DW_AT_type : <0x39b>
<99c> DW_AT_ranges : 0x40
<9a0> DW_AT_frame_base : 1 byte block: 9c (DW_OP_call_frame_cfa)
<9a2> DW_AT_GNU_all_call_sites: 1
<9a2> DW_AT_sibling : <0x9b4>
...
In DWARF 4 standard 2.18 Entry Address we read:
...
If no DW_AT_entry_pc attribute is present, then the entry address is assumed to be the same as the value of the DW_AT_low_pc attribute, if present; otherwise, the entry address is unknown.
...
In this case, we have no DW_AT_entry_pc and no DW_AT_low_pc attribute. Arguably, this is a gcc bug.
Either way, in absence of a fix in gcc for this, we could adapt a interpretation that the entry pc is the start address of the first range. At least, this works for this exec:
...
00000070 00000000004005b0 00000000004005d7
00000070 0000000000400455 000000000040045a
00000070 <End of list>
...
This also seems to be the current interpretation for full symtabs, AFAIU from the comment for BLOCK_ENTRY_PC in block.h:
...
/* Define the "entry pc" for a block BL to be the lowest (start) address
for the block when all addresses within the block are contiguous. If
non-contiguous, then use the start address for the first range in the
block.
...
(In reply to Tom de Vries from comment #3) > Another thing I noticed with cold clones: > ... > $ gdb -q -batch a.out -ex "maint print psymbols" | egrep "foo|bar" > `bar', function, 0x400455 > `foo', function, 0x400450 > ... > > The psymbol address of the function bar is the address of "bar() [clone > .cold]". > > Presumably we'd want the entry address here. > > Looking at the corresponding DWARF, we have: > ... > <1><990>: Abbrev Number: 35 (DW_TAG_subprogram) > <991> DW_AT_name : foo > <995> DW_AT_decl_file : 1 > <996> DW_AT_decl_line : 8 > <997> DW_AT_decl_column : 1 > <998> DW_AT_type : <0x39b> > <99c> DW_AT_ranges : 0x40 > <9a0> DW_AT_frame_base : 1 byte block: 9c > (DW_OP_call_frame_cfa) > <9a2> DW_AT_GNU_all_call_sites: 1 > <9a2> DW_AT_sibling : <0x9b4> > ... > > In DWARF 4 standard 2.18 Entry Address we read: > ... > If no DW_AT_entry_pc attribute is present, then the entry address is assumed > to be the same as the value of the DW_AT_low_pc attribute, if present; > otherwise, the entry address is unknown. > ... > > In this case, we have no DW_AT_entry_pc and no DW_AT_low_pc attribute. > Arguably, this is a gcc bug. > > Either way, in absence of a fix in gcc for this, we could adapt a > interpretation that the entry pc is the start address of the first range. At > least, this works for this exec: > ... > 00000070 00000000004005b0 00000000004005d7 > 00000070 0000000000400455 000000000040045a > 00000070 <End of list> > ... > > This also seems to be the current interpretation for full symtabs, AFAIU > from the comment for BLOCK_ENTRY_PC in block.h: > ... > /* Define the "entry pc" for a block BL to be the lowest (start) address > > for the block when all addresses within the block are contiguous. If > > non-contiguous, then use the start address for the first range in the > > block. > > ... This is related to this patch: https://sourceware.org/pipermail/gdb-patches/2021-May/179369.html The master branch has been updated by Tom de Vries <vries@sourceware.org>: https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=17d305ef8f4b5bf20beaaad427490b3c6773909b commit 17d305ef8f4b5bf20beaaad427490b3c6773909b Author: Tom de Vries <tdevries@suse.de> Date: Tue Jun 1 15:25:51 2021 +0200 [gdb/symtab] Ignore cold clones Consider the test-case contained in this patch, compiled for c using gcc-10: ... $ gcc-10 -x c src/gdb/testsuite/gdb.cp/cold-clone.cc -O2 -g -Wall -Wextra ... When setting a breakpoint on foo, we get one breakpoint location: ... $ gdb -q -batch a.out -ex "b foo" Breakpoint 1 at 0x400560: file cold-clone.cc, line 28. ... However, when we compile for c++ instead, we get two breakpoint locations: ... $ gdb -q -batch a.out -ex "b foo" -ex "info break" Breakpoint 1 at 0x400430: foo. (2 locations) Num Type Disp Enb Address What 1 breakpoint keep y <MULTIPLE> 1.1 y 0x0000000000400430 in foo() at cold-clone.cc:30 1.2 y 0x0000000000400560 in foo() at cold-clone.cc:28 ... The additional breakpoint location at 0x400430 corresponds to the cold clone: ... $ nm a.out | grep foo 0000000000400560 t _ZL3foov 0000000000400430 t _ZL3foov.cold ... which demangled looks like this: ... $ nm -C a.out | grep foo 0000000000400560 t foo() 0000000000400430 t foo() [clone .cold] ... [ Or, in the case of the cc1 mentioned in PR23710: ... $ nm cc1 | grep do_rpo_vn.*cold 000000000058659d t \ _ZL9do_rpo_vnP8functionP8edge_defP11bitmap_headbb.cold.138 $ nm -C cc1 | grep do_rpo_vn.*cold 000000000058659d t \ do_rpo_vn(function*, edge_def*, bitmap_head*, bool, bool) [clone .cold.138] ... ] The cold clone is a part of the function that is split off from the rest of the function because it's considered cold (not frequently executed). So while the symbol points to code that is part of a function, it doesn't point to a function entry, so the desirable behaviour for "break foo" is to ignore this symbol. When compiling for c, the symbol "foo.cold" is entered as minimal symbol with the search name "foo.cold", and the lookup using "foo" fails to find that symbol. But when compiling for c++, the symbol "foo.cold" is entered as minimal symbol with both the mangled and demangled name, and for the demangled name "foo() [clone .cold]" we get the search name "foo" (because cp_search_name_hash stops hashing at '('), and the lookup using "foo" succeeds. Fix this by recognizing the cold clone suffix and returning false for such a minimal symbol in msymbol_is_function. Tested on x86_64-linux. gdb/ChangeLog: 2021-06-01 Tom de Vries <tdevries@suse.de> PR symtab/26096 * minsyms.c (msymbol_is_cold_clone): New function. (msymbol_is_function): Use msymbol_is_cold_clone. gdb/testsuite/ChangeLog: 2021-06-01 Tom de Vries <tdevries@suse.de> PR symtab/26096 * gdb.cp/cold-clone.cc: New test. * gdb.cp/cold-clone.exp: New file. (In reply to Bernd Edlinger from comment #6) > This is related to this patch: > https://sourceware.org/pipermail/gdb-patches/2021-May/179369.html I looked at the patch, and it doesn't look related. AFAUI, your patch does something related to full symbols, my observation was related to partial symbols. To confirm, I've applied the patch, and ran the example, no changes. Patch with test-case committed, marking resolved-fixed. Apparently I misunderstood your comment above:
> Either way, in absence of a fix in gcc for this, we could adapt a interpretation
> that the entry pc is the start address of the first range. At least, this works
> for this exec:
What I meant, is that currently the entry_pc is ignored at least when
you want to place a break point on an inline function.
Instead the start of the first subrange is used.
But the entry_pc is sometimes not the start of the first subrange,
but maybe the second or third.
My patch re-orders the sub-range so that the subrange that starts at
the entry_pc becomes the first one.
but I always looked at the breakpoint addresses:
(gdb) b <symbol>
(gdb) info b
|
Consider test-case test.c: ... $ cat test.c #include <stdlib.h> int a; int b; int c; static int __attribute__((used, noinline, noclone)) foo (void) { a = 2; if (b) abort (); return c; } static int __attribute__((used, noinline, noclone)) bar (void) { a = 1; if (c) abort (); return b; } int main (int argc, char **) { b = argc * 2; c = argc / 2; if (b + c == 5) abort (); return foo () + bar (); } ... Compiled with gcc-10: ... $ g++-10 test.c -O2 -g -Wall -Wextra ... The functions foo and bar get split into a hot and cold part by gcc's freorder-blocks-and-partition: ... $ nm a.out | c++filt | egrep "foo|bar" 00000000004005b0 t bar() 0000000000400455 t bar() [clone .cold] 0000000000400580 t foo() 0000000000400450 t foo() [clone .cold] ... When we set breakpoints on foo and bar however, we get two breakpoint locations per function: ... $ gdb -batch a.out -ex "b foo" -ex "b bar" -ex "info break" Breakpoint 1 at 0x400450: foo. (2 locations) Breakpoint 2 at 0x400455: bar. (2 locations) Num Type Disp Enb Address What 1 breakpoint keep y <MULTIPLE> 1.1 y 0x0000000000400450 in foo() at test.c:12 1.2 y 0x0000000000400580 in foo() at test.c:10 2 breakpoint keep y <MULTIPLE> 2.1 y 0x0000000000400455 in bar() at test.c:22 2.2 y 0x00000000004005b0 in bar() at test.c:20 ... So, the cold part of the function gets its own breakpoint. In other words, we have: ... $ gdb -q a.out Reading symbols from a.out... (gdb) b foo Breakpoint 1 at 0x400450: foo. (2 locations) (gdb) r Starting program: /home/vries/a.out Breakpoint 1, foo () at test.c:10 10 a = 2; (gdb) c Continuing. Breakpoint 1, foo () at test.c:12 12 abort (); ...