Skip to content

Commit 1b4b5fc

Browse files
acozzettecopybara-github
authored andcommitted
Update Rust generator to output minidescriptors instead of C minitables
With this change, we no longer emit any C generated code for the Rust upb implementation. Instead, we output minidescriptors in the Rust generated code, and use these to construct the minitables at run time. Each message's `AssociatedMiniTable::mini_table()` method will lazily build the minitable on first use. We need a place to put the minidescriptor and lazily-built minitable for enums, so I added a new `AssociatedMiniTableEnum` trait for this purpose. This is only necessary for closed enums. There are a couple of things that were somewhat difficult about building the minitables at runtime: - Each map entry is represented as its own implicit message type and needs a minitable. To facilitate this, I updated the generator to output a visibility-controlled struct to represent each map entry. This way we can implement `AssociatedMiniTable` on it and treat it like any other message for the purpose of building minitables. - The naive way of recursively building minitables is vulnerable to deadlock in the case of cycles in the message graph. Such cycles are allowed within a single .proto file, so we need to be able to handle this. My change handles this problem by treating each strongly-connected component (SCC) as its own unit, and then the graph of SCCs has no cycles. Each SCC has an arbitrariliy chosen representative responsible for linking all messages in the SCC. PiperOrigin-RevId: 761654774
1 parent 49d9e2d commit 1b4b5fc

26 files changed

+462
-99
lines changed

rust/bazel/aspects.bzl

Lines changed: 6 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@ load("@rules_rust//rust/private:providers.bzl", "CrateInfo", "DepInfo", "DepVari
77

88
# buildifier: disable=bzl-visibility
99
load("@rules_rust//rust/private:rustc.bzl", "rustc_compile_action")
10-
load("//bazel:upb_minitable_proto_library.bzl", "UpbMinitableCcInfo", "upb_minitable_proto_library_aspect")
1110
load("//bazel/common:proto_common.bzl", "proto_common")
1211
load("//bazel/common:proto_info.bzl", "ProtoInfo")
1312
load("//bazel/private:cc_proto_aspect.bzl", "cc_proto_aspect")
@@ -347,9 +346,8 @@ def _rust_proto_aspect_common(target, ctx, is_upb):
347346
is_upb,
348347
)
349348

350-
if is_upb:
351-
thunks_cc_info = target[UpbMinitableCcInfo].cc_info
352-
else:
349+
dep_variant_info_for_native_gencode = []
350+
if not is_upb:
353351
dep_cc_infos = []
354352
for dep in proto_deps:
355353
dep_cc_infos.append(dep[CcInfo])
@@ -363,6 +361,8 @@ def _rust_proto_aspect_common(target, ctx, is_upb):
363361
cc_infos = [target[CcInfo]] + [dep[CcInfo] for dep in ctx.attr._cpp_thunks_deps] + dep_cc_infos,
364362
) for thunk in cc_thunks_gencode])
365363

364+
dep_variant_info_for_native_gencode = [DepVariantInfo(cc_info = thunks_cc_info)]
365+
366366
runtime = proto_lang_toolchain.runtime
367367
dep_variant_info_for_runtime = DepVariantInfo(
368368
crate_info = runtime[CrateInfo] if CrateInfo in runtime else None,
@@ -371,14 +371,12 @@ def _rust_proto_aspect_common(target, ctx, is_upb):
371371
build_info = None,
372372
)
373373

374-
dep_variant_info_for_native_gencode = DepVariantInfo(cc_info = thunks_cc_info)
375-
376374
dep_variant_info = _compile_rust(
377375
ctx = ctx,
378376
attr = ctx.rule.attr,
379377
src = entry_point_rs_output,
380378
extra_srcs = rs_gencode,
381-
deps = [dep_variant_info_for_runtime, dep_variant_info_for_native_gencode] + dep_variant_infos,
379+
deps = [dep_variant_info_for_runtime] + dep_variant_info_for_native_gencode + dep_variant_infos,
382380
runtime = runtime,
383381
)
384382
return [RustProtoInfo(
@@ -396,7 +394,7 @@ def _make_proto_library_aspect(is_upb):
396394
return aspect(
397395
implementation = (_rust_upb_proto_aspect_impl if is_upb else _rust_cc_proto_aspect_impl),
398396
attr_aspects = ["deps"],
399-
requires = ([upb_minitable_proto_library_aspect] if is_upb else [cc_proto_aspect]),
397+
requires = ([] if is_upb else [cc_proto_aspect]),
400398
attrs = {
401399
"_collect_cc_coverage": attr.label(
402400
default = Label("@rules_rust//util:collect_coverage"),

rust/test/rust_proto_library_unit_test/rust_proto_library_unit_test.bzl

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -116,10 +116,6 @@ def _rust_upb_aspect_test_impl(ctx):
116116
# The action needs to produce a .rlib artifact (sometimes .rmeta as well, not tested here).
117117
asserts.true(env, rustc_action.outputs.to_list()[0].path.endswith(".rlib"))
118118

119-
# The aspect needs to provide CcInfo that passes UPB gencode to the eventual linking.
120-
_find_linker_input(target_under_test[RustProtoInfo], "child.upb_minitable")
121-
_find_linker_input(target_under_test[RustProtoInfo], "parent.upb_minitable")
122-
123119
return analysistest.end(env)
124120

125121
rust_upb_aspect_test = analysistest.make(_rust_upb_aspect_test_impl)

rust/test/shared/BUILD

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -320,6 +320,7 @@ rust_test(
320320
},
321321
deps = [
322322
"//rust:protobuf_cpp_export",
323+
"//rust/test:map_unittest_cpp_rust_proto",
323324
"//rust/test:nested_cpp_rust_proto",
324325
"@crate_index//:googletest",
325326
],
@@ -333,6 +334,7 @@ rust_test(
333334
},
334335
deps = [
335336
"//rust:protobuf_upb_export",
337+
"//rust/test:map_unittest_upb_rust_proto",
336338
"//rust/test:nested_upb_rust_proto",
337339
"@crate_index//:googletest",
338340
],

rust/test/shared/simple_nested_test.rs

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,9 +6,11 @@
66
// https://developers.google.com/open-source/licenses/bsd
77

88
use googletest::prelude::*;
9+
use map_unittest_rust_proto::TestRecursiveMapMessage;
910
use nested_rust_proto::outer::inner::InnerEnum;
1011
use nested_rust_proto::outer::InnerView;
1112
use nested_rust_proto::*;
13+
use protobuf::prelude::*;
1214

1315
#[gtest]
1416
fn test_deeply_nested_message() {
@@ -120,3 +122,12 @@ fn test_recursive_mut() {
120122
// let nested = rec.rec_mut().rec_mut().rec_mut();
121123
// assert_that!(nested.num(), eq(0));
122124
}
125+
126+
#[gtest]
127+
fn test_recursive_map() {
128+
let mut m1 = TestRecursiveMapMessage::new();
129+
m1.a_mut().insert("k", TestRecursiveMapMessage::new());
130+
let m2 = TestRecursiveMapMessage::parse(&m1.serialize().unwrap()).unwrap();
131+
assert_that!(m2.a().len(), eq(1));
132+
expect_that!(m2.a().get("k"), not(none()));
133+
}

rust/test/upb/BUILD

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,3 +49,16 @@ rust_test(
4949
"@crate_index//:googletest",
5050
],
5151
)
52+
53+
rust_test(
54+
name = "thread_local_arena_test",
55+
srcs = ["thread_local_arena_test.rs"],
56+
aliases = {
57+
"//rust:protobuf_upb_export": "protobuf",
58+
},
59+
deps = [
60+
"//rust:protobuf_upb_export",
61+
"//rust/test:unittest_upb_rust_proto",
62+
"@crate_index//:googletest",
63+
],
64+
)
Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
// Protocol Buffers - Google's data interchange format
2+
// Copyright 2024 Google LLC. All rights reserved.
3+
//
4+
// Use of this source code is governed by a BSD-style
5+
// license that can be found in the LICENSE file or at
6+
// https://developers.google.com/open-source/licenses/bsd
7+
8+
use googletest::prelude::*;
9+
use protobuf::prelude::*;
10+
use unittest_rust_proto::TestAllTypes;
11+
12+
#[gtest]
13+
fn test_thread_local_arena() {
14+
// We use a thread-local arena to allocate minitables, so let's verify that a minitable remains
15+
// valid even after that the thread that initialized it has exited.
16+
let handle = std::thread::spawn(|| {
17+
let mut m = TestAllTypes::new();
18+
m.set_optional_int32(3);
19+
let _ = m.serialize();
20+
});
21+
handle.join().unwrap();
22+
23+
let mut m1 = TestAllTypes::new();
24+
m1.set_optional_int32(5);
25+
let m2 = TestAllTypes::parse(&m1.serialize().unwrap()).unwrap();
26+
assert_that!(m1.optional_int32(), eq(m2.optional_int32()));
27+
}

rust/upb.rs

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,18 @@ pub type RawRepeatedField = upb::RawArray;
3333
pub type RawMap = upb::RawMap;
3434
pub type PtrAndLen = upb::StringView;
3535

36+
// This struct represents a raw minitable pointer. We need it to be Send and Sync so that we can
37+
// store it in a static OnceLock for lazy initialization of minitables. It should not be used for
38+
// any other purpose.
39+
pub struct MiniTablePtr(pub *mut upb_MiniTable);
40+
unsafe impl Send for MiniTablePtr {}
41+
unsafe impl Sync for MiniTablePtr {}
42+
43+
// Same as above, but for enum minitables.
44+
pub struct MiniTableEnumPtr(pub *const upb_MiniTableEnum);
45+
unsafe impl Send for MiniTableEnumPtr {}
46+
unsafe impl Sync for MiniTableEnumPtr {}
47+
3648
impl From<&ProtoStr> for PtrAndLen {
3749
fn from(s: &ProtoStr) -> Self {
3850
let bytes = s.as_bytes();
@@ -61,6 +73,12 @@ impl ScratchSpace {
6173
}
6274
}
6375

76+
thread_local! {
77+
// We need to avoid dropping this Arena, because we use it to build mini tables that
78+
// effectively have 'static lifetimes.
79+
pub static THREAD_LOCAL_ARENA: ManuallyDrop<Arena> = ManuallyDrop::new(Arena::new());
80+
}
81+
6482
#[doc(hidden)]
6583
pub type SerializedData = upb::OwnedArenaBox<[u8]>;
6684

rust/upb/BUILD

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,7 @@ cc_library(
5252
"//upb:message",
5353
"//upb:message_compare",
5454
"//upb:message_copy",
55+
"//upb/mini_descriptor",
5556
"//upb/mini_table",
5657
"//upb/text:debug",
5758
],

rust/upb/associated_mini_table.rs

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -6,9 +6,10 @@
66
// https://developers.google.com/open-source/licenses/bsd
77

88
use super::upb_MiniTable;
9+
use super::upb_MiniTableEnum;
910

10-
/// A trait for types which have a constant associated MiniTable (e.g.
11-
/// generated messages, and their mut and view proxy types).
11+
/// A trait for types which have an associated MiniTable (e.g. generated
12+
/// messages, and their mut and view proxy types).
1213
///
1314
/// upb_Message in C is effectively a DST type, where instances are created with
1415
/// a MiniTable (and have a size dependent on the given MiniTable). Many upb
@@ -20,12 +21,6 @@ use super::upb_MiniTable;
2021
/// which hold upb_Message* to simplify ensuring the upb C invariants
2122
/// are maintained.
2223
///
23-
/// Note that this would prefer to be a `const MINI_TABLE: *const upb_MiniTable`
24-
/// to statically associate a single MiniTable, but as long as the MiniTable is
25-
/// an extern "C" we cannot do that without the unstable `const_refs_to_static`.
26-
/// After that feature is stabilized (or if we move the MiniTable generation to
27-
/// .rs) this will be switched.
28-
///
2924
/// SAFETY:
3025
/// - The MiniTable pointer must be from Protobuf code generation and follow the
3126
/// corresponding invariants associated with upb's C API (the pointer should
@@ -34,3 +29,8 @@ use super::upb_MiniTable;
3429
pub unsafe trait AssociatedMiniTable {
3530
fn mini_table() -> *const upb_MiniTable;
3631
}
32+
33+
/// A trait for closed enums that have an associated MiniTable.
34+
pub unsafe trait AssociatedMiniTableEnum {
35+
fn mini_table() -> *const upb_MiniTableEnum;
36+
}

rust/upb/lib.rs

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@ pub use array::{
1919
};
2020

2121
mod associated_mini_table;
22-
pub use associated_mini_table::AssociatedMiniTable;
22+
pub use associated_mini_table::{AssociatedMiniTable, AssociatedMiniTableEnum};
2323

2424
mod ctype;
2525
pub use ctype::CType;
@@ -41,8 +41,9 @@ pub use message_value::{upb_MessageValue, upb_MutableMessageValue};
4141

4242
mod mini_table;
4343
pub use mini_table::{
44-
upb_MiniTable, upb_MiniTableField, upb_MiniTable_FindFieldByNumber,
45-
upb_MiniTable_GetFieldByIndex, upb_MiniTable_SubMessage, RawMiniTable, RawMiniTableField,
44+
upb_MiniTable, upb_MiniTableEnum, upb_MiniTableEnum_Build, upb_MiniTableField,
45+
upb_MiniTable_Build, upb_MiniTable_FindFieldByNumber, upb_MiniTable_GetFieldByIndex,
46+
upb_MiniTable_Link, upb_MiniTable_SubMessage, upb_Status, RawMiniTable, RawMiniTableField,
4647
};
4748

4849
mod opaque_pointee;

0 commit comments

Comments
 (0)