Go patch committed: Pad structs that end with a zero-sized field

Go patch committed: Pad structs that end with a zero-sized field

[Ian Lance Taylor]

[-- Attachment #1: Type: text/plain, Size: 357 bytes --]

This patch by Cherry Zhang changes the Go frontend to pad structs
ending with a zero-sized field.  For a struct with zero-sized last
field, the address of the field falls out of the object boundary,
which confuses the garbage collector.  Pad an extra byte in this case.
Bootstrapped and ran Go testsuite on x86_64-pc-linux-gnu.  Committed
to mainline.

Ian

[-- Attachment #2: patch.txt --]
[-- Type: text/plain, Size: 5751 bytes --]

Index: gcc/go/gofrontend/MERGE
===================================================================
--- gcc/go/gofrontend/MERGE	(revision 267789)
+++ gcc/go/gofrontend/MERGE	(working copy)
@@ -1,4 +1,4 @@
-960637781ca9546ea2db913e48afd7eccbdadfa9
+0d64279c01a37b2579c0c62ca4f2c3e3f81de07c
 
 The first line of this file holds the git revision number of the last
 merge done from the gofrontend repository.
Index: gcc/go/gofrontend/expressions.cc
===================================================================
--- gcc/go/gofrontend/expressions.cc	(revision 267789)
+++ gcc/go/gofrontend/expressions.cc	(working copy)
@@ -13082,6 +13082,12 @@ Struct_construction_expression::do_get_b
 	  ++pv;
 	}
     }
+  if (this->type_->struct_type()->has_padding())
+    {
+      // Feed an extra value if there is a padding field.
+      Btype *fbtype = Type::lookup_integer_type("uint8")->get_backend(gogo);
+      init.push_back(gogo->backend()->zero_expression(fbtype));
+    }
   return gogo->backend()->constructor_expression(btype, init, this->location());
 }
 
Index: gcc/go/gofrontend/types.cc
===================================================================
--- gcc/go/gofrontend/types.cc	(revision 267789)
+++ gcc/go/gofrontend/types.cc	(working copy)
@@ -24,8 +24,7 @@
 // backend.h.
 
 static void
-get_backend_struct_fields(Gogo* gogo, const Struct_field_list* fields,
-			  bool use_placeholder,
+get_backend_struct_fields(Gogo* gogo, Struct_type* type, bool use_placeholder,
 			  std::vector<Backend::Btyped_identifier>* bfields);
 
 static void
@@ -1162,8 +1161,7 @@ Type::get_backend_placeholder(Gogo* gogo
       // struct field.
       {
 	std::vector<Backend::Btyped_identifier> bfields;
-	get_backend_struct_fields(gogo, this->struct_type()->fields(),
-				  true, &bfields);
+	get_backend_struct_fields(gogo, this->struct_type(), true, &bfields);
 	bt = gogo->backend()->struct_type(bfields);
       }
       break;
@@ -6140,12 +6138,14 @@ Struct_type::interface_method_table(Inte
 // backend.h.
 
 static void
-get_backend_struct_fields(Gogo* gogo, const Struct_field_list* fields,
-			  bool use_placeholder,
+get_backend_struct_fields(Gogo* gogo, Struct_type* type, bool use_placeholder,
 			  std::vector<Backend::Btyped_identifier>* bfields)
 {
+  const Struct_field_list* fields = type->fields();
   bfields->resize(fields->size());
   size_t i = 0;
+  int64_t lastsize = 0;
+  bool saw_nonzero = false;
   for (Struct_field_list::const_iterator p = fields->begin();
        p != fields->end();
        ++p, ++i)
@@ -6155,8 +6155,24 @@ get_backend_struct_fields(Gogo* gogo, co
 			     ? p->type()->get_backend_placeholder(gogo)
 			     : p->type()->get_backend(gogo));
       (*bfields)[i].location = p->location();
+      lastsize = gogo->backend()->type_size((*bfields)[i].btype);
+      if (lastsize != 0)
+        saw_nonzero = true;
     }
   go_assert(i == fields->size());
+  if (saw_nonzero && lastsize == 0)
+    {
+      // For nonzero-sized structs which end in a zero-sized thing, we add
+      // an extra byte of padding to the type. This padding ensures that
+      // taking the address of the zero-sized thing can't manufacture a
+      // pointer to the next object in the heap. See issue 9401.
+      size_t n = fields->size();
+      bfields->resize(n + 1);
+      (*bfields)[n].name = "_";
+      (*bfields)[n].btype = Type::lookup_integer_type("uint8")->get_backend(gogo);
+      (*bfields)[n].location = (*bfields)[n-1].location;
+      type->set_has_padding();
+    }
 }
 
 // Get the backend representation for a struct type.
@@ -6165,7 +6181,7 @@ Btype*
 Struct_type::do_get_backend(Gogo* gogo)
 {
   std::vector<Backend::Btyped_identifier> bfields;
-  get_backend_struct_fields(gogo, this->fields_, false, &bfields);
+  get_backend_struct_fields(gogo, this, false, &bfields);
   return gogo->backend()->struct_type(bfields);
 }
 
@@ -10504,8 +10520,7 @@ Named_type::convert(Gogo* gogo)
     case TYPE_STRUCT:
       {
 	std::vector<Backend::Btyped_identifier> bfields;
-	get_backend_struct_fields(gogo, base->struct_type()->fields(),
-				  true, &bfields);
+	get_backend_struct_fields(gogo, base->struct_type(), true, &bfields);
 	if (!gogo->backend()->set_placeholder_struct_type(bt, bfields))
 	  bt = gogo->backend()->error_type();
       }
Index: gcc/go/gofrontend/types.h
===================================================================
--- gcc/go/gofrontend/types.h	(revision 267789)
+++ gcc/go/gofrontend/types.h	(working copy)
@@ -2432,7 +2432,7 @@ class Struct_type : public Type
   Struct_type(Struct_field_list* fields, Location location)
     : Type(TYPE_STRUCT),
       fields_(fields), location_(location), all_methods_(NULL),
-      is_struct_incomparable_(false)
+      is_struct_incomparable_(false), has_padding_(false)
   { }
 
   // Return the field NAME.  This only looks at local fields, not at
@@ -2552,6 +2552,17 @@ class Struct_type : public Type
   set_is_struct_incomparable()
   { this->is_struct_incomparable_ = true; }
 
+  // Return whether this struct's backend type has padding, due to
+  // trailing zero-sized field.
+  bool
+  has_padding() const
+  { return this->has_padding_; }
+
+  // Record that this struct's backend type has padding.
+  void
+  set_has_padding()
+  { this->has_padding_ = true; }
+
   // Write the hash function for this type.
   void
   write_hash_function(Gogo*, Named_type*, Function_type*, Function_type*);
@@ -2656,6 +2667,9 @@ class Struct_type : public Type
   // True if this is a generated struct that is not considered to be
   // comparable.
   bool is_struct_incomparable_;
+  // True if this struct's backend type has padding, due to trailing
+  // zero-sized field.
+  bool has_padding_;
 };
 
 // The type of an array.

Re: Go patch committed: Pad structs that end with a zero-sized field

[Ian Lance Taylor]

[-- Attachment #1: Type: text/plain, Size: 753 bytes --]

On Fri, Jan 11, 2019 at 3:16 PM Ian Lance Taylor <iant@golang.org> wrote:
>
> This patch by Cherry Zhang changes the Go frontend to pad structs
> ending with a zero-sized field.  For a struct with zero-sized last
> field, the address of the field falls out of the object boundary,
> which confuses the garbage collector.  Pad an extra byte in this case.
> Bootstrapped and ran Go testsuite on x86_64-pc-linux-gnu.  Committed
> to mainline.

This follow-up patch by Cherry Zhang adds the padding to the FFI type
when using using libffi.  This fixes reflect.Call with structs that
end in a zero-sized field.  This fixes test/fixedbugs/issue26335.go in
the main repo.  Bootstrapped and ran Go testsuite on
x86_64-pc-linux-gnu.  Committed to mainline.

Ian

[-- Attachment #2: patch.txt --]
[-- Type: text/plain, Size: 1503 bytes --]

Index: gcc/go/gofrontend/MERGE
===================================================================
--- gcc/go/gofrontend/MERGE	(revision 267950)
+++ gcc/go/gofrontend/MERGE	(working copy)
@@ -1,4 +1,4 @@
-87005025fcd0d7e7908b3aae7062b52cb80eb0f3
+9a79c333e896ea49f6a708d459148074d29a2af6
 
 The first line of this file holds the git revision number of the last
 merge done from the gofrontend repository.
Index: libgo/go/runtime/ffi.go
===================================================================
--- libgo/go/runtime/ffi.go	(revision 267941)
+++ libgo/go/runtime/ffi.go	(working copy)
@@ -227,6 +227,7 @@ func structToFFI(typ *structtype) *__ffi
 
 	fields := make([]*__ffi_type, 0, c+1)
 	checkPad := false
+	lastzero := false
 	for i, v := range typ.fields {
 		// Skip zero-sized fields; they confuse libffi,
 		// and there is no value to pass in any case.
@@ -235,8 +236,10 @@ func structToFFI(typ *structtype) *__ffi
 		// next field.
 		if v.typ.size == 0 {
 			checkPad = true
+			lastzero = true
 			continue
 		}
+		lastzero = false
 
 		if checkPad {
 			off := uintptr(0)
@@ -257,6 +260,13 @@ func structToFFI(typ *structtype) *__ffi
 		fields = append(fields, typeToFFI(v.typ))
 	}
 
+	if lastzero {
+		// The compiler adds one byte padding to non-empty struct ending
+		// with a zero-sized field (types.cc:get_backend_struct_fields).
+		// Add this padding to the FFI type.
+		fields = append(fields, ffi_type_uint8())
+	}
+
 	fields = append(fields, nil)
 
 	return &__ffi_type{