diff mbox

Fix constexpr evaluation of comparisons involving pointer-to-members

Message ID alpine.DEB.2.20.9.1602031242530.1505@idea
State New
Headers show

Commit Message

Patrick Palka Feb. 3, 2016, 5:51 p.m. UTC 
On Tue, 2 Feb 2016, Jason Merrill wrote:

> On 12/22/2015 12:07 AM, Patrick Palka wrote:
>> +  if (code == EQ_EXPR || code == NE_EXPR)
>> +    {
>> +      if (TREE_CODE (lhs) == PTRMEM_CST && CONSTANT_CLASS_P (rhs))
>> +	lhs = cplus_expand_constant (lhs);
>> +      if (TREE_CODE (rhs) == PTRMEM_CST && CONSTANT_CLASS_P (lhs))
>> +	rhs = cplus_expand_constant (rhs);
>> +    }
>
> If both sides are PTRMEM_CST, we should be able to compare them without 
> expanding, using cp_tree_equal.

Ah, okay.  Here's an updated patch that uses cp_tree_equal and avoids
calling cplus_expand_constant altogether, by assuming that we should
only expect to compare a PTRMEM_CST against another PTRMEM_CST or
against the constant -1.  I also improved the coverage of the test case.
Does this look reasonable?


gcc/cp/ChangeLog:

 	* constexpr.c (cxx_eval_binary_expression): Fold equality
 	comparisons involving PTRMEM_CSTs.

gcc/testsuite/ChangeLog:

 	* g++.dg/cpp0x/constexpr-ptrmem5.C: New test.
---
  gcc/cp/constexpr.c                             | 21 +++++++++++++++++++--
  gcc/testsuite/g++.dg/cpp0x/constexpr-ptrmem5.C | 17 +++++++++++++++++
  2 files changed, 36 insertions(+), 2 deletions(-)
  create mode 100644 gcc/testsuite/g++.dg/cpp0x/constexpr-ptrmem5.C

Comments

Jason Merrill Feb. 4, 2016, 2:24 p.m. UTC | #1 
On 02/03/2016 12:51 PM, Patrick Palka wrote:
> +           && (integer_minus_onep (lhs)
> +           || integer_minus_onep (rhs)))

Let's use null_member_pointer_value_p here.

Please add pointers to member functions to the testcase.

Jason
Patrick Palka Feb. 4, 2016, 3:32 p.m. UTC | #2 
On Thu, Feb 4, 2016 at 9:24 AM, Jason Merrill <jason@redhat.com> wrote:
> On 02/03/2016 12:51 PM, Patrick Palka wrote:
>>
>> +           && (integer_minus_onep (lhs)
>> +           || integer_minus_onep (rhs)))
>
>
> Let's use null_member_pointer_value_p here.

Done.

>
> Please add pointers to member functions to the testcase.

This does not currently work properly because comparisons between
distinct pointers to (non-virtual) member functions eventually perform
a comparison between two distinct FUNCTION_DECLs, and fold_binary_loc
currently does not fold away such comparisons. So any static_asserts
that perform comparisons between distinct function pointers or between
distinct pointers to (non-virtual) member functions fail with
"non-constant condition for static assertion".  (Comparisons between
pointers to distinct virtual member functions _do_ work because in
that case we are ultimately just comparing integer offsets, not
FUNCTION_DECLs.  And comparisons between equivalent pointers trivially
work.)

I think the problem may lie in symtab_node::equal_address_to, which
apparently returns -1, instead of 0, for two obviously distinct
FUNCTION_DECLs.

Test case:

#define SA(x) static_assert ((x), #x)

void f ();
void g ();

struct X { void f (); void g (); };

void
foo ()
{
  SA (&f != &g);  // "non-constant expression"
  SA (&X::f != &X::g);  // "non-constant expression"
}
Jason Merrill Feb. 4, 2016, 4:57 p.m. UTC | #3 
On 02/04/2016 10:32 AM, Patrick Palka wrote:
> On Thu, Feb 4, 2016 at 9:24 AM, Jason Merrill <jason@redhat.com> wrote:
>> On 02/03/2016 12:51 PM, Patrick Palka wrote:
>>>
>>> +           && (integer_minus_onep (lhs)
>>> +           || integer_minus_onep (rhs)))
>>
>>
>> Let's use null_member_pointer_value_p here.
>
> Done.
>
>>
>> Please add pointers to member functions to the testcase.
>
> This does not currently work properly because comparisons between
> distinct pointers to (non-virtual) member functions eventually perform
> a comparison between two distinct FUNCTION_DECLs, and fold_binary_loc
> currently does not fold away such comparisons. So any static_asserts
> that perform comparisons between distinct function pointers or between
> distinct pointers to (non-virtual) member functions fail with
> "non-constant condition for static assertion".  (Comparisons between
> pointers to distinct virtual member functions _do_ work because in
> that case we are ultimately just comparing integer offsets, not
> FUNCTION_DECLs.  And comparisons between equivalent pointers trivially
> work.)
>
> I think the problem may lie in symtab_node::equal_address_to, which
> apparently returns -1, instead of 0, for two obviously distinct
> FUNCTION_DECLs.

Right, because they might be defined as aliases elsewhere.  But it looks 
like it should work if you define f and g.

> Test case:
>
> #define SA(x) static_assert ((x), #x)
>
> void f ();
> void g ();
>
> struct X { void f (); void g (); };
>
> void
> foo ()
> {
>    SA (&f != &g);  // "non-constant expression"
>    SA (&X::f != &X::g);  // "non-constant expression"
> }
>
Patrick Palka Feb. 4, 2016, 5:24 p.m. UTC | #4 
On Thu, Feb 4, 2016 at 11:57 AM, Jason Merrill <jason@redhat.com> wrote:
> On 02/04/2016 10:32 AM, Patrick Palka wrote:
>>
>> On Thu, Feb 4, 2016 at 9:24 AM, Jason Merrill <jason@redhat.com> wrote:
>>>
>>> On 02/03/2016 12:51 PM, Patrick Palka wrote:
>>>>
>>>>
>>>> +           && (integer_minus_onep (lhs)
>>>> +           || integer_minus_onep (rhs)))
>>>
>>>
>>>
>>> Let's use null_member_pointer_value_p here.
>>
>>
>> Done.
>>
>>>
>>> Please add pointers to member functions to the testcase.
>>
>>
>> This does not currently work properly because comparisons between
>> distinct pointers to (non-virtual) member functions eventually perform
>> a comparison between two distinct FUNCTION_DECLs, and fold_binary_loc
>> currently does not fold away such comparisons. So any static_asserts
>> that perform comparisons between distinct function pointers or between
>> distinct pointers to (non-virtual) member functions fail with
>> "non-constant condition for static assertion".  (Comparisons between
>> pointers to distinct virtual member functions _do_ work because in
>> that case we are ultimately just comparing integer offsets, not
>> FUNCTION_DECLs.  And comparisons between equivalent pointers trivially
>> work.)
>>
>> I think the problem may lie in symtab_node::equal_address_to, which
>> apparently returns -1, instead of 0, for two obviously distinct
>> FUNCTION_DECLs.
>
>
> Right, because they might be defined as aliases elsewhere.  But it looks
> like it should work if you define f and g.

That makes sense.  But even when I define f and g, the comparisons
don't get folded away:

#define SA(x) static_assert ((x), #x)

void f () { }
void g () { }

struct X { int a, b; void f (); void g (); };

void X::f () { }
void X::g () { }

void
foo ()
{
  SA (&f != &g);
  SA (&X::f != &X::g);
}

bool bar = &f != &g;

Although the static_asserts fail, the initialization of the variable
bar _does_ get folded to 1 during gimplification.  When evaluating the
static_asserts, symtab_node::equal_address_to again returns -1 but
during gimplification the same call to symtab_node::equal_address_to
returns 0.  So probably the symbols are not fully defined yet in the
symbol table at the point when we are evaluating the static_asserts.
Patrick Palka Feb. 4, 2016, 5:40 p.m. UTC | #5 
On Thu, Feb 4, 2016 at 12:24 PM, Patrick Palka <patrick@parcs.ath.cx> wrote:
> On Thu, Feb 4, 2016 at 11:57 AM, Jason Merrill <jason@redhat.com> wrote:
>> On 02/04/2016 10:32 AM, Patrick Palka wrote:
>>>
>>> On Thu, Feb 4, 2016 at 9:24 AM, Jason Merrill <jason@redhat.com> wrote:
>>>>
>>>> On 02/03/2016 12:51 PM, Patrick Palka wrote:
>>>>>
>>>>>
>>>>> +           && (integer_minus_onep (lhs)
>>>>> +           || integer_minus_onep (rhs)))
>>>>
>>>>
>>>>
>>>> Let's use null_member_pointer_value_p here.
>>>
>>>
>>> Done.
>>>
>>>>
>>>> Please add pointers to member functions to the testcase.
>>>
>>>
>>> This does not currently work properly because comparisons between
>>> distinct pointers to (non-virtual) member functions eventually perform
>>> a comparison between two distinct FUNCTION_DECLs, and fold_binary_loc
>>> currently does not fold away such comparisons. So any static_asserts
>>> that perform comparisons between distinct function pointers or between
>>> distinct pointers to (non-virtual) member functions fail with
>>> "non-constant condition for static assertion".  (Comparisons between
>>> pointers to distinct virtual member functions _do_ work because in
>>> that case we are ultimately just comparing integer offsets, not
>>> FUNCTION_DECLs.  And comparisons between equivalent pointers trivially
>>> work.)
>>>
>>> I think the problem may lie in symtab_node::equal_address_to, which
>>> apparently returns -1, instead of 0, for two obviously distinct
>>> FUNCTION_DECLs.
>>
>>
>> Right, because they might be defined as aliases elsewhere.  But it looks
>> like it should work if you define f and g.
>
> That makes sense.  But even when I define f and g, the comparisons
> don't get folded away:
>
> #define SA(x) static_assert ((x), #x)
>
> void f () { }
> void g () { }
>
> struct X { int a, b; void f (); void g (); };
>
> void X::f () { }
> void X::g () { }
>
> void
> foo ()
> {
>   SA (&f != &g);
>   SA (&X::f != &X::g);
> }
>
> bool bar = &f != &g;
>
> Although the static_asserts fail, the initialization of the variable
> bar _does_ get folded to 1 during gimplification.  When evaluating the
> static_asserts, symtab_node::equal_address_to again returns -1 but
> during gimplification the same call to symtab_node::equal_address_to
> returns 0.  So probably the symbols are not fully defined yet in the
> symbol table at the point when we are evaluating the static_asserts.

Indeed, symtab_node::equal_address_to returns 0 only if both symbols
have their ->analyzed flag set.  But cgraph_node::analyze() (which
sets the analyzed flag) is called during finalization of the
compilation unit -- obviously not in time for the static_asserts to
get folded.
Jason Merrill Feb. 4, 2016, 6:48 p.m. UTC | #6 
On 02/04/2016 12:24 PM, Patrick Palka wrote:
> On Thu, Feb 4, 2016 at 11:57 AM, Jason Merrill <jason@redhat.com> wrote:
>> On 02/04/2016 10:32 AM, Patrick Palka wrote:
>>>
>>> On Thu, Feb 4, 2016 at 9:24 AM, Jason Merrill <jason@redhat.com> wrote:
>>>>
>>>> On 02/03/2016 12:51 PM, Patrick Palka wrote:
>>>>>
>>>>>
>>>>> +           && (integer_minus_onep (lhs)
>>>>> +           || integer_minus_onep (rhs)))
>>>>
>>>>
>>>>
>>>> Let's use null_member_pointer_value_p here.
>>>
>>>
>>> Done.
>>>
>>>>
>>>> Please add pointers to member functions to the testcase.
>>>
>>>
>>> This does not currently work properly because comparisons between
>>> distinct pointers to (non-virtual) member functions eventually perform
>>> a comparison between two distinct FUNCTION_DECLs, and fold_binary_loc
>>> currently does not fold away such comparisons. So any static_asserts
>>> that perform comparisons between distinct function pointers or between
>>> distinct pointers to (non-virtual) member functions fail with
>>> "non-constant condition for static assertion".  (Comparisons between
>>> pointers to distinct virtual member functions _do_ work because in
>>> that case we are ultimately just comparing integer offsets, not
>>> FUNCTION_DECLs.  And comparisons between equivalent pointers trivially
>>> work.)
>>>
>>> I think the problem may lie in symtab_node::equal_address_to, which
>>> apparently returns -1, instead of 0, for two obviously distinct
>>> FUNCTION_DECLs.
>>
>>
>> Right, because they might be defined as aliases elsewhere.  But it looks
>> like it should work if you define f and g.
>
> That makes sense.  But even when I define f and g, the comparisons
> don't get folded away:
>
> #define SA(x) static_assert ((x), #x)
>
> void f () { }
> void g () { }
>
> struct X { int a, b; void f (); void g (); };
>
> void X::f () { }
> void X::g () { }
>
> void
> foo ()
> {
>    SA (&f != &g);
>    SA (&X::f != &X::g);
> }
>
> bool bar = &f != &g;
>
> Although the static_asserts fail, the initialization of the variable
> bar _does_ get folded to 1 during gimplification.  When evaluating the
> static_asserts, symtab_node::equal_address_to again returns -1 but
> during gimplification the same call to symtab_node::equal_address_to
> returns 0.  So probably the symbols are not fully defined yet in the
> symbol table at the point when we are evaluating the static_asserts.

Aha.  That seems wrong, but certainly not something to fix now.  The 
patch is OK.

Jason
diff mbox

Patch

diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c
index b076991..c5e6642 100644
--- a/gcc/cp/constexpr.c
+++ b/gcc/cp/constexpr.c
@@ -1593,7 +1593,7 @@  cxx_eval_binary_expression (const constexpr_ctx *ctx, tree t,
  			    bool /*lval*/,
  			    bool *non_constant_p, bool *overflow_p)
  {
-  tree r;
+  tree r = NULL_TREE;
    tree orig_lhs = TREE_OPERAND (t, 0);
    tree orig_rhs = TREE_OPERAND (t, 1);
    tree lhs, rhs;
@@ -1612,7 +1612,24 @@  cxx_eval_binary_expression (const constexpr_ctx *ctx, tree t,
    location_t loc = EXPR_LOCATION (t);
    enum tree_code code = TREE_CODE (t);
    tree type = TREE_TYPE (t);
-  r = fold_binary_loc (loc, code, type, lhs, rhs);
+
+  if ((code == EQ_EXPR || code == NE_EXPR))
+    {
+      bool is_code_eq = (code == EQ_EXPR);
+
+      if (TREE_CODE (lhs) == PTRMEM_CST
+	  && TREE_CODE (rhs) == PTRMEM_CST)
+	r = constant_boolean_node (cp_tree_equal (lhs, rhs) == is_code_eq,
+				   type);
+      else if ((TREE_CODE (lhs) == PTRMEM_CST
+		|| TREE_CODE (rhs) == PTRMEM_CST)
+	       && (integer_minus_onep (lhs)
+		   || integer_minus_onep (rhs)))
+	r = constant_boolean_node (code == NE_EXPR, type);
+    }
+
+  if (r == NULL_TREE)
+    r = fold_binary_loc (loc, code, type, lhs, rhs);
    if (r == NULL_TREE)
      {
        if (lhs == orig_lhs && rhs == orig_rhs)
diff --git a/gcc/testsuite/g++.dg/cpp0x/constexpr-ptrmem5.C b/gcc/testsuite/g++.dg/cpp0x/constexpr-ptrmem5.C
new file mode 100644
index 0000000..b1318c4
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp0x/constexpr-ptrmem5.C
@@ -0,0 +1,17 @@ 
+// { dg-do compile { target c++11 } }
+
+#define SA(x) static_assert ((x), #x)
+
+struct X { int a, b; };
+
+void
+foo ()
+{
+  SA (&X::a);
+  SA (&X::a == &X::a);
+  SA (!(&X::a != &X::a));
+  SA (&X::a != &X::b);
+  SA (!(&X::a == &X::b));
+  SA ((!&X::b) == 0);
+  SA (!(&X::b == 0));
+}