[patch+7.8] Fix 7.8 regression: resolve_dynamic_struct: Assertion `TYPE_NFIELDS (type) > 0' (PR 17642)
Commit Message
Hi,
https://sourceware.org/bugzilla/show_bug.cgi?id=17642
------------------------------------------------------------------------------
cat >2.c <<EOH
struct b;
struct c {
struct b *bp;
} c;
int main(void) { return 0; }
EOH
cat >2b.c <<EOH
struct b {
int a[0];
} use_b;
EOH
gcc -o 2 2.c 2b.c -Wall -g
# gcc-4.9.2-1.fc21.x86_64
gdb ./2 -ex 'p *c.bp'
# 520c7b56ac91e91120c59d7a85466ec9394277cf
gdbtypes.c:1811: internal-error: resolve_dynamic_struct: Assertion `TYPE_NFIELDS (type) > 0' failed.
A problem internal to GDB has been detected,
further debugging may prove unreliable.
Regression since:
commit 012370f6818657a816df1463ee71ca4e4ee40b33
Author: Tom Tromey <tromey@redhat.com>
Date: Thu May 8 11:26:44 2014 -0600
handle VLA in a struct or union
Bugreport:
Regression with gdb scripts for Linux kernel
https://sourceware.org/ml/gdb/2014-08/msg00127.html
It is another missing check_typedef().
------------------------------------------------------------------------------
No regressions on {x86_64,x86_64-m32,i686}-fedora21pre-linux-gnu.
That big change after "else" is just reindentation.
Thanks,
Jan
gdb/
2014-11-25 Jan Kratochvil <jan.kratochvil@redhat.com>
Fix internal error on stubs of dynamic types.
* gdbtypes.c (resolve_dynamic_type_internal): Apply check_typedef to
TYPE if not TYPE_CODE_TYPEDEF.
gdb/testsuite/
2014-11-25 Jan Kratochvil <jan.kratochvil@redhat.com>
Fix internal error on stubs of dynamic types.
* gdb.base/vla-stub-define.c: New file.
* gdb.base/vla-stub.c: New file.
* gdb.base/vla-stub.exp: New file.
Comments
> It is another missing check_typedef().
> gdb/
> 2014-11-25 Jan Kratochvil <jan.kratochvil@redhat.com>
>
> Fix internal error on stubs of dynamic types.
> * gdbtypes.c (resolve_dynamic_type_internal): Apply check_typedef to
> TYPE if not TYPE_CODE_TYPEDEF.
>
> gdb/testsuite/
> 2014-11-25 Jan Kratochvil <jan.kratochvil@redhat.com>
>
> Fix internal error on stubs of dynamic types.
> * gdb.base/vla-stub-define.c: New file.
> * gdb.base/vla-stub.c: New file.
> * gdb.base/vla-stub.exp: New file.
Pre-approved with the following comments. Ok for 7.8 as well, although
the chances of a 7.8.2 are getting slimmer, and you'll need also to
create a PR.
> diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
> index 61d259d..aa76604 100644
> --- a/gdb/gdbtypes.c
> +++ b/gdb/gdbtypes.c
> @@ -1875,41 +1875,48 @@ resolve_dynamic_type_internal (struct type *type, CORE_ADDR addr,
> if (!is_dynamic_type_internal (real_type, top_level))
> return type;
>
> - switch (TYPE_CODE (type))
> + if (TYPE_CODE (type) == TYPE_CODE_TYPEDEF)
> {
> - case TYPE_CODE_TYPEDEF:
> resolved_type = copy_type (type);
> TYPE_TARGET_TYPE (resolved_type)
> = resolve_dynamic_type_internal (TYPE_TARGET_TYPE (type), addr,
> top_level);
> - break;
> + }
> + else
Trailing space here...
> + {
> + /* Typedefs do not need to be preserved here but we need
> + to resolve any possible stub types. */
> + type = real_type;
It took me a moment, despite your added comment, to understand that
it was not so much about TYPE_CODE_TYPEDEF but about stubs! I keep
forgetting that check_typedef does more than unwrapping typedef
layers.
We know that type isn't a typedef here, so mentioning typedefs
is a bit confusing, IMO. How about we adjust the comment to say,
for instance:
/* Before trying to resolve TYPE, make sure it is not a stub. */
This is only a suggestion, and it's subjective, so feel free to ignore.
Thank you,
On Sat, 13 Dec 2014 15:23:51 +0100, Joel Brobecker wrote:
> Ok for 7.8 as well, although the chances of a 7.8.2 are getting slimmer, and
> you'll need also to create a PR.
The PR already exists, I was aware 7.8.2 won't happen but I had some
downstream 7.8 branches maintenance reasons for it but... I just haven't
checked it into the 7.8 branch as you are right it really does not make sense.
> /* Before trying to resolve TYPE, make sure it is not a stub. */
Yes, I am fine with this comment, used it.
Checked in:
5537b577695dd5cd72395590d7ebd7aa92ee856a
Thanks,
Jan
On Sat, Dec 13, 2014 at 6:23 AM, Joel Brobecker <brobecker@adacore.com> wrote:
>> - switch (TYPE_CODE (type))
>> + if (TYPE_CODE (type) == TYPE_CODE_TYPEDEF)
>> {
>> - case TYPE_CODE_TYPEDEF:
>> resolved_type = copy_type (type);
>> TYPE_TARGET_TYPE (resolved_type)
>> = resolve_dynamic_type_internal (TYPE_TARGET_TYPE (type), addr,
>> top_level);
>> - break;
>> + }
>> + else
>
> Trailing space here...
>
>> + {
>> + /* Typedefs do not need to be preserved here but we need
>> + to resolve any possible stub types. */
>> + type = real_type;
>
> It took me a moment, despite your added comment, to understand that
> it was not so much about TYPE_CODE_TYPEDEF but about stubs! I keep
> forgetting that check_typedef does more than unwrapping typedef
> layers.
Yeah. PR 17450 describes this a bit better IMO.
https://sourceware.org/bugzilla/show_bug.cgi?id=17450
Hi Jan,
> > gdb/
> > 2014-11-25 Jan Kratochvil <jan.kratochvil@redhat.com>
> >
> > Fix internal error on stubs of dynamic types.
> > * gdbtypes.c (resolve_dynamic_type_internal): Apply check_typedef to
> > TYPE if not TYPE_CODE_TYPEDEF.
> >
> > gdb/testsuite/
> > 2014-11-25 Jan Kratochvil <jan.kratochvil@redhat.com>
> >
> > Fix internal error on stubs of dynamic types.
> > * gdb.base/vla-stub-define.c: New file.
> > * gdb.base/vla-stub.c: New file.
> > * gdb.base/vla-stub.exp: New file.
>
> Pre-approved with the following comments. Ok for 7.8 as well, although
> the chances of a 7.8.2 are getting slimmer, and you'll need also to
> create a PR.
I took a look at the todo list for 7.8 again this morning, and found
that there is not just...
PR gdb/17525 (target-async: breakpoint commands not executed when
program run from -x script)
... which I remembered, but also...
PR gdb/16215 (SPARC: can't compute CFA for this frame)
... which I had forgotten about, and makes debugging on sparc
completely broken with GCC 4.9. Combined with your patch, we might
want to produce a 7.8.2 after all. So, if you'd like, you could
push your patch on the branch as well.
Sorry for changing my tune on this, but I has just forgotten about
the situation on SPARC!
Thanks,
On Mon, 15 Dec 2014 16:06:09 +0100, Joel Brobecker wrote:
> ... which I had forgotten about, and makes debugging on sparc
> completely broken with GCC 4.9. Combined with your patch, we might
> want to produce a 7.8.2 after all. So, if you'd like, you could
> push your patch on the branch as well.
BTW working now on 7.8 regression wrt ignored "q" abort for "bt" more-prompt
so that one always has to backtrace all the hundreds of frames
OTOH without backported all the acceleration fixes and even the still pending
[patchv2] Fix 100x slowdown regression on DWZ files
https://sourceware.org/ml/gdb-patches/2014-10/msg00031.html
one would not wait 5 minutes for a backtrace anyway so not sure if 7.8*
releases incl. FSF GDB HEAD make sense.</grin>
Jan
On Mon, 15 Dec 2014 16:06:09 +0100, Joel Brobecker wrote:
> So, if you'd like, you could push your patch on the branch as well.
Checked in 7.8:
c644a8577431101e5f50b6d9664a4606bd92cbfd reindent
dbdc8a04a60670542cfda8749a2be78779ff7720 the fix
https://sourceware.org/gdb/wiki/GDB_7.8_Release?action=diff&rev2=103&rev1=101
Jan
On Mon, Dec 15, 2014 at 11:12 AM, Jan Kratochvil
<jan.kratochvil@redhat.com> wrote:
> On Mon, 15 Dec 2014 16:06:09 +0100, Joel Brobecker wrote:
>> So, if you'd like, you could push your patch on the branch as well.
>
> Checked in 7.8:
> c644a8577431101e5f50b6d9664a4606bd92cbfd reindent
> dbdc8a04a60670542cfda8749a2be78779ff7720 the fix
> https://sourceware.org/gdb/wiki/GDB_7.8_Release?action=diff&rev2=103&rev1=101
>
>
I'd like to backport
https://sourceware.org/ml/gdb-patches/2014-12/msg00116.html
to 7.8 branch.
Thanks.
> I'd like to backport
>
> https://sourceware.org/ml/gdb-patches/2014-12/msg00116.html
>
> to 7.8 branch.
This is a fairly big patch...
1. How bad is performance without it?
2. Can you find a binutils maintainer that will vouch that
this patch is 100% safe?
The 7.8.2 is in the pipe because we found a number of crippling issues
in 7.8.1. At this point, we should only consider extra-safe patches,
or patches that fix issues considered to be blocking the use of 7.8.1.
For yours to qualify, you'll need to answer both questions above.
On Mon, Dec 15, 2014 at 11:36 AM, Joel Brobecker <brobecker@adacore.com> wrote:
>> I'd like to backport
>>
>> https://sourceware.org/ml/gdb-patches/2014-12/msg00116.html
>>
>> to 7.8 branch.
>
> This is a fairly big patch...
> 1. How bad is performance without it?
It was very bad, like > 100x slower.
> 2. Can you find a binutils maintainer that will vouch that
> this patch is 100% safe?
As the x86 binutils maintainer, do I count?
> The 7.8.2 is in the pipe because we found a number of crippling issues
> in 7.8.1. At this point, we should only consider extra-safe patches,
> or patches that fix issues considered to be blocking the use of 7.8.1.
> For yours to qualify, you'll need to answer both questions above.
>
> --
> Joel
> > This is a fairly big patch...
> > 1. How bad is performance without it?
>
> It was very bad, like > 100x slower.
OK, but 100x slower than 1usec is still unnoticeable. When did you start
noticing it, for what kind of program (size?), and what type of delay
were you seeing? Also, is that a regression compared to 7.7? The idea
is that, if it's just a second or two, or even ten, that's still quite
bearable, and unless you are absolutely sure that your patch is safe,
perhaps we should pass...
> > 2. Can you find a binutils maintainer that will vouch that
> > this patch is 100% safe?
>
> As the x86 binutils maintainer, do I count?
Absolutely. I just need someone who knows the code well enough to
be trusted with its maintainance to stand behind the code and
confirm that it is considered safe. In this case, I need extra safe,
considering the fact that this is a .2 release.
I hope I've described my concerns well enough for you to make
the decision. Now that I have explained my thought process, I feel
you're more qualified to make an informed decision now. Your call.
Please remember that, if you do push it to 7.8, you'll need a PR
associated to is so you can document the fix in the release wiki
page (see fixes in GDB 7.8.2):
https://sourceware.org/gdb/wiki/GDB_7.8_Release
On Mon, Dec 15, 2014 at 12:41 PM, Joel Brobecker <brobecker@adacore.com> wrote:
>> > This is a fairly big patch...
>> > 1. How bad is performance without it?
>>
>> It was very bad, like > 100x slower.
>
> OK, but 100x slower than 1usec is still unnoticeable. When did you start
> noticing it, for what kind of program (size?), and what type of delay
> were you seeing? Also, is that a regression compared to 7.7? The idea
> is that, if it's just a second or two, or even ten, that's still quite
> bearable, and unless you are absolutely sure that your patch is safe,
> perhaps we should pass...
>
>> > 2. Can you find a binutils maintainer that will vouch that
>> > this patch is 100% safe?
>>
>> As the x86 binutils maintainer, do I count?
>
> Absolutely. I just need someone who knows the code well enough to
> be trusted with its maintainance to stand behind the code and
> confirm that it is considered safe. In this case, I need extra safe,
> considering the fact that this is a .2 release.
>
> I hope I've described my concerns well enough for you to make
> the decision. Now that I have explained my thought process, I feel
> you're more qualified to make an informed decision now. Your call.
>
> Please remember that, if you do push it to 7.8, you'll need a PR
> associated to is so you can document the fix in the release wiki
> page (see fixes in GDB 7.8.2):
> https://sourceware.org/gdb/wiki/GDB_7.8_Release
The bug was filed against bintuils:
https://sourceware.org/bugzilla/show_bug.cgi?id=17677
since the code in question is in bfd. But the actual bug is for GDB:
https://bugs.launchpad.net/ubuntu/+source/gdb/+bug/1388999
where the issues was observed. I will retest by my patch and
backport it to 7.8 branch.
Thanks.
On Mon, Dec 15, 2014 at 1:09 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Mon, Dec 15, 2014 at 12:41 PM, Joel Brobecker <brobecker@adacore.com> wrote:
>>> > This is a fairly big patch...
>>> > 1. How bad is performance without it?
>>>
>>> It was very bad, like > 100x slower.
>>
>> OK, but 100x slower than 1usec is still unnoticeable. When did you start
>> noticing it, for what kind of program (size?), and what type of delay
>> were you seeing? Also, is that a regression compared to 7.7? The idea
>> is that, if it's just a second or two, or even ten, that's still quite
>> bearable, and unless you are absolutely sure that your patch is safe,
>> perhaps we should pass...
>>
>>> > 2. Can you find a binutils maintainer that will vouch that
>>> > this patch is 100% safe?
>>>
>>> As the x86 binutils maintainer, do I count?
>>
>> Absolutely. I just need someone who knows the code well enough to
>> be trusted with its maintainance to stand behind the code and
>> confirm that it is considered safe. In this case, I need extra safe,
>> considering the fact that this is a .2 release.
>>
>> I hope I've described my concerns well enough for you to make
>> the decision. Now that I have explained my thought process, I feel
>> you're more qualified to make an informed decision now. Your call.
>>
>> Please remember that, if you do push it to 7.8, you'll need a PR
>> associated to is so you can document the fix in the release wiki
>> page (see fixes in GDB 7.8.2):
>> https://sourceware.org/gdb/wiki/GDB_7.8_Release
>
> The bug was filed against bintuils:
>
> https://sourceware.org/bugzilla/show_bug.cgi?id=17677
>
> since the code in question is in bfd. But the actual bug is for GDB:
>
> https://bugs.launchpad.net/ubuntu/+source/gdb/+bug/1388999
>
> where the issues was observed. I will retest by my patch and
> backport it to 7.8 branch.
>
There is no regression in GDB 7.8 testsuite. I pushed my change
into 7.8 branch. Its PR is
https://sourceware.org/bugzilla/show_bug.cgi?id=17677
Thanks.
> There is no regression in GDB 7.8 testsuite. I pushed my change
> into 7.8 branch. Its PR is
>
> https://sourceware.org/bugzilla/show_bug.cgi?id=17677
OK - Please remember to also update the wiki as explained below:
> >> Please remember that, if you do push it to 7.8, you'll need a PR
> >> associated to is so you can document the fix in the release wiki
> >> page (see fixes in GDB 7.8.2):
> >> https://sourceware.org/gdb/wiki/GDB_7.8_Release
On Mon, Dec 15, 2014 at 2:44 PM, Joel Brobecker <brobecker@adacore.com> wrote:
>> There is no regression in GDB 7.8 testsuite. I pushed my change
>> into 7.8 branch. Its PR is
>>
>> https://sourceware.org/bugzilla/show_bug.cgi?id=17677
>
> OK - Please remember to also update the wiki as explained below:
>
>> >> Please remember that, if you do push it to 7.8, you'll need a PR
>> >> associated to is so you can document the fix in the release wiki
>> >> page (see fixes in GDB 7.8.2):
>> >> https://sourceware.org/gdb/wiki/GDB_7.8_Release
>
I created a wiki account. But I can't edit it.
@@ -1875,41 +1875,48 @@ resolve_dynamic_type_internal (struct type *type, CORE_ADDR addr,
if (!is_dynamic_type_internal (real_type, top_level))
return type;
- switch (TYPE_CODE (type))
+ if (TYPE_CODE (type) == TYPE_CODE_TYPEDEF)
{
- case TYPE_CODE_TYPEDEF:
resolved_type = copy_type (type);
TYPE_TARGET_TYPE (resolved_type)
= resolve_dynamic_type_internal (TYPE_TARGET_TYPE (type), addr,
top_level);
- break;
+ }
+ else
+ {
+ /* Typedefs do not need to be preserved here but we need
+ to resolve any possible stub types. */
+ type = real_type;
- case TYPE_CODE_REF:
- {
- CORE_ADDR target_addr = read_memory_typed_address (addr, type);
+ switch (TYPE_CODE (type))
+ {
+ case TYPE_CODE_REF:
+ {
+ CORE_ADDR target_addr = read_memory_typed_address (addr, type);
- resolved_type = copy_type (type);
- TYPE_TARGET_TYPE (resolved_type)
- = resolve_dynamic_type_internal (TYPE_TARGET_TYPE (type),
- target_addr, top_level);
- break;
- }
+ resolved_type = copy_type (type);
+ TYPE_TARGET_TYPE (resolved_type)
+ = resolve_dynamic_type_internal (TYPE_TARGET_TYPE (type),
+ target_addr, top_level);
+ break;
+ }
- case TYPE_CODE_ARRAY:
- resolved_type = resolve_dynamic_array (type, addr);
- break;
+ case TYPE_CODE_ARRAY:
+ resolved_type = resolve_dynamic_array (type, addr);
+ break;
- case TYPE_CODE_RANGE:
- resolved_type = resolve_dynamic_range (type, addr);
- break;
+ case TYPE_CODE_RANGE:
+ resolved_type = resolve_dynamic_range (type, addr);
+ break;
- case TYPE_CODE_UNION:
- resolved_type = resolve_dynamic_union (type, addr);
- break;
+ case TYPE_CODE_UNION:
+ resolved_type = resolve_dynamic_union (type, addr);
+ break;
- case TYPE_CODE_STRUCT:
- resolved_type = resolve_dynamic_struct (type, addr);
- break;
+ case TYPE_CODE_STRUCT:
+ resolved_type = resolve_dynamic_struct (type, addr);
+ break;
+ }
}
/* Resolve data_location attribute. */
new file mode 100644
@@ -0,0 +1,21 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+ Copyright 2014 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/>. */
+
+struct dynamic_struct
+{
+ int dynamic_field[0];
+} use_dynamic_struct;
new file mode 100644
@@ -0,0 +1,37 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+ Copyright 2014 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/>. */
+
+struct dynamic_struct;
+typedef struct dynamic_struct dynamic_struct_t;
+
+struct static_struct
+{
+ int field;
+};
+typedef struct static_struct static_struct_t;
+
+struct local_struct
+{
+ static_struct_t here;
+ dynamic_struct_t *ptr;
+} local_struct;
+
+int
+main (void)
+{
+ return 0;
+}
new file mode 100644
@@ -0,0 +1,25 @@
+# Copyright 2014 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/>.
+
+standard_testfile .c vla-stub-define.c
+if { [prepare_for_testing "failed to prepare for vla-stub.exp" \
+ ${testfile} [list ${srcfile} ${srcfile2}]] } {
+ return -1
+}
+
+gdb_test "p *local_struct.ptr" { = {dynamic_field = 0x0}}
+
+gdb_test "whatis local_struct.here" "type = static_struct_t"
+gdb_test "whatis *local_struct.ptr" "type = dynamic_struct_t"