On Thu, Feb 01, 2024 at 02:32:33PM +0000, Alex Coplan wrote:
> On 31/01/2024 15:53, Marek Polacek wrote:
> > On Wed, Jan 31, 2024 at 07:44:41PM +0000, Alex Coplan wrote:
> > > Hi Marek,
> > >
> > > On 30/01/2024 13:15, Marek Polacek wrote:
> > > > On Thu, Jan 25, 2024 at 10:13:10PM -0500, Jason Merrill wrote:
> > > > > On 1/25/24 20:36, Marek Polacek wrote:
> > > > > > Better version:
> > > > > >
> > > > > > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?
> > > > > >
> > > > > > -- >8 --
> > > > > > Real-world experience shows that -Wdangling-reference triggers for
> > > > > > user-defined std::span-like classes a lot. We can easily avoid that
> > > > > > by considering classes like
> > > > > >
> > > > > > template<typename T>
> > > > > > struct Span {
> > > > > > T* data_;
> > > > > > std::size len_;
> > > > > > };
> > > > > >
> > > > > > to be std::span-like, and not warning for them. Unlike the previous
> > > > > > patch, this one considers a non-union class template that has a
> > > > > > pointer
> > > > > > data member and a trivial destructor as std::span-like.
> > > > > >
> > > > > > PR c++/110358
> > > > > > PR c++/109640
> > > > > >
> > > > > > gcc/cp/ChangeLog:
> > > > > >
> > > > > > * call.cc (reference_like_class_p): Don't warn for
> > > > > > std::span-like
> > > > > > classes.
> > > > > >
> > > > > > gcc/ChangeLog:
> > > > > >
> > > > > > * doc/invoke.texi: Update -Wdangling-reference description.
> > > > > >
> > > > > > gcc/testsuite/ChangeLog:
> > > > > >
> > > > > > * g++.dg/warn/Wdangling-reference18.C: New test.
> > > > > > * g++.dg/warn/Wdangling-reference19.C: New test.
> > > > > > * g++.dg/warn/Wdangling-reference20.C: New test.
> > > > > > ---
> > > > > > gcc/cp/call.cc | 18 ++++++++
> > > > > > gcc/doc/invoke.texi | 14 +++++++
> > > > > > .../g++.dg/warn/Wdangling-reference18.C | 24 +++++++++++
> > > > > > .../g++.dg/warn/Wdangling-reference19.C | 25 +++++++++++
> > > > > > .../g++.dg/warn/Wdangling-reference20.C | 42
> > > > > > +++++++++++++++++++
> > > > > > 5 files changed, 123 insertions(+)
> > > > > > create mode 100644
> > > > > > gcc/testsuite/g++.dg/warn/Wdangling-reference18.C
> > > > > > create mode 100644
> > > > > > gcc/testsuite/g++.dg/warn/Wdangling-reference19.C
> > > > > > create mode 100644
> > > > > > gcc/testsuite/g++.dg/warn/Wdangling-reference20.C
> > > > > >
> > > > > > diff --git a/gcc/cp/call.cc b/gcc/cp/call.cc
> > > > > > index 9de0d77c423..afd3e1ff024 100644
> > > > > > --- a/gcc/cp/call.cc
> > > > > > +++ b/gcc/cp/call.cc
> > > > > > @@ -14082,6 +14082,24 @@ reference_like_class_p (tree ctype)
> > > > > > return true;
> > > > > > }
> > > > > > + /* Avoid warning if CTYPE looks like std::span: it's a class
> > > > > > template,
> > > > > > + has a T* member, and a trivial destructor. For example,
> > > > > > +
> > > > > > + template<typename T>
> > > > > > + struct Span {
> > > > > > + T* data_;
> > > > > > + std::size len_;
> > > > > > + };
> > > > > > +
> > > > > > + is considered std::span-like. */
> > > > > > + if (NON_UNION_CLASS_TYPE_P (ctype)
> > > > > > + && CLASSTYPE_TEMPLATE_INSTANTIATION (ctype)
> > > > > > + && TYPE_HAS_TRIVIAL_DESTRUCTOR (ctype))
> > > > > > + for (tree field = next_aggregate_field (TYPE_FIELDS (ctype));
> > > > > > + field; field = next_aggregate_field (DECL_CHAIN (field)))
> > > > > > + if (TYPE_PTR_P (TREE_TYPE (field)))
> > > > > > + return true;
> > > > > > +
> > > > > > /* Some classes, such as std::tuple, have the reference member
> > > > > > in its
> > > > > > (non-direct) base class. */
> > > > > > if (dfs_walk_once (TYPE_BINFO (ctype),
> > > > > > class_has_reference_member_p_r,
> > > > > > diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
> > > > > > index 6ec56493e59..e0ff18a86f5 100644
> > > > > > --- a/gcc/doc/invoke.texi
> > > > > > +++ b/gcc/doc/invoke.texi
> > > > > > @@ -3916,6 +3916,20 @@ where @code{std::minmax} returns
> > > > > > @code{std::pair<const int&, const int&>}, and
> > > > > > both references dangle after the end of the full expression that
> > > > > > contains
> > > > > > the call to @code{std::minmax}.
> > > > > > +The warning does not warn for @code{std::span}-like classes. We
> > > > > > consider
> > > > > > +classes of the form:
> > > > > > +
> > > > > > +@smallexample
> > > > > > +template<typename T>
> > > > > > +struct Span @{
> > > > > > + T* data_;
> > > > > > + std::size len_;
> > > > > > +@};
> > > > > > +@end smallexample
> > > > > > +
> > > > > > +as @code{std::span}-like; that is, the class is a non-union class
> > > > > > template
> > > > > > +that has a pointer data member and a trivial destructor.
> > > > > > +
> > > > > > This warning is enabled by @option{-Wall}.
> > > > > > @opindex Wdelete-non-virtual-dtor
> > > > > > diff --git a/gcc/testsuite/g++.dg/warn/Wdangling-reference18.C
> > > > > > b/gcc/testsuite/g++.dg/warn/Wdangling-reference18.C
> > > > > > new file mode 100644
> > > > > > index 00000000000..e088c177769
> > > > > > --- /dev/null
> > > > > > +++ b/gcc/testsuite/g++.dg/warn/Wdangling-reference18.C
> > > > > > @@ -0,0 +1,24 @@
> > > > > > +// PR c++/110358
> > > > > > +// { dg-do compile { target c++11 } }
> > > > > > +// { dg-options "-Wdangling-reference" }
> > > > > > +// Don't warn for std::span-like classes.
> > > > > > +
> > > > > > +template <typename T>
> > > > > > +struct Span {
> > > > > > + T* data_;
> > > > > > + int len_;
> > > > > > +
> > > > > > + [[nodiscard]] constexpr auto operator[](int n) const noexcept
> > > > > > -> T& { return data_[n]; }
> > > > > > + [[nodiscard]] constexpr auto front() const noexcept -> T& {
> > > > > > return data_[0]; }
> > > > > > + [[nodiscard]] constexpr auto back() const noexcept -> T& {
> > > > > > return data_[len_ - 1]; }
> > > > > > +};
> > > > > > +
> > > > > > +auto get() -> Span<int>;
> > > > > > +
> > > > > > +auto f() -> int {
> > > > > > + int const& a = get().front(); // { dg-bogus "dangling
> > > > > > reference" }
> > > > > > + int const& b = get().back(); // { dg-bogus "dangling
> > > > > > reference" }
> > > > > > + int const& c = get()[0]; // { dg-bogus "dangling
> > > > > > reference" }
> > > > > > +
> > > > > > + return a + b + c;
> > > > > > +}
> > > > > > diff --git a/gcc/testsuite/g++.dg/warn/Wdangling-reference19.C
> > > > > > b/gcc/testsuite/g++.dg/warn/Wdangling-reference19.C
> > > > > > new file mode 100644
> > > > > > index 00000000000..053467d822f
> > > > > > --- /dev/null
> > > > > > +++ b/gcc/testsuite/g++.dg/warn/Wdangling-reference19.C
> > > > > > @@ -0,0 +1,25 @@
> > > > > > +// PR c++/110358
> > > > > > +// { dg-do compile { target c++11 } }
> > > > > > +// { dg-options "-Wdangling-reference" }
> > > > > > +// Like Wdangling-reference18.C but not actually a span-like class.
> > > > > > +
> > > > > > +template <typename T>
> > > > > > +struct Span {
> > > > > > + T* data_;
> > > > > > + int len_;
> > > > > > + ~Span ();
> > > > > > +
> > > > > > + [[nodiscard]] constexpr auto operator[](int n) const noexcept
> > > > > > -> T& { return data_[n]; }
> > > > > > + [[nodiscard]] constexpr auto front() const noexcept -> T& {
> > > > > > return data_[0]; }
> > > > > > + [[nodiscard]] constexpr auto back() const noexcept -> T& {
> > > > > > return data_[len_ - 1]; }
> > > > > > +};
> > > > > > +
> > > > > > +auto get() -> Span<int>;
> > > > > > +
> > > > > > +auto f() -> int {
> > > > > > + int const& a = get().front(); // { dg-warning "dangling
> > > > > > reference" }
> > > > > > + int const& b = get().back(); // { dg-warning "dangling
> > > > > > reference" }
> > > > > > + int const& c = get()[0]; // { dg-warning "dangling
> > > > > > reference" }
> > > > > > +
> > > > > > + return a + b + c;
> > > > > > +}
> > > > > > diff --git a/gcc/testsuite/g++.dg/warn/Wdangling-reference20.C
> > > > > > b/gcc/testsuite/g++.dg/warn/Wdangling-reference20.C
> > > > > > new file mode 100644
> > > > > > index 00000000000..463c7380283
> > > > > > --- /dev/null
> > > > > > +++ b/gcc/testsuite/g++.dg/warn/Wdangling-reference20.C
> > > > > > @@ -0,0 +1,42 @@
> > > > > > +// PR c++/109640
> > > > > > +// { dg-do compile { target c++20 } }
> > > > > > +// { dg-options "-Wdangling-reference" }
> > > > > > +// Don't warn for std::span-like classes.
> > > > > > +
> > > > > > +#include <iterator>
> > > > > > +#include <span>
> > > > > > +
> > > > > > +template <typename T>
> > > > > > +struct MySpan
> > > > > > +{
> > > > > > + MySpan(T* data, std::size_t size) :
> > > > > > + data_(data),
> > > > > > + size_(size)
> > > > > > + {}
> > > > > > +
> > > > > > + T& operator[](std::size_t idx) { return data_[idx]; }
> > > > > > +
> > > > > > +private:
> > > > > > + T* data_;
> > > > > > + std::size_t size_;
> > > > > > +};
> > > > > > +
> > > > > > +template <typename T, std::size_t n>
> > > > > > +MySpan<T const> make_my_span(T const(&x)[n])
> > > > > > +{
> > > > > > + return MySpan(std::begin(x), n);
> > > > > > +}
> > > > > > +
> > > > > > +template <typename T, std::size_t n>
> > > > > > +std::span<T const> make_span(T const(&x)[n])
> > > > > > +{
> > > > > > + return std::span(std::begin(x), n);
> > > > > > +}
> > > > > > +
> > > > > > +int main()
> > > > > > +{
> > > > > > + int x[10]{};
> > > > > > + int const& y{make_my_span(x)[0]};
> > > > > > + int const& y2{make_span(x)[0]};
> > > > >
> > > > > Let's also test that we do warn if the argument to make_*span is a
> > > > > temporary. OK with that change, assuming it works as expected.
> > > >
> > > > We do warn then, I've added
> > > > using T = int[10];
> > > > [[maybe_unused]] int const& y3{make_my_span(T{})[0]};
> > > > [[maybe_unused]] int const& y4{make_span(T{})[0]};
> > > > and get two warnings. I'll push with that adjusted; thanks.
> > >
> > > It looks like this patch breaks bootstrap on aarch64-linux-gnu, I see
> > > the following building aarch64-early-ra.cc in stage2:
> > >
> > > /work/builds/bstrap/./prev-gcc/xg++ -B/work/builds/bstrap/./prev-gcc/
> > > -B/work/builds/bstrap/aarch64-unknown-linux-gnu/bin/ -nostdinc++
> > > -B/work/builds/bstrap/prev-aarch64-unknown-linux-gnu/libstdc++-v3/src/.libs
> > >
> > > -B/work/builds/bstrap/prev-aarch64-unknown-linux-gnu/libstdc++-v3/libsupc++/.libs
> > >
> > > -I/work/builds/bstrap/prev-aarch64-unknown-linux-gnu/libstdc++-v3/include/aarch64-unknown-linux-gnu
> > >
> > > -I/work/builds/bstrap/prev-aarch64-unknown-linux-gnu/libstdc++-v3/include
> > > -I/home/alecop01/toolchain/src/gcc/libstdc++-v3/libsupc++
> > > -L/work/builds/bstrap/prev-aarch64-unknown-linux-gnu/libstdc++-v3/src/.libs
> > >
> > > -L/work/builds/bstrap/prev-aarch64-unknown-linux-gnu/libstdc++-v3/libsupc++/.libs
> > > -fno-PIE -c -g -O2 -fno-checking -gtoggle -DIN_GCC -fno-exceptions
> > > -fno-rtti -fasynchronous-unwind-tables -W -Wall -Wno-narrowing
> > > -Wwrite-strings -Wcast-qual -Wmissing-format-attribute
> > > -Wconditionally-supported -Woverloaded-virtual -pedantic -Wno-long-long
> > > -Wno-variadic-macros -Wno-overlength-strings -Werror -fno-common
> > > -DHAVE_CONFIG_H -fno-PIE -I. -I. -I/home/alecop01/toolchain/src/gcc/gcc
> > > -I/home/alecop01/toolchain/src/gcc/gcc/.
> > > -I/home/alecop01/toolchain/src/gcc/gcc/../include
> > > -I/home/alecop01/toolchain/src/gcc/gcc/../libcpp/include
> > > -I/home/alecop01/toolchain/src/gcc/gcc/../libcody
> > > -I/home/alecop01/toolchain/src/gcc/gcc/../libdecnumber
> > > -I/home/alecop01/toolchain/src/gcc/gcc/../libdecnumber/bid
> > > -I../libdecnumber -I/home/alecop01/toolchain/src/gcc/gcc/../libbacktrace
> > > -I. -I. -I/home/alecop01/toolchain/src/gcc/gcc
> > > -I/home/alecop01/toolchain/src/gcc/gcc/.
> > > -I/home/alecop01/toolchain/src/gcc/gcc/../include
> > > -I/home/alecop01/toolchain/src/gcc/gcc/../libcpp/include
> > > -I/home/alecop01/toolchain/src/gcc/gcc/../libcody
> > > -I/home/alecop01/toolchain/src/gcc/gcc/../libdecnumber
> > > -I/home/alecop01/toolchain/src/gcc/gcc/../libdecnumber/bid
> > > -I../libdecnumber -I/home/alecop01/toolchain/src/gcc/gcc/../libbacktrace
> > > \
> > >
> > > /home/alecop01/toolchain/src/gcc/gcc/config/aarch64/aarch64-early-ra.cc
> > > /home/alecop01/toolchain/src/gcc/gcc/config/aarch64/aarch64-early-ra.cc:
> > > In member function ‘void
> > > {anonymous}::early_ra::record_constraints(rtx_insn*)’:
> > > /home/alecop01/toolchain/src/gcc/gcc/config/aarch64/aarch64-early-ra.cc:1858:66:
> > > error: possibly dangling reference to a temporary
> > > [-Werror=dangling-reference]
> > > 1858 | for (auto &allocno : get_allocno_subgroup (op).allocnos
> > > ())
> > > | ^
> > > /home/alecop01/toolchain/src/gcc/gcc/config/aarch64/aarch64-early-ra.cc:1858:65:
> > > note: the temporary was destroyed at the end of the full expression
> > > ‘(({anonymous}::early_ra*)this)->{anonymous}::early_ra::get_allocno_subgroup(op).{anonymous}::early_ra::allocno_subgroup::allocnos()’
> > > 1858 | for (auto &allocno : get_allocno_subgroup (op).allocnos
> > > ())
> > > | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~
> > > cc1plus: all warnings being treated as errors
> > > make[3]: ***
> > > [/home/alecop01/toolchain/src/gcc/gcc/config/aarch64/t-aarch64:200:
> > > aarch64-early-ra.o] Error 1
> > > make[3]: Leaving directory '/work/builds/bstrap/gcc'
> > > make[2]: *** [Makefile:5096: all-stage2-gcc] Error 2
> > > make[2]: Leaving directory '/work/builds/bstrap'
> > > make[1]: *** [Makefile:25405: stage2-bubble] Error 2
> > > make[1]: Leaving directory '/work/builds/bstrap'
> > > make: *** [Makefile:1100: all] Error 2
> >
> > Very sorry about that.
> >
> > > Is the patch expected to introduce new warnings?
> >
> > No, on the contrary, it was supposed to strictly reduce the # of warnings.
> >
> > > I'll try to reduce a testcase from this where we don't see the warning
> > > before and we see the warning afterwards.
> >
> > That would be great. I think the fix may be just removing one line.
>
> Here is a reduced testcase as promised:
>
> ```
> template <typename T> struct array_slice {
> using iterator = T *;
> iterator begin();
> iterator end();
> iterator m_base;
> };
> char recog_data_2;
> int record_constraints_op;
> struct early_ra {
> using operand_mask = int;
> struct allocno_info {
> int is_earlyclobbered;
> };
> struct allocno_subgroup {
> array_slice<allocno_info> allocnos();
> };
> allocno_subgroup get_allocno_subgroup(int);
> void record_constraints();
> };
> void early_ra::record_constraints() {
> operand_mask earlyclobber_operands, matched_operands, unmatched_operands,
> matches_operands, op_mask = operand_mask();
> auto record_operand = [&](int, int) {
> operand_mask overlaps;
> matches_operands |= overlaps;
> };
> for (int opno; recog_data_2; ++opno) {
> operand_mask op_mask = earlyclobber_operands |= op_mask;
> if (0)
> record_operand(1, 0);
> }
> if (op_mask || (matched_operands & unmatched_operands && 0))
> for (auto &allocno :
> get_allocno_subgroup(record_constraints_op).allocnos())
> allocno.is_earlyclobbered = true;
> }
> ```
Thanks. I've added a pointer member to allocno_subgroup so that it
reflects what we have in the codebase, and I'm testing a patch as well
as a bootstrap on aarch64 (with Jakub's fix to get past 113705).
Marek
