[Lldb-commits] [PATCH] D29615: Convert Log class to llvm streams

[Pavel Labath via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL294736: Convert Log class to llvm streams (authored by 
labath).

Changed prior to commit:
  https://reviews.llvm.org/D29615?vs=87349=87978#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D29615

Files:
  lldb/trunk/include/lldb/Core/Debugger.h
  lldb/trunk/include/lldb/Core/Log.h
  lldb/trunk/include/lldb/Core/Logging.h
  lldb/trunk/include/lldb/Core/StreamCallback.h
  lldb/trunk/source/Core/Debugger.cpp
  lldb/trunk/source/Core/Log.cpp
  lldb/trunk/source/Core/Logging.cpp
  lldb/trunk/source/Core/StreamCallback.cpp
  lldb/trunk/source/Plugins/Process/POSIX/ProcessPOSIXLog.cpp
  lldb/trunk/source/Plugins/Process/POSIX/ProcessPOSIXLog.h
  lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemoteLog.cpp
  lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemoteLog.h
  lldb/trunk/source/Plugins/SymbolFile/DWARF/LogChannelDWARF.cpp
  lldb/trunk/source/Plugins/SymbolFile/DWARF/LogChannelDWARF.h
  lldb/trunk/tools/lldb-server/LLDBServerUtilities.cpp
  lldb/trunk/unittests/Core/CMakeLists.txt
  lldb/trunk/unittests/Core/LogTest.cpp
  lldb/trunk/unittests/Core/StreamCallbackTest.cpp

Index: lldb/trunk/unittests/Core/CMakeLists.txt
===
--- lldb/trunk/unittests/Core/CMakeLists.txt
+++ lldb/trunk/unittests/Core/CMakeLists.txt
@@ -7,6 +7,7 @@
   LogTest.cpp
   ScalarTest.cpp
   StateTest.cpp
+  StreamCallbackTest.cpp
   StructuredDataTest.cpp
   TimerTest.cpp
 
Index: lldb/trunk/unittests/Core/StreamCallbackTest.cpp
===
--- lldb/trunk/unittests/Core/StreamCallbackTest.cpp
+++ lldb/trunk/unittests/Core/StreamCallbackTest.cpp
@@ -0,0 +1,28 @@
+//===-- StreamCallbackTest.cpp --*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "lldb/Core/StreamCallback.h"
+#include "gtest/gtest.h"
+
+using namespace lldb;
+using namespace lldb_private;
+
+static char test_baton;
+static size_t callback_count = 0;
+static void TestCallback(const char *data, void *baton) {
+  EXPECT_STREQ("Foobar", data);
+  EXPECT_EQ(_baton, baton);
+  ++callback_count;
+}
+
+TEST(StreamCallbackTest, Callback) {
+  StreamCallback stream(TestCallback, _baton);
+  stream << "Foobar";
+  EXPECT_EQ(1u, callback_count);
+}
Index: lldb/trunk/unittests/Core/LogTest.cpp
===
--- lldb/trunk/unittests/Core/LogTest.cpp
+++ lldb/trunk/unittests/Core/LogTest.cpp
@@ -18,12 +18,14 @@
 
 static std::string GetLogString(uint32_t log_options, const char *format,
 int arg) {
-  std::shared_ptr stream_sp(new StreamString());
+  std::string stream_string;
+  std::shared_ptr stream_sp(
+  new llvm::raw_string_ostream(stream_string));
   Log log_(stream_sp);
   log_.GetOptions().Reset(log_options);
   Log *log = _;
   LLDB_LOG(log, format, arg);
-  return stream_sp->GetString();
+  return stream_sp->str();
 }
 
 TEST(LogTest, LLDB_LOG_nullptr) {
Index: lldb/trunk/tools/lldb-server/LLDBServerUtilities.cpp
===
--- lldb/trunk/tools/lldb-server/LLDBServerUtilities.cpp
+++ lldb/trunk/tools/lldb-server/LLDBServerUtilities.cpp
@@ -16,25 +16,32 @@
 
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/Support/FileSystem.h"
 
 using namespace lldb;
 using namespace lldb_private::lldb_server;
 using namespace llvm;
 
+static std::shared_ptr GetLogStream(StringRef log_file) {
+  if (!log_file.empty()) {
+std::error_code EC;
+std::shared_ptr stream_sp = std::make_shared(
+log_file, EC, sys::fs::F_Text | sys::fs::F_Append);
+if (!EC)
+  return stream_sp;
+errs() << llvm::formatv(
+"Failed to open log file `{0}`: {1}\nWill log to stderr instead.\n",
+log_file, EC.message());
+  }
+  // No need to delete the stderr stream.
+  return std::shared_ptr((), [](raw_ostream *) {});
+}
+
 bool LLDBServerUtilities::SetupLogging(const std::string _file,
const StringRef _channels,
uint32_t log_options) {
-  lldb::StreamSP log_stream_sp;
-  if (log_file.empty()) {
-log_stream_sp.reset(new StreamFile(stdout, false));
-  } else {
-uint32_t options = File::eOpenOptionWrite | File::eOpenOptionCanCreate |
-   File::eOpenOptionCloseOnExec | File::eOpenOptionAppend;
-if (!(log_options & LLDB_LOG_OPTION_APPEND))
-  options |= File::eOpenOptionTruncate;
 
-log_stream_sp.reset(new StreamFile(log_file.c_str(), options));
-  }
+  auto log_stream_sp = 

[Lldb-commits] [PATCH] D29615: Convert Log class to llvm streams

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Thank you for the review. I'll submit this tomorrow.

In https://reviews.llvm.org/D29615#670879, @clayborg wrote:

> Looks good as long as if we type two log enable commands like:
>
>   (lldb) log enable -f /tmp/a.txt lldb process
>   (lldb) log enable -f /tmp/a.txt lldb api
>


Yes, the log file still gets shared. I am not changing that.

> @zturner:
>  Aside from the thread stuff, nothing seems particularly risky about this 
> change.  Did the thread-local stuff you removed have anything to do with the 
> -t option to the log command?

Well... the thread-safe logging would make that unnecessary, but with the 
current temporary-buffer implementation you don't need that even without 
threadsafe logging. This makes be believe the logging callback was written 
before we had the temporary buffer logic in place, but I haven't dug through 
the history to verify that.


https://reviews.llvm.org/D29615



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D29615: Convert Log class to llvm streams

[Greg Clayton via Phabricator via lldb-commits]

clayborg accepted this revision.
clayborg added a comment.
This revision is now accepted and ready to land.

Looks good as long as if we type two log enable commands like:

  (lldb) log enable -f /tmp/a.txt lldb process
  (lldb) log enable -f /tmp/a.txt lldb api

share the same log stream (we don't open /tmp/a.txt twice) if is still open and 
available.




Comment at: source/Core/StreamCallback.cpp:22
 StreamCallback::StreamCallback(lldb::LogOutputCallback callback, void *baton)
-: Stream(0, 4, eByteOrderBig), m_callback(callback), m_baton(baton),
-  m_accumulated_data(), m_collection_mutex() {}

labath wrote:
> zturner wrote:
> > I find it rather odd that this was hardcoding big endian.  Was the 
> > endianness here important for some reason?
> I think that was there just because you needed to specify some value. As we 
> were always printing strings, it did not matter anyway.
Pavel is correct, this is because streams can be put into binary mode and that 
you can use Stream::Put32(uint32_t) which will write a number if in non-binary 
mode, or endian correct bytes if in binary mode. Logs didn't ever use the 
binary feature so we just set it to defaults.


https://reviews.llvm.org/D29615



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D29615: Convert Log class to llvm streams

[Zachary Turner via lldb-commits]

Aside from the thread stuff, nothing seems particularly risky about this
change.  Did the thread-local stuff you removed have anything to do with
the -t option to the log command?

On Wed, Feb 8, 2017 at 9:15 AM Pavel Labath via Phabricator <
revi...@reviews.llvm.org> wrote:

> labath added a comment.
>
> Ping. Any thoughts on this?
>
>
> https://reviews.llvm.org/D29615
>
>
>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D29615: Convert Log class to llvm streams

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Ping. Any thoughts on this?


https://reviews.llvm.org/D29615



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D29615: Convert Log class to llvm streams

[Jim Ingham via lldb-commits]

> On Feb 6, 2017, at 7:19 PM, Pavel Labath via Phabricator via lldb-commits 
>  wrote:
> 
> labath added a comment.
> 
> The log callback gets passed down from the SB API (in the SBDebugger 
> constructor, no less). My best guess is that it is (was?) used to display the 
> debugger log in some IDE window.

That's a pretty good guess.  It is used so that the logs from Xcode and the 
logs from lldb get interleaved and written to one place.  It's quite handy in 
some circumstances to see Xcode say "I was trying to do X" then lldb say "I was 
told to do X and did it this way."  Plus it means there's an easy way to funnel 
the whole log to one place.

It has to get passed in when you construct the debugger otherwise you stand to 
lose some log information.  We don't put it in a window, there's not much 
information in there that an ordinary user could understand, it would just be 
noise.  But we do tell people to generate appropriate logs and send them to us.

Jim


> 
> 
> 
> 
> Comment at: source/Core/StreamCallback.cpp:22
> StreamCallback::StreamCallback(lldb::LogOutputCallback callback, void *baton)
> -: Stream(0, 4, eByteOrderBig), m_callback(callback), m_baton(baton),
> -  m_accumulated_data(), m_collection_mutex() {}
> 
> zturner wrote:
>> I find it rather odd that this was hardcoding big endian.  Was the 
>> endianness here important for some reason?
> I think that was there just because you needed to specify some value. As we 
> were always printing strings, it did not matter anyway.
> 
> 
> https://reviews.llvm.org/D29615
> 
> 
> 
> ___
> lldb-commits mailing list
> lldb-commits@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D29615: Convert Log class to llvm streams

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

The log callback gets passed down from the SB API (in the SBDebugger 
constructor, no less). My best guess is that it is (was?) used to display the 
debugger log in some IDE window.




Comment at: source/Core/StreamCallback.cpp:22
 StreamCallback::StreamCallback(lldb::LogOutputCallback callback, void *baton)
-: Stream(0, 4, eByteOrderBig), m_callback(callback), m_baton(baton),
-  m_accumulated_data(), m_collection_mutex() {}

zturner wrote:
> I find it rather odd that this was hardcoding big endian.  Was the endianness 
> here important for some reason?
I think that was there just because you needed to specify some value. As we 
were always printing strings, it did not matter anyway.


https://reviews.llvm.org/D29615



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D29615: Convert Log class to llvm streams

[Zachary Turner via Phabricator via lldb-commits]

zturner added a comment.

Where is the logging callback used?  It seems rather odd that we would need to 
notify someone every time a log message was generated.




Comment at: source/Core/StreamCallback.cpp:22
 StreamCallback::StreamCallback(lldb::LogOutputCallback callback, void *baton)
-: Stream(0, 4, eByteOrderBig), m_callback(callback), m_baton(baton),
-  m_accumulated_data(), m_collection_mutex() {}

I find it rather odd that this was hardcoding big endian.  Was the endianness 
here important for some reason?


https://reviews.llvm.org/D29615



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D29615: Convert Log class to llvm streams

[Pavel Labath via Phabricator via lldb-commits]

labath created this revision.
Herald added subscribers: mgorny, emaste.

This converts LLDB's logging to use llvm streams instead of
lldb_private::Stream and friends. The changes are mostly
straight-forward and amount to s/lldb_private::Stream/llvm::raw_ostream.

The part worth calling out is the rewrite of the StreamCallback class.
Previously this class contained a per-thread buffer of data written. I
assume this had something to do with it trying to make sure each log
line is delivered as a single event, instead of multiple (possibly
interleaved) events. However, this is no longer relevant as the Log
class already writes things to a temporary buffer and then delivers the
message as a single "write", so I have just removed the code in
question.


https://reviews.llvm.org/D29615

Files:
  include/lldb/Core/Debugger.h
  include/lldb/Core/Log.h
  include/lldb/Core/Logging.h
  include/lldb/Core/StreamCallback.h
  source/Core/Debugger.cpp
  source/Core/Log.cpp
  source/Core/Logging.cpp
  source/Core/StreamCallback.cpp
  source/Plugins/Process/POSIX/ProcessPOSIXLog.cpp
  source/Plugins/Process/POSIX/ProcessPOSIXLog.h
  source/Plugins/Process/gdb-remote/ProcessGDBRemoteLog.cpp
  source/Plugins/Process/gdb-remote/ProcessGDBRemoteLog.h
  source/Plugins/SymbolFile/DWARF/LogChannelDWARF.cpp
  source/Plugins/SymbolFile/DWARF/LogChannelDWARF.h
  tools/lldb-server/LLDBServerUtilities.cpp
  unittests/Core/CMakeLists.txt
  unittests/Core/LogTest.cpp
  unittests/Core/StreamCallbackTest.cpp

Index: unittests/Core/StreamCallbackTest.cpp
===
--- /dev/null
+++ unittests/Core/StreamCallbackTest.cpp
@@ -0,0 +1,28 @@
+//===-- StreamCallbackTest.cpp --*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "lldb/Core/StreamCallback.h"
+#include "gtest/gtest.h"
+
+using namespace lldb;
+using namespace lldb_private;
+
+static char test_baton;
+static size_t callback_count = 0;
+static void TestCallback(const char *data, void *baton) {
+  EXPECT_STREQ("Foobar", data);
+  EXPECT_EQ(_baton, baton);
+  ++callback_count;
+}
+
+TEST(StreamCallbackTest, Callback) {
+  StreamCallback stream(TestCallback, _baton);
+  stream << "Foobar";
+  EXPECT_EQ(1u, callback_count);
+}
Index: unittests/Core/LogTest.cpp
===
--- unittests/Core/LogTest.cpp
+++ unittests/Core/LogTest.cpp
@@ -18,12 +18,14 @@
 
 static std::string GetLogString(uint32_t log_options, const char *format,
 int arg) {
-  std::shared_ptr stream_sp(new StreamString());
+  std::string stream_string;
+  std::shared_ptr stream_sp(
+  new llvm::raw_string_ostream(stream_string));
   Log log_(stream_sp);
   log_.GetOptions().Reset(log_options);
   Log *log = _;
   LLDB_LOG(log, format, arg);
-  return stream_sp->GetString();
+  return stream_sp->str();
 }
 
 TEST(LogTest, LLDB_LOG_nullptr) {
Index: unittests/Core/CMakeLists.txt
===
--- unittests/Core/CMakeLists.txt
+++ unittests/Core/CMakeLists.txt
@@ -7,6 +7,7 @@
   LogTest.cpp
   ScalarTest.cpp
   StateTest.cpp
+  StreamCallbackTest.cpp
   StructuredDataTest.cpp
   TimerTest.cpp
 
Index: tools/lldb-server/LLDBServerUtilities.cpp
===
--- tools/lldb-server/LLDBServerUtilities.cpp
+++ tools/lldb-server/LLDBServerUtilities.cpp
@@ -16,25 +16,32 @@
 
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/Support/FileSystem.h"
 
 using namespace lldb;
 using namespace lldb_private::lldb_server;
 using namespace llvm;
 
+static std::shared_ptr GetLogStream(StringRef log_file) {
+  if (!log_file.empty()) {
+std::error_code EC;
+std::shared_ptr stream_sp = std::make_shared(
+log_file, EC, sys::fs::F_Text | sys::fs::F_Append);
+if (!EC)
+  return stream_sp;
+errs() << llvm::formatv(
+"Failed to open log file `{0}`: {1}\nWill log to stderr instead.\n",
+log_file, EC.message());
+  }
+  // No need to delete the stderr stream.
+  return std::shared_ptr((), [](raw_ostream *) {});
+}
+
 bool LLDBServerUtilities::SetupLogging(const std::string _file,
const StringRef _channels,
uint32_t log_options) {
-  lldb::StreamSP log_stream_sp;
-  if (log_file.empty()) {
-log_stream_sp.reset(new StreamFile(stdout, false));
-  } else {
-uint32_t options = File::eOpenOptionWrite | File::eOpenOptionCanCreate |
-   File::eOpenOptionCloseOnExec | File::eOpenOptionAppend;
-if (!(log_options & LLDB_LOG_OPTION_APPEND))
-  options |=