[Lldb-commits] [PATCH] D120319: Set error message if ValueObjectRegister fails to write back to register

[Ilya Nozhkin via Phabricator via lldb-commits]

ilya-nozhkin updated this revision to Diff 411015.
ilya-nozhkin edited the summary of this revision.
ilya-nozhkin added a comment.

Added a test based on core files debugging. `RegisterContextCorePOSIX_x86_64` 
indeed forbids writing registers.


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

https://reviews.llvm.org/D120319

Files:
  lldb/source/Core/ValueObjectRegister.cpp
  lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py


Index: lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py
===
--- lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py
+++ lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py
@@ -133,6 +133,30 @@
 self.assertEqual(len(bytesread), 16)
 self.dbg.DeleteTarget(target)
 
+@skipIfLLVMTargetMissing("X86")
+def test_write_register(self):
+"""Test that writing to register results in an error and that error
+   message is set."""
+target: lldb.SBTarget = self.dbg.CreateTarget("linux-x86_64.out")
+process: lldb.SBProcess = target.LoadCore("linux-x86_64.core")
+self.assertTrue(process, PROCESS_IS_VALID)
+
+thread: lldb.SBThread = process.GetSelectedThread()
+self.assertTrue(thread)
+
+frame: lldb.SBFrame = thread.GetSelectedFrame()
+self.assertTrue(frame)
+
+reg_value: lldb.SBValue = frame.FindRegister('eax')
+if not reg_value.IsValid():
+  return
+
+error = lldb.SBError()
+success = reg_value.SetValueFromCString('10', error)
+self.assertFalse(success)
+self.assertTrue(error.Fail())
+self.assertIsNotNone(error.GetCString())
+
 @skipIfLLVMTargetMissing("X86")
 def test_FPR_SSE(self):
 # check x86_64 core file
Index: lldb/source/Core/ValueObjectRegister.cpp
===
--- lldb/source/Core/ValueObjectRegister.cpp
+++ lldb/source/Core/ValueObjectRegister.cpp
@@ -269,26 +269,30 @@
   // The new value will be in the m_data.  Copy that into our register value.
   error =
   m_reg_value.SetValueFromString(_reg_info, llvm::StringRef(value_str));
-  if (error.Success()) {
-if (m_reg_ctx_sp->WriteRegister(_reg_info, m_reg_value)) {
-  SetNeedsUpdate();
-  return true;
-} else
-  return false;
-  } else
+  if (!error.Success())
 return false;
+
+  if (!m_reg_ctx_sp->WriteRegister(_reg_info, m_reg_value)) {
+error.SetErrorString("unable to write back to register");
+return false;
+  }
+
+  SetNeedsUpdate();
+  return true;
 }
 
 bool ValueObjectRegister::SetData(DataExtractor , Status ) {
   error = m_reg_value.SetValueFromData(_reg_info, data, 0, false);
-  if (error.Success()) {
-if (m_reg_ctx_sp->WriteRegister(_reg_info, m_reg_value)) {
-  SetNeedsUpdate();
-  return true;
-} else
-  return false;
-  } else
+  if (!error.Success())
 return false;
+
+  if (!m_reg_ctx_sp->WriteRegister(_reg_info, m_reg_value)) {
+error.SetErrorString("unable to write back to register");
+return false;
+  }
+
+  SetNeedsUpdate();
+  return true;
 }
 
 bool ValueObjectRegister::ResolveValue(Scalar ) {


Index: lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py
===
--- lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py
+++ lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py
@@ -133,6 +133,30 @@
 self.assertEqual(len(bytesread), 16)
 self.dbg.DeleteTarget(target)
 
+@skipIfLLVMTargetMissing("X86")
+def test_write_register(self):
+"""Test that writing to register results in an error and that error
+   message is set."""
+target: lldb.SBTarget = self.dbg.CreateTarget("linux-x86_64.out")
+process: lldb.SBProcess = target.LoadCore("linux-x86_64.core")
+self.assertTrue(process, PROCESS_IS_VALID)
+
+thread: lldb.SBThread = process.GetSelectedThread()
+self.assertTrue(thread)
+
+frame: lldb.SBFrame = thread.GetSelectedFrame()
+self.assertTrue(frame)
+
+reg_value: lldb.SBValue = frame.FindRegister('eax')
+if not reg_value.IsValid():
+  return
+
+error = lldb.SBError()
+success = reg_value.SetValueFromCString('10', error)
+self.assertFalse(success)
+self.assertTrue(error.Fail())
+self.assertIsNotNone(error.GetCString())
+
 @skipIfLLVMTargetMissing("X86")
 def test_FPR_SSE(self):
 # check x86_64 core file
Index: lldb/source/Core/ValueObjectRegister.cpp
===
--- lldb/source/Core/ValueObjectRegister.cpp
+++ lldb/source/Core/ValueObjectRegister.cpp
@@ -269,26 +269,30 @@
   // The new value will be in the m_data.  Copy that into our register value.
   error 

[Lldb-commits] [PATCH] D113616: [lldb] Hyphenate Objective-C exception breakpoint labels ✍️

[Stephane Moore via Phabricator via lldb-commits]

stephanemoore added a comment.

In D113616#3341803 , @JDevlieghere 
wrote:

> This seems trivial enough, but adding Greg as he wrote most of lldb-vscode.

Thanks! 

I tried looking through the commit history but wasn't sure who would be a good 
candidate to review.

I also assume this should be a trivial change but I am happy to do extra 
digging if anyone has any concerns!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113616

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

[Lldb-commits] [PATCH] D120284: [lldb/test] Fix TestProgressReporting.py race issue with the event listener

[Med Ismail Bennani via Phabricator via lldb-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG3e3e79a9e4c3: [lldb/test] Fix TestProgressReporting.py race 
issue with the event listener (authored by mib).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120284

Files:
  lldb/bindings/interface/SBDebugger.i
  lldb/test/API/functionalities/progress_reporting/TestProgressReporting.py


Index: lldb/test/API/functionalities/progress_reporting/TestProgressReporting.py
===
--- lldb/test/API/functionalities/progress_reporting/TestProgressReporting.py
+++ lldb/test/API/functionalities/progress_reporting/TestProgressReporting.py
@@ -17,41 +17,42 @@
 TestBase.setUp(self)
 self.progress_events = []
 
-def fetch_events(self, test_broadcaster):
-listener = lldb.SBListener("lldb.progress.listener")
-listener.StartListeningForEvents(test_broadcaster,
- self.eBroadcastBitStopProgressThread)
-
-progress_broadcaster = self.dbg.GetBroadcaster()
-progress_broadcaster.AddListener(listener, 
lldb.SBDebugger.eBroadcastBitProgress)
-
+def fetch_events(self):
 event = lldb.SBEvent()
 
 done = False
 while not done:
-if listener.WaitForEvent(1, event):
+if self.listener.WaitForEvent(1, event):
 event_mask = event.GetType();
-if event.BroadcasterMatchesRef(test_broadcaster):
+if event.BroadcasterMatchesRef(self.test_broadcaster):
 if event_mask & self.eBroadcastBitStopProgressThread:
 done = True;
-elif event.BroadcasterMatchesRef(progress_broadcaster):
-message = lldb.SBDebugger().GetProgressFromEvent(event, 0, 
0, 0, False);
+elif event.BroadcasterMatchesRef(self.progress_broadcaster):
+ret_args = lldb.SBDebugger().GetProgressFromEvent(event);
+self.assertGreater(len(ret_args), 1)
+
+message = ret_args[0]
 if message:
 self.progress_events.append((message, event))
 
-@skipUnlessDarwin
 def test_dwarf_symbol_loading_progress_report(self):
 """Test that we are able to fetch dwarf symbol loading progress 
events"""
 self.build()
 
-test_broadcaster = lldb.SBBroadcaster('lldb.broadcaster.test')
-listener_thread = threading.Thread(target=self.fetch_events,
-   args=[test_broadcaster])
+self.listener = lldb.SBListener("lldb.progress.listener")
+self.test_broadcaster = lldb.SBBroadcaster('lldb.broadcaster.test')
+self.listener.StartListeningForEvents(self.test_broadcaster,
+  
self.eBroadcastBitStopProgressThread)
+
+self.progress_broadcaster = self.dbg.GetBroadcaster()
+self.progress_broadcaster.AddListener(self.listener, 
lldb.SBDebugger.eBroadcastBitProgress)
+
+listener_thread = threading.Thread(target=self.fetch_events)
 listener_thread.start()
 
 lldbutil.run_to_source_breakpoint(self, 'break here', 
lldb.SBFileSpec('main.c'))
 
-
test_broadcaster.BroadcastEventByType(self.eBroadcastBitStopProgressThread)
+
self.test_broadcaster.BroadcastEventByType(self.eBroadcastBitStopProgressThread)
 listener_thread.join()
 
-self.assertTrue(len(self.progress_events) > 0)
+self.assertGreater(len(self.progress_events), 0)
Index: lldb/bindings/interface/SBDebugger.i
===
--- lldb/bindings/interface/SBDebugger.i
+++ lldb/bindings/interface/SBDebugger.i
@@ -123,14 +123,11 @@
 };
 
 
-%apply uint64_t& INOUT { uint64_t& progress_id };
-%apply uint64_t& INOUT { uint64_t& completed };
-%apply uint64_t& INOUT { uint64_t& total };
-%apply bool& INOUT { bool& is_debugger_specific };
 static const char *GetProgressFromEvent(const lldb::SBEvent ,
-uint64_t _id,
-uint64_t , uint64_t ,
-bool _debugger_specific);
+uint64_t ,
+uint64_t ,
+uint64_t ,
+bool );
 
 SBBroadcaster GetBroadcaster();
 


Index: lldb/test/API/functionalities/progress_reporting/TestProgressReporting.py
===
--- lldb/test/API/functionalities/progress_reporting/TestProgressReporting.py
+++ 

[Lldb-commits] [lldb] 3e3e79a - [lldb/test] Fix TestProgressReporting.py race issue with the event listener

[Med Ismail Bennani via lldb-commits]

Author: Med Ismail Bennani
Date: 2022-02-23T16:45:28-08:00
New Revision: 3e3e79a9e4c378b59f5f393f556e6a84edcd8898

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

LOG: [lldb/test] Fix TestProgressReporting.py race issue with the event listener

This patch is a follow-up of D120100 to address some feedbacks from
@labath.

This should mainly fix the race issue with the even listener by moving
the listener setup to the main thread.

This also changes the SBDebugger::GetProgressFromEvent SWIG binding
arguments to be output only, so the user don't have to provide them.

Finally, this updates the test to check it the out arguments are returned
in a tuple and re-enables the test on all platforms.

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

Signed-off-by: Med Ismail Bennani 

Added: 


Modified: 
lldb/bindings/interface/SBDebugger.i
lldb/test/API/functionalities/progress_reporting/TestProgressReporting.py

Removed: 




diff  --git a/lldb/bindings/interface/SBDebugger.i 
b/lldb/bindings/interface/SBDebugger.i
index 3790857b8ab61..0ef1766a50c6b 100644
--- a/lldb/bindings/interface/SBDebugger.i
+++ b/lldb/bindings/interface/SBDebugger.i
@@ -123,14 +123,11 @@ public:
 };
 
 
-%apply uint64_t& INOUT { uint64_t& progress_id };
-%apply uint64_t& INOUT { uint64_t& completed };
-%apply uint64_t& INOUT { uint64_t& total };
-%apply bool& INOUT { bool& is_debugger_specific };
 static const char *GetProgressFromEvent(const lldb::SBEvent ,
-uint64_t _id,
-uint64_t , uint64_t ,
-bool _debugger_specific);
+uint64_t ,
+uint64_t ,
+uint64_t ,
+bool );
 
 SBBroadcaster GetBroadcaster();
 

diff  --git 
a/lldb/test/API/functionalities/progress_reporting/TestProgressReporting.py 
b/lldb/test/API/functionalities/progress_reporting/TestProgressReporting.py
index b9d9953539c11..79ef4e3f9f861 100644
--- a/lldb/test/API/functionalities/progress_reporting/TestProgressReporting.py
+++ b/lldb/test/API/functionalities/progress_reporting/TestProgressReporting.py
@@ -17,41 +17,42 @@ def setUp(self):
 TestBase.setUp(self)
 self.progress_events = []
 
-def fetch_events(self, test_broadcaster):
-listener = lldb.SBListener("lldb.progress.listener")
-listener.StartListeningForEvents(test_broadcaster,
- self.eBroadcastBitStopProgressThread)
-
-progress_broadcaster = self.dbg.GetBroadcaster()
-progress_broadcaster.AddListener(listener, 
lldb.SBDebugger.eBroadcastBitProgress)
-
+def fetch_events(self):
 event = lldb.SBEvent()
 
 done = False
 while not done:
-if listener.WaitForEvent(1, event):
+if self.listener.WaitForEvent(1, event):
 event_mask = event.GetType();
-if event.BroadcasterMatchesRef(test_broadcaster):
+if event.BroadcasterMatchesRef(self.test_broadcaster):
 if event_mask & self.eBroadcastBitStopProgressThread:
 done = True;
-elif event.BroadcasterMatchesRef(progress_broadcaster):
-message = lldb.SBDebugger().GetProgressFromEvent(event, 0, 
0, 0, False);
+elif event.BroadcasterMatchesRef(self.progress_broadcaster):
+ret_args = lldb.SBDebugger().GetProgressFromEvent(event);
+self.assertGreater(len(ret_args), 1)
+
+message = ret_args[0]
 if message:
 self.progress_events.append((message, event))
 
-@skipUnlessDarwin
 def test_dwarf_symbol_loading_progress_report(self):
 """Test that we are able to fetch dwarf symbol loading progress 
events"""
 self.build()
 
-test_broadcaster = lldb.SBBroadcaster('lldb.broadcaster.test')
-listener_thread = threading.Thread(target=self.fetch_events,
-   args=[test_broadcaster])
+self.listener = lldb.SBListener("lldb.progress.listener")
+self.test_broadcaster = lldb.SBBroadcaster('lldb.broadcaster.test')
+self.listener.StartListeningForEvents(self.test_broadcaster,
+  
self.eBroadcastBitStopProgressThread)
+
+self.progress_broadcaster = self.dbg.GetBroadcaster()
+self.progress_broadcaster.AddListener(self.listener, 
lldb.SBDebugger.eBroadcastBitProgress)
+
+listener_thread = 

[Lldb-commits] [PATCH] D120284: [lldb/test] Fix TestProgressReporting.py race issue with the event listener

[Med Ismail Bennani via Phabricator via lldb-commits]

mib updated this revision to Diff 410973.
mib edited the summary of this revision.
mib added a comment.

Make the interface binding arguments `OUTPUT` only.
Re-enable test on all platforms.


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

https://reviews.llvm.org/D120284

Files:
  lldb/bindings/interface/SBDebugger.i
  lldb/test/API/functionalities/progress_reporting/TestProgressReporting.py


Index: lldb/test/API/functionalities/progress_reporting/TestProgressReporting.py
===
--- lldb/test/API/functionalities/progress_reporting/TestProgressReporting.py
+++ lldb/test/API/functionalities/progress_reporting/TestProgressReporting.py
@@ -17,41 +17,42 @@
 TestBase.setUp(self)
 self.progress_events = []
 
-def fetch_events(self, test_broadcaster):
-listener = lldb.SBListener("lldb.progress.listener")
-listener.StartListeningForEvents(test_broadcaster,
- self.eBroadcastBitStopProgressThread)
-
-progress_broadcaster = self.dbg.GetBroadcaster()
-progress_broadcaster.AddListener(listener, 
lldb.SBDebugger.eBroadcastBitProgress)
-
+def fetch_events(self):
 event = lldb.SBEvent()
 
 done = False
 while not done:
-if listener.WaitForEvent(1, event):
+if self.listener.WaitForEvent(1, event):
 event_mask = event.GetType();
-if event.BroadcasterMatchesRef(test_broadcaster):
+if event.BroadcasterMatchesRef(self.test_broadcaster):
 if event_mask & self.eBroadcastBitStopProgressThread:
 done = True;
-elif event.BroadcasterMatchesRef(progress_broadcaster):
-message = lldb.SBDebugger().GetProgressFromEvent(event, 0, 
0, 0, False);
+elif event.BroadcasterMatchesRef(self.progress_broadcaster):
+ret_args = lldb.SBDebugger().GetProgressFromEvent(event);
+self.assertGreater(len(ret_args), 1)
+
+message = ret_args[0]
 if message:
 self.progress_events.append((message, event))
 
-@skipUnlessDarwin
 def test_dwarf_symbol_loading_progress_report(self):
 """Test that we are able to fetch dwarf symbol loading progress 
events"""
 self.build()
 
-test_broadcaster = lldb.SBBroadcaster('lldb.broadcaster.test')
-listener_thread = threading.Thread(target=self.fetch_events,
-   args=[test_broadcaster])
+self.listener = lldb.SBListener("lldb.progress.listener")
+self.test_broadcaster = lldb.SBBroadcaster('lldb.broadcaster.test')
+self.listener.StartListeningForEvents(self.test_broadcaster,
+  
self.eBroadcastBitStopProgressThread)
+
+self.progress_broadcaster = self.dbg.GetBroadcaster()
+self.progress_broadcaster.AddListener(self.listener, 
lldb.SBDebugger.eBroadcastBitProgress)
+
+listener_thread = threading.Thread(target=self.fetch_events)
 listener_thread.start()
 
 lldbutil.run_to_source_breakpoint(self, 'break here', 
lldb.SBFileSpec('main.c'))
 
-
test_broadcaster.BroadcastEventByType(self.eBroadcastBitStopProgressThread)
+
self.test_broadcaster.BroadcastEventByType(self.eBroadcastBitStopProgressThread)
 listener_thread.join()
 
-self.assertTrue(len(self.progress_events) > 0)
+self.assertGreater(len(self.progress_events), 0)
Index: lldb/bindings/interface/SBDebugger.i
===
--- lldb/bindings/interface/SBDebugger.i
+++ lldb/bindings/interface/SBDebugger.i
@@ -123,14 +123,11 @@
 };
 
 
-%apply uint64_t& INOUT { uint64_t& progress_id };
-%apply uint64_t& INOUT { uint64_t& completed };
-%apply uint64_t& INOUT { uint64_t& total };
-%apply bool& INOUT { bool& is_debugger_specific };
 static const char *GetProgressFromEvent(const lldb::SBEvent ,
-uint64_t _id,
-uint64_t , uint64_t ,
-bool _debugger_specific);
+uint64_t ,
+uint64_t ,
+uint64_t ,
+bool );
 
 SBBroadcaster GetBroadcaster();
 


Index: lldb/test/API/functionalities/progress_reporting/TestProgressReporting.py
===
--- lldb/test/API/functionalities/progress_reporting/TestProgressReporting.py
+++ lldb/test/API/functionalities/progress_reporting/TestProgressReporting.py
@@ -17,41 +17,42 @@
 TestBase.setUp(self)
 self.progress_events = []
 
-def 

[Lldb-commits] [PATCH] D113616: [lldb] Hyphenate Objective-C exception breakpoint labels ✍️

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

This seems trivial enough, but adding Greg as he wrote most of lldb-vscode.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113616

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

[Lldb-commits] [PATCH] D113616: [lldb] Hyphenate Objective-C exception breakpoint labels ✍️

[Michael Wyman via Phabricator via lldb-commits]

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

LGTM.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113616

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

[Lldb-commits] [PATCH] D113616: [lldb] Hyphenate Objective-C exception breakpoint labels ✍️

[Stephane Moore via Phabricator via lldb-commits]

stephanemoore created this revision.
stephanemoore updated this revision to Diff 410926.
stephanemoore added a comment.
stephanemoore added reviewers: mwyman, jingham.
stephanemoore published this revision for review.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

Updated.


mwyman added a comment.

LGTM


Objective-C is officially hyphenated:
https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/ObjectiveC/Introduction/introObjectiveC.html#//apple_ref/doc/uid/TP30001163


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D113616

Files:
  lldb/tools/lldb-vscode/VSCode.cpp


Index: lldb/tools/lldb-vscode/VSCode.cpp
===
--- lldb/tools/lldb-vscode/VSCode.cpp
+++ lldb/tools/lldb-vscode/VSCode.cpp
@@ -34,8 +34,8 @@
   exception_breakpoints(
   {{"cpp_catch", "C++ Catch", lldb::eLanguageTypeC_plus_plus},
{"cpp_throw", "C++ Throw", lldb::eLanguageTypeC_plus_plus},
-   {"objc_catch", "Objective C Catch", lldb::eLanguageTypeObjC},
-   {"objc_throw", "Objective C Throw", lldb::eLanguageTypeObjC},
+   {"objc_catch", "Objective-C Catch", lldb::eLanguageTypeObjC},
+   {"objc_throw", "Objective-C Throw", lldb::eLanguageTypeObjC},
{"swift_catch", "Swift Catch", lldb::eLanguageTypeSwift},
{"swift_throw", "Swift Throw", lldb::eLanguageTypeSwift}}),
   focus_tid(LLDB_INVALID_THREAD_ID), sent_terminated_event(false),


Index: lldb/tools/lldb-vscode/VSCode.cpp
===
--- lldb/tools/lldb-vscode/VSCode.cpp
+++ lldb/tools/lldb-vscode/VSCode.cpp
@@ -34,8 +34,8 @@
   exception_breakpoints(
   {{"cpp_catch", "C++ Catch", lldb::eLanguageTypeC_plus_plus},
{"cpp_throw", "C++ Throw", lldb::eLanguageTypeC_plus_plus},
-   {"objc_catch", "Objective C Catch", lldb::eLanguageTypeObjC},
-   {"objc_throw", "Objective C Throw", lldb::eLanguageTypeObjC},
+   {"objc_catch", "Objective-C Catch", lldb::eLanguageTypeObjC},
+   {"objc_throw", "Objective-C Throw", lldb::eLanguageTypeObjC},
{"swift_catch", "Swift Catch", lldb::eLanguageTypeSwift},
{"swift_throw", "Swift Throw", lldb::eLanguageTypeSwift}}),
   focus_tid(LLDB_INVALID_THREAD_ID), sent_terminated_event(false),
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D120319: Set error message if ValueObjectRegister fails to write back to register

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

Code looks good. Lets see if we can test it with a core file. I am guessing we 
can't from my previous comment, but it would be worth checking


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120319

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

[Lldb-commits] [PATCH] D120319: Set error message if ValueObjectRegister fails to write back to register

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

In D120319#3340298 , @labath wrote:

> Core files have read-only registers. Would that help? (you could just throw 
> in a SetValueFromCString call into one of the existing core file tests)

Actually I believe we allow writing registers for core files. This was 
requested so that people could fix the registers like SP or FP and the get a 
real backtrace. Since core files contain crashes, people wanted to be able to 
make sure they could get a backtrace.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120319

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

[Lldb-commits] [PATCH] D111409: proposed support for Java interface to Scripting Bridge

[David Millar via Phabricator via lldb-commits]

d-millar updated this revision to Diff 410920.
d-millar added a comment.

Rebased on v14


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D111409

Files:
  lldb/CMakeLists.txt
  lldb/bindings/CMakeLists.txt
  lldb/bindings/java/CMakeLists.txt
  lldb/bindings/java/SWIG/LldbScriptInterpreter.java
  lldb/bindings/java/java-typemaps.swig
  lldb/bindings/java/java-wrapper.swig
  lldb/bindings/java/java.swig
  lldb/cmake/modules/FindJavaAndSwig.cmake
  lldb/cmake/modules/LLDBConfig.cmake
  lldb/docs/resources/build.rst
  lldb/include/lldb/Core/IOHandler.h
  lldb/include/lldb/Host/Config.h.cmake
  lldb/include/lldb/lldb-enumerations.h
  lldb/source/API/CMakeLists.txt
  lldb/source/API/SBDebugger.cpp
  lldb/source/API/liblldb-private.exports
  lldb/source/API/liblldb.exports
  lldb/source/Commands/CommandObjectBreakpointCommand.cpp
  lldb/source/Commands/CommandObjectScript.cpp
  lldb/source/Commands/CommandObjectWatchpointCommand.cpp
  lldb/source/Interpreter/OptionArgParser.cpp
  lldb/source/Interpreter/ScriptInterpreter.cpp
  lldb/source/Plugins/ScriptInterpreter/CMakeLists.txt
  lldb/source/Plugins/ScriptInterpreter/Java/CMakeLists.txt
  lldb/source/Plugins/ScriptInterpreter/Java/Java.cpp
  lldb/source/Plugins/ScriptInterpreter/Java/Java.h
  lldb/source/Plugins/ScriptInterpreter/Java/ScriptInterpreterJava.cpp
  lldb/source/Plugins/ScriptInterpreter/Java/ScriptInterpreterJava.h
  lldb/test/CMakeLists.txt
  lldb/test/Shell/ScriptInterpreter/Java/Inputs/independent_state.in
  lldb/test/Shell/ScriptInterpreter/Java/Inputs/nested_sessions.in
  lldb/test/Shell/ScriptInterpreter/Java/Inputs/nested_sessions_2.in
  lldb/test/Shell/ScriptInterpreter/Java/Inputs/testmodule.java
  lldb/test/Shell/ScriptInterpreter/Java/bindings.test
  lldb/test/Shell/ScriptInterpreter/Java/breakpoint_callback.test
  lldb/test/Shell/ScriptInterpreter/Java/breakpoint_function_callback.test
  lldb/test/Shell/ScriptInterpreter/Java/breakpoint_oneline_callback.test
  lldb/test/Shell/ScriptInterpreter/Java/command_script_import.test
  lldb/test/Shell/ScriptInterpreter/Java/convenience_variables.test
  lldb/test/Shell/ScriptInterpreter/Java/fail_breakpoint_oneline.test
  lldb/test/Shell/ScriptInterpreter/Java/independent_state.test
  lldb/test/Shell/ScriptInterpreter/Java/io.test
  lldb/test/Shell/ScriptInterpreter/Java/java-python.test
  lldb/test/Shell/ScriptInterpreter/Java/java.test
  lldb/test/Shell/ScriptInterpreter/Java/lit.local.cfg
  lldb/test/Shell/ScriptInterpreter/Java/nested_sessions.test
  lldb/test/Shell/ScriptInterpreter/Java/partial_statements.test
  lldb/test/Shell/ScriptInterpreter/Java/persistent_state.test
  lldb/test/Shell/ScriptInterpreter/Java/print.test
  lldb/test/Shell/ScriptInterpreter/Java/quit.test
  lldb/test/Shell/ScriptInterpreter/Java/watchpoint_callback.test
  lldb/test/Shell/lit.cfg.py
  lldb/test/Shell/lit.site.cfg.py.in
  lldb/test/Unit/lit.cfg.py
  lldb/unittests/ScriptInterpreter/CMakeLists.txt
  lldb/unittests/ScriptInterpreter/Java/CMakeLists.txt
  lldb/unittests/ScriptInterpreter/Java/JavaTests.cpp
  lldb/unittests/ScriptInterpreter/Java/ScriptInterpreterTests.cpp

Index: lldb/unittests/ScriptInterpreter/Java/ScriptInterpreterTests.cpp
===
--- /dev/null
+++ lldb/unittests/ScriptInterpreter/Java/ScriptInterpreterTests.cpp
@@ -0,0 +1,59 @@
+//===-- JavaTests.cpp --===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "Plugins/Platform/Linux/PlatformLinux.h"
+#include "Plugins/ScriptInterpreter/Java/ScriptInterpreterJava.h"
+#include "lldb/Core/Debugger.h"
+#include "lldb/Host/FileSystem.h"
+#include "lldb/Host/HostInfo.h"
+#include "lldb/Interpreter/CommandReturnObject.h"
+#include "lldb/Target/Platform.h"
+#include "lldb/Utility/Reproducer.h"
+#include "gtest/gtest.h"
+
+using namespace lldb_private;
+using namespace lldb_private::repro;
+using namespace lldb;
+
+namespace {
+class ScriptInterpreterTest : public ::testing::Test {
+public:
+  void SetUp() override {
+llvm::cantFail(Reproducer::Initialize(ReproducerMode::Off, llvm::None));
+FileSystem::Initialize();
+HostInfo::Initialize();
+
+// Pretend Linux is the host platform.
+platform_linux::PlatformLinux::Initialize();
+ArchSpec arch("powerpc64-pc-linux");
+Platform::SetHostPlatform(
+platform_linux::PlatformLinux::CreateInstance(true, ));
+  }
+  void TearDown() override {
+platform_linux::PlatformLinux::Terminate();
+HostInfo::Terminate();
+FileSystem::Terminate();
+Reproducer::Terminate();
+  }
+};
+} // namespace
+
+TEST_F(ScriptInterpreterTest, 

[Lldb-commits] [PATCH] D120101: [lldb] Fix (unintentional) recursion in CommandObjectRegexCommand

[Jonas Devlieghere via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG2a6dbedf5a92: [lldb] Fix (unintentional) recursion in 
CommandObjectRegexCommand (authored by JDevlieghere).
Herald added a project: LLDB.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120101

Files:
  lldb/include/lldb/Interpreter/CommandReturnObject.h
  lldb/source/Commands/CommandObjectRegexCommand.cpp
  lldb/source/Commands/CommandObjectRegexCommand.h
  lldb/source/Interpreter/CommandReturnObject.cpp
  lldb/unittests/Interpreter/CMakeLists.txt
  lldb/unittests/Interpreter/TestRegexCommand.cpp

Index: lldb/unittests/Interpreter/TestRegexCommand.cpp
===
--- /dev/null
+++ lldb/unittests/Interpreter/TestRegexCommand.cpp
@@ -0,0 +1,68 @@
+//===-- TestRegexCommand.cpp --===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "Commands/CommandObjectRegexCommand.h"
+#include "llvm/Testing/Support/Error.h"
+#include "gtest/gtest.h"
+
+using namespace lldb_private;
+using namespace lldb;
+
+namespace {
+class TestRegexCommand : public CommandObjectRegexCommand {
+public:
+  using CommandObjectRegexCommand::SubstituteVariables;
+
+  static std::string
+  Substitute(llvm::StringRef input,
+ const llvm::SmallVectorImpl ) {
+llvm::Expected str = SubstituteVariables(input, replacements);
+if (!str)
+  return llvm::toString(str.takeError());
+return *str;
+  }
+};
+} // namespace
+
+TEST(RegexCommandTest, SubstituteVariablesSuccess) {
+  const llvm::SmallVector substitutions = {"all", "foo",
+   "bar", "baz"};
+
+  EXPECT_EQ(TestRegexCommand::Substitute("%0", substitutions), "all");
+  EXPECT_EQ(TestRegexCommand::Substitute("%1", substitutions), "foo");
+  EXPECT_EQ(TestRegexCommand::Substitute("%2", substitutions), "bar");
+  EXPECT_EQ(TestRegexCommand::Substitute("%3", substitutions), "baz");
+  EXPECT_EQ(TestRegexCommand::Substitute("%1%2%3", substitutions), "foobarbaz");
+  EXPECT_EQ(TestRegexCommand::Substitute("#%1#%2#%3#", substitutions),
+"#foo#bar#baz#");
+}
+
+TEST(RegexCommandTest, SubstituteVariablesFailed) {
+  const llvm::SmallVector substitutions = {"all", "foo",
+   "bar", "baz"};
+
+  ASSERT_THAT_EXPECTED(
+  TestRegexCommand::SubstituteVariables("%1%2%3%4", substitutions),
+  llvm::Failed());
+  ASSERT_THAT_EXPECTED(
+  TestRegexCommand::SubstituteVariables("%5", substitutions),
+  llvm::Failed());
+  ASSERT_THAT_EXPECTED(
+  TestRegexCommand::SubstituteVariables("%11", substitutions),
+  llvm::Failed());
+}
+
+TEST(RegexCommandTest, SubstituteVariablesNoRecursion) {
+  const llvm::SmallVector substitutions = {"all", "%2",
+   "%3", "%4"};
+  EXPECT_EQ(TestRegexCommand::Substitute("%0", substitutions), "all");
+  EXPECT_EQ(TestRegexCommand::Substitute("%1", substitutions), "%2");
+  EXPECT_EQ(TestRegexCommand::Substitute("%2", substitutions), "%3");
+  EXPECT_EQ(TestRegexCommand::Substitute("%3", substitutions), "%4");
+  EXPECT_EQ(TestRegexCommand::Substitute("%1%2%3", substitutions), "%2%3%4");
+}
Index: lldb/unittests/Interpreter/CMakeLists.txt
===
--- lldb/unittests/Interpreter/CMakeLists.txt
+++ lldb/unittests/Interpreter/CMakeLists.txt
@@ -4,6 +4,7 @@
   TestOptionArgParser.cpp
   TestOptionValue.cpp
   TestOptionValueFileColonLine.cpp
+  TestRegexCommand.cpp
 
   LINK_LIBS
   lldbCore
@@ -14,4 +15,5 @@
   lldbUtilityHelpers
   lldbInterpreter
   lldbPluginPlatformMacOSX
+  LLVMTestingSupport
 )
Index: lldb/source/Interpreter/CommandReturnObject.cpp
===
--- lldb/source/Interpreter/CommandReturnObject.cpp
+++ lldb/source/Interpreter/CommandReturnObject.cpp
@@ -109,6 +109,11 @@
   AppendError(error.AsCString(fallback_error_cstr));
 }
 
+void CommandReturnObject::SetError(llvm::Error error) {
+  if (error)
+AppendError(llvm::toString(std::move(error)));
+}
+
 // Similar to AppendError, but do not prepend 'Status: ' to message, and don't
 // append "\n" to the end of it.
 
Index: lldb/source/Commands/CommandObjectRegexCommand.h
===
--- lldb/source/Commands/CommandObjectRegexCommand.h
+++ lldb/source/Commands/CommandObjectRegexCommand.h
@@ -39,6 +39,11 @@
 protected:
   bool DoExecute(llvm::StringRef command, 

[Lldb-commits] [lldb] 2a6dbed - [lldb] Fix (unintentional) recursion in CommandObjectRegexCommand

[Jonas Devlieghere via lldb-commits]

Author: Jonas Devlieghere
Date: 2022-02-23T12:34:14-08:00
New Revision: 2a6dbedf5a923512ba8b5c2d192b5fc8bb1210fd

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

LOG: [lldb] Fix (unintentional) recursion in CommandObjectRegexCommand

Jim noticed that the regex command is unintentionally recursive. Let's
use the following command regex as an example:

  (lldb) com regex humm 's/([^ ]+) ([^ ]+)/p %1 %2 %1 %2/'

If we call it with arguments foo bar, thing behave as expected:

  (lldb) humm foo bar
  (...)
  foo bar foo bar

However, if we include %2 in the arguments, things break down:

  (lldb) humm fo%2o bar
  (...)
  fobaro bar fobaro bar

The problem is that the implementation of the substitution is too naive.
It substitutes the %1 token into the target template in place, then does
the %2 substitution starting with the resultant string. So if the
previous substitution introduced a %2 token, it would get processed in
the second sweep, etc.

This patch addresses the issue by walking the command once and
substituting the % variables in place.

  (lldb) humm fo%2o bar
  (...)
  fo%2o bar fo%2o bar

Furthermore, this patch also reports an error if not enough variables
were provided and add support for substituting %0.

rdar://81236994

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

Added: 
lldb/unittests/Interpreter/TestRegexCommand.cpp

Modified: 
lldb/include/lldb/Interpreter/CommandReturnObject.h
lldb/source/Commands/CommandObjectRegexCommand.cpp
lldb/source/Commands/CommandObjectRegexCommand.h
lldb/source/Interpreter/CommandReturnObject.cpp
lldb/unittests/Interpreter/CMakeLists.txt

Removed: 




diff  --git a/lldb/include/lldb/Interpreter/CommandReturnObject.h 
b/lldb/include/lldb/Interpreter/CommandReturnObject.h
index 72518a902144b..2177e721f9a40 100644
--- a/lldb/include/lldb/Interpreter/CommandReturnObject.h
+++ b/lldb/include/lldb/Interpreter/CommandReturnObject.h
@@ -15,6 +15,7 @@
 #include "lldb/lldb-private.h"
 
 #include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Error.h"
 #include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/WithColor.h"
 
@@ -132,6 +133,8 @@ class CommandReturnObject {
 
   void SetError(const Status , const char *fallback_error_cstr = 
nullptr);
 
+  void SetError(llvm::Error error);
+
   lldb::ReturnStatus GetStatus() const;
 
   void SetStatus(lldb::ReturnStatus status);

diff  --git a/lldb/source/Commands/CommandObjectRegexCommand.cpp 
b/lldb/source/Commands/CommandObjectRegexCommand.cpp
index 7ddc5c0c7e083..a99a9e760cb26 100644
--- a/lldb/source/Commands/CommandObjectRegexCommand.cpp
+++ b/lldb/source/Commands/CommandObjectRegexCommand.cpp
@@ -10,6 +10,9 @@
 #include "lldb/Interpreter/CommandInterpreter.h"
 #include "lldb/Interpreter/CommandReturnObject.h"
 
+#include "llvm/Support/Errc.h"
+#include "llvm/Support/Error.h"
+
 using namespace lldb;
 using namespace lldb_private;
 
@@ -25,35 +28,53 @@ CommandObjectRegexCommand::CommandObjectRegexCommand(
 // Destructor
 CommandObjectRegexCommand::~CommandObjectRegexCommand() = default;
 
+llvm::Expected CommandObjectRegexCommand::SubstituteVariables(
+llvm::StringRef input,
+const llvm::SmallVectorImpl ) {
+  std::string buffer;
+  llvm::raw_string_ostream output(buffer);
+
+  llvm::SmallVector parts;
+  input.split(parts, '%');
+
+  output << parts[0];
+  for (llvm::StringRef part : drop_begin(parts)) {
+size_t idx = 0;
+if (part.consumeInteger(10, idx))
+  output << '%';
+else if (idx < replacements.size())
+  output << replacements[idx];
+else
+  return llvm::make_error(
+  llvm::formatv("%{0} is out of range: not enough arguments specified",
+idx),
+  llvm::errc::invalid_argument);
+output << part;
+  }
+
+  return output.str();
+}
+
 bool CommandObjectRegexCommand::DoExecute(llvm::StringRef command,
   CommandReturnObject ) {
   EntryCollection::const_iterator pos, end = m_entries.end();
   for (pos = m_entries.begin(); pos != end; ++pos) {
 llvm::SmallVector matches;
 if (pos->regex.Execute(command, )) {
-  std::string new_command(pos->command);
-  char percent_var[8];
-  size_t idx, percent_var_idx;
-  for (uint32_t match_idx = 1; match_idx <= m_max_matches; ++match_idx) {
-if (match_idx < matches.size()) {
-  const std::string match_str = matches[match_idx].str();
-  const int percent_var_len =
-  ::snprintf(percent_var, sizeof(percent_var), "%%%u", match_idx);
-  for (idx = 0; (percent_var_idx = new_command.find(
- percent_var, idx)) != std::string::npos;) {
-new_command.erase(percent_var_idx, percent_var_len);
-   

[Lldb-commits] [PATCH] D120320: [lldb/driver] Fix SIGTSTP handling

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.



Comment at: lldb/tools/driver/Driver.cpp:673-674
+static void sigtstp_handler(int signo) {
   if (g_driver != nullptr)
 g_driver->GetDebugger().SaveInputTerminalState();
 

I see an opportunity for a little RAII helper.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120320

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

[Lldb-commits] [PATCH] D120425: [lldb/host] Remove monitor_signals argument from process monitoring functions

[Jonas Devlieghere via Phabricator via lldb-commits]

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

Nice


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120425

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

[Lldb-commits] [PATCH] D120425: [lldb/host] Remove monitor_signals argument from process monitoring functions

[Pavel Labath via Phabricator via lldb-commits]

labath created this revision.
labath added a reviewer: JDevlieghere.
Herald added a subscriber: krytarowski.
labath requested review of this revision.
Herald added a project: LLDB.

All current callers set the argument to false. monitor_signals=true used
to be used in the Process plugins (which needed to know when the
debugged process gets a signal), but this implementation has several
serious issues, which means that individual process plugins now
orchestrate the monitoring of debugged processes themselves.

This allows us to simplify the implementation (no need to play with
process groups), and the interface (we only catch fatal events, so the
callback is always called just once).


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D120425

Files:
  lldb/include/lldb/Host/Host.h
  lldb/include/lldb/Host/HostNativeProcessBase.h
  lldb/include/lldb/Host/HostProcess.h
  lldb/include/lldb/Host/ProcessLaunchInfo.h
  lldb/include/lldb/Host/posix/HostProcessPosix.h
  lldb/include/lldb/Host/windows/HostProcessWindows.h
  lldb/source/Host/common/Host.cpp
  lldb/source/Host/common/HostProcess.cpp
  lldb/source/Host/common/MonitoringProcessLauncher.cpp
  lldb/source/Host/common/ProcessLaunchInfo.cpp
  lldb/source/Host/macosx/objcxx/Host.mm
  lldb/source/Host/posix/HostProcessPosix.cpp
  lldb/source/Host/windows/HostProcessWindows.cpp
  lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp
  lldb/source/Plugins/Platform/QemuUser/PlatformQemuUser.cpp
  
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.cpp
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.h
  lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
  lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
  lldb/unittests/tools/lldb-server/tests/TestClient.cpp

Index: lldb/unittests/tools/lldb-server/tests/TestClient.cpp
===
--- lldb/unittests/tools/lldb-server/tests/TestClient.cpp
+++ lldb/unittests/tools/lldb-server/tests/TestClient.cpp
@@ -102,8 +102,7 @@
   // TODO: Use this callback to detect botched launches. If lldb-server does not
   // start, we can print a nice error message here instead of hanging in
   // Accept().
-  Info.SetMonitorProcessCallback(::NoOpMonitorCallback,
- false);
+  Info.SetMonitorProcessCallback(::NoOpMonitorCallback);
 
   status = Host::LaunchProcess(Info);
   if (status.Fail())
Index: lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
===
--- lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
+++ lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
@@ -344,10 +344,9 @@
 
   lldb::thread_result_t AsyncThread();
 
-  static bool
+  static void
   MonitorDebugserverProcess(std::weak_ptr process_wp,
-lldb::pid_t pid, bool exited, int signo,
-int exit_status);
+lldb::pid_t pid, int signo, int exit_status);
 
   lldb::StateType SetThreadStopInfo(StringExtractor _packet);
 
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
@@ -3366,7 +3366,7 @@
 const std::weak_ptr this_wp =
 std::static_pointer_cast(shared_from_this());
 debugserver_launch_info.SetMonitorProcessCallback(
-std::bind(MonitorDebugserverProcess, this_wp, _1, _2, _3, _4), false);
+std::bind(MonitorDebugserverProcess, this_wp, _1, _2, _3));
 debugserver_launch_info.SetUserID(process_info.GetUserID());
 
 #if defined(__APPLE__)
@@ -3445,16 +3445,14 @@
   return error;
 }
 
-bool ProcessGDBRemote::MonitorDebugserverProcess(
+void ProcessGDBRemote::MonitorDebugserverProcess(
 std::weak_ptr process_wp, lldb::pid_t debugserver_pid,
-bool exited,// True if the process did exit
 int signo,  // Zero for no signal
 int exit_status // Exit value of process if signal is zero
 ) {
   // "debugserver_pid" argument passed in is the process ID for debugserver
   // that we are tracking...
   Log *log = GetLog(GDBRLog::Process);
-  const bool handled = true;
 
   LLDB_LOGF(log,
 "ProcessGDBRemote::%s(process_wp, pid=%" PRIu64
@@ -3465,7 +3463,7 @@
   LLDB_LOGF(log, "ProcessGDBRemote::%s(process = %p)", __FUNCTION__,
 static_cast(process_sp.get()));
   if (!process_sp || process_sp->m_debugserver_pid != debugserver_pid)
-return handled;
+return;
 
   // Sleep for a half a second to make sure our inferior process has time to
   // set its exit status before we set it incorrectly when both the debugserver
@@ -3499,7 +3497,6 @@
   // Debugserver has exited we need to let our ProcessGDBRemote know that it no
   // longer has a debugserver instance
   

[Lldb-commits] [PATCH] D119963: [LLDB] Dump valid ranges of variables

[Zequan Wu via Phabricator via lldb-commits]

zequanwu added inline comments.



Comment at: lldb/test/Shell/SymbolFile/DWARF/x86/debug_loc.s:28-29
 # CHECK: Variable{{.*}}, name = "x0", {{.*}}, scope = parameter, location =
 # CHECK-NEXT:  [0x, 0x0001): DW_OP_reg5 RDI
 # CHECK-NEXT:  [0x0001, 0x0006): DW_OP_reg0 RAX
 # CHECK: Variable{{.*}}, name = "x1", {{.*}}, scope = parameter

labath wrote:
> zequanwu wrote:
> > labath wrote:
> > > zequanwu wrote:
> > > > labath wrote:
> > > > > zequanwu wrote:
> > > > > > labath wrote:
> > > > > > > zequanwu wrote:
> > > > > > > > `image dump symfile` already prints valid ranges for variables 
> > > > > > > > along with where the value is at each range.
> > > > > > > Are you sure it does?
> > > > > > > 
> > > > > > > I was under the impression that there are two distinct range 
> > > > > > > concepts being combined here. One is the range list member of the 
> > > > > > > Variable object (as given by `GetScopeRange` -- that's the one 
> > > > > > > you're printing now), and the other is the list of ranges hidden 
> > > > > > > in the DWARFExpression object, which come from the 
> > > > > > > debug_loc(lists) section (that's the one we've been printing so 
> > > > > > > far). And that the root cause of the confusion is the very 
> > > > > > > existence of these two concepts.
> > > > > > > 
> > > > > > > If I got it wrong, then do let me know, cause it would make 
> > > > > > > things a lot simpler if there is only one validity concept to 
> > > > > > > think about.
> > > > > > Dwarf plugin is supposed to construct the `m_scope_range` member of 
> > > > > > an Variable, but it doesn't. `scope_ranges` is empty at 
> > > > > > https://github.com/llvm/llvm-project/blob/main/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp#L3468.
> > > > > >  
> > > > > > `image dump symfile` dumps the dwarf location list in `m_location` 
> > > > > > in `Variable`. 
> > > > > > The dwarf location list has more information than `m_scope_range` 
> > > > > > as it contains info about where the value is during each range. 
> > > > > > (e.g. which register the variable lives in). 
> > > > > > 
> > > > > > So, I think we need to use similar logic to construct 
> > > > > > `m_scope_range` when creating `Variable` in dwarf plugin like this 
> > > > > > https://github.com/llvm/llvm-project/blob/main/lldb/source/Expression/DWARFExpression.cpp#L145.
> > > > > Ok, I see where you're coming from. You're essentially saying that 
> > > > > the fact that the dwarf plugin does not fill this out is a bug.
> > > > > 
> > > > > I don't think that's the case. My interpretation was (and [[ 
> > > > > https://github.com/llvm/llvm-project/blob/main/lldb/source/Symbol/Variable.cpp#L313
> > > > >  | this comment]] confirms it) that an empty range here means the 
> > > > > entire enclosing block. (Also, DWARF was for a long time the only 
> > > > > symbol file plugin, so what it does is kinda "correct by definition").
> > > > > 
> > > > > I don't think we want to change that interpretation, as forcing a 
> > > > > copy of the range in the location list would be wasteful (it would be 
> > > > > different if this was an interface that one could query, and that the 
> > > > > dwarf plugin could implement by consulting the location list). 
> > > > > However, since the dwarf class does not actually make use of this 
> > > > > functionality (it was [[ https://reviews.llvm.org/D17449 | added ]] 
> > > > > to support DW_AT_start_scope, then broken at some point, and 
> > > > > eventually [[ https://reviews.llvm.org/D62302 | removed ]]), we do 
> > > > > have some freedom in defining the interactions of the two fields (if 
> > > > > you still want to pursue this, that is).
> > > > > 
> > > > > So how about this: if the user passes the extra flag, then we print 
> > > > > both the range field (if it exists), and the *full* location list (in 
> > > > > that order, ideally). That way the output will be either `range = [a, 
> > > > > b), [c, d), location = DW_OP_reg47` or `location = [a,b) -> 
> > > > > DW_OP_reg4, [c,d) -> DW_OP_reg7`. If the dwarf plugin starts using 
> > > > > the range field again then the output will contain both fields, which 
> > > > > will be slightly confusing, but at least not misleading (and we can 
> > > > > also change the format then).
> > > > Oh, I think I misunderstood `m_scope_range`. It's the range list where 
> > > > the variable is valid regardless whether its value is accessible or not 
> > > > (valid range). As for `m_location` in `Variable`, it's describing the 
> > > > ranges where the value is (value range). They are not the same. 
> > > > 
> > > > So, currently how NativePDB creates local Variable's range is not 
> > > > correct. That only works when it's not optimized build such that the 
> > > > valid range is the same as the value range. It still need to create 
> > > > dwarf location lists to correctly represent the value range, 

[Lldb-commits] [PATCH] D119963: [LLDB] Dump valid ranges of variables

[Zequan Wu via Phabricator via lldb-commits]

zequanwu updated this revision to Diff 410882.
zequanwu marked 3 inline comments as done.
zequanwu added a comment.

- Remove short option. `-show-variable-ranges` prints all ranges. If not given, 
only print single range.
- Address comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119963

Files:
  lldb/include/lldb/Core/Address.h
  lldb/include/lldb/Expression/DWARFExpression.h
  lldb/source/Commands/CommandObjectTarget.cpp
  lldb/source/Commands/Options.td
  lldb/source/Core/Address.cpp
  lldb/source/Expression/DWARFExpression.cpp
  lldb/test/Shell/SymbolFile/DWARF/x86/DW_AT_loclists_base.s
  lldb/test/Shell/SymbolFile/DWARF/x86/debug_loc.s
  lldb/test/Shell/SymbolFile/DWARF/x86/debug_loc_and_loclists.s
  lldb/test/Shell/SymbolFile/DWARF/x86/debug_loclists-dwo.s
  lldb/test/Shell/SymbolFile/DWARF/x86/debug_loclists-dwp.s
  lldb/test/Shell/SymbolFile/DWARF/x86/dwp.s
  lldb/test/Shell/SymbolFile/DWARF/x86/unused-inlined-params.test

Index: lldb/test/Shell/SymbolFile/DWARF/x86/unused-inlined-params.test
===
--- lldb/test/Shell/SymbolFile/DWARF/x86/unused-inlined-params.test
+++ lldb/test/Shell/SymbolFile/DWARF/x86/unused-inlined-params.test
@@ -14,35 +14,35 @@
 # at the inlined function entry.
 image lookup -v -s break_at_inlined_f_in_main
 # CHECK-LABEL: image lookup -v -s break_at_inlined_f_in_main
-# CHECK: name = "unused1", type = "void *", location = 
-# CHECK: name = "used", type = "int", location = DW_OP_consts +42
-# CHECK: name = "unused2", type = "int", location = 
-# CHECK: name = "partial", type = "int", location = DW_OP_reg4 RSI
-# CHECK: name = "unused3", type = "int", location = 
+# CHECK: name = "unused1", type = "void *", valid ranges = , location = 
+# CHECK: name = "used", type = "int", valid ranges = , location = [0x0011, 0x0014) -> DW_OP_consts +42
+# CHECK: name = "unused2", type = "int", valid ranges = , location = 
+# CHECK: name = "partial", type = "int", valid ranges = , location = [0x0011, 0x0019) -> DW_OP_reg4 RSI
+# CHECK: name = "unused3", type = "int", valid ranges = , location = 
 
 # Show variables outsid of the live range of the 'partial' parameter
 # and verify that the output is as expected.
 image lookup -v -s break_at_inlined_f_in_main_between_printfs
 # CHECK-LABEL: image lookup -v -s break_at_inlined_f_in_main_between_printfs
-# CHECK: name = "unused1", type = "void *", location = 
-# CHECK: name = "used", type = "int", location = DW_OP_reg3 RBX
-# CHECK: name = "unused2", type = "int", location = 
+# CHECK: name = "unused1", type = "void *", valid ranges = , location = 
+# CHECK: name = "used", type = "int", valid ranges = , location = [0x0014, 0x001e) -> DW_OP_reg3 RBX
+# CHECK: name = "unused2", type = "int", valid ranges = , location = 
 # Note: image lookup does not show variables outside of their
 #   location, so |partial| is missing here.
 # CHECK-NOT: partial
-# CHECK: name = "unused3", type = "int", location = 
+# CHECK: name = "unused3", type = "int", valid ranges = , location = 
 
 # Check that we show parameters even if all of them are compiled away.
 image lookup -v -s  break_at_inlined_g_in_main
 # CHECK-LABEL: image lookup -v -s  break_at_inlined_g_in_main
-# CHECK: name = "unused", type = "int", location = 
+# CHECK: name = "unused", type = "int", valid ranges = , location = 
 
 # Check that even the other inlined instance of f displays the correct
 # parameters.
 image lookup -v -s  break_at_inlined_f_in_other
 # CHECK-LABEL: image lookup -v -s  break_at_inlined_f_in_other
-# CHECK: name = "unused1", type = "void *", location = 
-# CHECK: name = "used", type = "int", location = DW_OP_consts +1
-# CHECK: name = "unused2", type = "int", location = 
-# CHECK: name = "partial", type = "int", location =  DW_OP_consts +2
-# CHECK: name = "unused3", type = "int", location = 
+# CHECK: name = "unused1", type = "void *", valid ranges = , location = 
+# CHECK: name = "used", type = "int", valid ranges = , location = [0x0001, 0x000b) -> DW_OP_consts +1
+# CHECK: name = "unused2", type = "int", valid ranges = , location = 
+# CHECK: name = "partial", type = "int", valid ranges = , location = [0x0001, 0x0006) -> DW_OP_consts +2
+# CHECK: name = "unused3", type = "int", valid ranges = , location = 
Index: lldb/test/Shell/SymbolFile/DWARF/x86/dwp.s
===
--- lldb/test/Shell/SymbolFile/DWARF/x86/dwp.s
+++ lldb/test/Shell/SymbolFile/DWARF/x86/dwp.s
@@ -12,7 +12,7 @@
 # CHECK-LABEL: image lookup -v -n F1
 # CHECK: CompileUnit: id = {0x0001}, file = "1.c", language = ""
 # CHECK: Function: {{.*}}, name = "F1", range = [0x0001-0x0002)
-# CHECK: Variable: {{.*}}, name = "x", type = "int", location = 

[Lldb-commits] [PATCH] D120195: Cleanup llvm/DebugInfo/PDB headers

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

In D120195#3334697 , 
@serge-sans-paille wrote:

> In D120195#943 , @MaskRay wrote:
>
>> It'd be good to test `-DLLVM_ENABLE_MODULES=on` build.
>
> Sure, I'll add that to my local test setup.
>
>> Some files get pure new headers.
>
> That's expected. It happens a lot when some headers gets a forward 
> declaration instead of a header include when referencing a type.
>
>> I still think it is good thing to do it separately. There is a risk that 
>> someone may revert the change if it breaks some build modes.
>> Splitting the change can be mechanical, perhaps with some git commands to 
>> detect what files get pure addition.
>
> I fear I don't have the energy to go at that grain of detail :-/ I'm 
> currently testing with all projects enabled, in release mode. I'll add a 
> setup with ENABLE_MODULE and DEBUG mode to increase the coverage of my 
> pre-commit test

OK, that should be fine. Thanks for the efforts :)
I was just thinking of potential build problems (like thakis') if you happened 
not to be around to fix...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120195

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

[Lldb-commits] [PATCH] D120101: [lldb] Fix (unintentional) recursion in CommandObjectRegexCommand

[Med Ismail Bennani via Phabricator via lldb-commits]

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

LGTM !


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

https://reviews.llvm.org/D120101

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

[Lldb-commits] [PATCH] D120320: [lldb/driver] Fix SIGTSTP handling

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added a comment.

Got it, thanks for the explanation.

> What's important is that the second signal gets delivered (not "handled", 
> because at that point we have removed the handler) to the same thread, as 
> that's the only one we're sure that will have it unblocked (although, in 
> practice, all threads will probably have it unblocked).

Right, I was focusing on the "will return only after the signal handler has 
returned" aspect, but that's not the reason to you're choosing to use `raise`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120320

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

[Lldb-commits] [PATCH] D120320: [lldb/driver] Fix SIGTSTP handling

[Pavel Labath via Phabricator via lldb-commits]

labath marked 4 inline comments as done.
labath added a comment.

The behavior of the stop signals (SIGTTIN, SIGTTOU, SIGTSTP) is this:

- if the process has a signal handler, then the kernel picks an arbitrary 
thread (which does not have the signal blocked) to handle it. As far as the 
kernel is concerned, that's it. Everything else is up to the handler.
- if the process has no handler (and the signal is unblocked by at least one 
thread), then the _entire process_ goes to sleep

So, the answer to your question is: "thread which is running the signal 
handler" is "whichever is picked by the kernel". Its identity is not important. 
What's important is that the second signal gets delivered (not "handled", 
because at that point we have removed the handler) to the same thread, as 
that's the only one we're sure that will have it unblocked (although, in 
practice, all threads will probably have it unblocked).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120320

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

[Lldb-commits] [PATCH] D120320: [lldb/driver] Fix SIGTSTP handling

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added a comment.

I think I mostly get it and the code looks fine, but my signal foo is weak so 
@mgorny should give a second look.

> we use raise to send the signal, which makes sure it gets delivered to the 
> thread which is running the handler

https://man7.org/linux/man-pages/man3/raise.3.html says:

> If the signal causes a handler to be called, raise() will return
> only after the signal handler has returned.

So what is going to be the "thread which is running the signal handler" here? 
Will all the lldb threads follow these steps of enter handler -> unregister 
handler -> raise until finally lldb as a whole sleeps until we get a continue 
and do the reverse?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120320

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

[Lldb-commits] [PATCH] D120319: Set error message if ValueObjectRegister fails to write back to register

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Core files have read-only registers. Would that help? (you could just throw in 
a SetValueFromCString call into one of the existing core file tests)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120319

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

[Lldb-commits] [PATCH] D120320: [lldb/driver] Fix SIGTSTP handling

[Pavel Labath via Phabricator via lldb-commits]

labath marked 4 inline comments as done.
labath added inline comments.



Comment at: lldb/test/API/driver/job_control/TestJobControl.py:9
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+from lldbsuite.test.lldbpexpect import PExpectTest

DavidSpickett wrote:
> Unused import?
Yes, so are `lldb` and `decorator` imports. :)



Comment at: lldb/test/API/driver/job_control/TestJobControl.py:24
+self.launch(run_under=run_under, post_spawn=post_spawn)
+child = self.child
+

DavidSpickett wrote:
> Am I correct that `self.launch` will wait until `post_spawn` finishes, 
> meaning that self.child will always be set by this point?
> (or `post_spawn` timed out and we don't get here anyway)
> 
> Also if the point is just to assert that self.child has been set, it would be 
> a bit clearer to use an assert. Even if it is:
> ```
> self.assertTrue(hasattr(self, "child"))
> ```
> At least it looks like a deliberate check rather than a mistake if it fails. 
> (I probably have the arguments the wrong way around, I always do somehow)
The point of this was that I copy-pasted this code from some other test, which 
wanted to save some characters by typing `child` instead of `self.child`. 
However, I did not make use of that variable in any new code I actually wrote. 
:)

I'll just delete this line.



Comment at: lldb/test/API/driver/job_control/shell.py:10
+
+def preexec_fn():
+orig_mask = signal.pthread_sigmask(signal.SIG_BLOCK, [signal.SIGTTOU])

DavidSpickett wrote:
> Please comment this function.
> 
> I'm not sure if this intends to put the new process into a new process group 
> or the same process group as this script. (but mostly because I haven't used 
> process groups before now)
It creates a new process group.

The way that process groups (and sessions), terminals and signals interact is 
both awesome and horrifying at the same time. One has to admire the ingenuity 
of the job control implementation, which completely avoids threads and waits in 
the shell. But otoh, we're talking about signals, so chances are any random 
implementation will have a bug/race somewhere, particularly if the app is 
multi-threaded.



Comment at: lldb/tools/driver/Driver.cpp:834
   signal(SIGTSTP, sigtstp_handler);
-  signal(SIGCONT, sigcont_handler);
 #endif

DavidSpickett wrote:
> Is `sigcont_handler` now unused? It only references itself and is registered 
> here.
The patch actually does delete the function. It's just that the phabricator 
formatting makes that unobvious, since the patch essentially merges the two 
functions into one.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120320

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

[Lldb-commits] [PATCH] D120320: [lldb/driver] Fix SIGTSTP handling

[Pavel Labath via Phabricator via lldb-commits]

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

Add comments, delete unused code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120320

Files:
  lldb/packages/Python/lldbsuite/test/lldbpexpect.py
  lldb/test/API/driver/job_control/TestJobControl.py
  lldb/test/API/driver/job_control/shell.py
  lldb/tools/driver/Driver.cpp

Index: lldb/tools/driver/Driver.cpp
===
--- lldb/tools/driver/Driver.cpp
+++ lldb/tools/driver/Driver.cpp
@@ -668,23 +668,30 @@
   _exit(signo);
 }
 
-void sigtstp_handler(int signo) {
+#ifndef _WIN32
+static void sigtstp_handler(int signo) {
   if (g_driver != nullptr)
 g_driver->GetDebugger().SaveInputTerminalState();
 
+  // Unblock the signal and remove our handler.
+  sigset_t set;
+  sigemptyset();
+  sigaddset(, signo);
+  pthread_sigmask(SIG_UNBLOCK, , nullptr);
   signal(signo, SIG_DFL);
-  kill(getpid(), signo);
+
+  // Now re-raise the signal. We will immediately suspend...
+  raise(signo);
+  // ... and resume after a SIGCONT.
+
+  // Now undo the modifications.
+  pthread_sigmask(SIG_BLOCK, , nullptr);
   signal(signo, sigtstp_handler);
-}
 
-void sigcont_handler(int signo) {
   if (g_driver != nullptr)
 g_driver->GetDebugger().RestoreInputTerminalState();
-
-  signal(signo, SIG_DFL);
-  kill(getpid(), signo);
-  signal(signo, sigcont_handler);
 }
+#endif
 
 void reproducer_handler(void *finalize_cmd) {
   if (SBReproducer::Generate()) {
@@ -831,7 +838,6 @@
   signal(SIGPIPE, SIG_IGN);
   signal(SIGWINCH, sigwinch_handler);
   signal(SIGTSTP, sigtstp_handler);
-  signal(SIGCONT, sigcont_handler);
 #endif
 
   int exit_code = 0;
Index: lldb/test/API/driver/job_control/shell.py
===
--- /dev/null
+++ lldb/test/API/driver/job_control/shell.py
@@ -0,0 +1,32 @@
+"""
+Launch a process (given through argv) similar to how a shell would do it.
+"""
+
+import signal
+import subprocess
+import sys
+import os
+
+def preexec_fn():
+# Block SIGTTOU generated by the tcsetpgrp call
+orig_mask = signal.pthread_sigmask(signal.SIG_BLOCK, [signal.SIGTTOU])
+
+# Put us in a new process group.
+os.setpgid(0, 0)
+
+# And put it in the foreground.
+fd = os.open("/dev/tty", os.O_RDONLY)
+os.tcsetpgrp(fd, os.getpgid(0))
+os.close(fd)
+
+signal.pthread_sigmask(signal.SIG_SETMASK, orig_mask)
+
+if __name__ == "__main__":
+child = subprocess.Popen(sys.argv[1:], preexec_fn=preexec_fn)
+print("PID=%d" % child.pid)
+
+_, status = os.waitpid(child.pid, os.WUNTRACED)
+print("STATUS=%d" % status)
+
+returncode = child.wait()
+print("RETURNCODE=%d" % returncode)
Index: lldb/test/API/driver/job_control/TestJobControl.py
===
--- /dev/null
+++ lldb/test/API/driver/job_control/TestJobControl.py
@@ -0,0 +1,32 @@
+"""
+Test lldb's handling of job control signals (SIGTSTP, SIGCONT).
+"""
+
+
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test.lldbpexpect import PExpectTest
+
+
+class JobControlTest(PExpectTest):
+
+mydir = TestBase.compute_mydir(__file__)
+
+def test_job_control(self):
+def post_spawn():
+self.child.expect("PID=([0-9]+)")
+self.lldb_pid = int(self.child.match[1])
+
+run_under = [sys.executable, self.getSourcePath('shell.py')]
+self.launch(run_under=run_under, post_spawn=post_spawn)
+
+os.kill(self.lldb_pid, signal.SIGTSTP)
+self.child.expect("STATUS=([0-9]+)")
+status = int(self.child.match[1])
+
+self.assertTrue(os.WIFSTOPPED(status))
+self.assertEquals(os.WSTOPSIG(status), signal.SIGTSTP)
+
+os.kill(self.lldb_pid, signal.SIGCONT)
+
+self.child.sendline("quit")
+self.child.expect("RETURNCODE=0")
Index: lldb/packages/Python/lldbsuite/test/lldbpexpect.py
===
--- lldb/packages/Python/lldbsuite/test/lldbpexpect.py
+++ lldb/packages/Python/lldbsuite/test/lldbpexpect.py
@@ -23,11 +23,15 @@
 def expect_prompt(self):
 self.child.expect_exact(self.PROMPT)
 
-def launch(self, executable=None, extra_args=None, timeout=60, dimensions=None):
+def launch(self, executable=None, extra_args=None, timeout=60,
+dimensions=None, run_under=None, post_spawn=None):
 logfile = getattr(sys.stdout, 'buffer',
 sys.stdout) if self.TraceOn() else None
 
-args = ['--no-lldbinit', '--no-use-colors']
+args = []
+if run_under:
+args += run_under
+args += [lldbtest_config.lldbExec, '--no-lldbinit', '--no-use-colors']
 for cmd in self.setUpCommands():
 args += ['-O', cmd]
 if executable is not None:
@@ -41,8 +45,11 @@
 
 import pexpect
  

[Lldb-commits] [PATCH] D120321: [lldb] Modernize ThreadLauncher

[Nico Weber via Phabricator via lldb-commits]

thakis added a comment.

Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120321

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

[Lldb-commits] [lldb] 82951cf - Fix HostProcessWindows for D120321

[Pavel Labath via lldb-commits]

Author: Pavel Labath
Date: 2022-02-23T14:52:34+01:00
New Revision: 82951cfb8a413aab9c4b8aeecbd7475dda8f1fb1

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

LOG: Fix HostProcessWindows for D120321

Added: 


Modified: 
lldb/include/lldb/Host/windows/HostProcessWindows.h
lldb/source/Host/windows/HostProcessWindows.cpp

Removed: 




diff  --git a/lldb/include/lldb/Host/windows/HostProcessWindows.h 
b/lldb/include/lldb/Host/windows/HostProcessWindows.h
index dc27bdc46bb8f..4c69a2f434cf1 100644
--- a/lldb/include/lldb/Host/windows/HostProcessWindows.h
+++ b/lldb/include/lldb/Host/windows/HostProcessWindows.h
@@ -34,8 +34,6 @@ class HostProcessWindows : public HostNativeProcessBase {
   bool monitor_signals) override;
 
 private:
-  static lldb::thread_result_t MonitorThread(void *thread_arg);
-
   void Close();
 
   bool m_owns_handle;

diff  --git a/lldb/source/Host/windows/HostProcessWindows.cpp 
b/lldb/source/Host/windows/HostProcessWindows.cpp
index 741ec68d1d1ee..6ccb725ef56ee 100644
--- a/lldb/source/Host/windows/HostProcessWindows.cpp
+++ b/lldb/source/Host/windows/HostProcessWindows.cpp
@@ -63,38 +63,36 @@ bool HostProcessWindows::IsRunning() const {
   return (code == STILL_ACTIVE);
 }
 
+static lldb::thread_result_t
+MonitorThread(const Host::MonitorChildProcessCallback ,
+  HANDLE process_handle) {
+  DWORD exit_code;
+
+  ::WaitForSingleObject(process_handle, INFINITE);
+  ::GetExitCodeProcess(process_handle, _code);
+  callback(::GetProcessId(process_handle), true, 0, exit_code);
+  ::CloseHandle(process_handle);
+  return {};
+}
+
 llvm::Expected HostProcessWindows::StartMonitoring(
 const Host::MonitorChildProcessCallback , bool monitor_signals) {
-  MonitorInfo *info = new MonitorInfo;
-  info->callback = callback;
+  HANDLE process_handle;
 
   // Since the life of this HostProcessWindows instance and the life of the
   // process may be 
diff erent, duplicate the handle so that the monitor thread
   // can have ownership over its own copy of the handle.
   if (::DuplicateHandle(GetCurrentProcess(), m_process, GetCurrentProcess(),
->process_handle, 0, FALSE, 
DUPLICATE_SAME_ACCESS)) {
-return ThreadLauncher::LaunchThread("ChildProcessMonitor",
-HostProcessWindows::MonitorThread,
-info);
+_handle, 0, FALSE, DUPLICATE_SAME_ACCESS)) {
+return ThreadLauncher::LaunchThread(
+"ChildProcessMonitor", [callback, process_handle] {
+  return MonitorThread(callback, process_handle);
+});
   } else {
 return llvm::errorCodeToError(llvm::mapWindowsError(GetLastError()));
   }
 }
 
-lldb::thread_result_t HostProcessWindows::MonitorThread(void *thread_arg) {
-  DWORD exit_code;
-
-  MonitorInfo *info = static_cast(thread_arg);
-  if (info) {
-::WaitForSingleObject(info->process_handle, INFINITE);
-::GetExitCodeProcess(info->process_handle, _code);
-info->callback(::GetProcessId(info->process_handle), true, 0, exit_code);
-::CloseHandle(info->process_handle);
-delete (info);
-  }
-  return {};
-}
-
 void HostProcessWindows::Close() {
   if (m_owns_handle && m_process != LLDB_INVALID_PROCESS)
 ::CloseHandle(m_process);



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

[Lldb-commits] [PATCH] D120195: Cleanup llvm/DebugInfo/PDB headers

[serge via Phabricator via lldb-commits]

serge-sans-paille added a comment.

@thakis should be fixed by 57c6012213b50804ed78530b89bae30c0ee4fe82 
 , the new 
failure (seems) unrelated to this change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120195

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

[Lldb-commits] [PATCH] D120321: [lldb] Modernize ThreadLauncher

[Nico Weber via Phabricator via lldb-commits]

thakis added a comment.

Looks like this breaks building on Windows: 
http://45.33.8.238/win/53761/step_4.txt

Please take a look.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120321

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

[Lldb-commits] [PATCH] D120320: [lldb/driver] Fix SIGTSTP handling

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added inline comments.



Comment at: lldb/test/API/driver/job_control/TestJobControl.py:9
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+from lldbsuite.test.lldbpexpect import PExpectTest

Unused import?



Comment at: lldb/test/API/driver/job_control/TestJobControl.py:24
+self.launch(run_under=run_under, post_spawn=post_spawn)
+child = self.child
+

Am I correct that `self.launch` will wait until `post_spawn` finishes, meaning 
that self.child will always be set by this point?
(or `post_spawn` timed out and we don't get here anyway)

Also if the point is just to assert that self.child has been set, it would be a 
bit clearer to use an assert. Even if it is:
```
self.assertTrue(hasattr(self, "child"))
```
At least it looks like a deliberate check rather than a mistake if it fails. (I 
probably have the arguments the wrong way around, I always do somehow)



Comment at: lldb/test/API/driver/job_control/shell.py:10
+
+def preexec_fn():
+orig_mask = signal.pthread_sigmask(signal.SIG_BLOCK, [signal.SIGTTOU])

Please comment this function.

I'm not sure if this intends to put the new process into a new process group or 
the same process group as this script. (but mostly because I haven't used 
process groups before now)



Comment at: lldb/tools/driver/Driver.cpp:834
   signal(SIGTSTP, sigtstp_handler);
-  signal(SIGCONT, sigcont_handler);
 #endif

Is `sigcont_handler` now unused? It only references itself and is registered 
here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120320

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

[Lldb-commits] [PATCH] D120322: [lldb] Simplify HostThreadMacOSX

[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 rGf4568e12219f: [lldb] Simplify HostThreadMacOSX (authored by 
labath).

Changed prior to commit:
  https://reviews.llvm.org/D120322?vs=410518=410779#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120322

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


Index: lldb/source/Host/macosx/objcxx/HostThreadMacOSX.mm
===
--- lldb/source/Host/macosx/objcxx/HostThreadMacOSX.mm
+++ lldb/source/Host/macosx/objcxx/HostThreadMacOSX.mm
@@ -7,62 +7,14 @@
 
//===--===//
 
 #include "lldb/Host/macosx/HostThreadMacOSX.h"
-#include "lldb/Host/Host.h"
-
 #include 
 #include 
 
-#include 
-
 using namespace lldb_private;
 
-
-static pthread_once_t g_thread_create_once = PTHREAD_ONCE_INIT;
-static pthread_key_t g_thread_create_key = 0;
-
-namespace {
-class MacOSXDarwinThread {
-public:
-  MacOSXDarwinThread() { m_pool = [[NSAutoreleasePool alloc] init]; }
-
-  ~MacOSXDarwinThread() {
-if (m_pool) {
-  [m_pool drain];
-  m_pool = nil;
-}
-  }
-
-  static void PThreadDestructor(void *v) {
-if (v)
-  delete static_cast(v);
-::pthread_setspecific(g_thread_create_key, NULL);
-  }
-
-protected:
-  NSAutoreleasePool *m_pool = nil;
-
-private:
-  MacOSXDarwinThread(const MacOSXDarwinThread &) = delete;
-  const MacOSXDarwinThread =(const MacOSXDarwinThread &) = delete;
-};
-} // namespace
-
-static void InitThreadCreated() {
-  ::pthread_key_create(_thread_create_key,
-   MacOSXDarwinThread::PThreadDestructor);
-}
-
-HostThreadMacOSX::HostThreadMacOSX() : HostThreadPosix() {}
-
-HostThreadMacOSX::HostThreadMacOSX(lldb::thread_t thread)
-: HostThreadPosix(thread) {}
-
 lldb::thread_result_t
 HostThreadMacOSX::ThreadCreateTrampoline(lldb::thread_arg_t arg) {
-  ::pthread_once(_thread_create_once, InitThreadCreated);
-  if (g_thread_create_key) {
-::pthread_setspecific(g_thread_create_key, new MacOSXDarwinThread());
+  @autoreleasepool {
+return HostThreadPosix::ThreadCreateTrampoline(arg);
   }
-
-  return HostThreadPosix::ThreadCreateTrampoline(arg);
 }
Index: lldb/include/lldb/Host/macosx/HostThreadMacOSX.h
===
--- lldb/include/lldb/Host/macosx/HostThreadMacOSX.h
+++ lldb/include/lldb/Host/macosx/HostThreadMacOSX.h
@@ -17,8 +17,7 @@
   friend class ThreadLauncher;
 
 public:
-  HostThreadMacOSX();
-  HostThreadMacOSX(lldb::thread_t thread);
+  using HostThreadPosix::HostThreadPosix;
 
 protected:
   static lldb::thread_result_t ThreadCreateTrampoline(lldb::thread_arg_t arg);


Index: lldb/source/Host/macosx/objcxx/HostThreadMacOSX.mm
===
--- lldb/source/Host/macosx/objcxx/HostThreadMacOSX.mm
+++ lldb/source/Host/macosx/objcxx/HostThreadMacOSX.mm
@@ -7,62 +7,14 @@
 //===--===//
 
 #include "lldb/Host/macosx/HostThreadMacOSX.h"
-#include "lldb/Host/Host.h"
-
 #include 
 #include 
 
-#include 
-
 using namespace lldb_private;
 
-
-static pthread_once_t g_thread_create_once = PTHREAD_ONCE_INIT;
-static pthread_key_t g_thread_create_key = 0;
-
-namespace {
-class MacOSXDarwinThread {
-public:
-  MacOSXDarwinThread() { m_pool = [[NSAutoreleasePool alloc] init]; }
-
-  ~MacOSXDarwinThread() {
-if (m_pool) {
-  [m_pool drain];
-  m_pool = nil;
-}
-  }
-
-  static void PThreadDestructor(void *v) {
-if (v)
-  delete static_cast(v);
-::pthread_setspecific(g_thread_create_key, NULL);
-  }
-
-protected:
-  NSAutoreleasePool *m_pool = nil;
-
-private:
-  MacOSXDarwinThread(const MacOSXDarwinThread &) = delete;
-  const MacOSXDarwinThread =(const MacOSXDarwinThread &) = delete;
-};
-} // namespace
-
-static void InitThreadCreated() {
-  ::pthread_key_create(_thread_create_key,
-   MacOSXDarwinThread::PThreadDestructor);
-}
-
-HostThreadMacOSX::HostThreadMacOSX() : HostThreadPosix() {}
-
-HostThreadMacOSX::HostThreadMacOSX(lldb::thread_t thread)
-: HostThreadPosix(thread) {}
-
 lldb::thread_result_t
 HostThreadMacOSX::ThreadCreateTrampoline(lldb::thread_arg_t arg) {
-  ::pthread_once(_thread_create_once, InitThreadCreated);
-  if (g_thread_create_key) {
-::pthread_setspecific(g_thread_create_key, new MacOSXDarwinThread());
+  @autoreleasepool {
+return HostThreadPosix::ThreadCreateTrampoline(arg);
   }
-
-  return HostThreadPosix::ThreadCreateTrampoline(arg);
 }
Index: lldb/include/lldb/Host/macosx/HostThreadMacOSX.h
===
--- 

[Lldb-commits] [PATCH] D120321: [lldb] Modernize ThreadLauncher

[Pavel Labath via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGd0810779b1f3: [lldb] Modernize ThreadLauncher (authored by 
labath).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120321

Files:
  lldb/include/lldb/Core/Communication.h
  lldb/include/lldb/Core/Debugger.h
  lldb/include/lldb/Host/ThreadLauncher.h
  lldb/include/lldb/Target/Process.h
  lldb/source/API/SBHostOS.cpp
  lldb/source/Core/Communication.cpp
  lldb/source/Core/Debugger.cpp
  lldb/source/Host/common/Host.cpp
  lldb/source/Host/common/HostNativeThreadBase.cpp
  lldb/source/Host/common/ThreadLauncher.cpp
  lldb/source/Host/macosx/objcxx/Host.mm
  lldb/source/Plugins/Process/MacOSX-Kernel/ProcessKDP.cpp
  lldb/source/Plugins/Process/MacOSX-Kernel/ProcessKDP.h
  lldb/source/Plugins/Process/Windows/Common/DebuggerThread.cpp
  lldb/source/Plugins/Process/Windows/Common/DebuggerThread.h
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h
  lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
  lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
  lldb/source/Target/Process.cpp
  lldb/unittests/Host/CMakeLists.txt
  lldb/unittests/Host/ThreadLauncherTest.cpp

Index: lldb/unittests/Host/ThreadLauncherTest.cpp
===
--- /dev/null
+++ lldb/unittests/Host/ThreadLauncherTest.cpp
@@ -0,0 +1,29 @@
+//===-- ThreadLauncherTest.cpp ===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "lldb/Host/ThreadLauncher.h"
+#include "llvm/Testing/Support/Error.h"
+#include "gtest/gtest.h"
+#include 
+
+using namespace lldb_private;
+
+TEST(ThreadLauncherTest, LaunchThread) {
+  std::promise promise;
+  std::future future = promise.get_future();
+  llvm::Expected thread =
+  ThreadLauncher::LaunchThread("test", [] {
+promise.set_value(47);
+return (lldb::thread_result_t)47;
+  });
+  ASSERT_THAT_EXPECTED(thread, llvm::Succeeded());
+  EXPECT_EQ(future.get(), 47);
+  lldb::thread_result_t result;
+  thread->Join();
+  EXPECT_EQ(result, (lldb::thread_result_t)47);
+}
Index: lldb/unittests/Host/CMakeLists.txt
===
--- lldb/unittests/Host/CMakeLists.txt
+++ lldb/unittests/Host/CMakeLists.txt
@@ -12,6 +12,7 @@
   SocketAddressTest.cpp
   SocketTest.cpp
   SocketTestUtilities.cpp
+  ThreadLauncherTest.cpp
   XMLTest.cpp
 )
 
Index: lldb/source/Target/Process.cpp
===
--- lldb/source/Target/Process.cpp
+++ lldb/source/Target/Process.cpp
@@ -3509,12 +3509,13 @@
"", GetID());
   }
 
-  // Create the private state thread, and start it running.
-  PrivateStateThreadArgs *args_ptr =
-  new PrivateStateThreadArgs(this, is_secondary_thread);
   llvm::Expected private_state_thread =
-  ThreadLauncher::LaunchThread(thread_name, Process::PrivateStateThread,
-   (void *)args_ptr, 8 * 1024 * 1024);
+  ThreadLauncher::LaunchThread(
+  thread_name,
+  [this, is_secondary_thread] {
+return RunPrivateStateThread(is_secondary_thread);
+  },
+  8 * 1024 * 1024);
   if (!private_state_thread) {
 LLDB_LOG(GetLog(LLDBLog::Host), "failed to launch host thread: {}",
  llvm::toString(private_state_thread.takeError()));
@@ -3729,14 +3730,6 @@
   return error;
 }
 
-thread_result_t Process::PrivateStateThread(void *arg) {
-  std::unique_ptr args_up(
-  static_cast(arg));
-  thread_result_t result =
-  args_up->process->RunPrivateStateThread(args_up->is_secondary_thread);
-  return result;
-}
-
 thread_result_t Process::RunPrivateStateThread(bool is_secondary_thread) {
   bool control_only = true;
 
Index: lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
===
--- lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
+++ lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
@@ -342,7 +342,7 @@
 
   void StopAsyncThread();
 
-  static lldb::thread_result_t AsyncThread(void *arg);
+  lldb::thread_result_t AsyncThread();
 
   static bool
   MonitorDebugserverProcess(std::weak_ptr process_wp,
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
@@ -3541,8 +3541,10 @@
 // 

[Lldb-commits] [lldb] f4568e1 - [lldb] Simplify HostThreadMacOSX

[Pavel Labath via lldb-commits]

Author: Pavel Labath
Date: 2022-02-23T14:25:59+01:00
New Revision: f4568e12219f3a6ada20035ea40223680e274203

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

LOG: [lldb] Simplify HostThreadMacOSX

The class is using an incredibly elaborate setup to create and destroy
an NSAutoreleasePool object. We can do it in a much simpler way by
making those calls inside our thread startup function.

The only effect of this patch is that the pool gets released at the end
of the ThreadCreateTrampoline function, instead of slightly later, when
pthreads begin thread-specific cleanup. However, the key destruction
order is unspecified, so nothing should be relying on that.

I didn't find a specific reason for why this would have to be done that
way in git history. It seems that before D5198, this was thread-specific
keys were the only way an os implementation (in Host::ThreadCreated)
could attach some value to a thread.

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

Added: 


Modified: 
lldb/include/lldb/Host/macosx/HostThreadMacOSX.h
lldb/source/Host/macosx/objcxx/HostThreadMacOSX.mm

Removed: 




diff  --git a/lldb/include/lldb/Host/macosx/HostThreadMacOSX.h 
b/lldb/include/lldb/Host/macosx/HostThreadMacOSX.h
index 4e41119d97c6a..0299be3874085 100644
--- a/lldb/include/lldb/Host/macosx/HostThreadMacOSX.h
+++ b/lldb/include/lldb/Host/macosx/HostThreadMacOSX.h
@@ -17,8 +17,7 @@ class HostThreadMacOSX : public HostThreadPosix {
   friend class ThreadLauncher;
 
 public:
-  HostThreadMacOSX();
-  HostThreadMacOSX(lldb::thread_t thread);
+  using HostThreadPosix::HostThreadPosix;
 
 protected:
   static lldb::thread_result_t ThreadCreateTrampoline(lldb::thread_arg_t arg);

diff  --git a/lldb/source/Host/macosx/objcxx/HostThreadMacOSX.mm 
b/lldb/source/Host/macosx/objcxx/HostThreadMacOSX.mm
index 4f7e7ab248ad6..b24fe187b1a55 100644
--- a/lldb/source/Host/macosx/objcxx/HostThreadMacOSX.mm
+++ b/lldb/source/Host/macosx/objcxx/HostThreadMacOSX.mm
@@ -7,62 +7,14 @@
 
//===--===//
 
 #include "lldb/Host/macosx/HostThreadMacOSX.h"
-#include "lldb/Host/Host.h"
-
 #include 
 #include 
 
-#include 
-
 using namespace lldb_private;
 
-
-static pthread_once_t g_thread_create_once = PTHREAD_ONCE_INIT;
-static pthread_key_t g_thread_create_key = 0;
-
-namespace {
-class MacOSXDarwinThread {
-public:
-  MacOSXDarwinThread() { m_pool = [[NSAutoreleasePool alloc] init]; }
-
-  ~MacOSXDarwinThread() {
-if (m_pool) {
-  [m_pool drain];
-  m_pool = nil;
-}
-  }
-
-  static void PThreadDestructor(void *v) {
-if (v)
-  delete static_cast(v);
-::pthread_setspecific(g_thread_create_key, NULL);
-  }
-
-protected:
-  NSAutoreleasePool *m_pool = nil;
-
-private:
-  MacOSXDarwinThread(const MacOSXDarwinThread &) = delete;
-  const MacOSXDarwinThread =(const MacOSXDarwinThread &) = delete;
-};
-} // namespace
-
-static void InitThreadCreated() {
-  ::pthread_key_create(_thread_create_key,
-   MacOSXDarwinThread::PThreadDestructor);
-}
-
-HostThreadMacOSX::HostThreadMacOSX() : HostThreadPosix() {}
-
-HostThreadMacOSX::HostThreadMacOSX(lldb::thread_t thread)
-: HostThreadPosix(thread) {}
-
 lldb::thread_result_t
 HostThreadMacOSX::ThreadCreateTrampoline(lldb::thread_arg_t arg) {
-  ::pthread_once(_thread_create_once, InitThreadCreated);
-  if (g_thread_create_key) {
-::pthread_setspecific(g_thread_create_key, new MacOSXDarwinThread());
+  @autoreleasepool {
+return HostThreadPosix::ThreadCreateTrampoline(arg);
   }
-
-  return HostThreadPosix::ThreadCreateTrampoline(arg);
 }



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

[Lldb-commits] [lldb] d081077 - [lldb] Modernize ThreadLauncher

[Pavel Labath via lldb-commits]

Author: Pavel Labath
Date: 2022-02-23T14:25:59+01:00
New Revision: d0810779b1f310d99176467d5d5b5aa4e26d7eb5

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

LOG: [lldb] Modernize ThreadLauncher

Accept a function object instead of a raw pointer. This avoids a bunch
of boilerplate typically needed to pass arguments to the thread
functions.

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

Added: 
lldb/unittests/Host/ThreadLauncherTest.cpp

Modified: 
lldb/include/lldb/Core/Communication.h
lldb/include/lldb/Core/Debugger.h
lldb/include/lldb/Host/ThreadLauncher.h
lldb/include/lldb/Target/Process.h
lldb/source/API/SBHostOS.cpp
lldb/source/Core/Communication.cpp
lldb/source/Core/Debugger.cpp
lldb/source/Host/common/Host.cpp
lldb/source/Host/common/HostNativeThreadBase.cpp
lldb/source/Host/common/ThreadLauncher.cpp
lldb/source/Host/macosx/objcxx/Host.mm
lldb/source/Plugins/Process/MacOSX-Kernel/ProcessKDP.cpp
lldb/source/Plugins/Process/MacOSX-Kernel/ProcessKDP.h
lldb/source/Plugins/Process/Windows/Common/DebuggerThread.cpp
lldb/source/Plugins/Process/Windows/Common/DebuggerThread.h
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h
lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
lldb/source/Target/Process.cpp
lldb/unittests/Host/CMakeLists.txt

Removed: 




diff  --git a/lldb/include/lldb/Core/Communication.h 
b/lldb/include/lldb/Core/Communication.h
index fdcb6c5fb9822..44b3a16a05269 100644
--- a/lldb/include/lldb/Core/Communication.h
+++ b/lldb/include/lldb/Core/Communication.h
@@ -277,7 +277,7 @@ class Communication : public Broadcaster {
   /// \b True if the read thread is running, \b false otherwise.
   bool ReadThreadIsRunning();
 
-  /// The static read thread function. This function will call the "DoRead"
+  /// The read thread function. This function will call the "DoRead"
   /// function continuously and wait for data to become available. When data
   /// is received it will append the available data to the internal cache and
   /// broadcast a \b eBroadcastBitReadThreadGotBytes event.
@@ -289,7 +289,7 @@ class Communication : public Broadcaster {
   /// \b NULL.
   ///
   /// \see void Communication::ReadThreadGotBytes (const uint8_t *, size_t);
-  static lldb::thread_result_t ReadThread(lldb::thread_arg_t comm_ptr);
+  lldb::thread_result_t ReadThread();
 
   void SetReadThreadBytesReceivedCallback(ReadThreadBytesReceived callback,
   void *callback_baton);

diff  --git a/lldb/include/lldb/Core/Debugger.h 
b/lldb/include/lldb/Core/Debugger.h
index f9a1f1eea54f2..d4fae0c5d09cb 100644
--- a/lldb/include/lldb/Core/Debugger.h
+++ b/lldb/include/lldb/Core/Debugger.h
@@ -439,8 +439,6 @@ class Debugger : public 
std::enable_shared_from_this,
 
   void StopEventHandlerThread();
 
-  static lldb::thread_result_t EventHandlerThread(lldb::thread_arg_t arg);
-
   void PushIOHandler(const lldb::IOHandlerSP _sp,
  bool cancel_top_handler = true);
 
@@ -454,9 +452,9 @@ class Debugger : public 
std::enable_shared_from_this,
 
   void JoinIOHandlerThread();
 
-  static lldb::thread_result_t IOHandlerThread(lldb::thread_arg_t arg);
+  lldb::thread_result_t IOHandlerThread();
 
-  void DefaultEventHandler();
+  lldb::thread_result_t DefaultEventHandler();
 
   void HandleBreakpointEvent(const lldb::EventSP _sp);
 

diff  --git a/lldb/include/lldb/Host/ThreadLauncher.h 
b/lldb/include/lldb/Host/ThreadLauncher.h
index 00b42fa6a11d5..8bb6c79466a76 100644
--- a/lldb/include/lldb/Host/ThreadLauncher.h
+++ b/lldb/include/lldb/Host/ThreadLauncher.h
@@ -20,8 +20,8 @@ namespace lldb_private {
 class ThreadLauncher {
 public:
   static llvm::Expected
-  LaunchThread(llvm::StringRef name, lldb::thread_func_t thread_function,
-   lldb::thread_arg_t thread_arg,
+  LaunchThread(llvm::StringRef name,
+   std::function thread_function,
size_t min_stack_byte_size = 0); // Minimum stack size in bytes,
 // set stack size to zero for
 // default platform thread 
stack
@@ -29,12 +29,11 @@ class ThreadLauncher {
 
   struct HostThreadCreateInfo {
 std::string thread_name;
-lldb::thread_func_t thread_fptr;
-lldb::thread_arg_t thread_arg;
+std::function impl;
 
-HostThreadCreateInfo(const char *name, lldb::thread_func_t fptr,
- lldb::thread_arg_t arg)
-: thread_name(name ? name : ""), thread_fptr(fptr), thread_arg(arg) {}
+   

[Lldb-commits] [PATCH] D120321: [lldb] Modernize ThreadLauncher

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.



Comment at: lldb/source/Host/common/HostNativeThreadBase.cpp:55-56
 HostNativeThreadBase::ThreadCreateTrampoline(lldb::thread_arg_t arg) {
-  ThreadLauncher::HostThreadCreateInfo *info =
-  (ThreadLauncher::HostThreadCreateInfo *)arg;
-  llvm::set_thread_name(info->thread_name);
-
-  thread_func_t thread_fptr = info->thread_fptr;
-  thread_arg_t thread_arg = info->thread_arg;
+  std::unique_ptr info_up(
+  (ThreadLauncher::HostThreadCreateInfo *)arg);
+  llvm::set_thread_name(info_up->thread_name);

JDevlieghere wrote:
> make_unique maybe?
This isn't creating a new object. It's taking ownership of an object that was 
created in the caller (and passed disguised as a `void*`).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120321

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

[Lldb-commits] [PATCH] D119963: [LLDB] Dump valid ranges of variables

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I'm not sure about `ModuleLookupShowRange::Single` option. It feels like it 
just complicates things. Did anyone request that?

FWIW, I don't think that dumping a /single/ range is particularly verbose, so I 
could imagine even doing that by default.

I think it should be sufficient to have just two modes, either None+All, or 
Single+All. Having three seems like overkill.




Comment at: lldb/source/Core/Address.cpp:734-740
+s->PutCString(" [");
+s->AsRawOstream()
+<< llvm::format_hex(range.GetRangeBase(), 2 + 2 * addr_size);
+s->PutCString(", ");
+s->AsRawOstream()
+<< llvm::format_hex(range.GetRangeEnd(), 2 + 2 * addr_size);
+s->PutCString(")");

Use `DumpAddressRange` from `Utility/Stream.h`



Comment at: lldb/source/Core/Address.cpp:790-801
+  auto range_printer = [&](llvm::DWARFAddressRange range,
+   bool is_first) {
+if (!is_first)
+  s->PutCString(", ");
+s->PutCString("[");
+s->AsRawOstream()
+<< llvm::format_hex(range.LowPC, 2 + 2 * addr_size);

Can we get rid of this lambda by inlining it into `DumpLocations`. I don't 
think we need that much flexibility.



Comment at: lldb/test/Shell/SymbolFile/DWARF/x86/debug_loc.s:22
+# SINGLE-RANGE-LABEL: image lookup -v -a 0 -R s
+# SINGLE-RANGE: Variable: id = {{.*}}, name = "x0", type = "int", valid ranges 
=, location = [0x, 0x0001) -> DW_OP_reg5 RDI, decl =
+# SINGLE-RANGE: Variable: id = {{.*}}, name = "x1", type = "int", valid ranges 
=, location = , decl =

print something like `valid ranges = `, or maybe don't print anything at 
all?



Comment at: lldb/test/Shell/SymbolFile/DWARF/x86/debug_loc.s:28-29
 # CHECK: Variable{{.*}}, name = "x0", {{.*}}, scope = parameter, location =
 # CHECK-NEXT:  [0x, 0x0001): DW_OP_reg5 RDI
 # CHECK-NEXT:  [0x0001, 0x0006): DW_OP_reg0 RAX
 # CHECK: Variable{{.*}}, name = "x1", {{.*}}, scope = parameter

zequanwu wrote:
> labath wrote:
> > zequanwu wrote:
> > > labath wrote:
> > > > zequanwu wrote:
> > > > > labath wrote:
> > > > > > zequanwu wrote:
> > > > > > > `image dump symfile` already prints valid ranges for variables 
> > > > > > > along with where the value is at each range.
> > > > > > Are you sure it does?
> > > > > > 
> > > > > > I was under the impression that there are two distinct range 
> > > > > > concepts being combined here. One is the range list member of the 
> > > > > > Variable object (as given by `GetScopeRange` -- that's the one 
> > > > > > you're printing now), and the other is the list of ranges hidden in 
> > > > > > the DWARFExpression object, which come from the debug_loc(lists) 
> > > > > > section (that's the one we've been printing so far). And that the 
> > > > > > root cause of the confusion is the very existence of these two 
> > > > > > concepts.
> > > > > > 
> > > > > > If I got it wrong, then do let me know, cause it would make things 
> > > > > > a lot simpler if there is only one validity concept to think about.
> > > > > Dwarf plugin is supposed to construct the `m_scope_range` member of 
> > > > > an Variable, but it doesn't. `scope_ranges` is empty at 
> > > > > https://github.com/llvm/llvm-project/blob/main/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp#L3468.
> > > > >  
> > > > > `image dump symfile` dumps the dwarf location list in `m_location` in 
> > > > > `Variable`. 
> > > > > The dwarf location list has more information than `m_scope_range` as 
> > > > > it contains info about where the value is during each range. (e.g. 
> > > > > which register the variable lives in). 
> > > > > 
> > > > > So, I think we need to use similar logic to construct `m_scope_range` 
> > > > > when creating `Variable` in dwarf plugin like this 
> > > > > https://github.com/llvm/llvm-project/blob/main/lldb/source/Expression/DWARFExpression.cpp#L145.
> > > > Ok, I see where you're coming from. You're essentially saying that the 
> > > > fact that the dwarf plugin does not fill this out is a bug.
> > > > 
> > > > I don't think that's the case. My interpretation was (and [[ 
> > > > https://github.com/llvm/llvm-project/blob/main/lldb/source/Symbol/Variable.cpp#L313
> > > >  | this comment]] confirms it) that an empty range here means the 
> > > > entire enclosing block. (Also, DWARF was for a long time the only 
> > > > symbol file plugin, so what it does is kinda "correct by definition").
> > > > 
> > > > I don't think we want to change that interpretation, as forcing a copy 
> > > > of the range in the location list would be wasteful (it would be 
> > > > different if this was an interface that one could query, and that 

[Lldb-commits] [PATCH] D120195: Cleanup llvm/DebugInfo/PDB headers

[Nico Weber via Phabricator via lldb-commits]

thakis added a comment.

Breaks building on win: http://45.33.8.238/win/53749/step_4.txt

Please take a look and revert for now if it takes a while to fix.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120195

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

[Lldb-commits] [PATCH] D120195: Cleanup llvm/DebugInfo/PDB headers

[serge via Phabricator via lldb-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGeb4c8608115c: Cleanup llvm/DebugInfo/PDB headers (authored 
by serge-sans-paille).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120195

Files:
  lld/COFF/PDB.cpp
  lldb/source/Plugins/SymbolFile/PDB/PDBASTParser.cpp
  lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp
  llvm/include/llvm/DebugInfo/MSF/MSFBuilder.h
  llvm/include/llvm/DebugInfo/PDB/Native/DbiModuleDescriptorBuilder.h
  llvm/include/llvm/DebugInfo/PDB/Native/DbiStreamBuilder.h
  llvm/include/llvm/DebugInfo/PDB/Native/GSIStreamBuilder.h
  llvm/include/llvm/DebugInfo/PDB/Native/InfoStreamBuilder.h
  llvm/include/llvm/DebugInfo/PDB/Native/NativeEnumGlobals.h
  llvm/include/llvm/DebugInfo/PDB/Native/NativeEnumLineNumbers.h
  llvm/include/llvm/DebugInfo/PDB/Native/NativeEnumSymbols.h
  llvm/include/llvm/DebugInfo/PDB/Native/NativeEnumTypes.h
  llvm/include/llvm/DebugInfo/PDB/Native/NativeExeSymbol.h
  llvm/include/llvm/DebugInfo/PDB/Native/NativeFunctionSymbol.h
  llvm/include/llvm/DebugInfo/PDB/Native/NativeInlineSiteSymbol.h
  llvm/include/llvm/DebugInfo/PDB/Native/NativeLineNumber.h
  llvm/include/llvm/DebugInfo/PDB/Native/NativePublicSymbol.h
  llvm/include/llvm/DebugInfo/PDB/Native/NativeSession.h
  llvm/include/llvm/DebugInfo/PDB/Native/NativeSourceFile.h
  llvm/include/llvm/DebugInfo/PDB/Native/NativeSymbolEnumerator.h
  llvm/include/llvm/DebugInfo/PDB/Native/NativeTypeEnum.h
  llvm/include/llvm/DebugInfo/PDB/Native/NativeTypeFunctionSig.h
  llvm/include/llvm/DebugInfo/PDB/Native/NativeTypePointer.h
  llvm/include/llvm/DebugInfo/PDB/Native/NativeTypeTypedef.h
  llvm/include/llvm/DebugInfo/PDB/Native/NativeTypeUDT.h
  llvm/include/llvm/DebugInfo/PDB/Native/NativeTypeVTShape.h
  llvm/include/llvm/DebugInfo/PDB/Native/PDBFileBuilder.h
  llvm/include/llvm/DebugInfo/PDB/Native/SymbolCache.h
  llvm/include/llvm/DebugInfo/PDB/Native/TpiStreamBuilder.h
  llvm/include/llvm/DebugInfo/PDB/PDBSymbol.h
  llvm/include/llvm/DebugInfo/PDB/PDBSymbolAnnotation.h
  llvm/include/llvm/DebugInfo/PDB/PDBSymbolBlock.h
  llvm/include/llvm/DebugInfo/PDB/PDBSymbolCompilandDetails.h
  llvm/include/llvm/DebugInfo/PDB/PDBSymbolCompilandEnv.h
  llvm/include/llvm/DebugInfo/PDB/PDBSymbolCustom.h
  llvm/include/llvm/DebugInfo/PDB/PDBSymbolData.h
  llvm/include/llvm/DebugInfo/PDB/PDBSymbolFunc.h
  llvm/include/llvm/DebugInfo/PDB/PDBSymbolFuncDebugEnd.h
  llvm/include/llvm/DebugInfo/PDB/PDBSymbolFuncDebugStart.h
  llvm/include/llvm/DebugInfo/PDB/PDBSymbolLabel.h
  llvm/include/llvm/DebugInfo/PDB/PDBSymbolPublicSymbol.h
  llvm/include/llvm/DebugInfo/PDB/PDBSymbolThunk.h
  llvm/include/llvm/DebugInfo/PDB/PDBSymbolTypeArray.h
  llvm/include/llvm/DebugInfo/PDB/PDBSymbolTypeBaseClass.h
  llvm/include/llvm/DebugInfo/PDB/PDBSymbolTypeBuiltin.h
  llvm/include/llvm/DebugInfo/PDB/PDBSymbolTypeCustom.h
  llvm/include/llvm/DebugInfo/PDB/PDBSymbolTypeDimension.h
  llvm/include/llvm/DebugInfo/PDB/PDBSymbolTypeEnum.h
  llvm/include/llvm/DebugInfo/PDB/PDBSymbolTypeFriend.h
  llvm/include/llvm/DebugInfo/PDB/PDBSymbolTypeFunctionArg.h
  llvm/include/llvm/DebugInfo/PDB/PDBSymbolTypeManaged.h
  llvm/include/llvm/DebugInfo/PDB/PDBSymbolTypePointer.h
  llvm/include/llvm/DebugInfo/PDB/PDBSymbolTypeTypedef.h
  llvm/include/llvm/DebugInfo/PDB/PDBSymbolTypeUDT.h
  llvm/include/llvm/DebugInfo/PDB/PDBSymbolTypeVTable.h
  llvm/include/llvm/DebugInfo/PDB/PDBSymbolTypeVTableShape.h
  llvm/include/llvm/DebugInfo/PDB/PDBSymbolUnknown.h
  llvm/include/llvm/DebugInfo/PDB/PDBSymbolUsingNamespace.h
  llvm/include/llvm/DebugInfo/PDB/UDTLayout.h
  llvm/lib/DebugInfo/DWARF/DWARFCompileUnit.cpp
  llvm/lib/DebugInfo/MSF/MappedBlockStream.cpp
  llvm/lib/DebugInfo/PDB/Native/DbiModuleDescriptorBuilder.cpp
  llvm/lib/DebugInfo/PDB/Native/DbiStreamBuilder.cpp
  llvm/lib/DebugInfo/PDB/Native/GSIStreamBuilder.cpp
  llvm/lib/DebugInfo/PDB/Native/InfoStreamBuilder.cpp
  llvm/lib/DebugInfo/PDB/Native/NativeCompilandSymbol.cpp
  llvm/lib/DebugInfo/PDB/Native/NativeEnumGlobals.cpp
  llvm/lib/DebugInfo/PDB/Native/NativeEnumLineNumbers.cpp
  llvm/lib/DebugInfo/PDB/Native/NativeEnumModules.cpp
  llvm/lib/DebugInfo/PDB/Native/NativeEnumSymbols.cpp
  llvm/lib/DebugInfo/PDB/Native/NativeEnumTypes.cpp
  llvm/lib/DebugInfo/PDB/Native/NativeExeSymbol.cpp
  llvm/lib/DebugInfo/PDB/Native/NativeFunctionSymbol.cpp
  llvm/lib/DebugInfo/PDB/Native/NativeInlineSiteSymbol.cpp
  llvm/lib/DebugInfo/PDB/Native/NativeLineNumber.cpp
  llvm/lib/DebugInfo/PDB/Native/NativePublicSymbol.cpp
  llvm/lib/DebugInfo/PDB/Native/NativeRawSymbol.cpp
  llvm/lib/DebugInfo/PDB/Native/NativeSession.cpp
  llvm/lib/DebugInfo/PDB/Native/NativeSourceFile.cpp
  llvm/lib/DebugInfo/PDB/Native/NativeSymbolEnumerator.cpp
  llvm/lib/DebugInfo/PDB/Native/NativeTypeArray.cpp
  llvm/lib/DebugInfo/PDB/Native/NativeTypeEnum.cpp