* C++ PATCH to fix wrong-code with pointer-to-data-members (PR c++/79687)
@ 2017-02-24 17:42 Marek Polacek
2017-02-24 19:13 ` Jason Merrill
0 siblings, 1 reply; 7+ messages in thread
From: Marek Polacek @ 2017-02-24 17:42 UTC (permalink / raw)
To: GCC Patches, Jason Merrill
I had an interesting time tracking down some of the problems with this code.
Hopefully I've sussed out now how this stuff works.
We've got
struct A { char c; };
char A::*p = &A::c;
static char A::*const q = p;
and then
&(a.*q) - &a.c
which should evaluate to 0. Here "p" will be 0, that's the offset from the
start of the struct to "c". "q" is const-qualified and static and initialized
with "p", so we get to cp_fold_maybe_rvalue -> decl_constant_value ->
constant_value_1. Now, NULL pointer-to-data-members are represented by -1, so
that a null pointer is distinguishable from an offset of the first member of a
struct (0). So constant_value_1 looks at the DECL_INITIAL of "q", which is -1,
a constant, we fold "q" to -1, and sadness ensues. I believe the -1 value is
only an internal representation and shouldn't be used like that.
Hence my attempt to fix this as below. Anybody see a problem with this
approach? My first version just skipped folding TYPE_PTRMEMDATA_P, much like
with REFERENCE_TYPEs above, but the problem is just with NULL member pointers,
methinks.
Bootstrapped/regtested on x86_64-linux, ok for trunk?
2017-02-24 Marek Polacek <polacek@redhat.com>
PR c++/79687
* cp-gimplify.c (cp_fold_maybe_rvalue): Don't fold NULL
pointer-to-data-members.
* g++.dg/expr/ptrmem8.C: New test.
* g++.dg/expr/ptrmem9.C: New test.
diff --git gcc/cp/cp-gimplify.c gcc/cp/cp-gimplify.c
index 3eec940..6de88af 100644
--- gcc/cp/cp-gimplify.c
+++ gcc/cp/cp-gimplify.c
@@ -1976,7 +1976,9 @@ cp_fold_maybe_rvalue (tree x, bool rval)
&& TREE_CODE (TREE_TYPE (x)) != REFERENCE_TYPE)
{
tree v = decl_constant_value (x);
- if (v != x && v != error_mark_node)
+ if (v != x
+ && v != error_mark_node
+ && !null_member_pointer_value_p (v))
{
x = v;
continue;
diff --git gcc/testsuite/g++.dg/expr/ptrmem8.C gcc/testsuite/g++.dg/expr/ptrmem8.C
index e69de29..c5a766a 100644
--- gcc/testsuite/g++.dg/expr/ptrmem8.C
+++ gcc/testsuite/g++.dg/expr/ptrmem8.C
@@ -0,0 +1,15 @@
+// PR c++/79687
+// { dg-do run }
+
+struct A
+{
+ char c;
+};
+
+int main()
+{
+ char A::* p = &A::c;
+ static char A::* const q = p;
+ A a;
+ return &(a.*q) - &a.c;
+}
diff --git gcc/testsuite/g++.dg/expr/ptrmem9.C gcc/testsuite/g++.dg/expr/ptrmem9.C
index e69de29..32ce777 100644
--- gcc/testsuite/g++.dg/expr/ptrmem9.C
+++ gcc/testsuite/g++.dg/expr/ptrmem9.C
@@ -0,0 +1,19 @@
+// PR c++/79687
+// { dg-do run }
+
+struct A
+{
+ char c;
+};
+
+int main()
+{
+ static char A::* p1 = &A::c;
+ char A::* const q1 = p1;
+
+ char A::* p2 = &A::c;
+ static char A::* const q2 = p2;
+
+ A a;
+ return (&(a.*q1) - &a.c) || (&(a.*q2) - &a.c);
+}
Marek
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: C++ PATCH to fix wrong-code with pointer-to-data-members (PR c++/79687)
2017-02-24 17:42 C++ PATCH to fix wrong-code with pointer-to-data-members (PR c++/79687) Marek Polacek
@ 2017-02-24 19:13 ` Jason Merrill
2017-02-28 20:41 ` Marek Polacek
0 siblings, 1 reply; 7+ messages in thread
From: Jason Merrill @ 2017-02-24 19:13 UTC (permalink / raw)
To: Marek Polacek; +Cc: GCC Patches
On Fri, Feb 24, 2017 at 8:22 AM, Marek Polacek <polacek@redhat.com> wrote:
> I had an interesting time tracking down some of the problems with this code.
> Hopefully I've sussed out now how this stuff works.
>
> We've got
>
> struct A { char c; };
> char A::*p = &A::c;
> static char A::*const q = p;
> and then
> &(a.*q) - &a.c
> which should evaluate to 0. Here "p" will be 0, that's the offset from the
> start of the struct to "c". "q" is const-qualified and static and initialized
> with "p", so we get to cp_fold_maybe_rvalue -> decl_constant_value ->
> constant_value_1. Now, NULL pointer-to-data-members are represented by -1, so
> that a null pointer is distinguishable from an offset of the first member of a
> struct (0). So constant_value_1 looks at the DECL_INITIAL of "q", which is -1,
> a constant, we fold "q" to -1, and sadness ensues. I believe the -1 value is
> only an internal representation and shouldn't be used like that.
Since q is initialized from p, it shouldn't have a DECL_INITIAL of -1;
that sounds like the bug.
Jason
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: C++ PATCH to fix wrong-code with pointer-to-data-members (PR c++/79687)
2017-02-24 19:13 ` Jason Merrill
@ 2017-02-28 20:41 ` Marek Polacek
2017-02-28 23:57 ` Jason Merrill
0 siblings, 1 reply; 7+ messages in thread
From: Marek Polacek @ 2017-02-28 20:41 UTC (permalink / raw)
To: Jason Merrill; +Cc: GCC Patches
On Fri, Feb 24, 2017 at 11:11:05AM -0800, Jason Merrill wrote:
> On Fri, Feb 24, 2017 at 8:22 AM, Marek Polacek <polacek@redhat.com> wrote:
> > I had an interesting time tracking down some of the problems with this code.
> > Hopefully I've sussed out now how this stuff works.
> >
> > We've got
> >
> > struct A { char c; };
> > char A::*p = &A::c;
> > static char A::*const q = p;
> > and then
> > &(a.*q) - &a.c
> > which should evaluate to 0. Here "p" will be 0, that's the offset from the
> > start of the struct to "c". "q" is const-qualified and static and initialized
> > with "p", so we get to cp_fold_maybe_rvalue -> decl_constant_value ->
> > constant_value_1. Now, NULL pointer-to-data-members are represented by -1, so
> > that a null pointer is distinguishable from an offset of the first member of a
> > struct (0). So constant_value_1 looks at the DECL_INITIAL of "q", which is -1,
> > a constant, we fold "q" to -1, and sadness ensues. I believe the -1 value is
> > only an internal representation and shouldn't be used like that.
>
> Since q is initialized from p, it shouldn't have a DECL_INITIAL of -1;
> that sounds like the bug.
The DECL_INITIAL of -1 comes from cp_finish_decl:
7038 The memory occupied by any object of static storage
7039 duration is zero-initialized at program startup before
7040 any other initialization takes place.
7041
7042 We cannot create an appropriate initializer until after
7043 the type of DECL is finalized. If DECL_INITIAL is set,
7044 then the DECL is statically initialized, and any
7045 necessary zero-initialization has already been performed. */
7046 if (TREE_STATIC (decl) && !DECL_INITIAL (decl))
7047 DECL_INITIAL (decl) = build_zero_init (TREE_TYPE (decl),
7048 /*nelts=*/NULL_TREE,
7049 /*static_storage_p=*/true);
Later on, we reach expand_static_init which creates an initializer for q (to
set it to p) guarded with __cxa_guard_acquire, but that DECL_INITIAL is left
untouched. My first attempts to clear the DECL_INITIAL had flopped and I
decided to do just this, but I can't see how this could be correct :(.
Any better ideas?
Bootstrapped/regtested on x86_64-linux.
2017-02-28 Marek Polacek <polacek@redhat.com>
PR c++/79687
* decl.c (expand_static_init): Clear DECL_INITIAL for
static pointer-to-data-members.
* g++.dg/expr/ptrmem8.C: New test.
* g++.dg/expr/ptrmem9.C: New test.
diff --git gcc/cp/decl.c gcc/cp/decl.c
index 828359f..9cfeaa1d 100644
--- gcc/cp/decl.c
+++ gcc/cp/decl.c
@@ -8139,6 +8139,14 @@ expand_static_init (tree decl, tree init)
finish_expr_stmt (init);
+ /* A static pointer-to-data-member had been initialized to -1,
+ and then initialized again with INIT. Remove its DECL_INITIAL
+ so that it won't confuse us later when folding. */
+ if (TYPE_PTRDATAMEM_P (TREE_TYPE (decl))
+ && DECL_INITIAL (decl)
+ && null_member_pointer_value_p (DECL_INITIAL (decl)))
+ DECL_INITIAL (decl) = NULL_TREE;
+
if (thread_guard)
{
finish_compound_stmt (inner_then_clause);
diff --git gcc/testsuite/g++.dg/expr/ptrmem8.C gcc/testsuite/g++.dg/expr/ptrmem8.C
index e69de29..c5a766a 100644
--- gcc/testsuite/g++.dg/expr/ptrmem8.C
+++ gcc/testsuite/g++.dg/expr/ptrmem8.C
@@ -0,0 +1,15 @@
+// PR c++/79687
+// { dg-do run }
+
+struct A
+{
+ char c;
+};
+
+int main()
+{
+ char A::* p = &A::c;
+ static char A::* const q = p;
+ A a;
+ return &(a.*q) - &a.c;
+}
diff --git gcc/testsuite/g++.dg/expr/ptrmem9.C gcc/testsuite/g++.dg/expr/ptrmem9.C
index e69de29..32ce777 100644
--- gcc/testsuite/g++.dg/expr/ptrmem9.C
+++ gcc/testsuite/g++.dg/expr/ptrmem9.C
@@ -0,0 +1,19 @@
+// PR c++/79687
+// { dg-do run }
+
+struct A
+{
+ char c;
+};
+
+int main()
+{
+ static char A::* p1 = &A::c;
+ char A::* const q1 = p1;
+
+ char A::* p2 = &A::c;
+ static char A::* const q2 = p2;
+
+ A a;
+ return (&(a.*q1) - &a.c) || (&(a.*q2) - &a.c);
+}
Marek
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: C++ PATCH to fix wrong-code with pointer-to-data-members (PR c++/79687)
2017-02-28 20:41 ` Marek Polacek
@ 2017-02-28 23:57 ` Jason Merrill
2017-03-01 15:17 ` Marek Polacek
0 siblings, 1 reply; 7+ messages in thread
From: Jason Merrill @ 2017-02-28 23:57 UTC (permalink / raw)
To: Marek Polacek; +Cc: GCC Patches
On Tue, Feb 28, 2017 at 10:10 AM, Marek Polacek <polacek@redhat.com> wrote:
> On Fri, Feb 24, 2017 at 11:11:05AM -0800, Jason Merrill wrote:
>> On Fri, Feb 24, 2017 at 8:22 AM, Marek Polacek <polacek@redhat.com> wrote:
>> > I had an interesting time tracking down some of the problems with this code.
>> > Hopefully I've sussed out now how this stuff works.
>> >
>> > We've got
>> >
>> > struct A { char c; };
>> > char A::*p = &A::c;
>> > static char A::*const q = p;
>> > and then
>> > &(a.*q) - &a.c
>> > which should evaluate to 0. Here "p" will be 0, that's the offset from the
>> > start of the struct to "c". "q" is const-qualified and static and initialized
>> > with "p", so we get to cp_fold_maybe_rvalue -> decl_constant_value ->
>> > constant_value_1. Now, NULL pointer-to-data-members are represented by -1, so
>> > that a null pointer is distinguishable from an offset of the first member of a
>> > struct (0). So constant_value_1 looks at the DECL_INITIAL of "q", which is -1,
>> > a constant, we fold "q" to -1, and sadness ensues. I believe the -1 value is
>> > only an internal representation and shouldn't be used like that.
>>
>> Since q is initialized from p, it shouldn't have a DECL_INITIAL of -1;
>> that sounds like the bug.
>
> The DECL_INITIAL of -1 comes from cp_finish_decl:
> 7038 The memory occupied by any object of static storage
> 7039 duration is zero-initialized at program startup before
> 7040 any other initialization takes place.
> 7041
> 7042 We cannot create an appropriate initializer until after
> 7043 the type of DECL is finalized. If DECL_INITIAL is set,
> 7044 then the DECL is statically initialized, and any
> 7045 necessary zero-initialization has already been performed. */
> 7046 if (TREE_STATIC (decl) && !DECL_INITIAL (decl))
> 7047 DECL_INITIAL (decl) = build_zero_init (TREE_TYPE (decl),
> 7048 /*nelts=*/NULL_TREE,
> 7049 /*static_storage_p=*/true);
Ah, that makes sense. We do want to do constant-initialization with
-1 before we do dynamic initialization with p.
So we need to detect in constant_value_1 that the variable has a
dynamic initializer and therefore return the variable rather than -1.
DECL_INITIALIZED_BY_CONSTANT_EXPRESSION_P seems useful, perhaps in
combination with DECL_NONTRIVIALLY_INITIALIZED.
Jason
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: C++ PATCH to fix wrong-code with pointer-to-data-members (PR c++/79687)
2017-02-28 23:57 ` Jason Merrill
@ 2017-03-01 15:17 ` Marek Polacek
2017-03-09 9:54 ` Marek Polacek
2017-03-09 16:20 ` Jason Merrill
0 siblings, 2 replies; 7+ messages in thread
From: Marek Polacek @ 2017-03-01 15:17 UTC (permalink / raw)
To: Jason Merrill; +Cc: GCC Patches
On Tue, Feb 28, 2017 at 01:12:38PM -1000, Jason Merrill wrote:
> On Tue, Feb 28, 2017 at 10:10 AM, Marek Polacek <polacek@redhat.com> wrote:
> > On Fri, Feb 24, 2017 at 11:11:05AM -0800, Jason Merrill wrote:
> >> On Fri, Feb 24, 2017 at 8:22 AM, Marek Polacek <polacek@redhat.com> wrote:
> >> > I had an interesting time tracking down some of the problems with this code.
> >> > Hopefully I've sussed out now how this stuff works.
> >> >
> >> > We've got
> >> >
> >> > struct A { char c; };
> >> > char A::*p = &A::c;
> >> > static char A::*const q = p;
> >> > and then
> >> > &(a.*q) - &a.c
> >> > which should evaluate to 0. Here "p" will be 0, that's the offset from the
> >> > start of the struct to "c". "q" is const-qualified and static and initialized
> >> > with "p", so we get to cp_fold_maybe_rvalue -> decl_constant_value ->
> >> > constant_value_1. Now, NULL pointer-to-data-members are represented by -1, so
> >> > that a null pointer is distinguishable from an offset of the first member of a
> >> > struct (0). So constant_value_1 looks at the DECL_INITIAL of "q", which is -1,
> >> > a constant, we fold "q" to -1, and sadness ensues. I believe the -1 value is
> >> > only an internal representation and shouldn't be used like that.
> >>
> >> Since q is initialized from p, it shouldn't have a DECL_INITIAL of -1;
> >> that sounds like the bug.
> >
> > The DECL_INITIAL of -1 comes from cp_finish_decl:
> > 7038 The memory occupied by any object of static storage
> > 7039 duration is zero-initialized at program startup before
> > 7040 any other initialization takes place.
> > 7041
> > 7042 We cannot create an appropriate initializer until after
> > 7043 the type of DECL is finalized. If DECL_INITIAL is set,
> > 7044 then the DECL is statically initialized, and any
> > 7045 necessary zero-initialization has already been performed. */
> > 7046 if (TREE_STATIC (decl) && !DECL_INITIAL (decl))
> > 7047 DECL_INITIAL (decl) = build_zero_init (TREE_TYPE (decl),
> > 7048 /*nelts=*/NULL_TREE,
> > 7049 /*static_storage_p=*/true);
>
> Ah, that makes sense. We do want to do constant-initialization with
> -1 before we do dynamic initialization with p.
>
> So we need to detect in constant_value_1 that the variable has a
> dynamic initializer and therefore return the variable rather than -1.
> DECL_INITIALIZED_BY_CONSTANT_EXPRESSION_P seems useful, perhaps in
> combination with DECL_NONTRIVIALLY_INITIALIZED.
Got it. I think the following should be the real fix. I ran g++ dg.exp
with some logging to see how often the new check triggers, and it only
triggered in the two new tests, so I'm fairly happy with that.
Bootstrapped/regtested on x86_64-linux, ok for trunk and 6?
2017-03-01 Marek Polacek <polacek@redhat.com>
PR c++/79687
* init.c (constant_value_1): Break if the variable has a dynamic
initializer.
* g++.dg/expr/ptrmem8.C: New test.
* g++.dg/expr/ptrmem9.C: New test.
diff --git gcc/cp/init.c gcc/cp/init.c
index 7ded37e..12e6bf4 100644
--- gcc/cp/init.c
+++ gcc/cp/init.c
@@ -2193,6 +2193,13 @@ constant_value_1 (tree decl, bool strict_p, bool return_aggregate_cst_ok_p)
if (TREE_CODE (init) == CONSTRUCTOR
&& !DECL_INITIALIZED_BY_CONSTANT_EXPRESSION_P (decl))
break;
+ /* If the variable has a dynamic initializer, don't use its
+ DECL_INITIAL which doesn't reflect the real value. */
+ if (VAR_P (decl)
+ && TREE_STATIC (decl)
+ && !DECL_INITIALIZED_BY_CONSTANT_EXPRESSION_P (decl)
+ && DECL_NONTRIVIALLY_INITIALIZED_P (decl))
+ break;
decl = unshare_expr (init);
}
return decl;
diff --git gcc/testsuite/g++.dg/expr/ptrmem8.C gcc/testsuite/g++.dg/expr/ptrmem8.C
index e69de29..c5a766a 100644
--- gcc/testsuite/g++.dg/expr/ptrmem8.C
+++ gcc/testsuite/g++.dg/expr/ptrmem8.C
@@ -0,0 +1,15 @@
+// PR c++/79687
+// { dg-do run }
+
+struct A
+{
+ char c;
+};
+
+int main()
+{
+ char A::* p = &A::c;
+ static char A::* const q = p;
+ A a;
+ return &(a.*q) - &a.c;
+}
diff --git gcc/testsuite/g++.dg/expr/ptrmem9.C gcc/testsuite/g++.dg/expr/ptrmem9.C
index e69de29..32ce777 100644
--- gcc/testsuite/g++.dg/expr/ptrmem9.C
+++ gcc/testsuite/g++.dg/expr/ptrmem9.C
@@ -0,0 +1,19 @@
+// PR c++/79687
+// { dg-do run }
+
+struct A
+{
+ char c;
+};
+
+int main()
+{
+ static char A::* p1 = &A::c;
+ char A::* const q1 = p1;
+
+ char A::* p2 = &A::c;
+ static char A::* const q2 = p2;
+
+ A a;
+ return (&(a.*q1) - &a.c) || (&(a.*q2) - &a.c);
+}
Marek
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: C++ PATCH to fix wrong-code with pointer-to-data-members (PR c++/79687)
2017-03-01 15:17 ` Marek Polacek
@ 2017-03-09 9:54 ` Marek Polacek
2017-03-09 16:20 ` Jason Merrill
1 sibling, 0 replies; 7+ messages in thread
From: Marek Polacek @ 2017-03-09 9:54 UTC (permalink / raw)
To: Jason Merrill; +Cc: GCC Patches
Ping.
On Wed, Mar 01, 2017 at 04:16:56PM +0100, Marek Polacek wrote:
> On Tue, Feb 28, 2017 at 01:12:38PM -1000, Jason Merrill wrote:
> > On Tue, Feb 28, 2017 at 10:10 AM, Marek Polacek <polacek@redhat.com> wrote:
> > > On Fri, Feb 24, 2017 at 11:11:05AM -0800, Jason Merrill wrote:
> > >> On Fri, Feb 24, 2017 at 8:22 AM, Marek Polacek <polacek@redhat.com> wrote:
> > >> > I had an interesting time tracking down some of the problems with this code.
> > >> > Hopefully I've sussed out now how this stuff works.
> > >> >
> > >> > We've got
> > >> >
> > >> > struct A { char c; };
> > >> > char A::*p = &A::c;
> > >> > static char A::*const q = p;
> > >> > and then
> > >> > &(a.*q) - &a.c
> > >> > which should evaluate to 0. Here "p" will be 0, that's the offset from the
> > >> > start of the struct to "c". "q" is const-qualified and static and initialized
> > >> > with "p", so we get to cp_fold_maybe_rvalue -> decl_constant_value ->
> > >> > constant_value_1. Now, NULL pointer-to-data-members are represented by -1, so
> > >> > that a null pointer is distinguishable from an offset of the first member of a
> > >> > struct (0). So constant_value_1 looks at the DECL_INITIAL of "q", which is -1,
> > >> > a constant, we fold "q" to -1, and sadness ensues. I believe the -1 value is
> > >> > only an internal representation and shouldn't be used like that.
> > >>
> > >> Since q is initialized from p, it shouldn't have a DECL_INITIAL of -1;
> > >> that sounds like the bug.
> > >
> > > The DECL_INITIAL of -1 comes from cp_finish_decl:
> > > 7038 The memory occupied by any object of static storage
> > > 7039 duration is zero-initialized at program startup before
> > > 7040 any other initialization takes place.
> > > 7041
> > > 7042 We cannot create an appropriate initializer until after
> > > 7043 the type of DECL is finalized. If DECL_INITIAL is set,
> > > 7044 then the DECL is statically initialized, and any
> > > 7045 necessary zero-initialization has already been performed. */
> > > 7046 if (TREE_STATIC (decl) && !DECL_INITIAL (decl))
> > > 7047 DECL_INITIAL (decl) = build_zero_init (TREE_TYPE (decl),
> > > 7048 /*nelts=*/NULL_TREE,
> > > 7049 /*static_storage_p=*/true);
> >
> > Ah, that makes sense. We do want to do constant-initialization with
> > -1 before we do dynamic initialization with p.
> >
> > So we need to detect in constant_value_1 that the variable has a
> > dynamic initializer and therefore return the variable rather than -1.
> > DECL_INITIALIZED_BY_CONSTANT_EXPRESSION_P seems useful, perhaps in
> > combination with DECL_NONTRIVIALLY_INITIALIZED.
>
> Got it. I think the following should be the real fix. I ran g++ dg.exp
> with some logging to see how often the new check triggers, and it only
> triggered in the two new tests, so I'm fairly happy with that.
>
> Bootstrapped/regtested on x86_64-linux, ok for trunk and 6?
>
> 2017-03-01 Marek Polacek <polacek@redhat.com>
>
> PR c++/79687
> * init.c (constant_value_1): Break if the variable has a dynamic
> initializer.
>
> * g++.dg/expr/ptrmem8.C: New test.
> * g++.dg/expr/ptrmem9.C: New test.
>
> diff --git gcc/cp/init.c gcc/cp/init.c
> index 7ded37e..12e6bf4 100644
> --- gcc/cp/init.c
> +++ gcc/cp/init.c
> @@ -2193,6 +2193,13 @@ constant_value_1 (tree decl, bool strict_p, bool return_aggregate_cst_ok_p)
> if (TREE_CODE (init) == CONSTRUCTOR
> && !DECL_INITIALIZED_BY_CONSTANT_EXPRESSION_P (decl))
> break;
> + /* If the variable has a dynamic initializer, don't use its
> + DECL_INITIAL which doesn't reflect the real value. */
> + if (VAR_P (decl)
> + && TREE_STATIC (decl)
> + && !DECL_INITIALIZED_BY_CONSTANT_EXPRESSION_P (decl)
> + && DECL_NONTRIVIALLY_INITIALIZED_P (decl))
> + break;
> decl = unshare_expr (init);
> }
> return decl;
> diff --git gcc/testsuite/g++.dg/expr/ptrmem8.C gcc/testsuite/g++.dg/expr/ptrmem8.C
> index e69de29..c5a766a 100644
> --- gcc/testsuite/g++.dg/expr/ptrmem8.C
> +++ gcc/testsuite/g++.dg/expr/ptrmem8.C
> @@ -0,0 +1,15 @@
> +// PR c++/79687
> +// { dg-do run }
> +
> +struct A
> +{
> + char c;
> +};
> +
> +int main()
> +{
> + char A::* p = &A::c;
> + static char A::* const q = p;
> + A a;
> + return &(a.*q) - &a.c;
> +}
> diff --git gcc/testsuite/g++.dg/expr/ptrmem9.C gcc/testsuite/g++.dg/expr/ptrmem9.C
> index e69de29..32ce777 100644
> --- gcc/testsuite/g++.dg/expr/ptrmem9.C
> +++ gcc/testsuite/g++.dg/expr/ptrmem9.C
> @@ -0,0 +1,19 @@
> +// PR c++/79687
> +// { dg-do run }
> +
> +struct A
> +{
> + char c;
> +};
> +
> +int main()
> +{
> + static char A::* p1 = &A::c;
> + char A::* const q1 = p1;
> +
> + char A::* p2 = &A::c;
> + static char A::* const q2 = p2;
> +
> + A a;
> + return (&(a.*q1) - &a.c) || (&(a.*q2) - &a.c);
> +}
>
> Marek
Marek
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: C++ PATCH to fix wrong-code with pointer-to-data-members (PR c++/79687)
2017-03-01 15:17 ` Marek Polacek
2017-03-09 9:54 ` Marek Polacek
@ 2017-03-09 16:20 ` Jason Merrill
1 sibling, 0 replies; 7+ messages in thread
From: Jason Merrill @ 2017-03-09 16:20 UTC (permalink / raw)
To: Marek Polacek; +Cc: GCC Patches
OK.
On Wed, Mar 1, 2017 at 10:16 AM, Marek Polacek <polacek@redhat.com> wrote:
> On Tue, Feb 28, 2017 at 01:12:38PM -1000, Jason Merrill wrote:
>> On Tue, Feb 28, 2017 at 10:10 AM, Marek Polacek <polacek@redhat.com> wrote:
>> > On Fri, Feb 24, 2017 at 11:11:05AM -0800, Jason Merrill wrote:
>> >> On Fri, Feb 24, 2017 at 8:22 AM, Marek Polacek <polacek@redhat.com> wrote:
>> >> > I had an interesting time tracking down some of the problems with this code.
>> >> > Hopefully I've sussed out now how this stuff works.
>> >> >
>> >> > We've got
>> >> >
>> >> > struct A { char c; };
>> >> > char A::*p = &A::c;
>> >> > static char A::*const q = p;
>> >> > and then
>> >> > &(a.*q) - &a.c
>> >> > which should evaluate to 0. Here "p" will be 0, that's the offset from the
>> >> > start of the struct to "c". "q" is const-qualified and static and initialized
>> >> > with "p", so we get to cp_fold_maybe_rvalue -> decl_constant_value ->
>> >> > constant_value_1. Now, NULL pointer-to-data-members are represented by -1, so
>> >> > that a null pointer is distinguishable from an offset of the first member of a
>> >> > struct (0). So constant_value_1 looks at the DECL_INITIAL of "q", which is -1,
>> >> > a constant, we fold "q" to -1, and sadness ensues. I believe the -1 value is
>> >> > only an internal representation and shouldn't be used like that.
>> >>
>> >> Since q is initialized from p, it shouldn't have a DECL_INITIAL of -1;
>> >> that sounds like the bug.
>> >
>> > The DECL_INITIAL of -1 comes from cp_finish_decl:
>> > 7038 The memory occupied by any object of static storage
>> > 7039 duration is zero-initialized at program startup before
>> > 7040 any other initialization takes place.
>> > 7041
>> > 7042 We cannot create an appropriate initializer until after
>> > 7043 the type of DECL is finalized. If DECL_INITIAL is set,
>> > 7044 then the DECL is statically initialized, and any
>> > 7045 necessary zero-initialization has already been performed. */
>> > 7046 if (TREE_STATIC (decl) && !DECL_INITIAL (decl))
>> > 7047 DECL_INITIAL (decl) = build_zero_init (TREE_TYPE (decl),
>> > 7048 /*nelts=*/NULL_TREE,
>> > 7049 /*static_storage_p=*/true);
>>
>> Ah, that makes sense. We do want to do constant-initialization with
>> -1 before we do dynamic initialization with p.
>>
>> So we need to detect in constant_value_1 that the variable has a
>> dynamic initializer and therefore return the variable rather than -1.
>> DECL_INITIALIZED_BY_CONSTANT_EXPRESSION_P seems useful, perhaps in
>> combination with DECL_NONTRIVIALLY_INITIALIZED.
>
> Got it. I think the following should be the real fix. I ran g++ dg.exp
> with some logging to see how often the new check triggers, and it only
> triggered in the two new tests, so I'm fairly happy with that.
>
> Bootstrapped/regtested on x86_64-linux, ok for trunk and 6?
>
> 2017-03-01 Marek Polacek <polacek@redhat.com>
>
> PR c++/79687
> * init.c (constant_value_1): Break if the variable has a dynamic
> initializer.
>
> * g++.dg/expr/ptrmem8.C: New test.
> * g++.dg/expr/ptrmem9.C: New test.
>
> diff --git gcc/cp/init.c gcc/cp/init.c
> index 7ded37e..12e6bf4 100644
> --- gcc/cp/init.c
> +++ gcc/cp/init.c
> @@ -2193,6 +2193,13 @@ constant_value_1 (tree decl, bool strict_p, bool return_aggregate_cst_ok_p)
> if (TREE_CODE (init) == CONSTRUCTOR
> && !DECL_INITIALIZED_BY_CONSTANT_EXPRESSION_P (decl))
> break;
> + /* If the variable has a dynamic initializer, don't use its
> + DECL_INITIAL which doesn't reflect the real value. */
> + if (VAR_P (decl)
> + && TREE_STATIC (decl)
> + && !DECL_INITIALIZED_BY_CONSTANT_EXPRESSION_P (decl)
> + && DECL_NONTRIVIALLY_INITIALIZED_P (decl))
> + break;
> decl = unshare_expr (init);
> }
> return decl;
> diff --git gcc/testsuite/g++.dg/expr/ptrmem8.C gcc/testsuite/g++.dg/expr/ptrmem8.C
> index e69de29..c5a766a 100644
> --- gcc/testsuite/g++.dg/expr/ptrmem8.C
> +++ gcc/testsuite/g++.dg/expr/ptrmem8.C
> @@ -0,0 +1,15 @@
> +// PR c++/79687
> +// { dg-do run }
> +
> +struct A
> +{
> + char c;
> +};
> +
> +int main()
> +{
> + char A::* p = &A::c;
> + static char A::* const q = p;
> + A a;
> + return &(a.*q) - &a.c;
> +}
> diff --git gcc/testsuite/g++.dg/expr/ptrmem9.C gcc/testsuite/g++.dg/expr/ptrmem9.C
> index e69de29..32ce777 100644
> --- gcc/testsuite/g++.dg/expr/ptrmem9.C
> +++ gcc/testsuite/g++.dg/expr/ptrmem9.C
> @@ -0,0 +1,19 @@
> +// PR c++/79687
> +// { dg-do run }
> +
> +struct A
> +{
> + char c;
> +};
> +
> +int main()
> +{
> + static char A::* p1 = &A::c;
> + char A::* const q1 = p1;
> +
> + char A::* p2 = &A::c;
> + static char A::* const q2 = p2;
> +
> + A a;
> + return (&(a.*q1) - &a.c) || (&(a.*q2) - &a.c);
> +}
>
> Marek
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2017-03-09 16:20 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-02-24 17:42 C++ PATCH to fix wrong-code with pointer-to-data-members (PR c++/79687) Marek Polacek
2017-02-24 19:13 ` Jason Merrill
2017-02-28 20:41 ` Marek Polacek
2017-02-28 23:57 ` Jason Merrill
2017-03-01 15:17 ` Marek Polacek
2017-03-09 9:54 ` Marek Polacek
2017-03-09 16:20 ` Jason Merrill
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).