public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* [PATCH 2/2] Avoid default-initializing auto_vec<T, N> storage, fix vec<vl_embed>
@ 2023-02-24 11:44 Richard Biener
  2023-02-24 11:52 ` Jakub Jelinek
  0 siblings, 1 reply; 10+ messages in thread
From: Richard Biener @ 2023-02-24 11:44 UTC (permalink / raw)
  To: gcc-patches; +Cc: jwakely, Jakub Jelinek

The following avoids default-initializing auto_vec storage for
non-POD T since that's not what the allocated storage fallback
will do and it's also not expected for existing cases like

  auto_vec<std::pair<unsigned, unsigned>, 64> elts;

which exist to optimize the allocation.

It also fixes the array accesses done by vec<vl_embed> to not
use its own m_vecdata member but instead access the container
provided storage via pointer arithmetic.

This seems to work but it also somehow breaks genrecog which now
goes OOM with this change.  I'm going to see if the testsuite
shows anything, but maybe it's obvious from a second eye what
I did wrong ...

Comments welcome of course.

Thanks,
Richard.

	* vec.h (vec<T, A, vl_embed>::m_vecdata): Remove.
	(vec<T, A, vl_embed>::m_vecpfx): Align as T to avoid
	changing alignment of vec<T, A, vl_embed> and simplifying
	address.
	(vec<T, A, vl_embed>::address): Compute as this + 1.
	(vec<T, A, vl_embed>::embedded_size): Use sizeof the
	vector instead of the offset of the m_vecdata member.
	(auto_vec<T, N>): Turn m_data storage into
	uninitialized unsigned char aligned as T.
	* vec.cc (test_auto_alias): New.
	(vec_cc_tests): Call it.
---
 gcc/vec.cc | 17 +++++++++++++++++
 gcc/vec.h  | 11 +++++------
 2 files changed, 22 insertions(+), 6 deletions(-)

diff --git a/gcc/vec.cc b/gcc/vec.cc
index 511e6dff50d..2128f6666b1 100644
--- a/gcc/vec.cc
+++ b/gcc/vec.cc
@@ -568,6 +568,22 @@ test_auto_delete_vec ()
   ASSERT_EQ (dtor_count, 2);
 }
 
+/* Verify accesses to m_vecdata are done indirectly.  */
+
+static void
+test_auto_alias ()
+{
+  volatile int i = 1;
+  auto_vec<int, 8> v;
+  v.quick_grow (2);
+  v[0] = 1;
+  v[1] = 2;
+  int val;
+  for (int ix = i; v.iterate (ix, &val); ix++)
+    ASSERT_EQ (val, 2);
+  ASSERT_EQ (val, 0);
+}
+
 /* Run all of the selftests within this file.  */
 
 void
@@ -587,6 +603,7 @@ vec_cc_tests ()
   test_qsort ();
   test_reverse ();
   test_auto_delete_vec ();
+  test_auto_alias ();
 }
 
 } // namespace selftest
diff --git a/gcc/vec.h b/gcc/vec.h
index 5a2ee9c0294..b680efebe7a 100644
--- a/gcc/vec.h
+++ b/gcc/vec.h
@@ -586,8 +586,8 @@ public:
   unsigned allocated (void) const { return m_vecpfx.m_alloc; }
   unsigned length (void) const { return m_vecpfx.m_num; }
   bool is_empty (void) const { return m_vecpfx.m_num == 0; }
-  T *address (void) { return m_vecdata; }
-  const T *address (void) const { return m_vecdata; }
+  T *address (void) { return reinterpret_cast <T *> (this + 1); }
+  const T *address (void) const { return reinterpret_cast <const T *> (this + 1); }
   T *begin () { return address (); }
   const T *begin () const { return address (); }
   T *end () { return address () + length (); }
@@ -631,8 +631,7 @@ public:
 
   /* FIXME - These fields should be private, but we need to cater to
 	     compilers that have stricter notions of PODness for types.  */
-  vec_prefix m_vecpfx;
-  T m_vecdata[1];
+  alignas (T) vec_prefix m_vecpfx;
 };
 
 
@@ -1313,7 +1312,7 @@ vec<T, A, vl_embed>::embedded_size (unsigned alloc)
 				    vec, vec_embedded>::type vec_stdlayout;
   static_assert (sizeof (vec_stdlayout) == sizeof (vec), "");
   static_assert (alignof (vec_stdlayout) == alignof (vec), "");
-  return offsetof (vec_stdlayout, m_vecdata) + alloc * sizeof (T);
+  return sizeof (vec_stdlayout) + alloc * sizeof (T);
 }
 
 
@@ -1588,7 +1587,7 @@ public:
 
 private:
   vec<T, va_heap, vl_embed> m_auto;
-  T m_data[MAX (N - 1, 1)];
+  alignas(T) unsigned char m_data[sizeof (T) * N];
 };
 
 /* auto_vec is a sub class of vec whose storage is released when it is
-- 
2.35.3

^ permalink raw reply	[flat|nested] 10+ messages in thread
* [PATCH 2/2] Avoid default-initializing auto_vec<T, N> storage, fix vec<vl_embed>
@ 2023-02-24 13:47 Richard Biener
  2023-02-24 14:08 ` Jakub Jelinek
  0 siblings, 1 reply; 10+ messages in thread
From: Richard Biener @ 2023-02-24 13:47 UTC (permalink / raw)
  To: gcc-patches; +Cc: Jakub Jelinek

The following avoids default-initializing auto_vec storage for
non-POD T since that's not what the allocated storage fallback
will do and it's also not expected for existing cases like

  auto_vec<std::pair<unsigned, unsigned>, 64> elts;

which exist to optimize the allocation.

It also fixes the array accesses done by vec<vl_embed> to not
use its own m_vecdata member but instead access the container
provided storage via pointer arithmetic.

I've built the series with GCC 4.8 and clang 13 up to the stage1
target libs, a bootstrap and regtest on x86_64-unknown-linux-gnu
with GCC 12 was successful with the diagnostic pragma, I'm
currently re-bootstrapping and testing with a GCC 7 host compiler.

OK if that succeeds?

Thanks,
Richard.

	* vec.h (vec<T, A, vl_embed>::m_vecdata): Remove.
	(vec<T, A, vl_embed>::m_vecpfx): Align as T to avoid
	changing alignment of vec<T, A, vl_embed> and simplifying
	address.
	(vec<T, A, vl_embed>::address): Compute as this + 1.
	(vec<T, A, vl_embed>::embedded_size): Use sizeof the
	vector instead of the offset of the m_vecdata member.
	(auto_vec<T, N>::m_data): Turn storage into
	uninitialized unsigned char.
	(auto_vec<T, N>::auto_vec): Allow allocation of one
	stack member.  Initialize m_vec in a special way to
	avoid later stringop overflow diagnostics.
	* vec.cc (test_auto_alias): New.
	(vec_cc_tests): Call it.
---
 gcc/vec.cc | 17 +++++++++++++++++
 gcc/vec.h  | 27 +++++++++++++++++----------
 2 files changed, 34 insertions(+), 10 deletions(-)

diff --git a/gcc/vec.cc b/gcc/vec.cc
index 511e6dff50d..2128f6666b1 100644
--- a/gcc/vec.cc
+++ b/gcc/vec.cc
@@ -568,6 +568,22 @@ test_auto_delete_vec ()
   ASSERT_EQ (dtor_count, 2);
 }
 
+/* Verify accesses to m_vecdata are done indirectly.  */
+
+static void
+test_auto_alias ()
+{
+  volatile int i = 1;
+  auto_vec<int, 8> v;
+  v.quick_grow (2);
+  v[0] = 1;
+  v[1] = 2;
+  int val;
+  for (int ix = i; v.iterate (ix, &val); ix++)
+    ASSERT_EQ (val, 2);
+  ASSERT_EQ (val, 0);
+}
+
 /* Run all of the selftests within this file.  */
 
 void
@@ -587,6 +603,7 @@ vec_cc_tests ()
   test_qsort ();
   test_reverse ();
   test_auto_delete_vec ();
+  test_auto_alias ();
 }
 
 } // namespace selftest
diff --git a/gcc/vec.h b/gcc/vec.h
index 2b36f065234..3b03bfe076a 100644
--- a/gcc/vec.h
+++ b/gcc/vec.h
@@ -586,8 +586,9 @@ public:
   unsigned allocated (void) const { return m_vecpfx.m_alloc; }
   unsigned length (void) const { return m_vecpfx.m_num; }
   bool is_empty (void) const { return m_vecpfx.m_num == 0; }
-  T *address (void) { return m_vecdata; }
-  const T *address (void) const { return m_vecdata; }
+  T *address (void) { return reinterpret_cast <T *> (this + 1); }
+  const T *address (void) const
+    { return reinterpret_cast <const T *> (this + 1); }
   T *begin () { return address (); }
   const T *begin () const { return address (); }
   T *end () { return address () + length (); }
@@ -629,10 +630,10 @@ public:
   friend struct va_gc_atomic;
   friend struct va_heap;
 
-  /* FIXME - These fields should be private, but we need to cater to
+  /* FIXME - This field should be private, but we need to cater to
 	     compilers that have stricter notions of PODness for types.  */
-  vec_prefix m_vecpfx;
-  T m_vecdata[1];
+  /* Align m_vecpfx to simplify address ().  */
+  alignas (T) alignas (vec_prefix) vec_prefix m_vecpfx;
 };
 
 
@@ -1315,7 +1316,7 @@ vec<T, A, vl_embed>::embedded_size (unsigned alloc)
 				    vec, vec_embedded>::type vec_stdlayout;
   static_assert (sizeof (vec_stdlayout) == sizeof (vec), "");
   static_assert (alignof (vec_stdlayout) == alignof (vec), "");
-  return offsetof (vec_stdlayout, m_vecdata) + alloc * sizeof (T);
+  return sizeof (vec_stdlayout) + alloc * sizeof (T);
 }
 
 
@@ -1559,8 +1560,14 @@ class auto_vec : public vec<T, va_heap>
 public:
   auto_vec ()
   {
-    m_auto.embedded_init (MAX (N, 2), 0, 1);
-    this->m_vec = &m_auto;
+    m_auto.embedded_init (N, 0, 1);
+    /* ???  Instead of initializing m_vec from &m_auto directly use an
+       expression that avoids refering to a specific member of 'this'
+       to derail the -Wstringop-overflow diagnostic code, avoiding
+       the impression that data accesses are supposed to be to the
+       m_auto memmber storage.  */
+    size_t off = (char *) &m_auto - (char *) this;
+    this->m_vec = (vec<T, va_heap, vl_embed> *) ((char *) this + off);
   }
 
   auto_vec (size_t s CXX_MEM_STAT_INFO)
@@ -1571,7 +1578,7 @@ public:
 	return;
       }
 
-    m_auto.embedded_init (MAX (N, 2), 0, 1);
+    m_auto.embedded_init (N, 0, 1);
     this->m_vec = &m_auto;
   }
 
@@ -1590,7 +1597,7 @@ public:
 
 private:
   vec<T, va_heap, vl_embed> m_auto;
-  T m_data[MAX (N - 1, 1)];
+  unsigned char m_data[sizeof (T) * N];
 };
 
 /* auto_vec is a sub class of vec whose storage is released when it is
-- 
2.35.3

^ permalink raw reply	[flat|nested] 10+ messages in thread
end of thread, other threads:[~2023-02-24 14:13 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-02-24 11:44 [PATCH 2/2] Avoid default-initializing auto_vec<T, N> storage, fix vec<vl_embed> Richard Biener
2023-02-24 11:52 ` Jakub Jelinek
2023-02-24 11:54   ` Jonathan Wakely
2023-02-24 12:13     ` Jakub Jelinek
2023-02-24 12:15     ` Richard Biener
2023-02-24 12:26       ` Jakub Jelinek
2023-02-24 13:22       ` Jakub Jelinek
2023-02-24 13:47 Richard Biener
2023-02-24 14:08 ` Jakub Jelinek
2023-02-24 14:13   ` Richard Biener

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).