[Lldb-commits] [PATCH] D86670: [intel-pt] Add a basic implementation of the dump command

[walter erquinigo via Phabricator via lldb-commits]

wallace added inline comments.



Comment at: lldb/include/lldb/Target/Target.h:1105
 
+  void SetTrace(const lldb::TraceSP _sp);
+

wallace wrote:
> JDevlieghere wrote:
> > Who owns the trace? If there's a 1:1 relationship between a trace and a 
> > target, can we make the target its owner? I'm trying to avoid adding shared 
> > pointers if possible. 
> Well, there's a 1 to many relationship. Many targets can own the same trace 
> object, as a single trace can have data of several processes.  On the other 
> hand, there should no other object that could own a trace. I haven't found a 
> better solution :(
What do you think about changing GetTrace() to returns a Trace & instead of 
TraceSP &, so that no other object can share the ownership?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86670

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

[Lldb-commits] [PATCH] D85705: Add a "Trace" plug-in to LLDB to add process trace support in stages.

[walter erquinigo via Phabricator via lldb-commits]

wallace added a comment.

Thanks for the review! Those are very useful comments. So, I'll split the 
parsing out from the Trace object. Regarding a possible inconsistent state, you 
are right. That could happen. The targets, modules and processes used in the 
parsing are all created there, and it should be enough to only "commit" those 
targets if there has been no error. I'll make the necessary updates.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85705

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

[Lldb-commits] [PATCH] D86670: [intel-pt] Add a basic implementation of the dump command

[walter erquinigo via Phabricator via lldb-commits]

wallace added inline comments.



Comment at: lldb/include/lldb/Target/Target.h:1105
 
+  void SetTrace(const lldb::TraceSP _sp);
+

JDevlieghere wrote:
> Who owns the trace? If there's a 1:1 relationship between a trace and a 
> target, can we make the target its owner? I'm trying to avoid adding shared 
> pointers if possible. 
Well, there's a 1 to many relationship. Many targets can own the same trace 
object, as a single trace can have data of several processes.  On the other 
hand, there should no other object that could own a trace. I haven't found a 
better solution :(



Comment at: lldb/include/lldb/Target/Target.h:1341
   unsigned m_next_persistent_variable_index = 0;
+  lldb::TraceSP m_trace;
   /// Stores the frame recognizers of this target.

JDevlieghere wrote:
> Doxygen comment?
good catch


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86670

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

[Lldb-commits] [PATCH] D86667: [lldb/Target] Add custom interpreter option to `platform shell`

[Dave Lee via Phabricator via lldb-commits]

kastiglione added inline comments.



Comment at: lldb/source/API/SBPlatform.cpp:59
+
+if (command_interpreter && command_interpreter[0]) {
+  full_command += command_interpreter;

JDevlieghere wrote:
> Given that this pattern repeats a few times in this struct, maybe a small 
> static helper function would be nice:
> 
> ```static bool is_not_empty(cont char* c) { return c && c[0]; }```
Can these be `StringRef`, and then check with `empty()`?



Comment at: lldb/source/Commands/CommandObjectPlatform.cpp:1627
+
+if (!FileSystem::Instance().Readable(option_arg)) {
+  error.SetErrorStringWithFormat("File \"%s\" is not readable.",

Is it necessary to check readability? If not done here, at execution time would 
a non-readable interpreter lead to a useful error message?

If checking for readability is desirable here, is checking that it's executable 
also desirable?



Comment at: lldb/test/API/commands/platform/basic/TestPlatformCommand.py:109
+self.assertTrue(err.Success())
+self.assertIn("/bin/sh", sh_cmd.GetOutput())
+




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86667

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

[Lldb-commits] [PATCH] D86667: [lldb/Target] Add custom interpreter option to `platform shell`

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.



Comment at: lldb/source/API/SBPlatform.cpp:59
+
+if (command_interpreter && command_interpreter[0]) {
+  full_command += command_interpreter;

Given that this pattern repeats a few times in this struct, maybe a small 
static helper function would be nice:

```static bool is_not_empty(cont char* c) { return c && c[0]; }```



Comment at: lldb/source/API/SBPlatform.cpp:64
+
+if (shell_command && shell_command[0]) {
+  full_command += " \"";

Shouldn't we just bail out if the command is null/empty? Now it will run `shell 
-c`, right?



Comment at: lldb/source/API/SBPlatform.cpp:71
+if (!full_command.empty()) {
+  m_command = full_command.c_str();
+}

m_command is a `std::string`, why call `c_str()`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86667

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

[Lldb-commits] [PATCH] D86670: [intel-pt] Add a basic implementation of the dump command

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.



Comment at: lldb/include/lldb/Target/Target.h:1105
 
+  void SetTrace(const lldb::TraceSP _sp);
+

Who owns the trace? If there's a 1:1 relationship between a trace and a target, 
can we make the target its owner? I'm trying to avoid adding shared pointers if 
possible. 



Comment at: lldb/include/lldb/Target/Target.h:1341
   unsigned m_next_persistent_variable_index = 0;
+  lldb::TraceSP m_trace;
   /// Stores the frame recognizers of this target.

Doxygen comment?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86670

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

[Lldb-commits] [PATCH] D85705: Add a "Trace" plug-in to LLDB to add process trace support in stages.

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

In D85705#2240249 , @wallace wrote:

> Given that this feature is intended to be used by non debugger developers, I 
> want to stick to using JSON as a default input format, as it's more 
> ubiquitous than YAML and I prefer to make this decision based on the user 
> experience than on the technical solution.

Alright, fair enough I guess. In that case I would prefer to split off the 
parser from the Trace data structure.

> I like the YAML parser though, but converting JSON to YAML for parsing might 
> throw away relevant error messaging.

I don't think anyone suggested this.

One more thing about the parser. I really like the usage of 
llvm::Error/Excepted/Optional in this path btw. However, these functions take a 
non-const ref to classes like Process, Thread, etc. It seems like they might be 
left in a potentially inconsistent state when we error out. Is that something 
to worry about or are we guaranteed to discard them in the caller if an error 
occurred? Could they for instance return an `Expected` instead?




Comment at: lldb/include/lldb/Target/Trace.h:86
+  static llvm::Expected
+  FindPlugin(StructuredData::Dictionary settings, const char *settings_dir);
+

StringRef



Comment at: lldb/include/lldb/Target/Trace.h:105
+protected:
+  Trace(StructuredData::Dictionary settings, const char *settings_dir)
+  : m_settings(std::move(settings)), m_settings_dir(settings_dir) {}

StringRef



Comment at: lldb/include/lldb/Target/Trace.h:127
+protected:
+  virtual const char *GetPluginSchema() = 0;
+

StringRef



Comment at: lldb/include/lldb/Target/Trace.h:150
+public:
+  const char *GetSchema();
+  /// \}

StringRef



Comment at: lldb/include/lldb/Utility/StructuredData.h:196
   private:
+llvm::Error CreateWrongTypeError(const char *type) {
+  StreamString object_stream;

StringRef



Comment at: lldb/source/Plugins/Trace/intel-pt/CMakeLists.txt:3
+
+if (NOT LLDB_BUILD_INTEL_PT)
+  return()

Move this up to where this directory is included. e.g.
```
if (LLDB_BUILD_INTEL_PT)
  add_subdirectory(intel-pt)
endif()
```
That way a change to the CMake file won't trigger a reconfig if 
`LLDB_BUILD_INTEL_PT` is false.



Comment at: lldb/source/Plugins/Trace/intel-pt/CMakeLists.txt:22
+  endif()
+  find_library(LIBIPT_LIBRARY ipt PATHS ${LIBIPT_LIBRARY_PATH})
+endif()

Why not pass this in the first place? `PATHS` will be checked *in addition* to 
the default locations.



Comment at: lldb/source/Plugins/Trace/intel-pt/CMakeLists.txt:26
+if (NOT LIBIPT_LIBRARY)
+  message (FATAL_ERROR "libipt library not found")
+endif()

If you `REQUIRED` to `find_library` CMake will take care of the error 
reporting. 



Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h:22
+
+  /// Static Functions
+  /// \{

Redundant. 



Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h:54
+  /// \{
+  llvm::Error
+  ParsePTCPU(const lldb_private::StructuredData::Dictionary );

I think it would be nice to separate the plugin from the parser.



Comment at: lldb/source/Target/Trace.cpp:27
+llvm::Expected Trace::FindPlugin(StructuredData::Dictionary 
info,
+const char *info_dir) {
+  llvm::Expected plugin =

StringRef



Comment at: lldb/source/Target/Trace.cpp:50
+
+const char *Trace::GetSchema() {
+  static std::string schema;

StringRef



Comment at: lldb/source/Target/Trace.cpp:146
+
+llvm::Expected Trace::ParseAddress(const StringRef _str) {
+  addr_t address;

`StringRef` should always be passed by value.



Comment at: lldb/test/API/commands/trace/intelpt-trace/trace.json:2
+{
+  "plugin": {
+"type": "intel-pt",

It seems like the key here should be trace rather than "plugin"?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85705

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

[Lldb-commits] [PATCH] D86670: [intel-pt] Add a basic implementation of the dump command

[walter erquinigo via Phabricator via lldb-commits]

wallace created this revision.
wallace added a reviewer: clayborg.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.
wallace requested review of this revision.
Herald added a subscriber: JDevlieghere.

Depends on D85705 .
The previous diff left the Dump command unimplemented, so I'm implementing it.
As we add more stuff into the intel-pt plugin, we'll dump more information. For 
now, showing the current settings seems okay.

The dump command will print the information of the currently selected target.

This diff is also important because it introduces Target as the owner of the 
Trace object. If a trace corresponds
to multiple targets, then all of then will share the same Trace.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D86670

Files:
  lldb/include/lldb/Target/Target.h
  lldb/include/lldb/Target/Trace.h
  lldb/source/Commands/CommandObjectTrace.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
  lldb/source/Target/Target.cpp
  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
@@ -35,6 +35,8 @@
 
 self.assertEqual("6AA9A4E2-6F28-2F33-377D-59FECE874C71-5B41261A", module.GetUUIDString())
 
+self.expect("trace dump", substrs=["Settings", "intel-pt", "3842849.trace", "Settings directory"])
+
 
 def testLoadInvalidTraces(self):
 src_dir = self.getSourceDir()
@@ -64,3 +66,5 @@
 }
 Schema"""
 self.expect("trace load -v " + trace_definition_file2, substrs=[failed_output2], error=True)
+
+self.expect("trace dump", substrs=["the current target does not have any traces"], error=True)
Index: lldb/source/Target/Trace.cpp
===
--- lldb/source/Target/Trace.cpp
+++ lldb/source/Target/Trace.cpp
@@ -235,6 +235,7 @@
   if (!target_sp)
 return error.ToError();
 
+  target_sp->SetTrace(shared_from_this());
   debugger.GetTargetList().SetSelectedTarget(target_sp.get());
 
   ProcessSP process_sp(target_sp->CreateProcess(
Index: lldb/source/Target/Target.cpp
===
--- lldb/source/Target/Target.cpp
+++ lldb/source/Target/Target.cpp
@@ -2965,6 +2965,10 @@
   return error;
 }
 
+void Target::SetTrace(const lldb::TraceSP _sp) { m_trace = trace_sp; }
+
+lldb::TraceSP ::GetTrace() { return m_trace; }
+
 Status Target::Attach(ProcessAttachInfo _info, Stream *stream) {
   auto state = eStateInvalid;
   auto process_sp = GetProcessSP();
Index: lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
===
--- lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
+++ lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
@@ -94,7 +94,17 @@
 
 uint32_t TraceIntelPT::GetPluginVersion() { return 1; }
 
-void TraceIntelPT::Dump(lldb_private::Stream *s) const {}
+void TraceIntelPT::Dump(lldb_private::Stream *s) const {
+  s->PutCString("Settings\n");
+  s->Indent();
+  m_settings.Dump(*s, /*pretty_print*/ true);
+  s->IndentLess();
+
+  s->PutCString("\nSettings directory\n");
+  s->Indent();
+  s->PutCString(m_settings_dir.c_str());
+  s->IndentLess();
+}
 
 lldb::TraceSP TraceIntelPT::CreateInstance(StructuredData::Dictionary settings,
const char *settings_dir) {
Index: lldb/source/Commands/CommandObjectTrace.cpp
===
--- lldb/source/Commands/CommandObjectTrace.cpp
+++ lldb/source/Commands/CommandObjectTrace.cpp
@@ -160,9 +160,10 @@
   };
 
   CommandObjectTraceDump(CommandInterpreter )
-  : CommandObjectParsed(interpreter, "trace dump",
-"Dump the loaded processor trace data.",
-"trace dump"),
+  : CommandObjectParsed(
+interpreter, "trace dump",
+"Dump the loaded processor trace data from the current target.",
+"trace dump"),
 m_options() {}
 
   ~CommandObjectTraceDump() override = default;
@@ -172,7 +173,14 @@
 protected:
   bool DoExecute(Args , CommandReturnObject ) override {
 Status error;
-// TODO: fill in the dumping code here!
+TraceSP trace = GetSelectedOrDummyTarget().GetTrace();
+if (trace) {
+  trace->Dump(());
+  result.AppendMessage("\n");
+} else
+  error.SetErrorString(
+  "the current target does not have any traces loaded.");
+
 if (error.Success()) {
   result.SetStatus(eReturnStatusSuccessFinishResult);
 } else {
Index: lldb/include/lldb/Target/Trace.h
===
--- lldb/include/lldb/Target/Trace.h
+++ lldb/include/lldb/Target/Trace.h
@@ -35,7 +35,8 

[Lldb-commits] [PATCH] D86615: [lldb/DWARF] Fix handling of variables with both location and const_value attributes

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added inline comments.



Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:3257
+} else if (const char *str = const_value_form.AsCString()) {
+  uint32_t string_length = strlen(str) + 1;
+  location = DWARFExpression(

shafik wrote:
> aprantl wrote:
> > If we do this a lot a StringRef DWARFFormValue::AsCStringRef() call would 
> > make sense...
> Why `+1`?
The NUL-terminator?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86615

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

[Lldb-commits] [PATCH] D86662: Simplify Symbol Status Message to Only Debug Info Size

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added a comment.

Sorry, I missed this!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86662

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

[Lldb-commits] [PATCH] D86667: [lldb/Target] Add custom interpreter option to `platform shell`

[Med Ismail Bennani via Phabricator via lldb-commits]

mib updated this revision to Diff 288147.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86667

Files:
  lldb/bindings/interface/SBPlatform.i
  lldb/include/lldb/API/SBPlatform.h
  lldb/include/lldb/Target/Platform.h
  lldb/include/lldb/Target/RemoteAwarePlatform.h
  lldb/source/API/SBPlatform.cpp
  lldb/source/Commands/CommandObjectPlatform.cpp
  lldb/source/Commands/Options.td
  lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp
  lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.h
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
  lldb/source/Target/Platform.cpp
  lldb/source/Target/RemoteAwarePlatform.cpp
  lldb/test/API/commands/platform/basic/TestPlatformCommand.py

Index: lldb/test/API/commands/platform/basic/TestPlatformCommand.py
===
--- lldb/test/API/commands/platform/basic/TestPlatformCommand.py
+++ lldb/test/API/commands/platform/basic/TestPlatformCommand.py
@@ -92,3 +92,24 @@
 "error: timed out waiting for shell command to complete"])
 self.expect("shell -t 1 --  sleep 3", error=True, substrs=[
 "error: timed out waiting for shell command to complete"])
+
+@expectedFailureAll(oslist=["windows"])
+@no_debug_info_test
+def test_shell_interpreter(self):
+""" Test a shell with a different interpreter """
+platform = self.dbg.GetSelectedPlatform()
+self.assertTrue(platform.IsValid())
+
+sh_cmd = lldb.SBPlatformShellCommand('/bin/sh', 'echo $0')
+self.assertIn('/bin/sh', sh_cmd.GetCommand())
+self.assertIn('echo $0', sh_cmd.GetCommand())
+
+err = platform.Run(sh_cmd)
+self.assertTrue(err.Success())
+self.assertIn("/bin/sh", sh_cmd.GetOutput())
+
+@expectedFailureAll(oslist=["windows"])
+@no_debug_info_test
+def test_host_shell_interpreter(self):
+""" Test the host platform shell with a different interpreter """
+self.expect("platform shell -h -i /bin/sh -- 'echo $0'", substrs=['/bin/sh'])
Index: lldb/source/Target/RemoteAwarePlatform.cpp
===
--- lldb/source/Target/RemoteAwarePlatform.cpp
+++ lldb/source/Target/RemoteAwarePlatform.cpp
@@ -170,16 +170,19 @@
   return error;
 }
 
-Status RemoteAwarePlatform::RunShellCommand(
-const char *command, const FileSpec _dir, int *status_ptr,
-int *signo_ptr, std::string *command_output,
-const Timeout ) {
+Status RemoteAwarePlatform::RunShellCommand(const char *command,
+const FileSpec _dir,
+int *status_ptr, int *signo_ptr,
+std::string *command_output,
+const Timeout ,
+const bool run_in_default_shell) {
   if (IsHost())
 return Host::RunShellCommand(command, working_dir, status_ptr, signo_ptr,
- command_output, timeout);
+ command_output, timeout, run_in_default_shell);
   if (m_remote_platform_sp)
 return m_remote_platform_sp->RunShellCommand(
-command, working_dir, status_ptr, signo_ptr, command_output, timeout);
+command, working_dir, status_ptr, signo_ptr, command_output, timeout,
+run_in_default_shell);
   return Status("unable to run a remote command without a platform");
 }
 
Index: lldb/source/Target/Platform.cpp
===
--- lldb/source/Target/Platform.cpp
+++ lldb/source/Target/Platform.cpp
@@ -1327,10 +1327,10 @@
 // process to exit
 std::string
 *command_output, // Pass nullptr if you don't want the command output
-const Timeout ) {
+const Timeout , const bool run_in_default_shell) {
   if (IsHost())
 return Host::RunShellCommand(command, working_dir, status_ptr, signo_ptr,
- command_output, timeout);
+ command_output, timeout, run_in_default_shell);
   else
 return Status("unimplemented");
 }
Index: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
===
--- lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
+++ lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
@@ -407,7 +407,8 @@
// the process to exit
   std::string
   *command_output, // Pass nullptr if you don't want the command output
-  const Timeout );
+  const Timeout ,
+  const bool run_in_default_shell = true);
 
   bool 

[Lldb-commits] [PATCH] D86667: [lldb/Target] Add custom interpreter option to `platform shell`

[Med Ismail Bennani via Phabricator via lldb-commits]

mib created this revision.
mib added reviewers: labath, JDevlieghere, jingham.
mib added a project: LLDB.
Herald added subscribers: lldb-commits, dang.
mib requested review of this revision.

This patch adds the ability to use a custom interpreter with the
`platform shell` command. If the user enables the `-i|--interpreter`
option, and passes it the path to a binary, lldb will prepend that path
before the rest of the command and set a flag so the host interpreter
isn't fetched.

The `Platform::RunShellCommand` method already had a default parameter
`m_run_in_default_shell`, so it was mostly a matter of hooking up the
the CommandObject flag to the method parameter.

rdar://67759256

Signed-off-by: Med Ismail Bennani 


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D86667

Files:
  lldb/bindings/interface/SBPlatform.i
  lldb/include/lldb/API/SBPlatform.h
  lldb/include/lldb/Target/Platform.h
  lldb/include/lldb/Target/RemoteAwarePlatform.h
  lldb/source/API/SBPlatform.cpp
  lldb/source/Commands/CommandObjectPlatform.cpp
  lldb/source/Commands/Options.td
  lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp
  lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.h
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
  lldb/source/Target/Platform.cpp
  lldb/source/Target/RemoteAwarePlatform.cpp
  lldb/test/API/commands/platform/basic/TestPlatformCommand.py

Index: lldb/test/API/commands/platform/basic/TestPlatformCommand.py
===
--- lldb/test/API/commands/platform/basic/TestPlatformCommand.py
+++ lldb/test/API/commands/platform/basic/TestPlatformCommand.py
@@ -92,3 +92,24 @@
 "error: timed out waiting for shell command to complete"])
 self.expect("shell -t 1 --  sleep 3", error=True, substrs=[
 "error: timed out waiting for shell command to complete"])
+
+@expectedFailureAll(oslist=["windows"])
+@no_debug_info_test
+def test_shell_interpreter(self):
+""" Test a shell with a different interpreter """
+platform = self.dbg.GetSelectedPlatform()
+self.assertTrue(platform.IsValid())
+
+sh_cmd = lldb.SBPlatformShellCommand('/bin/sh', 'echo $0')
+self.assertIn('/bin/sh', sh_cmd.GetCommand())
+self.assertIn('echo $0', sh_cmd.GetCommand())
+
+err = platform.Run(sh_cmd)
+self.assertTrue(err.Success())
+self.assertIn("/bin/sh", sh_cmd.GetOutput())
+
+@expectedFailureAll(oslist=["windows"])
+@no_debug_info_test
+def test_shell_interpreter(self):
+""" Test a shell with a different interpreter """
+self.expect("platform shell -h -i /bin/sh -- 'echo $0'", substrs=['/bin/sh'])
Index: lldb/source/Target/RemoteAwarePlatform.cpp
===
--- lldb/source/Target/RemoteAwarePlatform.cpp
+++ lldb/source/Target/RemoteAwarePlatform.cpp
@@ -170,16 +170,19 @@
   return error;
 }
 
-Status RemoteAwarePlatform::RunShellCommand(
-const char *command, const FileSpec _dir, int *status_ptr,
-int *signo_ptr, std::string *command_output,
-const Timeout ) {
+Status RemoteAwarePlatform::RunShellCommand(const char *command,
+const FileSpec _dir,
+int *status_ptr, int *signo_ptr,
+std::string *command_output,
+const Timeout ,
+const bool run_in_default_shell) {
   if (IsHost())
 return Host::RunShellCommand(command, working_dir, status_ptr, signo_ptr,
- command_output, timeout);
+ command_output, timeout, run_in_default_shell);
   if (m_remote_platform_sp)
 return m_remote_platform_sp->RunShellCommand(
-command, working_dir, status_ptr, signo_ptr, command_output, timeout);
+command, working_dir, status_ptr, signo_ptr, command_output, timeout,
+run_in_default_shell);
   return Status("unable to run a remote command without a platform");
 }
 
Index: lldb/source/Target/Platform.cpp
===
--- lldb/source/Target/Platform.cpp
+++ lldb/source/Target/Platform.cpp
@@ -1327,10 +1327,10 @@
 // process to exit
 std::string
 *command_output, // Pass nullptr if you don't want the command output
-const Timeout ) {
+const Timeout , const bool run_in_default_shell) {
   if (IsHost())
 return Host::RunShellCommand(command, working_dir, status_ptr, signo_ptr,
- command_output, timeout);
+ command_output, timeout, run_in_default_shell);
   else
 return 

[Lldb-commits] [PATCH] D86388: Fix use-after-free in ThreadPlan, and add test.

[Nicholas Allegra via Phabricator via lldb-commits]

comex added a comment.

Disabling the thread list while the target is running sounds like a pretty 
complex change.  For example, what should happen if a Python script calls 
`lldb.process.GetThreadAtIndex(n)` while the target is running, which currently 
works?

And is it really the right direction to be moving in?  Long-term, wouldn't it 
be better if user-facing commands like `thread list` worked while the target is 
running?  Or extra-long-term, one of the ideas on the LLDB projects page [1] is 
non-stop debugging (like GDB supports), where one thread is paused and can be 
inspected while other threads are still running.  That would require a model 
where threads are created and destroyed on the fly, without a rigid sequence of 
"resume and thread list goes away; stop and thread list comes back".

FWIW, this bug causes intermittent crashes whenever I try to debug xnu, so I'd 
like to get it fixed relatively quickly if possible.  Even just calculating 
ShouldReportRun earlier, while certainly doable, would be a considerably more 
complex change than this.

[1] https://lldb.llvm.org/status/projects.html#non-stop-debugging


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86388

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

[Lldb-commits] [PATCH] D85705: Add a "Trace" plug-in to LLDB to add process trace support in stages.

[walter erquinigo via Phabricator via lldb-commits]

wallace updated this revision to Diff 288145.
wallace added a comment.

- As error messaging is important when parsing, I create a new set of functions 
in StructuredData for parsing that return an llvm::Expected.

This allowed me to move the helper functions for parsing from the Trace file to 
StructuredData, so that in the future other contributors can
use those methods when needed.

- I modified the schema to allow for the "traceFile" entry at several levels.
- "systemPath" was also added for the module entry
- I didn't use the PlaceholderObject yet, as Greg suggested. That might involve 
a few other changes and I need to see if that class is sufficient for decoding 
intel-pt traces. However, as soon as I'm working on the decoding part, I'll 
move to PlaceholderObject, as it will be nice to avoid failing hard if a module 
is not available.
- I simplified the code here and there as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85705

Files:
  lldb/include/lldb/Core/PluginManager.h
  lldb/include/lldb/Target/Trace.h
  lldb/include/lldb/Utility/StructuredData.h
  lldb/include/lldb/lldb-forward.h
  lldb/include/lldb/lldb-private-interfaces.h
  lldb/source/Commands/CMakeLists.txt
  lldb/source/Commands/CommandObjectTrace.cpp
  lldb/source/Commands/CommandObjectTrace.h
  lldb/source/Commands/Options.td
  lldb/source/Core/PluginManager.cpp
  lldb/source/Interpreter/CommandInterpreter.cpp
  lldb/source/Plugins/CMakeLists.txt
  lldb/source/Plugins/Trace/CMakeLists.txt
  lldb/source/Plugins/Trace/intel-pt/CMakeLists.txt
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h
  lldb/source/Target/CMakeLists.txt
  lldb/source/Target/Trace.cpp
  lldb/source/Utility/StructuredData.cpp
  lldb/test/API/commands/trace/TestTraceLoad.py
  lldb/test/API/commands/trace/TestTraceSchema.py
  lldb/test/API/commands/trace/intelpt-trace/3842849.trace
  lldb/test/API/commands/trace/intelpt-trace/a.out
  lldb/test/API/commands/trace/intelpt-trace/main.cpp
  lldb/test/API/commands/trace/intelpt-trace/trace.json
  lldb/test/API/commands/trace/intelpt-trace/trace_bad.json
  lldb/test/API/commands/trace/intelpt-trace/trace_bad2.json

Index: lldb/test/API/commands/trace/intelpt-trace/trace_bad2.json
===
--- /dev/null
+++ lldb/test/API/commands/trace/intelpt-trace/trace_bad2.json
@@ -0,0 +1,12 @@
+{
+  "plugin": {
+"type": "intel-pt",
+"pt_cpu": {
+  "vendor": "intel",
+  "family": 6,
+  "model": 79,
+  "stepping": 1
+}
+  },
+  "processes": []
+}
Index: lldb/test/API/commands/trace/intelpt-trace/trace_bad.json
===
--- /dev/null
+++ lldb/test/API/commands/trace/intelpt-trace/trace_bad.json
@@ -0,0 +1,15 @@
+{
+  "plugin": {
+"type": "intel-pt",
+"pt_cpu": {
+  "vendor": "intel",
+  "family": 6,
+  "model": 79,
+  "stepping": 1
+}
+  },
+  "triple": "x86_64-*-linux",
+  "processes": [
+123
+  ]
+}
Index: lldb/test/API/commands/trace/intelpt-trace/trace.json
===
--- /dev/null
+++ lldb/test/API/commands/trace/intelpt-trace/trace.json
@@ -0,0 +1,31 @@
+{
+  "plugin": {
+"type": "intel-pt",
+"pt_cpu": {
+  "vendor": "intel",
+  "family": 6,
+  "model": 79,
+  "stepping": 1
+}
+  },
+  "triple": "x86_64-*-linux",
+  "processes": [
+{
+  "pid": 1234,
+  "threads": [
+{
+  "tid": 5678,
+  "traceFile": "3842849.trace"
+}
+  ],
+  "modules": [
+{
+  "file": "a.out",
+  "systemPath": "a.out",
+  "loadAddress": "0x0040",
+  "uuid": "6AA9A4E2-6F28-2F33-377D-59FECE874C71-5B41261A"
+}
+  ]
+}
+  ]
+}
Index: lldb/test/API/commands/trace/intelpt-trace/main.cpp
===
--- /dev/null
+++ lldb/test/API/commands/trace/intelpt-trace/main.cpp
@@ -0,0 +1,8 @@
+int main() {
+  int ret = 0;
+
+  for (int i = 0; i < 4; i++)
+ret ^= 1;
+
+  return ret;
+}
Index: lldb/test/API/commands/trace/TestTraceSchema.py
===
--- /dev/null
+++ lldb/test/API/commands/trace/TestTraceSchema.py
@@ -0,0 +1,22 @@
+import lldb
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+from lldbsuite.test.decorators import *
+
+class TestTraceLoad(TestBase):
+
+mydir = TestBase.compute_mydir(__file__)
+NO_DEBUG_INFO_TESTCASE = True
+
+def setUp(self):
+TestBase.setUp(self)
+if 'intel-pt' not in configuration.enabled_plugins:
+self.skipTest("The intel-pt test plugin is not enabled")
+
+
+def testSchema(self):
+self.expect("trace schema intel-pt", 

[Lldb-commits] [PATCH] D86375: Load correct module for linux and android when duplicates exist in minidump.

[Greg Clayton 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 rGc55db4600b4b: Load correct module for linux and android when 
duplicates exist in minidump. (authored by clayborg).

Changed prior to commit:
  https://reviews.llvm.org/D86375?vs=287853=288124#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86375

Files:
  lldb/source/Plugins/Process/minidump/MinidumpParser.cpp
  lldb/source/Plugins/Process/minidump/MinidumpParser.h
  lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp
  lldb/unittests/Process/minidump/MinidumpParserTest.cpp

Index: lldb/unittests/Process/minidump/MinidumpParserTest.cpp
===
--- lldb/unittests/Process/minidump/MinidumpParserTest.cpp
+++ lldb/unittests/Process/minidump/MinidumpParserTest.cpp
@@ -689,6 +689,130 @@
   EXPECT_EQ(0x1000u, filtered_modules[0]->BaseOfImage);
 }
 
+TEST_F(MinidumpParserTest, MinidumpDuplicateModuleMappedFirst) {
+  ASSERT_THAT_ERROR(SetUpFromYaml(R"(
+--- !minidump
+Streams:
+  - Type:ModuleList
+Modules:
+  - Base of Image:   0x400d
+Size of Image:   0x2000
+Module Name: '/usr/lib/libc.so'
+CodeView Record: ''
+  - Base of Image:   0x400d3000
+Size of Image:   0x1000
+Module Name: '/usr/lib/libc.so'
+CodeView Record: ''
+  - Type:LinuxMaps
+Text: |
+  400d-400d2000 r--p  b3:04 227/usr/lib/libc.so
+  400d2000-400d3000 rw-p  00:00 0
+  400d3000-400d4000 r-xp 0001 b3:04 227/usr/lib/libc.so
+  400d4000-400d5000 rwxp 1000 b3:04 227/usr/lib/libc.so
+...
+)"),
+llvm::Succeeded());
+  // If we have a module mentioned twice in the module list, and we have full
+  // linux maps for all of the memory regions, make sure we pick the one that
+  // has a consecutive region with a matching path that has executable
+  // permissions. If clients open an object file with mmap, breakpad can create
+  // multiple mappings for a library errnoneously and the lowest address isn't
+  // always the right address. In this case we check the consective memory
+  // regions whose path matches starting at the base of image address and make
+  // sure one of the regions is executable and prefer that one.
+  //
+  // This test will make sure that if the executable is second in the module
+  // list, that it will become the selected module in the filtered list.
+  std::vector filtered_modules =
+  parser->GetFilteredModuleList();
+  ASSERT_EQ(1u, filtered_modules.size());
+  EXPECT_EQ(0x400d3000u, filtered_modules[0]->BaseOfImage);
+}
+
+TEST_F(MinidumpParserTest, MinidumpDuplicateModuleMappedSecond) {
+  ASSERT_THAT_ERROR(SetUpFromYaml(R"(
+--- !minidump
+Streams:
+  - Type:ModuleList
+Modules:
+  - Base of Image:   0x400d
+Size of Image:   0x2000
+Module Name: '/usr/lib/libc.so'
+CodeView Record: ''
+  - Base of Image:   0x400d3000
+Size of Image:   0x1000
+Module Name: '/usr/lib/libc.so'
+CodeView Record: ''
+  - Type:LinuxMaps
+Text: |
+  400d-400d1000 r-xp 0001 b3:04 227/usr/lib/libc.so
+  400d1000-400d2000 rwxp 1000 b3:04 227/usr/lib/libc.so
+  400d2000-400d3000 rw-p  00:00 0
+  400d3000-400d5000 r--p  b3:04 227/usr/lib/libc.so
+...
+)"),
+llvm::Succeeded());
+  // If we have a module mentioned twice in the module list, and we have full
+  // linux maps for all of the memory regions, make sure we pick the one that
+  // has a consecutive region with a matching path that has executable
+  // permissions. If clients open an object file with mmap, breakpad can create
+  // multiple mappings for a library errnoneously and the lowest address isn't
+  // always the right address. In this case we check the consective memory
+  // regions whose path matches starting at the base of image address and make
+  // sure one of the regions is executable and prefer that one.
+  //
+  // This test will make sure that if the executable is first in the module
+  // list, that it will remain the correctly selected module in the filtered
+  // list.
+  std::vector filtered_modules =
+  parser->GetFilteredModuleList();
+  ASSERT_EQ(1u, filtered_modules.size());
+  EXPECT_EQ(0x400du, filtered_modules[0]->BaseOfImage);
+}
+
+TEST_F(MinidumpParserTest, MinidumpDuplicateModuleSeparateCode) {
+  ASSERT_THAT_ERROR(SetUpFromYaml(R"(
+--- !minidump
+Streams:
+  - Type:ModuleList
+Modules:
+  - Base of Image:   0x400d
+Size of Image:   0x2000
+Module Name: '/usr/lib/libc.so'
+

[Lldb-commits] [lldb] c55db46 - Load correct module for linux and android when duplicates exist in minidump.

[Greg Clayton via lldb-commits]

Author: Greg Clayton
Date: 2020-08-26T15:48:34-07:00
New Revision: c55db4600b4bdc5664784983fefb82bd8189bafc

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

LOG: Load correct module for linux and android when duplicates exist in 
minidump.

Breakpad creates minidump files that can a module loaded multiple times. We 
found that when a process mmap's the object file for a library, this can 
confuse breakpad into creating multiple modules in the module list. This patch 
fixes the GetFilteredModules() to check the linux maps for permissions and use 
the one that has execute permissions. Typically when people mmap a file into 
memory they don't map it as executable. This helps people to correctly load 
minidump files for post mortem analysis.

Differential Revision: https://reviews.llvm.org/D86375

Added: 


Modified: 
lldb/source/Plugins/Process/minidump/MinidumpParser.cpp
lldb/source/Plugins/Process/minidump/MinidumpParser.h
lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp
lldb/unittests/Process/minidump/MinidumpParserTest.cpp

Removed: 




diff  --git a/lldb/source/Plugins/Process/minidump/MinidumpParser.cpp 
b/lldb/source/Plugins/Process/minidump/MinidumpParser.cpp
index 0c7f4cbbb859..5f2982b3c09c 100644
--- a/lldb/source/Plugins/Process/minidump/MinidumpParser.cpp
+++ b/lldb/source/Plugins/Process/minidump/MinidumpParser.cpp
@@ -267,6 +267,88 @@ llvm::ArrayRef 
MinidumpParser::GetModuleList() {
   return {};
 }
 
+static bool
+CreateRegionsCacheFromLinuxMaps(MinidumpParser ,
+std::vector ) {
+  auto data = parser.GetStream(StreamType::LinuxMaps);
+  if (data.empty())
+return false;
+  ParseLinuxMapRegions(llvm::toStringRef(data),
+   [&](const lldb_private::MemoryRegionInfo ,
+   const lldb_private::Status ) -> bool {
+ if (status.Success())
+   regions.push_back(region);
+ return true;
+   });
+  return !regions.empty();
+}
+
+/// Check for the memory regions starting at \a load_addr for a contiguous
+/// section that has execute permissions that matches the module path.
+///
+/// When we load a breakpad generated minidump file, we might have the
+/// /proc//maps text for a process that details the memory map of the
+/// process that the minidump is describing. This checks the sorted memory
+/// regions for a section that has execute permissions. A sample maps files
+/// might look like:
+///
+/// 0040-00401000 r--p  fd:01 2838574   /tmp/a.out
+/// 00401000-00402000 r-xp 1000 fd:01 2838574   /tmp/a.out
+/// 00402000-00403000 r--p 2000 fd:01 2838574   /tmp/a.out
+/// 00403000-00404000 r--p 2000 fd:01 2838574   /tmp/a.out
+/// 00404000-00405000 rw-p 3000 fd:01 2838574   /tmp/a.out
+/// ...
+///
+/// This function should return true when given 0x0040 and "/tmp/a.out"
+/// is passed in as the path since it has a consecutive memory region for
+/// "/tmp/a.out" that has execute permissions at 0x00401000. This will help us
+/// 
diff erentiate if a file has been memory mapped into a process for reading
+/// and breakpad ends up saving a minidump file that has two module entries for
+/// a given file: one that is read only for the entire file, and then one that
+/// is the real executable that is loaded into memory for execution. For memory
+/// mapped files they will typically show up and r--p permissions and a range
+/// matcning the entire range of the file on disk:
+///
+/// 0080-00805000 r--p  fd:01 2838574   /tmp/a.out
+/// 00805000-00806000 r-xp 1000 fd:01 1234567   /usr/lib/libc.so
+///
+/// This function should return false when asked about 0x0080 with
+/// "/tmp/a.out" as the path.
+///
+/// \param[in] path
+///   The path to the module to check for in the memory regions. Only 
sequential
+///   memory regions whose paths match this path will be considered when 
looking
+///   for execute permissions.
+///
+/// \param[in] regions
+///   A sorted list of memory regions obtained from a call to
+///   CreateRegionsCacheFromLinuxMaps.
+///
+/// \param[in] base_of_image
+///   The load address of this module from BaseOfImage in the modules list.
+///
+/// \return
+///   True if a contiguous region of memory belonging to the module with a
+///   matching path exists that has executable permissions. Returns false if
+///   \a regions is empty or if there are no regions with execute permissions
+///   that match \a path.
+
+static bool CheckForLinuxExecutable(ConstString path,
+const MemoryRegionInfos ,
+

[Lldb-commits] [PATCH] D86615: [lldb/DWARF] Fix handling of variables with both location and const_value attributes

[Shafik Yaghmour via Phabricator via lldb-commits]

shafik accepted this revision.
shafik added a comment.

LGTM with minor comments. Thank you for these fixes, they are awesome!




Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:3213
+  bool use_type_size_for_value = false;
+  if (location_form.IsValid()) {
+has_explicit_location = true;

It might be helpful to document that `DW_AT_location` is taken over 
`DW_AT_const_value` and the types of cases this can show up in.



Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:3216
+if (DWARFFormValue::IsBlockForm(location_form.Form())) {
+  auto data = die.GetData();
+

`const DWARFDataExtractor&`?

I don't think `auto` adds any value here.





Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:3241
+// string.
+auto debug_info_data = die.GetData();
+if (DWARFFormValue::IsBlockForm(const_value_form.Form())) {

`const DWARFDataExtractor&`?



Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:3257
+} else if (const char *str = const_value_form.AsCString()) {
+  uint32_t string_length = strlen(str) + 1;
+  location = DWARFExpression(

aprantl wrote:
> If we do this a lot a StringRef DWARFFormValue::AsCStringRef() call would 
> make sense...
Why `+1`?



Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:3446
 SymbolFileTypeSP type_sp(
 new SymbolFileType(*this, GetUID(type_die_form.Reference(;
 

`make_shared`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86615

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

[Lldb-commits] [PATCH] D86662: Simplify Symbol Status Message to Only Debug Info Size

[walter erquinigo via Phabricator via lldb-commits]

wallace added inline comments.



Comment at: lldb/test/API/tools/lldb-vscode/module/TestVSCode_module.py:44
+# symbol_regex = re.compile(r"Symbols loaded. 
\([0-9]+(\.[0-9]*)?[KMG]?B\)")
+symbol_regex = re.compile(r"[0-9]+(\.[0-9]*)?[KMG]?B")
 return symbol_regex.match(program_module['symbolStatus'])

@aprantl , this is the part of the code that is testing the field change


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86662

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

[Lldb-commits] [PATCH] D86662: Simplify Symbol Status Message to Only Debug Info Size

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added a comment.

Could you please also include a testcase?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86662

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

[Lldb-commits] [PATCH] D86388: Fix use-after-free in ThreadPlan, and add test.

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

We should be able to calculate ShouldReportRun before we actually set the run 
going.  That's better than just querying potentially stale threads.  It would 
also be good to find a way to prevent ourselves from consulting the thread list 
after we've decided to invalidate it for run, but that's a second order 
consideration.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86388

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

[Lldb-commits] [PATCH] D86652: [lldb] Fix ProcessEventData::DoOnRemoval to support multiple listeners

[Jim Ingham via Phabricator via lldb-commits]

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

This change is fine for what it does, but I don't think the model that it 
allows is really supportable.  If you have multiple listeners on the process 
events, and Listener A wants to do a "step" when notified that the process has 
stopped, but Listener B wants to do a "Continue", then there's no way to 
coordinate these and we're just allowing race conditions in controlling the 
process.  I think you really need to have one agent that controls the process, 
and then other threads that are passive listeners.

My intention for this was to have the primary Listener (the one that was 
registered with the process when it was created) be the "controlling listener". 
 We would ensure that that listener gets notified first, and add some kind of 
handshake where the controlling listener relinquishes control, after which the 
other listeners will be informed.

Anyway, my feeling is we should think a bit more about policy w.r.t. fetching 
process events from multiple Process listeners before we allow them.

There should also be some test cases for handling a process with multiple 
listeners, since that's not something we've supported before.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86652

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

[Lldb-commits] [PATCH] D86662: Simplify Symbol Status Message to Only Debug Info Size

[walter erquinigo via Phabricator via lldb-commits]

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

For the record, this came out of discussion in which the consensus was to 
display less characters and maximize the relevant information, which in this 
case is just the debug info size.




Comment at: lldb/test/API/tools/lldb-vscode/module/TestVSCode_module.py:35
 self.assertTrue('symbolFilePath' not in program_module, 'Make sure 
a.out.stripped has no debug info')
-self.assertEqual('Symbols not found.', program_module['symbolStatus'])
+# self.assertEqual('Symbols not found.', 
program_module['symbolStatus'])
 symbols_path = self.getBuildArtifact(symbol_basename)

remove this line instead of commenting 



Comment at: lldb/test/API/tools/lldb-vscode/module/TestVSCode_module.py:43
 symbolsStatus = program_module['symbolStatus']
-symbol_regex = re.compile(r"Symbols loaded. 
\([0-9]+(\.[0-9]*)?[KMG]?B\)")
+# symbol_regex = re.compile(r"Symbols loaded. 
\([0-9]+(\.[0-9]*)?[KMG]?B\)")
+symbol_regex = re.compile(r"[0-9]+(\.[0-9]*)?[KMG]?B")

same here



Comment at: lldb/tools/lldb-vscode/JSONUtils.cpp:389
+  std::string debug_info_size = ConvertDebugInfoSizeToString(debug_info);
+  object.try_emplace("symbolStatus", debug_info_size);
 }

rename this field to "debugInfoSize"


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86662

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

[Lldb-commits] [PATCH] D86662: Simplify Symbol Status Message to Only Debug Info Size

[Yifan Shen via Phabricator via lldb-commits]

aelitashen created this revision.
aelitashen added reviewers: wallace, clayborg.
Herald added subscribers: lldb-commits, aprantl.
Herald added a project: LLDB.
aelitashen requested review of this revision.
Herald added a subscriber: JDevlieghere.

The Symbol Status in modules view is simplified so that only when the module 
has debug info and its size is non-zero, will the status message be displayed. 
The symbol status message is renamed to debug info size and flag message like 
"Symbols not found" and "Symbols loaded" is deleted.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D86662

Files:
  lldb/test/API/tools/lldb-vscode/module/TestVSCode_module.py
  lldb/tools/lldb-vscode/JSONUtils.cpp


Index: lldb/tools/lldb-vscode/JSONUtils.cpp
===
--- lldb/tools/lldb-vscode/JSONUtils.cpp
+++ lldb/tools/lldb-vscode/JSONUtils.cpp
@@ -354,7 +354,6 @@
 
 static std::string ConvertDebugInfoSizeToString(uint64_t debug_info) {
   std::ostringstream oss;
-  oss << " (";
   oss << std::fixed << std::setprecision(1);
 
   if (debug_info < 1024) {
@@ -370,7 +369,6 @@
 oss << gb << "GB";
 ;
   }
-  oss << ")";
   return oss.str();
 }
 llvm::json::Value CreateModule(lldb::SBModule ) {
@@ -385,19 +383,16 @@
   std::string module_path(module_path_arr);
   object.try_emplace("path", module_path);
   if (module.GetNumCompileUnits() > 0) {
-std::string symbol_str = "Symbols loaded.";
 uint64_t debug_info = GetDebugInfoSize(module);
 if (debug_info > 0) {
-  symbol_str += ConvertDebugInfoSizeToString(debug_info);
+  std::string debug_info_size = ConvertDebugInfoSizeToString(debug_info);
+  object.try_emplace("symbolStatus", debug_info_size);
 }
-object.try_emplace("symbolStatus", symbol_str);
 char symbol_path_arr[PATH_MAX];
 module.GetSymbolFileSpec().GetPath(symbol_path_arr,
sizeof(symbol_path_arr));
 std::string symbol_path(symbol_path_arr);
 object.try_emplace("symbolFilePath", symbol_path);
-  } else {
-object.try_emplace("symbolStatus", "Symbols not found.");
   }
   std::string loaded_addr = std::to_string(
   module.GetObjectFileHeaderAddress().GetLoadAddress(g_vsc.target));
Index: lldb/test/API/tools/lldb-vscode/module/TestVSCode_module.py
===
--- lldb/test/API/tools/lldb-vscode/module/TestVSCode_module.py
+++ lldb/test/API/tools/lldb-vscode/module/TestVSCode_module.py
@@ -32,26 +32,20 @@
 self.assertIn('path', program_module, 'make sure path is in module')
 self.assertEqual(program, program_module['path'])
 self.assertTrue('symbolFilePath' not in program_module, 'Make sure 
a.out.stripped has no debug info')
-self.assertEqual('Symbols not found.', program_module['symbolStatus'])
+# self.assertEqual('Symbols not found.', 
program_module['symbolStatus'])
 symbols_path = self.getBuildArtifact(symbol_basename)
 self.vscode.request_evaluate('`%s' % ('target symbols add -s "%s" 
"%s"' % (program, symbols_path)))
 
-def checkSymbolsLoaded():
-active_modules = self.vscode.get_active_modules()
-program_module = active_modules[program_basename]
-return 'Symbols loaded.' == program_module['symbolStatus']
-
 def checkSymbolsLoadedWithSize():
 active_modules = self.vscode.get_active_modules()
 program_module = active_modules[program_basename]
 symbolsStatus = program_module['symbolStatus']
-symbol_regex = re.compile(r"Symbols loaded. 
\([0-9]+(\.[0-9]*)?[KMG]?B\)")
+# symbol_regex = re.compile(r"Symbols loaded. 
\([0-9]+(\.[0-9]*)?[KMG]?B\)")
+symbol_regex = re.compile(r"[0-9]+(\.[0-9]*)?[KMG]?B")
 return symbol_regex.match(program_module['symbolStatus'])
 
 if expect_debug_info_size:
 self.waitUntil(checkSymbolsLoadedWithSize)
-else:
-self.waitUntil(checkSymbolsLoaded)
 active_modules = self.vscode.get_active_modules()
 program_module = active_modules[program_basename]
 self.assertEqual(program_basename, program_module['name'])


Index: lldb/tools/lldb-vscode/JSONUtils.cpp
===
--- lldb/tools/lldb-vscode/JSONUtils.cpp
+++ lldb/tools/lldb-vscode/JSONUtils.cpp
@@ -354,7 +354,6 @@
 
 static std::string ConvertDebugInfoSizeToString(uint64_t debug_info) {
   std::ostringstream oss;
-  oss << " (";
   oss << std::fixed << std::setprecision(1);
 
   if (debug_info < 1024) {
@@ -370,7 +369,6 @@
 oss << gb << "GB";
 ;
   }
-  oss << ")";
   return oss.str();
 }
 llvm::json::Value CreateModule(lldb::SBModule ) {
@@ -385,19 +383,16 @@
   std::string module_path(module_path_arr);
   object.try_emplace("path", module_path);
   if (module.GetNumCompileUnits() > 0) 

[Lldb-commits] [PATCH] D85705: Add a "Trace" plug-in to LLDB to add process trace support in stages.

[walter erquinigo via Phabricator via lldb-commits]

wallace added a comment.

Given that this feature is intended to be used by non debugger developers, I 
want to stick to using JSON as a default input format, as it's more ubiquitous 
than YAML and I prefer to make this decision based on the user experience than 
on the technical solution.  I like the YAML parser though, but converting JSON 
to YAML to parsing might through away relevant error messaging.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85705

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

[Lldb-commits] [PATCH] D86660: Modifying ImportDeclContext(...) to ensure that we also handle the case when the FieldDecl is an ArrayType whose ElementType is a RecordDecl

[Shafik Yaghmour via Phabricator via lldb-commits]

shafik created this revision.
shafik added reviewers: martong, teemperor.
Herald added a subscriber: rnkovacs.
shafik requested review of this revision.

When we fixed `ImportDeclContext(...)` in D71378 
 to make sure we complete each `FieldDecl` of 
a `RecordDecl` when we are importing the definition we missed the case where a 
`FeildDecl` was an `ArrayType` whose `ElementType` is a record.

This fix was motivated by a codegen crash during LLDB expression parsing. Since 
we were not importing the definition we were crashing during layout which 
required all the records be defined.


https://reviews.llvm.org/D86660

Files:
  clang/lib/AST/ASTImporter.cpp
  
lldb/test/API/commands/expression/codegen-crash-import-def-arraytype-element/Makefile
  
lldb/test/API/commands/expression/codegen-crash-import-def-arraytype-element/TestImportDefinitionArrayType.py
  
lldb/test/API/commands/expression/codegen-crash-import-def-arraytype-element/main.cpp

Index: lldb/test/API/commands/expression/codegen-crash-import-def-arraytype-element/main.cpp
===
--- /dev/null
+++ lldb/test/API/commands/expression/codegen-crash-import-def-arraytype-element/main.cpp
@@ -0,0 +1,51 @@
+// This is a reproducer for a crash during codegen. The base issue is when we
+// Import the DeclContext we force FieldDecl that are RecordType to be defined
+// since we need these to be defined in order to layout the class.
+// This case involves an array member whose ElementType are records. In this
+// case we need to check the ElementType of an ArrayType and if it is a record
+// we need to import the definition.
+struct A {
+  int x;
+};
+
+struct B {
+  // When we import the all the FieldDecl we need to check if we have an
+  // ArrayType and then check if the ElementType is a RecordDecl and if so
+  // import the defintion. Otherwise during codegen we will attempt to layout A
+  // but won't be able to.
+  A s[2];
+  char o;
+};
+
+class FB {
+public:
+  union {
+struct {
+  unsigned char *_s;
+} t;
+char *tt[1];
+  } U;
+
+  FB(B *p) : __private(p) {}
+
+  // We import A but we don't import the definition.
+  void f(A **bounds) {}
+
+  void init();
+
+private:
+  B *__private;
+};
+
+void FB::init() {
+  return; // break here
+}
+
+int main() {
+  B b;
+  FB fb();
+
+  b.o = 'A';
+
+  fb.init();
+}
Index: lldb/test/API/commands/expression/codegen-crash-import-def-arraytype-element/TestImportDefinitionArrayType.py
===
--- /dev/null
+++ lldb/test/API/commands/expression/codegen-crash-import-def-arraytype-element/TestImportDefinitionArrayType.py
@@ -0,0 +1,14 @@
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+class TestImportDefinitionArrayType(TestBase):
+
+mydir = TestBase.compute_mydir(__file__)
+
+def test(self):
+self.build()
+lldbutil.run_to_source_breakpoint(self, "// break here", lldb.SBFileSpec("main.cpp"))
+
+self.expect_expr("__private->o", result_type="char", result_value="'A'")
Index: lldb/test/API/commands/expression/codegen-crash-import-def-arraytype-element/Makefile
===
--- /dev/null
+++ lldb/test/API/commands/expression/codegen-crash-import-def-arraytype-element/Makefile
@@ -0,0 +1,3 @@
+CXX_SOURCES := main.cpp
+
+include Makefile.rules
Index: clang/lib/AST/ASTImporter.cpp
===
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -1743,12 +1743,34 @@
   Decl *ImportedDecl = *ImportedOrErr;
   FieldDecl *FieldTo = dyn_cast_or_null(ImportedDecl);
   if (FieldFrom && FieldTo) {
-const RecordType *RecordFrom = FieldFrom->getType()->getAs();
-const RecordType *RecordTo = FieldTo->getType()->getAs();
-if (RecordFrom && RecordTo) {
-  RecordDecl *FromRecordDecl = RecordFrom->getDecl();
-  RecordDecl *ToRecordDecl = RecordTo->getDecl();
+RecordDecl *FromRecordDecl = nullptr;
+RecordDecl *ToRecordDecl = nullptr;
+// If we have a field that is an ArrayType we need to check if the array
+// element is a RecordDecl and if so we need to import the defintion.
+const ArrayType *ArrayFrom =
+FieldFrom->getType()->getAsArrayTypeUnsafe();
+const ArrayType *ArrayTo = FieldTo->getType()->getAsArrayTypeUnsafe();
+
+if (ArrayFrom && ArrayTo) {
+  QualType FromTy = ArrayFrom->getElementType();
+  QualType ToTy = ArrayTo->getElementType();
+
+  FromRecordDecl = FromTy->getAsRecordDecl();
+  ToRecordDecl = ToTy->getAsRecordDecl();
+}
+
+if (!FromRecordDecl || !ToRecordDecl) {
+  const RecordType *RecordFrom =
+  

[Lldb-commits] [PATCH] D85705: Add a "Trace" plug-in to LLDB to add process trace support in stages.

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

In D85705#2237506 , @clayborg wrote:

> In D85705#2237397 , @JDevlieghere 
> wrote:
>
>> A large part of this patch is concerned with parsing which worries me from a 
>> maintenance perspective. Did you consider using Yaml I/O 
>> ? While I'm not a particularly big fan of 
>> the format, the benefits of being able to (de)serialize any class by 
>> implementing the appropriate traits are quite significant. We already have 
>> traits implemented for a bunch of utility classes, such as `ArchSpec` and 
>> `FileSpec` which we could reuse for this. I know changing the format would 
>> be invasive, but I think it might be worth it in the long term.
>
> So the nice thing about using StructuredData is it can be in any format: 
> JSON, XML, Apple property list, YAML etc. It seems like the functions that 
> were added to ArchSpec and FileSpec for the YAML I/O could be converted to 
> use StructuredData and then any of the formats would work.

If we invest in creating a library like Yaml IO for StructuredData, which 
allows you to declaratively specify how a class gets (de)serialized, then I'm 
all for using it here and for the reproducers. I think that would be really, 
really valuable. Given the way that structured data works and how similar it is 
to how these formats look, I'm also fairly confident it can be done. Still it 
would be a significant amount of work. But barring that I don't see a benefit 
from switching from YAML I/O to StructuredData, at least not for the 
reproducers and not really for this either. Unless we need to be able to 
support all these formats? Like I said, I don't really care about YAML. What I 
do care about is that I don't want to be in the business of writing a parser 
for every data structure I need to serialize. That's really where Yaml I/O 
shines, even though it has other drawbacks, like the traits themselves being 
verbose and the parser being far from fast.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85705

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

[Lldb-commits] [PATCH] D86652: [lldb] Fix ProcessEventData::DoOnRemoval to support multiple listeners

[Ilya Nozhkin via Phabricator via lldb-commits]

ilya-nozhkin created this revision.
ilya-nozhkin added reviewers: jingham, clayborg, labath.
ilya-nozhkin added a project: LLDB.
Herald added subscribers: lldb-commits, JDevlieghere.
ilya-nozhkin requested review of this revision.

Process does something with its state and threads when an event is
removed from some public listener's event queue. The comment at the
beginning of DoOnRemoval and the condition checking m_update_state
tell that all these actions on the process state and threads must
be executed only once and after the private listener has already
handled an event. However, there is no any modification of
m_update_state except for invocation of SetUpdateStateOnRemoval
in HandlePrivateEvent. It means that m_update_state becomes 1 after an
event is handled by the private listener and is not changed anymore.
So, if more than one listener listen for process events
(for example, if someone add a listener following the example [1])
then the code in DoOnRemoval may be executed more than once. Moreover,
since events are handled asynchronously, DoOnRemoval may be executed by
two threads simultaneously that leads to data race.

This commit replaces m_update_state-based mechanism with a check of the
event broadcaster at the beginning of DoOnRemoval and a boolean flag
telling whether DoOnRemoval's logic has been already executed.
Also, there added a mutex which is locked during the whole execution of
DoOnRemoval. It makes DoOnRemoval thread-safe and ensures that the
public state of a process will be correct for all threads invoking
DoOnRemoval.

[1] https://lldb.llvm.org/python_reference/lldb.SBEvent-class.html


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D86652

Files:
  lldb/include/lldb/Target/Process.h
  lldb/source/Target/Process.cpp
  lldb/unittests/Process/ProcessEventDataTest.cpp

Index: lldb/unittests/Process/ProcessEventDataTest.cpp
===
--- lldb/unittests/Process/ProcessEventDataTest.cpp
+++ lldb/unittests/Process/ProcessEventDataTest.cpp
@@ -169,7 +169,7 @@
   ProcessEventDataSP event_data_sp =
   std::make_shared(process_sp, eStateStopped);
   EventSP event_sp = std::make_shared(0, event_data_sp);
-  event_data_sp->SetUpdateStateOnRemoval(event_sp.get());
+  process_sp->BroadcastEvent(event_sp);
   event_data_sp->DoOnRemoval(event_sp.get());
   bool result = static_cast(event_data_sp.get())
 ->m_should_stop_hit_count == 1;
@@ -181,7 +181,7 @@
   event_data_sp =
   std::make_shared(process_sp, eStateStepping);
   event_sp = std::make_shared(0, event_data_sp);
-  event_data_sp->SetUpdateStateOnRemoval(event_sp.get());
+  process_sp->BroadcastEvent(event_sp);
   event_data_sp->DoOnRemoval(event_sp.get());
   result = static_cast(event_data_sp.get())
->m_should_stop_hit_count == 0;
Index: lldb/source/Target/Process.cpp
===
--- lldb/source/Target/Process.cpp
+++ lldb/source/Target/Process.cpp
@@ -3774,7 +3774,6 @@
 StateAsCString(GetState()),
 is_hijacked ? "hijacked" : "public");
 }
-Process::ProcessEventData::SetUpdateStateOnRemoval(event_sp.get());
 if (StateIsRunningState(new_state)) {
   // Only push the input handler if we aren't fowarding events, as this
   // means the curses GUI is in use... Or don't push it if we are launching
@@ -3987,12 +3986,12 @@
 
 Process::ProcessEventData::ProcessEventData()
 : EventData(), m_process_wp(), m_state(eStateInvalid), m_restarted(false),
-  m_update_state(0), m_interrupted(false) {}
+  m_do_on_removal_executed(false), m_interrupted(false) {}
 
 Process::ProcessEventData::ProcessEventData(const ProcessSP _sp,
 StateType state)
 : EventData(), m_process_wp(), m_state(state), m_restarted(false),
-  m_update_state(0), m_interrupted(false) {
+  m_do_on_removal_executed(false), m_interrupted(false) {
   if (process_sp)
 m_process_wp = process_sp;
 }
@@ -4117,20 +4116,34 @@
 }
 
 void Process::ProcessEventData::DoOnRemoval(Event *event_ptr) {
-  ProcessSP process_sp(m_process_wp.lock());
+  // This function gets called each time an event gets pulled off of some
+  // listener's event queue. However, all the code below must be executed only
+  // once and after an event is already handled by the private state listener.
+  // So, here are two guards: the first of them checks that this event has been
+  // broadcasted by the public broadcaster and the second checks that this code
+  // hasn't been executed yet.
+
+  Broadcaster *broadcaster = event_ptr->GetBroadcaster();
+  if (!broadcaster ||
+  broadcaster->GetBroadcasterName() != Process::GetStaticBroadcasterClass())
+return;
 
-  if (!process_sp)
+  // Since different listeners are usually monitored by different threads, two
+  // threads may enter this function simultaneously. This lock 

[Lldb-commits] [PATCH] D86497: [lldb] Add reproducer verifier

[Jonas Devlieghere via Phabricator via lldb-commits]

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

- Address code review feedback
- Run the verifier when replaying a reproducer
- Add SBReplayOptions instead of adding another overload
- Add `--reproducer-skip-verify` flag


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

https://reviews.llvm.org/D86497

Files:
  lldb/include/lldb/API/SBReproducer.h
  lldb/include/lldb/Utility/Reproducer.h
  lldb/source/API/SBReproducer.cpp
  lldb/source/Commands/CommandObjectReproducer.cpp
  lldb/source/Commands/Options.td
  lldb/source/Utility/Reproducer.cpp
  lldb/source/Utility/ReproducerProvider.cpp
  lldb/test/Shell/Reproducer/TestDebugSymbols.test
  lldb/test/Shell/Reproducer/TestVerify.test
  lldb/tools/driver/Driver.cpp
  lldb/tools/driver/Options.td
  llvm/include/llvm/Support/VirtualFileSystem.h
  llvm/lib/Support/VirtualFileSystem.cpp

Index: llvm/lib/Support/VirtualFileSystem.cpp
===
--- llvm/lib/Support/VirtualFileSystem.cpp
+++ llvm/lib/Support/VirtualFileSystem.cpp
@@ -1159,6 +1159,17 @@
   return ExternalContentsPrefixDir;
 }
 
+void RedirectingFileSystem::setFallthrough(bool Fallthrough) {
+  IsFallthrough = Fallthrough;
+}
+
+std::vector RedirectingFileSystem::getRoots() const {
+  std::vector R;
+  for (const auto  : Roots)
+R.push_back(Root->getName());
+  return R;
+}
+
 void RedirectingFileSystem::dump(raw_ostream ) const {
   for (const auto  : Roots)
 dumpEntry(OS, Root.get());
Index: llvm/include/llvm/Support/VirtualFileSystem.h
===
--- llvm/include/llvm/Support/VirtualFileSystem.h
+++ llvm/include/llvm/Support/VirtualFileSystem.h
@@ -749,6 +749,10 @@
 
   StringRef getExternalContentsPrefixDir() const;
 
+  void setFallthrough(bool Fallthrough);
+
+  std::vector getRoots() const;
+
   void dump(raw_ostream ) const;
   void dumpEntry(raw_ostream , Entry *E, int NumSpaces = 0) const;
 #if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
Index: lldb/tools/driver/Options.td
===
--- lldb/tools/driver/Options.td
+++ lldb/tools/driver/Options.td
@@ -234,6 +234,8 @@
   HelpText<"Tells the debugger to replay a reproducer from .">;
 def no_version_check: F<"reproducer-no-version-check">,
   HelpText<"Disable the reproducer version check.">;
+def no_verification: F<"reproducer-no-verify">,
+  HelpText<"Disable the reproducer verification.">;
 def no_generate_on_signal: F<"reproducer-no-generate-on-signal">,
   HelpText<"Don't generate reproducer when a signal is received.">;
 def generate_on_exit: F<"reproducer-generate-on-exit">,
Index: lldb/tools/driver/Driver.cpp
===
--- lldb/tools/driver/Driver.cpp
+++ lldb/tools/driver/Driver.cpp
@@ -800,9 +800,11 @@
 llvm::Optional InitializeReproducer(llvm::StringRef argv0,
  opt::InputArgList _args) {
   if (auto *replay_path = input_args.getLastArg(OPT_replay)) {
-const bool no_version_check = input_args.hasArg(OPT_no_version_check);
+SBReplayOptions replay_options;
+replay_options.SetSkipVersionCheck(input_args.hasArg(OPT_no_version_check));
+replay_options.SetSkipVerification(input_args.hasArg(OPT_no_verification));
 if (const char *error =
-SBReproducer::Replay(replay_path->getValue(), no_version_check)) {
+SBReproducer::Replay(replay_path->getValue(), replay_options)) {
   WithColor::error() << "reproducer replay failed: " << error << '\n';
   return 1;
 }
Index: lldb/test/Shell/Reproducer/TestVerify.test
===
--- /dev/null
+++ lldb/test/Shell/Reproducer/TestVerify.test
@@ -0,0 +1,26 @@
+# RUN: rm -rf %t.repro
+# RUN: %clang_host %S/Inputs/simple.c -g -o %t.out
+# RUN: %lldb -x -b -s %S/Inputs/GDBRemoteCapture.in --capture --capture-path %t.repro %t.out
+# RUN: %lldb --replay %t.repro
+
+# RUN: echo "/bogus/home/dir" > %t.repro/home.txt
+# RUN: echo "/bogus/current/working/dir" > %t.repro/cwd.txt
+
+# RUN: not %lldb -b -o 'reproducer verify -f %t.repro' 2>&1 | FileCheck %s
+# CHECK: working directory '/bogus/current/working/dir' not in VFS
+# CHECK: home directory '/bogus/current/working/dir' not in VFS
+
+# RUN: rm %t.repro/root/%S/Inputs/GDBRemoteCapture.in
+# RUN: echo "CHECK: '%S/Inputs/GDBRemoteCapture.in': No such file or directory" > %t.check
+# RUN: not %lldb -b -o 'reproducer verify -f %t.repro' 2>&1 | FileCheck %t.check
+
+# RUN: not %lldb --replay %t.repro 2>&1 | FileCheck %s
+
+# At this point the reproducer is too broken to pass testing with
+# --reproducer-no-verify. Capture a new reproducer and only change the home
+# directory, which is recoverable as far as this test goes.
+
+# RUN: %lldb -x -b -s %S/Inputs/GDBRemoteCapture.in --capture --capture-path %t.repro2 %t.out
+# RUN: echo 

[Lldb-commits] [PATCH] D86417: [lldb] do not propagate eTrapHandlerFrame repeatedly

[Joseph Tremoulet via Phabricator via lldb-commits]

JosephTremoulet added a comment.

To clarify, the failure did not reproduce for me in the SUSE 15.02 container.  
I don't know what I'm doing differently.  To create the environment, I just did 
`docker run ... opensuse/leap:15.2`, and then `zypper in gcc-c++ python3-devel` 
inside the container.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D86417

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

[Lldb-commits] [PATCH] D86497: [lldb] Add reproducer verifier

[Raphael Isemann via Phabricator via lldb-commits]

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

As discussed offline, I think the warnings here seem also useful when just 
replaying. I guess we should make it clear that things like missing files are 
fine when replaying a reproducer (e.g., just pointing that out as a 'note' and 
not warning/error). Beside that this LGTM.




Comment at: lldb/source/Commands/CommandObjectReproducer.cpp:146
+
+/// Create a loader form the given path if specified. Otherwise use the current
+/// loader used for replay.

`form` -> `from`



Comment at: lldb/source/Commands/CommandObjectReproducer.cpp:156
+if (Error err = loader->LoadIndex()) {
+  // This is a hard error and will set the result tot eReturnStatusFailed.
+  SetError(result, std::move(err));

`tot` -> `to`



Comment at: lldb/source/Commands/CommandObjectReproducer.cpp:603
+"If no reproducer is specified during replay, it "
+"dumps the content of the current reproducer.",
+nullptr) {}

Is that dumping implemented? It's also kind of surprising that `verify` dumps 
the current reproducer instead of verifying it.


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

https://reviews.llvm.org/D86497

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

[Lldb-commits] [PATCH] D86615: [lldb/DWARF] Fix handling of variables with both location and const_value attributes

[Adrian Prantl via Phabricator via lldb-commits]

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

What a function :-)




Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:3134
   if ((tag == DW_TAG_variable) || (tag == DW_TAG_constant) ||
   (tag == DW_TAG_formal_parameter && sc.function)) {
 DWARFAttributes attributes;

Would be nice to early-exit here, too.



Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:3257
+} else if (const char *str = const_value_form.AsCString()) {
+  uint32_t string_length = strlen(str) + 1;
+  location = DWARFExpression(

If we do this a lot a StringRef DWARFFormValue::AsCStringRef() call would make 
sense...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86615

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

[Lldb-commits] [PATCH] D86417: [lldb] do not propagate eTrapHandlerFrame repeatedly

[Joseph Tremoulet via Phabricator via lldb-commits]

JosephTremoulet added a comment.

FWIW, I tried running the test in an SUSE 15.2 docker container (on an Ubuntu 
18.04 host), and here's what I'm seeing.  `raise` and `abort` indeed do not 
indicate that they had the `S` set in the augmentation:

  (lldb) bt
   * thread #1, name = 'a.out', stop reason = breakpoint 2.1
* frame #0: 0x0040064b a.out`handler(sig=6) at main.c:7:5
  frame #1: 0x771245a0 libc.so.6`__restore_rt
  frame #2: 0x77124520 libc.so.6`raise + 272
  frame #3: 0x77125b01 libc.so.6`abort + 337
  frame #4: 0x00400679 a.out`abort_caller at main.c:12:5
  frame #5: 0x004006d3 a.out`main at main.c:23:5
  frame #6: 0x7710f34a libc.so.6`__libc_start_main + 234
  frame #7: 0x0040058a a.out`_start at start.S:120



  (lldb) image show-unwind -n __restore_rt
  UNWIND PLANS for ld-2.26.so`__restore_rt (start addr 0x77df2270)
  This function's name is listed by the platform as a trap handler.
  
  Asynchronous (not restricted to call-sites) UnwindPlan is 'eh_frame CFI'
  Synchronous (restricted to call-sites) UnwindPlan is 'eh_frame CFI'
  
  Assembly language inspection UnwindPlan:
  This UnwindPlan originally sourced from assembly insn profiling
  This UnwindPlan is sourced from the compiler: no.
  This UnwindPlan is valid at all instruction locations: yes.
  This UnwindPlan is for a trap handler function: no.
  Address range of this UnwindPlan: [ld-2.26.so.PT_LOAD[0]..text + 
107856-0x0001a560)
  row[0]:0: CFA=rsp +8 => rsp=CFA+0 rip=[CFA-8] 
  
  eh_frame UnwindPlan:
  This UnwindPlan originally sourced from eh_frame CFI
  This UnwindPlan is sourced from the compiler: yes.
  This UnwindPlan is valid at all instruction locations: no.
  This UnwindPlan is for a trap handler function: yes.
  Address range of this UnwindPlan: [ld-2.26.so.PT_LOAD[0]..text + 
107855-0x0001a559)
  row[0]:0: CFA=DW_OP_breg7 +160, DW_OP_deref => rax=[DW_OP_breg7 +144] 
rdx=[DW_OP_breg7 +136] rcx=[DW_OP_breg7 +152] rbx=[DW_OP_breg7 +128] 
rsi=[DW_OP_breg7 +112] rdi=[DW_OP_breg7 +104] rbp=[DW_OP_breg7 +120] 
rsp=[DW_OP_breg7 +160] r8=[DW_OP_breg7 +40] r9=[DW_OP_breg7 +48] 
r10=[DW_OP_breg7 +56] r11=[DW_OP_breg7 +64] r12=[DW_OP_breg7 +72] 
r13=[DW_OP_breg7 +80] r14=[DW_OP_breg7 +88] r15=[DW_OP_breg7 +96] 
rip=[DW_OP_breg7 +168] 
  
  Arch default UnwindPlan:
  This UnwindPlan originally sourced from x86_64 default unwind plan
  This UnwindPlan is sourced from the compiler: no.
  This UnwindPlan is valid at all instruction locations: no.
  This UnwindPlan is for a trap handler function: no.
  row[0]:0: CFA=rbp+16 => rbp=[CFA-16] rsp=CFA+0 rip=[CFA-8] 
  
  Arch default at entry point UnwindPlan:
  This UnwindPlan originally sourced from x86_64 at-func-entry default
  This UnwindPlan is sourced from the compiler: no.
  This UnwindPlan is valid at all instruction locations: not specified.
  This UnwindPlan is for a trap handler function: not specified.
  row[0]:0: CFA=rsp +8 => rsp=CFA+0 rip=[CFA-8] 
  
  
  UNWIND PLANS for libc.so.6`__restore_rt (start addr 0x771245a0)
  This function's name is listed by the platform as a trap handler.
  
  Asynchronous (not restricted to call-sites) UnwindPlan is 'eh_frame CFI'
  Synchronous (restricted to call-sites) UnwindPlan is 'eh_frame CFI'
  
  Assembly language inspection UnwindPlan:
  This UnwindPlan originally sourced from assembly insn profiling
  This UnwindPlan is sourced from the compiler: no.
  This UnwindPlan is valid at all instruction locations: yes.
  This UnwindPlan is for a trap handler function: no.
  Address range of this UnwindPlan: [libc.so.6.PT_LOAD[0]..text + 
88624-0x00015a40)
  row[0]:0: CFA=rsp +8 => rsp=CFA+0 rip=[CFA-8] 
  
  eh_frame UnwindPlan:
  This UnwindPlan originally sourced from eh_frame CFI
  This UnwindPlan is sourced from the compiler: yes.
  This UnwindPlan is valid at all instruction locations: no.
  This UnwindPlan is for a trap handler function: yes.
  Address range of this UnwindPlan: [libc.so.6.PT_LOAD[0]..text + 
88623-0x00015a39)
  row[0]:0: CFA=DW_OP_breg7 +160, DW_OP_deref => rax=[DW_OP_breg7 +144] 
rdx=[DW_OP_breg7 +136] rcx=[DW_OP_breg7 +152] rbx=[DW_OP_breg7 +128] 
rsi=[DW_OP_breg7 +112] rdi=[DW_OP_breg7 +104] rbp=[DW_OP_breg7 +120] 
rsp=[DW_OP_breg7 +160] r8=[DW_OP_breg7 +40] r9=[DW_OP_breg7 +48] 
r10=[DW_OP_breg7 +56] r11=[DW_OP_breg7 +64] r12=[DW_OP_breg7 +72] 
r13=[DW_OP_breg7 +80] r14=[DW_OP_breg7 +88] r15=[DW_OP_breg7 +96] 
rip=[DW_OP_breg7 +168] 
  
  Arch default UnwindPlan:
  This UnwindPlan originally sourced from x86_64 default unwind plan
  This UnwindPlan is sourced from the compiler: no.
  This UnwindPlan is valid at all instruction locations: no.
  This UnwindPlan is for a trap handler function: no.
  row[0]:0: CFA=rbp+16 => rbp=[CFA-16] rsp=CFA+0 rip=[CFA-8] 
  
  Arch default at entry point UnwindPlan:
  This UnwindPlan originally sourced from x86_64 

[Lldb-commits] [PATCH] D86616: [cmake] Make gtest include directories a part of the library interface

[Siva Chandra via Phabricator via lldb-commits]

sivachandra accepted this revision.
sivachandra added a comment.

Libc change LGTM.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86616

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

[Lldb-commits] [PATCH] D86292: [LLDB][RISCV] Distinguish between riscv32 and riscv64 based on ELF class

[Jessica Clarke via Phabricator via lldb-commits]

jrtc27 added a comment.

Not so silly; gdb (well, the names are inherited from bfd) has `set 
architecture riscv:rv32`/`set architecture riscv:rv64` :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86292

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

[Lldb-commits] [PATCH] D85820: Use find_library for ncurses

[Petr Hosek via Phabricator via lldb-commits]

phosek added a comment.

In D85820#2235461 , @haampie wrote:

> @gkistanova It's true that this change has lead to more issues I could ever 
> imagine, but I think the link you provided is the last remaining problem.
>
> Pinging @phosek for a similar issue w.r.t. zlib: since 
> https://reviews.llvm.org/D79219 zlib gets disabled on static builds of LLVM.  
> The reason is `find_package(ZLIB)` finds a shared library, and 
> `check_symbol_exists(compress2 zlib.h HAVE_ZLIB)` tries to statically link to 
> that -- it can't so zlib is disabled. I guess that's a regression too?
>
> What would be the best way forward @phosek? A quick fix for ncurses is to add 
> a simple `check_symbol_exists` test too, but that would in practice just 
> disable ncurses in static builds. And additionally we could find static libs 
> by adding explicit names like `libtinfo.a` to find_library when 
> `LLVM_BUILD_STATIC=ON`. This trick won't help for zlib though, the 
> find_library stuff is hidden inside find_package, and there is no way to 
> target static libs it seems.

@haampie `find_package` and `find_library` uses `CMAKE_FIND_LIBRARY_SUFFIXES` 
to determine the suffix of the library (the default is `".so" ".a"` which is 
why it always picks shared library). I think we should extend this block 
https://github.com/llvm/llvm-project/blob/master/llvm/CMakeLists.txt#L626 and 
filter out `CMAKE_SHARED_LIBRARY_SUFFIX` from `CMAKE_FIND_LIBRARY_SUFFIXES` 
when `LLVM_BUILD_STATIC` is enabled which will handle this case globally.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85820

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

[Lldb-commits] [PATCH] D86521: Revert "Use find_library for ncurses"

[Galina via Phabricator via lldb-commits]

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

I have checked this patch with 
http://lab.llvm.org:8011/builders/lld-perf-testsuite bot. It makes it green.

Thanks for reverting, Harmen!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86521

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

[Lldb-commits] [PATCH] D86292: [LLDB][RISCV] Distinguish between riscv32 and riscv64 based on ELF class

[Luís Marques via Phabricator via lldb-commits]

luismarques added a comment.

In D86292#2237143 , @jrtc27 wrote:

> Not so silly; gdb (well, the names are inherited from bfd) has `set 
> architecture riscv:rv32`/`set architecture riscv:rv64` :)

Ha! We're in good company then. Thanks for sharing, Jessica.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86292

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

[Lldb-commits] [PATCH] D86417: [lldb] do not propagate eTrapHandlerFrame repeatedly

[Joseph Tremoulet via Phabricator via lldb-commits]

JosephTremoulet added a comment.

> eh_frame augmented UnwindPlan:
> ...
> This UnwindPlan is for a trap handler function: yes.

From memory, I'd have expected that for `__restore_rt`, but not for `raise` or 
`abort`.  Can you dump the augmentation fields from the CIEs (or instrument 
`FDEToUnwindPlan`) to verify we're getting that right?


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D86417

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

[Lldb-commits] [lldb] 4a15f51 - [lldb][NFC] Simplify string literal in GDBRemoteCommunicationClient

[Raphael Isemann via lldb-commits]

Author: Raphael Isemann
Date: 2020-08-26T16:25:11+02:00
New Revision: 4a15f51a4f7726e12c327fa30e76d90a2b90430b

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

LOG: [lldb][NFC] Simplify string literal in GDBRemoteCommunicationClient

Added: 


Modified: 
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp

Removed: 




diff  --git 
a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp 
b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
index 2e6d174e4674..0949b9918523 100644
--- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
+++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
@@ -1701,14 +1701,9 @@ Status 
GDBRemoteCommunicationClient::GetWatchpointSupportInfo(uint32_t ) {
   // Set num to 0 first.
   num = 0;
   if (m_supports_watchpoint_support_info != eLazyBoolNo) {
-char packet[64];
-const int packet_len =
-::snprintf(packet, sizeof(packet), "qWatchpointSupportInfo:");
-assert(packet_len < (int)sizeof(packet));
-UNUSED_IF_ASSERT_DISABLED(packet_len);
 StringExtractorGDBRemote response;
-if (SendPacketAndWaitForResponse(packet, response, false) ==
-PacketResult::Success) {
+if (SendPacketAndWaitForResponse("qWatchpointSupportInfo:", response,
+ false) == PacketResult::Success) {
   m_supports_watchpoint_support_info = eLazyBoolYes;
   llvm::StringRef name;
   llvm::StringRef value;



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

[Lldb-commits] [PATCH] D86616: [cmake] Make gtest include directories a part of the library interface

[Raphael Isemann via Phabricator via lldb-commits]

teemperor accepted this revision.
teemperor added a comment.

LGTM too


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86616

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

[Lldb-commits] [PATCH] D86616: [cmake] Make gtest include directories a part of the library interface

[Louis Dionne via Phabricator via lldb-commits]

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

LGTM, but I'm not an owner for any of the projects touched by this change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86616

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

[Lldb-commits] [PATCH] D86436: [lldb] Fix Type::GetByteSize for pointer types

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D86436#2234110 , @davide wrote:

> @labath something I noticed when finding this (and related bugs) is that 
> `frame var` carries a decent diagnostic
>
>   (int *) l_125 = 
>
> and the expression parser returns something not particularly useful:
>
>   (lldb) p l_125 
>   error: :43:31: no member named 'l_125' in namespace 
> '$__lldb_local_vars'
>   using $__lldb_local_vars::l_125;
> ^
>   error: :1:1: use of undeclared identifier 'l_125'
>   l_125
>
> From my testing infrastructure/fuzzing perspective the two are 
> indistinguishable, as the script I've written chokes on both, but it would be 
> better from an ergonomics point of view if `p` would return something 
> meaningful, if possible (even if there's a bug in lldb). Do you think it's 
> worth filing a PR? (also, cc: @teemperor for ideas as he spent a fair amount 
> of time working on the expression parser)

For this particular case, I think the best diagnostic would be one of those 
module-level warnings we print to stderr, as I think that a DW_AT_const_value 
attribute with no data means the debug info is broken.

Speaking more generally, it may be interesting to try to present variables with 
no value (e.g. optimized out) to clang as undefined (`extern`) variables. That 
way we could get past the "compile" stage, and handle this stuff in the "link" 
stage. Here, I believe we have more control so it may be easier to print 
something like "Cannot evaluate expression because variable 'foo' is optimized 
out. But I am not really an expert here so I am just guessing...
I'm not really an expert here, and we are sort of limited by what error 
messages we can get clang to produce. The current error message is not 
completely unreasonable because it basically says "this variable does not exist"




Comment at: lldb/source/Symbol/Type.cpp:378
 m_byte_size_has_value = true;
+return m_byte_size;
   }

davide wrote:
> shafik wrote:
> > labath wrote:
> > > davide wrote:
> > > > shafik wrote:
> > > > > Wouldn't it be better to turn `m_byte_size` into an `Optional`? As 
> > > > > this fix shows this having this additional state we carry around is 
> > > > > error prone.
> > > > This is a much larger project, probably worth considering.
> > > > With that in mind, it wouldn't have prevented this bug from happening, 
> > > > as this falls through and returns an `Optional` anyway. 
> > > > Yay fuzz testing (that found this), I would say.
> > > IIRC, this was done this way to reduce sizeof(Type). Note that the 
> > > external interface is Optional and the only place that knows 
> > > (should know?) about this encoding is this function.
> > It would have prevented this bug as you just return `m_byte_size` no matter 
> > what.
> If you think it's an improvement, please consider submitting a review, I 
> would be eager to look at it!
FWIW, there is a way to make this more robust without the memory size increase 
due to llvm::Optional:
```
if (!m_byte_size_has_value) {
  if (Optional byte_size = ComputeByteSize()) {
m_byte_size_has_value = true;
m_byte_size = *byte_size;
  }
}
if (m_byte_size_has_value)
  return m_byte_size;
return None;
```
That way the optional-to-flat conversion is fully contained in 9 lines of code, 
which can be easily audited for correctness.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86436

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

[Lldb-commits] [PATCH] D86616: [cmake] Make gtest include directories a part of the library interface

[Pavel Labath via Phabricator via lldb-commits]

labath created this revision.
labath added reviewers: ldionne, beanz.
Herald added subscribers: libc-commits, lldb-commits, dexonsmith, hiraditya, 
mgorny.
Herald added a reviewer: bollu.
Herald added a reviewer: DavidTruby.
Herald added projects: LLDB, libc-project, LLVM.
labath requested review of this revision.
Herald added a subscriber: JDevlieghere.

This applies the same fix that D84748  did for 
macro definitions.
Appropriate include path is now automatically set for all libraries
which link against gtest targets, which avoids the need to set
include_directories in various parts of the project.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D86616

Files:
  flang/CMakeLists.txt
  libc/benchmarks/CMakeLists.txt
  lldb/unittests/TestingSupport/Symbol/CMakeLists.txt
  llvm/cmake/modules/AddLLVM.cmake
  llvm/lib/Testing/Support/CMakeLists.txt
  llvm/utils/unittest/CMakeLists.txt
  polly/CMakeLists.txt

Index: polly/CMakeLists.txt
===
--- polly/CMakeLists.txt
+++ polly/CMakeLists.txt
@@ -30,12 +30,6 @@
 if (NOT TARGET gtest)
   add_subdirectory(${UNITTEST_DIR} utils/unittest)
 endif()
-
-# LLVM Doesn't export gtest's include directorys, so do that here
-set_target_properties(gtest
-  PROPERTIES INTERFACE_INCLUDE_DIRECTORIES
-  "${UNITTEST_DIR}/googletest/include;${UNITTEST_DIR}/googlemock/include"
-  )
 set(POLLY_GTEST_AVAIL 1)
   endif()
 
Index: llvm/utils/unittest/CMakeLists.txt
===
--- llvm/utils/unittest/CMakeLists.txt
+++ llvm/utils/unittest/CMakeLists.txt
@@ -11,14 +11,6 @@
 #
 # Project-wide settings
 
-# Where gtest's .h files can be found.
-include_directories(
-  googletest/include
-  googletest
-  googlemock/include
-  googlemock
-  )
-
 if(WIN32)
   add_definitions(-DGTEST_OS_WINDOWS=1)
 endif()
@@ -76,6 +68,11 @@
   target_compile_definitions(gtest PUBLIC GTEST_HAS_PTHREAD=0)
 endif ()
 
+target_include_directories(gtest
+  PUBLIC googletest/include googlemock/include
+  PRIVATE googletest googlemock
+  )
+
 add_subdirectory(UnitTestMain)
 
 # When LLVM_LINK_LLVM_DYLIB is enabled, libLLVM.so is added to the interface
Index: llvm/lib/Testing/Support/CMakeLists.txt
===
--- llvm/lib/Testing/Support/CMakeLists.txt
+++ llvm/lib/Testing/Support/CMakeLists.txt
@@ -12,6 +12,4 @@
   Support
   )
 
-include_directories(${LLVM_MAIN_SRC_DIR}/utils/unittest/googletest/include)
-include_directories(${LLVM_MAIN_SRC_DIR}/utils/unittest/googlemock/include)
 target_link_libraries(LLVMTestingSupport PRIVATE gtest)
Index: llvm/cmake/modules/AddLLVM.cmake
===
--- llvm/cmake/modules/AddLLVM.cmake
+++ llvm/cmake/modules/AddLLVM.cmake
@@ -1401,9 +1401,6 @@
 set(EXCLUDE_FROM_ALL ON)
   endif()
 
-  include_directories(${LLVM_MAIN_SRC_DIR}/utils/unittest/googletest/include)
-  include_directories(${LLVM_MAIN_SRC_DIR}/utils/unittest/googlemock/include)
-
   if (SUPPORTS_VARIADIC_MACROS_FLAG)
 list(APPEND LLVM_COMPILE_FLAGS "-Wno-variadic-macros")
   endif ()
Index: lldb/unittests/TestingSupport/Symbol/CMakeLists.txt
===
--- lldb/unittests/TestingSupport/Symbol/CMakeLists.txt
+++ lldb/unittests/TestingSupport/Symbol/CMakeLists.txt
@@ -2,7 +2,3 @@
 add_lldb_library(lldbSymbolHelpers
   YAMLModuleTester.cpp
   )
-
-target_include_directories(lldbSymbolHelpers PUBLIC
-  ${LLVM_MAIN_SRC_DIR}/utils/unittest/googletest/include
-  ${LLVM_MAIN_SRC_DIR}/utils/unittest/googlemock/include)
Index: libc/benchmarks/CMakeLists.txt
===
--- libc/benchmarks/CMakeLists.txt
+++ libc/benchmarks/CMakeLists.txt
@@ -53,11 +53,6 @@
 EXCLUDE_FROM_ALL
 ${LIBC_BENCHMARKS_UNITTEST_SRCS}
   )
-  target_include_directories(${target_name}
-PRIVATE
-${LLVM_MAIN_SRC_DIR}/utils/unittest/googletest/include
-${LLVM_MAIN_SRC_DIR}/utils/unittest/googlemock/include
-  )
   target_link_libraries(${target_name}
 PRIVATE
 gtest_main
Index: flang/CMakeLists.txt
===
--- flang/CMakeLists.txt
+++ flang/CMakeLists.txt
@@ -135,13 +135,7 @@
   if (FLANG_INCLUDE_TESTS)
 set(UNITTEST_DIR ${LLVM_BUILD_MAIN_SRC_DIR}/utils/unittest)
 if(EXISTS ${UNITTEST_DIR}/googletest/include/gtest/gtest.h)
-  if (TARGET gtest)
-# LLVM Doesn't export gtest's include directorys, so do that here
-set_target_properties(gtest
-  PROPERTIES INTERFACE_INCLUDE_DIRECTORIES
-  "${UNITTEST_DIR}/googletest/include;${UNITTEST_DIR}/googlemock/include"
-  )
-  else()
+  if (NOT TARGET gtest)
 add_library(gtest
   ${UNITTEST_DIR}/googletest/src/gtest-all.cc

[Lldb-commits] [PATCH] D86615: [lldb/DWARF] Fix handling of variables with both location and const_value attributes

[Pavel Labath via Phabricator via lldb-commits]

labath created this revision.
labath added reviewers: aprantl, shafik.
Herald added a project: LLDB.
labath requested review of this revision.
Herald added a subscriber: JDevlieghere.

Class-level static constexpr variables can have both DW_AT_const_value
(in the "declaration") and a DW_AT_location (in the "definition")
attributes. Our code was trying to handle this, but it was brittle and
hard to follow (and broken) because it was processing the attributes in
the order in which they were found.

Refactor the code to make the intent clearer -- DW_AT_location trumps
DW_AT_const_value, and fix the bug which meant that we were not
displaying these variables properly (the culprit was the delayed parsing
of the const_value attribute due to a need to fetch the variable type.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D86615

Files:
  lldb/source/Plugins/SymbolFile/DWARF/DWARFFormValue.h
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
  lldb/test/Shell/SymbolFile/DWARF/DW_AT_location-DW_AT_const_value.s

Index: lldb/test/Shell/SymbolFile/DWARF/DW_AT_location-DW_AT_const_value.s
===
--- /dev/null
+++ lldb/test/Shell/SymbolFile/DWARF/DW_AT_location-DW_AT_const_value.s
@@ -0,0 +1,144 @@
+## Test that we don't get confused by variables with both location and
+## const_value attributes. Such values are produced in C++ for class-level
+## static constexpr variables.
+
+# REQUIRES: x86
+
+# RUN: llvm-mc -filetype=obj -triple x86_64-pc-linux %s -o %t
+# RUN: %lldb %t -o "target variable A::x A::y" -o exit | FileCheck %s
+
+# CHECK-LABEL: target variable
+# CHECK: (const int) A::x = 142
+# CHECK: (const int) A::y = 242
+
+.section.rodata,"a",@progbits
+.p2align2
+_ZN1A1xE:
+.long   142
+_ZN1A1yE:
+.long   242
+
+.section.debug_abbrev,"",@progbits
+.byte   1   # Abbreviation Code
+.byte   17  # DW_TAG_compile_unit
+.byte   1   # DW_CHILDREN_yes
+.byte   37  # DW_AT_producer
+.byte   8   # DW_FORM_string
+.byte   3   # DW_AT_name
+.byte   8   # DW_FORM_string
+.byte   0   # EOM(1)
+.byte   0   # EOM(2)
+.byte   3   # Abbreviation Code
+.byte   19  # DW_TAG_structure_type
+.byte   1   # DW_CHILDREN_yes
+.byte   3   # DW_AT_name
+.byte   8   # DW_FORM_string
+.byte   11  # DW_AT_byte_size
+.byte   11  # DW_FORM_data1
+.byte   0   # EOM(1)
+.byte   0   # EOM(2)
+.byte   4   # Abbreviation Code
+.byte   13  # DW_TAG_member
+.byte   0   # DW_CHILDREN_no
+.byte   3   # DW_AT_name
+.byte   8   # DW_FORM_string
+.byte   73  # DW_AT_type
+.byte   19  # DW_FORM_ref4
+.byte   60  # DW_AT_declaration
+.byte   25  # DW_FORM_flag_present
+.byte   28  # DW_AT_const_value
+.byte   13  # DW_FORM_sdata
+.byte   0   # EOM(1)
+.byte   0   # EOM(2)
+.byte   5   # Abbreviation Code
+.byte   38  # DW_TAG_const_type
+.byte   0   # DW_CHILDREN_no
+.byte   73  # DW_AT_type
+.byte   19  # DW_FORM_ref4
+.byte   0   # EOM(1)
+.byte   0   # EOM(2)
+.byte   6   # Abbreviation Code
+.byte   36  # DW_TAG_base_type
+.byte   0   # DW_CHILDREN_no
+.byte   3   # DW_AT_name
+.byte   8   # DW_FORM_string
+.byte   62  # DW_AT_encoding
+.byte   11  # DW_FORM_data1
+.byte   11  # DW_AT_byte_size
+.byte   11  # DW_FORM_data1
+.byte   0 

[Lldb-commits] [lldb] 642cb78 - Copy m_plan_is_for_signal_trap member.

[Benjamin Kramer via lldb-commits]

Author: Benjamin Kramer
Date: 2020-08-26T13:27:01+02:00
New Revision: 642cb7865f35ad7dbac78d716fcddaff561e8639

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

LOG: Copy m_plan_is_for_signal_trap member.

Otherwise it would stay uninitialized. Found by msan.

Added: 


Modified: 
lldb/include/lldb/Symbol/UnwindPlan.h

Removed: 




diff  --git a/lldb/include/lldb/Symbol/UnwindPlan.h 
b/lldb/include/lldb/Symbol/UnwindPlan.h
index 8902b5f4eaa7..40814da3de4a 100644
--- a/lldb/include/lldb/Symbol/UnwindPlan.h
+++ b/lldb/include/lldb/Symbol/UnwindPlan.h
@@ -393,6 +393,7 @@ class UnwindPlan {
 m_plan_is_sourced_from_compiler(rhs.m_plan_is_sourced_from_compiler),
 m_plan_is_valid_at_all_instruction_locations(
 rhs.m_plan_is_valid_at_all_instruction_locations),
+m_plan_is_for_signal_trap(rhs.m_plan_is_for_signal_trap),
 m_lsda_address(rhs.m_lsda_address),
 m_personality_func_addr(rhs.m_personality_func_addr) {
 m_row_list.reserve(rhs.m_row_list.size());



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

[Lldb-commits] [PATCH] D86348: [lldb/DWARF] More DW_AT_const_value fixes

[Pavel Labath 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 rG82982304d709: [lldb/DWARF] More DW_AT_const_value fixes 
(authored by labath).

Changed prior to commit:
  https://reviews.llvm.org/D86348?vs=287023=287916#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86348

Files:
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
  lldb/test/Shell/SymbolFile/DWARF/DW_AT_const_value-bitfields.s
  lldb/test/Shell/SymbolFile/DWARF/DW_AT_const_value.s

Index: lldb/test/Shell/SymbolFile/DWARF/DW_AT_const_value.s
===
--- lldb/test/Shell/SymbolFile/DWARF/DW_AT_const_value.s
+++ lldb/test/Shell/SymbolFile/DWARF/DW_AT_const_value.s
@@ -1,409 +1,158 @@
+# Test handling of (optimized-out/location-less) variables whose value is
+# specified by DW_AT_const_value
+
 # REQUIRES: x86
 
-# RUN: llvm-mc -filetype=obj -o %t -triple x86_64-apple-macosx10.15.0 %s
-# RUN: %lldb %t -o "target variable constant" -b | FileCheck %s
+# RUN: llvm-mc -filetype=obj -triple x86_64-pc-linux %s -o %t
+# RUN: %lldb %t \
+# RUN:   -o "target variable udata data1 data2 data4 data8 string strp ref4" \
+# RUN:   -o exit | FileCheck %s
+
+# CHECK-LABEL: target variable udata data1 data2 data4 data8 string strp ref4
+## Variable specified via DW_FORM_udata. This is typical for clang (10).
+# CHECK: (unsigned long) udata = 4742474247424742
+## Variables specified via fixed-size forms. This is typical for gcc (9).
+# CHECK: (unsigned long) data1 = 47
+# CHECK: (unsigned long) data2 = 4742
+# CHECK: (unsigned long) data4 = 47424742
+# CHECK: (unsigned long) data8 = 4742474247424742
+## Variables specified using string forms. This behavior purely speculative -- I
+## don't know of any compiler that would represent character strings this way.
+# CHECK: (char [7]) string = "string"
+# CHECK: (char [7]) strp = "strp"
+## Bogus attribute form. Let's make sure we don't crash at least.
+# CHECK: (char [7]) ref4 = 
+
+.section.debug_abbrev,"",@progbits
+.byte   1   # Abbreviation Code
+.byte   17  # DW_TAG_compile_unit
+.byte   1   # DW_CHILDREN_yes
+.byte   37  # DW_AT_producer
+.byte   8   # DW_FORM_string
+.byte   3   # DW_AT_name
+.byte   8   # DW_FORM_string
+.byte   0   # EOM(1)
+.byte   0   # EOM(2)
+.byte   4   # Abbreviation Code
+.byte   1   # DW_TAG_array_type
+.byte   1   # DW_CHILDREN_yes
+.byte   73  # DW_AT_type
+.byte   19  # DW_FORM_ref4
+.byte   0   # EOM(1)
+.byte   0   # EOM(2)
+.byte   5   # Abbreviation Code
+.byte   33  # DW_TAG_subrange_type
+.byte   0   # DW_CHILDREN_no
+.byte   73  # DW_AT_type
+.byte   19  # DW_FORM_ref4
+.byte   55  # DW_AT_count
+.byte   11  # DW_FORM_data1
+.byte   0   # EOM(1)
+.byte   0   # EOM(2)
+.byte   6   # Abbreviation Code
+.byte   36  # DW_TAG_base_type
+.byte   0   # DW_CHILDREN_no
+.byte   3   # DW_AT_name
+.byte   8   # DW_FORM_string
+.byte   11  # DW_AT_byte_size
+.byte   11  # DW_FORM_data1
+.byte   62  # DW_AT_encoding
+.byte   11  # DW_FORM_data1
+.byte   0   # EOM(1)
+.byte   0   # EOM(2)
+.macro var code, form
+.byte   \code   # Abbreviation Code
+.byte   52  # DW_TAG_variable
+.byte   0   # DW_CHILDREN_no
+.byte   3   # DW_AT_name
+.byte   8   # DW_FORM_string
+.byte   73  # DW_AT_type
+.byte   19  # DW_FORM_ref4
+.byte   28  # DW_AT_const_value
+.byte   \form
+.byte   0   # EOM(1)
+.byte   0   # EOM(2)
+.endm
+var 10, 0xf # DW_FORM_udata
+var 11, 0xb # DW_FORM_data1
+var 12, 0x5 # 

[Lldb-commits] [lldb] 8298230 - [lldb/DWARF] More DW_AT_const_value fixes

[Pavel Labath via lldb-commits]

Author: Pavel Labath
Date: 2020-08-26T13:17:26+02:00
New Revision: 82982304d7095891b10faacdbf9b4eb73e92a92f

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

LOG: [lldb/DWARF] More DW_AT_const_value fixes

This fixes several issues in handling of DW_AT_const_value attributes:
- the first is that the size of the data given by data forms does not
  need to match the size of the underlying variable. We already had the
  case to handle this for DW_FORM_(us)data -- this extends the handling
  to other data forms. The main reason this was not picked up is because
  clang uses leb forms in these cases while gcc prefers the fixed-size
  ones.
- The handling of DW_AT_strp form was completely broken -- we would end
  up using the pointer value as the result. I've reorganized this code
  so that it handles all string forms uniformly.
- In case of a completely bogus form we would crash due to
  strlen(nullptr).

Depends on D86311.

Differential Revision: https://reviews.llvm.org/D86348

Added: 
lldb/test/Shell/SymbolFile/DWARF/DW_AT_const_value-bitfields.s

Modified: 
lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
lldb/test/Shell/SymbolFile/DWARF/DW_AT_const_value.s

Removed: 




diff  --git a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp 
b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
index 5ce392a57e0c..500d7567536e 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
@@ -3169,44 +3169,18 @@ VariableSP SymbolFileDWARF::ParseVariableDIE(const 
SymbolContext ,
 DataExtractor(debug_info_data, block_offset, block_length),
 die.GetCU());
   } else if (DWARFFormValue::IsDataForm(form_value.Form())) {
-// Retrieve the value as a data expression.
-uint32_t data_offset = attributes.DIEOffsetAtIndex(i);
-if (auto data_length = form_value.GetFixedSize())
-  location = DWARFExpression(
-  module,
-  DataExtractor(debug_info_data, data_offset, 
*data_length),
-  die.GetCU());
-else {
-  const uint8_t *data_pointer = form_value.BlockData();
-  if (data_pointer) {
-form_value.Unsigned();
-  } else if (DWARFFormValue::IsDataForm(form_value.Form())) {
-// we need to get the byte size of the type later after we
-// create the variable
-const_value = form_value;
-  }
-}
-  } else {
-// Retrieve the value as a string expression.
-if (form_value.Form() == DW_FORM_strp) {
-  uint32_t data_offset = attributes.DIEOffsetAtIndex(i);
-  if (auto data_length = form_value.GetFixedSize())
-location = DWARFExpression(module,
-   DataExtractor(debug_info_data,
- data_offset,
- *data_length),
-   die.GetCU());
-} else {
-  const char *str = form_value.AsCString();
-  uint32_t string_offset =
-  str - (const char *)debug_info_data.GetDataStart();
-  uint32_t string_length = strlen(str) + 1;
-  location = DWARFExpression(module,
- DataExtractor(debug_info_data,
-   string_offset,
-   string_length),
- die.GetCU());
-}
+// Constant value size does not have to match the size of the
+// variable. We will fetch the size of the type after we create
+// it.
+const_value = form_value;
+  } else if (const char *str = form_value.AsCString()) {
+uint32_t string_length = strlen(str) + 1;
+location = DWARFExpression(
+module,
+DataExtractor(str, string_length,
+  die.GetCU()->GetByteOrder(),
+  die.GetCU()->GetAddressByteSize()),
+die.GetCU());
   }
 }
 break;

diff  --git a/lldb/test/Shell/SymbolFile/DWARF/DW_AT_const_value-bitfields.s 
b/lldb/test/Shell/SymbolFile/DWARF/DW_AT_const_value-bitfields.s

[Lldb-commits] [PATCH] D80659: [lldb-vscode] Redirect stderr and stdout to DAPs console message

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Sorry, I missed this patch. Modulo the inline comments and the missing windows 
support and tests, I think this patch is ok-ish. However, I am still not 
convinced of its usefulness. Why would we be doing something (particularly a 
thing which will be hard to do in a cross-platform manner, and will very likely 
border on, or downright cross into, undefined behavior territory), if we get 
that from vscode for free?




Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:519-520
+void StopRedirecting() {
+  if (!m_thread.joinable())
+return;
+  close(m_captured_fd[1]);

This is very fishy. If the thread is detached in `StartRedirecting`, `joinable` 
will never be true.



Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:521
+return;
+  close(m_captured_fd[1]);
+  m_thread.join();

Who's closing `m_captured_fd[0]` ?



Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:2919-2922
+  // Can now safely stop redirecting since lldb will no longer emit stdout
+  // or stderr messages.
+  stdout_redirector.StopRedirecting();
+  stderr_redirector.StopRedirecting();

Rely on  the desctructor calling these? Or have the  destructor assert they 
have been called?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80659

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

[Lldb-commits] [PATCH] D86603: [lldb] Correct wording of EXP_MSG

[David Spickett via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG9ad5d37fd917: [lldb] Correct wording of EXP_MSG (authored by 
DavidSpickett).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86603

Files:
  lldb/packages/Python/lldbsuite/test/lldbtest.py


Index: lldb/packages/Python/lldbsuite/test/lldbtest.py
===
--- lldb/packages/Python/lldbsuite/test/lldbtest.py
+++ lldb/packages/Python/lldbsuite/test/lldbtest.py
@@ -190,11 +190,11 @@
 
 
 def EXP_MSG(str, actual, exe):
-'''A generic "'%s' returns expected result" message generator if exe.
-Otherwise, it generates "'%s' matches expected result" message.'''
+'''A generic "'%s' returned unexpected result" message generator if exe.
+Otherwise, it generates "'%s' does not match expected result" message.'''
 
-return "'%s' %s expected result, got '%s'" % (
-str, 'returns' if exe else 'matches', actual.strip())
+return "'%s' %s result, got '%s'" % (
+str, 'returned unexpected' if exe else 'does not match expected', 
actual.strip())
 
 
 def SETTING_MSG(setting):


Index: lldb/packages/Python/lldbsuite/test/lldbtest.py
===
--- lldb/packages/Python/lldbsuite/test/lldbtest.py
+++ lldb/packages/Python/lldbsuite/test/lldbtest.py
@@ -190,11 +190,11 @@
 
 
 def EXP_MSG(str, actual, exe):
-'''A generic "'%s' returns expected result" message generator if exe.
-Otherwise, it generates "'%s' matches expected result" message.'''
+'''A generic "'%s' returned unexpected result" message generator if exe.
+Otherwise, it generates "'%s' does not match expected result" message.'''
 
-return "'%s' %s expected result, got '%s'" % (
-str, 'returns' if exe else 'matches', actual.strip())
+return "'%s' %s result, got '%s'" % (
+str, 'returned unexpected' if exe else 'does not match expected', actual.strip())
 
 
 def SETTING_MSG(setting):
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] 9ad5d37 - [lldb] Correct wording of EXP_MSG

[David Spickett via lldb-commits]

Author: David Spickett
Date: 2020-08-26T11:52:30+01:00
New Revision: 9ad5d37fd917a5b8b3ffe5c12145c85021ee2578

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

LOG: [lldb] Correct wording of EXP_MSG

EXP_MSG generates a message to show on assert
failure. Currently it looks like:
AssertionError: False is not True : ''
returns expected result, got ''

Which seems to say that the test failed but
also got the expected result.

It should say:
AssertionError: False is not True : ''
returned unexpected result, got ''

Reviewed By: teemperor, #lldb

Differential Revision: https://reviews.llvm.org/D86603

Added: 


Modified: 
lldb/packages/Python/lldbsuite/test/lldbtest.py

Removed: 




diff  --git a/lldb/packages/Python/lldbsuite/test/lldbtest.py 
b/lldb/packages/Python/lldbsuite/test/lldbtest.py
index de9a9a2c7002..4180ba271613 100644
--- a/lldb/packages/Python/lldbsuite/test/lldbtest.py
+++ b/lldb/packages/Python/lldbsuite/test/lldbtest.py
@@ -190,11 +190,11 @@ def COMPLETION_MSG(str_before, str_after, completions):
 
 
 def EXP_MSG(str, actual, exe):
-'''A generic "'%s' returns expected result" message generator if exe.
-Otherwise, it generates "'%s' matches expected result" message.'''
+'''A generic "'%s' returned unexpected result" message generator if exe.
+Otherwise, it generates "'%s' does not match expected result" message.'''
 
-return "'%s' %s expected result, got '%s'" % (
-str, 'returns' if exe else 'matches', actual.strip())
+return "'%s' %s result, got '%s'" % (
+str, 'returned unexpected' if exe else 'does not match expected', 
actual.strip())
 
 
 def SETTING_MSG(setting):



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

[Lldb-commits] [PATCH] D86603: [lldb] Correct wording of EXP_MSG

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added a comment.

That's true, `EXP_MSG` is not used like the other messages so the only problem 
with it is the confusing error message.

IMHO this is still good to land as it improves the error message. If you want 
to implement Pavel's much nicer error message in a follow-up patch, that would 
be very much appreciated!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86603

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

[Lldb-commits] [PATCH] D86603: [lldb] Correct wording of EXP_MSG

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added a comment.

Ok to land this as is just to remove some confusion?

I agree it could be much improved, I'll look into that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86603

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

[Lldb-commits] [PATCH] D86375: Load correct module for linux and android when duplicates exist in minidump.

[Pavel Labath via Phabricator via lldb-commits]

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

Thanks for the clarification. The new version looks great.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86375

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

[Lldb-commits] [PATCH] D86603: [lldb] Correct wording of EXP_MSG

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Though these extra messages are often superfluous in general, I don't think 
that's the case for this particular message. This macro is only used in the 
`expect` and `match` checks. These are the most common way of asserting 
something right now, so I think it's reasonable to spend some effort to make 
the errors it produces clear and actionable. The current state is pretty far 
from ideal.

What I would do here is:

- remove the "False is not True" part from the message. This can be done by 
using the `fail` method to well.. fail the test instead of assertTrue/False.
- provide a good error message to the fail method. It should include the 
command being run, the actual output, and also list any expectations that 
failed. Maybe something like this (slightly inspired by the gtest assertion 
format):

  Command: frame variable foo
  Actual output:
  (int) foo = 47
  Failed because:
  - output does not start with "(float)"
  - output does not contain "47.0"

The functions already track most of this output, but they only print it out in 
the "trace" mode, so this would only be a matter of reorganizing the code so 
that it makes it into the assertion message too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86603

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

[Lldb-commits] [PATCH] D86603: [lldb] Correct wording of EXP_MSG

[Raphael Isemann via Phabricator via lldb-commits]

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

FWIW, I actually think this patch itself is perfectly fine. As these messages 
are only displayed when an assert failed they should be worded with the 
assumption that the assert did indeed fail. It would be nice to reword all the 
other messages around this change to also follow this style, but I'm not a fan 
of signing up others for tedious work and this is a step in the right 
direction, so LGTM.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86603

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

[Lldb-commits] [PATCH] D86603: [lldb] Correct wording of EXP_MSG

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added a reviewer: LLDB.
teemperor added a comment.

If you look at the other "assert messages" above and below your patch you'll 
see that all (most?) of them use this very confusing "Say the opposite of what 
actually happened" style. Weirdly enough only around 70% of the "assertion 
messages" in the test suite are using this style, the others are using an 
"unexpected thing has happened" error message.

IMHO we should just completely remove all these assertion messages. Not once 
have they been useful to me when debugging a test. I would argue that 99% of 
them don't add any additional information that isn't obvious when looking at 
the assert itself. They also are usually just making the assertions longer and 
harder to read. Even worse, when I started hacking on LLDB they were often even 
suppressing the *actual error* which made it impossible to debug a test on a 
build bot (but that is thankfully fixed these days). And finally the fact that 
they can be worded in these two different ways just ends up causing confusion.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86603

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

[Lldb-commits] [PATCH] D86603: [lldb] Correct wording of EXP_MSG

[David Spickett via Phabricator via lldb-commits]

DavidSpickett created this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.
DavidSpickett requested review of this revision.
Herald added a subscriber: JDevlieghere.

EXP_MSG generates a message to show on assert
failure. Currently it looks like:
AssertionError: False is not True : ''
returns expected result, got ''

Which seems to say that the test failed but
also got the expected result.

It should say:
AssertionError: False is not True : ''
returned unexpected result, got ''


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D86603

Files:
  lldb/packages/Python/lldbsuite/test/lldbtest.py


Index: lldb/packages/Python/lldbsuite/test/lldbtest.py
===
--- lldb/packages/Python/lldbsuite/test/lldbtest.py
+++ lldb/packages/Python/lldbsuite/test/lldbtest.py
@@ -190,11 +190,11 @@
 
 
 def EXP_MSG(str, actual, exe):
-'''A generic "'%s' returns expected result" message generator if exe.
-Otherwise, it generates "'%s' matches expected result" message.'''
+'''A generic "'%s' returned unexpected result" message generator if exe.
+Otherwise, it generates "'%s' does not match expected result" message.'''
 
-return "'%s' %s expected result, got '%s'" % (
-str, 'returns' if exe else 'matches', actual.strip())
+return "'%s' %s result, got '%s'" % (
+str, 'returned unexpected' if exe else 'does not match expected', 
actual.strip())
 
 
 def SETTING_MSG(setting):


Index: lldb/packages/Python/lldbsuite/test/lldbtest.py
===
--- lldb/packages/Python/lldbsuite/test/lldbtest.py
+++ lldb/packages/Python/lldbsuite/test/lldbtest.py
@@ -190,11 +190,11 @@
 
 
 def EXP_MSG(str, actual, exe):
-'''A generic "'%s' returns expected result" message generator if exe.
-Otherwise, it generates "'%s' matches expected result" message.'''
+'''A generic "'%s' returned unexpected result" message generator if exe.
+Otherwise, it generates "'%s' does not match expected result" message.'''
 
-return "'%s' %s expected result, got '%s'" % (
-str, 'returns' if exe else 'matches', actual.strip())
+return "'%s' %s result, got '%s'" % (
+str, 'returned unexpected' if exe else 'does not match expected', actual.strip())
 
 
 def SETTING_MSG(setting):
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D82865: [LLDB] Add GetByteOffset to SBValue interface for reading register offset

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Sounds good. FWIW, I wouldn't be opposed to some CLI command which would dump 
some low-level details of the register context, similar to how we have the 
"target modules dump" command tree for Module objects...


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

https://reviews.llvm.org/D82865

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

[Lldb-commits] [PATCH] D82855: [LLDB] Send SVE vg register in custom expedited registerset

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

This looks good, though it would be nice to add a test for it. It looks like 
TestGdbRemoteExpeditedRegisters.py has a lot of functionality that could be 
reused for this purpose.


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

https://reviews.llvm.org/D82855

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

[Lldb-commits] [PATCH] D82853: [LLDB] Support custom expedited register set in gdb-remote

[Pavel Labath via Phabricator via lldb-commits]

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

I like this a lot. LGTM with some small fixes inline.




Comment at: lldb/source/Host/common/NativeRegisterContext.cpp:428-434
+static const uint32_t k_expedited_registers[] = {
+LLDB_REGNUM_GENERIC_PC, LLDB_REGNUM_GENERIC_SP, LLDB_REGNUM_GENERIC_FP,
+LLDB_REGNUM_GENERIC_RA, LLDB_INVALID_REGNUM};
+
+std::vector expedited_reg_nums;
+for (const uint32_t *generic_reg_p = k_expedited_registers;
+ *generic_reg_p != LLDB_INVALID_REGNUM; ++generic_reg_p) {

Remove LLDB_INVALID_REGNUM from the list. Then iterate as `for(uint32_t 
gen_reg: k_expedited_registers)`.



Comment at: 
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:513-515
+  if (expedited_regs.empty())
+return llvm::make_error("failed to get registers",
+   llvm::inconvertibleErrorCode());

Let's change the result type to `Optional` and `return None` here -- 
now that we support customizing the expedited registers, I think it's 
reasonable to allow a register context to say it does not want to expedite any 
registers. (The current for of the code already kind of supports that, but the 
use of Expected makes it sound like that is an erroneous state.)



Comment at: 
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:798
 
-for (const uint32_t *reg_num_p = reg_set_p->registers;
- *reg_num_p != LLDB_INVALID_REGNUM; ++reg_num_p) {
+  if (!expedited_regs.empty()) {
+for (auto _num : expedited_regs) {

I don't think this if is needed now -- we could remove it and save one level of 
indentation.


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

https://reviews.llvm.org/D82853

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

[Lldb-commits] [lldb] 7518006 - [lldb] XFAIL TestMemoryHistory on Linux

[Raphael Isemann via lldb-commits]

Author: Raphael Isemann
Date: 2020-08-26T10:24:13+02:00
New Revision: 7518006d75accd21325747430d6bced66b2c5ada

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

LOG: [lldb] XFAIL TestMemoryHistory on Linux

This test appears to have never worked on Linux but it seems none of the current
bots ever ran this test as it required enabling compiler-rt (otherwise it
would have just been skipped).

This just copies over the XFAIL decorator that are already on all other 
sanitizer
tests.

Added: 


Modified: 
lldb/test/API/functionalities/asan/TestMemoryHistory.py

Removed: 




diff  --git a/lldb/test/API/functionalities/asan/TestMemoryHistory.py 
b/lldb/test/API/functionalities/asan/TestMemoryHistory.py
index 37c34984f43b..0b8dc20f27c5 100644
--- a/lldb/test/API/functionalities/asan/TestMemoryHistory.py
+++ b/lldb/test/API/functionalities/asan/TestMemoryHistory.py
@@ -15,6 +15,9 @@ class AsanTestCase(TestBase):
 
 mydir = TestBase.compute_mydir(__file__)
 
+@expectedFailureAll(
+oslist=["linux"],
+bugnumber="non-core functionality, need to reenable and fix later (DES 
2014.11.07)")
 @skipIfFreeBSD  # llvm.org/pr21136 runtimes not yet available by default
 @expectedFailureNetBSD
 @skipUnlessAddressSanitizer



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

[Lldb-commits] [PATCH] D86375: Load correct module for linux and android when duplicates exist in minidump.

[Greg Clayton via Phabricator via lldb-commits]

clayborg updated this revision to Diff 287853.
clayborg added a comment.

Added a better algorithm for detecting if an address from the module list for 
linux processes with valid /proc//maps file is executable. We now search 
consective addresses that match the path to the module for any that are marked 
as executable. Added tests to test if the mmap'ed file comes first or second, 
and also a test for "-z separate-code" when the first mapping for an executable 
isn't executable, but a subsequent mapping with a matching path is.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86375

Files:
  lldb/source/Plugins/Process/minidump/MinidumpParser.cpp
  lldb/source/Plugins/Process/minidump/MinidumpParser.h
  lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp
  lldb/unittests/Process/minidump/MinidumpParserTest.cpp

Index: lldb/unittests/Process/minidump/MinidumpParserTest.cpp
===
--- lldb/unittests/Process/minidump/MinidumpParserTest.cpp
+++ lldb/unittests/Process/minidump/MinidumpParserTest.cpp
@@ -689,6 +689,130 @@
   EXPECT_EQ(0x1000u, filtered_modules[0]->BaseOfImage);
 }
 
+TEST_F(MinidumpParserTest, MinidumpDuplicateModuleMappedFirst) {
+  ASSERT_THAT_ERROR(SetUpFromYaml(R"(
+--- !minidump
+Streams:
+  - Type:ModuleList
+Modules:
+  - Base of Image:   0x400d
+Size of Image:   0x2000
+Module Name: '/usr/lib/libc.so'
+CodeView Record: ''
+  - Base of Image:   0x400d3000
+Size of Image:   0x1000
+Module Name: '/usr/lib/libc.so'
+CodeView Record: ''
+  - Type:LinuxMaps
+Text: |
+  400d-400d2000 r--p  b3:04 227/usr/lib/libc.so
+  400d2000-400d3000 rw-p  00:00 0
+  400d3000-400d4000 r-xp 0001 b3:04 227/usr/lib/libc.so
+  400d4000-400d5000 rwxp 1000 b3:04 227/usr/lib/libc.so
+...
+)"),
+llvm::Succeeded());
+  // If we have a module mentioned twice in the module list, and we have full
+  // linux maps for all of the memory regions, make sure we pick the one that
+  // has a consecutive region with a matching path that has executable
+  // permissions. If clients open an object file with mmap, breakpad can create
+  // multiple mappings for a library errnoneously and the lowest address isn't
+  // always the right address. In this case we check the consective memory
+  // regions whose path matches starting at the base of image address and make
+  // sure one of the regions is executable and prefer that one.
+  //
+  // This test will make sure that if the executable is second in the module
+  // list, that it will become the selected module in the filtered list.
+  std::vector filtered_modules =
+  parser->GetFilteredModuleList();
+  ASSERT_EQ(1u, filtered_modules.size());
+  EXPECT_EQ(0x400d3000u, filtered_modules[0]->BaseOfImage);
+}
+
+TEST_F(MinidumpParserTest, MinidumpDuplicateModuleMappedSecond) {
+  ASSERT_THAT_ERROR(SetUpFromYaml(R"(
+--- !minidump
+Streams:
+  - Type:ModuleList
+Modules:
+  - Base of Image:   0x400d
+Size of Image:   0x2000
+Module Name: '/usr/lib/libc.so'
+CodeView Record: ''
+  - Base of Image:   0x400d3000
+Size of Image:   0x1000
+Module Name: '/usr/lib/libc.so'
+CodeView Record: ''
+  - Type:LinuxMaps
+Text: |
+  400d-400d1000 r-xp 0001 b3:04 227/usr/lib/libc.so
+  400d1000-400d2000 rwxp 1000 b3:04 227/usr/lib/libc.so
+  400d2000-400d3000 rw-p  00:00 0
+  400d3000-400d5000 r--p  b3:04 227/usr/lib/libc.so
+...
+)"),
+llvm::Succeeded());
+  // If we have a module mentioned twice in the module list, and we have full
+  // linux maps for all of the memory regions, make sure we pick the one that
+  // has a consecutive region with a matching path that has executable
+  // permissions. If clients open an object file with mmap, breakpad can create
+  // multiple mappings for a library errnoneously and the lowest address isn't
+  // always the right address. In this case we check the consective memory
+  // regions whose path matches starting at the base of image address and make
+  // sure one of the regions is executable and prefer that one.
+  //
+  // This test will make sure that if the executable is first in the module
+  // list, that it will remain the correctly selected module in the filtered
+  // list.
+  std::vector filtered_modules =
+  parser->GetFilteredModuleList();
+  ASSERT_EQ(1u, filtered_modules.size());
+  EXPECT_EQ(0x400du, filtered_modules[0]->BaseOfImage);
+}
+
+TEST_F(MinidumpParserTest, MinidumpDuplicateModuleSeparateCode) {
+  ASSERT_THAT_ERROR(SetUpFromYaml(R"(
+--- !minidump

[Lldb-commits] [PATCH] D86417: [lldb] do not propagate eTrapHandlerFrame repeatedly

[Luboš Luňák via Phabricator via lldb-commits]

llunak added a comment.

In D86417#2237602 , @jasonmolenda 
wrote:

> lldb thinks that both frames 1 & 2 are trap handler frames.  They have full 
> register context available for the frame above them on the stack (that is, 
> frames 2 & 3) and frames 2 & 3 were interrupted asynchronously.  This doesn't 
> sound right?  How do we decide what is a trap handler frame?  One way is to 
> look for the 'S' augmentation in the eh_frame / dwarf debug_frame CIE/FDE for 
> the function -

...

> The other way is from the Platform `CalculateTrapHandlerSymbolNames` method.  
> PlatformLinux sets these to

...

> is one of these wrong?

I don't know. I do have some knowledge about how stack frames and traps work, 
but the reason I find it hard to explain the actual problem is because I'm not 
familiar with the LLDB unwind code and struggle to understand what and why it's 
really doing (e.g. for the "GetSymbolOrFunctionName(m_sym_ctx).AsCString("")" 
debug output I posted above, I'm still not sure if this printing "abort" means 
it's finding out information about "abort" or the next frame above it).

> Maybe start with a simpler question -- does `abort` call `raise`?  Like, 
> through a normal CALLQ?

Yes.

  libc.so.6`abort:
  ->  0x77a56afc <+332>: callq  0x77a55410; raise



> Does `raise` call `__restore_rt`?  Through a normal CALLQ?

No.

  libc.so.6`raise:
  ->  0x77a5551e <+270>: syscall 
  0x77a55520 <+272>: movq   0x108(%rsp), %rcx
  0x77a55528 <+280>: xorq   %fs:0x28, %rcx
  0x77a55531 <+289>: movl   %r8d, %eax



In D86417#2237743 , @jasonmolenda 
wrote:

> Minor followup on the 'image show-unwind' output -- I just landed a patch to 
> print when a function or unwindplan are marked as being a trap handler.



  (lldb) image show-unwind -n __restore_rt
  UNWIND PLANS for ld-2.26.so`__restore_rt (start addr 0x77df2270)
  This function's name is listed by the platform as a trap handler.
  
  Asynchronous (not restricted to call-sites) UnwindPlan is 'eh_frame CFI'
  Synchronous (restricted to call-sites) UnwindPlan is 'eh_frame CFI'
  
  Assembly language inspection UnwindPlan:
  This UnwindPlan originally sourced from assembly insn profiling
  This UnwindPlan is sourced from the compiler: no.
  This UnwindPlan is valid at all instruction locations: yes.
  This UnwindPlan is for a trap handler function: no.
  Address range of this UnwindPlan: [ld-2.26.so.PT_LOAD[0]..text + 
107856-0x0001a560)
  row[0]:0: CFA=rsp +8 => rsp=CFA+0 rip=[CFA-8] 
  
  eh_frame UnwindPlan:
  This UnwindPlan originally sourced from eh_frame CFI
  This UnwindPlan is sourced from the compiler: yes.
  This UnwindPlan is valid at all instruction locations: no.
  This UnwindPlan is for a trap handler function: yes.
  Address range of this UnwindPlan: [ld-2.26.so.PT_LOAD[0]..text + 
107855-0x0001a559)
  row[0]:0: CFA=DW_OP_breg7 +160, DW_OP_deref => rax=[DW_OP_breg7 +144] 
rdx=[DW_OP_breg7 +136] rcx=[DW_OP_breg7 +152] rbx=[DW_OP_breg7 +128] 
rsi=[DW_OP_breg7 +112] rdi=[DW_OP_breg7 +104] rbp=[DW_OP_breg7 +120] 
rsp=[DW_OP_breg7 +160] r8=[DW_OP_breg7 +40] r9=[DW_OP_breg7 +48] 
r10=[DW_OP_breg7 +56] r11=[DW_OP_breg7 +64] r12=[DW_OP_breg7 +72] 
r13=[DW_OP_breg7 +80] r14=[DW_OP_breg7 +88] r15=[DW_OP_breg7 +96] 
rip=[DW_OP_breg7 +168] 
  
  Arch default UnwindPlan:
  This UnwindPlan originally sourced from x86_64 default unwind plan
  This UnwindPlan is sourced from the compiler: no.
  This UnwindPlan is valid at all instruction locations: no.
  This UnwindPlan is for a trap handler function: no.
  row[0]:0: CFA=rbp+16 => rbp=[CFA-16] rsp=CFA+0 rip=[CFA-8] 
  
  Arch default at entry point UnwindPlan:
  This UnwindPlan originally sourced from x86_64 at-func-entry default
  This UnwindPlan is sourced from the compiler: no.
  This UnwindPlan is valid at all instruction locations: not specified.
  This UnwindPlan is for a trap handler function: not specified.
  row[0]:0: CFA=rsp +8 => rsp=CFA+0 rip=[CFA-8] 
  
  
  UNWIND PLANS for libc.so.6`__restore_rt (start addr 0x77a555a0)
  This function's name is listed by the platform as a trap handler.
  
  Asynchronous (not restricted to call-sites) UnwindPlan is 'eh_frame CFI'
  Synchronous (restricted to call-sites) UnwindPlan is 'eh_frame CFI'
  
  Assembly language inspection UnwindPlan:
  This UnwindPlan originally sourced from assembly insn profiling
  This UnwindPlan is sourced from the compiler: no.
  This UnwindPlan is valid at all instruction locations: yes.
  This UnwindPlan is for a trap handler function: no.
  Address range of this UnwindPlan: [libc.so.6.PT_LOAD[0]..text + 
88624-0x00015a40)
  row[0]:0: CFA=rsp +8 => rsp=CFA+0 rip=[CFA-8] 
  
  eh_frame UnwindPlan:
  This UnwindPlan originally sourced from eh_frame CFI
  This UnwindPlan is sourced from the compiler: yes.
  This UnwindPlan is valid at all instruction