date:20220821

[Lldb-commits] [PATCH] D132283: [lldb] [Core] Reimplement Communication::ReadThread using MainLoop

2022-08-21 Thread Michał Górny via Phabricator via lldb-commits

mgorny updated this revision to Diff 454331.
mgorny added a comment.

Update `Communication::SynchronizeWithReadThread()`, fixing tests.


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

https://reviews.llvm.org/D132283

Files:
  lldb/include/lldb/Core/Communication.h
  lldb/source/Core/Communication.cpp
  lldb/unittests/Core/CommunicationTest.cpp

Index: lldb/unittests/Core/CommunicationTest.cpp
===
--- lldb/unittests/Core/CommunicationTest.cpp
+++ lldb/unittests/Core/CommunicationTest.cpp
@@ -13,6 +13,9 @@
 #include "llvm/Testing/Support/Error.h"
 #include "gtest/gtest.h"
 
+#include 
+#include 
+#include 
 #include 
 
 #if LLDB_ENABLE_POSIX
@@ -88,6 +91,31 @@
   CommunicationReadTest(/*use_thread=*/true);
 }
 
+TEST(CommunicationTest, StopReadThread) {
+  std::condition_variable finished;
+  std::mutex finished_mutex;
+
+  std::thread test_thread{[&finished]() {
+Pipe pipe;
+ASSERT_THAT_ERROR(pipe.CreateNew(/*child_process_inherit=*/false).ToError(),
+  llvm::Succeeded());
+
+Communication comm("test");
+comm.SetConnection(std::make_unique(
+pipe.ReleaseReadFileDescriptor(), /*owns_fd=*/true));
+comm.SetCloseOnEOF(true);
+
+ASSERT_TRUE(comm.StartReadThread());
+ASSERT_TRUE(comm.StopReadThread());
+finished.notify_all();
+  }};
+
+  // StopReadThread() can hang, so force an external timeout.
+  std::unique_lock lock{finished_mutex};
+  ASSERT_EQ(finished.wait_for(lock, std::chrono::seconds(3)), std::cv_status::no_timeout);
+  test_thread.join();
+}
+
 TEST(CommunicationTest, SynchronizeWhileClosing) {
   // Set up a communication object reading from a pipe.
   Pipe pipe;
Index: lldb/source/Core/Communication.cpp
===
--- lldb/source/Core/Communication.cpp
+++ lldb/source/Core/Communication.cpp
@@ -262,7 +262,9 @@
 
   BroadcastEvent(eBroadcastBitReadThreadShouldExit, nullptr);
 
-  // error = m_read_thread.Cancel();
+  m_io_loop.AddPendingCallback(
+  [](MainLoopBase &loop) { loop.RequestTermination(); });
+  m_io_loop.TriggerPendingCallbacks();
 
   Status error = m_read_thread.Join(nullptr);
   return error.Success();
@@ -336,54 +338,65 @@
 
   LLDB_LOG(log, "Communication({0}) thread starting...", this);
 
-  uint8_t buf[1024];
-
-  Status error;
-  ConnectionStatus status = eConnectionStatusSuccess;
-  bool done = false;
   bool disconnect = false;
-  while (!done && m_read_thread_enabled) {
-size_t bytes_read = ReadFromConnection(
-buf, sizeof(buf), std::chrono::seconds(5), status, &error);
-if (bytes_read > 0 || status == eConnectionStatusEndOfFile)
-  AppendBytesToCache(buf, bytes_read, true, status);
-
-switch (status) {
-case eConnectionStatusSuccess:
-  break;
-
-case eConnectionStatusEndOfFile:
-  done = true;
-  disconnect = GetCloseOnEOF();
-  break;
-case eConnectionStatusError: // Check GetError() for details
-  if (error.GetType() == eErrorTypePOSIX && error.GetError() == EIO) {
-// EIO on a pipe is usually caused by remote shutdown
-disconnect = GetCloseOnEOF();
-done = true;
-  }
-  if (error.Fail())
-LLDB_LOG(log, "error: {0}, status = {1}", error,
- Communication::ConnectionStatusAsString(status));
-  break;
-case eConnectionStatusInterrupted: // Synchronization signal from
-   // SynchronizeWithReadThread()
-  // The connection returns eConnectionStatusInterrupted only when there is
-  // no input pending to be read, so we can signal that.
-  BroadcastEvent(eBroadcastBitNoMorePendingInput);
-  break;
-case eConnectionStatusNoConnection:   // No connection
-case eConnectionStatusLostConnection: // Lost connection while connected to
-  // a valid connection
-  done = true;
-  [[fallthrough]];
-case eConnectionStatusTimedOut: // Request timed out
-  if (error.Fail())
-LLDB_LOG(log, "error: {0}, status = {1}", error,
- Communication::ConnectionStatusAsString(status));
-  break;
-}
+  if (m_connection_sp) {
+Status error;
+auto handle = m_io_loop.RegisterReadObject(
+m_connection_sp->GetReadObject(),
+[this, &disconnect](MainLoopBase &loop) {
+  Log *log = GetLog(LLDBLog::Communication);
+
+  if (!m_read_thread_enabled) {
+loop.RequestTermination();
+return;
+  }
+
+  uint8_t buf[1024];
+  ConnectionStatus status = eConnectionStatusSuccess;
+  Status error;
+  size_t bytes_read = ReadFromConnection(
+  buf, sizeof(buf), std::chrono::seconds(5), status, &error);
+  if (bytes_read > 0 || status == eConnectionStatusEndOfFile)
+AppendBytesToCache(buf, bytes_read, true, status);
+
+

[Lldb-commits] [PATCH] D132231: [lldb][ClangExpression] Remove storage-class check when creating AsmLabel

2022-08-21 Thread Michael Buch via Phabricator via lldb-commits

Michael137 updated this revision to Diff 454339.
Michael137 added a comment.

- Combine xfail tests cases


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132231

Files:
  lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
  lldb/test/API/lang/cpp/namespace/TestNamespaceLookup.py

Index: lldb/test/API/lang/cpp/namespace/TestNamespaceLookup.py
===
--- lldb/test/API/lang/cpp/namespace/TestNamespaceLookup.py
+++ lldb/test/API/lang/cpp/namespace/TestNamespaceLookup.py
@@ -36,6 +36,75 @@
 substrs=['stopped',
  'stop reason = breakpoint'])
 
+@skipIfWindows # This is flakey on Windows: llvm.org/pr38373
+@expectedFailure("CU-local objects incorrectly scoped")
+def test_scope_lookup_with_run_command_globals(self):
+"""Test scope lookup of functions in lldb."""
+self.build()
+
+lldbutil.run_to_source_breakpoint(
+self,
+self.line_break_global_scope,
+lldb.SBFileSpec("ns.cpp"))
+
+lldbutil.run_break_set_by_file_and_line(
+self,
+"ns2.cpp",
+self.line_break_file_scope,
+num_expected_locations=1,
+loc_exact=False)
+
+lldbutil.run_break_set_by_file_and_line(
+self,
+"ns3.cpp",
+self.line_break_before_using_directive,
+num_expected_locations=1,
+loc_exact=False)
+
+lldbutil.run_break_set_by_file_and_line(
+self,
+"ns3.cpp",
+self.line_break_after_using_directive,
+num_expected_locations=1,
+loc_exact=False)
+
+# Run to BP_global_scope at file scope
+self.runToBkpt("run")
+
+# FIXME: LLDB does not correctly scope CU-local objects.
+# LLDB currently lumps functions from all files into
+# a single AST and depending on the order with which
+# functions are considered, LLDB can incorrectly call
+# the static local ns.cpp::func() instead of the expected
+# ::func()
+
+# Evaluate func() - should call ::func()
+self.expect_expr("func()", expect_type="int", expect_value="1")
+
+# Evaluate ::func() - should call A::func()
+self.expect_expr("::func()", result_type="int", result_value="1")
+
+# Run to BP_file_scope at file scope
+self.runToBkpt("continue")
+
+# Evaluate func() - should call static ns2.cpp:func()
+self.expect_expr("func()", result_type="int", result_value="2")
+
+# Run to BP_before_using_directive at file scope
+self.runToBkpt("continue")
+
+# Evaluate func() - should call ::func()
+self.expect_expr("func()", result_type="int", result_value="1")
+
+# Evaluate ::func() - should call ::func()
+self.expect_expr("::func()", result_type="int", result_value="1")
+
+# Run to BP_after_using_directive at file scope
+self.runToBkpt("continue")
+
+# Evaluate ::func() - should call ::func()
+self.expect_expr("::func()", result_type="int", result_value="1")
+
 @skipIfWindows # This is flakey on Windows: llvm.org/pr38373
 def test_scope_lookup_with_run_command(self):
 """Test scope lookup of functions in lldb."""
@@ -81,14 +150,11 @@
 
 # Run to BP_global_scope at global scope
 self.runToBkpt("run")
-# Evaluate func() - should call ::func()
-self.expect_expr("func()", result_type="int", result_value="1")
+
 # Evaluate A::B::func() - should call A::B::func()
 self.expect_expr("A::B::func()", result_type="int", result_value="4")
 # Evaluate func(10) - should call ::func(int)
 self.expect_expr("func(10)", result_type="int", result_value="11")
-# Evaluate ::func() - should call A::func()
-self.expect_expr("::func()", result_type="int", result_value="1")
 # Evaluate A::foo() - should call A::foo()
 self.expect_expr("A::foo()", result_type="int", result_value="42")
 
@@ -124,16 +190,12 @@
 # Continue to BP_before_using_directive at global scope before using
 # declaration
 self.runToBkpt("continue")
-# Evaluate ::func() - should call ::func()
-self.expect_expr("::func()", result_type="int", result_value="1")
 # Evaluate B::func() - should call B::func()
 self.expect_expr("B::func()", result_type="int", result_value="4")
 
 # Continue to BP_after_using_directive at global scope after using
 # declaration
 self.runToBkpt("continue")
-# Evaluate ::func() - should call ::func()
-self.expect_expr("::func()", result_type="int", result_value="1")
 # Evaluate B::func() - should call B::func()
 self.expect_expr("B::func()", result_type

[Lldb-commits] [PATCH] D132231: [lldb][ClangExpression] Remove storage-class check when creating AsmLabel

2022-08-21 Thread Michael Buch via Phabricator via lldb-commits

Michael137 updated this revision to Diff 454340.
Michael137 added a comment.

- Remove redundant breakpoint


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132231

Files:
  lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
  lldb/test/API/lang/cpp/namespace/TestNamespaceLookup.py

Index: lldb/test/API/lang/cpp/namespace/TestNamespaceLookup.py
===
--- lldb/test/API/lang/cpp/namespace/TestNamespaceLookup.py
+++ lldb/test/API/lang/cpp/namespace/TestNamespaceLookup.py
@@ -36,6 +36,68 @@
 substrs=['stopped',
  'stop reason = breakpoint'])
 
+@skipIfWindows # This is flakey on Windows: llvm.org/pr38373
+@expectedFailure("CU-local objects incorrectly scoped")
+def test_scope_lookup_with_run_command_globals(self):
+"""Test scope lookup of functions in lldb."""
+self.build()
+
+lldbutil.run_to_source_breakpoint(
+self,
+self.line_break_global_scope,
+lldb.SBFileSpec("ns.cpp"))
+
+lldbutil.run_break_set_by_file_and_line(
+self,
+"ns2.cpp",
+self.line_break_file_scope,
+num_expected_locations=1,
+loc_exact=False)
+
+lldbutil.run_break_set_by_file_and_line(
+self,
+"ns3.cpp",
+self.line_break_before_using_directive,
+num_expected_locations=1,
+loc_exact=False)
+
+# Run to BP_global_scope at file scope
+self.runToBkpt("run")
+
+# FIXME: LLDB does not correctly scope CU-local objects.
+# LLDB currently lumps functions from all files into
+# a single AST and depending on the order with which
+# functions are considered, LLDB can incorrectly call
+# the static local ns.cpp::func() instead of the expected
+# ::func()
+
+# Evaluate func() - should call ::func()
+self.expect_expr("func()", expect_type="int", expect_value="1")
+
+# Evaluate ::func() - should call A::func()
+self.expect_expr("::func()", result_type="int", result_value="1")
+
+# Run to BP_file_scope at file scope
+self.runToBkpt("continue")
+
+# Evaluate func() - should call static ns2.cpp:func()
+self.expect_expr("func()", result_type="int", result_value="2")
+
+# Run to BP_before_using_directive at file scope
+self.runToBkpt("continue")
+
+# Evaluate func() - should call ::func()
+self.expect_expr("func()", result_type="int", result_value="1")
+
+# Evaluate ::func() - should call ::func()
+self.expect_expr("::func()", result_type="int", result_value="1")
+
+# Run to BP_after_using_directive at file scope
+self.runToBkpt("continue")
+
+# Evaluate ::func() - should call ::func()
+self.expect_expr("::func()", result_type="int", result_value="1")
+
 @skipIfWindows # This is flakey on Windows: llvm.org/pr38373
 def test_scope_lookup_with_run_command(self):
 """Test scope lookup of functions in lldb."""
@@ -81,14 +143,11 @@
 
 # Run to BP_global_scope at global scope
 self.runToBkpt("run")
-# Evaluate func() - should call ::func()
-self.expect_expr("func()", result_type="int", result_value="1")
+
 # Evaluate A::B::func() - should call A::B::func()
 self.expect_expr("A::B::func()", result_type="int", result_value="4")
 # Evaluate func(10) - should call ::func(int)
 self.expect_expr("func(10)", result_type="int", result_value="11")
-# Evaluate ::func() - should call A::func()
-self.expect_expr("::func()", result_type="int", result_value="1")
 # Evaluate A::foo() - should call A::foo()
 self.expect_expr("A::foo()", result_type="int", result_value="42")
 
@@ -124,16 +183,12 @@
 # Continue to BP_before_using_directive at global scope before using
 # declaration
 self.runToBkpt("continue")
-# Evaluate ::func() - should call ::func()
-self.expect_expr("::func()", result_type="int", result_value="1")
 # Evaluate B::func() - should call B::func()
 self.expect_expr("B::func()", result_type="int", result_value="4")
 
 # Continue to BP_after_using_directive at global scope after using
 # declaration
 self.runToBkpt("continue")
-# Evaluate ::func() - should call ::func()
-self.expect_expr("::func()", result_type="int", result_value="1")
 # Evaluate B::func() - should call B::func()
 self.expect_expr("B::func()", result_type="int", result_value="4")
 
@@ -174,46 +229,6 @@
 # before functions.
 self.expect_expr("foo()", result_type="int", result_value="42")
 
-@expectedFailure("lldb file scope lookup bugs")
-@sk

[Lldb-commits] [PATCH] D132283: [lldb] [Core] Reimplement Communication::ReadThread using MainLoop

[Lldb-commits] [PATCH] D132231: [lldb][ClangExpression] Remove storage-class check when creating AsmLabel

[Lldb-commits] [PATCH] D132231: [lldb][ClangExpression] Remove storage-class check when creating AsmLabel

3 matches

Site Navigation

Mail list logo

Footer information