From ddfd1b2f9d30675198f202586489061b349addd3 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Tue, 21 Jan 2020 18:30:20 +0100 Subject: [PATCH] src: add C++-style sprintf utility Add an utility that handles C++-style strings and objects well. PR-URL: https://github.com/nodejs/node/pull/31446 Fixes: https://github.com/nodejs/node/issues/28761 Fixes: https://github.com/nodejs/node/issues/31218 Reviewed-By: James M Snell Reviewed-By: Gus Caplan Reviewed-By: Ben Noordhuis Reviewed-By: Colin Ihrig Reviewed-By: Rich Trott --- node.gyp | 1 + src/debug_utils-inl.h | 90 +++++++++++++++++++++++++++++++++++++++ src/debug_utils.cc | 2 +- src/debug_utils.h | 14 +++++- src/inspector_io.cc | 2 +- src/inspector_profiler.cc | 2 +- src/node.cc | 2 +- src/node_http2.cc | 6 +-- src/node_messaging.cc | 2 +- src/node_platform.cc | 2 +- src/node_report.cc | 2 +- src/node_wasi.cc | 4 +- src/node_watchdog.cc | 2 +- src/node_worker.cc | 2 +- src/spawn_sync.cc | 2 +- src/tls_wrap.cc | 2 +- src/tracing/agent.cc | 2 +- test/cctest/test_util.cc | 44 +++++++++++++++++++ 18 files changed, 165 insertions(+), 18 deletions(-) create mode 100644 src/debug_utils-inl.h diff --git a/node.gyp b/node.gyp index 9806b165ab78eb..0faea0a32a78ee 100644 --- a/node.gyp +++ b/node.gyp @@ -609,6 +609,7 @@ 'src/connect_wrap.h', 'src/connection_wrap.h', 'src/debug_utils.h', + 'src/debug_utils-inl.h', 'src/env.h', 'src/env-inl.h', 'src/handle_wrap.h', diff --git a/src/debug_utils-inl.h b/src/debug_utils-inl.h new file mode 100644 index 00000000000000..69b49d9c50d2e3 --- /dev/null +++ b/src/debug_utils-inl.h @@ -0,0 +1,90 @@ +#ifndef SRC_DEBUG_UTILS_INL_H_ +#define SRC_DEBUG_UTILS_INL_H_ + +#if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS + +#include "debug_utils.h" + +#include + +namespace node { + +struct ToStringHelper { + template + static std::string Convert( + const T& value, + std::string(T::* to_string)() const = &T::ToString) { + return (value.*to_string)(); + } + template ::value, bool>::type, + typename dummy = bool> + static std::string Convert(const T& value) { return std::to_string(value); } + static std::string Convert(const char* value) { return value; } + static std::string Convert(const std::string& value) { return value; } + static std::string Convert(bool value) { return value ? "true" : "false"; } +}; + +template +std::string ToString(const T& value) { + return ToStringHelper::Convert(value); +} + +inline std::string SPrintFImpl(const char* format) { + const char* p = strchr(format, '%'); + if (LIKELY(p == nullptr)) return format; + CHECK_EQ(p[1], '%'); // Only '%%' allowed when there are no arguments. + + return std::string(format, p + 1) + SPrintFImpl(p + 2); +} + +template +std::string COLD_NOINLINE SPrintFImpl( // NOLINT(runtime/string) + const char* format, Arg&& arg, Args&&... args) { + const char* p = strchr(format, '%'); + CHECK_NOT_NULL(p); // If you hit this, you passed in too many arguments. + std::string ret(format, p); + // Ignore long / size_t modifiers + while (strchr("lz", *++p) != nullptr) {} + switch (*p) { + case '%': { + return ret + '%' + SPrintFImpl(p + 1, + std::forward(arg), + std::forward(args)...); + } + default: { + return ret + '%' + SPrintFImpl(p, + std::forward(arg), + std::forward(args)...); + } + case 'd': + case 'i': + case 'u': + case 's': ret += ToString(arg); break; + case 'p': { + CHECK(std::is_pointer::type>::value); + char out[20]; + int n = snprintf(out, + sizeof(out), + "%p", + *reinterpret_cast(&arg)); + CHECK_GE(n, 0); + ret += out; + break; + } + } + return ret + SPrintFImpl(p + 1, std::forward(args)...); +} + +template +std::string COLD_NOINLINE SPrintF( // NOLINT(runtime/string) + const char* format, Args&&... args) { + return SPrintFImpl(format, std::forward(args)...); +} + +} // namespace node + +#endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS + +#endif // SRC_DEBUG_UTILS_INL_H_ diff --git a/src/debug_utils.cc b/src/debug_utils.cc index 8dd51b3931959b..b41cdd7a3f3f2f 100644 --- a/src/debug_utils.cc +++ b/src/debug_utils.cc @@ -1,4 +1,4 @@ -#include "debug_utils.h" +#include "debug_utils-inl.h" // NOLINT(build/include) #include "env-inl.h" #ifdef __POSIX__ diff --git a/src/debug_utils.h b/src/debug_utils.h index db01cacba6a1b6..08d23bb7703fe6 100644 --- a/src/debug_utils.h +++ b/src/debug_utils.h @@ -22,6 +22,17 @@ namespace node { +template +inline std::string ToString(const T& value); + +// C++-style variant of sprintf() that: +// - Returns an std::string +// - Handles \0 bytes correctly +// - Supports %p and %s. %d, %i and %u are aliases for %s. +// - Accepts any class that has a ToString() method for stringification. +template +inline std::string SPrintF(const char* format, Args&&... args); + template inline void FORCE_INLINE Debug(Environment* env, DebugCategory cat, @@ -29,7 +40,8 @@ inline void FORCE_INLINE Debug(Environment* env, Args&&... args) { if (!UNLIKELY(env->debug_enabled(cat))) return; - fprintf(stderr, format, std::forward(args)...); + std::string out = SPrintF(format, std::forward(args)...); + fwrite(out.data(), out.size(), 1, stderr); } inline void FORCE_INLINE Debug(Environment* env, diff --git a/src/inspector_io.cc b/src/inspector_io.cc index 76e481c9530d95..ed9035136c51db 100644 --- a/src/inspector_io.cc +++ b/src/inspector_io.cc @@ -4,7 +4,7 @@ #include "inspector/main_thread_interface.h" #include "inspector/node_string.h" #include "base_object-inl.h" -#include "debug_utils.h" +#include "debug_utils-inl.h" #include "node.h" #include "node_crypto.h" #include "node_internals.h" diff --git a/src/inspector_profiler.cc b/src/inspector_profiler.cc index e0d02d6952a3f9..f9d3c2b512f1c1 100644 --- a/src/inspector_profiler.cc +++ b/src/inspector_profiler.cc @@ -1,6 +1,6 @@ #include "inspector_profiler.h" #include "base_object-inl.h" -#include "debug_utils.h" +#include "debug_utils-inl.h" #include "diagnosticfilename-inl.h" #include "memory_tracker-inl.h" #include "node_file.h" diff --git a/src/node.cc b/src/node.cc index 5118feed602a93..86bc2d9850f958 100644 --- a/src/node.cc +++ b/src/node.cc @@ -23,7 +23,7 @@ // ========== local headers ========== -#include "debug_utils.h" +#include "debug_utils-inl.h" #include "env-inl.h" #include "memory_tracker-inl.h" #include "node_binding.h" diff --git a/src/node_http2.cc b/src/node_http2.cc index cf83a1c612b5b5..150237709bc31e 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -1,5 +1,5 @@ #include "aliased_buffer.h" -#include "debug_utils.h" +#include "debug_utils-inl.h" #include "memory_tracker-inl.h" #include "node.h" #include "node_buffer.h" @@ -1967,7 +1967,7 @@ std::string Http2Stream::diagnostic_name() const { // Notify the Http2Stream that a new block of HEADERS is being processed. void Http2Stream::StartHeaders(nghttp2_headers_category category) { - Debug(this, "starting headers, category: %d", id_, category); + Debug(this, "starting headers, category: %d", category); CHECK(!this->IsDestroyed()); session_->DecrementCurrentSessionMemory(current_headers_length_); current_headers_length_ = 0; @@ -2220,7 +2220,7 @@ int Http2Stream::DoWrite(WriteWrap* req_wrap, req_wrap->Done(UV_EOF); return 0; } - Debug(this, "queuing %d buffers to send", id_, nbufs); + Debug(this, "queuing %d buffers to send", nbufs); for (size_t i = 0; i < nbufs; ++i) { // Store the req_wrap on the last write info in the queue, so that it is // only marked as finished once all buffers associated with it are finished. diff --git a/src/node_messaging.cc b/src/node_messaging.cc index b0e7d15a8c40a6..b51607cc0ee371 100644 --- a/src/node_messaging.cc +++ b/src/node_messaging.cc @@ -1,7 +1,7 @@ #include "node_messaging.h" #include "async_wrap-inl.h" -#include "debug_utils.h" +#include "debug_utils-inl.h" #include "memory_tracker-inl.h" #include "node_contextify.h" #include "node_buffer.h" diff --git a/src/node_platform.cc b/src/node_platform.cc index a3e39df6ccce96..1858b6b0f1144e 100644 --- a/src/node_platform.cc +++ b/src/node_platform.cc @@ -2,7 +2,7 @@ #include "node_internals.h" #include "env-inl.h" -#include "debug_utils.h" +#include "debug_utils-inl.h" #include // find_if(), find(), move() #include // llround() #include // unique_ptr(), shared_ptr(), make_shared() diff --git a/src/node_report.cc b/src/node_report.cc index ddeb216c82d6bc..3a6cb42556461d 100644 --- a/src/node_report.cc +++ b/src/node_report.cc @@ -1,6 +1,6 @@ #include "env-inl.h" #include "node_report.h" -#include "debug_utils.h" +#include "debug_utils-inl.h" #include "diagnosticfilename-inl.h" #include "node_internals.h" #include "node_metadata.h" diff --git a/src/node_wasi.cc b/src/node_wasi.cc index 1edafe2172cee9..2305a75d031659 100644 --- a/src/node_wasi.cc +++ b/src/node_wasi.cc @@ -1,6 +1,6 @@ #include "env-inl.h" #include "base_object-inl.h" -#include "debug_utils.h" +#include "debug_utils-inl.h" #include "memory_tracker-inl.h" #include "node_mem-inl.h" #include "util-inl.h" @@ -1062,7 +1062,7 @@ void WASI::PathFilestatGet(const FunctionCallbackInfo& args) { CHECK_TO_TYPE_OR_RETURN(args, args[4], Uint32, buf_ptr); ASSIGN_OR_RETURN_UNWRAP(&wasi, args.This()); Debug(wasi, - "path_filestat_get(%d, %d, %d, %d, %d)\n", + "path_filestat_get(%d, %d, %d)\n", fd, path_ptr, path_len); diff --git a/src/node_watchdog.cc b/src/node_watchdog.cc index e0f606c949a009..f11902a1fa44bc 100644 --- a/src/node_watchdog.cc +++ b/src/node_watchdog.cc @@ -21,7 +21,7 @@ #include -#include "debug_utils.h" +#include "debug_utils-inl.h" #include "env-inl.h" #include "node_errors.h" #include "node_internals.h" diff --git a/src/node_worker.cc b/src/node_worker.cc index b89166e9f48ad5..fe427b44410e53 100644 --- a/src/node_worker.cc +++ b/src/node_worker.cc @@ -1,5 +1,5 @@ #include "node_worker.h" -#include "debug_utils.h" +#include "debug_utils-inl.h" #include "memory_tracker-inl.h" #include "node_errors.h" #include "node_buffer.h" diff --git a/src/spawn_sync.cc b/src/spawn_sync.cc index 0751bc21a7eb3d..3b277ad70adb66 100644 --- a/src/spawn_sync.cc +++ b/src/spawn_sync.cc @@ -20,7 +20,7 @@ // USE OR OTHER DEALINGS IN THE SOFTWARE. #include "spawn_sync.h" -#include "debug_utils.h" +#include "debug_utils-inl.h" #include "env-inl.h" #include "node_internals.h" #include "string_bytes.h" diff --git a/src/tls_wrap.cc b/src/tls_wrap.cc index 263505c78d8558..8aab19ea90987d 100644 --- a/src/tls_wrap.cc +++ b/src/tls_wrap.cc @@ -21,7 +21,7 @@ #include "tls_wrap.h" #include "async_wrap-inl.h" -#include "debug_utils.h" +#include "debug_utils-inl.h" #include "memory_tracker-inl.h" #include "node_buffer.h" // Buffer #include "node_crypto.h" // SecureContext diff --git a/src/tracing/agent.cc b/src/tracing/agent.cc index 3d19c6c75844fc..7d265dcb0c4c3b 100644 --- a/src/tracing/agent.cc +++ b/src/tracing/agent.cc @@ -3,7 +3,7 @@ #include #include "trace_event.h" #include "tracing/node_trace_buffer.h" -#include "debug_utils.h" +#include "debug_utils-inl.h" #include "env-inl.h" namespace node { diff --git a/test/cctest/test_util.cc b/test/cctest/test_util.cc index 7a0da1e6185d51..843d16d9f527c6 100644 --- a/test/cctest/test_util.cc +++ b/test/cctest/test_util.cc @@ -1,4 +1,6 @@ #include "util-inl.h" +#include "debug_utils-inl.h" +#include "env-inl.h" #include "gtest/gtest.h" TEST(UtilTest, ListHead) { @@ -250,3 +252,45 @@ TEST(UtilTest, MaybeStackBuffer) { EXPECT_TRUE(buf.IsInvalidated()); } } + +TEST(UtilTest, SPrintF) { + using node::SPrintF; + + // %d, %u and %s all do the same thing. The actual C++ type is used to infer + // the right representation. + EXPECT_EQ(SPrintF("%s", false), "false"); + EXPECT_EQ(SPrintF("%s", true), "true"); + EXPECT_EQ(SPrintF("%d", true), "true"); + EXPECT_EQ(SPrintF("%u", true), "true"); + EXPECT_EQ(SPrintF("%d", 10000000000LL), "10000000000"); + EXPECT_EQ(SPrintF("%d", -10000000000LL), "-10000000000"); + EXPECT_EQ(SPrintF("%u", 10000000000LL), "10000000000"); + EXPECT_EQ(SPrintF("%u", -10000000000LL), "-10000000000"); + EXPECT_EQ(SPrintF("%i", 10), "10"); + EXPECT_EQ(SPrintF("%d", 10), "10"); + + EXPECT_EQ(atof(SPrintF("%s", 0.5).c_str()), 0.5); + EXPECT_EQ(atof(SPrintF("%s", -0.5).c_str()), -0.5); + + void (*fn)() = []() {}; + void* p = reinterpret_cast(&fn); + EXPECT_GE(SPrintF("%p", fn).size(), 4u); + EXPECT_GE(SPrintF("%p", p).size(), 4u); + + const std::string foo = "foo"; + const char* bar = "bar"; + EXPECT_EQ(SPrintF("%s %s", foo, "bar"), "foo bar"); + EXPECT_EQ(SPrintF("%s %s", foo, bar), "foo bar"); + + EXPECT_EQ(SPrintF("[%% %s %%]", foo), "[% foo %]"); + + struct HasToString { + std::string ToString() const { + return "meow"; + } + }; + EXPECT_EQ(SPrintF("%s", HasToString{}), "meow"); + + const std::string with_zero = std::string("a") + '\0' + 'b'; + EXPECT_EQ(SPrintF("%s", with_zero), with_zero); +}