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 <pola...@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 <pola...@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 <pola...@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
