[PING] [PATCH v2] PR gdb/28480: Improve ambiguous member detection

[Bruno Larsen]

Ping

On 11/8/21 15:27, Bruno Larsen wrote:
> Basic ambiguity detection assumes that when 2 fields with the same name
> have the same boffset, it must be an unambiguous request. This is not
> always correct. Consider the following code:
> 
> class empty { };
> 
> class A {
> public:
>    [[no_unique_address]] empty e;
> };
> 
> class B {
> public:
>    int e;
> };
> 
> class C: public A, public B { };
> 
> if we tried to use c.e in code, the compiler would warn of an ambiguity,
> however, since A::e does not demand an unique address, it gets the same
> address (and thus boffset) of the members, making A::e and B::e have the
> same address. however, "print c.e" would fail to report the ambiguity,
> and would instead print it as an empty class (first path found).
> 
> The new code solves this by checking for other found_fields that have
> different m_struct_path.back() (final class that the member was found
> in), despite having the same boffset.
> 
> The testcase gdb.cp/ambiguous.exp was also changed to test for this
> behavior.
> ---
>   gdb/testsuite/gdb.cp/ambiguous.cc  | 19 +++++++++++++++++++
>   gdb/testsuite/gdb.cp/ambiguous.exp | 10 ++++++++++
>   gdb/valops.c                       | 27 +++++++++++++++++++++++++++
>   3 files changed, 56 insertions(+)
> 
> diff --git a/gdb/testsuite/gdb.cp/ambiguous.cc b/gdb/testsuite/gdb.cp/ambiguous.cc
> index a55686547f2..af2198dcfbc 100644
> --- a/gdb/testsuite/gdb.cp/ambiguous.cc
> +++ b/gdb/testsuite/gdb.cp/ambiguous.cc
> @@ -1,3 +1,4 @@
> +class empty { };
>   
>   class A1 {
>   public:
> @@ -17,6 +18,17 @@ public:
>     int y;
>   };
>   
> +#if !defined (__GNUC__) || __GNUC__ > 7
> +# define NO_UNIQUE_ADDRESS [[no_unique_address]]
> +#else
> +# define NO_UNIQUE_ADDRESS
> +#endif
> +
> +class A4 {
> +public:
> +    NO_UNIQUE_ADDRESS empty x;
> +};
> +
>   class X : public A1, public A2 {
>   public:
>     int z;
> @@ -77,6 +89,10 @@ public:
>     int jva1v;
>   };
>   
> +class JE : public A1, public A4 {
> +public:
> +};
> +
>   int main()
>   {
>     A1 a1;
> @@ -92,6 +108,7 @@ int main()
>     JVA1 jva1;
>     JVA2 jva2;
>     JVA1V jva1v;
> +  JE je;
>     
>     int i;
>   
> @@ -173,5 +190,7 @@ int main()
>     jva1v.i = 4;
>     jva1v.jva1v = 5;
>   
> +  je.A1::x = 1;
> +
>     return 0; /* set breakpoint here */
>   }
> diff --git a/gdb/testsuite/gdb.cp/ambiguous.exp b/gdb/testsuite/gdb.cp/ambiguous.exp
> index 008898c5818..a2a7b02b113 100644
> --- a/gdb/testsuite/gdb.cp/ambiguous.exp
> +++ b/gdb/testsuite/gdb.cp/ambiguous.exp
> @@ -264,3 +264,13 @@ gdb_test "print (A1)(KV)jva1" " = \{x = 3, y = 4\}"
>   # JVA1V is derived from A1; A1 is a virtual base indirectly
>   # and also directly; must not report ambiguity when a JVA1V is cast to an A1.
>   gdb_test "print (A1)jva1v" " = {x = 1, y = 2}"
> +
> +# C++20 introduced a way to have ambiguous fields with the same boffset.
> +# This class explicitly tests for that.
> +# if this is tested with a compiler that can't handle [[no_unique_address]]
> +# the code should still correctly identify the ambiguity because of
> +# different boffsets.
> +test_ambiguous "je.x" "x" "JE" {
> +    "'int A1::x' (JE -> A1)"
> +    "'empty A4::x' (JE -> A4)"
> +}
> diff --git a/gdb/valops.c b/gdb/valops.c
> index 9787cdbb513..2989a93df1a 100644
> --- a/gdb/valops.c
> +++ b/gdb/valops.c
> @@ -1962,6 +1962,33 @@ struct_field_searcher::update_result (struct value *v, LONGEST boffset)
>   	     space.  */
>   	  if (m_fields.empty () || m_last_boffset != boffset)
>   	    m_fields.push_back ({m_struct_path, v});
> +	  else
> +	  /* Some fields may occupy the same space and still be ambiguous.
> +	     This happens when [[no_unique_address]] is used by a member
> +	     of the class.  We assume that this only happens when the types are
> +	     different.  This is not necessarily complete, but a situation where
> +	     this assumption is incorrect is currently (2021) impossible.  */
> +	  {
> +	      bool ambiguous = false, insert = true;
> +	      for (const found_field& field: m_fields) {
> +		  if(field.path.back () != m_struct_path.back ())
> +		  {
> +		      /* Same boffset points to members of different classes.
> +			 We have found an ambiguity and should record it.  */
> +		      ambiguous = true;
> +		  }
> +		  else
> +		  {
> +		      /* We don't need to insert this value again, because a
> +			 non-ambiguous path already leads to it.  */
> +		      insert = false;
> +		      break;
> +		  }
> +	      }
> +	      if (ambiguous && insert) {
> +		  m_fields.push_back ({m_struct_path, v});
> +	      }
> +	  }
>   	}
>       }
>   }
> 


-- 
Cheers!
Bruno Larsen