Merge pull request #755 from protobuf-c/edmonds/gcc15-union-initialization-changes

Order oneof union members from largest to smallest

Makefile.am

@@ -295,6 +295,24 @@ BUILT_SOURCES += \
 EXTRA_DIST += \
 	t/issue389/issue389.proto
 
+# Issue #375
+check_PROGRAMS += \
+	t/issue375/issue375
+TESTS += \
+	t/issue375/issue375
+t_issue375_issue375_SOURCES = \
+	t/issue375/issue375.c \
+	t/issue375/issue375.pb-c.c
+t_issue375_issue375_LDADD = \
+	protobuf-c/libprotobuf-c.la
+t/issue375/issue375.pb-c.c t/issue375/issue375.pb-c.h: $(top_builddir)/protoc-c/protoc-gen-c$(EXEEXT) $(top_srcdir)/t/issue375/issue375.proto
+	$(AM_V_GEN)@PROTOC@ --plugin=protoc-gen-c=$(top_builddir)/protoc-c/protoc-gen-c$(EXEEXT) -I$(top_srcdir) --c_out=$(top_builddir) $(top_srcdir)/t/issue375/issue375.proto
+BUILT_SOURCES += \
+	t/issue375/issue375.pb-c.c t/issue375/issue375.pb-c.h
+EXTRA_DIST += \
+	t/issue375/issue375.proto
+
+# Issue #440
 check_PROGRAMS += \
 	t/issue440/issue440
 TESTS += \
@@ -311,22 +329,22 @@ BUILT_SOURCES += \
 EXTRA_DIST += \
 	t/issue440/issue440.proto
 
-# Issue #375
+# Issue #745
 check_PROGRAMS += \
-	t/issue375/issue375
+	t/issue745/issue745
 TESTS += \
-	t/issue375/issue375
-t_issue375_issue375_SOURCES = \
-	t/issue375/issue375.c \
-	t/issue375/issue375.pb-c.c
-t_issue375_issue375_LDADD = \
+	t/issue745/issue745
+t_issue745_issue745_SOURCES = \
+	t/issue745/issue745.c \
+	t/issue745/issue745.pb-c.c
+t_issue745_issue745_LDADD = \
 	protobuf-c/libprotobuf-c.la
-t/issue375/issue375.pb-c.c t/issue375/issue375.pb-c.h: $(top_builddir)/protoc-c/protoc-gen-c$(EXEEXT) $(top_srcdir)/t/issue375/issue375.proto
-	$(AM_V_GEN)@PROTOC@ --plugin=protoc-gen-c=$(top_builddir)/protoc-c/protoc-gen-c$(EXEEXT) -I$(top_srcdir) --c_out=$(top_builddir) $(top_srcdir)/t/issue375/issue375.proto
+t/issue745/issue745.pb-c.c t/issue745/issue745.pb-c.h: $(top_builddir)/protoc-c/protoc-gen-c$(EXEEXT) $(top_srcdir)/t/issue745/issue745.proto
+	$(AM_V_GEN)@PROTOC@ --plugin=protoc-gen-c=$(top_builddir)/protoc-c/protoc-gen-c$(EXEEXT) -I$(top_srcdir) --c_out=$(top_builddir) $(top_srcdir)/t/issue745/issue745.proto
 BUILT_SOURCES += \
-	t/issue375/issue375.pb-c.c t/issue375/issue375.pb-c.h
+	t/issue745/issue745.pb-c.c t/issue745/issue745.pb-c.h
 EXTRA_DIST += \
-	t/issue375/issue375.proto
+	t/issue745/issue745.proto
 
 endif # CROSS_COMPILING
 

protoc-c/c_message.cc

@@ -32,7 +32,8 @@
 //  Based on original Protocol Buffers design by
 //  Sanjay Ghemawat, Jeff Dean, and others.
 
-// Copyright (c) 2008-2013, Dave Benson.  All rights reserved.
+// Copyright (c) 2008-2025, Dave Benson and the protobuf-c authors.
+// All rights reserved.
 //
 // Redistribution and use in source and binary forms, with or without
 // modification, are permitted provided that the following conditions are
@@ -63,6 +64,9 @@
 #include <algorithm>
 #include <map>
 #include <memory>
+#include <string_view>
+#include <tuple>
+#include <vector>
 #include <protoc-c/c_message.h>
 #include <protoc-c/c_enum.h>
 #include <protoc-c/c_extension.h>
@@ -211,15 +215,28 @@ GenerateStructDefinition(io::Printer* printer) {
 
     printer->Print("union {\n");
     printer->Indent();
+
+    std::vector<std::tuple<int, std::string_view, const FieldDescriptor *>> sorted_fds;
+
     for (int j = 0; j < oneof->field_count(); j++) {
       const FieldDescriptor *field = oneof->field(j);
+      std::string_view name = field->name();
+      int order = MessageGenerator::GetOneofUnionOrder(field);
+      sorted_fds.push_back({order, name, field});
+    }
+
+    std::sort(sorted_fds.begin(), sorted_fds.end());
+
+    for (const auto& tuple : sorted_fds) {
+      const auto& [order, name, field] = tuple;
       SourceLocation fieldSourceLoc;
       field->GetSourceLocation(&fieldSourceLoc);
 
-      PrintComment (printer, fieldSourceLoc.leading_comments);
-      PrintComment (printer, fieldSourceLoc.trailing_comments);
+      PrintComment(printer, fieldSourceLoc.leading_comments);
+      PrintComment(printer, fieldSourceLoc.trailing_comments);
       field_generators_.get(field).GenerateStructMembers(printer);
     }
+
     printer->Outdent();
     printer->Print(vars, "};\n");
   }
@@ -236,6 +253,7 @@ GenerateStructDefinition(io::Printer* printer) {
 
   printer->Print(vars, "#define $ucclassname$__INIT \\\n"
 		       " { PROTOBUF_C_MESSAGE_INIT (&$lcclassname$__descriptor) \\\n    ");
+
   for (int i = 0; i < descriptor_->field_count(); i++) {
     const FieldDescriptor *field = descriptor_->field(i);
     if (field->containing_oneof() == NULL) {
@@ -243,16 +261,32 @@ GenerateStructDefinition(io::Printer* printer) {
       field_generators_.get(field).GenerateStaticInit(printer);
     }
   }
+
   for (int i = 0; i < descriptor_->oneof_decl_count(); i++) {
     const OneofDescriptor *oneof = descriptor_->oneof_decl(i);
     vars["foneofname"] = FullNameToUpper(oneof->full_name(), oneof->file());
+
     // Initialize the case enum
     printer->Print(vars, ", $foneofname$__NOT_SET");
+
     // Initialize the union
-    printer->Print(", {0}");
+    bool want_extra_braces = false;
+    for (int j = 0; j < oneof->field_count(); j++) {
+      const FieldDescriptor *field = oneof->field(j);
+      if (field->cpp_type() == FieldDescriptor::CPPTYPE_STRING &&
+          field->type() == FieldDescriptor::TYPE_BYTES)
+      {
+        want_extra_braces = true;
+      }
+    }
+    if (want_extra_braces) {
+      printer->Print(", { {0} }");
+    } else {
+      printer->Print(", {0}");
+    }
   }
-  printer->Print(" }\n\n\n");
 
+  printer->Print(" }\n\n\n");
 }
 
 void MessageGenerator::
@@ -627,6 +661,60 @@ GenerateMessageDescriptor(io::Printer* printer, bool gen_init) {
       "};\n");
 }
 
+int MessageGenerator::GetOneofUnionOrder(const FieldDescriptor *fd)
+{
+  auto cpp_type = fd->cpp_type();
+  auto pb_type = fd->type();
+
+  switch (cpp_type) {
+    case FieldDescriptor::CPPTYPE_STRING:
+      if (pb_type == FieldDescriptor::TYPE_BYTES) {
+        return 1;
+      } else if (pb_type == FieldDescriptor::TYPE_STRING) {
+        return 4;
+      } else {
+        GOOGLE_LOG(FATAL)
+          << fd->full_name()
+          << ": Unhandled combination of CPPTYPE ("
+          << cpp_type
+          << ") and protobuf type ("
+          << pb_type
+          << ")";
+        return -1;
+      }
+
+    case FieldDescriptor::CPPTYPE_DOUBLE:
+      return 2;
+
+    case FieldDescriptor::CPPTYPE_INT64:
+    case FieldDescriptor::CPPTYPE_UINT64:
+      return 3;
+
+    case FieldDescriptor::CPPTYPE_MESSAGE:
+      return 5;
+
+    case FieldDescriptor::CPPTYPE_FLOAT:
+      return 6;
+
+    case FieldDescriptor::CPPTYPE_INT32:
+    case FieldDescriptor::CPPTYPE_UINT32:
+      return 7;
+
+    case FieldDescriptor::CPPTYPE_ENUM:
+      return 8;
+
+    case FieldDescriptor::CPPTYPE_BOOL:
+      return 9;
+
+    default:
+      GOOGLE_LOG(FATAL)
+        << fd->full_name()
+        << ": Unhandled CPPTYPE "
+        << cpp_type;
+      return -1;
+  }
+}
+
 }  // namespace c
 }  // namespace compiler
 }  // namespace protobuf

protoc-c/c_message.h

@@ -32,7 +32,8 @@
 //  Based on original Protocol Buffers design by
 //  Sanjay Ghemawat, Jeff Dean, and others.
 
-// Copyright (c) 2008-2013, Dave Benson.  All rights reserved.
+// Copyright (c) 2008-2025, Dave Benson and the protobuf-c authors.
+// All rights reserved.
 //
 // Redistribution and use in source and binary forms, with or without
 // modification, are permitted provided that the following conditions are
@@ -128,7 +129,7 @@ class MessageGenerator {
 
  private:
 
-  std::string GetDefaultValueC(const FieldDescriptor *fd);
+  int GetOneofUnionOrder(const FieldDescriptor *fd);
 
   const Descriptor* descriptor_;
   std::string dllexport_decl_;

t/issue745/.gitignore

+/issue745

t/issue745/issue745.c

+#include <assert.h>
+#include <stdio.h>
+#include <stdlib.h>
+
+#include "t/issue745/issue745.pb-c.h"
+
+int main(void)
+{
+	T t = T__INIT;
+
+	size_t offset_to_union = offsetof(T, test_bool);
+	size_t size_of_union = sizeof(T) - offset_to_union;
+	unsigned char *ptr_to_union = ((unsigned char *)&t) + offset_to_union;
+
+	assert(offsetof(T, test_bool) == offsetof(T, test_enum));
+	assert(offsetof(T, test_enum) == offsetof(T, test_float));
+	assert(offsetof(T, test_float) == offsetof(T, test_uint32));
+	assert(offsetof(T, test_uint32) == offsetof(T, test_message));
+	assert(offsetof(T, test_message) == offsetof(T, test_string));
+	assert(offsetof(T, test_string) == offsetof(T, test_double));
+	assert(offsetof(T, test_double) == offsetof(T, test_uint64));
+	assert(offsetof(T, test_uint64) == offsetof(T, test_bytes));
+
+	for (size_t i = 0; i < size_of_union; i++) {
+		fprintf(stderr, "ptr_to_union[%zd] = %02x\n", i, ptr_to_union[i]);
+	}
+
+	// The following will probably crash on gcc >= 15 under its default
+	// `-fzero-init-padding-bits=standard` behavior, if the ordering of oneof union
+	// members in T are as performed by protobuf-c <= 1.5.0.
+	//
+	// The code generator in protobuf-c >= 1.5.1 should order union members from
+	// largest to smallest, which should correctly zero all the bits used by the
+	// object representations of the members of the oneof union even on gcc >= 15.
+
+	for (size_t i = 0; i < size_of_union; i++) {
+		assert(ptr_to_union[i] == 0);
+	}
+
+	return EXIT_SUCCESS;
+}

t/issue745/issue745.proto

+syntax = "proto3";
+
+enum E {
+	FIRST_VALUE = 0;
+	SECOND_VALUE = 1;
+}
+
+message M {
+	int32 test = 1;
+}
+
+message T {
+	oneof test_oneof {
+		bool test_bool = 1;
+		E test_enum = 2;
+		float test_float = 3;
+		fixed32 test_fixed32 = 4;
+		int32 test_int32 = 5;
+		sfixed32 test_sfixed32 = 6;
+		sint32 test_sint32 = 7;
+		uint32 test_uint32 = 8;
+		M test_message = 9;
+		string test_string = 10;
+		double test_double = 11;
+		fixed64 test_fixed64 = 12;
+		int64 test_int64 = 13;
+		sfixed64 test_sfixed64 = 14;
+		sint64 test_sint64 = 15;
+		uint64 test_uint64 = 16;
+		bytes test_bytes = 17;
+	}
+}