Bug 106393 - Add warnings for common dangling problems
Summary: Add warnings for common dangling problems
Status: NEW
Alias: None
Product: gcc
Classification: Unclassified
Component: c++ (show other bugs)
Version: 13.0
: P3 enhancement
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords: diagnostic
Depends on:
Blocks: new-warning, new_warning
  Show dependency treegraph
 
Reported: 2022-07-21 18:31 UTC by Jonathan Wakely
Modified: 2024-06-13 13:53 UTC (History)
5 users (show)

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


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Wakely 2022-07-21 18:31:53 UTC
It would be great if G++ was able to warn about the dangling problems below, some of which are easier than others. Some might be too hard to do without -fanalyze support for C++, some might be practical to do by leveraging the new __reference_converts_from_temporary machinery to detect problems, some might need entirely new ideas.


Simple:

const int& f(const int& i) { return i; }
const int& i = f(10);

This creates a temporary from the literal, returns a reference to the temporary, but the temporary is destroyed at the end of the full expression.
We've had numerous bugs reported for this case when using std::max and std::min.


Hard:

std::string_view s = std::string("blah");

This creates a temporary std::string that copies the literal "blah" to its own memory (in this case in an internal buffer, but for longer strings into heap buffers), then calls a member function of std::string that creates a string view that holds a pointer to that memory, then destroys the std::string.

The analyzer should be able to tell that the pointer in the string view refers to something that either goes out of scope with the string, or is deallocated in the string's dtor. Doing it in the FE or ME without -fanalyze might be hard.

Maybe we could have a heuristic that warns when:

- constructing a local variable whose type is a borrowed view
- from an rvalue, non-view, non-borrowed range.

(Thanks to Ville V and Barry R for brainstorming this).
There will be some edge cases where the non-view range has iterators that do outlive the range itself, and borrowing them is safe, but that will be rare.

The FE would either have to check the std::ranges::view and std::ranges::borrowed_range concepts (and if they're not in scope, just don't check ... it's probably not a view or a range if <ranges> isn't included!) or as a simpler heuristic, just check for the value of the enable_view and enable_borrowed_range bool variable templates.


Galaxy Brain:

#include <ranges>
#include <string>
#include <iostream>

std::string f() { return "dangle dangle dangle"; }

int main()
{
  auto v = std::string_view(f()) | std::views::transform([](char c) {
    if (c == 'l') return 'e';
    if (c == 'e') return 'r';
    return c;
  });
  for (char c : v)
    std::cout << c;
  std::cout << '\n';
}

Constructing a temporary string view from a std::string is fine, but then we create another view of that temporary string view that outlives the thing the string view refers to (the temporary std::string). By the time we iterate over the range, the std::string is long gone.
Comment 1 GCC Commits 2022-10-26 19:14:11 UTC
The trunk branch has been updated by Marek Polacek <mpolacek@gcc.gnu.org>:

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

commit r13-3511-gd2249cd9adf5ae638577139177a50f7e62d8abd9
Author: Marek Polacek <polacek@redhat.com>
Date:   Fri Oct 14 10:05:57 2022 -0400

    c++: Implement -Wdangling-reference [PR106393]
    
    This patch implements a new experimental warning (enabled by -Wall) to
    detect references bound to temporaries whose lifetime has ended.  The
    primary motivation is the Note in
    <https://en.cppreference.com/w/cpp/algorithm/max>:
    
      Capturing the result of std::max by reference produces a dangling reference
      if one of the parameters is a temporary and that parameter is returned:
    
      int n = 1;
      const int& r = std::max(n-1, n+1); // r is dangling
    
    That's because both temporaries for n-1 and n+1 are destroyed at the end
    of the full expression.  With this warning enabled, you'll get:
    
    g.C:3:12: warning: possibly dangling reference to a temporary [-Wdangling-reference]
        3 | const int& r = std::max(n-1, n+1);
          |            ^
    g.C:3:24: note: the temporary was destroyed at the end of the full expression 'std::max<int>((n - 1), (n + 1))'
        3 | const int& r = std::max(n-1, n+1);
          |                ~~~~~~~~^~~~~~~~~~
    
    The warning works by checking if a reference is initialized with a function
    that returns a reference, and at least one parameter of the function is
    a reference that is bound to a temporary.  It assumes that such a function
    actually returns one of its arguments!  (I added code to check_return_expr
    to suppress the warning when we've seen the definition of the function
    and we can say that it can return a variable with static storage
    duration.)
    
    It warns when the function in question is a member function, but only if
    the function is invoked on a temporary object, otherwise the warning
    would emit loads of warnings for valid code like obj.emplace<T>({0}, 0).
    It does detect the dangling reference in:
    
      struct S {
        const S& self () { return *this; }
      };
      const S& s = S().self();
    
    It warns in member initializer lists as well:
    
      const int& f(const int& i) { return i; }
      struct S {
        const int &r;
        S() : r(f(10)) { }
      };
    
    I've run the testsuite/bootstrap with the warning enabled by default.
    There were just a few FAILs, all of which look like genuine bugs.
    A bootstrap with the warning enabled by default passed as well.
    
    When testing a previous version of the patch, there were many FAILs in
    libstdc++'s 22_locale/; all of them because the warning triggered on
    
      const test_type& obj = std::use_facet<test_type>(std::locale());
    
    but this code looks valid -- std::use_facet doesn't return a reference
    to its parameter.  Therefore I added a #pragma and code to suppress the
    warning.
    
            PR c++/106393
    
    gcc/c-family/ChangeLog:
    
            * c.opt (Wdangling-reference): New.
    
    gcc/cp/ChangeLog:
    
            * call.cc (expr_represents_temporary_p): New, factored out of...
            (conv_binds_ref_to_temporary): ...here.  Don't return false just
            because a ck_base is missing.  Use expr_represents_temporary_p.
            (do_warn_dangling_reference): New.
            (maybe_warn_dangling_reference): New.
            (extend_ref_init_temps): Call maybe_warn_dangling_reference.
            * cp-tree.h: Adjust comment.
            * typeck.cc (check_return_expr): Suppress -Wdangling-reference
            warnings.
    
    gcc/ChangeLog:
    
            * doc/invoke.texi: Document -Wdangling-reference.
    
    libstdc++-v3/ChangeLog:
    
            * include/bits/locale_classes.tcc: Add #pragma to disable
            -Wdangling-reference with std::use_facet.
    
    gcc/testsuite/ChangeLog:
    
            * g++.dg/cpp23/elision4.C: Use -Wdangling-reference, add dg-warning.
            * g++.dg/cpp23/elision7.C: Likewise.
            * g++.dg/warn/Wdangling-pointer-2.C: Use -Wno-dangling-reference.
            * g++.dg/warn/Wdangling-reference1.C: New test.
            * g++.dg/warn/Wdangling-reference2.C: New test.
            * g++.dg/warn/Wdangling-reference3.C: New test.
Comment 2 Marek Polacek 2022-10-26 19:15:29 UTC
The first test in the original post is now diagnosed.  The rest may have to be implemented in the analyzer.  I'm not sure if I should close the BZ.
Comment 3 Eric Gallager 2024-06-12 21:30:31 UTC
(In reply to Marek Polacek from comment #2)
> The rest may have to be implemented in the analyzer.

Hm, let's ask David?
Comment 4 David Malcolm 2024-06-13 13:53:52 UTC
(In reply to Eric Gallager from comment #3)
> (In reply to Marek Polacek from comment #2)
> > The rest may have to be implemented in the analyzer.
> 
> Hm, let's ask David?

Please open it as a fresh bug against "analyzer", cite the specific test cases you'd like it to handle, reference this bug, and add the new bug to the C++ analyzer tracker (bug 97110).

Note that to avoid burnout I'm still focusing on C for GCC 15.