[Lldb-commits] [PATCH] D135768: Counterexample for D134849

[Pavel Labath via Phabricator via lldb-commits]

labath created this revision.
labath added a reviewer: zequanwu.
Herald added a project: All.
labath requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D135768

Files:
  lldb/unittests/SymbolFile/NativePDB/UdtRecordCompleterTests.cpp

Index: lldb/unittests/SymbolFile/NativePDB/UdtRecordCompleterTests.cpp
===
--- lldb/unittests/SymbolFile/NativePDB/UdtRecordCompleterTests.cpp
+++ lldb/unittests/SymbolFile/NativePDB/UdtRecordCompleterTests.cpp
@@ -15,38 +15,80 @@
 
 namespace {
 using Member = UdtRecordCompleter::Member;
-using Record = UdtRecordCompleter::Record;
 using MemberUP = std::unique_ptr;
+using Record = UdtRecordCompleter::Record;
+
+class WrappedMember {
+public:
+  WrappedMember(const Member ) : m_obj(obj) {}
+
+private:
+  const Member _obj;
+
+  friend bool operator==(const WrappedMember , const WrappedMember ) {
+return lhs.m_obj.kind == rhs.m_obj.kind &&
+   lhs.m_obj.name == rhs.m_obj.name &&
+   lhs.m_obj.bit_offset == rhs.m_obj.bit_offset &&
+   lhs.m_obj.bit_size == rhs.m_obj.bit_size &&
+   lhs.m_obj.base_offset == rhs.m_obj.base_offset &&
+   std::equal(lhs.m_obj.fields.begin(), lhs.m_obj.fields.end(),
+  rhs.m_obj.fields.begin(), rhs.m_obj.fields.end(),
+  [](const MemberUP , const MemberUP ) {
+return WrappedMember(*lhs) == WrappedMember(*rhs);
+  });
+  }
+
+  friend llvm::raw_ostream <<(llvm::raw_ostream ,
+   const WrappedMember ) {
+os << llvm::formatv("Member{.kind={0}, .name=\"{1}\", .bit_offset={2}, "
+".bit_size={3}, .base_offset={4}, .fields=[",
+w.m_obj.kind, w.m_obj.name, w.m_obj.bit_offset,
+w.m_obj.bit_size, w.m_obj.base_offset);
+llvm::ListSeparator sep;
+for (auto  : w.m_obj.fields)
+  os << sep << WrappedMember(*f);
+return os << "]}";
+  }
+};
+
+class WrappedRecord {
+public:
+  WrappedRecord(const Record ) : m_obj(obj) {}
+
+private:
+  const Record _obj;
+
+  friend bool operator==(const WrappedRecord , const WrappedRecord ) {
+return lhs.m_obj.start_offset == rhs.m_obj.start_offset &&
+   std::equal(
+   lhs.m_obj.record.fields.begin(), lhs.m_obj.record.fields.end(),
+   rhs.m_obj.record.fields.begin(), rhs.m_obj.record.fields.end(),
+   [](const MemberUP , const MemberUP ) {
+ return WrappedMember(*lhs) == WrappedMember(*rhs);
+   });
+  }
+
+  friend llvm::raw_ostream <<(llvm::raw_ostream ,
+   const WrappedRecord ) {
+os << llvm::formatv("Record{.start_offset={0}, .record.fields=[",
+w.m_obj.start_offset);
+llvm::ListSeparator sep;
+for (const MemberUP  : w.m_obj.record.fields)
+  os << sep << WrappedMember(*f);
+return os << "]}";
+  }
+};
 
 class UdtRecordCompleterRecordTests : public testing::Test {
+protected:
   Record record;
 
-public:
   void SetKind(Member::Kind kind) { record.record.kind = kind; }
   void CollectMember(StringRef name, uint64_t byte_offset, uint64_t byte_size) {
 record.CollectMember(name, byte_offset * 8, byte_size * 8,
  clang::QualType(), lldb::eAccessPublic, 0);
   }
   void ConstructRecord() { record.ConstructRecord(); }
-
-  static bool VerifyMembers(const llvm::SmallVector ,
-const llvm::SmallVector ) {
-if (lhs.size() != rhs.size())
-  return false;
-for (size_t i = 0; i < lhs.size(); ++i) {
-  if (lhs[i]->kind != rhs[i]->kind || lhs[i]->name != rhs[i]->name ||
-  lhs[i]->bit_offset != rhs[i]->bit_offset ||
-  lhs[i]->bit_size != rhs[i]->bit_size ||
-  lhs[i]->base_offset != rhs[i]->base_offset ||
-  !VerifyMembers(lhs[i]->fields, rhs[i]->fields))
-return false;
-}
-return true;
-  }
-  bool VerifyRecord(Record ) {
-return record.start_offset == testRecord.start_offset &&
-   VerifyMembers(record.record.fields, testRecord.record.fields);
-  }
 };
 Member *AddField(Member *member, StringRef name, uint64_t byte_offset,
  uint64_t byte_size, Member::Kind kind,
@@ -84,7 +126,7 @@
   AddField(u, "m2", 0, 4, Member::Field);
   AddField(u, "m3", 0, 1, Member::Field);
   AddField(u, "m4", 0, 8, Member::Field);
-  EXPECT_TRUE(VerifyRecord(record));
+  EXPECT_EQ(WrappedRecord(this->record), WrappedRecord(record));
 }
 
 TEST_F(UdtRecordCompleterRecordTests, TestAnonymousUnionInUnion) {
@@ -107,7 +149,30 @@
   AddField(, "m2", 0, 4, Member::Field);
   AddField(, "m3", 0, 1, Member::Field);
   AddField(, "m4", 0, 8, Member::Field);
-  EXPECT_TRUE(VerifyRecord(record));
+  

[Lldb-commits] [PATCH] D135631: [lldb] Copy log files into diagnostic directory (RFC)

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

seems fine to me.




Comment at: lldb/test/Shell/Diagnostics/TestCopyLogs.test:3
+# RUN: mkdir -p %t
+# The "ll''db" below is not a typo but a way to prevent lit from substituting 
'lldb'.
+# RUN: %lldb -o 'log enable ll''db commands -f %t/commands.log' -o 
'diagnostics dump -d %t/diags'

That's cute, but I suspect windows will have a problem with that. `-s %s` (and 
putting the commands in this file) would be safer.


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

https://reviews.llvm.org/D135631

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

[Lldb-commits] [PATCH] D135621: [lldb] Add an always-on log channel

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I'm not sure if this will do what you wanted it to do. Despite the title, this 
patch is not adding a new log *channel* -- it is adding a new log *category* to 
the "lldb" channel. Our logging infra, perhaps somewhat misleadingly, manages 
all of the log state on a per-channel basis. This means that it is not e.g. 
possible to redirect one log category to a different sink than a different 
category in the same channel. I haven't checked this, but I suspect that for 
this reason, any explicit "log enable" commands by the user will redirect this 
special channel into the new (user-provided) destination. If this is not what 
you intended to do then, you probably ought to add a new log channel, for real.

That said, without knowing what kind of information do you want to log here, I 
am wondering if the same kind of information could be gathered by reusing some 
existing data collection mechanism, like session transcripts, or the 
yet-to-be-implemented telemetry data.


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

https://reviews.llvm.org/D135621

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

[Lldb-commits] [PATCH] D135413: [lldb][CPlusPlusLanguage] Respect the step-avoid-regex for functions with auto return types

[Pavel Labath via Phabricator via lldb-commits]

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

> There are longer-term plans of replacing the hand-rolled C++ parser with an 
> alternative that uses the mangle tree API to do the parsing for us.

You may be aware of this, but I feel I should mention that there are cases when 
a function just does not have a mangled name, either because it is in an 
`extern "C"` block, or because it was complied with 
`--dwarf-linkage-names=Abstract` (default for `-gsce`). In this case, we 
construct a fake demangled name from the DWARF debug info (the names of 
enclosing (DW_TAG_)namespaces, and the types for (DW_TAG_)formal_parameters. Of 
course, in this case, it makes even less sense to parse the resulting string, 
since we're the ones who constructed it in the first place. However, it may not 
be sufficient to assume that one can just start with a mangled name, and get 
everything out that way.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135413

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

[Lldb-commits] [PATCH] D134849: [LLDB][NativePDB] Fix struct layout when it has anonymous unions.

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

The test looks great, and the comments have helped. I still have a couple of 
questions about the algorithm though.




Comment at: lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp:431
+uint64_t offset = pair.first;
+auto  = pair.second;
+lldbassert(offset >= start_offset);

This shadowing the fields member confused me for quite some time. Could you 
choose a different name for one of them?



Comment at: lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp:459
+  uint64_t end_offset = offset + fields.back()->bit_size;
+  parent->fields.push_back(fields.back());
+  end_offset_map[end_offset].push_back(parent);

Can `parent` be a union here? Would the algorithm still be correct?



Comment at: lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp:471
+  } else {
+for (auto  : fields) {
+  parent->fields.push_back(field);

IIUC, this code is reached when the `parent` object is a union. However, the 
parent is chosen such that it ends before the start of these new fields? 
Therefore its start address will be before the start of these fields as well. 
Is it correct to add the fields to the union under these circumstances, or am I 
misunderstanding something?



Comment at: lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.h:64
+uint64_t base_offset;
+llvm::SmallVector fields;
+

zequanwu wrote:
> labath wrote:
> > Can we drop the `SP` part? I think that the owning references (I guess 
> > that's this field) could be just plain Field instances (unique_ptr 
> > at worst), and the rest could just be plain pointers and references.
> The Field object is shared between fields and m_fields. And we can't have 
> Field member instance inside Field class.
You can't have a `Field` member, but you can have a Field*, unique_ptr 
and std::vector members. SmallVector is also not possible, for 
reasons that are mostly obvious, but then again, storing a pointer inside a 
SmallVector negates most of the benefits that one could hope to gain by using 
it.

My point is that that using a shared pointer makes it harder to understand the 
relationships between the objects because it obfuscates the ownership aspect. 
Sometimes that is unavoidable, like when there just isn't a single object that 
can own some other object (although llvm mostly avoids that by putting the 
ownership inside some "context" object), but it's not clear to me why that 
would be the case here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134849

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

[Lldb-commits] [PATCH] D135516: [lldb] [MainLoopPosix] Fix crash upon adding lots of pending callbacks

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I never expected we would have so many callbacks that we'd have to worry about 
this, but yes, this is one way to fix the problem. Another (slightly simpler, 
but also less performant) would be to make the write pipe nonblocking, and do 
not treat EAGAIN as an error.




Comment at: lldb/source/Host/posix/MainLoopPosix.cpp:408
   assert(bytes_written == 1);
+  m_trigger_done = true;
 }

I /think/ this is not right. The other thread can wake up as soon as the Write 
call is done, and can proceed to clear the `done` flag before we are able to 
set it. If we set it afterwards, then we we suppress all subsequent writes, and 
the callbacks would never run. If we set the flag before we make the Write 
call, then it should be ok -- though the flag should probably have a different 
name then.


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

https://reviews.llvm.org/D135516

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

[Lldb-commits] [PATCH] D134882: [lldb] Move breakpoint hit reset code to Target::CleanupProcess

[Pavel Labath via Phabricator via lldb-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rG08c4a6795ac4: [lldb] Move breakpoint hit reset code to 
Target::CleanupProcess (authored by labath).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134882

Files:
  lldb/source/Target/Process.cpp
  lldb/source/Target/Target.cpp
  lldb/test/API/functionalities/gdb_remote_client/TestProcessConnect.py


Index: lldb/test/API/functionalities/gdb_remote_client/TestProcessConnect.py
===
--- lldb/test/API/functionalities/gdb_remote_client/TestProcessConnect.py
+++ lldb/test/API/functionalities/gdb_remote_client/TestProcessConnect.py
@@ -63,3 +63,57 @@
 self.dbg.GetSelectedTarget().GetProcess().Kill()
 lldbutil.expect_state_changes(self, self.dbg.GetListener(),
   self.process(), [lldb.eStateExited])
+def test_breakpoint_count(self):
+"""
+Test that breakpoint count gets reset for each new connection.
+"""
+class MyResponder(MockGDBServerResponder):
+
+def __init__(self):
+super().__init__()
+self.continued = False
+
+def qfThreadInfo(self):
+return "m47"
+
+def qsThreadInfo(self):
+return "l"
+
+def setBreakpoint(self, packet):
+return "OK"
+
+def readRegister(self, reg):
+# Pretend we're at the breakpoint after we've been resumed.
+return "3412" if self.continued else 
"4747"
+
+def cont(self):
+self.continued = True
+return "T05thread=47;reason:breakpoint"
+
+# Connect to the first process and set our breakpoint.
+self.server.responder = MyResponder()
+target = self.createTarget("a.yaml")
+process = self.connect(target)
+
+bkpt = target.BreakpointCreateByAddress(0x1234)
+self.assertTrue(bkpt.IsValid())
+self.assertEqual(bkpt.GetNumLocations(), 1)
+
+# "continue" the process. It should hit our breakpoint.
+process.Continue()
+self.assertState(process.GetState(), lldb.eStateStopped)
+self.assertEqual(bkpt.GetHitCount(), 1)
+
+# Now kill it. The breakpoint should still show a hit count of one.
+process.Kill()
+self.server.stop()
+self.assertEqual(bkpt.GetHitCount(), 1)
+
+# Start over, and reconnect.
+self.server = MockGDBServer(self.server_socket_class())
+self.server.start()
+
+process = self.connect(target)
+
+# The hit count should be reset.
+self.assertEqual(bkpt.GetHitCount(), 0)
Index: lldb/source/Target/Target.cpp
===
--- lldb/source/Target/Target.cpp
+++ lldb/source/Target/Target.cpp
@@ -177,6 +177,7 @@
   // clean up needs some help from the process.
   m_breakpoint_list.ClearAllBreakpointSites();
   m_internal_breakpoint_list.ClearAllBreakpointSites();
+  ResetBreakpointHitCounts();
   // Disable watchpoints just on the debugger side.
   std::unique_lock lock;
   this->GetWatchpointList().GetListMutex(lock);
Index: lldb/source/Target/Process.cpp
===
--- lldb/source/Target/Process.cpp
+++ lldb/source/Target/Process.cpp
@@ -2761,18 +2761,15 @@
 }
 
 Status Process::WillLaunch(Module *module) {
-  GetTarget().ResetBreakpointHitCounts();
   return DoWillLaunch(module);
 }
 
 Status Process::WillAttachToProcessWithID(lldb::pid_t pid) {
-  GetTarget().ResetBreakpointHitCounts();
   return DoWillAttachToProcessWithID(pid);
 }
 
 Status Process::WillAttachToProcessWithName(const char *process_name,
 bool wait_for_launch) {
-  GetTarget().ResetBreakpointHitCounts();
   return DoWillAttachToProcessWithName(process_name, wait_for_launch);
 }
 


Index: lldb/test/API/functionalities/gdb_remote_client/TestProcessConnect.py
===
--- lldb/test/API/functionalities/gdb_remote_client/TestProcessConnect.py
+++ lldb/test/API/functionalities/gdb_remote_client/TestProcessConnect.py
@@ -63,3 +63,57 @@
 self.dbg.GetSelectedTarget().GetProcess().Kill()
 lldbutil.expect_state_changes(self, self.dbg.GetListener(),
   self.process(), [lldb.eStateExited])
+def test_breakpoint_count(self):
+"""
+Test that breakpoint count gets reset for each new connection.
+"""
+class MyResponder(MockGDBServerResponder):
+
+def __init__(self):
+super().__init__()
+

[Lldb-commits] [PATCH] D134754: [lldb/gdb-server] Better reporting of launch errors

[Pavel Labath via Phabricator via lldb-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rG8d1de7b34af4: [lldb/gdb-server] Better reporting of launch 
errors (authored by labath).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134754

Files:
  lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
  lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
  lldb/test/API/functionalities/gdb_remote_client/TestGDBRemoteClient.py

Index: lldb/test/API/functionalities/gdb_remote_client/TestGDBRemoteClient.py
===
--- lldb/test/API/functionalities/gdb_remote_client/TestGDBRemoteClient.py
+++ lldb/test/API/functionalities/gdb_remote_client/TestGDBRemoteClient.py
@@ -86,7 +86,30 @@
 error = lldb.SBError()
 target.Launch(lldb.SBListener(), None, None, None, None, None,
 None, 0, True, error)
-self.assertEquals("'A' packet returned an error: 71", error.GetCString())
+self.assertRegex(error.GetCString(), "Cannot launch '.*a': Error 71")
+
+def test_launch_rich_error(self):
+class MyResponder(MockGDBServerResponder):
+def qC(self):
+return "E42"
+
+def qfThreadInfo(self):
+return "OK" # No threads.
+
+# Then, when we are asked to attach, error out.
+def vRun(self, packet):
+return "Eff;" + seven.hexlify("I'm a teapot")
+
+self.server.responder = MyResponder()
+
+target = self.createTarget("a.yaml")
+process = self.connect(target)
+lldbutil.expect_state_changes(self, self.dbg.GetListener(), process, [lldb.eStateConnected])
+
+error = lldb.SBError()
+target.Launch(lldb.SBListener(), None, None, None, None, None,
+None, 0, True, error)
+self.assertRegex(error.GetCString(), "Cannot launch '.*a': I'm a teapot")
 
 def test_read_registers_using_g_packets(self):
 """Test reading registers using 'g' packets (default behavior)"""
Index: lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
===
--- lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
+++ lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
@@ -84,6 +84,7 @@
 
 #include "llvm/ADT/ScopeExit.h"
 #include "llvm/ADT/StringSwitch.h"
+#include "llvm/Support/FormatAdapters.h"
 #include "llvm/Support/Threading.h"
 #include "llvm/Support/raw_ostream.h"
 
@@ -799,17 +800,17 @@
   GDBRemoteCommunication::ScopedTimeout timeout(m_gdb_comm,
 std::chrono::seconds(10));
 
-  int arg_packet_err = m_gdb_comm.SendArgumentsPacket(launch_info);
-  if (arg_packet_err == 0) {
-std::string error_str;
-if (m_gdb_comm.GetLaunchSuccess(error_str)) {
-  SetID(m_gdb_comm.GetCurrentProcessID());
-} else {
-  error.SetErrorString(error_str.c_str());
-}
+  // Since we can't send argv0 separate from the executable path, we need to
+  // make sure to use the actual executable path found in the launch_info...
+  Args args = launch_info.GetArguments();
+  if (FileSpec exe_file = launch_info.GetExecutableFile())
+args.ReplaceArgumentAtIndex(0, exe_file.GetPath(false));
+  if (llvm::Error err = m_gdb_comm.LaunchProcess(args)) {
+error.SetErrorStringWithFormatv("Cannot launch '{0}': {1}",
+args.GetArgumentAtIndex(0),
+llvm::fmt_consume(std::move(err)));
   } else {
-error.SetErrorStringWithFormat("'A' packet returned an error: %i",
-   arg_packet_err);
+SetID(m_gdb_comm.GetCurrentProcessID());
   }
 }
 
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
@@ -80,8 +80,6 @@
 
   lldb::pid_t GetCurrentProcessID(bool allow_lazy = true);
 
-  bool GetLaunchSuccess(std::string _str);
-
   bool LaunchGDBServer(const char *remote_accept_hostname, lldb::pid_t ,
uint16_t , std::string _name);
 
@@ -90,19 +88,11 @@
 
   bool KillSpawnedProcess(lldb::pid_t pid);
 
-  /// Sends a GDB remote protocol 'A' packet that delivers program
-  /// arguments to the remote server.
-  ///
-  /// \param[in] launch_info
-  /// A NULL terminated array of const C strings 

[Lldb-commits] [PATCH] D134570: [lldb] Skip check for conflicting filter/synth when adding a new regex.

[Pavel Labath via Phabricator via lldb-commits]

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

Let's just skip this check. Though it might be fun to implement an 
emptyness-of-intersection check for two regular expressions, that's: a) 
overkill; b) impossible if you include backreferences.


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

https://reviews.llvm.org/D134570

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

[Lldb-commits] [PATCH] D134771: [NFCI] Simplify TypeCategoryImpl for-each callbacks.

[Pavel Labath via Phabricator via lldb-commits]

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

(it looks like I did not click "submit" for some reason)




Comment at: lldb/include/lldb/DataFormatters/TypeCategory.h:169
+template  ForEachCallback(Callable c) : callback(c) {}
+std::function)> 
callback;
   };

And one reference here as well.



Comment at: lldb/include/lldb/DataFormatters/TypeCategory.h:159-162
+  // TypeFilterImpl inherits from SyntheticChildren, so we can't simply 
overload
+  // ForEach on the type of the callback because it would result in "call to
+  // member function 'ForEach' is ambiguous" errors. Instead we use this
+  // templated struct to hold the formatter type and the callback.

jgorbe wrote:
> labath wrote:
> > What if we just embed the type information into the method name? (So we 
> > could have a set of `ForEachFormat`,`ForEachSummary`, ... methods instead 
> > of a single overloaded ForEach)
> The problem with that is that the call site is
> 
> ```
> template 
> class CommandObjectTypeFormatterList {
>   [...]
>   bool DoExecute(...) {
>   TypeCategoryImpl::ForEachCallbacks foreach;
>   category->ForEach(foreach);
> ```
> 
> So if we want to keep that template for `CommandObjectTypeFormatterList` to 
> avoid repeating the implementation of `type {format, summary, filter, 
> synthetic} list`, we still need to switch based on type //somewhere//. So it 
> might as well be here. Or did you have anything else in mind?
No, this is what I had in mind, but I somehow missed the fact that the call 
site is templated. Ok, let's stick to this then.


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

https://reviews.llvm.org/D134771

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

[Lldb-commits] [PATCH] D134849: [LLDB][NativePDB] Fix struct layout when it has anonymous unions.

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.



Comment at: lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp:331
+ clang::DeclContext *parent_decl_ctx) {
+  static lldb::user_id_t anonymous_id = LLDB_INVALID_UID - 1;
+  clang::FieldDecl *field_decl = nullptr;

Now multiple symbol files can race when accessing this variable (and this also 
introduces a strange interaction between two supposedly-independent symbol 
files). Better make this member variable of the parent symbol file, or 
something like that.



Comment at: 
lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp:392-443
+  // For anonymous unions in a struct, msvc generated pdb doesn't have the
+  // entity for that union. So, we need to construct anonymous union and struct
+  // based on field offsets. The final AST is likely not matching the exact
+  // original AST, but the memory layout is preseved.
+
+  // The end offset to a field/struct/union that ends at the offset.
+  std::map end_offset_map;

This could use a high-level explanation of the algorithm. Like, I know it tries 
to stuff the fields into structs and unions, but I wasn't able to figure out 
how it does that, and what are the operating invariants.

The thing I like about this algorithm, is that the most complicated part (the 
thing I highlighted) is basically just playing with numbers and it is 
independent of any complex objects and state. If this part is separated out 
into a separate function, then it would be perfectly suitable for unit testing, 
and the unit tests could also serve as documentation/examples of the kinds of 
transformations that the algorithm does.



Comment at: lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.h:54
+  struct Field {
+enum Kind { FIELD, STRUCT, UNION } kind;
+// Following are only used for field.

according to 
,
 these should be called `Field, Struct, Union`



Comment at: lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.h:64
+uint64_t base_offset;
+llvm::SmallVector fields;
+

Can we drop the `SP` part? I think that the owning references (I guess that's 
this field) could be just plain Field instances (unique_ptr at worst), 
and the rest could just be plain pointers and references.



Comment at: lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.h:92
+  // llvm::SmallVector m_fields;
+  uint64_t start_offset = UINT64_MAX;
+  bool m_is_struct;

m_start_offset ?



Comment at: lldb/test/Shell/SymbolFile/NativePDB/class_layout.cpp:53-59
+// CHECK-NEXT: | |-DefinitionData pass_in_registers aggregate standard_layout 
trivially_copyable pod trivial literal
+// CHECK-NEXT: | | |-DefaultConstructor exists trivial needs_implicit
+// CHECK-NEXT: | | |-CopyConstructor simple trivial has_const_param 
needs_implicit implicit_has_const_param
+// CHECK-NEXT: | | |-MoveConstructor exists simple trivial needs_implicit
+// CHECK-NEXT: | | |-CopyAssignment simple trivial has_const_param 
needs_implicit implicit_has_const_param
+// CHECK-NEXT: | | |-MoveAssignment exists simple trivial needs_implicit
+// CHECK-NEXT: | | `-Destructor simple irrelevant trivial needs_implicit

I think we should exclude all of these DefinitionData fields from the test 
expectation, as they are largely irrelevant to the test (and they also 
obfuscate it very well). Otherwise, people will have to update these whenever a 
new field gets added.

I don't know whether it contains the thing you wanted to test (as I don't know 
what that is), but the `type lookup C` output will contain the general 
structure of the type, and it will be a lot more readable.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134849

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

[Lldb-commits] [PATCH] D134906: Have MemoryCache cache addresses that were unreadable, so duplicate read attempts can be suppressed

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D134906#3835260 , @jasonmolenda 
wrote:

> In D134906#3832642 , @labath wrote:
>
>> I don't know about debugserver, but both lldb-server and gdbserver currently 
>> return an error when the memory is partially accessible, even though the 
>> protocol explicitly allows the possibility of truncated reads. It is 
>> somewhat hard to reproduce, because the caching mechanism in lldb aligns 
>> memory reads, and the cache "line" size is usually smaller than the page 
>> size -- which is probably why this behavior hasn't bothered anyone so far. 
>> Nonetheless, I would say that this behavior (not returning partial contents) 
>> is a (QoI) bug, but the fact that two stubs have it makes me wonder how many 
>> other stubs do the same as well..
>
> Hi Pavel, thanks for pointing this out.  I did a quick check with debugserver 
> on darwin, using `memory region` to find the end of an accessible region in 
> memory, and starting a read request a little earlier, in readable memory:
>
>   (lldb) sett set target.process.disable-memory-cache true
>   
>   (lldb) mem region 0x00010180
><  31> send packet: $qMemoryRegionInfo:10180#ce
><  34> read packet: $start:10180;size:6a60;#00
>   [0x00010180-0x00016be0) ---
>   
>   (lldb) mem region 0x00010180-4
><  31> send packet: $qMemoryRegionInfo:1017c#d8
>< 122> read packet: 
> $start:10100;size:80;permissions:rw;dirty-pages:10100,101008000,10100c000,1017fc000;type:heap,malloc-small;#00
>   [0x00010100-0x00010180) rw-
>   
>   (lldb) x/8wx 0x00010180-4
>   
><  17> send packet: $x1017c,20#ca
><   8> read packet: $#00
>   
><  17> send packet: $x10180,1c#f2
><   7> read packet: $E80#00
>   
>   0x1017c: 0x 0x 0x 0x
>   0x1018c: 0x 0x 0x 0x
>   warning: Not all bytes (4/32) were able to be read from 0x1017c.
>   (lldb) 
>
> We ask for 32 bytes starting at 0x1017c, get back 4 bytes.  Then we try 
> to read the remaining 28 bytes starting at 0x10180, and get an error. So 
> this is different behavior from other stubs where you might simply get an 
> error for the request to read more bytes than are readable.

Yes, that's pretty much what I did, except that I was not able to read any data 
with the caching turned off.

In D134906#3835291 , @jasonmolenda 
wrote:

> to be clear, I think I'll need to abandon this.

I don't think this is necessarily a lost cause. I mean, the debugserver 
behavior (truncated reads) is definitely better here, and the caching of 
negative acesses makes sense. And, as the example above shows, the current 
behavior with the non-truncating stubs is already kind of broken, because you 
cannot read the areas near the end of mapped space without doing some kind of a 
bisection on the size of the read (we could implement the bisection in lldb, 
but... ewww).

The safest way to pursue this would be to have the stub indicate (maybe via 
qSupported) its intention to return truncated reads, and then key the behavior 
off of that. However, if that's not possible (it seems you have some hardware 
stub here), I could imagine just enabling this behavior by default. We can 
definitely change the lldb-server behavior, and for the rest, we can tell them 
to fix their stubs.

That is, if they even notice this. The memory read alignment hides this problem 
fairly well. To demonstrate this, we've had to turn the caching off -- but that 
would also turn off the negative cache, and avoid this problem. So, if someone 
can't fix their stub, we can always tell them to turn the cache off as a 
workaround.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134906

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

[Lldb-commits] [PATCH] D135031: [lldb] [llgs] Move client-server communication into a separate thread (WIP)

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I think we should get some measurements (e.g. from the `process plugin packet 
speed-test` cmd) of the overhead of this approach. The latency/RTT of the 
connection is very important for some users, and I fear that all of this ping 
pong could significantly regress that.


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

https://reviews.llvm.org/D135031

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

[Lldb-commits] [PATCH] D135029: [lldb] [gdb-remote] Isolate ClientBase from Communication

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.



Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteClientBase.h:116-118
+  GDBRemoteCommunication () {
+return m_comm;
+  }

mgorny wrote:
> labath wrote:
> > Is the plan to make this private/protected at some point, or something like 
> > that? Otherwise, I'm not really sure what's the benefit of this over the 
> > regular inheritance.
> > 
> > I like the idea of using composition instead of inheritance (I think we 
> > could do something similar with GDBRemoteCommunication and Communication), 
> > but right now this seems fairly messy, and the benefit is unclear.
> Ideally, yes. However, I don't think I'm going to pursue it that far, i.e. 
> someone else will have to take up the effort. And yes, I honestly doubt 
> anybody will.
> 
> The main goal is make ground for D135031, i.e. communication via separate 
> thread. What I've been aiming at is leaving `GetCommunication()` for stuff 
> that's unlikely to break when invoked cross-thread (or unlikely to be invoked 
> cross-thread), while abstracting away the part of the API that needs to be 
> reimplemented to communicate via the comm thread.
Ok, maybe that's fine. Let's figure out what to do with the other patch first.


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

https://reviews.llvm.org/D135029

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

[Lldb-commits] [PATCH] D134882: [lldb] Move breakpoint hit reset code to Target::CleanupProcess

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D134882#3831137 , @jingham wrote:

> That seems fine.  I think it's useful to be able to see breakpoint hit counts 
> up to the point where you start a new process.  From looking at the code, it 
> looks like putting the clear in CleanupProcess will do that.  If you agree 
> this is useful, can you add to the test that the hit count is still 1 between 
> process.Kill and connecting to the new process?

That makes sense to me. I've added the extra check.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134882

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

[Lldb-commits] [PATCH] D134882: [lldb] Move breakpoint hit reset code to Target::CleanupProcess

[Pavel Labath via Phabricator via lldb-commits]

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

Check breakpoint hit counts after termination


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134882

Files:
  lldb/source/Target/Process.cpp
  lldb/source/Target/Target.cpp
  lldb/test/API/functionalities/gdb_remote_client/TestProcessConnect.py


Index: lldb/test/API/functionalities/gdb_remote_client/TestProcessConnect.py
===
--- lldb/test/API/functionalities/gdb_remote_client/TestProcessConnect.py
+++ lldb/test/API/functionalities/gdb_remote_client/TestProcessConnect.py
@@ -63,3 +63,57 @@
 self.dbg.GetSelectedTarget().GetProcess().Kill()
 lldbutil.expect_state_changes(self, self.dbg.GetListener(),
   self.process(), [lldb.eStateExited])
+def test_breakpoint_count(self):
+"""
+Test that breakpoint count gets reset for each new connection.
+"""
+class MyResponder(MockGDBServerResponder):
+
+def __init__(self):
+super().__init__()
+self.continued = False
+
+def qfThreadInfo(self):
+return "m47"
+
+def qsThreadInfo(self):
+return "l"
+
+def setBreakpoint(self, packet):
+return "OK"
+
+def readRegister(self, reg):
+# Pretend we're at the breakpoint after we've been resumed.
+return "3412" if self.continued else 
"4747"
+
+def cont(self):
+self.continued = True
+return "T05thread=47;reason:breakpoint"
+
+# Connect to the first process and set our breakpoint.
+self.server.responder = MyResponder()
+target = self.createTarget("a.yaml")
+process = self.connect(target)
+
+bkpt = target.BreakpointCreateByAddress(0x1234)
+self.assertTrue(bkpt.IsValid())
+self.assertEqual(bkpt.GetNumLocations(), 1)
+
+# "continue" the process. It should hit our breakpoint.
+process.Continue()
+self.assertState(process.GetState(), lldb.eStateStopped)
+self.assertEqual(bkpt.GetHitCount(), 1)
+
+# Now kill it. The breakpoint should still show a hit count of one.
+process.Kill()
+self.server.stop()
+self.assertEqual(bkpt.GetHitCount(), 1)
+
+# Start over, and reconnect.
+self.server = MockGDBServer(self.server_socket_class())
+self.server.start()
+
+process = self.connect(target)
+
+# The hit count should be reset.
+self.assertEqual(bkpt.GetHitCount(), 0)
Index: lldb/source/Target/Target.cpp
===
--- lldb/source/Target/Target.cpp
+++ lldb/source/Target/Target.cpp
@@ -177,6 +177,7 @@
   // clean up needs some help from the process.
   m_breakpoint_list.ClearAllBreakpointSites();
   m_internal_breakpoint_list.ClearAllBreakpointSites();
+  ResetBreakpointHitCounts();
   // Disable watchpoints just on the debugger side.
   std::unique_lock lock;
   this->GetWatchpointList().GetListMutex(lock);
Index: lldb/source/Target/Process.cpp
===
--- lldb/source/Target/Process.cpp
+++ lldb/source/Target/Process.cpp
@@ -2761,18 +2761,15 @@
 }
 
 Status Process::WillLaunch(Module *module) {
-  GetTarget().ResetBreakpointHitCounts();
   return DoWillLaunch(module);
 }
 
 Status Process::WillAttachToProcessWithID(lldb::pid_t pid) {
-  GetTarget().ResetBreakpointHitCounts();
   return DoWillAttachToProcessWithID(pid);
 }
 
 Status Process::WillAttachToProcessWithName(const char *process_name,
 bool wait_for_launch) {
-  GetTarget().ResetBreakpointHitCounts();
   return DoWillAttachToProcessWithName(process_name, wait_for_launch);
 }
 


Index: lldb/test/API/functionalities/gdb_remote_client/TestProcessConnect.py
===
--- lldb/test/API/functionalities/gdb_remote_client/TestProcessConnect.py
+++ lldb/test/API/functionalities/gdb_remote_client/TestProcessConnect.py
@@ -63,3 +63,57 @@
 self.dbg.GetSelectedTarget().GetProcess().Kill()
 lldbutil.expect_state_changes(self, self.dbg.GetListener(),
   self.process(), [lldb.eStateExited])
+def test_breakpoint_count(self):
+"""
+Test that breakpoint count gets reset for each new connection.
+"""
+class MyResponder(MockGDBServerResponder):
+
+def __init__(self):
+super().__init__()
+self.continued = False
+
+def qfThreadInfo(self):
+return "m47"
+
+def qsThreadInfo(self):
+return "l"
+
+ 

[Lldb-commits] [PATCH] D134906: Have MemoryCache cache addresses that were unreadable, so duplicate read attempts can be suppressed

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I don't know about debugserver, but both lldb-server and gdbserver currently 
return an error when the memory is partially accessible, even though the 
protocol explicitly allows the possibility of truncated reads. It is somewhat 
hard to reproduce, because the caching mechanism in lldb aligns memory reads, 
and the cache "line" size is usually smaller than the page size -- which is 
probably why this behavior hasn't bothered anyone so far. Nonetheless, I would 
say that this behavior (not returning partial contents) is a (QoI) bug, but the 
fact that two stubs have it makes me wonder how many other stubs do the same as 
well..


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134906

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

[Lldb-commits] [PATCH] D134882: [lldb] Move breakpoint hit reset code to Target::CleanupProcess

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Here's an even better idea. I've now doing it in Target::CleanupProcess, right 
next to the code for resetting **watch**point hit counts.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134882

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

[Lldb-commits] [PATCH] D134882: [lldb] Move breakpoint hit reset code to Target::CreateProcess

[Pavel Labath via Phabricator via lldb-commits]

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

use CleanupProcess instead


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134882

Files:
  lldb/source/Target/Process.cpp
  lldb/source/Target/Target.cpp
  lldb/test/API/functionalities/gdb_remote_client/TestProcessConnect.py


Index: lldb/test/API/functionalities/gdb_remote_client/TestProcessConnect.py
===
--- lldb/test/API/functionalities/gdb_remote_client/TestProcessConnect.py
+++ lldb/test/API/functionalities/gdb_remote_client/TestProcessConnect.py
@@ -63,3 +63,54 @@
 self.dbg.GetSelectedTarget().GetProcess().Kill()
 lldbutil.expect_state_changes(self, self.dbg.GetListener(),
   self.process(), [lldb.eStateExited])
+def test_breakpoint_count(self):
+"""
+Test that breakpoint count gets reset for each new connection.
+"""
+class MyResponder(MockGDBServerResponder):
+
+def __init__(self):
+super().__init__()
+self.continued = False
+
+def qfThreadInfo(self):
+return "m47"
+
+def qsThreadInfo(self):
+return "l"
+
+def setBreakpoint(self, packet):
+return "OK"
+
+def readRegister(self, reg):
+# Pretend we're at the breakpoint after we've been resumed.
+return "3412" if self.continued else 
"4747"
+
+def cont(self):
+self.continued = True
+return "T05thread=47;reason:breakpoint"
+
+# Connect to the first process and set our breakpoint.
+self.server.responder = MyResponder()
+target = self.createTarget("a.yaml")
+process = self.connect(target)
+
+bkpt = target.BreakpointCreateByAddress(0x1234)
+self.assertTrue(bkpt.IsValid())
+self.assertEqual(bkpt.GetNumLocations(), 1)
+
+# "continue" the process. It should hit our breakpoint.
+process.Continue()
+self.assertState(process.GetState(), lldb.eStateStopped)
+self.assertEqual(bkpt.GetHitCount(), 1)
+
+# Now kill it, and connect again.
+process.Kill()
+self.server.stop()
+self.server = MockGDBServer(self.server_socket_class())
+self.server.start()
+
+process = self.connect(target)
+
+# The hit count should be reset.
+self.assertEqual(bkpt.GetHitCount(), 0)
Index: lldb/source/Target/Target.cpp
===
--- lldb/source/Target/Target.cpp
+++ lldb/source/Target/Target.cpp
@@ -177,6 +177,7 @@
   // clean up needs some help from the process.
   m_breakpoint_list.ClearAllBreakpointSites();
   m_internal_breakpoint_list.ClearAllBreakpointSites();
+  ResetBreakpointHitCounts();
   // Disable watchpoints just on the debugger side.
   std::unique_lock lock;
   this->GetWatchpointList().GetListMutex(lock);
Index: lldb/source/Target/Process.cpp
===
--- lldb/source/Target/Process.cpp
+++ lldb/source/Target/Process.cpp
@@ -2761,18 +2761,15 @@
 }
 
 Status Process::WillLaunch(Module *module) {
-  GetTarget().ResetBreakpointHitCounts();
   return DoWillLaunch(module);
 }
 
 Status Process::WillAttachToProcessWithID(lldb::pid_t pid) {
-  GetTarget().ResetBreakpointHitCounts();
   return DoWillAttachToProcessWithID(pid);
 }
 
 Status Process::WillAttachToProcessWithName(const char *process_name,
 bool wait_for_launch) {
-  GetTarget().ResetBreakpointHitCounts();
   return DoWillAttachToProcessWithName(process_name, wait_for_launch);
 }
 


Index: lldb/test/API/functionalities/gdb_remote_client/TestProcessConnect.py
===
--- lldb/test/API/functionalities/gdb_remote_client/TestProcessConnect.py
+++ lldb/test/API/functionalities/gdb_remote_client/TestProcessConnect.py
@@ -63,3 +63,54 @@
 self.dbg.GetSelectedTarget().GetProcess().Kill()
 lldbutil.expect_state_changes(self, self.dbg.GetListener(),
   self.process(), [lldb.eStateExited])
+def test_breakpoint_count(self):
+"""
+Test that breakpoint count gets reset for each new connection.
+"""
+class MyResponder(MockGDBServerResponder):
+
+def __init__(self):
+super().__init__()
+self.continued = False
+
+def qfThreadInfo(self):
+return "m47"
+
+def qsThreadInfo(self):
+return "l"
+
+def setBreakpoint(self, packet):
+return "OK"
+
+def readRegister(self, reg):
+# Pretend we're 

[Lldb-commits] [PATCH] D134656: [LLDB][NativePDB] Add class/union layout bit size.

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D134656#3819025 , @zequanwu wrote:

>>> For members from parent classes, pdb only has information saying that its 
>>> direct parents class are at some offsets for this class. For class without 
>>> vtable, it's easy to calculate inherited member offsets by adding parent 
>>> class offsets with their member offsets. For class with vtable, it's more 
>>> complicated to calculate the offsets.
>>
>> Yes, so should we take the offsets from the debug info and provide them to 
>> clang (so that it does not have to recompute the offsets) ?
>
> Oh, it's already there 
> .
>  This patch just add the missing bit_size.

Ah, cool. Ok then.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134656

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

[Lldb-commits] [PATCH] D135029: [lldb] [gdb-remote] Isolate ClientBase from Communication

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.



Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteClientBase.h:116-118
+  GDBRemoteCommunication () {
+return m_comm;
+  }

Is the plan to make this private/protected at some point, or something like 
that? Otherwise, I'm not really sure what's the benefit of this over the 
regular inheritance.

I like the idea of using composition instead of inheritance (I think we could 
do something similar with GDBRemoteCommunication and Communication), but right 
now this seems fairly messy, and the benefit is unclear.


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

https://reviews.llvm.org/D135029

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

[Lldb-commits] [PATCH] D134991: [lldb] Add diagnostics

[Pavel Labath via Phabricator via lldb-commits]

labath added a subscriber: rupprecht.
labath added a comment.

Adding @rupprecht, as he might be interested in following this.

I don't want to get too involved in this, but the first thought on my mind is 
"do you really want to do all this from within a signal handler?". The fact 
that we're running this code means that we're already in a bad state, and its 
pretty hard to say how far we will manage to get without triggering another 
crash. At the very least, I think we should print the crash dump directory as 
the first order of business, before we start running random callbacks.

There are various ways to solve that, like moving the dumping code to another 
process, or being very careful about what you run inside the crash handler. The 
one thing that they all have in common is that they are harder to 
design/implement that what you have now. So, if you want try this out, and 
accept the risk that it may not be enough to capture all the crashes you're 
interested in, then I won't stand in your way...




Comment at: lldb/include/lldb/Target/Statistics.h:180
 
+class Stats {
+public:

What's the difference between this class and DebuggerStats above? e.g. if I 
wanted to add some new method, how would I know where to add it?



Comment at: lldb/source/Utility/Diagnostics.cpp:54
+bool Diagnostics::Dump(raw_ostream ) {
+  SmallString<128> diagnostics_dir;
+  std::error_code ec =

I am not sure how common this is, but I have recently seen (not in lldb, but 
another app) a bug, which essentially caused two threads to crash at once (one 
SEGV, one ABRT). In those situations, you probably don't want to crash-printing 
routines to race with each other. You can consider putting a (global) mutex in 
this function, or something like that. (unless the llvm function takes care of 
that already).


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

https://reviews.llvm.org/D134991

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

[Lldb-commits] [PATCH] D134877: [lldb] Get rid of __STDC_LIMIT_MACROS and __STDC_CONSTANT_MACROS

[Pavel Labath via Phabricator via lldb-commits]

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

ship it.


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

https://reviews.llvm.org/D134877

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

[Lldb-commits] [PATCH] D134882: [lldb] Move breakpoint hit reset code to Target::CreateProcess

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I'm not sure what kind of guarantees are you looking for. `m_process_sp` is a 
private member of the Target class, so it's not like just anyone can come in 
and change it. There's no way to stop code from inside the Target class from 
changing it without going through the `CreateProcess` method, but the same can 
be said about the calls to `WillXXX` methods in the process class.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134882

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

[Lldb-commits] [PATCH] D134877: [lldb] Fixes for swig-4.1.0 macro definition correction

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

sure, sounds good. TBH, I am not sure if either of those is really needed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134877

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

[Lldb-commits] [PATCH] D134882: [lldb] Move breakpoint hit reset code to Target::CreateProcess

[Pavel Labath via Phabricator via lldb-commits]

labath created this revision.
labath added reviewers: fdeazeve, jingham.
Herald added a project: All.
labath requested review of this revision.
Herald added a project: LLDB.

This ensures it is run regardless of the method we use to initiate the
session (previous version did not handle connects).


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D134882

Files:
  lldb/source/Target/Process.cpp
  lldb/source/Target/Target.cpp
  lldb/test/API/functionalities/gdb_remote_client/TestProcessConnect.py


Index: lldb/test/API/functionalities/gdb_remote_client/TestProcessConnect.py
===
--- lldb/test/API/functionalities/gdb_remote_client/TestProcessConnect.py
+++ lldb/test/API/functionalities/gdb_remote_client/TestProcessConnect.py
@@ -63,3 +63,54 @@
 self.dbg.GetSelectedTarget().GetProcess().Kill()
 lldbutil.expect_state_changes(self, self.dbg.GetListener(),
   self.process(), [lldb.eStateExited])
+def test_breakpoint_count(self):
+"""
+Test that breakpoint count gets reset for each new connection.
+"""
+class MyResponder(MockGDBServerResponder):
+
+def __init__(self):
+super().__init__()
+self.continued = False
+
+def qfThreadInfo(self):
+return "m47"
+
+def qsThreadInfo(self):
+return "l"
+
+def setBreakpoint(self, packet):
+return "OK"
+
+def readRegister(self, reg):
+# Pretend we're at the breakpoint after we've been resumed.
+return "3412" if self.continued else 
"4747"
+
+def cont(self):
+self.continued = True
+return "T05thread=47;reason:breakpoint"
+
+# Connect to the first process and set our breakpoint.
+self.server.responder = MyResponder()
+target = self.createTarget("a.yaml")
+process = self.connect(target)
+
+bkpt = target.BreakpointCreateByAddress(0x1234)
+self.assertTrue(bkpt.IsValid())
+self.assertEqual(bkpt.GetNumLocations(), 1)
+
+# "continue" the process. It should hit our breakpoint.
+process.Continue()
+self.assertState(process.GetState(), lldb.eStateStopped)
+self.assertEqual(bkpt.GetHitCount(), 1)
+
+# Now kill it, and connect again.
+process.Kill()
+self.server.stop()
+self.server = MockGDBServer(self.server_socket_class())
+self.server.start()
+
+process = self.connect(target)
+
+# The hit count should be reset.
+self.assertEqual(bkpt.GetHitCount(), 0)
Index: lldb/source/Target/Target.cpp
===
--- lldb/source/Target/Target.cpp
+++ lldb/source/Target/Target.cpp
@@ -209,6 +209,7 @@
   if (!listener_sp)
 listener_sp = GetDebugger().GetListener();
   DeleteCurrentProcess();
+  ResetBreakpointHitCounts();
   m_process_sp = Process::FindPlugin(shared_from_this(), plugin_name,
  listener_sp, crash_file, can_connect);
   return m_process_sp;
Index: lldb/source/Target/Process.cpp
===
--- lldb/source/Target/Process.cpp
+++ lldb/source/Target/Process.cpp
@@ -2761,18 +2761,15 @@
 }
 
 Status Process::WillLaunch(Module *module) {
-  GetTarget().ResetBreakpointHitCounts();
   return DoWillLaunch(module);
 }
 
 Status Process::WillAttachToProcessWithID(lldb::pid_t pid) {
-  GetTarget().ResetBreakpointHitCounts();
   return DoWillAttachToProcessWithID(pid);
 }
 
 Status Process::WillAttachToProcessWithName(const char *process_name,
 bool wait_for_launch) {
-  GetTarget().ResetBreakpointHitCounts();
   return DoWillAttachToProcessWithName(process_name, wait_for_launch);
 }
 


Index: lldb/test/API/functionalities/gdb_remote_client/TestProcessConnect.py
===
--- lldb/test/API/functionalities/gdb_remote_client/TestProcessConnect.py
+++ lldb/test/API/functionalities/gdb_remote_client/TestProcessConnect.py
@@ -63,3 +63,54 @@
 self.dbg.GetSelectedTarget().GetProcess().Kill()
 lldbutil.expect_state_changes(self, self.dbg.GetListener(),
   self.process(), [lldb.eStateExited])
+def test_breakpoint_count(self):
+"""
+Test that breakpoint count gets reset for each new connection.
+"""
+class MyResponder(MockGDBServerResponder):
+
+def __init__(self):
+super().__init__()
+self.continued = False
+
+def qfThreadInfo(self):
+return "m47"
+
+def qsThreadInfo(self):
+return "l"
+
+def 

[Lldb-commits] [PATCH] D134877: [lldb] Fixes for swig-4.1.0 macro definition correction

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Looking at https://bugzilla.redhat.com/show_bug.cgi?id=2128646, I'd say that 
the real bug is that we're defining this macro in two places. How about we 
leave these definitions as they are, and remove the one at 
`bindings/interfaces.swig:5` ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134877

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

[Lldb-commits] [PATCH] D134842: Improve dynamic loader support in DynamicLoaderPOSIXDYLD when using core files.

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D134842#3822740 , @yinghuitan 
wrote:

> I am surprised other major companies did not hit this issue.

That could be because this is something specific to your environment. Just to 
be clear, is this happening for *all* core files or only for some of them? If 
only some, is there anything special about the state of the applications that 
produced those core files (e.g. are they in the middle of loading a shared 
library?)
Even though this may very well be the right fix for middle-of-dlopen core dumps 
(we can't really wait for the loading to finish), I suspect this is actually 
masking some other problem, as the amount of time an application spends in the 
RT_ADD state is very brief, and it shouldn't be doing anything crash-prone 
while in there.




Comment at: 
lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DYLDRendezvous.cpp:174-181
+  // If we have a core file, we will read the current rendezvous state
+  // from the core file's memory which will indicate there is nothing
+  // to do, but we need it to load all of the shared libraries. If we
+  // don't change this, then "info.state" will be set to eAdd and the
+  // m_previous.state will be eConsistent and GetAction() will return
+  // eNoAction when we need it to return eTakeSnapshot.
+  if (IsCoreFile())

How about we just change `GetAction` to directly return `eTakeSnapshot` in this 
case?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134842

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

[Lldb-commits] [PATCH] D134849: [LLDB][NativePDB] Fix struct layout when it has anonymous unions.

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

The fact that MSVC does not store all of the anonymous union data is 
unfortunate, though not entirely surprising, given that the goal of debug info 
never was to offer an exact reconstruction of the source AST.

That, I'm not sure if checking for matching initial offsets is sufficient to 
create a semblance of a consistent structure definition. What will this code do 
with structures like this:

  struct S3 {
char x[3];
  };
  struct A {
union {
  struct {
char c1;
S3 s1;
  };
  struct {
S3 s2;
char c2;
  };
};
  } a;

In this case, `a.s1` and `a.c2` overlap, but they don't share the same starting 
offset. If the compiler does not provide data for anonymous structs as well, 
then I think this algorithm will need to be more complicated. I'm not even sure 
they can be reconstructed correctly, as the anonymous structs and unions can 
nest indefinitely. Maybe it would be sufficient to represent this as a "union 
of structs", where each struct gets (greedily) packed with as many members as 
it can hold, and we create as many structs as we need (we can replace the 
struct with a simple member if it would hold just one member)?




Comment at: lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp:321
+  // the artificial anonymous union.
+  lldb::user_id_t au_id = LLDB_INVALID_UID - toOpaqueUid(m_id);
+  uint64_t au_field_bitoffset = 0;

How are these opaque ids computed? Will they produce unique IDs if you have two 
structs like this close together? Might it be better to just make a global 
(local to a symbol file or something) counter and always increment/decrement 
that when creating another type ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134849

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

[Lldb-commits] [PATCH] D134768: [NFCI] More TypeCategoryImpl refactoring.

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

FWIW, phabricator allows you to create stacked patches. Just base your diff on 
top of the previous patch, and then list that patch as a "parent revision" for 
the new one.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134768

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

[Lldb-commits] [PATCH] D134771: [NFCI] Simplify TypeCategoryImpl for-each callbacks.

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.



Comment at: lldb/include/lldb/DataFormatters/TypeCategory.h:136
+  void ForEach(std::function)>
+   callback) {

did you want to add a reference here? A const by-value argument is not 
particularly useful.



Comment at: lldb/include/lldb/DataFormatters/TypeCategory.h:159-162
+  // TypeFilterImpl inherits from SyntheticChildren, so we can't simply 
overload
+  // ForEach on the type of the callback because it would result in "call to
+  // member function 'ForEach' is ambiguous" errors. Instead we use this
+  // templated struct to hold the formatter type and the callback.

What if we just embed the type information into the method name? (So we could 
have a set of `ForEachFormat`,`ForEachSummary`, ... methods instead of a single 
overloaded ForEach)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134771

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

[Lldb-commits] [PATCH] D134656: [LLDB][NativePDB] Add class/union layout bit size.

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D134656#3818759 , @zequanwu wrote:

> In D134656#3818234 , @labath wrote:
>
>> That said, I am kinda surprised that this is the whole thing. I would have 
>> expected to see more here. In dwarf we specify the offsets of individual 
>> class members. Does PDB encode that information?
>
> Yes. It has offsets to non-inherited individual class members.

Yes, I only meant direct members. And when I speak of members here, I am also 
thinking about base classes as they behave very similarly here.

> For members from parent classes, pdb only has information saying that its 
> direct parents class are at some offsets for this class. For class without 
> vtable, it's easy to calculate inherited member offsets by adding parent 
> class offsets with their member offsets. For class with vtable, it's more 
> complicated to calculate the offsets.

Yes, so should we take the offsets from the debug info and provide them to 
clang (so that it does not have to recompute the offsets) ?

>> If not, how does it handle the case when the definition of some class is 
>> missing? If that class is a member of some other class, the offsets of all 
>> subsequent members in the bigger class will be wrong? That will probably be 
>> true even if we are always able to obtain the size of the smaller class, 
>> because of things like vtable pointers.
>
> I don't quite understand this. vtable pointers are ignored in this visitor 
> https://github.com/llvm/llvm-project/blob/main/lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp#L136-L139.

Imagine a class like `struct B : A { virtual void foo(); int b; };` and two 
classes like `struct A1 { virtual void bar(); }; struct A2 { void *a; };`. Both 
A1 and A2 have the same size (`sizeof(void *)`), but that doesn't mean the 
layout of B will be the same if we substitute it for the base 
. If A = A1, then B will reuse the vtable 
pointer of the base, and `offsetof(B, b)` will be 8. That can't happen with A2, 
so the compiler will create a new vtable pointer and `b` offset will be 16 
(2*sizeof(void)) (at least that's how it works in the itanium ABI, but I'm sure 
you could create something similar with the MSVC ABI as well). If you don't 
have the definition of A, then you just can't know which of these two cases  
you are in. That's why I think that getting the member locations from the debug 
info is important.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134656

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

[Lldb-commits] [PATCH] D133858: [lldb] Reset breakpoint hit count before new runs

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D133858#3818755 , @jingham wrote:

> If we use Target::CreateProcess to handle this task, how do we ensure that 
> that will always get called any time we make a new process?

I'm not really sure how to answer that.

Clearly something like that can happen, but I think the overall risk is low. 
The most likely scenario for that happening would be if someone adds a new, 
fourth, method of initiating a process (in addition to launching, attaching and 
connecting), but I just don't know what would that be. But if one is doing 
that, I think it's at least as likely that he will forget to create the 
`DoWillInitiate` method and call the breakpoint reset function from there.

I think the only way to ensure that can't happen is to store the actual hit 
counts within the Process class itself, but I don't think anyone has an apetite 
to design something like that.

Anyone trying to create a function by bypassing a function called CreateProcess 
should think really hard before proceeding. And the more stuff we put into that 
function, the harder it will be for someone to bypass it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133858

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

[Lldb-commits] [PATCH] D134754: [lldb/gdb-server] Better reporting of launch errors

[Pavel Labath via Phabricator via lldb-commits]

labath created this revision.
labath added reviewers: mgorny, jasonmolenda.
Herald added a project: All.
labath requested review of this revision.
Herald added a project: LLDB.

Use our "rich error" facility to propagate error reported by the stub to
the user. lldb-server reports rich launch errors as of D133352 
.

To make this easier to implement, and reduce code duplication, I have
moved the vRun/A/qLaunchSuccess handling into a single
GDBRemoteCommunicationClient function.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D134754

Files:
  lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
  lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
  lldb/test/API/functionalities/gdb_remote_client/TestGDBRemoteClient.py

Index: lldb/test/API/functionalities/gdb_remote_client/TestGDBRemoteClient.py
===
--- lldb/test/API/functionalities/gdb_remote_client/TestGDBRemoteClient.py
+++ lldb/test/API/functionalities/gdb_remote_client/TestGDBRemoteClient.py
@@ -86,7 +86,30 @@
 error = lldb.SBError()
 target.Launch(lldb.SBListener(), None, None, None, None, None,
 None, 0, True, error)
-self.assertEquals("'A' packet returned an error: 71", error.GetCString())
+self.assertRegex(error.GetCString(), "Cannot launch '.*a': Error 71")
+
+def test_launch_rich_error(self):
+class MyResponder(MockGDBServerResponder):
+def qC(self):
+return "E42"
+
+def qfThreadInfo(self):
+return "OK" # No threads.
+
+# Then, when we are asked to attach, error out.
+def vRun(self, packet):
+return "Eff;" + seven.hexlify("I'm a teapot")
+
+self.server.responder = MyResponder()
+
+target = self.createTarget("a.yaml")
+process = self.connect(target)
+lldbutil.expect_state_changes(self, self.dbg.GetListener(), process, [lldb.eStateConnected])
+
+error = lldb.SBError()
+target.Launch(lldb.SBListener(), None, None, None, None, None,
+None, 0, True, error)
+self.assertRegex(error.GetCString(), "Cannot launch '.*a': I'm a teapot")
 
 def test_read_registers_using_g_packets(self):
 """Test reading registers using 'g' packets (default behavior)"""
Index: lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
===
--- lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
+++ lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
@@ -84,6 +84,7 @@
 
 #include "llvm/ADT/ScopeExit.h"
 #include "llvm/ADT/StringSwitch.h"
+#include "llvm/Support/FormatAdapters.h"
 #include "llvm/Support/Threading.h"
 #include "llvm/Support/raw_ostream.h"
 
@@ -799,17 +800,17 @@
   GDBRemoteCommunication::ScopedTimeout timeout(m_gdb_comm,
 std::chrono::seconds(10));
 
-  int arg_packet_err = m_gdb_comm.SendArgumentsPacket(launch_info);
-  if (arg_packet_err == 0) {
-std::string error_str;
-if (m_gdb_comm.GetLaunchSuccess(error_str)) {
-  SetID(m_gdb_comm.GetCurrentProcessID());
-} else {
-  error.SetErrorString(error_str.c_str());
-}
+  // Since we can't send argv0 separate from the executable path, we need to
+  // make sure to use the actual executable path found in the launch_info...
+  Args args = launch_info.GetArguments();
+  if (FileSpec exe_file = launch_info.GetExecutableFile())
+args.ReplaceArgumentAtIndex(0, exe_file.GetPath(false));
+  if (llvm::Error err = m_gdb_comm.LaunchProcess(args)) {
+error.SetErrorStringWithFormatv("Cannot launch '{0}': {1}",
+args.GetArgumentAtIndex(0),
+llvm::fmt_consume(std::move(err)));
   } else {
-error.SetErrorStringWithFormat("'A' packet returned an error: %i",
-   arg_packet_err);
+SetID(m_gdb_comm.GetCurrentProcessID());
   }
 }
 
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
@@ -80,8 +80,6 @@
 
   lldb::pid_t GetCurrentProcessID(bool allow_lazy = true);
 
-  bool GetLaunchSuccess(std::string _str);
-
   bool LaunchGDBServer(const char *remote_accept_hostname, lldb::pid_t ,
uint16_t , std::string _name);
 
@@ -90,19 +88,11 @@
 
   bool KillSpawnedProcess(lldb::pid_t pid);
 
-  /// Sends 

[Lldb-commits] [PATCH] D134570: [lldb] Skip check for conflicting filter/synth when adding a new regex.

[Pavel Labath via Phabricator via lldb-commits]

labath added subscribers: jingham, labath.
labath added a reviewer: jingham.
labath added a comment.

The current behavior is clearly not useful, but I don't have any context to be 
able to determine whether there's something else that can be done about that. 
@jingham might have it.


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

https://reviews.llvm.org/D134570

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

[Lldb-commits] [PATCH] D134518: [lldb][COFF] Add note to forwarder export symbols in symtab

[Pavel Labath via Phabricator via lldb-commits]

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

In D134518#3811382 , @alvinhochun 
wrote:

> In D134518#3811218 , @labath wrote:
>
>> Ok, so is there any harm in keeping them this way?
>>
>> The symbols may not be very useful, but I would not say that they are 
>> useless. If, for whatever reason, the user finds himself inspecting the part 
>> of the memory covered by the forwarder string, then with this symbol, that 
>> memory would symbolicate as `forwarded_symbol+X`, which seems nice.
>
> I guess you're right, we can keep these symbols. Though do you think it make 
> sense to synthesize a demangled name for the symbol, let's say `ExportName 
> (forwarded to kernel32.LoadLibrary)`?

Possibly. Are these basically the same as "undefined" (i.e., defined by another 
shared library) symbols on ELF? If so, then it may make sense to mark them as 
eSymbolTypeUndefined, in case some generic algorithm wants to do something with 
them. I believe MachO also has this notion of forwarding a symbol to a 
particular shared library (I think they call it "two-level namespacing"), so 
you may want to look at how those are represented.

But overall, I don't think this is particularly important, and we can change 
this later if we have a need for it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134518

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

[Lldb-commits] [PATCH] D134426: [lldb][COFF] Match symbols from COFF symbol table to export symbols

[Pavel Labath via Phabricator via lldb-commits]

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

Seems fine to me from a general perspective. I think others have already 
checked the windows parts.




Comment at: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:892
   }
+  std::sort(export_list.begin(), export_list.end(), RVASymbolListCompareRVA);
+  return export_list;

alvinhochun wrote:
> labath wrote:
> > Can you have multiple symbols pointing to the same address? Make this 
> > should use `stable_sort` instead?
> Yes, it can happen. The ordering shouldn't affect what 
> AppendFromCOFFSymbolTable does but I suppose stable_sort does make it more 
> deterministic to avoid future issues down the line.
Yeah, it's better to avoid having this kind of nondeterministic behavior that 
can differ from run to run. LLDB is not so string about it as clang/llvm, but 
it's still a good idea.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134426

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

[Lldb-commits] [PATCH] D134636: [lldb][Windows] Always call SetExecutableModule on debugger connected

[Pavel Labath via Phabricator via lldb-commits]

labath accepted this revision.
labath added inline comments.
This revision is now accepted and ready to land.



Comment at: lldb/test/Shell/Target/dependent-modules-nodupe-windows.test:21-22
 # CHECK-NEXT: .main.exe
-# CHECK-NEXT: .shlib.dll
+# CHECK-NEXT: ntdll.dll
+# CHECK-NEXT: kernel32.dll
+# CHECK:  .shlib.dll

I'm not sure if hardcoding the order of system libraries (something which you 
have no control of) is such a good idea.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134636

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

[Lldb-commits] [PATCH] D134661: [lldb][TypeSystemClang] Honor DW_AT_rvalue_reference when creating C++ FunctionPrototypes

[Pavel Labath via Phabricator via lldb-commits]

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

Looks straight forward enough. Tagging @zequanwu, as he might want to do 
something similar for PDB.




Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:41
 #include "llvm/Demangle/Demangle.h"
 
 #include "clang/AST/CXXInheritance.h"

I guess the `clang/AST/Type.h` include should be grouped with the other clang 
includes. I'd recommend deleting this empty line and letting clang-format sort 
the includes for you.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134661

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

[Lldb-commits] [PATCH] D133858: [lldb] Reset breakpoint hit count before new runs

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D133858#3816105 , @fdeazeve wrote:

> In D133858#3805630 , @labath wrote:
>
>> I am afraid that this patch misses one method of initiating a debug session 
>> -- connecting to a running debug server (`process connect`, 
>> `SBTarget::ConnectRemote`). The breakpoint hit counts don't get reset in 
>> case of a reconnect. This isn't a particularly common use case (and the only 
>> reason I've noticed it is that for `PlatformQemuUser`, all "launches" are 
>> actually "connects" under the hood 
>> ),
>>  but I've verified that this problem can be reproduced by issuing connect 
>> commands manually (on the regular host platform). I'm pretty sure that was 
>> not intentional.
>>
>> Fixing this by adding another callout to `ResetBreakpointHitCounts` would be 
>> easy enough, but I'm also thinking if there isn't a better place from which 
>> to call this function, one that would capture all three scenarios in a 
>> single statement. I think that one such place could be 
>> `Target::CreateProcess`. This function is called by all three code paths, 
>> and it's a very good indicator that we will be starting a new debug session.
>>
>> What do you think?
>
> My understanding is that there is an obligation of calling the WillXX methods 
> before calling XX, so by placing the reset code in the WillXX functions we 
> can rely on that guarantee. Right now, the same is implicit for "one must 
> call Target::CreateProcess before launching or attaching". We could instead 
> define a WillConnect and have the usual contract for that.

I wouldn't really call it a "usual" contract, but yes, I'm sure this could be 
fixed by adding a WillConnect method. It might be also sufficient to just call 
WillAttach from at some appropriate place, since a "connect" operation looks 
very similar to an "attach", and a lot of the initialization operations are the 
same. I think we're already something like this somewhere (maybe for 
DidAttach?). However, that still leaves us with three (or two) places that need 
to be kept in sync.

> The code is fairly new to me, so I'm not confident enough to make a judgement 
> call here. Thoughts?

I don't see any advantage in doing this particular action "just before" a 
launch, as opposed to doing in on process creation, so I would definitely do it 
that way.

I also find it weird to be going through the Process class just to call another 
Target method, when all of the launch/attach/connect operations already go 
through the Target class, and so it should be perfectly capable of calling that 
method on its own.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133858

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

[Lldb-commits] [PATCH] D134333: When there are variable errors, display an error in VS Code's local variables view.

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

The vscode change seems fine to me, but I don't consider myself an authority on 
that.

In D134333#3812653 , @jingham wrote:

> In D134333#3812448 , @clayborg 
> wrote:
>
>> I just did a search through our test sources and we use 
>> GetError().GetCString() 34 times in our test suites python files. So the 
>> SBError::GetCString() is being misused a lot like this already and is 
>> probably making some tests flaky. I would highly recommend we fix 
>> SBError::GetCString() to make sure things work correctly. If people are 
>> already doing this, or if they can do this with our API, then we should make 
>> our API as stable as possible for users even if it costs a bit more memory 
>> in our string pools.
>
> When we return Python strings from the SWIG'ed version of 
> GetError().GetCString() does the Python string we make & return copy the 
> string data, or is it just holding onto the pointer?  In C/C++ if someone 
> returns you a char * unless explicitly specified you assume that string is 
> only going to live as long as the object that handed it out.  But a python 
> string is generally self-contained, so you wouldn't expect it to lose its 
> data when the generating object goes away.  I haven't checked yet, but if 
> this is how SWIG handles making python strings from C strings, then this 
> wouldn't be an error in the testsuite, only in C++ code.  If that's not how 
> it works, that might be a good way to finesse the issue.

The python version is fine because swig SBError::GetCString wrapper gets 
essentially a pointer to the SBError objects (wrapped as PyObject), and returns 
another PyObject with the (copied) string (also a PyObject). All the lifetime 
management happens outside of that function.




Comment at: 
lldb/test/API/tools/lldb-vscode/variables/TestVSCode_variables.py:48-53
+contains = verify_value in actual_value
+self.assertTrue(contains,
+('"%s" value "%s" doesn\'t contain'
+ ' "%s")') % (
+key, actual_value,
+verify_value))




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134333

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

[Lldb-commits] [PATCH] D134581: [lldb] Prevent re-adding a module that is already loaded

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D134581#3817457 , @alvinhochun 
wrote:

> In D134581#3815766 , @jasonmolenda 
> wrote:
>
>> In D134581#3813757 , @alvinhochun 
>> wrote:
>>
>>> In `ProcessWindows::OnDebuggerConnected`, it first checks 
>>> `GetTarget().GetExecutableModule()`. Only when the returned module is null 
>>> (i.e. the binary hasn't already been loaded) then it calls 
>>> `GetTarget().SetExecutableModule(module, eLoadDependentsNo)`. If I 
>>> understood you correctly, then the right thing to do there should be to 
>>> call `GetTarget().SetExecutableModule(module, eLoadDependentsNo)` in all 
>>> cases, without checking `GetExecutableModule`, right?
>>>
>>> It seems to make sense, but I may need some comments from other reviewers 
>>> on this.
>>
>> Just my opinion, but I know how DynamicDarwinLoader works is that when it 
>> starts the actual debug session, it clears the image list entirely (iirc or 
>> maybe it just calls Target::SetExecutableModule - which is effectively the 
>> same thing).  I don't know how Windows works, but on Darwin we pre-load the 
>> binaries we THINK will be loaded, but when the process actually runs, 
>> different binaries may end up getting loaded, and we need to use what the 
>> dynamic linker tells us instead of our original logic.  
>> `GetTarget().SetExecutableModule(module, eLoadDependentsNo)` would be one 
>> option, or clear the list and start adding, effectively the same thing.
>
> Sounds right. On Windows, events from the debug loop will tell us which DLLs 
> are actually being loaded by the process and we add them to the module list.

Seems reasonable to me. I'm not actually sure if we're doing this on 
linux/posix (as I still see duplicated vdso modules occasionaly -- but this 
could be caused by other problems as well), but if we're not -- we probably 
should.

>> I think it would be more straightforward than adding this change to 
>> Target::GetOrAddModule.
>
> Here's the thing, even if we change the Windows debugging support to clear 
> the module list when starting the process, the logic of 
> `Target::GetOrAddModule` still appears to be flawed if it can result in 
> duplicated modules in the target module list, so IMO it needs to be fixed 
> regardless.

I can totally believe that there is a bug in the GetOrAddModule, but I would 
not be able to tell you if this is the right fix or not. As for multiple 
modules, I can say that on linux it is actually possible to load the same 
shared library more than once (into different "linker namespaces"). LLDB does 
not currently support that, but I would like to support it one day. I don't 
know whether this change would make that harder or easier, though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134581

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

[Lldb-commits] [PATCH] D134656: [LLDB][NativePDB] Add class/union layout bit size.

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

That said, I am kinda surprised that this is the whole thing. I would have 
expected to see more here. In dwarf we specify the offsets of individual class 
members. Does PDB encode that information? If not, how does it handle the case 
when the definition of some class is missing? If that class is a member of some 
other class, the offsets of all subsequent members in the bigger class will be 
wrong? That will probably be true even if we are always able to obtain the size 
of the smaller class, because of things like vtable pointers.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134656

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

[Lldb-commits] [PATCH] D134656: [LLDB][NativePDB] Add class/union layout bit size.

[Pavel Labath via Phabricator via lldb-commits]

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

LGTM modulo formatting.




Comment at: lldb/test/Shell/SymbolFile/NativePDB/packed_class_layout.cpp:24
+
+union __attribute__((packed, aligned(1))) U {
+  char c[2];

I suspect these attributes do not materially change the layout of the union 
when it stands on its own. It might be more interesting to take this 
under-aligned union, place it into a (regular) struct, and then check whether 
we can print it correctly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134656

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

[Lldb-commits] [PATCH] D134041: [LLDB] Enable non-trivial types in EmulateInstruction::Context

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D134041#3806994 , @jasonmolenda 
wrote:

> In D134041#3805941 , @labath wrote:
>
>> In D134041#3805034 , 
>> @DavidSpickett wrote:
>>
 That said I would *love* is someone changed the RegisterInfo structs into 
 something saner, but I think that will need to be more elaborate than 
 simply stuffing a std::vector member into it. I can give you my idea of 
 how this could work, if you're interested in trying this out.
>>>
>>> Sure I'd be interested in that. I've just been hacking on this small part 
>>> of it so I don't have the full picture yet.
>>
>> I think that part of the problem is that nobody has a full picture of how 
>> RegisterInfos work anymore. :)
>>
>> I don't claim to have this fully thought out, but the idea goes roughly like 
>> this. For the past few years, we've been moving towards a world where LLDB 
>> is able to fill in lots of details about the target registers. I think we're 
>> now at a state where it is sufficient for the remote stub to specify the 
>> register name and number, and lldb will be able to fill on most of the 
>> details about that register: EH/DWARF/"generic" numbers, subregisters, etc. 
>> However, this code is only invoked when communicating remote stub -- not for 
>> core files.
>> On one hand, this kind of makes sense -- for core files, we are the source 
>> of the register info, so why wouldn't we provide the complete info straight 
>> away?
>
> An aside, I'm working with a group inside apple that has a JTAG style 
> debugger that can access not only the normal general purpose 
> registers/floating point/vector registers, but control registers like 
> AArch64's MDSCR_EL1 as one example. I haven't figured out the best format for 
> them to express this information in a Mach-O corefile yet, but I am thinking 
> towards a Mach-O LC_NOTE where they embed an XML register description for all 
> the registers they can provide (and it will require that size and maybe 
> offset are explicitly specified, at least), and a register context buffer of 
> bytes for all of the registers.  lldb would need to augment its list of 
> available registers with this.

Thanks for jumping in Jason. I forgot about the size field. I think that we 
need that as well. As for the offset, how do the Mach-O core files work? Are 
all registers placed into a single note segment? (so that an "offset" is 
well-defined). In elf, the registers are scattered in multiple notes (roughly 
according to register sets), and we need to (arbitrarily) concatenate them so 
that a single offset value is meaningful. But what this really confirms to me 
that the notion of "static" register info lists is just not sufficient any more.

> My vague memory is that they may have different registers available on each 
> core ("thread") so we would need to do this on per-"thread" basis.

That's interesting, but I think we should actually be in a fairly good shape 
for that now, since in the case of ARM SVE, we can have each thread configure 
the scalable registers with a different size.

> This is all vague hand-wavy at this point, but they have the information and 
> the debugger users want this information. At some point I'll want the 
> corefile to be able to augment or replace lldb's register information 
> (probably augment).

Seems reasonable to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134041

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

[Lldb-commits] [PATCH] D134493: [lldb][TypeSystemClang] Deduce lldb::eEncodingUint for unsigned enum types

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

lol, I knew about the last two parts (not that I agree with them, but I think 
we have about an equal amount of code which relies on it, and that which tries 
to work around it), but the first one is really weird. I think we have invented 
a middle ground between sign- and zero-extension.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134493

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

[Lldb-commits] [PATCH] D134333: When there are variable errors, display an error in VS Code's local variables view.

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D134333#3807511 , @clayborg wrote:

> In D134333#3805945 , @labath wrote:
>
>> Do we actually promise that strings returned by the SB API will live 
>> forever? That's not something *I* would want to promote. I always though 
>> that we're ConstStringifying the return values only when we don't know any 
>> better...
>
> Anything that does return a "const char *" should currently live forever, at 
> least that is what we have been doing so far.

I know there have been patches recently which changed some functions to return 
constified strings, but my impression is that things did not start out that 
way. From a cursory search I can find a number of (fairly old) APIs that return 
strings whose lifetime is less than "forever": SBBreakpoint::GetCondition, 
SBBreakpoint::GetThreadName, SBData::GetString, SBFrame::Disassemble, ... The 
constifying changes I remember were usually tied to us changing some `const 
char *`s into StringRefs internally, which mean we could no longer forward the 
string at the SB level (as it might not be null terminated). Constructing a 
temporary string was not possible because it would destruct before the function 
returns.

However, that's not the case here. The strings lifetime is the same as the 
enclosing SBError object, and I don't think it's unusual for c++ objects to be 
handing out pointers to parts of themselves. In this case, one could write the 
code in question as something like:

  if (SBError err = top_scope->GetError()) {
do_stuff(err.GetCString());
  }



> We don't want anyone thinking they should free the pointer.

I agree with that, and I am not aware of any situation where we do that.

The issue here is we were previously handing out a "Status::m_string.c_str()" 
pointer whose lifespan used to be tied to the SBError's lifespan, but we have a 
lot of APIs that return a SBError instance. So if you did code like:

> So that the temp objects can be use to safely grab the string. But seeing as 
> we already have the GEtCString() API, I am guessing that are probably latent 
> bugs right now due to the unexpectedly short lifespan of the string.

One can use the a temporary object to grab the string, though it is somewhat 
complicated by the fact that the result can be null. You need some wrapper like:

  optional to_optional_string(const char *s) { if (s) return string(s); 
else return nullopt; }
  
  ...
  auto s = to_optional_string(get_temporary_error().GetCString());

Without the null values, a plain string assignment would work fine.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134333

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

[Lldb-commits] [PATCH] D134518: [lldb][COFF] Skip forwarder export symbols in symtab

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D134518#3811206 , @alvinhochun 
wrote:

> In D134518#3811160 , @labath wrote:
>
>> Where is the "dll description string" that they point to? Could they be made 
>> to point to that?
>
> The current symbol address is already pointing to it.

Ok, so is there any harm in keeping them this way?

The symbols may not be very useful, but I would not say that they are useless. 
If, for whatever reason, the user finds himself inspecting the part of the 
memory covered by the forwarder string, then with this symbol, that memory 
would symbolicate as `forwarded_symbol+X`, which seems nice.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134518

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

[Lldb-commits] [PATCH] D132815: [LLDB] Do not dereference promise pointer in `coroutine_handle` pretty printer

[Pavel Labath via Phabricator via lldb-commits]

labath accepted this revision.
labath added a comment.

Seems fine (with the caveat that all I know about coroutines was learned in 
this review).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132815

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

[Lldb-commits] [PATCH] D134426: [lldb][COFF] Match symbols from COFF symbol table to export symbols

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.



Comment at: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:892
   }
+  std::sort(export_list.begin(), export_list.end(), RVASymbolListCompareRVA);
+  return export_list;

Can you have multiple symbols pointing to the same address? Make this should 
use `stable_sort` instead?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134426

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

[Lldb-commits] [PATCH] D134509: [LLDB][NativePDB] Let native pdb use class layout in debug info.

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

It's not clear to me how much of this patch is pure refactoring, and how much 
of it is adding new features. It would have been better to split that out into 
two patches.

I see some layout handling code in `UdtRecordCompleter` constructor, but that's 
just two lines of code. Is that it, or should I look elsewhere?




Comment at: lldb/include/lldb/Symbol/TypeSystem.h:31-32
 class DWARFASTParser;
 class PDBASTParser;
+class PdbAstBuilder;
 

Uh-oh. This is definitely not an intuitive naming scheme. How about we keep 
this class in the `lldb_private::npdb` namespace? You can forward-declare it 
there just as well.

Layering-wise, this code here is pretty bad, but moving the parser declaration 
into the global namespace is not going to make that better. And then you can 
undo all of the namespacing churn in the patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134509

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

[Lldb-commits] [PATCH] D134518: [lldb][COFF] Skip forwarder export symbols in symtab

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Where is the "dll description string" that they point to? Could they be made to 
point to that?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134518

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

[Lldb-commits] [PATCH] D134518: [lldb][COFF] Skip forwarder export symbols in symtab

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

They may not be useful (at the moment), but if they're not actively causing 
harm (e.g. stopping some other feature from functioning, or if we're badly 
misrepresenting them in the symtab output), then I think we should keep them. 
You never know -- maybe someone will find a use for them.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134518

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

[Lldb-commits] [PATCH] D134493: [lldb][TypeSystemClang] Deduce lldb::eEncodingUint for unsigned enum types

[Pavel Labath via Phabricator via lldb-commits]

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

Ok, sounds good then.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134493

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

[Lldb-commits] [PATCH] D134344: [WIP][lldb][test] 1 - Remove gmodules debug_info variant: add decorator for API tests that explicitly test gmodules

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D134344#3805953 , @Michael137 
wrote:

> that would require an audit of each API test right?

Not really. I think this could be a purely mechanical change which replaces 
`NO_DEBUG_INFO_TESTCASE = False` with something different. Ideally I'd make 
that something a "inheriting from a different class". So we could have 
something like `APITestCase` and a `DebugInfoTestCase` (inheriting from the 
first), and the tests which want debug info replication (one can also imagine 
different kinds of replication for some other tests) would inherit from the 
latter.

In D134344#3806509 , @aprantl wrote:

> But I'm also missing the context as to why this would be desirable, so if 
> there's a good reason, let me know!

I have two reasons for that:
The first is that from a simply engineering perspective, doing it the other way 
around seems cleaner, as now we kind of have two ways to avoid replication. 
Either you don't inherit from the replicated test base clase (all lldb-server 
and lldb-vscode tests do that), or you do, but then mark yourself as 
NO_DEBUG_INFO_TESTCASE.
Secondly, it seems to be that no-replication is a better default. We have a lot 
of features that don't (or shouldn't) depend on the kind of debug info we're 
using, and we're probably forgetting to add this attribute to some of them. 
It's possible those tests are adding some marginal debug info coverage, but 
it's hard to rely on that, because noone know what that is. So I'd say that an 
opt-in is a better default (particularly for the tests we're adding nowadays), 
and the replication should be done when you know you're doing something 
debug-heavy.
I also think that having a more opt-in mechanism could enable us to do *more* 
replication. For example, I think that running the some tests in both DWARF v4 
and v5 modes would be interesting, but I definitely wouldn't want to run all of 
them, all the time.

In D134344#3811091 , @Michael137 
wrote:

>> (B) Keep the `gmodules` category in the debug_info categories but add an 
>> indicator (e.g., by making the `debug_info_categories` a dictionary) that 
>> will skip replication if set. That would solve (1). And (2) would work as it 
>> does today without changes.
>
> Uploaded alternative diff that implements this option. Seems simpler since 
> tests in the `gmodules` category Just Work and we don't need to special-case 
> `gmodules` in several places
>
> https://reviews.llvm.org/D134524

It's not exactly how I was imagining this, but I like it. :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134344

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

[Lldb-commits] [PATCH] D134493: [lldb][TypeSystemClang] Deduce lldb::eEncodingUint for unsigned enum types

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D134493#3810290 , @Michael137 
wrote:

> Wasn't sure how to properly test this since the original reproducer 
> technically relies on implementation-defined behaviour (i.e., initialising a 
> bitfield with an out-of-range value). Suggestions are welcome

I'm probably missing something, but what exactly is undefined about that test 
program? The number eight fits comfortably in four bits, and afaik it is a 
valid value for the `EnumVals` type because it has a fixed underlying type.

Other than that, this seems fine to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134493

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

[Lldb-commits] [PATCH] D134333: When there are variable errors, display an error in VS Code's local variables view.

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Do we actually promise that strings returned by the SB API will live forever? 
That's not something *I* would want to promote. I always though that we're 
ConstStringifying the return values only when we don't know any better...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134333

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

[Lldb-commits] [PATCH] D134041: [LLDB] Enable non-trivial types in EmulateInstruction::Context

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D134041#3805034 , @DavidSpickett 
wrote:

>> That said I would *love* is someone changed the RegisterInfo structs into 
>> something saner, but I think that will need to be more elaborate than simply 
>> stuffing a std::vector member into it. I can give you my idea of how this 
>> could work, if you're interested in trying this out.
>
> Sure I'd be interested in that. I've just been hacking on this small part of 
> it so I don't have the full picture yet.

I think that part of the problem is that nobody has a full picture of how 
RegisterInfos work anymore. :)

I don't claim to have this fully thought out, but the idea goes roughly like 
this. For the past few years, we've been moving towards a world where LLDB is 
able to fill in lots of details about the target registers. I think we're now 
at a state where it is sufficient for the remote stub to specify the register 
name and number, and lldb will be able to fill on most of the details about 
that register: EH/DWARF/"generic" numbers, subregisters, etc. However, this 
code is only invoked when communicating remote stub -- not for core files.
On one hand, this kind of makes sense -- for core files, we are the source of 
the register info, so why wouldn't we provide the complete info straight away? 
However, it means that the information that "`ah` is a one byte sub-register of 
`rax` at offset 1" is repeated multiple times (we currently have three core 
file plugins, times the number of architectures they support). If we made core 
file register infos go through this "augmentation" process, then we could unify 
our core file/live process flow more, and relieve the core file plugins of the 
burden of dealing with the subregisters -- all they'd need to know is how to 
read/write whole registers, and the generic code would take care of all the 
subregisters.
This would also mean that *all* register infos being handled by generic code 
would be DynamicRegisterInfos, which means we could drop the avoid this POD 
business, and just replace that class with something modern and easy to use. 
The only place where we would need to store static arrays would be in the 
bowels of individual plugins, but these would be simpler than the current 
RegisterInfo struct, as a lot of this info would be deduced (maybe including 
the register type information that you're trying to introduce), and we might 
even have each plugin store the info in whichever format it sees fit -- the 
only requirement would be that a DynamicRegisterInfo comes out at the end. Some 
plugins may choose not to store static info at all, as we're already running 
into the limits of what can be stored statically -- if an architecture has 
multiple optional registers sets (whose presence is only known at runtime), 
then its impossible to stuff those registers into a static array -- I believe 
all our AArch64 registers are currently dynamic for this reason.

I know this is somewhat vague, but that's why this is just an idea. Someone 
would have to try it out to find all the issues and figure them out. I can try 
to help if you want to take it on.




Comment at: lldb/include/lldb/Core/EmulateInstruction.h:196
+  struct RegisterPlusOffsetStruct {
 RegisterInfo reg;  // base register
 int64_t signed_offset; // signed offset added to base register

DavidSpickett wrote:
> labath wrote:
> > I actually think that the simplest solution here would be to store the 
> > RegisterInfos as pointers. Copying them around doesn't make sense, 
> > particularly if their size is about to grow.
> True, but sounds like we're going toward adding a pointer to the info. So 
> that should keep the size constant.
It should, but regardless of that, I'm surprised to see the structs being 
stored here, I'm not aware of any other place which stores RegisterInfos be 
value.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134041

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

[Lldb-commits] [PATCH] D134344: [WIP][lldb][test] 1 - Remove gmodules debug_info variant: add decorator for API tests that explicitly test gmodules

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

celebration_balloons 


I like this a lot. Incidentally, I believe the main reason these tests don't 
work on non-darwin platforms is because their system libraries are not 
modularized. If you make the gmodules tests self-contained (which I would 
recommend, for the sake of reproducibility, anyway), then I think a lot of 
these tests could run elsewhere as well.

In D134344#3805270 , @Michael137 
wrote:

> An alternative would be to keep the `debug_info` category and simply flag it 
> as not to be run by default unless specified explicitly in the test. That way 
> we wouldn't need the `MAKE_GMODULES` in the Makefile and the special 
> `MAKE_DSYM` rule in https://reviews.llvm.org/D134345

Avoiding the MAKE_GMODULES repetition would definitely be nice, but I might 
try(*) do it slightly differently:

- keep `gmodules` as a category, but not a *debug info* category. Among other 
things this enables running all gmodules tests with the `--category gmodules` 
flag.
- teach the debug info replication to ignore tests with the gmodules category 
(just like it does for `@no_debug_info_test_case` tests). This step wouldn't be 
necessary if we made debug info replication opt-in instead of opt-out, as 
discussed on one of the previous patches (@JDevlieghere might remember which 
one it was)
- teach `buildDefault` to build with gmodules enabled in case the test is 
annotated with the gmodules category.

(*) I'm not entirely sure how this would work out, but I think it should be 
fine.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134344

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

[Lldb-commits] [PATCH] D133858: [lldb] Reset breakpoint hit count before new runs

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I am afraid that this patch misses one method of initiating a debug session -- 
connecting to a running debug server (`process connect`, 
`SBTarget::ConnectRemote`). The breakpoint hit counts don't get reset in case 
of a reconnect. This isn't a particularly common use case (and the only reason 
I've noticed it is that for `PlatformQemuUser`, all "launches" are actually 
"connects" under the hood 
),
 but I've verified that this problem can be reproduced by issuing connect 
commands manually (on the regular host platform). I'm pretty sure that was not 
intentional.

Fixing this by adding another callout to `ResetBreakpointHitCounts` would be 
easy enough, but I'm also thinking if there isn't a better place from which to 
call this function, one that would capture all three scenarios in a single 
statement. I think that one such place could be `Target::CreateProcess`. This 
function is called by all three code paths, and it's a very good indicator that 
we will be starting a new debug session.

What do you think?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133858

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

[Lldb-commits] [PATCH] D133910: [NFCI] Refactor FormatterContainerPair into TieredFormatterContainer.

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.



Comment at: lldb/include/lldb/DataFormatters/TypeCategory.h:76
+uint32_t result = 0;
+for (auto sc : m_subcontainers) {
+  result += sc->GetCount();

According to 

 this loop body should not use the `{}` braces.



Comment at: lldb/include/lldb/DataFormatters/TypeCategory.h:90
+}
+assert(false && "formatter index out of range");
+  }

Is this reachable from the SB API (perhaps via 
`SBTypeCategory::GetXXXAtIndex`). If so, we'd want to do something different 
here (return a default/empty value), or add a check somewhere in the SB layer.



Comment at: lldb/include/lldb/DataFormatters/TypeCategory.h:132
+}
+assert(false && "formatter index out of range");
   }

same question.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133910

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

[Lldb-commits] [PATCH] D134196: [lldb][COFF] Rewrite ParseSymtab to list both export and symbol tables

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I don't know how coff symbol tables work, but this seems reasonable to me.




Comment at: lldb/test/Shell/ObjectFile/PECOFF/symbols-export-table.yaml:10
+# CHECK-NEXT: D   Code 0x0001800010100x{{[0-9a-f]+}} 
0x{{[0-9a-f]+}} exportfunc
+# CHECK-NEXT: Code 0x0001800010000x{{[0-9a-f]+}} 
0x{{[0-9a-f]+}} entry
+# CHECK-NEXT: Code 0x0001800010100x{{[0-9a-f]+}} 
0x{{[0-9a-f]+}} exportfunc

Even though it does not look like that, this will match an entry with any flags 
(because FileCheck does a substring match). The simplest solution would be to 
match the preceding UserID field as well.



Comment at: lldb/test/Shell/ObjectFile/PECOFF/symbols-export-table.yaml:34-75
+  ImportTable:
+RelativeVirtualAddress: 0
+Size:0
+  ResourceTable:
+RelativeVirtualAddress: 0
+Size:0
+  ExceptionTable:

Could this be reduced? Maybe by removing these zero fields?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134196

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

[Lldb-commits] [PATCH] D134033: [lldb/Plugins] Improve error reporting with reading/writing memory in a Scripted Process (WIP)

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.



Comment at: lldb/include/lldb/API/SBError.h:95
 private:
-  std::unique_ptr m_opaque_up;
+  std::shared_ptr m_opaque_sp;
 

This is technically an ABI break (changes `sizeof(SBError)`). I don't care, but 
someone might.



Comment at: lldb/source/Plugins/ScriptInterpreter/Python/SWIGPythonBridge.cpp:27
 template  const char *GetPythonValueFormatString(T t);
+// FIXME: This should probably be a PyObject * instead of PythonObject
+template <> const char *GetPythonValueFormatString(python::PythonObject*) { 
return "O"; }

Yes, it should.



Comment at: lldb/source/Plugins/ScriptInterpreter/Python/SWIGPythonBridge.h:35
 namespace lldb_private {
+  namespace python {
+  typedef struct swig_type_info swig_type_info;

we don't indent namespaces



Comment at: 
lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.cpp:186
+  StatusSP status_sp = std::make_shared(error);
+  PythonObject* sb_error = new PythonObject(ToSWIGWrapper(status_sp));
+  

mib wrote:
> @labath In order to pass down the `Status&` to python, I create a `StatusSP` 
> (to avoid leaking the `lldb_private::Status` type to python), and turn that 
> into a `python::PyObject` by calling `ToSWIGWrapper`. This is why I moved its 
> declaration to `SWIGPythonBridge.h`.
> 
> Finally, the `python::PyObject` is wrapped into another `PythonObject*` so it 
> can be passed to `GetPythonValueFormatString` and `PyObject_CallMethod` in 
> `ScriptedPythonInterface::Dispatch`
> 
> I tried to follow the logic explained in 7f09ab0, however, when 
> `ToSWIGWrapper` is called at runtime, it crashes in Python:
> 
> ```
> * thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS 
> (code=1, address=0x10)
> frame #0: 0x0001056f3c3c Python`PyTuple_New + 212
> frame #1: 0x000135524720 
> liblldb.16.0.0git.dylib`SWIG_Python_NewShadowInstance(data=0x611d6340,
>  swig_this=0x000105ec5d70) at LLDBWrapPython.cpp:2284:28
>   * frame #2: 0x000135515a00 
> liblldb.16.0.0git.dylib`SWIG_Python_NewPointerObj(self=0x, 
> ptr=0x61dc4040, type=0x00014448c140, flags=1) at 
> LLDBWrapPython.cpp:2395:22
> frame #3: 0x0001355157f4 
> liblldb.16.0.0git.dylib`lldb_private::python::ToSWIGHelper(obj=0x61dc4040,
>  info=0x00014448c140) at python-swigsafecast.swig:5:29
> frame #4: 0x000135515e14 
> liblldb.16.0.0git.dylib`lldb_private::python::ToSWIGWrapper(status_sp=std::__1::shared_ptr::element_type
>  @ 0x60ac9a18 strong=3 weak=1) at python-swigsafecast.swig:37:10
> frame #5: 0x0001367383c4 
> liblldb.16.0.0git.dylib`lldb_private::ScriptedProcessPythonInterface::ReadMemoryAtAddress(this=0x611cd2c0,
>  address=8034160640, size=512, error=0x00016b35c650) at 
> ScriptedProcessPythonInterface.cpp:161:45
> ```
> 
> Am I doing something wrong or maybe I'm missing something ?
> 
> 
Well.. I'm not sure if this is the cause of the crash, but one problem I see is 
the passing of `PythonObject*` to the Dispatch function, which forwards it 
unmodified to the `PyObject_CallMethod` function. Python clearly does not know 
anything about our lldb_private::python::PythonObject wrappers. You'll either 
need to pass a PyObject here, or teach Dispatch how to unwrap a PythonObject.

I also don't think that it was necessary to move all of this code around. I 
don't see why would Status be any different from say `StructuredDataImpl`, 
which is already passed as a plain reference (no smart pointers). Using an 
lldb-private object inside `python-swigsafecast.swig` is still ok, as that code 
is still part of liblldb. We already include [[ 
https://github.com/llvm/llvm-project/blob/main/lldb/bindings/python/python.swig#L121
 | some ]] internal headers from the swig files, but I think that in this case 
a forward declaration should be enough as well (you may need to add a 
`SBError(const Status &)` constructor). The opposite (moving this code to 
"regular" files) is not ideal as well, as you've needed to add a bunch of SB 
forward declarations into `SWIGPythonBridge`, and split the implementation of 
ToSWIGWrapper into two files.

I also don't think the shared_ptr transition is helping with anything (and it 
is an ABI break). It might be necessary if the problem was that the SBError 
object was being copied, and you couldn't get a hold of the copy which the user 
code has modified, but in that case you'd also have to modify the objects copy 
constructors to do a shallow (instead of a deep) copy (which in turn could 
break other code which was relying on this).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134033

___
lldb-commits mailing list
lldb-commits@lists.llvm.org

[Lldb-commits] [PATCH] D133906: [lldb] Generate lldb-forward with .def file

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D133906#3792666 , @JDevlieghere 
wrote:

> In D133906#3792230 , @labath wrote:
>
>>   // no .def file
>>   #define LLDB_FORWARD_CLASS(cls) \
>> namespace lldb_private { class cls; } \
>> namespace lldb {  using cls##SP = std::shared_ptr; } \
>> ...
>>   
>>   LLDB_FORWARD_CLASS(Foo)
>>   LLDB_FORWARD_CLASS(Bar)
>>   ...
>
> Works for me, but I don't see how that would help with go-to definition. 
> Xcode still won't show you the macro expansion so there's nothing to click 
> through, which was Jim's complaint.

Right, I see. I misunderstood the problem somehow. That said, I can imagine 
Xcode offering a (clickable) tooltip showin the macro expansion in this case. I 
know of an IDE that does that, and it's pretty cool.

In D133906#3793505 , @clayborg wrote:

> Wouldn't this also slow down compilation a bit? Each file that #include 
> "lldb-forward.h" will not do a large amount of preprocessor stuff for each 
> and every file that is compiled that includes this?

Technically yes, but I very much doubt the difference will be measurable. The 
slowness of compiling c++ comes from instantiating templates and other fancy 
stuff. The preprocessor step is ridiculously fast.


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

https://reviews.llvm.org/D133906

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

[Lldb-commits] [PATCH] D134041: [LLDB] Enable non-trivial types in EmulateInstruction::Context

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I think the main reason that the RegisterInfo struct is such as it is, is 
because it wants to be trivially constructible (or whatever the c++ term for 
that is). I'm pretty sure that adding a std::string member will make it stop 
being that, and I don't think we suddenly want to run global constructors for 
all of the global RegisterInfo instances we have (and we have a lot of them).

If you wanted to preserve the existing patterns, the right way to store a 
string/array member inside a static struct would be to copy what is being done 
in the value_regs/invalidate_regs field, i.e., store the value itself in 
another static object, and then store the pointer to that object into 
RegisterInfo. For dynamic RegisterInfos (like those received from the 
lldb-server), you'd use DynamicRegisterInfo and DynamicRegisterInfo::Register, 
which have logic to persist/create storage for these values (that logic would 
have to be extended to handle the new fields as well).

That said I would *love* is someone changed the RegisterInfo structs into 
something saner, but I think that will need to be more elaborate than simply 
stuffing a std::vector member into it. I can give you my idea of how this could 
work, if you're interested in trying this out.




Comment at: lldb/include/lldb/Core/EmulateInstruction.h:196
+  struct RegisterPlusOffsetStruct {
 RegisterInfo reg;  // base register
 int64_t signed_offset; // signed offset added to base register

I actually think that the simplest solution here would be to store the 
RegisterInfos as pointers. Copying them around doesn't make sense, particularly 
if their size is about to grow.



Comment at: lldb/include/lldb/Core/EmulateInstruction.h:342-345
+  info.~ContextUnion();
+  info.info_type = eInfoTypeRegisterRegisterOperands;
+  new () RegisterInfo(op1_reg);
+  new () RegisterInfo(op2_reg);

I am not a C++ expert, but I have a strong suspicion that this is not correct. 
You're destroyed the whole object, but then are only recreating its individual 
submembers. I get that these are the only members which have non-trivial 
constructors, but I don't think that's how c++ works. I can confirm this with 
some c++ experts if you want, but I am pretty sure that you need to recreate 
the ContextUnion object as a whole here to avoid straying into UB territory.



Comment at: lldb/include/lldb/lldb-private-types.h:67-77
+  RegisterInfo() { kinds.fill(LLDB_INVALID_REGNUM); }
+
+  RegisterInfo(const char *name, const char *alt_name, uint32_t byte_size,
+   uint32_t byte_offset, lldb::Encoding encoding,
+   lldb::Format format,
+   std::array kinds,
+   uint32_t *value_regs, uint32_t *invalidate_regs)

Is this class still trivially constructible (not requiring dynamic 
initialization)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134041

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

[Lldb-commits] [PATCH] D134111: [lldb] Add newline in output of `target modules lookup`

[Pavel Labath via Phabricator via lldb-commits]

labath accepted this revision.
labath added inline comments.
This revision is now accepted and ready to land.



Comment at: lldb/test/Shell/Commands/command-target-modules-lookup.test:13
+# CHECK-NEXT: Summary: [[MODULE]]`someOtherFunc(double)
+# CHECK-NOT:  ignoreThisFunction

This might be better off as an --implicit-check-not argument to FileCheck. As 
it stands now, it will only check that ignoreThisFunction does not appear at 
the very end of the output.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134111

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

[Lldb-commits] [PATCH] D134066: [LLDB][NativePDB] Forcefully complete a record type it has incomplete type debug info.

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.



Comment at: lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp:681
+auto  = llvm::cast(*ct.GetTypeSystem());
+ts.GetMetadata()->SetIsForcefullyCompleted();
+  }

zequanwu wrote:
> rnk wrote:
> > Is this what we do for DWARF? The same kind of situation seems like it can 
> > arise, where clang requires a type to be complete, but the information is 
> > missing, and we still try to proceed with evaluation.
> I think it's here: 
> https://github.com/llvm/llvm-project/blob/main/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp#L221-L250,
>  relevant: https://reviews.llvm.org/D85968.
The trick is to do the forced completion only when the type is used within 
contexts where the c++ rules require it to be complete. something like `class 
A; A* a;` is perfectly legal c++. `class A; class B : A {};` is not. You can't 
do this from within the completion callback. In DWARF code, we check for this 
when we're parsing the enclosing entity (so, when we're parsing `B`, we'd 
check, and if needed, "forcefully complete" the class `A`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134066

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

[Lldb-commits] [PATCH] D133906: [lldb] Generate lldb-forward with .def file

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D133906#3791352 , @JDevlieghere 
wrote:

> In D133906#3791153 , @jingham wrote:
>
>> This patch makes me a little sad because it breaks the "Jump to Definition" 
>> chain in Xcode (and I bet it does in other IDE's that support this.)  You 
>> used to be able to do "Jump to Definition" on ProcessSP, then jump to 
>> definition on the class name in the shared pointer definition to jump to the 
>> class.  Now the first jump takes you to:
>>
>> LLDB_FORWARD_CLASS(Process)
>>
>> in the lldb-forward.def file, and you can't go any further because the IDE 
>> can't tell what to do with the contents of the .def file (it has no way to 
>> know how it was imported to create real definitions).  These .def insertions 
>> almost always make things harder to find in the actual code, so they aren't 
>> free.
>
> The alternative would be to have tablegen generate the header, but I think 
> that's overkill, and I'm not even sure Xcode would pick the generated header.

I have a feeling that the code would be simpler if you reversed the "iteration 
order", and it might even make the go-to definition command more useful. I'm 
thinking of something like

  // no .def file
  #define LLDB_FORWARD_CLASS(cls) \
namespace lldb_private { class cls; } \
namespace lldb {  using cls##SP = std::shared_ptr; } \
...
  
  LLDB_FORWARD_CLASS(Foo)
  LLDB_FORWARD_CLASS(Bar)
  ...

That said, my preferred solution would be something like `template 
using SP = std::shared_ptr`, and then replacing all `XyzSP` with `SP`. 
The two main benefits (besides simplifying the forward file) I see are:

- being able to write `SP`. With the current pattern, you'd have to 
introduce a whole new class of typedefs, or live with the fact that 
`shared_ptr` looks very different from XyzSP
- being compatible with the llvm naming scheme. XyzSP and xyz_sp would collide 
if they both used camel case. With this, they won't because one of them will 
not exist.


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

https://reviews.llvm.org/D133906

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

[Lldb-commits] [PATCH] D133446: [LLDB][NativePDB] Global ctor and dtor should be global decls.

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D133446#3787362 , @zequanwu wrote:

> In D133446#3786561 , @labath wrote:
>
>> In D133446#3780961 , @zequanwu 
>> wrote:
>>
>>> In D133446#3779600 , @labath 
>>> wrote:
>>>
 I believe that this fixes the crash, but the names of generated functions 
 still look fairly weird. Could we create them using their mangled name 
 instead? That way, someone might actually call them, if he was so inclined.
>>>
>>> It looks like they don't have mangled name stored in pdb.
>>
>> Hmm.. That's unfortunate. In that case, I'm wondering if this shouldn't be 
>> handled somehow directly inside `MSVCUndecoratedNameParser`. What does the 
>> function return right now? Do you think that result is going to be useful 
>> for other users of that class?
>
> `MSVCUndecoratedNameParser` expects a undercoated name of a entity(class, 
> function, variable, etc) and returns the a pair of {pointer to parent decl 
> context, base name of the entity}.

Hmm... are you sure about that? `CreateDeclInfoForUndecoratedName` returns a 
{decl ctx, base name} pair. `MSVCUndecoratedNameParser` returns (creates) an 
array of `MSVCUndecoratedNameSpecifier`s. My question was about what those 
specifiers are specifically for the inputs we're talking about here (`static 
void B::dynamic atexit destructor for 'glob'()` or such), and whether those 
outputs "make sense" for some user of that class (it's clear that they don't 
make sense here).

Basically, I'm trying to see whether it's possible (desirable?) to move this 
logic into the parser class. The parser already does a lot of string 
manipulation, so checking for these strings there would make more sense to me 
than doing some sort of a pre-parse from the outside.

>> Also, I'm still continuing to be amazed (and scared) by the amount of name 
>> parsing that is going on here. Have you checked that there's no better way 
>> to associate an entity with its enclosing context?
>
> If the entity is a global variable, we don't have any other ways to know if 
> its enclosing context is record or namespace.  If the entity is record or 
> function, then we can know if its enclosing context is a record by looking at 
> its type info but need to fall back to parse name again if it's inside a 
> namespace. The issue is there isn't a way to represent namespaces in PDB, and 
> they are just embedded into the name strings.

Hmm.. well, that's unfortunate, but I guess we'll have to live with it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133446

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

[Lldb-commits] [PATCH] D132954: lldb: Add support for R_386_32 relocations to ObjectFileELF

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Yeah, we can enable them. I've done that now in  d079bf33 
.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132954

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

[Lldb-commits] [PATCH] D132954: lldb: Add support for R_386_32 relocations to ObjectFileELF

[Pavel Labath via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGf4fc4056319d: lldb: Add support for R_386_32 relocations to 
ObjectFileELF (authored by dmlary, committed by labath).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132954

Files:
  lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
  lldb/test/Shell/ObjectFile/ELF/i386-relocations.yaml

Index: lldb/test/Shell/ObjectFile/ELF/i386-relocations.yaml
===
--- /dev/null
+++ lldb/test/Shell/ObjectFile/ELF/i386-relocations.yaml
@@ -0,0 +1,57 @@
+# RUN: yaml2obj %s -o %t
+# RUN: lldb-test object-file --contents %t | FileCheck %s
+#
+# CHECK-LABEL:  Name: .debug_info
+# CHECK:Data: (
+# CHECK-NEXT: : 
+#
+# CHECK-LABEL:  Name: .debug_lines
+# CHECK:Data: (
+# CHECK-NEXT: : 
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS32
+  Data:ELFDATA2LSB
+  Type:ET_REL
+  Machine: EM_386
+Sections:
+  - Name:.data
+Type:SHT_PROGBITS
+Flags:   [ SHF_WRITE, SHF_ALLOC ]
+AddressAlign:0x20
+  - Name:.debug_info
+Type:SHT_PROGBITS
+AddressAlign:0x1
+Content: 
+  - Name:.debug_lines
+Type:SHT_PROGBITS
+AddressAlign:0x1
+Content: 
+  - Name:.rel.debug_info
+Type:SHT_REL
+Flags:   [ SHF_INFO_LINK ]
+Link:.symtab
+AddressAlign:0x0
+Info:.debug_info
+Relocations:
+  - Offset:  0x0
+Symbol:  var
+Type:R_386_32
+  - Name:.rela.debug_lines
+Type:SHT_RELA
+Link:.symtab
+AddressAlign:0x4
+Info:.debug_lines
+Relocations:
+  - Offset:  0x0
+Addend:  0x
+Symbol:  var
+Type:R_386_32
+Symbols:
+  - Name:var
+Type:STT_OBJECT
+Section: .data
+Binding: STB_GLOBAL
+Value:   0x
+Size:0x5
+...
Index: lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
===
--- lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
+++ lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
@@ -121,6 +121,8 @@
 
   static unsigned RelocAddend64(const ELFRelocation );
 
+  bool IsRela() { return (reloc.is()); }
+
 private:
   typedef llvm::PointerUnion RelocUnion;
 
@@ -2597,25 +2599,46 @@
   }
 
   for (unsigned i = 0; i < num_relocations; ++i) {
-if (!rel.Parse(rel_data, ))
+if (!rel.Parse(rel_data, )) {
+  GetModule()->ReportError(".rel%s[%d] failed to parse relocation",
+   rel_section->GetName().AsCString(), i);
   break;
-
+}
 Symbol *symbol = nullptr;
 
 if (hdr->Is32Bit()) {
   switch (reloc_type(rel)) {
   case R_386_32:
+symbol = symtab->FindSymbolByID(reloc_symbol(rel));
+if (symbol) {
+  addr_t f_offset =
+  rel_section->GetFileOffset() + ELFRelocation::RelocOffset32(rel);
+  DataBufferSP _buffer_sp = debug_data.GetSharedDataBuffer();
+  // ObjectFileELF creates a WritableDataBuffer in CreateInstance.
+  WritableDataBuffer *data_buffer =
+  llvm::cast(data_buffer_sp.get());
+  uint32_t *dst = reinterpret_cast(
+  data_buffer->GetBytes() + f_offset);
+
+  addr_t value = symbol->GetAddressRef().GetFileAddress();
+  if (rel.IsRela()) {
+value += ELFRelocation::RelocAddend32(rel);
+  } else {
+value += *dst;
+  }
+  *dst = value;
+} else {
+  GetModule()->ReportError(".rel%s[%u] unknown symbol id: %d",
+   rel_section->GetName().AsCString(), i,
+   reloc_symbol(rel));
+}
+break;
   case R_386_PC32:
   default:
-// FIXME: This asserts with this input:
-//
-// foo.cpp
-// int main(int argc, char **argv) { return 0; }
-//
-// clang++.exe --target=i686-unknown-linux-gnu -g -c foo.cpp -o foo.o
-//
-// and running this on the foo.o module.
-assert(false && "unexpected relocation type");
+GetModule()->ReportError("unsupported 32-bit relocation:"
+ " .rel%s[%u], type %u",
+ rel_section->GetName().AsCString(), i,
+ reloc_type(rel));
   }
 } else {
   switch (reloc_type(rel)) {
___
lldb-commits mailing list

[Lldb-commits] [PATCH] D133446: [LLDB][NativePDB] Global ctor and dtor should be global decls.

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D133446#3780961 , @zequanwu wrote:

> In D133446#3779600 , @labath wrote:
>
>> I believe that this fixes the crash, but the names of generated functions 
>> still look fairly weird. Could we create them using their mangled name 
>> instead? That way, someone might actually call them, if he was so inclined.
>
> It looks like they don't have mangled name stored in pdb.

Hmm.. That's unfortunate. In that case, I'm wondering if this shouldn't be 
handled somehow directly inside `MSVCUndecoratedNameParser`. What does the 
function return right now? Do you think that result is going to be useful for 
other users of that class?

Also, I'm still continuing to be amazed (and scared) by the amount of name 
parsing that is going on here. Have you checked that there's no better way to 
associate an entity with its enclosing context?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133446

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

[Lldb-commits] [PATCH] D133763: WIP strawman building perf.cpp on an older kernel

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

(This file is only used for the processor trace feature, and one can debug just 
fine without it. So, I'd just conditionally compile out the processor trace 
support on older kernels -- it's highly questionable whether this works there 
anyway).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133763

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

[Lldb-commits] [PATCH] D133240: [Formatters][NFCI] Replace 'is_regex' arguments with an enum.

[Pavel Labath via Phabricator via lldb-commits]

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

This seems in line with what was being discussed. Jim, do you have any thoughts 
on this?




Comment at: lldb/include/lldb/lldb-enumerations.h:841
+
+  kNumFormatterMatchTypes,
+};

I see the enums in this file use wildly inconsistent styles for the "largest 
value" enumerator. However, you've picked the one I like the least, because 
this tends to produce `unhandled switch case "kNumFormatterMatchTypes"` 
warnings. If you feel like you need to have a sentinel, I'd recommend going 
with the `eLastFormatterMatchType = ` pattern (used e.g. in 
the StateType enum).


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

https://reviews.llvm.org/D133240

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

[Lldb-commits] [PATCH] D133129: [lldb] Add boilerplate for debugger interrupts

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D133129#3778125 , @jingham wrote:

> To be clear, I'm not trying to implement a preemptive interrupt using these 
> callbacks.  There are so many places where lldb is doing work that you really 
> can't interrupt - e.g. we can't drop symbol parsing and then pick it up again 
> later - that that doesn't really even make sense.
>
> I started out with the goal of extending the current 
> InterruptCommand/WasInterrupted style of voluntary interruption to more 
> places in lldb.  A voluntary "I'm doing something in a loop that can be 
> interrupted, so I'll check for interruption at the top of the loop" mechanism 
> seems like the best fit for lldb, so that structure makes sense.  But we only 
> check for interruption a few place in the "target modules" commands.  It 
> would be useful to have other places check - e.g. if you did a `bt` and there 
> were 4 frames and you don't want to wait for us to print them all out...  
> So I was going to look into adding more interrupt checks.
>
> But then it seemed a shame that setting this interrupt only works when you 
> are running command line commands, there's no reason this can't work for SB 
> API calls as well, then UI's could also allow this sort of interruption.  If 
> the UI code is doing something in a loop, then it's up to the UI code to 
> handle interrupting that operation.

I agree with all that.

> So all I'm trying to do is set the interrupt flag for use while the currently 
> executing SB API is in flight, then turning it off when that call exits.

The problem is that, even with a single SB call, it's very hard to tell the 
difference between "I am doing X" vs. "I am about to do X" vs. "I have just 
completed X (but haven't told anyone about it)". And I don't see a way to do 
that reliably through any kind of automatic API boundary tracking.

> The debugger is the one setting the interrupt flag, so we always know who is 
> sending the interrupt.  The tricky bit is how to turn off the 
> "WasInterrupted" flag after the API that was in flight when the flag was set 
> finishes.

Maybe the solution is to not (automatically) turn off the flag -- but put it in 
the hands of the user instead? If the interrupt flag was sticky (with explicit 
SetInterrupt/ClearInterrupt actions), then one can handle all of the scenarios 
above fairly easily.
It doesn't matter that whether SetInterrupt is called before the blocking call 
or not -- the call is going to be interrupted anyway. And one can explicitly 
clear the interrupt flag after the blocking call returns (either successfully 
or because it was interrupted).


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

https://reviews.llvm.org/D133129

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

[Lldb-commits] [PATCH] D133427: [gdb-remote] Move broadcasting logic down to GDBRemoteClientBase

[Pavel Labath via Phabricator via lldb-commits]

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

cool.




Comment at: lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationTest.cpp:20
 public:
-  TestClient()
-  : GDBRemoteCommunication("test.client", "test.client.listener") {}
+  TestClient() : GDBRemoteCommunication() {}
 

I think you can just delete this altogether.


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

https://reviews.llvm.org/D133427

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

[Lldb-commits] [PATCH] D132954: lldb: Add support for R_386_32 relocations to ObjectFileELF

[Pavel Labath via Phabricator via lldb-commits]

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

Cool. Thanks for figuring this out.


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

https://reviews.llvm.org/D132954

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

[Lldb-commits] [PATCH] D133352: [lldb-server] Report launch error in vRun packets

[Pavel Labath via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG681d0d9e5f05: [lldb-server] Report launch error in vRun 
packets (authored by labath).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133352

Files:
  lldb/source/Host/posix/ProcessLauncherPosixFork.cpp
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
  lldb/test/API/tools/lldb-server/TestGdbRemoteLaunch.py
  lldb/test/API/tools/lldb-server/TestLldbGdbServer.py

Index: lldb/test/API/tools/lldb-server/TestLldbGdbServer.py
===
--- lldb/test/API/tools/lldb-server/TestLldbGdbServer.py
+++ lldb/test/API/tools/lldb-server/TestLldbGdbServer.py
@@ -1205,128 +1205,6 @@
 self.assertEqual(read_value, expected_reg_values[thread_index])
 thread_index += 1
 
-@skipIfWindows # No pty support to test any inferior output
-@add_test_categories(["llgs"])
-def test_launch_via_A(self):
-self.build()
-exe_path = self.getBuildArtifact("a.out")
-args = [exe_path, "stderr:arg1", "stderr:arg2", "stderr:arg3"]
-hex_args = [binascii.b2a_hex(x.encode()).decode() for x in args]
-
-server = self.connect_to_debug_monitor()
-self.assertIsNotNone(server)
-self.do_handshake()
-# NB: strictly speaking we should use %x here but this packet
-# is deprecated, so no point in changing lldb-server's expectations
-self.test_sequence.add_log_lines(
-["read packet: $A %d,0,%s,%d,1,%s,%d,2,%s,%d,3,%s#00" %
- tuple(itertools.chain.from_iterable(
- [(len(x), x) for x in hex_args])),
- "send packet: $OK#00",
- "read packet: $c#00",
- "send packet: $W00#00"],
-True)
-context = self.expect_gdbremote_sequence()
-self.assertEqual(context["O_content"],
- b'arg1\r\narg2\r\narg3\r\n')
-
-@skipIfWindows # No pty support to test any inferior output
-@add_test_categories(["llgs"])
-def test_launch_via_vRun(self):
-self.build()
-exe_path = self.getBuildArtifact("a.out")
-args = [exe_path, "stderr:arg1", "stderr:arg2", "stderr:arg3"]
-hex_args = [binascii.b2a_hex(x.encode()).decode() for x in args]
-
-server = self.connect_to_debug_monitor()
-self.assertIsNotNone(server)
-self.do_handshake()
-self.test_sequence.add_log_lines(
-["read packet: $vRun;%s;%s;%s;%s#00" % tuple(hex_args),
- {"direction": "send",
-  "regex": r"^\$T([0-9a-fA-F]+)"},
- "read packet: $c#00",
- "send packet: $W00#00"],
-True)
-context = self.expect_gdbremote_sequence()
-self.assertEqual(context["O_content"],
- b'arg1\r\narg2\r\narg3\r\n')
-
-@add_test_categories(["llgs"])
-def test_launch_via_vRun_no_args(self):
-self.build()
-exe_path = self.getBuildArtifact("a.out")
-hex_path = binascii.b2a_hex(exe_path.encode()).decode()
-
-server = self.connect_to_debug_monitor()
-self.assertIsNotNone(server)
-self.do_handshake()
-self.test_sequence.add_log_lines(
-["read packet: $vRun;%s#00" % (hex_path,),
- {"direction": "send",
-  "regex": r"^\$T([0-9a-fA-F]+)"},
- "read packet: $c#00",
- "send packet: $W00#00"],
-True)
-self.expect_gdbremote_sequence()
-
-@skipIfWindows # No pty support to test any inferior output
-@add_test_categories(["llgs"])
-def test_QEnvironment(self):
-self.build()
-exe_path = self.getBuildArtifact("a.out")
-env = {"FOO": "test", "BAR": "a=z"}
-args = [exe_path, "print-env:FOO", "print-env:BAR"]
-hex_args = [binascii.b2a_hex(x.encode()).decode() for x in args]
-
-server = self.connect_to_debug_monitor()
-self.assertIsNotNone(server)
-self.do_handshake()
-
-for key, value in env.items():
-self.test_sequence.add_log_lines(
-["read packet: $QEnvironment:%s=%s#00" % (key, value),
- "send packet: $OK#00"],
-True)
-self.test_sequence.add_log_lines(
-["read packet: $vRun;%s#00" % (";".join(hex_args),),
- {"direction": "send",
-  "regex": r"^\$T([0-9a-fA-F]+)"},
- "read packet: $c#00",
- "send packet: $W00#00"],
-True)
-context = self.expect_gdbremote_sequence()
-self.assertEqual(context["O_content"], b"test\r\na=z\r\n")
-
-@skipIfWindows # No pty support to test any inferior output
-@add_test_categories(["llgs"])
-def test_QEnvironmentHexEncoded(self):
-self.build()
-exe_path = 

[Lldb-commits] [PATCH] D133410: [lldb] Fix ThreadedCommunication races

[Pavel Labath via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG89a3691b794c: [lldb] Fix ThreadedCommunication races 
(authored by labath).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133410

Files:
  lldb/source/Core/ThreadedCommunication.cpp

Index: lldb/source/Core/ThreadedCommunication.cpp
===
--- lldb/source/Core/ThreadedCommunication.cpp
+++ lldb/source/Core/ThreadedCommunication.cpp
@@ -104,34 +104,50 @@
   return 0;
 }
 
+// No data yet, we have to start listening.
 ListenerSP listener_sp(
 Listener::MakeListener("ThreadedCommunication::Read"));
 listener_sp->StartListeningForEvents(
 this, eBroadcastBitReadThreadGotBytes | eBroadcastBitReadThreadDidExit);
-EventSP event_sp;
-while (listener_sp->GetEvent(event_sp, timeout)) {
-  const uint32_t event_type = event_sp->GetType();
-  if (event_type & eBroadcastBitReadThreadGotBytes) {
-return GetCachedBytes(dst, dst_len);
-  }
 
-  if (event_type & eBroadcastBitReadThreadDidExit) {
-// If the thread exited of its own accord, it either means it
-// hit an end-of-file condition or an error.
-status = m_pass_status;
-if (error_ptr)
-  *error_ptr = std::move(m_pass_error);
+// Re-check for data, as it might have arrived while we were setting up our
+// listener.
+cached_bytes = GetCachedBytes(dst, dst_len);
+if (cached_bytes > 0) {
+  status = eConnectionStatusSuccess;
+  return cached_bytes;
+}
 
-if (GetCloseOnEOF())
-  Disconnect(nullptr);
+EventSP event_sp;
+// Explicitly check for the thread exit, for the same reason.
+if (m_read_thread_did_exit) {
+  // We've missed the event, lets just conjure one up.
+  event_sp = std::make_shared(eBroadcastBitReadThreadDidExit);
+} else {
+  if (!listener_sp->GetEvent(event_sp, timeout)) {
+if (error_ptr)
+  error_ptr->SetErrorString("Timed out.");
+status = eConnectionStatusTimedOut;
 return 0;
   }
 }
+const uint32_t event_type = event_sp->GetType();
+if (event_type & eBroadcastBitReadThreadGotBytes) {
+  return GetCachedBytes(dst, dst_len);
+}
 
-if (error_ptr)
-  error_ptr->SetErrorString("Timed out.");
-status = eConnectionStatusTimedOut;
-return 0;
+if (event_type & eBroadcastBitReadThreadDidExit) {
+  // If the thread exited of its own accord, it either means it
+  // hit an end-of-file condition or an error.
+  status = m_pass_status;
+  if (error_ptr)
+*error_ptr = std::move(m_pass_error);
+
+  if (GetCloseOnEOF())
+Disconnect(nullptr);
+  return 0;
+}
+llvm_unreachable("Got unexpected event type!");
   }
 
   // We aren't using a read thread, just read the data synchronously in this
@@ -299,22 +315,25 @@
   m_pass_error = std::move(error);
   LLDB_LOG(log, "Communication({0}) thread exiting...", this);
 
-  // Handle threads wishing to synchronize with us.
-  {
-// Prevent new ones from showing up.
-m_read_thread_did_exit = true;
+  // Start shutting down. We need to do this in a very specific order to ensure
+  // we don't race with threads wanting to read/synchronize with us.
 
-// Unblock any existing thread waiting for the synchronization signal.
-BroadcastEvent(eBroadcastBitNoMorePendingInput);
+  // First, we signal our intent to exit. This ensures no new thread start
+  // waiting on events from us.
+  m_read_thread_did_exit = true;
 
-// Wait for the thread to finish...
+  // Unblock any existing thread waiting for the synchronization signal.
+  BroadcastEvent(eBroadcastBitNoMorePendingInput);
+
+  {
+// Wait for the synchronization thread to finish...
 std::lock_guard guard(m_synchronize_mutex);
 // ... and disconnect.
 if (disconnect)
   Disconnect();
   }
 
-  // Let clients know that this thread is exiting
+  // Finally, unblock any readers waiting for us to exit.
   BroadcastEvent(eBroadcastBitReadThreadDidExit);
   return {};
 }
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D133164: Add the ability to show when variables fails to be available when debug info is valid.

[Pavel Labath via Phabricator via lldb-commits]

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

Looks good. Thanks.




Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:4161
+  if (command) {
+if (command->contains(" -gline-tables-only"))
+  return Status("-gline-tables-only enabled, no variable information is "

clayborg wrote:
> labath wrote:
> > clayborg wrote:
> > > labath wrote:
> > > > This isn't a particularly reliable way of detecting whether variable 
> > > > information was emitted. For example a command line `clang 
> > > > -gline-tables-only -g2` will in fact produce full debug info and `clang 
> > > > -g1` will not. Could we make that determination based on the presence 
> > > > of actual variable DIEs in the debug info? Perhaps query the index 
> > > > whether it knows of any (global) variable or any type defined within 
> > > > the compile unit?
> > > This function only gets called when there are no variables in a stack 
> > > frame at all and checks for reasons why. So if "-gline-tables-only -g2" 
> > > is used, this function won't get called if there were variables.
> > > 
> > > I planned to make a follow up patch that detected no variables in a 
> > > compile uint by checking the compile unit's abbreviation set to see if it 
> > > had any DW_TAG_variable or DW_TAG_formal_parameter definitions, and if 
> > > there weren't any respond with "-gline-tables-only might be enabled". 
> > > 
> > > If we see this option for sure and have the side affect of there being no 
> > > variables, I would like to user the users know what they can do to fix 
> > > things if at all possible. 
> > I get that, but this check is not completely correct in either direction. 
> > For example, `clang -g1` will not produce variable information, but this 
> > code will not detect it. Same goes for `clang -gmlt`. And I probably missed 
> > some other ways one can prevent variable info from being generated. Keeping 
> > up with all the changes in clang flags will just be a game of whack-a-mole. 
> > If we checked the actual debug info, then we would catch all of these 
> > cases, including the (default) case when the compiler did not embed command 
> > line information into the debug info.
> > 
> > And I don't think we need to know the precise command line to give 
> > meaningful advice to the users. In all of these cases, sticking an extra 
> > `-g` at the end of the command line will cause the variables to reappear. 
> > If we wanted to, we could also put a link to the lldb web page where we can 
> > (at length) describe the various reasons why variables may not be available 
> > and how to fix them.
> I switched over to just looking for any variable DIEs anywhere in the CU. 
> This should cover all options. Let me know if you think we should add a top 
> level web page and reference it in the error message?
I think it might be nice to have some kind of a "I can't debug" troubleshooting 
page, but I don't think we need to tie it to the future of this patch. 



Comment at: lldb/test/API/commands/frame/var/TestFrameVar.py:173
+'''
+self.build(dictionary={'CFLAGS_EXTRAS': '-gline-tables-only 
-grecord-command-line'})
+exe = self.getBuildArtifact("a.out")

I guess this isn't necessary any more.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133164

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

[Lldb-commits] [PATCH] D133461: [LLDB][NativePDB] Set block address range.

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.



Comment at: lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp:372
+func->GetAddressRange().GetBaseAddress().GetFileAddress();
+Block::Range range = Block::Range(block_base - func_base, block.CodeSize);
+if (block_base >= func_base)

better move this inside the if block, so one does not have to think about what 
will happen when the address wraps around.



Comment at: lldb/test/Shell/SymbolFile/NativePDB/blocks.cpp:17
+
+// CHECK:  Function: id = {{.*}}, name = "main", range = 
[0x000140001000-0x00014000104b)
+// CHECK-NEXT: FuncType: id = {{.*}}, byte-size = 0, compiler_type = "int 
(void)"

I fear this test is going to be extremely fragile (susceptible to changes in 
codegen AND debug info generation). I'd probably write it in asm (you could 
even test the error msg then).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133461

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

[Lldb-commits] [PATCH] D133534: Complete support of loading a darwin kernel over a live gdb-remote connection given the address of a mach-o fileset

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.



Comment at: lldb/unittests/Interpreter/CMakeLists.txt:17-18
   lldbInterpreter
+  lldbPluginDynamicLoaderDarwinKernel
+  lldbPluginObjectContainerMachOFileset
   lldbPluginPlatformMacOSX

These dependencies should be added to the PlatformDarwinKernel CMakeLists.txt, 
not to the every file that depends on it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133534

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

[Lldb-commits] [PATCH] D133519: Document some of the clang-specific DWARF extensions supported by LLDB

[Pavel Labath via Phabricator via lldb-commits]

labath accepted this revision.
labath added a comment.

cool




Comment at: lldb/docs/use/extensions.rst:84
+   DW_TAG_compile_unit
+ DW_AT_GNU_dwo_id  (0xabcdef)
+ DW_AT_GNU_dwo_name("M.pcm")

Michael137 wrote:
> Is it worth commenting on the difference to the `dwo` mechanism on Linux. 
> Since we aren't actually dealing with `dwo` files despite what the name 
> suggests
+1


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

https://reviews.llvm.org/D133519

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

[Lldb-commits] [PATCH] D133530: [lldb] Add zstd support

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Seems fine to me, though you may want to have a llvm test for the new 
functionality, given that the patch is exclusively changing llvm code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133530

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

[Lldb-commits] [PATCH] D133393: [test] Use either `127.0.0.1` or `[::1]` to run in ipv6-only environments.

[Pavel Labath via Phabricator via lldb-commits]

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

In D133393#3775995 , @rupprecht wrote:

> In D133393#3773793 , @labath wrote:
>
>> I believe the reasons are still relevant. Basically the problem is that 
>> listening on `localhost:x` creates two sockets (one for 127.0.0.1, one for 
>> ::1), and there's no way to guarantee that we'll be able to grab the same 
>> port for both (one could be taken by some other application). Our listening 
>> code will succeed if it opens at least one socket, but then if we again try 
>> to connect using the `localhost` name, we may end up connecting to the wrong 
>> thing. I think the correct fix is to take the address (ip+port) that we've 
>> *actually* started listening on, and then pass *that* as the argument to the 
>> connect command, but I'm not sure if our current Socket APIs allow you to 
>> get that information.
>
> There's `listen_socket.GetLocalIPAddress()`, but that returns an empty string 
> here.

Yeah, that probably only works on connected sockets.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133393

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

[Lldb-commits] [PATCH] D133446: [LLDB][NativePDB] Global ctor and dtor should be global decls.

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I believe that this fixes the crash, but the names of generated functions still 
look fairly weird. Could we create them using their mangled name instead? That 
way, someone might actually call them, if he was so inclined.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133446

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

[Lldb-commits] [PATCH] D133164: Add the ability to show when variables fails to be available when debug info is valid.

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.



Comment at: lldb/include/lldb/Target/StackFrame.h:264
   /// A pointer to a list of variables.
-  VariableList *GetVariableList(bool get_file_globals);
+  VariableList *GetVariableList(bool get_file_globals, Status *error_ptr);
 

clayborg wrote:
> There are many places that call this function that also don't need to check 
> the error and if we use a Expected, we need to add a bunch of 
> consumeError(...) code. See all of the call sites where I added a "nullptr" 
> for the "error_ptr" to see why I chose to do it this way to keep the code 
> cleaner. Many places are getting the variable list to look for things or use 
> them for autocomplete.
Ok, I am convinced by the optionalness of the error in this case.



Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:4161
+  if (command) {
+if (command->contains(" -gline-tables-only"))
+  return Status("-gline-tables-only enabled, no variable information is "

clayborg wrote:
> labath wrote:
> > This isn't a particularly reliable way of detecting whether variable 
> > information was emitted. For example a command line `clang 
> > -gline-tables-only -g2` will in fact produce full debug info and `clang 
> > -g1` will not. Could we make that determination based on the presence of 
> > actual variable DIEs in the debug info? Perhaps query the index whether it 
> > knows of any (global) variable or any type defined within the compile unit?
> This function only gets called when there are no variables in a stack frame 
> at all and checks for reasons why. So if "-gline-tables-only -g2" is used, 
> this function won't get called if there were variables.
> 
> I planned to make a follow up patch that detected no variables in a compile 
> uint by checking the compile unit's abbreviation set to see if it had any 
> DW_TAG_variable or DW_TAG_formal_parameter definitions, and if there weren't 
> any respond with "-gline-tables-only might be enabled". 
> 
> If we see this option for sure and have the side affect of there being no 
> variables, I would like to user the users know what they can do to fix things 
> if at all possible. 
I get that, but this check is not completely correct in either direction. For 
example, `clang -g1` will not produce variable information, but this code will 
not detect it. Same goes for `clang -gmlt`. And I probably missed some other 
ways one can prevent variable info from being generated. Keeping up with all 
the changes in clang flags will just be a game of whack-a-mole. If we checked 
the actual debug info, then we would catch all of these cases, including the 
(default) case when the compiler did not embed command line information into 
the debug info.

And I don't think we need to know the precise command line to give meaningful 
advice to the users. In all of these cases, sticking an extra `-g` at the end 
of the command line will cause the variables to reappear. If we wanted to, we 
could also put a link to the lldb web page where we can (at length) describe 
the various reasons why variables may not be available and how to fix them.



Comment at: lldb/test/API/commands/frame/var/TestFrameVar.py:101
+for s in error_strings:
+self.assertTrue(s in command_error, 'Make sure "%s" exists in the 
command error "%s"' % (s, command_error))
+for s in error_strings:

just change to `assertIn(s, command_error)`, and then you get the error message 
for free.



Comment at: lldb/test/API/commands/frame/var/TestFrameVar.py:174-175
+'''
+self.build(dictionary={'CFLAGS_EXTRAS': '-gline-tables-only'},
+   env={"RC_DEBUG_OPTIONS": "1"})
+exe = self.getBuildArtifact("a.out")

clayborg wrote:
> labath wrote:
> > Why not just pass `-grecord-command-line` in CFLAGS_EXTRAS? I think then 
> > you should be able to remove @skipUnlessDarwin from this test...
> I thought I tried that, but I was able to get this to work as suggested. I 
> will change this.
Cool. Can we also remove @skipUnlessDarwin then?



Comment at: lldb/test/API/functionalities/archives/Makefile:12
$(AR) $(ARFLAGS) $@ $^
-   $(RM) $^
+   # $(RM) $^
 

I don't know how important this is, but I believe the build was deleting the .o 
files to ensure that we access the copies within the archive. If you think 
that's fine, then just delete this line.



Comment at: lldb/test/API/functionalities/archives/Makefile:19
$(LLVM_AR) $(LLVM_ARFLAGS) $@ $^
# Note for thin archive case, we cannot remove c.o
 

And probably this comment as well, because it doesn't make sense if we don't 
delete the other files.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133164

[Lldb-commits] [PATCH] D133410: [lldb] Fix ThreadedCommunication races

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.



Comment at: lldb/source/Core/ThreadedCommunication.cpp:113
 
-  if (event_type & eBroadcastBitReadThreadDidExit) {
-// If the thread exited of its own accord, it either means it
-// hit an end-of-file condition or an error.
-status = m_pass_status;
-if (error_ptr)
-  *error_ptr = std::move(m_pass_error);
+// Re-check for data, as it might have arrived while we were setting up our
+// listener.

mgorny wrote:
> Can you think of any reason not to move listener setup before the first 
> `GetCachedBytes()` call instead of duplicating it?
The only possible reason is "efficiency" (avoiding the creation of the listener 
and all that goes with it). But I'm definitely not convinced that this actually 
matters.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133410

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

[Lldb-commits] [PATCH] D133352: [lldb-server] Report launch error in vRun packets

[Pavel Labath via Phabricator via lldb-commits]

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

Use `pos` instead of the previous size in the resize expression.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133352

Files:
  lldb/source/Host/posix/ProcessLauncherPosixFork.cpp
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
  lldb/test/API/tools/lldb-server/TestGdbRemoteLaunch.py
  lldb/test/API/tools/lldb-server/TestLldbGdbServer.py

Index: lldb/test/API/tools/lldb-server/TestLldbGdbServer.py
===
--- lldb/test/API/tools/lldb-server/TestLldbGdbServer.py
+++ lldb/test/API/tools/lldb-server/TestLldbGdbServer.py
@@ -1205,128 +1205,6 @@
 self.assertEqual(read_value, expected_reg_values[thread_index])
 thread_index += 1
 
-@skipIfWindows # No pty support to test any inferior output
-@add_test_categories(["llgs"])
-def test_launch_via_A(self):
-self.build()
-exe_path = self.getBuildArtifact("a.out")
-args = [exe_path, "stderr:arg1", "stderr:arg2", "stderr:arg3"]
-hex_args = [binascii.b2a_hex(x.encode()).decode() for x in args]
-
-server = self.connect_to_debug_monitor()
-self.assertIsNotNone(server)
-self.do_handshake()
-# NB: strictly speaking we should use %x here but this packet
-# is deprecated, so no point in changing lldb-server's expectations
-self.test_sequence.add_log_lines(
-["read packet: $A %d,0,%s,%d,1,%s,%d,2,%s,%d,3,%s#00" %
- tuple(itertools.chain.from_iterable(
- [(len(x), x) for x in hex_args])),
- "send packet: $OK#00",
- "read packet: $c#00",
- "send packet: $W00#00"],
-True)
-context = self.expect_gdbremote_sequence()
-self.assertEqual(context["O_content"],
- b'arg1\r\narg2\r\narg3\r\n')
-
-@skipIfWindows # No pty support to test any inferior output
-@add_test_categories(["llgs"])
-def test_launch_via_vRun(self):
-self.build()
-exe_path = self.getBuildArtifact("a.out")
-args = [exe_path, "stderr:arg1", "stderr:arg2", "stderr:arg3"]
-hex_args = [binascii.b2a_hex(x.encode()).decode() for x in args]
-
-server = self.connect_to_debug_monitor()
-self.assertIsNotNone(server)
-self.do_handshake()
-self.test_sequence.add_log_lines(
-["read packet: $vRun;%s;%s;%s;%s#00" % tuple(hex_args),
- {"direction": "send",
-  "regex": r"^\$T([0-9a-fA-F]+)"},
- "read packet: $c#00",
- "send packet: $W00#00"],
-True)
-context = self.expect_gdbremote_sequence()
-self.assertEqual(context["O_content"],
- b'arg1\r\narg2\r\narg3\r\n')
-
-@add_test_categories(["llgs"])
-def test_launch_via_vRun_no_args(self):
-self.build()
-exe_path = self.getBuildArtifact("a.out")
-hex_path = binascii.b2a_hex(exe_path.encode()).decode()
-
-server = self.connect_to_debug_monitor()
-self.assertIsNotNone(server)
-self.do_handshake()
-self.test_sequence.add_log_lines(
-["read packet: $vRun;%s#00" % (hex_path,),
- {"direction": "send",
-  "regex": r"^\$T([0-9a-fA-F]+)"},
- "read packet: $c#00",
- "send packet: $W00#00"],
-True)
-self.expect_gdbremote_sequence()
-
-@skipIfWindows # No pty support to test any inferior output
-@add_test_categories(["llgs"])
-def test_QEnvironment(self):
-self.build()
-exe_path = self.getBuildArtifact("a.out")
-env = {"FOO": "test", "BAR": "a=z"}
-args = [exe_path, "print-env:FOO", "print-env:BAR"]
-hex_args = [binascii.b2a_hex(x.encode()).decode() for x in args]
-
-server = self.connect_to_debug_monitor()
-self.assertIsNotNone(server)
-self.do_handshake()
-
-for key, value in env.items():
-self.test_sequence.add_log_lines(
-["read packet: $QEnvironment:%s=%s#00" % (key, value),
- "send packet: $OK#00"],
-True)
-self.test_sequence.add_log_lines(
-["read packet: $vRun;%s#00" % (";".join(hex_args),),
- {"direction": "send",
-  "regex": r"^\$T([0-9a-fA-F]+)"},
- "read packet: $c#00",
- "send packet: $W00#00"],
-True)
-context = self.expect_gdbremote_sequence()
-self.assertEqual(context["O_content"], b"test\r\na=z\r\n")
-
-@skipIfWindows # No pty support to test any inferior output
-@add_test_categories(["llgs"])
-def test_QEnvironmentHexEncoded(self):
-self.build()
-exe_path = self.getBuildArtifact("a.out")
-env = {"FOO": 

[Lldb-commits] [PATCH] D133352: [lldb-server] Report launch error in vRun packets

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.



Comment at: lldb/source/Host/posix/ProcessLauncherPosixFork.cpp:293
+r = llvm::sys::RetryAfterSignal(-1, read, pipe.GetReadFileDescriptor(), 
pos,
+buf.end() - pos);
+  } while (r > 0);

rupprecht wrote:
> IIUC, this will overrun the buffer if there are >1000 bytes to read; whereas 
> previously we just wouldn't have read everything.
> 
> Should each loop iteration grow the buffer by a certain amount? Otherwise I 
> think we need to remove the loop.
It won't overrun because I am passing the remaining part of the buffer as the 
size argument. However, I am not totally sure what would happen if we actually 
filled the buffer (and size becomes zero).  That won't happen right now because 
the error string is coming from `ExitWithError` (line 50) and its length is 
limited by the longest errno message. That said, the dynamical resize is quite 
simple in this case, so I might as well do it.



Comment at: lldb/test/API/tools/lldb-server/TestGdbRemoteLaunch.py:18
+args = [exe_path, "stderr:arg1", "stderr:arg2", "stderr:arg3"]
+hex_args = [seven.hexlify(x) for x in args]
+

mgorny wrote:
> labath wrote:
> > mgorny wrote:
> > > Since we no longer support Python 2, I'd rather prefer to work towards 
> > > removing `seven` rather than making more use of it.
> > Is the problem with the name/location of the function or the functionality 
> > (string/byte conversion) itself?
> > Because, if it's the first, then that could easily be solved by renaming 
> > the module (now or later), but in order to avoid elaborate casts we'd have 
> > to make all code be byte/string correct. This is not a problem here 
> > (because of the fixed strings), but it becomes one once you start working 
> > with things that aren't necessarily valid utf8 strings.
> Ok, I suppose this makes sense given how LLDB's Python API is screwed up :-(.
Just to clarify, I would like if we used the byte/string separation more (and I 
started that when I refactored the gdb server test harness), but it's fairly 
tricky because we're often working with things (e.g. memory contents, or 
stdout) that can represent arbitrary data in general, but which usually (for 
our own convenience and sanity) contain simple ASCII strings. So we have a 
problem where the API (and here I just mean the gdbserver test API, which we 
can change) really wants us to use bytes, but the the most natural way to write 
the test is with strings. I haven't come up with a solution to that yet.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133352

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

[Lldb-commits] [PATCH] D133352: [lldb-server] Report launch error in vRun packets

[Pavel Labath via Phabricator via lldb-commits]

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

dynamically resize error buffer


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133352

Files:
  lldb/source/Host/posix/ProcessLauncherPosixFork.cpp
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
  lldb/test/API/tools/lldb-server/TestGdbRemoteLaunch.py
  lldb/test/API/tools/lldb-server/TestLldbGdbServer.py

Index: lldb/test/API/tools/lldb-server/TestLldbGdbServer.py
===
--- lldb/test/API/tools/lldb-server/TestLldbGdbServer.py
+++ lldb/test/API/tools/lldb-server/TestLldbGdbServer.py
@@ -1205,128 +1205,6 @@
 self.assertEqual(read_value, expected_reg_values[thread_index])
 thread_index += 1
 
-@skipIfWindows # No pty support to test any inferior output
-@add_test_categories(["llgs"])
-def test_launch_via_A(self):
-self.build()
-exe_path = self.getBuildArtifact("a.out")
-args = [exe_path, "stderr:arg1", "stderr:arg2", "stderr:arg3"]
-hex_args = [binascii.b2a_hex(x.encode()).decode() for x in args]
-
-server = self.connect_to_debug_monitor()
-self.assertIsNotNone(server)
-self.do_handshake()
-# NB: strictly speaking we should use %x here but this packet
-# is deprecated, so no point in changing lldb-server's expectations
-self.test_sequence.add_log_lines(
-["read packet: $A %d,0,%s,%d,1,%s,%d,2,%s,%d,3,%s#00" %
- tuple(itertools.chain.from_iterable(
- [(len(x), x) for x in hex_args])),
- "send packet: $OK#00",
- "read packet: $c#00",
- "send packet: $W00#00"],
-True)
-context = self.expect_gdbremote_sequence()
-self.assertEqual(context["O_content"],
- b'arg1\r\narg2\r\narg3\r\n')
-
-@skipIfWindows # No pty support to test any inferior output
-@add_test_categories(["llgs"])
-def test_launch_via_vRun(self):
-self.build()
-exe_path = self.getBuildArtifact("a.out")
-args = [exe_path, "stderr:arg1", "stderr:arg2", "stderr:arg3"]
-hex_args = [binascii.b2a_hex(x.encode()).decode() for x in args]
-
-server = self.connect_to_debug_monitor()
-self.assertIsNotNone(server)
-self.do_handshake()
-self.test_sequence.add_log_lines(
-["read packet: $vRun;%s;%s;%s;%s#00" % tuple(hex_args),
- {"direction": "send",
-  "regex": r"^\$T([0-9a-fA-F]+)"},
- "read packet: $c#00",
- "send packet: $W00#00"],
-True)
-context = self.expect_gdbremote_sequence()
-self.assertEqual(context["O_content"],
- b'arg1\r\narg2\r\narg3\r\n')
-
-@add_test_categories(["llgs"])
-def test_launch_via_vRun_no_args(self):
-self.build()
-exe_path = self.getBuildArtifact("a.out")
-hex_path = binascii.b2a_hex(exe_path.encode()).decode()
-
-server = self.connect_to_debug_monitor()
-self.assertIsNotNone(server)
-self.do_handshake()
-self.test_sequence.add_log_lines(
-["read packet: $vRun;%s#00" % (hex_path,),
- {"direction": "send",
-  "regex": r"^\$T([0-9a-fA-F]+)"},
- "read packet: $c#00",
- "send packet: $W00#00"],
-True)
-self.expect_gdbremote_sequence()
-
-@skipIfWindows # No pty support to test any inferior output
-@add_test_categories(["llgs"])
-def test_QEnvironment(self):
-self.build()
-exe_path = self.getBuildArtifact("a.out")
-env = {"FOO": "test", "BAR": "a=z"}
-args = [exe_path, "print-env:FOO", "print-env:BAR"]
-hex_args = [binascii.b2a_hex(x.encode()).decode() for x in args]
-
-server = self.connect_to_debug_monitor()
-self.assertIsNotNone(server)
-self.do_handshake()
-
-for key, value in env.items():
-self.test_sequence.add_log_lines(
-["read packet: $QEnvironment:%s=%s#00" % (key, value),
- "send packet: $OK#00"],
-True)
-self.test_sequence.add_log_lines(
-["read packet: $vRun;%s#00" % (";".join(hex_args),),
- {"direction": "send",
-  "regex": r"^\$T([0-9a-fA-F]+)"},
- "read packet: $c#00",
- "send packet: $W00#00"],
-True)
-context = self.expect_gdbremote_sequence()
-self.assertEqual(context["O_content"], b"test\r\na=z\r\n")
-
-@skipIfWindows # No pty support to test any inferior output
-@add_test_categories(["llgs"])
-def test_QEnvironmentHexEncoded(self):
-self.build()
-exe_path = self.getBuildArtifact("a.out")
-env = {"FOO": "test", "BAR": "a=z", "BAZ": 

[Lldb-commits] [PATCH] D133129: [lldb] Add boilerplate for debugger interrupts

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I've added you to D133410 , as I think it 
demonstrates very well the difficulties in getting two threads to talk to one 
another. This code has been here since forever (and may have contributed to 
your desire to introduce interrupts), but we've only found the problem now, 
when Michał added some unit tests for this class.


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

https://reviews.llvm.org/D133129

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

[Lldb-commits] [PATCH] D133410: [lldb] Fix ThreadedCommunication races

[Pavel Labath via Phabricator via lldb-commits]

labath created this revision.
labath added reviewers: mgorny, JDevlieghere.
Herald added a project: All.
labath requested review of this revision.
Herald added a project: LLDB.

The Read function could end up blocking if data (or EOF) arrived just as
it was about to start waiting for the events. This was only discovered
now, because we did not have unit tests for this functionality before.
We need to check for data *after* we start listening for incoming
events. There were no changes to the read thread code needed, as we
already use this pattern in SynchronizeWithReadThread, so I just updated
the comments to make it clear that it is used for reading as well.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D133410

Files:
  lldb/source/Core/ThreadedCommunication.cpp

Index: lldb/source/Core/ThreadedCommunication.cpp
===
--- lldb/source/Core/ThreadedCommunication.cpp
+++ lldb/source/Core/ThreadedCommunication.cpp
@@ -104,34 +104,50 @@
   return 0;
 }
 
+// No data yet, we have to start listening.
 ListenerSP listener_sp(
 Listener::MakeListener("ThreadedCommunication::Read"));
 listener_sp->StartListeningForEvents(
 this, eBroadcastBitReadThreadGotBytes | eBroadcastBitReadThreadDidExit);
-EventSP event_sp;
-while (listener_sp->GetEvent(event_sp, timeout)) {
-  const uint32_t event_type = event_sp->GetType();
-  if (event_type & eBroadcastBitReadThreadGotBytes) {
-return GetCachedBytes(dst, dst_len);
-  }
 
-  if (event_type & eBroadcastBitReadThreadDidExit) {
-// If the thread exited of its own accord, it either means it
-// hit an end-of-file condition or an error.
-status = m_pass_status;
-if (error_ptr)
-  *error_ptr = std::move(m_pass_error);
+// Re-check for data, as it might have arrived while we were setting up our
+// listener.
+cached_bytes = GetCachedBytes(dst, dst_len);
+if (cached_bytes > 0) {
+  status = eConnectionStatusSuccess;
+  return cached_bytes;
+}
 
-if (GetCloseOnEOF())
-  Disconnect(nullptr);
+EventSP event_sp;
+// Explicitly check for the thread exit, for the same reason.
+if (m_read_thread_did_exit) {
+  // We've missed the event, lets just conjure one up.
+  event_sp = std::make_shared(eBroadcastBitReadThreadDidExit);
+} else {
+  if (!listener_sp->GetEvent(event_sp, timeout)) {
+if (error_ptr)
+  error_ptr->SetErrorString("Timed out.");
+status = eConnectionStatusTimedOut;
 return 0;
   }
 }
+const uint32_t event_type = event_sp->GetType();
+if (event_type & eBroadcastBitReadThreadGotBytes) {
+  return GetCachedBytes(dst, dst_len);
+}
 
-if (error_ptr)
-  error_ptr->SetErrorString("Timed out.");
-status = eConnectionStatusTimedOut;
-return 0;
+if (event_type & eBroadcastBitReadThreadDidExit) {
+  // If the thread exited of its own accord, it either means it
+  // hit an end-of-file condition or an error.
+  status = m_pass_status;
+  if (error_ptr)
+*error_ptr = std::move(m_pass_error);
+
+  if (GetCloseOnEOF())
+Disconnect(nullptr);
+  return 0;
+}
+llvm_unreachable("Got unexpected event type!");
   }
 
   // We aren't using a read thread, just read the data synchronously in this
@@ -299,22 +315,25 @@
   m_pass_error = std::move(error);
   LLDB_LOG(log, "Communication({0}) thread exiting...", this);
 
-  // Handle threads wishing to synchronize with us.
-  {
-// Prevent new ones from showing up.
-m_read_thread_did_exit = true;
+  // Start shutting down. We need to do this in a very specific order to ensure
+  // we don't race with threads wanting to read/synchronize with us.
 
-// Unblock any existing thread waiting for the synchronization signal.
-BroadcastEvent(eBroadcastBitNoMorePendingInput);
+  // First, we signal our intent to exit. This ensures no new thread start
+  // waiting on events from us.
+  m_read_thread_did_exit = true;
 
-// Wait for the thread to finish...
+  // Unblock any existing thread waiting for the synchronization signal.
+  BroadcastEvent(eBroadcastBitNoMorePendingInput);
+
+  {
+// Wait for the synchronization thread to finish...
 std::lock_guard guard(m_synchronize_mutex);
 // ... and disconnect.
 if (disconnect)
   Disconnect();
   }
 
-  // Let clients know that this thread is exiting
+  // Finally, unblock any readers waiting for us to exit.
   BroadcastEvent(eBroadcastBitReadThreadDidExit);
   return {};
 }
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D133410: [lldb] Fix ThreadedCommunication races

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I've only seen one flake 
 on linux so far, but 
the windows bot seems more susceptible 
 to this 
 problem. 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133410

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

[Lldb-commits] [PATCH] D133181: [test] Remove problematic thread from MainLoopTest to fix flakiness

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D133181#3772830 , @rupprecht wrote:

> In D133181#3771747 , @labath wrote:
>
>> (I've reverted the pthread_kill part, as the mac build did not like it.)
>
> Thanks! I didn't get any buildbot notification; do LLDB build bots not send 
> email?

The regular buildbot bots do, but I'm not sure about the GreenDragon 
(@JDevlieghere ?). Although no emails would help in this case, as the bot was 
already red at the time this landed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133181

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

[Lldb-commits] [PATCH] D133129: [lldb] Add boilerplate for debugger interrupts

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I don't know how this is intended to be used, but I find it a rather 
heavyweight approach towards implementing, well... anything. The idea that 
something as innocuous as SBAddress::IsValid() will end up iterating through 
_all_ SBDebugger objects seems wrong, regardless of whether it has a measurable 
performance impact (and I'd be surprised if it doesn't). I'd hope that the 
interrupter can at least know which debugger it is trying to interrupt.

It's also not clear how something like this can be used to reliably interrupt 
an activity, as it's prone to all sorts of race conditions. (If the interuptee 
has not started the blocking call yet, then the interrupt request can be 
"eaten" by an innocuous IsValid call, whereas if it has already finished, then 
the interrupt request can remain pending and interrupt a totally unrelated 
call).

Overall, I think it would be good to have a RFC on this.


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

https://reviews.llvm.org/D133129

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

[Lldb-commits] [PATCH] D133243: [LLDB][NativePDB] Fix PdbAstBuilder::GetParentDeclContextForSymbol when ICF happens.

[Pavel Labath via Phabricator via lldb-commits]

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

Well, I still don't claim to understand this code, but I am happy to stamp 
anything that fixes bugs by deleting code.

Just as an idea for future work, you may want to check that we're correctly 
handling functions with unusual linkage names (e.g. due to asm labels). In 
DWARF, we're adding explicit asm labels with the debug-info-provided linkage 
names to make this work.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133243

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

[Lldb-commits] [PATCH] D133393: [test] Use localhost in place of 127.0.0.1 to run in ipv6-only environments.

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I believe the reasons are still relevant. Basically the problem is that 
listening on `localhost:x` creates two sockets (one for 127.0.0.1, one for 
::1), and there's no way to guarantee that we'll be able to grab the same port 
for both (one could be taken by some other application). Our listening code 
will succeed if it opens at least one socket, but then if we again try to 
connect using the `localhost` name, we may end up connecting to the wrong 
thing. I think the correct fix is to take the address (ip+port) that we've 
*actually* started listening on, and then pass *that* as the argument to the 
connect command, but I'm not sure if our current Socket APIs allow you to get 
that information.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133393

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

[Lldb-commits] [PATCH] D132940: [lldb] Use just-built libcxx for tests when available

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.



Comment at: lldb/test/API/commands/expression/fixits/TestFixIts.py:54
+# FIXME: LLDB struggles with this when stdlib has debug info.
+if not lldbutil.has_debug_info_in_libcxx(target):
+self.assertEquals(value.GetError().GetCString(), "error: No value")

What kind of error are you getting here? Are you sure this is not some form of 
https://github.com/llvm/llvm-project/issues/34391, which could be worked around 
by renaming some variables in the expression?



Comment at: 
lldb/test/API/commands/expression/import-std-module/forward_list-dbg-info-content/TestDbgInfoContentForwardListFromStdModule.py:35
 # FIXME: The value here isn't actually empty.
-self.expect_expr("a.front()",
- result_type=value_type,
- result_children=[ValueCheck()])
+# FIXME: LLDB struggles with this when stdlib has debug info.
+if not lldbutil.has_debug_info_in_libcxx(target):

And here it might be better to just remove the check, as it's checking for 
buggy behavior anyway.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132940

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

[Lldb-commits] [PATCH] D133352: [lldb-server] Report launch error in vRun packets

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.



Comment at: lldb/test/API/tools/lldb-server/TestGdbRemoteLaunch.py:18
+args = [exe_path, "stderr:arg1", "stderr:arg2", "stderr:arg3"]
+hex_args = [seven.hexlify(x) for x in args]
+

mgorny wrote:
> Since we no longer support Python 2, I'd rather prefer to work towards 
> removing `seven` rather than making more use of it.
Is the problem with the name/location of the function or the functionality 
(string/byte conversion) itself?
Because, if it's the first, then that could easily be solved by renaming the 
module (now or later), but in order to avoid elaborate casts we'd have to make 
all code be byte/string correct. This is not a problem here (because of the 
fixed strings), but it becomes one once you start working with things that 
aren't necessarily valid utf8 strings.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133352

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