On 10/12/23 18:05, Nathaniel Shead wrote:
> On Thu, Oct 12, 2023 at 04:24:00PM -0400, Jason Merrill wrote:
>> On 10/12/23 04:53, Nathaniel Shead wrote:
>>> On Wed, Oct 11, 2023 at 12:48:12AM +1100, Nathaniel Shead wrote:
>>>> On Mon, Oct 09, 2023 at 04:46:46PM -0400, Jason Merrill wrote:
>>>>> On 10/8/23 21:03, Nathaniel Shead wrote:
>>>>>> Ping for https://gcc.gnu.org/pipermail/gcc-patches/2023-September/631203.html
>>>>>>
>>>>>> +	  && (TREE_CODE (t) == MODIFY_EXPR
>>>>>> +	      /* Also check if initializations have implicit change of active
>>>>>> +		 member earlier up the access chain.  */
>>>>>> +	      || !refs->is_empty())
>>>>>
>>>>> I'm not sure what the cumulative point of these two tests is.  TREE_CODE (t)
>>>>> will be either MODIFY_EXPR or INIT_EXPR, and either should be OK.
>>>>>
>>>>> As I understand it, the problematic case is something like
>>>>> constexpr-union2.C, where we're also looking at a MODIFY_EXPR.  So what is
>>>>> this check doing?
>>>>
>>>> The reasoning was to correctly handle cases like the the following (in
>>>> constexpr-union6.C):
>>>>
>>>>     constexpr int test1() {
>>>>       U u {};
>>>>       std::construct_at(&u.s, S{ 1, 2 });
>>>>       return u.s.b;
>>>>     }
>>>>     static_assert(test1() == 2);
>>>>
>>>> The initialisation of &u.s here is not a member access expression within
>>>> the call to std::construct_at, since it's just a pointer, but this code
>>>> is still legal; in general, an INIT_EXPR to initialise a union member
>>>> should always be OK (I believe?), hence constraining to just
>>>> MODIFY_EXPR.
>>>>
>>>> However, just that would then (incorrectly) allow all the following
>>>> cases in that test to compile, such as
>>>>
>>>>     constexpr int test2() {
>>>>       U u {};
>>>>       int* p = &u.s.b;
>>>>       std::construct_at(p, 5);
>>>>       return u.s.b;
>>>>     }
>>>>     constexpr int x2 = test2();
>>>>
>>>> since the INIT_EXPR is really only initialising 'b', but the implicit
>>>> "modification" of active member to 'u.s' is illegal.
>>>>
>>>> Maybe a better way of expressing this condition would be something like
>>>> this?
>>>>
>>>>     /* An INIT_EXPR of the last member in an access chain is always OK,
>>>>        but still check implicit change of members earlier on; see
>>>>        cpp2a/constexpr-union6.C.  */
>>>>     && !(TREE_CODE (t) == INIT_EXPR && refs->is_empty ())
>>>>
>>>> Otherwise I'll see if I can rework some of the other conditions instead.
>>>>
>>>>> Incidentally, I think constexpr-union6.C could use a test where we pass &u.s
>>>>> to a function other than construct_at, and then try (and fail) to assign to
>>>>> the b member from that function.
>>>>>
>>>>> Jason
>>>>>
>>>>
>>>> Sounds good; I've added the following test:
>>>>
>>>>     constexpr void foo(S* s) {
>>>>       s->b = 10;  // { dg-error "accessing .U::s. member instead of initialized .U::k." }
>>>>     }
>>>>     constexpr int test3() {
>>>>       U u {};
>>>>       foo(&u.s);  // { dg-message "in .constexpr. expansion" }
>>>>       return u.s.b;
>>>>     }
>>>>     constexpr int x3 = test3();  // { dg-message "in .constexpr. expansion" }
>>>>
>>>> Incidentally I found this particular example caused a very unhelpful
>>>> error + ICE due to reporting that S could not be value-initialized in
>>>> the current version of the patch. The updated version below fixes that
>>>> by using 'build_zero_init' instead -- is this an appropriate choice
>>>> here?
>>>>
>>>> A similar (but unrelated) issue is with e.g.
>>>>     struct S { const int a; int b; };
>>>>     union U { int k; S s; };
>>>>
>>>>     constexpr int test() {
>>>>       U u {};
>>>>       return u.s.b;
>>>>     }
>>>>     constexpr int x = test();
>>>>
>>>> giving me this pretty unhelpful error message:
>>>>
>>>> /home/ns/main.cpp:8:23:   in ‘constexpr’ expansion of ‘test()’
>>>> /home/ns/main.cpp:6:12: error: use of deleted function ‘S::S()’
>>>>       6 |   return u.s.b;
>>>>         |          ~~^
>>>> /home/ns/main.cpp:1:8: note: ‘S::S()’ is implicitly deleted because the default definition would be ill-formed:
>>>>       1 | struct S { const int a; int b; };
>>>>         |        ^
>>>> /home/ns/main.cpp:1:8: error: uninitialised const member in ‘struct S’
>>>> /home/ns/main.cpp:1:22: note: ‘const int S::a’ should be initialised
>>>>       1 | struct S { const int a; int b; };
>>>>         |                      ^
>>>> /home/ns/main.cpp:8:23:   in ‘constexpr’ expansion of ‘test()’
>>>> /home/ns/main.cpp:6:12: error: use of deleted function ‘S::S()’
>>>>       6 |   return u.s.b;
>>>>         |          ~~^
>>>> /home/ns/main.cpp:8:23:   in ‘constexpr’ expansion of ‘test()’
>>>> /home/ns/main.cpp:6:12: error: use of deleted function ‘S::S()’
>>>>
>>>> but I'll try and fix this separately (it exists on current trunk without
>>>> this patch as well).
>>>
>>> While attempting to fix this I found a much better way of handling
>>> value-initialised unions. Here's a new version of the patch which also
>>> includes the fix for accessing the wrong member of a value-initialised
>>> union as well.
>>>
>>> Additionally includes an `auto_diagnostic_group` for the `inform`
>>> diagnostics as Marek helpfully informed me about in my other patch.
>>>
>>> Bootstrapped and regtested on x86_64-pc-linux-gnu.
>>>
>>>> @@ -4496,21 +4491,36 @@ cxx_eval_component_reference (const constexpr_ctx *ctx, tree t,
>>>    	    break;
>>>    	}
>>>        }
>>> -  if (TREE_CODE (TREE_TYPE (whole)) == UNION_TYPE
>>> -      && CONSTRUCTOR_NELTS (whole) > 0)
>>> +  if (TREE_CODE (TREE_TYPE (whole)) == UNION_TYPE)
>>>        {
>>> -      /* DR 1188 says we don't have to deal with this.  */
>>> -      if (!ctx->quiet)
>>> +      if (CONSTRUCTOR_NELTS (whole) > 0)
>>>    	{
>>> -	  constructor_elt *cep = CONSTRUCTOR_ELT (whole, 0);
>>> -	  if (cep->value == NULL_TREE)
>>> -	    error ("accessing uninitialized member %qD", part);
>>> -	  else
>>> -	    error ("accessing %qD member instead of initialized %qD member in "
>>> -		   "constant expression", part, cep->index);
>>> +	  /* DR 1188 says we don't have to deal with this.  */
>>> +	  if (!ctx->quiet)
>>> +	    {
>>> +	      constructor_elt *cep = CONSTRUCTOR_ELT (whole, 0);
>>> +	      if (cep->value == NULL_TREE)
>>> +		error ("accessing uninitialized member %qD", part);
>>> +	      else
>>> +		error ("accessing %qD member instead of initialized %qD member in "
>>> +		       "constant expression", part, cep->index);
>>> +	    }
>>> +	  *non_constant_p = true;
>>> +	  return t;
>>> +	}
>>> +      else if (!CONSTRUCTOR_NO_CLEARING (whole))
>>> +	{
>>> +	  /* Value-initialized union, check if looking at the first member.  */
>>> +	  tree first = next_aggregate_field (TYPE_FIELDS (TREE_TYPE (whole)));
>>> +	  if (pmf ? DECL_NAME (first) != DECL_NAME (part) : first != part)
>>
>> You don't need to consider pmf here, since a PMF isn't UNION_TYPE.
> 
> Ah right, thanks.
> 
>>> @@ -6267,23 +6288,74 @@ cxx_eval_store_expression (const constexpr_ctx *ctx, tree t,
>>>    	  break;
>>>    	}
>>> +      /* Process value-initialization of a union.  */
>>> +      if (code == UNION_TYPE
>>> +	  && !CONSTRUCTOR_NO_CLEARING (*valp)
>>> +	  && CONSTRUCTOR_NELTS (*valp) == 0)
>>> +	if (tree first = next_aggregate_field (TYPE_FIELDS (type)))
>>> +	  CONSTRUCTOR_APPEND_ELT (CONSTRUCTOR_ELTS (*valp), first, NULL_TREE);
>>
>> Isn't this adding an uninitialized member instead of value-initialized?
> 
> This is equivalent to what was going to happen at the end of the loop
> when 'get_or_insert_ctor_field (*valp, index)' is called regardless.
> A value-initialized subobject is then created at the start of the loop
> (based on 'no_zero_init') replacing the NULL_TREE here.

Ah, indeed.

I'm applying this tweaked patch, thanks: