Reject ambiguous C++ field accesses (PR exp/26602)

The gdb.cp/ambiguous.exp testcase had been disabled for many years,
but recently it was re-enabled.  However, it is failing everywhere.
That is because it is testing an old feature that is gone from GDB.

The testcase is expecting to see an ambiguous field warning, like:

 # X is derived from A1 and A2; both A1 and A2 have a member 'x'
 send_gdb "print x.x\n"
 gdb_expect {
    -re "warning: x ambiguous; using X::A2::x. Use a cast to disambiguate.\r\n\\$\[0-9\]* = \[-\]*\[0-9\]*\r\n$gdb_prompt $" {
	pass "print x.x"
    }
    -re "warning: x ambiguous; using X::A1::x. Use a cast to disambiguate.\r\n\\$\[0-9\]* = \[-\]*\[0-9\]*\r\n$gdb_prompt $" {
	pass "print x.x"
    }
    -re ".*$gdb_prompt $" { fail "print x.x" }
    timeout { fail "(timeout) print x.x" }
  }

However, GDB just accesses one of the candidates without warning or
error:

 print x.x
 $1 = 1431655296
 (gdb) FAIL: gdb.cp/ambiguous.exp: print x.x

(The weird number is because the testcase does not initialize the
variables.)

The testcase come in originally with the big HP merge:

 +Sun Jan 10 23:44:11 1999  David Taylor  <taylor@texas.cygnus.com>
 +
 +
 +       The following files are part of the HP merge; some had longer
 +       names at HP, but have been renamed to be no more than 14
 +       characters in length.

Looking at the tree back then, we find that warning:

 /* Helper function used by value_struct_elt to recurse through baseclasses.
    Look for a field NAME in ARG1. Adjust the address of ARG1 by OFFSET bytes,
    and search in it assuming it has (class) type TYPE.
    If found, return value, else return NULL.

    If LOOKING_FOR_BASECLASS, then instead of looking for struct fields,
    look for a baseclass named NAME.  */

 static value_ptr
 search_struct_field (name, arg1, offset, type, looking_for_baseclass)
      char *name;
      register value_ptr arg1;
      int offset;
      register struct type *type;
      int looking_for_baseclass;
 {
   int found = 0;
   char found_class[1024];
   value_ptr v;
   struct type *vbase = NULL;

   found_class[0] = '\000';

   v = search_struct_field_aux (name, arg1, offset, type, looking_for_baseclass, &found, found_class, &vbase);
   if (found > 1)
     warning ("%s ambiguous; using %s::%s. Use a cast to disambiguate.",
              name, found_class, name);

   return v;
 }


However, in current GDB, search_struct_field does not handle the
ambiguous field case, nor is that warning found anywhere.  Somehow it
got lost over the years.  That seems like a regression, because the
compiler (as per language rules) rejects the ambiguous accesses as
well.  E.g.:

 gdb.cp/ambiguous.cc:98:5: error: request for member 'x' is ambiguous
    98 |   x.x = 1;
       |     ^
 gdb.cp/ambiguous.cc:10:7: note: candidates are: 'int A2::x'
    10 |   int x;
       |       ^
 gdb.cp/ambiguous.cc:4:7: note:                 'int A1::x'
     4 |   int x;
       |       ^


This patch restores the feature, though implemented differently and
with better user experience, IMHO.  An ambiguous access is now an
error instead of a warning, and also GDB shows you all the candidates,
like:

 (gdb) print x.x
 Request for member 'x' is ambiguous in type 'X'. Candidates are:
   'int A1::x' (X -> A1)
   'int A2::x' (X -> A2)
 (gdb) print j.x
 Request for member 'x' is ambiguous in type 'J'. Candidates are:
   'int A1::x' (J -> K -> A1)
   'int A1::x' (J -> L -> A1)

Users can then fix their commands by casting or by specifying the
baseclass explicitly, like:

 (gdb) p x.A1::x
 $1 = 1
 (gdb) p x.A2::x
 $2 = 2
 (gdb) p ((A1) x).x
 $3 = 1
 (gdb) p ((A2) x).x
 $4 = 2
 (gdb) p j.K::x
 $12 = 1
 (gdb) p j.L::x
 $13 = 2
 (gdb) p j.A1::x
 base class 'A1' is ambiguous in type 'J'

The last error I've not touched; could be improved to also list the
baseclass candidates.

The showing the class "path" for each candidate was inspired by GCC's
output when you try an ambiguous cast:

  gdb.cp/ambiguous.cc:161:8: error: ambiguous conversion from derived class 'const JVA1' to base class 'const A1':
      class JVA1 -> class KV -> class A1
      class JVA1 -> class A1
    (A1) jva1;
	 ^~~~

I did not include the "class" word as it seemed unnecessarily
repetitive, but I can include it if people prefer it:

 (gdb) print j.x
 Request for member 'x' is ambiguous in type 'J'. Candidates are:
   'int A1::x' (class J -> class K -> class A1)
   'int A1::x' (class J -> class L -> class A1)

The testcase is adjusted accordingly.  I also took the chance to
modernize it at the same time.

Also, as mentioned above, the testcase doesn't currently initialize
the tested variables.  This patch inializes them all, giving each
field a distinct value, so that we can be sure that GDB is accessing
the right fields / offsets.  The testcase is extended accordingly.

Unfortunately, this exposes a bug, not addressed in this patch.  The
bug is around a class that inherits from A1 directly and also inherits
from two other distinct base classes that inherit virtually from A1 in
turn:

 print jva1.KV::x
 $51 = 1431665544
 (gdb) FAIL: gdb.cp/ambiguous.exp: all fields: print jva1.KV::x
 print jva1.KV::y
 $52 = 21845
 (gdb) FAIL: gdb.cp/ambiguous.exp: all fields: print jva1.KV::y

 (gdb) print /x (KV)jva1
 $4 = {<A1> = <invalid address>, _vptr.KV = 0x555555557b88 <vtable for JVA1+24>, i = 0x457}
 (gdb) print /x (A1)(KV)jva1
 Cannot access memory at address 0x0

Since that's an orthogonal issue, I filed PR c++/26550 and kfailed the
tests that fail because of it.

gdb/ChangeLog:

	PR exp/26602
	* valops.c (struct struct_field_searcher): New.
	(update_search_result): Rename to ...
	(struct_field_searcher::update_result): ... this.  Simplify
	prototype.  Record all found fields.
	(do_search_struct_field): Rename to ...
	(struct_field_searcher::search): ... this.  Simplify prototype.
	Maintain stack of visited baseclass path.  Call update_result for
	fields too.  Keep searching fields in baseclasses instead of
	stopping at the first found field.
	(search_struct_field): Use struct_field_searcher.  When looking
	for fields, report ambiguous access attempts.

gdb/testsuite/ChangeLog:

	PR exp/26602
	PR c++/26550
	* gdb.cp/ambiguous.cc (marker1): Delete.
	(main): Initialize all the fields of the locals.  Replace marker1
	call with a "set breakpoint here" marker.
	* gdb.cp/ambiguous.exp: Modernize.  Use gdb_continue_to_breakpoint
	instead of running to marker1.  Add tests printing all the
	variables and all the fields of the variables.
	(test_ambiguous): New proc, expecting the new GDB output when a
	field access is ambiguous.  Change all "warning: X ambiguous"
	tests to use it.

1 files changed, 164 insertions, 63 deletions

diff --git a/gdb/valops.c b/gdb/valops.c
index 4906b3b..6549613 100644
--- a/gdb/valops.c
+++ b/gdb/valops.c
@@ -1766,54 +1766,132 @@ typecmp (int staticp, int varargs, int nargs,
   return i + 1;
 }
 
-/* Helper class for do_search_struct_field that updates *RESULT_PTR
-   and *LAST_BOFFSET, and possibly throws an exception if the field
-   search has yielded ambiguous results.  */
+/* Helper class for search_struct_field that keeps track of found
+   results and possibly throws an exception if the search yields
+   ambiguous results.  See search_struct_field for description of
+   LOOKING_FOR_BASECLASS.  */
 
-static void
-update_search_result (struct value **result_ptr, struct value *v,
-		      LONGEST *last_boffset, LONGEST boffset,
-		      const char *name, struct type *type)
+struct struct_field_searcher
+{
+  /* A found field.  */
+  struct found_field
+  {
+    /* Path to the structure where the field was found.  */
+    std::vector<struct type *> path;
+
+    /* The field found.  */
+    struct value *field_value;
+  };
+
+  /* See corresponding fields for description of parameters.  */
+  struct_field_searcher (const char *name,
+			 struct type *outermost_type,
+			 bool looking_for_baseclass)
+    : m_name (name),
+      m_looking_for_baseclass (looking_for_baseclass),
+      m_outermost_type (outermost_type)
+  {
+  }
+
+  /* The search entry point.  If LOOKING_FOR_BASECLASS is true and the
+     base class search yields ambiguous results, this throws an
+     exception.  If LOOKING_FOR_BASECLASS is false, the found fields
+     are accumulated and the caller (search_struct_field) takes care
+     of throwing an error if the field search yields ambiguous
+     results.  The latter is done that way so that the error message
+     can include a list of all the found candidates.  */
+  void search (struct value *arg, LONGEST offset, struct type *type);
+
+  const std::vector<found_field> &fields ()
+  {
+    return m_fields;
+  }
+
+  struct value *baseclass ()
+  {
+    return m_baseclass;
+  }
+
+private:
+  /* Update results to include V, a found field/baseclass.  */
+  void update_result (struct value *v, LONGEST boffset);
+
+  /* The name of the field/baseclass we're searching for.  */
+  const char *m_name;
+
+  /* Whether we're looking for a baseclass, or a field.  */
+  const bool m_looking_for_baseclass;
+
+  /* The offset of the baseclass containing the field/baseclass we
+     last recorded.  */
+  LONGEST m_last_boffset = 0;
+
+  /* If looking for a baseclass, then the result is stored here.  */
+  struct value *m_baseclass = nullptr;
+
+  /* When looking for fields, the found candidates are stored
+     here.  */
+  std::vector<found_field> m_fields;
+
+  /* The type of the initial type passed to search_struct_field; this
+     is used for error reporting when the lookup is ambiguous.  */
+  struct type *m_outermost_type;
+
+  /* The full path to the struct being inspected.  E.g. for field 'x'
+     defined in class B inherited by class A, we have A and B pushed
+     on the path.  */
+  std::vector <struct type *> m_struct_path;
+};
+
+void
+struct_field_searcher::update_result (struct value *v, LONGEST boffset)
 {
   if (v != NULL)
     {
-      if (*result_ptr != NULL
-	  /* The result is not ambiguous if all the classes that are
-	     found occupy the same space.  */
-	  && *last_boffset != boffset)
-	error (_("base class '%s' is ambiguous in type '%s'"),
-	       name, TYPE_SAFE_NAME (type));
-      *result_ptr = v;
-      *last_boffset = boffset;
+      if (m_looking_for_baseclass)
+	{
+	  if (m_baseclass != nullptr
+	      /* The result is not ambiguous if all the classes that are
+		 found occupy the same space.  */
+	      && m_last_boffset != boffset)
+	    error (_("base class '%s' is ambiguous in type '%s'"),
+		   m_name, TYPE_SAFE_NAME (m_outermost_type));
+
+	  m_baseclass = v;
+	  m_last_boffset = boffset;
+	}
+      else
+	{
+	  /* The field is not ambiguous if it occupies the same
+	     space.  */
+	  if (m_fields.empty () || m_last_boffset != boffset)
+	    m_fields.push_back ({m_struct_path, v});
+	}
     }
 }
 
 /* A helper for search_struct_field.  This does all the work; most
-   arguments are as passed to search_struct_field.  The result is
-   stored in *RESULT_PTR, which must be initialized to NULL.
-   OUTERMOST_TYPE is the type of the initial type passed to
-   search_struct_field; this is used for error reporting when the
-   lookup is ambiguous.  */
+   arguments are as passed to search_struct_field.  */
 
-static void
-do_search_struct_field (const char *name, struct value *arg1, LONGEST offset,
-			struct type *type, int looking_for_baseclass,
-			struct value **result_ptr,
-			LONGEST *last_boffset,
-			struct type *outermost_type)
+void
+struct_field_searcher::search (struct value *arg1, LONGEST offset,
+			       struct type *type)
 {
   int i;
   int nbases;
 
+  m_struct_path.push_back (type);
+  SCOPE_EXIT { m_struct_path.pop_back (); };
+
   type = check_typedef (type);
   nbases = TYPE_N_BASECLASSES (type);
 
-  if (!looking_for_baseclass)
+  if (!m_looking_for_baseclass)
     for (i = type->num_fields () - 1; i >= nbases; i--)
       {
 	const char *t_field_name = TYPE_FIELD_NAME (type, i);
 
-	if (t_field_name && (strcmp_iw (t_field_name, name) == 0))
+	if (t_field_name && (strcmp_iw (t_field_name, m_name) == 0))
 	  {
 	    struct value *v;
 
@@ -1821,7 +1899,8 @@ do_search_struct_field (const char *name, struct value *arg1, LONGEST offset,
 	      v = value_static_field (type, i);
 	    else
 	      v = value_primitive_field (arg1, offset, i, type);
-	    *result_ptr = v;
+
+	    update_result (v, offset);
 	    return;
 	  }
 
@@ -1845,7 +1924,6 @@ do_search_struct_field (const char *name, struct value *arg1, LONGEST offset,
 		   represented as a struct, with a member for each
 		   <variant field>.  */
 
-		struct value *v = NULL;
 		LONGEST new_offset = offset;
 
 		/* This is pretty gross.  In G++, the offset in an
@@ -1859,16 +1937,7 @@ do_search_struct_field (const char *name, struct value *arg1, LONGEST offset,
 			&& TYPE_FIELD_BITPOS (field_type, 0) == 0))
 		  new_offset += TYPE_FIELD_BITPOS (type, i) / 8;
 
-		do_search_struct_field (name, arg1, new_offset, 
-					field_type,
-					looking_for_baseclass, &v,
-					last_boffset,
-					outermost_type);
-		if (v)
-		  {
-		    *result_ptr = v;
-		    return;
-		  }
+		search (arg1, new_offset, field_type);
 	      }
 	  }
       }
@@ -1880,10 +1949,10 @@ do_search_struct_field (const char *name, struct value *arg1, LONGEST offset,
       /* If we are looking for baseclasses, this is what we get when
          we hit them.  But it could happen that the base part's member
          name is not yet filled in.  */
-      int found_baseclass = (looking_for_baseclass
+      int found_baseclass = (m_looking_for_baseclass
 			     && TYPE_BASECLASS_NAME (type, i) != NULL
-			     && (strcmp_iw (name, 
-					    TYPE_BASECLASS_NAME (type, 
+			     && (strcmp_iw (m_name,
+					    TYPE_BASECLASS_NAME (type,
 								 i)) == 0));
       LONGEST boffset = value_embedded_offset (arg1) + offset;
 
@@ -1924,28 +1993,17 @@ do_search_struct_field (const char *name, struct value *arg1, LONGEST offset,
 	  if (found_baseclass)
 	    v = v2;
 	  else
-	    {
-	      do_search_struct_field (name, v2, 0,
-				      TYPE_BASECLASS (type, i),
-				      looking_for_baseclass,
-				      result_ptr, last_boffset,
-				      outermost_type);
-	    }
+	    search (v2, 0, TYPE_BASECLASS (type, i));
 	}
       else if (found_baseclass)
 	v = value_primitive_field (arg1, offset, i, type);
       else
 	{
-	  do_search_struct_field (name, arg1,
-				  offset + TYPE_BASECLASS_BITPOS (type, 
-								  i) / 8,
-				  basetype, looking_for_baseclass,
-				  result_ptr, last_boffset,
-				  outermost_type);
+	  search (arg1, offset + TYPE_BASECLASS_BITPOS (type, i) / 8,
+		  basetype);
 	}
 
-      update_search_result (result_ptr, v, last_boffset,
-			    boffset, name, outermost_type);
+      update_result (v, boffset);
     }
 }
 
@@ -1960,12 +2018,55 @@ static struct value *
 search_struct_field (const char *name, struct value *arg1,
 		     struct type *type, int looking_for_baseclass)
 {
-  struct value *result = NULL;
-  LONGEST boffset = 0;
+  struct_field_searcher searcher (name, type, looking_for_baseclass);
 
-  do_search_struct_field (name, arg1, 0, type, looking_for_baseclass,
-			  &result, &boffset, type);
-  return result;
+  searcher.search (arg1, 0, type);
+
+  if (!looking_for_baseclass)
+    {
+      const auto &fields = searcher.fields ();
+
+      if (fields.empty ())
+	return nullptr;
+      else if (fields.size () == 1)
+	return fields[0].field_value;
+      else
+	{
+	  std::string candidates;
+
+	  for (auto &&candidate : fields)
+	    {
+	      gdb_assert (!candidate.path.empty ());
+
+	      struct type *field_type = value_type (candidate.field_value);
+	      struct type *struct_type = candidate.path.back ();
+
+	      std::string path;
+	      bool first = true;
+	      for (struct type *t : candidate.path)
+		{
+		  if (first)
+		    first = false;
+		  else
+		    path += " -> ";
+		  path += t->name ();
+		}
+
+	      candidates += string_printf ("\n  '%s %s::%s' (%s)",
+					   TYPE_SAFE_NAME (field_type),
+					   TYPE_SAFE_NAME (struct_type),
+					   name,
+					   path.c_str ());
+	    }
+
+	  error (_("Request for member '%s' is ambiguous in type '%s'."
+		   " Candidates are:%s"),
+		 name, TYPE_SAFE_NAME (type),
+		 candidates.c_str ());
+	}
+    }
+  else
+    return searcher.baseclass ();
 }
 
 /* Helper function used by value_struct_elt to recurse through