[Bug c++/97388] New: constexpr evaluator incorrectly claims double delete with function parameter

[Bug c++/97388] New: constexpr evaluator incorrectly claims double delete with function parameter

[david at doublewise dot net]

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

            Bug ID: 97388
           Summary: constexpr evaluator incorrectly claims double delete
                    with function parameter
           Product: gcc
           Version: 11.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: david at doublewise dot net
  Target Milestone: ---

The following translation unit

```
struct S {
        int * m_ptr;

        constexpr S():
                m_ptr(new int())
        {
        }
        constexpr S(S && other) noexcept:
                m_ptr(other.m_ptr)
        {
                other.m_ptr = nullptr;
        }
        constexpr ~S() noexcept {
                delete m_ptr;
        }
};

constexpr bool test(S v) {
        auto x = static_cast<S &&>(v);
        return true;
}

static_assert(test(S()));
```

is rejected with 

```
<source>:23:19: error: non-constant condition for static assertion

   23 | static_assert(test(S()));

      |               ~~~~^~~~~

Compiler returned: 1
```

This problem does not occur if `v` is turned into a local variable instead of a
function parameter.

The error message is also not helpful in this case. It gives a much more
helpful (but still erroneous) error message if `std::allocator` is used instead
of `new` and `delete`:

```
<source>:28:19: error: non-constant condition for static assertion

   28 | static_assert(test(S()));

      |               ~~~~^~~~~

In file included from
/opt/compiler-explorer/gcc-trunk-20201012/include/c++/11.0.0/memory:64,

                 from <source>:1:

<source>:28:25:   in 'constexpr' expansion of '(&<anonymous>)->S::~S()'

<source>:17:36:   in 'constexpr' expansion of
'std::allocator<int>().std::allocator<int>::deallocate(((S*)this)->S::m_ptr,
1)'

/opt/compiler-explorer/gcc-trunk-20201012/include/c++/11.0.0/bits/allocator.h:183:30:
error: deallocation of already deallocated storage

  183 |             ::operator delete(__p);

      |             ~~~~~~~~~~~~~~~~~^~~~~

Compiler returned: 1
```

See it live: https://godbolt.org/z/hcv88h

[Bug c++/97388] constexpr evaluator incorrectly claims double delete with function parameter

[david at doublewise dot net]

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

David Stone <david at doublewise dot net> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |david at doublewise dot net

--- Comment #1 from David Stone <david at doublewise dot net> ---
This seems like it could be the same issue. The following code should be
rejected, but it's accepted.

```
#include <algorithm>

struct S {
        int * m_ptr;

        constexpr S():
                m_ptr(new int)
        {
        }
        S(const S&) = delete;
        S& operator=(const S&) = delete;
        constexpr ~S() {
                delete m_ptr;
        }
};

constexpr bool test(S v) {
        v.m_ptr = nullptr;
        return true;
}

static_assert(test(S()));
```

Here, we have a memory leak that fails to be reported. It seems like changes to
by-value function parameters are not maintained for the destructor?

[Bug c++/97388] constexpr evaluator incorrectly claims double delete with function parameter

[david at doublewise dot net]

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

--- Comment #2 from David Stone <david at doublewise dot net> ---
That is what seems to be happening here. It looks like by-value function
parameters have all modifications rolled back before the object is destroyed.
The following code is also erroneously rejected:

```
struct S {
        int m;

        constexpr S():
                m(1)
        {
        }
        constexpr ~S() noexcept(false) {
                if (m == 1) {
                        throw;
                }
        }
};

constexpr bool test(S v) {
        v.m = 2;
        return true;
}

static_assert(test(S()));
```

See it live: https://godbolt.org/z/qMjEfo

[Bug c++/97388] By-value function parameter changes are rolled back prior to destructor call during constant evaluation

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Assignee|unassigned at gcc dot gnu.org      |jakub at gcc dot gnu.org
             Status|UNCONFIRMED                 |ASSIGNED
     Ever confirmed|0                           |1
   Last reconfirmed|                            |2020-10-19

--- Comment #3 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Created attachment 49400
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=49400&action=edit
gcc11-pr97388.patch

Untested fix.

[Bug c++/97388] By-value function parameter changes are rolled back prior to destructor call during constant evaluation

[cvs-commit at gcc dot gnu.org]

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

--- Comment #4 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Jason Merrill <jason@gcc.gnu.org>:

https://gcc.gnu.org/g:5afd90c5f36bf45291ca09ef3791f4a574e90d5d

commit r11-4541-g5afd90c5f36bf45291ca09ef3791f4a574e90d5d
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Tue Oct 20 09:33:20 2020 +0200

    c++: Fix constexpr dtors vs invisible ref [PR97388]

    For arguments passed by invisible reference, in the IL until genericization
    we have the source types on the callee side and while on the caller side
    we already pass references to the actual argument slot in the caller, we
    undo that in cxx_bind_parameters_in_call's
          if (TREE_ADDRESSABLE (type))
            /* Undo convert_for_arg_passing work here.  */
            x = convert_from_reference (x);
    This works fine most of the time, except when the type also has constexpr
    destructor; in that case the destructor is invoked in the caller and thus
    the unsharing we do to make sure that the callee doesn't modify caller's
    values is in that case undesirable, it prevents the changes done in the
    callee propagating to the caller which should see them for the constexpr
    dtor evaluation.

    The following patch fixes that.  While it could be perhaps done for all
    TREE_ADDRESSABLE types, I don't see the need to change the behavior
    if there is no constexpr non-trivial dtor.

    Jason: And we need to avoid memoizing the call, because a later equivalent
    call also needs to modify its argument.  And we don't need to unshare
    constructors when we aren't memoizing the call, because we already unshared
    them when evaluating the TARGET_EXPR representing the copy-initialization
of
    the argument.

    2020-10-20  Jakub Jelinek  <jakub@redhat.com>
                Jason Merrill  <jason@redhat.com>

            PR c++/97388
            * constexpr.c (cxx_bind_parameters_in_call): Set non_constant_args
            if the parameter type has a non-trivial destructor.
            (cxx_eval_call_expression): Only unshare arguments if we're
            memoizing this evaluation.

            * g++.dg/cpp2a/constexpr-dtor5.C: New test.
            * g++.dg/cpp2a/constexpr-dtor6.C: New test.
            * g++.dg/cpp2a/constexpr-dtor7.C: New test.

[Bug c++/97388] By-value function parameter changes are rolled back prior to destructor call during constant evaluation

[cvs-commit at gcc dot gnu.org]

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

--- Comment #5 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Jason Merrill <jason@gcc.gnu.org>:

https://gcc.gnu.org/g:8895443a42db4045aad8e4b42cd5dd2ad6ffa7d7

commit r11-4542-g8895443a42db4045aad8e4b42cd5dd2ad6ffa7d7
Author: Jason Merrill <jason@redhat.com>
Date:   Wed Oct 28 17:30:05 2020 -0400

    c++: Fix constexpr cleanup error handling.

    In this testcase, the primary evaluation successfully produces 'true', and
    then running one of the cleanups hits a double delete, making the whole
    thing not a valid constant expression.  So we were returning 'true' wrapped
    in a NOP_EXPR to indicate its non-constancy, but evaluating that again is a
    perfectly acceptable constant expression, so we weren't getting the verbose
    diagnostic we were looking for.

    So if non_constant_p gets set other than for overflow, go back to the
    original expression.

    With this change, we should never hit the manifestly_const_eval test, and
    the is-constant-evaluated1.C test passes without it.

    gcc/cp/ChangeLog:

            PR c++/97388
            * constexpr.c (cxx_eval_outermost_constant_expr): Revert to
            original expression if evaluation sets non_constant_p.

    gcc/testsuite/ChangeLog:

            PR c++/97388
            * g++.dg/cpp2a/constexpr-dtor8.C: New test.

[Bug c++/97388] By-value function parameter changes are rolled back prior to destructor call during constant evaluation

[jason at gcc dot gnu.org]

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

Jason Merrill <jason at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|---                         |FIXED
                 CC|                            |jason at gcc dot gnu.org

--- Comment #6 from Jason Merrill <jason at gcc dot gnu.org> ---
Fixed.