[Bug c++/102482] New: Winit-list-lifetime false positive for temporaries with std::initializer_list

[Bug c++/102482] New: Winit-list-lifetime false positive for temporaries with std::initializer_list

[federico.kircheis at gmail dot com]

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

            Bug ID: 102482
           Summary: Winit-list-lifetime false positive for temporaries
                    with std::initializer_list
           Product: gcc
           Version: 11.2.1
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: federico.kircheis at gmail dot com
  Target Milestone: ---

Following program


----
#include <initializer_list>

struct span {
    span(std::initializer_list<int> il) noexcept : begin(il.begin()),
size(il.size()) {}
    const int* begin;
    std::size_t size;
};


int foo(span sp){
        return sp.begin[0];
}

int main() {
    return foo(span(std::initializer_list<int>{}));
}
----


triggers

warning: initializing 'span::begin' from 'std::initializer_list<int>::begin'
does not extend the lifetime of the underlying array [-Winit-list-lifetime]

with no compiler flags except "--std=c++20"

(https://godbolt.org/z/ocaxh46oW)


While the warning is true, the code is completely safe, but the warnings seems
to imply that foo will access a dangling pointer.

Strangely, it does not diagnose anything on


----
#include <vector>

struct span {
    span(std::vector<int> il) noexcept : begin(il.data()), size(il.size()) {}
    const int* begin;
    std::size_t size;
};


int foo(span sp){
        return sp.begin[0];
}

int main() {
    return foo(span(std::vector<int>{}));
}
----

(https://godbolt.org/z/raMoTohvc)

where the lifetime rules are the same.

so I assume the warning is std::initializer_list specific.

[Bug c++/102482] Winit-list-lifetime false positive for temporaries with std::initializer_list

[redi at gcc dot gnu.org]

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

Jonathan Wakely <redi at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |diagnostic

--- Comment #1 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to Federico Kircheis from comment #0)
> While the warning is true, the code is completely safe, but the warnings
> seems to imply that foo will access a dangling pointer.

In this case it's safe because the span obejct has the same lifetime as the
initializer_list, but in general that's not true. The warning comes from the
constructor, and is independent of the context in which it's being constructed.

> so I assume the warning is std::initializer_list specific.

Yes. std::initializer_list is a magic language type that the compiler knows all
about. std::vector is just opaque C++ code that the compiler knows nothing
about.

[Bug c++/102482] Winit-list-lifetime false positive for temporaries with std::initializer_list

[redi at gcc dot gnu.org]

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

--- Comment #2 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to Jonathan Wakely from comment #1)
> > so I assume the warning is std::initializer_list specific.
> 
> Yes. std::initializer_list is a magic language type that the compiler knows
> all about. std::vector is just opaque C++ code that the compiler knows
> nothing about.

The clue is in the name: -Winit-list-lifetime

[Bug c++/102482] Winit-list-lifetime false positive for temporaries with std::initializer_list

[redi at gcc dot gnu.org]

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

--- Comment #3 from Jonathan Wakely <redi at gcc dot gnu.org> ---
And the fabulous manual:

> warn about uses of "std::initializer_list" that are likely to result in
> dangling pointers

This is behaving exactly as documented:

> *   When a list constructor stores the "begin" pointer from the
>     "initializer_list" argument, this doesn't extend the lifetime of
>     the array, so if a class variable is constructed from a temporary
>     "initializer_list", the pointer is left dangling by the end of the
>     variable declaration statement.

[Bug c++/102482] Winit-list-lifetime false positive for temporaries with std::initializer_list

[federico.kircheis at gmail dot com]

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

--- Comment #4 from Federico Kircheis <federico.kircheis at gmail dot com> ---
Note that following equivalent snippet

----
#include <initializer_list>

struct span {
    span(std::initializer_list<int> il) noexcept : begin(nullptr),
size(il.size()) { begin = il.begin();}
    const int* begin;
    std::size_t size;
};
----

does not trigger the warning.

> And the fabulous manual [...]

Mhm, I should have done a little more research before opening this ticket.

> *   When a list constructor stores the "begin" pointer from the
>     "initializer_list" argument, this doesn't extend the lifetime of
>     the array, so if a class variable is constructed from a temporary
>     "initializer_list", the pointer is left dangling by the end of the
>     variable declaration statement.

The first statement is correct (does not extend lifetime), but the second is
not always (the pointer is not always left dangling).


Also following snippet generates the warning:

----
#include <initializer_list>

struct span {
    span(std::initializer_list<int>& il) noexcept : begin(il.begin()),
size(il.size()) {}
    const int* begin;
    std::size_t size;
};
----

but I currently cannot imagine a scenario where this would dangle, as one
cannot write "span({1,2,3})"

[Bug c++/102482] Winit-list-lifetime false positive for temporaries with std::initializer_list

[redi at gcc dot gnu.org]

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

Jonathan Wakely <redi at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |dmalcolm at gcc dot gnu.org
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2021-09-29
     Ever confirmed|0                           |1

--- Comment #5 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to Federico Kircheis from comment #4)
> Note that following equivalent snippet
> 
> ----
> #include <initializer_list>
> 
> struct span {
>     span(std::initializer_list<int> il) noexcept : begin(nullptr),
> size(il.size()) { begin = il.begin();}
>     const int* begin;
>     std::size_t size;
> };
> ----
> 
> does not trigger the warning.

Ideally it would warn, but I hope the reason it doesn't its fairly obvious. The
C++ front end doesn't perform arbitrarily complex data flow analysis for the
constructor body, it just looks for the most common pattern of storing a
pointer to the temporary array in a mem-initializer. Using the array in the
constructor body /without storing it/ is less problematic, because only the
non-static data members persist beyond the end of the constructor e.g. this is
fine:

span(std::initializer_list<int> il)
{ for (auto i : il) std::cout << i << ' '; }


As a simple heuristic to warn about the majority of cases, GCC checks for
storing the pointer in the mem-initializer-list only.

Maybe you're misunderstanding what the warning is telling you. It's not saying
that the statement `return foo(span(std::initializer_list<int>{}));` has a bug,
because that's not the location of the warning. It's telling you that your
constructor definition is potentially dangerous. Which is totally true. How is
the compiler supposed to know that you never plan to use that constructor for
lvalues? (C++ doesn't allow a && ref-qualifier on a constructor, which would
enforce that at the language level). Should it re-compile the constructor every
time it's called, passing in whether it's constructing an rvalue or lvalue, so
that only some uses of it warn? That would slow down compilation for every
class with an initializer-list constructor. What if the constructor is not
defined inline, so is compiled completely separate from its callers?

If **you** know that nobody will never use that constructor for lvalues, then
just disable the warning around the constructor:

struct span {
#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Winit-list-lifetime"
    span(std::initializer_list<int> il) noexcept : begin(il.begin()),
size(il.size()) {}
#pragma GCC diagnostic pop
    const int* begin;
    std::size_t size;
};

Or if you don't think the warning is useful, just disable it on the command
line via -Wno-init-list-lifetime. But the compiler can't know how you plan to
use that constructor, so it tells you it's got a potential bug.

> > And the fabulous manual [...]
> 
> Mhm, I should have done a little more research before opening this ticket.

Reading the documentation for the warning you're reporting a bug for is
probably a good start :-)

> > *   When a list constructor stores the "begin" pointer from the
> >     "initializer_list" argument, this doesn't extend the lifetime of
> >     the array, so if a class variable is constructed from a temporary
> >     "initializer_list", the pointer is left dangling by the end of the
> >     variable declaration statement.
> 
> The first statement is correct (does not extend lifetime), but the second is
> not always (the pointer is not always left dangling).

If the pointer's lifetime has not ended, it dangles. The manual seems correct
as written (it could be made more verbose to talk about special cases, but
approximately nobody reads the manual now, adding more words isn't going to
help).


> Also following snippet generates the warning:
> 
> ----
> #include <initializer_list>
> 
> struct span {
>     span(std::initializer_list<int>& il) noexcept : begin(il.begin()),
> size(il.size()) {}
>     const int* begin;
>     std::size_t size;
> };
> ----
> 
> but I currently cannot imagine a scenario where this would dangle, as one
> cannot write "span({1,2,3})"

True, but approximately nobody writes constructors taking a non-const lvalue
reference to std::initializer_list, so this corner case doesn't seem very
important. This does seem like a concrete enhancement that could be made easily
though, even if low value in practice.

The point is, this is a warning that warns about **potentially** problematic
code in the constructor. All warnings like this have false positives (otherwise
they would be errors not warnings).

If you have concrete suggestions for improving the heuristics then it might be
possible to reduce the false positive rate, but otherwise this amounts to "this
warning has false positives" and the response is "yes".

The current -Winit-list-lifetime is a simple, heuristic-based warning done in
the front end, so is going to have false positives. Pushing the warning into
the middle end (where flow analysis and inlining happens) would make it
smarter, but that would require teaching the language-independent middle end
about C++ std::initializer_list semantics, and would be a completely different
warning to the current simple heiristic in the front end. So a lot of work.

I'll confirm this as a diagnostic bug, but I would be surprised if anything can
be done about it.

Maybe Dave Malcolm's -fanalyzer could be taught to track std::initializer_list
lifetimes and check if the pointer escapes past the array's lifetime. Then if
you know you are going to check your code with -fanalyzer you could use
-Wno-init-list-lifetime during normal compilation, and rely on -fanalyzer
builds to find these problems (without false positives). But that doesn't exist
yet, and not everybody is going to use -fanalyzer, so the current warning is
still useful and working as designed.

[Bug c++/102482] Winit-list-lifetime false positive for temporaries with std::initializer_list

[redi at gcc dot gnu.org]

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

--- Comment #6 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to Jonathan Wakely from comment #5)
> True, but approximately nobody writes constructors taking a non-const lvalue
> reference to std::initializer_list, so this corner case doesn't seem very
> important. This does seem like a concrete enhancement that could be made
> easily though, even if low value in practice.

Just to make this bit easier to find in my wall of text above:

The warning should not trigger if the constructor takes the initializer_list by
non-const lvalue reference.

[Bug c++/102482] Winit-list-lifetime false positive for temporaries with std::initializer_list

[redi at gcc dot gnu.org]

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

--- Comment #7 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to Jonathan Wakely from comment #6)
> The warning should not trigger if the constructor takes the initializer_list
> by non-const lvalue reference.

I think this change to cp/init.c:maybe_warn_list_ctor will check if the
initializer_list parameter is a non-const lvalue reference:

--- a/gcc/cp/init.c
+++ b/gcc/cp/init.c
@@ -750,7 +750,8 @@ maybe_warn_list_ctor (tree member, tree init)
     return;

   tree parms = FUNCTION_FIRST_USER_PARMTYPE (current_function_decl);
-  tree initlist = non_reference (TREE_VALUE (parms));
+  tree parm1 = TREE_VALUE (parms);
+  tree initlist = non_reference (parm1);
   tree targs = CLASSTYPE_TI_ARGS (initlist);
   tree elttype = TREE_VEC_ELT (targs, 0);

@@ -758,6 +759,10 @@ maybe_warn_list_ctor (tree member, tree init)
       (TREE_TYPE (memtype), elttype))
     return;

+  if (TYPE_REF_P (parm1) && !TYPE_REF_IS_RVALUE (parm1)
+      && !(cp_type_quals (initlist) & TYPE_QUAL_CONST))
+    return;
+
   tree begin = find_list_begin (init);
   if (!begin)
     return;

[Bug c++/102482] Winit-list-lifetime false positive for temporaries with std::initializer_list

[redi at gcc dot gnu.org]

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

Jonathan Wakely <redi at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |patch

--- Comment #8 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Patch: https://gcc.gnu.org/pipermail/gcc-patches/2021-October/580771.html

[Bug c++/102482] Winit-list-lifetime false positive for temporaries with std::initializer_list

[cvs-commit at gcc dot gnu.org]

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

--- Comment #9 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Jonathan Wakely <redi@gcc.gnu.org>:

https://gcc.gnu.org/g:9b239d05ffd5a17ede44abd55bc6622c6e279868

commit r12-4228-g9b239d05ffd5a17ede44abd55bc6622c6e279868
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Wed Sep 29 21:19:49 2021 +0100

    c++: Do not warn about lifetime of std::initializer_list<T>& [PR102482]

    An initializer-list constructor taking a non-const lvalue cannot be
    called with a temporary, so the array's lifetime probably doesn't end
    with the full expression. -Winit-list-lifetime should not warn for that
    case.

            PR c++/102482

    gcc/cp/ChangeLog:

            * init.c (maybe_warn_list_ctor): Do not warn for a reference to
            a non-const std::initializer_list.

    gcc/testsuite/ChangeLog:

            * g++.dg/warn/Winit-list5.C: New test.