pw_protobuf: Disambiguate maximum size constants

pw_protobuf messages define a `kMaxEncodedSizeBytes` constant to help
users with buffer sizing. However, this constant did not truly represent
the message's maximum size in cases where field sizes were unknown,
which could easily result in bugs when these constants were blindly
trusted (there were instances of this in pw_protobuf's own unit tests).

This changes the code generator to rename this constants to
`kMaxEncodedSizeBytesWithoutValues`, better describing its actual
function. The original definition of `kMaxEncodedSizeBytes` is kept
temporarily to allow projects to migrate to the new constant.

In a follow-up change, `kMaxEncodedSizeBytes` will only be generated
if the size of every field in a message struct is statically known.

Bug: 379868242
Change-Id: I212ca327529d3b679cdc7757ca759eb6a87847d1
Reviewed-on: https://pigweed-review.googlesource.com/c/pigweed/pigweed/+/259012
Commit-Queue: Alexei Frolov <[email protected]>
Lint: Lint 🤖 <[email protected]>
Reviewed-by: Armando Montanez <[email protected]>

pw_metric/metric_service_pwpb.cc

@@ -22,6 +22,7 @@
 #include "pw_metric_private/metric_walker.h"
 #include "pw_metric_proto/metric_service.pwpb.h"
 #include "pw_preprocessor/util.h"
+#include "pw_protobuf/serialized_size.h"
 #include "pw_rpc/raw/server_reader_writer.h"
 #include "pw_span/span.h"
 #include "pw_status/status.h"
@@ -94,10 +95,18 @@ class PwpbMetricWriter : public virtual internal::MetricWriter {
 void MetricService::Get(ConstByteSpan /*request*/,
                         rpc::RawServerWriter& raw_response) {
   // For now, ignore the request and just stream all the metrics back.
-  // TODO(amontanez): Make this follow the metric_service.options configuration.
+
+  // The `string_path` field of Metric is not supported. The maximum size
+  // without values includes the maximum token path. Additionally, include the
+  // maximum size of the `as_int` field.
   constexpr size_t kSizeOfOneMetric =
-      pw::metric::proto::pwpb::MetricResponse::kMaxEncodedSizeBytes +
-      pw::metric::proto::pwpb::Metric::kMaxEncodedSizeBytes;
+      pw::metric::proto::pwpb::MetricResponse::
+          kMaxEncodedSizeBytesWithoutValues +
+      pw::metric::proto::pwpb::Metric::kMaxEncodedSizeBytesWithoutValues +
+      protobuf::SizeOfFieldUint32(
+          pw::metric::proto::pwpb::Metric::Fields::kAsInt);
+
+  // TODO(amontanez): Make this follow the metric_service.options configuration.
   constexpr size_t kEncodeBufferSize = kMaxNumPackedEntries * kSizeOfOneMetric;
 
   std::array<std::byte, kEncodeBufferSize> encode_buffer;

pw_protobuf/codegen_encoder_test.cc

@@ -66,10 +66,20 @@ constexpr std::byte ToByte() {
 }
 
 TEST(Codegen, Codegen) {
-  std::byte encode_buffer[Pigweed::kMaxEncodedSizeBytes +
-                          DeviceInfo::kMaxEncodedSizeBytes];
+  // The encoded size constants don't account for variable length values, so
+  // add some additional space for the expected sizes of values we intend to
+  // encode.
+  constexpr size_t kPigweedMaxValuesSize = 64;
+  constexpr size_t kDeviceInfoMaxValuesSize = 32;
+
+  std::byte encode_buffer[Pigweed::kMaxEncodedSizeBytesWithoutValues +
+                          kPigweedMaxValuesSize +
+                          DeviceInfo::kMaxEncodedSizeBytesWithoutValues +
+                          kDeviceInfoMaxValuesSize];
   std::byte temp_buffer[Pigweed::kScratchBufferSizeBytes +
-                        DeviceInfo::kMaxEncodedSizeBytes];
+                        kPigweedMaxValuesSize +
+                        DeviceInfo::kMaxEncodedSizeBytesWithoutValues +
+                        kDeviceInfoMaxValuesSize];
   stream::MemoryWriter writer(encode_buffer);
 
   Pigweed::StreamEncoder pigweed(writer, temp_buffer);
@@ -221,7 +231,7 @@ TEST(Codegen, Codegen) {
 TEST(Codegen, RecursiveSubmessage) {
   // 12 here represents the longest name. Note that all field structure is taken
   // care of, we just have to multiply by how many crates we're encoding, ie. 4.
-  std::byte encode_buffer[(Crate::kMaxEncodedSizeBytes + 12) * 4];
+  std::byte encode_buffer[(Crate::kMaxEncodedSizeBytesWithoutValues + 12) * 4];
 
   Crate::MemoryEncoder biggest_crate(encode_buffer);
   ASSERT_EQ(OkStatus(), biggest_crate.WriteName("Huge crate"));
@@ -293,7 +303,7 @@ TEST(CodegenRepeated, ConstrainedFull) {
 }
 
 TEST(CodegenRepeated, NonPackedScalar) {
-  std::byte encode_buffer[RepeatedTest::kMaxEncodedSizeBytes];
+  std::byte encode_buffer[RepeatedTest::kMaxEncodedSizeBytesWithoutValues + 32];
 
   stream::MemoryWriter writer(encode_buffer);
   RepeatedTest::StreamEncoder repeated_test(writer, ByteSpan());
@@ -328,7 +338,7 @@ TEST(CodegenRepeated, NonPackedScalar) {
 }
 
 TEST(CodegenRepeated, PackedScalar) {
-  std::byte encode_buffer[RepeatedTest::kMaxEncodedSizeBytes];
+  std::byte encode_buffer[RepeatedTest::kMaxEncodedSizeBytesWithoutValues + 32];
 
   stream::MemoryWriter writer(encode_buffer);
   RepeatedTest::StreamEncoder repeated_test(writer, ByteSpan());
@@ -361,7 +371,7 @@ TEST(CodegenRepeated, PackedScalar) {
 }
 
 TEST(CodegenRepeated, PackedBool) {
-  std::byte encode_buffer[RepeatedTest::kMaxEncodedSizeBytes];
+  std::byte encode_buffer[RepeatedTest::kMaxEncodedSizeBytesWithoutValues + 8];
 
   stream::MemoryWriter writer(encode_buffer);
   RepeatedTest::StreamEncoder repeated_test(writer, ByteSpan());
@@ -383,7 +393,7 @@ TEST(CodegenRepeated, PackedBool) {
 }
 
 TEST(CodegenRepeated, PackedScalarVector) {
-  std::byte encode_buffer[RepeatedTest::kMaxEncodedSizeBytes];
+  std::byte encode_buffer[RepeatedTest::kMaxEncodedSizeBytesWithoutValues + 32];
 
   stream::MemoryWriter writer(encode_buffer);
   RepeatedTest::StreamEncoder repeated_test(writer, ByteSpan());
@@ -416,7 +426,7 @@ TEST(CodegenRepeated, PackedScalarVector) {
 }
 
 TEST(CodegenRepeated, PackedEnum) {
-  std::byte encode_buffer[RepeatedTest::kMaxEncodedSizeBytes];
+  std::byte encode_buffer[RepeatedTest::kMaxEncodedSizeBytesWithoutValues + 8];
 
   stream::MemoryWriter writer(encode_buffer);
   RepeatedTest::StreamEncoder repeated_test(writer, ByteSpan());
@@ -438,7 +448,7 @@ TEST(CodegenRepeated, PackedEnum) {
 }
 
 TEST(CodegenRepeated, PackedEnumVector) {
-  std::byte encode_buffer[RepeatedTest::kMaxEncodedSizeBytes];
+  std::byte encode_buffer[RepeatedTest::kMaxEncodedSizeBytesWithoutValues + 8];
 
   stream::MemoryWriter writer(encode_buffer);
   RepeatedTest::StreamEncoder repeated_test(writer, ByteSpan());
@@ -461,7 +471,7 @@ TEST(CodegenRepeated, PackedEnumVector) {
 }
 
 TEST(CodegenRepeated, NonScalar) {
-  std::byte encode_buffer[RepeatedTest::kMaxEncodedSizeBytes];
+  std::byte encode_buffer[RepeatedTest::kMaxEncodedSizeBytesWithoutValues + 32];
 
   stream::MemoryWriter writer(encode_buffer);
   RepeatedTest::StreamEncoder repeated_test(writer, ByteSpan());
@@ -481,10 +491,14 @@ TEST(CodegenRepeated, NonScalar) {
 }
 
 TEST(CodegenRepeated, Message) {
-  std::byte encode_buffer[RepeatedTest::kMaxEncodedSizeBytes];
+  constexpr size_t kNumStructs = 3;
+
+  std::byte
+      encode_buffer[RepeatedTest::kMaxEncodedSizeBytesWithoutValues +
+                    kNumStructs * test::pwpb::Struct::kMaxEncodedSizeBytes];
 
   RepeatedTest::MemoryEncoder repeated_test(encode_buffer);
-  for (uint32_t i = 0; i < 3; ++i) {
+  for (uint32_t i = 0; i < kNumStructs; ++i) {
     auto structs = repeated_test.GetStructsEncoder();
     ASSERT_EQ(OkStatus(), structs.WriteOne(i * 1));
     ASSERT_EQ(OkStatus(), structs.WriteTwo(i * 2));
@@ -504,7 +518,7 @@ TEST(CodegenRepeated, Message) {
 }
 
 TEST(Codegen, Proto2) {
-  std::byte encode_buffer[Foo::kMaxEncodedSizeBytes];
+  std::byte encode_buffer[Foo::kMaxEncodedSizeBytesWithoutValues + 8];
 
   Foo::MemoryEncoder foo(encode_buffer);
   ASSERT_EQ(OkStatus(), foo.WriteInteger(3));
@@ -548,7 +562,7 @@ TEST(Codegen, Import) {
 TEST(Codegen, NonPigweedPackage) {
   namespace Packed = ::non::pigweed::package::name::pwpb::Packed;
 
-  std::byte encode_buffer[Packed::kMaxEncodedSizeBytes];
+  std::byte encode_buffer[Packed::kMaxEncodedSizeBytesWithoutValues + 8];
   std::array<const int64_t, 2> repeated = {0, 1};
   stream::MemoryWriter writer(encode_buffer);
   Packed::StreamEncoder packed(writer, ByteSpan());

pw_protobuf/codegen_message_test.cc

@@ -1343,7 +1343,9 @@ TEST(CodegenMessage, Write) {
   message.proto.meta.pigweed_bin = Pigweed::Pigweed::Binary::ONE;
   std::memcpy(message.data.data(), pigweed_data, sizeof(pigweed_data));
 
-  std::byte encode_buffer[Pigweed::kMaxEncodedSizeBytes];
+  // No callback fields are being written, so the max size without values is
+  // sufficient.
+  std::byte encode_buffer[Pigweed::kMaxEncodedSizeBytesWithoutValues];
   std::byte temp_buffer[Pigweed::kScratchBufferSizeBytes];
 
   stream::MemoryWriter writer(encode_buffer);
@@ -1399,7 +1401,7 @@ TEST(CodegenMessage, Write) {
 TEST(CodegenMessage, WriteDefaults) {
   Pigweed::Message message{};
 
-  std::byte encode_buffer[Pigweed::kMaxEncodedSizeBytes];
+  std::byte encode_buffer[Pigweed::kMaxEncodedSizeBytesWithoutValues];
   std::byte temp_buffer[Pigweed::kScratchBufferSizeBytes];
 
   stream::MemoryWriter writer(encode_buffer);
@@ -1420,7 +1422,10 @@ TEST(CodegenMessage, WritePackedScalar) {
     message.fixed32s.push_back(i * 16u);
   }
 
-  std::byte encode_buffer[RepeatedTest::kMaxEncodedSizeBytes];
+  // RepeatedTest has several different repeated fields, some of which have a
+  // static size while others don't. `uint32s`  and `fixed32` have maximum
+  // static sizes, so the max encoded size accounts for them.
+  std::byte encode_buffer[RepeatedTest::kMaxEncodedSizeBytesWithoutValues];
 
   stream::MemoryWriter writer(encode_buffer);
   RepeatedTest::StreamEncoder repeated_test(writer, ByteSpan());
@@ -1459,7 +1464,10 @@ TEST(CodegenMessage, WritePackedScalarFixedLength) {
   message.doubles[0] = 3.14159;
   message.doubles[1] = 2.71828;
 
-  std::byte encode_buffer[RepeatedTest::kMaxEncodedSizeBytes];
+  // RepeatedTest has several different repeated fields, some of which have a
+  // static size while others don't. `doubles` has a fixed length, so the max
+  // encoded size accounts for it.
+  std::byte encode_buffer[RepeatedTest::kMaxEncodedSizeBytesWithoutValues];
 
   stream::MemoryWriter writer(encode_buffer);
   RepeatedTest::StreamEncoder repeated_test(writer, ByteSpan());
@@ -1491,7 +1499,7 @@ TEST(CodegenMessage, WritePackedScalarCallback) {
     return encoder.WriteSint32s(sint32s);
   });
 
-  std::byte encode_buffer[RepeatedTest::kMaxEncodedSizeBytes +
+  std::byte encode_buffer[RepeatedTest::kMaxEncodedSizeBytesWithoutValues +
                           varint::kMaxVarint32SizeBytes * 5];
 
   stream::MemoryWriter writer(encode_buffer);
@@ -1525,7 +1533,11 @@ TEST(CodegenMessage, WritePackedEnum) {
   message.enums.push_back(Enum::AMBER);
   message.enums.push_back(Enum::RED);
 
-  std::byte encode_buffer[RepeatedTest::kMaxEncodedSizeBytes];
+  // RepeatedTest has several different repeated fields, some of which have a
+  // static size while others don't. `enums` is one of the fields which does
+  // specify a static size option, so the max encoded size accounts for its
+  // worst case length.
+  std::byte encode_buffer[RepeatedTest::kMaxEncodedSizeBytesWithoutValues];
 
   stream::MemoryWriter writer(encode_buffer);
   RepeatedTest::StreamEncoder repeated_test(writer, ByteSpan());
@@ -1556,7 +1568,7 @@ TEST(CodegenMessage, WriteStringCallback) {
         "libraries-or as we like to call them, modules");
   });
 
-  std::byte encode_buffer[Pigweed::kMaxEncodedSizeBytes + 92];
+  std::byte encode_buffer[Pigweed::kMaxEncodedSizeBytesWithoutValues + 92];
   std::byte temp_buffer[Pigweed::kScratchBufferSizeBytes];
 
   stream::MemoryWriter writer(encode_buffer);
@@ -1592,7 +1604,9 @@ TEST(CodegenMessage, WriteForcedCallback) {
     return encoder.WriteSpecialProperty(42u);
   });
 
-  std::byte encode_buffer[Pigweed::kMaxEncodedSizeBytes];
+  // As the field is a scalar, it doesn't have an associated variable-length
+  // value, so the max size without values is sufficient.
+  std::byte encode_buffer[Pigweed::kMaxEncodedSizeBytesWithoutValues];
   std::byte temp_buffer[Pigweed::kScratchBufferSizeBytes];
 
   stream::MemoryWriter writer(encode_buffer);
@@ -1663,7 +1677,7 @@ TEST(CodegenMessage, WriteNestedRepeated) {
     return OkStatus();
   });
 
-  std::byte encode_buffer[RepeatedTest::kMaxEncodedSizeBytes +
+  std::byte encode_buffer[RepeatedTest::kMaxEncodedSizeBytesWithoutValues +
                           Struct::kMaxEncodedSizeBytes * 2];
   std::byte temp_buffer[RepeatedTest::kScratchBufferSizeBytes +
                         Struct::kMaxEncodedSizeBytes];
@@ -1698,6 +1712,10 @@ TEST(CodegenMessage, WriteNestedRepeated) {
 }
 
 TEST(CodegenMessage, WriteNestedForcedCallback) {
+  // Manually add some additional buffer space for the values we know we want
+  // to encode to the message.
+  constexpr size_t kEncodedAttributesSize = 32;
+
   Pigweed::Message message{};
   // pigweed.device_info has use_callback=true to force the use of a callback.
   message.device_info.SetEncoder([](Pigweed::StreamEncoder& encoder) {
@@ -1725,10 +1743,12 @@ TEST(CodegenMessage, WriteNestedForcedCallback) {
     return encoder.GetDeviceInfoEncoder().Write(device_info);
   });
 
-  std::byte encode_buffer[Pigweed::kMaxEncodedSizeBytes +
-                          DeviceInfo::kMaxEncodedSizeBytes];
+  std::byte encode_buffer[Pigweed::kMaxEncodedSizeBytesWithoutValues +
+                          DeviceInfo::kMaxEncodedSizeBytesWithoutValues +
+                          kEncodedAttributesSize];
   std::byte temp_buffer[Pigweed::kScratchBufferSizeBytes +
-                        DeviceInfo::kMaxEncodedSizeBytes];
+                        DeviceInfo::kMaxEncodedSizeBytesWithoutValues +
+                        kEncodedAttributesSize];
 
   stream::MemoryWriter writer(encode_buffer);
   Pigweed::StreamEncoder pigweed(writer, temp_buffer);

pw_protobuf/docs.rst

@@ -181,9 +181,30 @@ The ``Point`` message can be fully compared for equality, but ``Label`` cannot.
 Buffer Sizes
 ------------
 Initializing a ``MemoryEncoder`` requires that you specify the size of the
-buffer to encode to. The code generation includes a ``kMaxEncodedSizeBytes``
-constant that represents the maximum encoded size of the protobuf message,
-excluding the contents of any field values which require a callback.
+buffer to encode to. The code generation includes constants which assist with
+sizing your buffers, listed below:
+
+- ``kMaxEncodedSizeBytes``: In messages where the size of every field is known,
+  either due to them being scalar fields or having size/count limits specified
+  through an options file, this constant defines the maximum size of the message
+  if every field is set to its largest possible value.
+
+  If the size of the message cannot be statically determined, typically due to
+  callback fields within the message, this constant will not be generated.
+
+- ``kMaxEncodedSizeBytesWithoutValues``: The maximum size of the encoded message
+  excluding any dynamic variable-length fields such as those requiring
+  callbacks. If the message contains only statically known field sizes, meeting
+  the criteria to generate ``kMaxEncodedSizeBytes``, this value will be
+  identical to it.
+
+- Field-specific size constants (``k{FieldName}MaxSize``): String or bytes
+  fields specifying a maximum or fixed size option define those sizes as
+  constants.
+
+In the below example, because the ``name`` field has a ``max_size`` specified
+in the accompanying options file, ``kMaxEncodedSizeBytes`` includes the maximum
+length of the value for that field.
 
 .. code-block:: c++
 
@@ -202,18 +223,15 @@ excluding the contents of any field values which require a callback.
      PW_LOG_INFO("Failed to encode proto; %s", encoder.status().str());
    }
 
-In the above example, because the ``name`` field has a ``max_size`` specified
-in the accompanying options file, ``kMaxEncodedSizeBytes`` includes the maximum
-length of the value for that field.
-
 Where the maximum length of a field value is not known, indicated by the
-structure requiring a callback for that field, the constant includes
-all relevant overhead and only requires that you add the length of the field
-values.
+structure requiring a callback for that field, ``kMaxEncodedSizeBytes`` will
+not be defined to indicate that additional computation is required from the
+user.
 
 For example if a ``bytes`` field length is not specified in the options file,
 but is known to your code (``kMaxImageDataSize`` in this example being a
-constant in your own code), you can simply add it to the generated constant:
+constant in your own code), you can simply add it to the generated
+``kMaxEncodedSizeBytesWithoutValues`` constant:
 
 .. code-block:: c++
 
@@ -227,7 +245,7 @@ constant in your own code), you can simply add it to the generated constant:
      return encoder.WriteImageData(image_data);
    });
 
-   std::byte buffer[Store::kMaxEncodedSizeBytes + kMaxImageDataSize];
+   std::byte buffer[Store::kMaxEncodedSizeBytesWithoutValues + kMaxImageDataSize];
    Store::MemoryEncoder encoder(buffer);
    const auto status = encoder.Write(store);
 
@@ -254,7 +272,7 @@ from one message type to another:
      return pw::OkStatus();
    });
 
-   std::byte buffer[Person::kMaxEncodedSizeBytes +
+   std::byte buffer[Person::kMaxEncodedSizeBytesWithoutValues +
                     Grandparent::kMaxEncodedSizeBytes * 4];
    Person::MemoryEncoder encoder(buffer);
    const auto status = encoder.Write(grandchild);

pw_protobuf/pw_protobuf_test_protos/full_test.proto

@@ -92,6 +92,7 @@ message Pigweed {
   bytes data = 11;
   string description = 12;
   uint32 special_property = 13;
+
   int32 bungle = 14;
 }