Comment on attachment 45656 [details]
Minimal reproducer code

The while-loop can be removed as well, and it will still give an ICE.

This started to ICE with r259629 and stopped ICEing with r267272.
Not really sure if this is ice-on-invalid (and thus accepts-invalid) on the trunk or just ice-on-valid on the branch, given the reinterpret_casts in the constexpr body.

I think it's valid (but the constructor can't be used in a constant expression).

Ok, so:
struct A {
  int a {};
};
struct B {
  int b {};
  constexpr B (A *x) {
    int *c = &x->a;
    while (*c)
      c = reinterpret_cast<int *>((reinterpret_cast<char *>(c) + *c));
    *c = reinterpret_cast<char *>(this) - reinterpret_cast<char *>(c);
  }
};
struct C : A {
  B bar {this};
};

constexpr C foo {};

would then be accepts-invalid.

I've tried to fix that by doing:
--- gcc/cp/cp-gimplify.c	(revision 268765)
+++ gcc/cp/cp-gimplify.c	(working copy)
@@ -2307,6 +2307,22 @@ cp_fold (tree x)
 	  && TREE_OVERFLOW_P (x) && !TREE_OVERFLOW_P (op0))
 	TREE_OVERFLOW (x) = false;
 
+      /* Preserve REINTERPRET_CAST_P.  */
+      if (code == NOP_EXPR && REINTERPRET_CAST_P (org_x))
+	{
+	  if (TREE_CODE (x) == NOP_EXPR && REINTERPRET_CAST_P (x))
+	    break;
+	  if (TREE_CODE (x) == NOP_EXPR)
+	    {
+	      x = copy_node (x);
+	      REINTERPRET_CAST_P (x) = 1;
+	    }
+	  else
+	    {
+	      x = build1_loc (loc, code, TREE_TYPE (x), x);
+	      REINTERPRET_CAST_P (x) = 1;
+	    }
+	}
       break;
 
     case INDIRECT_REF:
and with that we reject the testcase again (like we've done in 8.x, so this part is [9 Regression] accepts-invalid).
But that also means we ICE again on the:
C bar {};

So, maybe we want to ignore that:
      if (REINTERPRET_CAST_P (t))
        {
          if (!ctx->quiet)
            error_at (cp_expr_loc_or_loc (t, input_location),
                      "a reinterpret_cast is not a constant expression");
          *non_constant_p = true;
          return t;
        }
for perhaps !ctx->manifestly_const_eval , as in when we really don't require constant expression.  But we don't really call constexpr with zero manifestly_const_eval anyway.

Though, I wonder if something isn't wrong with the r267272 commit, I'd have thought that constexpr evaluation should be done on the pre-cp_folded bodies and cp_fold could be removing REINTERPRET_CAST_Ps.

Bisected that to the finish_id_expression change, reverting the penultimate and antepenultimate hunk of https://gcc.gnu.org/viewcvs/gcc/trunk/gcc/cp/semantics.c?limit_changes=0&r1=267272&r2=267271&pathrev=267272
makes gcc trunk reject the #c4 testcase again and ICE again with C bar {} which should be valid.  Ah, and likely that is the reason why the #c4 patch makes a difference, previously it was just a REINTERPRET_CAST_P NOP_EXPR around PARM_DECL, but now it is REINTERPRET_CAST_P NOP_EXPR around the location wrapper nop and cp_fold optimizes that away.

Ah, so one problem is that while we save the inline bodies of functions before cp_fold_function, cp_fold_function is destructive and clobbers the saved copy.
cp_fold itself is (hopefully) not destructive and creates new trees, but cp_fold_function is destructive and happily modifies the trees shared with something else (in this case with the constexpr funcdef table).

One way out of this is throwing away cp_fold_r/cp_fold_function and teaching cp_fold to handle all the trees that cp_walk_tree can walk (dunno if we want to cache everything or not).  That means handling e.g. STATEMENT_LIST, BIND_EXPR, *_STMT etc.
Another possibility is to use cp_fold_r the way it is for non-constexpr functions, and for constexpr functions use instead a hand written walker that will handle everything that can appear in constexpr functions with copy on write behavior, say if it is processing a STATEMENT_LIST and determines a recursive call wants to fold one of the statements, unshare the whole STATEMENT_LIST, modify it on the copy (and the parent would notice and unshare etc.).
Yet another possibility is to use the inliner's copy_tree_body_r for this in
register_constexpr_fundef, but in a way that we don't create new decls.  I'm afraid it would still create new VLA types, can those appear in constexpr functions?

Jason, any preferences here?

Note, the above would fix the [9 Regression] part of this PR, that we don't reject that #c4 testcase.  We'd still need to figure out a fix for the [8 Regression] part that once this [9 Regression] part is fixed would become [8/9 Regression].  I guess it could fix various other accepts-invalid cases where the cp_folding which modifies the saved tree makes us accept invalid constexpr code.

I'll try to copy & adjust (mostly kill almost everything) copy_tree_body_r and use that in register_constexpr_fundef.  As we don't need to remap decls or types, 
I think we just need copy_statement_list for STATEMENT_LIST, remap_save_expr for SAVE_EXPR/TARGET_EXPR, copy_tree_r for the rest?

(In reply to Jakub Jelinek from comment #4)
> +      /* Preserve REINTERPRET_CAST_P.  */
> +      if (code == NOP_EXPR && REINTERPRET_CAST_P (org_x))
> +	{
> +	  if (TREE_CODE (x) == NOP_EXPR && REINTERPRET_CAST_P (x))
> +	    break;
> +	  if (TREE_CODE (x) == NOP_EXPR)
> +	    {
> +	      x = copy_node (x);
> +	      REINTERPRET_CAST_P (x) = 1;
> +	    }
> +	  else
> +	    {
> +	      x = build1_loc (loc, code, TREE_TYPE (x), x);
> +	      REINTERPRET_CAST_P (x) = 1;
> +	    }
> +	}
>        break;

Looks good, though can't we assume that if x != org_x it's a new NOP_EXPR so we can just set REINTERPRET_CAST_P?

> and with that we reject the testcase again (like we've done in 8.x, so this
> part is [9 Regression] accepts-invalid).
> But that also means we ICE again on the:
> C bar {};

I'm not seeing this.

> So, maybe we want to ignore that:
>       if (REINTERPRET_CAST_P (t))
>         {
>           if (!ctx->quiet)
>             error_at (cp_expr_loc_or_loc (t, input_location),
>                       "a reinterpret_cast is not a constant expression");
>           *non_constant_p = true;
>           return t;
>         }
> for perhaps !ctx->manifestly_const_eval , as in when we really don't require
> constant expression.

That should depend on ctx->strict, not manifestly_const_eval.

(In reply to Jakub Jelinek from comment #7)
> Ah, so one problem is that while we save the inline bodies of functions
> before cp_fold_function, cp_fold_function is destructive and clobbers the
> saved copy.
> cp_fold itself is (hopefully) not destructive and creates new trees, but
> cp_fold_function is destructive and happily modifies the trees shared with
> something else (in this case with the constexpr funcdef table).
> 
> Jason, any preferences here?

My theory has been to switch to saving pre-gimplification bodies of constexpr functions and doing constant evaluation using them.

Created attachment 45666 [details]
gcc9-pr89285-wip.patch

Non-working WIP.

I've tried this, thinking that we don't really need to duplicate decls etc. in the function and could just copy the statements.  Unfortunately, there are issues with that:
1) the genericization modifies in place the PARM_DECLs and RESULT_DECL, so mixing the pre-cp_fold_function/cp_genericize* statements (body) with the post cp_genericize* is not going to work flawlessly
2) apparently cxx_eval_constant_expression assumes post genericization IL:
      if (STATEMENT_CODE_P (TREE_CODE (t)))
        {
          /* This function doesn't know how to deal with pre-genericize
             statements; this can only happen with statement-expressions,
             so for now just fail.  */
          if (!ctx->quiet)
            error_at (EXPR_LOCATION (t),
                      "statement is not a constant expression");
        }
so it is upset e.g. about WHILE_STMT and others.

From the language conformance POV though, I think it is highly desirable to evaluate constexpr functions on the original, pre-folding/genericization, trees, because the middle-end (e.g. match.pd, fold-const.c) is not willing to guarantee not to fold away stuff that should be diagnosed as constexpr violations.

So, for 1), I'd think we could just use copy_fn and remember not just the body, but also parms and result in constexpr_fundef
For 2), I guess that would mean teaching constexpr.c about the pre-genericization *_STMT constructs.

Created attachment 45667 [details]
gcc9-pr89285-wip.patch

Updated patch to address 1).  For 2), I guess we need to handle e.g.
CLEANUP_STMT, IF_STMT, FOR_STMT, WHILE_STMT, DO_STMT, SWITCH_STMT, CONTINUE_STMT, BREAK_STMT in cxx_eval_constant_expression.

Created attachment 45671 [details]
gcc9-pr89285-wip.patch

Updated version that handles the above mentioned C++ stmt trees.

That said, there are still various regressions:
/usr/src/gcc/obj/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/vector.tcc:636:48:   in 'constexpr' expansion of 'std::vector<int, std::allocator<int> >::_S_use_relocate()'
/usr/src/gcc/obj/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/vector.tcc:636:8: error: modification of '#'result_decl' not supported by dump_expr#<expression error>' is not a constant expression
etc.

Created attachment 45673 [details]
gcc9-pr89285-wip.patch

Updated WIP patch.  There are still various regressions, but it is getting better.  Before I continue further on this, I'd like to know if I'm not going completely in a wrong direction.

Created attachment 45752 [details]
gcc9-pr89285.patch

Updated untested patch for the constexpr evaluation on pre-folding trees.

Author: jakub
Date: Thu Feb 21 21:21:25 2019
New Revision: 269078

URL: https://gcc.gnu.org/viewcvs?rev=269078&root=gcc&view=rev
Log:
	PR c++/89285
	* builtins.c (fold_builtin_arith_overflow): If first two args are
	INTEGER_CSTs, set intres and ovfres to constants rather than calls
	to ifn.

	* constexpr.c (struct constexpr_fundef): Add parms and result members.
	(retrieve_constexpr_fundef): Adjust for the above change.
	(register_constexpr_fundef): Save constexpr body with copy_fn,
	temporarily set DECL_CONTEXT on DECL_RESULT before that.
	(get_fundef_copy): Change FUN argument to FUNDEF with
	constexpr_fundef * type, grab body and parms/result out of
	constexpr_fundef struct and temporarily change it for copy_fn calls
	too.
	(cxx_eval_builtin_function_call): For __builtin_FUNCTION temporarily
	adjust current_function_decl from ctx->call context.  Test
	!potential_constant_expression instead of !is_constant_expression.
	(cxx_bind_parameters_in_call): Grab parameters from new_call.  Undo
	convert_for_arg_passing changes for TREE_ADDRESSABLE type passing.
	(cxx_eval_call_expression): Adjust get_fundef_copy caller.
	(cxx_eval_conditional_expression): For IF_STMT, allow then or else
	operands to be NULL.
	(label_matches): Handle BREAK_STMT and CONTINUE_STMT.
	(cxx_eval_loop_expr): Add support for FOR_STMT, WHILE_STMT and DO_STMT.
	(cxx_eval_switch_expr): Add support for SWITCH_STMT.
	(cxx_eval_constant_expression): Handle IF_STMT, FOR_STMT, WHILE_STMT,
	DO_STMT, CONTINUE_STMT, SWITCH_STMT, BREAK_STMT and CONTINUE_STMT.
	For SIZEOF_EXPR, recurse on the result of fold_sizeof_expr.  Ignore
	DECL_EXPR with USING_DECL operand.
	* lambda.c (maybe_add_lambda_conv_op): Build thisarg using
	build_int_cst to make it a valid constant expression.

	* g++.dg/ubsan/vptr-4.C: Expect reinterpret_cast errors.
	* g++.dg/cpp1y/constexpr-84192.C (f2): Adjust expected diagnostics.
	* g++.dg/cpp1y/constexpr-70265-2.C (foo): Adjust expected line of
	diagnostics.
	* g++.dg/cpp1y/constexpr-89285.C: New test.
	* g++.dg/cpp0x/constexpr-arith-overflow.C (add, sub, mul): Ifdef out
	for C++11.
	(TEST_ADD, TEST_SUB, TEST_MUL): Define to Assert (true) for C++11.
	* g++.dg/cpp0x/constexpr-arith-overflow2.C: New test.

Added:
    trunk/gcc/testsuite/g++.dg/cpp0x/constexpr-arith-overflow2.C
    trunk/gcc/testsuite/g++.dg/cpp1y/constexpr-89285.C
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/builtins.c
    trunk/gcc/cp/ChangeLog
    trunk/gcc/cp/constexpr.c
    trunk/gcc/cp/lambda.c
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/testsuite/g++.dg/cpp0x/constexpr-arith-overflow.C
    trunk/gcc/testsuite/g++.dg/cpp1y/constexpr-70265-2.C
    trunk/gcc/testsuite/g++.dg/cpp1y/constexpr-84192.C
    trunk/gcc/testsuite/g++.dg/ubsan/vptr-4.C

The above commit just brings 9.x into the similar state as 8.x on this testcase, thus it is ice-on-valid.

GCC 8.3 has been released.

Author: jakub
Date: Mon Feb 25 15:01:01 2019
New Revision: 269188

URL: https://gcc.gnu.org/viewcvs?rev=269188&root=gcc&view=rev
Log:
	PR c++/89285
	* g++.dg/cpp1y/constexpr-89285-2.C: New test.

Added:
    trunk/gcc/testsuite/g++.dg/cpp1y/constexpr-89285-2.C
Modified:
    trunk/gcc/testsuite/ChangeLog

Actually, #c16 comment is incorrect, that was the case of the earlier approach (#c4) but not of what has been actually committed.
Keeping this open for 8.x, though the patch is unlikely to be backportable.

GCC 8.4.0 has been released, adjusting target milestone.

The GCC 8 branch is being closed, fixed in GCC 9.1.