This required enabling the feature in the code generator and fixing a few edge cases around label and type.
Also added tests to verify the special cases, and to verify that required fields work as expected.
PiperOrigin-RevId: 580263087
This change moves almost everything in the `upb/` directory up one level, so
that for example `upb/upb/generated_code_support.h` becomes just
`upb/generated_code_support.h`. The only exceptions I made to this were that I
left `upb/cmake` and `upb/BUILD` where they are, mostly because that avoids
conflict with other files and the current locations seem reasonable for now.
The `python/` directory is a little bit of a challenge because we had to merge
the existing directory there with `upb/python/`. I made `upb/python/BUILD` into
the BUILD file for the merged directory, and it effectively loads the contents
of the other BUILD file via `python/build_targets.bzl`, but I plan to clean
this up soon.
PiperOrigin-RevId: 568651768
The next in a series of CLs to split upb/BUILD into subdirs.
Create mem/internal/
Delete the deprecated upb/arena.h and upb/alloc.h stub headers
PiperOrigin-RevId: 552864952
This special `proto_library` behavior is known as an *alias library*:
https://bazel.build/reference/be/protocol-buffer#proto_library_args
Without this CL, the compilation of the added test fails with:
```
test_import_empty_srcs.upb.c:12:10: error:
module :test_import_empty_srcs_proto.upb does not depend on a
module exporting 'test.upb.h'
```
PiperOrigin-RevId: 545153380
After this change, `mini_table` only has MiniTable definitions themselves. Everything having to do with the MiniDescriptor wire format is in `mini_descriptor`.
Also rearranged some of the files in mini_table to have better structure for `internal/`.
This CL contains no functional change.
PiperOrigin-RevId: 543529112
The `kUpb_DecodeOption_ExperimentalAllowUnlinked` flag to the decoder will enable the new behavior. When that flag is not passed, tree shaking with the old model will still be possible.
"Dynamic tree shaking" in upb is a feature that allows messages to be parsed even if the MiniTables have not been fully linked. Unlinked sub-message fields can be parsed by preserving their data in the unknown fields. If the application later discovers that the message field is actually needed, the MiniTable can be patched to properly link that field, and existing message instances can "promote" the data from the unknown fields to an actual message of the correct type.
Before this change, dynamic tree shaking stored unparsed message data in the unknown fields of the *parent*. In effect, we were treating the field as if it did not exist at all. This meant that parsing an unlinked field did not affect the hasbits or oneof cases of the parent, nor did it create a `upb_Array` or `upb_Map` for array/map fields. Only when a message was linked and promoted did any of these things occur.
While this model had some amount of conceptual simplicity, it caused significant problems with oneofs. When multiple fields inside a single oneof are parsed from the wire, order matters, because later oneof fields must overwrite earlier ones. Dynamic tree shaking can mean that some fields in a oneof are linked while others are not. It is essential that we preserve this ordering semantic even when dynamic tree shaking is being used, but it is difficult to do if the oneof's data can be split between linked fields (which have been reified into parsed field data) and unlinked fields (whose data lives in the unknown fields of the parent).
To solve this problem, this CL changes the representation for unlinked fields. Instead of being placed in the parent's unknown fields, we create an actual message instance for each unlinked message we parse, but we use a placeholder "empty message" MiniTable as the message's type. All of the message's data will therefore be placed into the "empty message's" unknown fields. But unlike before, this "empty message" is actually present according to the hasbits, oneof case, and `upb_Array`/`upb_Map` of the parent. This means that all of the oneof presence logic works as normal.
Since the MiniTable can be patched at any time, we need a bit in the message instance itself to signal whether a pointer to a sub-message is an "empty message" or not. When dynamic tree shaking is in use, all users must be capable of recognizing an empty message and acting accordingly (promoting, etc) even if the MiniTable itself says that the field is linked.
Because dynamic tree shaking imposes this extra requirement on users, we require that users pass an extra option to the decoder to allow parsing of unlinked sub-messages. Many existing users of upb (Ruby, PHP, Python, etc) will always have fully-linked MiniTables, so there is no reason for them to add extra logic to handle empty messages. By omitting the `kUpb_DecodeOption_ExperimentalAllowUnlinked` option, they will be relieved of the duty to check the tagged pointer that would indicate an empty, unlinked message.
For existing users of dynamic tree shaking, there are three main changes:
1. The APIs in message/promote.h have changed, and users will need to update to the new interfaces.
2. The model for maps has changed slightly. Before, we required that map entries always had their values linked; for dynamic tree shaking to apply to maps, we required that the *entry* was left unlinked, not the entry's value. In the new model, that is reversed: map entries must always be linked, but a map entry's value can be unlinked.
3. The presence model for unlinked fields has changed. Unlinked fields will now register as "present" from the perspective of hasbits, oneof cases, and array/map entries. Users must test the tagged pointer to know if a message is of the correct, linked type or whether it is a placeholder "empty" message. There is a new function `upb_Message_GetTaggedMessagePtr()`, as well as a new accessor `upb_MessageValue.tagged_msg_val` that can be used to read and test the tagged pointer directly.
PiperOrigin-RevId: 535288031
It no longer has any users. If we need it later, we can add it back.
This saves one pointer of memory from `sizeof(upb_Arena)`.
Also, we now allow fuses if if the block allocators differ. This is made possible by cl/520144430, but was not take advantage of in that CL.
PiperOrigin-RevId: 520174588
This fixes an MSAN warning of the form:
```
WARNING: MemorySanitizer: use-of-uninitialized-value
#0 0x7fc964f2a597 in arena_findroot third_party/upb/upb/mem/arena.c:64:3
#1 0x7fc964f2a597 in upb_Arena_Free third_party/upb/upb/mem/arena.c:211:7
#2 0x7fc9d2af0028 in std::__msan::unique_ptr<upb_Arena, void (*)(upb_Arena*)>::~unique_ptr() third_party/crosstool/v18/stable/toolchain/bin/../include/c++/v1/__memory/unique_ptr.h:277:7
#3 0x7fc9d2aef7a2 in ~Arena third_party/upb/upb/upb.hpp:70:7
#4 0x7fc9d2aef7a2 in ~InlinedArena third_party/upb/upb/upb.hpp:97:7
#5 0x7fc9d2aef7a2 in Cpp_InlinedArena2_Test::TestBody() third_party/upb/upb/test/test_cpp.cc:187:1
#6 0x7fc97da78a57 in testing::Test::Run() third_party/googletest/googletest/src/gtest.cc:2695:5
#7 0x7fc97da7a3e8 in testing::TestInfo::Run() third_party/googletest/googletest/src/gtest.cc:2844:11
#8 0x7fc97da7b897 in testing::TestSuite::Run() third_party/googletest/googletest/src/gtest.cc:3003:30
#9 0x7fc97daa5136 in testing::internal::UnitTestImpl::RunAllTests() third_party/googletest/googletest/src/gtest.cc:5899:44
#10 0x7fc97daa455c in HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool> third_party/googletest/googletest/src/gtest.cc
#11 0x7fc97daa455c in testing::UnitTest::Run() third_party/googletest/googletest/src/gtest.cc:5464:10
#12 0x562a7fb876f0 in RUN_ALL_TESTS third_party/googletest/googletest/include/gtest/gtest.h:2329:73
#13 0x562a7fb876f0 in main testing/base/internal/gunit_main.cc:86:10
#14 0x7fc9ba9b7632 in __libc_start_main (/usr/grte/v5/lib64/libc.so.6+0x61632) (BuildId: 280088eab084c30a3992a9bce5c35b44)
#15 0x562a7fafdbe9 in _start /build/work/ab393f4ac612f9027aae6b1a7226027ba2a2/google3/blaze-out/k8-opt/bin/third_party/grte/v5_src/grte-scratch/BUILD/src/csu/../sysdeps/x86_64/start.S:120
Member fields were destroyed
#0 0x562a7fb0b13d in __sanitizer_dtor_callback_fields third_party/llvm/llvm-project/compiler-rt/lib/msan/msan_interceptors.cpp:960:5
#1 0x7fc9d2aef79a in ~InlinedArena third_party/upb/upb/upb.hpp:105:8
#2 0x7fc9d2aef79a in ~InlinedArena third_party/upb/upb/upb.hpp:97:7
#3 0x7fc9d2aef79a in Cpp_InlinedArena2_Test::TestBody() third_party/upb/upb/test/test_cpp.cc:187:1
#4 0x7fc97da78a57 in testing::Test::Run() third_party/googletest/googletest/src/gtest.cc:2695:5
#5 0x7fc97da7a3e8 in testing::TestInfo::Run() third_party/googletest/googletest/src/gtest.cc:2844:11
#6 0x7fc97da7b897 in testing::TestSuite::Run() third_party/googletest/googletest/src/gtest.cc:3003:30
#7 0x7fc97daa5136 in testing::internal::UnitTestImpl::RunAllTests() third_party/googletest/googletest/src/gtest.cc:5899:44
#8 0x7fc97daa455c in HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool> third_party/googletest/googletest/src/gtest.cc
#9 0x7fc97daa455c in testing::UnitTest::Run() third_party/googletest/googletest/src/gtest.cc:5464:10
#10 0x562a7fb876f0 in RUN_ALL_TESTS third_party/googletest/googletest/include/gtest/gtest.h:2329:73
#11 0x562a7fb876f0 in main testing/base/internal/gunit_main.cc:86:10
#12 0x7fc9ba9b7632 in __libc_start_main (/usr/grte/v5/lib64/libc.so.6+0x61632) (BuildId: 280088eab084c30a3992a9bce5c35b44)
#13 0x562a7fafdbe9 in _start /build/work/ab393f4ac612f9027aae6b1a7226027ba2a2/google3/blaze-out/k8-opt/bin/third_party/grte/v5_src/grte-scratch/BUILD/src/csu/../sysdeps/x86_64/start.S:120
```
PiperOrigin-RevId: 511849224
The upb convention is that "_Build()" means to also allocate, which this function does not do, so rename it as "_Init()" to free up the name for a future function that does allocate.
PiperOrigin-RevId: 510282736
The initial motivation for this change was to fix a bug found by fuzzing. The old fuzz test (built on `cc_fuzz_target()`) detected an infinite loop if a bytes field default has an unterminated `\x` escape.
To fix this bug while expanding fuzz coverage, I created a fuzz test that verifies that we can do a lossless round trip from descriptor -> DefPool -> descriptor. We use C++ as the source of truth for whether a descriptor is valid or not, and what the canonical serialization back to protobuf form should be.
I wrote the new fuzz test using go/FuzzTest, which makes it easier and more readable to use an arbitrary `FileDescriptorSet` as input, while adding test cases for regressions.
The fuzz test highlighted a handful of errors that I subsequently fixed and added regression tests for:
1. The aforementioned unterminated `\x` bug.
2. We were not propagating the `edition` field.
3. We were missing the CheckIdent() check in a few places.
4. We were rejecting files with empty name, whereas C++ allows this.
5. There were a few bugs with escaping string defaults.
Since FuzzTest is Clang-only, I split the `FUZZ_TEST()` invocation from the regression tests, since the latter are portable and should be run on all platforms. Only `FUZZ_TEST()` itself is in a google3/Clang-only file.
PiperOrigin-RevId: 506997362
Currently these functions are hardwired to always return true, but the upstream
code now checks for failures (which will be implemented soon).
PiperOrigin-RevId: 504943663
The initial motivation for this CL was to fix a bug found by fuzzing. But the fuzz bug pointed out a few edge cases that this CL corrects:
1. The core bug is that we were allowing a map entry sub-message to be linked to a group field. This is not allowed in protobuf schemas, but we did not check for this edge case in `upb_MiniTable_SetSubMessage()`, so we were de facto allowing it. This triggered some bad behavior in the parser whereby we pushed a limit without checking its validity first.
2. To defend against this, I added asserts in `upb_MiniTable_SetSubMessage()` to verify the type of the field we are linking, to ensure that a group field is not linked to a map entry sub-message. But this should probably be changed to return an error instead of relying on asserts for this.
3. I changed the fuzz util code that builds the MiniTable so that it will never violate this new invariant. The fuzz util code now can run into situations where a group field has no valid non-map-entry sub-message to select. In those cases it will simply not register any sub-message for that field.
4. Previously group did not support leaving sub-messages unregistered. Previously I added this feature for sub-messages but not for groups. There is no reason why dynamic tree shaking should not work for group fields, so I extended the feature to support groups also.
PiperOrigin-RevId: 504913630
Eric Salo
Protobuf Team Bot
Joshua Haberman
Jakob Buchgraber
Adam Cozzette