From df93cf65a2957bd0921462cb9d055c5b82a6d68b Mon Sep 17 00:00:00 2001 From: Joshua Haberman Date: Mon, 10 Apr 2023 11:25:06 -0700 Subject: [PATCH] Hide upb_MiniTableField.submsg_index with new `UPB_PRIVATE()` macro The fields of upb_MiniTableField are intended to be internal-only, accessed only through public functions like `upb_MiniTable_GetSubMessageTable()`. But over time, clients have started accessing many of these fields directly. This is an easy mistake to make, as there is no clear signal that the fields should not be used in applications. This makes the implementation difficult to change without breaking users. The new `UPB_PRIVATE()` macro appends an unpredictable string to each private symbol. This makes it very difficult to accidentally use a private symbol, since users would need to write something like `field->submsg_index_dont_copy_me__upb_internal_use_only`. This is still possible to do, but it leaves a clear wart in the code showing that an an encapsulation break has occurred. The `UPB_PRIVATE()` macro itself is defined in `port/def.inc`, which users cannot include directly. Once we land this, more such CLs will follow for the other fields of `upb_MiniTable*`. We will add inline functions as needed to provide the semantic functionality needed by users. PiperOrigin-RevId: 523166901 --- BUILD | 1 + upb/message/accessors.c | 7 ++++--- upb/message/accessors.h | 4 ++-- upb/message/accessors_test.cc | 11 +++++++---- upb/message/copy.c | 6 +++--- upb/mini_table/common.h | 17 +++++++++++++++-- upb/mini_table/decode.c | 10 ++++++---- upb/mini_table/field_internal.h | 6 +++++- upb/port/def.inc | 2 ++ upb/port/undef.inc | 1 + upb/wire/decode.c | 17 +++++++++-------- upb/wire/encode.c | 10 +++++----- upbc/protoc-gen-upb.cc | 8 ++++---- 13 files changed, 64 insertions(+), 36 deletions(-) diff --git a/BUILD b/BUILD index 24bde078ab..3f06ac371e 100644 --- a/BUILD +++ b/BUILD @@ -329,6 +329,7 @@ cc_test( ":collections", ":message_accessors", ":mini_table_internal", + ":port", ":upb", "//upb/test:test_messages_proto2_upb_proto", "//upb/test:test_messages_proto3_upb_proto", diff --git a/upb/message/accessors.c b/upb/message/accessors.c index c10ffece99..dc3ea01e3d 100644 --- a/upb/message/accessors.c +++ b/upb/message/accessors.c @@ -199,7 +199,8 @@ upb_UnknownToMessageRet upb_MiniTable_PromoteUnknownToMessage( upb_Message* message = NULL; // Callers should check that message is not set first before calling // PromotoUnknownToMessage. - UPB_ASSERT(mini_table->subs[field->submsg_index].submsg == sub_mini_table); + UPB_ASSERT(upb_MiniTable_GetSubMessageTable(mini_table, field) == + sub_mini_table); bool is_oneof = _upb_MiniTableField_InOneOf(field); if (!is_oneof || _upb_getoneofcase_field(msg, field) == field->number) { UPB_ASSERT(upb_Message_GetMessage(msg, field, NULL) == NULL); @@ -286,7 +287,7 @@ upb_MapInsertStatus upb_Message_InsertMapEntry(upb_Map* map, upb_Message* map_entry_message, upb_Arena* arena) { const upb_MiniTable* map_entry_mini_table = - mini_table->subs[field->submsg_index].submsg; + mini_table->subs[field->UPB_PRIVATE(submsg_index)].submsg; UPB_ASSERT(map_entry_mini_table); UPB_ASSERT(map_entry_mini_table->field_count == 2); const upb_MiniTableField* map_entry_key_field = @@ -311,7 +312,7 @@ upb_UnknownToMessage_Status upb_MiniTable_PromoteUnknownToMap( upb_Message* msg, const upb_MiniTable* mini_table, const upb_MiniTableField* field, int decode_options, upb_Arena* arena) { const upb_MiniTable* map_entry_mini_table = - mini_table->subs[field->submsg_index].submsg; + mini_table->subs[field->UPB_PRIVATE(submsg_index)].submsg; UPB_ASSERT(map_entry_mini_table); UPB_ASSERT(map_entry_mini_table); UPB_ASSERT(map_entry_mini_table->field_count == 2); diff --git a/upb/message/accessors.h b/upb/message/accessors.h index 74ca1f2d5a..141a97d4df 100644 --- a/upb/message/accessors.h +++ b/upb/message/accessors.h @@ -552,7 +552,7 @@ UPB_API_INLINE void upb_Message_SetMessage(upb_Message* msg, UPB_ASSUME(!upb_IsRepeatedOrMap(field)); UPB_ASSUME(_upb_MiniTableField_GetRep(field) == UPB_SIZE(kUpb_FieldRep_4Byte, kUpb_FieldRep_8Byte)); - UPB_ASSERT(mini_table->subs[field->submsg_index].submsg); + UPB_ASSERT(mini_table->subs[field->UPB_PRIVATE(submsg_index)].submsg); _upb_Message_SetNonExtensionField(msg, field, &sub_message); } @@ -564,7 +564,7 @@ UPB_API_INLINE upb_Message* upb_Message_GetOrCreateMutableMessage( upb_Message* sub_message = *UPB_PTR_AT(msg, field->offset, upb_Message*); if (!sub_message) { const upb_MiniTable* sub_mini_table = - mini_table->subs[field->submsg_index].submsg; + mini_table->subs[field->UPB_PRIVATE(submsg_index)].submsg; UPB_ASSERT(sub_mini_table); sub_message = _upb_Message_New(sub_mini_table, arena); *UPB_PTR_AT(msg, field->offset, upb_Message*) = sub_message; diff --git a/upb/message/accessors_test.cc b/upb/message/accessors_test.cc index a0dece4317..910a700994 100644 --- a/upb/message/accessors_test.cc +++ b/upb/message/accessors_test.cc @@ -49,6 +49,9 @@ #include "upb/wire/common.h" #include "upb/wire/decode.h" +// Must be last +#include "upb/port/def.inc" + namespace { // Proto2 test messages field numbers used for reflective access. @@ -580,10 +583,10 @@ upb_MiniTable* CreateMiniTableWithEmptySubTables(upb_Arena* arena) { // Initialize sub table to null. Not using upb_MiniTable_SetSubMessage // since it checks ->ext on parameter. upb_MiniTableSub* sub = const_cast( - &table->subs[table->fields[1].submsg_index]); + &table->subs[table->fields[1].UPB_PRIVATE(submsg_index)]); sub->submsg = nullptr; sub = const_cast( - &table->subs[table->fields[2].submsg_index]); + &table->subs[table->fields[2].UPB_PRIVATE(submsg_index)]); sub->submsg = nullptr; return table; } @@ -605,10 +608,10 @@ upb_MiniTable* CreateMiniTableWithEmptySubTablesForMaps(upb_Arena* arena) { // Initialize sub table to null. Not using upb_MiniTable_SetSubMessage // since it checks ->ext on parameter. upb_MiniTableSub* sub = const_cast( - &table->subs[table->fields[1].submsg_index]); + &table->subs[table->fields[1].UPB_PRIVATE(submsg_index)]); sub->submsg = nullptr; sub = const_cast( - &table->subs[table->fields[2].submsg_index]); + &table->subs[table->fields[2].UPB_PRIVATE(submsg_index)]); sub->submsg = nullptr; return table; } diff --git a/upb/message/copy.c b/upb/message/copy.c index 570e6c1c72..3f2c6079e8 100644 --- a/upb/message/copy.c +++ b/upb/message/copy.c @@ -101,7 +101,7 @@ upb_Map* upb_Map_DeepClone(const upb_Map* map, upb_CType key_type, while (upb_Map_Next(map, &key, &val, &iter)) { const upb_MiniTableField* value_field = &map_entry_table->fields[1]; const upb_MiniTable* value_sub = - (value_field->submsg_index != kUpb_NoSub) + (value_field->UPB_PRIVATE(submsg_index) != kUpb_NoSub) ? upb_MiniTable_GetSubMessageTable(map_entry_table, value_field) : NULL; upb_CType value_field_type = upb_MiniTableField_CType(value_field); @@ -122,7 +122,7 @@ static upb_Map* upb_Message_Map_DeepClone(const upb_Map* map, upb_Message* clone, upb_Arena* arena) { const upb_MiniTable* map_entry_table = - mini_table->subs[field->submsg_index].submsg; + mini_table->subs[field->UPB_PRIVATE(submsg_index)].submsg; UPB_ASSERT(map_entry_table); const upb_MiniTableField* key_field = &map_entry_table->fields[0]; @@ -166,7 +166,7 @@ static bool upb_Message_Array_DeepClone(const upb_Array* array, _upb_MiniTableField_CheckIsArray(field); upb_Array* cloned_array = upb_Array_DeepClone( array, upb_MiniTableField_CType(field), - field->submsg_index != kUpb_NoSub + field->UPB_PRIVATE(submsg_index) != kUpb_NoSub ? upb_MiniTable_GetSubMessageTable(mini_table, field) : NULL, arena); diff --git a/upb/mini_table/common.h b/upb/mini_table/common.h index 5169d1c9f1..4fc9473688 100644 --- a/upb/mini_table/common.h +++ b/upb/mini_table/common.h @@ -112,14 +112,27 @@ UPB_API_INLINE bool upb_MiniTableField_HasPresence( } } +// Returns the MiniTable for this message field. If the field is unlinked, +// returns NULL. UPB_API_INLINE const upb_MiniTable* upb_MiniTable_GetSubMessageTable( const upb_MiniTable* mini_table, const upb_MiniTableField* field) { - return mini_table->subs[field->submsg_index].submsg; + UPB_ASSERT(upb_MiniTableField_CType(field) == kUpb_CType_Message); + return mini_table->subs[field->UPB_PRIVATE(submsg_index)].submsg; } +// Returns the MiniTableEnum for this enum field. If the field is unlinked, +// returns NULL. UPB_API_INLINE const upb_MiniTableEnum* upb_MiniTable_GetSubEnumTable( const upb_MiniTable* mini_table, const upb_MiniTableField* field) { - return mini_table->subs[field->submsg_index].subenum; + UPB_ASSERT(upb_MiniTableField_CType(field) == kUpb_CType_Enum); + return mini_table->subs[field->UPB_PRIVATE(submsg_index)].subenum; +} + +// Returns true if this MiniTable field is linked to a MiniTable for the +// sub-message. +UPB_API_INLINE bool upb_MiniTable_MessageFieldIsLinked( + const upb_MiniTable* mini_table, const upb_MiniTableField* field) { + return upb_MiniTable_GetSubMessageTable(mini_table, field) != NULL; } // If this field is in a oneof, returns the first field in the oneof. diff --git a/upb/mini_table/decode.c b/upb/mini_table/decode.c index 72f6dd0b82..71e15898c2 100644 --- a/upb/mini_table/decode.c +++ b/upb/mini_table/decode.c @@ -172,9 +172,9 @@ static void upb_MiniTable_SetTypeAndSub(upb_MiniTableField* field, } if (upb_MiniTable_HasSub(field, msg_modifiers)) { - field->submsg_index = sub_count ? (*sub_count)++ : 0; + field->UPB_PRIVATE(submsg_index) = sub_count ? (*sub_count)++ : 0; } else { - field->submsg_index = kUpb_NoSub; + field->UPB_PRIVATE(submsg_index) = kUpb_NoSub; } if (upb_MtDecoder_FieldIsPackable(field) && @@ -1046,7 +1046,8 @@ bool upb_MiniTable_SetSubMessage(upb_MiniTable* table, return false; } - upb_MiniTableSub* table_sub = (void*)&table->subs[field->submsg_index]; + upb_MiniTableSub* table_sub = + (void*)&table->subs[field->UPB_PRIVATE(submsg_index)]; table_sub->submsg = sub; return true; } @@ -1058,7 +1059,8 @@ bool upb_MiniTable_SetSubEnum(upb_MiniTable* table, upb_MiniTableField* field, (uintptr_t)(table->fields + table->field_count)); UPB_ASSERT(sub); - upb_MiniTableSub* table_sub = (void*)&table->subs[field->submsg_index]; + upb_MiniTableSub* table_sub = + (void*)&table->subs[field->UPB_PRIVATE(submsg_index)]; table_sub->subenum = sub; return true; } diff --git a/upb/mini_table/field_internal.h b/upb/mini_table/field_internal.h index a258c10f74..0e36a076bf 100644 --- a/upb/mini_table/field_internal.h +++ b/upb/mini_table/field_internal.h @@ -40,7 +40,11 @@ struct upb_MiniTableField { uint32_t number; uint16_t offset; int16_t presence; // If >0, hasbit_index. If <0, ~oneof_index - uint16_t submsg_index; // kUpb_NoSub if descriptortype != MESSAGE/GROUP/ENUM + + // Indexes into `upb_MiniTable.subs` + // Will be set to `kUpb_NoSub` if `descriptortype` != MESSAGE/GROUP/ENUM + uint16_t UPB_PRIVATE(submsg_index); + uint8_t descriptortype; // upb_FieldMode | upb_LabelFlags | (upb_FieldRep << kUpb_FieldRep_Shift) diff --git a/upb/port/def.inc b/upb/port/def.inc index 7c36a03fd4..c1cf36f287 100644 --- a/upb/port/def.inc +++ b/upb/port/def.inc @@ -200,6 +200,8 @@ /* UPB_PTRADD(ptr, ofs): add pointer while avoiding "NULL + 0" UB */ #define UPB_PTRADD(ptr, ofs) ((ofs) ? (ptr) + (ofs) : (ptr)) +#define UPB_PRIVATE(x) x##_dont_copy_me__upb_internal_use_only + /* Configure whether fasttable is switched on or not. *************************/ #ifdef __has_attribute diff --git a/upb/port/undef.inc b/upb/port/undef.inc index 12e05d22ab..e1752f72f6 100644 --- a/upb/port/undef.inc +++ b/upb/port/undef.inc @@ -70,3 +70,4 @@ #undef UPB_IS_GOOGLE3 #undef UPB_ATOMIC #undef UPB_USE_C11_ATOMICS +#undef UPB_PRIVATE diff --git a/upb/wire/decode.c b/upb/wire/decode.c index c2ab7e0709..4a33689571 100644 --- a/upb/wire/decode.c +++ b/upb/wire/decode.c @@ -220,7 +220,7 @@ static void _upb_Decoder_Munge(int type, wireval* val) { static upb_Message* _upb_Decoder_NewSubMessage( upb_Decoder* d, const upb_MiniTableSub* subs, const upb_MiniTableField* field) { - const upb_MiniTable* subl = subs[field->submsg_index].submsg; + const upb_MiniTable* subl = subs[field->UPB_PRIVATE(submsg_index)].submsg; UPB_ASSERT(subl); upb_Message* msg = _upb_Message_New(subl, &d->arena); if (!msg) _upb_Decoder_ErrorJmp(d, kUpb_DecodeStatus_OutOfMemory); @@ -259,7 +259,7 @@ static const char* _upb_Decoder_DecodeSubMessage( upb_Decoder* d, const char* ptr, upb_Message* submsg, const upb_MiniTableSub* subs, const upb_MiniTableField* field, int size) { int saved_delta = upb_EpsCopyInputStream_PushLimit(&d->input, ptr, size); - const upb_MiniTable* subl = subs[field->submsg_index].submsg; + const upb_MiniTable* subl = subs[field->UPB_PRIVATE(submsg_index)].submsg; UPB_ASSERT(subl); ptr = _upb_Decoder_RecurseSubMessage(d, ptr, submsg, subl, DECODE_NOGROUP); upb_EpsCopyInputStream_PopLimit(&d->input, ptr, saved_delta); @@ -290,7 +290,7 @@ UPB_FORCEINLINE static const char* _upb_Decoder_DecodeKnownGroup( upb_Decoder* d, const char* ptr, upb_Message* submsg, const upb_MiniTableSub* subs, const upb_MiniTableField* field) { - const upb_MiniTable* subl = subs[field->submsg_index].submsg; + const upb_MiniTable* subl = subs[field->UPB_PRIVATE(submsg_index)].submsg; UPB_ASSERT(subl); return _upb_Decoder_DecodeGroup(d, ptr, submsg, subl, field->number); } @@ -354,7 +354,7 @@ static const char* _upb_Decoder_DecodeEnumArray(upb_Decoder* d, const char* ptr, const upb_MiniTableSub* subs, const upb_MiniTableField* field, wireval* val) { - const upb_MiniTableEnum* e = subs[field->submsg_index].subenum; + const upb_MiniTableEnum* e = subs[field->UPB_PRIVATE(submsg_index)].subenum; if (!_upb_Decoder_CheckEnum(d, ptr, msg, e, field, val)) return ptr; void* mem = UPB_PTR_AT(_upb_array_ptr(arr), arr->size * 4, void); arr->size++; @@ -425,7 +425,7 @@ static const char* _upb_Decoder_DecodeEnumPacked( upb_Decoder* d, const char* ptr, upb_Message* msg, upb_Array* arr, const upb_MiniTableSub* subs, const upb_MiniTableField* field, wireval* val) { - const upb_MiniTableEnum* e = subs[field->submsg_index].subenum; + const upb_MiniTableEnum* e = subs[field->UPB_PRIVATE(submsg_index)].subenum; int saved_limit = upb_EpsCopyInputStream_PushLimit(&d->input, ptr, val->size); char* out = UPB_PTR_AT(_upb_array_ptr(arr), arr->size * 4, void); while (!_upb_Decoder_IsDone(d, &ptr)) { @@ -586,7 +586,7 @@ static const char* _upb_Decoder_DecodeToMap(upb_Decoder* d, const char* ptr, upb_Map* map = *map_p; upb_MapEntry ent; UPB_ASSERT(upb_MiniTableField_Type(field) == kUpb_FieldType_Message); - const upb_MiniTable* entry = subs[field->submsg_index].submsg; + const upb_MiniTable* entry = subs[field->UPB_PRIVATE(submsg_index)].submsg; UPB_ASSERT(entry->field_count == 2); UPB_ASSERT(!upb_IsRepeatedOrMap(&entry->fields[0])); @@ -647,7 +647,8 @@ static const char* _upb_Decoder_DecodeToSubMessage( int type = field->descriptortype; if (UPB_UNLIKELY(op == kUpb_DecodeOp_Enum) && - !_upb_Decoder_CheckEnum(d, ptr, msg, subs[field->submsg_index].subenum, + !_upb_Decoder_CheckEnum(d, ptr, msg, + subs[field->UPB_PRIVATE(submsg_index)].subenum, field, val)) { return ptr; } @@ -963,7 +964,7 @@ static void _upb_Decoder_CheckUnlinked(const upb_MiniTable* mt, int* op) { // If sub-message is not linked, treat as unknown. if (field->mode & kUpb_LabelFlags_IsExtension) return; - const upb_MiniTableSub* sub = &mt->subs[field->submsg_index]; + const upb_MiniTableSub* sub = &mt->subs[field->UPB_PRIVATE(submsg_index)]; if (!sub->submsg) *op = kUpb_DecodeOp_UnknownField; } diff --git a/upb/wire/encode.c b/upb/wire/encode.c index ef931a7300..c75bf6f1d1 100644 --- a/upb/wire/encode.c +++ b/upb/wire/encode.c @@ -263,7 +263,7 @@ static void encode_scalar(upb_encstate* e, const void* _field_mem, case kUpb_FieldType_Group: { size_t size; void* submsg = *(void**)field_mem; - const upb_MiniTable* subm = subs[f->submsg_index].submsg; + const upb_MiniTable* subm = subs[f->UPB_PRIVATE(submsg_index)].submsg; if (submsg == NULL) { return; } @@ -277,7 +277,7 @@ static void encode_scalar(upb_encstate* e, const void* _field_mem, case kUpb_FieldType_Message: { size_t size; void* submsg = *(void**)field_mem; - const upb_MiniTable* subm = subs[f->submsg_index].submsg; + const upb_MiniTable* subm = subs[f->UPB_PRIVATE(submsg_index)].submsg; if (submsg == NULL) { return; } @@ -366,7 +366,7 @@ static void encode_array(upb_encstate* e, const upb_Message* msg, case kUpb_FieldType_Group: { const void* const* start = _upb_array_constptr(arr); const void* const* ptr = start + arr->size; - const upb_MiniTable* subm = subs[f->submsg_index].submsg; + const upb_MiniTable* subm = subs[f->UPB_PRIVATE(submsg_index)].submsg; if (--e->depth == 0) encode_err(e, kUpb_EncodeStatus_MaxDepthExceeded); do { size_t size; @@ -381,7 +381,7 @@ static void encode_array(upb_encstate* e, const upb_Message* msg, case kUpb_FieldType_Message: { const void* const* start = _upb_array_constptr(arr); const void* const* ptr = start + arr->size; - const upb_MiniTable* subm = subs[f->submsg_index].submsg; + const upb_MiniTable* subm = subs[f->UPB_PRIVATE(submsg_index)].submsg; if (--e->depth == 0) encode_err(e, kUpb_EncodeStatus_MaxDepthExceeded); do { size_t size; @@ -420,7 +420,7 @@ static void encode_map(upb_encstate* e, const upb_Message* msg, const upb_MiniTableSub* subs, const upb_MiniTableField* f) { const upb_Map* map = *UPB_PTR_AT(msg, f->offset, const upb_Map*); - const upb_MiniTable* layout = subs[f->submsg_index].submsg; + const upb_MiniTable* layout = subs[f->UPB_PRIVATE(submsg_index)].submsg; UPB_ASSERT(layout->field_count == 2); if (map == NULL) return; diff --git a/upbc/protoc-gen-upb.cc b/upbc/protoc-gen-upb.cc index 9fa67c9bba..71ebd7fa1b 100644 --- a/upbc/protoc-gen-upb.cc +++ b/upbc/protoc-gen-upb.cc @@ -1115,7 +1115,7 @@ bool TryFillTableEntry(const DefPoolPair& pools, upb::FieldDefPtr field, } if (field.ctype() == kUpb_CType_Message) { - uint64_t idx = mt_f->submsg_index; + uint64_t idx = mt_f->UPB_PRIVATE(submsg_index); if (idx > 255) return false; data |= idx << 16; @@ -1263,9 +1263,9 @@ std::string FieldInitializer(upb::FieldDefPtr field, "{$0, $1, $2, $3, $4, $5}", field64->number, ArchDependentSize(field32->offset, field64->offset), ArchDependentSize(field32->presence, field64->presence), - field64->submsg_index == kUpb_NoSub + field64->UPB_PRIVATE(submsg_index) == kUpb_NoSub ? "kUpb_NoSub" - : absl::StrCat(field64->submsg_index).c_str(), + : absl::StrCat(field64->UPB_PRIVATE(submsg_index)).c_str(), field64->descriptortype, GetModeInit(field32, field64)); } } @@ -1311,7 +1311,7 @@ void WriteMessage(upb::MessageDefPtr message, const DefPoolPair& pools, for (int i = 0; i < mt_64->field_count; i++) { const upb_MiniTableField* f = &mt_64->fields[i]; - if (f->submsg_index != kUpb_NoSub) { + if (f->UPB_PRIVATE(submsg_index) != kUpb_NoSub) { subs.push_back(GetSub(message.FindFieldByNumber(f->number))); } }