Skip to content

Commit 60db669

Browse files
habermancopybara-github
haberman
authored and
copybara-github
committed
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 6f887c3 commit 60db669

8 files changed

Lines changed: 134 additions & 11 deletions

File tree

‎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: 22 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,

‎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

‎upb/mini_table/generated_registry_empty_test.cc‎

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,52 @@
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+
// If linker GC of un-referenced linker array elements is working properly,
43+
// this should be zero. However, this only seems to work for ELF, and even
44+
// then seems to be ineffective on some platforms.
45+
#if defined(__ELF__) && !defined(__powerpc__)
46+
EXPECT_EQ(upb_ExtensionRegistry_Size(reg), 0);
47+
#endif
48+
49+
upb_GeneratedRegistry_Release(ref);
50+
}
51+
52+
} // namespace

‎upb/mini_table/generated_registry_empty_test.proto‎

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: 34 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -566,29 +566,52 @@ Error, UINTPTR_MAX is undefined
566566
// }
567567
// }
568568

569-
#define UPB_LINKARR_ATTR
570-
571569
#define UPB_LINKARR_SENTINEL UPB_RETAIN __attribute__((weak, used))
572570

573571
#if defined(__ELF__) || defined(__wasm__)
574572

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

586610
#elif defined(__MACH__)
587611

588612
/* As described in: https://stackoverflow.com/a/22366882 */
589613
#define UPB_LINKARR_APPEND(name) \
590-
__attribute__(( \
591-
section("__DATA,__la_" #name))) UPB_LINKARR_ATTR UPB_NO_SANITIZE_ADDRESS
614+
__attribute__((section("__DATA,__la_" #name))) UPB_NO_SANITIZE_ADDRESS
592615
#define UPB_LINKARR_DECLARE(name, type) \
593616
extern type __start_linkarr_##name __asm( \
594617
"section$start$__DATA$__la_" #name); \

0 commit comments

Comments
 (0)