[Lldb-commits] [PATCH] D128557: [lldb] Add a log dump command to dump the circular log buffer

[Greg Clayton via Phabricator via lldb-commits]

clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.

So we either need to track categories for each buffered log message in the circular buffer so that if the user specifies a category within a channel to dump with "log dump lldb <category>" we can filter out the messages we dump if they are not in the one or more categories that were supplied, or remove categories from the "log dump" command and only specify the channel. Easy to fix the code to work either way.

The idea is if the user does:

  (lldb) log enable -b 5 -h circular lldb command state process
  (lldb) ...
  (lldb log dump lldb process

I would expect only "process" category messages to be shown. The current code allows the user to specify categories, but they don't do anything. So we should either obey the categories correctly by changing "LogHandler::Emit(StringRef message)" to take an extra category mask like "LogHandler::Emit(StringRef message, uint64_t category)" so that we can cache a vector of "std::pair<std::string, uint64_t>" so we can emit only the requested categories when using "log dump", or just remove the categories from the "log dump" command.



================
Comment at: lldb/source/Commands/CommandObjectLog.cpp:359
+    CommandArgumentData channel_arg;
+    CommandArgumentData category_arg;
+
----------------
If we aren't going to correctly track what category that each log channel comes from, then specifying the categories doesn't do anything and the categories shouldn't be part of this command unless you want to fix that. We could easily do this if we modify LogHandler::Emit(...) to take a category integer and then our buffer would need to store category + message in an array.


================
Comment at: lldb/source/Utility/Log.cpp:239
+bool Log::DumpLogChannel(llvm::StringRef channel,
+                         llvm::ArrayRef<const char *> categories,
+                         llvm::raw_ostream &output_stream,
----------------
We don't need categories here. We have no way to separate each log message since we don't store this, and the user shouldn't be required to know the categories right?


================
Comment at: lldb/test/API/commands/log/basic/TestLogHandlers.py:20
+
+        self.runCmd("log enable -b 5 -h circular lldb commands")
+        self.runCmd("bogus", check=False)
----------------
Do we want to test that non circular buffers can't be dumped too to make sure nothing goes wrong or that we get a good error message stating that this log channel isn't circular?


================
Comment at: lldb/test/API/commands/log/basic/TestLogHandlers.py:22
+        self.runCmd("bogus", check=False)
+        self.runCmd("log dump lldb commands -f {}".format(self.log_file))
+
----------------
It seems wrong to specify the categories in the "log dump" command since we won't do the right thing. If you can specified categories I would expect this to work:
```
(lldb) log enable -b 5 -h circular lldb command state process
(lldb) ...
(lldb) log dump lldb process
```
And that only process log lines would come out. Since we don't store this information, we shouldn't have to specify the categories at all, so the "log dump" command should just take the channel:
```
(lldb) log dump lldb
```



CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D128557/new/

https://reviews.llvm.org/D128557