[Lldb-commits] [PATCH] D128477: [trace] Add a flag to the decoder to output the instruction type

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

This categorization seems like it is relevant for any instruction, not just 
instructions found in a trace buffer.  Is there any reason why this isn't just 
an annotation added by the standard instruction printer, that you then turn on 
with the -k flag to thread trace dump (but which might also be useful for the 
regular disassemble command)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128477

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

[Lldb-commits] [PATCH] D128480: [lldb] Replace Host::SystemLog with Debugger::Report{Error, Warning}

[Med Ismail Bennani via Phabricator via lldb-commits]

mib added inline comments.



Comment at: lldb/source/Core/Module.cpp:1187
+
+Debugger::ReportError(std::string(strm.GetString()));
+  }

It's unrelated to this patch, but it looks like 
`Debugger::HandleDiagnosticEvent` dumps everything to the error stream without 
checking if the event is a warning or an error. I'm mentioning that here 
because if we did that distinction at the event handling level, we could get 
rid of `strm.PutCString("error: ")`.

Note however, the reason I'm bringing this up, is that the error message prefix 
is not consistent with `ReportErrorIfModifyDetected`, since it's not prepended 
by `error: `.

We should at least fix that for this patch before before making it consistent 
at the event handler level.


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

https://reviews.llvm.org/D128480

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

[Lldb-commits] [PATCH] D128480: [lldb] Replace Host::SystemLog with Debugger::Report{Error, Warning}

[Med Ismail Bennani via Phabricator via lldb-commits]

mib added inline comments.



Comment at: lldb/source/Core/Module.cpp:1187
+
+Debugger::ReportError(std::string(strm.GetString()));
+  }

mib wrote:
> It's unrelated to this patch, but it looks like 
> `Debugger::HandleDiagnosticEvent` dumps everything to the error stream 
> without checking if the event is a warning or an error. I'm mentioning that 
> here because if we did that distinction at the event handling level, we could 
> get rid of `strm.PutCString("error: ")`.
> 
> Note however, the reason I'm bringing this up, is that the error message 
> prefix is not consistent with `ReportErrorIfModifyDetected`, since it's not 
> prepended by `error: `.
> 
> We should at least fix that for this patch before before making it consistent 
> at the event handler level.
I hadn't looked at the rest of the patch and this looks very inconsistent 
throughout the other files as well ... may be we should just leave that for a 
follow-up patch


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

https://reviews.llvm.org/D128480

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

[Lldb-commits] [PATCH] D128480: [lldb] Replace Host::SystemLog with Debugger::Report{Error, Warning}

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.



Comment at: lldb/source/Core/Module.cpp:1187
+
+Debugger::ReportError(std::string(strm.GetString()));
+  }

mib wrote:
> mib wrote:
> > It's unrelated to this patch, but it looks like 
> > `Debugger::HandleDiagnosticEvent` dumps everything to the error stream 
> > without checking if the event is a warning or an error. I'm mentioning that 
> > here because if we did that distinction at the event handling level, we 
> > could get rid of `strm.PutCString("error: ")`.
> > 
> > Note however, the reason I'm bringing this up, is that the error message 
> > prefix is not consistent with `ReportErrorIfModifyDetected`, since it's not 
> > prepended by `error: `.
> > 
> > We should at least fix that for this patch before before making it 
> > consistent at the event handler level.
> I hadn't looked at the rest of the patch and this looks very inconsistent 
> throughout the other files as well ... may be we should just leave that for a 
> follow-up patch
Yeah this is an oversight. The prefix will get printed when the event gets 
handled (or by the IDE) so it shouldn't be part of the message. I'll fix that 
here. 


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

https://reviews.llvm.org/D128480

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

[Lldb-commits] [PATCH] D128480: [lldb] Replace Host::SystemLog with Debugger::Report{Error, Warning}

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

I don't have anything against this, though I still think the LogHandlers should 
use the Host layer by making the LogHandler objects into real LLDB plug-ins. 
But if everyone else feels otherwise, I won't object.


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

https://reviews.llvm.org/D128480

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

[Lldb-commits] [PATCH] D128321: [lldb] Add OSLog log handler

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

In D128321#3606172 , @clayborg wrote:

> So it seems like we are catering to our code organization here instead of 
> doing the right thing and relying on the host layer. I would rather move the 
> log code into the host layer or make log handlers actual plug-ins so that we 
> can do the right thing here.
>
> If we can make LogHandler objects plug-ins, where each plug-in has a name, 
> then this can be used to specify the different types of logs using "--kind 
> " or "--type ". Then we don't run into these layering issues, 
> because essentially we have small LogHandler implementations here. Any 
> objection to making LogHandler subclasses into actual plug-ins?

That wouldn't really solve my problem though. The plugin would still depend on 
host. And I would still need to depend on the plugin in Utility. So it's still 
a circle: Utility -> Log Handler Plugin -> Host -> Utility


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

https://reviews.llvm.org/D128321

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

[Lldb-commits] [PATCH] D128323: [lldb] Add support for specifying a log handler

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere marked 6 inline comments as done.
JDevlieghere added a subscriber: jingham.
JDevlieghere added inline comments.



Comment at: lldb/source/Commands/CommandObjectLog.cpp:26
 
+static constexpr OptionEnumValueElement g_log_handler_type[] = {
+{

clayborg wrote:
> Question: how does one see these values and their descriptions? Do the 
> descriptions get displayed anywhere? If they do, then my comments below make 
> sense, if the don't, then ignore below comments and maybe roll all that 
> documentation into the main description for this command.
The values are printed in the help output:

```
   -h  ( --log-handler  )
Specify a log handler which determines where log messages are 
written.
Values: default | circular | os
```

The usage/description is not. As far as I can tell, it isn't printed anywhere. 
I talked it over with @jingham and it should get printed in the `help 
` output but currently that's not the case. 

```
(lldb) help 
   -- The log handle that will be used to write out log messages.
```

To make that work we need to tie the actual enum to the ArgumentTableEntry in 
in CommandObject.cpp. I think that's something we should do, but that's outside 
the scope of this patch. 



Comment at: lldb/source/Commands/Options.td:436-437
 Desc<"Set the log to be buffered, using the specified buffer size.">;
+  def log_handler : Option<"handler", "h">, Group<1>,
+EnumArg<"Value", "LogHandlerType()">, Desc<"Use a custom handler.">;
   def log_threadsafe : Option<"threadsafe", "t">, Group<1>,

clayborg wrote:
> JDevlieghere wrote:
> > clayborg wrote:
> > > Maybe "--type" or "--kind" would be better? Handler seems like an 
> > > internal name. Maybe also mention what it will default to if not 
> > > specified? Can the user see a list of the valid values or do we need to 
> > > supply these in the description?
> > I think the concept of a "log handler" is sufficiently well known that it 
> > should be easy for a a (power) user to understand. I'm concerned that 
> > "type" and "kind" are a tad too high level, and therefore could easily be 
> > confused with the log category and channel. For example, when someone asks 
> > to enable the "gdb packet log", is that a category, channel, type or kind? 
> How can the user see the available options? I always have to type an 
> incorrect value in for enums and then I see a correction saying "you can only 
> specify 'a', 'b', or 'c'"
This is part of the help output. 


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

https://reviews.llvm.org/D128323

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

[Lldb-commits] [PATCH] D128323: [lldb] Add support for specifying a log handler

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere updated this revision to Diff 439575.
JDevlieghere marked 2 inline comments as done.

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

https://reviews.llvm.org/D128323

Files:
  lldb/include/lldb/Core/Debugger.h
  lldb/include/lldb/lldb-enumerations.h
  lldb/include/lldb/lldb-private-enumerations.h
  lldb/source/API/SBDebugger.cpp
  lldb/source/Commands/CommandObjectLog.cpp
  lldb/source/Commands/Options.td
  lldb/source/Core/Debugger.cpp
  lldb/source/Interpreter/CommandObject.cpp
  lldb/tools/lldb-test/lldb-test.cpp

Index: lldb/tools/lldb-test/lldb-test.cpp
===
--- lldb/tools/lldb-test/lldb-test.cpp
+++ lldb/tools/lldb-test/lldb-test.cpp
@@ -1120,7 +1120,7 @@
   /*add_to_history*/ eLazyBoolNo, Result);
 
   if (!opts::Log.empty())
-Dbg->EnableLog("lldb", {"all"}, opts::Log, 0, 0, errs());
+Dbg->EnableLog("lldb", {"all"}, opts::Log, 0, 0, eLogHandlerStream, errs());
 
   if (opts::BreakpointSubcommand)
 return opts::breakpoint::evaluateBreakpoints(*Dbg);
Index: lldb/source/Interpreter/CommandObject.cpp
===
--- lldb/source/Interpreter/CommandObject.cpp
+++ lldb/source/Interpreter/CommandObject.cpp
@@ -1126,7 +1126,8 @@
 { eArgTypeCommand, "command", CommandCompletions::eNoCompletion, { nullptr, false }, "An LLDB Command line command element." },
 { eArgTypeColumnNum, "column", CommandCompletions::eNoCompletion, { nullptr, false }, "Column number in a source file." },
 { eArgTypeModuleUUID, "module-uuid", CommandCompletions::eModuleUUIDCompletion, { nullptr, false }, "A module UUID value." },
-{ eArgTypeSaveCoreStyle, "corefile-style", CommandCompletions::eNoCompletion, { nullptr, false }, "The type of corefile that lldb will try to create, dependant on this target's capabilities." }
+{ eArgTypeSaveCoreStyle, "corefile-style", CommandCompletions::eNoCompletion, { nullptr, false }, "The type of corefile that lldb will try to create, dependant on this target's capabilities." },
+{ eArgTypeLogHandler, "log-handler", CommandCompletions::eNoCompletion, { nullptr, false }, "The log handle that will be used to write out log messages." },
 // clang-format on
 };
 
Index: lldb/source/Core/Debugger.cpp
===
--- lldb/source/Core/Debugger.cpp
+++ lldb/source/Core/Debugger.cpp
@@ -1406,11 +1406,27 @@
debugger_id, once);
 }
 
+static std::shared_ptr
+CreateLogHandler(LogHandlerKind log_handler_kind, int fd, bool should_close,
+ size_t buffer_size) {
+  switch (log_handler_kind) {
+  case eLogHandlerStream:
+return std::make_shared(fd, should_close, buffer_size);
+  case eLogHandlerCircular:
+return std::make_shared(buffer_size);
+  case eLogHandlerSystem:
+return std::make_shared();
+  case eLogHandlerCallback:
+return {};
+  }
+  return {};
+}
+
 bool Debugger::EnableLog(llvm::StringRef channel,
  llvm::ArrayRef categories,
  llvm::StringRef log_file, uint32_t log_options,
- size_t buffer_size, llvm::raw_ostream &error_stream) {
-  const bool should_close = true;
+ size_t buffer_size, LogHandlerKind log_handler_kind,
+ llvm::raw_ostream &error_stream) {
 
   std::shared_ptr log_handler_sp;
   if (m_callback_handler_sp) {
@@ -1419,8 +1435,9 @@
 log_options |=
 LLDB_LOG_OPTION_PREPEND_TIMESTAMP | LLDB_LOG_OPTION_PREPEND_THREAD_NAME;
   } else if (log_file.empty()) {
-log_handler_sp = std::make_shared(
-GetOutputFile().GetDescriptor(), !should_close, buffer_size);
+log_handler_sp =
+CreateLogHandler(log_handler_kind, GetOutputFile().GetDescriptor(),
+ /*should_close=*/false, buffer_size);
   } else {
 auto pos = m_stream_handlers.find(log_file);
 if (pos != m_stream_handlers.end())
@@ -1440,8 +1457,9 @@
 return false;
   }
 
-  log_handler_sp = std::make_shared(
-  (*file)->GetDescriptor(), should_close, buffer_size);
+  log_handler_sp =
+  CreateLogHandler(log_handler_kind, (*file)->GetDescriptor(),
+   /*should_close=*/true, buffer_size);
   m_stream_handlers[log_file] = log_handler_sp;
 }
   }
Index: lldb/source/Commands/Options.td
===
--- lldb/source/Commands/Options.td
+++ lldb/source/Commands/Options.td
@@ -431,6 +431,8 @@
 let Command = "log enable" in {
   def log_file : Option<"file", "f">, Group<1>, Arg<"Filename">,
 Desc<"Set the destination file to log to.">;
+  def log_handler : Option<"log-handler", "h">, Group<1>,
+EnumArg<"LogHandler", "LogHandlerType()">, Desc<"Specify a log handler which determines where log messages are written.">;
   def log_buffer_size : Option<"b

[Lldb-commits] [PATCH] D128480: [lldb] Replace Host::SystemLog with Debugger::Report{Error, Warning}

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

In D128480#3606598 , @clayborg wrote:

> I don't have anything against this, though I still think the LogHandlers 
> should use the Host layer by making the LogHandler objects into real LLDB 
> plug-ins. But if everyone else feels otherwise, I won't object.

This patch is meant to be entirely orthogonal to that. I was still going to add 
(what I believe is a better) implementation of SystemLog. In D128321 
 I said:

> But if we really want this to live in Host (and deal with the dependency 
> issues later) I can implement a new Host::SystemLog variant that essentially 
> does what the SystemLogHander is doing and implement the log handler in Host 
> based on that.

So that's what I was alluding to.


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

https://reviews.llvm.org/D128480

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

[Lldb-commits] [PATCH] D128480: [lldb] Replace Host::SystemLog with Debugger::Report{Error, Warning}

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere updated this revision to Diff 439577.
JDevlieghere added a comment.

Remove spurious "error:" strings.


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

https://reviews.llvm.org/D128480

Files:
  lldb/include/lldb/Host/Host.h
  lldb/source/Core/Module.cpp
  lldb/source/Host/common/Host.cpp
  lldb/source/Host/macosx/objcxx/Host.mm
  lldb/source/Interpreter/Options.cpp
  lldb/source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderDarwin.cpp
  lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
  lldb/source/Symbol/CompactUnwindInfo.cpp
  lldb/source/Symbol/DWARFCallFrameInfo.cpp
  lldb/source/Symbol/Function.cpp
  lldb/source/Symbol/SymbolContext.cpp
  lldb/test/API/lang/objc/modules-hash-mismatch/TestClangModulesHashMismatch.py
  lldb/tools/lldb-server/lldb-platform.cpp

Index: lldb/tools/lldb-server/lldb-platform.cpp
===
--- lldb/tools/lldb-server/lldb-platform.cpp
+++ lldb/tools/lldb-server/lldb-platform.cpp
@@ -79,8 +79,7 @@
 // And we should not call exit() here because it results in the global
 // destructors
 // to be invoked and wreaking havoc on the threads still running.
-Host::SystemLog(Host::eSystemLogWarning,
-"SIGHUP received, exiting lldb-server...\n");
+fprintf(stderr, "SIGHUP received, exiting lldb-server...\n");
 abort();
 break;
   }
Index: lldb/test/API/lang/objc/modules-hash-mismatch/TestClangModulesHashMismatch.py
===
--- lldb/test/API/lang/objc/modules-hash-mismatch/TestClangModulesHashMismatch.py
+++ lldb/test/API/lang/objc/modules-hash-mismatch/TestClangModulesHashMismatch.py
@@ -30,12 +30,15 @@
 self.assertTrue(os.path.isdir(mod_cache), "module cache exists")
 
 logfile = self.getBuildArtifact("host.log")
-self.runCmd("log enable -v -f %s lldb host" % logfile)
-target, _, _, _ = lldbutil.run_to_source_breakpoint(
-self, "break here", lldb.SBFileSpec("main.m"))
-target.GetModuleAtIndex(0).FindTypes('my_int')
+with open(logfile, 'w') as f:
+sbf = lldb.SBFile(f.fileno(), 'w', False)
+status = self.dbg.SetErrorFile(sbf)
+self.assertSuccess(status)
+
+target, _, _, _ = lldbutil.run_to_source_breakpoint(
+self, "break here", lldb.SBFileSpec("main.m"))
+target.GetModuleAtIndex(0).FindTypes('my_int')
 
-found = False
 with open(logfile, 'r') as f:
 for line in f:
 if "hash mismatch" in line and "Foo" in line:
Index: lldb/source/Symbol/SymbolContext.cpp
===
--- lldb/source/Symbol/SymbolContext.cpp
+++ lldb/source/Symbol/SymbolContext.cpp
@@ -8,6 +8,7 @@
 
 #include "lldb/Symbol/SymbolContext.h"
 
+#include "lldb/Core/Debugger.h"
 #include "lldb/Core/Module.h"
 #include "lldb/Core/ModuleSpec.h"
 #include "lldb/Host/Host.h"
@@ -494,20 +495,16 @@
   objfile = symbol_file->GetObjectFile();
   }
   if (objfile) {
-Host::SystemLog(
-Host::eSystemLogWarning,
-"warning: inlined block 0x%8.8" PRIx64
-" doesn't have a range that contains file address 0x%" PRIx64
-" in %s\n",
+Debugger::ReportWarning(llvm::formatv(
+"inlined block {0:x} doesn't have a range that contains file "
+"address {1:x} in {2}",
 curr_inlined_block->GetID(), curr_frame_pc.GetFileAddress(),
-objfile->GetFileSpec().GetPath().c_str());
+objfile->GetFileSpec().GetPath()));
   } else {
-Host::SystemLog(
-Host::eSystemLogWarning,
-"warning: inlined block 0x%8.8" PRIx64
-" doesn't have a range that contains file address 0x%" PRIx64
-"\n",
-curr_inlined_block->GetID(), curr_frame_pc.GetFileAddress());
+Debugger::ReportWarning(llvm::formatv(
+"inlined block {0:x} doesn't have a range that contains file "
+"address {1:x}",
+curr_inlined_block->GetID(), curr_frame_pc.GetFileAddress()));
   }
 }
 #endif
Index: lldb/source/Symbol/Function.cpp
===
--- lldb/source/Symbol/Function.cpp
+++ lldb/source/Symbol/Function.cpp
@@ -7,6 +7,7 @@
 //===--===//
 
 #include "lldb/Symbol/Function.h"
+#include "lldb/Core/Debugger.h"
 #include "lldb/Core/Disassembler.h"
 #include "lldb/Core/Module.h"
 #include "lldb/Core/ModuleList.h"
@@ -348,12 +349,9 @@
 if (module_sp) {
   module_sp->GetSymbolFile()->ParseBlocksRecursive(*this);
 } else {
-  Host::SystemLog(Host::eSystemLogError,
-  

[Lldb-commits] [PATCH] D128484: [NFC][lldb][trace] Rename trace session to trace bundle

[walter erquinigo via Phabricator via lldb-commits]

wallace created this revision.
wallace added reviewers: persona0220, jj10306.
Herald added a subscriber: mgorny.
Herald added a project: All.
wallace requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

As previously discussed with @jj10306, we didn't really have a name for
the post-mortem (or offline) trace session representation, which is in
fact a folder with a bunch of files. We decided to call this folder
"trace bundle", and the main JSON file in it "trace bundle description
file". This naming is pretty decent, so I'm refactoring all the existing
code to account for that.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D128484

Files:
  lldb/include/lldb/Core/PluginManager.h
  lldb/include/lldb/Target/Trace.h
  lldb/include/lldb/lldb-private-interfaces.h
  lldb/source/Commands/CommandObjectTrace.cpp
  lldb/source/Core/PluginManager.cpp
  lldb/source/Plugins/Trace/common/ThreadPostMortemTrace.h
  lldb/source/Plugins/Trace/intel-pt/CMakeLists.txt
  lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.h
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleLoader.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleLoader.h
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleSaver.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleSaver.h
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPTJSONStructs.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPTJSONStructs.h
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSessionFileParser.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSessionFileParser.h
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSessionSaver.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSessionSaver.h
  lldb/source/Target/Trace.cpp
  lldb/test/API/commands/trace/TestTraceLoad.py

Index: lldb/test/API/commands/trace/TestTraceLoad.py
===
--- lldb/test/API/commands/trace/TestTraceLoad.py
+++ lldb/test/API/commands/trace/TestTraceLoad.py
@@ -98,7 +98,7 @@
 
 # We test first an invalid type
 trace_description_file_path = os.path.join(src_dir, "intelpt-trace", "trace_bad.json")
-expected_substrs = ['''error: expected object at traceSession.processes[0]
+expected_substrs = ['''error: expected object at traceBundle.processes[0]
 
 Context:
 {
@@ -124,15 +124,15 @@
 self.traceLoad(traceDescriptionFilePath=trace_description_file_path, error=True, substrs=expected_substrs)
 
 
-# Now we test a wrong cpu family field in the global session file
+# Now we test a wrong cpu family field in the global bundle description file
 trace_description_file_path = os.path.join(src_dir, "intelpt-trace", "trace_bad2.json")
-expected_substrs = ['error: expected uint64_t at traceSession.cpuInfo.family', "Context", "Schema"]
+expected_substrs = ['error: expected uint64_t at traceBundle.cpuInfo.family', "Context", "Schema"]
 self.traceLoad(traceDescriptionFilePath=trace_description_file_path, error=True, substrs=expected_substrs)
 
 
 # Now we test a missing field in the intel-pt settings
 trace_description_file_path = os.path.join(src_dir, "intelpt-trace", "trace_bad4.json")
-expected_substrs = ['''error: missing value at traceSession.cpuInfo.family
+expected_substrs = ['''error: missing value at traceBundle.cpuInfo.family
 
 Context:
 {
@@ -149,7 +149,7 @@
 
 # Now we test an incorrect load address in the intel-pt settings
 trace_description_file_path = os.path.join(src_dir, "intelpt-trace", "trace_bad5.json")
-expected_substrs = ['error: missing value at traceSession.processes[1].pid', "Schema"]
+expected_substrs = ['error: missing value at traceBundle.processes[1].pid', "Schema"]
 self.traceLoad(traceDescriptionFilePath=trace_description_file_path, error=True, substrs=expected_substrs)
 
 
@@ -157,6 +157,6 @@
 # no targets should be created.
 self.assertEqual(self.dbg.GetNumTargets(), 0)
 trace_description_file_path = os.path.join(src_dir, "intelpt-trace", "trace_bad3.json")
-expected_substrs = ['error: missing value at traceSession.processes[1].pid']
+expected_substrs = ['error: missing value at traceBundle.processes[1].pid']
 self.traceLoad(traceDescriptionFilePath=trace_description_file_path, error=True, substrs=expected_substrs)
 self.assertEqual(self.dbg.GetNumTargets(), 0)
Index: lldb/source/Target/Trace.cpp
===
--- lldb/source/Target/Trace.cpp
+++ lldb/source/Target/Trace.cpp
@@ -24,19 +24,20 @@
 using namespace lldb_private;
 using namespace llvm;
 
-// Helper structs used to extract the type of a trace session json without
-// having to parse the entire object.
+//

[Lldb-commits] [PATCH] D128321: [lldb] Add OSLog log handler

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere updated this revision to Diff 439583.
JDevlieghere added a comment.

- Move SystemLogHandler into Host
- Create a new SystemLog helper function


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

https://reviews.llvm.org/D128321

Files:
  lldb/include/lldb/Host/Host.h
  lldb/source/Host/common/Host.cpp
  lldb/source/Host/macosx/objcxx/Host.mm


Index: lldb/source/Host/macosx/objcxx/Host.mm
===
--- lldb/source/Host/macosx/objcxx/Host.mm
+++ lldb/source/Host/macosx/objcxx/Host.mm
@@ -82,6 +82,7 @@
 #include "../cfcpp/CFCString.h"
 
 #include 
+#include 
 
 #include 
 #include 
@@ -98,6 +99,17 @@
 using namespace lldb;
 using namespace lldb_private;
 
+static os_log_t g_os_log;
+static std::once_flag g_os_log_once;
+
+void Host::SystemLog(std::string message) {
+  if (__builtin_available(macos 10.12, iOS 10, tvOS 10, watchOS 3, *)) {
+os_log(g_os_log, "%{public}s", message.c_str());
+  } else {
+fprintf(stderr, "%s", message.c_str());
+  }
+}
+
 bool Host::GetBundleDirectory(const FileSpec &file,
   FileSpec &bundle_directory) {
 #if defined(__APPLE__)
Index: lldb/source/Host/common/Host.cpp
===
--- lldb/source/Host/common/Host.cpp
+++ lldb/source/Host/common/Host.cpp
@@ -106,6 +106,12 @@
   });
 }
 
+#if !defined(__APPLE__)
+void Host::SystemLog(std::string message) {
+  fprintf(stderr, "%s", message.c_str());
+}
+#endif
+
 #ifndef __linux__
 // Scoped class that will disable thread canceling when it is constructed, and
 // exception safely restore the previous value it when it goes out of scope.
@@ -627,3 +633,9 @@
 
   return result;
 }
+
+SystemLogHandler::SystemLogHandler() {}
+
+void SystemLogHandler::Emit(llvm::StringRef message) {
+  Host::SystemLog(std::string(message));
+}
Index: lldb/include/lldb/Host/Host.h
===
--- lldb/include/lldb/Host/Host.h
+++ lldb/include/lldb/Host/Host.h
@@ -13,6 +13,7 @@
 #include "lldb/Host/HostThread.h"
 #include "lldb/Utility/Environment.h"
 #include "lldb/Utility/FileSpec.h"
+#include "lldb/Utility/Log.h"
 #include "lldb/Utility/Timeout.h"
 #include "lldb/lldb-private-forward.h"
 #include "lldb/lldb-private.h"
@@ -86,6 +87,9 @@
   StartMonitoringChildProcess(const MonitorChildProcessCallback &callback,
   lldb::pid_t pid);
 
+  /// Emit the given message to the operating system's log.
+  void SystemLog(std::string message);
+
   /// Get the process ID for the calling process.
   ///
   /// \return
@@ -252,6 +256,12 @@
 ProcessInstanceInfoList &proc_infos);
 };
 
+class SystemLogHandler : public LogHandler {
+public:
+  SystemLogHandler();
+  void Emit(llvm::StringRef message) override;
+};
+
 } // namespace lldb_private
 
 namespace llvm {


Index: lldb/source/Host/macosx/objcxx/Host.mm
===
--- lldb/source/Host/macosx/objcxx/Host.mm
+++ lldb/source/Host/macosx/objcxx/Host.mm
@@ -82,6 +82,7 @@
 #include "../cfcpp/CFCString.h"
 
 #include 
+#include 
 
 #include 
 #include 
@@ -98,6 +99,17 @@
 using namespace lldb;
 using namespace lldb_private;
 
+static os_log_t g_os_log;
+static std::once_flag g_os_log_once;
+
+void Host::SystemLog(std::string message) {
+  if (__builtin_available(macos 10.12, iOS 10, tvOS 10, watchOS 3, *)) {
+os_log(g_os_log, "%{public}s", message.c_str());
+  } else {
+fprintf(stderr, "%s", message.c_str());
+  }
+}
+
 bool Host::GetBundleDirectory(const FileSpec &file,
   FileSpec &bundle_directory) {
 #if defined(__APPLE__)
Index: lldb/source/Host/common/Host.cpp
===
--- lldb/source/Host/common/Host.cpp
+++ lldb/source/Host/common/Host.cpp
@@ -106,6 +106,12 @@
   });
 }
 
+#if !defined(__APPLE__)
+void Host::SystemLog(std::string message) {
+  fprintf(stderr, "%s", message.c_str());
+}
+#endif
+
 #ifndef __linux__
 // Scoped class that will disable thread canceling when it is constructed, and
 // exception safely restore the previous value it when it goes out of scope.
@@ -627,3 +633,9 @@
 
   return result;
 }
+
+SystemLogHandler::SystemLogHandler() {}
+
+void SystemLogHandler::Emit(llvm::StringRef message) {
+  Host::SystemLog(std::string(message));
+}
Index: lldb/include/lldb/Host/Host.h
===
--- lldb/include/lldb/Host/Host.h
+++ lldb/include/lldb/Host/Host.h
@@ -13,6 +13,7 @@
 #include "lldb/Host/HostThread.h"
 #include "lldb/Utility/Environment.h"
 #include "lldb/Utility/FileSpec.h"
+#include "lldb/Utility/Log.h"
 #include "lldb/Utility/Timeout.h"
 #include "lldb/lldb-private-forward.h"
 #include "lldb/lldb-private.h"
@@ -86,6 +87,9 @@
   StartMonitoringChildProcess(const MonitorChildProcessCallback &ca

[Lldb-commits] [PATCH] D128480: [lldb] Replace Host::SystemLog with Debugger::Report{Error, Warning}

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

In D128480#3606632 , @JDevlieghere 
wrote:

> In D128480#3606598 , @clayborg 
> wrote:
>
>> I don't have anything against this, though I still think the LogHandlers 
>> should use the Host layer by making the LogHandler objects into real LLDB 
>> plug-ins. But if everyone else feels otherwise, I won't object.
>
> This patch is meant to be entirely orthogonal to that. I was still going to 
> add (what I believe is a better) implementation of SystemLog. In D128321 
>  I said:
>
>> But if we really want this to live in Host (and deal with the dependency 
>> issues later) I can implement a new Host::SystemLog variant that essentially 
>> does what the SystemLogHander is doing and implement the log handler in Host 
>> based on that.
>
> So that's what I was alluding to.

I've updated D128321  with the new ConsoleLog 
implementation.


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

https://reviews.llvm.org/D128480

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

[Lldb-commits] [PATCH] D128477: [trace] Add a flag to the decoder to output the instruction type

[walter erquinigo via Phabricator via lldb-commits]

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

@jingham, that's a pretty good idea. We'll see how to implement it


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128477

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

[Lldb-commits] [PATCH] D128321: [lldb] Add OSLog log handler

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere updated this revision to Diff 439588.

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

https://reviews.llvm.org/D128321

Files:
  lldb/include/lldb/Host/Host.h
  lldb/source/Host/common/Host.cpp
  lldb/source/Host/macosx/objcxx/Host.mm


Index: lldb/source/Host/macosx/objcxx/Host.mm
===
--- lldb/source/Host/macosx/objcxx/Host.mm
+++ lldb/source/Host/macosx/objcxx/Host.mm
@@ -82,6 +82,7 @@
 #include "../cfcpp/CFCString.h"
 
 #include 
+#include 
 
 #include 
 #include 
@@ -98,6 +99,20 @@
 using namespace lldb;
 using namespace lldb_private;
 
+static os_log_t g_os_log;
+static std::once_flag g_os_log_once;
+
+void Host::SystemLog(const std::string &message) {
+  if (__builtin_available(macos 10.12, iOS 10, tvOS 10, watchOS 3, *)) {
+std::call_once(g_os_log_once, []() {
+  g_os_log = os_log_create("com.apple.dt.lldb", "lldb");
+});
+os_log(g_os_log, "%{public}s", message.c_str());
+  } else {
+fprintf(stderr, "%s", message.c_str());
+  }
+}
+
 bool Host::GetBundleDirectory(const FileSpec &file,
   FileSpec &bundle_directory) {
 #if defined(__APPLE__)
Index: lldb/source/Host/common/Host.cpp
===
--- lldb/source/Host/common/Host.cpp
+++ lldb/source/Host/common/Host.cpp
@@ -106,6 +106,12 @@
   });
 }
 
+#if !defined(__APPLE__)
+void Host::SystemLog(const std::string &message) {
+  fprintf(stderr, "%s", message.c_str());
+}
+#endif
+
 #ifndef __linux__
 // Scoped class that will disable thread canceling when it is constructed, and
 // exception safely restore the previous value it when it goes out of scope.
@@ -627,3 +633,9 @@
 
   return result;
 }
+
+SystemLogHandler::SystemLogHandler() {}
+
+void SystemLogHandler::Emit(llvm::StringRef message) {
+  Host::SystemLog(std::string(message));
+}
Index: lldb/include/lldb/Host/Host.h
===
--- lldb/include/lldb/Host/Host.h
+++ lldb/include/lldb/Host/Host.h
@@ -13,6 +13,7 @@
 #include "lldb/Host/HostThread.h"
 #include "lldb/Utility/Environment.h"
 #include "lldb/Utility/FileSpec.h"
+#include "lldb/Utility/Log.h"
 #include "lldb/Utility/Timeout.h"
 #include "lldb/lldb-private-forward.h"
 #include "lldb/lldb-private.h"
@@ -86,6 +87,9 @@
   StartMonitoringChildProcess(const MonitorChildProcessCallback &callback,
   lldb::pid_t pid);
 
+  /// Emit the given message to the operating system's log.
+  static void SystemLog(const std::string &message);
+
   /// Get the process ID for the calling process.
   ///
   /// \return
@@ -252,6 +256,12 @@
 ProcessInstanceInfoList &proc_infos);
 };
 
+class SystemLogHandler : public LogHandler {
+public:
+  SystemLogHandler();
+  void Emit(llvm::StringRef message) override;
+};
+
 } // namespace lldb_private
 
 namespace llvm {


Index: lldb/source/Host/macosx/objcxx/Host.mm
===
--- lldb/source/Host/macosx/objcxx/Host.mm
+++ lldb/source/Host/macosx/objcxx/Host.mm
@@ -82,6 +82,7 @@
 #include "../cfcpp/CFCString.h"
 
 #include 
+#include 
 
 #include 
 #include 
@@ -98,6 +99,20 @@
 using namespace lldb;
 using namespace lldb_private;
 
+static os_log_t g_os_log;
+static std::once_flag g_os_log_once;
+
+void Host::SystemLog(const std::string &message) {
+  if (__builtin_available(macos 10.12, iOS 10, tvOS 10, watchOS 3, *)) {
+std::call_once(g_os_log_once, []() {
+  g_os_log = os_log_create("com.apple.dt.lldb", "lldb");
+});
+os_log(g_os_log, "%{public}s", message.c_str());
+  } else {
+fprintf(stderr, "%s", message.c_str());
+  }
+}
+
 bool Host::GetBundleDirectory(const FileSpec &file,
   FileSpec &bundle_directory) {
 #if defined(__APPLE__)
Index: lldb/source/Host/common/Host.cpp
===
--- lldb/source/Host/common/Host.cpp
+++ lldb/source/Host/common/Host.cpp
@@ -106,6 +106,12 @@
   });
 }
 
+#if !defined(__APPLE__)
+void Host::SystemLog(const std::string &message) {
+  fprintf(stderr, "%s", message.c_str());
+}
+#endif
+
 #ifndef __linux__
 // Scoped class that will disable thread canceling when it is constructed, and
 // exception safely restore the previous value it when it goes out of scope.
@@ -627,3 +633,9 @@
 
   return result;
 }
+
+SystemLogHandler::SystemLogHandler() {}
+
+void SystemLogHandler::Emit(llvm::StringRef message) {
+  Host::SystemLog(std::string(message));
+}
Index: lldb/include/lldb/Host/Host.h
===
--- lldb/include/lldb/Host/Host.h
+++ lldb/include/lldb/Host/Host.h
@@ -13,6 +13,7 @@
 #include "lldb/Host/HostThread.h"
 #include "lldb/Utility/Environment.h"
 #include "lldb/Utility/FileSpec.h"
+#include "lldb/Utility/Log.h"
 #include "lldb/Utility/Timeout.

[Lldb-commits] [PATCH] D127889: [LLDB] Handle DIE with DW_AT_low_pc and empty ranges

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

It should be sufficient to replace the %clang line with the appropriate ld.lld 
equivalent, and replace REQUIRES: system-linux with REQUIRES: lld.

Also it test/Shell/SymbolFile/DWARF would be a better place for this test (and 
it also includes some tests that you can take inspiration from).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127889

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

[Lldb-commits] [PATCH] D128268: [lldb] Fix reading i686-windows executables with GNU environment

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

> As a separate path forward, one could also consider to stop returning
> two architecture specs from ObjectFilePECOFF::GetModuleSpecifications
> for i386 files.

I think that would be much more preferable. I think the current implementation 
GetModuleSpecifications is based on a misconception about what the multiple 
results of that function mean (a fat binary -- as you've discovered).

We wouldn't want to return armv6-*-* (or armv7, v8, ...) for a file that claims 
it can be run on armv5. Elf files also return just a single result for i386 
file.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128268

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

[Lldb-commits] [PATCH] D128201: [lldb][windows] Fix crash on getting nested exception

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

How can one trigger this behavior? Adding a test would be preferable to a long 
comment (we can have both, but the need for a (long) comment diminishes if you 
have a test which will break if someone implements it the wrong way.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128201

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

[Lldb-commits] [PATCH] D128069: [lldb] add SBSection.alignment to python bindings

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D128069#3602256 , @dmlary wrote:

>> We should test any APIs we add in a python test IMHO.  Also testing that the 
>> ".alignment" property works
>
> Ok, I'll add tests for the added python function and property.
>
> Now for the hard part, what's the recommended way to access elf section 
> details within python without using the function added in this commit?  I do 
> not think it is safe to hard code the expected alignment in the test case as 
> the default alignment may vary across architectures.  Shelling out to 
> `readelf` and parsing is possible, but feels clunky; is there an existing 
> function to get this data?

You can use yaml2obj to create sections with known alignments. You should be 
able to find examples of that in the existing tests (just grep for yaml2obj).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128069

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

[Lldb-commits] [PATCH] D128366: [lldb] Make Module::LookupInfo::Prune language-agnostic

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

How hot is this code? Parsing a demangled name isn't entirely cheap, and the 
fact that the objc++ class calls back into the c++ version means that the c++ 
name will be parsed twice.

Could we move this pruning elsewhere? These values come from the symbol file 
plugins anyway, and they can do a better job at determining which language does 
a particular name belong to.

(OK, they can an also come from the symtab, but there I guess we could infer 
something from the mangling scheme).




Comment at: lldb/include/lldb/Target/Language.h:320
+  virtual bool NamesAreEquivalentWithContext(
+  const ConstString &user_provided_name,
+  const ConstString &full_name_with_context) const {

I think we're passing ConstStrings by value these days...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128366

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

[Lldb-commits] [PATCH] D128321: [lldb] Add OSLog log handler

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D128321#3603007 , @JDevlieghere 
wrote:

> In D128321#3602129 , @clayborg 
> wrote:
>
>> I think we should allow this class to always exist and not conditionally 
>> compile it in or out. Then we add a new Host.h method to emit a log message 
>> in "lldb/Host/Host.h" and allow each host OS to emit a message. Maybe there 
>> is a default implementation that emits the message to stderr, and then we 
>> override this for in HostInfoMacOSX.h? Then each OS can use their OS logging 
>> API of choice.
>
> Yeah, I considered that, but that would make Utility depend on Host which 
> would (re)introduce the circular dependency between the two. The alternative 
> would be to still make the class available unconditionally, but use target 
> ifdefs to have different implementations.

We already have `Host::SystemLog`. Could we use that? Or change it so that it 
can be used?

As for the dependencies, I think we could structure things such that the 
SystemLogHandler lives in the Host module, and the general logging 
infrastructure is not aware of it. (The only one who would know about it would 
be the `log enable` command, which would pass it down as a generic reference.)


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

https://reviews.llvm.org/D128321

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

[Lldb-commits] [PATCH] D128386: [lldb] Make thread safety the responsibility of the log handlers

[Pavel Labath via Phabricator via lldb-commits]

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

Cool, thanks.


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

https://reviews.llvm.org/D128386

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

[Lldb-commits] [PATCH] D128268: [lldb] Fix reading i686-windows executables with GNU environment

[Martin Storsjö via Phabricator via lldb-commits]

mstorsjo added a comment.

In D128268#3603931 , @labath wrote:

>> As a separate path forward, one could also consider to stop returning
>> two architecture specs from ObjectFilePECOFF::GetModuleSpecifications
>> for i386 files.
>
> I think that would be much more preferable. I think the current 
> implementation GetModuleSpecifications is based on a misconception about what 
> the multiple results of that function mean (a fat binary -- as you've 
> discovered).
>
> We wouldn't want to return armv6-*-* (or armv7, v8, ...) for a file that 
> claims it can be run on armv5. Elf files also return just a single result for 
> i386 file.

Yep, that's probably a good path forward for the future - I was just weary of 
that possibly opening another huge can of worms. And regardless of that, I 
think the functional change of this patch, allowing considering various 
variations of triples that commonly occur on windows as compatible, is sensible 
on its own (such issues have caused unnecessary tweaking back and forth once or 
twice before too).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128268

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

[Lldb-commits] [PATCH] D128264: [lldb/dyld-posix] Avoid reading the module list in inconsistent states

[Pavel Labath via Phabricator via lldb-commits]

labath updated this revision to Diff 439277.
labath added a comment.

reuse the module pointer


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128264

Files:
  lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DYLDRendezvous.cpp
  lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp

Index: lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp
===
--- lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp
+++ lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp
@@ -433,27 +433,31 @@
 for (; I != E; ++I) {
   ModuleSP module_sp =
   LoadModuleAtAddress(I->file_spec, I->link_addr, I->base_addr, true);
-  if (module_sp.get()) {
-if (module_sp->GetObjectFile()->GetBaseAddress().GetLoadAddress(
-&m_process->GetTarget()) == m_interpreter_base &&
-module_sp != m_interpreter_module.lock()) {
-  if (m_interpreter_module.lock() == nullptr) {
-m_interpreter_module = module_sp;
-  } else {
-// If this is a duplicate instance of ld.so, unload it.  We may end
-// up with it if we load it via a different path than before
-// (symlink vs real path).
-// TODO: remove this once we either fix library matching or avoid
-// loading the interpreter when setting the rendezvous breakpoint.
-UnloadSections(module_sp);
-loaded_modules.Remove(module_sp);
-continue;
-  }
+  if (!module_sp.get())
+continue;
+
+  if (module_sp->GetObjectFile()->GetBaseAddress().GetLoadAddress(
+  &m_process->GetTarget()) == m_interpreter_base) {
+ModuleSP interpreter_sp = m_interpreter_module.lock();
+if (m_interpreter_module.lock() == nullptr) {
+  m_interpreter_module = module_sp;
+} else if (module_sp == interpreter_sp) {
+  // Module already loaded.
+  continue;
+} else {
+  // If this is a duplicate instance of ld.so, unload it.  We may end
+  // up with it if we load it via a different path than before
+  // (symlink vs real path).
+  // TODO: remove this once we either fix library matching or avoid
+  // loading the interpreter when setting the rendezvous breakpoint.
+  UnloadSections(module_sp);
+  loaded_modules.Remove(module_sp);
+  continue;
 }
-
-loaded_modules.AppendIfNeeded(module_sp);
-new_modules.Append(module_sp);
   }
+
+  loaded_modules.AppendIfNeeded(module_sp);
+  new_modules.Append(module_sp);
 }
 m_process->GetTarget().ModulesDidLoad(new_modules);
   }
Index: lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DYLDRendezvous.cpp
===
--- lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DYLDRendezvous.cpp
+++ lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DYLDRendezvous.cpp
@@ -210,14 +210,7 @@
 
   case eAdd:
   case eDelete:
-// Some versions of the android dynamic linker might send two
-// notifications with state == eAdd back to back. Ignore them until we
-// get an eConsistent notification.
-if (!(m_previous.state == eConsistent ||
-  (m_previous.state == eAdd && m_current.state == eDelete)))
-  return eNoAction;
-
-return eTakeSnapshot;
+return eNoAction;
   }
 
   return eNoAction;
@@ -229,9 +222,9 @@
   if (action == eNoAction)
 return false;
 
+  m_added_soentries.clear();
+  m_removed_soentries.clear();
   if (action == eTakeSnapshot) {
-m_added_soentries.clear();
-m_removed_soentries.clear();
 // We already have the loaded list from the previous update so no need to
 // find all the modules again.
 if (!m_loaded_modules.m_list.empty())
@@ -260,11 +253,11 @@
 }
 
 bool DYLDRendezvous::UpdateSOEntries() {
+  m_added_soentries.clear();
+  m_removed_soentries.clear();
   switch (GetAction()) {
   case eTakeSnapshot:
 m_soentries.clear();
-m_added_soentries.clear();
-m_removed_soentries.clear();
 return TakeSnapshot(m_soentries);
   case eAddModules:
 return AddSOEntries();
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D128264: [lldb/dyld-posix] Avoid reading the module list in inconsistent states

[Pavel Labath via Phabricator via lldb-commits]

labath marked an inline comment as done.
labath added inline comments.



Comment at: 
lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp:441
+  &m_process->GetTarget()) == m_interpreter_base) {
+ModuleSP interpreter_sp = m_interpreter_module.lock();
+if (m_interpreter_module.lock() == nullptr) {

emrekultursay wrote:
> Use this variable below, instead of calling lock() again?
Yes. In fact, that's why I created that variable in the first place. :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128264

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

[Lldb-commits] [PATCH] D128410: [lldb] Add a testcase for nested exceptions on Windows

[Martin Storsjö via Phabricator via lldb-commits]

mstorsjo created this revision.
mstorsjo added reviewers: labath, alvinhochun, DavidSpickett.
Herald added a project: All.
mstorsjo requested review of this revision.
Herald added a project: LLDB.

This adds a testcase for
4d123783957e547009e55346bf3a8ae43a88fa14 
 / D128201 
. Before that commit,
lldb would crash here, while now lldb manages to stop after the
exception.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D128410

Files:
  lldb/test/Shell/Process/Windows/wndproc_exception.cpp


Index: lldb/test/Shell/Process/Windows/wndproc_exception.cpp
===
--- /dev/null
+++ lldb/test/Shell/Process/Windows/wndproc_exception.cpp
@@ -0,0 +1,54 @@
+// clang-format off
+
+// Check that lldb doesn't crash when there's a nested exception.
+
+// REQUIRES: system-windows
+// RUN: %clangxx_host -o %t.exe -luser32 -v -- %s
+// RUN: %lldb -f %t.exe -o "run"
+
+#include 
+
+static LRESULT CALLBACK WindowProc(HWND hwnd, UINT uMsg, WPARAM wParam,
+   LPARAM lParam) {
+  switch (uMsg) {
+  case WM_DESTROY:
+PostQuitMessage(0);
+return 0;
+
+  case WM_PAINT:
+*(volatile int *)nullptr = 1;
+return 0;
+  }
+  return DefWindowProcA(hwnd, uMsg, wParam, lParam);
+}
+
+int WINAPI WinMain(HINSTANCE hInstance, HINSTANCE hPrevInstance, LPSTR 
pCmdLine,
+   int nCmdShow) {
+  const char CLASS_NAME[] = "Crash Window Class";
+
+  WNDCLASSA wc = {};
+
+  wc.lpfnWndProc = WindowProc;
+  wc.hInstance = hInstance;
+  wc.lpszClassName = CLASS_NAME;
+
+  RegisterClassA(&wc);
+
+  HWND hwnd = CreateWindowExA(0, CLASS_NAME, "Crash", WS_OVERLAPPEDWINDOW,
+  CW_USEDEFAULT, CW_USEDEFAULT, CW_USEDEFAULT,
+  CW_USEDEFAULT,
+  NULL, NULL, hInstance, NULL);
+
+  if (hwnd == NULL)
+return 0;
+
+  ShowWindow(hwnd, nCmdShow);
+
+  MSG msg = {};
+  while (GetMessageA(&msg, NULL, 0, 0) > 0) {
+TranslateMessage(&msg);
+DispatchMessageA(&msg);
+  }
+
+  return 0;
+}


Index: lldb/test/Shell/Process/Windows/wndproc_exception.cpp
===
--- /dev/null
+++ lldb/test/Shell/Process/Windows/wndproc_exception.cpp
@@ -0,0 +1,54 @@
+// clang-format off
+
+// Check that lldb doesn't crash when there's a nested exception.
+
+// REQUIRES: system-windows
+// RUN: %clangxx_host -o %t.exe -luser32 -v -- %s
+// RUN: %lldb -f %t.exe -o "run"
+
+#include 
+
+static LRESULT CALLBACK WindowProc(HWND hwnd, UINT uMsg, WPARAM wParam,
+   LPARAM lParam) {
+  switch (uMsg) {
+  case WM_DESTROY:
+PostQuitMessage(0);
+return 0;
+
+  case WM_PAINT:
+*(volatile int *)nullptr = 1;
+return 0;
+  }
+  return DefWindowProcA(hwnd, uMsg, wParam, lParam);
+}
+
+int WINAPI WinMain(HINSTANCE hInstance, HINSTANCE hPrevInstance, LPSTR pCmdLine,
+   int nCmdShow) {
+  const char CLASS_NAME[] = "Crash Window Class";
+
+  WNDCLASSA wc = {};
+
+  wc.lpfnWndProc = WindowProc;
+  wc.hInstance = hInstance;
+  wc.lpszClassName = CLASS_NAME;
+
+  RegisterClassA(&wc);
+
+  HWND hwnd = CreateWindowExA(0, CLASS_NAME, "Crash", WS_OVERLAPPEDWINDOW,
+  CW_USEDEFAULT, CW_USEDEFAULT, CW_USEDEFAULT,
+  CW_USEDEFAULT,
+  NULL, NULL, hInstance, NULL);
+
+  if (hwnd == NULL)
+return 0;
+
+  ShowWindow(hwnd, nCmdShow);
+
+  MSG msg = {};
+  while (GetMessageA(&msg, NULL, 0, 0) > 0) {
+TranslateMessage(&msg);
+DispatchMessageA(&msg);
+  }
+
+  return 0;
+}
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D128410: [lldb] Add a testcase for nested exceptions on Windows

[Martin Storsjö via Phabricator via lldb-commits]

mstorsjo added a comment.
Herald added a subscriber: JDevlieghere.

For this testcase to work, it needs to be able to open a window - so it can't 
run entirely in headless mode. But I guess that a bunch of other tests in LLDB 
also already do that, since running the LLDB tests on Windows pops up a dozen 
of windows temporarily.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128410

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

[Lldb-commits] [PATCH] D128410: [lldb] Add a testcase for nested exceptions on Windows

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D128410#3604066 , @mstorsjo wrote:

> For this testcase to work, it needs to be able to open a window - so it can't 
> run entirely in headless mode. But I guess that a bunch of other tests in 
> LLDB also already do that, since running the LLDB tests on Windows pops up a 
> dozen of windows temporarily.

I think those just come from the tests (or more like test runners) which forget 
to suppress a console window from opening (see 
LLDB_LAUNCH_INFERIORS_WITHOUT_CONSOLE) -- this is probably the first "gui" 
application. This isn't necessarily a showstopper, but I am surprised that it 
is necessary. I'm hardly a windows expert, but I would expect that it should be 
possible to generate an exception (even nested exception, which to me sounds 
like the equivalent of getting a signal inside a signal handler) in a "regular" 
text-mode application. Are you sure that is not possible?




Comment at: lldb/test/Shell/Process/Windows/wndproc_exception.cpp:7
+// RUN: %clangxx_host -o %t.exe -luser32 -v -- %s
+// RUN: %lldb -f %t.exe -o "run"
+

Is there something reasonable we could assert here? The process exit status for 
instance?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128410

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

[Lldb-commits] [PATCH] D128268: [lldb] Fix reading i686-windows executables with GNU environment

[Martin Storsjö via Phabricator via lldb-commits]

mstorsjo added a comment.

In D128268#3603931 , @labath wrote:

>> As a separate path forward, one could also consider to stop returning
>> two architecture specs from ObjectFilePECOFF::GetModuleSpecifications
>> for i386 files.
>
> I think that would be much more preferable. I think the current 
> implementation GetModuleSpecifications is based on a misconception about what 
> the multiple results of that function mean (a fat binary -- as you've 
> discovered).
>
> We wouldn't want to return armv6-*-* (or armv7, v8, ...) for a file that 
> claims it can be run on armv5. Elf files also return just a single result for 
> i386 file.

If we'd just set this to the baseline, i386, would that have any effect for how 
lldb e.g. is able to disassemble/interpret instructions that don't exist in the 
i386 baseline architecture?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128268

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

[Lldb-commits] [PATCH] D128268: [lldb] Fix reading i686-windows executables with GNU environment

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D128268#3604077 , @mstorsjo wrote:

> In D128268#3603931 , @labath wrote:
>
>>> As a separate path forward, one could also consider to stop returning
>>> two architecture specs from ObjectFilePECOFF::GetModuleSpecifications
>>> for i386 files.
>>
>> I think that would be much more preferable. I think the current 
>> implementation GetModuleSpecifications is based on a misconception about 
>> what the multiple results of that function mean (a fat binary -- as you've 
>> discovered).
>>
>> We wouldn't want to return armv6-*-* (or armv7, v8, ...) for a file that 
>> claims it can be run on armv5. Elf files also return just a single result 
>> for i386 file.
>
> If we'd just set this to the baseline, i386, would that have any effect for 
> how lldb e.g. is able to disassemble/interpret instructions that don't exist 
> in the i386 baseline architecture?

It should not have any effect (if it does, that's a separate fix). In the 
disassembler, we explicitly enable the latest instruction set, and I can't 
think of anything else that would be impacted by it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128268

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

[Lldb-commits] [PATCH] D128410: [lldb] Add a testcase for nested exceptions on Windows

[Martin Storsjö via Phabricator via lldb-commits]

mstorsjo added a subscriber: stella.stamenova.
mstorsjo added a comment.

In D128410#3604075 , @labath wrote:

> In D128410#3604066 , @mstorsjo 
> wrote:
>
>> For this testcase to work, it needs to be able to open a window - so it 
>> can't run entirely in headless mode. But I guess that a bunch of other tests 
>> in LLDB also already do that, since running the LLDB tests on Windows pops 
>> up a dozen of windows temporarily.
>
> I think those just come from the tests (or more like test runners) which 
> forget to suppress a console window from opening (see 
> LLDB_LAUNCH_INFERIORS_WITHOUT_CONSOLE) -- this is probably the first "gui" 
> application. This isn't necessarily a showstopper, but I am surprised that it 
> is necessary. I'm hardly a windows expert, but I would expect that it should 
> be possible to generate an exception (even nested exception, which to me 
> sounds like the equivalent of getting a signal inside a signal handler) in a 
> "regular" text-mode application. Are you sure that is not possible?

I didn't say that this isn't necessarily possible - this is the reduced 
testcase that @alvinhochun provided in 
https://github.com/mstorsjo/llvm-mingw/issues/292#issuecomment-1160239522 (that 
I shrunk down further a bit).

I'm not all that familiar with exactly what happens in these APIs here that 
makes this a nested exception and what other non-GUI APIs would produce the 
same effect. Maybe any API where Windows system code calls back to a user 
provided callback? Does @stella.stamenova know?




Comment at: lldb/test/Shell/Process/Windows/wndproc_exception.cpp:7
+// RUN: %clangxx_host -o %t.exe -luser32 -v -- %s
+// RUN: %lldb -f %t.exe -o "run"
+

labath wrote:
> Is there something reasonable we could assert here? The process exit status 
> for instance?
This in itself requires the `%lldb` command to succeed - the exit status for 
each `RUN` line needs to be successful (unless a failure is expected and it can 
be inverted with the `not` command). Or did you mean checking the process exit 
code of the child process (that intentionally does crash)?

When this test case is run, it prints the following to the terminal:
```
(lldb) run
Process 3168 stopped
* thread #1, stop reason = Exception 0xc41d encountered at address 
0x7ff68e6f1227
frame #0: 0x7ff68e6f1227 wndproc_exception.cpp.tmp.exe
->  0x7ff68e6f1227: movl   $0x1, (%rax)
0x7ff68e6f122d: movq   $0x0, 0x50(%rsp)
0x7ff68e6f1236: jmp0x7ff68e6f1259
0x7ff68e6f123b: movq   0x48(%rsp), %r9
Process 3168 launched: 
'C:\dev\llvm-project\llvm\build-msvc\tools\lldb\test\Shell\Process\Windows\Output\wndproc_exception.cpp.tmp.exe'
 (x86_64)
```
I guess we could check for `Exception 0xc41d encountered` maybe, although 
I'm afraid of making the testcase unnecessarily brittle too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128410

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

[Lldb-commits] [PATCH] D128324: [lldb] [llgs] Introduce a AppendThreadIDToResponse() helper

[Michał Górny via Phabricator via lldb-commits]

mgorny marked an inline comment as done.
mgorny added inline comments.



Comment at: 
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:4162
+void GDBRemoteCommunicationServerLLGS::AppendThreadIDToResponse(
+StreamString &response, lldb::pid_t pid, lldb::tid_t tid) {
+  if (bool(m_extensions_supported &

labath wrote:
> This could technically be any Stream object, right?
Right.


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

https://reviews.llvm.org/D128324

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

[Lldb-commits] [PATCH] D128324: [lldb] [llgs] Introduce an AppendThreadIDToResponse() helper

[Michał Górny via Phabricator via lldb-commits]

mgorny updated this revision to Diff 439286.
mgorny marked an inline comment as done.
mgorny added a comment.

Allow any `Stream` object.


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

https://reviews.llvm.org/D128324

Files:
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h


Index: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h
===
--- lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h
+++ lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h
@@ -273,6 +273,9 @@
   // in non-stop mode, no response otherwise.
   PacketResult SendContinueSuccessResponse();
 
+  void AppendThreadIDToResponse(Stream &response, lldb::pid_t pid,
+lldb::tid_t tid);
+
 private:
   llvm::Expected> BuildTargetXml();
 
Index: 
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
===
--- lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
+++ lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
@@ -826,10 +826,8 @@
 
   // Include the (pid and) tid.
   response.PutCString("thread:");
-  if (bool(m_extensions_supported &
-   NativeProcessProtocol::Extension::multiprocess))
-response.Format("p{0:x-}.", process.GetID());
-  response.Format("{0:x-};", thread.GetID());
+  AppendThreadIDToResponse(response, process.GetID(), thread.GetID());
+  response.PutChar(';');
 
   // Include the thread name if there is one.
   const std::string thread_name = thread.GetName();
@@ -1448,9 +1446,8 @@
 
   StreamString response;
   response.PutCString("QC");
-  if (bool(m_extensions_supported & 
NativeProcessProtocol::Extension::multiprocess))
-response.Format("p{0:x-}.", m_current_process->GetID());
-  response.Format("{0:x-}", thread->GetID());
+  AppendThreadIDToResponse(response, m_current_process->GetID(),
+   thread->GetID());
 
   return SendPacketNoLock(response.GetString());
 }
@@ -2019,10 +2016,7 @@
 LLDB_LOG(log, "iterated thread {0} (tid={1})", thread_index,
  thread->GetID());
 response.PutChar(had_any ? ',' : 'm');
-if (bool(m_extensions_supported &
- NativeProcessProtocol::Extension::multiprocess))
-  response.Format("p{0:x-}.", pid);
-response.Format("{0:x-}", thread->GetID());
+AppendThreadIDToResponse(response, pid, thread->GetID());
 had_any = true;
   }
 }
@@ -4164,6 +4158,14 @@
   return m_non_stop ? SendOKResponse() : PacketResult::Success;
 }
 
+void GDBRemoteCommunicationServerLLGS::AppendThreadIDToResponse(
+Stream &response, lldb::pid_t pid, lldb::tid_t tid) {
+  if (bool(m_extensions_supported &
+   NativeProcessProtocol::Extension::multiprocess))
+response.Format("p{0:x-}.", pid);
+  response.Format("{0:x-}", tid);
+}
+
 std::string
 lldb_private::process_gdb_remote::LLGSArgToURL(llvm::StringRef url_arg,
bool reverse_connect) {


Index: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h
===
--- lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h
+++ lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h
@@ -273,6 +273,9 @@
   // in non-stop mode, no response otherwise.
   PacketResult SendContinueSuccessResponse();
 
+  void AppendThreadIDToResponse(Stream &response, lldb::pid_t pid,
+lldb::tid_t tid);
+
 private:
   llvm::Expected> BuildTargetXml();
 
Index: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
===
--- lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
+++ lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
@@ -826,10 +826,8 @@
 
   // Include the (pid and) tid.
   response.PutCString("thread:");
-  if (bool(m_extensions_supported &
-   NativeProcessProtocol::Extension::multiprocess))
-response.Format("p{0:x-}.", process.GetID());
-  response.Format("{0:x-};", thread.GetID());
+  AppendThreadIDToResponse(response, process.GetID(), thread.GetID());
+  response.PutChar(';');
 
   // Include the thread name if there is one.
   const std::string thread_name = thread.GetName();
@@ -1448,9 +1446,8 @@
 
   StreamString response;
   response.PutCString("QC");
-  if (bool(m_extensions_supported & NativeProcessProtocol::Extension::multiprocess))
-response.Format("p{0:x-}.", m_current_process->GetID());
-  response.Format("{0:x-}", thread->GetID());
+  AppendThreadIDToResponse(response, m_current_process->GetID(),
+   

[Lldb-commits] [PATCH] D128410: [lldb] Add a testcase for nested exceptions on Windows

[Martin Storsjö via Phabricator via lldb-commits]

mstorsjo added inline comments.



Comment at: lldb/test/Shell/Process/Windows/wndproc_exception.cpp:7
+// RUN: %clangxx_host -o %t.exe -luser32 -v -- %s
+// RUN: %lldb -f %t.exe -o "run"
+

mstorsjo wrote:
> labath wrote:
> > Is there something reasonable we could assert here? The process exit status 
> > for instance?
> This in itself requires the `%lldb` command to succeed - the exit status for 
> each `RUN` line needs to be successful (unless a failure is expected and it 
> can be inverted with the `not` command). Or did you mean checking the process 
> exit code of the child process (that intentionally does crash)?
> 
> When this test case is run, it prints the following to the terminal:
> ```
> (lldb) run
> Process 3168 stopped
> * thread #1, stop reason = Exception 0xc41d encountered at address 
> 0x7ff68e6f1227
> frame #0: 0x7ff68e6f1227 wndproc_exception.cpp.tmp.exe
> ->  0x7ff68e6f1227: movl   $0x1, (%rax)
> 0x7ff68e6f122d: movq   $0x0, 0x50(%rsp)
> 0x7ff68e6f1236: jmp0x7ff68e6f1259
> 0x7ff68e6f123b: movq   0x48(%rsp), %r9
> Process 3168 launched: 
> 'C:\dev\llvm-project\llvm\build-msvc\tools\lldb\test\Shell\Process\Windows\Output\wndproc_exception.cpp.tmp.exe'
>  (x86_64)
> ```
> I guess we could check for `Exception 0xc41d encountered` maybe, although 
> I'm afraid of making the testcase unnecessarily brittle too.
If running with lldb-server enabled, it prints a different exception code, 
`0xc005` (which is `STATUS_ACCESS_VIOLATION`) which probably is the proper 
nested exception, while without lldb-server, it prints `0xc41d` 
(`STATUS_FATAL_USER_CALLBACK_EXCEPTION`).

So for the purpose of this testcase, just to make sure that it doesn't crash in 
this case, it'd be better to not specify exactly which exception code is to be 
returned.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128410

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

[Lldb-commits] [lldb] 5e7ddb0 - Revert "[LLDB] Handle DIE with DW_AT_low_pc and empty ranges"

[David Spickett via lldb-commits]

Author: David Spickett
Date: 2022-06-23T10:33:05Z
New Revision: 5e7ddb0ddfe81b704dd52f725e159fec7e20eba5

URL: 
https://github.com/llvm/llvm-project/commit/5e7ddb0ddfe81b704dd52f725e159fec7e20eba5
DIFF: 
https://github.com/llvm/llvm-project/commit/5e7ddb0ddfe81b704dd52f725e159fec7e20eba5.diff

LOG: Revert "[LLDB] Handle DIE with DW_AT_low_pc and empty ranges"

This reverts commit 1beededc0e7d86d09cee972f0b9f0030a139cab4.

Due to failures on the Arm/AArch64 build bots:
https://lab.llvm.org/buildbot/#/builders/96/builds/25032

Added: 


Modified: 
lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp

Removed: 
lldb/test/Shell/Commands/Inputs/dwarf4-low-pc-ranges-inlining.s
lldb/test/Shell/Commands/Inputs/dwarf5-low-pc-ranges-inlining.s
lldb/test/Shell/Commands/dwarf4-low-pc-ranges-inlining.test
lldb/test/Shell/Commands/dwarf5-low-pc-ranges-inlining.test



diff  --git a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp 
b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
index 7be67f83add3d..c0bf13e0281d3 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
@@ -1279,8 +1279,6 @@ size_t SymbolFileDWARF::ParseBlocksRecursive(
 const size_t num_ranges = ranges.GetSize();
 for (size_t i = 0; i < num_ranges; ++i) {
   const DWARFRangeList::Entry &range = ranges.GetEntryRef(i);
-  if (range.GetByteSize() == 0)
-continue;
   const addr_t range_base = range.GetRangeBase();
   if (range_base >= subprogram_low_pc)
 block->AddRange(Block::Range(range_base - subprogram_low_pc,

diff  --git a/lldb/test/Shell/Commands/Inputs/dwarf4-low-pc-ranges-inlining.s 
b/lldb/test/Shell/Commands/Inputs/dwarf4-low-pc-ranges-inlining.s
deleted file mode 100644
index 458e26250b429..0
--- a/lldb/test/Shell/Commands/Inputs/dwarf4-low-pc-ranges-inlining.s
+++ /dev/null
@@ -1,369 +0,0 @@
-
-# Manually modified to have DW_AT_ranges point to end list.
-# int helper(int i) {
-#   return ++i;
-# }
-#
-# int main(int argc, char *argv[]) {
-#   return helper(argc);
-# }
-
-# Manually modified DW_TAG_inlined_subroutine to have DW_AT_low_pc with value 
0,
-# and DW_AT_ranges to point to end ranges list.
-
-   .text
-   .file   "main.cpp"
-   .section.text._Z6helperi,"ax",@progbits
-   .globl  _Z6helperi  # -- Begin function _Z6helperi
-   .p2align4, 0x90
-   .type   _Z6helperi,@function
-_Z6helperi: # @_Z6helperi
-.Lfunc_begin0:
-   .file   1 "/home/test" "main.cpp"
-   .loc1 1 0   # main.cpp:1:0
-   .cfi_startproc
-# %bb.0:# %entry
-   #DEBUG_VALUE: helper:i <- $edi
-# kill: def $edi killed $edi def $rdi
-   .loc1 2 10 prologue_end # main.cpp:2:10
-   leal1(%rdi), %eax
-.Ltmp0:
-   #DEBUG_VALUE: helper:i <- $eax
-   .loc1 2 3 is_stmt 0 # main.cpp:2:3
-   retq
-.Ltmp1:
-.Lfunc_end0:
-   .size   _Z6helperi, .Lfunc_end0-_Z6helperi
-   .cfi_endproc
-# -- End function
-   .section.text.main,"ax",@progbits
-   .globl  main# -- Begin function main
-   .p2align4, 0x90
-   .type   main,@function
-main:   # @main
-.Lfunc_begin1:
-   .loc1 5 0 is_stmt 1 # main.cpp:5:0
-   .cfi_startproc
-# %bb.0:# %entry
-   #DEBUG_VALUE: main:argc <- $edi
-   #DEBUG_VALUE: main:argv <- $rsi
-   #DEBUG_VALUE: helper:i <- $edi
-# kill: def $edi killed $edi def $rdi
-   .loc1 2 10 prologue_end # main.cpp:2:10
-   leal1(%rdi), %eax
-.Ltmp2:
-   #DEBUG_VALUE: helper:i <- $eax
-   .loc1 6 3   # main.cpp:6:3
-   retq
-.Ltmp3:
-.Lfunc_end1:
-   .size   main, .Lfunc_end1-main
-   .cfi_endproc
-# -- End function
-   .section.debug_loc,"",@progbits
-.Ldebug_loc0:
-   .quad   -1
-   .quad   .Lfunc_begin0   #   base address
-   .quad   .Lfunc_begin0-.Lfunc_begin0
-   .quad   .Ltmp0-.Lfunc_begin0
-   .short  1   # Loc expr size
-   .byte   85  # super-register DW_OP_reg5
-   .quad   .Ltmp0-.Lfunc_begin0
-   .quad   .Lfunc_end0-.Lfunc_begin0
-   .short  1   # Loc expr size
-   .byte   80  # super-register DW_OP_reg0
-   .quad   0
-   .quad   0
-.Ldebug_loc1:
-   .quad   -1
-   .quad   .Lfunc_beg

[Lldb-commits] [PATCH] D127889: [LLDB] Handle DIE with DW_AT_low_pc and empty ranges

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added a comment.

I've reverted this due to failures on our bots: 
https://lab.llvm.org/buildbot/#/builders/96/builds/25032/steps/6/logs/stdio

  /usr/bin/ld: 
/home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/build/tools/lldb/test/Shell/Commands/Output/dwarf5-low-pc-ranges-inlining.test.tmp.dir/main5.o:
 Relocations in generic ELF (EM: 62)
  /usr/bin/ld: 
/home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/build/tools/lldb/test/Shell/Commands/Output/dwarf5-low-pc-ranges-inlining.test.tmp.dir/main5.o:
 error adding symbols: file in wrong format
  collect2: error: ld returned 1 exit status

I think that the system ld is expecting an AArch64 format object here. Perhaps 
you could use lld instead given that we always build that when running the lldb 
tests?

Also I'm pretty sure you'll need to require the x86 target in llvm as well. The 
test will fail if this is an AArch64 target only build for example.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127889

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

[Lldb-commits] [PATCH] D128410: [lldb] Add a testcase for nested exceptions on Windows

[Martin Storsjö via Phabricator via lldb-commits]

mstorsjo added a comment.

In D128410#3604083 , @mstorsjo wrote:

> I'm not all that familiar with exactly what happens in these APIs here that 
> makes this a nested exception and what other non-GUI APIs would produce the 
> same effect. Maybe any API where Windows system code calls back to a user 
> provided callback? Does @stella.stamenova know?

FWIW, I tested with doing a similar crash in the callback from 
`EnumerateLoadedModules`, but that doesn't trigger the same case.

I tested running the lldb tests within docker, which is a headless windows 
environment, and this test does indeed fail there (it hangs).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128410

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

[Lldb-commits] [PATCH] D128268: [lldb] Fix reading i686-windows executables with GNU environment

[Martin Storsjö via Phabricator via lldb-commits]

mstorsjo added a subscriber: zturner.
mstorsjo added a comment.

In D128268#3604081 , @labath wrote:

>> If we'd just set this to the baseline, i386, would that have any effect for 
>> how lldb e.g. is able to disassemble/interpret instructions that don't exist 
>> in the i386 baseline architecture?
>
> It should not have any effect (if it does, that's a separate fix). In the 
> disassembler, we explicitly enable the latest instruction set, and I can't 
> think of anything else that would be impacted by it.

Thanks - I did some cursory testing with removing the extra i686 everywhere, 
and at least on a quick test, it seems to work just fine (and requires a minor 
adjustment to only one testcase).

I found that this duality was introduced in 
5e6f45201f0b62c1e7a24fc396f3ea6e10dc880d / D7120 
 and ad587ae4ca143d388c0ec4ef2faa1b5eddedbf67 / 
D4658  (CC @zturner), what do you make out of 
the reasonings in those commits?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128268

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

[Lldb-commits] [PATCH] D128410: [lldb] Add a testcase for nested exceptions on Windows

[Alvin Wong via Phabricator via lldb-commits]

alvinhochun added a comment.

It may be possible to stuff a pointer to an `EXCEPTION_RECORD` into another 
`EXCEPTION_RECORD` and use `RtlRaiseException` to generate the exception, but 
you'll have to test how it actually works.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128410

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

[Lldb-commits] [PATCH] D128410: [lldb] Add a testcase for nested exceptions on Windows

[Alvin Wong via Phabricator via lldb-commits]

alvinhochun added inline comments.



Comment at: lldb/test/Shell/Process/Windows/wndproc_exception.cpp:7
+// RUN: %clangxx_host -o %t.exe -luser32 -v -- %s
+// RUN: %lldb -f %t.exe -o "run"
+

mstorsjo wrote:
> mstorsjo wrote:
> > labath wrote:
> > > Is there something reasonable we could assert here? The process exit 
> > > status for instance?
> > This in itself requires the `%lldb` command to succeed - the exit status 
> > for each `RUN` line needs to be successful (unless a failure is expected 
> > and it can be inverted with the `not` command). Or did you mean checking 
> > the process exit code of the child process (that intentionally does crash)?
> > 
> > When this test case is run, it prints the following to the terminal:
> > ```
> > (lldb) run
> > Process 3168 stopped
> > * thread #1, stop reason = Exception 0xc41d encountered at address 
> > 0x7ff68e6f1227
> > frame #0: 0x7ff68e6f1227 wndproc_exception.cpp.tmp.exe
> > ->  0x7ff68e6f1227: movl   $0x1, (%rax)
> > 0x7ff68e6f122d: movq   $0x0, 0x50(%rsp)
> > 0x7ff68e6f1236: jmp0x7ff68e6f1259
> > 0x7ff68e6f123b: movq   0x48(%rsp), %r9
> > Process 3168 launched: 
> > 'C:\dev\llvm-project\llvm\build-msvc\tools\lldb\test\Shell\Process\Windows\Output\wndproc_exception.cpp.tmp.exe'
> >  (x86_64)
> > ```
> > I guess we could check for `Exception 0xc41d encountered` maybe, 
> > although I'm afraid of making the testcase unnecessarily brittle too.
> If running with lldb-server enabled, it prints a different exception code, 
> `0xc005` (which is `STATUS_ACCESS_VIOLATION`) which probably is the 
> proper nested exception, while without lldb-server, it prints `0xc41d` 
> (`STATUS_FATAL_USER_CALLBACK_EXCEPTION`).
> 
> So for the purpose of this testcase, just to make sure that it doesn't crash 
> in this case, it'd be better to not specify exactly which exception code is 
> to be returned.
It may be that lldb-server is catching the first chance 
`STATUS_ACCESS_VIOLATION` exception before the exception is passed to the 
exception handler...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128410

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

[Lldb-commits] [PATCH] D127889: [LLDB] Handle DIE with DW_AT_low_pc and empty ranges

[Alexander Yermolovich via Phabricator via lldb-commits]

ayermolo added a comment.

In D127889#3604421 , @DavidSpickett 
wrote:

> I've reverted this due to failures on our bots: 
> https://lab.llvm.org/buildbot/#/builders/96/builds/25032/steps/6/logs/stdio
>
>   /usr/bin/ld: 
> /home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/build/tools/lldb/test/Shell/Commands/Output/dwarf5-low-pc-ranges-inlining.test.tmp.dir/main5.o:
>  Relocations in generic ELF (EM: 62)
>   /usr/bin/ld: 
> /home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/build/tools/lldb/test/Shell/Commands/Output/dwarf5-low-pc-ranges-inlining.test.tmp.dir/main5.o:
>  error adding symbols: file in wrong format
>   collect2: error: ld returned 1 exit status
>
> I think that the system ld is expecting an AArch64 format object here. 
> Perhaps you could use lld instead given that we always build that when 
> running the lldb tests?
>
> Also I'm pretty sure you'll need to require the x86 target in llvm as well. 
> The test will fail if this is an AArch64 target only build for example.

Ah thanks. Will make changes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127889

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

[Lldb-commits] [PATCH] D128410: [lldb] Add a testcase for nested exceptions on Windows

[Alvin Wong via Phabricator via lldb-commits]

alvinhochun added inline comments.



Comment at: lldb/test/Shell/Process/Windows/wndproc_exception.cpp:7
+// RUN: %clangxx_host -o %t.exe -luser32 -v -- %s
+// RUN: %lldb -f %t.exe -o "run"
+

alvinhochun wrote:
> mstorsjo wrote:
> > mstorsjo wrote:
> > > labath wrote:
> > > > Is there something reasonable we could assert here? The process exit 
> > > > status for instance?
> > > This in itself requires the `%lldb` command to succeed - the exit status 
> > > for each `RUN` line needs to be successful (unless a failure is expected 
> > > and it can be inverted with the `not` command). Or did you mean checking 
> > > the process exit code of the child process (that intentionally does 
> > > crash)?
> > > 
> > > When this test case is run, it prints the following to the terminal:
> > > ```
> > > (lldb) run
> > > Process 3168 stopped
> > > * thread #1, stop reason = Exception 0xc41d encountered at address 
> > > 0x7ff68e6f1227
> > > frame #0: 0x7ff68e6f1227 wndproc_exception.cpp.tmp.exe
> > > ->  0x7ff68e6f1227: movl   $0x1, (%rax)
> > > 0x7ff68e6f122d: movq   $0x0, 0x50(%rsp)
> > > 0x7ff68e6f1236: jmp0x7ff68e6f1259
> > > 0x7ff68e6f123b: movq   0x48(%rsp), %r9
> > > Process 3168 launched: 
> > > 'C:\dev\llvm-project\llvm\build-msvc\tools\lldb\test\Shell\Process\Windows\Output\wndproc_exception.cpp.tmp.exe'
> > >  (x86_64)
> > > ```
> > > I guess we could check for `Exception 0xc41d encountered` maybe, 
> > > although I'm afraid of making the testcase unnecessarily brittle too.
> > If running with lldb-server enabled, it prints a different exception code, 
> > `0xc005` (which is `STATUS_ACCESS_VIOLATION`) which probably is the 
> > proper nested exception, while without lldb-server, it prints `0xc41d` 
> > (`STATUS_FATAL_USER_CALLBACK_EXCEPTION`).
> > 
> > So for the purpose of this testcase, just to make sure that it doesn't 
> > crash in this case, it'd be better to not specify exactly which exception 
> > code is to be returned.
> It may be that lldb-server is catching the first chance 
> `STATUS_ACCESS_VIOLATION` exception before the exception is passed to the 
> exception handler...
If you test with windbg or gdb, you will get the access violation / sigsegv 
first, and only after continuing the debuggee you will get the `0xc41d` 
exception.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128410

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

[Lldb-commits] [PATCH] D127500: [lldb] [llgs] Make `k` kill all processes, and fix multiple exits

[Michał Górny via Phabricator via lldb-commits]

mgorny updated this revision to Diff 439405.
mgorny marked an inline comment as done.
mgorny added a comment.

Limited variables passed into the lambda. Reworked starting stdio forwarding 
per the suggestion.

TODO: rework stopping.


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

https://reviews.llvm.org/D127500

Files:
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
  lldb/test/API/tools/lldb-server/TestGdbRemoteFork.py

Index: lldb/test/API/tools/lldb-server/TestGdbRemoteFork.py
===
--- lldb/test/API/tools/lldb-server/TestGdbRemoteFork.py
+++ lldb/test/API/tools/lldb-server/TestGdbRemoteFork.py
@@ -262,3 +262,37 @@
 "send packet: $Eff#00",
 ], True)
 self.expect_gdbremote_sequence()
+
+@add_test_categories(["fork"])
+def test_kill_all(self):
+self.build()
+self.prep_debug_monitor_and_inferior(inferior_args=["fork"])
+self.add_qSupported_packets(["multiprocess+",
+ "fork-events+"])
+ret = self.expect_gdbremote_sequence()
+self.assertIn("fork-events+", ret["qSupported_response"])
+self.reset_test_sequence()
+
+# continue and expect fork
+self.test_sequence.add_log_lines([
+"read packet: $c#00",
+{"direction": "send", "regex": self.fork_regex.format("fork"),
+ "capture": self.fork_capture},
+], True)
+ret = self.expect_gdbremote_sequence()
+parent_pid = ret["parent_pid"]
+child_pid = ret["child_pid"]
+self.reset_test_sequence()
+
+exit_regex = "[$]X09;process:([0-9a-f]+)#.*"
+self.test_sequence.add_log_lines([
+# kill all processes
+"read packet: $k#00",
+{"direction": "send", "regex": exit_regex,
+ "capture": {1: "pid1"}},
+{"direction": "send", "regex": exit_regex,
+ "capture": {1: "pid2"}},
+], True)
+ret = self.expect_gdbremote_sequence()
+self.assertEqual(set([ret["pid1"], ret["pid2"]]),
+ set([parent_pid, child_pid]))
Index: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
===
--- lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
+++ lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
@@ -1041,17 +1041,26 @@
   __FUNCTION__, process->GetID());
   }
 
-  // Close the pipe to the inferior terminal i/o if we launched it and set one
-  // up.
-  MaybeCloseInferiorTerminalConnection();
-
-  // When running in non-stop mode, wait for the vStopped to clear
-  // the notification queue.
-  if (!m_non_stop) {
-// We are ready to exit the debug monitor.
-m_exit_now = true;
-m_mainloop.RequestTermination();
-  }
+  if (m_current_process == process)
+m_current_process = nullptr;
+  if (m_continue_process == process)
+m_continue_process = nullptr;
+
+  lldb::pid_t pid = process->GetID();
+  m_mainloop.AddPendingCallback([this, pid](MainLoopBase &loop) {
+m_debugged_processes.erase(pid);
+// When running in non-stop mode, wait for the vStopped to clear
+// the notification queue.
+if (m_debugged_processes.empty() && !m_non_stop) {
+  // Close the pipe to the inferior terminal i/o if we launched it and set
+  // one up.
+  MaybeCloseInferiorTerminalConnection();
+
+  // We are ready to exit the debug monitor.
+  m_exit_now = true;
+  loop.RequestTermination();
+}
+  });
 }
 
 void GDBRemoteCommunicationServerLLGS::HandleInferiorState_Stopped(
@@ -1094,7 +1103,6 @@
 
   switch (state) {
   case StateType::eStateRunning:
-StartSTDIOForwarding();
 break;
 
   case StateType::eStateStopped:
@@ -1219,7 +1227,7 @@
 return;
 
   Status error;
-  lldbassert(!m_stdio_handle_up);
+  assert(!m_stdio_handle_up);
   m_stdio_handle_up = m_mainloop.RegisterReadObject(
   m_stdio_communication.GetConnection()->GetReadObject(),
   [this](MainLoopBase &) { SendProcessOutput(); }, error);
@@ -1228,14 +1236,27 @@
 // Not much we can do about the failure. Log it and continue without
 // forwarding.
 if (Log *log = GetLog(LLDBLog::Process))
-  LLDB_LOGF(log,
-"GDBRemoteCommunicationServerLLGS::%s Failed to set up stdio "
-"forwarding: %s",
-__FUNCTION__, error.AsCString());
+  LLDB_LOG(log, "Failed to set up stdio forwarding: {0}", error);
   }
 }
 
 void GDBRemoteCommunicationServerLLGS::StopSTDIOForwarding() {
+  Log *log = GetLog(LLDBLog::Process);
+
+  if (bool(m_extensions_supported &
+   NativeProcessProtocol::Extension::multiprocess)) {
+// Leave stdio forwarding open if there's at least one running process
+// still.
+for (auto &x : m_debugged_processes) {

[Lldb-commits] [PATCH] D128450: [lldb/Fuzzer] Have target fuzzer write artifacts to specific directory

[Chelsea Cassanova via Phabricator via lldb-commits]

cassanova created this revision.
cassanova added reviewers: JDevlieghere, mib.
cassanova added a project: LLDB.
Herald added a subscriber: mgorny.
Herald added a project: All.
cassanova requested review of this revision.
Herald added a subscriber: lldb-commits.

This makes the LLDB target fuzzer write its fuzzer artifacts to its own 
directory in the build directory. It also adds an artifact prefix to make it 
easier to tell which fuzzer wrote the artifact.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D128450

Files:
  lldb/tools/lldb-fuzzer/lldb-target-fuzzer/CMakeLists.txt


Index: lldb/tools/lldb-fuzzer/lldb-target-fuzzer/CMakeLists.txt
===
--- lldb/tools/lldb-fuzzer/lldb-target-fuzzer/CMakeLists.txt
+++ lldb/tools/lldb-fuzzer/lldb-target-fuzzer/CMakeLists.txt
@@ -17,7 +17,9 @@
 
   add_custom_target(fuzz-lldb-target
 COMMENT "Running the LLDB target fuzzer..."
-COMMAND cd ${CMAKE_CURRENT_SOURCE_DIR} && $
+COMMAND mkdir -p ${CMAKE_BINARY_DIR}/fuzzer-artifacts/target-artifacts &&
+cd ${CMAKE_BINARY_DIR}/fuzzer-artifacts/target-artifacts
+&& $ -artifact_prefix=target-
 USES_TERMINAL
 )
 endif()


Index: lldb/tools/lldb-fuzzer/lldb-target-fuzzer/CMakeLists.txt
===
--- lldb/tools/lldb-fuzzer/lldb-target-fuzzer/CMakeLists.txt
+++ lldb/tools/lldb-fuzzer/lldb-target-fuzzer/CMakeLists.txt
@@ -17,7 +17,9 @@
 
   add_custom_target(fuzz-lldb-target
 COMMENT "Running the LLDB target fuzzer..."
-COMMAND cd ${CMAKE_CURRENT_SOURCE_DIR} && $
+COMMAND mkdir -p ${CMAKE_BINARY_DIR}/fuzzer-artifacts/target-artifacts &&
+cd ${CMAKE_BINARY_DIR}/fuzzer-artifacts/target-artifacts
+&& $ -artifact_prefix=target-
 USES_TERMINAL
 )
 endif()
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D127500: [lldb] [llgs] Make `k` kill all processes, and fix multiple exits

[Michał Górny via Phabricator via lldb-commits]

mgorny added inline comments.



Comment at: 
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:1196
+  if (!bool(m_extensions_supported &
+NativeProcessProtocol::Extension::multiprocess))
+assert(!m_stdio_handle_up);

labath wrote:
> mgorny wrote:
> > labath wrote:
> > > I don't think this is right. I believe the important distinction is 
> > > whether we have non-stop mode enabled or not, because in all-stop mode we 
> > > are not able to send stdio while stopped, regardless of how many 
> > > processes we're debugging.
> > Well, the code exploded in all-stop mode as well because in order to kill 
> > multiple processes, we effectively resume them all. I suppose there could 
> > be a way around it (synchronously killing one process after another and 
> > waiting for everything to happen) but I'm not convinced this is really 
> > worth the added complexity.
> I don't think this needs to be complex at all. What we need to basically do, 
> is call StartSTDIOForwarding whenever the protocol allows us to send stdio, 
> and StopSTDIOForwarding when we cannot. When we supported just a single 
> process, the easiest way to achieve this was to hook into the started/stopped 
> events of that process. Now that's no longer true, so maybe we just need to 
> hook it up elsewhere.
> 
> I think starting could be done directly from the packet handlers 
> (c/s/vCont/...). And those handlers already have to check for nonstop mode, 
> so any deviations could be handled there:
> ```
> if (all_stop) {
>   StartSTDIOForwarding(); // Can forward from now until the stop-reply packet 
> is send
>   return Success;
> } else {
>   SendOKResponse();
>   // Can we forward now? Or maybe it can be sent all the time?
> }
> ```
> 
> Stopping could probably stay coupled with the sending of the stop-reply 
> packet (i.e., handling of the process event), since it's the stop reply which 
> determines whether the stdio packet can be sent.
Thanks, this makes sense. Good news is that it seems that I can just wire it 
into our current `SendContinueSuccessResponse()`, unless I've missed some other 
case (I've grepped for everything calling `Resume()` or `Kill()`). So far I 
didn't special-case non-stop mode, as I still need to rework `O` packets for it.

That said, do we want to enable forwarding for kill actions at all? I suppose 
this was kinda implicit due to the whole Linux `PTRACE_EVENT_EXIT` ping-pong 
but I honestly doubt any output can happen there.


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

https://reviews.llvm.org/D127500

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

[Lldb-commits] [PATCH] D128450: [lldb/Fuzzer] Have target fuzzer write artifacts to specific directory

[Jonas Devlieghere via Phabricator via lldb-commits]

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

Instead of bundling together multiple shell commands into a single one, you 
should break them down so CMake can have the build system (e.g. ninja) schedule 
the different parts of it.

To create the directory, you can either create a separate target and make the 
`fuzz-lldb-target` depend on it:

  add_custom_target(foo ALL
  COMMAND ${CMAKE_COMMAND} -E make_directory ${directory})

But even better would. be to make it a pre-build command for the 
fuzz-lldb-target:

  add_custom_command(TARGET fuzz-lldb-target PRE_BUILD COMMAND ${CMAKE_COMMAND} 
-E make_directory ${directory})

Then you can avoid the `cd` by using the `WORKING_DIRECTORY` in 
`add_custom_target`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128450

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

[Lldb-commits] [lldb] 09dea54 - [lldb] Support a buffered logging mode

[Jonas Devlieghere via lldb-commits]

Author: Jonas Devlieghere
Date: 2022-06-23T09:12:01-07:00
New Revision: 09dea546692f4e9f32bf16f1a9d5d0de52a64691

URL: 
https://github.com/llvm/llvm-project/commit/09dea546692f4e9f32bf16f1a9d5d0de52a64691
DIFF: 
https://github.com/llvm/llvm-project/commit/09dea546692f4e9f32bf16f1a9d5d0de52a64691.diff

LOG: [lldb] Support a buffered logging mode

This patch adds a buffered logging mode to lldb. A buffer size can be
passed to `log enable` with the -b flag. If no buffer size is specified,
logging is unbuffered.

Differential revision: https://reviews.llvm.org/D127986

Added: 


Modified: 
lldb/include/lldb/Core/Debugger.h
lldb/include/lldb/Utility/Log.h
lldb/source/API/SBDebugger.cpp
lldb/source/Commands/CommandObjectLog.cpp
lldb/source/Commands/Options.td
lldb/source/Core/Debugger.cpp
lldb/source/Utility/Log.cpp
lldb/tools/lldb-test/lldb-test.cpp

Removed: 




diff  --git a/lldb/include/lldb/Core/Debugger.h 
b/lldb/include/lldb/Core/Debugger.h
index 4cdc6b6237c7e..b5e6c30f5f77a 100644
--- a/lldb/include/lldb/Core/Debugger.h
+++ b/lldb/include/lldb/Core/Debugger.h
@@ -245,7 +245,7 @@ class Debugger : public 
std::enable_shared_from_this,
   bool EnableLog(llvm::StringRef channel,
  llvm::ArrayRef categories,
  llvm::StringRef log_file, uint32_t log_options,
- llvm::raw_ostream &error_stream);
+ size_t buffer_size, llvm::raw_ostream &error_stream);
 
   void SetLoggingCallback(lldb::LogOutputCallback log_callback, void *baton);
 

diff  --git a/lldb/include/lldb/Utility/Log.h b/lldb/include/lldb/Utility/Log.h
index 806eb390773b9..3fd474d834489 100644
--- a/lldb/include/lldb/Utility/Log.h
+++ b/lldb/include/lldb/Utility/Log.h
@@ -58,9 +58,11 @@ class LogHandler {
 
 class StreamLogHandler : public LogHandler {
 public:
-  StreamLogHandler(int fd, bool should_close, bool unbuffered = true);
+  StreamLogHandler(int fd, bool should_close, size_t buffer_size = 0);
+  ~StreamLogHandler() override;
 
   void Emit(llvm::StringRef message) override;
+  void Flush();
 
 private:
   llvm::raw_fd_ostream m_stream;

diff  --git a/lldb/source/API/SBDebugger.cpp b/lldb/source/API/SBDebugger.cpp
index 37469989dc5b5..8aa2c74d9e378 100644
--- a/lldb/source/API/SBDebugger.cpp
+++ b/lldb/source/API/SBDebugger.cpp
@@ -1621,7 +1621,7 @@ bool SBDebugger::EnableLog(const char *channel, const 
char **categories) {
 std::string error;
 llvm::raw_string_ostream error_stream(error);
 return m_opaque_sp->EnableLog(channel, GetCategoryArray(categories), "",
-  log_options, error_stream);
+  log_options, /*buffer_size=*/0, 
error_stream);
   } else
 return false;
 }

diff  --git a/lldb/source/Commands/CommandObjectLog.cpp 
b/lldb/source/Commands/CommandObjectLog.cpp
index c8df30849628a..190b262ed8edc 100644
--- a/lldb/source/Commands/CommandObjectLog.cpp
+++ b/lldb/source/Commands/CommandObjectLog.cpp
@@ -11,6 +11,7 @@
 #include "lldb/Host/OptionParser.h"
 #include "lldb/Interpreter/CommandReturnObject.h"
 #include "lldb/Interpreter/OptionArgParser.h"
+#include "lldb/Interpreter/OptionValueUInt64.h"
 #include "lldb/Interpreter/Options.h"
 #include "lldb/Utility/Args.h"
 #include "lldb/Utility/FileSpec.h"
@@ -21,7 +22,7 @@
 using namespace lldb;
 using namespace lldb_private;
 
-#define LLDB_OPTIONS_log
+#define LLDB_OPTIONS_log_enable
 #include "CommandOptions.inc"
 
 /// Common completion logic for log enable/disable.
@@ -89,6 +90,10 @@ class CommandObjectLogEnable : public CommandObjectParsed {
 log_file.SetFile(option_arg, FileSpec::Style::native);
 FileSystem::Instance().Resolve(log_file);
 break;
+  case 'b':
+error =
+buffer_size.SetValueFromString(option_arg, eVarSetOperationAssign);
+break;
   case 't':
 log_options |= LLDB_LOG_OPTION_THREADSAFE;
 break;
@@ -125,16 +130,16 @@ class CommandObjectLogEnable : public CommandObjectParsed 
{
 
 void OptionParsingStarting(ExecutionContext *execution_context) override {
   log_file.Clear();
+  buffer_size.Clear();
   log_options = 0;
 }
 
 llvm::ArrayRef GetDefinitions() override {
-  return llvm::makeArrayRef(g_log_options);
+  return llvm::makeArrayRef(g_log_enable_options);
 }
 
-// Instance variables to hold the values for command options.
-
 FileSpec log_file;
+OptionValueUInt64 buffer_size;
 uint32_t log_options = 0;
   };
 
@@ -164,9 +169,9 @@ class CommandObjectLogEnable : public CommandObjectParsed {
 
 std::string error;
 llvm::raw_string_ostream error_stream(error);
-bool success =
-GetDebugger().EnableLog(channel, args.GetArgumentArrayRef(), log_file,
-m_options.log_options, error_stream);
+bool success = GetDebugger().EnableLog(
+  

[Lldb-commits] [lldb] 70841b9 - [lldb] Make thread safety the responsibility of the log handlers

[Jonas Devlieghere via lldb-commits]

Author: Jonas Devlieghere
Date: 2022-06-23T09:12:05-07:00
New Revision: 70841b97eb2e5c68d46b70579877e378511a47b4

URL: 
https://github.com/llvm/llvm-project/commit/70841b97eb2e5c68d46b70579877e378511a47b4
DIFF: 
https://github.com/llvm/llvm-project/commit/70841b97eb2e5c68d46b70579877e378511a47b4.diff

LOG: [lldb] Make thread safety the responsibility of the log handlers

Drop the thread-safe flag and make the locking strategy the
responsibility of the individual log handler.

Previously we got away with a non-thread safe mode because we were using
unbuffered streams that rely on the underlying syscalls/OS for
synchronization. With the introduction of log handlers, we can have
arbitrary logic involved in writing out the logs. With this patch the
log handlers can pick the most appropriate locking strategy for their
particular implementation.

Differential revision: https://reviews.llvm.org/D127922

Added: 


Modified: 
lldb/include/lldb/Utility/Log.h
lldb/source/Commands/CommandObjectLog.cpp
lldb/source/Commands/Options.td
lldb/source/Core/Debugger.cpp
lldb/source/Utility/Log.cpp
lldb/test/API/commands/log/basic/TestLogging.py
lldb/unittests/Utility/LogTest.cpp

Removed: 




diff  --git a/lldb/include/lldb/Utility/Log.h b/lldb/include/lldb/Utility/Log.h
index 3fd474d834489..4772291e43079 100644
--- a/lldb/include/lldb/Utility/Log.h
+++ b/lldb/include/lldb/Utility/Log.h
@@ -33,7 +33,6 @@ namespace llvm {
 class raw_ostream;
 }
 // Logging Options
-#define LLDB_LOG_OPTION_THREADSAFE (1u << 0)
 #define LLDB_LOG_OPTION_VERBOSE (1u << 1)
 #define LLDB_LOG_OPTION_PREPEND_SEQUENCE (1u << 3)
 #define LLDB_LOG_OPTION_PREPEND_TIMESTAMP (1u << 4)
@@ -50,10 +49,6 @@ class LogHandler {
 public:
   virtual ~LogHandler() = default;
   virtual void Emit(llvm::StringRef message) = 0;
-  void EmitThreadSafe(llvm::StringRef message);
-
-private:
-  std::mutex m_mutex;
 };
 
 class StreamLogHandler : public LogHandler {
@@ -65,6 +60,7 @@ class StreamLogHandler : public LogHandler {
   void Flush();
 
 private:
+  std::mutex m_mutex;
   llvm::raw_fd_ostream m_stream;
 };
 
@@ -91,6 +87,7 @@ class RotatingLogHandler : public LogHandler {
   size_t GetNumMessages() const;
   size_t GetFirstMessageIndex() const;
 
+  mutable std::mutex m_mutex;
   std::unique_ptr m_messages;
   const size_t m_size = 0;
   size_t m_next_index = 0;

diff  --git a/lldb/source/Commands/CommandObjectLog.cpp 
b/lldb/source/Commands/CommandObjectLog.cpp
index 190b262ed8edc..91277e33cf360 100644
--- a/lldb/source/Commands/CommandObjectLog.cpp
+++ b/lldb/source/Commands/CommandObjectLog.cpp
@@ -94,9 +94,6 @@ class CommandObjectLogEnable : public CommandObjectParsed {
 error =
 buffer_size.SetValueFromString(option_arg, eVarSetOperationAssign);
 break;
-  case 't':
-log_options |= LLDB_LOG_OPTION_THREADSAFE;
-break;
   case 'v':
 log_options |= LLDB_LOG_OPTION_VERBOSE;
 break;

diff  --git a/lldb/source/Commands/Options.td b/lldb/source/Commands/Options.td
index f3c041eab0a32..b95fc6b1443d8 100644
--- a/lldb/source/Commands/Options.td
+++ b/lldb/source/Commands/Options.td
@@ -433,8 +433,6 @@ let Command = "log enable" in {
 Desc<"Set the destination file to log to.">;
   def log_buffer_size : Option<"buffer", "b">, Group<1>, 
Arg<"UnsignedInteger">,
 Desc<"Set the log to be buffered, using the specified buffer size.">;
-  def log_threadsafe : Option<"threadsafe", "t">, Group<1>,
-Desc<"Enable thread safe logging to avoid interweaved log lines.">;
   def log_verbose : Option<"verbose", "v">, Group<1>,
 Desc<"Enable verbose logging.">;
   def log_sequence : Option<"sequence", "s">, Group<1>,

diff  --git a/lldb/source/Core/Debugger.cpp b/lldb/source/Core/Debugger.cpp
index 0be7748bb326b..fd9679c15c2e3 100644
--- a/lldb/source/Core/Debugger.cpp
+++ b/lldb/source/Core/Debugger.cpp
@@ -1448,8 +1448,7 @@ bool Debugger::EnableLog(llvm::StringRef channel,
   assert(log_handler_sp);
 
   if (log_options == 0)
-log_options =
-LLDB_LOG_OPTION_PREPEND_THREAD_NAME | LLDB_LOG_OPTION_THREADSAFE;
+log_options = LLDB_LOG_OPTION_PREPEND_THREAD_NAME;
 
   return Log::EnableLogChannel(log_handler_sp, log_options, channel, 
categories,
error_stream);

diff  --git a/lldb/source/Utility/Log.cpp b/lldb/source/Utility/Log.cpp
index 37704b7663bbf..4c3e05acd8995 100644
--- a/lldb/source/Utility/Log.cpp
+++ b/lldb/source/Utility/Log.cpp
@@ -317,12 +317,7 @@ void Log::WriteMessage(const std::string &message) {
   auto handler_sp = GetHandler();
   if (!handler_sp)
 return;
-
-  Flags options = GetOptions();
-  if (options.Test(LLDB_LOG_OPTION_THREADSAFE))
-handler_sp->EmitThreadSafe(message);
-  else
-handler_sp->Emit(message);
+  handler_sp->Emit(message);
 }
 
 void Log::Format(llvm::StringRef file, llvm::StringRef function,
@@

[Lldb-commits] [PATCH] D127986: [lldb] Support a buffered logging mode

[Jonas Devlieghere via Phabricator via lldb-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG09dea546692f: [lldb] Support a buffered logging mode 
(authored by JDevlieghere).
Herald added a project: LLDB.

Changed prior to commit:
  https://reviews.llvm.org/D127986?vs=438831&id=439427#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127986

Files:
  lldb/include/lldb/Core/Debugger.h
  lldb/include/lldb/Utility/Log.h
  lldb/source/API/SBDebugger.cpp
  lldb/source/Commands/CommandObjectLog.cpp
  lldb/source/Commands/Options.td
  lldb/source/Core/Debugger.cpp
  lldb/source/Utility/Log.cpp
  lldb/tools/lldb-test/lldb-test.cpp

Index: lldb/tools/lldb-test/lldb-test.cpp
===
--- lldb/tools/lldb-test/lldb-test.cpp
+++ lldb/tools/lldb-test/lldb-test.cpp
@@ -1120,7 +1120,7 @@
   /*add_to_history*/ eLazyBoolNo, Result);
 
   if (!opts::Log.empty())
-Dbg->EnableLog("lldb", {"all"}, opts::Log, 0, errs());
+Dbg->EnableLog("lldb", {"all"}, opts::Log, 0, 0, errs());
 
   if (opts::BreakpointSubcommand)
 return opts::breakpoint::evaluateBreakpoints(*Dbg);
Index: lldb/source/Utility/Log.cpp
===
--- lldb/source/Utility/Log.cpp
+++ lldb/source/Utility/Log.cpp
@@ -339,14 +339,19 @@
   Emit(message);
 }
 
-StreamLogHandler::StreamLogHandler(int fd, bool should_close, bool unbuffered)
-: m_stream(fd, should_close, unbuffered) {}
-
-void StreamLogHandler::Emit(llvm::StringRef message) {
-  m_stream << message;
-  m_stream.flush();
+StreamLogHandler::StreamLogHandler(int fd, bool should_close,
+   size_t buffer_size)
+: m_stream(fd, should_close, buffer_size == 0) {
+  if (buffer_size > 0)
+m_stream.SetBufferSize(buffer_size);
 }
 
+StreamLogHandler::~StreamLogHandler() { Flush(); }
+
+void StreamLogHandler::Flush() { m_stream.flush(); }
+
+void StreamLogHandler::Emit(llvm::StringRef message) { m_stream << message; }
+
 CallbackLogHandler::CallbackLogHandler(lldb::LogOutputCallback callback,
void *baton)
 : m_callback(callback), m_baton(baton) {}
Index: lldb/source/Core/Debugger.cpp
===
--- lldb/source/Core/Debugger.cpp
+++ lldb/source/Core/Debugger.cpp
@@ -1409,7 +1409,7 @@
 bool Debugger::EnableLog(llvm::StringRef channel,
  llvm::ArrayRef categories,
  llvm::StringRef log_file, uint32_t log_options,
- llvm::raw_ostream &error_stream) {
+ size_t buffer_size, llvm::raw_ostream &error_stream) {
   const bool should_close = true;
 
   std::shared_ptr log_handler_sp;
@@ -1420,7 +1420,7 @@
 LLDB_LOG_OPTION_PREPEND_TIMESTAMP | LLDB_LOG_OPTION_PREPEND_THREAD_NAME;
   } else if (log_file.empty()) {
 log_handler_sp = std::make_shared(
-GetOutputFile().GetDescriptor(), !should_close);
+GetOutputFile().GetDescriptor(), !should_close, buffer_size);
   } else {
 auto pos = m_stream_handlers.find(log_file);
 if (pos != m_stream_handlers.end())
@@ -1441,7 +1441,7 @@
   }
 
   log_handler_sp = std::make_shared(
-  (*file)->GetDescriptor(), should_close);
+  (*file)->GetDescriptor(), should_close, buffer_size);
   m_stream_handlers[log_file] = log_handler_sp;
 }
   }
Index: lldb/source/Commands/Options.td
===
--- lldb/source/Commands/Options.td
+++ lldb/source/Commands/Options.td
@@ -428,9 +428,11 @@
 Desc<"Clears the current command history.">;
 }
 
-let Command = "log" in {
+let Command = "log enable" in {
   def log_file : Option<"file", "f">, Group<1>, Arg<"Filename">,
 Desc<"Set the destination file to log to.">;
+  def log_buffer_size : Option<"buffer", "b">, Group<1>, Arg<"UnsignedInteger">,
+Desc<"Set the log to be buffered, using the specified buffer size.">;
   def log_threadsafe : Option<"threadsafe", "t">, Group<1>,
 Desc<"Enable thread safe logging to avoid interweaved log lines.">;
   def log_verbose : Option<"verbose", "v">, Group<1>,
Index: lldb/source/Commands/CommandObjectLog.cpp
===
--- lldb/source/Commands/CommandObjectLog.cpp
+++ lldb/source/Commands/CommandObjectLog.cpp
@@ -11,6 +11,7 @@
 #include "lldb/Host/OptionParser.h"
 #include "lldb/Interpreter/CommandReturnObject.h"
 #include "lldb/Interpreter/OptionArgParser.h"
+#include "lldb/Interpreter/OptionValueUInt64.h"
 #include "lldb/Interpreter/Options.h"
 #include "lldb/Utility/Args.h"
 #include "lldb/Utility/FileSpec.h"
@@ -21,7 +22,7 @@
 using namespace lldb;
 using namespace lldb_private;
 
-#define LLDB_OPTIONS_log
+#define LLDB_OPTIO

[Lldb-commits] [PATCH] D128386: [lldb] Make thread safety the responsibility of the log handlers

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere closed this revision.
JDevlieghere added a comment.

Landed as 70841b97eb2e5c68d46b70579877e378511a47b4 



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

https://reviews.llvm.org/D128386

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

[Lldb-commits] [PATCH] D128366: [lldb] Make Module::LookupInfo::Prune language-agnostic

[Alex Langford via Phabricator via lldb-commits]

bulbazord added a comment.

In D128366#3603943 , @labath wrote:

> How hot is this code? Parsing a demangled name isn't entirely cheap, and the 
> fact that the objc++ class calls back into the c++ version means that the c++ 
> name will be parsed twice.

Not sure, I'll measure this. Also, at least on my machine, the list of language 
plugins includes at least 3 C++ language plugins, so it happens *at least* 
twice...

> Could we move this pruning elsewhere? These values come from the symbol file 
> plugins anyway, and they can do a better job at determining which language 
> does a particular name belong to.
> (OK, they can an also come from the symtab, but there I guess we could infer 
> something from the mangling scheme).

We're specifically pruning the results from a name lookup, so I'm not sure 
where would be a better place to move it. That being said, based on your 
observation, we could try to do a better job of inferring the language a name 
belongs to by looking at the mangling scheme before we look at every single 
language plugin.




Comment at: lldb/include/lldb/Target/Language.h:320
+  virtual bool NamesAreEquivalentWithContext(
+  const ConstString &user_provided_name,
+  const ConstString &full_name_with_context) const {

labath wrote:
> I think we're passing ConstStrings by value these days...
I see. Is there a reason we do that instead of passing by reference?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128366

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

[Lldb-commits] [PATCH] D127500: [lldb] [llgs] Make `k` kill all processes, and fix multiple exits

[Michał Górny via Phabricator via lldb-commits]

mgorny added inline comments.



Comment at: 
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:1196
+  if (!bool(m_extensions_supported &
+NativeProcessProtocol::Extension::multiprocess))
+assert(!m_stdio_handle_up);

mgorny wrote:
> labath wrote:
> > mgorny wrote:
> > > labath wrote:
> > > > I don't think this is right. I believe the important distinction is 
> > > > whether we have non-stop mode enabled or not, because in all-stop mode 
> > > > we are not able to send stdio while stopped, regardless of how many 
> > > > processes we're debugging.
> > > Well, the code exploded in all-stop mode as well because in order to kill 
> > > multiple processes, we effectively resume them all. I suppose there could 
> > > be a way around it (synchronously killing one process after another and 
> > > waiting for everything to happen) but I'm not convinced this is really 
> > > worth the added complexity.
> > I don't think this needs to be complex at all. What we need to basically 
> > do, is call StartSTDIOForwarding whenever the protocol allows us to send 
> > stdio, and StopSTDIOForwarding when we cannot. When we supported just a 
> > single process, the easiest way to achieve this was to hook into the 
> > started/stopped events of that process. Now that's no longer true, so maybe 
> > we just need to hook it up elsewhere.
> > 
> > I think starting could be done directly from the packet handlers 
> > (c/s/vCont/...). And those handlers already have to check for nonstop mode, 
> > so any deviations could be handled there:
> > ```
> > if (all_stop) {
> >   StartSTDIOForwarding(); // Can forward from now until the stop-reply 
> > packet is send
> >   return Success;
> > } else {
> >   SendOKResponse();
> >   // Can we forward now? Or maybe it can be sent all the time?
> > }
> > ```
> > 
> > Stopping could probably stay coupled with the sending of the stop-reply 
> > packet (i.e., handling of the process event), since it's the stop reply 
> > which determines whether the stdio packet can be sent.
> Thanks, this makes sense. Good news is that it seems that I can just wire it 
> into our current `SendContinueSuccessResponse()`, unless I've missed some 
> other case (I've grepped for everything calling `Resume()` or `Kill()`). So 
> far I didn't special-case non-stop mode, as I still need to rework `O` 
> packets for it.
> 
> That said, do we want to enable forwarding for kill actions at all? I suppose 
> this was kinda implicit due to the whole Linux `PTRACE_EVENT_EXIT` ping-pong 
> but I honestly doubt any output can happen there.
Oh, and regarding non-stop mode, I've left it as TODO for now. The whole stdio 
forwarding needs to be fixed for non-stop mode anyway, and since we don't 
expect to have two processes running simultaneously yet, I'm going to look into 
it separately.

That said, this is probably going to be a "LLDB extension". FWICS gdb pretty 
much uses `O` packets only to deliver debugger-specific messages and doesn't 
forward stdio at all, nor special-cases `O` in non-stop mode. My initial idea 
is to always send `O` as asynchronous notifications. I suppose we could then 
enable stdio forwarding as soon as non-stop mode is enabled, and keep it 
enabled until the very end.


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

https://reviews.llvm.org/D127500

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

[Lldb-commits] [PATCH] D127500: [lldb] [llgs] Make `k` kill all processes, and fix multiple exits

[Michał Górny via Phabricator via lldb-commits]

mgorny updated this revision to Diff 439433.
mgorny edited the summary of this revision.
mgorny added a comment.

Remove the extra check from `StopSTDIOForwarding()` — we currently don't have 
to account for multiple processes actually running simultaneously (only the 
weird Linux `PTRACE_EVENT_EXIT` handling), so we can just stop it when the 
first of killed processes exits.


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

https://reviews.llvm.org/D127500

Files:
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
  lldb/test/API/tools/lldb-server/TestGdbRemoteFork.py

Index: lldb/test/API/tools/lldb-server/TestGdbRemoteFork.py
===
--- lldb/test/API/tools/lldb-server/TestGdbRemoteFork.py
+++ lldb/test/API/tools/lldb-server/TestGdbRemoteFork.py
@@ -262,3 +262,37 @@
 "send packet: $Eff#00",
 ], True)
 self.expect_gdbremote_sequence()
+
+@add_test_categories(["fork"])
+def test_kill_all(self):
+self.build()
+self.prep_debug_monitor_and_inferior(inferior_args=["fork"])
+self.add_qSupported_packets(["multiprocess+",
+ "fork-events+"])
+ret = self.expect_gdbremote_sequence()
+self.assertIn("fork-events+", ret["qSupported_response"])
+self.reset_test_sequence()
+
+# continue and expect fork
+self.test_sequence.add_log_lines([
+"read packet: $c#00",
+{"direction": "send", "regex": self.fork_regex.format("fork"),
+ "capture": self.fork_capture},
+], True)
+ret = self.expect_gdbremote_sequence()
+parent_pid = ret["parent_pid"]
+child_pid = ret["child_pid"]
+self.reset_test_sequence()
+
+exit_regex = "[$]X09;process:([0-9a-f]+)#.*"
+self.test_sequence.add_log_lines([
+# kill all processes
+"read packet: $k#00",
+{"direction": "send", "regex": exit_regex,
+ "capture": {1: "pid1"}},
+{"direction": "send", "regex": exit_regex,
+ "capture": {1: "pid2"}},
+], True)
+ret = self.expect_gdbremote_sequence()
+self.assertEqual(set([ret["pid1"], ret["pid2"]]),
+ set([parent_pid, child_pid]))
Index: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
===
--- lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
+++ lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
@@ -1041,17 +1041,26 @@
   __FUNCTION__, process->GetID());
   }
 
-  // Close the pipe to the inferior terminal i/o if we launched it and set one
-  // up.
-  MaybeCloseInferiorTerminalConnection();
-
-  // When running in non-stop mode, wait for the vStopped to clear
-  // the notification queue.
-  if (!m_non_stop) {
-// We are ready to exit the debug monitor.
-m_exit_now = true;
-m_mainloop.RequestTermination();
-  }
+  if (m_current_process == process)
+m_current_process = nullptr;
+  if (m_continue_process == process)
+m_continue_process = nullptr;
+
+  lldb::pid_t pid = process->GetID();
+  m_mainloop.AddPendingCallback([this, pid](MainLoopBase &loop) {
+m_debugged_processes.erase(pid);
+// When running in non-stop mode, wait for the vStopped to clear
+// the notification queue.
+if (m_debugged_processes.empty() && !m_non_stop) {
+  // Close the pipe to the inferior terminal i/o if we launched it and set
+  // one up.
+  MaybeCloseInferiorTerminalConnection();
+
+  // We are ready to exit the debug monitor.
+  m_exit_now = true;
+  loop.RequestTermination();
+}
+  });
 }
 
 void GDBRemoteCommunicationServerLLGS::HandleInferiorState_Stopped(
@@ -1094,7 +1103,6 @@
 
   switch (state) {
   case StateType::eStateRunning:
-StartSTDIOForwarding();
 break;
 
   case StateType::eStateStopped:
@@ -1219,7 +1227,7 @@
 return;
 
   Status error;
-  lldbassert(!m_stdio_handle_up);
+  assert(!m_stdio_handle_up);
   m_stdio_handle_up = m_mainloop.RegisterReadObject(
   m_stdio_communication.GetConnection()->GetReadObject(),
   [this](MainLoopBase &) { SendProcessOutput(); }, error);
@@ -1228,10 +1236,7 @@
 // Not much we can do about the failure. Log it and continue without
 // forwarding.
 if (Log *log = GetLog(LLDBLog::Process))
-  LLDB_LOGF(log,
-"GDBRemoteCommunicationServerLLGS::%s Failed to set up stdio "
-"forwarding: %s",
-__FUNCTION__, error.AsCString());
+  LLDB_LOG(log, "Failed to set up stdio forwarding: {0}", error);
   }
 }
 
@@ -1416,15 +1421,19 @@
 
   StopSTDIOForwarding();
 
-  if (!m_current_process) {
+  if (m_debugged_processes.empty()) {
 LLDB_LOG(log, "No debugged process found.");
 return PacketResult

[Lldb-commits] [PATCH] D127667: [lldb] [llgs] Implement the vKill packet

[Michał Górny via Phabricator via lldb-commits]

mgorny added a comment.

In D127667#3603910 , @labath wrote:

> Do we need the kill command to be asynchronous (return before the process 
> actually dies) for anything. Could we just make it synchronous instead (call 
> wait inside the process plugin, and return the wait result)?

Well, I have two concerns with that:

1. I have no clue about Windows or an environment to test it, so I wouldn't be 
able to update the Windows plugin code.
2. I'm a bit concerned about the Linux `PTRACE_EVENT_EXIT` handling. I mean, I 
suppose it's unlikely for something to go wrong there and cause us to hang but 
I'm not sure.


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

https://reviews.llvm.org/D127667

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

[Lldb-commits] [PATCH] D128450: [lldb/Fuzzer] Have target fuzzer write artifacts to specific directory

[Chelsea Cassanova via Phabricator via lldb-commits]

cassanova added a comment.

This is a lot cleaner than chaining shell commands, I just implemented the 
second solution on my end. To clarify, it would create the directory before 
running the `fuzz-lldb-target` and within the `fuzz-lldb-target` we would just 
change the working directory to the one that the pre-build command created?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128450

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

[Lldb-commits] [PATCH] D128450: [lldb/Fuzzer] Have target fuzzer write artifacts to specific directory

[Med Ismail Bennani via Phabricator via lldb-commits]

mib added a comment.

@cassanova You probably also want to implement this for the 
`command-interpreter-fuzzer` target


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128450

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

[Lldb-commits] [PATCH] D128321: [lldb] Add OSLog log handler

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

In D128321#3603947 , @labath wrote:

> In D128321#3603007 , @JDevlieghere 
> wrote:
>
>> In D128321#3602129 , @clayborg 
>> wrote:
>>
>>> I think we should allow this class to always exist and not conditionally 
>>> compile it in or out. Then we add a new Host.h method to emit a log message 
>>> in "lldb/Host/Host.h" and allow each host OS to emit a message. Maybe there 
>>> is a default implementation that emits the message to stderr, and then we 
>>> override this for in HostInfoMacOSX.h? Then each OS can use their OS 
>>> logging API of choice.
>>
>> Yeah, I considered that, but that would make Utility depend on Host which 
>> would (re)introduce the circular dependency between the two. The alternative 
>> would be to still make the class available unconditionally, but use target 
>> ifdefs to have different implementations.
>
> We already have `Host::SystemLog`. Could we use that? Or change it so that it 
> can be used?

Not in its current state, no. The first problem is that it writes the message 
to the host log channel if it's enabled and in verbose mode. The second problem 
is that on macOS, it writes both to the system log and to stderr (all other 
platforms just write to stderr). If you look at its callers, it's clear that 
this function is really used for error reporting (the fact that it takes two 
log level "warning" and "error") is already a good indication of that. When I 
was looking at it yesterday I was thinking about ripping it out and replacing 
it with the diagnostics I added a little while ago.

> As for the dependencies, I think we could structure things such that the 
> SystemLogHandler lives in the Host module, and the general logging 
> infrastructure is not aware of it. (The only one who would know about it 
> would be the `log enable` command, which would pass it down as a generic 
> reference.)

I also considered that, but that just delays the problem until I want to enable 
this handler for the always-on logging (as outlined in 
https://discourse.llvm.org/c/subprojects/lldb/8). I haven't really looked into 
where that logic would live, but the reproducers lived in Utility so I was 
expecting all of this to be there as well.

So both things combined, I still think the current approach makes the most 
sense. But if we really want this to live in Host (and deal with the dependency 
issues later) I can implement a new `Host::SystemLog` variant that essentially 
does what the SystemLogHander is doing and implement the log handler in Host 
based on that.


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

https://reviews.llvm.org/D128321

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

[Lldb-commits] [PATCH] D128450: [lldb/Fuzzer] Have target fuzzer write artifacts to specific directory

[Chelsea Cassanova via Phabricator via lldb-commits]

cassanova added a comment.

Yes, I can include the command interpreter's cmake file in the diff.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128450

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

[Lldb-commits] [PATCH] D128453: Automate checking for "command that takes no arguments" being passed arguments in CommandObjectParsed

[Jim Ingham via Phabricator via lldb-commits]

jingham created this revision.
jingham added reviewers: JDevlieghere, clayborg, kastiglione, ADodds.
Herald added a project: All.
jingham requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

At present, if a command takes no arguments, it's required to check by hand 
that it hasn't been passed an argument.  That's  error-prone and unnecessary, 
since CommandObject has a way for sub-classes to register what arguments it 
takes, and that could be checked directly after the command line parse.  I only 
do this for CommandObjectParsed.  There isn't enough information to do a 
generic check for CommandObjectRaw, since these are explicitly unstructured 
commands.

This patch has four parts:

1. The tiny bit of code in CommandObjectParsed::Execute that does the check
2. Fixing up the handful of commands that hadn't registered their arguments (up 
to now this was not required)

These were pretty straightforward.  I did have to fix up a bunch of the 
RenderScript commands.  I don't know
who's responsible for the Renderscript plugin these days, so I picked one of 
the more recent contributors to
that file at random...

3. Removing all the ad hoc checks for arguments passed to commands that don't 
take them
4. Fixing up the tests that relied on the exact wording of the ad hoc errors

We can do a lot more with the argument info - automating completions and 
validation of arguments for commands that do take them.  But here I'm only 
dealing with the automatic error generation for commands that take no arguments.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D128453

Files:
  lldb/include/lldb/lldb-enumerations.h
  lldb/source/API/SBCommandInterpreter.cpp
  lldb/source/Commands/CommandObjectCommands.cpp
  lldb/source/Commands/CommandObjectFrame.cpp
  lldb/source/Commands/CommandObjectGUI.cpp
  lldb/source/Commands/CommandObjectPlatform.cpp
  lldb/source/Commands/CommandObjectProcess.cpp
  lldb/source/Commands/CommandObjectQuit.cpp
  lldb/source/Commands/CommandObjectReproducer.cpp
  lldb/source/Commands/CommandObjectSource.cpp
  lldb/source/Commands/CommandObjectTarget.cpp
  lldb/source/Commands/CommandObjectThreadUtil.cpp
  lldb/source/Commands/CommandObjectTrace.cpp
  lldb/source/Commands/CommandObjectType.cpp
  lldb/source/Commands/CommandObjectVersion.cpp
  lldb/source/Interpreter/CommandObject.cpp
  
lldb/source/Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/RenderScriptRuntime.cpp
  
lldb/source/Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/RenderScriptScriptGroup.cpp
  lldb/source/Plugins/Process/MacOSX-Kernel/ProcessKDP.cpp
  lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
  lldb/test/API/commands/gui/invalid-args/TestInvalidArgsGui.py
  lldb/test/API/commands/reproducer/invalid-args/TestInvalidArgsReproducer.py
  lldb/test/API/commands/target/basic/TestTargetCommand.py
  lldb/test/API/commands/target/dump/TestTargetDumpTypeSystem.py
  lldb/test/API/commands/version/TestVersion.py
  
lldb/test/API/commands/watchpoints/multi_watchpoint_slots/TestWatchpointMultipleSlots.py
  lldb/test/API/functionalities/completion/TestCompletion.py

Index: lldb/test/API/functionalities/completion/TestCompletion.py
===
--- lldb/test/API/functionalities/completion/TestCompletion.py
+++ lldb/test/API/functionalities/completion/TestCompletion.py
@@ -675,7 +675,7 @@
 
 self.build()
 self.dbg.CreateTarget(self.getBuildArtifact("a.out"))
-self.runCmd('target stop-hook add test DONE')
+self.runCmd('target stop-hook add -o test')
 
 for subcommand in subcommands:
 self.complete_from_to('target stop-hook ' + subcommand + ' ',
Index: lldb/test/API/commands/watchpoints/multi_watchpoint_slots/TestWatchpointMultipleSlots.py
===
--- lldb/test/API/commands/watchpoints/multi_watchpoint_slots/TestWatchpointMultipleSlots.py
+++ lldb/test/API/commands/watchpoints/multi_watchpoint_slots/TestWatchpointMultipleSlots.py
@@ -88,10 +88,10 @@
 # The stop reason of the thread should be watchpoint.
 if self.platformIsDarwin():
 # On darwin we'll hit byteArray[3] which is watchpoint 2
-self.expect("thread list -v", STOPPED_DUE_TO_WATCHPOINT,
+self.expect("thread list", STOPPED_DUE_TO_WATCHPOINT,
 substrs=['stopped', 'stop reason = watchpoint 2'])
 else:
-self.expect("thread list -v", STOPPED_DUE_TO_WATCHPOINT,
+self.expect("thread list", STOPPED_DUE_TO_WATCHPOINT,
 substrs=['stopped', 'stop reason = watchpoint 3'])
 
 # Resume inferior.
Index: lldb/test/API/commands/version/TestVersion.py
===
--- lldb/test/API/commands/version/TestVersion.py
+++ lldb/test/API/commands/version/T

[Lldb-commits] [PATCH] D128450: [lldb/Fuzzer] Have fuzzers write artifacts to specific directory

[Chelsea Cassanova via Phabricator via lldb-commits]

cassanova updated this revision to Diff 439454.
cassanova retitled this revision from "[lldb/Fuzzer] Have target fuzzer write 
artifacts to specific directory" to "[lldb/Fuzzer] Have fuzzers write artifacts 
to specific directory".
cassanova edited the summary of this revision.
cassanova added a comment.

Removed the chain of shell commands in the target fuzzer's CMakeLists file and 
added a pre build command that creates the necessary directory and changes the 
working directory in the target to this directory.

Also implemented these changes to the command interpreter fuzzer's CMakeLists 
file and added it to this diff.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128450

Files:
  lldb/tools/lldb-fuzzer/lldb-commandinterpreter-fuzzer/CMakeLists.txt
  lldb/tools/lldb-fuzzer/lldb-target-fuzzer/CMakeLists.txt


Index: lldb/tools/lldb-fuzzer/lldb-target-fuzzer/CMakeLists.txt
===
--- lldb/tools/lldb-fuzzer/lldb-target-fuzzer/CMakeLists.txt
+++ lldb/tools/lldb-fuzzer/lldb-target-fuzzer/CMakeLists.txt
@@ -15,9 +15,14 @@
 lldbFuzzerUtils
 )
 
+  add_custom_command(TARGET lldb-target-fuzzer PRE_BUILD
+COMMAND ${CMAKE_COMMAND} -E make_directory 
${CMAKE_BINARY_DIR}/fuzzer-artifacts/target-artifacts
+)
+
   add_custom_target(fuzz-lldb-target
 COMMENT "Running the LLDB target fuzzer..."
-COMMAND cd ${CMAKE_CURRENT_SOURCE_DIR} && $
+WORKING_DIRECTORY ${CMAKE_BINARY_DIR}/fuzzer-artifacts/target-artifacts
+COMMAND $ -artifact_prefix=target-
 USES_TERMINAL
 )
 endif()
Index: lldb/tools/lldb-fuzzer/lldb-commandinterpreter-fuzzer/CMakeLists.txt
===
--- lldb/tools/lldb-fuzzer/lldb-commandinterpreter-fuzzer/CMakeLists.txt
+++ lldb/tools/lldb-fuzzer/lldb-commandinterpreter-fuzzer/CMakeLists.txt
@@ -14,15 +14,19 @@
 liblldb
 )
 
-  # This will create a directory specifically for the fuzzer's artifacts, go 
to that
-  # directory and run the fuzzer from there. When the fuzzer exits the input
-  # artifact that caused it to exit will be written to a directory within the
-  # build directory
+  # A directory in the build directory is created to hold the fuzzer's
+  # artifacts as a pre-build command for the command interpreter's executable
+  # target. When the fuzzer exits the input artifact that caused it to exit
+  # will be written to this directory.
+
+  add_custom_command(TARGET lldb-commandinterpreter-fuzzer PRE_BUILD
+COMMAND ${CMAKE_COMMAND} -E make_directory 
${CMAKE_BINARY_DIR}/fuzzer-artifacts/commandinterpreter-artifacts
+)
+
   add_custom_target(fuzz-lldb-commandinterpreter
 COMMENT "Running the LLDB command interpreter fuzzer..."
-COMMAND mkdir -p 
${CMAKE_BINARY_DIR}/fuzzer-artifacts/commandinterpreter-artifacts &&
-cd ${CMAKE_BINARY_DIR}/fuzzer-artifacts/commandinterpreter-artifacts
-&& $ 
-dict=${CMAKE_CURRENT_SOURCE_DIR}/inputdictionary.txt  -only_ascii=1 
-artifact_prefix=commandinterpreter-
+WORKING_DIRECTORY 
${CMAKE_BINARY_DIR}/fuzzer-artifacts/commandinterpreter-artifacts
+COMMAND  $ 
-dict=${CMAKE_CURRENT_SOURCE_DIR}/inputdictionary.txt  -only_ascii=1 
-artifact_prefix=commandinterpreter-
 USES_TERMINAL
 )
 endif()


Index: lldb/tools/lldb-fuzzer/lldb-target-fuzzer/CMakeLists.txt
===
--- lldb/tools/lldb-fuzzer/lldb-target-fuzzer/CMakeLists.txt
+++ lldb/tools/lldb-fuzzer/lldb-target-fuzzer/CMakeLists.txt
@@ -15,9 +15,14 @@
 lldbFuzzerUtils
 )
 
+  add_custom_command(TARGET lldb-target-fuzzer PRE_BUILD
+COMMAND ${CMAKE_COMMAND} -E make_directory ${CMAKE_BINARY_DIR}/fuzzer-artifacts/target-artifacts
+)
+
   add_custom_target(fuzz-lldb-target
 COMMENT "Running the LLDB target fuzzer..."
-COMMAND cd ${CMAKE_CURRENT_SOURCE_DIR} && $
+WORKING_DIRECTORY ${CMAKE_BINARY_DIR}/fuzzer-artifacts/target-artifacts
+COMMAND $ -artifact_prefix=target-
 USES_TERMINAL
 )
 endif()
Index: lldb/tools/lldb-fuzzer/lldb-commandinterpreter-fuzzer/CMakeLists.txt
===
--- lldb/tools/lldb-fuzzer/lldb-commandinterpreter-fuzzer/CMakeLists.txt
+++ lldb/tools/lldb-fuzzer/lldb-commandinterpreter-fuzzer/CMakeLists.txt
@@ -14,15 +14,19 @@
 liblldb
 )
 
-  # This will create a directory specifically for the fuzzer's artifacts, go to that
-  # directory and run the fuzzer from there. When the fuzzer exits the input
-  # artifact that caused it to exit will be written to a directory within the
-  # build directory
+  # A directory in the build directory is created to hold the fuzzer's
+  # artifacts as a pre-build command for the command interpreter's executable
+  # target. When the fuzzer exits the input artifact that caused it to exit
+  # will be written to this direct

[Lldb-commits] [PATCH] D128450: [lldb/Fuzzer] Have fuzzers write artifacts to specific directory

[Med Ismail Bennani via Phabricator via lldb-commits]

mib accepted this revision.
mib added a comment.

This looks way better :) !


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128450

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

[Lldb-commits] [PATCH] D128321: [lldb] Add OSLog log handler

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

So it seems like we are catering to our code organization here instead of doing 
the right thing and relying on the host layer. I would rather move the log code 
into the host layer or make log handlers actual plug-ins so that we can do the 
right thing here.

If we can make LogHandler objects plug-ins, where each plug-in has a name, then 
this can be used to specify the different types of logs using "--kind " 
or "--type ". Then we don't run into these layering issues, because 
essentially we have small LogHandler implementations here. Any objection to 
making LogHandler subclasses into actual plug-ins?


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

https://reviews.llvm.org/D128321

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

[Lldb-commits] [PATCH] D128321: [lldb] Add OSLog log handler

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

If we do add plug-ins, you could make an Apple specific plug-in named 
"darwin-os-log" or something that would only be compiled in for Apple builds, 
or we can do a generic "os-log" plug-in that uses the host layer.


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

https://reviews.llvm.org/D128321

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

[Lldb-commits] [lldb] a1f20da - The help string for stop-on-shared-library-load was copied to stop-on-exec.

[Jim Ingham via lldb-commits]

Author: Jim Ingham
Date: 2022-06-23T13:54:50-07:00
New Revision: a1f20da315c8f9aa109b49c47797898ad6559d1a

URL: 
https://github.com/llvm/llvm-project/commit/a1f20da315c8f9aa109b49c47797898ad6559d1a
DIFF: 
https://github.com/llvm/llvm-project/commit/a1f20da315c8f9aa109b49c47797898ad6559d1a.diff

LOG: The help string for stop-on-shared-library-load was copied to stop-on-exec.
Fix it so it says it does what it does.

Added: 


Modified: 
lldb/source/Target/TargetProperties.td

Removed: 




diff  --git a/lldb/source/Target/TargetProperties.td 
b/lldb/source/Target/TargetProperties.td
index 02d82fbc109b9..acee09ca0469e 100644
--- a/lldb/source/Target/TargetProperties.td
+++ b/lldb/source/Target/TargetProperties.td
@@ -230,7 +230,7 @@ let Definition = "process" in {
   def StopOnExec: Property<"stop-on-exec", "Boolean">,
 Global,
 DefaultTrue,
-Desc<"If true, stop when a shared library is loaded or unloaded.">;
+Desc<"If true, stop when the inferior exec's.">;
   def UtilityExpressionTimeout: Property<"utility-expression-timeout", 
"UInt64">,
 DefaultUnsignedValue<15>,
 Desc<"The time in seconds to wait for LLDB-internal utility expressions.">;



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

[Lldb-commits] [lldb] 40aace5 - [lldb/Fuzzer] Have fuzzers write artifacts to specific directory

[Chelsea Cassanova via lldb-commits]

Author: Chelsea Cassanova
Date: 2022-06-23T16:55:23-04:00
New Revision: 40aace59cc58ca438060cf4dfd97ba01ff4f0ebc

URL: 
https://github.com/llvm/llvm-project/commit/40aace59cc58ca438060cf4dfd97ba01ff4f0ebc
DIFF: 
https://github.com/llvm/llvm-project/commit/40aace59cc58ca438060cf4dfd97ba01ff4f0ebc.diff

LOG: [lldb/Fuzzer] Have fuzzers write artifacts to specific directory

This makes the LLDB fuzzers write their fuzzer artifacts to
their own directory in the build directory. It also adds an artifact
prefix to the target fuzzer to make it easier to tell which fuzzer
wrote the artifact.

Differential revision: https://reviews.llvm.org/D128450

Added: 


Modified: 
lldb/tools/lldb-fuzzer/lldb-commandinterpreter-fuzzer/CMakeLists.txt
lldb/tools/lldb-fuzzer/lldb-target-fuzzer/CMakeLists.txt

Removed: 




diff  --git 
a/lldb/tools/lldb-fuzzer/lldb-commandinterpreter-fuzzer/CMakeLists.txt 
b/lldb/tools/lldb-fuzzer/lldb-commandinterpreter-fuzzer/CMakeLists.txt
index 5bfae5b574e95..7eb85ba916709 100644
--- a/lldb/tools/lldb-fuzzer/lldb-commandinterpreter-fuzzer/CMakeLists.txt
+++ b/lldb/tools/lldb-fuzzer/lldb-commandinterpreter-fuzzer/CMakeLists.txt
@@ -14,15 +14,19 @@ if(TARGET lldb-commandinterpreter-fuzzer)
 liblldb
 )
 
-  # This will create a directory specifically for the fuzzer's artifacts, go 
to that
-  # directory and run the fuzzer from there. When the fuzzer exits the input
-  # artifact that caused it to exit will be written to a directory within the
-  # build directory
+  # A directory in the build directory is created to hold the fuzzer's
+  # artifacts as a pre-build command for the command interpreter's executable
+  # target. When the fuzzer exits the input artifact that caused it to exit
+  # will be written to this directory.
+
+  add_custom_command(TARGET lldb-commandinterpreter-fuzzer PRE_BUILD
+COMMAND ${CMAKE_COMMAND} -E make_directory 
${CMAKE_BINARY_DIR}/fuzzer-artifacts/commandinterpreter-artifacts
+)
+
   add_custom_target(fuzz-lldb-commandinterpreter
 COMMENT "Running the LLDB command interpreter fuzzer..."
-COMMAND mkdir -p 
${CMAKE_BINARY_DIR}/fuzzer-artifacts/commandinterpreter-artifacts &&
-cd ${CMAKE_BINARY_DIR}/fuzzer-artifacts/commandinterpreter-artifacts
-&& $ 
-dict=${CMAKE_CURRENT_SOURCE_DIR}/inputdictionary.txt  -only_ascii=1 
-artifact_prefix=commandinterpreter-
+WORKING_DIRECTORY 
${CMAKE_BINARY_DIR}/fuzzer-artifacts/commandinterpreter-artifacts
+COMMAND  $ 
-dict=${CMAKE_CURRENT_SOURCE_DIR}/inputdictionary.txt  -only_ascii=1 
-artifact_prefix=commandinterpreter-
 USES_TERMINAL
 )
 endif()

diff  --git a/lldb/tools/lldb-fuzzer/lldb-target-fuzzer/CMakeLists.txt 
b/lldb/tools/lldb-fuzzer/lldb-target-fuzzer/CMakeLists.txt
index 06df06c07d927..6876945c08da6 100644
--- a/lldb/tools/lldb-fuzzer/lldb-target-fuzzer/CMakeLists.txt
+++ b/lldb/tools/lldb-fuzzer/lldb-target-fuzzer/CMakeLists.txt
@@ -15,9 +15,14 @@ if(TARGET lldb-target-fuzzer)
 lldbFuzzerUtils
 )
 
+  add_custom_command(TARGET lldb-target-fuzzer PRE_BUILD
+COMMAND ${CMAKE_COMMAND} -E make_directory 
${CMAKE_BINARY_DIR}/fuzzer-artifacts/target-artifacts
+)
+
   add_custom_target(fuzz-lldb-target
 COMMENT "Running the LLDB target fuzzer..."
-COMMAND cd ${CMAKE_CURRENT_SOURCE_DIR} && $
+WORKING_DIRECTORY ${CMAKE_BINARY_DIR}/fuzzer-artifacts/target-artifacts
+COMMAND $ -artifact_prefix=target-
 USES_TERMINAL
 )
 endif()



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

[Lldb-commits] [PATCH] D128450: [lldb/Fuzzer] Have fuzzers write artifacts to specific directory

[Chelsea Cassanova via Phabricator via lldb-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rG40aace59cc58: [lldb/Fuzzer] Have fuzzers write artifacts to 
specific directory (authored by cassanova).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128450

Files:
  lldb/tools/lldb-fuzzer/lldb-commandinterpreter-fuzzer/CMakeLists.txt
  lldb/tools/lldb-fuzzer/lldb-target-fuzzer/CMakeLists.txt


Index: lldb/tools/lldb-fuzzer/lldb-target-fuzzer/CMakeLists.txt
===
--- lldb/tools/lldb-fuzzer/lldb-target-fuzzer/CMakeLists.txt
+++ lldb/tools/lldb-fuzzer/lldb-target-fuzzer/CMakeLists.txt
@@ -15,9 +15,14 @@
 lldbFuzzerUtils
 )
 
+  add_custom_command(TARGET lldb-target-fuzzer PRE_BUILD
+COMMAND ${CMAKE_COMMAND} -E make_directory 
${CMAKE_BINARY_DIR}/fuzzer-artifacts/target-artifacts
+)
+
   add_custom_target(fuzz-lldb-target
 COMMENT "Running the LLDB target fuzzer..."
-COMMAND cd ${CMAKE_CURRENT_SOURCE_DIR} && $
+WORKING_DIRECTORY ${CMAKE_BINARY_DIR}/fuzzer-artifacts/target-artifacts
+COMMAND $ -artifact_prefix=target-
 USES_TERMINAL
 )
 endif()
Index: lldb/tools/lldb-fuzzer/lldb-commandinterpreter-fuzzer/CMakeLists.txt
===
--- lldb/tools/lldb-fuzzer/lldb-commandinterpreter-fuzzer/CMakeLists.txt
+++ lldb/tools/lldb-fuzzer/lldb-commandinterpreter-fuzzer/CMakeLists.txt
@@ -14,15 +14,19 @@
 liblldb
 )
 
-  # This will create a directory specifically for the fuzzer's artifacts, go 
to that
-  # directory and run the fuzzer from there. When the fuzzer exits the input
-  # artifact that caused it to exit will be written to a directory within the
-  # build directory
+  # A directory in the build directory is created to hold the fuzzer's
+  # artifacts as a pre-build command for the command interpreter's executable
+  # target. When the fuzzer exits the input artifact that caused it to exit
+  # will be written to this directory.
+
+  add_custom_command(TARGET lldb-commandinterpreter-fuzzer PRE_BUILD
+COMMAND ${CMAKE_COMMAND} -E make_directory 
${CMAKE_BINARY_DIR}/fuzzer-artifacts/commandinterpreter-artifacts
+)
+
   add_custom_target(fuzz-lldb-commandinterpreter
 COMMENT "Running the LLDB command interpreter fuzzer..."
-COMMAND mkdir -p 
${CMAKE_BINARY_DIR}/fuzzer-artifacts/commandinterpreter-artifacts &&
-cd ${CMAKE_BINARY_DIR}/fuzzer-artifacts/commandinterpreter-artifacts
-&& $ 
-dict=${CMAKE_CURRENT_SOURCE_DIR}/inputdictionary.txt  -only_ascii=1 
-artifact_prefix=commandinterpreter-
+WORKING_DIRECTORY 
${CMAKE_BINARY_DIR}/fuzzer-artifacts/commandinterpreter-artifacts
+COMMAND  $ 
-dict=${CMAKE_CURRENT_SOURCE_DIR}/inputdictionary.txt  -only_ascii=1 
-artifact_prefix=commandinterpreter-
 USES_TERMINAL
 )
 endif()


Index: lldb/tools/lldb-fuzzer/lldb-target-fuzzer/CMakeLists.txt
===
--- lldb/tools/lldb-fuzzer/lldb-target-fuzzer/CMakeLists.txt
+++ lldb/tools/lldb-fuzzer/lldb-target-fuzzer/CMakeLists.txt
@@ -15,9 +15,14 @@
 lldbFuzzerUtils
 )
 
+  add_custom_command(TARGET lldb-target-fuzzer PRE_BUILD
+COMMAND ${CMAKE_COMMAND} -E make_directory ${CMAKE_BINARY_DIR}/fuzzer-artifacts/target-artifacts
+)
+
   add_custom_target(fuzz-lldb-target
 COMMENT "Running the LLDB target fuzzer..."
-COMMAND cd ${CMAKE_CURRENT_SOURCE_DIR} && $
+WORKING_DIRECTORY ${CMAKE_BINARY_DIR}/fuzzer-artifacts/target-artifacts
+COMMAND $ -artifact_prefix=target-
 USES_TERMINAL
 )
 endif()
Index: lldb/tools/lldb-fuzzer/lldb-commandinterpreter-fuzzer/CMakeLists.txt
===
--- lldb/tools/lldb-fuzzer/lldb-commandinterpreter-fuzzer/CMakeLists.txt
+++ lldb/tools/lldb-fuzzer/lldb-commandinterpreter-fuzzer/CMakeLists.txt
@@ -14,15 +14,19 @@
 liblldb
 )
 
-  # This will create a directory specifically for the fuzzer's artifacts, go to that
-  # directory and run the fuzzer from there. When the fuzzer exits the input
-  # artifact that caused it to exit will be written to a directory within the
-  # build directory
+  # A directory in the build directory is created to hold the fuzzer's
+  # artifacts as a pre-build command for the command interpreter's executable
+  # target. When the fuzzer exits the input artifact that caused it to exit
+  # will be written to this directory.
+
+  add_custom_command(TARGET lldb-commandinterpreter-fuzzer PRE_BUILD
+COMMAND ${CMAKE_COMMAND} -E make_directory ${CMAKE_BINARY_DIR}/fuzzer-artifacts/commandinterpreter-artifacts
+)
+
   add_custom_target(fuzz-lldb-commandinterpreter
 COMMENT "Running the LLDB command interpreter fuzzer..."
-COMMAND mkdir -p ${CMAKE_BINARY_D

[Lldb-commits] [lldb] 6fa9120 - [lldb] Fix up Objective-C ISA pointers

[Jonas Devlieghere via lldb-commits]

Author: Jonas Devlieghere
Date: 2022-06-23T14:16:29-07:00
New Revision: 6fa9120080c35a5ff851c3fc3358692c4ef7ce0d

URL: 
https://github.com/llvm/llvm-project/commit/6fa9120080c35a5ff851c3fc3358692c4ef7ce0d
DIFF: 
https://github.com/llvm/llvm-project/commit/6fa9120080c35a5ff851c3fc3358692c4ef7ce0d.diff

LOG: [lldb] Fix up Objective-C ISA pointers

Support stripping the PAC bits from Objective-C ISA pointers in the
Objective-C runtime plugin.

Added: 


Modified: 

lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.cpp

Removed: 




diff  --git 
a/lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.cpp
 
b/lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.cpp
index f53b82ee33c80..1c2447b2bec5f 100644
--- 
a/lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.cpp
+++ 
b/lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.cpp
@@ -1543,6 +1543,12 @@ AppleObjCRuntimeV2::GetClassDescriptor(ValueObject 
&valobj) {
 return objc_class_sp;
 
   objc_class_sp = GetClassDescriptorFromISA(isa);
+  if (!objc_class_sp) {
+if (ABISP abi_sp = process->GetABI())
+  isa = abi_sp->FixCodeAddress(isa);
+objc_class_sp = GetClassDescriptorFromISA(isa);
+  }
+
   if (isa && !objc_class_sp) {
 Log *log = GetLog(LLDBLog::Process | LLDBLog::Types);
 LLDB_LOGF(log,



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

[Lldb-commits] [PATCH] D128453: Automate checking for "command that takes no arguments" being passed arguments in CommandObjectParsed

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere accepted this revision.
JDevlieghere added a subscriber: labath.
JDevlieghere added a comment.
This revision is now accepted and ready to land.

This is great, it both guarantees consistently and enforces command objects 
registering their arguments. LGTM.

For the RenderScript plugin, I remember this at an LLVM social when @labath was 
in town. At the time, we were very close to being able to get rid of it, which 
is now several years ago. Pavel, do you remember who we spoke to and if we've 
reached the point where this can go away?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128453

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

[Lldb-commits] [PATCH] D128386: [lldb] Make thread safety the responsibility of the log handlers

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

Maybe we should leave the option in the command for history purposes??? Taking 
it out will now make anyone's log enable lines fail when they didn't use to...


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

https://reviews.llvm.org/D128386

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

[Lldb-commits] [PATCH] D128323: [lldb] Add support for specifying a log handler

[Greg Clayton via Phabricator via lldb-commits]

clayborg added inline comments.



Comment at: lldb/source/Commands/CommandObjectLog.cpp:26
 
+static constexpr OptionEnumValueElement g_log_handler_type[] = {
+{

Question: how does one see these values and their descriptions? Do the 
descriptions get displayed anywhere? If they do, then my comments below make 
sense, if the don't, then ignore below comments and maybe roll all that 
documentation into the main description for this command.



Comment at: lldb/source/Commands/CommandObjectLog.cpp:30
+"stream",
+"Use the stream log handler",
+},

It doesn't seem very clear to the user what "stream" means here. Is says it 
will use one, but what is a stream? Maybe expand on this will output the log to 
the debugger stderr or to a file if used with the -f option? Maybe "default" is 
a better user facing term here?



Comment at: lldb/source/Commands/CommandObjectLog.cpp:34-35
+eLogHandlerCircular,
+"rotating",
+"Use the rotating log handler",
+},

circular makes more sense to use as the name IMHO. Not sure what rotating means 
for a log handler. Maybe expand on if this is enabled, you can see the cached 
log lines by using "log dump" and it will keep X amount of messages? Or does 
the user need to specify a size of a buffer somewhere?



Comment at: lldb/source/Commands/CommandObjectLog.cpp:40
+"system",
+"Use the system log handler",
+},

Just saying that we will use the system log handler doesn't really explain what 
is going on here. Maybe "Log messages to the OS's default system log." 



Comment at: lldb/source/Commands/CommandObjectLog.cpp:124-126
+  error.SetErrorStringWithFormat(
+  "unrecognized value for log handler '%s'",
+  option_arg.str().c_str());

Can we also add the valid values that users can choose from in this error 
message? If the user types "log enable -h wrong", how does the user know what 
the valid values to use are?



Comment at: lldb/source/Commands/Options.td:438-439
+EnumArg<"Value", "LogHandlerType()">, Desc<"Redirect the log output to a "
+"specific log handler. The default log handler (stream) writes to the "
+"debugger output stream or to a file if one is specified with -f.">;
   def log_threadsafe : Option<"threadsafe", "t">, Group<1>,

Should this be moved to the enum description?



Comment at: lldb/source/Commands/Options.td:436-437
 Desc<"Set the log to be buffered, using the specified buffer size.">;
+  def log_handler : Option<"handler", "h">, Group<1>,
+EnumArg<"Value", "LogHandlerType()">, Desc<"Use a custom handler.">;
   def log_threadsafe : Option<"threadsafe", "t">, Group<1>,

JDevlieghere wrote:
> clayborg wrote:
> > Maybe "--type" or "--kind" would be better? Handler seems like an internal 
> > name. Maybe also mention what it will default to if not specified? Can the 
> > user see a list of the valid values or do we need to supply these in the 
> > description?
> I think the concept of a "log handler" is sufficiently well known that it 
> should be easy for a a (power) user to understand. I'm concerned that "type" 
> and "kind" are a tad too high level, and therefore could easily be confused 
> with the log category and channel. For example, when someone asks to enable 
> the "gdb packet log", is that a category, channel, type or kind? 
How can the user see the available options? I always have to type an incorrect 
value in for enums and then I see a correction saying "you can only specify 
'a', 'b', or 'c'"


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

https://reviews.llvm.org/D128323

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

[Lldb-commits] [PATCH] D126513: Add -b (--continue-to-breakpoint) option to the "process continue" command

[Caroline Tice via Phabricator via lldb-commits]

cmtice added a comment.

This appears to have broken a lot of LLDB tests in our bootstrap as well. Is 
there any chance this might get reverted?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126513

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

[Lldb-commits] [PATCH] D128477: [trace] Add a flag to the decoder to output the instruction type

[Sujin Park via Phabricator via lldb-commits]

persona0220 created this revision.
persona0220 added reviewers: wallace, jj10306.
Herald added a project: All.
persona0220 requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

To build complex binding upon instruction trace, additional metadata 
'instruction type' is needed.

This diff has followings:

- Add a flag -k  / --kind for instruction dump
- Remove SetGranularity and SetIgnoreErros from Trace cursor

Sample output:

  (lldb) thread trace dump instruction -k
  thread #1: tid = 3198805
libc.so.6`_IO_puts + 356
  2107: 0x77163594 (return) retq
  2106: 0x77163592 ( other) popq   %r13
  2105: 0x77163590 ( other) popq   %r12
  2104: 0x7716358f ( other) popq   %rbp
  2103: 0x7716358e ( other) popq   %rbx
  2102: 0x7716358c ( other) movl   %ebx, %eax
  2101: 0x77163588 ( other) addq   $0x8, %rsp
  2100: 0x77163570 ( cond jump) je 0x89588  
 ; <+344>
  2099: 0x7716356e ( other) decl   (%rdx)
  2098: 0x77163565 ( cond jump) je 0x8956e  
 ; <+318>
  2097: 0x7716355e ( other) cmpl   $0x0, 0x33c02b(%rip) 
 ; __libc_multiple_threads
  2096: 0x77163556 ( other) movq   $0x0, 0x8(%rdx)
  2095: 0x77163554 ( cond jump) jne0x89588  
 ; <+344>
  2094: 0x77163550 ( other) subl   $0x1, 0x4(%rdx)
  2093: 0x77163549 ( other) movq   0x88(%rbp), %rdx
  2092: 0x77163547 ( cond jump) jne0x89588  
 ; <+344>
  2091: 0x77163540 ( other) testl  $0x8000, (%rbp)  
 ; imm = 0x8000
  2090: 0x7716353c ( other) cmovaq %rax, %rbx
  2089: 0x77163535 ( other) cmpq   $0x7fff, %rbx
 ; imm = 0x7FFF
  2088: 0x77163530 ( other) movl   $0x7fff, %eax
 ; imm = 0x7FFF


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D128477

Files:
  lldb/include/lldb/Target/TraceCursor.h
  lldb/include/lldb/Target/TraceInstructionDumper.h
  lldb/include/lldb/lldb-enumerations.h
  lldb/source/Commands/CommandObjectThread.cpp
  lldb/source/Commands/Options.td
  lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp
  lldb/source/Target/TraceCursor.cpp
  lldb/source/Target/TraceInstructionDumper.cpp

Index: lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp
===
--- lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp
+++ lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp
@@ -46,10 +46,7 @@
 if (m_tsc_range && !m_tsc_range->InRange(m_pos))
   m_tsc_range = IsForwards() ? m_tsc_range->Next() : m_tsc_range->Prev();
 
-if (!m_ignore_errors && IsError())
-  return true;
-if (GetInstructionControlFlowType() & m_granularity)
-  return true;
+return true;
   }
 
   // Didn't find any matching instructions
Index: lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp
===
--- lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp
+++ lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp
@@ -62,31 +62,34 @@
 TraceInstructionControlFlowType
 DecodedThread::GetInstructionControlFlowType(size_t insn_index) const {
   if (IsInstructionAnError(insn_index))
-return (TraceInstructionControlFlowType)0;
+return eTraceInstructionControlFlowTypeError;
 
   TraceInstructionControlFlowType mask =
   eTraceInstructionControlFlowTypeInstruction;
 
-  lldb::addr_t load_address = m_instruction_ips[insn_index];
-  uint8_t insn_byte_size = m_instruction_sizes[insn_index];
   pt_insn_class iclass = m_instruction_classes[insn_index];
 
   switch (iclass) {
-  case ptic_cond_jump:
-  case ptic_jump:
-  case ptic_far_jump:
-mask |= eTraceInstructionControlFlowTypeBranch;
-if (insn_index + 1 < m_instruction_ips.size() &&
-load_address + insn_byte_size != m_instruction_ips[insn_index + 1])
-  mask |= eTraceInstructionControlFlowTypeTakenBranch;
+  case ptic_call:
+mask |= eTraceInstructionControlFlowTypeCall;
 break;
   case ptic_return:
-  case ptic_far_return:
 mask |= eTraceInstructionControlFlowTypeReturn;
 break;
-  case ptic_call:
+  case ptic_jump:
+mask |= eTraceInstructionControlFlowTypeJump;
+break;
+  case ptic_cond_jump:
+mask |= eTraceInstructionControlFlowTypeCondJump;
+break;
   case ptic_far_call:
-mask |= eTraceInstructionControlFlowTypeCall;
+mask |= eTraceInstructionControlFlowTypeFarCall;
+break;
+  case ptic_far_return:
+mask |= eTraceInstructionControlFlowTypeFarReturn;
+break;
+  case ptic_far_jump:
+

[Lldb-commits] [PATCH] D126513: Add -b (--continue-to-breakpoint) option to the "process continue" command

[Caroline Tice via Phabricator via lldb-commits]

cmtice added a comment.

Ignore my revert request; Your later commit seems to have fixed most of my 
issues.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126513

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

[Lldb-commits] [PATCH] D128480: [lldb] Replace Host::SystemLog with Debugger::Report{Error, Warning}

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere created this revision.
JDevlieghere added reviewers: labath, clayborg, aprantl, mib.
Herald added a project: All.
JDevlieghere requested review of this revision.
Herald added a reviewer: jdoerfert.
Herald added a subscriber: sstefan1.

As it exists today, Host::SystemLog is used exclusively for error reporting. 
With the introduction of diagnostic events, we have a better way of reporting 
those. By switching over, we print these error messages to the debugger's error 
stream (when using the default event handler) or report them to an IDE such as 
Xcode (if they have subscribed to these events). It also means we nog longer 
write the messages to the system log on Darwin, but as far as I know, nobody is 
relying on this functionality today. If this is deemed important externally, I 
can add it again when I add the system logging functionality again in the 
context of D128321 .


https://reviews.llvm.org/D128480

Files:
  lldb/include/lldb/Host/Host.h
  lldb/source/Core/Module.cpp
  lldb/source/Host/common/Host.cpp
  lldb/source/Host/macosx/objcxx/Host.mm
  lldb/source/Interpreter/Options.cpp
  lldb/source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderDarwin.cpp
  lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
  lldb/source/Symbol/CompactUnwindInfo.cpp
  lldb/source/Symbol/DWARFCallFrameInfo.cpp
  lldb/source/Symbol/Function.cpp
  lldb/source/Symbol/SymbolContext.cpp
  lldb/test/API/lang/objc/modules-hash-mismatch/TestClangModulesHashMismatch.py
  lldb/tools/lldb-server/lldb-platform.cpp

Index: lldb/tools/lldb-server/lldb-platform.cpp
===
--- lldb/tools/lldb-server/lldb-platform.cpp
+++ lldb/tools/lldb-server/lldb-platform.cpp
@@ -79,8 +79,7 @@
 // And we should not call exit() here because it results in the global
 // destructors
 // to be invoked and wreaking havoc on the threads still running.
-Host::SystemLog(Host::eSystemLogWarning,
-"SIGHUP received, exiting lldb-server...\n");
+fprintf(stderr, "SIGHUP received, exiting lldb-server...\n");
 abort();
 break;
   }
Index: lldb/test/API/lang/objc/modules-hash-mismatch/TestClangModulesHashMismatch.py
===
--- lldb/test/API/lang/objc/modules-hash-mismatch/TestClangModulesHashMismatch.py
+++ lldb/test/API/lang/objc/modules-hash-mismatch/TestClangModulesHashMismatch.py
@@ -30,12 +30,15 @@
 self.assertTrue(os.path.isdir(mod_cache), "module cache exists")
 
 logfile = self.getBuildArtifact("host.log")
-self.runCmd("log enable -v -f %s lldb host" % logfile)
-target, _, _, _ = lldbutil.run_to_source_breakpoint(
-self, "break here", lldb.SBFileSpec("main.m"))
-target.GetModuleAtIndex(0).FindTypes('my_int')
+with open(logfile, 'w') as f:
+sbf = lldb.SBFile(f.fileno(), 'w', False)
+status = self.dbg.SetErrorFile(sbf)
+self.assertSuccess(status)
+
+target, _, _, _ = lldbutil.run_to_source_breakpoint(
+self, "break here", lldb.SBFileSpec("main.m"))
+target.GetModuleAtIndex(0).FindTypes('my_int')
 
-found = False
 with open(logfile, 'r') as f:
 for line in f:
 if "hash mismatch" in line and "Foo" in line:
Index: lldb/source/Symbol/SymbolContext.cpp
===
--- lldb/source/Symbol/SymbolContext.cpp
+++ lldb/source/Symbol/SymbolContext.cpp
@@ -8,6 +8,7 @@
 
 #include "lldb/Symbol/SymbolContext.h"
 
+#include "lldb/Core/Debugger.h"
 #include "lldb/Core/Module.h"
 #include "lldb/Core/ModuleSpec.h"
 #include "lldb/Host/Host.h"
@@ -494,20 +495,16 @@
   objfile = symbol_file->GetObjectFile();
   }
   if (objfile) {
-Host::SystemLog(
-Host::eSystemLogWarning,
-"warning: inlined block 0x%8.8" PRIx64
-" doesn't have a range that contains file address 0x%" PRIx64
-" in %s\n",
+Debugger::ReportWarning(llvm::formatv(
+"inlined block {0:x} doesn't have a range that contains file "
+"address {1:x} in {2}",
 curr_inlined_block->GetID(), curr_frame_pc.GetFileAddress(),
-objfile->GetFileSpec().GetPath().c_str());
+objfile->GetFileSpec().GetPath()));
   } else {
-Host::SystemLog(
-Host::eSystemLogWarning,
-"warning: inlined block 0x%8.8" PRIx64
-" doesn't have a range that contains file address 0x%" PRIx64
-"\n",
-curr_inlined_block->GetID(), curr_frame_pc.GetFileAddress());
+Debugger::ReportWarning(llvm::formatv(
+"inlined block {0:x} doesn't have a range that contains file "
+"address {1: