Bug 107532 - [13 Regression] -Werror=dangling-reference false positives in libcamera-0.0.1
Summary: [13 Regression] -Werror=dangling-reference false positives in libcamera-0.0.1
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: c++ (show other bugs)
Version: 13.0
: P1 normal
Target Milestone: 13.0
Assignee: Marek Polacek
URL:
Keywords: diagnostic
Depends on:
Blocks:
 
Reported: 2022-11-05 05:48 UTC by Sergei Trofimovich
Modified: 2023-12-31 05:58 UTC (History)
18 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2022-11-11 00:00:00


Attachments
Unreduced test-case (221.50 KB, application/zstd)
2023-03-09 11:47 UTC, Martin Liška
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Sergei Trofimovich 2022-11-05 05:48:24 UTC
libcamera-0.0.1 and a few other projects (like cvise) fail to compile due to -Werror on -Werror=dangling-reference.

I think those are false positives related to value-like types that pass references through.

Extracted example from libcamera:

// $ cat a.cpp.cpp

struct Plane { unsigned int bytesused; };

// Passes a reference through. Does not change lifetime.
template <typename Inner>
struct Ref {
    const Inner & i_;
    Ref(const Inner & i) : i_(i) {}
    const Inner & inner() { return i_; }
};

struct FrameMetadata {
    Ref<const Plane> planes() const { return p_; }

    Plane p_;
};

void bar(const Plane & meta);
void foo(const FrameMetadata & fm)
{
    const Plane & meta = fm.planes().inner();
    bar(meta);
}

$ g++-13.0.0 -c -Wall -Werror=dangling-reference a.cpp
a.cpp: In function 'void foo(const FrameMetadata&)':
a.cpp:20:19: error: possibly dangling reference to a temporary [-Werror=dangling-reference]
   20 |     const Plane & meta = fm.planes().inner();
      |                   ^~~~
a.cpp:20:43: note: the temporary was destroyed at the end of the full expression '(& fm)->FrameMetadata::planes().Ref<const Plane>::inner()'
   20 |     const Plane & meta = fm.planes().inner();
      |                          ~~~~~~~~~~~~~~~~~^~
cc1plus: some warnings being treated as errors

This gcc version is this week's gcc-13 snapshot with https://gcc.gnu.org/PR107488 applied on top.
Comment 1 Martin Liška 2022-11-21 10:20:28 UTC
@Marek: Any progress on this, please?
Comment 2 Martin Liška 2022-12-23 08:28:22 UTC
@Marek: PING
Comment 3 Marek Polacek 2023-01-16 16:43:52 UTC
Sorry about the long delay.  Unfortunately I'm not sure yet how to fix it.

We have
Ref<const Plane>::inner (&TARGET_EXPR <D.2839, FrameMetadata::planes ((const struct FrameMetadata *) fm)>)

which returns a ref and its arg is a temporary.
Comment 4 Jakub Jelinek 2023-01-24 17:38:26 UTC
Yeah, without analyzing what the function call (constructor in this case) actually does
hard to say whether the warning is ok or not.
Maybe some heuristics based on the types?
The reference in question is const Plane &, so see if the reference could bind to some member of the class that is destructed at the end of the statement (Ref<Plane>) or the class itself?
Even with that there can be false positives and false negatives,
e.g. the to be destructed temporary could contain a pointer to a heap allocated Plane and return a reference to that and then deallocate it in the destructor (it would then be a dangling reference that with the change wouldn't be warned about), or
on the other side, e.g. if the possibly dangling reference is const Whatever &
and the temporary is Whatever, it might be likely that the reference is to the
temporary, but it could be just a value of some member of it that would happen to have that type.
Comment 5 Marek Polacek 2023-01-24 17:41:28 UTC
(In reply to Jakub Jelinek from comment #4)
> Yeah, without analyzing what the function call (constructor in this case)
> actually does
> hard to say whether the warning is ok or not.
> Maybe some heuristics based on the types?

Yes, I'm about to post another patch in response to <https://gcc.gnu.org/pipermail/gcc-patches/2023-January/610353.html>.

> The reference in question is const Plane &, so see if the reference could
> bind to some member of the class that is destructed at the end of the
> statement (Ref<Plane>) or the class itself?
> Even with that there can be false positives and false negatives,
> e.g. the to be destructed temporary could contain a pointer to a heap
> allocated Plane and return a reference to that and then deallocate it in the
> destructor (it would then be a dangling reference that with the change
> wouldn't be warned about), or
> on the other side, e.g. if the possibly dangling reference is const Whatever
> &
> and the temporary is Whatever, it might be likely that the reference is to
> the
> temporary, but it could be just a value of some member of it that would
> happen to have that type.

It's tricky.  I've seen unfixable problems with range-based for loops, for instance.
Comment 6 Jakub Jelinek 2023-01-24 17:44:32 UTC
(In reply to Marek Polacek from comment #5)
> It's tricky.  I've seen unfixable problems with range-based for loops, for
> instance.

Shouldn't those be fixed once P2718R0 is implemented (at least for C++23)?
Comment 7 Marek Polacek 2023-01-24 17:50:30 UTC
Sadly, I'm pretty certain it won't get fixed by P2718R0.
Comment 8 Barnabás Pőcze 2023-02-01 14:44:36 UTC
Here is another very simple example that only uses STL types: https://gcc.godbolt.org/z/43cKxdqr3

  void f(const std::vector<int>& v)
  {
      const int& r = std::span<const int>(v)[0];
  }
Comment 9 ecurtin 2023-02-14 13:31:56 UTC
This is still causing problems in Fedora 38 onwards
Comment 10 Marek Polacek 2023-02-14 13:45:24 UTC
Sorry.  A patch is on review.
Comment 11 GCC Commits 2023-03-07 16:11:12 UTC
The trunk branch has been updated by Marek Polacek <mpolacek@gcc.gnu.org>:

https://gcc.gnu.org/g:80f0052b3924569af904d1bab0858fe985f33a94

commit r13-6529-g80f0052b3924569af904d1bab0858fe985f33a94
Author: Marek Polacek <polacek@redhat.com>
Date:   Tue Jan 17 17:34:58 2023 -0500

    c++: -Wdangling-reference with reference wrapper [PR107532]
    
    Here, -Wdangling-reference triggers where it probably shouldn't, causing
    some grief.  The code in question uses a reference wrapper with a member
    function returning a reference to a subobject of a non-temporary object:
    
      const Plane & meta = fm.planes().inner();
    
    I've tried a few approaches, e.g., checking that the member function's
    return type is the same as the type of the enclosing class (which is
    the case for member functions returning *this), but that then breaks
    Wdangling-reference4.C with std::optional<std::string>.
    
    This patch adjusts do_warn_dangling_reference so that we look through
    reference wrapper classes (meaning, has a reference member and a
    constructor taking the same reference type, or is std::reference_wrapper
    or std::ranges::ref_view) and don't warn for them, supposing that the
    member function returns a reference to a non-temporary object.
    
            PR c++/107532
    
    gcc/cp/ChangeLog:
    
            * call.cc (reference_like_class_p): New.
            (do_warn_dangling_reference): Add new bool parameter.  See through
            reference_like_class_p.
    
    gcc/testsuite/ChangeLog:
    
            * g++.dg/warn/Wdangling-reference8.C: New test.
            * g++.dg/warn/Wdangling-reference9.C: New test.
Comment 12 Marek Polacek 2023-03-07 16:11:43 UTC
Fixed.
Comment 13 Martin Liška 2023-03-08 11:00:52 UTC
I can confirm the reduced test-case is fixed, however the original file is still not. I've created: https://bugs.libcamera.org/show_bug.cgi?id=185
Comment 14 Martin Liška 2023-03-09 11:47:58 UTC
Created attachment 54621 [details]
Unreduced test-case

Marek, can you please check if the unreduced test-case is a false-positive of the warning or a real problem?
Comment 15 Marek Polacek 2023-03-10 16:46:10 UTC
Hmm, so it's this line

const FrameMetadata::Plane &meta = buffer->metadata().planes()[i];

and we complain because we have

libcamera::Span<const libcamera::FrameMetadata::Plane>::operator[] (&TARGET_EXPR <D.153699, libcamera::FrameMetadata::planes ((const struct FrameMetadata *) libcamera::FrameBuffer::metadata (NON_LVALUE_EXPR <VIEW_CONVERT_EXPR<struct FrameBuffer *>(buffer)>))>, (size_type) VIEW_CONVERT_EXPR<unsigned int>(i))

and the argument to operator[] is a temporary of type Span.  Span is not a reference wrapper class so I *think* this is a valid warning.
Comment 16 Jonathan Wakely 2023-03-10 16:52:35 UTC
Span is a view, so it is like reference-wrapper. The lifetime of the underlying data is not tied to the lifetime of the Span.
Comment 17 Barnabás Pőcze 2023-03-10 17:02:53 UTC
The simple test case with std::span still triggers the warning: https://gcc.godbolt.org/z/43cKxdqr3. I feel that without deeper code analysis such a warning will generate too many false positives and people will simply turn it off.
Comment 18 Marek Polacek 2023-03-10 17:17:25 UTC
(In reply to Jonathan Wakely from comment #16)
> Span is a view, so it is like reference-wrapper. The lifetime of the
> underlying data is not tied to the lifetime of the Span.

Aha, I could add a check for std::span but that wouldn't help, because here it's a custom-made Span.  And I don't think there's a pattern I could look for that would tell us "this is a std::span-like class".  :/

(In reply to Barnabás Pőcze from comment #17)
> The simple test case with std::span still triggers the warning:
> https://gcc.godbolt.org/z/43cKxdqr3. I feel that without deeper code
> analysis such a warning will generate too many false positives and people
> will simply turn it off.

There really haven't been that many, except this and one with range-based for loops.
Comment 19 Marek Polacek 2023-03-10 17:18:27 UTC
(In reply to Barnabás Pőcze from comment #17)
> The simple test case with std::span still triggers the warning:
> https://gcc.godbolt.org/z/43cKxdqr3. I feel that without deeper code
> analysis such a warning will generate too many false positives and people
> will simply turn it off.

...but I could at least add a std::span check to fix this test...
Comment 20 Rolf Eike Beer 2023-03-12 10:49:21 UTC
I'm running this rev:

g++-13 (Gentoo 13.0.1.9999 p, commit 27495bc8fe028d4a68e97ad12b52231772e48dcf) 13.0.1 20230308 (experimental)

And I still get a warning for this testcase:

// $ cat ref.cpp
#include <string>

const std::string &trigger(const std::string &server);

int verifyDevXml()
{
  const auto &str = trigger("");

  if (str.empty())
    return 1;

  return 0;
}
// $ g++-13 -Wdangling-reference -c -o ref.o ref.cpp
ref.cpp: In function ?int verifyDevXml()?:
ref.cpp:7:15: warning: possibly dangling reference to a temporary [-Wdangling-reference]
    7 |   const auto &str = trigger("");
      |               ^~~
ref.cpp:7:28: note: the temporary was destroyed at the end of the full expression ?trigger(std::__cxx11::basic_string<char>(((const char*)""), std::allocator<char>()))?
    7 |   const auto &str = trigger("");
      |                     ~~~~~~~^~~~

The problem is to my understanding that this warns about the temporary constructed in the argument, i.e. it warns that the std::string() formed from "" could be bound here. Which could be true if the function does that, but in my case it is just used as a lookup for a map and not returned.
Comment 21 Kohei Takahashi 2023-03-12 13:37:53 UTC
(In reply to Marek Polacek from comment #18)
> (In reply to Barnabás Pőcze from comment #17)
> > The simple test case with std::span still triggers the warning:
> > https://gcc.godbolt.org/z/43cKxdqr3. I feel that without deeper code
> > analysis such a warning will generate too many false positives and people
> > will simply turn it off.
> 
> There really haven't been that many, except this and one with range-based
> for loops.

I think it warns many usage of zip_iterator idiom such as boost.iterator and P2321 style flat_map. Those uses tuple of references like std::tuple<T&...> by dereferencing iterator, so that any algorithms may yield this warning.
Comment 22 GCC Commits 2023-03-13 15:21:48 UTC
The trunk branch has been updated by Marek Polacek <mpolacek@gcc.gnu.org>:

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

commit r13-6633-gced122b849b8961b854053f0d1ac96983c5802e5
Author: Marek Polacek <polacek@redhat.com>
Date:   Fri Mar 10 12:23:13 2023 -0500

    c++: suppress -Wdangling-reference for std::span [PR107532]
    
    std::span is a view and therefore should be treated as a reference
    wrapper class for the purposes of -Wdangling-reference.  I'm not sure
    there's a pattern that we could check for.
    
            PR c++/107532
    
    gcc/cp/ChangeLog:
    
            * call.cc (reference_like_class_p): Check for std::span.
    
    gcc/testsuite/ChangeLog:
    
            * g++.dg/warn/Wdangling-reference10.C: New test.
Comment 23 Marek Polacek 2023-03-13 15:53:22 UTC
(In reply to Kohei Takahashi from comment #21)
> (In reply to Marek Polacek from comment #18)
> > (In reply to Barnabás Pőcze from comment #17)
> > > The simple test case with std::span still triggers the warning:
> > > https://gcc.godbolt.org/z/43cKxdqr3. I feel that without deeper code
> > > analysis such a warning will generate too many false positives and people
> > > will simply turn it off.
> > 
> > There really haven't been that many, except this and one with range-based
> > for loops.
> 
> I think it warns many usage of zip_iterator idiom such as boost.iterator and
> P2321 style flat_map. Those uses tuple of references like std::tuple<T&...>
> by dereferencing iterator, so that any algorithms may yield this warning.

Ah, would you please have a testcase?  If that's the case and the warning can't be taught to recognize that pattern, then I think we need to move it to -Wextra.  Thanks.
Comment 24 Kohei Takahashi 2023-03-14 23:09:07 UTC
(In reply to Marek Polacek from comment #23)
> (In reply to Kohei Takahashi from comment #21)
> > (In reply to Marek Polacek from comment #18)
> > > (In reply to Barnabás Pőcze from comment #17)
> > > > The simple test case with std::span still triggers the warning:
> > > > https://gcc.godbolt.org/z/43cKxdqr3. I feel that without deeper code
> > > > analysis such a warning will generate too many false positives and people
> > > > will simply turn it off.
> > > 
> > > There really haven't been that many, except this and one with range-based
> > > for loops.
> > 
> > I think it warns many usage of zip_iterator idiom such as boost.iterator and
> > P2321 style flat_map. Those uses tuple of references like std::tuple<T&...>
> > by dereferencing iterator, so that any algorithms may yield this warning.
> 
> Ah, would you please have a testcase?  If that's the case and the warning
> can't be taught to recognize that pattern, then I think we need to move it
> to -Wextra.  Thanks.

In my flat map implementation, https://github.com/Flast/flat_map, the warning is shown here https://github.com/Flast/flat_map/blob/f7d547fd4dbde763c07eb8d35796248c41989a66/flat_map/__flat_tree.hpp#LL435C42-L435C52 . You can reproduce it by following
```
flat_map$ mkdir build
flat_map$ cd build
flat_map/build$ cmake ..
flat_map/build$ make map_tie_test_17
```

`_key_extractor` is defined here, https://github.com/Flast/flat_map/blob/f7d547fd4dbde763c07eb8d35796248c41989a66/flat_map/flat_map.hpp#L89-L90, and the iterator dereference is here, https://github.com/Flast/flat_map/blob/f7d547fd4dbde763c07eb8d35796248c41989a66/flat_map/tied_sequence.hpp#L67-L72. Hence, reduced code is like https://wandbox.org/permlink/DloAyU3dQgydo7PS, or https://wandbox.org/permlink/7fM4NDF8u1hiRMFC.
Comment 25 Marek Polacek 2023-03-16 15:51:29 UTC
Maybe it would help to say that *any* class that has a reference member is a reference-wrapper and don't warn.
Comment 26 Marek Polacek 2023-03-16 16:24:53 UTC
(In reply to Kohei Takahashi from comment #24)
> (In reply to Marek Polacek from comment #23)
> > (In reply to Kohei Takahashi from comment #21)
> > > (In reply to Marek Polacek from comment #18)
> > > > (In reply to Barnabás Pőcze from comment #17)
> > > > > The simple test case with std::span still triggers the warning:
> > > > > https://gcc.godbolt.org/z/43cKxdqr3. I feel that without deeper code
> > > > > analysis such a warning will generate too many false positives and people
> > > > > will simply turn it off.
> > > > 
> > > > There really haven't been that many, except this and one with range-based
> > > > for loops.
> > > 
> > > I think it warns many usage of zip_iterator idiom such as boost.iterator and
> > > P2321 style flat_map. Those uses tuple of references like std::tuple<T&...>
> > > by dereferencing iterator, so that any algorithms may yield this warning.
> > 
> > Ah, would you please have a testcase?  If that's the case and the warning
> > can't be taught to recognize that pattern, then I think we need to move it
> > to -Wextra.  Thanks.
> 
> In my flat map implementation, https://github.com/Flast/flat_map, the
> warning is shown here
> https://github.com/Flast/flat_map/blob/
> f7d547fd4dbde763c07eb8d35796248c41989a66/flat_map/__flat_tree.hpp#LL435C42-
> L435C52 . You can reproduce it by following
> ```
> flat_map$ mkdir build
> flat_map$ cd build
> flat_map/build$ cmake ..
> flat_map/build$ make map_tie_test_17
> ```
> 
> `_key_extractor` is defined here,
> https://github.com/Flast/flat_map/blob/
> f7d547fd4dbde763c07eb8d35796248c41989a66/flat_map/flat_map.hpp#L89-L90, and
> the iterator dereference is here,
> https://github.com/Flast/flat_map/blob/
> f7d547fd4dbde763c07eb8d35796248c41989a66/flat_map/tied_sequence.hpp#L67-L72.
> Hence, reduced code is like https://wandbox.org/permlink/DloAyU3dQgydo7PS,
> or https://wandbox.org/permlink/7fM4NDF8u1hiRMFC.

Thanks for those reduced testcases.  I may be able to fix the warning there.
Comment 27 GCC Commits 2023-03-23 13:32:28 UTC
The trunk branch has been updated by Marek Polacek <mpolacek@gcc.gnu.org>:

https://gcc.gnu.org/g:59bfdd5f467292a368d0d628084a4b65d1bb06bb

commit r13-6830-g59bfdd5f467292a368d0d628084a4b65d1bb06bb
Author: Marek Polacek <polacek@redhat.com>
Date:   Fri Mar 17 14:36:10 2023 -0400

    c++: further -Wdangling-reference refinement [PR107532]
    
    Based on <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107532#c24>,
    it seems like we should treat *any* class with a reference member
    as a reference wrapper.  To suppress the warning in
    
      int i = 42;
      auto const& v = std::get<0>(std::tuple<int&>(i));
    
    we have to look into base classes as well.  For std::tuple, this means
    that we have to check the _Head_base subobject, which is a non-direct
    base class of std::tuple.  So I've employed a DFS walk.
    
            PR c++/107532
    
    gcc/cp/ChangeLog:
    
            * call.cc (class_has_reference_member_p): New.
            (class_has_reference_member_p_r): New.
            (reference_like_class_p): Don't look for a specific constructor.
            Use a DFS walk with class_has_reference_member_p_r.
    
    gcc/testsuite/ChangeLog:
    
            * g++.dg/warn/Wdangling-reference11.C: New test.
            * g++.dg/warn/Wdangling-reference12.C: New test.
Comment 28 Marek Polacek 2023-03-23 13:33:37 UTC
Fixed some more.
Comment 29 Andrey Alekseenko 2023-03-23 15:22:24 UTC
> it seems like we should treat *any* class with a reference member as a reference wrapper. 

And any class with a pointer, I suspect.

This is a reduced/simplified example from our codebase still triggering the error even with 59bfdd5f467292a368d0d628084a4b65d1bb06bb:

$ cat test.cpp 
struct ArrayRef
{
    ArrayRef(int* ptr) : ptr_(ptr) {}
    int& operator[](int n) const { return ptr_[n]; }
    int* ptr_;
};

int main()
{
    int        a;
    const int& r = ArrayRef(&a)[0];
}

$ g++ -std=c++17 -Wdangling-reference test.cpp -o test
test.cpp: In function ‘int main()’:
test.cpp:11:16: warning: possibly dangling reference to a temporary [-Wdangling-reference]
   11 |     const int& r = ArrayRef(&a)[0];
      |                ^
test.cpp:11:34: note: the temporary was destroyed at the end of the full expression ‘ArrayRef((& a)).ArrayRef::operator[](0)’
   11 |     const int& r = ArrayRef(&a)[0];
      |                                  ^
Comment 30 maic 2023-04-11 16:09:03 UTC
This bug still exists for our project. To reproduce:


# g++ --version 
g++ (GCC) 13.0.1 20230404 (Red Hat 13.0.1-0)


# cat /tmp/2.cpp 
const int &Select(const int &i, const bool &b) { return i; }
int main() {
  int a;
  const auto &b{Select(a, true)};
}


# g++ -Wdangling-reference /tmp/2.cpp 
/tmp/2.cpp: In function ‘int main()’:
/tmp/2.cpp:4:15: warning: possibly dangling reference to a temporary [-Wdangling-reference]
    4 |   const auto &b{Select(a, true)};
      |               ^
/tmp/2.cpp:4:23: note: the temporary was destroyed at the end of the full expression ‘Select(a, true)’
    4 |   const auto &b{Select(a, true)};
      |                 ~~~~~~^~~~~~~~~
Comment 31 maic 2023-04-12 19:46:14 UTC
Would be nice if this was re-opened, or should a new bug be filed?
Comment 32 Marek Polacek 2023-04-12 19:49:45 UTC
(In reply to maic from comment #31)
> Would be nice if this was re-opened, or should a new bug be filed?

This is the same problem as
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108165#c9

Unfortunately, it's not feasible to fix it in the front end, sorry about that.
Comment 33 Chris Uzdavinis 2023-04-27 22:29:51 UTC
Could an attribute or annotation be added so we can mark our classes to opt-out of this warning for them?

I like the warning in general but it is hitting one of our core "span-like" classes that stores pointers, and is going off so much that I'm going to need to disable it.  I'd be much happier disabling it on a per-object basis, rather than globally.
Comment 34 Boris Kolpackov 2023-06-12 03:25:35 UTC
Would like to echo other's comments: getting a large number false positives in our codebase (build2). Thankfully this warning seems to only be enabled with -Wextra and not with -Wall as stated in #106393.
Comment 35 Marek Polacek 2023-06-12 16:27:07 UTC
(In reply to Boris Kolpackov from comment #34)
> Would like to echo other's comments: getting a large number false positives
> in our codebase (build2). Thankfully this warning seems to only be enabled
> with -Wextra and not with -Wall as stated in #106393.

Yup.  I'm also planning to add a bunch of tweaks to significantly reduce the number of false positives (and devise a way for users to easily suppress the warning by adding a pragma around the class).  That work ought to happen in GCC 14.