diff mbox series

c++: Small initial fixes for zeroing of padding bits [PR117256]

Message ID Zy3Z5ceStPmFb4I1@tucnak
State New
Headers show
Series c++: Small initial fixes for zeroing of padding bits [PR117256] | expand

Commit Message

Jakub Jelinek Nov. 8, 2024, 9:29 a.m. UTC
Hi!

https://eel.is/c++draft/dcl.init#general-6
says that even padding bits are supposed to be zeroed during
zero-initialization.
The following patch on top of the
https://gcc.gnu.org/pipermail/gcc-patches/2024-October/665565.html
patch attempts to implement that, though only for the easy
cases so far, in particular marks the CONSTRUCTOR created during
zero-initialization (or zero-initialization done during the
value-initialization) as having padding bits cleared and for
constexpr evaluation attempts to preserve that bit on a new CONSTRUCTOR
created for CONSTRUCTOR_ZERO_PADDING_BITS lhs.

I think we need far more than that, but am not sure where exactly
to implement that.
In particular, I think __builtin_bitcast should take it into account
during constant evaluation, if the padding bits in something are guaranteed
to be zero, then I'd think std::bitcast out of it and testing those
bits in there should be well defined.
But if we do that, the flag needs to be maintained precisely, not just
conservatively, so e.g. any place where some object is copied into another
one (except bitcast?) which would be element-wise copy, the bit should
be cleared (or preserved from the earlier state?  I'd hope
element-wise copying invalidates even the padding bits, but then what
about just stores into some members, do those invalidate the padding bits
in the rest of the object?).  But if it is an elided copy, it shouldn't.
And am not really sure what happens e.g. with non-automatic constexpr
variables.  If it is constructed by something that doesn't guarantee
the zeroing of the padding bits (so similarly constructed constexpr automatic
variable would have undefined state of the padding bits), are those padding
bits well defined because it isn't automatic variable?

Anyway, I hope the following patch is at least a small step in the right
direction.

Bootstrapped/regtested on x86_64-linux and i686-linux, it caused
g++.dg/tm/pr45940-3.C and g++.dg/tm/pr45940-4.C regressions like
+FAIL: g++.dg/tm/pr45940-3.C  -std=gnu++11 (internal compiler error: in create_tmp_var, at gimple-expr.cc:484)
+FAIL: g++.dg/tm/pr45940-3.C  -std=gnu++11 (test for excess errors)
but that seems to be a trans-mem.cc preexisting bug which I'm going
to post a patch for separately, ok for trunk (of course, provided the
above mentioned patch is acked too)?

2024-11-07  Jakub Jelinek  <jakub@redhat.com>

	PR c++/78620
	PR c++/117256
	* init.cc (build_zero_init_1): Set CONSTRUCTOR_ZERO_PADDING_BITS.
	(build_value_init_noctor): Likewise.
	* constexpr.cc (cxx_eval_store_expression): Propagate
	CONSTRUCTOR_ZERO_PADDING_BITS flag.


	Jakub

Comments

Jakub Jelinek Nov. 8, 2024, 9:50 a.m. UTC | #1
On Fri, Nov 08, 2024 at 10:29:09AM +0100, Jakub Jelinek wrote:
> I think we need far more than that, but am not sure where exactly
> to implement that.
> In particular, I think __builtin_bitcast should take it into account
> during constant evaluation, if the padding bits in something are guaranteed
> to be zero, then I'd think std::bitcast out of it and testing those
> bits in there should be well defined.
> But if we do that, the flag needs to be maintained precisely, not just
> conservatively, so e.g. any place where some object is copied into another
> one (except bitcast?) which would be element-wise copy, the bit should
> be cleared (or preserved from the earlier state?  I'd hope
> element-wise copying invalidates even the padding bits, but then what
> about just stores into some members, do those invalidate the padding bits
> in the rest of the object?).  But if it is an elided copy, it shouldn't.
> And am not really sure what happens e.g. with non-automatic constexpr
> variables.  If it is constructed by something that doesn't guarantee
> the zeroing of the padding bits (so similarly constructed constexpr automatic
> variable would have undefined state of the padding bits), are those padding
> bits well defined because it isn't automatic variable?

And there is another thing I'm unsure about, what happens in a union which
is zero-initialized and has padding bits if the active member is changed
(especially in constant evaluation).  Are all padding bits invalidated
through that, or something else?

	Jakub
diff mbox series

Patch

--- gcc/cp/init.cc.jj	2024-11-06 10:19:11.435260625 +0100
+++ gcc/cp/init.cc	2024-11-07 17:23:13.335275180 +0100
@@ -249,6 +249,7 @@  build_zero_init_1 (tree type, tree nelts
 
       /* Build a constructor to contain the initializations.  */
       init = build_constructor (type, v);
+      CONSTRUCTOR_ZERO_PADDING_BITS (init) = 1;
     }
   else if (TREE_CODE (type) == ARRAY_TYPE)
     {
@@ -467,7 +468,9 @@  build_value_init_noctor (tree type, tsub
 	    }
 
 	  /* Build a constructor to contain the zero- initializations.  */
-	  return build_constructor (type, v);
+	  tree ret = build_constructor (type, v);
+	  CONSTRUCTOR_ZERO_PADDING_BITS (ret) = 1;
+	  return ret;
 	}
     }
   else if (TREE_CODE (type) == ARRAY_TYPE)
--- gcc/cp/constexpr.cc.jj	2024-11-05 08:58:25.144845731 +0100
+++ gcc/cp/constexpr.cc	2024-11-07 17:59:54.053170842 +0100
@@ -6421,6 +6421,7 @@  cxx_eval_store_expression (const constex
 
   type = TREE_TYPE (object);
   bool no_zero_init = true;
+  bool zero_padding_bits = false;
 
   auto_vec<tree *> ctors;
   releasing_vec indexes;
@@ -6433,6 +6434,7 @@  cxx_eval_store_expression (const constex
 	{
 	  *valp = build_constructor (type, NULL);
 	  CONSTRUCTOR_NO_CLEARING (*valp) = no_zero_init;
+	  CONSTRUCTOR_ZERO_PADDING_BITS (*valp) = zero_padding_bits;
 	}
       else if (STRIP_ANY_LOCATION_WRAPPER (*valp),
 	       TREE_CODE (*valp) == STRING_CST)
@@ -6492,8 +6494,10 @@  cxx_eval_store_expression (const constex
 	}
 
       /* If the value of object is already zero-initialized, any new ctors for
-	 subobjects will also be zero-initialized.  */
+	 subobjects will also be zero-initialized.  Similarly with zeroing of
+	 padding bits.  */
       no_zero_init = CONSTRUCTOR_NO_CLEARING (*valp);
+      zero_padding_bits = CONSTRUCTOR_ZERO_PADDING_BITS (*valp);
 
       if (code == RECORD_TYPE && is_empty_field (index))
 	/* Don't build a sub-CONSTRUCTOR for an empty base or field, as they
@@ -6678,6 +6682,7 @@  cxx_eval_store_expression (const constex
 	{
 	  *valp = build_constructor (type, NULL);
 	  CONSTRUCTOR_NO_CLEARING (*valp) = no_zero_init;
+	  CONSTRUCTOR_ZERO_PADDING_BITS (*valp) = zero_padding_bits;
 	}
       new_ctx.ctor = empty_base ? NULL_TREE : *valp;
       new_ctx.object = target;
@@ -6719,6 +6724,7 @@  cxx_eval_store_expression (const constex
 	  /* But do make sure we have something in *valp.  */
 	  *valp = build_constructor (type, nullptr);
 	  CONSTRUCTOR_NO_CLEARING (*valp) = no_zero_init;
+	  CONSTRUCTOR_ZERO_PADDING_BITS (*valp) = zero_padding_bits;
 	}
     }
   else if (*valp && TREE_CODE (*valp) == CONSTRUCTOR
@@ -6731,6 +6737,8 @@  cxx_eval_store_expression (const constex
       TREE_SIDE_EFFECTS (*valp) = TREE_SIDE_EFFECTS (init);
       CONSTRUCTOR_NO_CLEARING (*valp)
 	= CONSTRUCTOR_NO_CLEARING (init);
+      CONSTRUCTOR_ZERO_PADDING_BITS (*valp)
+        = CONSTRUCTOR_ZERO_PADDING_BITS (init);
     }
   else
     *valp = init;
--- gcc/testsuite/g++.dg/cpp0x/zero-init1.C.jj	2024-11-07 18:12:57.932091864 +0100
+++ gcc/testsuite/g++.dg/cpp0x/zero-init1.C	2024-11-07 18:16:08.070403865 +0100
@@ -0,0 +1,70 @@ 
+// PR c++/117256
+// { dg-do run { target c++11 } }
+// { dg-options "-O0" }
+
+void *operator new (decltype (sizeof 0), void *p) noexcept { return p; }
+
+struct A { char c; int i; };
+#if __cplusplus >= 201402L
+struct B { A a; constexpr B (char x, int y) : a () { a.c = x; a.i = y; } };
+#else
+struct B { A a; B (char x, int y) : a () { a.c = x; a.i = y; } };
+#endif
+
+[[gnu::noipa]] void
+foo ()
+{
+  unsigned char buf[sizeof (B)];
+  A a1 = A ();
+  __builtin_memcpy (buf, &a1, sizeof (buf));
+  if (buf[1])
+    __builtin_abort ();
+  unsigned char m1 alignas (A) [sizeof (A)];
+  __builtin_memset (m1, -1, sizeof (m1));
+  A *a2 = new (m1) A ();
+  __builtin_memcpy (buf, a2, sizeof (*a2));
+  if (buf[1])
+    __builtin_abort ();
+  B b1 (42, -42);
+  __builtin_memcpy (buf, &b1, sizeof (b1));
+  if (buf[1])
+    __builtin_abort ();
+  unsigned char m2 alignas (B) [sizeof (B)];
+  B *b2 = new (m2) B (1, 2);
+  __builtin_memcpy (buf, b2, sizeof (*b2));
+  if (buf[1])
+    __builtin_abort ();
+#if __cplusplus >= 201402L
+  constexpr B b3 (3, 4);
+  __builtin_memcpy (buf, &b3, sizeof (b3));
+  if (buf[1])
+    __builtin_abort ();
+#endif
+}
+
+[[gnu::noipa]] void
+bar (unsigned char *p)
+{
+  (void) p;
+}
+
+[[gnu::noipa]] void
+baz ()
+{
+  unsigned char buf[256];
+  __builtin_memset (buf, -1, sizeof (buf));
+  bar (buf);
+}
+
+int
+main ()
+{
+  if (__builtin_offsetof (A, c) == 0
+      && __builtin_offsetof (A, i) != 1
+      && __builtin_offsetof (B, a) == 0
+      && sizeof (A) == sizeof (B))
+    {
+      baz ();
+      foo ();
+    }
+}