From bdab59a8613c732df5d746328ff2268be8da13ab Mon Sep 17 00:00:00 2001 From: Spencer Low Date: Tue, 11 Aug 2015 16:00:13 -0700 Subject: libbase: logging fixes Win32: - getprogname(): call basename() which is available in mingw's crt. Don't potentially go recursive with DCHECK_GT(). - Use Win32 critical section instead of mutex. Other: - Change log_characters check to compile-time. - Fix code that gets the basename of __FILE__. The previous code was not setting _file, so it didn't work. - Save and restore errno for LOG calls. Inspired by similar Chromium code. Change-Id: Ie7bb700918be726fa81d60177d1894d2daeff296 Signed-off-by: Spencer Low --- base/logging.cpp | 62 +++++++++++++++++++++++++++++++++++---------------- base/logging_test.cpp | 16 ++++++------- 2 files changed, 51 insertions(+), 27 deletions(-) (limited to 'base') diff --git a/base/logging.cpp b/base/logging.cpp index 34229a2f8..e9e06dfc4 100644 --- a/base/logging.cpp +++ b/base/logging.cpp @@ -70,9 +70,14 @@ const char* getprogname() { static char progname[MAX_PATH] = {}; if (first) { - // TODO(danalbert): This is a full path on Windows. Just get the basename. - DWORD nchars = GetModuleFileName(nullptr, progname, sizeof(progname)); - DCHECK_GT(nchars, 0U); + CHAR longname[MAX_PATH]; + DWORD nchars = GetModuleFileNameA(nullptr, longname, arraysize(longname)); + if ((nchars >= arraysize(longname)) || (nchars == 0)) { + // String truncation or some other error. + strcpy(progname, ""); + } else { + strcpy(progname, basename(longname)); + } first = false; } @@ -82,25 +87,22 @@ const char* getprogname() { class mutex { public: mutex() { - semaphore_ = CreateSemaphore(nullptr, 1, 1, nullptr); - CHECK(semaphore_ != nullptr) << "Failed to create Mutex"; + InitializeCriticalSection(&critical_section_); } ~mutex() { - CloseHandle(semaphore_); + DeleteCriticalSection(&critical_section_); } void lock() { - DWORD result = WaitForSingleObject(semaphore_, INFINITE); - CHECK_EQ(result, WAIT_OBJECT_0) << GetLastError(); + EnterCriticalSection(&critical_section_); } void unlock() { - bool result = ReleaseSemaphore(semaphore_, 1, nullptr); - CHECK(result); + LeaveCriticalSection(&critical_section_); } private: - HANDLE semaphore_; + CRITICAL_SECTION critical_section_; }; template @@ -147,8 +149,9 @@ static const char* ProgramInvocationName() { void StderrLogger(LogId, LogSeverity severity, const char*, const char* file, unsigned int line, const char* message) { - static const char* log_characters = "VDIWEF"; - CHECK_EQ(strlen(log_characters), FATAL + 1U); + static const char log_characters[] = "VDIWEF"; + static_assert(arraysize(log_characters) - 1 == FATAL + 1, + "Mismatch in size of log_characters and values in LogSeverity"); char severity_char = log_characters[severity]; fprintf(stderr, "%s %c %5d %5d %s:%u] %s\n", ProgramInvocationName(), severity_char, getpid(), gettid(), file, line, message); @@ -197,6 +200,20 @@ void InitLogging(char* argv[], LogFunction&& logger) { InitLogging(argv); } +// TODO: make this public; it's independently useful. +class ErrnoRestorer { + public: + ErrnoRestorer(int saved_errno) : saved_errno_(saved_errno) { + } + + ~ErrnoRestorer() { + errno = saved_errno_; + } + + private: + const int saved_errno_; +}; + void InitLogging(char* argv[]) { if (gInitialized) { return; @@ -257,19 +274,25 @@ void SetLogger(LogFunction&& logger) { gLogger = std::move(logger); } +// We can't use basename(3) because this code runs on the Mac, which doesn't +// have a non-modifying basename. +static const char* GetFileBasename(const char* file) { + const char* last_slash = strrchr(file, '/'); + return (last_slash == nullptr) ? file : last_slash + 1; +} + // This indirection greatly reduces the stack impact of having lots of // checks/logging in a function. class LogMessageData { public: LogMessageData(const char* file, unsigned int line, LogId id, - LogSeverity severity, int error) - : file_(file), + LogSeverity severity, int error, int saved_errno) + : file_(GetFileBasename(file)), line_number_(line), id_(id), severity_(severity), - error_(error) { - const char* last_slash = strrchr(file, '/'); - file = (last_slash == nullptr) ? file : last_slash + 1; + error_(error), + errno_restorer_(saved_errno) { } const char* GetFile() const { @@ -307,13 +330,14 @@ class LogMessageData { const LogId id_; const LogSeverity severity_; const int error_; + ErrnoRestorer errno_restorer_; DISALLOW_COPY_AND_ASSIGN(LogMessageData); }; LogMessage::LogMessage(const char* file, unsigned int line, LogId id, LogSeverity severity, int error) - : data_(new LogMessageData(file, line, id, severity, error)) { + : data_(new LogMessageData(file, line, id, severity, error, errno)) { } LogMessage::~LogMessage() { diff --git a/base/logging_test.cpp b/base/logging_test.cpp index 1a92c9451..a1e31a632 100644 --- a/base/logging_test.cpp +++ b/base/logging_test.cpp @@ -76,10 +76,10 @@ std::string make_log_pattern(android::base::LogSeverity severity, const char* message) { static const char* log_characters = "VDIWEF"; char log_char = log_characters[severity]; + std::string holder(__FILE__); return android::base::StringPrintf( - "%c[[:space:]]+[[:digit:]]+[[:space:]]+[[:digit:]]+ " __FILE__ - ":[[:digit:]]+] %s", - log_char, message); + "%c[[:space:]]+[[:digit:]]+[[:space:]]+[[:digit:]]+ %s:[[:digit:]]+] %s", + log_char, basename(&holder[0]), message); } TEST(logging, LOG) { @@ -100,7 +100,7 @@ TEST(logging, LOG) { #if !defined(_WIN32) std::regex message_regex( make_log_pattern(android::base::WARNING, "foobar")); - ASSERT_TRUE(std::regex_search(output, message_regex)); + ASSERT_TRUE(std::regex_search(output, message_regex)) << output; #endif } @@ -116,7 +116,7 @@ TEST(logging, LOG) { #if !defined(_WIN32) std::regex message_regex( make_log_pattern(android::base::INFO, "foobar")); - ASSERT_TRUE(std::regex_search(output, message_regex)); + ASSERT_TRUE(std::regex_search(output, message_regex)) << output; #endif } @@ -143,7 +143,7 @@ TEST(logging, LOG) { #if !defined(_WIN32) std::regex message_regex( make_log_pattern(android::base::DEBUG, "foobar")); - ASSERT_TRUE(std::regex_search(output, message_regex)); + ASSERT_TRUE(std::regex_search(output, message_regex)) << output; #endif } } @@ -162,7 +162,7 @@ TEST(logging, PLOG) { #if !defined(_WIN32) std::regex message_regex(make_log_pattern( android::base::INFO, "foobar: No such file or directory")); - ASSERT_TRUE(std::regex_search(output, message_regex)); + ASSERT_TRUE(std::regex_search(output, message_regex)) << output; #endif } } @@ -183,7 +183,7 @@ TEST(logging, UNIMPLEMENTED) { android::base::StringPrintf("%s unimplemented ", __PRETTY_FUNCTION__); std::regex message_regex( make_log_pattern(android::base::ERROR, expected_message.c_str())); - ASSERT_TRUE(std::regex_search(output, message_regex)); + ASSERT_TRUE(std::regex_search(output, message_regex)) << output; #endif } } -- cgit v1.2.3