From f60cbe585fb33ad1ed1d5063c52f189a953068c8 Mon Sep 17 00:00:00 2001 From: Martijn Vels Date: Mon, 20 Mar 2023 11:21:47 -0700 Subject: [PATCH] Fix absl log and check headers and deps for port_def.inc PiperOrigin-RevId: 518025056 --- src/google/protobuf/BUILD.bazel | 13 ++++ src/google/protobuf/json/internal/parser.cc | 20 +++--- src/google/protobuf/json/internal/unparser.cc | 19 +++--- src/google/protobuf/port.cc | 52 +++++++++++++++ src/google/protobuf/port_def.inc | 66 ++++++++++++------- src/google/protobuf/port_test.cc | 56 ++++++++++++++++ src/google/protobuf/port_undef.inc | 1 - 7 files changed, 186 insertions(+), 41 deletions(-) create mode 100644 src/google/protobuf/port.cc create mode 100644 src/google/protobuf/port_test.cc diff --git a/src/google/protobuf/BUILD.bazel b/src/google/protobuf/BUILD.bazel index f96ebe174d..4d3268bbca 100644 --- a/src/google/protobuf/BUILD.bazel +++ b/src/google/protobuf/BUILD.bazel @@ -168,6 +168,7 @@ proto_library( cc_library( name = "port_def", + srcs = ["port.cc"], hdrs = [ "port.h", "port_def.inc", @@ -210,6 +211,18 @@ cc_test( ], ) +cc_test( + name = "port_test", + srcs = ["port_test.cc"], + deps = [ + ":port_def", + ":varint_shuffle", + "@com_google_absl//absl/log:absl_check", + "@com_google_googletest//:gtest", + "@com_google_googletest//:gtest_main", + ], +) + cc_library( name = "arena_align", srcs = ["arena_align.cc"], diff --git a/src/google/protobuf/json/internal/parser.cc b/src/google/protobuf/json/internal/parser.cc index 8975e10d32..af12372de0 100644 --- a/src/google/protobuf/json/internal/parser.cc +++ b/src/google/protobuf/json/internal/parser.cc @@ -45,6 +45,8 @@ #include "google/protobuf/message.h" #include "absl/base/attributes.h" #include "absl/container/flat_hash_set.h" +#include "absl/log/absl_check.h" +#include "absl/log/absl_log.h" #include "absl/status/status.h" #include "absl/status/statusor.h" #include "absl/strings/ascii.h" @@ -1302,7 +1304,9 @@ absl::Status ParseMessage(JsonLexer& lex, const Desc& desc, absl::Status JsonStringToMessage(absl::string_view input, Message* message, json_internal::ParseOptions options) { MessagePath path(message->GetDescriptor()->full_name()); - PROTOBUF_DLOG(INFO) << "json2/input: " << absl::CHexEscape(input); + if (PROTOBUF_DEBUG) { + ABSL_DLOG(INFO) << "json2/input: " << absl::CHexEscape(input); + } io::ArrayInputStream in(input.data(), input.size()); JsonLexer lex(&in, options, &path); @@ -1315,9 +1319,10 @@ absl::Status JsonStringToMessage(absl::string_view input, Message* message, "extraneous characters after end of JSON object"); } - PROTOBUF_DLOG(INFO) << "json2/status: " << s; - PROTOBUF_DLOG(INFO) << "json2/output: " << message->DebugString(); - + if (PROTOBUF_DEBUG) { + ABSL_DLOG(INFO) << "json2/status: " << s; + ABSL_DLOG(INFO) << "json2/output: " << message->DebugString(); + } return s; } @@ -1348,10 +1353,9 @@ absl::Status JsonToBinaryStream(google::protobuf::util::TypeResolver* resolver, } tee_input.emplace(copy.data(), copy.size()); tee_output.emplace(&out); + ABSL_DLOG(INFO) << "json2/input: " << absl::CHexEscape(copy); } - PROTOBUF_DLOG(INFO) << "json2/input: " << absl::CHexEscape(copy); - // This scope forces the CodedOutputStream inside of `msg` to flush before we // possibly handle logging the binary protobuf output. absl::Status s; @@ -1377,10 +1381,10 @@ absl::Status JsonToBinaryStream(google::protobuf::util::TypeResolver* resolver, tee_output.reset(); // Flush the output stream. io::zc_sink_internal::ZeroCopyStreamByteSink(binary_output) .Append(out.data(), out.size()); + ABSL_DLOG(INFO) << "json2/status: " << s; + ABSL_DLOG(INFO) << "json2/output: " << absl::BytesToHexString(out); } - PROTOBUF_DLOG(INFO) << "json2/status: " << s; - PROTOBUF_DLOG(INFO) << "json2/output: " << absl::BytesToHexString(out); return s; } } // namespace json_internal diff --git a/src/google/protobuf/json/internal/unparser.cc b/src/google/protobuf/json/internal/unparser.cc index b363ad52aa..db277ca25d 100644 --- a/src/google/protobuf/json/internal/unparser.cc +++ b/src/google/protobuf/json/internal/unparser.cc @@ -46,6 +46,7 @@ #include "google/protobuf/dynamic_message.h" #include "google/protobuf/message.h" #include "absl/log/absl_check.h" +#include "absl/log/absl_log.h" #include "absl/status/status.h" #include "absl/strings/ascii.h" #include "absl/strings/escaping.h" @@ -854,16 +855,20 @@ absl::Status WriteMessage(JsonWriter& writer, const Msg& msg, absl::Status MessageToJsonString(const Message& message, std::string* output, json_internal::WriterOptions options) { - PROTOBUF_DLOG(INFO) << "json2/input: " << message.DebugString(); + if (PROTOBUF_DEBUG) { + ABSL_DLOG(INFO) << "json2/input: " << message.DebugString(); + } io::StringOutputStream out(output); JsonWriter writer(&out, options); absl::Status s = WriteMessage( writer, message, *message.GetDescriptor(), /*is_top_level=*/true); - PROTOBUF_DLOG(INFO) << "json2/status: " << s; + if (PROTOBUF_DEBUG) ABSL_DLOG(INFO) << "json2/status: " << s; RETURN_IF_ERROR(s); writer.NewLine(); - PROTOBUF_DLOG(INFO) << "json2/output: " << absl::CHexEscape(*output); + if (PROTOBUF_DEBUG) { + ABSL_DLOG(INFO) << "json2/output: " << absl::CHexEscape(*output); + } return absl::OkStatus(); } @@ -894,10 +899,9 @@ absl::Status BinaryToJsonStream(google::protobuf::util::TypeResolver* resolver, } tee_input.emplace(copy.data(), copy.size()); tee_output.emplace(&out); + ABSL_DLOG(INFO) << "json2/input: " << absl::BytesToHexString(copy); } - PROTOBUF_DLOG(INFO) << "json2/input: " << absl::BytesToHexString(copy); - ResolverPool pool(resolver); auto desc = pool.FindMessage(type_url); RETURN_IF_ERROR(desc.status()); @@ -912,17 +916,16 @@ absl::Status BinaryToJsonStream(google::protobuf::util::TypeResolver* resolver, absl::Status s = WriteMessage( writer, *msg, UnparseProto3Type::GetDesc(*msg), /*is_top_level=*/true); - PROTOBUF_DLOG(INFO) << "json2/status: " << s; + if (PROTOBUF_DEBUG) ABSL_DLOG(INFO) << "json2/status: " << s; RETURN_IF_ERROR(s); if (PROTOBUF_DEBUG) { tee_output.reset(); // Flush the output stream. io::zc_sink_internal::ZeroCopyStreamByteSink(json_output) .Append(out.data(), out.size()); + ABSL_DLOG(INFO) << "json2/output: " << absl::CHexEscape(out); } - PROTOBUF_DLOG(INFO) << "json2/output: " << absl::CHexEscape(out); - writer.NewLine(); return absl::OkStatus(); } diff --git a/src/google/protobuf/port.cc b/src/google/protobuf/port.cc new file mode 100644 index 0000000000..eb167ddcd8 --- /dev/null +++ b/src/google/protobuf/port.cc @@ -0,0 +1,52 @@ +// Protocol Buffers - Google's data interchange format +// Copyright 2023 Google Inc. All rights reserved. +// https://developers.google.com/protocol-buffers/ +// +// Redistribution and use in source and binary forms, with or without +// modification, are permitted provided that the following conditions are +// met: +// +// * Redistributions of source code must retain the above copyright +// notice, this list of conditions and the following disclaimer. +// * Redistributions in binary form must reproduce the above +// copyright notice, this list of conditions and the following disclaimer +// in the documentation and/or other materials provided with the +// distribution. +// * Neither the name of Google Inc. nor the names of its +// contributors may be used to endorse or promote products derived from +// this software without specific prior written permission. +// +// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS +// "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT +// LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR +// A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT +// OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, +// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT +// LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, +// DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY +// THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT +// (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE +// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. +// +#include +#include + +// Must be included last +#include "google/protobuf/port_def.inc" + +namespace google { +namespace protobuf { +namespace internal { + +// protobuf_assumption_failed() is declared and used in port_def.inc to assert +// PROTOBUF_ASSUME conditions in debug mode. This function avoids having +// port_def.inc depend on assert.h or other headers, minimizing the compilation +// footprint. +void protobuf_assumption_failed(const char* pred, const char* file, int line) { + fprintf(stderr, "%s: %d: Assumption failed: '%s'\n", file, line, pred); + abort(); +} + +} // namespace internal +} // namespace protobuf +} // namespace google diff --git a/src/google/protobuf/port_def.inc b/src/google/protobuf/port_def.inc index 7e0310e01f..57a752e134 100644 --- a/src/google/protobuf/port_def.inc +++ b/src/google/protobuf/port_def.inc @@ -621,21 +621,6 @@ static_assert(PROTOBUF_CPLUSPLUS_MIN(201402L), "Protobuf only supports C++14 and #define PROTOBUF_FALLTHROUGH_INTENDED #endif -// PROTOBUF_ASSUME(pred) tells the compiler that it can assume pred is true. To -// be safe, we also validate the assumption with a ABSL_DCHECK in unoptimized -// builds. The macro does not do anything useful if the compiler does not -// support __builtin_assume. -#ifdef PROTOBUF_ASSUME -#error PROTOBUF_ASSUME was previously defined -#endif -#if __has_builtin(__builtin_assume) -#define PROTOBUF_ASSUME(pred) \ - ABSL_DCHECK(pred); \ - __builtin_assume(pred) -#else -#define PROTOBUF_ASSUME(pred) ABSL_DCHECK(pred) -#endif - // Specify memory alignment for structs, classes, etc. // Use like: // class PROTOBUF_ALIGNAS(16) MyClass { ... } @@ -1075,21 +1060,54 @@ static_assert(PROTOBUF_CPLUSPLUS_MIN(201402L), "Protobuf only supports C++14 and #define PROTOBUF_DEBUG false #endif -// This `for` allows us to condition the `ABSL_LOG` on the define above, so that -// code can write `PROTOBUF_DLOG(INFO) << ...;` and have it turned off when -// debug logging is off. -// -// This is a `for`, not and `if`, to avoid it accidentally chaining with an -// `else` below it. -#define PROTOBUF_DLOG(x) \ - for (bool b = PROTOBUF_DEBUG; b; b = false) ABSL_LOG(x) - #define PROTO2_IS_OSS true #ifdef PROTOBUF_NO_THREADLOCAL #error PROTOBUF_NO_THREADLOCAL was previously defined #endif +// port_def.inc may be included in very large compilation targets, so we need to +// minimize adding symbol and source file information here. For this reason we +// implement our own simple `protobuf_assumption_failed()` function for +// asserting PROTOBUF_ASSUME predicates in debug builds. +namespace google { +namespace protobuf { +namespace internal { +PROTOBUF_EXPORT void protobuf_assumption_failed(const char *pred, + const char *file, int line); +} // namespace internal +} // namespace protobuf +} // namespace google + +// PROTOBUF_ASSUME(pred) tells the compiler that it can assume pred is true. +// To be safe, we also validate the assumption in debug builds, printing an +// assert style "Assumption failed: ..." message and aborting the program if +// the predicate is false. The macro does not do anything useful if the +// compiler does not support __builtin_assume. +#ifdef PROTOBUF_ASSUME +#error PROTOBUF_ASSUME was previously defined +#endif +#if __has_builtin(__builtin_assume) +#ifdef NDEBUG +#define PROTOBUF_ASSUME(pred) __builtin_assume(pred) +#else // NDEBUG +#define PROTOBUF_ASSUME(pred) \ + if (!(pred)) { \ + ::google::protobuf::internal::protobuf_assumption_failed(#pred, __FILE__, __LINE__); \ + } \ + __builtin_assume(pred) +#endif // NDEBUG +#else // has_builtin(__builtin_assume) +#ifndef NDEBUG +#define PROTOBUF_ASSUME(pred) \ + if (!(pred)) { \ + ::google::protobuf::internal::protobuf_assumption_failed(#pred, __FILE__, __LINE__); \ + } +#else // !NDEBUG +#define PROTOBUF_ASSUME(pred) +#endif // !NDEBUG +#endif // has_builtin(__builtin_assume) + // We don't want code outside port_def doing complex testing, so // remove our portable condition test macros to nudge folks away from // using it themselves. diff --git a/src/google/protobuf/port_test.cc b/src/google/protobuf/port_test.cc new file mode 100644 index 0000000000..798ff4849d --- /dev/null +++ b/src/google/protobuf/port_test.cc @@ -0,0 +1,56 @@ +// Protocol Buffers - Google's data interchange format +// Copyright 2023 Google Inc. All rights reserved. +// https://developers.google.com/protocol-buffers/ +// +// Redistribution and use in source and binary forms, with or without +// modification, are permitted provided that the following conditions are +// met: +// +// * Redistributions of source code must retain the above copyright +// notice, this list of conditions and the following disclaimer. +// * Redistributions in binary form must reproduce the above +// copyright notice, this list of conditions and the following disclaimer +// in the documentation and/or other materials provided with the +// distribution. +// * Neither the name of Google Inc. nor the names of its +// contributors may be used to endorse or promote products derived from +// this software without specific prior written permission. +// +// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS +// "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT +// LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR +// A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT +// OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, +// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT +// LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, +// DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY +// THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT +// (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE +// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. +// +#include +#include + +#include + +// Must be included last +#include "google/protobuf/port_def.inc" + +namespace google { +namespace protobuf { +namespace internal { + +int assume_var_for_test = 1; + +TEST(PortTest, ProtobufAssume) { + PROTOBUF_ASSUME(assume_var_for_test == 1); +#ifdef GTEST_HAS_DEATH_TEST + EXPECT_DEBUG_DEATH( + PROTOBUF_ASSUME(assume_var_for_test == 2), + "port_test\\.cc:.*Assumption failed: 'assume_var_for_test == 2'"); +#endif +} + +} // namespace internal +} // namespace protobuf +} // namespace google diff --git a/src/google/protobuf/port_undef.inc b/src/google/protobuf/port_undef.inc index 2be01b1480..6455512569 100644 --- a/src/google/protobuf/port_undef.inc +++ b/src/google/protobuf/port_undef.inc @@ -113,7 +113,6 @@ #undef PROTOBUF_NO_THREAD_SAFETY_ANALYSIS #undef PROTOBUF_GUARDED_BY #undef PROTOBUF_DEBUG -#undef PROTOBUF_DLOG #undef PROTO2_IS_OSS #undef PROTOBUF_NO_THREADLOCAL