[Bug c++/107206] New: Bogus -Wuninitialized in std::optional

[Bug c++/107206] New: Bogus -Wuninitialized in std::optional

[ed at catmur dot uk]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107206

            Bug ID: 107206
           Summary: Bogus -Wuninitialized in std::optional
           Product: gcc
           Version: 12.1.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: ed at catmur dot uk
  Target Milestone: ---

Since 12, at -std=c++17 -O -Wuninitialized:

#include <optional>
struct X {
    X() = default;
    X(X const& r) : i(r.i) {}
    int i;
};
struct Y {
    Y() : x() {}
    X x;
    std::optional<int> o;
};
struct Z {
    Y y;
    explicit Z(Y y) : y(y) {}
};
void f(Y const&);
void test() {
    Y const y;
    Z z(y);
    z.y.o = 1;
    auto const w = z;
    f(w.y);
}

In copy constructor 'Y::Y(const Y&)',
    inlined from 'void test()' at <source>:19:10:
<source>:7:8: warning: '*(int*)((char*)&y + offsetof(const Y,
Y::o.std::optional<int>::<unnamed>.std::_Optional_base<int, true,
true>::<unnamed>))' is used uninitialized [-Wuninitialized]
    7 | struct Y {
      |        ^
<source>: In function 'void test()':
<source>:18:13: note: 'y' declared here
   18 |     Y const y;
      |             ^

Most similar to #80635 and #86465, I think? Although this is -Wuninitialized,
not -Wmaybe-uninitialized.

[Bug libstdc++/107206] Bogus -Wuninitialized in std::optional

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107206

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
          Component|c++                         |libstdc++
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2022-10-11
     Ever confirmed|0                           |1

--- Comment #1 from Richard Biener <rguenth at gcc dot gnu.org> ---
Confirmed.  We diagnose

<bb 2> [local count: 1073741824]:
MEM[(struct optional *)&y + 4B] ={v} {CLOBBER};
MEM[(union _Storage *)&y + 4B] ={v} {CLOBBER};
MEM[(struct _Optional_payload_base *)&y + 4B]._M_engaged = 0;
D.11546.o = y.o;
SR.27_14 = MEM <int> [(struct optional *)&y + 4B];    <--------- this
MEM <int> [(struct optional *)&D.11546 + 4B] = SR.27_14;
MEM <unsigned char> [(struct optional *)&D.11546 + 8B] = 0;
MEM[(struct Y *)&z].o = MEM[(const struct Y &)&D.11546].o;
D.11546 ={v} {CLOBBER(eol)};
MEM[(struct Y *)&w] ={v} {CLOBBER};
MEM[(struct X *)&w] ={v} {CLOBBER};
MEM[(struct X *)&w].i = 0;
MEM <int> [(struct optional *)&z + 4B] = 1;
MEM <unsigned char> [(struct optional *)&z + 8B] = 1;
MEM[(struct Y *)&w].o = MEM[(const struct Y &)&z].o;
f (&w.y);
y ={v} {CLOBBER(eol)};
z ={v} {CLOBBER(eol)};
w ={v} {CLOBBER(eol)};
return;

Here the aggregate copy

   D.11546.o = y.o;

is effectively scalarized and that accesses the data member of the not
engaged std::optional<>.

IMHO that's a standard library issue?  The copy is due to the by value
passing and we get in .original

  <<cleanup_point <<< Unknown tree: expr_stmt
    Z::Z (&z, &TARGET_EXPR <D.11546, <<< Unknown tree: aggr_init_expr
      5
      __ct_comp
      D.11546
      (struct Y *) <<< Unknown tree: void_cst >>>
      (const struct Y &) &y >>>>) >>>>>;

which eventually leads to

          Y::Y (&D.11546, &y);
          Z::Z (&z, &D.11546);

which contains
void Y::Y (struct Y * const this, const struct Y & D.11498)
{
  *this = {CLOBBER};
  {
    _1 = &this->x;
    _2 = &D.11498->x;
    X::X (_1, _2);
    this->o = D.11498->o;
  }

so the std::optional<> is simply aggregate copied, accessing any not
engaged member.

GCC is correct in diagnosing this uninitialized access.

[Bug libstdc++/107206] Bogus -Wuninitialized in std::optional

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107206

--- Comment #2 from Jonathan Wakely <redi at gcc dot gnu.org> ---
The aggregate copy is intended, because std::optional<T> must have a trivial
copy constructor if T has a trivial copy constructor.

There should be no uninitialized value, because the std::optional<T>'s storage
member has a  default constructor that inits the first member of the union (an
empty struct):

      struct _Empty_byte { };

      template<typename _Up, bool = is_trivially_destructible_v<_Up>>
        union _Storage
        {
          constexpr _Storage() noexcept : _M_empty() { }

          // ...

          _Empty_byte _M_empty;
          _Up _M_value;
        };

The compiler seems to be eliding the _M_empty() initialization, because
_M_empty has no value bits, it's all padding.

[Bug libstdc++/107206] Bogus -Wuninitialized in std::optional

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107206

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jamborm at gcc dot gnu.org

--- Comment #3 from Richard Biener <rguenth at gcc dot gnu.org> ---
So this goes wrong in SRA somewhere where we decide to scalarize yD.11526
with its std::optional member which isn't engaged.  Before early SRA we have

  MEM[(struct YD.10174 *)&yD.11526] ={v} {CLOBBER};
  MEM[(struct YD.10174 *)&yD.11526].xD.10179.iD.10156 = 0;
  MEM[(struct optionalD.10180 *)&yD.11526 + 4B] ={v} {CLOBBER};
  MEM[(union _StorageD.10576 *)&yD.11526 + 4B] ={v} {CLOBBER};
  MEM[(struct _Optional_payload_baseD.10507 *)&yD.11526 + 4B]._M_engagedD.10644
= 0;
  D.11546 ={v} {CLOBBER};
  MEM[(struct XD.10147 *)&D.11546] ={v} {CLOBBER};
  _22 = MEM[(const struct XD.10147 &)&yD.11526].iD.10156;
  MEM[(struct XD.10147 *)&D.11546].iD.10156 = _22;
  D.11546.oD.11384 = yD.11526.oD.11384;
  MEM[(struct YD.10174 *)&zD.11527] ={v} {CLOBBER};
  MEM[(struct XD.10147 *)&zD.11527] ={v} {CLOBBER};
  _21 = MEM[(const struct XD.10147 &)&D.11546].iD.10156;
  MEM[(struct XD.10147 *)&zD.11527].iD.10156 = _21;
  MEM[(struct YD.10174 *)&zD.11527].oD.11384 = MEM[(const struct YD.10174
&)&D.11546].oD.11384;
  D.11546 ={v} {CLOBBER(eol)};
  MEM[(struct _Optional_baseD.10197 *)&D.11678] ={v} {CLOBBER};
  MEM[(struct __as_base D.10805 &)&D.11678] ={v} {CLOBBER};
  MEM[(union _StorageD.10576 *)&D.11678] ={v} {CLOBBER};
  MEM[(union _StorageD.10576 *)&D.11678]._M_valueD.10616 = 1;
  MEM[(struct _Optional_payload_baseD.10507 *)&D.11678]._M_engagedD.10644 = 1;
  zD.11527.yD.11476.oD.11384 = D.11678;
  D.11678 ={v} {CLOBBER(eol)};
  MEM[(struct YD.10174 *)&wD.11680] ={v} {CLOBBER};
  MEM[(struct XD.10147 *)&wD.11680] ={v} {CLOBBER};
  _19 = MEM[(const struct XD.10147 &)&zD.11527].iD.10156;
  MEM[(struct XD.10147 *)&wD.11680].iD.10156 = _19;
  MEM[(struct YD.10174 *)&wD.11680].oD.11384 = MEM[(const struct YD.10174
&)&zD.11527].oD.11384;
  fD.11523 (&wD.11680.yD.11476);

and SRA possibly follows the copy chains eventually seeing the
value accesses to D.11678 and deciding based on that the type to use
to access the value union of std::optional which is (simplified)

      struct _Empty_byte { };
      union _Storage {
          _Empty_byte _M_empty;
          _Up _M_value;
      }

with _Up being int in this testcase.  Note that _M_empty is properly
initialized but as the store is to an empty type this store is elided
by gimplification.

Note that SRA doesn't dump anything on yD.11526 but still generates

  D.11546.oD.11384 = yD.11526.oD.11384;
  SR.19_9 = MEM <intD.9> [(struct optionalD.10180 *)&yD.11526 + 4B];
  SR.20_6 = MEM <unsigned char> [(struct optionalD.10180 *)&yD.11526 + 8B];

possibly from the scalarization of D.11546:

Created a replacement for D.11546 offset: 0, size: 32: SR.18D.11910
Created a replacement for D.11546 offset: 32, size: 32: SR.19D.11911
Changing the type of a replacement for D.11546 offset: 64, size: 8  to an
integer.
Created a replacement for D.11546 offset: 64, size: 8: SR.20D.11912
...
access { base = (11546)'D.11546', offset = 32, size = 64, expr = D.11546.o,
type = struct optional, reverse = 0, grp_read = 1, grp_write = 1,
grp_assignment_read = 1, grp_assignment_write = 1, grp_scalar_read = 0,
grp_scalar_write = 0, grp_total_scalarization = 0, grp_hint = 0, grp_covered =
0, grp_unscalarizable_region = 0, grp_unscalarized_data = 1,
grp_same_access_path = 1, grp_partial_lhs = 0, grp_to_be_replaced = 0,
grp_to_be_debug_replaced = 0}

here D.11546.o is of an aggregate type containing a union member.  Somehow
there's

* access { base = (11546)'D.11546', offset = 32, size = 32, expr =
D.11546.o.D.11379._M_payload.D.10892._M_payload._M_value, type = int, reverse =
0, grp_read = 1, grp_write = 1, grp_assignment_read = 1, grp_assignment_write =
1, grp_scalar_read = 0, grp_scalar_write = 0, grp_total_scalarization = 0,
grp_hint = 0, grp_covered = 1, grp_unscalarizable_region = 0,
grp_unscalarized_data = 0, grp_same_access_path = 0, grp_partial_lhs = 0,
grp_to_be_replaced = 1, grp_to_be_debug_replaced = 0}

but I don't see any such access in the IL before SRA?!

[Bug tree-optimization/107206] Bogus -Wuninitialized in std::optional

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107206

--- Comment #4 from Richard Biener <rguenth at gcc dot gnu.org> ---
Btw, in other places I saw it's very much wanted to scalarize std::optional
because that's often the best way to figure whether it's engaged or not and
optimize dead code based on that.  So not scalarizing probably isn't the best
answer, still it would be nice to understand why SRA picks 'int' here even
though there's no access of that.

[Bug tree-optimization/107206] Bogus -Wuninitialized in std::optional

[jamborm at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107206

--- Comment #5 from Martin Jambor <jamborm at gcc dot gnu.org> ---
I believe this is fallout from the fix to PR 92706 where we started
propagating accesses across assignments also from LHS to RHS and
because everything is totally flow-insensitive, we have an access
representing:

   MEM[(union _StorageD.10576 *)&D.11678]._M_valueD.10616 = 1;

which gets propagated across:

  zD.11527.yD.11476.oD.11384 = D.11678;

but this then triggers the LHS->RHS propagation across:

  MEM[(struct YD.10174 *)&zD.11527].oD.11384 = MEM[(const struct YD.10174
&)&D.11546].oD.11384;

So another reason to re-invent SRA from scratch.  Apart from that, I
don't have a good solution except for adding another flag to mark
accesses that are result of LHS->RHS propagation and ignore them if
their base did not get totally scalarized because in the PR they were
added basically to prevent bad total scalarization.  But of course it
is rather another band-aid than a real fix, but it seems to work for
this case:

diff --git a/gcc/tree-sra.cc b/gcc/tree-sra.cc
index 1a3e12f18cc..6cbeddfc548 100644
--- a/gcc/tree-sra.cc
+++ b/gcc/tree-sra.cc
@@ -260,6 +260,9 @@ struct access

   /* Should TREE_NO_WARNING of a replacement be set?  */
   unsigned grp_no_warning : 1;
+
+  /* Result of propagation accross link from LHS to RHS.  */
+  unsigned grp_result_of_prop_from_lhs : 1;
 };

 typedef struct access *access_p;
@@ -2532,6 +2535,9 @@ analyze_access_subtree (struct access *root, struct
access *parent,
   if (allow_replacements && expr_with_var_bounded_array_refs_p (root->expr))
     allow_replacements = false;

+  if (!totally && root->grp_result_of_prop_from_lhs)
+    allow_replacements = false;
+
   for (child = root->first_child; child; child = child->next_sibling)
     {
       hole |= covered_to < child->offset;
@@ -2959,6 +2965,7 @@ propagate_subaccesses_from_lhs (struct access *lacc,
struct access *racc)
          struct access *new_acc
            = create_artificial_child_access (racc, lchild, norm_offset,
                                              true, false);
+         new_acc->grp_result_of_prop_from_lhs = 1;
          propagate_subaccesses_from_lhs (lchild, new_acc);
        }
       else

[Bug tree-optimization/107206] Bogus -Wuninitialized in std::optional

[rguenther at suse dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107206

--- Comment #6 from rguenther at suse dot de <rguenther at suse dot de> ---
> Am 14.10.2022 um 18:53 schrieb jamborm at gcc dot gnu.org <gcc-bugzilla@gcc.gnu.org>:
> 
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107206
> 
> --- Comment #5 from Martin Jambor <jamborm at gcc dot gnu.org> ---
> I believe this is fallout from the fix to PR 92706 where we started
> propagating accesses across assignments also from LHS to RHS and
> because everything is totally flow-insensitive, we have an access
> representing:
> 
>   MEM[(union _StorageD.10576 *)&D.11678]._M_valueD.10616 = 1;
> 
> which gets propagated across:
> 
>  zD.11527.yD.11476.oD.11384 = D.11678;
> 
> but this then triggers the LHS->RHS propagation across:
> 
>  MEM[(struct YD.10174 *)&zD.11527].oD.11384 = MEM[(const struct YD.10174
> &)&D.11546].oD.11384;
> 
> So another reason to re-invent SRA from scratch.

Yes

>  Apart from that, I
> don't have a good solution except for adding another flag to mark
> accesses that are result of LHS->RHS propagation and ignore them if
> their base did not get totally scalarized because in the PR they were
> added basically to prevent bad total scalarization.  But of course it
> is rather another band-aid than a real fix, but it seems to work for
> this case:
> 
> diff --git a/gcc/tree-sra.cc b/gcc/tree-sra.cc
> index 1a3e12f18cc..6cbeddfc548 100644
> --- a/gcc/tree-sra.cc
> +++ b/gcc/tree-sra.cc
> @@ -260,6 +260,9 @@ struct access
> 
>   /* Should TREE_NO_WARNING of a replacement be set?  */
>   unsigned grp_no_warning : 1;
> +
> +  /* Result of propagation accross link from LHS to RHS.  */
> +  unsigned grp_result_of_prop_from_lhs : 1;
> };
> 
> typedef struct access *access_p;
> @@ -2532,6 +2535,9 @@ analyze_access_subtree (struct access *root, struct
> access *parent,
>   if (allow_replacements && expr_with_var_bounded_array_refs_p (root->expr))
>     allow_replacements = false;
> 
> +  if (!totally && root->grp_result_of_prop_from_lhs)
> +    allow_replacements = false;
> +
>   for (child = root->first_child; child; child = child->next_sibling)
>     {
>       hole |= covered_to < child->offset;
> @@ -2959,6 +2965,7 @@ propagate_subaccesses_from_lhs (struct access *lacc,
> struct access *racc)
>          struct access *new_acc
>            = create_artificial_child_access (racc, lchild, norm_offset,
>                                              true, false);
> +         new_acc->grp_result_of_prop_from_lhs = 1;
>          propagate_subaccesses_from_lhs (lchild, new_acc);
>        }
>       else

LGTM

[Bug tree-optimization/107206] Bogus -Wuninitialized in std::optional

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107206

--- Comment #7 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Martin Jambor <jamborm@gcc.gnu.org>:

https://gcc.gnu.org/g:f6c168f8c06047bfaa3005e570126831b8855dcc

commit r13-3378-gf6c168f8c06047bfaa3005e570126831b8855dcc
Author: Martin Jambor <mjambor@suse.cz>
Date:   Wed Oct 19 14:43:04 2022 +0200

    SRA: Limit replacement creation for accesses propagated from LHSs

    PR 107206 is fallout from the fix to PR 92706 where we started
    propagating accesses across assignments also from LHS to RHS of
    assignments so that we would not do harmful total scalarization of the
    aggregates on the RHS.

    But this can lead to new scalarization of these aggregates and in the
    testcase of PR 107206 these can appear in superfluous uses of
    un-initialized values and spurious warnings.

    Fixed by making sure the the accesses created by propagation in this
    direction are only used as a basis for replacements when the structure
    would be totally scalarized anyway.

    gcc/ChangeLog:

    2022-10-18  Martin Jambor  <mjambor@suse.cz>

            PR tree-optimization/107206
            * tree-sra.cc (struct access): New field
grp_result_of_prop_from_lhs.
            (analyze_access_subtree): Do not create replacements for accesses
with
            this flag when not toally scalarizing.
            (propagate_subaccesses_from_lhs): Set the new flag.

    gcc/testsuite/ChangeLog:

    2022-10-18  Martin Jambor  <mjambor@suse.cz>

            PR tree-optimization/107206
            * g++.dg/tree-ssa/pr107206.C: New test.

[Bug tree-optimization/107206] Bogus -Wuninitialized in std::optional

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107206

--- Comment #8 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-12 branch has been updated by Martin Jambor
<jamborm@gcc.gnu.org>:

https://gcc.gnu.org/g:985a8962712a56d1c9418cf05e1761f8bd20cf8f

commit r12-8913-g985a8962712a56d1c9418cf05e1761f8bd20cf8f
Author: Martin Jambor <mjambor@suse.cz>
Date:   Wed Nov 16 13:11:27 2022 +0100

    SRA: Limit replacement creation for accesses propagated from LHSs

    PR 107206 is fallout from the fix to PR 92706 where we started
    propagating accesses across assignments also from LHS to RHS of
    assignments so that we would not do harmful total scalarization of the
    aggregates on the RHS.

    But this can lead to new scalarization of these aggregates and in the
    testcase of PR 107206 these can appear in superfluous uses of
    un-initialized values and spurious warnings.

    Fixed by making sure the the accesses created by propagation in this
    direction are only used as a basis for replacements when the structure
    would be totally scalarized anyway.

    gcc/ChangeLog:

    2022-10-18  Martin Jambor  <mjambor@suse.cz>

            PR tree-optimization/107206
            * tree-sra.cc (struct access): New field
grp_result_of_prop_from_lhs.
            (analyze_access_subtree): Do not create replacements for accesses
with
            this flag when not toally scalarizing.
            (propagate_subaccesses_from_lhs): Set the new flag.

    gcc/testsuite/ChangeLog:

    2022-10-18  Martin Jambor  <mjambor@suse.cz>

            PR tree-optimization/107206
            * g++.dg/tree-ssa/pr107206.C: New test.

    (cherry picked from commit f6c168f8c06047bfaa3005e570126831b8855dcc)

[Bug tree-optimization/107206] Bogus -Wuninitialized in std::optional

[jamborm at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107206

Martin Jambor <jamborm at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|---                         |FIXED

--- Comment #9 from Martin Jambor <jamborm at gcc dot gnu.org> ---
Fixed on both master and the gcc-12 branch.

[Bug tree-optimization/107206] Bogus -Wuninitialized in std::optional

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107206

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |12.3
      Known to work|                            |12.2.1