[PATCH] Special case NULL pointers in dynamic type resolution

[PATCH] Special case NULL pointers in dynamic type resolution

[Tom Tromey]

commit f18fc7e5 ("gdb, types: Resolve pointer types dynamically")
caused a regression on a test case in the AdaCore internal test suite.

The issue here is that gdb would try to resolve the type of a dynamic
pointer that happened to be NULL.  In this case, the "Location address
is not set." error would end up being thrown from the DWARF expression
evaluator.

I think it makes more sense to special-case NULL pointers and not try
to resolve their target type, as that type can't really be accessed
anyway.

This patch implements this idea, and also adds the missing Ada test
case.
---
 gdb/gdbtypes.c                                | 13 +++++--
 gdb/testsuite/gdb.ada/recursive-variant.exp   | 31 +++++++++++++++
 .../gdb.ada/recursive-variant/main.adb        | 38 +++++++++++++++++++
 gdb/testsuite/gdb.cp/vla-cxx.exp              |  4 +-
 4 files changed, 80 insertions(+), 6 deletions(-)
 create mode 100644 gdb/testsuite/gdb.ada/recursive-variant.exp
 create mode 100644 gdb/testsuite/gdb.ada/recursive-variant/main.adb

diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
index dcd7321d979..c46bf3ae39e 100644
--- a/gdb/gdbtypes.c
+++ b/gdb/gdbtypes.c
@@ -2809,10 +2809,15 @@ resolve_dynamic_type_internal (struct type *type,
 	      pinfo.addr = read_memory_typed_address (addr_stack->addr, type);
 	    pinfo.next = addr_stack;
 
-	    resolved_type = copy_type (type);
-	    resolved_type->set_target_type
-	      (resolve_dynamic_type_internal (type->target_type (),
-					      &pinfo, frame, top_level));
+	    /* Special case a NULL pointer here -- we don't want to
+	       dereference it.  */
+	    if (pinfo.addr != 0)
+	      {
+		resolved_type = copy_type (type);
+		resolved_type->set_target_type
+		  (resolve_dynamic_type_internal (type->target_type (),
+						  &pinfo, frame, true));
+	      }
 	    break;
 	  }
 
diff --git a/gdb/testsuite/gdb.ada/recursive-variant.exp b/gdb/testsuite/gdb.ada/recursive-variant.exp
new file mode 100644
index 00000000000..ea13f83f2d5
--- /dev/null
+++ b/gdb/testsuite/gdb.ada/recursive-variant.exp
@@ -0,0 +1,31 @@
+# Copyright 2024 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/>.
+
+load_lib "ada.exp"
+
+require allow_ada_tests
+
+standard_ada_testfile main
+
+if {[gdb_compile_ada "${srcfile}" "${binfile}" executable debug] != ""} {
+    return -1
+}
+
+clean_restart ${testfile}
+
+set bp_location [gdb_get_line_number "STOP" ${testdir}/main.adb]
+runto "main.adb:$bp_location"
+
+gdb_test "print *instance" [string_to_regexp "= (x => false, link => 0x0)"]
diff --git a/gdb/testsuite/gdb.ada/recursive-variant/main.adb b/gdb/testsuite/gdb.ada/recursive-variant/main.adb
new file mode 100644
index 00000000000..93b47e5002a
--- /dev/null
+++ b/gdb/testsuite/gdb.ada/recursive-variant/main.adb
@@ -0,0 +1,38 @@
+--  Copyright 2024 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/>.
+
+procedure Main is
+
+   type Rec_Type;
+
+   type Rec_Type_Ref is access all Rec_Type;
+
+   type Rec_Type (X : Boolean) is record
+      Link : Rec_Type_Ref;
+      case X is
+         when True =>
+            Value : Integer;
+         when False =>
+            null;
+      end case;
+   end record;
+
+   Instance : Rec_Type_Ref := new Rec_Type'(X => False, Link => null);
+
+begin
+
+   null; -- STOP
+
+end Main;
diff --git a/gdb/testsuite/gdb.cp/vla-cxx.exp b/gdb/testsuite/gdb.cp/vla-cxx.exp
index 0033a968268..bf3ca792d58 100644
--- a/gdb/testsuite/gdb.cp/vla-cxx.exp
+++ b/gdb/testsuite/gdb.cp/vla-cxx.exp
@@ -26,10 +26,10 @@ if ![runto_main] {
 gdb_breakpoint [gdb_get_line_number "Before pointer assignment"]
 gdb_continue_to_breakpoint "Before pointer assignment"
 
-gdb_test "ptype ptr" "= int \\(\\*\\)\\\[3\\\]" \
+gdb_test "ptype ptr" "= int \\(\\*\\)\\\[variable length\\\]" \
     "ptype ptr, before pointer assignment"
 
-gdb_test "print ptr" "= \\(int \\(\\*\\)\\\[3\\\]\\) 0x0" \
+gdb_test "print ptr" "= \\(int \\(\\*\\)\\\[variable length\\\]\\) 0x0" \
     "print ptr, before pointer assignment"
 
 gdb_test "print *ptr" "Cannot access memory at address 0x0" \
-- 
2.43.0

Re: [PATCH] Special case NULL pointers in dynamic type resolution

[Tom Tromey]

>>>>> "Tom" == Tom Tromey <tromey@adacore.com> writes:

Tom> commit f18fc7e5 ("gdb, types: Resolve pointer types dynamically")
Tom> caused a regression on a test case in the AdaCore internal test suite.

Tom> The issue here is that gdb would try to resolve the type of a dynamic
Tom> pointer that happened to be NULL.  In this case, the "Location address
Tom> is not set." error would end up being thrown from the DWARF expression
Tom> evaluator.

Tom> I think it makes more sense to special-case NULL pointers and not try
Tom> to resolve their target type, as that type can't really be accessed
Tom> anyway.

Tom> This patch implements this idea, and also adds the missing Ada test
Tom> case.

I'm checking this in.

Tom