On 10/7/20 2:48 PM, Marek Polacek wrote:
On Tue, Oct 06, 2020 at 01:06:37AM -0400, Jason Merrill via Gcc-patches wrote:
On 10/1/20 1:08 PM, Marek Polacek wrote:
@@ -2496,8 +2502,72 @@ cxx_eval_call_expression (const constexpr_ctx *ctx, tree
t,
new_obj = NULL_TREE;
}
}
+ /* Verify that the object we're invoking the function on is sane. */
+ else if (DECL_NONSTATIC_MEMBER_FUNCTION_P (fun)
+ /* maybe_add_lambda_conv_op creates a null 'this' pointer. */
+ && !LAMBDA_TYPE_P (CP_DECL_CONTEXT (fun)))
Let's look at lambda_static_thunk_p of ctx->call->fundef->decl instead; we
don't want to allow calling a lambda op() with a null object from other
contexts.
OK, fixed.
+ {
+ tree thisarg = TREE_VEC_ELT (new_call.bindings, 0);
+ if (integer_zerop (thisarg))
+ {
+ if (!ctx->quiet)
+ error_at (loc, "member call on null pointer is not allowed "
+ "in a constant expression");
+ *non_constant_p = true;
+ result = error_mark_node;
+ }
+ else
+ {
+ STRIP_NOPS (thisarg);
+ if (TREE_CODE (thisarg) == ADDR_EXPR)
+ thisarg = TREE_OPERAND (thisarg, 0);
+ /* Detect out-of-bounds accesses. */
+ if (TREE_CODE (thisarg) == ARRAY_REF)
+ {
+ eval_and_check_array_index (ctx, thisarg, /*allow_one_past*/false,
+ non_constant_p, overflow_p);
+ if (*non_constant_p)
+ result = error_mark_node;
+ }
+ /* Detect using an inactive member of a union. */
+ else if (TREE_CODE (thisarg) == COMPONENT_REF)
+ {
+ cxx_eval_component_reference (ctx, thisarg, /*lval*/false,
+ non_constant_p, overflow_p);
+ if (*non_constant_p)
+ result = error_mark_node;
+ }
+ /* Detect other invalid accesses like
- tree result = NULL_TREE;
+ X x;
+ (&x)[1].foo();
+
+ where we'll end up with &x p+ 1. */
Is there any way to combine this POINTER_PLUS_EXPR handling with
cxx_fold_pointer_plus_expression or cxx_fold_indirect_ref?
cxx_fold_pointer_plus_expression looked like a place where the new checking
could go but in the end it didn't work. E.g., it STRIP_NOPS the lhs of the
POINTER_PLUS_EXPR so any casts would get lost.
+ else if (TREE_CODE (thisarg) == POINTER_PLUS_EXPR)
+ {
+ tree op0 = TREE_OPERAND (thisarg, 0);
+ /* This shouldn't trigger if we're accessing a base class of
+ the object in question. */
+ if (TREE_CODE (op0) == ADDR_EXPR
+ && DECL_P (TREE_OPERAND (op0, 0)))
+ {
+ if (!ctx->quiet)
+ {
+ tree op1 = TREE_OPERAND (thisarg, 1);
+ if (integer_onep (op1))
+ error_at (loc, "cannot dereference one-past-the-end "
+ "pointer in a constant expression");
+ else
+ error_at (loc, "cannot access element %qE of a "
+ "non-array object in a constant expression",
+ op1);
+ }
+ *non_constant_p = true;
+ result = error_mark_node;
+ }
+ }
+ }
+ }
These checks seem like the requirement that "A reference shall be
initialized to refer to a valid object or function.", which matches with the
change to describe the object argument as a reference rather than a pointer.
Let's split this out into a separate function that expresses this, and
perhaps then also use it to check the initializer of a reference variable.
I've moved it to a new function called cxx_verify_object_argument.
But checking the initializers of reference variables is still not handled
in this patch, because I don't know how to do that. For something like
constexpr const int &r1 = (&x)[1].m;
store_init_value calls cxx_constant_init for the init, which is
(const int &) &(&x + 4)->m
It could also be something like
(const int &) &arr[3].m
That could also be an object argument for a member function, right? I'm
not sure what point you're making.
Maybe cxx_eval_constant_expression/NOP_EXPR should set some flag so that
we perform some checking when we encounter a POINTER_PLUS_EXPR or an
ARRAY_REF when evaluating op0 of the NOP_EXPR?
Hmm, I suppose it might make sense to check for a valid object when we
see a conversion to reference rather than wait for initialization.
I've attached constexpr-ref13.C which is a testcase I played with; we
diagnose no errors there.
And let's call it from cxx_bind_parameters_in_call.
Done.
Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?
-- >8 --
This PR points out that when we're invoking a non-static member function
on a null instance during constant evaluation, we should reject.
cxx_eval_call_expression calls cxx_bind_parameters_in_call which
evaluates function arguments, but it won't detect problems like these.
Well, ok, so use integer_zerop to detect a null 'this'. This also
detects member calls on a variable whose lifetime has ended, because
check_return_expr creates an artificial nullptr:
10195 else if (!processing_template_decl
10196 && maybe_warn_about_returning_address_of_local (retval,
loc)
10197 && INDIRECT_TYPE_P (valtype))
10198 retval = build2 (COMPOUND_EXPR, TREE_TYPE (retval), retval,
10199 build_zero_cst (TREE_TYPE (retval)));
It would be great if we could somehow distinguish between those two
cases, but experiments with setting TREE_THIS_VOLATILE on the zero
didn't work, so I left it be.
But by the same token, we should detect out-of-bounds accesses. For
this I'm (ab)using eval_and_check_array_index so that I don't have
to reimplement bounds checking yet again. But this only works for
ARRAY_REFs, so won't detect
X x;
(&x)[0].foo(); // ok
(&x)[1].foo(); // bad
so I've added a special handling of POINTER_PLUS_EXPRs.
While here, we should also detect using an inactive union member. For
that, I'm using cxx_eval_component_reference.
gcc/cp/ChangeLog:
PR c++/97230
* constexpr.c (eval_and_check_array_index): Forward declare.
(cxx_eval_component_reference): Likewise.
(cxx_verify_object_argument): New function.
(cxx_bind_parameters_in_call): Call it.
gcc/testsuite/ChangeLog:
PR c++/97230
* g++.dg/cpp0x/constexpr-member-fn1.C: New test.
---
gcc/cp/constexpr.c | 103 +++++++++++++++---
.../g++.dg/cpp0x/constexpr-member-fn1.C | 44 ++++++++
2 files changed, 134 insertions(+), 13 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/cpp0x/constexpr-member-fn1.C
diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c
index a118f8a810b..2155ed1ca19 100644
--- a/gcc/cp/constexpr.c
+++ b/gcc/cp/constexpr.c
@@ -1548,6 +1548,69 @@ free_constructor (tree t)
}
}
+static tree eval_and_check_array_index (const constexpr_ctx *, tree, bool,
+ bool *, bool *);
+static tree cxx_eval_component_reference (const constexpr_ctx *, tree,
+ bool, bool *, bool *);
+
+/* Verify that the object THISARG we're invoking the function on is sane. */
+
+static void
+cxx_verify_object_argument (const constexpr_ctx *ctx, tree thisarg,
+ bool *non_constant_p, bool *overflow_p)
+{
+ location_t loc = cp_expr_loc_or_input_loc (thisarg);
+ if (integer_zerop (thisarg))
+ {
+ if (!ctx->quiet)
+ error_at (loc, "member call on null pointer is not allowed "
+ "in a constant expression");
+ *non_constant_p = true;
+ }
+ else
+ {
+ STRIP_NOPS (thisarg);
+ if (TREE_CODE (thisarg) == ADDR_EXPR)
+ thisarg = TREE_OPERAND (thisarg, 0);
Treating the argument the same with or without & seems questionable;
what if we got the object address from an array of pointers or a pointer
member of a class? Won't we then check whether that pointer is a valid
object, rather than what it points to?
+ /* Detect out-of-bounds accesses. */
+ if (TREE_CODE (thisarg) == ARRAY_REF)
+ eval_and_check_array_index (ctx, thisarg, /*allow_one_past*/false,
+ non_constant_p, overflow_p);
+ /* Detect using an inactive member of a union. */
+ else if (TREE_CODE (thisarg) == COMPONENT_REF)
+ cxx_eval_component_reference (ctx, thisarg, /*lval*/false,
+ non_constant_p, overflow_p);
+ /* Detect other invalid accesses like
+
+ X x;
+ (&x)[1].foo();
+
+ where we'll end up with &x p+ 1. */
+ else if (TREE_CODE (thisarg) == POINTER_PLUS_EXPR)
+ {
+ tree op0 = TREE_OPERAND (thisarg, 0);
+ /* This shouldn't trigger if we're accessing a base class of
+ the object in question. */
+ if (TREE_CODE (op0) == ADDR_EXPR
+ && DECL_P (TREE_OPERAND (op0, 0)))
+ {
+ if (!ctx->quiet)
+ {
+ tree op1 = TREE_OPERAND (thisarg, 1);
+ if (integer_onep (op1))
+ error_at (loc, "cannot dereference one-past-the-end "
+ "pointer in a constant expression");
+ else
+ error_at (loc, "cannot access element %qE of a "
+ "non-array object in a constant expression",
+ op1);
+ }
+ *non_constant_p = true;
+ }
+ }
+ }
+}
I think you'll need to recurse to check compound lvalues like arr[1][2]
or foo.x.y.
Actually, I notice that you're using lval==false to try to read from the
lvalue, and that seems like a better more general strategy for checking
that the object is valid: if we can read from it, it's valid. The only
difference is that we don't care if all the subobjects have been
initialized. Can we just evaluate the object expression as an rvalue
rather than write new verification code?
/* Subroutine of cxx_eval_call_expression.
We are processing a call expression (either CALL_EXPR or
AGGR_INIT_EXPR) in the context of CTX. Evaluate
@@ -1605,21 +1668,35 @@ cxx_bind_parameters_in_call (const constexpr_ctx *ctx,
tree t,
/* For virtual calls, adjust the this argument, so that it is
the object on which the method is called, rather than
one of its bases. */
- if (i == 0 && DECL_VIRTUAL_P (fun))
+ if (i == 0)
{
- tree addr = arg;
- STRIP_NOPS (addr);
- if (TREE_CODE (addr) == ADDR_EXPR)
+ if (DECL_VIRTUAL_P (fun))
+ {
+ tree addr = arg;
+ STRIP_NOPS (addr);
+ if (TREE_CODE (addr) == ADDR_EXPR)
+ {
+ tree obj = TREE_OPERAND (addr, 0);
+ while (TREE_CODE (obj) == COMPONENT_REF
+ && DECL_FIELD_IS_BASE (TREE_OPERAND (obj, 1))
+ && !same_type_ignoring_top_level_qualifiers_p
+ (TREE_TYPE (obj), DECL_CONTEXT (fun)))
+ obj = TREE_OPERAND (obj, 0);
+ if (obj != TREE_OPERAND (addr, 0))
+ arg = build_fold_addr_expr_with_type (obj,
+ TREE_TYPE (arg));
+ }
+ }
+ else if (DECL_NONSTATIC_MEMBER_FUNCTION_P (fun)
+ && !DECL_CONSTRUCTOR_P (fun)
+ && !(ctx->call
+ && ctx->call->fundef
+ && lambda_static_thunk_p (ctx->call->fundef->decl)))
{
- tree obj = TREE_OPERAND (addr, 0);
- while (TREE_CODE (obj) == COMPONENT_REF
- && DECL_FIELD_IS_BASE (TREE_OPERAND (obj, 1))
- && !same_type_ignoring_top_level_qualifiers_p
- (TREE_TYPE (obj), DECL_CONTEXT (fun)))
- obj = TREE_OPERAND (obj, 0);
- if (obj != TREE_OPERAND (addr, 0))
- arg = build_fold_addr_expr_with_type (obj,
- TREE_TYPE (arg));
+ cxx_verify_object_argument (ctx, arg, non_constant_p,
+ overflow_p);
+ if (*non_constant_p && ctx->quiet)
+ return;
}
}
TREE_VEC_ELT (binds, i) = arg;
diff --git a/gcc/testsuite/g++.dg/cpp0x/constexpr-member-fn1.C
b/gcc/testsuite/g++.dg/cpp0x/constexpr-member-fn1.C
new file mode 100644
index 00000000000..8ba5b87286f
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp0x/constexpr-member-fn1.C
@@ -0,0 +1,44 @@
+// PR c++/97230
+// { dg-do compile { target c++11 } }
+
+struct X {
+ constexpr int foo () const { return 1; }
+};
+
+constexpr X *eval () { return nullptr; }
+constexpr const X *dead (X tmp) { return &tmp; } // { dg-warning "address of local
variable" }
+
+static constexpr X x;
+static constexpr X arr[3];
+
+constexpr const X *gimme () { return &x; }
+
+union U {
+ int i;
+ X x;
+};
+constexpr U u{42};
+
+void
+fn ()
+{
+ constexpr auto x1 = ((X *) nullptr)->foo(); // { dg-error "member call on null
pointer" }
+ constexpr auto x2 = (eval())->foo(); // { dg-error "member call on null
pointer" }
+ constexpr auto x3 = dead ({})->foo(); // { dg-error "member call" }
+ constexpr auto x4 = (&x)[0].foo();
+ constexpr auto x5 = (&x)[1].foo(); // { dg-error "cannot dereference
one-past-the-end pointer" }
+ constexpr auto x6 = (&x)[2].foo(); // { dg-error "cannot access element .2. of a
non-array object" }
+ constexpr auto x7 = (&x)[3].foo(); // { dg-error "cannot access element .3. of a
non-array object" }
+ constexpr auto x8 = arr[2].foo();
+ constexpr auto x9 = arr[3].foo(); // { dg-error "outside the bounds" }
+ constexpr auto *p = &arr[0];
+ constexpr auto x10 = (*p).foo();
+ constexpr auto x11 = p->foo();
+ constexpr auto x12 = (*(p + 1)).foo();
+ constexpr auto x13 = (*(p + 3)).foo(); // { dg-error "outside the bounds" }
+ constexpr auto x14 = (p + 3)->foo(); // { dg-error "outside the bounds" }
+ constexpr auto x15 = gimme()->foo();
+ constexpr auto x16 = (gimme() + 1)->foo(); // { dg-error "cannot dereference
one-past-the-end pointer" }
+ constexpr auto x17 = (gimme() + 2)->foo(); // { dg-error "cannot access element
.2. of a non-array object" }
+ constexpr auto x18 = u.x.foo(); // { dg-error "accessing .U::x. member instead of
initialized .U::i. member" }
+}
base-commit: 1e247c60df52e93c9814a3a1789a63bc07aa4542
