diff --git a/.clang-format b/.clang-format new file mode 100644 index 0000000000..24d97f2b22 --- /dev/null +++ b/.clang-format @@ -0,0 +1,3 @@ +BasedOnStyle: Google +DerivePointerAlignment: false +PointerAlignment: Left diff --git a/.github/workflows/bazel_tests.yml b/.github/workflows/bazel_tests.yml index 062342aeea..b6e9fe24dd 100644 --- a/.github/workflows/bazel_tests.yml +++ b/.github/workflows/bazel_tests.yml @@ -1,4 +1,4 @@ -name: Bazel Tests +name: Check ClangFormat on: push: @@ -10,27 +10,11 @@ on: workflow_dispatch: jobs: - - ubuntu: - runs-on: ${{ matrix.os }} - - strategy: - fail-fast: false # Don't cancel all jobs if one fails. - matrix: - include: - - { CC: clang, os: ubuntu-20.04, flags: "" } - - { CC: clang, os: ubuntu-20.04, flags: "-c opt" } # Some warnings only fire with -c opt - - { CC: gcc, os: ubuntu-20.04, flags: "-c opt" } - - { CC: clang, os: ubuntu-20.04, flags: "--//:fasttable_enabled=true -- -cmake:test_generated_files" } - - { CC: clang, os: ubuntu-20.04, flags: "--config=asan -- -benchmarks:benchmark -python/..." } - - { CC: clang, os: macos-11, flags: "" } - + check_clang_format: + runs-on: ubuntu-20.04 steps: - uses: actions/checkout@v2 - - name: Setup Python venv - run: rm -rf /tmp/venv && python3 -m venv /tmp/venv - - name: Install dependencies - run: sudo apt install -y ${{ matrix.install }} - if: matrix.install != '' - - name: Run tests - run: cd ${{ github.workspace }} && PATH=/tmp/venv/bin:$PATH CC=${{ matrix.CC }} bazel test --test_output=errors ... ${{ matrix.flags }} + - name: Run ClangFormat + run: find . | grep -E '\.(c|h|cc)$' | grep -E -v '^./(third_party|cmake)' | xargs clang-format -i + - name: Check for differences + run: git diff --exit-code diff --git a/benchmarks/benchmark.cc b/benchmarks/benchmark.cc index 06c16fd91a..368780e1f3 100644 --- a/benchmarks/benchmark.cc +++ b/benchmarks/benchmark.cc @@ -33,6 +33,7 @@ #include "benchmarks/descriptor_sv.pb.h" #include "google/ads/googleads/v7/services/google_ads_service.upbdefs.h" #include "google/protobuf/descriptor.pb.h" +#include "google/protobuf/dynamic_message.h" #include "upb/def.hpp" upb_StringView descriptor = benchmarks_descriptor_proto_upbdefinit.descriptor; @@ -70,47 +71,89 @@ static void BM_ArenaInitialBlockOneAlloc(benchmark::State& state) { } BENCHMARK(BM_ArenaInitialBlockOneAlloc); -static void BM_LoadDescriptor_Upb(benchmark::State& state) { - size_t bytes_per_iter = 0; - for (auto _ : state) { - upb::SymbolTable symtab; - upb_benchmark_DescriptorProto_getmsgdef(symtab.ptr()); - bytes_per_iter = _upb_DefPool_BytesLoaded(symtab.ptr()); +enum LoadDescriptorMode { + NoLayout, + WithLayout, +}; + +// This function is mostly copied from upb/def.c, but it is modified to avoid +// passing in the pre-generated mini-tables, in order to force upb to compute +// them dynamically. Generally you would never want to do this, but we want to +// simulate the cost we would pay if we were loading these types purely from +// descriptors, with no mini-tales available. +bool LoadDefInit_BuildLayout(upb_DefPool* s, const _upb_DefPool_Init* init, + size_t* bytes) { + _upb_DefPool_Init** deps = init->deps; + google_protobuf_FileDescriptorProto* file; + upb_Arena* arena; + upb_Status status; + + upb_Status_Clear(&status); + + if (upb_DefPool_FindFileByName(s, init->filename)) { + return true; } - state.SetBytesProcessed(state.iterations() * bytes_per_iter); + + arena = upb_Arena_New(); + + for (; *deps; deps++) { + if (!LoadDefInit_BuildLayout(s, *deps, bytes)) goto err; + } + + file = google_protobuf_FileDescriptorProto_parse_ex( + init->descriptor.data, init->descriptor.size, NULL, + kUpb_DecodeOption_AliasString, arena); + *bytes += init->descriptor.size; + + if (!file) { + upb_Status_SetErrorFormat( + &status, + "Failed to parse compiled-in descriptor for file '%s'. This should " + "never happen.", + init->filename); + goto err; + } + + // KEY DIFFERENCE: Here we pass in only the descriptor, and not the + // pre-generated minitables. + if (!upb_DefPool_AddFile(s, file, &status)) { + goto err; + } + + upb_Arena_Free(arena); + return true; + +err: + fprintf(stderr, + "Error loading compiled-in descriptor for file '%s' (this should " + "never happen): %s\n", + init->filename, upb_Status_ErrorMessage(&status)); + exit(1); } -BENCHMARK(BM_LoadDescriptor_Upb); +template static void BM_LoadAdsDescriptor_Upb(benchmark::State& state) { size_t bytes_per_iter = 0; for (auto _ : state) { upb::SymbolTable symtab; - google_ads_googleads_v7_services_SearchGoogleAdsRequest_getmsgdef( - symtab.ptr()); - bytes_per_iter = _upb_DefPool_BytesLoaded(symtab.ptr()); - } - state.SetBytesProcessed(state.iterations() * bytes_per_iter); -} -BENCHMARK(BM_LoadAdsDescriptor_Upb); - -static void BM_LoadDescriptor_Proto2(benchmark::State& state) { - for (auto _ : state) { - protobuf::Arena arena; - protobuf::StringPiece input(descriptor.data, descriptor.size); - auto proto = - protobuf::Arena::CreateMessage(&arena); - protobuf::DescriptorPool pool; - bool ok = proto->ParseFrom(input) && - pool.BuildFile(*proto) != nullptr; - if (!ok) { - printf("Failed to add file.\n"); - exit(1); + if (Mode == NoLayout) { + google_ads_googleads_v7_services_SearchGoogleAdsRequest_getmsgdef( + symtab.ptr()); + bytes_per_iter = _upb_DefPool_BytesLoaded(symtab.ptr()); + } else { + bytes_per_iter = 0; + LoadDefInit_BuildLayout( + symtab.ptr(), + &google_ads_googleads_v7_services_google_ads_service_proto_upbdefinit, + &bytes_per_iter); } } - state.SetBytesProcessed(state.iterations() * descriptor.size); + state.SetBytesProcessed(state.iterations() * bytes_per_iter); } -BENCHMARK(BM_LoadDescriptor_Proto2); +BENCHMARK_TEMPLATE(BM_LoadAdsDescriptor_Upb, NoLayout); +BENCHMARK_TEMPLATE(BM_LoadAdsDescriptor_Upb, WithLayout); +template static void BM_LoadAdsDescriptor_Proto2(benchmark::State& state) { extern _upb_DefPool_Init google_ads_googleads_v7_services_google_ads_service_proto_upbdefinit; @@ -136,10 +179,22 @@ static void BM_LoadAdsDescriptor_Proto2(benchmark::State& state) { } bytes_per_iter += input.size(); } + + if (Mode == WithLayout) { + protobuf::DynamicMessageFactory factory; + const protobuf::Descriptor* d = pool.FindMessageTypeByName( + "google.ads.googleads.v7.services.SearchGoogleAdsResponse"); + if (!d) { + printf("Failed to find descriptor.\n"); + exit(1); + } + factory.GetPrototype(d); + } } state.SetBytesProcessed(state.iterations() * bytes_per_iter); } -BENCHMARK(BM_LoadAdsDescriptor_Proto2); +BENCHMARK_TEMPLATE(BM_LoadAdsDescriptor_Proto2, NoLayout); +BENCHMARK_TEMPLATE(BM_LoadAdsDescriptor_Proto2, WithLayout); enum CopyStrings { Copy, @@ -154,7 +209,6 @@ enum ArenaMode { template static void BM_Parse_Upb_FileDesc(benchmark::State& state) { - size_t bytes = 0; for (auto _ : state) { upb_Arena* arena; if (AMode == InitBlock) { @@ -170,7 +224,6 @@ static void BM_Parse_Upb_FileDesc(benchmark::State& state) { printf("Failed to parse.\n"); exit(1); } - bytes += descriptor.size; upb_Arena_Free(arena); } state.SetBytesProcessed(state.iterations() * descriptor.size); @@ -223,7 +276,6 @@ using FileDescSV = ::upb_benchmark::sv::FileDescriptorProto; template void BM_Parse_Proto2(benchmark::State& state) { - size_t bytes = 0; constexpr protobuf::MessageLite::ParseFlags kParseFlags = kCopy == Copy ? protobuf::MessageLite::ParseFlags::kMergePartial @@ -237,7 +289,6 @@ void BM_Parse_Proto2(benchmark::State& state) { printf("Failed to parse.\n"); exit(1); } - bytes += descriptor.size; } state.SetBytesProcessed(state.iterations() * descriptor.size); } @@ -247,12 +298,10 @@ BENCHMARK_TEMPLATE(BM_Parse_Proto2, FileDesc, InitBlock, Copy); BENCHMARK_TEMPLATE(BM_Parse_Proto2, FileDescSV, InitBlock, Alias); static void BM_SerializeDescriptor_Proto2(benchmark::State& state) { - size_t bytes = 0; upb_benchmark::FileDescriptorProto proto; proto.ParseFromArray(descriptor.data, descriptor.size); for (auto _ : state) { proto.SerializePartialToArray(buf, sizeof(buf)); - bytes += descriptor.size; } state.SetBytesProcessed(state.iterations() * descriptor.size); } diff --git a/tests/BUILD b/tests/BUILD index f00360711d..3d2474450a 100644 --- a/tests/BUILD +++ b/tests/BUILD @@ -83,6 +83,7 @@ proto_library( srcs = [ "test_cpp.proto", ], + deps = ["@com_google_protobuf//:timestamp_proto"] ) upb_proto_library( diff --git a/tests/bindings/lua/test.proto b/tests/bindings/lua/test.proto index 4c3b64edb5..c751436016 100644 --- a/tests/bindings/lua/test.proto +++ b/tests/bindings/lua/test.proto @@ -32,3 +32,38 @@ message TestLargeFieldNumber { message TestTimestamp { optional google.protobuf.Timestamp ts = 1; } + +message HelloRequest { + optional uint32 id = 1; + optional uint32 random_name_a0 = 2; + optional uint32 random_name_a1 = 3; + optional uint32 random_name_a2 = 4; + optional uint32 random_name_a3 = 5; + optional uint32 random_name_a4 = 6; + optional uint32 random_name_a5 = 7; + optional uint32 random_name_a6 = 8; + optional uint32 random_name_a7 = 9; + optional uint32 random_name_a8 = 10; + optional uint32 random_name_a9 = 11; + optional uint32 random_name_b0 = 12; + optional uint32 random_name_b1 = 13; + optional uint32 random_name_b2 = 14; + optional uint32 random_name_b3 = 15; + optional uint32 random_name_b4 = 16; + optional uint32 random_name_b5 = 17; + optional uint32 random_name_b6 = 18; + optional uint32 random_name_b7 = 19; + optional uint32 random_name_b8 = 20; + optional uint32 random_name_b9 = 21; + optional uint32 random_name_c0 = 22; + optional uint32 random_name_c1 = 23; + optional uint32 random_name_c2 = 24; + optional uint32 random_name_c3 = 25; + optional uint32 random_name_c4 = 26; + optional uint32 random_name_c5 = 27; + optional uint32 random_name_c6 = 28; + optional uint32 random_name_c7 = 29; + optional uint32 random_name_c8 = 30; + optional uint32 random_name_c9 = 31; + optional string version = 32; +} diff --git a/tests/bindings/lua/test_upb.lua b/tests/bindings/lua/test_upb.lua index 5f1aae8837..329c78ac6b 100644 --- a/tests/bindings/lua/test_upb.lua +++ b/tests/bindings/lua/test_upb.lua @@ -831,6 +831,14 @@ function test_gc() end end +function test_b9440() + local m = upb_test.HelloRequest() + m.id = 8 + assert_equal(8, m.id) + m.version = "1" + assert_equal(8, m.id) +end + local stats = lunit.main() if stats.failed > 0 or stats.errors > 0 then diff --git a/tests/test.proto b/tests/test.proto index 9c24aa05ab..f4b94b4032 100644 --- a/tests/test.proto +++ b/tests/test.proto @@ -11,3 +11,38 @@ message MessageName { optional int32 field1 = 1; optional int32 field2 = 2; } + +message HelloRequest { + optional uint32 id = 1; + optional uint32 random_name_a0 = 2; + optional uint32 random_name_a1 = 3; + optional uint32 random_name_a2 = 4; + optional uint32 random_name_a3 = 5; + optional uint32 random_name_a4 = 6; + optional uint32 random_name_a5 = 7; + optional uint32 random_name_a6 = 8; + optional uint32 random_name_a7 = 9; + optional uint32 random_name_a8 = 10; + optional uint32 random_name_a9 = 11; + optional uint32 random_name_b0 = 12; + optional uint32 random_name_b1 = 13; + optional uint32 random_name_b2 = 14; + optional uint32 random_name_b3 = 15; + optional uint32 random_name_b4 = 16; + optional uint32 random_name_b5 = 17; + optional uint32 random_name_b6 = 18; + optional uint32 random_name_b7 = 19; + optional uint32 random_name_b8 = 20; + optional uint32 random_name_b9 = 21; + optional uint32 random_name_c0 = 22; + optional uint32 random_name_c1 = 23; + optional uint32 random_name_c2 = 24; + optional uint32 random_name_c3 = 25; + optional uint32 random_name_c4 = 26; + optional uint32 random_name_c5 = 27; + optional uint32 random_name_c6 = 28; + optional uint32 random_name_c7 = 29; + optional uint32 random_name_c8 = 30; + optional uint32 random_name_c9 = 31; + optional string version = 32; +} diff --git a/tests/test_cpp.cc b/tests/test_cpp.cc index 6424452c4f..cbc4b08aa0 100644 --- a/tests/test_cpp.cc +++ b/tests/test_cpp.cc @@ -35,6 +35,8 @@ #include #include +#include "google/protobuf/timestamp.upb.h" +#include "google/protobuf/timestamp.upbdefs.h" #include "gtest/gtest.h" #include "tests/test_cpp.upb.h" #include "tests/test_cpp.upbdefs.h" @@ -148,3 +150,37 @@ TEST(Cpp, JsonNull) { EXPECT_EQ(0, strcmp(str_f.default_value().str_val.data, "abc")); EXPECT_EQ(3, str_f.default_value().str_val.size); } + +TEST(Cpp, TimestampEncoder) { + upb::SymbolTable symtab; + upb::Arena arena; + upb::MessageDefPtr md(google_protobuf_Timestamp_getmsgdef(symtab.ptr())); + google_protobuf_Timestamp* timestamp_upb = + google_protobuf_Timestamp_new(arena.ptr()); + google_protobuf_Timestamp* timestamp_upb_decoded = + google_protobuf_Timestamp_new(arena.ptr()); + + long timestamps[] = { + 253402300799, // 9999-12-31T23:59:59Z + 1641006000, // 2022-01-01T03:00:00Z + 0, // 1970-01-01T00:00:00Z + -31525200, // 1969-01-01T03:00:00Z + -2208988800, // 1900-01-01T00:00:00Z + -62135596800, // 0000-01-01T00:00:00Z + }; + + for (long timestamp : timestamps) { + google_protobuf_Timestamp_set_seconds(timestamp_upb, timestamp); + + char json[128]; + size_t size = upb_JsonEncode(timestamp_upb, md.ptr(), NULL, 0, json, + sizeof(json), NULL); + bool result = upb_JsonDecode(json, size, timestamp_upb_decoded, md.ptr(), + NULL, 0, arena.ptr(), NULL); + const long timestamp_decoded = + google_protobuf_Timestamp_seconds(timestamp_upb_decoded); + + ASSERT_TRUE(result); + EXPECT_EQ(timestamp, timestamp_decoded); + } +} diff --git a/tests/test_generated_code.cc b/tests/test_generated_code.cc index 89372890f9..4f3d715e0b 100644 --- a/tests/test_generated_code.cc +++ b/tests/test_generated_code.cc @@ -30,9 +30,10 @@ * upb/def.c and tests/conformance_upb.c, respectively). */ +#include "gtest/gtest.h" #include "src/google/protobuf/test_messages_proto3.upb.h" #include "tests/test.upb.h" -#include "gtest/gtest.h" +#include "upb/upb.hpp" #define MIN(x, y) ((x) < (y) ? (x) : (y)) @@ -295,12 +296,14 @@ static void check_int32_map_one_entry( EXPECT_EQ( 1, protobuf_test_messages_proto3_TestAllTypesProto3_map_int32_int32_size( msg)); - EXPECT_TRUE(protobuf_test_messages_proto3_TestAllTypesProto3_map_int32_int32_get( - msg, test_int32, &val)); + EXPECT_TRUE( + protobuf_test_messages_proto3_TestAllTypesProto3_map_int32_int32_get( + msg, test_int32, &val)); EXPECT_EQ(val, test_int32_2); - EXPECT_FALSE(protobuf_test_messages_proto3_TestAllTypesProto3_map_int32_int32_get( - msg, test_int32_3, &val)); + EXPECT_FALSE( + protobuf_test_messages_proto3_TestAllTypesProto3_map_int32_int32_get( + msg, test_int32_3, &val)); /* Test that iteration reveals a single k/v pair in the map. */ iter = kUpb_Map_Begin; @@ -411,6 +414,16 @@ TEST(GeneratedCode, TestRepeated) { upb_Arena_Free(arena); } +TEST(GeneratedCode, Issue9440) { + upb::Arena arena; + upb_test_HelloRequest* msg = upb_test_HelloRequest_new(arena.ptr()); + upb_test_HelloRequest_set_id(msg, 8); + EXPECT_EQ(8, upb_test_HelloRequest_id(msg)); + char str[] = "1"; + upb_test_HelloRequest_set_version(msg, upb_StringView{str, strlen(str)}); + EXPECT_EQ(8, upb_test_HelloRequest_id(msg)); +} + TEST(GeneratedCode, NullDecodeBuffer) { upb_Arena* arena = upb_Arena_New(); protobuf_test_messages_proto3_TestAllTypesProto3* msg = diff --git a/tests/test_table.cc b/tests/test_table.cc index 5ba2276438..e1a1d40733 100644 --- a/tests/test_table.cc +++ b/tests/test_table.cc @@ -460,7 +460,7 @@ TEST_P(IntTableTest, TestIntTable) { uint32_t largest_key = 0; std::map m; std::unordered_map hm; - for (const auto& key: keys_) { + for (const auto& key : keys_) { largest_key = UPB_MAX((int32_t)largest_key, key); table.Insert(key, key * 2); m[key] = key * 2; diff --git a/upb/def.c b/upb/def.c index d8db187c7e..9fe8c7e9fa 100644 --- a/upb/def.c +++ b/upb/def.c @@ -1574,7 +1574,7 @@ static void make_layout(symtab_addctx* ctx, const upb_MessageDef* m) { } /* Account for space used by hasbits. */ - l->size = div_round_up(hasbit, 8); + l->size = hasbit ? div_round_up(hasbit + 1, 8) : 0; /* Allocate non-oneof fields. */ for (int i = 0; i < m->field_count; i++) { diff --git a/upb/encode.c b/upb/encode.c index d800df84b5..f09dd4a245 100644 --- a/upb/encode.c +++ b/upb/encode.c @@ -544,8 +544,8 @@ static void encode_message(upb_encstate* e, const upb_Message* msg, * other. */ size_t ext_count; const upb_Message_Extension* ext = _upb_Message_Getexts(msg, &ext_count); - const upb_Message_Extension* end = ext + ext_count; if (ext_count) { + const upb_Message_Extension* end = ext + ext_count; for (; ext != end; ext++) { if (UPB_UNLIKELY(m->ext == upb_ExtMode_IsMessageSet)) { encode_msgset_item(e, ext); diff --git a/upb/json_encode.c b/upb/json_encode.c index 64826e34dd..e0b35f1320 100644 --- a/upb/json_encode.c +++ b/upb/json_encode.c @@ -162,7 +162,8 @@ static void jsonenc_timestamp(jsonenc* e, const upb_Message* msg, * Fliegel, H. F., and Van Flandern, T. C., "A Machine Algorithm for * Processing Calendar Dates," Communications of the Association of * Computing Machines, vol. 11 (1968), p. 657. */ - L = (int)(seconds / 86400) + 68569 + 2440588; + seconds += 62135596800; // Ensure seconds is positive. + L = (int)(seconds / 86400) - 719162 + 68569 + 2440588; N = 4 * L / 146097; L = L - (146097 * N + 3) / 4; I = 4000 * (L + 1) / 1461001; diff --git a/upb/upb.c b/upb/upb.c index a702d48e55..55730bbe82 100644 --- a/upb/upb.c +++ b/upb/upb.c @@ -341,7 +341,6 @@ static void upb_FixLocale(char* p) { } } - void _upb_EncodeRoundTripDouble(double val, char* buf, size_t size) { assert(size >= kUpb_RoundTripBufferSize); snprintf(buf, size, "%.*g", DBL_DIG, val); diff --git a/upb/upb_internal.h b/upb/upb_internal.h index d14a175836..1eb166f7d3 100644 --- a/upb/upb_internal.h +++ b/upb/upb_internal.h @@ -61,9 +61,7 @@ struct upb_Arena { // the beginning. // // The given buffer size must be at least kUpb_RoundTripBufferSize. -enum { - kUpb_RoundTripBufferSize = 32 -}; +enum { kUpb_RoundTripBufferSize = 32 }; void _upb_EncodeRoundTripDouble(double val, char* buf, size_t size); void _upb_EncodeRoundTripFloat(float val, char* buf, size_t size); diff --git a/upbc/message_layout.cc b/upbc/message_layout.cc index dd2edcbc0b..be63a105c8 100644 --- a/upbc/message_layout.cc +++ b/upbc/message_layout.cc @@ -190,7 +190,7 @@ void MessageLayout::PlaceNonOneofFields( } // Place hasbits at the beginning. - hasbit_bytes_ = DivRoundUp(hasbit_count_, 8); + hasbit_bytes_ = hasbit_count_ ? DivRoundUp(hasbit_count_ + 1, 8) : 0; Place(SizeAndAlign{{hasbit_bytes_, hasbit_bytes_}, {1, 1}}); // Place non-oneof fields.