[Lldb-commits] [PATCH] D100965: [lldb] Refactor argument group by SourceLocationSpec (NFCI)

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.



Comment at: lldb/source/API/SBThread.cpp:853
+if (!location_spec) {
+  sb_error.SetErrorString(
+  llvm::toString(location_spec.takeError()).c_str());

mib wrote:
> JDevlieghere wrote:
> > Why not `sb_error = location_spec.takeError()`?
> `takeError` returns an `llvm::Error` not a `lldb_private::Status`, and there 
> is no `SBError` constructor for `Status`  so I don't think that's possible.
There is:
```
  explicit Status(llvm::Error error) { *this = std::move(error); }
  const Status =(llvm::Error error);
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100965

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

[Lldb-commits] [PATCH] D100965: [lldb] Refactor argument group by SourceLocationSpec (NFCI)

[Med Ismail Bennani via Phabricator via lldb-commits]

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



Comment at: lldb/source/API/SBThread.cpp:853
+if (!location_spec) {
+  sb_error.SetErrorString(
+  llvm::toString(location_spec.takeError()).c_str());

JDevlieghere wrote:
> Why not `sb_error = location_spec.takeError()`?
`takeError` returns an `llvm::Error` not a `lldb_private::Status`, and there is 
no `SBError` constructor for `Status`  so I don't think that's possible.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100965

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

[Lldb-commits] [PATCH] D100965: [lldb] Refactor argument group by SourceLocationSpec (NFCI)

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.



Comment at: lldb/source/API/SBThread.cpp:853
+if (!location_spec) {
+  sb_error.SetErrorString(
+  llvm::toString(location_spec.takeError()).c_str());

Why not `sb_error = location_spec.takeError()`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100965

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

[Lldb-commits] [PATCH] D100965: [lldb] Refactor argument group by SourceLocationSpec (NFCI)

[Med Ismail Bennani via Phabricator via lldb-commits]

mib updated this revision to Diff 340211.
mib added a comment.

Fix test failures.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100965

Files:
  lldb/include/lldb/Breakpoint/BreakpointResolverFileLine.h
  lldb/include/lldb/Core/AddressResolverFileLine.h
  lldb/include/lldb/Symbol/CompileUnit.h
  lldb/include/lldb/Symbol/LineTable.h
  lldb/include/lldb/Symbol/SymbolFile.h
  lldb/source/API/SBThread.cpp
  lldb/source/Breakpoint/Breakpoint.cpp
  lldb/source/Breakpoint/BreakpointResolverFileLine.cpp
  lldb/source/Breakpoint/BreakpointResolverFileRegex.cpp
  lldb/source/Core/AddressResolverFileLine.cpp
  lldb/source/Core/Module.cpp
  lldb/source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.cpp
  lldb/source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.h
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.cpp
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.h
  lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
  lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.h
  lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp
  lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.h
  lldb/source/Symbol/CompileUnit.cpp
  lldb/source/Symbol/LineTable.cpp
  lldb/source/Symbol/SymbolFile.cpp
  lldb/source/Target/Target.cpp
  lldb/unittests/Symbol/TestLineEntry.cpp

Index: lldb/unittests/Symbol/TestLineEntry.cpp
===
--- lldb/unittests/Symbol/TestLineEntry.cpp
+++ lldb/unittests/Symbol/TestLineEntry.cpp
@@ -53,8 +53,10 @@
 }
 
 llvm::Expected LineEntryTest::GetLineEntryForLine(uint32_t line) {
-  bool check_inlines = true;
-  bool exact = true;
+  // TODO: Handle SourceLocationSpec column information
+  const llvm::Optional column = llvm::None;
+  const bool check_inlines = true;
+  const bool exact_match = true;
   SymbolContextList sc_comp_units;
   SymbolContextList sc_line_entries;
   FileSpec file_spec("inlined-functions.cpp");
@@ -64,9 +66,14 @@
   if (sc_comp_units.GetSize() == 0)
 return llvm::createStringError(llvm::inconvertibleErrorCode(),
"No comp unit found on the test object.");
+
+  auto location_spec = SourceLocationSpec::Create(file_spec, line, column,
+  check_inlines, exact_match);
+  if (!location_spec)
+return location_spec.takeError();
+
   sc_comp_units[0].comp_unit->ResolveSymbolContext(
-  file_spec, line, check_inlines, exact, eSymbolContextLineEntry,
-  sc_line_entries);
+  *location_spec, eSymbolContextLineEntry, sc_line_entries);
   if (sc_line_entries.GetSize() == 0)
 return llvm::createStringError(llvm::inconvertibleErrorCode(),
"No line entry found on the test object.");
Index: lldb/source/Target/Target.cpp
===
--- lldb/source/Target/Target.cpp
+++ lldb/source/Target/Target.cpp
@@ -371,9 +371,16 @@
   if (move_to_nearest_code == eLazyBoolCalculate)
 move_to_nearest_code = GetMoveToNearestCode() ? eLazyBoolYes : eLazyBoolNo;
 
+  auto location_spec =
+  SourceLocationSpec::Create(remapped_file, line_no, column, check_inlines,
+ !static_cast(move_to_nearest_code));
+  if (!location_spec) {
+llvm::errs() << llvm::toString(location_spec.takeError()) << "\n";
+return nullptr;
+  }
+
   BreakpointResolverSP resolver_sp(new BreakpointResolverFileLine(
-  nullptr, remapped_file, line_no, column, offset, check_inlines,
-  skip_prologue, !static_cast(move_to_nearest_code)));
+  nullptr, offset, skip_prologue, *location_spec));
   return CreateBreakpoint(filter_sp, resolver_sp, internal, hardware, true);
 }
 
Index: lldb/source/Symbol/SymbolFile.cpp
===
--- lldb/source/Symbol/SymbolFile.cpp
+++ lldb/source/Symbol/SymbolFile.cpp
@@ -97,10 +97,10 @@
   return type_system_or_err;
 }
 
-uint32_t SymbolFile::ResolveSymbolContext(const FileSpec _spec,
-  uint32_t line, bool check_inlines,
-  lldb::SymbolContextItem resolve_scope,
-  SymbolContextList _list) {
+uint32_t
+SymbolFile::ResolveSymbolContext(const SourceLocationSpec _location_spec,
+ lldb::SymbolContextItem resolve_scope,
+ SymbolContextList _list) {
   return 0;
 }
 
Index: lldb/source/Symbol/LineTable.cpp
===
--- lldb/source/Symbol/LineTable.cpp
+++ lldb/source/Symbol/LineTable.cpp
@@ -304,7 +304,7 @@
 
 uint32_t LineTable::FindLineEntryIndexByFileIndex(
 

[Lldb-commits] [PATCH] D100962: [lldb/Utility] Add SourceLocationSpec class (NFC)

[Med Ismail Bennani via Phabricator via lldb-commits]

mib updated this revision to Diff 340210.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100962

Files:
  lldb/include/lldb/Utility/SourceLocationSpec.h
  lldb/source/Utility/CMakeLists.txt
  lldb/source/Utility/SourceLocationSpec.cpp
  lldb/unittests/Utility/CMakeLists.txt
  lldb/unittests/Utility/SourceLocationSpecTest.cpp

Index: lldb/unittests/Utility/SourceLocationSpecTest.cpp
===
--- /dev/null
+++ lldb/unittests/Utility/SourceLocationSpecTest.cpp
@@ -0,0 +1,209 @@
+//===-- SourceLocationSpecTest.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 "gtest/gtest.h"
+
+#include "lldb/Utility/LLDBAssert.h"
+#include "lldb/Utility/SourceLocationSpec.h"
+
+#include "llvm/Testing/Support/Error.h"
+
+using namespace lldb_private;
+
+TEST(SourceLocationSpecTest, CreateSourceLocationSpec) {
+  FileSpec fs("/foo/bar", FileSpec::Style::posix);
+  const uint32_t line = 19;
+  const uint32_t column = 4;
+  const bool check_inlines = false;
+  const bool exact_match = true;
+
+  // Invalid FileSpec
+  ASSERT_THAT_EXPECTED(
+  SourceLocationSpec::Create(FileSpec(), line, check_inlines, exact_match),
+  llvm::Failed());
+  // Invalid Line
+  ASSERT_THAT_EXPECTED(SourceLocationSpec::Create(fs, LLDB_INVALID_LINE_NUMBER,
+  check_inlines, exact_match),
+   llvm::Failed());
+  // Invalid FileSpec & Line
+  ASSERT_THAT_EXPECTED(SourceLocationSpec::Create(FileSpec(),
+  LLDB_INVALID_LINE_NUMBER,
+  check_inlines, exact_match),
+   llvm::Failed());
+
+  // Null Line
+  ASSERT_THAT_EXPECTED(
+  SourceLocationSpec::Create(fs, 0, check_inlines, exact_match),
+  llvm::Succeeded());
+  // Valid FileSpec & Line
+  ASSERT_THAT_EXPECTED(
+  SourceLocationSpec::Create(fs, line, check_inlines, exact_match),
+  llvm::Succeeded());
+  // Valid FileSpec, Line & Column
+  ASSERT_THAT_EXPECTED(
+  SourceLocationSpec::Create(fs, line, column, check_inlines, exact_match),
+  llvm::Succeeded());
+}
+
+TEST(SourceLocationSpecTest, FileLineColumnComponents) {
+  FileSpec fs("/foo/bar", FileSpec::Style::posix);
+  const uint32_t line = 19;
+  const uint16_t column = 4;
+
+  auto location_spec = SourceLocationSpec::Create(
+  fs, line, LLDB_INVALID_COLUMN_NUMBER, false, true);
+  ASSERT_THAT_EXPECTED(location_spec, llvm::Succeeded());
+  SourceLocationSpec without_column = *location_spec;
+  EXPECT_EQ(fs, without_column.GetFileSpec());
+  EXPECT_EQ(line, without_column.GetLine());
+  EXPECT_EQ(llvm::None, without_column.GetColumn());
+  EXPECT_FALSE(without_column.GetCheckInlines());
+  EXPECT_TRUE(without_column.GetExactMatch());
+  EXPECT_STREQ("/foo/bar:19", without_column.GetCString());
+
+  location_spec = SourceLocationSpec::Create(fs, line, column, true, false);
+  ASSERT_THAT_EXPECTED(location_spec, llvm::Succeeded());
+  SourceLocationSpec with_column = *location_spec;
+  EXPECT_EQ(column, *with_column.GetColumn());
+  EXPECT_TRUE(with_column.GetCheckInlines());
+  EXPECT_FALSE(with_column.GetExactMatch());
+  EXPECT_STREQ("/foo/bar:19:4", with_column.GetCString());
+}
+
+static SourceLocationSpec Create(bool check_inlines, bool exact_match,
+ FileSpec fs, uint32_t line,
+ uint16_t column = LLDB_INVALID_COLUMN_NUMBER) {
+  auto location_spec =
+  SourceLocationSpec::Create(fs, line, column, check_inlines, exact_match);
+  if (!location_spec) {
+llvm::errs() << "Invalid SourceLocationSpec.\n";
+llvm::cantFail(location_spec.takeError());
+  }
+
+  return *location_spec;
+}
+
+TEST(SourceLocationSpecTest, Equal) {
+  auto Equal = [](SourceLocationSpec lhs, SourceLocationSpec rhs, bool full) {
+return SourceLocationSpec::Equal(lhs, rhs, full);
+  };
+
+  const FileSpec fs("/foo/bar", FileSpec::Style::posix);
+  const FileSpec other_fs("/foo/baz", FileSpec::Style::posix);
+
+  // mutating FileSpec + const Inlined, ExactMatch, Line
+  EXPECT_TRUE(
+  Equal(Create(false, false, fs, 4), Create(false, false, fs, 4), true));
+  EXPECT_TRUE(
+  Equal(Create(true, true, fs, 4), Create(true, true, fs, 4), false));
+  EXPECT_FALSE(Equal(Create(false, false, fs, 4),
+ Create(false, false, other_fs, 4), true));
+  EXPECT_FALSE(
+  Equal(Create(true, true, fs, 4), Create(true, true, other_fs, 4), false));
+
+  // Mutating FileSpec + const Inlined, ExactMatch, Line, Column
+  

[Lldb-commits] [PATCH] D100962: [lldb/Utility] Add SourceLocationSpec class (NFC)

[Med Ismail Bennani via Phabricator via lldb-commits]

mib updated this revision to Diff 340202.
mib added a comment.

Fix unit test failure.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100962

Files:
  lldb/include/lldb/Utility/SourceLocationSpec.h
  lldb/source/Utility/CMakeLists.txt
  lldb/source/Utility/SourceLocationSpec.cpp
  lldb/unittests/Utility/CMakeLists.txt
  lldb/unittests/Utility/SourceLocationSpecTest.cpp

Index: lldb/unittests/Utility/SourceLocationSpecTest.cpp
===
--- /dev/null
+++ lldb/unittests/Utility/SourceLocationSpecTest.cpp
@@ -0,0 +1,209 @@
+//===-- SourceLocationSpecTest.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 "gtest/gtest.h"
+
+#include "lldb/Utility/LLDBAssert.h"
+#include "lldb/Utility/SourceLocationSpec.h"
+
+#include "llvm/Testing/Support/Error.h"
+
+using namespace lldb_private;
+
+TEST(SourceLocationSpecTest, CreateSourceLocationSpec) {
+  FileSpec fs("/foo/bar", FileSpec::Style::posix);
+  const uint32_t line = 19;
+  const uint32_t column = 4;
+  const bool check_inlines = false;
+  const bool exact_match = true;
+
+  // Invalid FileSpec
+  ASSERT_THAT_EXPECTED(
+  SourceLocationSpec::Create(FileSpec(), line, check_inlines, exact_match),
+  llvm::Failed());
+  // Invalid Line
+  ASSERT_THAT_EXPECTED(SourceLocationSpec::Create(fs, LLDB_INVALID_LINE_NUMBER,
+  check_inlines, exact_match),
+   llvm::Failed());
+  // Invalid FileSpec & Line
+  ASSERT_THAT_EXPECTED(SourceLocationSpec::Create(FileSpec(),
+  LLDB_INVALID_LINE_NUMBER,
+  check_inlines, exact_match),
+   llvm::Failed());
+
+  // Null Line
+  ASSERT_THAT_EXPECTED(
+  SourceLocationSpec::Create(fs, 0, check_inlines, exact_match),
+  llvm::Succeeded());
+  // Valid FileSpec & Line
+  ASSERT_THAT_EXPECTED(
+  SourceLocationSpec::Create(fs, line, check_inlines, exact_match),
+  llvm::Succeeded());
+  // Valid FileSpec, Line & Column
+  ASSERT_THAT_EXPECTED(
+  SourceLocationSpec::Create(fs, line, column, check_inlines, exact_match),
+  llvm::Succeeded());
+}
+
+TEST(SourceLocationSpecTest, FileLineColumnComponents) {
+  FileSpec fs("/foo/bar", FileSpec::Style::posix);
+  const uint32_t line = 19;
+  const uint16_t column = 4;
+
+  auto location_spec = SourceLocationSpec::Create(
+  fs, line, LLDB_INVALID_COLUMN_NUMBER, false, true);
+  ASSERT_THAT_EXPECTED(location_spec, llvm::Succeeded());
+  SourceLocationSpec without_column = *location_spec;
+  EXPECT_EQ(fs, without_column.GetFileSpec());
+  EXPECT_EQ(line, without_column.GetLine());
+  EXPECT_EQ(llvm::None, without_column.GetColumn());
+  EXPECT_FALSE(without_column.GetCheckInlines());
+  EXPECT_TRUE(without_column.GetExactMatch());
+  EXPECT_STREQ("/foo/bar:19", without_column.GetCString());
+
+  location_spec = SourceLocationSpec::Create(fs, line, column, true, false);
+  ASSERT_THAT_EXPECTED(location_spec, llvm::Succeeded());
+  SourceLocationSpec with_column = *location_spec;
+  EXPECT_EQ(column, *with_column.GetColumn());
+  EXPECT_TRUE(with_column.GetCheckInlines());
+  EXPECT_FALSE(with_column.GetExactMatch());
+  EXPECT_STREQ("/foo/bar:19:4", with_column.GetCString());
+}
+
+static SourceLocationSpec Create(bool check_inlines, bool exact_match,
+ FileSpec fs, uint32_t line,
+ uint16_t column = LLDB_INVALID_COLUMN_NUMBER) {
+  auto location_spec =
+  SourceLocationSpec::Create(fs, line, column, check_inlines, exact_match);
+  if (!location_spec) {
+llvm::errs() << "Invalid SourceLocationSpec.\n";
+llvm::cantFail(location_spec.takeError());
+  }
+
+  return *location_spec;
+}
+
+TEST(SourceLocationSpecTest, Equal) {
+  auto Equal = [](SourceLocationSpec lhs, SourceLocationSpec rhs, bool full) {
+return SourceLocationSpec::Equal(lhs, rhs, full);
+  };
+
+  const FileSpec fs("/foo/bar", FileSpec::Style::posix);
+  const FileSpec other_fs("/foo/baz", FileSpec::Style::posix);
+
+  // mutating FileSpec + const Inlined, ExactMatch, Line
+  EXPECT_TRUE(
+  Equal(Create(false, false, fs, 4), Create(false, false, fs, 4), true));
+  EXPECT_TRUE(
+  Equal(Create(true, true, fs, 4), Create(true, true, fs, 4), false));
+  EXPECT_FALSE(Equal(Create(false, false, fs, 4),
+ Create(false, false, other_fs, 4), true));
+  EXPECT_FALSE(
+  Equal(Create(true, true, fs, 4), Create(true, true, other_fs, 4), false));
+
+  // Mutating FileSpec + 

[Lldb-commits] [PATCH] D100208: [lldb] [Process/Linux] Report fork/vfork stop reason

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

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

Add `multiprocess` to supported features.

Tests were moved to earlier commit. Now this commit only enables the relevant 
category.


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

https://reviews.llvm.org/D100208

Files:
  lldb/packages/Python/lldbsuite/test/dotest.py
  lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp
  lldb/source/Plugins/Process/Linux/NativeProcessLinux.h
  lldb/source/Plugins/Process/Linux/NativeThreadLinux.cpp
  lldb/source/Plugins/Process/Linux/NativeThreadLinux.h

Index: lldb/source/Plugins/Process/Linux/NativeThreadLinux.h
===
--- lldb/source/Plugins/Process/Linux/NativeThreadLinux.h
+++ lldb/source/Plugins/Process/Linux/NativeThreadLinux.h
@@ -85,6 +85,10 @@
 
   void SetStoppedByTrace();
 
+  void SetStoppedByFork(bool is_vfork, lldb::pid_t child_pid);
+
+  void SetStoppedByVForkDone();
+
   void SetStoppedWithNoReason();
 
   void SetStoppedByProcessorTrace(llvm::StringRef description);
Index: lldb/source/Plugins/Process/Linux/NativeThreadLinux.cpp
===
--- lldb/source/Plugins/Process/Linux/NativeThreadLinux.cpp
+++ lldb/source/Plugins/Process/Linux/NativeThreadLinux.cpp
@@ -394,6 +394,21 @@
   m_stop_info.details.signal.signo = SIGTRAP;
 }
 
+void NativeThreadLinux::SetStoppedByFork(bool is_vfork, lldb::pid_t child_pid) {
+  SetStopped();
+
+  m_stop_info.reason =
+  is_vfork ? StopReason::eStopReasonVFork : StopReason::eStopReasonFork;
+  m_stop_info.details.fork.child_pid = child_pid;
+  m_stop_info.details.fork.child_tid = child_pid;
+}
+
+void NativeThreadLinux::SetStoppedByVForkDone() {
+  SetStopped();
+
+  m_stop_info.reason = StopReason::eStopReasonVForkDone;
+}
+
 void NativeThreadLinux::SetStoppedWithNoReason() {
   SetStopped();
 
Index: lldb/source/Plugins/Process/Linux/NativeProcessLinux.h
===
--- lldb/source/Plugins/Process/Linux/NativeProcessLinux.h
+++ lldb/source/Plugins/Process/Linux/NativeProcessLinux.h
@@ -49,6 +49,8 @@
 llvm::Expected>
 Attach(lldb::pid_t pid, NativeDelegate _delegate,
MainLoop ) const override;
+
+Extension GetSupportedExtensions() const override;
   };
 
   // NativeProcessProtocol Interface
@@ -136,6 +138,7 @@
 private:
   MainLoop::SignalHandleUP m_sigchld_handle;
   ArchSpec m_arch;
+  MainLoop& m_main_loop;
 
   LazyBool m_supports_mem_region = eLazyBoolCalculate;
   std::vector> m_mem_region_cache;
Index: lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp
===
--- lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp
+++ lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp
@@ -281,6 +281,11 @@
   pid, -1, native_delegate, Info.GetArchitecture(), mainloop, *tids_or));
 }
 
+NativeProcessLinux::Extension
+NativeProcessLinux::Factory::GetSupportedExtensions() const {
+  return Extension::multiprocess | Extension::fork | Extension::vfork;
+}
+
 // Public Instance Methods
 
 NativeProcessLinux::NativeProcessLinux(::pid_t pid, int terminal_fd,
@@ -288,7 +293,7 @@
const ArchSpec , MainLoop ,
llvm::ArrayRef<::pid_t> tids)
 : NativeProcessELF(pid, terminal_fd, delegate), m_arch(arch),
-  m_intel_pt_manager(pid) {
+  m_main_loop(mainloop), m_intel_pt_manager(pid) {
   if (m_terminal_fd != -1) {
 Status status = EnsureFDFlags(m_terminal_fd, O_NONBLOCK);
 assert(status.Success());
@@ -647,7 +652,12 @@
   }
 
   case (SIGTRAP | (PTRACE_EVENT_VFORK_DONE << 8)): {
-ResumeThread(thread, thread.GetState(), LLDB_INVALID_SIGNAL_NUMBER);
+if (bool(m_enabled_extensions & Extension::vfork)) {
+  thread.SetStoppedByVForkDone();
+  StopRunningThreads(thread.GetID());
+}
+else
+  ResumeThread(thread, thread.GetState(), LLDB_INVALID_SIGNAL_NUMBER);
 break;
   }
 
@@ -912,16 +922,24 @@
 LLVM_FALLTHROUGH;
   case PTRACE_EVENT_FORK:
   case PTRACE_EVENT_VFORK: {
-MainLoop unused_loop;
-NativeProcessLinux child_process{static_cast<::pid_t>(child_pid),
- m_terminal_fd,
- m_delegate,
- m_arch,
- unused_loop,
- {static_cast<::pid_t>(child_pid)}};
-child_process.Detach();
-ResumeThread(*parent_thread, parent_thread->GetState(),
- LLDB_INVALID_SIGNAL_NUMBER);
+bool is_vfork = clone_info->event == PTRACE_EVENT_VFORK;
+std::unique_ptr child_process{new NativeProcessLinux(
+static_cast<::pid_t>(child_pid), m_terminal_fd, m_delegate, m_arch,
+m_main_loop, {static_cast<::pid_t>(child_pid)})};
+if (!is_vfork)
+ 

[Lldb-commits] [PATCH] D100191: [lldb] [llgs] Support owning and detaching extra processes

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

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

Fix tests to include `multiprocess+` in `qSupported`.


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

https://reviews.llvm.org/D100191

Files:
  lldb/include/lldb/Host/common/NativeProcessProtocol.h
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h
  lldb/source/Utility/StringExtractorGDBRemote.cpp
  lldb/test/API/tools/lldb-server/TestGdbRemoteFork.py
  lldb/test/API/tools/lldb-server/main.cpp
  lldb/unittests/TestingSupport/Host/NativeProcessTestUtils.h

Index: lldb/unittests/TestingSupport/Host/NativeProcessTestUtils.h
===
--- lldb/unittests/TestingSupport/Host/NativeProcessTestUtils.h
+++ lldb/unittests/TestingSupport/Host/NativeProcessTestUtils.h
@@ -25,6 +25,15 @@
   MOCK_METHOD2(ProcessStateChanged,
void(NativeProcessProtocol *Process, StateType State));
   MOCK_METHOD1(DidExec, void(NativeProcessProtocol *Process));
+  MOCK_METHOD2(NewSubprocessImpl,
+   void(NativeProcessProtocol *parent_process,
+std::unique_ptr _process));
+  // This is a hack to avoid MOCK_METHOD2 incompatibility with std::unique_ptr
+  // passed as value.
+  void NewSubprocess(NativeProcessProtocol *parent_process,
+ std::unique_ptr child_process) {
+NewSubprocessImpl(parent_process, child_process);
+  }
 };
 
 // NB: This class doesn't use the override keyword to avoid
Index: lldb/test/API/tools/lldb-server/main.cpp
===
--- lldb/test/API/tools/lldb-server/main.cpp
+++ lldb/test/API/tools/lldb-server/main.cpp
@@ -216,7 +216,13 @@
 
   sig_result = signal(SIGSEGV, signal_handler);
   if (sig_result == SIG_ERR) {
-fprintf(stderr, "failed to set SIGUSR1 handler: errno=%d\n", errno);
+fprintf(stderr, "failed to set SIGSEGV handler: errno=%d\n", errno);
+exit(1);
+  }
+
+  sig_result = signal(SIGCHLD, SIG_IGN);
+  if (sig_result == SIG_ERR) {
+fprintf(stderr, "failed to set SIGCHLD handler: errno=%d\n", errno);
 exit(1);
   }
 #endif
@@ -293,6 +299,14 @@
   else if (arg == "swap_chars")
 func_p = swap_chars;
   func_p();
+#if !defined(_WIN32)
+} else if (arg == "fork") {
+  if (fork() == 0)
+_exit(0);
+} else if (arg == "vfork") {
+  if (vfork() == 0)
+_exit(0);
+#endif
 } else if (consume_front(arg, "thread:new")) {
 threads.push_back(std::thread(thread_func, nullptr));
 } else if (consume_front(arg, "thread:print-ids")) {
Index: lldb/test/API/tools/lldb-server/TestGdbRemoteFork.py
===
--- /dev/null
+++ lldb/test/API/tools/lldb-server/TestGdbRemoteFork.py
@@ -0,0 +1,59 @@
+import gdbremote_testcase
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+class TestGdbRemoteFork(gdbremote_testcase.GdbRemoteTestCaseBase):
+mydir = TestBase.compute_mydir(__file__)
+
+def fork_and_detach_test(self, variant):
+self.build()
+self.prep_debug_monitor_and_inferior(inferior_args=[variant])
+self.add_qSupported_packets(["multiprocess+",
+ "{}-events+".format(variant)])
+ret = self.expect_gdbremote_sequence()
+self.assertIn("{}-events+".format(variant), ret["qSupported_response"])
+self.reset_test_sequence()
+
+# continue and expect fork
+fork_regex = "[$]T.*;{}:p([0-9a-f]*)[.]([0-9a-f]*).*".format(variant)
+self.test_sequence.add_log_lines([
+"read packet: $c#00",
+{"direction": "send", "regex": fork_regex,
+ "capture": {1: "pid", 2: "tid"}},
+], True)
+ret = self.expect_gdbremote_sequence()
+pid = int(ret["pid"], 16)
+self.reset_test_sequence()
+
+# detach the forked child
+self.test_sequence.add_log_lines([
+"read packet: $D;{:x}#00".format(pid),
+{"direction": "send", "regex": r"[$]OK#.*"},
+], True)
+ret = self.expect_gdbremote_sequence()
+self.reset_test_sequence()
+
+@add_test_categories(["fork"])
+def test_fork(self):
+self.fork_and_detach_test("fork")
+
+# resume the parent
+self.test_sequence.add_log_lines([
+"read packet: $c#00",
+{"direction": "send", "regex": r"[$]W00#.*"},
+], True)
+self.expect_gdbremote_sequence()
+
+@add_test_categories(["fork"])
+def test_vfork(self):
+self.fork_and_detach_test("vfork")
+
+# resume the parent
+self.test_sequence.add_log_lines([
+"read packet: $c#00",
+{"direction": "send", "regex": r"[$]T.*vforkdone.*"},
+"read 

[Lldb-commits] [PATCH] D100191: [lldb] [llgs] Support owning and detaching extra processes

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

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

Add the comment for mock stuff.

Move tests from the later patch here and skip them based on category.


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

https://reviews.llvm.org/D100191

Files:
  lldb/include/lldb/Host/common/NativeProcessProtocol.h
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h
  lldb/source/Utility/StringExtractorGDBRemote.cpp
  lldb/test/API/tools/lldb-server/TestGdbRemoteFork.py
  lldb/test/API/tools/lldb-server/main.cpp
  lldb/unittests/TestingSupport/Host/NativeProcessTestUtils.h

Index: lldb/unittests/TestingSupport/Host/NativeProcessTestUtils.h
===
--- lldb/unittests/TestingSupport/Host/NativeProcessTestUtils.h
+++ lldb/unittests/TestingSupport/Host/NativeProcessTestUtils.h
@@ -25,6 +25,15 @@
   MOCK_METHOD2(ProcessStateChanged,
void(NativeProcessProtocol *Process, StateType State));
   MOCK_METHOD1(DidExec, void(NativeProcessProtocol *Process));
+  MOCK_METHOD2(NewSubprocessImpl,
+   void(NativeProcessProtocol *parent_process,
+std::unique_ptr _process));
+  // This is a hack to avoid MOCK_METHOD2 incompatibility with std::unique_ptr
+  // passed as value.
+  void NewSubprocess(NativeProcessProtocol *parent_process,
+ std::unique_ptr child_process) {
+NewSubprocessImpl(parent_process, child_process);
+  }
 };
 
 // NB: This class doesn't use the override keyword to avoid
Index: lldb/test/API/tools/lldb-server/main.cpp
===
--- lldb/test/API/tools/lldb-server/main.cpp
+++ lldb/test/API/tools/lldb-server/main.cpp
@@ -216,7 +216,13 @@
 
   sig_result = signal(SIGSEGV, signal_handler);
   if (sig_result == SIG_ERR) {
-fprintf(stderr, "failed to set SIGUSR1 handler: errno=%d\n", errno);
+fprintf(stderr, "failed to set SIGSEGV handler: errno=%d\n", errno);
+exit(1);
+  }
+
+  sig_result = signal(SIGCHLD, SIG_IGN);
+  if (sig_result == SIG_ERR) {
+fprintf(stderr, "failed to set SIGCHLD handler: errno=%d\n", errno);
 exit(1);
   }
 #endif
@@ -293,6 +299,14 @@
   else if (arg == "swap_chars")
 func_p = swap_chars;
   func_p();
+#if !defined(_WIN32)
+} else if (arg == "fork") {
+  if (fork() == 0)
+_exit(0);
+} else if (arg == "vfork") {
+  if (vfork() == 0)
+_exit(0);
+#endif
 } else if (consume_front(arg, "thread:new")) {
 threads.push_back(std::thread(thread_func, nullptr));
 } else if (consume_front(arg, "thread:print-ids")) {
Index: lldb/test/API/tools/lldb-server/TestGdbRemoteFork.py
===
--- /dev/null
+++ lldb/test/API/tools/lldb-server/TestGdbRemoteFork.py
@@ -0,0 +1,58 @@
+import gdbremote_testcase
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+class TestGdbRemoteFork(gdbremote_testcase.GdbRemoteTestCaseBase):
+mydir = TestBase.compute_mydir(__file__)
+
+def fork_and_detach_test(self, variant):
+self.build()
+self.prep_debug_monitor_and_inferior(inferior_args=[variant])
+self.add_qSupported_packets(["{}-events+".format(variant)])
+ret = self.expect_gdbremote_sequence()
+self.assertIn("{}-events+".format(variant), ret["qSupported_response"])
+self.reset_test_sequence()
+
+# continue and expect fork
+fork_regex = "[$]T.*;{}:p([0-9a-f]*)[.]([0-9a-f]*).*".format(variant)
+self.test_sequence.add_log_lines([
+"read packet: $c#00",
+{"direction": "send", "regex": fork_regex,
+ "capture": {1: "pid", 2: "tid"}},
+], True)
+ret = self.expect_gdbremote_sequence()
+pid = int(ret["pid"], 16)
+self.reset_test_sequence()
+
+# detach the forked child
+self.test_sequence.add_log_lines([
+"read packet: $D;{:x}#00".format(pid),
+{"direction": "send", "regex": r"[$]OK#.*"},
+], True)
+ret = self.expect_gdbremote_sequence()
+self.reset_test_sequence()
+
+@add_test_categories(["fork"])
+def test_fork(self):
+self.fork_and_detach_test("fork")
+
+# resume the parent
+self.test_sequence.add_log_lines([
+"read packet: $c#00",
+{"direction": "send", "regex": r"[$]W00#.*"},
+], True)
+self.expect_gdbremote_sequence()
+
+@add_test_categories(["fork"])
+def test_vfork(self):
+self.fork_and_detach_test("vfork")
+
+# resume the parent
+self.test_sequence.add_log_lines([
+"read packet: $c#00",
+{"direction": "send", "regex": 

[Lldb-commits] [PATCH] D100965: [lldb] Refactor argument group by SourceLocationSpec (NFCI)

[Med Ismail Bennani via Phabricator via lldb-commits]

mib updated this revision to Diff 340183.
mib added a comment.

Addressed @JDevlieghere comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100965

Files:
  lldb/include/lldb/Breakpoint/BreakpointResolverFileLine.h
  lldb/include/lldb/Core/AddressResolverFileLine.h
  lldb/include/lldb/Symbol/CompileUnit.h
  lldb/include/lldb/Symbol/LineTable.h
  lldb/include/lldb/Symbol/SymbolFile.h
  lldb/source/API/SBThread.cpp
  lldb/source/Breakpoint/Breakpoint.cpp
  lldb/source/Breakpoint/BreakpointResolverFileLine.cpp
  lldb/source/Breakpoint/BreakpointResolverFileRegex.cpp
  lldb/source/Core/AddressResolverFileLine.cpp
  lldb/source/Core/Module.cpp
  lldb/source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.cpp
  lldb/source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.h
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.cpp
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.h
  lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
  lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.h
  lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp
  lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.h
  lldb/source/Symbol/CompileUnit.cpp
  lldb/source/Symbol/LineTable.cpp
  lldb/source/Symbol/SymbolFile.cpp
  lldb/source/Target/Target.cpp
  lldb/source/Utility/SourceLocationSpec.cpp
  lldb/unittests/Symbol/TestLineEntry.cpp

Index: lldb/unittests/Symbol/TestLineEntry.cpp
===
--- lldb/unittests/Symbol/TestLineEntry.cpp
+++ lldb/unittests/Symbol/TestLineEntry.cpp
@@ -53,8 +53,10 @@
 }
 
 llvm::Expected LineEntryTest::GetLineEntryForLine(uint32_t line) {
-  bool check_inlines = true;
-  bool exact = true;
+  // TODO: Handle SourceLocationSpec column information
+  const llvm::Optional column = llvm::None;
+  const bool check_inlines = true;
+  const bool exact_match = true;
   SymbolContextList sc_comp_units;
   SymbolContextList sc_line_entries;
   FileSpec file_spec("inlined-functions.cpp");
@@ -64,9 +66,14 @@
   if (sc_comp_units.GetSize() == 0)
 return llvm::createStringError(llvm::inconvertibleErrorCode(),
"No comp unit found on the test object.");
+
+  auto location_spec = SourceLocationSpec::Create(file_spec, line, column,
+  check_inlines, exact_match);
+  if (!location_spec)
+return location_spec.takeError();
+
   sc_comp_units[0].comp_unit->ResolveSymbolContext(
-  file_spec, line, check_inlines, exact, eSymbolContextLineEntry,
-  sc_line_entries);
+  *location_spec, eSymbolContextLineEntry, sc_line_entries);
   if (sc_line_entries.GetSize() == 0)
 return llvm::createStringError(llvm::inconvertibleErrorCode(),
"No line entry found on the test object.");
Index: lldb/source/Utility/SourceLocationSpec.cpp
===
--- lldb/source/Utility/SourceLocationSpec.cpp
+++ lldb/source/Utility/SourceLocationSpec.cpp
@@ -22,12 +22,7 @@
   if (!file_spec)
 return llvm::createStringError(
 llvm::errc::invalid_argument,
-"SourceLocationSpec requires a valid FileSpec.");
-
-  if (!line || line == LLDB_INVALID_LINE_NUMBER)
-return llvm::createStringError(
-llvm::errc::invalid_argument,
-"SourceLocationSpec requires a valid line number.");
+"Invalid source location spec. Requires a valid file spec.");
 
   return SourceLocationSpec(file_spec, line, column, check_inlines,
 exact_match);
Index: lldb/source/Target/Target.cpp
===
--- lldb/source/Target/Target.cpp
+++ lldb/source/Target/Target.cpp
@@ -371,9 +371,16 @@
   if (move_to_nearest_code == eLazyBoolCalculate)
 move_to_nearest_code = GetMoveToNearestCode() ? eLazyBoolYes : eLazyBoolNo;
 
+  auto location_spec =
+  SourceLocationSpec::Create(remapped_file, line_no, column, check_inlines,
+ !static_cast(move_to_nearest_code));
+  if (!location_spec) {
+llvm::errs() << llvm::toString(location_spec.takeError()) << "\n";
+return nullptr;
+  }
+
   BreakpointResolverSP resolver_sp(new BreakpointResolverFileLine(
-  nullptr, remapped_file, line_no, column, offset, check_inlines,
-  skip_prologue, !static_cast(move_to_nearest_code)));
+  nullptr, offset, skip_prologue, *location_spec));
   return CreateBreakpoint(filter_sp, resolver_sp, internal, hardware, true);
 }
 
Index: lldb/source/Symbol/SymbolFile.cpp
===
--- lldb/source/Symbol/SymbolFile.cpp
+++ lldb/source/Symbol/SymbolFile.cpp
@@ -97,10 +97,10 @@
   return 

[Lldb-commits] [PATCH] D100196: [lldb] [gdb-remote server] Introduce new stop reasons for fork and vfork

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

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

Actually, add asserts for fork/vfork extensions too.


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

https://reviews.llvm.org/D100196

Files:
  lldb/bindings/interface/SBThread.i
  lldb/bindings/interface/SBThreadPlan.i
  lldb/docs/python_api_enums.rst
  lldb/examples/python/performance.py
  lldb/include/lldb/API/SBThread.h
  lldb/include/lldb/API/SBThreadPlan.h
  lldb/include/lldb/Host/Debug.h
  lldb/include/lldb/lldb-enumerations.h
  lldb/packages/Python/lldbsuite/test/lldbutil.py
  lldb/source/API/SBThread.cpp
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
  lldb/source/Target/Process.cpp
  lldb/source/Target/StackFrameList.cpp
  lldb/source/Target/Thread.cpp
  lldb/tools/lldb-vscode/JSONUtils.cpp
  lldb/tools/lldb-vscode/LLDBUtils.cpp

Index: lldb/tools/lldb-vscode/LLDBUtils.cpp
===
--- lldb/tools/lldb-vscode/LLDBUtils.cpp
+++ lldb/tools/lldb-vscode/LLDBUtils.cpp
@@ -56,6 +56,9 @@
   case lldb::eStopReasonException:
   case lldb::eStopReasonExec:
   case lldb::eStopReasonProcessorTrace:
+  case lldb::eStopReasonFork:
+  case lldb::eStopReasonVFork:
+  case lldb::eStopReasonVForkDone:
 return true;
   case lldb::eStopReasonThreadExiting:
   case lldb::eStopReasonInvalid:
Index: lldb/tools/lldb-vscode/JSONUtils.cpp
===
--- lldb/tools/lldb-vscode/JSONUtils.cpp
+++ lldb/tools/lldb-vscode/JSONUtils.cpp
@@ -878,6 +878,15 @@
   case lldb::eStopReasonExec:
 body.try_emplace("reason", "entry");
 break;
+  case lldb::eStopReasonFork:
+body.try_emplace("reason", "fork");
+break;
+  case lldb::eStopReasonVFork:
+body.try_emplace("reason", "vfork");
+break;
+  case lldb::eStopReasonVForkDone:
+body.try_emplace("reason", "vforkdone");
+break;
   case lldb::eStopReasonThreadExiting:
   case lldb::eStopReasonInvalid:
   case lldb::eStopReasonNone:
Index: lldb/source/Target/Thread.cpp
===
--- lldb/source/Target/Thread.cpp
+++ lldb/source/Target/Thread.cpp
@@ -1679,6 +1679,12 @@
 return "exception";
   case eStopReasonExec:
 return "exec";
+  case eStopReasonFork:
+return "fork";
+  case eStopReasonVFork:
+return "vfork";
+  case eStopReasonVForkDone:
+return "vfork done";
   case eStopReasonPlanComplete:
 return "plan complete";
   case eStopReasonThreadExiting:
Index: lldb/source/Target/StackFrameList.cpp
===
--- lldb/source/Target/StackFrameList.cpp
+++ lldb/source/Target/StackFrameList.cpp
@@ -131,6 +131,9 @@
   case eStopReasonWatchpoint:
   case eStopReasonException:
   case eStopReasonExec:
+  case eStopReasonFork:
+  case eStopReasonVFork:
+  case eStopReasonVForkDone:
   case eStopReasonSignal:
 // In all these cases we want to stop in the deepest frame.
 m_current_inlined_pc = curr_pc;
Index: lldb/source/Target/Process.cpp
===
--- lldb/source/Target/Process.cpp
+++ lldb/source/Target/Process.cpp
@@ -822,6 +822,9 @@
 case eStopReasonWatchpoint:
 case eStopReasonException:
 case eStopReasonExec:
+case eStopReasonFork:
+case eStopReasonVFork:
+case eStopReasonVForkDone:
 case eStopReasonThreadExiting:
 case eStopReasonInstrumentation:
 case eStopReasonProcessorTrace:
Index: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
===
--- lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
+++ lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
@@ -659,6 +659,12 @@
 return "exec";
   case eStopReasonProcessorTrace:
 return "processor trace";
+  case eStopReasonFork:
+return "fork";
+  case eStopReasonVFork:
+return "vfork";
+  case eStopReasonVForkDone:
+return "vforkdone";
   case eStopReasonInstrumentation:
   case eStopReasonInvalid:
   case eStopReasonPlanComplete:
@@ -934,6 +940,22 @@
 }
   }
 
+  // Include child process PID/TID for forks.
+  if (tid_stop_info.reason == eStopReasonFork ||
+  tid_stop_info.reason == eStopReasonVFork) {
+assert(bool(m_extensions_supported &
+NativeProcessProtocol::Extension::multiprocess));
+if (tid_stop_info.reason == eStopReasonFork)
+  assert(bool(m_extensions_supported &
+  NativeProcessProtocol::Extension::fork));
+if (tid_stop_info.reason == eStopReasonVFork)
+  assert(bool(m_extensions_supported &
+  NativeProcessProtocol::Extension::vfork));
+response.Printf("%s:p%" PRIx64 ".%" PRIx64 ";", reason_str,
+

[Lldb-commits] [PATCH] D100196: [lldb] [gdb-remote server] Introduce new stop reasons for fork and vfork

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

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

Add an assert to make sure that process doesn't report fork/vfork stop reason 
without multiprocess support.


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

https://reviews.llvm.org/D100196

Files:
  lldb/bindings/interface/SBThread.i
  lldb/bindings/interface/SBThreadPlan.i
  lldb/docs/python_api_enums.rst
  lldb/examples/python/performance.py
  lldb/include/lldb/API/SBThread.h
  lldb/include/lldb/API/SBThreadPlan.h
  lldb/include/lldb/Host/Debug.h
  lldb/include/lldb/lldb-enumerations.h
  lldb/packages/Python/lldbsuite/test/lldbutil.py
  lldb/source/API/SBThread.cpp
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
  lldb/source/Target/Process.cpp
  lldb/source/Target/StackFrameList.cpp
  lldb/source/Target/Thread.cpp
  lldb/tools/lldb-vscode/JSONUtils.cpp
  lldb/tools/lldb-vscode/LLDBUtils.cpp

Index: lldb/tools/lldb-vscode/LLDBUtils.cpp
===
--- lldb/tools/lldb-vscode/LLDBUtils.cpp
+++ lldb/tools/lldb-vscode/LLDBUtils.cpp
@@ -56,6 +56,9 @@
   case lldb::eStopReasonException:
   case lldb::eStopReasonExec:
   case lldb::eStopReasonProcessorTrace:
+  case lldb::eStopReasonFork:
+  case lldb::eStopReasonVFork:
+  case lldb::eStopReasonVForkDone:
 return true;
   case lldb::eStopReasonThreadExiting:
   case lldb::eStopReasonInvalid:
Index: lldb/tools/lldb-vscode/JSONUtils.cpp
===
--- lldb/tools/lldb-vscode/JSONUtils.cpp
+++ lldb/tools/lldb-vscode/JSONUtils.cpp
@@ -878,6 +878,15 @@
   case lldb::eStopReasonExec:
 body.try_emplace("reason", "entry");
 break;
+  case lldb::eStopReasonFork:
+body.try_emplace("reason", "fork");
+break;
+  case lldb::eStopReasonVFork:
+body.try_emplace("reason", "vfork");
+break;
+  case lldb::eStopReasonVForkDone:
+body.try_emplace("reason", "vforkdone");
+break;
   case lldb::eStopReasonThreadExiting:
   case lldb::eStopReasonInvalid:
   case lldb::eStopReasonNone:
Index: lldb/source/Target/Thread.cpp
===
--- lldb/source/Target/Thread.cpp
+++ lldb/source/Target/Thread.cpp
@@ -1679,6 +1679,12 @@
 return "exception";
   case eStopReasonExec:
 return "exec";
+  case eStopReasonFork:
+return "fork";
+  case eStopReasonVFork:
+return "vfork";
+  case eStopReasonVForkDone:
+return "vfork done";
   case eStopReasonPlanComplete:
 return "plan complete";
   case eStopReasonThreadExiting:
Index: lldb/source/Target/StackFrameList.cpp
===
--- lldb/source/Target/StackFrameList.cpp
+++ lldb/source/Target/StackFrameList.cpp
@@ -131,6 +131,9 @@
   case eStopReasonWatchpoint:
   case eStopReasonException:
   case eStopReasonExec:
+  case eStopReasonFork:
+  case eStopReasonVFork:
+  case eStopReasonVForkDone:
   case eStopReasonSignal:
 // In all these cases we want to stop in the deepest frame.
 m_current_inlined_pc = curr_pc;
Index: lldb/source/Target/Process.cpp
===
--- lldb/source/Target/Process.cpp
+++ lldb/source/Target/Process.cpp
@@ -822,6 +822,9 @@
 case eStopReasonWatchpoint:
 case eStopReasonException:
 case eStopReasonExec:
+case eStopReasonFork:
+case eStopReasonVFork:
+case eStopReasonVForkDone:
 case eStopReasonThreadExiting:
 case eStopReasonInstrumentation:
 case eStopReasonProcessorTrace:
Index: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
===
--- lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
+++ lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
@@ -659,6 +659,12 @@
 return "exec";
   case eStopReasonProcessorTrace:
 return "processor trace";
+  case eStopReasonFork:
+return "fork";
+  case eStopReasonVFork:
+return "vfork";
+  case eStopReasonVForkDone:
+return "vforkdone";
   case eStopReasonInstrumentation:
   case eStopReasonInvalid:
   case eStopReasonPlanComplete:
@@ -934,6 +940,16 @@
 }
   }
 
+  // Include child process PID/TID for forks.
+  if (tid_stop_info.reason == eStopReasonFork ||
+  tid_stop_info.reason == eStopReasonVFork) {
+assert(bool(m_extensions_supported &
+NativeProcessProtocol::Extension::multiprocess));
+response.Printf("%s:p%" PRIx64 ".%" PRIx64 ";", reason_str,
+tid_stop_info.details.fork.child_pid,
+tid_stop_info.details.fork.child_tid);
+  }
+
   return SendPacketNoLock(response.GetString());
 }
 
Index: lldb/source/API/SBThread.cpp
===
--- 

[Lldb-commits] [PATCH] D100153: [lldb] [Process] Introduce protocol extension support API

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

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

Added `Extension` bit for `multiprocess`. Made `fork-events` and `vfork-events` 
both depend on it — i.e. be implicitly disabled if `multiprocess` wasn't 
reported as supported.

Simplify `GDBRemoteCommunicationServerLLGS::SetEnabledExtensions()` — we can 
just take the value instead of copying it bit-by-bit ;-).

Move `add_qSupported_packets()` change from later patch. Add tests for 
`qSupported` responses (particularly, rejecting `[v]fork-events` if 
`multiprocess` is not present). The new tests are in `fork` category that's 
skipped on all platforms where fork/vfork isn't enabled yet (i.e. everywhere in 
this patch).


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

https://reviews.llvm.org/D100153

Files:
  lldb/include/lldb/Host/common/NativeProcessProtocol.h
  lldb/packages/Python/lldbsuite/test/dotest.py
  lldb/packages/Python/lldbsuite/test/test_categories.py
  lldb/packages/Python/lldbsuite/test/tools/lldb-server/gdbremote_testcase.py
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h
  lldb/test/API/tools/lldb-server/TestLldbGdbServer.py

Index: lldb/test/API/tools/lldb-server/TestLldbGdbServer.py
===
--- lldb/test/API/tools/lldb-server/TestLldbGdbServer.py
+++ lldb/test/API/tools/lldb-server/TestLldbGdbServer.py
@@ -954,23 +954,58 @@
 self.set_inferior_startup_launch()
 self.breakpoint_set_and_remove_work(want_hardware=True)
 
-def test_qSupported_returns_known_stub_features(self):
+def get_qSupported_dict(self, features=[]):
 self.build()
 self.set_inferior_startup_launch()
 
 # Start up the stub and start/prep the inferior.
 procs = self.prep_debug_monitor_and_inferior()
-self.add_qSupported_packets()
+self.add_qSupported_packets(features)
 
 # Run the packet stream.
 context = self.expect_gdbremote_sequence()
 self.assertIsNotNone(context)
 
 # Retrieve the qSupported features.
-supported_dict = self.parse_qSupported_response(context)
+return self.parse_qSupported_response(context)
+
+def test_qSupported_returns_known_stub_features(self):
+supported_dict = self.get_qSupported_dict()
 self.assertIsNotNone(supported_dict)
 self.assertTrue(len(supported_dict) > 0)
 
+@add_test_categories(["fork"])
+def test_qSupported_fork_events(self):
+supported_dict = (
+self.get_qSupported_dict(['multiprocess+', 'fork-events+']))
+self.assertEqual(supported_dict.get('multiprocess', '-'), '+')
+self.assertEqual(supported_dict.get('fork-events', '-'), '+')
+self.assertEqual(supported_dict.get('vfork-events', '-'), '-')
+
+@add_test_categories(["fork"])
+def test_qSupported_fork_events_without_multiprocess(self):
+supported_dict = (
+self.get_qSupported_dict(['fork-events+']))
+self.assertEqual(supported_dict.get('multiprocess', '-'), '-')
+self.assertEqual(supported_dict.get('fork-events', '-'), '-')
+self.assertEqual(supported_dict.get('vfork-events', '-'), '-')
+
+@add_test_categories(["fork"])
+def test_qSupported_vfork_events(self):
+supported_dict = (
+self.get_qSupported_dict(['multiprocess+', 'vfork-events+']))
+self.assertEqual(supported_dict.get('multiprocess', '-'), '+')
+self.assertEqual(supported_dict.get('fork-events', '-'), '-')
+self.assertEqual(supported_dict.get('vfork-events', '-'), '+')
+
+@add_test_categories(["fork"])
+def test_qSupported_vfork_events_without_multiprocess(self):
+supported_dict = (
+self.get_qSupported_dict(['vfork-events+']))
+self.assertEqual(supported_dict.get('multiprocess', '-'), '-')
+self.assertEqual(supported_dict.get('fork-events', '-'), '-')
+self.assertEqual(supported_dict.get('vfork-events', '-'), '-')
+
 @skipIfWindows # No pty support to test any inferior output
 def test_written_M_content_reads_back_correctly(self):
 self.build()
Index: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h
===
--- lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h
+++ lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h
@@ -103,6 +103,8 @@
   bool m_thread_suffix_supported = false;
   bool m_list_threads_in_stop_reply = false;
 
+  NativeProcessProtocol::Extension m_extensions_supported = {};
+
   PacketResult SendONotification(const char *buffer, uint32_t len);
 
   PacketResult SendWResponse(NativeProcessProtocol *process);
@@ -264,6 +266,9 @@
   llvm::Expected ReadTid(StringExtractorGDBRemote ,

[Lldb-commits] [PATCH] D100196: [lldb] [gdb-remote server] Introduce new stop reasons for fork and vfork

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

mgorny added inline comments.



Comment at: 
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:946-948
+response.Printf("%s:p%" PRIx64 ".%" PRIx64 ";", reason_str,
+tid_stop_info.details.fork.child_pid,
+tid_stop_info.details.fork.child_tid);

labath wrote:
> I just realized this is implicitly assuming the multiprocess extension is 
> enabled/supported. That makes sense since forking always involves multiple 
> processes, though maybe the server should check that the client actually 
> reports it as supported.
Makes sense. I'll add it to the extension patch since that's where we determine 
if fork/vfork is supported.


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

https://reviews.llvm.org/D100196

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

[Lldb-commits] [PATCH] D101198: [lldb-vscode] Read requests asynchronously

[walter erquinigo via Phabricator via lldb-commits]

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

Currently, lldb-vscode is reading and processing requests one at a time, which
doesn't match exactly the interactivity of the UI. What happens when LLDB is
working hard parsing symbols, and at the same time the user is hovering around,
pressing resume multiple times, expanding variables in the variables view, etc?
In this case, LLDB will eventually process all of these actions one by one 
even though the user has already pressed resume, which means that most of the
responses will be discarded by the UI. This imposes a big burden on lldb.

A way to fix that is to parallelize the reading and processing of requests,
so that while a request is being processed, the reader can filter out pending
useless requests.

This diff only contains the concurrent queue used for this. If this code makes
sense, I'll proceed to add some filtering logic for pending requests.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D101198

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

Index: lldb/tools/lldb-vscode/lldb-vscode.cpp
===
--- lldb/tools/lldb-vscode/lldb-vscode.cpp
+++ lldb/tools/lldb-vscode/lldb-vscode.cpp
@@ -3145,6 +3145,93 @@
   return new_stdout_fd;
 }
 
+class RequestQueue {
+public:
+  RequestQueue() = default;
+
+  RequestQueue(const RequestQueue &) = delete ;
+  RequestQueue& operator=(const RequestQueue &) = delete;
+
+  /// Get the next front element from the queue or \a None if
+  /// the queue has been terminated.
+  llvm::Optional Pop() {
+std::unique_lock lock(m_mutex);
+while(m_data.empty() && !m_termination_status)
+  m_can_consume.wait(lock);
+
+if (m_termination_status)
+  return llvm::None;
+
+llvm::json::Object request(std::move(m_data.front()));
+m_data.pop_front();
+return std::move(request);
+  }
+
+  /// Push a new element in the queue, unless it has been terminated.
+  void Push(llvm::json::Object &) {
+std::lock_guard lock(m_mutex);
+if (m_termination_status)
+  return;
+
+bool was_empty = m_data.empty();
+m_data.emplace_back(std::move(obj));
+if (was_empty)
+  m_can_consume.notify_one();
+  }
+
+  /// Terminate the queue. It won't accept more requests not provide more elements to the reader.
+  void Terminate(bool success) {
+std::lock_guard lock(m_mutex);
+
+if (!m_termination_status)
+  m_termination_status = success;
+m_can_consume.notify_one();
+  }
+
+  bool GetTerminationStatus() {
+return *m_termination_status;
+  }
+
+private:
+  std::list m_data;
+  std::mutex m_mutex;
+  std::condition_variable m_can_consume;
+  llvm::Optional m_termination_status;
+};
+
+int StartReceivingPackets() {
+  RequestQueue requests;
+
+  std::thread reader_thread ([&]{
+uint32_t packet_idx = 0;
+while (!g_vsc.sent_terminated_event) {
+  llvm::json::Object object;
+  lldb_vscode::PacketStatus status = g_vsc.GetNextObject(object);
+  if (status == lldb_vscode::PacketStatus::EndOfFile)
+break;
+  if (status != lldb_vscode::PacketStatus::Success) {
+requests.Terminate(false);
+break;
+  }
+
+  requests.Push(std::move(object));
+  ++packet_idx;
+}
+requests.Terminate(true);
+  });
+
+  std::thread worker_thread([&]() {
+while (llvm::Optional object = requests.Pop()) {
+  if (!g_vsc.HandleObject(*object))
+requests.Terminate(false);
+}
+  });
+  reader_thread.join();
+  worker_thread.join();
+
+  return requests.GetTerminationStatus() ? EXIT_SUCCESS : EXIT_FAILURE;
+}
+
 int main(int argc, char *argv[]) {
   llvm::InitLLVM IL(argc, argv, /*InstallPipeSignalExitHandler=*/false);
   llvm::PrettyStackTraceProgram X(argc, argv);
@@ -3224,19 +3311,5 @@
 g_vsc.output.descriptor = StreamDescriptor::from_file(new_stdout_fd, false);
   }
 
-  uint32_t packet_idx = 0;
-  while (!g_vsc.sent_terminated_event) {
-llvm::json::Object object;
-lldb_vscode::PacketStatus status = g_vsc.GetNextObject(object);
-if (status == lldb_vscode::PacketStatus::EndOfFile)
-  break;
-if (status != lldb_vscode::PacketStatus::Success)
-  return 1; // Fatal error
-
-if (!g_vsc.HandleObject(object))
-  return 1;
-++packet_idx;
-  }
-
-  return EXIT_SUCCESS;
+  return StartReceivingPackets();
 }
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D100208: [lldb] [Process/Linux] Report fork/vfork stop reason

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

mgorny added inline comments.



Comment at: lldb/source/Plugins/Process/Linux/NativeThreadLinux.cpp:402-403
+  is_vfork ? StopReason::eStopReasonVFork : StopReason::eStopReasonFork;
+  m_stop_info.details.fork.child_pid = child_pid;
+  m_stop_info.details.fork.child_tid = child_pid;
+}

labath wrote:
> Still duplicated. :)
I'm confused. Maybe you're not noticing the tiny one-letter difference, i.e. 
`pid` vs `tid`? ;-) Linux is the only target where both are equal and the stop 
info needs to be generic, so that other platforms can assign distinct `pid` and 
`tid`.


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

https://reviews.llvm.org/D100208

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

[Lldb-commits] [PATCH] D101128: [lldb-vscode] only report long running progress events

[Greg Clayton via Phabricator via lldb-commits]

clayborg requested changes to this revision.
clayborg added inline comments.
This revision now requires changes to proceed.



Comment at: lldb/tools/lldb-vscode/ProgressEvent.cpp:89
+   m_start_event_timestamp >=
+   std::chrono::seconds(3);
+}

The patch description says 5 seconds, here it is 3 seconds, and I would prefer 
maybe 1 second.



Comment at: lldb/tools/lldb-vscode/ProgressEvent.cpp:92
+
+bool TaskProgressEventQueue::SetUpdate(const ProgressEvent ) {
+  if (event.GetEventType() == progressEnd)

I would just name this "TaskProgressEventQueue::Update(...)"



Comment at: lldb/tools/lldb-vscode/ProgressEvent.cpp:93-94
+bool TaskProgressEventQueue::SetUpdate(const ProgressEvent ) {
+  if (event.GetEventType() == progressEnd)
+m_finished = true;
+

If this is an progressEnd event, we could track if this event did report any 
progressStart or progressUpdate events and just send the update right away 
here? That would require that the report_callback is stored during construction 
though. It would be easy if TaskProgressEventQueue just got a reference to the 
ProgressEventFilterQueue since it owns these TaskProgressEventQueue. Then we 
could extract the callback from it.



Comment at: lldb/tools/lldb-vscode/ProgressEvent.cpp:113
+  if (m_waiting) {
+report_callback(m_start_event);
+m_waiting = false;

Should we check m_last_update_event here and make sure it isn't a progressEnd 
event? It is is, we will notify both start and end right away? Edge case, but 
just wanted to check.



Comment at: lldb/tools/lldb-vscode/ProgressEvent.cpp:124
+: m_report_callback(report_callback) {
+  m_aux_reporter_thread_finished = false;
+  m_aux_reporter_thread = std::thread([&] {

This might be better named "m_aux_reporter_thread_should_exit". This variable 
sounds like the thread itself will modify the value when it finishes, but this 
is a "I want to tell the thread when it is no longer needed when I want it to 
exit.



Comment at: lldb/tools/lldb-vscode/ProgressEvent.cpp:127
+while (!m_aux_reporter_thread_finished) {
+  std::this_thread::sleep_for(std::chrono::milliseconds(100));
+  ReportWaitingEvents();

If we are waiting a number of seconds for each progress, not sure we need to 
sleep for only 100ms. Shouldn't we sleep for a bit longer time?



Comment at: lldb/tools/lldb-vscode/ProgressEvent.cpp:151
+
+  while (!m_event_ids.empty()) {
+uint64_t progress_id = m_event_ids.front();

If I am reading this loop correctly, we are going to only notify the progress 
whose id is m_event_ids.front()? So if we have 10 progress start events we will 
only report the first one? Can VS Code only handle one progress at a time? If 
not we need to iterate over all m_event_ids and report any possible progress on 
all events that have new status as many different threads could be progressing 
on indexing DWARF.



Comment at: lldb/tools/lldb-vscode/ProgressEvent.cpp:157
+else if (TryReport(it->second))
+  m_event_ids.pop();
+else

ProgressEventFilterQueue::TryReport() returns true if the event was reported 
even if it didn't finish. Do we always want to pop from m_event_ids even if the 
task didn't finish? Is this loop only to catch reporting the start event?



Comment at: lldb/tools/lldb-vscode/ProgressEvent.cpp:176-177
+TaskProgressEventQueue  = it->second;
+if (task.SetUpdate(event))
+  TryReport(task);
   }

Can't "SetUpdate" just report things if needed? This could be just:

```
it->second.SetUpdate(event);
```





Comment at: lldb/tools/lldb-vscode/ProgressEvent.h:44
   uint64_t m_progress_id;
-  const char *m_message;
   ProgressEventType m_event_type;

Yikes this must have caused some serious bugs in the reporting mechanism as the 
old string would have been freed!!



Comment at: lldb/tools/lldb-vscode/ProgressEvent.h:57
+/// It controls when the event should start being reported to the IDE.
+class TaskProgressEventQueue {
+public:

TaskProgressEventQueue might be better named "ProgressEventManager"? It doesn't 
have a queue, but it does manage event deliveries



Comment at: lldb/tools/lldb-vscode/ProgressEvent.h:61-64
+  /// \return
+  /// \b true if enough seconds have passed since the start event,
+  /// which means the events should start being reported.
+  bool ShouldReport() const;

make private?



Comment at: lldb/tools/lldb-vscode/ProgressEvent.h:86
+  llvm::Optional m_last_update_event;
+  std::chrono::duration m_start_event_timestamp;
+  bool m_waiting;

Should be just be storing 

[Lldb-commits] [PATCH] D100208: [lldb] [Process/Linux] Report fork/vfork stop reason

[Pavel Labath via Phabricator via lldb-commits]

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

In D100208#2705891 , @mgorny wrote:

> In D100208#2705455 , @labath wrote:
>
>> This seems reasonable.
>>
>> Why don't we have tests that detach from the parent process (and maybe 
>> continue the child)? Does that already work? (or in general, what is 
>> expected to work after this patch?)
>
> It doesn't, we need D100261  for that. 
> Right now `current_process` will become `nullptr` if we detach the parent.
>
> That said, technically we could test that after detaching the parent we get 
> an error from other requests ;-).

I think that's a good thing to test, though I suppose it could also be a part 
of the Hg patch.




Comment at: lldb/source/Plugins/Process/Linux/NativeThreadLinux.cpp:402-403
+  is_vfork ? StopReason::eStopReasonVFork : StopReason::eStopReasonFork;
+  m_stop_info.details.fork.child_pid = child_pid;
+  m_stop_info.details.fork.child_tid = child_pid;
+}

Still duplicated. :)


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

https://reviews.llvm.org/D100208

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

[Lldb-commits] [PATCH] D100962: [lldb/Utility] Add SourceLocationSpec class (NFC)

[Jonas Devlieghere via Phabricator via lldb-commits]

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

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100962

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

[Lldb-commits] [PATCH] D100965: [lldb] Refactor argument group by SourceLocationSpec (NFCI)

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.



Comment at: lldb/source/API/SBThread.cpp:853-856
+  std::string err_msg = "Invalid SourceLocationSpec: ";
+  err_msg += llvm::toString(location_spec.takeError());
+  sb_error.SetErrorString(err_msg.c_str());
+  return sb_error;

Let's move `Invalid SourceLocationSpec:` (without the capitalization) into the 
error reported by `SourceLocationSpec::Create` and use the `Status` ctor that 
takes an `llvm::Error` to avoid this code duplication. 



Comment at: lldb/source/API/SBThread.cpp:852
+auto location_spec = SourceLocationSpec::Create(
+step_file_spec, line, column, check_inlines, exact);
+lldbassert(location_spec && "Invalid SourceLocationSpec.");

shafik wrote:
> ```
> step_file_spec, line, /*column*/llvm::None, check_inlines, exact);
> ```
As @teemperor pointed out in another patch, the llvm style includes a `=` at 
the end (https://www.llvm.org/docs/CodingStandards.html#comment-formatting)

```
auto location_spec = SourceLocationSpec::Create(
step_file_spec, line, /*colum=*/llvm::None, check_inlines, exact);```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100965

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

[Lldb-commits] [PATCH] D100191: [lldb] [llgs] Support owning and detaching extra processes

[Pavel Labath via Phabricator via lldb-commits]

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



Comment at: lldb/unittests/TestingSupport/Host/NativeProcessTestUtils.h:31-34
+  void NewSubprocess(NativeProcessProtocol *parent_process,
+ std::unique_ptr child_process) {
+NewSubprocessImpl(parent_process, child_process);
+  }

Maybe add a quick comment to explain why this is here.


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

https://reviews.llvm.org/D100191

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

[Lldb-commits] [PATCH] D100196: [lldb] [gdb-remote server] Introduce new stop reasons for fork and vfork

[Pavel Labath via Phabricator via lldb-commits]

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



Comment at: 
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:946-948
+response.Printf("%s:p%" PRIx64 ".%" PRIx64 ";", reason_str,
+tid_stop_info.details.fork.child_pid,
+tid_stop_info.details.fork.child_tid);

I just realized this is implicitly assuming the multiprocess extension is 
enabled/supported. That makes sense since forking always involves multiple 
processes, though maybe the server should check that the client actually 
reports it as supported.


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

https://reviews.llvm.org/D100196

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

[Lldb-commits] [PATCH] D100965: [lldb] Refactor argument group by SourceLocationSpec (NFCI)

[Med Ismail Bennani via Phabricator via lldb-commits]

mib updated this revision to Diff 340128.
mib marked 6 inline comments as done.
mib added a comment.

Addressed @JDevlieghere @shafik feedbacks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100965

Files:
  lldb/include/lldb/Breakpoint/BreakpointResolverFileLine.h
  lldb/include/lldb/Core/AddressResolverFileLine.h
  lldb/include/lldb/Symbol/CompileUnit.h
  lldb/include/lldb/Symbol/LineTable.h
  lldb/include/lldb/Symbol/SymbolFile.h
  lldb/source/API/SBThread.cpp
  lldb/source/Breakpoint/Breakpoint.cpp
  lldb/source/Breakpoint/BreakpointResolverFileLine.cpp
  lldb/source/Breakpoint/BreakpointResolverFileRegex.cpp
  lldb/source/Core/AddressResolverFileLine.cpp
  lldb/source/Core/Module.cpp
  lldb/source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.cpp
  lldb/source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.h
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.cpp
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.h
  lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
  lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.h
  lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp
  lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.h
  lldb/source/Symbol/CompileUnit.cpp
  lldb/source/Symbol/LineTable.cpp
  lldb/source/Symbol/SymbolFile.cpp
  lldb/source/Target/Target.cpp
  lldb/unittests/Symbol/TestLineEntry.cpp

Index: lldb/unittests/Symbol/TestLineEntry.cpp
===
--- lldb/unittests/Symbol/TestLineEntry.cpp
+++ lldb/unittests/Symbol/TestLineEntry.cpp
@@ -53,8 +53,10 @@
 }
 
 llvm::Expected LineEntryTest::GetLineEntryForLine(uint32_t line) {
-  bool check_inlines = true;
-  bool exact = true;
+  // TODO: Handle SourceLocationSpec column information
+  const llvm::Optional column = llvm::None;
+  const bool check_inlines = true;
+  const bool exact_match = true;
   SymbolContextList sc_comp_units;
   SymbolContextList sc_line_entries;
   FileSpec file_spec("inlined-functions.cpp");
@@ -64,9 +66,17 @@
   if (sc_comp_units.GetSize() == 0)
 return llvm::createStringError(llvm::inconvertibleErrorCode(),
"No comp unit found on the test object.");
+
+  auto location_spec = SourceLocationSpec::Create(file_spec, line, column,
+  check_inlines, exact_match);
+  if (!location_spec) {
+std::string err_msg = "Invalid SourceLocationSpec: ";
+err_msg += llvm::toString(location_spec.takeError());
+return llvm::createStringError(llvm::inconvertibleErrorCode(), err_msg);
+  }
+
   sc_comp_units[0].comp_unit->ResolveSymbolContext(
-  file_spec, line, check_inlines, exact, eSymbolContextLineEntry,
-  sc_line_entries);
+  *location_spec, eSymbolContextLineEntry, sc_line_entries);
   if (sc_line_entries.GetSize() == 0)
 return llvm::createStringError(llvm::inconvertibleErrorCode(),
"No line entry found on the test object.");
Index: lldb/source/Target/Target.cpp
===
--- lldb/source/Target/Target.cpp
+++ lldb/source/Target/Target.cpp
@@ -371,9 +371,17 @@
   if (move_to_nearest_code == eLazyBoolCalculate)
 move_to_nearest_code = GetMoveToNearestCode() ? eLazyBoolYes : eLazyBoolNo;
 
+  auto location_spec =
+  SourceLocationSpec::Create(remapped_file, line_no, column, check_inlines,
+ !static_cast(move_to_nearest_code));
+  if (!location_spec) {
+llvm::errs() << "Invalid SourceLocationSpec: ";
+llvm::errs() << llvm::toString(location_spec.takeError()) << "\n";
+return nullptr;
+  }
+
   BreakpointResolverSP resolver_sp(new BreakpointResolverFileLine(
-  nullptr, remapped_file, line_no, column, offset, check_inlines,
-  skip_prologue, !static_cast(move_to_nearest_code)));
+  nullptr, offset, skip_prologue, *location_spec));
   return CreateBreakpoint(filter_sp, resolver_sp, internal, hardware, true);
 }
 
Index: lldb/source/Symbol/SymbolFile.cpp
===
--- lldb/source/Symbol/SymbolFile.cpp
+++ lldb/source/Symbol/SymbolFile.cpp
@@ -97,10 +97,10 @@
   return type_system_or_err;
 }
 
-uint32_t SymbolFile::ResolveSymbolContext(const FileSpec _spec,
-  uint32_t line, bool check_inlines,
-  lldb::SymbolContextItem resolve_scope,
-  SymbolContextList _list) {
+uint32_t
+SymbolFile::ResolveSymbolContext(const SourceLocationSpec _location_spec,
+ lldb::SymbolContextItem resolve_scope,
+ SymbolContextList _list) {
   

[Lldb-commits] [PATCH] D100962: [lldb/Utility] Add SourceLocationSpec class (NFC)

[Med Ismail Bennani via Phabricator via lldb-commits]

mib updated this revision to Diff 340117.
mib added a comment.

Address @JDevlieghere comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100962

Files:
  lldb/include/lldb/Utility/SourceLocationSpec.h
  lldb/source/Utility/CMakeLists.txt
  lldb/source/Utility/SourceLocationSpec.cpp
  lldb/unittests/Utility/CMakeLists.txt
  lldb/unittests/Utility/SourceLocationSpecTest.cpp

Index: lldb/unittests/Utility/SourceLocationSpecTest.cpp
===
--- /dev/null
+++ lldb/unittests/Utility/SourceLocationSpecTest.cpp
@@ -0,0 +1,208 @@
+//===-- SourceLocationSpecTest.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 "gtest/gtest.h"
+
+#include "lldb/Utility/LLDBAssert.h"
+#include "lldb/Utility/SourceLocationSpec.h"
+
+#include "llvm/Testing/Support/Error.h"
+
+using namespace lldb_private;
+
+TEST(SourceLocationSpecTest, CreateSourceLocationSpec) {
+  FileSpec fs("/foo/bar", FileSpec::Style::posix);
+  const uint32_t line = 19;
+  const uint32_t column = 4;
+  const bool check_inlines = false;
+  const bool exact_match = true;
+
+  // Invalid FileSpec
+  ASSERT_THAT_EXPECTED(
+  SourceLocationSpec::Create(FileSpec(), line, check_inlines, exact_match),
+  llvm::Failed());
+  // Null Line
+  ASSERT_THAT_EXPECTED(
+  SourceLocationSpec::Create(fs, 0, check_inlines, exact_match),
+  llvm::Failed());
+  // Invalid Line
+  ASSERT_THAT_EXPECTED(SourceLocationSpec::Create(fs, LLDB_INVALID_LINE_NUMBER,
+  check_inlines, exact_match),
+   llvm::Failed());
+  // Invalid FileSpec & Line
+  ASSERT_THAT_EXPECTED(
+  SourceLocationSpec::Create(FileSpec(), 0, check_inlines, exact_match),
+  llvm::Failed());
+
+  // Valid FileSpec & Line
+  ASSERT_THAT_EXPECTED(
+  SourceLocationSpec::Create(fs, line, check_inlines, exact_match),
+  llvm::Succeeded());
+  // Valid FileSpec, Line & Column
+  ASSERT_THAT_EXPECTED(
+  SourceLocationSpec::Create(fs, line, column, check_inlines, exact_match),
+  llvm::Succeeded());
+}
+
+TEST(SourceLocationSpecTest, FileLineColumnComponents) {
+  FileSpec fs("/foo/bar", FileSpec::Style::posix);
+  const uint32_t line = 19;
+  const uint16_t column = 4;
+
+  auto location_spec = SourceLocationSpec::Create(
+  fs, line, LLDB_INVALID_COLUMN_NUMBER, false, true);
+  ASSERT_THAT_EXPECTED(location_spec, llvm::Succeeded());
+  SourceLocationSpec without_column = *location_spec;
+  EXPECT_EQ(fs, without_column.GetFileSpec());
+  EXPECT_EQ(line, without_column.GetLine());
+  EXPECT_EQ(llvm::None, without_column.GetColumn());
+  EXPECT_FALSE(without_column.GetCheckInlines());
+  EXPECT_TRUE(without_column.GetExactMatch());
+  EXPECT_STREQ("/foo/bar:19", without_column.GetCString());
+
+  location_spec = SourceLocationSpec::Create(fs, line, column, true, false);
+  ASSERT_THAT_EXPECTED(location_spec, llvm::Succeeded());
+  SourceLocationSpec with_column = *location_spec;
+  EXPECT_EQ(column, *with_column.GetColumn());
+  EXPECT_TRUE(with_column.GetCheckInlines());
+  EXPECT_FALSE(with_column.GetExactMatch());
+  EXPECT_STREQ("/foo/bar:19:4", with_column.GetCString());
+}
+
+static SourceLocationSpec Create(bool check_inlines, bool exact_match,
+ FileSpec fs, uint32_t line,
+ uint16_t column = LLDB_INVALID_COLUMN_NUMBER) {
+  auto location_spec =
+  SourceLocationSpec::Create(fs, line, column, check_inlines, exact_match);
+  if (!location_spec) {
+llvm::errs() << "Invalid SourceLocationSpec.\n";
+llvm::cantFail(location_spec.takeError());
+  }
+
+  return *location_spec;
+}
+
+TEST(SourceLocationSpecTest, Equal) {
+  auto Equal = [](SourceLocationSpec lhs, SourceLocationSpec rhs, bool full) {
+return SourceLocationSpec::Equal(lhs, rhs, full);
+  };
+
+  const FileSpec fs("/foo/bar", FileSpec::Style::posix);
+  const FileSpec other_fs("/foo/baz", FileSpec::Style::posix);
+
+  // mutating FileSpec + const Inlined, ExactMatch, Line
+  EXPECT_TRUE(
+  Equal(Create(false, false, fs, 4), Create(false, false, fs, 4), true));
+  EXPECT_TRUE(
+  Equal(Create(true, true, fs, 4), Create(true, true, fs, 4), false));
+  EXPECT_FALSE(Equal(Create(false, false, fs, 4),
+ Create(false, false, other_fs, 4), true));
+  EXPECT_FALSE(
+  Equal(Create(true, true, fs, 4), Create(true, true, other_fs, 4), false));
+
+  // Mutating FileSpec + const Inlined, ExactMatch, Line, Column
+  EXPECT_TRUE(Equal(Create(false, false, fs, 4, 19),
+Create(false, 

[Lldb-commits] [PATCH] D101131: [lldb-vscode] Follow up of D99989 - store some strings more safely

[walter erquinigo via Phabricator via lldb-commits]

wallace added inline comments.



Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:2934
 // We first find out which variable names are duplicated
-llvm::DenseMap variable_name_counts;
+std::map variable_name_counts;
 for (auto i = start_idx; i < end_idx; ++i) {

JDevlieghere wrote:
> Have you considered an`llvm::StringMap`, it should be more performant. 
I've been reading 
https://llvm.org/devmtg/2014-04/PDFs/LightningTalks/data_structure_llvm.pdf and 
it seems that it's not more performant speed-wise. I imagine it's more 
performance space-wise, but here I mostly care about time.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101131

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

[Lldb-commits] [PATCH] D100965: [lldb] Refactor argument group by SourceLocationSpec (NFCI)

[Shafik Yaghmour via Phabricator via lldb-commits]

shafik added inline comments.



Comment at: lldb/source/API/SBThread.cpp:852
+auto location_spec = SourceLocationSpec::Create(
+step_file_spec, line, column, check_inlines, exact);
+lldbassert(location_spec && "Invalid SourceLocationSpec.");

```
step_file_spec, line, /*column*/llvm::None, check_inlines, exact);
```



Comment at: lldb/source/Breakpoint/BreakpointResolverFileRegex.cpp:114
+auto location_spec = SourceLocationSpec::Create(
+cu_file_spec, line_matches[i], column, search_inlines, m_exact_match);
+lldbassert(location_spec && "Invalid SourceLocationSpec.");

```
cu_file_spec, line_matches[i], /*column*/llvm::None, search_inlines, 
m_exact_match);
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100965

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

[Lldb-commits] [PATCH] D101153: [lldb][NFC] Specify guidelines for API tests

[Jordan Rupprecht via Phabricator via lldb-commits]

rupprecht added a comment.

LGTM too, thanks for writing this up!




Comment at: lldb/docs/resources/test.rst:264
+::
+self.expect("expr 1 - 1", substrs=["0"])
+

shafik wrote:
> Maybe some more examples with alternatives would be helpful here.
Also mentioning why this check is bad would help spell it out. IIUC, it's 
because the full output will include `$0` (if it's the first expression, as 
noted); in which case, a stronger example is something that's clearly wrong:

```
(lldb) expr 5-3
(int) $0 = 2
```

In which case, `self.expect("expr 5 - 3", substrs=["0"])` passes, but shouldn't.



Comment at: lldb/docs/resources/test.rst:279-280
+self.assertTrue(expected_string in list_of_results)
+# Good. Will print expected_string and the contents of list_of_results.
+self.assertIn(expected_string, list_of_results) # Good.
+

nit: extra "# Good"


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

https://reviews.llvm.org/D101153

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

[Lldb-commits] [PATCH] D101153: [lldb][NFC] Specify guidelines for API tests

[Shafik Yaghmour via Phabricator via lldb-commits]

shafik added a comment.

Thank you, this is awesome.




Comment at: lldb/docs/resources/test.rst:206
+
+**Don't unnecessarily launch the test executable.**
+Launching a process and running to a breakpoint can often be the most

While I agree with this, it also feels unhelpful because it does not give any 
examples nor explain the alternatives.

I can see the problem with pointing at specific tests which may disappear or 
change. I can also see the problem with attempting to enumerate all the 
possibilities below this as well.

Maybe we need a set of example tests as well?

Most of the rest of advice stands alone pretty well though. 



Comment at: lldb/docs/resources/test.rst:264
+::
+self.expect("expr 1 - 1", substrs=["0"])
+

Maybe some more examples with alternatives would be helpful here.


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

https://reviews.llvm.org/D101153

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

Re: [Lldb-commits] [PATCH] D100898: [CMake][lldb] add_lldb_library's functions require default visibility

[Jim Radford via lldb-commits]

Yes, this IMO should eventually be addressed by explicitly exporting each 
meant-to-be-exported function with LLDB_ABI and then changing the line this 
patch adds to use `hidden` instead of `default`.

This patch is meant to be a band-aid until that can be done.

> On Apr 20, 2021, at 11:58 PM, Pavel Labath via Phabricator 
>  wrote:
> 
> labath added a comment.
> 
> Is there a way to address this through the preprocessor? We already have the 
> `LLDB_API` macro, which expands to `__declspec(dllexport/import)` on windows. 
> Would expanding it do `__attribute__((visibility("default")))` (or something) 
> on other platforms help with anything?
> 
> 
> Repository:
>  rG LLVM Github Monorepo
> 
> CHANGES SINCE LAST ACTION
>  https://reviews.llvm.org/D100898/new/
> 
> https://reviews.llvm.org/D100898
> 

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

[Lldb-commits] [PATCH] D99484: Use `GNUInstallDirs` to support custom installation dirs.

[John Ericson via Phabricator via lldb-commits]

Ericson2314 updated this revision to Diff 339639.
Ericson2314 added a comment.

Resplit on @LebedevRI's advice that it's OK if not all patches build alone


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99484

Files:
  clang-tools-extra/clang-doc/tool/CMakeLists.txt
  clang-tools-extra/clang-include-fixer/find-all-symbols/tool/CMakeLists.txt
  clang-tools-extra/clang-include-fixer/tool/CMakeLists.txt
  clang-tools-extra/clang-tidy/CMakeLists.txt
  clang-tools-extra/clang-tidy/tool/CMakeLists.txt
  clang-tools-extra/modularize/CMakeLists.txt
  clang/CMakeLists.txt
  clang/cmake/modules/AddClang.cmake
  clang/tools/c-index-test/CMakeLists.txt
  clang/tools/clang-format/CMakeLists.txt
  clang/tools/clang-rename/CMakeLists.txt
  clang/tools/libclang/CMakeLists.txt
  clang/tools/scan-build/CMakeLists.txt
  clang/tools/scan-view/CMakeLists.txt
  clang/utils/hmaptool/CMakeLists.txt
  flang/CMakeLists.txt
  flang/cmake/modules/AddFlang.cmake
  flang/tools/f18/CMakeLists.txt
  flang/tools/flang-driver/CMakeLists.txt
  libc/CMakeLists.txt
  libcxx/CMakeLists.txt
  libcxx/cmake/Modules/HandleLibCXXABI.cmake
  libcxx/include/CMakeLists.txt
  libcxx/src/CMakeLists.txt
  libcxxabi/CMakeLists.txt
  libunwind/CMakeLists.txt
  libunwind/src/CMakeLists.txt
  lld/CMakeLists.txt
  lld/cmake/modules/AddLLD.cmake
  lld/tools/lld/CMakeLists.txt
  lldb/CMakeLists.txt
  lldb/cmake/modules/AddLLDB.cmake
  lldb/cmake/modules/LLDBConfig.cmake
  mlir/CMakeLists.txt
  mlir/cmake/modules/AddMLIR.cmake
  openmp/CMakeLists.txt
  openmp/runtime/src/CMakeLists.txt
  openmp/tools/multiplex/CMakeLists.txt
  polly/CMakeLists.txt
  polly/cmake/CMakeLists.txt
  polly/lib/External/CMakeLists.txt
  pstl/CMakeLists.txt

Index: pstl/CMakeLists.txt
===
--- pstl/CMakeLists.txt
+++ pstl/CMakeLists.txt
@@ -7,6 +7,8 @@
 #===--===##
 cmake_minimum_required(VERSION 3.13.4)
 
+include(GNUInstallDirs)
+
 set(PARALLELSTL_VERSION_FILE "${CMAKE_CURRENT_SOURCE_DIR}/include/pstl/internal/pstl_config.h")
 file(STRINGS "${PARALLELSTL_VERSION_FILE}" PARALLELSTL_VERSION_SOURCE REGEX "#define _PSTL_VERSION .*$")
 string(REGEX REPLACE "#define _PSTL_VERSION (.*)$" "\\1" PARALLELSTL_VERSION_SOURCE "${PARALLELSTL_VERSION_SOURCE}")
@@ -86,10 +88,10 @@
   "${CMAKE_CURRENT_BINARY_DIR}/ParallelSTLConfigVersion.cmake"
 DESTINATION lib/cmake/ParallelSTL)
 install(DIRECTORY include/
-DESTINATION include
+DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}
 PATTERN "*.in" EXCLUDE)
 install(FILES "${PSTL_CONFIG_SITE_PATH}"
-DESTINATION include)
+DESTINATION ${CMAKE_INSTALL_INCLUDEDIR})
 
 add_custom_target(install-pstl
   COMMAND "${CMAKE_COMMAND}" -P "${PROJECT_BINARY_DIR}/cmake_install.cmake" -DCOMPONENT=ParallelSTL)
Index: polly/lib/External/CMakeLists.txt
===
--- polly/lib/External/CMakeLists.txt
+++ polly/lib/External/CMakeLists.txt
@@ -290,7 +290,7 @@
 install(DIRECTORY
   ${ISL_SOURCE_DIR}/include/
   ${ISL_BINARY_DIR}/include/
-  DESTINATION include/polly
+  DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}/polly
   FILES_MATCHING
   PATTERN "*.h"
   PATTERN "CMakeFiles" EXCLUDE
Index: polly/cmake/CMakeLists.txt
===
--- polly/cmake/CMakeLists.txt
+++ polly/cmake/CMakeLists.txt
@@ -83,14 +83,15 @@
 set(POLLY_CONFIG_LLVM_CMAKE_DIR "${LLVM_BINARY_DIR}/${LLVM_INSTALL_PACKAGE_DIR}")
 set(POLLY_CONFIG_CMAKE_DIR "${POLLY_INSTALL_PREFIX}/${POLLY_INSTALL_PACKAGE_DIR}")
 set(POLLY_CONFIG_LIBRARY_DIRS "${POLLY_INSTALL_PREFIX}/lib${LLVM_LIBDIR_SUFFIX}")
+get_filename_component(base_includedir "${CMAKE_INSTALL_INCLUDEDIR}" ABSOLUTE BASE_DIR "${POLLY_INSTALL_PREFIX}")
 if (POLLY_BUNDLED_ISL)
   set(POLLY_CONFIG_INCLUDE_DIRS
-"${POLLY_INSTALL_PREFIX}/include"
-"${POLLY_INSTALL_PREFIX}/include/polly"
+"${base_includedir}"
+"${base_includedir}/polly"
 )
 else()
   set(POLLY_CONFIG_INCLUDE_DIRS
-"${POLLY_INSTALL_PREFIX}/include"
+"${base_includedir}"
 ${ISL_INCLUDE_DIRS}
 )
 endif()
Index: polly/CMakeLists.txt
===
--- polly/CMakeLists.txt
+++ polly/CMakeLists.txt
@@ -2,7 +2,11 @@
 if (NOT DEFINED LLVM_MAIN_SRC_DIR)
   project(Polly)
   cmake_minimum_required(VERSION 3.13.4)
+endif()
+
+include(GNUInstallDirs)
 
+if (NOT DEFINED LLVM_MAIN_SRC_DIR)
   # Where is LLVM installed?
   find_package(LLVM CONFIG REQUIRED)
   set(CMAKE_MODULE_PATH ${CMAKE_MODULE_PATH} ${LLVM_CMAKE_DIR})
@@ -122,13 +126,13 @@
 
 if (NOT LLVM_INSTALL_TOOLCHAIN_ONLY)
   install(DIRECTORY include/
-DESTINATION include
+DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}
 FILES_MATCHING
 PATTERN 

[Lldb-commits] [PATCH] D99484: Use `GNUInstallDirs` to support custom installation dirs.

[John Ericson via Phabricator via lldb-commits]

Ericson2314 updated this revision to Diff 339626.
Ericson2314 added a comment.

Recombind revisions, need to convert everything at once


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99484

Files:
  clang-tools-extra/clang-doc/tool/CMakeLists.txt
  clang-tools-extra/clang-include-fixer/find-all-symbols/tool/CMakeLists.txt
  clang-tools-extra/clang-include-fixer/tool/CMakeLists.txt
  clang-tools-extra/clang-tidy/CMakeLists.txt
  clang-tools-extra/clang-tidy/tool/CMakeLists.txt
  clang-tools-extra/modularize/CMakeLists.txt
  clang/CMakeLists.txt
  clang/cmake/modules/AddClang.cmake
  clang/tools/c-index-test/CMakeLists.txt
  clang/tools/clang-format/CMakeLists.txt
  clang/tools/clang-rename/CMakeLists.txt
  clang/tools/libclang/CMakeLists.txt
  clang/tools/scan-build/CMakeLists.txt
  clang/tools/scan-view/CMakeLists.txt
  clang/utils/hmaptool/CMakeLists.txt
  flang/CMakeLists.txt
  flang/cmake/modules/AddFlang.cmake
  flang/tools/f18/CMakeLists.txt
  flang/tools/flang-driver/CMakeLists.txt
  libc/CMakeLists.txt
  libcxx/CMakeLists.txt
  libcxx/cmake/Modules/HandleLibCXXABI.cmake
  libcxx/include/CMakeLists.txt
  libcxx/src/CMakeLists.txt
  libcxxabi/CMakeLists.txt
  libunwind/CMakeLists.txt
  libunwind/src/CMakeLists.txt
  lld/CMakeLists.txt
  lld/cmake/modules/AddLLD.cmake
  lld/tools/lld/CMakeLists.txt
  lldb/CMakeLists.txt
  lldb/cmake/modules/AddLLDB.cmake
  lldb/cmake/modules/LLDBConfig.cmake
  llvm/CMakeLists.txt
  llvm/cmake/modules/AddLLVM.cmake
  llvm/cmake/modules/AddSphinxTarget.cmake
  llvm/cmake/modules/CMakeLists.txt
  llvm/cmake/modules/LLVMInstallSymlink.cmake
  llvm/docs/CMake.rst
  llvm/examples/Bye/CMakeLists.txt
  llvm/include/llvm/CMakeLists.txt
  llvm/tools/llvm-config/BuildVariables.inc.in
  llvm/tools/llvm-config/llvm-config.cpp
  llvm/tools/lto/CMakeLists.txt
  llvm/tools/opt-viewer/CMakeLists.txt
  llvm/tools/remarks-shlib/CMakeLists.txt
  mlir/CMakeLists.txt
  mlir/cmake/modules/AddMLIR.cmake
  openmp/CMakeLists.txt
  openmp/runtime/src/CMakeLists.txt
  openmp/tools/multiplex/CMakeLists.txt
  polly/CMakeLists.txt
  polly/cmake/CMakeLists.txt
  polly/lib/External/CMakeLists.txt
  pstl/CMakeLists.txt

Index: pstl/CMakeLists.txt
===
--- pstl/CMakeLists.txt
+++ pstl/CMakeLists.txt
@@ -7,6 +7,8 @@
 #===--===##
 cmake_minimum_required(VERSION 3.13.4)
 
+include(GNUInstallDirs)
+
 set(PARALLELSTL_VERSION_FILE "${CMAKE_CURRENT_SOURCE_DIR}/include/pstl/internal/pstl_config.h")
 file(STRINGS "${PARALLELSTL_VERSION_FILE}" PARALLELSTL_VERSION_SOURCE REGEX "#define _PSTL_VERSION .*$")
 string(REGEX REPLACE "#define _PSTL_VERSION (.*)$" "\\1" PARALLELSTL_VERSION_SOURCE "${PARALLELSTL_VERSION_SOURCE}")
@@ -86,10 +88,10 @@
   "${CMAKE_CURRENT_BINARY_DIR}/ParallelSTLConfigVersion.cmake"
 DESTINATION lib/cmake/ParallelSTL)
 install(DIRECTORY include/
-DESTINATION include
+DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}
 PATTERN "*.in" EXCLUDE)
 install(FILES "${PSTL_CONFIG_SITE_PATH}"
-DESTINATION include)
+DESTINATION ${CMAKE_INSTALL_INCLUDEDIR})
 
 add_custom_target(install-pstl
   COMMAND "${CMAKE_COMMAND}" -P "${PROJECT_BINARY_DIR}/cmake_install.cmake" -DCOMPONENT=ParallelSTL)
Index: polly/lib/External/CMakeLists.txt
===
--- polly/lib/External/CMakeLists.txt
+++ polly/lib/External/CMakeLists.txt
@@ -290,7 +290,7 @@
 install(DIRECTORY
   ${ISL_SOURCE_DIR}/include/
   ${ISL_BINARY_DIR}/include/
-  DESTINATION include/polly
+  DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}/polly
   FILES_MATCHING
   PATTERN "*.h"
   PATTERN "CMakeFiles" EXCLUDE
Index: polly/cmake/CMakeLists.txt
===
--- polly/cmake/CMakeLists.txt
+++ polly/cmake/CMakeLists.txt
@@ -83,14 +83,15 @@
 set(POLLY_CONFIG_LLVM_CMAKE_DIR "${LLVM_BINARY_DIR}/${LLVM_INSTALL_PACKAGE_DIR}")
 set(POLLY_CONFIG_CMAKE_DIR "${POLLY_INSTALL_PREFIX}/${POLLY_INSTALL_PACKAGE_DIR}")
 set(POLLY_CONFIG_LIBRARY_DIRS "${POLLY_INSTALL_PREFIX}/lib${LLVM_LIBDIR_SUFFIX}")
+get_filename_component(base_includedir "${CMAKE_INSTALL_INCLUDEDIR}" ABSOLUTE BASE_DIR "${POLLY_INSTALL_PREFIX}")
 if (POLLY_BUNDLED_ISL)
   set(POLLY_CONFIG_INCLUDE_DIRS
-"${POLLY_INSTALL_PREFIX}/include"
-"${POLLY_INSTALL_PREFIX}/include/polly"
+"${base_includedir}"
+"${base_includedir}/polly"
 )
 else()
   set(POLLY_CONFIG_INCLUDE_DIRS
-"${POLLY_INSTALL_PREFIX}/include"
+"${base_includedir}"
 ${ISL_INCLUDE_DIRS}
 )
 endif()
Index: polly/CMakeLists.txt
===
--- polly/CMakeLists.txt
+++ polly/CMakeLists.txt
@@ -2,7 +2,11 @@
 if (NOT DEFINED LLVM_MAIN_SRC_DIR)
   

[Lldb-commits] [PATCH] D62732: [RISCV] Add SystemV ABI

[Jade via Phabricator via lldb-commits]

jade added a comment.

In D62732#2306055 , @labath wrote:

> ABI plugins are one of the hardest things to test in lldb, particularly 
> without actual hardware. That's why we've let them be added in the past 
> without any accompanying tests. The situation is not ideal though, because 
> we've accumulated various ABI plugins which are hard to change without 
> breaking (or even to know if they work) because they only work with 
> third-party (possibly proprietary) stubs.
>
> I'm not sure what's the state of risc-v hardware these days and how much 
> resources do you have available, but if it's at all possible, I'd definitely 
> recommend adding the lldb-server bits for risc-v and adding a builtbot for 
> testing this configuration.
>
> Even if there's no hardware which could run the lldb test suite in a 
> reasonable amount of time, the availability of an lldb-server implementation 
> would enable anyone to test this via qemu -- that's the strategy we're 
> pursuing for the ARM SVE stuff, and we're about to add instructions on how to 
> run lldb+sve+qemu here: D82064 .

This is also testable with the gdbserver built into qemu, at least for kernel 
debugging. It seems to work, although for some reason, step-out is broken for 
me on a rust target (https://github.com/lf-/mu):

  (lldb) fin
  error: Could not create return address breakpoint.

I have yet to debug this but I think I will do so when I get more time.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62732

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

[Lldb-commits] [PATCH] D99484: Use `GNUInstallDirs` to support custom installation dirs.

[John Ericson via Phabricator via lldb-commits]

Ericson2314 updated this revision to Diff 338697.
Ericson2314 added a comment.

Rebase


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99484

Files:
  clang-tools-extra/clang-doc/tool/CMakeLists.txt
  clang-tools-extra/clang-include-fixer/find-all-symbols/tool/CMakeLists.txt
  clang-tools-extra/clang-include-fixer/tool/CMakeLists.txt
  clang-tools-extra/clang-tidy/CMakeLists.txt
  clang-tools-extra/clang-tidy/tool/CMakeLists.txt
  clang-tools-extra/modularize/CMakeLists.txt
  clang/CMakeLists.txt
  clang/cmake/modules/AddClang.cmake
  clang/tools/c-index-test/CMakeLists.txt
  clang/tools/clang-format/CMakeLists.txt
  clang/tools/clang-rename/CMakeLists.txt
  clang/tools/libclang/CMakeLists.txt
  clang/tools/scan-build/CMakeLists.txt
  clang/tools/scan-view/CMakeLists.txt
  clang/utils/hmaptool/CMakeLists.txt
  flang/CMakeLists.txt
  flang/cmake/modules/AddFlang.cmake
  flang/tools/f18/CMakeLists.txt
  flang/tools/flang-driver/CMakeLists.txt
  libc/CMakeLists.txt
  libcxx/CMakeLists.txt
  libcxx/cmake/Modules/HandleLibCXXABI.cmake
  libcxx/include/CMakeLists.txt
  libcxx/src/CMakeLists.txt
  libcxxabi/CMakeLists.txt
  libunwind/CMakeLists.txt
  libunwind/src/CMakeLists.txt
  lld/CMakeLists.txt
  lld/cmake/modules/AddLLD.cmake
  lld/tools/lld/CMakeLists.txt
  lldb/CMakeLists.txt
  lldb/cmake/modules/AddLLDB.cmake
  lldb/cmake/modules/LLDBConfig.cmake
  mlir/CMakeLists.txt
  mlir/cmake/modules/AddMLIR.cmake
  openmp/CMakeLists.txt
  openmp/runtime/src/CMakeLists.txt
  openmp/tools/multiplex/CMakeLists.txt
  polly/CMakeLists.txt
  polly/cmake/CMakeLists.txt
  polly/lib/External/CMakeLists.txt
  pstl/CMakeLists.txt

Index: pstl/CMakeLists.txt
===
--- pstl/CMakeLists.txt
+++ pstl/CMakeLists.txt
@@ -7,6 +7,8 @@
 #===--===##
 cmake_minimum_required(VERSION 3.13.4)
 
+include(GNUInstallDirs)
+
 set(PARALLELSTL_VERSION_FILE "${CMAKE_CURRENT_SOURCE_DIR}/include/pstl/internal/pstl_config.h")
 file(STRINGS "${PARALLELSTL_VERSION_FILE}" PARALLELSTL_VERSION_SOURCE REGEX "#define _PSTL_VERSION .*$")
 string(REGEX REPLACE "#define _PSTL_VERSION (.*)$" "\\1" PARALLELSTL_VERSION_SOURCE "${PARALLELSTL_VERSION_SOURCE}")
@@ -86,10 +88,10 @@
   "${CMAKE_CURRENT_BINARY_DIR}/ParallelSTLConfigVersion.cmake"
 DESTINATION lib/cmake/ParallelSTL)
 install(DIRECTORY include/
-DESTINATION include
+DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}
 PATTERN "*.in" EXCLUDE)
 install(FILES "${PSTL_CONFIG_SITE_PATH}"
-DESTINATION include)
+DESTINATION ${CMAKE_INSTALL_INCLUDEDIR})
 
 add_custom_target(install-pstl
   COMMAND "${CMAKE_COMMAND}" -P "${PROJECT_BINARY_DIR}/cmake_install.cmake" -DCOMPONENT=ParallelSTL)
Index: polly/lib/External/CMakeLists.txt
===
--- polly/lib/External/CMakeLists.txt
+++ polly/lib/External/CMakeLists.txt
@@ -275,7 +275,7 @@
 install(DIRECTORY
   ${ISL_SOURCE_DIR}/include/
   ${ISL_BINARY_DIR}/include/
-  DESTINATION include/polly
+  DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}/polly
   FILES_MATCHING
   PATTERN "*.h"
   PATTERN "CMakeFiles" EXCLUDE
Index: polly/cmake/CMakeLists.txt
===
--- polly/cmake/CMakeLists.txt
+++ polly/cmake/CMakeLists.txt
@@ -83,14 +83,15 @@
 set(POLLY_CONFIG_LLVM_CMAKE_DIR "${LLVM_BINARY_DIR}/${LLVM_INSTALL_PACKAGE_DIR}")
 set(POLLY_CONFIG_CMAKE_DIR "${POLLY_INSTALL_PREFIX}/${POLLY_INSTALL_PACKAGE_DIR}")
 set(POLLY_CONFIG_LIBRARY_DIRS "${POLLY_INSTALL_PREFIX}/lib${LLVM_LIBDIR_SUFFIX}")
+get_filename_component(base_includedir "${CMAKE_INSTALL_INCLUDEDIR}" ABSOLUTE BASE_DIR "${POLLY_INSTALL_PREFIX}")
 if (POLLY_BUNDLED_ISL)
   set(POLLY_CONFIG_INCLUDE_DIRS
-"${POLLY_INSTALL_PREFIX}/include"
-"${POLLY_INSTALL_PREFIX}/include/polly"
+"${base_includedir}"
+"${base_includedir}/polly"
 )
 else()
   set(POLLY_CONFIG_INCLUDE_DIRS
-"${POLLY_INSTALL_PREFIX}/include"
+"${base_includedir}"
 ${ISL_INCLUDE_DIRS}
 )
 endif()
Index: polly/CMakeLists.txt
===
--- polly/CMakeLists.txt
+++ polly/CMakeLists.txt
@@ -2,7 +2,11 @@
 if (NOT DEFINED LLVM_MAIN_SRC_DIR)
   project(Polly)
   cmake_minimum_required(VERSION 3.13.4)
+endif()
+
+include(GNUInstallDirs)
 
+if (NOT DEFINED LLVM_MAIN_SRC_DIR)
   # Where is LLVM installed?
   find_package(LLVM CONFIG REQUIRED)
   set(CMAKE_MODULE_PATH ${CMAKE_MODULE_PATH} ${LLVM_CMAKE_DIR})
@@ -122,13 +126,13 @@
 
 if (NOT LLVM_INSTALL_TOOLCHAIN_ONLY)
   install(DIRECTORY include/
-DESTINATION include
+DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}
 FILES_MATCHING
 PATTERN "*.h"
 )
 
   install(DIRECTORY ${POLLY_BINARY_DIR}/include/
-  

[Lldb-commits] [PATCH] D99812: [PowerPC] [GlobalISel] Implementation of formal arguments lowering in the IRTranslator for the PPC backend

[Matt Arsenault via Phabricator via lldb-commits]

arsenm added inline comments.



Comment at: llvm/lib/Target/PowerPC/GISel/PPCCallLowering.cpp:46-58
+/**
+ * @brief Lower incoming arguments into generic MIR, this method is responsible
+ *  for splitting aggregate arguments into multiple single value types as well
+ *  as setting argument flags for each argument. This method depends on a
+ *  calling convention selector to select the correct calling convention based
+ *  on the F.getCallingConv(). Finally, FormalArgHandler takes care of the reg
+ *  assignments.

There's no reason to duplicate the documentation effort from the function this 
is overriding. This will just add bitrotting comments



Comment at: llvm/lib/Target/PowerPC/GISel/PPCCallLowering.cpp:69-71
+  // initialize instruction writer
+  if (!MBB.empty())
+MIRBuilder.setInstr(*MBB.begin());

This is probably duplicated in every target, but I don't think it is necessary 
(or it shouldn't be, the caller should have set the insert point appropriately)



Comment at: llvm/lib/Target/PowerPC/GISel/PPCCallLowering.cpp:73
+
+  // loop over each arg, set flags and split to single value types
+  SmallVector InArgs;

Capitalize



Comment at: llvm/lib/Target/PowerPC/GISel/PPCCallLowering.cpp:99
+/**
+ * @brief generates COPYs of values to registers and G_TRUNCs them whenever
+ *  the bit widths mismatch. Formal arguments lowering depends on this method

Same as above



Comment at: llvm/lib/Target/PowerPC/GISel/PPCCallLowering.cpp:111-138
+  switch (VA.getLocInfo()) {
+  default: {
+// If we are copying the value from a physical register with the
+// size larger than the size of the value itself - build the copy
+// of the phys reg first and then build the truncation of that copy.
+// The example of that would be copying from xmm0 to s32, for which
+// case ValVT == LocVT == MVT::f32. If LocSize and ValSize are not equal

Can you just rely on the default implementation?

I've been working on a patch for a while to try to clean all of this up. 
Essentially what's happening is GlobalISel is using the CCAssignFns in a way 
that's subtly incompatible with how the DAG uses it. All of the code in this 
function ends up hacking around this, and I would at least prefer to keep this 
consolidated in one place to fix.



Comment at: llvm/lib/Target/PowerPC/GISel/PPCCallLowering.cpp:143
+/**
+ * @brief Generate a load instruction to load value from the given address.
+ * @param ValVReg

Same as above



Comment at: llvm/lib/Target/PowerPC/GISel/PPCCallLowering.cpp:168-169
+  // and then truncate it
+  if (VA.getLocInfo() == CCValAssign::SExt or
+  VA.getLocInfo() == CCValAssign::ZExt) {
+Size = 4;

You shouldn't rely on the extension hint to know if the result size is larger, 
it's not necessarily true



Comment at: llvm/lib/Target/PowerPC/GISel/PPCCallLowering.cpp:174
+  }
+  // otherwise, simply load the address into the destination register
+  else {

Weird comment between the control flow blocks


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99812

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

[Lldb-commits] [PATCH] D97786: LLDB: Use path relative to binary, not relative to debugger CWD, for finding .dwo files.

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

Since you just mainly want to run this past the bots again, probably best to 
just resubmit.  You could update the patch here or not at your convenience, but 
I don't see it would serve any purpose to wait on approval.

Sorry for not noticing it was a shell test, BTW...

In D97786#2694686 , @cmtice wrote:

> Hi Pavel,
>
> Thank you for both the detailed explanation and the updated test case (and 
> thank you Jim for your recommendations as well!).  I'm testing the updated 
> test case now.  Assuming the tests pass, is it ok for me to re-land this CL 
> using the updated test case?  and if so, do I need to upload the updated 
> patch to this code review?




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97786

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

[Lldb-commits] [PATCH] D97786: LLDB: Use path relative to binary, not relative to debugger CWD, for finding .dwo files.

[Caroline Tice via Phabricator via lldb-commits]

cmtice added a comment.

Pavel's test case passed on my local machine (x86_64 linux workstation).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97786

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

[Lldb-commits] [PATCH] D97786: LLDB: Use path relative to binary, not relative to debugger CWD, for finding .dwo files.

[Caroline Tice via Phabricator via lldb-commits]

cmtice added a comment.

Hi Pavel,

Thank you for both the detailed explanation and the updated test case (and 
thank you Jim for your recommendations as well!).  I'm testing the updated test 
case now.  Assuming the tests pass, is it ok for me to re-land this CL using 
the updated test case?  and if so, do I need to upload the updated patch to 
this code review?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97786

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

[Lldb-commits] [PATCH] D97786: LLDB: Use path relative to binary, not relative to debugger CWD, for finding .dwo files.

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

It looks like you've managed to find a bug in lldb. Congratulations. ;)

I don't yet fully understand what's going on, but the rough idea is this: Lldb 
relies heavily on knowing the boundaries of every function. For this reason, 
our ELF code contains logic to parse the unwind info for an object file, and 
use it to create fake (synthetic) symbols if we don't have real ones 
(ObjectFileELF::ParseUnwindSymbols). The first bug is that it tries to create a 
new symbol for the unwind info of main. This is probably due to incomplete 
handling of unlinked files. The second bug is that the code assigns 
`symtab.size()` as the ID of the new symbol. This is a problem, because the 
symtab can contain real symbols with IDs larger than that (because we skip some 
useless symbols when creating lldb Symbol objects). This seems to be the case 
with this test file which contains an undefined STT_NOTYPE symbol as a first 
entry.  (I'm not sure why is that the case, nor if this is specific to this 
test file). In any case, the result of all this is that we end up with two 
symbols with ID 12 ("x", and the synthetic one) and we end up picking one 
nondeterministically when resolving relocations for `x`.

In this case, windows actually chooses the right symbol, which is why it 
correctly ends up printing `x = 10`. On linux, we pick the wrong one, and `x` 
ends up pointing to a random block of zeroes.

I'm going to look into these issues, but for the time being, you can just work 
around the issue by removing the `main` function, which will stop all of these 
bad things from happening. I have attached a reduced version of this test file, 
which should work correctly everywhere. (Strictly speaking, it would've been 
enough to remove the .cfi directives, but while I was in there, I took the 
opportunity to remove other unnecessary cruft as well)

F16352196: dwo-relative-path.s 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97786

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

[Lldb-commits] [PATCH] D98179: [lit] Sort test start times based on prior test timing data

[David Zarzycki via Phabricator via lldb-commits]

davezarzycki added a comment.

In D98179#2689079 , @lebedev.ri wrote:

> In D98179#2689075 , @mstorsjo wrote:
>
>> Something related to the time recording seems to fail intermittently on 
>> buildbots: https://lab.llvm.org/buildbot#builders/93/builds/2697
>
> I'll cut to the fix here: @davezarzycki is there any reason why the timing 
> data is stored in a CSV format, and not JSON?
> The fix is to switch to latter.

What? Weird. The file isn't CSV. It's just a number followed by a space and 
then everything until the newline is the file name. Nothing fancy.  Switching 
to JSON probably just means papering over the real problem which seems to be 
either that sometimes a test has an empty string for a name or that there is an 
edge case in python's number printing that can result in pure whitespace. We'd 
need to look at the timing data file to see why it's bogus (or corrupt).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98179

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

[Lldb-commits] [PATCH] D97786: LLDB: Use path relative to binary, not relative to debugger CWD, for finding .dwo files.

[Jordan Rupprecht via Phabricator via lldb-commits]

rupprecht added a comment.

In D97786#2693410 , @jingham wrote:

> In D97786#2693381 , @cmtice wrote:
>
>> I had to revert this change because the test case broke the windows builder. 
>>  What's the right way to update/mark the test case so that it is only run on 
>> appropriate architectures & operating systems?
>
> You add the python skipIfWindows "decorator" before each test case 
> definition.  Just grep around in the test/API directory and you'll see plenty 
> of instances of its use.

Since this is a shell (lit) test, I think you mean adding `# REQUIRES: windows` 
at the top of the test?

> This does seem like it should work on Windows, however.  Maybe some path 
> handling problem?

This would be my guess too. But the APIs used here seem to handle file paths 
generically...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97786

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

[Lldb-commits] [PATCH] D98179: [lit] Sort test start times based on prior test timing data

[Roman Lebedev via Phabricator via lldb-commits]

lebedev.ri added a comment.

In D98179#2689075 , @mstorsjo wrote:

> Something related to the time recording seems to fail intermittently on 
> buildbots: https://lab.llvm.org/buildbot#builders/93/builds/2697

I'll cut to the fix here: @davezarzycki is there any reason why the timing data 
is stored in a CSV format, and not JSON.
The fix is to switch to latter.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98179

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

[Lldb-commits] [PATCH] D98179: [lit] Sort test start times based on prior test timing data

[Martin Storsjö via Phabricator via lldb-commits]

mstorsjo added a comment.

Something related to the time recording seems to fail intermittently on 
buildbots: https://lab.llvm.org/buildbot#builders/93/builds/2697


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98179

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

[Lldb-commits] [PATCH] D99812: [PowerPC] [GlobalISel] Implementation of formal arguments lowering in the IRTranslator for the PPC backend

[Anshil Gandhi via Phabricator via lldb-commits]

gandhi21299 marked 2 inline comments as done.
gandhi21299 added a comment.

I am looking towards more feedback on this patch, please do follow up at your 
convenience.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99812

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

[Lldb-commits] [PATCH] D100962: [lldb/Utility] Add SourceLocationSpec class (NFC)

[Med Ismail Bennani via Phabricator via lldb-commits]

mib marked 7 inline comments as done.
mib added inline comments.



Comment at: lldb/source/Utility/SourceLocationSpec.cpp:59-61
+bool SourceLocationSpec::operator!=(const SourceLocationSpec ) const {
+  return !(*this == rhs);
+}

JDevlieghere wrote:
> Isn't this the default implementation of `operator !=`? I think we can 
> probably omit it? 
No, it has to be implemented separately.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100962

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

[Lldb-commits] [PATCH] D101131: [lldb-vscode] Follow up of D99989 - store some strings more safely

[walter erquinigo via Phabricator via lldb-commits]

wallace added a comment.

TIL. I'll give it a try


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101131

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

[Lldb-commits] [PATCH] D101131: [lldb-vscode] Follow up of D99989 - store some strings more safely

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.



Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:2934
 // We first find out which variable names are duplicated
-llvm::DenseMap variable_name_counts;
+std::map variable_name_counts;
 for (auto i = start_idx; i < end_idx; ++i) {

Have you considered an`llvm::StringMap`, it should be more performant. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101131

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

[Lldb-commits] [PATCH] D100965: [lldb] Refactor argument group by SourceLocationSpec (NFCI)

[Jonas Devlieghere via Phabricator via lldb-commits]

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

In several places you have `SourceLocationSpec::Create` followed by a 
`lldbassert`. That's not sufficient, because in a non-assert build, the 
lldbassert is going to print a message but not halt the program and we'll crash 
when trying to dereference the expected. You'll need to add proper error 
handling to those places to bail out if the location spec is not valid.




Comment at: lldb/include/lldb/Breakpoint/BreakpointResolverFileLine.h:63
   bool m_skip_prologue;
-  bool m_exact_match;
+  SourceLocationSpec m_location_spec;
 

Move this before the bool to reduce padding. 



Comment at: lldb/source/API/SBThread.cpp:856
 SymbolContextList sc_list;
-frame_sc.comp_unit->ResolveSymbolContext(step_file_spec, line,
- check_inlines, exact,
+frame_sc.comp_unit->ResolveSymbolContext(*location_spec,
  eSymbolContextLineEntry, sc_list);

This will crash in a non-assert build. You can't rely on the assert to make 
this unreachable. 



Comment at: lldb/source/Breakpoint/BreakpointResolverFileLine.cpp:274-275
+m_location_spec.GetLine());
+  if (m_location_spec.HasColumn())
+s->Printf("column = %u, ", *m_location_spec.GetColumn());
+  s->Printf("exact_match = %d", m_location_spec.GetExactMatch());

I'd get the column, check if it's valid, and use it, instead of calling 
`HasColumn` before `GetColumn`. 



Comment at: lldb/source/Breakpoint/BreakpointResolverFileRegex.cpp:116
+lldbassert(location_spec && "Invalid SourceLocationSpec.");
+cu->ResolveSymbolContext(*location_spec, eSymbolContextEverything, 
sc_list);
 // Find all the function names:

Same here


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100965

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

[Lldb-commits] [PATCH] D100962: [lldb/Utility] Add SourceLocationSpec class (NFC)

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.



Comment at: lldb/include/lldb/Utility/SourceLocationSpec.h:13-15
+#include "lldb/lldb-defines.h"
+
+#include "llvm/ADT/Optional.h"





Comment at: lldb/include/lldb/Utility/SourceLocationSpec.h:108-138
+  /// Convert to boolean operator.
+  ///
+  /// This allows code to check a SourceLocationSpec object to see if it
+  /// contains anything valid using code such as:
+  ///
+  /// \code
+  /// SourceLocationSpec location_spec(...);

I guess this is  no longer needed now that the factory guarantees the spec is 
valid?



Comment at: lldb/include/lldb/Utility/SourceLocationSpec.h:183
+
+  bool HasColumn() const { return m_column.hasValue(); }
+

Is there any value to having this? I assume that if you want to know if there 
is a column, it's to use it after, so you might as well call get column and use 
the value if it's set. 



Comment at: lldb/include/lldb/Utility/SourceLocationSpec.h:195-196
+  llvm::Optional m_column;
+  bool m_check_inlines;
+  bool m_exact_match;
+};

Maybe add a Doxygen comment to specify what these two mean exactly. 



Comment at: lldb/source/Utility/SourceLocationSpec.cpp:59-61
+bool SourceLocationSpec::operator!=(const SourceLocationSpec ) const {
+  return !(*this == rhs);
+}

Isn't this the default implementation of `operator !=`? I think we can probably 
omit it? 



Comment at: lldb/unittests/Utility/SourceLocationSpecTest.cpp:96
+
+  // mutating FileSpec + const Inlined, ExactMatch, Line
+  EXPECT_TRUE(





Comment at: lldb/unittests/Utility/SourceLocationSpecTest.cpp:136-143
+  auto Create =
+  [](bool check_inlines, bool exact_match, FileSpec fs, uint32_t line,
+ uint16_t column = LLDB_INVALID_COLUMN_NUMBER) -> SourceLocationSpec {
+auto location = SourceLocationSpec::Create(fs, line, column, check_inlines,
+   exact_match);
+lldbassert(location && "Invalid SourceLocationSpec.");
+return *location;

This looks identical to the `Create` above. If it is, make it a static function 
to avoid code duplication. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100962

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

[Lldb-commits] [PATCH] D101153: [lldb][NFC] Specify guidelines for API tests

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

Thanks for taking all the lore around API tests and writing it down. I think 
this is going to be very useful and make our lives easier during code review.

This LGTM but I won't accept (yet) to give others a chance to see this show up 
in their review queue.




Comment at: lldb/docs/resources/test.rst:215
+and it makes reproducing test failures on other operating systems or
+configurations harder. There is usually
+

Seems like the last sentence is incomplete? 



Comment at: lldb/docs/resources/test.rst:229
+and makes the test much harder to debug and maintain. The test programs
+should always be deterministic (i.e., do not generate and check against
+random test values).




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

https://reviews.llvm.org/D101153

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

[Lldb-commits] [PATCH] D101157: [lldb] [test] Add tests for core dumps of multithreaded programs [WIP]

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

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

Complete set of tests.


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

https://reviews.llvm.org/D101157

Files:
  lldb/test/Shell/Register/Core/Inputs/multithread.cpp
  lldb/test/Shell/Register/Core/Inputs/x86-32-freebsd-multithread.core
  lldb/test/Shell/Register/Core/Inputs/x86-32-linux-multithread.core
  lldb/test/Shell/Register/Core/Inputs/x86-32-netbsd-multithread.core
  lldb/test/Shell/Register/Core/Inputs/x86-64-freebsd-multithread.core
  lldb/test/Shell/Register/Core/Inputs/x86-64-linux-multithread.core
  lldb/test/Shell/Register/Core/Inputs/x86-64-netbsd-multithread.core
  lldb/test/Shell/Register/Core/x86-32-freebsd-multithread.test
  lldb/test/Shell/Register/Core/x86-32-linux-multithread.test
  lldb/test/Shell/Register/Core/x86-32-netbsd-multithread.test
  lldb/test/Shell/Register/Core/x86-64-freebsd-multithread.test
  lldb/test/Shell/Register/Core/x86-64-linux-multithread.test
  lldb/test/Shell/Register/Core/x86-64-netbsd-multithread.test

Index: lldb/test/Shell/Register/Core/x86-64-netbsd-multithread.test
===
--- /dev/null
+++ lldb/test/Shell/Register/Core/x86-64-netbsd-multithread.test
@@ -0,0 +1,41 @@
+# RUN: %lldb -b -s %s -c %p/Inputs/x86-64-netbsd-multithread.core | FileCheck %s
+
+thread list
+# CHECK: * thread #1: tid = 2, 0x00400f82, stop reason = signal SIGSEGV 
+# CHECK-NEXT:   thread #2: tid = 4, 0x00400f88, stop reason = signal 0 
+# CHECK-NEXT:   thread #3: tid = 3, 0x00400f88, stop reason = signal 0 
+# CHECK-NEXT:   thread #4: tid = 1, 0x791280ca1faa, stop reason = signal 0 
+
+register read --all
+# CHECK-DAG: ecx = 0x04040404
+# CHECK-DAG: edx = 0x03030303
+# CHECK-DAG: edi = 0x01010101
+# CHECK-DAG: esi = 0x02020202
+# CHECK-DAG: xmm0 = {0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x40 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00}
+# CHECK-DAG: xmm1 = {0x00 0x00 0x00 0x00 0x00 0x00 0x10 0x40 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00}
+# CHECK-DAG: xmm2 = {0x00 0x00 0x00 0x00 0x00 0x00 0x20 0x40 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00}
+# CHECK-DAG: xmm3 = {0x00 0x00 0x00 0x00 0x00 0x00 0x30 0x40 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00}
+
+thread select 3
+# CHECK: (lldb) thread select 3
+register read --all
+# CHECK-DAG: ecx = 0x14141414
+# CHECK-DAG: edx = 0x13131313
+# CHECK-DAG: edi = 0x
+# CHECK-DAG: esi = 0x12121212
+# CHECK-DAG: xmm0 = {0x00 0x00 0x00 0x00 0x00 0x00 0x08 0x40 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00}
+# CHECK-DAG: xmm1 = {0x00 0x00 0x00 0x00 0x00 0x00 0x18 0x40 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00}
+# CHECK-DAG: xmm2 = {0x00 0x00 0x00 0x00 0x00 0x00 0x22 0x40 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00}
+# CHECK-DAG: xmm3 = {0x00 0x00 0x00 0x00 0x00 0x00 0x28 0x40 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00}
+
+thread select 2
+# CHECK: (lldb) thread select 2
+register read --all
+# CHECK-DAG: ecx = 0x24242424
+# CHECK-DAG: edx = 0x23232323
+# CHECK-DAG: edi = 0x21212121
+# CHECK-DAG: esi = 0x
+# CHECK-DAG: xmm0 = {0x00 0x00 0x00 0x00 0x00 0x00 0x14 0x40 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00}
+# CHECK-DAG: xmm1 = {0x00 0x00 0x00 0x00 0x00 0x00 0x24 0x40 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00}
+# CHECK-DAG: xmm2 = {0x00 0x00 0x00 0x00 0x00 0x00 0x2e 0x40 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00}
+# CHECK-DAG: xmm3 = {0x00 0x00 0x00 0x00 0x00 0x00 0x34 0x40 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00}
Index: lldb/test/Shell/Register/Core/x86-64-linux-multithread.test
===
--- /dev/null
+++ lldb/test/Shell/Register/Core/x86-64-linux-multithread.test
@@ -0,0 +1,41 @@
+# RUN: %lldb -b -s %s -c %p/Inputs/x86-64-linux-multithread.core | FileCheck %s
+
+thread list
+# CHECK: * thread #1: tid = 329384, 0x00401262, name = 'a.out', stop reason = signal SIGSEGV
+# CHECK-NEXT:   thread #2: tid = 329385, 0x0040126d, stop reason = signal 0
+# CHECK-NEXT:   thread #3: tid = 329386, 0x0040126d, stop reason = signal 0
+# CHECK-NEXT:   thread #4: tid = 329383, 0x7fcf5582f762, stop reason = signal 0
+
+register read --all
+# CHECK-DAG: ecx = 0x04040404
+# CHECK-DAG: edx = 0x03030303
+# CHECK-DAG: edi = 0x01010101
+# CHECK-DAG: esi = 0x02020202
+# CHECK-DAG: xmm0 = {0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x40 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00}
+# CHECK-DAG: xmm1 = {0x00 0x00 0x00 0x00 0x00 0x00 0x10 0x40 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00}
+# CHECK-DAG: xmm2 = {0x00 0x00 0x00 0x00 0x00 0x00 0x20 0x40 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00}
+# CHECK-DAG: xmm3 = {0x00 0x00 0x00 0x00 0x00 0x00 0x30 0x40 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00}
+
+thread select 2
+# CHECK: (lldb) thread select 2
+register read --all
+# CHECK-DAG: ecx = 0x14141414
+# CHECK-DAG: edx = 0x13131313
+# CHECK-DAG: edi = 0x
+# CHECK-DAG: esi = 0x12121212
+# CHECK-DAG: xmm0 = {0x00 

[Lldb-commits] [PATCH] D101157: [lldb] [test] Add tests for core dumps of multithreaded programs [WIP]

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

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

Now with complete set of amd64 tests.


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

https://reviews.llvm.org/D101157

Files:
  lldb/test/Shell/Register/Core/Inputs/multithread.cpp
  lldb/test/Shell/Register/Core/Inputs/x86-32-freebsd-multithread.core
  lldb/test/Shell/Register/Core/Inputs/x86-32-linux-multithread.core
  lldb/test/Shell/Register/Core/Inputs/x86-32-netbsd-multithread.core
  lldb/test/Shell/Register/Core/Inputs/x86-64-freebsd-multithread.core
  lldb/test/Shell/Register/Core/Inputs/x86-64-linux-multithread.core
  lldb/test/Shell/Register/Core/Inputs/x86-64-netbsd-multithread.core
  lldb/test/Shell/Register/Core/x86-64-freebsd-multithread.test
  lldb/test/Shell/Register/Core/x86-64-linux-multithread.test
  lldb/test/Shell/Register/Core/x86-64-netbsd-multithread.test

Index: lldb/test/Shell/Register/Core/x86-64-netbsd-multithread.test
===
--- /dev/null
+++ lldb/test/Shell/Register/Core/x86-64-netbsd-multithread.test
@@ -0,0 +1,41 @@
+# RUN: %lldb -b -s %s -c %p/Inputs/x86-64-netbsd-multithread.core | FileCheck %s
+
+thread list
+# CHECK: * thread #1: tid = 2, 0x00400f82, stop reason = signal SIGSEGV 
+# CHECK-NEXT:   thread #2: tid = 4, 0x00400f88, stop reason = signal 0 
+# CHECK-NEXT:   thread #3: tid = 3, 0x00400f88, stop reason = signal 0 
+# CHECK-NEXT:   thread #4: tid = 1, 0x791280ca1faa, stop reason = signal 0 
+
+register read --all
+# CHECK-DAG: ecx = 0x04040404
+# CHECK-DAG: edx = 0x03030303
+# CHECK-DAG: edi = 0x01010101
+# CHECK-DAG: esi = 0x02020202
+# CHECK-DAG: xmm0 = {0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x40 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00}
+# CHECK-DAG: xmm1 = {0x00 0x00 0x00 0x00 0x00 0x00 0x10 0x40 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00}
+# CHECK-DAG: xmm2 = {0x00 0x00 0x00 0x00 0x00 0x00 0x20 0x40 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00}
+# CHECK-DAG: xmm3 = {0x00 0x00 0x00 0x00 0x00 0x00 0x30 0x40 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00}
+
+thread select 3
+# CHECK: (lldb) thread select 3
+register read --all
+# CHECK-DAG: ecx = 0x14141414
+# CHECK-DAG: edx = 0x13131313
+# CHECK-DAG: edi = 0x
+# CHECK-DAG: esi = 0x12121212
+# CHECK-DAG: xmm0 = {0x00 0x00 0x00 0x00 0x00 0x00 0x08 0x40 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00}
+# CHECK-DAG: xmm1 = {0x00 0x00 0x00 0x00 0x00 0x00 0x18 0x40 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00}
+# CHECK-DAG: xmm2 = {0x00 0x00 0x00 0x00 0x00 0x00 0x22 0x40 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00}
+# CHECK-DAG: xmm3 = {0x00 0x00 0x00 0x00 0x00 0x00 0x28 0x40 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00}
+
+thread select 2
+# CHECK: (lldb) thread select 2
+register read --all
+# CHECK-DAG: ecx = 0x24242424
+# CHECK-DAG: edx = 0x23232323
+# CHECK-DAG: edi = 0x21212121
+# CHECK-DAG: esi = 0x
+# CHECK-DAG: xmm0 = {0x00 0x00 0x00 0x00 0x00 0x00 0x14 0x40 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00}
+# CHECK-DAG: xmm1 = {0x00 0x00 0x00 0x00 0x00 0x00 0x24 0x40 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00}
+# CHECK-DAG: xmm2 = {0x00 0x00 0x00 0x00 0x00 0x00 0x2e 0x40 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00}
+# CHECK-DAG: xmm3 = {0x00 0x00 0x00 0x00 0x00 0x00 0x34 0x40 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00}
Index: lldb/test/Shell/Register/Core/x86-64-linux-multithread.test
===
--- /dev/null
+++ lldb/test/Shell/Register/Core/x86-64-linux-multithread.test
@@ -0,0 +1,41 @@
+# RUN: %lldb -b -s %s -c %p/Inputs/x86-64-linux-multithread.core | FileCheck %s
+
+thread list
+# CHECK: * thread #1: tid = 329384, 0x00401262, name = 'a.out', stop reason = signal SIGSEGV
+# CHECK-NEXT:   thread #2: tid = 329385, 0x0040126d, stop reason = signal 0
+# CHECK-NEXT:   thread #3: tid = 329386, 0x0040126d, stop reason = signal 0
+# CHECK-NEXT:   thread #4: tid = 329383, 0x7fcf5582f762, stop reason = signal 0
+
+register read --all
+# CHECK-DAG: ecx = 0x04040404
+# CHECK-DAG: edx = 0x03030303
+# CHECK-DAG: edi = 0x01010101
+# CHECK-DAG: esi = 0x02020202
+# CHECK-DAG: xmm0 = {0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x40 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00}
+# CHECK-DAG: xmm1 = {0x00 0x00 0x00 0x00 0x00 0x00 0x10 0x40 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00}
+# CHECK-DAG: xmm2 = {0x00 0x00 0x00 0x00 0x00 0x00 0x20 0x40 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00}
+# CHECK-DAG: xmm3 = {0x00 0x00 0x00 0x00 0x00 0x00 0x30 0x40 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00}
+
+thread select 2
+# CHECK: (lldb) thread select 2
+register read --all
+# CHECK-DAG: ecx = 0x14141414
+# CHECK-DAG: edx = 0x13131313
+# CHECK-DAG: edi = 0x
+# CHECK-DAG: esi = 0x12121212
+# CHECK-DAG: xmm0 = {0x00 0x00 0x00 0x00 0x00 0x00 0x08 0x40 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00}
+# CHECK-DAG: xmm1 = {0x00 0x00 0x00 0x00 0x00 0x00 0x18 0x40 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00}
+# CHECK-DAG: xmm2 = {0x00 0x00 0x00 

[Lldb-commits] [PATCH] D101157: [lldb] [test] Add tests for core dumps of multithreaded programs [WIP]

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

mgorny added inline comments.



Comment at: lldb/test/Shell/Register/Core/x86-64-linux-multithread.test:20
+thread select 2
+# CHECK: (lldb) thread select 2
+register read --all

Fun fact: it seems that `register read` outputs immediately while `thread 
select` can output after `register read` output. Weird.


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

https://reviews.llvm.org/D101157

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

[Lldb-commits] [PATCH] D101157: [lldb] [test] Add tests for core dumps of multithreaded programs [WIP]

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

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

Check for `thread select` instructions rather than output.


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

https://reviews.llvm.org/D101157

Files:
  lldb/test/Shell/Register/Core/Inputs/multithread.cpp
  lldb/test/Shell/Register/Core/Inputs/x86-32-freebsd-multithread.core
  lldb/test/Shell/Register/Core/Inputs/x86-32-linux-multithread.core
  lldb/test/Shell/Register/Core/Inputs/x86-32-netbsd-multithread.core
  lldb/test/Shell/Register/Core/Inputs/x86-64-freebsd-multithread.core
  lldb/test/Shell/Register/Core/Inputs/x86-64-linux-multithread.core
  lldb/test/Shell/Register/Core/Inputs/x86-64-netbsd-multithread.core
  lldb/test/Shell/Register/Core/x86-64-linux-multithread.test

Index: lldb/test/Shell/Register/Core/x86-64-linux-multithread.test
===
--- /dev/null
+++ lldb/test/Shell/Register/Core/x86-64-linux-multithread.test
@@ -0,0 +1,41 @@
+# RUN: %lldb -b -s %s -c %p/Inputs/x86-64-linux-multithread.core | FileCheck %s
+
+thread list
+# CHECK: * thread #1: tid = 329384, 0x00401262, name = 'a.out', stop reason = signal SIGSEGV
+# CHECK-NEXT:   thread #2: tid = 329385, 0x0040126d, stop reason = signal 0
+# CHECK-NEXT:   thread #3: tid = 329386, 0x0040126d, stop reason = signal 0
+# CHECK-NEXT:   thread #4: tid = 329383, 0x7fcf5582f762, stop reason = signal 0
+
+register read --all
+# CHECK-DAG: ecx = 0x04040404
+# CHECK-DAG: edx = 0x03030303
+# CHECK-DAG: edi = 0x01010101
+# CHECK-DAG: esi = 0x02020202
+# CHECK-DAG: xmm0 = {0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x40 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00}
+# CHECK-DAG: xmm1 = {0x00 0x00 0x00 0x00 0x00 0x00 0x10 0x40 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00}
+# CHECK-DAG: xmm2 = {0x00 0x00 0x00 0x00 0x00 0x00 0x20 0x40 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00}
+# CHECK-DAG: xmm3 = {0x00 0x00 0x00 0x00 0x00 0x00 0x30 0x40 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00}
+
+thread select 2
+# CHECK: (lldb) thread select 2
+register read --all
+# CHECK-DAG: ecx = 0x14141414
+# CHECK-DAG: edx = 0x13131313
+# CHECK-DAG: edi = 0x
+# CHECK-DAG: esi = 0x12121212
+# CHECK-DAG: xmm0 = {0x00 0x00 0x00 0x00 0x00 0x00 0x08 0x40 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00}
+# CHECK-DAG: xmm1 = {0x00 0x00 0x00 0x00 0x00 0x00 0x18 0x40 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00}
+# CHECK-DAG: xmm2 = {0x00 0x00 0x00 0x00 0x00 0x00 0x22 0x40 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00}
+# CHECK-DAG: xmm3 = {0x00 0x00 0x00 0x00 0x00 0x00 0x28 0x40 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00}
+
+thread select 3
+# CHECK: (lldb) thread select 3
+register read --all
+# CHECK-DAG: ecx = 0x24242424
+# CHECK-DAG: edx = 0x23232323
+# CHECK-DAG: edi = 0x21212121
+# CHECK-DAG: esi = 0x
+# CHECK-DAG: xmm0 = {0x00 0x00 0x00 0x00 0x00 0x00 0x14 0x40 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00}
+# CHECK-DAG: xmm1 = {0x00 0x00 0x00 0x00 0x00 0x00 0x24 0x40 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00}
+# CHECK-DAG: xmm2 = {0x00 0x00 0x00 0x00 0x00 0x00 0x2e 0x40 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00}
+# CHECK-DAG: xmm3 = {0x00 0x00 0x00 0x00 0x00 0x00 0x34 0x40 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00}
Index: lldb/test/Shell/Register/Core/Inputs/multithread.cpp
===
--- /dev/null
+++ lldb/test/Shell/Register/Core/Inputs/multithread.cpp
@@ -0,0 +1,77 @@
+// This program is used to generate a core dump for testing multithreading
+// support.
+
+#include 
+#include 
+#include 
+#include 
+
+std::atomic_int start_counter{3};
+
+void pseudobarrier_wait() {
+  start_counter--;
+  while (start_counter > 0);
+}
+
+void fcommon(uint32_t a, uint32_t b, uint32_t c, uint32_t d, double fa, double fb, double fc, double fd, bool segv) {
+  if (segv) {
+int *foo = nullptr;
+*foo = 0;
+  }
+  while (1);
+}
+
+void f1() {
+  volatile uint32_t a = 0x01010101;
+  volatile uint32_t b = 0x02020202;
+  volatile uint32_t c = 0x03030303;
+  volatile uint32_t d = 0x04040404;
+  volatile double fa = 2.0;
+  volatile double fb = 4.0;
+  volatile double fc = 8.0;
+  volatile double fd = 16.0;
+
+  pseudobarrier_wait();
+  std::this_thread::sleep_for(std::chrono::milliseconds(200));
+  fcommon(a, b, c, d, fa, fb, fc, fd, true);
+}
+
+void f2() {
+  volatile uint32_t a = 0x;
+  volatile uint32_t b = 0x12121212;
+  volatile uint32_t c = 0x13131313;
+  volatile uint32_t d = 0x14141414;
+  volatile double fa = 3.0;
+  volatile double fb = 6.0;
+  volatile double fc = 9.0;
+  volatile double fd = 12.0;
+
+  pseudobarrier_wait();
+  fcommon(a, b, c, d, fa, fb, fc, fd, false);
+}
+
+void f3() {
+  volatile uint32_t a = 0x21212121;
+  volatile uint32_t b = 0x;
+  volatile uint32_t c = 0x23232323;
+  volatile uint32_t d = 0x24242424;
+  volatile double fa = 5.0;
+  volatile double fb = 10.0;
+  volatile double fc = 15.0;
+  volatile double fd = 20.0;
+
+  pseudobarrier_wait();
+  

[Lldb-commits] [PATCH] D101157: [lldb] [test] Add tests for core dumps of multithreaded programs [WIP]

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

mgorny created this revision.
mgorny added reviewers: labath, krytarowski, emaste.
Herald added a subscriber: jfb.
mgorny requested review of this revision.

(adding for early review)


https://reviews.llvm.org/D101157

Files:
  lldb/test/Shell/Register/Core/Inputs/multithread.cpp
  lldb/test/Shell/Register/Core/Inputs/x86-32-freebsd-multithread.core
  lldb/test/Shell/Register/Core/Inputs/x86-32-linux-multithread.core
  lldb/test/Shell/Register/Core/Inputs/x86-32-netbsd-multithread.core
  lldb/test/Shell/Register/Core/Inputs/x86-64-freebsd-multithread.core
  lldb/test/Shell/Register/Core/Inputs/x86-64-linux-multithread.core
  lldb/test/Shell/Register/Core/Inputs/x86-64-netbsd-multithread.core
  lldb/test/Shell/Register/Core/x86-64-linux-multithread.test

Index: lldb/test/Shell/Register/Core/x86-64-linux-multithread.test
===
--- /dev/null
+++ lldb/test/Shell/Register/Core/x86-64-linux-multithread.test
@@ -0,0 +1,41 @@
+# RUN: %lldb -b -s %s -c %p/Inputs/x86-64-linux-multithread.core | FileCheck %s
+
+thread list
+# CHECK: * thread #1: tid = 329384, 0x00401262, name = 'a.out', stop reason = signal SIGSEGV
+# CHECK-NEXT:   thread #2: tid = 329385, 0x0040126d, stop reason = signal 0
+# CHECK-NEXT:   thread #3: tid = 329386, 0x0040126d, stop reason = signal 0
+# CHECK-NEXT:   thread #4: tid = 329383, 0x7fcf5582f762, stop reason = signal 0
+
+register read --all
+# CHECK-DAG: ecx = 0x04040404
+# CHECK-DAG: edx = 0x03030303
+# CHECK-DAG: edi = 0x01010101
+# CHECK-DAG: esi = 0x02020202
+# CHECK-DAG: xmm0 = {0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x40 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00}
+# CHECK-DAG: xmm1 = {0x00 0x00 0x00 0x00 0x00 0x00 0x10 0x40 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00}
+# CHECK-DAG: xmm2 = {0x00 0x00 0x00 0x00 0x00 0x00 0x20 0x40 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00}
+# CHECK-DAG: xmm3 = {0x00 0x00 0x00 0x00 0x00 0x00 0x30 0x40 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00}
+
+thread select 2
+# CHECK: * thread #2, stop reason = signal 0
+register read --all
+# CHECK-DAG: ecx = 0x14141414
+# CHECK-DAG: edx = 0x13131313
+# CHECK-DAG: edi = 0x
+# CHECK-DAG: esi = 0x12121212
+# CHECK-DAG: xmm0 = {0x00 0x00 0x00 0x00 0x00 0x00 0x08 0x40 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00}
+# CHECK-DAG: xmm1 = {0x00 0x00 0x00 0x00 0x00 0x00 0x18 0x40 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00}
+# CHECK-DAG: xmm2 = {0x00 0x00 0x00 0x00 0x00 0x00 0x22 0x40 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00}
+# CHECK-DAG: xmm3 = {0x00 0x00 0x00 0x00 0x00 0x00 0x28 0x40 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00}
+
+thread select 3
+# CHECK: * thread #3, stop reason = signal 0
+register read --all
+# CHECK-DAG: ecx = 0x24242424
+# CHECK-DAG: edx = 0x23232323
+# CHECK-DAG: edi = 0x21212121
+# CHECK-DAG: esi = 0x
+# CHECK-DAG: xmm0 = {0x00 0x00 0x00 0x00 0x00 0x00 0x14 0x40 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00}
+# CHECK-DAG: xmm1 = {0x00 0x00 0x00 0x00 0x00 0x00 0x24 0x40 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00}
+# CHECK-DAG: xmm2 = {0x00 0x00 0x00 0x00 0x00 0x00 0x2e 0x40 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00}
+# CHECK-DAG: xmm3 = {0x00 0x00 0x00 0x00 0x00 0x00 0x34 0x40 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00}
Index: lldb/test/Shell/Register/Core/Inputs/multithread.cpp
===
--- /dev/null
+++ lldb/test/Shell/Register/Core/Inputs/multithread.cpp
@@ -0,0 +1,77 @@
+// This program is used to generate a core dump for testing multithreading
+// support.
+
+#include 
+#include 
+#include 
+#include 
+
+std::atomic_int start_counter{3};
+
+void pseudobarrier_wait() {
+  start_counter--;
+  while (start_counter > 0);
+}
+
+void fcommon(uint32_t a, uint32_t b, uint32_t c, uint32_t d, double fa, double fb, double fc, double fd, bool segv) {
+  if (segv) {
+int *foo = nullptr;
+*foo = 0;
+  }
+  while (1);
+}
+
+void f1() {
+  volatile uint32_t a = 0x01010101;
+  volatile uint32_t b = 0x02020202;
+  volatile uint32_t c = 0x03030303;
+  volatile uint32_t d = 0x04040404;
+  volatile double fa = 2.0;
+  volatile double fb = 4.0;
+  volatile double fc = 8.0;
+  volatile double fd = 16.0;
+
+  pseudobarrier_wait();
+  std::this_thread::sleep_for(std::chrono::milliseconds(200));
+  fcommon(a, b, c, d, fa, fb, fc, fd, true);
+}
+
+void f2() {
+  volatile uint32_t a = 0x;
+  volatile uint32_t b = 0x12121212;
+  volatile uint32_t c = 0x13131313;
+  volatile uint32_t d = 0x14141414;
+  volatile double fa = 3.0;
+  volatile double fb = 6.0;
+  volatile double fc = 9.0;
+  volatile double fd = 12.0;
+
+  pseudobarrier_wait();
+  fcommon(a, b, c, d, fa, fb, fc, fd, false);
+}
+
+void f3() {
+  volatile uint32_t a = 0x21212121;
+  volatile uint32_t b = 0x;
+  volatile uint32_t c = 0x23232323;
+  volatile uint32_t d = 0x24242424;
+  volatile double fa = 5.0;
+  volatile double fb = 10.0;
+  volatile double fc = 15.0;
+  volatile double fd = 20.0;
+
+  

[Lldb-commits] [lldb] f3e6f85 - [lldb][NFC] Remove a stray unicode character in the LLDB test docs

[Raphael Isemann via lldb-commits]

Author: Raphael Isemann
Date: 2021-04-23T13:20:10+02:00
New Revision: f3e6f856c2905362f54acebc5ef500df10cb06fa

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

LOG: [lldb][NFC] Remove a stray unicode character in the LLDB test docs

There was a U+2028 character in this line (a special paragraph separator).

Added: 


Modified: 
lldb/docs/resources/test.rst

Removed: 




diff  --git a/lldb/docs/resources/test.rst b/lldb/docs/resources/test.rst
index 2c08ddde28dc4..4c70e131f0d73 100644
--- a/lldb/docs/resources/test.rst
+++ b/lldb/docs/resources/test.rst
@@ -156,7 +156,7 @@ test, the API test also allow for much more complex 
scenarios when it comes to
 building inferiors. Every test has its own ``Makefile``, most of them only a
 few lines long. A shared ``Makefile`` (``Makefile.rules``) with about a
 thousand lines of rules takes care of most if not all of the boiler plate,
-while individual make files can be used to build more advanced tests. 

+while individual make files can be used to build more advanced tests.
 
 Here's an example of a simple ``Makefile`` used by the example test.
 



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

[Lldb-commits] [PATCH] D101153: [lldb][NFC] Specify guidelines for API tests

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added a comment.

If anyone feels like any of the guidelines is actually controversial then let 
me know and I'll remove it from this review and split it out into its own patch.

(I am also aware that the text directly above has some small overlap with the 
guidelines as it for example also recommends the lldb utility functions for 
setup/checking. But given how frequently this comes up in patches I just put it 
in as its own guideline).


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

https://reviews.llvm.org/D101153

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

[Lldb-commits] [PATCH] D101153: [lldb][NFC] Specify guidelines for API tests

[Raphael Isemann via Phabricator via lldb-commits]

teemperor updated this revision to Diff 339978.
teemperor added a comment.

- Improve some wording.


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

https://reviews.llvm.org/D101153

Files:
  lldb/docs/resources/test.rst


Index: lldb/docs/resources/test.rst
===
--- lldb/docs/resources/test.rst
+++ lldb/docs/resources/test.rst
@@ -196,6 +196,89 @@
 variants mean that more general tests should be API tests, so that they can be
 run against the different variants.
 
+Guidelines for API tests
+
+
+API tests are expected to be fast, reliable and maintainable. To achieve this
+goal, API tests should conform to the following guidelines in addition to 
normal
+good testing practices.
+
+**Don't unnecessarily launch the test executable.**
+Launching a process and running to a breakpoint can often be the most
+expensive part of a test. While it's often required to have a running
+process to test a certain feature, sometimes its possible to test a feature
+with a (non-running) target.
+
+**Don't unnecessarily include system headers in test sources.**
+Including external headers slows down the compilation of the test 
executable
+and it makes reproducing test failures on other operating systems or
+configurations harder. There is usually
+
+**Avoid specifying test-specific compiler flags when including system 
headers.**
+If a test requires including a system header (e.g., a test for a libc++
+formatter includes a libc++ header), always avoid specifying custom 
compiler
+flags in the test's ``Makefile``. Certain debug information formats such as
+``gmodules`` need to recompile external headers when they encounter
+test-specific flags (including defines) which can be very expensive.
+
+**Test programs should be kept simple.**
+Test executables should do the minimum amount of work to bring the process
+into the state that is required for the test. Simulating a 'real' program
+that actually tries to do some useful task rarely helps with catching bugs
+and makes the test much harder to debug and maintain. The test programs
+should always be deterministic (i.e., do not generate and check against
+random test values).
+
+**Identifiers in tests should be simple and descriptive.**
+Often test program need to declare functions and classes which require
+choosing some form of identifier for them. These identifiers should always
+either be kept simple for small tests (e.g., ``A``, ``B``, ...) or have 
some
+descriptive name (e.g., ``ClassWithTailPadding``, ``inlined_func``, ...).
+Never choose identifiers that are already used anywhere else in LLVM or
+other programs (e.g., don't name a class  ``VirtualFileSystem``, a function
+``llvm_unreachable``, or a namespace ``rapidxml``) as this will just 
mislead
+people ``grep``'ing the LLVM repository for those strings.
+
+**Prefer LLDB testing utilities over directly working with the SB API.**
+The ``lldbutil`` module and the ``TestBase`` class come with a large amount
+of utility functions that can do common test setup tasks (e.g., starting a
+test executable and running the process to a breakpoint). Using these
+functions not only keeps the test shorter and free of duplicated code, but
+they also follow best test suite practices and usually give much clearer
+error messages if something goes wrong. The test utilities also contain
+custom asserts and checks that should be preferably used (e.g.
+``self.assertSuccess``).
+
+**Prefer calling the SB API over checking command output.**
+Avoid writing your tests on top of ``self.expect(...)`` calls that check
+the output of LLDB commands and instead try calling into the SB API. 
Relying
+on LLDB commands makes changing (and improving) the output/syntax of
+commands harder and the resulting tests are often prone to accepting
+incorrect test results. Especially improved error messages that contain
+more information might cause these ``self.expect`` calls to unintentionally
+find the required ``substrs``. For example, the following ``self.expect``
+check always passes as long as it's the first expression in a test which
+is far from ideal:
+
+::
+self.expect("expr 1 - 1", substrs=["0"])
+
+**Prefer using specific asserts over the generic assertTrue/assertFalse.**.
+The `self.assertTrue`/`self.assertFalse` functions should always be your
+last option as they give non-descriptive error messages. The test class has
+several expressive asserts such as `self.assertIn` that automatically
+generate an explanation how the received values differ from the expected
+ones. See the documentation of Python's `unittest` module to see what
+asserts are available. If you can't find a specific assert that fits your
+needs and you fall back to a 

[Lldb-commits] [PATCH] D101153: [lldb][NFC] Specify guidelines for API tests

[Raphael Isemann via Phabricator via lldb-commits]

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

This patch specifies a few guidelines that our API tests should follow.

The motivations for this are twofold:

1. API tests have unexpected pitfalls that especially new contributors run into 
when writing tests. To prevent the frustration of letting people figure those 
pitfalls out by trial-and-error, let's just document them briefly in one place.

2. It prevents some arguing about what is the right way to write tests. I 
really like to have fast and reliable API test suite, but I also don't want to 
be the bogeyman that has to insist in every review that the test should be 
rewritten to not launch a process for no good reason. It's much easier to just 
point to a policy document.

I omitted some guidelines that I think could be controversial (e.g., the whole 
"should assert message describe failure or success").


https://reviews.llvm.org/D101153

Files:
  lldb/docs/resources/test.rst


Index: lldb/docs/resources/test.rst
===
--- lldb/docs/resources/test.rst
+++ lldb/docs/resources/test.rst
@@ -196,6 +196,88 @@
 variants mean that more general tests should be API tests, so that they can be
 run against the different variants.
 
+Guidelines for API tests
+
+
+API tests are expected to be fast, reliable and maintainable. To achieve this
+goal, API tests should conform to the following guidelines in addition to 
normal
+good testing practices.
+
+**Don't unnecessarily launch the test executable.**
+Launching a process and running to a breakpoint can often be the most
+expensive part of a test. While it's often required to have a running
+process to test a certain feature, sometimes its possible to test a feature
+with a (non-running) target.
+
+**Don't unnecessarily include system headers in test sources.**
+Including external headers slows down the compilation of the test 
executable
+and it makes reproducing test failures on other operating systems or
+configurations harder. There is usually
+
+**Avoid specifying test-specific compiler flags when including system 
headers.**
+If a test requires including a system header (e.g., a test for a libc++
+formatter includes a libc++ header), always avoid specifying custom 
compiler
+flags in the test's ``Makefile``. Certain debug information formats such as
+``gmodules`` need to recompile external headers when they encounter
+test-specific flags (including defines) which can be very expensive.
+
+**Test programs should be kept simple.**
+Test executables should do the minimum amount of work to bring the process
+into the state that is required for the test. Simulating a 'real' program
+that actually tries to do some useful task rarely helps with catching bugs
+and makes the test much harder to debug and maintain. The test programs
+should always be deterministic (i.e., do not generate and check against
+random test values).
+
+**Identifiers in tests should be simple and descriptive.**
+Often test program need to declare functions and classes which require
+choosing some form of identifier for them. These identifiers should always
+either be kept simple for small tests (e.g., ``A``, ``B``, ...) or have 
some
+descriptive name (e.g., ``ClassWithTailPadding``, ``inlined_func``, ...).
+Never choose identifiers that are already used anywhere else in LLVM or
+other programs (e.g., don't name a class  ``VirtualFileSystem``, a function
+``llvm_unreachable``, or a namespace ``rapidxml``) as this will just 
mislead
+people ``grep``'ing the LLVM repository for those strings.
+
+**Prefer LLDB testing utilities over directly working with the SB API.**
+The ``lldbutil`` module and the ``TestBase`` class come with a large amount
+of utility functions that can do common test setup tasks (e.g., starting a
+test executable and running the process to a breakpoint). Using these
+functions not only keeps the test shorter and free of duplicated code, but
+they also follow best test suite practices and usually give much clearer
+error messages if something goes wrong. The test utilities also contain
+custom asserts and checks that should be preferably used (e.g.
+``self.assertSuccess``).
+
+**Prefer calling the SB API over checking command output.**
+Avoid writing your tests on top of ``self.expect(...)`` calls that check
+the output of LLDB commands and instead try calling into the SB API. 
Relying
+on LLDB commands makes changing (and improving) the output/syntax of
+commands harder and the resulting tests are often prone to accepting
+incorrect test results. Especially improved error messages that contain
+more information might cause these 

[Lldb-commits] [lldb] f8f3fc1 - [lldb][NFC] Delete a checked-in build log in docs/testsuite

[Raphael Isemann via lldb-commits]

Author: Raphael Isemann
Date: 2021-04-23T10:36:43+02:00
New Revision: f8f3fc1fbad608ee7c7e0c2a5af0637006e1ba0c

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

LOG: [lldb][NFC] Delete a checked-in build log in docs/testsuite

Added: 


Modified: 


Removed: 

lldb/docs/testsuite/2010-10-19-14_10_49.059609/TestSettings.SettingsCommandTestCase.test_set_output_path.log



diff  --git 
a/lldb/docs/testsuite/2010-10-19-14_10_49.059609/TestSettings.SettingsCommandTestCase.test_set_output_path.log
 
b/lldb/docs/testsuite/2010-10-19-14_10_49.059609/TestSettings.SettingsCommandTestCase.test_set_output_path.log
deleted file mode 100644
index e18199537e6f..
--- 
a/lldb/docs/testsuite/2010-10-19-14_10_49.059609/TestSettings.SettingsCommandTestCase.test_set_output_path.log
+++ /dev/null
@@ -1,43 +0,0 @@
-
-os command: [['/bin/sh', '-c', 'make clean; make']]
-stdout: rm -rf "a.out" "a.out.dSYM"  main.o main.d
-g++ -arch x86_64 -gdwarf-2 -O0   -c -o main.o main.cpp
-g++ -arch x86_64 -gdwarf-2 -O0  main.o -o "a.out"
-/usr/bin/dsymutil  -o "a.out.dSYM" "a.out"
-
-stderr: None
-retcode: 0
-
-
-runCmd: file /Volumes/data/lldb/svn/trunk/test/settings/a.out
-output: Current executable set to 
'/Volumes/data/lldb/svn/trunk/test/settings/a.out' (x86_64).
-
-
-runCmd: settings set target.process.output-path 'stdout.txt'
-output: 
-
-runCmd: settings show target.process.output-path
-output: target.process.output-path (string) = 'stdout.txt'
-
-
-Expecting start string: target.process.output-path (string) = 'stdout.txt'
-Matched
-
-runCmd: run
-output: Process 43533 launched: 
'/Volumes/data/lldb/svn/trunk/test/settings/a.out' (x86_64)
-
-
-FAIL
-
-runCmd: process kill
-check of return status not required
-runCmd failed!
-error: Process must be launched.
-
-
-Traceback (most recent call last):
-  File "/Volumes/data/lldb/svn/trunk/test/settings/TestSettings.py", line 125, 
in test_set_output_path
-"'stdout.txt' exists due to target.process.output-path.")
-AssertionError: False is not True : 'stdout.txt' exists due to 
target.process.output-path.
-
-



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

[Lldb-commits] [PATCH] D96236: [lldb] DWZ 1/9: Pass main DWARFUnit * along DWARFDIEs

[Jan Kratochvil via Phabricator via lldb-commits]

jankratochvil planned changes to this revision.
jankratochvil added a comment.

In D96236#2710376 , @clayborg wrote:

> I would be fine with DWARFDie getting bigger to 24B.

Great, so I can return back to 2019.

> These objects are used temporarily and not used as part of the storage of all 
> of a DWARFUnit's DIEs.

This is what I also found in D73206 . There 
was originally a different initial comment:

-

I have found `DWARFDIE` is mostly used only for parameters and autovariables. I 
found (by `grep -r '<.*DWARF\(Base\|\)DIE' lldb`) only two cases of `DWARFDIE` 
in permanent structures:

- `DWARFASTParserClang::m_decl_ctx_to_die` refactored in this patch
- `ElaboratingDIEIterator::m_worklist` but I think it will never grow to any 
more `DWARFDIE`s than a few so it is worse to refactor it to `DIERef` which is 
slower and due to few entries it does not save any memory space.

-

For the `DWARFASTParserClang::m_decl_ctx_to_die` refactoring I will make it a 
standalone patch in a new series.

> DWARFUnit objects store an array of DWARFDebugInfoEntry objects and those are 
> what we care about not getting bigger.

The problem is I still need to increase element size of various indexes, such 
as:

  -  typedef llvm::DenseMap
  +  typedef llvm::DenseMap, clang::Decl *>
 DIEToDeclMap;

I was thinking I could templatize it so that there are two variants of the code 
and it would switch to the larger + (a bit) slower variant only when it detects 
DWZ file. I understand it is bad to affect LLDB performance by the marginally 
used DWZ feature.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96236

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