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 <CompareAndSwap@gmail.com>

2 files changed, 51 insertions, 27 deletions

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, "<unknown>");
+    } 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 <typename LockT>
@@ -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
   }
 }