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

[Simon Marchi <simark@simark.ca>]

On 2021-11-04 5:04 p.m., Bruno Larsen via Gdb-patches 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  | 13 +++++++++++++
>  gdb/testsuite/gdb.cp/ambiguous.exp |  7 +++++++
>  gdb/valops.c                       | 26 ++++++++++++++++++++++++++
>  3 files changed, 46 insertions(+)
>
> diff --git a/gdb/testsuite/gdb.cp/ambiguous.cc b/gdb/testsuite/gdb.cp/ambiguous.cc
> index a55686547f2..b2be7297b28 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,11 @@ public:
>    int y;
>  };
>
> +class A4 {
> +public:
> +    [[no_unique_address]] empty x;

In theory, no_unique_address appeared in C++20.  Having
[[no_unique_address]] in the source code means that compiling this file
with an older compiler that doesn't know about it, or in a standard mode
where it isn't recognized, will lead to the entire test being skipped.
In practice, it looks like older-but-not-too-old g++s cope well with it.
For example, g++ 7 prints a warning:

    $ g++-7 ambiguous.cc
    ambiguous.cc:23:33: warning: ‘no_unique_address’ attribute directive ignored [-Wattributes]
         [[no_unique_address]] empty x;
                                     ^

... but since we disable all warnings in that tests for g++ < 10, then
it still compiles.  The no_unique_address attribute has no effect,  but
that's fine.

With older g++s, like 4.8, it fails though:

    $ make check TESTS="gdb.cp/ambiguous.exp" RUNTESTFLAGS='CC_FOR_TARGET=gcc-4.8 CXX_FOR_TARGET=g++-4.8'
    ...
    Running /home/smarchi/src/binutils-gdb/gdb/testsuite/gdb.cp/ambiguous.exp ...
    Gdb compile failed, /home/smarchi/src/binutils-gdb/gdb/testsuite/gdb.cp/ambiguous.cc:23:5: error: expected unqualified-id before '[' token
         [[no_unique_address]] empty x;
         ^

I don't know if others care about this test running with older
compilers, but if so we could make that case optional with some
preprocessor conditionals.

> diff --git a/gdb/testsuite/gdb.cp/ambiguous.exp b/gdb/testsuite/gdb.cp/ambiguous.exp
> index 008898c5818..68b82d45b68 100644
> --- a/gdb/testsuite/gdb.cp/ambiguous.exp
> +++ b/gdb/testsuite/gdb.cp/ambiguous.exp
> @@ -264,3 +264,10 @@ 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}"
> +
> +#unique_ptr is a weird edge-case that interacts differently with the
> +#ambiguity detection, so we should test it directly
> +test_ambiguous "je.x" "x" "JE" {
> +    "'int A1::x' (JE -> A1)"
> +    "'empty A4::x' (JE -> A4)"
> +}

I think the comment needs to be re-worded to not talk about unique_ptr,
but talk about ambiguous fields that have the same address due to
no_unique_address.  Make sure to use a space at the beginning of the
line and a period at the end (like the other comments).

> diff --git a/gdb/valops.c b/gdb/valops.c
> index 9787cdbb513..75b732af62b 100644
> --- a/gdb/valops.c
> +++ b/gdb/valops.c
> @@ -1962,6 +1962,32 @@ 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

Two spaces after period.

> +	     different. This is not necessarily complete, but a situation where
> +	     this assumption is incorrect is unlikely*/

Period and two spaces:

 ... unlikely.  */

> +	  {
> +	      bool ambiguous = false, insert = true;
> +	      for(auto finds: m_fields){

Space before opening parentheses and after closing.

Iterate using const-reference, to avoid copying the items as we are
iterating.  I would also use the type name instead of auto, for clarity,
but I am ok with either.  Lastly, the variable name "finds" is a bit
strange, or I don't understand what you mean by it.

> +		  if(finds.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 */

First letter capital, period + two spaces at the end.

There are two lines above that have more than 8 spaces, that should be
replaced with a tab.  Most editors have a way to configure whitespaces
to do that automatically.

> +		      insert = false;

Since this path makes it so that we'll definitely not insert, we can
break the iteration.

Simon