Skip to content

Commit 01da5b4

Browse files
habermancopybara-github
authored andcommitted
Fixed linker array sentinel on GCC, and added a test.
This change modifies the `UPB_LINKARR_DECLARE` macro to use inline assembly to create a weak anchor symbol within the linkarr section. This ensures that the `__start_linkarr_` and `__stop_linkarr_` symbols are always defined by the linker, even when no other objects are placed in that section. The previous sentinel mechanism was broken on GCC. A new test, generated_registry_empty_test, is added to verify that the generated registry can be loaded successfully and is empty when no extensions are actually linked into the binary. Also adds `upb_exttable_size` and `upb_ExtensionRegistry_Size` for testing purposes. PiperOrigin-RevId: 904021133
1 parent 8d82427 commit 01da5b4

9 files changed

Lines changed: 140 additions & 14 deletions

File tree

upb/BUILD

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -310,6 +310,7 @@ filegroup(
310310
"//src/google/protobuf:well_known_type_protos",
311311
"//upb/json:test_protos",
312312
"//upb/message:test_protos",
313+
"//upb/mini_table:test_protos",
313314
"//upb/test:test_protos",
314315
"//upb/util:test_protos",
315316
"//upb/wire:test_protos",

upb/hash/common.c

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -718,6 +718,8 @@ const uint32_t* upb_exttable_remove(upb_exttable* t, const void* k,
718718
return NULL;
719719
}
720720

721+
size_t upb_exttable_size(const upb_exttable* t) { return t->t.count; }
722+
721723
/* upb_inttable ***************************************************************/
722724

723725
/* For inttables we use a hybrid structure where small keys are kept in an

upb/hash/ext_table.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,8 @@ const uint32_t* upb_exttable_lookup(const upb_exttable* t, const void* k,
5555
const uint32_t* upb_exttable_remove(upb_exttable* t, const void* k,
5656
uint32_t ext_number);
5757

58+
size_t upb_exttable_size(const upb_exttable* t);
59+
5860
#ifdef __cplusplus
5961
} /* extern "C" */
6062
#endif

upb/mini_table/BUILD

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -154,6 +154,28 @@ cc_test(
154154
],
155155
)
156156

157+
cc_test(
158+
name = "generated_registry_empty_test",
159+
srcs = ["generated_registry_empty_test.cc"],
160+
linkstatic = 1,
161+
deps = [
162+
":generated_registry_empty_test_upb_minitable_proto",
163+
":mini_table",
164+
"@googletest//:gtest",
165+
"@googletest//:gtest_main",
166+
],
167+
)
168+
169+
proto_library(
170+
name = "generated_registry_empty_test_proto",
171+
srcs = ["generated_registry_empty_test.proto"],
172+
)
173+
174+
upb_minitable_proto_library(
175+
name = "generated_registry_empty_test_upb_minitable_proto",
176+
deps = [":generated_registry_empty_test_proto"],
177+
)
178+
157179
proto_library(
158180
name = "message_benchmark_proto",
159181
testonly = 1,
@@ -240,3 +262,13 @@ filegroup(
240262
),
241263
visibility = ["//upb:__pkg__"],
242264
)
265+
266+
filegroup(
267+
name = "test_protos",
268+
srcs = glob(
269+
[
270+
"**/*test.proto",
271+
],
272+
),
273+
visibility = ["//upb:__pkg__"],
274+
)

upb/mini_table/extension_registry.c

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -84,3 +84,7 @@ const upb_MiniTableExtension* upb_ExtensionRegistry_Lookup(
8484
const uint32_t* v = upb_exttable_lookup(&r->exts, t, num);
8585
return (const upb_MiniTableExtension*)v;
8686
}
87+
88+
size_t upb_ExtensionRegistry_Size(const upb_ExtensionRegistry* r) {
89+
return upb_exttable_size(&r->exts);
90+
}

upb/mini_table/extension_registry.h

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -87,6 +87,9 @@ UPB_NODISCARD upb_ExtensionRegistryStatus upb_ExtensionRegistry_AddArray(
8787
UPB_API const upb_MiniTableExtension* upb_ExtensionRegistry_Lookup(
8888
const upb_ExtensionRegistry* r, const upb_MiniTable* t, uint32_t num);
8989

90+
// Returns the number of extensions in the registry. For testing/debugging only.
91+
UPB_API size_t upb_ExtensionRegistry_Size(const upb_ExtensionRegistry* r);
92+
9093
#ifdef __cplusplus
9194
} /* extern "C" */
9295
#endif
Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,45 @@
1+
2+
#include <gtest/gtest.h>
3+
#include "upb/mini_table/extension_registry.h"
4+
#include "upb/mini_table/generated_registry.h"
5+
#include "upb/mini_table/generated_registry_empty_test.upb_minitable.h"
6+
#include "upb/mini_table/message.h"
7+
8+
namespace {
9+
10+
// Tests that we can instantiate the registry even if no extensions are linked.
11+
// This ensures that the sentinel object in the linker array properly ensures
12+
// that the encapsulation symbols are defined (eg. __start_linkarr_exts,
13+
// __stop_linkarr_exts) even when no extensions were linked.
14+
TEST(GeneratedRegistryEmptyTest, Instantiate) {
15+
// Strongly reference the generated MiniTable to ensure that the TU for
16+
// generated_registry_empty_test.upb_minitable.c is pulled in.
17+
const upb_MiniTable* volatile table =
18+
&upb_0test_0empty_0registry__EmptyRegistryTestMessage_msg_init;
19+
(void)table;
20+
21+
// Test that the registry can be loaded, even if no extensions are linked.
22+
// If we did not have a sentinel in the linker array, we would get a linker
23+
// error here like:
24+
//
25+
// ld: error: undefined symbol: __start_linkarr_upb_AllExts
26+
// >>> referenced by generated_registry_empty_test.upb_minitable.c
27+
// >>>
28+
// generated_registry_empty_test.upb_minitable.pic.o:(upb_GeneratedRegistry_Constructor_dont_copy_me__upb_internal_use_only.entry)
29+
// >>> the encapsulation symbol needs to be retained under --gc-sections
30+
// properly; consider -z nostart-stop-gc (see
31+
// https://lld.llvm.org/ELF/start-stop-gc)
32+
//
33+
// ld: error: undefined symbol: __stop_linkarr_upb_AllExts
34+
// >>> referenced by generated_registry_empty_test.upb_minitable.c
35+
// >>>
36+
// generated_registry_empty_test.upb_minitable.pic.o:(upb_GeneratedRegistry_Constructor_dont_copy_me__upb_internal_use_only.entry)
37+
const upb_GeneratedRegistryRef* ref = upb_GeneratedRegistry_Load();
38+
EXPECT_NE(ref, nullptr);
39+
const upb_ExtensionRegistry* reg = upb_GeneratedRegistry_Get(ref);
40+
EXPECT_NE(reg, nullptr);
41+
42+
upb_GeneratedRegistry_Release(ref);
43+
}
44+
45+
} // namespace
Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
edition = "2024";
2+
3+
package upb_test_empty_registry;
4+
5+
// A message that is in the same file as the unused extension.
6+
message EmptyRegistryTestMessage {
7+
extensions 1 to max;
8+
}
9+
10+
// An extension that we will *not* reference. The existence of this extension
11+
// will cause us to emit the code to register all extensions, but since this
12+
// extension is never referenced, that list of extensions will be empty.
13+
extend EmptyRegistryTestMessage {
14+
int32 unused_extension = 1;
15+
}

upb/port/def.inc

Lines changed: 36 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -566,37 +566,59 @@ Error, UINTPTR_MAX is undefined
566566
// }
567567
// }
568568

569-
#define UPB_LINKARR_ATTR
570-
571-
#define UPB_LINKARR_SENTINEL UPB_RETAIN __attribute__((weak, used))
572-
569+
#define UPB_LINKARR_SENTINEL
573570
#if defined(__ELF__) || defined(__wasm__)
574571

575572
#define UPB_LINKARR_APPEND(name) \
576-
__attribute__(( \
577-
section("linkarr_" #name))) UPB_LINKARR_ATTR UPB_NO_SANITIZE_ADDRESS
578-
#define UPB_LINKARR_DECLARE(name, type) \
579-
extern type __start_linkarr_##name; \
580-
extern type __stop_linkarr_##name; \
581-
UPB_LINKARR_APPEND(name) \
582-
UPB_LINKARR_SENTINEL type UPB_linkarr_internal_empty_##name[1]
573+
__attribute__((section("linkarr_" #name))) UPB_NO_SANITIZE_ADDRESS
574+
#define UPB_LINKARR_DECLARE(name, type) \
575+
extern type __start_linkarr_##name; \
576+
extern type __stop_linkarr_##name; \
577+
\
578+
/* This function defines a sentinel symbol that is used to ensure that the \
579+
* linker symbols __start_linkarr_##name and __stop_linkarr_##name are \
580+
* defined even if no (real) elements are added to the linkarr. \
581+
* \
582+
* We unfortunately have to use inline assembly here because GCC will \
583+
* refuse to create multiple sections with the same name and different \
584+
* flags: https://github.com/protocolbuffers/protobuf/issues/26385 \
585+
* \
586+
* We have to use a function wrapper in order to propagate `sizeof(type)` \
587+
* into the assembly (globally-scoped asm() blocks can't access values, \
588+
* even constant expressions). \
589+
*/ \
590+
/* clang-format off */ \
591+
static __attribute__((used)) void upb_linkarr_wrapper_##name(void) { \
592+
__asm__ volatile ( \
593+
".pushsection linkarr_" #name ",\"awR\",@progbits,unique,1\n\t" \
594+
".p2align 8\n\t" \
595+
".weak UPB_linkarr_internal_anchor_" #name "\n\t" \
596+
".type UPB_linkarr_internal_anchor_" #name ", @object\n\t" \
597+
"UPB_linkarr_internal_anchor_" #name ":\n\t" \
598+
".fill %c[size], 1, 0\n\t" \
599+
".size UPB_linkarr_internal_anchor_" #name ", %c[size]\n\t" \
600+
".popsection" \
601+
: \
602+
: [size] "n" (sizeof(type)) \
603+
); \
604+
}
605+
/* clang-format on */
583606
#define UPB_LINKARR_START(name) (&__start_linkarr_##name)
584607
#define UPB_LINKARR_STOP(name) (&__stop_linkarr_##name)
585608

586609
#elif defined(__MACH__)
587610

588611
/* As described in: https://stackoverflow.com/a/22366882 */
589612
#define UPB_LINKARR_APPEND(name) \
590-
__attribute__(( \
591-
section("__DATA,__la_" #name))) UPB_LINKARR_ATTR UPB_NO_SANITIZE_ADDRESS
613+
__attribute__((section("__DATA,__la_" #name))) UPB_NO_SANITIZE_ADDRESS
592614
#define UPB_LINKARR_DECLARE(name, type) \
593615
extern type __start_linkarr_##name __asm( \
594616
"section$start$__DATA$__la_" #name); \
595617
extern type __stop_linkarr_##name __asm( \
596618
"section$end$__DATA$" \
597619
"__la_" #name); \
598620
UPB_LINKARR_APPEND(name) \
599-
UPB_LINKARR_SENTINEL type UPB_linkarr_internal_empty_##name[1]
621+
__attribute__((weak, used)) type UPB_linkarr_internal_empty_##name[1]
600622
#define UPB_LINKARR_START(name) (&__start_linkarr_##name)
601623
#define UPB_LINKARR_STOP(name) (&__stop_linkarr_##name)
602624

0 commit comments

Comments
 (0)