Skip to content

Commit 65d9c6d

Browse files
mkruskal-googlecopybara-github
authored andcommitted
Define a generated registry for tracking linked-in extensions.
#test-continuous This behaves similarly to the generated extension registry in other languages. A global ref-counted (lock-free thread-safe) singleton registry can be lazily constructed from the linker array data produced by gencode. For now, this is simply used to preserve custom options on the descriptors in our DefPool. This design makes heavy use of various non-standard C extensions. It expands on the pre-existing linker arrays used for extensions, adding weak constructors to register all of the extensions linked into each binary. This was tested and works with clang and gcc, and a similar approach was implemented for MSVC. For unsupported compilers or architectures, the generated registry will be unpopulated. PiperOrigin-RevId: 825343000
1 parent 12a8943 commit 65d9c6d

29 files changed

+872
-146
lines changed

.github/workflows/test_upb.yml

Lines changed: 3 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,8 @@ jobs:
3030
config:
3131
- { name: "Fastbuild" }
3232
- { name: "Optimized", flags: "-c opt", continuous-only: true }
33+
- { name: "GCC Optimized", flags: "-c opt --force_pic --java_runtime_version=remotejdk_11 --copt=\"-Wno-error=maybe-uninitialized\"", image: "us-docker.pkg.dev/protobuf-build/containers/test/linux/gcc:8.0.1-12.2-12e21b8dda91028bc14212a3ab582c7c4d149fac" }
34+
- { name: "GCC Static", flags: "-c opt --dynamic_mode=off --java_runtime_version=remotejdk_11 --copt=\"-Wno-error=maybe-uninitialized\"", image: "us-docker.pkg.dev/protobuf-build/containers/test/linux/gcc:8.0.1-12.2-12e21b8dda91028bc14212a3ab582c7c4d149fac", continuous-only: true }
3335
- { name: "ASAN", flags: "--config=asan -c dbg", exclude-targets: "-//benchmarks:benchmark -//python/...", runner: ubuntu-22-4core }
3436
- { name: "UBSAN", flags: "--config=ubsan -c dbg", exclude-targets: "-//benchmarks:benchmark -//python/... -//lua/...", continuous-only: true }
3537
- { name: "32-bit", flags: "--copt=-m32 --linkopt=-m32", exclude-targets: "-//benchmarks:benchmark -//python/..." }
@@ -49,33 +51,12 @@ jobs:
4951
if: ${{ !matrix.config.continuous-only || inputs.continuous-run }}
5052
uses: protocolbuffers/protobuf-ci/bazel-docker@v5
5153
with:
52-
image: us-docker.pkg.dev/protobuf-build/containers/test/linux/sanitize:${{ matrix.config.bazel_version || '8.0.1' }}-12e21b8dda91028bc14212a3ab582c7c4d149fac
54+
image: ${{ matrix.config.image || 'us-docker.pkg.dev/protobuf-build/containers/test/linux/sanitize:8.0.1-12e21b8dda91028bc14212a3ab582c7c4d149fac' }}
5355
credentials: ${{ secrets.GAR_SERVICE_ACCOUNT }}
5456
bazel-cache: upb-bazel
5557
bazel: test //bazel/... //benchmarks/... //lua/... //python/... //upb/... //upb_generator/... ${{ matrix.config.flags }}
5658
exclude-targets: ${{ matrix.config.exclude-targets }}
5759

58-
linux-gcc:
59-
strategy:
60-
fail-fast: false # Don't cancel all jobs if one fails.
61-
name: GCC Optimized
62-
runs-on: ubuntu-latest
63-
steps:
64-
- name: Checkout pending changes
65-
uses: protocolbuffers/protobuf-ci/checkout@v5
66-
with:
67-
ref: ${{ inputs.safe-checkout }}
68-
- name: Run tests
69-
uses: protocolbuffers/protobuf-ci/bazel-docker@v5
70-
with:
71-
image: "us-docker.pkg.dev/protobuf-build/containers/test/linux/gcc:8.0.1-12.2-12e21b8dda91028bc14212a3ab582c7c4d149fac"
72-
credentials: ${{ secrets.GAR_SERVICE_ACCOUNT }}
73-
bazel-cache: "upb-bazel-gcc"
74-
bazel: >-
75-
test -c opt
76-
--copt="-Wno-error=maybe-uninitialized" --java_runtime_version=remotejdk_11
77-
//bazel/... //benchmarks/... //lua/... //python/... //upb/... //upb_generator/...
78-
7960
windows:
8061
strategy:
8162
fail-fast: false # Don't cancel all jobs if one fails.

cmake/installed_include_golden.txt

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -185,12 +185,14 @@ upb/message/value.h
185185
upb/mini_descriptor/build_enum.h
186186
upb/mini_descriptor/decode.h
187187
upb/mini_descriptor/link.h
188+
upb/mini_table/compat.h
188189
upb/mini_table/debug_string.h
189190
upb/mini_table/enum.h
190191
upb/mini_table/extension.h
191192
upb/mini_table/extension_registry.h
192193
upb/mini_table/field.h
193194
upb/mini_table/file.h
195+
upb/mini_table/generated_registry.h
194196
upb/mini_table/message.h
195197
upb/mini_table/sub.h
196198
upb/port/atomic.h

pkg/BUILD.bazel

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -249,6 +249,7 @@ cc_dist_library(
249249
"//upb/json",
250250
"//upb/message:compare",
251251
"//upb/message:copy",
252+
"//upb/mini_table:compat",
252253
"//upb/mini_table:debug_string",
253254
"//upb/text",
254255
"//upb/text:debug",

upb/BUILD

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -98,6 +98,7 @@ cc_library(
9898
"//upb/message:internal",
9999
"//upb/mini_descriptor",
100100
"//upb/mini_table",
101+
"//upb/mini_table:internal",
101102
"//upb/wire",
102103
] + select({
103104
":fasttable_enabled_setting": [
@@ -308,9 +309,13 @@ filegroup(
308309
filegroup(
309310
name = "test_srcs",
310311
srcs = [
312+
"//upb/hash:test_srcs",
311313
"//upb/json:test_srcs",
314+
"//upb/lex:test_srcs",
312315
"//upb/mem:test_srcs",
313316
"//upb/message:test_srcs",
317+
"//upb/mini_descriptor:test_srcs",
318+
"//upb/mini_table:test_srcs",
314319
"//upb/test:test_srcs",
315320
"//upb/util:test_srcs",
316321
"//upb/wire:test_srcs",

upb/generated_code_support.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@
3636
#include "upb/mini_table/extension_registry.h"
3737
#include "upb/mini_table/field.h"
3838
#include "upb/mini_table/file.h"
39+
#include "upb/mini_table/internal/generated_registry.h"
3940
#include "upb/mini_table/message.h"
4041
#include "upb/mini_table/sub.h"
4142
#include "upb/wire/decode.h"

upb/hash/BUILD

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -65,5 +65,5 @@ filegroup(
6565
"**/*test.cc",
6666
],
6767
),
68-
visibility = ["//pkg:__pkg__"],
68+
visibility = ["//upb:__pkg__"],
6969
)

upb/lex/BUILD

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -72,5 +72,5 @@ filegroup(
7272
"**/*test.cc",
7373
],
7474
),
75-
visibility = ["//pkg:__pkg__"],
75+
visibility = ["//upb:__pkg__"],
7676
)

upb/mini_descriptor/BUILD

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -105,5 +105,5 @@ filegroup(
105105
"**/*test.cc",
106106
],
107107
),
108-
visibility = ["//pkg:__pkg__"],
108+
visibility = ["//upb:__pkg__"],
109109
)

upb/mini_table/BUILD

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@ cc_library(
2828
name = "mini_table",
2929
srcs = [
3030
"extension_registry.c",
31+
"generated_registry.c",
3132
"message.c",
3233
],
3334
hdrs = [
@@ -36,6 +37,7 @@ cc_library(
3637
"extension_registry.h",
3738
"field.h",
3839
"file.h",
40+
"generated_registry.h",
3941
"message.h",
4042
"sub.h",
4143
],
@@ -84,6 +86,7 @@ cc_library(
8486
"internal/extension.h",
8587
"internal/field.h",
8688
"internal/file.h",
89+
"internal/generated_registry.h",
8790
"internal/message.h",
8891
"internal/size_log2.h",
8992
"internal/sub.h",
@@ -92,7 +95,6 @@ cc_library(
9295
visibility = ["//visibility:public"],
9396
deps = [
9497
"//upb/base",
95-
"//upb/hash",
9698
"//upb/mem",
9799
"//upb/message:types",
98100
"//upb/port",
@@ -131,6 +133,21 @@ cc_test(
131133
],
132134
)
133135

136+
cc_test(
137+
name = "generated_registry_test",
138+
srcs = ["generated_registry_test.cc"],
139+
deps = [
140+
":mini_table",
141+
"//src/google/protobuf:descriptor_upb_minitable_proto",
142+
"//upb/test:custom_options_upb_minitable_proto",
143+
"//upb/test:editions_test_upb_minitable_proto",
144+
"//upb/test:test_multiple_files_upb_minitable_proto",
145+
"@abseil-cpp//absl/synchronization",
146+
"@googletest//:gtest",
147+
"@googletest//:gtest_main",
148+
],
149+
)
150+
134151
proto_library(
135152
name = "message_benchmark_proto",
136153
testonly = 1,

upb/mini_table/extension_registry.c

Lines changed: 33 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,8 @@
1515
#include "upb/hash/str_table.h"
1616
#include "upb/mem/arena.h"
1717
#include "upb/mini_table/extension.h"
18+
#include "upb/mini_table/generated_registry.h"
19+
#include "upb/mini_table/internal/generated_registry.h"
1820
#include "upb/mini_table/message.h"
1921

2022
// Must be last.
@@ -89,25 +91,46 @@ upb_ExtensionRegistryStatus upb_ExtensionRegistry_AddArray(
8991
return status;
9092
}
9193

92-
#ifdef UPB_LINKARR_DECLARE
93-
94-
UPB_LINKARR_DECLARE(upb_AllExts, const upb_MiniTableExtension);
94+
const UPB_PRIVATE(upb_GeneratedExtensionListEntry) *
95+
UPB_PRIVATE(upb_generated_extension_list) = NULL;
9596

9697
bool upb_ExtensionRegistry_AddAllLinkedExtensions(upb_ExtensionRegistry* r) {
97-
const upb_MiniTableExtension* start = UPB_LINKARR_START(upb_AllExts);
98-
const upb_MiniTableExtension* stop = UPB_LINKARR_STOP(upb_AllExts);
99-
for (const upb_MiniTableExtension* p = start; p < stop; p++) {
100-
// Windows can introduce zero padding, so we have to skip zeroes.
101-
if (upb_MiniTableExtension_Number(p) != 0) {
102-
if (upb_ExtensionRegistry_Add(r, p) != kUpb_ExtensionRegistryStatus_Ok) {
98+
const UPB_PRIVATE(upb_GeneratedExtensionListEntry)* entry =
99+
UPB_PRIVATE(upb_generated_extension_list);
100+
while (entry != NULL) {
101+
// Comparing pointers to different objects is undefined behavior, so we
102+
// convert them to uintptr_t and compare their values.
103+
uintptr_t begin = (uintptr_t)entry->start;
104+
uintptr_t end = (uintptr_t)entry->stop;
105+
uintptr_t current = begin;
106+
while (current < end) {
107+
const upb_MiniTableExtension* ext =
108+
(const upb_MiniTableExtension*)current;
109+
// Sentinels and padding introduced by the linker can result in zeroed
110+
// entries, so simply skip them.
111+
if (upb_MiniTableExtension_Number(ext) == 0) {
112+
// MSVC introduces padding that might not be sized exactly the same as
113+
// upb_MiniTableExtension, so we can't iterate by sizeof. This is a
114+
// valid thing for any linker to do, so it's safer to just always do it.
115+
current += UPB_ALIGN_OF(upb_MiniTableExtension);
116+
continue;
117+
}
118+
119+
if (upb_ExtensionRegistry_Add(r, ext) !=
120+
kUpb_ExtensionRegistryStatus_Ok) {
103121
return false;
104122
}
123+
current += sizeof(upb_MiniTableExtension);
105124
}
125+
entry = entry->next;
106126
}
107127
return true;
108128
}
109129

110-
#endif // UPB_LINKARR_DECLARE
130+
const upb_ExtensionRegistry* upb_ExtensionRegistry_GetGenerated(
131+
const upb_GeneratedRegistryRef* genreg) {
132+
return genreg != NULL ? genreg->registry : NULL;
133+
}
111134

112135
const upb_MiniTableExtension* upb_ExtensionRegistry_Lookup(
113136
const upb_ExtensionRegistry* r, const upb_MiniTable* t, uint32_t num) {

0 commit comments

Comments
 (0)