[C++] Fix PR86763, wrong-code with TYPE_TYPELESS_STORAGE

Message ID alpine.LSU.2.20.1808021028160.16707@zhemvz.fhfr.qr
State New
Headers show
Series
  • [C++] Fix PR86763, wrong-code with TYPE_TYPELESS_STORAGE
Related show

Commit Message

Richard Biener Aug. 2, 2018, 8:30 a.m.
This fixes the PR by making sure CLASSTYPE_AS_BASE types inherit
TYPE_TYPELESS_STORAGE from the main type so that types that inherit
a type with TYPE_TYPELESS_STORAGE also get TYPE_TYPELESS_STORAGE
propagated.

Bootstrap & regtest running on x86_64-unknown-linux-gnu, OK?

Thanks,
Richard.

2018-08-02  Richard Biener  <rguenther@suse.de>

	PR c++/86763
	* class.c (layout_class_type): Copy TYPE_TYPELESS_STORAGE
	to the CLASSTYPE_AS_BASE.

	* g++.dg/torture/pr86763.C: New testcase.

Comments

Nathan Sidwell Aug. 2, 2018, 2:19 p.m. | #1
On 08/02/2018 01:30 AM, Richard Biener wrote:
> 

> This fixes the PR by making sure CLASSTYPE_AS_BASE types inherit

> TYPE_TYPELESS_STORAGE from the main type so that types that inherit

> a type with TYPE_TYPELESS_STORAGE also get TYPE_TYPELESS_STORAGE

> propagated.

> 

> Bootstrap & regtest running on x86_64-unknown-linux-gnu, OK?

> 

> Thanks,

> Richard.

> 

> 2018-08-02  Richard Biener  <rguenther@suse.de>

> 

> 	PR c++/86763

> 	* class.c (layout_class_type): Copy TYPE_TYPELESS_STORAGE

> 	to the CLASSTYPE_AS_BASE.

> 

> 	* g++.dg/torture/pr86763.C: New testcase.


Ok, thanks.


-- 
Nathan Sidwell
Szabolcs Nagy Aug. 20, 2018, 9:29 a.m. | #2
On 02/08/18 09:30, Richard Biener wrote:
> --- gcc/testsuite/g++.dg/torture/pr86763.C	(nonexistent)

> +++ gcc/testsuite/g++.dg/torture/pr86763.C	(working copy)

> @@ -0,0 +1,36 @@

> +// { dg-do run }

> +// { dg-additional-options "-fschedule-insns2 -fstrict-aliasing" }

> +

> +#include <cstdint>

> +#include <cassert>

> +#include <time.h>

> +struct ID {

> +  uint64_t value;

> +};

> +uint64_t value(ID id) { return id.value; }

> +uint64_t gen { 1000 };

> +struct Msg {

> +  uint64_t time;

> +  ID id;

> +};

> +struct V {

> +  V() { }

> +  V(Msg const & msg) : msg(msg) { }

> +  Msg & get() { return msg; }

> +  Msg msg;

> +  char pad[237 - sizeof(Msg)];

> +};

> +struct T : V { using V::V; };

> +Msg init_msg() {

> +  Msg msg;

> +  timespec t;

> +  clock_gettime(CLOCK_REALTIME, &t);


this fails on targets without clock_gettime (baremetal)
or targets that need -lrt at link time.

can we use something else here?
e.g. time(&t) works on baremetal and does not depend on -lrt.

> +  msg.time = t.tv_sec + t.tv_nsec;

> +  msg.id.value = ++gen;

> +  return msg;

> +}

> +int main() {

> +  T t;

> +  t = init_msg();

> +  assert(value(t.get().id) == 1001);

> +}

>
Richard Biener Aug. 20, 2018, 9:33 a.m. | #3
On Mon, 20 Aug 2018, Szabolcs Nagy wrote:

> On 02/08/18 09:30, Richard Biener wrote:

> > --- gcc/testsuite/g++.dg/torture/pr86763.C	(nonexistent)

> > +++ gcc/testsuite/g++.dg/torture/pr86763.C	(working copy)

> > @@ -0,0 +1,36 @@

> > +// { dg-do run }

> > +// { dg-additional-options "-fschedule-insns2 -fstrict-aliasing" }

> > +

> > +#include <cstdint>

> > +#include <cassert>

> > +#include <time.h>

> > +struct ID {

> > +  uint64_t value;

> > +};

> > +uint64_t value(ID id) { return id.value; }

> > +uint64_t gen { 1000 };

> > +struct Msg {

> > +  uint64_t time;

> > +  ID id;

> > +};

> > +struct V {

> > +  V() { }

> > +  V(Msg const & msg) : msg(msg) { }

> > +  Msg & get() { return msg; }

> > +  Msg msg;

> > +  char pad[237 - sizeof(Msg)];

> > +};

> > +struct T : V { using V::V; };

> > +Msg init_msg() {

> > +  Msg msg;

> > +  timespec t;

> > +  clock_gettime(CLOCK_REALTIME, &t);

> 

> this fails on targets without clock_gettime (baremetal)

> or targets that need -lrt at link time.

> 

> can we use something else here?

> e.g. time(&t) works on baremetal and does not depend on -lrt.


You'd have to check that the original bug still reproduces.  Since
it depends on scheduling chances may be that it is fragile :/

I suppose we could restrict the testcase to { *-*-linux }?

Richard.

> > +  msg.time = t.tv_sec + t.tv_nsec;

> > +  msg.id.value = ++gen;

> > +  return msg;

> > +}

> > +int main() {

> > +  T t;

> > +  t = init_msg();

> > +  assert(value(t.get().id) == 1001);

> > +}

> > 

> 

> 


-- 
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)
Szabolcs Nagy Aug. 21, 2018, 9:14 a.m. | #4
On 20/08/18 10:33, Richard Biener wrote:
> On Mon, 20 Aug 2018, Szabolcs Nagy wrote:

>> On 02/08/18 09:30, Richard Biener wrote:

...
>>> +  clock_gettime(CLOCK_REALTIME, &t);

>>

>> this fails on targets without clock_gettime (baremetal)

>> or targets that need -lrt at link time.

>>

>> can we use something else here?

>> e.g. time(&t) works on baremetal and does not depend on -lrt.

> 

> You'd have to check that the original bug still reproduces.  Since

> it depends on scheduling chances may be that it is fragile :/

> 

> I suppose we could restrict the testcase to { *-*-linux }?

> 


i decided to restrict it to linux:

clock_gettime is not available on some baremetal targets
and may require -lrt on some non-linux targets.

OK for trunk?

gcc/testsuite/ChangeLog:
2018-08-21  Szabolcs Nagy  <szabolcs.nagy@arm.com>

	* g++.dg/torture/pr86763.C: Restrict to *-*-linux*.
diff --git a/gcc/testsuite/g++.dg/torture/pr86763.C b/gcc/testsuite/g++.dg/torture/pr86763.C
index 79523b24850..8455ac9ce12 100644
--- a/gcc/testsuite/g++.dg/torture/pr86763.C
+++ b/gcc/testsuite/g++.dg/torture/pr86763.C
@@ -1,6 +1,6 @@
-// { dg-do run }
+// { dg-do run { target { *-*-linux* } } }
 // { dg-additional-options "-fschedule-insns2 -fstrict-aliasing" }
-// { dg-additional-options "-lrt" { target *-*-linux-gnu } }
+// { dg-additional-options "-lrt" }
 
 #include <cstdint>
 #include <cassert>
Richard Biener Aug. 21, 2018, 9:18 a.m. | #5
On Tue, 21 Aug 2018, Szabolcs Nagy wrote:

> On 20/08/18 10:33, Richard Biener wrote:

> > On Mon, 20 Aug 2018, Szabolcs Nagy wrote:

> > > On 02/08/18 09:30, Richard Biener wrote:

> ...

> > > > +  clock_gettime(CLOCK_REALTIME, &t);

> > > 

> > > this fails on targets without clock_gettime (baremetal)

> > > or targets that need -lrt at link time.

> > > 

> > > can we use something else here?

> > > e.g. time(&t) works on baremetal and does not depend on -lrt.

> > 

> > You'd have to check that the original bug still reproduces.  Since

> > it depends on scheduling chances may be that it is fragile :/

> > 

> > I suppose we could restrict the testcase to { *-*-linux }?

> > 

> 

> i decided to restrict it to linux:

> 

> clock_gettime is not available on some baremetal targets

> and may require -lrt on some non-linux targets.

> 

> OK for trunk?


OK.  OK also for backports.

Richard.

> gcc/testsuite/ChangeLog:

> 2018-08-21  Szabolcs Nagy  <szabolcs.nagy@arm.com>

> 

> 	* g++.dg/torture/pr86763.C: Restrict to *-*-linux*.

Patch

Index: gcc/cp/class.c
===================================================================
--- gcc/cp/class.c	(revision 263209)
+++ gcc/cp/class.c	(working copy)
@@ -6243,6 +6243,7 @@  layout_class_type (tree t, tree *virtual
 				  bitsize_int (BITS_PER_UNIT)));
       SET_TYPE_ALIGN (base_t, rli->record_align);
       TYPE_USER_ALIGN (base_t) = TYPE_USER_ALIGN (t);
+      TYPE_TYPELESS_STORAGE (base_t) = TYPE_TYPELESS_STORAGE (t);
 
       /* Copy the non-static data members of T. This will include its
 	 direct non-virtual bases & vtable.  */
Index: gcc/testsuite/g++.dg/torture/pr86763.C
===================================================================
--- gcc/testsuite/g++.dg/torture/pr86763.C	(nonexistent)
+++ gcc/testsuite/g++.dg/torture/pr86763.C	(working copy)
@@ -0,0 +1,36 @@ 
+// { dg-do run }
+// { dg-additional-options "-fschedule-insns2 -fstrict-aliasing" }
+
+#include <cstdint>
+#include <cassert>
+#include <time.h>
+struct ID {
+  uint64_t value;
+};
+uint64_t value(ID id) { return id.value; }
+uint64_t gen { 1000 };
+struct Msg {
+  uint64_t time;
+  ID id;
+};
+struct V {
+  V() { }
+  V(Msg const & msg) : msg(msg) { }
+  Msg & get() { return msg; }
+  Msg msg;
+  char pad[237 - sizeof(Msg)];
+};
+struct T : V { using V::V; };
+Msg init_msg() {
+  Msg msg;
+  timespec t;
+  clock_gettime(CLOCK_REALTIME, &t);
+  msg.time = t.tv_sec + t.tv_nsec;
+  msg.id.value = ++gen;
+  return msg;
+}
+int main() {
+  T t;
+  t = init_msg();
+  assert(value(t.get().id) == 1001);
+}