Skip to content

Commit

Permalink
Clear oneof message fields even on arena on non-OPT builds.
Browse files Browse the repository at this point in the history
PiperOrigin-RevId: 616461373
  • Loading branch information
protobuf-github-bot authored and copybara-github committed Mar 16, 2024
1 parent 7ef5207 commit 5c183bd
Show file tree
Hide file tree
Showing 3 changed files with 49 additions and 1 deletion.
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@

#include "absl/log/absl_check.h"
#include "absl/memory/memory.h"
#include "absl/strings/str_cat.h"
#include "absl/strings/string_view.h"
#include "absl/strings/substitute.h"
#include "google/protobuf/compiler/cpp/field.h"
Expand Down Expand Up @@ -595,7 +596,12 @@ void OneofMessage::GenerateClearingCode(io::Printer* p) const {
p->Emit(R"cc(
if (GetArena() == nullptr) {
delete $field_$;
})cc");
} else if ($pbi$::DebugHardenClearOneofMessageOnArena()) {
if ($field_$ != nullptr) {
$field_$->Clear();
}
}
)cc");
}

void OneofMessage::GenerateMessageClearingCode(io::Printer* p) const {
Expand Down
9 changes: 9 additions & 0 deletions src/google/protobuf/port.h
Original file line number Diff line number Diff line change
Expand Up @@ -238,6 +238,15 @@ inline constexpr bool DebugHardenStringValues() {
#endif
}

// Returns true if force clearing oneof message on arena is enabled.
inline constexpr bool DebugHardenClearOneofMessageOnArena() {
#ifdef NDEBUG
return false;
#else
return true;
#endif
}

// Prefetch 5 64-byte cache line starting from 7 cache-lines ahead.
// Constants are somewhat arbitrary and pretty aggressive, but were
// chosen to give a better benchmark results. E.g. this is ~20%
Expand Down
33 changes: 33 additions & 0 deletions src/google/protobuf/proto3_arena_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
#include "absl/strings/match.h"
#include "google/protobuf/arena.h"
#include "google/protobuf/descriptor.h"
#include "google/protobuf/port.h"
#include "google/protobuf/text_format.h"
#include "google/protobuf/unittest.pb.h"
#include "google/protobuf/unittest_proto3_arena.pb.h"
Expand Down Expand Up @@ -256,6 +257,38 @@ TEST(Proto3OptionalTest, OptionalFields) {
EXPECT_EQ(serialized.size(), 0);
}

TEST(Proto3ArenaTest, CheckMessageFieldIsCleared) {
Arena arena;
auto msg = Arena::Create<TestAllTypes>(&arena);

// Referring to a saved pointer to a child message is never guaranteed to
// work. IOW, protobufs do not guarantee pointer stability. This test only
// does this to replicate (unsupported) user behaviors.
auto child = msg->mutable_optional_foreign_message();
child->set_c(100);
msg->Clear();

EXPECT_EQ(child->c(), 0);
}

TEST(Proto3ArenaTest, CheckOneofMessageFieldIsCleared) {
if (!internal::DebugHardenClearOneofMessageOnArena()) {
GTEST_SKIP() << "arena allocated oneof message fields are not cleared.";
}

Arena arena;
auto msg = Arena::Create<TestAllTypes>(&arena);

// Referring to a saved pointer to a child message is never guaranteed to
// work. IOW, protobufs do not guarantee pointer stability. This test only
// does this to replicate (unsupported) user behaviors.
auto child = msg->mutable_oneof_nested_message();
child->set_bb(100);
msg->Clear();

EXPECT_EQ(child->bb(), 0);
}

TEST(Proto3OptionalTest, OptionalFieldDescriptor) {
const Descriptor* d = protobuf_unittest::TestProto3Optional::descriptor();

Expand Down

0 comments on commit 5c183bd

Please sign in to comment.