[Lldb-commits] [lldb] bd3976f - [lldb] Refactor Symbols::DownloadObjectAndSymbolFile

[Jonas Devlieghere via lldb-commits]

Author: Jonas Devlieghere
Date: 2022-08-08T20:14:07-07:00
New Revision: bd3976fed470343be8df9caf0e35d290455b700c

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

LOG: [lldb] Refactor Symbols::DownloadObjectAndSymbolFile

 - Reduce indentation
 - Extract caching of the DbgShellCommand and the dsymForUUID executable
   (or equivalent)
 - Check the DBGShellCommands before falling back to
   /usr/local/bin/dsymForUUID
 - Don't check ~rc/bin/dsymForUUID
 - Improve error reporting
 - Don't cache the value of LLDB_APPLE_DSYMFORUUID_EXECUTABLE

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

Added: 


Modified: 
lldb/source/Symbol/LocateSymbolFileMacOSX.cpp

Removed: 




diff  --git a/lldb/source/Symbol/LocateSymbolFileMacOSX.cpp 
b/lldb/source/Symbol/LocateSymbolFileMacOSX.cpp
index 4dc42cf01716..00dbaa4b57fd 100644
--- a/lldb/source/Symbol/LocateSymbolFileMacOSX.cpp
+++ b/lldb/source/Symbol/LocateSymbolFileMacOSX.cpp
@@ -491,190 +491,184 @@ static bool 
GetModuleSpecInfoFromUUIDDictionary(CFDictionaryRef uuid_dict,
   return success;
 }
 
-bool Symbols::DownloadObjectAndSymbolFile(ModuleSpec _spec,
-  Status , bool force_lookup) {
-  bool success = false;
-  const UUID *uuid_ptr = module_spec.GetUUIDPtr();
-  const FileSpec *file_spec_ptr = module_spec.GetFileSpecPtr();
-
-  // It's expensive to check for the DBGShellCommands defaults setting, only do
-  // it once per lldb run and cache the result.
-  static bool g_have_checked_for_dbgshell_command = false;
-  static const char *g_dbgshell_command = NULL;
-  if (!g_have_checked_for_dbgshell_command) {
-g_have_checked_for_dbgshell_command = true;
+/// It's expensive to check for the DBGShellCommands defaults setting. Only do
+/// it once per lldb run and cache the result.
+static llvm::StringRef GetDbgShellCommand() {
+  static std::once_flag g_once_flag;
+  static std::string g_dbgshell_command;
+  std::call_once(g_once_flag, [&]() {
 CFTypeRef defaults_setting = CFPreferencesCopyAppValue(
 CFSTR("DBGShellCommands"), CFSTR("com.apple.DebugSymbols"));
 if (defaults_setting &&
 CFGetTypeID(defaults_setting) == CFStringGetTypeID()) {
-  char cstr_buf[PATH_MAX];
-  if (CFStringGetCString((CFStringRef)defaults_setting, cstr_buf,
- sizeof(cstr_buf), kCFStringEncodingUTF8)) {
-g_dbgshell_command =
-strdup(cstr_buf); // this malloc'ed memory will never be freed
+  char buffer[PATH_MAX];
+  if (CFStringGetCString((CFStringRef)defaults_setting, buffer,
+ sizeof(buffer), kCFStringEncodingUTF8)) {
+g_dbgshell_command = buffer;
   }
 }
 if (defaults_setting) {
   CFRelease(defaults_setting);
 }
+  });
+  return g_dbgshell_command;
+}
+
+/// Get the dsymForUUID executable and cache the result so we don't end up
+/// stat'ing the binary over and over.
+static FileSpec GetDsymForUUIDExecutable() {
+  // The LLDB_APPLE_DSYMFORUUID_EXECUTABLE environment variable is used by the
+  // test suite to override the dsymForUUID location. Because we must be able
+  // to change the value within a single test, don't bother caching it.
+  if (const char *dsymForUUID_env =
+  getenv("LLDB_APPLE_DSYMFORUUID_EXECUTABLE")) {
+FileSpec dsymForUUID_executable(dsymForUUID_env);
+FileSystem::Instance().Resolve(dsymForUUID_executable);
+if (FileSystem::Instance().Exists(dsymForUUID_executable))
+  return dsymForUUID_executable;
   }
 
-  // When g_dbgshell_command is NULL, the user has not enabled the use of an
+  static std::once_flag g_once_flag;
+  static FileSpec g_dsymForUUID_executable;
+  std::call_once(g_once_flag, [&]() {
+// Try the DBGShellCommand.
+llvm::StringRef dbgshell_command = GetDbgShellCommand();
+if (!dbgshell_command.empty()) {
+  g_dsymForUUID_executable = FileSpec(dbgshell_command);
+  FileSystem::Instance().Resolve(g_dsymForUUID_executable);
+  if (FileSystem::Instance().Exists(g_dsymForUUID_executable))
+return;
+}
+
+// Try dsymForUUID in /usr/local/bin
+{
+  g_dsymForUUID_executable = FileSpec("/usr/local/bin/dsymForUUID");
+  if (FileSystem::Instance().Exists(g_dsymForUUID_executable))
+return;
+}
+
+// We couldn't find the dsymForUUID binary.
+g_dsymForUUID_executable = {};
+  });
+  return g_dsymForUUID_executable;
+}
+
+bool Symbols::DownloadObjectAndSymbolFile(ModuleSpec _spec,
+  Status , bool force_lookup) {
+  const UUID *uuid_ptr = module_spec.GetUUIDPtr();
+  const FileSpec *file_spec_ptr = module_spec.GetFileSpecPtr();
+
+  llvm::StringRef dbgshell_command = GetDbgShellCommand();
+
+  // When 

[Lldb-commits] [PATCH] D131303: [lldb] Refactor Symbols::DownloadObjectAndSymbolFile

[Jonas Devlieghere via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGbd3976fed470: [lldb] Refactor 
Symbols::DownloadObjectAndSymbolFile (authored by JDevlieghere).
Herald added a project: LLDB.

Changed prior to commit:
  https://reviews.llvm.org/D131303?vs=450442=451033#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131303

Files:
  lldb/source/Symbol/LocateSymbolFileMacOSX.cpp

Index: lldb/source/Symbol/LocateSymbolFileMacOSX.cpp
===
--- lldb/source/Symbol/LocateSymbolFileMacOSX.cpp
+++ lldb/source/Symbol/LocateSymbolFileMacOSX.cpp
@@ -491,190 +491,184 @@
   return success;
 }
 
-bool Symbols::DownloadObjectAndSymbolFile(ModuleSpec _spec,
-  Status , bool force_lookup) {
-  bool success = false;
-  const UUID *uuid_ptr = module_spec.GetUUIDPtr();
-  const FileSpec *file_spec_ptr = module_spec.GetFileSpecPtr();
-
-  // It's expensive to check for the DBGShellCommands defaults setting, only do
-  // it once per lldb run and cache the result.
-  static bool g_have_checked_for_dbgshell_command = false;
-  static const char *g_dbgshell_command = NULL;
-  if (!g_have_checked_for_dbgshell_command) {
-g_have_checked_for_dbgshell_command = true;
+/// It's expensive to check for the DBGShellCommands defaults setting. Only do
+/// it once per lldb run and cache the result.
+static llvm::StringRef GetDbgShellCommand() {
+  static std::once_flag g_once_flag;
+  static std::string g_dbgshell_command;
+  std::call_once(g_once_flag, [&]() {
 CFTypeRef defaults_setting = CFPreferencesCopyAppValue(
 CFSTR("DBGShellCommands"), CFSTR("com.apple.DebugSymbols"));
 if (defaults_setting &&
 CFGetTypeID(defaults_setting) == CFStringGetTypeID()) {
-  char cstr_buf[PATH_MAX];
-  if (CFStringGetCString((CFStringRef)defaults_setting, cstr_buf,
- sizeof(cstr_buf), kCFStringEncodingUTF8)) {
-g_dbgshell_command =
-strdup(cstr_buf); // this malloc'ed memory will never be freed
+  char buffer[PATH_MAX];
+  if (CFStringGetCString((CFStringRef)defaults_setting, buffer,
+ sizeof(buffer), kCFStringEncodingUTF8)) {
+g_dbgshell_command = buffer;
   }
 }
 if (defaults_setting) {
   CFRelease(defaults_setting);
 }
+  });
+  return g_dbgshell_command;
+}
+
+/// Get the dsymForUUID executable and cache the result so we don't end up
+/// stat'ing the binary over and over.
+static FileSpec GetDsymForUUIDExecutable() {
+  // The LLDB_APPLE_DSYMFORUUID_EXECUTABLE environment variable is used by the
+  // test suite to override the dsymForUUID location. Because we must be able
+  // to change the value within a single test, don't bother caching it.
+  if (const char *dsymForUUID_env =
+  getenv("LLDB_APPLE_DSYMFORUUID_EXECUTABLE")) {
+FileSpec dsymForUUID_executable(dsymForUUID_env);
+FileSystem::Instance().Resolve(dsymForUUID_executable);
+if (FileSystem::Instance().Exists(dsymForUUID_executable))
+  return dsymForUUID_executable;
   }
 
-  // When g_dbgshell_command is NULL, the user has not enabled the use of an
+  static std::once_flag g_once_flag;
+  static FileSpec g_dsymForUUID_executable;
+  std::call_once(g_once_flag, [&]() {
+// Try the DBGShellCommand.
+llvm::StringRef dbgshell_command = GetDbgShellCommand();
+if (!dbgshell_command.empty()) {
+  g_dsymForUUID_executable = FileSpec(dbgshell_command);
+  FileSystem::Instance().Resolve(g_dsymForUUID_executable);
+  if (FileSystem::Instance().Exists(g_dsymForUUID_executable))
+return;
+}
+
+// Try dsymForUUID in /usr/local/bin
+{
+  g_dsymForUUID_executable = FileSpec("/usr/local/bin/dsymForUUID");
+  if (FileSystem::Instance().Exists(g_dsymForUUID_executable))
+return;
+}
+
+// We couldn't find the dsymForUUID binary.
+g_dsymForUUID_executable = {};
+  });
+  return g_dsymForUUID_executable;
+}
+
+bool Symbols::DownloadObjectAndSymbolFile(ModuleSpec _spec,
+  Status , bool force_lookup) {
+  const UUID *uuid_ptr = module_spec.GetUUIDPtr();
+  const FileSpec *file_spec_ptr = module_spec.GetFileSpecPtr();
+
+  llvm::StringRef dbgshell_command = GetDbgShellCommand();
+
+  // When dbgshell_command is empty, the user has not enabled the use of an
   // external program to find the symbols, don't run it for them.
-  if (!force_lookup && g_dbgshell_command == NULL) {
+  if (!force_lookup && dbgshell_command.empty())
+return false;
+
+  // We need a UUID or valid (existing FileSpec.
+  if (!uuid_ptr &&
+  (!file_spec_ptr || !FileSystem::Instance().Exists(*file_spec_ptr)))
+return false;
+
+  // We need a dsymForUUID binary or an equivalent executable/script.
+  FileSpec dsymForUUID_exe_spec = 

[Lldb-commits] [PATCH] D131460: [LLDB] Remove undefined behavior in TestConstStaticIntegralMember.py

[Shafik Yaghmour via Phabricator via lldb-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rG1438639a2f7e: [LLDB] Remove undefined behavior in 
TestConstStaticIntegralMember.py (authored by shafik).
Herald added a project: LLDB.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131460

Files:
  
lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py
  lldb/test/API/lang/cpp/const_static_integral_member/main.cpp


Index: lldb/test/API/lang/cpp/const_static_integral_member/main.cpp
===
--- lldb/test/API/lang/cpp/const_static_integral_member/main.cpp
+++ lldb/test/API/lang/cpp/const_static_integral_member/main.cpp
@@ -47,7 +47,6 @@
   std::numeric_limits::min();
 
   const static Enum enum_val = enum_case2;
-  const static Enum invalid_enum_val = static_cast(enum_case2 + 5);
   const static ScopedEnum scoped_enum_val = ScopedEnum::scoped_enum_case2;
   const static ScopedEnum invalid_scoped_enum_val = static_cast(5);
   const static ScopedCharEnum scoped_char_enum_val = ScopedCharEnum::case2;
@@ -102,7 +101,6 @@
   int member_copy = ClassWithOnlyConstStatic::member;
 
   Enum e = A::enum_val;
-  e = A::invalid_enum_val;
   ScopedEnum se = A::scoped_enum_val;
   se = A::invalid_scoped_enum_val;
   ScopedCharEnum sce = A::scoped_char_enum_val;
Index: 
lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py
===
--- 
lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py
+++ 
lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py
@@ -53,8 +53,6 @@
 
 # Test an unscoped enum.
 self.expect_expr("A::enum_val", result_value="enum_case2")
-# Test an unscoped enum with an invalid enum case.
-self.expect_expr("A::invalid_enum_val", result_value="enum_case1 | 
enum_case2 | 0x4")
 
 # Test a scoped enum.
 self.expect_expr("A::scoped_enum_val", 
result_value="scoped_enum_case2")


Index: lldb/test/API/lang/cpp/const_static_integral_member/main.cpp
===
--- lldb/test/API/lang/cpp/const_static_integral_member/main.cpp
+++ lldb/test/API/lang/cpp/const_static_integral_member/main.cpp
@@ -47,7 +47,6 @@
   std::numeric_limits::min();
 
   const static Enum enum_val = enum_case2;
-  const static Enum invalid_enum_val = static_cast(enum_case2 + 5);
   const static ScopedEnum scoped_enum_val = ScopedEnum::scoped_enum_case2;
   const static ScopedEnum invalid_scoped_enum_val = static_cast(5);
   const static ScopedCharEnum scoped_char_enum_val = ScopedCharEnum::case2;
@@ -102,7 +101,6 @@
   int member_copy = ClassWithOnlyConstStatic::member;
 
   Enum e = A::enum_val;
-  e = A::invalid_enum_val;
   ScopedEnum se = A::scoped_enum_val;
   se = A::invalid_scoped_enum_val;
   ScopedCharEnum sce = A::scoped_char_enum_val;
Index: lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py
===
--- lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py
+++ lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py
@@ -53,8 +53,6 @@
 
 # Test an unscoped enum.
 self.expect_expr("A::enum_val", result_value="enum_case2")
-# Test an unscoped enum with an invalid enum case.
-self.expect_expr("A::invalid_enum_val", result_value="enum_case1 | enum_case2 | 0x4")
 
 # Test a scoped enum.
 self.expect_expr("A::scoped_enum_val", result_value="scoped_enum_case2")
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] 1438639 - [LLDB] Remove undefined behavior in TestConstStaticIntegralMember.py

[Shafik Yaghmour via lldb-commits]

Author: Shafik Yaghmour
Date: 2022-08-08T19:23:53-07:00
New Revision: 1438639a2f7eb9e9cba01454d3a9b1b16d179c9a

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

LOG: [LLDB] Remove undefined behavior in TestConstStaticIntegralMember.py

Setting an enum without a fixed underlying type to a value which is outside the
value range is undefined behavior.

The initializer needs to be a constant expression and therefore this was always
ill-formed we just were not diagnosing it before.

See D130058 and D131307 for more details.

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

Added: 


Modified: 

lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py
lldb/test/API/lang/cpp/const_static_integral_member/main.cpp

Removed: 




diff  --git 
a/lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py
 
b/lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py
index 09345b98b16bc..794f382b8b68f 100644
--- 
a/lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py
+++ 
b/lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py
@@ -53,8 +53,6 @@ def test(self):
 
 # Test an unscoped enum.
 self.expect_expr("A::enum_val", result_value="enum_case2")
-# Test an unscoped enum with an invalid enum case.
-self.expect_expr("A::invalid_enum_val", result_value="enum_case1 | 
enum_case2 | 0x4")
 
 # Test a scoped enum.
 self.expect_expr("A::scoped_enum_val", 
result_value="scoped_enum_case2")

diff  --git a/lldb/test/API/lang/cpp/const_static_integral_member/main.cpp 
b/lldb/test/API/lang/cpp/const_static_integral_member/main.cpp
index b26251336371c..17b14da864a97 100644
--- a/lldb/test/API/lang/cpp/const_static_integral_member/main.cpp
+++ b/lldb/test/API/lang/cpp/const_static_integral_member/main.cpp
@@ -47,7 +47,6 @@ struct A {
   std::numeric_limits::min();
 
   const static Enum enum_val = enum_case2;
-  const static Enum invalid_enum_val = static_cast(enum_case2 + 5);
   const static ScopedEnum scoped_enum_val = ScopedEnum::scoped_enum_case2;
   const static ScopedEnum invalid_scoped_enum_val = static_cast(5);
   const static ScopedCharEnum scoped_char_enum_val = ScopedCharEnum::case2;
@@ -102,7 +101,6 @@ int main() {
   int member_copy = ClassWithOnlyConstStatic::member;
 
   Enum e = A::enum_val;
-  e = A::invalid_enum_val;
   ScopedEnum se = A::scoped_enum_val;
   se = A::invalid_scoped_enum_val;
   ScopedCharEnum sce = A::scoped_char_enum_val;



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

[Lldb-commits] [PATCH] D131460: [LLDB] Remove undefined behavior in TestConstStaticIntegralMember.py

[Shafik Yaghmour via Phabricator via lldb-commits]

shafik added a comment.

I will probably land this soon since this is being reported by several built 
bots and I this should bring the builds back to green. I just wanted the 
interested parties to see this in case there is some functionality this is 
testing that I am missing and some additions are needed to  cover what you want.


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

https://reviews.llvm.org/D131460

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

[Lldb-commits] [PATCH] D131460: [LLDB] Remove undefined behavior in TestConstStaticIntegralMember.py

[Shafik Yaghmour via Phabricator via lldb-commits]

shafik created this revision.
shafik added reviewers: JDevlieghere, werat, aaron.ballman.
Herald added a project: All.
shafik requested review of this revision.

Setting an enum without a fixed underlying type to a value which is outside the 
value range is undefined behavior.

The initializer needs to be a constant expression and therefore this was always 
ill-formed we just were not diagnosing it before.

See D130058  and D131307 
 for more details.


https://reviews.llvm.org/D131460

Files:
  
lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py
  lldb/test/API/lang/cpp/const_static_integral_member/main.cpp


Index: lldb/test/API/lang/cpp/const_static_integral_member/main.cpp
===
--- lldb/test/API/lang/cpp/const_static_integral_member/main.cpp
+++ lldb/test/API/lang/cpp/const_static_integral_member/main.cpp
@@ -47,7 +47,6 @@
   std::numeric_limits::min();
 
   const static Enum enum_val = enum_case2;
-  const static Enum invalid_enum_val = static_cast(enum_case2 + 5);
   const static ScopedEnum scoped_enum_val = ScopedEnum::scoped_enum_case2;
   const static ScopedEnum invalid_scoped_enum_val = static_cast(5);
   const static ScopedCharEnum scoped_char_enum_val = ScopedCharEnum::case2;
@@ -102,7 +101,6 @@
   int member_copy = ClassWithOnlyConstStatic::member;
 
   Enum e = A::enum_val;
-  e = A::invalid_enum_val;
   ScopedEnum se = A::scoped_enum_val;
   se = A::invalid_scoped_enum_val;
   ScopedCharEnum sce = A::scoped_char_enum_val;
Index: 
lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py
===
--- 
lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py
+++ 
lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py
@@ -53,8 +53,6 @@
 
 # Test an unscoped enum.
 self.expect_expr("A::enum_val", result_value="enum_case2")
-# Test an unscoped enum with an invalid enum case.
-self.expect_expr("A::invalid_enum_val", result_value="enum_case1 | 
enum_case2 | 0x4")
 
 # Test a scoped enum.
 self.expect_expr("A::scoped_enum_val", 
result_value="scoped_enum_case2")


Index: lldb/test/API/lang/cpp/const_static_integral_member/main.cpp
===
--- lldb/test/API/lang/cpp/const_static_integral_member/main.cpp
+++ lldb/test/API/lang/cpp/const_static_integral_member/main.cpp
@@ -47,7 +47,6 @@
   std::numeric_limits::min();
 
   const static Enum enum_val = enum_case2;
-  const static Enum invalid_enum_val = static_cast(enum_case2 + 5);
   const static ScopedEnum scoped_enum_val = ScopedEnum::scoped_enum_case2;
   const static ScopedEnum invalid_scoped_enum_val = static_cast(5);
   const static ScopedCharEnum scoped_char_enum_val = ScopedCharEnum::case2;
@@ -102,7 +101,6 @@
   int member_copy = ClassWithOnlyConstStatic::member;
 
   Enum e = A::enum_val;
-  e = A::invalid_enum_val;
   ScopedEnum se = A::scoped_enum_val;
   se = A::invalid_scoped_enum_val;
   ScopedCharEnum sce = A::scoped_char_enum_val;
Index: lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py
===
--- lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py
+++ lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py
@@ -53,8 +53,6 @@
 
 # Test an unscoped enum.
 self.expect_expr("A::enum_val", result_value="enum_case2")
-# Test an unscoped enum with an invalid enum case.
-self.expect_expr("A::invalid_enum_val", result_value="enum_case1 | enum_case2 | 0x4")
 
 # Test a scoped enum.
 self.expect_expr("A::scoped_enum_val", result_value="scoped_enum_case2")
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D131459: Move FormattersMatchCandidate flags to a struct.

[Jorge Gorbe Moya via Phabricator via lldb-commits]

jgorbe created this revision.
Herald added a project: All.
jgorbe requested review of this revision.
Herald added a project: LLDB.

Move FormattersMatchCandidate flags to a struct.

This removes some error-prone repetition in
FormatManager::GetPossibleMatches, where the same three boolean flags
are passed in a row multiple times as arguments to recursive calls to
GetPossibleMatches.

Instead of:

  // same flags, but with did_strip_typedef set to true.
  GetPossibleMatches(..., did_strip_ptr, did_strip_ref, true);

we can now say

  GetPossibleMatches(..., current_flags.WithStrippedTypedef());

which hopefully makes the intent clearer, and more readable in case we
add another flag.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D131459

Files:
  lldb/include/lldb/DataFormatters/FormatClasses.h
  lldb/include/lldb/DataFormatters/FormatManager.h
  lldb/source/DataFormatters/FormatManager.cpp

Index: lldb/source/DataFormatters/FormatManager.cpp
===
--- lldb/source/DataFormatters/FormatManager.cpp
+++ lldb/source/DataFormatters/FormatManager.cpp
@@ -175,60 +175,51 @@
 void FormatManager::GetPossibleMatches(
 ValueObject , CompilerType compiler_type,
 lldb::DynamicValueType use_dynamic, FormattersMatchVector ,
-bool did_strip_ptr, bool did_strip_ref, bool did_strip_typedef,
-bool root_level) {
+FormattersMatchCandidate::Flags current_flags, bool root_level) {
   compiler_type = compiler_type.GetTypeForFormatters();
   ConstString type_name(compiler_type.GetTypeName());
   if (valobj.GetBitfieldBitSize() > 0) {
 StreamString sstring;
 sstring.Printf("%s:%d", type_name.AsCString(), valobj.GetBitfieldBitSize());
 ConstString bitfieldname(sstring.GetString());
-entries.push_back(
-{bitfieldname, did_strip_ptr, did_strip_ref, did_strip_typedef});
+entries.push_back({bitfieldname, current_flags});
   }
 
   if (!compiler_type.IsMeaninglessWithoutDynamicResolution()) {
-entries.push_back(
-{type_name, did_strip_ptr, did_strip_ref, did_strip_typedef});
+entries.push_back({type_name, current_flags});
 
 ConstString display_type_name(compiler_type.GetTypeName());
 if (display_type_name != type_name)
-  entries.push_back({display_type_name, did_strip_ptr,
- did_strip_ref, did_strip_typedef});
+  entries.push_back({display_type_name, current_flags});
   }
 
   for (bool is_rvalue_ref = true, j = true;
j && compiler_type.IsReferenceType(nullptr, _rvalue_ref); j = false) {
 CompilerType non_ref_type = compiler_type.GetNonReferenceType();
-GetPossibleMatches(
-valobj, non_ref_type,
-use_dynamic, entries, did_strip_ptr, true, did_strip_typedef);
+GetPossibleMatches(valobj, non_ref_type, use_dynamic, entries,
+   current_flags.WithStrippedReference());
 if (non_ref_type.IsTypedefType()) {
   CompilerType deffed_referenced_type = non_ref_type.GetTypedefedType();
   deffed_referenced_type =
   is_rvalue_ref ? deffed_referenced_type.GetRValueReferenceType()
 : deffed_referenced_type.GetLValueReferenceType();
+  // this is not exactly the usual meaning of stripping typedefs
   GetPossibleMatches(
   valobj, deffed_referenced_type,
-  use_dynamic, entries, did_strip_ptr, did_strip_ref,
-  true); // this is not exactly the usual meaning of stripping typedefs
+  use_dynamic, entries, current_flags.WithStrippedTypedef());
 }
   }
 
   if (compiler_type.IsPointerType()) {
 CompilerType non_ptr_type = compiler_type.GetPointeeType();
-GetPossibleMatches(
-valobj, non_ptr_type,
-use_dynamic, entries, true, did_strip_ref, did_strip_typedef);
+GetPossibleMatches(valobj, non_ptr_type, use_dynamic, entries,
+   current_flags.WithStrippedPointer());
 if (non_ptr_type.IsTypedefType()) {
   CompilerType deffed_pointed_type =
   non_ptr_type.GetTypedefedType().GetPointerType();
-  const bool stripped_typedef = true;
-  GetPossibleMatches(
-  valobj, deffed_pointed_type,
-  use_dynamic, entries, did_strip_ptr, did_strip_ref,
-  stripped_typedef); // this is not exactly the usual meaning of
- // stripping typedefs
+  // this is not exactly the usual meaning of stripping typedefs
+  GetPossibleMatches(valobj, deffed_pointed_type, use_dynamic, entries,
+ current_flags.WithStrippedTypedef());
 }
   }
 
@@ -244,12 +235,10 @@
   // from it.
   CompilerType deffed_array_type =
   element_type.GetTypedefedType().GetArrayType(array_size);
-  const bool stripped_typedef = true;
+  // this is not exactly the usual meaning of stripping typedefs
   GetPossibleMatches(
   valobj, deffed_array_type,
-  use_dynamic, entries, 

[Lldb-commits] [PATCH] D131085: [lldb/crashlog] Refactor the CrashLogParser logic

[Med Ismail Bennani via Phabricator via lldb-commits]

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



Comment at: lldb/examples/python/crashlog.py:434
 except CrashLogFormatException:
-return TextCrashLogParser(debugger, path, verbose).parse()
+return  object().__new__(TextCrashLogParser)
 

JDevlieghere wrote:
> mib wrote:
> > kastiglione wrote:
> > > I have not seen the `object().__new__(SomeClass)` syntax. Why is it being 
> > > used for `TextCrashLogParser` but not `JSONCrashLogParser`? Also, 
> > > `__new__` is a static method, could it be `object.__new__(...)`? Or is 
> > > there a subtly that requires an `object` instance? Somewhat related, 
> > > would it be better to say `super().__new__(...)`?
> > > 
> > > Also: one class construction explicitly forwards the arguments, the other 
> > > does not. Is there a reason both aren't implicit (or both explicit)?
> > As you know, python class are implicitly derived from the `object` type, 
> > making `object.__new__` and `super().__new__` pretty much the same thing.
> > 
> > In this specific case, both the `TextCrashLogParser` and 
> > `JSONCrashLogParser` inherits from the `CrashLogParser` class, so 
> > `JSONCrashLogParser` will just inherits `CrashLogParser.__new__` 
> > implementation if we don't override it, which creates a recursive loop.
> > That's why I'm calling the `__new__` method specifying the class.
> What's the advantage of this over this compared to a factory method? Seems 
> like this could be:
> 
> ```
> def create(debugger, path, verbose)
> try:
> return JSONCrashLogParser(debugger, path, verbose)
> except CrashLogFormatException:
> return  TextCrashLogParser(debugger, path, verbose)
> ```
If we make a factory, then users could still call `__init__` on 
`CrashLogParser` and create a bogus object. With this approach, they're forced 
to instantiate a CrashLogParser like any another object.


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

https://reviews.llvm.org/D131085

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

[Lldb-commits] [PATCH] D131333: [lldb] abi_tag support 2/3 - Make FindBestAlternateFunctionMangledName local to the C++ language plugin

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.



Comment at: lldb/include/lldb/Target/Language.h:317
+  /// function names.
+  ///
+  /// \param[in] mangled_names List of mangled names to generate

I was wondering the same. I think it's worth the churn. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131333

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

[Lldb-commits] [PATCH] D131085: [lldb/crashlog] Refactor the CrashLogParser logic

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.



Comment at: lldb/examples/python/crashlog.py:434
 except CrashLogFormatException:
-return TextCrashLogParser(debugger, path, verbose).parse()
+return  object().__new__(TextCrashLogParser)
 

mib wrote:
> kastiglione wrote:
> > I have not seen the `object().__new__(SomeClass)` syntax. Why is it being 
> > used for `TextCrashLogParser` but not `JSONCrashLogParser`? Also, `__new__` 
> > is a static method, could it be `object.__new__(...)`? Or is there a subtly 
> > that requires an `object` instance? Somewhat related, would it be better to 
> > say `super().__new__(...)`?
> > 
> > Also: one class construction explicitly forwards the arguments, the other 
> > does not. Is there a reason both aren't implicit (or both explicit)?
> As you know, python class are implicitly derived from the `object` type, 
> making `object.__new__` and `super().__new__` pretty much the same thing.
> 
> In this specific case, both the `TextCrashLogParser` and `JSONCrashLogParser` 
> inherits from the `CrashLogParser` class, so `JSONCrashLogParser` will just 
> inherits `CrashLogParser.__new__` implementation if we don't override it, 
> which creates a recursive loop.
> That's why I'm calling the `__new__` method specifying the class.
What's the advantage of this over this compared to a factory method? Seems like 
this could be:

```
def create(debugger, path, verbose)
try:
return JSONCrashLogParser(debugger, path, verbose)
except CrashLogFormatException:
return  TextCrashLogParser(debugger, path, verbose)
```


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

https://reviews.llvm.org/D131085

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

[Lldb-commits] [PATCH] D131036: [lldb/crashlog] Add `-s|--skip-status` option to interactive mode

[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/D131036/new/

https://reviews.llvm.org/D131036

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

[Lldb-commits] [PATCH] D131085: [lldb/crashlog] Refactor the CrashLogParser logic

[Med Ismail Bennani via Phabricator via lldb-commits]

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



Comment at: lldb/examples/python/crashlog.py:434
 except CrashLogFormatException:
-return TextCrashLogParser(debugger, path, verbose).parse()
+return  object().__new__(TextCrashLogParser)
 

kastiglione wrote:
> I have not seen the `object().__new__(SomeClass)` syntax. Why is it being 
> used for `TextCrashLogParser` but not `JSONCrashLogParser`? Also, `__new__` 
> is a static method, could it be `object.__new__(...)`? Or is there a subtly 
> that requires an `object` instance? Somewhat related, would it be better to 
> say `super().__new__(...)`?
> 
> Also: one class construction explicitly forwards the arguments, the other 
> does not. Is there a reason both aren't implicit (or both explicit)?
As you know, python class are implicitly derived from the `object` type, making 
`object.__new__` and `super().__new__` pretty much the same thing.

In this specific case, both the `TextCrashLogParser` and `JSONCrashLogParser` 
inherits from the `CrashLogParser` class, so `JSONCrashLogParser` will just 
inherits `CrashLogParser.__new__` implementation if we don't override it, which 
creates a recursive loop.
That's why I'm calling the `__new__` method specifying the class.



Comment at: lldb/examples/python/crashlog.py:447
+class JSONCrashLogParser(CrashLogParser):
+def __new__(cls, debugger, path, verbose):
+with open(path, 'r') as f:

kastiglione wrote:
> can this be an `__init__`?
No unfortunately because at the `__init__` is called, the type of the object is 
already set, but we actually want to fallback to the `TextCrashLogParser` class 
if this one fails.

`__new__`: takes arguments and returns an instance
`__init__`: takes instance and arguments (from `__new__`) and returns nothing


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

https://reviews.llvm.org/D131085

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

[Lldb-commits] [PATCH] D131212: Allow host platform to use sdk sysroot directory option when resolving shared modules

[Trevor Joynson via Phabricator via lldb-commits]

trevorj added a comment.

> It does not. You _can_ connect to a remote server if you want to, but you 
> don't have to. Many workflows need to select a remote platform to let LLDB 
> know how to locate files when doing symbolication and when loading core 
> files, so you should just be able to use the workflow I suggested that was 
> based on Pavel's comments. Does that work for you?

Trying it out, awesome, I was completely wrong about what remote-linux was then.
Curious, why does the `host` platform support being sent a sysroot parameter 
when selecting it if it doesn't use it?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131212

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

[Lldb-commits] [PATCH] D131328: [lldb] Support fetching symbols with dsymForUUID in the background

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere updated this revision to Diff 450999.

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

https://reviews.llvm.org/D131328

Files:
  lldb/include/lldb/Core/Debugger.h
  lldb/include/lldb/Core/DebuggerEvents.h
  lldb/include/lldb/Core/ModuleList.h
  lldb/include/lldb/Symbol/LocateSymbolFile.h
  lldb/include/lldb/Target/Target.h
  lldb/source/Core/CoreProperties.td
  lldb/source/Core/Debugger.cpp
  lldb/source/Core/DebuggerEvents.cpp
  lldb/source/Core/Module.cpp
  lldb/source/Core/ModuleList.cpp
  lldb/source/Symbol/LocateSymbolFile.cpp
  lldb/source/Symbol/LocateSymbolFileMacOSX.cpp

Index: lldb/source/Symbol/LocateSymbolFileMacOSX.cpp
===
--- lldb/source/Symbol/LocateSymbolFileMacOSX.cpp
+++ lldb/source/Symbol/LocateSymbolFileMacOSX.cpp
@@ -492,7 +492,8 @@
 }
 
 bool Symbols::DownloadObjectAndSymbolFile(ModuleSpec _spec,
-  Status , bool force_lookup) {
+  Status , bool force_lookup,
+  bool copy_executable) {
   bool success = false;
   const UUID *uuid_ptr = module_spec.GetUUIDPtr();
   const FileSpec *file_spec_ptr = module_spec.GetFileSpecPtr();
@@ -592,12 +593,16 @@
 file_spec_ptr->GetPath(file_path, sizeof(file_path));
 
   StreamString command;
+  const char *copy_executable_str =
+  copy_executable ? " --copyExecutable" : "";
   if (!uuid_str.empty())
-command.Printf("%s --ignoreNegativeCache --copyExecutable %s",
-   g_dsym_for_uuid_exe_path, uuid_str.c_str());
+command.Printf("%s --ignoreNegativeCache %s%s",
+   g_dsym_for_uuid_exe_path, copy_executable_str,
+   uuid_str.c_str());
   else if (file_path[0] != '\0')
-command.Printf("%s --ignoreNegativeCache --copyExecutable %s",
-   g_dsym_for_uuid_exe_path, file_path);
+command.Printf("%s --ignoreNegativeCache %s%s",
+   g_dsym_for_uuid_exe_path, copy_executable_str,
+   file_path);
 
   if (!command.GetString().empty()) {
 Log *log = GetLog(LLDBLog::Host);
Index: lldb/source/Symbol/LocateSymbolFile.cpp
===
--- lldb/source/Symbol/LocateSymbolFile.cpp
+++ lldb/source/Symbol/LocateSymbolFile.cpp
@@ -8,6 +8,8 @@
 
 #include "lldb/Symbol/LocateSymbolFile.h"
 
+#include "lldb/Core/Debugger.h"
+#include "lldb/Core/Module.h"
 #include "lldb/Core/ModuleList.h"
 #include "lldb/Core/ModuleSpec.h"
 #include "lldb/Core/Progress.h"
@@ -23,7 +25,9 @@
 #include "lldb/Utility/Timer.h"
 #include "lldb/Utility/UUID.h"
 
+#include "llvm/ADT/SmallSet.h"
 #include "llvm/Support/FileSystem.h"
+#include "llvm/Support/ThreadPool.h"
 
 // From MacOSX system header "mach/machine.h"
 typedef int cpu_type_t;
@@ -397,6 +401,35 @@
   return LocateExecutableSymbolFileDsym(module_spec);
 }
 
+void Symbols::DownloadSymbolFileAsync(const UUID ) {
+  if (!ModuleList::GetGlobalModuleListProperties().GetEnableBackgroundLookup())
+return;
+
+  static llvm::SmallSet g_seen_uuids;
+  static std::mutex g_mutex;
+  Debugger::GetThreadPool().async([=]() {
+{
+  std::lock_guard guard(g_mutex);
+  if (g_seen_uuids.count(uuid))
+return;
+  g_seen_uuids.insert(uuid);
+}
+
+Status error;
+ModuleSpec module_spec;
+module_spec.GetUUID() = uuid;
+if (!Symbols::DownloadObjectAndSymbolFile(module_spec, error,
+  /*force_lookup=*/true,
+  /*copy_executable=*/false))
+  return;
+
+if (error.Fail())
+  return;
+
+Debugger::ReportSymbolChange(module_spec);
+  });
+}
+
 #if !defined(__APPLE__)
 
 FileSpec Symbols::FindSymbolFileInBundle(const FileSpec _bundle,
@@ -407,7 +440,8 @@
 }
 
 bool Symbols::DownloadObjectAndSymbolFile(ModuleSpec _spec,
-  Status , bool force_lookup) {
+  Status , bool force_lookup,
+  bool copy_executable) {
   // Fill in the module_spec.GetFileSpec() for the object file and/or the
   // module_spec.GetSymbolFileSpec() for the debug symbols file.
   return false;
Index: lldb/source/Core/ModuleList.cpp
===
--- lldb/source/Core/ModuleList.cpp
+++ lldb/source/Core/ModuleList.cpp
@@ -106,6 +106,12 @@
   nullptr, ePropertyEnableExternalLookup, new_value);
 }
 
+bool ModuleListProperties::GetEnableBackgroundLookup() const {
+  const uint32_t idx = ePropertyEnableBackgroundLookup;
+  return m_collection_sp->GetPropertyAtIndexAsBoolean(
+  nullptr, idx, g_modulelist_properties[idx].default_uint_value != 0);
+}
+
 FileSpec ModuleListProperties::GetClangModulesCachePath() const {
   

[Lldb-commits] [PATCH] D131328: [lldb] Support fetching symbols with dsymForUUID in the background

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

In D131328#3706814 , @labath wrote:

> In D131328#3706749 , @JDevlieghere 
> wrote:
>
>> There was another approach I considered initially which might alleviate some 
>> of the issues Pavel raised. Instead of using the global thread pool, that 
>> plan consisted of a dedicated thread to fetch symbols in the background. 
>> That would allow us to add the symbol info synchronously. For example on 
>> every stop we could ask if there are any new symbol files to add instead of 
>> trying to add the symbol info as soon as it becomes available.
>
> I kinda like that.

I tried prototyping this. I wasn't super happy with the results:

- This one's minor, but compared to the current patch, there's a bunch of 
synchronization boilerplate: input/output mutex, condition variable, etc
- Who should own this class? Inherently it's tied to the shared module list, 
but that never gets destroyed, so that has the flushing issue. The only viable 
alternative is to have an instance controlled by 
Debugger::{Initialize/Terminate}, which is exactly what we do for the global 
thread pool in D131407 .
- There's really no good place to poll for this in the target. You'd need to do 
it from the process.
- Once you poll for new symbol files, you can't flush the newly added modules 
from the download (unless you store them in the target) because other targets 
will need to be notified later.

All these things can be overcome, but that was the point where I decided that 
maybe it wasn't worth it.

Instead I focussed on avoiding doing all this work from an arbitrary thread and 
using a broadcast event to do it from the event handler thread.

> Bonus points if you make that single thread download multiple files at the 
> same time (using threads to paralelize downloading is not the best approach, 
> since the task is not cpu-bound). :)

If LLDB was doing the downloading itself we might be able to do better, but by 
shelling out to dsymForUUID all we see in an opaque blocking call.


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

https://reviews.llvm.org/D131328

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

[Lldb-commits] [PATCH] D131333: [lldb] abi_tag support 2/3 - Make FindBestAlternateFunctionMangledName local to the C++ language plugin

[Michael Buch via Phabricator via lldb-commits]

Michael137 added inline comments.
Herald added a subscriber: JDevlieghere.



Comment at: lldb/include/lldb/Target/Language.h:315
+  ///
+  /// \param[out] results Will contain alternative mangled
+  /// function names.

aprantl wrote:
> Why not return results? Are we expecting to add more results to an existing 
> vector regularly?
No reason in particular. Just tried to keep it consistent with the other 
related APIs. Mainly to cause less confusion for reviewers. If we wanted to 
return the vector we'd have to at least change 
`IRExecutionUnit::CollectCandidateCPlusPlusNames` also.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131333

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

[Lldb-commits] [PATCH] D131328: [lldb] Support fetching symbols with dsymForUUID in the background

[Jonas Devlieghere via Phabricator via lldb-commits]

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

- Use a debugger event to execute the notification of the targets on the event 
handler thread


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

https://reviews.llvm.org/D131328

Files:
  lldb/include/lldb/Core/Debugger.h
  lldb/include/lldb/Core/DebuggerEvents.h
  lldb/include/lldb/Core/ModuleList.h
  lldb/include/lldb/Symbol/LocateSymbolFile.h
  lldb/include/lldb/Target/Target.h
  lldb/source/Core/CoreProperties.td
  lldb/source/Core/Debugger.cpp
  lldb/source/Core/DebuggerEvents.cpp
  lldb/source/Core/Module.cpp
  lldb/source/Core/ModuleList.cpp
  lldb/source/Symbol/LocateSymbolFile.cpp
  lldb/source/Symbol/LocateSymbolFileMacOSX.cpp

Index: lldb/source/Symbol/LocateSymbolFileMacOSX.cpp
===
--- lldb/source/Symbol/LocateSymbolFileMacOSX.cpp
+++ lldb/source/Symbol/LocateSymbolFileMacOSX.cpp
@@ -492,7 +492,8 @@
 }
 
 bool Symbols::DownloadObjectAndSymbolFile(ModuleSpec _spec,
-  Status , bool force_lookup) {
+  Status , bool force_lookup,
+  bool copy_executable) {
   bool success = false;
   const UUID *uuid_ptr = module_spec.GetUUIDPtr();
   const FileSpec *file_spec_ptr = module_spec.GetFileSpecPtr();
@@ -592,12 +593,16 @@
 file_spec_ptr->GetPath(file_path, sizeof(file_path));
 
   StreamString command;
+  const char *copy_executable_str =
+  copy_executable ? " --copyExecutable" : "";
   if (!uuid_str.empty())
-command.Printf("%s --ignoreNegativeCache --copyExecutable %s",
-   g_dsym_for_uuid_exe_path, uuid_str.c_str());
+command.Printf("%s --ignoreNegativeCache %s%s",
+   g_dsym_for_uuid_exe_path, copy_executable_str,
+   uuid_str.c_str());
   else if (file_path[0] != '\0')
-command.Printf("%s --ignoreNegativeCache --copyExecutable %s",
-   g_dsym_for_uuid_exe_path, file_path);
+command.Printf("%s --ignoreNegativeCache %s%s",
+   g_dsym_for_uuid_exe_path, copy_executable_str,
+   file_path);
 
   if (!command.GetString().empty()) {
 Log *log = GetLog(LLDBLog::Host);
Index: lldb/source/Symbol/LocateSymbolFile.cpp
===
--- lldb/source/Symbol/LocateSymbolFile.cpp
+++ lldb/source/Symbol/LocateSymbolFile.cpp
@@ -8,6 +8,8 @@
 
 #include "lldb/Symbol/LocateSymbolFile.h"
 
+#include "lldb/Core/Debugger.h"
+#include "lldb/Core/Module.h"
 #include "lldb/Core/ModuleList.h"
 #include "lldb/Core/ModuleSpec.h"
 #include "lldb/Core/Progress.h"
@@ -23,7 +25,9 @@
 #include "lldb/Utility/Timer.h"
 #include "lldb/Utility/UUID.h"
 
+#include "llvm/ADT/SmallSet.h"
 #include "llvm/Support/FileSystem.h"
+#include "llvm/Support/ThreadPool.h"
 
 // From MacOSX system header "mach/machine.h"
 typedef int cpu_type_t;
@@ -397,6 +401,35 @@
   return LocateExecutableSymbolFileDsym(module_spec);
 }
 
+void Symbols::DownloadSymbolFileAsync(const UUID ) {
+  if (!ModuleList::GetGlobalModuleListProperties().GetEnableBackgroundLookup())
+return;
+
+  static llvm::SmallSet g_seen_uuids;
+  static std::mutex g_mutex;
+  Debugger::GetThreadPool().async([=]() {
+{
+  std::lock_guard guard(g_mutex);
+  if (g_seen_uuids.count(uuid))
+return;
+  g_seen_uuids.insert(uuid);
+}
+
+Status error;
+ModuleSpec module_spec;
+module_spec.GetUUID() = uuid;
+if (!Symbols::DownloadObjectAndSymbolFile(module_spec, error,
+  /*force_lookup=*/true,
+  /*copy_executable=*/false))
+  return;
+
+if (error.Fail())
+  return;
+
+Debugger::ReportSymbolChange(module_spec);
+  });
+}
+
 #if !defined(__APPLE__)
 
 FileSpec Symbols::FindSymbolFileInBundle(const FileSpec _bundle,
@@ -407,7 +440,8 @@
 }
 
 bool Symbols::DownloadObjectAndSymbolFile(ModuleSpec _spec,
-  Status , bool force_lookup) {
+  Status , bool force_lookup,
+  bool copy_executable) {
   // Fill in the module_spec.GetFileSpec() for the object file and/or the
   // module_spec.GetSymbolFileSpec() for the debug symbols file.
   return false;
Index: lldb/source/Core/ModuleList.cpp
===
--- lldb/source/Core/ModuleList.cpp
+++ lldb/source/Core/ModuleList.cpp
@@ -106,6 +106,12 @@
   nullptr, ePropertyEnableExternalLookup, new_value);
 }
 
+bool ModuleListProperties::GetEnableBackgroundLookup() const {
+  const uint32_t idx = ePropertyEnableBackgroundLookup;
+  return m_collection_sp->GetPropertyAtIndexAsBoolean(
+  nullptr, idx, 

[Lldb-commits] [PATCH] D131333: [lldb] abi_tag support 2/3 - Make FindBestAlternateFunctionMangledName local to the C++ language plugin

[Michael Buch via Phabricator via lldb-commits]

Michael137 created this revision.
Michael137 added a reviewer: aprantl.
Herald added a project: All.
Michael137 updated this revision to Diff 450757.
Michael137 added a comment.
Michael137 updated this revision to Diff 450991.
Michael137 retitled this revision from "[lldb] abi_tag support 2/4 - Make 
FindBestAlternateFunctionMangledName local to the C++ language plugin" to 
"[lldb] abi_tag support 2/3 - Make FindBestAlternateFunctionMangledName local 
to the C++ language plugin".
Michael137 added reviewers: labath, jingham.
Michael137 published this revision for review.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

- Rebase


Michael137 added a comment.

- Reword commit message




Comment at: lldb/include/lldb/Target/Language.h:315
+  ///
+  /// \param[out] results Will contain alternative mangled
+  /// function names.

Why not return results? Are we expecting to add more results to an existing 
vector regularly?


This patch makes `CPlusPlusLanguage::FindBestAlternateFunctionMangledName` local
to `CPlusPlusLanguage`. We do so by creating a new API 
`CollectAlternateFunctionNames`.
Both APIs are called only by `IRExecutionUnit`. However, by having
`FindBestAlternateFunctionMangledName` local to the C++ plugin we can
change its implementation in C++-specific ways, which we plan to do
in follow-up commits that add Itanium mangle tree API support to
`FindBestAlternateFunctionMangledName`.

**Testing**

- API tests still pass


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D131333

Files:
  lldb/include/lldb/Target/Language.h
  lldb/source/Expression/IRExecutionUnit.cpp
  lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
  lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.h

Index: lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.h
===
--- lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.h
+++ lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.h
@@ -135,8 +135,10 @@
   std::vector
   GenerateAlternateFunctionManglings(const ConstString mangled) const override;
 
-  ConstString FindBestAlternateFunctionMangledName(
-  const Mangled mangled, const SymbolContext _ctx) const override;
+  void
+  CollectAlternateFunctionNames(std::vector ,
+const std::vector _names,
+const SymbolContext ) const override;
 
   // PluginInterface protocol
   llvm::StringRef GetPluginName() override { return GetPluginNameStatic(); }
Index: lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
===
--- lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
+++ lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
@@ -510,8 +510,8 @@
   return alternates;
 }
 
-ConstString CPlusPlusLanguage::FindBestAlternateFunctionMangledName(
-const Mangled mangled, const SymbolContext _ctx) const {
+static ConstString FindBestAlternateFunctionMangledName(
+const Mangled mangled, const SymbolContext _ctx) {
   ConstString demangled = mangled.GetDemangledName();
   if (!demangled)
 return ConstString();
@@ -560,6 +560,7 @@
 return ConstString();
 }
 
+
 static void LoadLibCxxFormatters(lldb::TypeCategoryImplSP cpp_category_sp) {
   if (!cpp_category_sp)
 return;
@@ -1405,3 +1406,28 @@
   // that we could check for.
   return file_path.contains("/usr/include/c++/");
 }
+
+void CPlusPlusLanguage::CollectAlternateFunctionNames(
+std::vector ,
+const std::vector _names,
+const SymbolContext ) const {
+  for (const ConstString  : mangled_names) {
+Mangled mangled(name);
+if (SymbolNameFitsToLanguage(mangled)) {
+  if (ConstString best_alternate =
+  FindBestAlternateFunctionMangledName(mangled, sc)) {
+results.push_back(best_alternate);
+  }
+}
+
+std::vector alternates =
+GenerateAlternateFunctionManglings(name);
+results.insert(results.end(), alternates.begin(), alternates.end());
+
+// As a last-ditch fallback, try the base name for C++ names.  It's
+// terrible, but the DWARF doesn't always encode "extern C" correctly.
+ConstString basename =
+GetDemangledFunctionNameWithoutArguments(mangled);
+results.push_back(basename);
+  }
+}
Index: lldb/source/Expression/IRExecutionUnit.cpp
===
--- lldb/source/Expression/IRExecutionUnit.cpp
+++ lldb/source/Expression/IRExecutionUnit.cpp
@@ -675,25 +675,7 @@
 std::vector _names,
 const std::vector _names, const SymbolContext ) {
   if (auto *cpp_lang = Language::FindPlugin(lldb::eLanguageTypeC_plus_plus)) {
-for (const ConstString  : C_names) {
-  Mangled mangled(name);
-  if (cpp_lang->SymbolNameFitsToLanguage(mangled)) {
-if (ConstString best_alternate =
-  

[Lldb-commits] [PATCH] D131335: WIP: [lldb] abi_tag support 4/4 - Use mangle tree API to determine approximate mangle matches

[Michael Buch via Phabricator via lldb-commits]

Michael137 updated this revision to Diff 450992.
Michael137 added a comment.
Herald added a subscriber: JDevlieghere.

- Reword commit message
- Replace `SymbolNameFitsToLanguage` check


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131335

Files:
  lldb/packages/Python/lldbsuite/test/make/Makefile.rules
  lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
  lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.h
  lldb/test/API/lang/cpp/abi_tag_lookup/Makefile
  lldb/test/API/lang/cpp/abi_tag_lookup/TestAbiTagLookup.py
  lldb/test/API/lang/cpp/abi_tag_lookup/main.cpp

Index: lldb/test/API/lang/cpp/abi_tag_lookup/main.cpp
===
--- /dev/null
+++ lldb/test/API/lang/cpp/abi_tag_lookup/main.cpp
@@ -0,0 +1,69 @@
+#include 
+
+namespace A {
+template  bool operator<(const T &, const T &) { return true; }
+
+template 
+[[gnu::abi_tag("Two", "Tags")]] bool operator>(const T &, const T &) {
+  return true;
+}
+
+template 
+[[gnu::abi_tag("OneTag")]] bool operator==(const T &, const T &) {
+  return true;
+}
+
+[[gnu::abi_tag("Foo")]] int withAbiTagInNS(const int &, const int &) {
+  return 1;
+}
+
+template 
+[[gnu::abi_tag("Bar")]] int withAbiTagInNS(const T &, const T &) {
+  return 2;
+}
+
+struct B {};
+} // namespace A
+
+template 
+[[gnu::abi_tag("Baz")]] int withAbiTag(const T &, const T &) {
+  return 3;
+}
+
+struct Simple {
+  int mem;
+};
+
+struct [[gnu::abi_tag("Qux")]] Tagged {
+  int mem;
+
+  int const () const { return mem; }
+};
+
+template  struct [[gnu::abi_tag("Quux", "Quuux")]] TaggedTemplate {
+  T mem;
+
+  T const () const { return mem; }
+};
+
+// clang-format off
+inline namespace [[gnu::abi_tag("Inline", "NS")]] v1 {
+template  int withImplicitTag(T const ) { return t.mem; }
+} // namespace
+// clang-format on
+
+int main() {
+  A::B b1;
+  A::B b2;
+  Tagged t{.mem = 4};
+  TaggedTemplate tt{.mem = 5};
+
+  int result = (b1 < b2) + (b1 > b2) + (b1 == b2) + withAbiTag(b1, b2) +
+   A::withAbiTagInNS(1.0, 2.0) + withAbiTagInNS(b1, b2) +
+   A::withAbiTagInNS(1, 2) + withImplicitTag(Tagged{.mem = 6}) +
+   withImplicitTag(Simple{.mem = 6}) + t.Value() + tt.Value();
+
+  std::puts("Break here");
+
+  return result;
+}
Index: lldb/test/API/lang/cpp/abi_tag_lookup/TestAbiTagLookup.py
===
--- /dev/null
+++ lldb/test/API/lang/cpp/abi_tag_lookup/TestAbiTagLookup.py
@@ -0,0 +1,55 @@
+"""
+Test that we can call functions and use types
+annotated (and thus mangled) with ABI tags.
+"""
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+class AbiTagLookupTestCase(TestBase):
+
+@skipIfWindows
+@expectedFailureAll(debug_info=["dwarf", "gmodules", "dwo"])
+def test_abi_tag_lookup(self):
+self.build()
+lldbutil.run_to_source_breakpoint(self, 'Break here',
+lldb.SBFileSpec("main.cpp", False))
+
+# Qualified/unqualified lookup to templates in namespace
+self.expect_expr("operator<(b1, b2)", result_type="bool", result_value="true")
+self.expect_expr("A::operator<(b1, b2)", result_type="bool", result_value="true")
+self.expect_expr("b1 < b2", result_type="bool", result_value="true")
+
+# Qualified/unqualified lookup to templates with ABI tags in namespace
+self.expect_expr("operator>(b1, b2)", result_type="bool", result_value="true")
+self.expect_expr("A::operator>(b1, b2)", result_type="bool", result_value="true")
+self.expect_expr("b1 > b2", result_type="bool", result_value="true")
+
+# Call non-operator templates with ABI tags
+self.expect_expr("A::withAbiTagInNS(1, 1)", result_type="int", result_value="1")
+
+self.expect_expr("A::withAbiTagInNS(1.0, 1.0)", result_type="int", result_value="2")
+self.expect_expr("withAbiTagInNS(b1, b2)", result_type="int", result_value="2")
+self.expect_expr("A::withAbiTagInNS(b1, b2)", result_type="int", result_value="2")
+
+self.expect_expr("withAbiTag(b1, b2)", result_type="int", result_value="3")
+
+# Structures with ABI tags
+self.expect_expr("t.Value()", result_type="const int", result_value="4")
+self.expect_expr("tt.Value()", result_type="const int", result_value="5")
+
+self.expect_expr("Tagged{.mem = 6}", result_type="Tagged",
+ result_children=[ValueCheck(name="mem", value="6")])
+
+# Inline namespaces with ABI tags
+self.expect_expr("v1::withImplicitTag(Simple{.mem = 6})", result_type="int", result_value="6")
+self.expect_expr("withImplicitTag(Simple{.mem = 6})", result_type="int", result_value="6")
+
+# FIXME: this currently fails because 

[Lldb-commits] [PATCH] D131332: [lldb][unittests] Add more test cases to CPlusPlusNameParser unit-tests

[Adrian Prantl via Phabricator via lldb-commits]

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

More tests are always welcome!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131332

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

[Lldb-commits] [PATCH] D131335: WIP: [lldb] abi_tag support 4/4 - Use mangle tree API to determine approximate mangle matches

[Michael Buch via Phabricator via lldb-commits]

Michael137 created this revision.
Michael137 added a reviewer: aprantl.
Herald added a project: All.
Michael137 added a reviewer: jingham.
Michael137 edited the summary of this revision.
Michael137 updated this revision to Diff 450751.
Michael137 added a comment.
Michael137 added a reviewer: labath.
Michael137 published this revision for review.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

- Rebase
- Fix API tests:
  - Weaken pre-condition for non-function mangled names
  - Allow non-C++ mangled names


When resolving symbols during IR execution, lldb makes a last effort attempt
to resolve external symbols from object files by approximate name matching.
It currently uses `CPlusPlusNameParser` to parse the demangled function name
and arguments for the unresolved symbol and its candidates. However, this
hand-rolled C++ parser doesn’t support ABI tags which, depending on the 
demangler,
get demangled into `[abi:tag]`. This lack of parsing support causes lldb to 
never
consider a candidate mangled function name that has ABI tags.

The issue reproduces by calling an ABI-tagged template function from the
expression evaluator. This is particularly problematic with the recent
addition of ABI tags to numerous libcxx APIs.

This patch deals with the above symbol resolution issue by using the
Itanium mangle tree API to compare mangled function symbols and no
longer relying on parsing demangled names via `CPlusPlusNameParser`. Since
the MSVC mangler doesn't expose the mangle tree we continue using the
old matching logic when the candidate symbol has the MSVC mangling
scheme.

**Testing**

- Added API tests


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D131335

Files:
  lldb/packages/Python/lldbsuite/test/make/Makefile.rules
  lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
  lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.h
  lldb/test/API/lang/cpp/abi_tag_lookup/Makefile
  lldb/test/API/lang/cpp/abi_tag_lookup/TestAbiTagLookup.py
  lldb/test/API/lang/cpp/abi_tag_lookup/main.cpp

Index: lldb/test/API/lang/cpp/abi_tag_lookup/main.cpp
===
--- /dev/null
+++ lldb/test/API/lang/cpp/abi_tag_lookup/main.cpp
@@ -0,0 +1,69 @@
+#include 
+
+namespace A {
+template  bool operator<(const T &, const T &) { return true; }
+
+template 
+[[gnu::abi_tag("Two", "Tags")]] bool operator>(const T &, const T &) {
+  return true;
+}
+
+template 
+[[gnu::abi_tag("OneTag")]] bool operator==(const T &, const T &) {
+  return true;
+}
+
+[[gnu::abi_tag("Foo")]] int withAbiTagInNS(const int &, const int &) {
+  return 1;
+}
+
+template 
+[[gnu::abi_tag("Bar")]] int withAbiTagInNS(const T &, const T &) {
+  return 2;
+}
+
+struct B {};
+} // namespace A
+
+template 
+[[gnu::abi_tag("Baz")]] int withAbiTag(const T &, const T &) {
+  return 3;
+}
+
+struct Simple {
+  int mem;
+};
+
+struct [[gnu::abi_tag("Qux")]] Tagged {
+  int mem;
+
+  int const () const { return mem; }
+};
+
+template  struct [[gnu::abi_tag("Quux", "Quuux")]] TaggedTemplate {
+  T mem;
+
+  T const () const { return mem; }
+};
+
+// clang-format off
+inline namespace [[gnu::abi_tag("Inline", "NS")]] v1 {
+template  int withImplicitTag(T const ) { return t.mem; }
+} // namespace
+// clang-format on
+
+int main() {
+  A::B b1;
+  A::B b2;
+  Tagged t{.mem = 4};
+  TaggedTemplate tt{.mem = 5};
+
+  int result = (b1 < b2) + (b1 > b2) + (b1 == b2) + withAbiTag(b1, b2) +
+   A::withAbiTagInNS(1.0, 2.0) + withAbiTagInNS(b1, b2) +
+   A::withAbiTagInNS(1, 2) + withImplicitTag(Tagged{.mem = 6}) +
+   withImplicitTag(Simple{.mem = 6}) + t.Value() + tt.Value();
+
+  std::puts("Break here");
+
+  return result;
+}
Index: lldb/test/API/lang/cpp/abi_tag_lookup/TestAbiTagLookup.py
===
--- /dev/null
+++ lldb/test/API/lang/cpp/abi_tag_lookup/TestAbiTagLookup.py
@@ -0,0 +1,55 @@
+"""
+Test that we can call functions and use types
+annotated (and thus mangled) with ABI tags.
+"""
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+class AbiTagLookupTestCase(TestBase):
+
+@skipIfWindows
+@expectedFailureAll(debug_info=["dwarf", "gmodules", "dwo"])
+def test_abi_tag_lookup(self):
+self.build()
+lldbutil.run_to_source_breakpoint(self, 'Break here',
+lldb.SBFileSpec("main.cpp", False))
+
+# Qualified/unqualified lookup to templates in namespace
+self.expect_expr("operator<(b1, b2)", result_type="bool", result_value="true")
+self.expect_expr("A::operator<(b1, b2)", result_type="bool", result_value="true")
+self.expect_expr("b1 < b2", result_type="bool", result_value="true")
+
+# Qualified/unqualified lookup to templates with ABI tags in namespace
+self.expect_expr("operator>(b1, b2)", 

[Lldb-commits] [PATCH] D130401: Implement better path matching in FileSpecList::FindCompatibleIndex(...).

[Greg Clayton via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGc0124084537b: Implement better path matching in 
FileSpecList::FindCompatibleIndex(...). (authored by clayborg).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130401

Files:
  lldb/include/lldb/Breakpoint/BreakpointResolverFileLine.h
  lldb/include/lldb/Core/FileSpecList.h
  lldb/source/Breakpoint/BreakpointResolverFileLine.cpp
  lldb/source/Core/FileSpecList.cpp
  lldb/source/Symbol/CompileUnit.cpp
  
lldb/test/API/functionalities/breakpoint/breakpoint_command/TestBreakpointCommand.py
  lldb/test/API/functionalities/breakpoint/breakpoint_command/relative.yaml
  lldb/unittests/Core/CMakeLists.txt
  lldb/unittests/Core/FileSpecListTest.cpp

Index: lldb/unittests/Core/FileSpecListTest.cpp
===
--- /dev/null
+++ lldb/unittests/Core/FileSpecListTest.cpp
@@ -0,0 +1,125 @@
+//===-- FileSpecListTest.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/Core/FileSpecList.h"
+
+using namespace lldb_private;
+
+static FileSpec PosixSpec(llvm::StringRef path) {
+  return FileSpec(path, FileSpec::Style::posix);
+}
+
+static FileSpec WindowsSpec(llvm::StringRef path) {
+  return FileSpec(path, FileSpec::Style::windows);
+}
+
+TEST(FileSpecListTest, RelativePathMatchesPosix) {
+
+  const FileSpec fullpath = PosixSpec("/build/src/main.cpp");
+  const FileSpec relative = PosixSpec("./src/main.cpp");
+  const FileSpec basename = PosixSpec("./main.cpp");
+  const FileSpec full_wrong = PosixSpec("/other/wrong/main.cpp");
+  const FileSpec rel_wrong = PosixSpec("./wrong/main.cpp");
+  // Make sure these don't match "src/main.cpp" as we want to match full
+  // directories only
+  const FileSpec rel2_wrong = PosixSpec("asrc/main.cpp");
+  const FileSpec rel3_wrong = PosixSpec("rc/main.cpp");
+
+  FileSpecList files;
+  files.Append(fullpath);
+  files.Append(relative);
+  files.Append(basename);
+  files.Append(full_wrong);
+  files.Append(rel_wrong);
+  files.Append(rel2_wrong);
+  files.Append(rel3_wrong);
+
+  // Make sure the full path only matches the first entry
+  EXPECT_EQ((size_t)0, files.FindCompatibleIndex(0, fullpath));
+  EXPECT_EQ((size_t)1, files.FindCompatibleIndex(1, fullpath));
+  EXPECT_EQ((size_t)2, files.FindCompatibleIndex(2, fullpath));
+  EXPECT_EQ((size_t)UINT32_MAX, files.FindCompatibleIndex(3, fullpath));
+  // Make sure the relative path matches the all of the entries that contain
+  // the relative path
+  EXPECT_EQ((size_t)0, files.FindCompatibleIndex(0, relative));
+  EXPECT_EQ((size_t)1, files.FindCompatibleIndex(1, relative));
+  EXPECT_EQ((size_t)2, files.FindCompatibleIndex(2, relative));
+  EXPECT_EQ((size_t)UINT32_MAX, files.FindCompatibleIndex(3, relative));
+
+  // Make sure looking file a file using the basename matches all entries
+  EXPECT_EQ((size_t)0, files.FindCompatibleIndex(0, basename));
+  EXPECT_EQ((size_t)1, files.FindCompatibleIndex(1, basename));
+  EXPECT_EQ((size_t)2, files.FindCompatibleIndex(2, basename));
+  EXPECT_EQ((size_t)3, files.FindCompatibleIndex(3, basename));
+  EXPECT_EQ((size_t)4, files.FindCompatibleIndex(4, basename));
+  EXPECT_EQ((size_t)5, files.FindCompatibleIndex(5, basename));
+  EXPECT_EQ((size_t)6, files.FindCompatibleIndex(6, basename));
+
+  // Make sure that paths that have a common suffix don't return values that
+  // don't match on directory delimiters.
+  EXPECT_EQ((size_t)2, files.FindCompatibleIndex(0, rel2_wrong));
+  EXPECT_EQ((size_t)5, files.FindCompatibleIndex(3, rel2_wrong));
+  EXPECT_EQ((size_t)UINT32_MAX, files.FindCompatibleIndex(6, rel2_wrong));
+
+  EXPECT_EQ((size_t)2, files.FindCompatibleIndex(0, rel3_wrong));
+  EXPECT_EQ((size_t)6, files.FindCompatibleIndex(3, rel3_wrong));
+}
+
+TEST(FileSpecListTest, RelativePathMatchesWindows) {
+
+  const FileSpec fullpath = WindowsSpec(R"(C:\build\src\main.cpp)");
+  const FileSpec relative = WindowsSpec(R"(.\src\main.cpp)");
+  const FileSpec basename = WindowsSpec(R"(.\main.cpp)");
+  const FileSpec full_wrong = WindowsSpec(R"(\other\wrong\main.cpp)");
+  const FileSpec rel_wrong = WindowsSpec(R"(.\wrong\main.cpp)");
+  // Make sure these don't match "src\main.cpp" as we want to match full
+  // directories only
+  const FileSpec rel2_wrong = WindowsSpec(R"(asrc\main.cpp)");
+  const FileSpec rel3_wrong = WindowsSpec(R"("rc\main.cpp)");
+
+  FileSpecList files;
+  files.Append(fullpath);
+  files.Append(relative);
+  files.Append(basename);
+  files.Append(full_wrong);
+  files.Append(rel_wrong);
+  

[Lldb-commits] [lldb] c012408 - Implement better path matching in FileSpecList::FindCompatibleIndex(...).

[Greg Clayton via lldb-commits]

Author: Greg Clayton
Date: 2022-08-08T15:20:38-07:00
New Revision: c0124084537bad33180d2e675475baf1a4b79afe

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

LOG: Implement better path matching in FileSpecList::FindCompatibleIndex(...).

Currently a FileSpecList::FindFileIndex(...) will only match the specified 
FileSpec if:
- it has filename and directory and both match exactly
- if has a filename only and any filename in the list matches

Because of this, we modify our breakpoint resolving so it can handle relative 
paths by doing some extra code that removes the directory from the FileSpec 
when searching if the path is relative.

This patch is intended to fix breakpoints so they work as users expect them to 
by adding the following features:
- allow matches to relative paths in the file list to match as long as the 
relative path is at the end of the specified path at valid directory delimiters
- allow matches to paths to match if the specified path is relative and shorter 
than the file paths in the list

This allows us to remove the extra logic from BreakpointResolverFileLine.cpp 
that added support for setting breakpoints with relative paths.

This means we can still set breakpoints with relative paths when the debug info 
contains full paths. We add the ability to set breakpoints with full paths when 
the debug info contains relative paths.

Debug info contains "./a/b/c/main.cpp", the following will set breakpoints 
successfully:
- /build/a/b/c/main.cpp
- a/b/c/main.cpp
- b/c/main.cpp
- c/main.cpp
- main.cpp
- ./c/main.cpp
- ./a/b/c/main.cpp
- ./b/c/main.cpp
- ./main.cpp

This also ensures that we won't match partial directory names, if a relative 
path is in the list or is used for the match, things must match at the 
directory level.

The breakpoint resolving code will now use the new 
FileSpecList::FindCompatibleIndex(...) function to allow this fuzzy matching to 
work for breakpoints.

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

Added: 
lldb/test/API/functionalities/breakpoint/breakpoint_command/relative.yaml
lldb/unittests/Core/FileSpecListTest.cpp

Modified: 
lldb/include/lldb/Breakpoint/BreakpointResolverFileLine.h
lldb/include/lldb/Core/FileSpecList.h
lldb/source/Breakpoint/BreakpointResolverFileLine.cpp
lldb/source/Core/FileSpecList.cpp
lldb/source/Symbol/CompileUnit.cpp

lldb/test/API/functionalities/breakpoint/breakpoint_command/TestBreakpointCommand.py
lldb/unittests/Core/CMakeLists.txt

Removed: 




diff  --git a/lldb/include/lldb/Breakpoint/BreakpointResolverFileLine.h 
b/lldb/include/lldb/Breakpoint/BreakpointResolverFileLine.h
index bd8d0394d4d2d..e73632aad6a85 100644
--- a/lldb/include/lldb/Breakpoint/BreakpointResolverFileLine.h
+++ b/lldb/include/lldb/Breakpoint/BreakpointResolverFileLine.h
@@ -56,7 +56,7 @@ class BreakpointResolverFileLine : public BreakpointResolver {
   CopyForBreakpoint(lldb::BreakpointSP ) override;
 
 protected:
-  void FilterContexts(SymbolContextList _list, bool is_relative);
+  void FilterContexts(SymbolContextList _list);
 
   friend class Breakpoint;
   SourceLocationSpec m_location_spec;

diff  --git a/lldb/include/lldb/Core/FileSpecList.h 
b/lldb/include/lldb/Core/FileSpecList.h
index 117c6b691498c..a5720d85e7cde 100644
--- a/lldb/include/lldb/Core/FileSpecList.h
+++ b/lldb/include/lldb/Core/FileSpecList.h
@@ -116,6 +116,32 @@ class FileSpecList {
   /// else UINT32_MAX is returned.
   size_t FindFileIndex(size_t idx, const FileSpec , bool full) const;
 
+  /// Find a compatible file index.
+  ///
+  /// Find the index of a compatible file in the file spec list that matches \a
+  /// file starting \a idx entries into the file spec list. A file is 
considered
+  /// compatible if:
+  /// - The file matches exactly (only filename if \a file has no directory)
+  /// - If \a file is relative and any file in the list has this same suffix
+  /// - If any file in the list is relative and the relative path is a suffix
+  ///   of \a file
+  ///
+  /// This is used to implement better matching for setting breakpoints in
+  /// source files where an IDE might specify a full path when setting the
+  /// breakpoint and debug info contains relative paths, if a user specifies
+  /// a relative path when setting a breakpoint.
+  ///
+  /// \param[in] idx
+  /// An index into the file list.
+  ///
+  /// \param[in] file
+  /// The file specification to search for.
+  ///
+  /// \return
+  /// The index of the file that matches \a file if it is found,
+  /// else UINT32_MAX is returned.
+  size_t FindCompatibleIndex(size_t idx, const FileSpec ) const;
+
   /// Get file at index.
   ///
   /// Gets a file from the file list. If \a idx is not a valid index, an empty

diff  --git 

[Lldb-commits] [PATCH] D131085: [lldb/crashlog] Refactor the CrashLogParser logic

[Dave Lee via Phabricator via lldb-commits]

kastiglione added inline comments.



Comment at: lldb/examples/python/crashlog.py:447
+class JSONCrashLogParser(CrashLogParser):
+def __new__(cls, debugger, path, verbose):
+with open(path, 'r') as f:

can this be an `__init__`?


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

https://reviews.llvm.org/D131085

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

[Lldb-commits] [PATCH] D131085: [lldb/crashlog] Refactor the CrashLogParser logic

[Dave Lee via Phabricator via lldb-commits]

kastiglione added inline comments.



Comment at: lldb/examples/python/crashlog.py:422-425
+# When `__new__` is overriden, if the returned type is the same
+# as the class type, or related to it, the `__init__` method is
+# automatically called after `__new__`. If the return type is not
+# related, then the `__init__` method is not called.

Is this comment needed (maybe it was in an earlier draft)? The body of the 
function isn't calling init, so it might be fine to not have it.



Comment at: lldb/examples/python/crashlog.py:434
 except CrashLogFormatException:
-return TextCrashLogParser(debugger, path, verbose).parse()
+return  object().__new__(TextCrashLogParser)
 

I have not seen the `object().__new__(SomeClass)` syntax. Why is it being used 
for `TextCrashLogParser` but not `JSONCrashLogParser`? Also, `__new__` is a 
static method, could it be `object.__new__(...)`? Or is there a subtly that 
requires an `object` instance? Somewhat related, would it be better to say 
`super().__new__(...)`?

Also: one class construction explicitly forwards the arguments, the other does 
not. Is there a reason both aren't implicit (or both explicit)?


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

https://reviews.llvm.org/D131085

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

[Lldb-commits] [PATCH] D131212: Allow host platform to use sdk sysroot directory option when resolving shared modules

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

In D131212#3708142 , @trevorj wrote:

> @clayborg The problem with the second variation is it doesn't prefix the 
> path, it just looks for shared objects of the same basename in the folders 
> you specify.

Yes, that is correct now that I think about it.

> I thought that remote-linux requires a server?

It does not. You _can_ connect to a remote server if you want to, but you don't 
have to. Many workflows need to select a remote platform to let LLDB know how 
to locate files when doing symbolication and when loading core files, so you 
should just be able to use the workflow I suggested that was based on Pavel's 
comments. Does that work for you?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131212

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

[Lldb-commits] [PATCH] D131212: Allow host platform to use sdk sysroot directory option when resolving shared modules

[Trevor Joynson via Phabricator via lldb-commits]

trevorj added a comment.

@clayborg The problem with the second variation is it doesn't prefix the path, 
it just looks for shared objects of the same basename in the folders you 
specify.

I thought that remote-linux requires a server?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131212

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

[Lldb-commits] [PATCH] D130689: [LLVM] Update C++ standard to 17

[Martin Storsjö via Phabricator via lldb-commits]

mstorsjo added a comment.

In D130689#3700094 , @thakis wrote:

> In D130689#3696186 , @thieta wrote:
>
>> @nikic @thakis I fixed this issue in https://reviews.llvm.org/D131063 and it 
>> can be built with CXX_STANDARD=17 and OSX_DEPLOYMENT_TARGET=10.11.
>
> Thanks! We took this opportunity to bump our clang deployment target from 
> 10.7 to 10.12 (which makes a few other minor things easier too), so we don't 
> need this change any more. But it's a good change anyways :)

FWIW I ran into an issue when doing Universal macOS builds (by building for 
arm64 and x86_64 at the same time) where I now had to raise my deployment 
target to 10.14 - see https://github.com/llvm/llvm-project/issues/57017 for 
more details about that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130689

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

[Lldb-commits] [PATCH] D131081: [lldb] Prevent race condition when fetching /proc/cpuinfo

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

In D131081#3707956 , @wallace wrote:

> That's important to know. At least in this case this code only runs on Linux, 
> so hopefully we are good with the atomic static initialization.

Good point! Should be good to go


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131081

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

[Lldb-commits] [PATCH] D131212: Allow host platform to use sdk sysroot directory option when resolving shared modules

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

What Pavel said should work (there is always a "remote-" name for 
everything like "remote-linux" even if you are on linux to start with. The host 
platform assumes that everything comes from the current host.

That being said we also have a few ways to make this work without any changes:

  (lldb) platform select remote-linux -S 
/tmp/debug-[snip]-coredump.1869466.bL8C/sysroot
  (lldb) target create --core ...

Not sure if this would work:

  (lldb) setting set target.exec-search-paths 
/tmp/debug-[snip]-coredump.1869466.bL8C/sysroot
  (lldb) setting set target.debug-file-search-paths 
/tmp/debug-[snip]-coredump.1869466.bL8C/sysroot
  (lldb) target create --core ...

The first way is the best way I believe.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131212

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

[Lldb-commits] [PATCH] D131081: [lldb] Prevent race condition when fetching /proc/cpuinfo

[walter erquinigo via Phabricator via lldb-commits]

wallace added a comment.

That's important to know. At least in this case this code only runs on Linux, 
so hopefully we are good with the atomic static initialization.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131081

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

[Lldb-commits] [PATCH] D131081: [lldb] Prevent race condition when fetching /proc/cpuinfo

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

In D131081#3699295 , @labath wrote:

> It's not clear to me why you're insisting on `call_once`, when c++ guarantees 
> that the function-local statics will only be initialized once (atomically). 
> And with the new version, we don't even need the lambda..

I tend to use call_once to avoid an issue, IIRC, that older windows compilers 
had with the "statics will only be initialized once (atomically)". I seem to 
remember Microsoft compilers were not fully compliant with this and that it 
could cause multi-threading issues, but I don't know if this has been fixed or 
if this wouldn't be an issue in this case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131081

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

[Lldb-commits] [PATCH] D131437: Don't index the skeleton CU when we have a fission compile unit.

[Greg Clayton via Phabricator via lldb-commits]

clayborg created this revision.
clayborg added reviewers: labath, JDevlieghere.
Herald added a subscriber: arphaman.
Herald added a project: All.
clayborg requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

When fission is enabled, we were indexing the skeleton CU _and_ the .dwo CU. 
Issues arise when users enable compiler options that add extra data to the 
skeleton CU (like -fsplit-dwarf-inlining) and there can end up being types in 
the skeleton CU due to template parameters. We never want to index this 
information since the .dwo file has the real definition, and we really don't 
want function prototypes from this info since all parameters are removed. The 
index doesn't work correctly if it does index the skeleton CU as the DIE offset 
will assume it is from the .dwo file, so even if we do index the skeleton CU, 
the index entries will try and grab information from the .dwo file using the 
wrong DIE offset which can cause errors to be displayed or even worse, if the 
DIE offsets is valid in the .dwo CU, the wrong DIE will be used.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D131437

Files:
  lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.h
  lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp

Index: lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
@@ -148,19 +148,36 @@
 
   const LanguageType cu_language = SymbolFileDWARF::GetLanguage(unit);
 
-  IndexUnitImpl(unit, cu_language, set);
-
-  if (SymbolFileDWARFDwo *dwo_symbol_file = unit.GetDwoSymbolFile()) {
-// Type units in a dwp file are indexed separately, so we just need to
-// process the split unit here. However, if the split unit is in a dwo file,
-// then we need to process type units here.
-if (dwo_symbol_file == dwp) {
-  IndexUnitImpl(unit.GetNonSkeletonUnit(), cu_language, set);
-} else {
-  DWARFDebugInfo _info = dwo_symbol_file->DebugInfo();
-  for (size_t i = 0; i < dwo_info.GetNumUnits(); ++i)
-IndexUnitImpl(*dwo_info.GetUnitAtIndex(i), cu_language, set);
+  // First check if the unit has a DWO ID. If it does then we only want to index
+  // the .dwo file or nothing at all. If we have a compile unit where we can't
+  // locate the .dwo/.dwp file we don't want to index anything from the skeleton
+  // compile unit because it is usally has no children unless
+  // -fsplit-dwarf-inlining was used at compile time. This option will add a
+  // copy of all DW_TAG_subprogram and any contained DW_TAG_inline_subroutine
+  // DIEs so that symbolication will still work in the absence of the .dwo/.dwp
+  // file, but the functions have no return types and all arguments and locals
+  // have been removed. So we don't want to index any of these hacked up
+  // function types. Types can still exist in the skeleton compile unit DWARF
+  // though as some functions have template parameter types and other things
+  // that cause extra copies of types to be included, but we should find these
+  // types in the .dwo file only as methods could have return types removed and
+  // we don't have to index incomplete types from the skeletone compile unit.
+  if (unit.GetDWOId()) {
+if (SymbolFileDWARFDwo *dwo_symbol_file = unit.GetDwoSymbolFile()) {
+  // Type units in a dwp file are indexed separately, so we just need to
+  // process the split unit here. However, if the split unit is in a dwo file,
+  // then we need to process type units here.
+  if (dwo_symbol_file == dwp) {
+IndexUnitImpl(unit.GetNonSkeletonUnit(), cu_language, set);
+  } else {
+DWARFDebugInfo _info = dwo_symbol_file->DebugInfo();
+for (size_t i = 0; i < dwo_info.GetNumUnits(); ++i)
+  IndexUnitImpl(*dwo_info.GetUnitAtIndex(i), cu_language, set);
+  }
 }
+  } else {
+// We either have a normal compile unit which we want to index.
+IndexUnitImpl(unit, cu_language, set);
   }
 }
 
Index: lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.h
===
--- lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.h
+++ lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.h
@@ -51,7 +51,7 @@
   uint64_t m_type_hash = 0;
   uint32_t m_type_offset = 0;
 
-  uint64_t m_dwo_id = 0;
+  llvm::Optional m_dwo_id;
 
   DWARFUnitHeader() = default;
 
@@ -67,7 +67,7 @@
   }
   uint64_t GetTypeHash() const { return m_type_hash; }
   dw_offset_t GetTypeOffset() const { return m_type_offset; }
-  uint64_t GetDWOId() const { return m_dwo_id; }
+  llvm::Optional GetDWOId() const { return m_dwo_id; }
   bool IsTypeUnit() const {
 return m_unit_type == llvm::dwarf::DW_UT_type ||
m_unit_type == 

[Lldb-commits] [PATCH] D131159: [lldb] Use WSAEventSelect for MainLoop polling on windows

[Martin Storsjö via Phabricator via lldb-commits]

mstorsjo added a subscriber: alvinhochun.
mstorsjo added a comment.

I don't really have experience with this API - is this pattern (creating and 
closing event objects for each poll run) the common way of using this API? Is 
there any potential performance risk compared with keeping event objects around 
(if that's possible?).

Can @alvinhochun take the patch for a spin? I think you've got more realistic 
usecases of lldb to try it out in. (Do you want me to make a build with the 
patch for you to try out?) @labath What's the best way to exercise this code in 
actual use of lldb?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131159

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

[Lldb-commits] [PATCH] D131081: [lldb] Prevent race condition when fetching /proc/cpuinfo

[Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGddd576ab87a1: [lldb] Prevent race condition when fetching 
/proc/cpuinfo (authored by Walter Erquinigo wall...@fb.com).

Changed prior to commit:
  https://reviews.llvm.org/D131081?vs=450226=450905#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131081

Files:
  lldb/source/Plugins/Process/Linux/Procfs.cpp


Index: lldb/source/Plugins/Process/Linux/Procfs.cpp
===
--- lldb/source/Plugins/Process/Linux/Procfs.cpp
+++ lldb/source/Plugins/Process/Linux/Procfs.cpp
@@ -7,9 +7,9 @@
 
//===--===//
 
 #include "Procfs.h"
-
 #include "lldb/Host/linux/Support.h"
 #include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Support/Threading.h"
 
 using namespace lldb;
 using namespace lldb_private;
@@ -17,17 +17,14 @@
 using namespace llvm;
 
 Expected> lldb_private::process_linux::GetProcfsCpuInfo() {
-  static Optional> cpu_info;
-  if (!cpu_info) {
-auto buffer_or_error = errorOrToExpected(getProcFile("cpuinfo"));
-if (!buffer_or_error)
-  return buffer_or_error.takeError();
-MemoryBuffer  = **buffer_or_error;
-cpu_info = std::vector(
-reinterpret_cast(buffer.getBufferStart()),
-reinterpret_cast(buffer.getBufferEnd()));
-  }
-  return *cpu_info;
+  static ErrorOr> cpu_info_or_err =
+  getProcFile("cpuinfo");
+
+  if (!*cpu_info_or_err)
+cpu_info_or_err.getError();
+
+  MemoryBuffer  = **cpu_info_or_err;
+  return arrayRefFromStringRef(buffer.getBuffer());
 }
 
 Expected>


Index: lldb/source/Plugins/Process/Linux/Procfs.cpp
===
--- lldb/source/Plugins/Process/Linux/Procfs.cpp
+++ lldb/source/Plugins/Process/Linux/Procfs.cpp
@@ -7,9 +7,9 @@
 //===--===//
 
 #include "Procfs.h"
-
 #include "lldb/Host/linux/Support.h"
 #include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Support/Threading.h"
 
 using namespace lldb;
 using namespace lldb_private;
@@ -17,17 +17,14 @@
 using namespace llvm;
 
 Expected> lldb_private::process_linux::GetProcfsCpuInfo() {
-  static Optional> cpu_info;
-  if (!cpu_info) {
-auto buffer_or_error = errorOrToExpected(getProcFile("cpuinfo"));
-if (!buffer_or_error)
-  return buffer_or_error.takeError();
-MemoryBuffer  = **buffer_or_error;
-cpu_info = std::vector(
-reinterpret_cast(buffer.getBufferStart()),
-reinterpret_cast(buffer.getBufferEnd()));
-  }
-  return *cpu_info;
+  static ErrorOr> cpu_info_or_err =
+  getProcFile("cpuinfo");
+
+  if (!*cpu_info_or_err)
+cpu_info_or_err.getError();
+
+  MemoryBuffer  = **cpu_info_or_err;
+  return arrayRefFromStringRef(buffer.getBuffer());
 }
 
 Expected>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] ddd576a - [lldb] Prevent race condition when fetching /proc/cpuinfo

[Walter Erquinigo via lldb-commits]

Author: Walter Erquinigo
Date: 2022-08-08T12:31:42-07:00
New Revision: ddd576ab87a16eb7fb06531da8040ac2ece0c9a3

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

LOG: [lldb] Prevent race condition when fetching /proc/cpuinfo

@clayborg found a potential race condition when setting a static
variable. The fix seems simply to use call_once.

All relevant tests pass.

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

Added: 


Modified: 
lldb/source/Plugins/Process/Linux/Procfs.cpp

Removed: 




diff  --git a/lldb/source/Plugins/Process/Linux/Procfs.cpp 
b/lldb/source/Plugins/Process/Linux/Procfs.cpp
index 7c0c55a4f208..f59883d1caff 100644
--- a/lldb/source/Plugins/Process/Linux/Procfs.cpp
+++ b/lldb/source/Plugins/Process/Linux/Procfs.cpp
@@ -7,9 +7,9 @@
 
//===--===//
 
 #include "Procfs.h"
-
 #include "lldb/Host/linux/Support.h"
 #include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Support/Threading.h"
 
 using namespace lldb;
 using namespace lldb_private;
@@ -17,17 +17,14 @@ using namespace process_linux;
 using namespace llvm;
 
 Expected> lldb_private::process_linux::GetProcfsCpuInfo() {
-  static Optional> cpu_info;
-  if (!cpu_info) {
-auto buffer_or_error = errorOrToExpected(getProcFile("cpuinfo"));
-if (!buffer_or_error)
-  return buffer_or_error.takeError();
-MemoryBuffer  = **buffer_or_error;
-cpu_info = std::vector(
-reinterpret_cast(buffer.getBufferStart()),
-reinterpret_cast(buffer.getBufferEnd()));
-  }
-  return *cpu_info;
+  static ErrorOr> cpu_info_or_err =
+  getProcFile("cpuinfo");
+
+  if (!*cpu_info_or_err)
+cpu_info_or_err.getError();
+
+  MemoryBuffer  = **cpu_info_or_err;
+  return arrayRefFromStringRef(buffer.getBuffer());
 }
 
 Expected>



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

[Lldb-commits] [PATCH] D130689: [LLVM] Update C++ standard to 17

[Mehdi AMINI via Phabricator via lldb-commits]

mehdi_amini added a comment.

> There should be a better way than this. Comprehensive pre-merge testing of 
> all PRs etc.

We already have pre-commit tests on Phabricator on Windows and Linux, but 
that's not exhaustive and for many reasons I don't believe this is realistic in 
any way: we will always have some post-commit buildbots.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130689

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

[Lldb-commits] [PATCH] D131422: [lldb] Remove include/lldb/lldb-private.h

[Fangrui Song via Phabricator via lldb-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGcdeb50c32155: [lldb] Remove include/lldb/lldb-private.h 
(authored by MaskRay).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131422

Files:
  clang/docs/tools/clang-formatted-files.txt
  lldb/include/lldb/lldb-private-defines.h
  lldb/include/lldb/lldb-private.h
  lldb/include/lldb/module.modulemap


Index: lldb/include/lldb/module.modulemap
===
--- lldb/include/lldb/module.modulemap
+++ lldb/include/lldb/module.modulemap
@@ -134,7 +134,6 @@
   module lldb_enumerations { header "lldb-enumerations.h" export * }
   module lldb_forward { header "lldb-forward.h" export * }
   module lldb_private_enumerations { header "lldb-private-enumerations.h" 
export * }
-  module lldb_private_defines { header "lldb-private-defines.h" export * }
   module lldb_private_forward { header "lldb-private-forward.h" export * }
   module lldb_private { header "lldb-private.h" export * }
   module lldb_private_interfaces { header "lldb-private-interfaces.h" export * 
}
Index: lldb/include/lldb/lldb-private.h
===
--- lldb/include/lldb/lldb-private.h
+++ lldb/include/lldb/lldb-private.h
@@ -11,7 +11,6 @@
 
 #if defined(__cplusplus)
 
-#include "lldb/lldb-private-defines.h"
 #include "lldb/lldb-private-enumerations.h"
 #include "lldb/lldb-private-interfaces.h"
 #include "lldb/lldb-private-types.h"
Index: lldb/include/lldb/lldb-private-defines.h
===
--- lldb/include/lldb/lldb-private-defines.h
+++ /dev/null
@@ -1,36 +0,0 @@
-//===-- lldb-private-defines.h --*- C++ 
-*-===//
-//
-// 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
-//
-//===--===//
-
-#ifndef LLDB_LLDB_PRIVATE_DEFINES_H
-#define LLDB_LLDB_PRIVATE_DEFINES_H
-
-#if defined(__cplusplus)
-
-// Include Compiler.h here so we don't define LLVM_FALLTHROUGH and then
-// Compiler.h later tries to redefine it.
-#include "llvm/Support/Compiler.h"
-
-#ifndef LLVM_FALLTHROUGH
-
-#ifndef __has_cpp_attribute
-#define __has_cpp_attribute(x) 0
-#endif
-
-/// \macro LLVM_FALLTHROUGH
-/// Marks an empty statement preceding a deliberate switch fallthrough.
-#if __has_cpp_attribute(clang::fallthrough)
-#define LLVM_FALLTHROUGH [[clang::fallthrough]]
-#else
-#define LLVM_FALLTHROUGH
-#endif
-
-#endif // ifndef LLVM_FALLTHROUGH
-
-#endif // #if defined(__cplusplus)
-
-#endif // LLDB_LLDB_PRIVATE_DEFINES_H
Index: clang/docs/tools/clang-formatted-files.txt
===
--- clang/docs/tools/clang-formatted-files.txt
+++ clang/docs/tools/clang-formatted-files.txt
@@ -3591,7 +3591,6 @@
 lldb/examples/synthetic/bitfield/program.cpp
 lldb/include/lldb/lldb-defines.h
 lldb/include/lldb/lldb-forward.h
-lldb/include/lldb/lldb-private-defines.h
 lldb/include/lldb/lldb-private.h
 lldb/include/lldb/lldb-public.h
 lldb/include/lldb/lldb-versioning.h


Index: lldb/include/lldb/module.modulemap
===
--- lldb/include/lldb/module.modulemap
+++ lldb/include/lldb/module.modulemap
@@ -134,7 +134,6 @@
   module lldb_enumerations { header "lldb-enumerations.h" export * }
   module lldb_forward { header "lldb-forward.h" export * }
   module lldb_private_enumerations { header "lldb-private-enumerations.h" export * }
-  module lldb_private_defines { header "lldb-private-defines.h" export * }
   module lldb_private_forward { header "lldb-private-forward.h" export * }
   module lldb_private { header "lldb-private.h" export * }
   module lldb_private_interfaces { header "lldb-private-interfaces.h" export * }
Index: lldb/include/lldb/lldb-private.h
===
--- lldb/include/lldb/lldb-private.h
+++ lldb/include/lldb/lldb-private.h
@@ -11,7 +11,6 @@
 
 #if defined(__cplusplus)
 
-#include "lldb/lldb-private-defines.h"
 #include "lldb/lldb-private-enumerations.h"
 #include "lldb/lldb-private-interfaces.h"
 #include "lldb/lldb-private-types.h"
Index: lldb/include/lldb/lldb-private-defines.h
===
--- lldb/include/lldb/lldb-private-defines.h
+++ /dev/null
@@ -1,36 +0,0 @@
-//===-- lldb-private-defines.h --*- C++ -*-===//
-//
-// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
-// See https://llvm.org/LICENSE.txt for license information.
-// 

[Lldb-commits] [lldb] cdeb50c - [lldb] Remove include/lldb/lldb-private.h

[Fangrui Song via lldb-commits]

Author: Fangrui Song
Date: 2022-08-08T12:03:53-07:00
New Revision: cdeb50c3215593bffe9e5a46bdc9a9e34fd93dcc

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

LOG: [lldb] Remove include/lldb/lldb-private.h

The header from 62e0681afb478a4005efb6ba3598c24dc24866ee does something with
LLVM_FALLTHROUGH. Now that llvm-project has switched to C++17 and
LLVM_FALLTHROUGH uses have been migrated to [[fallthrough]], the header is
unneeded.

Reviewed By: JDevlieghere

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

Added: 


Modified: 
clang/docs/tools/clang-formatted-files.txt
lldb/include/lldb/lldb-private.h
lldb/include/lldb/module.modulemap

Removed: 
lldb/include/lldb/lldb-private-defines.h



diff  --git a/clang/docs/tools/clang-formatted-files.txt 
b/clang/docs/tools/clang-formatted-files.txt
index 81d0e9522e604..f89e19ca7cd3a 100644
--- a/clang/docs/tools/clang-formatted-files.txt
+++ b/clang/docs/tools/clang-formatted-files.txt
@@ -3591,7 +3591,6 @@ lldb/examples/plugins/commands/fooplugin.cpp
 lldb/examples/synthetic/bitfield/program.cpp
 lldb/include/lldb/lldb-defines.h
 lldb/include/lldb/lldb-forward.h
-lldb/include/lldb/lldb-private-defines.h
 lldb/include/lldb/lldb-private.h
 lldb/include/lldb/lldb-public.h
 lldb/include/lldb/lldb-versioning.h

diff  --git a/lldb/include/lldb/lldb-private-defines.h 
b/lldb/include/lldb/lldb-private-defines.h
deleted file mode 100644
index d66e6ef1518d7..0
--- a/lldb/include/lldb/lldb-private-defines.h
+++ /dev/null
@@ -1,36 +0,0 @@
-//===-- lldb-private-defines.h --*- C++ 
-*-===//
-//
-// 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
-//
-//===--===//
-
-#ifndef LLDB_LLDB_PRIVATE_DEFINES_H
-#define LLDB_LLDB_PRIVATE_DEFINES_H
-
-#if defined(__cplusplus)
-
-// Include Compiler.h here so we don't define LLVM_FALLTHROUGH and then
-// Compiler.h later tries to redefine it.
-#include "llvm/Support/Compiler.h"
-
-#ifndef LLVM_FALLTHROUGH
-
-#ifndef __has_cpp_attribute
-#define __has_cpp_attribute(x) 0
-#endif
-
-/// \macro LLVM_FALLTHROUGH
-/// Marks an empty statement preceding a deliberate switch fallthrough.
-#if __has_cpp_attribute(clang::fallthrough)
-#define LLVM_FALLTHROUGH [[clang::fallthrough]]
-#else
-#define LLVM_FALLTHROUGH
-#endif
-
-#endif // ifndef LLVM_FALLTHROUGH
-
-#endif // #if defined(__cplusplus)
-
-#endif // LLDB_LLDB_PRIVATE_DEFINES_H

diff  --git a/lldb/include/lldb/lldb-private.h 
b/lldb/include/lldb/lldb-private.h
index ac07047fb4edb..f56af06ec5973 100644
--- a/lldb/include/lldb/lldb-private.h
+++ b/lldb/include/lldb/lldb-private.h
@@ -11,7 +11,6 @@
 
 #if defined(__cplusplus)
 
-#include "lldb/lldb-private-defines.h"
 #include "lldb/lldb-private-enumerations.h"
 #include "lldb/lldb-private-interfaces.h"
 #include "lldb/lldb-private-types.h"

diff  --git a/lldb/include/lldb/module.modulemap 
b/lldb/include/lldb/module.modulemap
index 303d6b15e808c..c06e026076f49 100644
--- a/lldb/include/lldb/module.modulemap
+++ b/lldb/include/lldb/module.modulemap
@@ -134,7 +134,6 @@ module lldb_Utility {
   module lldb_enumerations { header "lldb-enumerations.h" export * }
   module lldb_forward { header "lldb-forward.h" export * }
   module lldb_private_enumerations { header "lldb-private-enumerations.h" 
export * }
-  module lldb_private_defines { header "lldb-private-defines.h" export * }
   module lldb_private_forward { header "lldb-private-forward.h" export * }
   module lldb_private { header "lldb-private.h" export * }
   module lldb_private_interfaces { header "lldb-private-interfaces.h" export * 
}



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

[Lldb-commits] [PATCH] D129611: [lldb/crashlog] Add '-t|--target' option to interactive mode

[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 modulo the `# Return error` but that's addressed in a follow up patch.


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

https://reviews.llvm.org/D129611

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

[Lldb-commits] [PATCH] D131032: [lldb/crashlog] Update frame regex matcher

[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


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

https://reviews.llvm.org/D131032

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

[Lldb-commits] [PATCH] D131422: [lldb] Remove include/lldb/lldb-private.h

[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/D131422/new/

https://reviews.llvm.org/D131422

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

[Lldb-commits] [PATCH] D131422: [lldb] Remove include/lldb/lldb-private.h

[Fangrui Song via Phabricator via lldb-commits]

MaskRay created this revision.
MaskRay added reviewers: JDevlieghere, kastiglione.
Herald added a subscriber: StephenFan.
Herald added a project: All.
MaskRay requested review of this revision.
Herald added projects: clang, LLDB.
Herald added subscribers: lldb-commits, cfe-commits.

The header from 62e0681afb478a4005efb6ba3598c24dc24866ee does something with
LLVM_FALLTHROUGH. Now that llvm-project has switched to C++17 and
LLVM_FALLTHROUGH uses have been migrated to [[fallthrough]], the header is
unneeded.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D131422

Files:
  clang/docs/tools/clang-formatted-files.txt
  lldb/include/lldb/lldb-private-defines.h
  lldb/include/lldb/lldb-private.h
  lldb/include/lldb/module.modulemap


Index: lldb/include/lldb/module.modulemap
===
--- lldb/include/lldb/module.modulemap
+++ lldb/include/lldb/module.modulemap
@@ -134,7 +134,6 @@
   module lldb_enumerations { header "lldb-enumerations.h" export * }
   module lldb_forward { header "lldb-forward.h" export * }
   module lldb_private_enumerations { header "lldb-private-enumerations.h" 
export * }
-  module lldb_private_defines { header "lldb-private-defines.h" export * }
   module lldb_private_forward { header "lldb-private-forward.h" export * }
   module lldb_private { header "lldb-private.h" export * }
   module lldb_private_interfaces { header "lldb-private-interfaces.h" export * 
}
Index: lldb/include/lldb/lldb-private.h
===
--- lldb/include/lldb/lldb-private.h
+++ lldb/include/lldb/lldb-private.h
@@ -11,7 +11,6 @@
 
 #if defined(__cplusplus)
 
-#include "lldb/lldb-private-defines.h"
 #include "lldb/lldb-private-enumerations.h"
 #include "lldb/lldb-private-interfaces.h"
 #include "lldb/lldb-private-types.h"
Index: lldb/include/lldb/lldb-private-defines.h
===
--- lldb/include/lldb/lldb-private-defines.h
+++ /dev/null
@@ -1,36 +0,0 @@
-//===-- lldb-private-defines.h --*- C++ 
-*-===//
-//
-// 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
-//
-//===--===//
-
-#ifndef LLDB_LLDB_PRIVATE_DEFINES_H
-#define LLDB_LLDB_PRIVATE_DEFINES_H
-
-#if defined(__cplusplus)
-
-// Include Compiler.h here so we don't define LLVM_FALLTHROUGH and then
-// Compiler.h later tries to redefine it.
-#include "llvm/Support/Compiler.h"
-
-#ifndef LLVM_FALLTHROUGH
-
-#ifndef __has_cpp_attribute
-#define __has_cpp_attribute(x) 0
-#endif
-
-/// \macro LLVM_FALLTHROUGH
-/// Marks an empty statement preceding a deliberate switch fallthrough.
-#if __has_cpp_attribute(clang::fallthrough)
-#define LLVM_FALLTHROUGH [[clang::fallthrough]]
-#else
-#define LLVM_FALLTHROUGH
-#endif
-
-#endif // ifndef LLVM_FALLTHROUGH
-
-#endif // #if defined(__cplusplus)
-
-#endif // LLDB_LLDB_PRIVATE_DEFINES_H
Index: clang/docs/tools/clang-formatted-files.txt
===
--- clang/docs/tools/clang-formatted-files.txt
+++ clang/docs/tools/clang-formatted-files.txt
@@ -3591,7 +3591,6 @@
 lldb/examples/synthetic/bitfield/program.cpp
 lldb/include/lldb/lldb-defines.h
 lldb/include/lldb/lldb-forward.h
-lldb/include/lldb/lldb-private-defines.h
 lldb/include/lldb/lldb-private.h
 lldb/include/lldb/lldb-public.h
 lldb/include/lldb/lldb-versioning.h


Index: lldb/include/lldb/module.modulemap
===
--- lldb/include/lldb/module.modulemap
+++ lldb/include/lldb/module.modulemap
@@ -134,7 +134,6 @@
   module lldb_enumerations { header "lldb-enumerations.h" export * }
   module lldb_forward { header "lldb-forward.h" export * }
   module lldb_private_enumerations { header "lldb-private-enumerations.h" export * }
-  module lldb_private_defines { header "lldb-private-defines.h" export * }
   module lldb_private_forward { header "lldb-private-forward.h" export * }
   module lldb_private { header "lldb-private.h" export * }
   module lldb_private_interfaces { header "lldb-private-interfaces.h" export * }
Index: lldb/include/lldb/lldb-private.h
===
--- lldb/include/lldb/lldb-private.h
+++ lldb/include/lldb/lldb-private.h
@@ -11,7 +11,6 @@
 
 #if defined(__cplusplus)
 
-#include "lldb/lldb-private-defines.h"
 #include "lldb/lldb-private-enumerations.h"
 #include "lldb/lldb-private-interfaces.h"
 #include "lldb/lldb-private-types.h"
Index: lldb/include/lldb/lldb-private-defines.h
===
--- lldb/include/lldb/lldb-private-defines.h
+++ /dev/null
@@ -1,36 +0,0 @@
-//===-- lldb-private-defines.h 

[Lldb-commits] [PATCH] D130689: [LLVM] Update C++ standard to 17

[Trass3r via Phabricator via lldb-commits]

Trass3r added a comment.

In D130689#3707296 , @mehdi_amini 
wrote:

> land the change, wait for a couple of hours to ensure that all bots have 
> picked up the revision, then revert

There should be a better way than this. Comprehensive pre-merge testing of all 
PRs etc.
But I know that the build times are a problem here. Even with incremental 
builds and ccache most commits cause a rebuild of many files.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130689

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

[Lldb-commits] [lldb] 59d2495 - [lldb] LLVM_FALLTHROUGH => [[fallthrough]]. NFC

[Fangrui Song via lldb-commits]

Author: Fangrui Song
Date: 2022-08-08T11:31:49-07:00
New Revision: 59d2495fe2c1254f84ad4d2bd61a41e79ff0d44d

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

LOG: [lldb] LLVM_FALLTHROUGH => [[fallthrough]]. NFC

Added: 


Modified: 
lldb/source/Commands/CommandObjectMemory.cpp
lldb/source/Core/Address.cpp
lldb/source/Core/AddressRange.cpp
lldb/source/Core/Communication.cpp
lldb/source/Core/FormatEntity.cpp
lldb/source/Core/ValueObject.cpp
lldb/source/Expression/DWARFExpression.cpp
lldb/source/Expression/REPL.cpp
lldb/source/Interpreter/OptionValueArray.cpp
lldb/source/Interpreter/OptionValueFileSpecList.cpp
lldb/source/Interpreter/OptionValuePathMappings.cpp
lldb/source/Interpreter/Options.cpp
lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
lldb/source/Plugins/Language/ObjC/Cocoa.cpp

lldb/source/Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/RenderScriptExpressionOpts.cpp
lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp
lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp
lldb/source/Plugins/Process/MacOSX-Kernel/CommunicationKDP.cpp
lldb/source/Plugins/Process/Windows/Common/NativeProcessWindows.cpp
lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp
lldb/source/Plugins/SymbolFile/DWARF/HashedNameToDIE.cpp
lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp
lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
lldb/source/Target/Process.cpp
lldb/source/Target/StackFrame.cpp
lldb/source/Target/StackFrameList.cpp
lldb/source/Utility/ArchSpec.cpp
lldb/tools/lldb-test/lldb-test.cpp
lldb/unittests/Expression/DWARFExpressionTest.cpp

Removed: 




diff  --git a/lldb/source/Commands/CommandObjectMemory.cpp 
b/lldb/source/Commands/CommandObjectMemory.cpp
index 5051f9aeec851..22483e7c597c7 100644
--- a/lldb/source/Commands/CommandObjectMemory.cpp
+++ b/lldb/source/Commands/CommandObjectMemory.cpp
@@ -427,7 +427,7 @@ class CommandObjectMemoryRead : public CommandObjectParsed {
   switch (type_str[type_str.size() - 1]) {
   case '*':
 ++pointer_count;
-LLVM_FALLTHROUGH;
+[[fallthrough]];
   case ' ':
   case '\t':
 type_str.erase(type_str.size() - 1);

diff  --git a/lldb/source/Core/Address.cpp b/lldb/source/Core/Address.cpp
index b84e1ac8e1695..5969cf11d865c 100644
--- a/lldb/source/Core/Address.cpp
+++ b/lldb/source/Core/Address.cpp
@@ -451,7 +451,7 @@ bool Address::Dump(Stream *s, ExecutionContextScope 
*exe_scope, DumpStyle style,
   else
 s->Printf("%s[", "");
 }
-LLVM_FALLTHROUGH;
+[[fallthrough]];
   case DumpStyleFileAddress: {
 addr_t file_addr = GetFileAddress();
 if (file_addr == LLDB_INVALID_ADDRESS) {

diff  --git a/lldb/source/Core/AddressRange.cpp 
b/lldb/source/Core/AddressRange.cpp
index 66dcda574890a..1830f2ccd47fe 100644
--- a/lldb/source/Core/AddressRange.cpp
+++ b/lldb/source/Core/AddressRange.cpp
@@ -169,7 +169,7 @@ bool AddressRange::Dump(Stream *s, Target *target, 
Address::DumpStyle style,
 
   case Address::DumpStyleModuleWithFileAddress:
 show_module = true;
-LLVM_FALLTHROUGH;
+[[fallthrough]];
   case Address::DumpStyleFileAddress:
 vmaddr = m_base_addr.GetFileAddress();
 break;

diff  --git a/lldb/source/Core/Communication.cpp 
b/lldb/source/Core/Communication.cpp
index f41ce46ede88f..d5f8f8922f5e1 100644
--- a/lldb/source/Core/Communication.cpp
+++ b/lldb/source/Core/Communication.cpp
@@ -356,7 +356,7 @@ lldb::thread_result_t Communication::ReadThread() {
 case eConnectionStatusLostConnection: // Lost connection while connected to
   // a valid connection
   done = true;
-  LLVM_FALLTHROUGH;
+  [[fallthrough]];
 case eConnectionStatusTimedOut: // Request timed out
   if (error.Fail())
 LLDB_LOG(log, "error: {0}, status = {1}", error,

diff  --git a/lldb/source/Core/FormatEntity.cpp 
b/lldb/source/Core/FormatEntity.cpp
index 6e7f44e461fa6..0ac8fcabc61b9 100644
--- a/lldb/source/Core/FormatEntity.cpp
+++ b/lldb/source/Core/FormatEntity.cpp
@@ -696,7 +696,7 @@ static bool DumpValue(Stream , const SymbolContext *sc,
 
 

[Lldb-commits] [PATCH] D130689: [LLVM] Update C++ standard to 17

[Nico Weber via Phabricator via lldb-commits]

thakis added a comment.

Re MSVC: For what it's worth, our Chromium windows bot (which builds trunk llvm 
and then chromium with that) is happy. It builds with MSVC 2019. Here's the 
build log (which includes cmake invocations): 
https://logs.chromium.org/logs/chromium/buildbucket/cr-buildbucket/8806536120229097233/+/u/gclient_runhooks/stdout?format=raw

(Look for `DCMAKE_BUILD_TYPE=Release'`.)

The bot builds clang;lld;clang-tools-extra. It also builds compiler-rt, but 
using the runtimes build, so I think compiler-rt is built with the just-built 
clang.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130689

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

[Lldb-commits] [PATCH] D131304: [lldb] Remove uses of six module (NFC)

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere accepted this revision.
JDevlieghere added a comment.

拾


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131304

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

[Lldb-commits] [lldb] 5fff4b7 - [lldb] Pass TestExternCSymbols.py on Windows

[Alex Langford via lldb-commits]

Author: Alex Langford
Date: 2022-08-08T11:01:16-07:00
New Revision: 5fff4b75ca0d72759f7ded4c96fc5493d917a3a9

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

LOG: [lldb] Pass TestExternCSymbols.py on Windows

This test previously was expected to fail on windows. As of my previous
patch (1d2a62afaf7561e331e2f3daf3937d14225b21bf) this test now passes on
windows consistently. This patch adjusts the expectations of the test
accordingly.

Added: 


Modified: 
lldb/test/API/lang/cpp/extern_c/TestExternCSymbols.py

Removed: 




diff  --git a/lldb/test/API/lang/cpp/extern_c/TestExternCSymbols.py 
b/lldb/test/API/lang/cpp/extern_c/TestExternCSymbols.py
index b1b7e1744f1f8..c8308c16011e0 100644
--- a/lldb/test/API/lang/cpp/extern_c/TestExternCSymbols.py
+++ b/lldb/test/API/lang/cpp/extern_c/TestExternCSymbols.py
@@ -1,4 +1,4 @@
 from lldbsuite.test import lldbinline
 from lldbsuite.test import decorators
 
-lldbinline.MakeInlineTest(__file__, globals(), 
lldbinline.expectedFailureAll(oslist=["windows"]))
+lldbinline.MakeInlineTest(__file__, globals())



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

[Lldb-commits] [PATCH] D130689: [LLVM] Update C++ standard to 17

[Mehdi AMINI via Phabricator via lldb-commits]

mehdi_amini added a comment.

> Agreed, I think we need to update the protocol for changing the C++ standard 
> in the future to account for more testing beforehand.

The way I would do it is: wait for a Sunday so that the bots aren't loaded, 
land the change, wait for a couple of hours to ensure that all bots have picked 
up the revision, then revert and survey the results for the runs. Communicating 
ahead of time on this also so that downstream can pickup the revision for their 
own tests if they want.
This should provide a pretty good signal ahead of time of the "real" migration 
hopefully!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130689

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

[Lldb-commits] [PATCH] D131294: [LLDB][NFC] Reliability fixes to TCPSocket code

[Slava Gurevich via Phabricator via lldb-commits]

fixathon added a comment.

Commented on feedback.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131294

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

[Lldb-commits] [PATCH] D131294: [LLDB][NFC] Reliability fixes to TCPSocket code

[Slava Gurevich via Phabricator via lldb-commits]

fixathon added a comment.

In D131294#3707154 , @jingham wrote:

> We don't use the constant first so you don't accidentally use "=" rather than 
> "==" convention anywhere else in lldb, so it looks weird here.

Good feedback. I'll switch that around.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131294

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

[Lldb-commits] [PATCH] D131294: [LLDB][NFC] Reliability fixes to TCPSocket code

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

We don't use the constant first so you don't accidentally use "=" rather than 
"==" convention anywhere else in lldb, so it looks weird here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131294

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

[Lldb-commits] [PATCH] D131244: [LLDB] Missing break in a switch statement alters the execution flow.

[Slava Gurevich via Phabricator via lldb-commits]

fixathon abandoned this revision.
fixathon marked an inline comment as done.
fixathon added a comment.

closing as already patched.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131244

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

[Lldb-commits] [PATCH] D131304: [lldb] Remove uses of six module (NFC)

[Dave Lee via Phabricator via lldb-commits]

kastiglione added inline comments.



Comment at: lldb/bindings/python/CMakeLists.txt:63-68
-  if(NOT LLDB_USE_SYSTEM_SIX)
-add_custom_command(TARGET ${swig_target} POST_BUILD VERBATIM
-  COMMAND ${CMAKE_COMMAND} -E copy
-"${LLDB_SOURCE_DIR}/third_party/Python/module/six/six.py"
-"${lldb_python_target_dir}/../six.py")
-  endif()

@DavidSpickett I removed `LLDB_USE_SYSTEM_SIX` but left the code that copies 
vendored copy of six.py. Is this what you had in mind? My thinking is there 
might be lldb scripts that use `six`, and I don't want the removal of internal 
use of `six` to be a breaking change downstream, not yet anyway.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131304

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

[Lldb-commits] [PATCH] D131304: [lldb] Remove uses of six module (NFC)

[Dave Lee via Phabricator via lldb-commits]

kastiglione updated this revision to Diff 450843.
kastiglione added a comment.
Herald added a subscriber: mgorny.

remove LLDB_USE_SYSTEM_SIX


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131304

Files:
  lldb/bindings/interface/SBData.i
  lldb/bindings/python/CMakeLists.txt
  lldb/bindings/python/python.swig
  lldb/cmake/modules/LLDBConfig.cmake
  lldb/examples/python/scripted_process/scripted_process.py
  lldb/examples/summaries/synth.py
  lldb/packages/Python/lldbsuite/support/encoded_file.py
  lldb/packages/Python/lldbsuite/support/seven.py
  lldb/packages/Python/lldbsuite/test/decorators.py
  lldb/packages/Python/lldbsuite/test/dotest.py
  lldb/packages/Python/lldbsuite/test/lldbpexpect.py
  lldb/packages/Python/lldbsuite/test/lldbplatform.py
  lldb/packages/Python/lldbsuite/test/lldbplatformutil.py
  lldb/packages/Python/lldbsuite/test/lldbtest.py
  lldb/packages/Python/lldbsuite/test/lldbutil.py
  lldb/packages/Python/lldbsuite/test/tools/lldb-server/gdbremote_testcase.py
  lldb/packages/Python/lldbsuite/test/tools/lldb-server/lldbgdbserverutils.py
  lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
  lldb/test/API/api/listeners/TestListener.py
  lldb/test/API/commands/command/script/import/thepackage/TPunitA.py
  lldb/test/API/commands/command/script/import/thepackage/TPunitB.py
  lldb/test/API/commands/process/launch/TestProcessLaunch.py
  lldb/test/API/functionalities/gdb_remote_client/TestGdbClientModuleLoad.py
  lldb/test/API/functionalities/postmortem/minidump-new/TestMiniDumpNew.py
  lldb/test/API/functionalities/postmortem/minidump-new/TestMiniDumpUUID.py
  lldb/test/API/functionalities/postmortem/minidump/TestMiniDump.py
  lldb/test/API/functionalities/postmortem/wow64_minidump/TestWow64MiniDump.py
  lldb/test/API/python_api/frame/TestFrames.py
  lldb/test/API/terminal/TestSTTYBeforeAndAfter.py
  lldb/test/API/test_utils/base/TestBaseTest.py
  lldb/third_party/Python/module/progress/progress.py
  lldb/third_party/Python/module/unittest2/unittest2/case.py
  lldb/third_party/Python/module/unittest2/unittest2/main.py
  lldb/third_party/Python/module/unittest2/unittest2/result.py
  lldb/third_party/Python/module/unittest2/unittest2/suite.py
  lldb/third_party/Python/module/unittest2/unittest2/test/test_case.py
  
lldb/third_party/Python/module/unittest2/unittest2/test/test_functiontestcase.py

Index: lldb/third_party/Python/module/unittest2/unittest2/test/test_functiontestcase.py
===
--- lldb/third_party/Python/module/unittest2/unittest2/test/test_functiontestcase.py
+++ lldb/third_party/Python/module/unittest2/unittest2/test/test_functiontestcase.py
@@ -1,5 +1,4 @@
 import unittest2
-import six
 
 from unittest2.test.support import LoggingResult
 
@@ -125,7 +124,7 @@
 def test_id(self):
 test = unittest2.FunctionTestCase(lambda: None)
 
-self.assertIsInstance(test.id(), six.string_types)
+self.assertIsInstance(test.id(), str)
 
 # "Returns a one-line description of the test, or None if no description
 # has been provided. The default implementation of this method returns
Index: lldb/third_party/Python/module/unittest2/unittest2/test/test_case.py
===
--- lldb/third_party/Python/module/unittest2/unittest2/test/test_case.py
+++ lldb/third_party/Python/module/unittest2/unittest2/test/test_case.py
@@ -1,7 +1,6 @@
 import difflib
 import pprint
 import re
-import six
 
 from copy import deepcopy
 
@@ -543,7 +542,7 @@
 def runTest(self):
 pass
 
-self.assertIsInstance(Foo().id(), six.string_types)
+self.assertIsInstance(Foo().id(), str)
 
 # "If result is omitted or None, a temporary result object is created
 # and used, but is not made available to the caller. As TestCase owns the
Index: lldb/third_party/Python/module/unittest2/unittest2/suite.py
===
--- lldb/third_party/Python/module/unittest2/unittest2/suite.py
+++ lldb/third_party/Python/module/unittest2/unittest2/suite.py
@@ -3,7 +3,6 @@
 import sys
 import unittest
 from unittest2 import case, util
-import six
 
 __unittest = True
 
@@ -50,7 +49,7 @@
 self._tests.append(test)
 
 def addTests(self, tests):
-if isinstance(tests, six.string_types):
+if isinstance(tests, str):
 raise TypeError("tests must be an iterable of tests, not a string")
 for test in tests:
 self.addTest(test)
Index: lldb/third_party/Python/module/unittest2/unittest2/result.py
===
--- lldb/third_party/Python/module/unittest2/unittest2/result.py
+++ lldb/third_party/Python/module/unittest2/unittest2/result.py
@@ -2,12 +2,11 @@
 
 import use_lldb_suite
 
+import io
 

[Lldb-commits] [PATCH] D131159: [lldb] Use WSAEventSelect for MainLoop polling on windows

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

mgorny accepted this revision.
mgorny added a comment.

Thanks for doing this. The common and POSIX parts LGTM. Just one minor 
suggestion since you've touched that line.




Comment at: lldb/source/Host/posix/MainLoopPosix.cpp:124
   assert(ret == 0);
-  (void) ret;
+  (void)ret;
 




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131159

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

[Lldb-commits] [PATCH] D131312: [LLDB][NFC] Fix suspicious bitwise expression in PrintBTEntry()

[Slava Gurevich via Phabricator via lldb-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
fixathon marked an inline comment as done.
Closed by commit rG06ff46d2d77f: [LLDB][NFC] Fix suspicious bitwise expression 
in PrintBTEntry() (authored by fixathon).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131312

Files:
  lldb/tools/intel-features/intel-mpx/cli-wrapper-mpxtable.cpp


Index: lldb/tools/intel-features/intel-mpx/cli-wrapper-mpxtable.cpp
===
--- lldb/tools/intel-features/intel-mpx/cli-wrapper-mpxtable.cpp
+++ lldb/tools/intel-features/intel-mpx/cli-wrapper-mpxtable.cpp
@@ -63,7 +63,7 @@
   const lldb::addr_t one_cmpl64 = ~((lldb::addr_t)0);
   const lldb::addr_t one_cmpl32 = ~((uint32_t)0);
 
-  if ((lbound == one_cmpl64 || one_cmpl32) && ubound == 0) {
+  if ((lbound == one_cmpl64 || lbound == one_cmpl32) && ubound == 0) {
 result.Printf("Null bounds on map: pointer value = 0x%" PRIu64 "\n", 
value);
   } else {
 result.Printf("lbound = 0x%" PRIu64 ",", lbound);


Index: lldb/tools/intel-features/intel-mpx/cli-wrapper-mpxtable.cpp
===
--- lldb/tools/intel-features/intel-mpx/cli-wrapper-mpxtable.cpp
+++ lldb/tools/intel-features/intel-mpx/cli-wrapper-mpxtable.cpp
@@ -63,7 +63,7 @@
   const lldb::addr_t one_cmpl64 = ~((lldb::addr_t)0);
   const lldb::addr_t one_cmpl32 = ~((uint32_t)0);
 
-  if ((lbound == one_cmpl64 || one_cmpl32) && ubound == 0) {
+  if ((lbound == one_cmpl64 || lbound == one_cmpl32) && ubound == 0) {
 result.Printf("Null bounds on map: pointer value = 0x%" PRIu64 "\n", value);
   } else {
 result.Printf("lbound = 0x%" PRIu64 ",", lbound);
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] 06ff46d - [LLDB][NFC] Fix suspicious bitwise expression in PrintBTEntry()

[Slava Gurevich via lldb-commits]

Author: Slava Gurevich
Date: 2022-08-08T09:04:22-07:00
New Revision: 06ff46d2d77feba285e672e2da42039c88dc965a

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

LOG: [LLDB][NFC] Fix suspicious bitwise expression in PrintBTEntry()

Current application of bitwise-OR to a binary mask always results in True, 
which seems
inconsistent with the intent of the statement, a likely typo.

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

Added: 


Modified: 
lldb/tools/intel-features/intel-mpx/cli-wrapper-mpxtable.cpp

Removed: 




diff  --git a/lldb/tools/intel-features/intel-mpx/cli-wrapper-mpxtable.cpp 
b/lldb/tools/intel-features/intel-mpx/cli-wrapper-mpxtable.cpp
index b19d8b7387bb..de6d50038a27 100644
--- a/lldb/tools/intel-features/intel-mpx/cli-wrapper-mpxtable.cpp
+++ b/lldb/tools/intel-features/intel-mpx/cli-wrapper-mpxtable.cpp
@@ -63,7 +63,7 @@ static void PrintBTEntry(lldb::addr_t lbound, lldb::addr_t 
ubound,
   const lldb::addr_t one_cmpl64 = ~((lldb::addr_t)0);
   const lldb::addr_t one_cmpl32 = ~((uint32_t)0);
 
-  if ((lbound == one_cmpl64 || one_cmpl32) && ubound == 0) {
+  if ((lbound == one_cmpl64 || lbound == one_cmpl32) && ubound == 0) {
 result.Printf("Null bounds on map: pointer value = 0x%" PRIu64 "\n", 
value);
   } else {
 result.Printf("lbound = 0x%" PRIu64 ",", lbound);



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

[Lldb-commits] [PATCH] D131244: [LLDB] Missing break in a switch statement alters the execution flow.

[Slava Gurevich via Phabricator via lldb-commits]

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



Comment at: lldb/source/Plugins/Process/Utility/ARMUtils.h:49
   shift_t = SRType_Invalid;
   return UINT32_MAX;
 }

DavidSpickett wrote:
> This is now dead code?
This will execute for the default case after its break.



Comment at: lldb/source/Plugins/Process/Utility/ARMUtils.h:29
+// assert(0 && "Invalid shift type");
+break;
   case 0:

DavidSpickett wrote:
> Looking at the codepaths and the reason this function exists, it's either 
> called with a 2 bit number which is the "type" field of an instruction, or 
> it's not called because the type is known to be SRType_RRX already.
> 
> In the enum ARM_ShifterType SRType_RRX would be 4, which wouldn't fit the 2 
> bit expectation.
> 
> So I think we can safely re-enable this assert and move the two unreachable 
> lines up here. I didn't get any test failures doing so and I don't see a 
> codepath that could hit it.
> 
> (but it would be fairly easy for future changes to make this mistake since 
> it's not very obvious from the types, so an assert is good here)
Added the assert back


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131244

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

[Lldb-commits] [PATCH] D131328: [lldb] Support fetching symbols with dsymForUUID in the background

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D131328#3706749 , @JDevlieghere 
wrote:

> There was another approach I considered initially which might alleviate some 
> of the issues Pavel raised. Instead of using the global thread pool, that 
> plan consisted of a dedicated thread to fetch symbols in the background. That 
> would allow us to add the symbol info synchronously. For example on every 
> stop we could ask if there are any new symbol files to add instead of trying 
> to add the symbol info as soon as it becomes available.

I kinda like that. Bonus points if you make that single thread download 
multiple files at the same time (using threads to paralelize downloading is not 
the best approach, since the task is not cpu-bound). :)


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

https://reviews.llvm.org/D131328

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

[Lldb-commits] [PATCH] D131407: [lldb] Flush the global thread pool in Debugger::Terminate

[Jonas Devlieghere via Phabricator via lldb-commits]

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

- Create the thread pool before the plugin callback


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

https://reviews.llvm.org/D131407

Files:
  lldb/source/Core/Debugger.cpp


Index: lldb/source/Core/Debugger.cpp
===
--- lldb/source/Core/Debugger.cpp
+++ lldb/source/Core/Debugger.cpp
@@ -104,6 +104,7 @@
 nullptr; // NOTE: intentional leak to avoid issues with C++ destructor 
chain
 static DebuggerList *g_debugger_list_ptr =
 nullptr; // NOTE: intentional leak to avoid issues with C++ destructor 
chain
+static llvm::ThreadPool *g_thread_pool = nullptr;
 
 static constexpr OptionEnumValueElement g_show_disassembly_enum_values[] = {
 {
@@ -538,6 +539,7 @@
  "Debugger::Initialize called more than once!");
   g_debugger_list_mutex_ptr = new std::recursive_mutex();
   g_debugger_list_ptr = new DebuggerList();
+  g_thread_pool = new llvm::ThreadPool(llvm::optimal_concurrency());
   g_load_plugin_callback = load_plugin_callback;
 }
 
@@ -545,6 +547,11 @@
   assert(g_debugger_list_ptr &&
  "Debugger::Terminate called without a matching 
Debugger::Initialize!");
 
+  if (g_thread_pool) {
+// The destructor will wait for all the threads to complete.
+delete g_thread_pool;
+  }
+
   if (g_debugger_list_ptr && g_debugger_list_mutex_ptr) {
 // Clear our global list of debugger objects
 {
@@ -2005,11 +2012,7 @@
 }
 
 llvm::ThreadPool ::GetThreadPool() {
-  // NOTE: intentional leak to avoid issues with C++ destructor chain
-  static llvm::ThreadPool *g_thread_pool = nullptr;
-  static llvm::once_flag g_once_flag;
-  llvm::call_once(g_once_flag, []() {
-g_thread_pool = new llvm::ThreadPool(llvm::optimal_concurrency());
-  });
+  assert(g_thread_pool &&
+ "Debugger::GetThreadPool called before Debugger::Initialize");
   return *g_thread_pool;
 }


Index: lldb/source/Core/Debugger.cpp
===
--- lldb/source/Core/Debugger.cpp
+++ lldb/source/Core/Debugger.cpp
@@ -104,6 +104,7 @@
 nullptr; // NOTE: intentional leak to avoid issues with C++ destructor chain
 static DebuggerList *g_debugger_list_ptr =
 nullptr; // NOTE: intentional leak to avoid issues with C++ destructor chain
+static llvm::ThreadPool *g_thread_pool = nullptr;
 
 static constexpr OptionEnumValueElement g_show_disassembly_enum_values[] = {
 {
@@ -538,6 +539,7 @@
  "Debugger::Initialize called more than once!");
   g_debugger_list_mutex_ptr = new std::recursive_mutex();
   g_debugger_list_ptr = new DebuggerList();
+  g_thread_pool = new llvm::ThreadPool(llvm::optimal_concurrency());
   g_load_plugin_callback = load_plugin_callback;
 }
 
@@ -545,6 +547,11 @@
   assert(g_debugger_list_ptr &&
  "Debugger::Terminate called without a matching Debugger::Initialize!");
 
+  if (g_thread_pool) {
+// The destructor will wait for all the threads to complete.
+delete g_thread_pool;
+  }
+
   if (g_debugger_list_ptr && g_debugger_list_mutex_ptr) {
 // Clear our global list of debugger objects
 {
@@ -2005,11 +2012,7 @@
 }
 
 llvm::ThreadPool ::GetThreadPool() {
-  // NOTE: intentional leak to avoid issues with C++ destructor chain
-  static llvm::ThreadPool *g_thread_pool = nullptr;
-  static llvm::once_flag g_once_flag;
-  llvm::call_once(g_once_flag, []() {
-g_thread_pool = new llvm::ThreadPool(llvm::optimal_concurrency());
-  });
+  assert(g_thread_pool &&
+ "Debugger::GetThreadPool called before Debugger::Initialize");
   return *g_thread_pool;
 }
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D131407: [lldb] Flush the global thread pool in Debugger::Terminate

[Jonas Devlieghere via Phabricator via lldb-commits]

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

Use the Initialize/Terminate pattern for the global thread pool to make sure it 
gets flushed during teardown.


https://reviews.llvm.org/D131407

Files:
  lldb/source/Core/Debugger.cpp


Index: lldb/source/Core/Debugger.cpp
===
--- lldb/source/Core/Debugger.cpp
+++ lldb/source/Core/Debugger.cpp
@@ -104,6 +104,7 @@
 nullptr; // NOTE: intentional leak to avoid issues with C++ destructor 
chain
 static DebuggerList *g_debugger_list_ptr =
 nullptr; // NOTE: intentional leak to avoid issues with C++ destructor 
chain
+static llvm::ThreadPool *g_thread_pool = nullptr;
 
 static constexpr OptionEnumValueElement g_show_disassembly_enum_values[] = {
 {
@@ -539,12 +540,18 @@
   g_debugger_list_mutex_ptr = new std::recursive_mutex();
   g_debugger_list_ptr = new DebuggerList();
   g_load_plugin_callback = load_plugin_callback;
+  g_thread_pool = new llvm::ThreadPool(llvm::optimal_concurrency());
 }
 
 void Debugger::Terminate() {
   assert(g_debugger_list_ptr &&
  "Debugger::Terminate called without a matching 
Debugger::Initialize!");
 
+  if (g_thread_pool) {
+// The destructor will wait for all the threads to complete.
+delete g_thread_pool;
+  }
+
   if (g_debugger_list_ptr && g_debugger_list_mutex_ptr) {
 // Clear our global list of debugger objects
 {
@@ -2005,11 +2012,7 @@
 }
 
 llvm::ThreadPool ::GetThreadPool() {
-  // NOTE: intentional leak to avoid issues with C++ destructor chain
-  static llvm::ThreadPool *g_thread_pool = nullptr;
-  static llvm::once_flag g_once_flag;
-  llvm::call_once(g_once_flag, []() {
-g_thread_pool = new llvm::ThreadPool(llvm::optimal_concurrency());
-  });
+  assert(g_thread_pool &&
+ "Debugger::GetThreadPool called before Debugger::Initialize");
   return *g_thread_pool;
 }


Index: lldb/source/Core/Debugger.cpp
===
--- lldb/source/Core/Debugger.cpp
+++ lldb/source/Core/Debugger.cpp
@@ -104,6 +104,7 @@
 nullptr; // NOTE: intentional leak to avoid issues with C++ destructor chain
 static DebuggerList *g_debugger_list_ptr =
 nullptr; // NOTE: intentional leak to avoid issues with C++ destructor chain
+static llvm::ThreadPool *g_thread_pool = nullptr;
 
 static constexpr OptionEnumValueElement g_show_disassembly_enum_values[] = {
 {
@@ -539,12 +540,18 @@
   g_debugger_list_mutex_ptr = new std::recursive_mutex();
   g_debugger_list_ptr = new DebuggerList();
   g_load_plugin_callback = load_plugin_callback;
+  g_thread_pool = new llvm::ThreadPool(llvm::optimal_concurrency());
 }
 
 void Debugger::Terminate() {
   assert(g_debugger_list_ptr &&
  "Debugger::Terminate called without a matching Debugger::Initialize!");
 
+  if (g_thread_pool) {
+// The destructor will wait for all the threads to complete.
+delete g_thread_pool;
+  }
+
   if (g_debugger_list_ptr && g_debugger_list_mutex_ptr) {
 // Clear our global list of debugger objects
 {
@@ -2005,11 +2012,7 @@
 }
 
 llvm::ThreadPool ::GetThreadPool() {
-  // NOTE: intentional leak to avoid issues with C++ destructor chain
-  static llvm::ThreadPool *g_thread_pool = nullptr;
-  static llvm::once_flag g_once_flag;
-  llvm::call_once(g_once_flag, []() {
-g_thread_pool = new llvm::ThreadPool(llvm::optimal_concurrency());
-  });
+  assert(g_thread_pool &&
+ "Debugger::GetThreadPool called before Debugger::Initialize");
   return *g_thread_pool;
 }
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D131159: [WIP][lldb] Use WSAEventSelect for MainLoop polling

[Pavel Labath via Phabricator via lldb-commits]

labath updated this revision to Diff 450825.
labath added a comment.
Herald added a subscriber: krytarowski.

non-wip version


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131159

Files:
  lldb/include/lldb/Host/MainLoop.h
  lldb/include/lldb/Host/MainLoopBase.h
  lldb/include/lldb/Host/posix/MainLoopPosix.h
  lldb/include/lldb/Host/windows/MainLoopWindows.h
  lldb/source/Host/CMakeLists.txt
  lldb/source/Host/common/MainLoop.cpp
  lldb/source/Host/common/MainLoopBase.cpp
  lldb/source/Host/posix/MainLoopPosix.cpp
  lldb/source/Host/windows/MainLoopWindows.cpp

Index: lldb/source/Host/windows/MainLoopWindows.cpp
===
--- /dev/null
+++ lldb/source/Host/windows/MainLoopWindows.cpp
@@ -0,0 +1,88 @@
+//===-- MainLoopWindows.cpp ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "lldb/Host/windows/MainLoopWindows.h"
+#include "lldb/Host/Config.h"
+#include "lldb/Utility/Status.h"
+#include "llvm/Config/llvm-config.h"
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+using namespace lldb;
+using namespace lldb_private;
+
+llvm::Expected MainLoopWindows::Poll() {
+  std::vector read_events;
+  read_events.reserve(m_read_fds.size());
+  for (auto  : m_read_fds) {
+WSAEVENT event = WSACreateEvent();
+assert(event != WSA_INVALID_EVENT);
+
+int result =
+WSAEventSelect(fd.first, event, FD_READ | FD_ACCEPT | FD_CLOSE);
+assert(result == 0);
+
+read_events.push_back(event);
+  }
+
+  DWORD result = WSAWaitForMultipleEvents(
+  read_events.size(), read_events.data(), FALSE, WSA_INFINITE, FALSE);
+
+  for (auto  : m_read_fds) {
+int result = WSAEventSelect(fd.first, WSA_INVALID_EVENT, 0);
+assert(result == 0);
+  }
+
+  for (auto  : read_events) {
+BOOL result = WSACloseEvent(event);
+assert(result == TRUE);
+  }
+
+  if (result >= WSA_WAIT_EVENT_0 &&
+  result < WSA_WAIT_EVENT_0 + read_events.size())
+return result - WSA_WAIT_EVENT_0;
+
+  return llvm::createStringError(llvm::inconvertibleErrorCode(),
+ "WSAWaitForMultipleEvents failed");
+}
+
+MainLoopWindows::ReadHandleUP
+MainLoopWindows::RegisterReadObject(const IOObjectSP _sp,
+const Callback , Status ) {
+  if (object_sp->GetFdType() != IOObject::eFDTypeSocket) {
+error.SetErrorString(
+"MainLoopWindows: non-socket types unsupported on Windows");
+return nullptr;
+  }
+  return MainLoopBase::RegisterReadObject(object_sp, callback, error);
+}
+
+Status MainLoopWindows::Run() {
+  m_terminate_request = false;
+
+  Status error;
+
+  // run until termination or until we run out of things to listen to
+  while (!m_terminate_request && !m_read_fds.empty()) {
+
+llvm::Expected signaled_event = Poll();
+if (!signaled_event)
+  return Status(signaled_event.takeError());
+
+auto _info = *std::next(m_read_fds.begin(), *signaled_event);
+
+ProcessReadObject(fd_info.first);
+ProcessPendingCallbacks();
+  }
+  return Status();
+}
Index: lldb/source/Host/posix/MainLoopPosix.cpp
===
--- lldb/source/Host/posix/MainLoopPosix.cpp
+++ lldb/source/Host/posix/MainLoopPosix.cpp
@@ -1,4 +1,4 @@
-//===-- MainLoop.cpp --===//
+//===-- MainLoopPosix.cpp -===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
@@ -6,12 +6,11 @@
 //
 //===--===//
 
-#include "llvm/Config/llvm-config.h"
+#include "lldb/Host/posix/MainLoopPosix.h"
 #include "lldb/Host/Config.h"
-
-#include "lldb/Host/MainLoop.h"
 #include "lldb/Host/PosixApi.h"
 #include "lldb/Utility/Status.h"
+#include "llvm/Config/llvm-config.h"
 #include 
 #include 
 #include 
@@ -26,59 +25,32 @@
 
 #if HAVE_SYS_EVENT_H
 #include 
-#elif defined(_WIN32)
-#include 
 #elif defined(__ANDROID__)
 #include 
 #else
 #include 
 #endif
 
-#ifdef _WIN32
-#define POLL WSAPoll
-#else
-#define POLL poll
-#endif
-
-#if SIGNAL_POLLING_UNSUPPORTED
-#ifdef _WIN32
-typedef int sigset_t;
-typedef int siginfo_t;
-#endif
-
-int ppoll(struct pollfd *fds, size_t nfds, const struct timespec *timeout_ts,
-  const sigset_t *) {
-  int timeout =
-  (timeout_ts == nullptr)
-  ? -1
-  : (timeout_ts->tv_sec * 1000 + timeout_ts->tv_nsec / 100);
-  return POLL(fds, nfds, 

[Lldb-commits] [PATCH] D131328: [lldb] Support fetching symbols with dsymForUUID in the background

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

There was another approach I considered initially which might alleviate some of 
the issues Pavel raised. Instead of using the global thread pool, that plan 
consisted of a dedicated thread to fetch symbols in the background. That would 
allow us to add the symbol info synchronously. For example on every stop we 
could ask if there are any new symbol files to add instead of trying to add the 
symbol info as soon as it becomes available.


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

https://reviews.llvm.org/D131328

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

[Lldb-commits] [PATCH] D131275: [lldb] Make Process and subclass constructors protected

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

This revision was automatically updated to reflect the committed changes.
Closed by commit rG9b031d5e3a7b: [lldb] Make Process and subclass constructors 
protected (authored by mgorny).
Herald added a project: LLDB.

Changed prior to commit:
  https://reviews.llvm.org/D131275?vs=450332=450822#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131275

Files:
  lldb/include/lldb/Target/PostMortemProcess.h
  lldb/include/lldb/Target/Process.h
  lldb/source/Plugins/Process/Windows/Common/ProcessWindows.h
  lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
  lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
  lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp
  lldb/source/Plugins/Process/scripted/ScriptedProcess.h
  lldb/unittests/Expression/DWARFExpressionTest.cpp
  lldb/unittests/Process/ProcessEventDataTest.cpp
  lldb/unittests/Target/ExecutionContextTest.cpp
  lldb/unittests/Thread/ThreadTest.cpp

Index: lldb/unittests/Thread/ThreadTest.cpp
===
--- lldb/unittests/Thread/ThreadTest.cpp
+++ lldb/unittests/Thread/ThreadTest.cpp
@@ -40,7 +40,8 @@
 
 class DummyProcess : public Process {
 public:
-  using Process::Process;
+  DummyProcess(lldb::TargetSP target_sp, lldb::ListenerSP listener_sp)
+  : Process(target_sp, listener_sp) {}
 
   bool CanDebug(lldb::TargetSP target, bool plugin_specified_by_name) override {
 return true;
Index: lldb/unittests/Target/ExecutionContextTest.cpp
===
--- lldb/unittests/Target/ExecutionContextTest.cpp
+++ lldb/unittests/Target/ExecutionContextTest.cpp
@@ -47,7 +47,8 @@
 
 class DummyProcess : public Process {
 public:
-  using Process::Process;
+  DummyProcess(lldb::TargetSP target_sp, lldb::ListenerSP listener_sp)
+  : Process(target_sp, listener_sp) {}
 
   bool CanDebug(lldb::TargetSP target, bool plugin_specified_by_name) override {
 return true;
Index: lldb/unittests/Process/ProcessEventDataTest.cpp
===
--- lldb/unittests/Process/ProcessEventDataTest.cpp
+++ lldb/unittests/Process/ProcessEventDataTest.cpp
@@ -42,7 +42,8 @@
 
 class DummyProcess : public Process {
 public:
-  using Process::Process;
+  DummyProcess(lldb::TargetSP target_sp, lldb::ListenerSP listener_sp)
+  : Process(target_sp, listener_sp) {}
 
   bool CanDebug(lldb::TargetSP target, bool plugin_specified_by_name) override {
 return true;
Index: lldb/unittests/Expression/DWARFExpressionTest.cpp
===
--- lldb/unittests/Expression/DWARFExpressionTest.cpp
+++ lldb/unittests/Expression/DWARFExpressionTest.cpp
@@ -328,7 +328,9 @@
   EXPECT_THAT_EXPECTED(Evaluate({DW_OP_lit0, DW_OP_deref}), llvm::Failed());
 
   struct MockProcess : Process {
-using Process::Process;
+MockProcess(lldb::TargetSP target_sp, lldb::ListenerSP listener_sp)
+: Process(target_sp, listener_sp) {}
+
 llvm::StringRef GetPluginName() override { return "mock process"; }
 bool CanDebug(lldb::TargetSP target,
   bool plugin_specified_by_name) override {
Index: lldb/source/Plugins/Process/scripted/ScriptedProcess.h
===
--- lldb/source/Plugins/Process/scripted/ScriptedProcess.h
+++ lldb/source/Plugins/Process/scripted/ScriptedProcess.h
@@ -50,10 +50,6 @@
 
   static llvm::StringRef GetPluginDescriptionStatic();
 
-  ScriptedProcess(lldb::TargetSP target_sp, lldb::ListenerSP listener_sp,
-  const ScriptedProcess::ScriptedProcessInfo _info,
-  Status );
-
   ~ScriptedProcess() override;
 
   bool CanDebug(lldb::TargetSP target_sp,
@@ -93,6 +89,10 @@
   GetLoadedDynamicLibrariesInfos() override;
 
 protected:
+  ScriptedProcess(lldb::TargetSP target_sp, lldb::ListenerSP listener_sp,
+  const ScriptedProcess::ScriptedProcessInfo _info,
+  Status );
+
   Status DoStop();
 
   void Clear();
Index: lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp
===
--- lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp
+++ lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp
@@ -62,8 +62,8 @@
   ScriptedProcess::ScriptedProcessInfo scripted_process_info(
   target_sp->GetProcessLaunchInfo());
 
-  auto process_sp = std::make_shared(
-  target_sp, listener_sp, scripted_process_info, error);
+  auto process_sp = std::shared_ptr(new ScriptedProcess(
+  target_sp, listener_sp, scripted_process_info, error));
 
   if (error.Fail() || !process_sp || !process_sp->m_script_object_sp ||
   !process_sp->m_script_object_sp->IsValid()) {
Index: lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h

[Lldb-commits] [lldb] 9b031d5 - [lldb] Make Process and subclass constructors protected

[Michał Górny via lldb-commits]

Author: Michał Górny
Date: 2022-08-08T17:34:27+02:00
New Revision: 9b031d5e3a7b455308257a71116a603e76c8c679

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

LOG: [lldb] Make Process and subclass constructors protected

Make constructors of the Process and its subclasses class protected,
to prevent accidentally constructing Process on stack when it could be
afterwards accessed via a shared_ptr (since it uses
std::enable_shared_from_this<>).

The only place where a stack allocation was used were unittests,
and fixing them via declaring an explicit public constructor
in the respective mock classes is trivial.

Sponsored by: The FreeBSD Foundation
Differential Revision: https://reviews.llvm.org/D131275

Added: 


Modified: 
lldb/include/lldb/Target/PostMortemProcess.h
lldb/include/lldb/Target/Process.h
lldb/source/Plugins/Process/Windows/Common/ProcessWindows.h
lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp
lldb/source/Plugins/Process/scripted/ScriptedProcess.h
lldb/unittests/Expression/DWARFExpressionTest.cpp
lldb/unittests/Process/ProcessEventDataTest.cpp
lldb/unittests/Target/ExecutionContextTest.cpp
lldb/unittests/Thread/ThreadTest.cpp

Removed: 




diff  --git a/lldb/include/lldb/Target/PostMortemProcess.h 
b/lldb/include/lldb/Target/PostMortemProcess.h
index 353bfc0919eec..7207fc99ef29a 100644
--- a/lldb/include/lldb/Target/PostMortemProcess.h
+++ b/lldb/include/lldb/Target/PostMortemProcess.h
@@ -21,9 +21,9 @@ namespace lldb_private {
 /// between these kinds of processes can have default implementations in this
 /// class.
 class PostMortemProcess : public Process {
-public:
   using Process::Process;
 
+public:
   bool IsLiveDebugSession() const override { return false; }
 };
 

diff  --git a/lldb/include/lldb/Target/Process.h 
b/lldb/include/lldb/Target/Process.h
index 505e211e09b63..05b0eb6237c71 100644
--- a/lldb/include/lldb/Target/Process.h
+++ b/lldb/include/lldb/Target/Process.h
@@ -475,15 +475,6 @@ class Process : public 
std::enable_shared_from_this,
 const ProcessEventData =(const ProcessEventData &) = delete;
   };
 
-  /// Construct with a shared pointer to a target, and the Process listener.
-  /// Uses the Host UnixSignalsSP by default.
-  Process(lldb::TargetSP target_sp, lldb::ListenerSP listener_sp);
-
-  /// Construct with a shared pointer to a target, the Process listener, and
-  /// the appropriate UnixSignalsSP for the process.
-  Process(lldb::TargetSP target_sp, lldb::ListenerSP listener_sp,
-  const lldb::UnixSignalsSP _signals_sp);
-
   /// Destructor.
   ///
   /// The destructor is virtual since this class is designed to be inherited
@@ -2499,6 +2490,16 @@ void PruneThreadPlans();
 
 protected:
   friend class Trace;
+
+  /// Construct with a shared pointer to a target, and the Process listener.
+  /// Uses the Host UnixSignalsSP by default.
+  Process(lldb::TargetSP target_sp, lldb::ListenerSP listener_sp);
+
+  /// Construct with a shared pointer to a target, the Process listener, and
+  /// the appropriate UnixSignalsSP for the process.
+  Process(lldb::TargetSP target_sp, lldb::ListenerSP listener_sp,
+  const lldb::UnixSignalsSP _signals_sp);
+
   ///  Get the processor tracing type supported for this process.
   ///  Responses might be 
diff erent depending on the architecture and
   ///  capabilities of the underlying OS.

diff  --git a/lldb/source/Plugins/Process/Windows/Common/ProcessWindows.h 
b/lldb/source/Plugins/Process/Windows/Common/ProcessWindows.h
index 043fd0f0f5a54..bf9e5fe7a633c 100644
--- a/lldb/source/Plugins/Process/Windows/Common/ProcessWindows.h
+++ b/lldb/source/Plugins/Process/Windows/Common/ProcessWindows.h
@@ -36,9 +36,6 @@ class ProcessWindows : public Process, public ProcessDebugger 
{
 
   static llvm::StringRef GetPluginDescriptionStatic();
 
-  // Constructors and destructors
-  ProcessWindows(lldb::TargetSP target_sp, lldb::ListenerSP listener_sp);
-
   ~ProcessWindows();
 
   size_t GetSTDOUT(char *buf, size_t buf_size, Status ) override;
@@ -104,6 +101,8 @@ class ProcessWindows : public Process, public 
ProcessDebugger {
   Status DisableWatchpoint(Watchpoint *wp, bool notify = true) override;
 
 protected:
+  ProcessWindows(lldb::TargetSP target_sp, lldb::ListenerSP listener_sp);
+
   Status DoGetMemoryRegionInfo(lldb::addr_t vm_addr,
MemoryRegionInfo ) override;
 

diff  --git a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp 
b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
index 233a665f65904..30fb27932d192 100644
--- 

[Lldb-commits] [PATCH] D131328: [lldb] Support fetching symbols with dsymForUUID in the background

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D131328#3706565 , @JDevlieghere 
wrote:

> In D131328#3706202 , @labath wrote:
>
>> I see two problems with this patch.
>>
>> - it makes shutting down lldb safely impossible (because you never know if 
>> there are any unfinished tasks running). We already have problems with 
>> shutting down (grep for "intentionally leaked"), but this is guaranteed to 
>> make the problem worse. Using the global thread pool would make things 
>> better, as then one could flush the pool in SBDebugger::Terminate, or 
>> something similar
>
> It *is* using the global thread pool, but it doesn't look like that's being 
> flushed. I can rework that in a separate patch to make it fit the 
> Initialize/Terminate pattern.

Ok, I'm sorry. I got confused by the `static ThreadPoolTaskGroup` thingy. In 
either case, I think we should flush that pool during termination. The reason 
we didn't need that before is because this is the first time that we have these 
sorts of fully async tasks (the other task pool tasks are "flushing" 
themselves).

>> - there seems to be a race between new debuggers/targets being added, and 
>> their existing instances being notified. I'm not 100% sure what will happen 
>> in this case, but it seems very easy to miss some debuggers (or targets) 
>> that were added after we took the "snapshot" of the current state
>
> As I (tried to) explain in the comment, since we set the symbol file in the 
> module before taking the "snapshot", I don't think this is a problem. If a 
> target was created after, it should already know about the symbol file. Did I 
> miss something with that approach?

Right. I missed that part. I think this should be mostly fine. I mean, I 
suppose this can still race in the other "direction", where a target picks up 
the new symbol file, but we still end up calling SymbolsDidLoad manually. But 
hopefully that is ok (?)

>> (There's also the (obvious) problem of any breakpoints that are set on code 
>> within those modules becoming effective at an unpredictable future data, but 
>> I am assuming you are aware of that.)
>
> Yeah, but I don't see any way around that. The unpredictability is 
> unfortunate, but from UX perspective it's not much worse than modules getting 
> loaded at runtime (although hopefully that's more deterministic).

Yeah, if that's the thing you want, then that's the thing you want. One could 
play some UX games, and add a mechanism to show the downloads that are in 
progress and/or force the completion of loading of some symbols of whatever, 
but there's no way to get rid of that completely.


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

https://reviews.llvm.org/D131328

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

[Lldb-commits] [PATCH] D131328: [lldb] Support fetching symbols with dsymForUUID in the background

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

In D131328#3706202 , @labath wrote:

> I see two problems with this patch.
>
> - it makes shutting down lldb safely impossible (because you never know if 
> there are any unfinished tasks running). We already have problems with 
> shutting down (grep for "intentionally leaked"), but this is guaranteed to 
> make the problem worse. Using the global thread pool would make things 
> better, as then one could flush the pool in SBDebugger::Terminate, or 
> something similar

It *is* using the global thread pool, but it doesn't look like that's being 
flushed. I can rework that in a separate patch to make it fit the 
Initialize/Terminate pattern.

> - there seems to be a race between new debuggers/targets being added, and 
> their existing instances being notified. I'm not 100% sure what will happen 
> in this case, but it seems very easy to miss some debuggers (or targets) that 
> were added after we took the "snapshot" of the current state

As I (tried to) explain in the comment, since we set the symbol file in the 
module before taking the "snapshot", I don't think this is a problem. If a 
target was created after, it should already know about the symbol file. Did I 
miss something with that approach?

> (There's also the (obvious) problem of any breakpoints that are set on code 
> within those modules becoming effective at an unpredictable future data, but 
> I am assuming you are aware of that.)

Yeah, but I don't see any way around that. The unpredictability is unfortunate, 
but from UX perspective it's not much worse than modules getting loaded at 
runtime (although hopefully that's more deterministic).


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

https://reviews.llvm.org/D131328

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

[Lldb-commits] [PATCH] D130689: [LLVM] Update C++ standard to 17

[Tobias Hieta via Phabricator via lldb-commits]

thieta added a comment.

In D130689#3706424 , @aaron.ballman 
wrote:

> +1, thank you for thinking about how we can improve this process in the 
> future! Given that C++17 adoption across compilers has been far better than 
> C++20, I suspect the next time we bump the language version will be even more 
> of a challenge with these sort of weird issues.

I'll happily ignore that for another 3 years 

> Yeah, this is rather deeply concerning to be honest. I triple-checked and the 
> only changes in my tree were yours to compiler-rt and when I looked at the 
> configure logs, they clearly showed `compiler-rt project is disabled` in the 
> logs. It is really weird that a change to compiler-rt's cmake would have any 
> impact on Clang's ability to build when compiler-rt is disabled. I think 
> that's worth some investigative work.

Hmmm - something is really funny, with my cache nuking change it seems like 
this bot is back to working again: 
https://lab.llvm.org/buildbot/#/builders/123/builds/12164

> It seems like we might need that, but it also seems like it would be good to 
> understand why compiler-rt is impacting the rest of Clang when it's disabled. 
> That said, the goal is to get the build lab back to green as quickly as 
> possible, so I think it may make sense to land sooner rather than later.

I'll hold off a bit on this - it seems like my commit above have fixed at least 
some of the issues. There are still some msvc bots failing - but they seem to 
be failing because they have a too old version of MSVC.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130689

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

[Lldb-commits] [PATCH] D130689: [LLVM] Update C++ standard to 17

[Aaron Ballman via Phabricator via lldb-commits]

aaron.ballman added a comment.

In D130689#3706377 , @thieta wrote:

> In D130689#3706336 , @aaron.ballman 
> wrote:
>
>> That's the only reason this hasn't been reverted already. Landing sweeping 
>> changes on a weekend is a good way to reduce the pain, but we really need to 
>> be sure someone watches the build lab and reacts when subsequent changes 
>> break everything like this.
>
> Agreed, I think we need to update the protocol for changing the C++ standard 
> in the future to account for more testing beforehand. I might push some 
> changes to the policy document when all this has settled down to see if we 
> can make sure it will be smoother the time we move to C++20. It's unfortunate 
> that some stuff broke considering we where running some bots before it was 
> merged and it didn't show any errors. And local windows builds for me have 
> been clean as well.

+1, thank you for thinking about how we can improve this process in the future! 
Given that C++17 adoption across compilers has been far better than C++20, I 
suspect the next time we bump the language version will be even more of a 
challenge with these sort of weird issues.

>>> Can you try to locally rebuild with this patch 
>>> https://reviews.llvm.org/D131382 ?
>>
>> That improves things but the build still isn't clean:
>> ...
>> (FWIW, I don't know if any of the Windows builders in the lab are building 
>> with /WX)
>
> I am not sure either - but at least this removed the problem with the 
> std=c++17 not being passed around correctly.

Agreed, I can look into fixing up those warnings when I have a spare moment if 
nobody else gets to them first.

>>> I think all the runtime errors is because of that one above - basically we 
>>> don't set std=c++17 for any of the compiler-rt projects.
>>
>> I wasn't building compiler-rt, so no idea why this improved things for me. 
>> FWIW, he's my CMake config: `-DCMAKE_EXPORT_COMPILE_COMMANDS=ON 
>> -DLLVM_TARGETS_TO_BUILD="X86" -DLLVM_ENABLE_IDE=ON 
>> -DLLVM_ENABLE_PROJECTS="clang;clang-tools-extra" 
>> -DLLVM_PARALLEL_COMPILE_JOBS=112 -DLLVM_PARALLEL_LINK_JOBS=16`
>
> That is very odd. I would suspect that the problem was the compiler-rt not 
> getting the right flag. But it seems to influence something else as well?

Yeah, this is rather deeply concerning to be honest. I triple-checked and the 
only changes in my tree were yours to compiler-rt and when I looked at the 
configure logs, they clearly showed `compiler-rt project is disabled` in the 
logs. It is really weird that a change to compiler-rt's cmake would have any 
impact on Clang's ability to build when compiler-rt is disabled. I think that's 
worth some investigative work.

>>> I also think we should merge https://reviews.llvm.org/D131367 for now - we 
>>> can revert that later on if we think it adds to much complexity, since it 
>>> will delete the bad cache values automatcially.
>>
>> Seems reasonable to me.
>
> I landed this now - hopefully that fixes some of the issues seen in the bots 
> - but we might need https://reviews.llvm.org/D131382 as well before they go 
> green. I am not sure what the protocol is here, since @MaskRay suggested the 
> change maybe we should wait for him to land it, or should we get it in ASAP 
> to see if that fixes the bots?

It seems like we might need that, but it also seems like it would be good to 
understand why compiler-rt is impacting the rest of Clang when it's disabled. 
That said, the goal is to get the build lab back to green as quickly as 
possible, so I think it may make sense to land sooner rather than later.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130689

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

[Lldb-commits] [PATCH] D131275: [lldb] Make Process and subclass constructors protected

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

mgorny added inline comments.



Comment at: lldb/include/lldb/Target/Process.h:478-487
+protected:
   /// Construct with a shared pointer to a target, and the Process listener.
   /// Uses the Host UnixSignalsSP by default.
   Process(lldb::TargetSP target_sp, lldb::ListenerSP listener_sp);
 
   /// Construct with a shared pointer to a target, the Process listener, and
   /// the appropriate UnixSignalsSP for the process.

labath wrote:
> Just move this to the existing protected section on line 2500 (boy is this 
> class big). Too many sections makes it hard to find what's the visibility of 
> something.
Will do. Wasn't sure whether this was desirable or not.



Comment at: lldb/unittests/Expression/DWARFExpressionTest.cpp:331
   struct MockProcess : Process {
-using Process::Process;
+MockProcess(lldb::TargetSP target_sp, lldb::ListenerSP listener_sp)
+: Process(target_sp, listener_sp) {}

labath wrote:
> huh, I did not expect this to be necessary. Inherited constructors are 
> weird...
Yes, they are. I've searched a bit and couldn't find a way to make `using` 
change their visibility.


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

https://reviews.llvm.org/D131275

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

[Lldb-commits] [PATCH] D130689: [LLVM] Update C++ standard to 17

[Tobias Hieta via Phabricator via lldb-commits]

thieta added a comment.

In D130689#3706336 , @aaron.ballman 
wrote:

> That's the only reason this hasn't been reverted already. Landing sweeping 
> changes on a weekend is a good way to reduce the pain, but we really need to 
> be sure someone watches the build lab and reacts when subsequent changes 
> break everything like this.

Agreed, I think we need to update the protocol for changing the C++ standard in 
the future to account for more testing beforehand. I might push some changes to 
the policy document when all this has settled down to see if we can make sure 
it will be smoother the time we move to C++20. It's unfortunate that some stuff 
broke considering we where running some bots before it was merged and it didn't 
show any errors. And local windows builds for me have been clean as well.

>> Can you try to locally rebuild with this patch 
>> https://reviews.llvm.org/D131382 ?
>
> That improves things but the build still isn't clean:
> ...
> (FWIW, I don't know if any of the Windows builders in the lab are building 
> with /WX)

I am not sure either - but at least this removed the problem with the std=c++17 
not being passed around correctly.

>> I think all the runtime errors is because of that one above - basically we 
>> don't set std=c++17 for any of the compiler-rt projects.
>
> I wasn't building compiler-rt, so no idea why this improved things for me. 
> FWIW, he's my CMake config: `-DCMAKE_EXPORT_COMPILE_COMMANDS=ON 
> -DLLVM_TARGETS_TO_BUILD="X86" -DLLVM_ENABLE_IDE=ON 
> -DLLVM_ENABLE_PROJECTS="clang;clang-tools-extra" 
> -DLLVM_PARALLEL_COMPILE_JOBS=112 -DLLVM_PARALLEL_LINK_JOBS=16`

That is very odd. I would suspect that the problem was the compiler-rt not 
getting the right flag. But it seems to influence something else as well?

>> I also think we should merge https://reviews.llvm.org/D131367 for now - we 
>> can revert that later on if we think it adds to much complexity, since it 
>> will delete the bad cache values automatcially.
>
> Seems reasonable to me.

I landed this now - hopefully that fixes some of the issues seen in the bots - 
but we might need https://reviews.llvm.org/D131382 as well before they go 
green. I am not sure what the protocol is here, since @MaskRay suggested the 
change maybe we should wait for him to land it, or should we get it in ASAP to 
see if that fixes the bots?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130689

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

[Lldb-commits] [PATCH] D131275: [lldb] Make Process and subclass constructors protected

[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/include/lldb/Target/Process.h:478-487
+protected:
   /// Construct with a shared pointer to a target, and the Process listener.
   /// Uses the Host UnixSignalsSP by default.
   Process(lldb::TargetSP target_sp, lldb::ListenerSP listener_sp);
 
   /// Construct with a shared pointer to a target, the Process listener, and
   /// the appropriate UnixSignalsSP for the process.

Just move this to the existing protected section on line 2500 (boy is this 
class big). Too many sections makes it hard to find what's the visibility of 
something.



Comment at: lldb/source/Plugins/Process/Windows/Common/ProcessWindows.h:41
+protected:
   ProcessWindows(lldb::TargetSP target_sp, lldb::ListenerSP listener_sp);
 

same here



Comment at: lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h:52
+protected:
   ProcessGDBRemote(lldb::TargetSP target_sp, lldb::ListenerSP listener_sp);
 

and here



Comment at: lldb/source/Plugins/Process/scripted/ScriptedProcess.h:54
+protected:
   ScriptedProcess(lldb::TargetSP target_sp, lldb::ListenerSP listener_sp,
   const ScriptedProcess::ScriptedProcessInfo _info,

and here



Comment at: lldb/unittests/Expression/DWARFExpressionTest.cpp:331
   struct MockProcess : Process {
-using Process::Process;
+MockProcess(lldb::TargetSP target_sp, lldb::ListenerSP listener_sp)
+: Process(target_sp, listener_sp) {}

huh, I did not expect this to be necessary. Inherited constructors are weird...


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

https://reviews.llvm.org/D131275

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

[Lldb-commits] [PATCH] D131312: [LLDB][NFC] Fix suspicious bitwise expression in PrintBTEntry()

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added inline comments.



Comment at: lldb/tools/intel-features/intel-mpx/cli-wrapper-mpxtable.cpp:66
 
-  if ((lbound == one_cmpl64 || one_cmpl32) && ubound == 0) {
 result.Printf("Null bounds on map: pointer value = 0x%" PRIu64 "\n", 
value);

DavidSpickett wrote:
> hawkinsw wrote:
> > DavidSpickett wrote:
> > > hawkinsw wrote:
> > > > According to 
> > > > https://en.cppreference.com/w/cpp/language/operator_precedence, I would 
> > > > read the left operand of the `&&` as 
> > > > 
> > > > 1. The `==` has higher precedence than `||` so, `b = (lbound == 
> > > > one_compl64)`
> > > > 2. `b || one_cmpl32`
> > > > 
> > > > which does not seem like what the original author intended. I 
> > > > absolutely think that the fix is correct, but I just wanted to get 
> > > > everyone's feedback on whether this seems like more than just a 
> > > > "suspicious bitwise expression" (and more like a "mistaken bitwise 
> > > > expression").
> > > > 
> > > > All that said, I could be completely, 100% wrong. And, if I am, feel 
> > > > free to ignore me!
> > > The corrected code also makes sense given that MPX is some kind of memory 
> > > protection across ranges.
> > > 
> > > If `((lbound == one_cmpl64 || lbound == one_cmpl32) && ubound == 0)` is 
> > > true then upper bound < lower bound making an invalid range. Which is 
> > > what I'd expect for some default/uninitialised state (especially if zero 
> > > size ranges are allowed, so upper == 0 and lower == 0 couldn't be used).
> > @DavidSpickett I think that you and I are saying the same thing, right? We 
> > are both saying that the corrected code looks much "better" than the 
> > original?
> > 
> > Will
> > We are both saying that the corrected code looks much "better" than the 
> > original?
> 
> Yes.
> 
> > whether this seems like more than just a "suspicious bitwise expression" 
> > (and more like a "mistaken bitwise expression").
> 
> Definitely a mistake that needs correcting.
(I think "suspicious bitwise expression" is what the static analyser would call 
it just because it can't be 100% sure it is wrong)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131312

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

[Lldb-commits] [PATCH] D131312: [LLDB][NFC] Fix suspicious bitwise expression in PrintBTEntry()

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added inline comments.



Comment at: lldb/tools/intel-features/intel-mpx/cli-wrapper-mpxtable.cpp:66
 
-  if ((lbound == one_cmpl64 || one_cmpl32) && ubound == 0) {
 result.Printf("Null bounds on map: pointer value = 0x%" PRIu64 "\n", 
value);

hawkinsw wrote:
> DavidSpickett wrote:
> > hawkinsw wrote:
> > > According to 
> > > https://en.cppreference.com/w/cpp/language/operator_precedence, I would 
> > > read the left operand of the `&&` as 
> > > 
> > > 1. The `==` has higher precedence than `||` so, `b = (lbound == 
> > > one_compl64)`
> > > 2. `b || one_cmpl32`
> > > 
> > > which does not seem like what the original author intended. I absolutely 
> > > think that the fix is correct, but I just wanted to get everyone's 
> > > feedback on whether this seems like more than just a "suspicious bitwise 
> > > expression" (and more like a "mistaken bitwise expression").
> > > 
> > > All that said, I could be completely, 100% wrong. And, if I am, feel free 
> > > to ignore me!
> > The corrected code also makes sense given that MPX is some kind of memory 
> > protection across ranges.
> > 
> > If `((lbound == one_cmpl64 || lbound == one_cmpl32) && ubound == 0)` is 
> > true then upper bound < lower bound making an invalid range. Which is what 
> > I'd expect for some default/uninitialised state (especially if zero size 
> > ranges are allowed, so upper == 0 and lower == 0 couldn't be used).
> @DavidSpickett I think that you and I are saying the same thing, right? We 
> are both saying that the corrected code looks much "better" than the original?
> 
> Will
> We are both saying that the corrected code looks much "better" than the 
> original?

Yes.

> whether this seems like more than just a "suspicious bitwise expression" (and 
> more like a "mistaken bitwise expression").

Definitely a mistake that needs correcting.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131312

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

[Lldb-commits] [PATCH] D130689: [LLVM] Update C++ standard to 17

[Aaron Ballman via Phabricator via lldb-commits]

aaron.ballman added a comment.

In D130689#3706303 , @thieta wrote:

> In D130689#3706263 , @aaron.ballman 
> wrote:
>
>> 
>
>
>
>> Something odd is going on here and we might want to consider a revert of 
>> this patch until we resolve it. When I do a git pull and cmake files change, 
>> Visual Studio's built-in CMake support automatically re-runs the configure 
>> step. This should be all that's necessary to switch the toolchain, but it 
>> isn't for some reason (today's build is also failing for me with C++17 
>> related issues after I did another pulldown this morning). I deleted my 
>> cache explicitly and regenerated CMake from scratch and am still getting the 
>> same build errors. The failures I am getting are the same as what's shown by 
>> the sanitizer bot for Windows: 
>> https://lab.llvm.org/buildbot/#/builders/127/builds/33980/steps/4/logs/stdio 
>> (I'm using VS 2019 16.11.17 FWIW).
>>
>> I hope we can resolve this quickly as basically no MSVC builds are green 
>> right now in the build lab.
>
> While we can revert this one - we also need to revert all changes that add 
> C++17 features at this point as well. That will be a lot of churn. Let's see 
> if we can figure out what's wrong first.

That's the only reason this hasn't been reverted already. Landing sweeping 
changes on a weekend is a good way to reduce the pain, but we really need to be 
sure someone watches the build lab and reacts when subsequent changes break 
everything like this.

> Can you try to locally rebuild with this patch 
> https://reviews.llvm.org/D131382 ?

That improves things but the build still isn't clean:

  Severity  CodeDescription Project FileLineSuppression 
State
  Warning   C4996   
'std::codecvt_utf8': warning STL4017: 
std::wbuffer_convert, std::wstring_convert, and the  header 
(containing std::codecvt_mode, std::codecvt_utf8, std::codecvt_utf16, and 
std::codecvt_utf8_utf16) are deprecated in C++17. (The std::codecvt class 
template is NOT deprecated.) The C++ Standard doesn't provide equivalent 
non-deprecated functionality; consider using MultiByteToWideChar() and 
WideCharToMultiByte() from  instead. You can define 
_SILENCE_CXX17_CODECVT_HEADER_DEPRECATION_WARNING or 
_SILENCE_ALL_CXX17_DEPRECATION_WARNINGS to acknowledge that you have received 
this warning.F:\source\llvm-project\llvm\out\build\x64-Debug\llvm
F:\source\llvm-project\third-party\benchmark\src\sysinfo.cc 429 
  Warning   C4996   
'std::wstring_convert,std::allocator>':
 warning STL4017: std::wbuffer_convert, std::wstring_convert, and the  
header (containing std::codecvt_mode, std::codecvt_utf8, std::codecvt_utf16, 
and std::codecvt_utf8_utf16) are deprecated in C++17. (The std::codecvt class 
template is NOT deprecated.) The C++ Standard doesn't provide equivalent 
non-deprecated functionality; consider using MultiByteToWideChar() and 
WideCharToMultiByte() from  instead. You can define 
_SILENCE_CXX17_CODECVT_HEADER_DEPRECATION_WARNING or 
_SILENCE_ALL_CXX17_DEPRECATION_WARNINGS to acknowledge that you have received 
this warning.F:\source\llvm-project\llvm\out\build\x64-Debug\llvm
F:\source\llvm-project\third-party\benchmark\src\sysinfo.cc 430 
  Warning   C4996   
'std::wstring_convert,std::allocator>::wstring_convert':
 warning STL4017: std::wbuffer_convert, std::wstring_convert, and the  
header (containing std::codecvt_mode, std::codecvt_utf8, std::codecvt_utf16, 
and std::codecvt_utf8_utf16) are deprecated in C++17. (The std::codecvt class 
template is NOT deprecated.) The C++ Standard doesn't provide equivalent 
non-deprecated functionality; consider using MultiByteToWideChar() and 
WideCharToMultiByte() from  instead. You can define 
_SILENCE_CXX17_CODECVT_HEADER_DEPRECATION_WARNING or 
_SILENCE_ALL_CXX17_DEPRECATION_WARNINGS to acknowledge that you have received 
this warning.   F:\source\llvm-project\llvm\out\build\x64-Debug\llvm
F:\source\llvm-project\third-party\benchmark\src\sysinfo.cc 430 
  Warning   C4996   
'std::wstring_convert,std::allocator>::to_bytes':
 warning STL4017: std::wbuffer_convert, std::wstring_convert, and the  
header (containing std::codecvt_mode, std::codecvt_utf8, std::codecvt_utf16, 
and std::codecvt_utf8_utf16) are deprecated in C++17. (The std::codecvt class 
template is NOT deprecated.) The C++ Standard doesn't provide equivalent 
non-deprecated functionality; consider using MultiByteToWideChar() and 
WideCharToMultiByte() from  instead. You can define 
_SILENCE_CXX17_CODECVT_HEADER_DEPRECATION_WARNING or 
_SILENCE_ALL_CXX17_DEPRECATION_WARNINGS to acknowledge that you have received 
this warning.  F:\source\llvm-project\llvm\out\build\x64-Debug\llvm
F:\source\llvm-project\third-party\benchmark\src\sysinfo.cc 432 
  Warning   C4996   'std::iterator': warning STL4015: The std::iterator class 
template (used as a base 

[Lldb-commits] [PATCH] D130689: [LLVM] Update C++ standard to 17

[James Henderson via Phabricator via lldb-commits]

jhenderson added a comment.

FWIW, I've got a Visual Studio configuration (admittedly using a direct Visual 
Studio generator, not ninja), and I don't have any issues - C++17 is being 
used. However, I currently only have LLD as an additional project enabled, and 
don't build compiler-rt.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130689

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

[Lldb-commits] [PATCH] D131160: [WIP][lldb] Add "event" capability to the MainLoop class

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D131160#3701839 , @mgorny wrote:

> To be honest, I'm not sure if "event" is the best name for it. I get it's a 
> WSA name but it's a bit non-obvious to outsiders (I mean, technically 
> everything is an event). Not that I have a better name in mind.

Yeah, I know... I checked the thesaurus for "event", and I didn't find anything 
reasonable. And "signal" is obviously a bad idea..

Another possibility (one that just occurred to me right now) would be to use a 
completely different paradigm to achieve the effect. For example, we could 
extend the `PendingCallback` mechanism to enable adding callbacks from 
different threads (and have them wake the loop if necessary). That would be a 
more generic mechanism, and it would avoid the need to introduce a new concept 
(and having to name it).

In D131160#3701907 , @mgorny wrote:

> BTW do you need me to implement the POSIX counterpart to this?

That would be appreciated, but I think we first need to figure out the API, and 
how this code is going to be structured (I'm going to try handling the second 
part now)...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131160

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

[Lldb-commits] [PATCH] D131312: [LLDB][NFC] Fix suspicious bitwise expression in PrintBTEntry()

[Will Hawkins via Phabricator via lldb-commits]

hawkinsw added inline comments.



Comment at: lldb/tools/intel-features/intel-mpx/cli-wrapper-mpxtable.cpp:66
 
-  if ((lbound == one_cmpl64 || one_cmpl32) && ubound == 0) {
 result.Printf("Null bounds on map: pointer value = 0x%" PRIu64 "\n", 
value);

DavidSpickett wrote:
> hawkinsw wrote:
> > According to 
> > https://en.cppreference.com/w/cpp/language/operator_precedence, I would 
> > read the left operand of the `&&` as 
> > 
> > 1. The `==` has higher precedence than `||` so, `b = (lbound == 
> > one_compl64)`
> > 2. `b || one_cmpl32`
> > 
> > which does not seem like what the original author intended. I absolutely 
> > think that the fix is correct, but I just wanted to get everyone's feedback 
> > on whether this seems like more than just a "suspicious bitwise expression" 
> > (and more like a "mistaken bitwise expression").
> > 
> > All that said, I could be completely, 100% wrong. And, if I am, feel free 
> > to ignore me!
> The corrected code also makes sense given that MPX is some kind of memory 
> protection across ranges.
> 
> If `((lbound == one_cmpl64 || lbound == one_cmpl32) && ubound == 0)` is true 
> then upper bound < lower bound making an invalid range. Which is what I'd 
> expect for some default/uninitialised state (especially if zero size ranges 
> are allowed, so upper == 0 and lower == 0 couldn't be used).
@DavidSpickett I think that you and I are saying the same thing, right? We are 
both saying that the corrected code looks much "better" than the original?

Will


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131312

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

[Lldb-commits] [PATCH] D130689: [LLVM] Update C++ standard to 17

[Tobias Hieta via Phabricator via lldb-commits]

thieta added a comment.

In D130689#3706263 , @aaron.ballman 
wrote:

> 



> Something odd is going on here and we might want to consider a revert of this 
> patch until we resolve it. When I do a git pull and cmake files change, 
> Visual Studio's built-in CMake support automatically re-runs the configure 
> step. This should be all that's necessary to switch the toolchain, but it 
> isn't for some reason (today's build is also failing for me with C++17 
> related issues after I did another pulldown this morning). I deleted my cache 
> explicitly and regenerated CMake from scratch and am still getting the same 
> build errors. The failures I am getting are the same as what's shown by the 
> sanitizer bot for Windows: 
> https://lab.llvm.org/buildbot/#/builders/127/builds/33980/steps/4/logs/stdio 
> (I'm using VS 2019 16.11.17 FWIW).
>
> I hope we can resolve this quickly as basically no MSVC builds are green 
> right now in the build lab.

While we can revert this one - we also need to revert all changes that add 
C++17 features at this point as well. That will be a lot of churn. Let's see if 
we can figure out what's wrong first.

Can you try to locally rebuild with this patch https://reviews.llvm.org/D131382 
?

I think all the runtime errors is because of that one above - basically we 
don't set std=c++17 for any of the compiler-rt projects.

I also think we should merge https://reviews.llvm.org/D131367 for now - we can 
revert that later on if we think it adds to much complexity, since it will 
delete the bad cache values automatcially.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130689

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

[Lldb-commits] [PATCH] D130689: [LLVM] Update C++ standard to 17

[Aaron Ballman via Phabricator via lldb-commits]

aaron.ballman added a comment.

In D130689#3706276 , @barannikov88 
wrote:

> In D130689#3706263 , @aaron.ballman 
> wrote:
>
>> The failures I am getting are the same as what's shown by the sanitizer bot 
>> for Windows: 
>> https://lab.llvm.org/buildbot/#/builders/127/builds/33980/steps/4/logs/stdio 
>> (I'm using VS 2019 16.11.17 FWIW).
>
> The log says:
>
>> The contents of  are available only with C++17 or later.
>
> if that helps.

Yes, and in my build logs, even after an explicit reconfigure where I deleted 
the cache, I'm seeing `-std:c++14` being passed to cl.exe; so something about 
these changes is not working properly with Visual Studio.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130689

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

[Lldb-commits] [PATCH] D130689: [LLVM] Update C++ standard to 17

[Sergei Barannikov via Phabricator via lldb-commits]

barannikov88 added a comment.

In D130689#3706263 , @aaron.ballman 
wrote:

> The failures I am getting are the same as what's shown by the sanitizer bot 
> for Windows: 
> https://lab.llvm.org/buildbot/#/builders/127/builds/33980/steps/4/logs/stdio 
> (I'm using VS 2019 16.11.17 FWIW).

The log says:

> The contents of  are available only with C++17 or later.

if that helps.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130689

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

[Lldb-commits] [PATCH] D130689: [LLVM] Update C++ standard to 17

[Aaron Ballman via Phabricator via lldb-commits]

aaron.ballman added a comment.

In D130689#3706200 , @thieta wrote:

> In D130689#3706199 , @cor3ntin 
> wrote:
>
>> Trying to read the logs,, notably 
>> `C:\PROGRA~2\MIB055~1\2019\PROFES~1\VC\Tools\MSVC\1429~1.301\bin\Hostx64\x64\cl.exe
>>  `, it would seem that this particular bot is running a version much older 
>> than the current requirements  (Visual Studio 2019 16.7)
>> Either I'm reading that wrong or the CMake script does not check the msvc 
>> version?
>
> The compilers are definitely version checked here: 
> https://github.com/llvm/llvm-project/blob/main/llvm/cmake/modules/CheckCompilerVersion.cmake#L50
>
> But reading the cmake log it seems like it's using a cache already and it's 
> hard to say if it's using the same version of MSVC.

Something odd is going on here and we might want to consider a revert of this 
patch until we resolve it. When I do a git pull and cmake files change, Visual 
Studio's built-in CMake support automatically re-runs the configure step. This 
should be all that's necessary to switch the toolchain, but it isn't for some 
reason (today's build is also failing for me with C++17 related issues after I 
did another pulldown this morning). I deleted my cache explicitly and 
regenerated CMake from scratch and am still getting the same build errors. The 
failures I am getting are the same as what's shown by the sanitizer bot for 
Windows: 
https://lab.llvm.org/buildbot/#/builders/127/builds/33980/steps/4/logs/stdio 
(I'm using VS 2019 16.11.17 FWIW).

I hope we can resolve this quickly as basically no MSVC builds are green right 
now in the build lab.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130689

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

[Lldb-commits] [PATCH] D131159: [WIP][lldb] Use WSAEventSelect for MainLoop polling

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D131159#3699826 , @jingham wrote:

> This is a WIP, presumably in the final version there won't be prominent 
> #ifdef _WIN32 in a file in the "Host/common" directory.

Yeah, that's the main reason for the WIPness. I still have to figure out how to 
factor this (there's a fair amount of code that can still be shared between the 
two implementations, but there's also an (increasing) portion that cannot.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131159

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

[Lldb-commits] [PATCH] D131212: Allow host platform to use sdk sysroot directory option when resolving shared modules

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

> In LLDB, I assume I should do something like `platform select host -S 
> /path/to/sysroot`

I /think/ you're expected to do `platform select remote-whatever -S 
/path/to/sysroot`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131212

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

[Lldb-commits] [PATCH] D131081: [lldb] Prevent race condition when fetching /proc/cpuinfo

[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/Linux/Procfs.cpp:27-29
+  return ArrayRef(
+  reinterpret_cast(buffer.getBufferStart()),
+  reinterpret_cast(buffer.getBufferEnd()));

`return arrayRefFromStringRef(buffer.getBuffer());`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131081

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

[Lldb-commits] [PATCH] D130796: [LLDB][NativePDB] Switch to use DWARFLocationList.

[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/include/lldb/Utility/RangeMap.h:54
+base += s;
+size -= size > s ? s : size;
+  }

`std::min(s, size)`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130796

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

[Lldb-commits] [PATCH] D131328: [lldb] Support fetching symbols with dsymForUUID in the background

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I see two problems with this patch.

- it makes shutting down lldb safely impossible (because you never know if 
there are any unfinished tasks running). We already have problems with shutting 
down (grep for "intentionally leaked"), but this is guaranteed to make the 
problem worse. Using the global thread pool would make things better, as then 
one could flush the pool in SBDebugger::Terminate, or something similar
- there seems to be a race between new debuggers/targets being added, and their 
existing instances being notified. I'm not 100% sure what will happen in this 
case, but it seems very easy to miss some debuggers (or targets) that were 
added after we took the "snapshot" of the current state

(There's also the (obvious) problem of any breakpoints that are set on code 
within those modules becoming effective at an unpredictable future data, but I 
am assuming you are aware of that.)


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

https://reviews.llvm.org/D131328

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

[Lldb-commits] [PATCH] D130689: [LLVM] Update C++ standard to 17

[Tobias Hieta via Phabricator via lldb-commits]

thieta added a comment.

In D130689#3706199 , @cor3ntin wrote:

> Trying to read the logs,, notably 
> `C:\PROGRA~2\MIB055~1\2019\PROFES~1\VC\Tools\MSVC\1429~1.301\bin\Hostx64\x64\cl.exe
>  `, it would seem that this particular bot is running a version much older 
> than the current requirements  (Visual Studio 2019 16.7)
> Either I'm reading that wrong or the CMake script does not check the msvc 
> version?

The compilers are definitely version checked here: 
https://github.com/llvm/llvm-project/blob/main/llvm/cmake/modules/CheckCompilerVersion.cmake#L50

But reading the cmake log it seems like it's using a cache already and it's 
hard to say if it's using the same version of MSVC.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130689

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

[Lldb-commits] [PATCH] D130689: [LLVM] Update C++ standard to 17

[Corentin Jabot via Phabricator via lldb-commits]

cor3ntin added a comment.

In D130689#3706135 , @aaron.ballman 
wrote:

> In D130689#3705131 , @royjacobson 
> wrote:
>
>> This seems to have been more disruptive than expected, since an existing 
>> CMakeCache.txt can make LLVM compile in previous C++14 configuration. This 
>> seems to make some of the bots fail in a way that makes the patches making 
>> use of C++17 features seem at fault.
>>
>> See:
>> https://github.com/llvm/llvm-project/commit/ede96de751224487aea122af8bfb4e82bc54840b#commitcomment-80507826
>> https://reviews.llvm.org/rG32fd0b7fd5ab
>>
>> How would you feel about adding something like
>>
>>   #if defined(__cplusplus) && __cplusplus < 201703L
>>   #error "LLVM requires at least C++17"
>>   #endif
>>
>> to some central header, to make this switch more visible?
>
> FWIW, that revert was not necessarily due to cmake cache issues (though those 
> issues may exist, I haven't checked). I reverted that change because it broke 
> the build for me locally as well as caused some bots to go red. My local 
> build definitely rebuilt the cache and still failed.
>
> https://lab.llvm.org/buildbot/#/builders/127/builds/33955 (build with revert, 
> passing)
> https://lab.llvm.org/buildbot/#/builders/127/builds/33954 (previous build, 
> failing due to compile errors)

Trying to read the logs,, notably 
`C:\PROGRA~2\MIB055~1\2019\PROFES~1\VC\Tools\MSVC\1429~1.301\bin\Hostx64\x64\cl.exe
 `, it would seem that this particular bot is running a version much older than 
the current requirements  (Visual Studio 2019 16.7)
Either I'm reading that wrong or the CMake script does not check the msvc 
version?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130689

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

[Lldb-commits] [PATCH] D130689: [LLVM] Update C++ standard to 17

[Aaron Ballman via Phabricator via lldb-commits]

aaron.ballman added a comment.

In D130689#3705131 , @royjacobson 
wrote:

> This seems to have been more disruptive than expected, since an existing 
> CMakeCache.txt can make LLVM compile in previous C++14 configuration. This 
> seems to make some of the bots fail in a way that makes the patches making 
> use of C++17 features seem at fault.
>
> See:
> https://github.com/llvm/llvm-project/commit/ede96de751224487aea122af8bfb4e82bc54840b#commitcomment-80507826
> https://reviews.llvm.org/rG32fd0b7fd5ab
>
> How would you feel about adding something like
>
>   #if defined(__cplusplus) && __cplusplus < 201703L
>   #error "LLVM requires at least C++17"
>   #endif
>
> to some central header, to make this switch more visible?

FWIW, that revert was not necessarily due to cmake cache issues (though those 
issues may exist, I haven't checked). I reverted that change because it broke 
the build for me locally as well as caused some bots to go red. My local build 
definitely rebuilt the cache and still failed.

https://lab.llvm.org/buildbot/#/builders/127/builds/33955 (build with revert, 
passing)
https://lab.llvm.org/buildbot/#/builders/127/builds/33954 (previous build, 
failing due to compile errors)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130689

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

[Lldb-commits] [PATCH] D131332: [lldb][unittests] Add more test cases to CPlusPlusNameParser unit-tests

[Michael Buch via Phabricator via lldb-commits]

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

- Rebase


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131332

Files:
  lldb/unittests/Language/CPlusPlus/CPlusPlusLanguageTest.cpp


Index: lldb/unittests/Language/CPlusPlus/CPlusPlusLanguageTest.cpp
===
--- lldb/unittests/Language/CPlusPlus/CPlusPlusLanguageTest.cpp
+++ lldb/unittests/Language/CPlusPlus/CPlusPlusLanguageTest.cpp
@@ -108,7 +108,10 @@
"llvm", "isUInt<10u>", "(unsigned long)", "", "llvm::isUInt<10u>"},
   {"f, sizeof(B)()", "",
"f, sizeof(B)", "()", "",
-   "f, sizeof(B)"}};
+   "f, sizeof(B)"},
+  {"llvm::Optional::operator*() const volatile &&",
+   "llvm::Optional", "operator*", "()", "const volatile 
&&",
+   "llvm::Optional::operator*"}};
 
   for (const auto  : test_cases) {
 CPlusPlusLanguage::MethodName method(ConstString(test.input));


Index: lldb/unittests/Language/CPlusPlus/CPlusPlusLanguageTest.cpp
===
--- lldb/unittests/Language/CPlusPlus/CPlusPlusLanguageTest.cpp
+++ lldb/unittests/Language/CPlusPlus/CPlusPlusLanguageTest.cpp
@@ -108,7 +108,10 @@
"llvm", "isUInt<10u>", "(unsigned long)", "", "llvm::isUInt<10u>"},
   {"f, sizeof(B)()", "",
"f, sizeof(B)", "()", "",
-   "f, sizeof(B)"}};
+   "f, sizeof(B)"},
+  {"llvm::Optional::operator*() const volatile &&",
+   "llvm::Optional", "operator*", "()", "const volatile &&",
+   "llvm::Optional::operator*"}};
 
   for (const auto  : test_cases) {
 CPlusPlusLanguage::MethodName method(ConstString(test.input));
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D131312: [LLDB][NFC] Fix suspicious bitwise expression in PrintBTEntry()

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added a comment.

The original change was https://reviews.llvm.org/D29078. Are you able to run 
the tests with this applied?




Comment at: lldb/tools/intel-features/intel-mpx/cli-wrapper-mpxtable.cpp:66
 
-  if ((lbound == one_cmpl64 || one_cmpl32) && ubound == 0) {
 result.Printf("Null bounds on map: pointer value = 0x%" PRIu64 "\n", 
value);

hawkinsw wrote:
> According to https://en.cppreference.com/w/cpp/language/operator_precedence, 
> I would read the left operand of the `&&` as 
> 
> 1. The `==` has higher precedence than `||` so, `b = (lbound == one_compl64)`
> 2. `b || one_cmpl32`
> 
> which does not seem like what the original author intended. I absolutely 
> think that the fix is correct, but I just wanted to get everyone's feedback 
> on whether this seems like more than just a "suspicious bitwise expression" 
> (and more like a "mistaken bitwise expression").
> 
> All that said, I could be completely, 100% wrong. And, if I am, feel free to 
> ignore me!
The corrected code also makes sense given that MPX is some kind of memory 
protection across ranges.

If `((lbound == one_cmpl64 || lbound == one_cmpl32) && ubound == 0)` is true 
then upper bound < lower bound making an invalid range. Which is what I'd 
expect for some default/uninitialised state (especially if zero size ranges are 
allowed, so upper == 0 and lower == 0 couldn't be used).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131312

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

[Lldb-commits] [PATCH] D131244: [LLDB] Missing break in a switch statement alters the execution flow.

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added inline comments.



Comment at: lldb/source/Plugins/Process/Utility/ARMUtils.h:49
   shift_t = SRType_Invalid;
   return UINT32_MAX;
 }

This is now dead code?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131244

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

[Lldb-commits] [PATCH] D131304: [lldb] Remove uses of six module (NFC)

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added a comment.

There is also a cmake var `LLDB_USE_SYSTEM_SIX` which can be removed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131304

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

[Lldb-commits] [PATCH] D130689: [LLVM] Update C++ standard to 17

[Tobias Hieta via Phabricator via lldb-commits]

thieta added a comment.

In D130689#3705579 , @Jake-Egan wrote:

> There is a failure on the AIX bot also:
> ...
> https://lab.llvm.org/buildbot/#/builders/214/builds/2707/steps/5/logs/stdio

Filed an issue here: https://github.com/llvm/llvm-project/issues/56995


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130689

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

[Lldb-commits] [PATCH] D130689: [LLVM] Update C++ standard to 17

[Tobias Hieta via Phabricator via lldb-commits]

thieta added a comment.

In D130689#3705474 , @dyung wrote:

> We are seeing an additional failure on an internal linux bot due to the 
> change to using C++17 by default when using GNU ld:
> ...
> Switching between BFD ld and gold still fails (although gold fails for a 
> slightly different reason). Superficially it seems that switching to C++17 
> for some reason might be causing the compiler to emit debug info that these 
> older non-lld linkers cannot understand for some reason?

Filed this issue here: https://github.com/llvm/llvm-project/issues/56994


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130689

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