From dd5ca05f47abba4ad425dd125a1d9ac7d53d2d8c Mon Sep 17 00:00:00 2001 From: Simon Marchi Date: Sat, 23 Jan 2021 17:36:55 -0500 Subject: [PATCH] gdb: fix regression in copy_type_recursive Commit 5b7d941b90d1 ("gdb: add owner-related methods to struct type") introduced a regression when running gdb.base/jit-reader-simple.exp and others. A NULL pointer dereference happens here: #3 0x0000557b7e9e8650 in gdbarch_obstack (arch=0x0) at /home/simark/src/binutils-gdb/gdb/gdbarch.c:484 #4 0x0000557b7ea5b138 in copy_type_recursive (objfile=0x614000006640, type=0x62100018da80, copied_types=0x62100018e280) at /home/simark/src/binutils-gdb/gdb/gdbtypes.c:5537 #5 0x0000557b7ea5dcbb in copy_type_recursive (objfile=0x614000006640, type=0x62100018e200, copied_types=0x62100018e280) at /home/simark/src/binutils-gdb/gdb/gdbtypes.c:5598 #6 0x0000557b802cef51 in preserve_one_value (value=0x6110000b3640, objfile=0x614000006640, copied_types=0x62100018e280) at /home/simark/src/binutils-gdb/gdb/value.c:2518 #7 0x0000557b802cf787 in preserve_values (objfile=0x614000006640) at /home/simark/src/binutils-gdb/gdb/value.c:2562 #8 0x0000557b7fbaf19b in reread_symbols () at /home/simark/src/binutils-gdb/gdb/symfile.c:2489 #9 0x0000557b7ec65d1d in run_command_1 (args=0x0, from_tty=1, run_how=RUN_NORMAL) at /home/simark/src/binutils-gdb/gdb/infcmd.c:439 #10 0x0000557b7ec67a97 in run_command (args=0x0, from_tty=1) at /home/simark/src/binutils-gdb/gdb/infcmd.c:546 This is inside a TYPE_ALLOC macro. The fact that gdbarch_obstack is called means that the type is flagged as being arch-owned, but arch=0x0 means that type::arch returned NULL, probably meaning that the m_owner field contains NULL. If we look at the code before the problematic patch, in the copy_type_recursive function, we see: if (! TYPE_OBJFILE_OWNED (type)) return type; ... TYPE_OBJFILE_OWNED (new_type) = 0; TYPE_OWNER (new_type).gdbarch = get_type_arch (type); The last two lines were replaced with: new_type->set_owner (type->arch ()); get_type_arch and type->arch isn't the same thing: get_type_arch gets the type's arch owner if it is arch-owned, and gets the objfile's arch if the type is objfile owned. So it always returns non-NULL. type->arch returns the type's arch if the type is arch-owned, else NULL. So since the original type is objfile owned, it effectively made the new type arch-owned (that is good) but set the owner to NULL (that is bad). Fix this by using get_type_arch again there. I spotted one other similar change in lookup_array_range_type, in the original patch. But that one appears to be correct, as it is executed only if the type is arch-owned. Add some asserts in type::set_owner to ensure we never set a NULL owner. That would have helped catch the issue a little bit earlier, so it could help in the future. gdb/ChangeLog: * gdbtypes.c (copy_type_recursive): Use get_type_arch. * gdbtypes.h (struct type) : Add asserts. Change-Id: I5d8bc7bfc83b3abc579be0b5aadeae4241179a00 --- gdb/ChangeLog | 5 +++++ gdb/gdbtypes.c | 2 +- gdb/gdbtypes.h | 4 ++++ 3 files changed, 10 insertions(+), 1 deletion(-) diff --git a/gdb/ChangeLog b/gdb/ChangeLog index bb186766e3f..c1eedea35d7 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,3 +1,8 @@ +2021-01-23 Simon Marchi + + * gdbtypes.c (copy_type_recursive): Use get_type_arch. + * gdbtypes.h (struct type) : Add asserts. + 2021-01-23 Lancelot SIX * Makefile.in (SELFTESTS_SRCS): Add diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c index 115f078787b..b66b89c7e56 100644 --- a/gdb/gdbtypes.c +++ b/gdb/gdbtypes.c @@ -5518,7 +5518,7 @@ copy_type_recursive (struct objfile *objfile, copy the entire thing and then update specific fields as needed. */ *TYPE_MAIN_TYPE (new_type) = *TYPE_MAIN_TYPE (type); - new_type->set_owner (type->arch ()); + new_type->set_owner (get_type_arch (type)); if (type->name ()) new_type->set_name (xstrdup (type->name ())); diff --git a/gdb/gdbtypes.h b/gdb/gdbtypes.h index 30e8ac54260..2e0b01a37df 100644 --- a/gdb/gdbtypes.h +++ b/gdb/gdbtypes.h @@ -1245,6 +1245,8 @@ struct type /* Set the owner of the type to be OBJFILE. */ void set_owner (objfile *objfile) { + gdb_assert (objfile != nullptr); + this->main_type->m_owner.objfile = objfile; this->main_type->m_flag_objfile_owned = true; } @@ -1252,6 +1254,8 @@ struct type /* Set the owner of the type to be ARCH. */ void set_owner (gdbarch *arch) { + gdb_assert (arch != nullptr); + this->main_type->m_owner.gdbarch = arch; this->main_type->m_flag_objfile_owned = false; } -- 2.30.2