[Lldb-commits] [lldb] [lldb][DWARFASTParser][NFC] Factor out CXX/ObjC method specifics out of ParseSubroutine (PR #95078)

[Pavel Labath via lldb-commits]

@@ -975,6 +975,219 @@ ConvertDWARFCallingConventionToClang(const 
ParsedDWARFTypeAttributes ) {
   return clang::CC_C;
 }
 
+bool DWARFASTParserClang::ParseObjCMethod(
+const ObjCLanguage::MethodName _method, const DWARFDIE ,
+CompilerType clang_type, const ParsedDWARFTypeAttributes ,
+bool is_variadic) {
+  SymbolFileDWARF *dwarf = die.GetDWARF();

labath wrote:

(I think the only way you can realistically can a null pointer here is if you 
start out with an invalid/empty dwarf die)

https://github.com/llvm/llvm-project/pull/95078
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][DWARFASTParser][NFC] Factor out CXX/ObjC method specifics out of ParseSubroutine (PR #95078)

[Pavel Labath via lldb-commits]

https://github.com/labath approved this pull request.


https://github.com/llvm/llvm-project/pull/95078
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][DWARFASTParser][NFC] Factor out CXX/ObjC method specifics out of ParseSubroutine (PR #95078)

[Pavel Labath via lldb-commits]

https://github.com/labath edited https://github.com/llvm/llvm-project/pull/95078
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb/DWARF] Remove some dead code (PR #95127)

[Pavel Labath via lldb-commits]

https://github.com/labath closed https://github.com/llvm/llvm-project/pull/95127
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb/DWARF] Remove some dead code (PR #95127)

[Pavel Labath via lldb-commits]

https://github.com/labath created 
https://github.com/llvm/llvm-project/pull/95127

`GetDeclContextDIEs` and `DIEDeclContextsMatch` are unused (possibly since we 
added support for simplified template names, but I haven't checked). 
`GetDeclContextDIEs` is also very similar (but subtly different) from 
`GetDeclContext` and `GetTypeLookupContext`.

I am keeping `GetParentDeclContextDIE` as that one still has some callers, but 
I want to look into the possibility of merging it with at least one of the 
functions mentioned above.

>From 72bee31a54df4072c5a2c4bdd83d3243bc2ac5f6 Mon Sep 17 00:00:00 2001
From: Pavel Labath 
Date: Tue, 11 Jun 2024 16:26:24 +0200
Subject: [PATCH] [lldb/DWARF] Remove some dead code

`GetDeclContextDIEs` and `DIEDeclContextsMatch` are unused (possibly
since we added support for simplified template names, but I haven't
checked). `GetDeclContextDIEs` is also very similar (but subtly
different) from `GetDeclContext` and `GetTypeLookupContext`.

I am keeping `GetParentDeclContextDIE` as that one still has some
callers, but I want to look into the possibility of merging it with at
least one of the functions mentioned above.
---
 .../Plugins/SymbolFile/DWARF/DWARFDIE.cpp | 14 ---
 .../Plugins/SymbolFile/DWARF/DWARFDIE.h   |  3 -
 .../SymbolFile/DWARF/SymbolFileDWARF.cpp  | 89 ---
 .../SymbolFile/DWARF/SymbolFileDWARF.h|  2 -
 4 files changed, 108 deletions(-)

diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFDIE.cpp 
b/lldb/source/Plugins/SymbolFile/DWARF/DWARFDIE.cpp
index 7cf92adc6ef57..0ef94ed9f17c3 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFDIE.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFDIE.cpp
@@ -367,20 +367,6 @@ lldb_private::Type *DWARFDIE::ResolveTypeUID(const 
DWARFDIE ) const {
   return nullptr;
 }
 
-std::vector DWARFDIE::GetDeclContextDIEs() const {
-  if (!IsValid())
-return {};
-
-  std::vector result;
-  DWARFDIE parent = GetParentDeclContextDIE();
-  while (parent.IsValid() && parent.GetDIE() != GetDIE()) {
-result.push_back(std::move(parent));
-parent = parent.GetParentDeclContextDIE();
-  }
-
-  return result;
-}
-
 static void GetDeclContextImpl(DWARFDIE die,
llvm::SmallSet ,
std::vector ) {
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFDIE.h 
b/lldb/source/Plugins/SymbolFile/DWARF/DWARFDIE.h
index 511ca62d0197a..c74a82061fccf 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFDIE.h
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFDIE.h
@@ -69,9 +69,6 @@ class DWARFDIE : public DWARFBaseDIE {
   DWARFDIE
   GetParentDeclContextDIE() const;
 
-  // DeclContext related functions
-  std::vector GetDeclContextDIEs() const;
-
   /// Return this DIE's decl context as it is needed to look up types
   /// in Clang modules. This context will include any modules or functions that
   /// the type is declared in so an exact module match can be efficiently made.
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp 
b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
index a52a7d6767374..d9e81f9c105b2 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
@@ -3039,95 +3039,6 @@ TypeSP 
SymbolFileDWARF::FindCompleteObjCDefinitionTypeForDIE(
   return type_sp;
 }
 
-// This function helps to ensure that the declaration contexts match for two
-// different DIEs. Often times debug information will refer to a forward
-// declaration of a type (the equivalent of "struct my_struct;". There will
-// often be a declaration of that type elsewhere that has the full definition.
-// When we go looking for the full type "my_struct", we will find one or more
-// matches in the accelerator tables and we will then need to make sure the
-// type was in the same declaration context as the original DIE. This function
-// can efficiently compare two DIEs and will return true when the declaration
-// context matches, and false when they don't.
-bool SymbolFileDWARF::DIEDeclContextsMatch(const DWARFDIE ,
-   const DWARFDIE ) {
-  if (die1 == die2)
-return true;
-
-  std::vector decl_ctx_1;
-  std::vector decl_ctx_2;
-  // The declaration DIE stack is a stack of the declaration context DIEs all
-  // the way back to the compile unit. If a type "T" is declared inside a class
-  // "B", and class "B" is declared inside a class "A" and class "A" is in a
-  // namespace "lldb", and the namespace is in a compile unit, there will be a
-  // stack of DIEs:
-  //
-  //   [0] DW_TAG_class_type for "B"
-  //   [1] DW_TAG_class_type for "A"
-  //   [2] DW_TAG_namespace  for "lldb"
-  //   [3] DW_TAG_compile_unit or DW_TAG_partial_unit for the source file.
-  //
-  // We grab both contexts and make sure that everything matches all the way
-  // back to the compiler unit.
-
-  // First lets grab the decl contexts for both 

[Lldb-commits] [lldb] [lldb] Skip declaration DIEs in the debug_names index (PR #94744)

[Pavel Labath via lldb-commits]

https://github.com/labath closed https://github.com/llvm/llvm-project/pull/94744
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [WIP][lldb] Use forward decls with redeclared definitions instead of minimal import for records (PR #95100)

[Pavel Labath via lldb-commits]

labath wrote:

Thank you for working on this. I'm very interested in the results of this 
effort, as it appears I may end up dabbling into these parts of lldb in the 
near future. For now just a couple of quick questions (with hopefully not too 
long answers).

- when you say "slower", what exactly does that mean. How much slow down are we 
talking about?
- the "increased number of DWARF searches", is that due to clang asking for 
definitions of types more eagerly? If yes, do you have some examples of where 
are these extra definitions coming from?
- I see one test which tries to skip itself conditionally based on a setting 
enabling this, but I don't see the code for the handling the setting itself. Is 
the intention to add the setting, or will this be a flag day change?
- I'm intrigued by this line "Instead of creating partially defined records 
that have fields but possibly no definition, ...". Until last week, I had 
assumed that types in lldb (and clang) can exist in only one of two states 
"empty/forward declared/undefined" and "fully defined". I was intrigued to find 
some support for loading only fields (`setHasLoadedFieldsFromExternalStorage`, 
et al.) in clang (but not in lldb, AIUI). Does this imply that the code for 
loading only the fields is broken (is the thing you're trying to remove)?

https://github.com/llvm/llvm-project/pull/95100
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][DWARFASTParser][NFC] Factor out CXX/ObjC method specifics out of ParseSubroutine (PR #95078)

[Pavel Labath via lldb-commits]

@@ -975,6 +975,226 @@ ConvertDWARFCallingConventionToClang(const 
ParsedDWARFTypeAttributes ) {
   return clang::CC_C;
 }
 
+bool DWARFASTParserClang::HandleObjCMethod(
+ObjCLanguage::MethodName const _method, DWARFDIE const ,
+CompilerType clang_type, ParsedDWARFTypeAttributes const ,
+bool is_variadic) {
+  SymbolFileDWARF *dwarf = die.GetDWARF();
+  const auto tag = die.Tag();
+  ConstString class_name(objc_method.GetClassName());
+  if (!class_name)
+return false;
+
+  TypeSP complete_objc_class_type_sp(
+  dwarf->FindCompleteObjCDefinitionTypeForDIE(DWARFDIE(), class_name,
+  false));
+
+  if (!complete_objc_class_type_sp)
+return false;
+
+  CompilerType type_clang_forward_type =
+  complete_objc_class_type_sp->GetForwardCompilerType();
+
+  if (!type_clang_forward_type)
+return false;
+
+  if (!TypeSystemClang::IsObjCObjectOrInterfaceType(type_clang_forward_type))
+return false;
+
+  clang::ObjCMethodDecl *objc_method_decl = m_ast.AddMethodToObjCObjectType(
+  type_clang_forward_type, attrs.name.GetCString(), clang_type,
+  attrs.is_artificial, is_variadic, attrs.is_objc_direct_call);
+
+  if (!objc_method_decl) {
+dwarf->GetObjectFile()->GetModule()->ReportError(
+"[{0:x16}]: invalid Objective-C method {1:x4} ({2}), "
+"please file a bug and attach the file at the start of "
+"this error message",
+die.GetOffset(), tag, DW_TAG_value_to_name(tag));
+return false;
+  }
+
+  LinkDeclContextToDIE(objc_method_decl, die);
+  m_ast.SetMetadataAsUserID(objc_method_decl, die.GetID());
+
+  return true;
+}
+
+std::pair DWARFASTParserClang::HandleCXXMethod(
+DWARFDIE const , CompilerType clang_type,
+ParsedDWARFTypeAttributes const , DWARFDIE const _ctx_die,
+bool is_static, bool _containing_context) {
+  Log *log = GetLog(DWARFLog::TypeCompletion | DWARFLog::Lookups);
+  SymbolFileDWARF *dwarf = die.GetDWARF();
+  // Look at the parent of this DIE and see if it is a class or
+  // struct and see if this is actually a C++ method
+  Type *class_type = dwarf->ResolveType(decl_ctx_die);
+  if (!class_type)
+return {};

labath wrote:

The difference I guess is that that check just looks at whether we're 
syntactically within a DW_TAG_class_type, whereas this checks whether we were 
able to resolve that TAG into an actual type. I don't know of a specific reason 
why that could fail, however. So yeah, maybe just remove the comment.

https://github.com/llvm/llvm-project/pull/95078
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][DWARFASTParser][NFC] Factor out CXX/ObjC method specifics out of ParseSubroutine (PR #95078)

[Pavel Labath via lldb-commits]

@@ -975,6 +975,226 @@ ConvertDWARFCallingConventionToClang(const 
ParsedDWARFTypeAttributes ) {
   return clang::CC_C;
 }
 
+bool DWARFASTParserClang::HandleObjCMethod(
+ObjCLanguage::MethodName const _method, DWARFDIE const ,
+CompilerType clang_type, ParsedDWARFTypeAttributes const ,
+bool is_variadic) {
+  SymbolFileDWARF *dwarf = die.GetDWARF();
+  const auto tag = die.Tag();
+  ConstString class_name(objc_method.GetClassName());
+  if (!class_name)
+return false;
+
+  TypeSP complete_objc_class_type_sp(
+  dwarf->FindCompleteObjCDefinitionTypeForDIE(DWARFDIE(), class_name,
+  false));
+
+  if (!complete_objc_class_type_sp)
+return false;
+
+  CompilerType type_clang_forward_type =
+  complete_objc_class_type_sp->GetForwardCompilerType();
+
+  if (!type_clang_forward_type)
+return false;
+
+  if (!TypeSystemClang::IsObjCObjectOrInterfaceType(type_clang_forward_type))
+return false;
+
+  clang::ObjCMethodDecl *objc_method_decl = m_ast.AddMethodToObjCObjectType(
+  type_clang_forward_type, attrs.name.GetCString(), clang_type,
+  attrs.is_artificial, is_variadic, attrs.is_objc_direct_call);
+
+  if (!objc_method_decl) {
+dwarf->GetObjectFile()->GetModule()->ReportError(
+"[{0:x16}]: invalid Objective-C method {1:x4} ({2}), "
+"please file a bug and attach the file at the start of "
+"this error message",
+die.GetOffset(), tag, DW_TAG_value_to_name(tag));
+return false;
+  }
+
+  LinkDeclContextToDIE(objc_method_decl, die);
+  m_ast.SetMetadataAsUserID(objc_method_decl, die.GetID());
+
+  return true;
+}
+
+std::pair DWARFASTParserClang::HandleCXXMethod(
+DWARFDIE const , CompilerType clang_type,
+ParsedDWARFTypeAttributes const , DWARFDIE const _ctx_die,
+bool is_static, bool _containing_context) {
+  Log *log = GetLog(DWARFLog::TypeCompletion | DWARFLog::Lookups);
+  SymbolFileDWARF *dwarf = die.GetDWARF();
+  // Look at the parent of this DIE and see if it is a class or
+  // struct and see if this is actually a C++ method
+  Type *class_type = dwarf->ResolveType(decl_ctx_die);
+  if (!class_type)
+return {};
+
+  if (class_type->GetID() != decl_ctx_die.GetID() ||
+  IsClangModuleFwdDecl(decl_ctx_die)) {
+
+// We uniqued the parent class of this function to another
+// class so we now need to associate all dies under
+// "decl_ctx_die" to DIEs in the DIE for "class_type"...
+DWARFDIE class_type_die = dwarf->GetDIE(class_type->GetID());
+
+if (class_type_die) {
+  std::vector failures;
+
+  CopyUniqueClassMethodTypes(decl_ctx_die, class_type_die, class_type,
+ failures);
+
+  // FIXME do something with these failures that's
+  // smarter than just dropping them on the ground.
+  // Unfortunately classes don't like having stuff added
+  // to them after their definitions are complete...
+
+  Type *type_ptr = dwarf->GetDIEToType()[die.GetDIE()];
+  if (type_ptr && type_ptr != DIE_IS_BEING_PARSED) {
+return {true, type_ptr->shared_from_this()};
+  }
+}
+  }
+
+  if (attrs.specification.IsValid()) {
+// We have a specification which we are going to base our
+// function prototype off of, so we need this type to be
+// completed so that the m_die_to_decl_ctx for the method in
+// the specification has a valid clang decl context.
+class_type->GetForwardCompilerType();
+// If we have a specification, then the function type should
+// have been made with the specification and not with this
+// die.
+DWARFDIE spec_die = attrs.specification.Reference();
+clang::DeclContext *spec_clang_decl_ctx =
+GetClangDeclContextForDIE(spec_die);
+if (spec_clang_decl_ctx) {
+  LinkDeclContextToDIE(spec_clang_decl_ctx, die);
+} else {
+  dwarf->GetObjectFile()->GetModule()->ReportWarning(
+  "{0:x8}: DW_AT_specification({1:x16}"
+  ") has no decl\n",
+  die.GetID(), spec_die.GetOffset());
+}
+return {true, nullptr};
+  }
+
+  if (attrs.abstract_origin.IsValid()) {
+// We have a specification which we are going to base our
+// function prototype off of, so we need this type to be
+// completed so that the m_die_to_decl_ctx for the method in
+// the abstract origin has a valid clang decl context.
+class_type->GetForwardCompilerType();
+
+DWARFDIE abs_die = attrs.abstract_origin.Reference();
+clang::DeclContext *abs_clang_decl_ctx = 
GetClangDeclContextForDIE(abs_die);
+if (abs_clang_decl_ctx) {
+  LinkDeclContextToDIE(abs_clang_decl_ctx, die);
+} else {
+  dwarf->GetObjectFile()->GetModule()->ReportWarning(
+  "{0:x8}: DW_AT_abstract_origin({1:x16}"
+  ") has no decl\n",
+  die.GetID(), abs_die.GetOffset());
+}
+
+return {true, nullptr};
+  }
+
+  CompilerType class_opaque_type = 

[Lldb-commits] [lldb] [lldb][DWARFASTParser][NFC] Factor out CXX/ObjC method specifics out of ParseSubroutine (PR #95078)

[Pavel Labath via lldb-commits]

@@ -975,6 +975,226 @@ ConvertDWARFCallingConventionToClang(const 
ParsedDWARFTypeAttributes ) {
   return clang::CC_C;
 }
 
+bool DWARFASTParserClang::HandleObjCMethod(
+ObjCLanguage::MethodName const _method, DWARFDIE const ,

labath wrote:

I believe we normally put `const` before the type name.

https://github.com/llvm/llvm-project/pull/95078
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][DWARFASTParser][NFC] Factor out CXX/ObjC method specifics out of ParseSubroutine (PR #95078)

[Pavel Labath via lldb-commits]

@@ -370,6 +371,20 @@ class DWARFASTParserClang : public 
lldb_private::plugin::dwarf::DWARFASTParser {
  ParsedDWARFTypeAttributes );
   lldb::TypeSP ParseSubroutine(const lldb_private::plugin::dwarf::DWARFDIE 
,
const ParsedDWARFTypeAttributes );
+
+  bool
+  HandleObjCMethod(lldb_private::ObjCLanguage::MethodName const _method,
+   lldb_private::plugin::dwarf::DWARFDIE const ,
+   lldb_private::CompilerType clang_type,
+   ParsedDWARFTypeAttributes const , bool is_variadic);
+
+  std::pair
+  HandleCXXMethod(lldb_private::plugin::dwarf::DWARFDIE const ,

labath wrote:

> LinkCXXMethodDeclContextToDIE

Is that all the function does? It seems to be doing a bit more, so maybe 
something with the word `Parse` in it might be better.

I can't really say anything confidently, since I don't really understand this 
code, even though I've spend a lot of time staring at it last week.

https://github.com/llvm/llvm-project/pull/95078
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][DWARFASTParser][NFC] Factor out CXX/ObjC method specifics out of ParseSubroutine (PR #95078)

[Pavel Labath via lldb-commits]

https://github.com/labath commented:

I can't say if this is the right split, but I think we should try something, as 
this function is long overdue for an overhaul.

https://github.com/llvm/llvm-project/pull/95078
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][DWARFASTParser][NFC] Factor out CXX/ObjC method specifics out of ParseSubroutine (PR #95078)

[Pavel Labath via lldb-commits]

@@ -975,6 +975,226 @@ ConvertDWARFCallingConventionToClang(const 
ParsedDWARFTypeAttributes ) {
   return clang::CC_C;
 }
 
+bool DWARFASTParserClang::HandleObjCMethod(
+ObjCLanguage::MethodName const _method, DWARFDIE const ,
+CompilerType clang_type, ParsedDWARFTypeAttributes const ,
+bool is_variadic) {
+  SymbolFileDWARF *dwarf = die.GetDWARF();
+  const auto tag = die.Tag();
+  ConstString class_name(objc_method.GetClassName());
+  if (!class_name)
+return false;
+
+  TypeSP complete_objc_class_type_sp(
+  dwarf->FindCompleteObjCDefinitionTypeForDIE(DWARFDIE(), class_name,
+  false));
+
+  if (!complete_objc_class_type_sp)
+return false;
+
+  CompilerType type_clang_forward_type =
+  complete_objc_class_type_sp->GetForwardCompilerType();
+
+  if (!type_clang_forward_type)
+return false;
+
+  if (!TypeSystemClang::IsObjCObjectOrInterfaceType(type_clang_forward_type))
+return false;
+
+  clang::ObjCMethodDecl *objc_method_decl = m_ast.AddMethodToObjCObjectType(
+  type_clang_forward_type, attrs.name.GetCString(), clang_type,
+  attrs.is_artificial, is_variadic, attrs.is_objc_direct_call);
+
+  if (!objc_method_decl) {
+dwarf->GetObjectFile()->GetModule()->ReportError(
+"[{0:x16}]: invalid Objective-C method {1:x4} ({2}), "
+"please file a bug and attach the file at the start of "
+"this error message",
+die.GetOffset(), tag, DW_TAG_value_to_name(tag));
+return false;
+  }
+
+  LinkDeclContextToDIE(objc_method_decl, die);
+  m_ast.SetMetadataAsUserID(objc_method_decl, die.GetID());
+
+  return true;
+}
+
+std::pair DWARFASTParserClang::HandleCXXMethod(
+DWARFDIE const , CompilerType clang_type,
+ParsedDWARFTypeAttributes const , DWARFDIE const _ctx_die,
+bool is_static, bool _containing_context) {
+  Log *log = GetLog(DWARFLog::TypeCompletion | DWARFLog::Lookups);
+  SymbolFileDWARF *dwarf = die.GetDWARF();
+  // Look at the parent of this DIE and see if it is a class or
+  // struct and see if this is actually a C++ method
+  Type *class_type = dwarf->ResolveType(decl_ctx_die);
+  if (!class_type)
+return {};

labath wrote:

Maybe this should be outside of the function, since going through the branch 
means we're not actually processing a method.

https://github.com/llvm/llvm-project/pull/95078
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][DWARFASTParser][NFC] Factor out CXX/ObjC method specifics out of ParseSubroutine (PR #95078)

[Pavel Labath via lldb-commits]

https://github.com/labath edited https://github.com/llvm/llvm-project/pull/95078
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] Fix type lookup bug where wrong decl context was being used for a DIE. (PR #94846)

[Pavel Labath via lldb-commits]

https://github.com/labath approved this pull request.

Thanks.

https://github.com/llvm/llvm-project/pull/94846
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] Fix type lookup bug where wrong decl context was being used for a DIE. (PR #94846)

[Pavel Labath via lldb-commits]

@@ -0,0 +1,69 @@
+"""
+Test the SBModule and SBTarget type lookup APIs to find multiple types.
+"""
+
+import lldb
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class TypeFindFirstTestCase(TestBase):
+def test_find_first_type(self):
+"""
+Test SBTarget::FindTypes() and SBModule::FindTypes() APIs.
+
+We had issues where our declaration context when finding types was
+incorrectly calculated where a type in a namepace, and a type in a
+function that was also in the same namespace would match a lookup. For
+example:
+
+namespace a {
+  struct Foo {
+int foo;
+  };
+
+  unsigned foo() {
+typedef unsigned Foo;
+Foo foo = 12;
+return foo;
+  }
+} // namespace a
+
+
+Previously LLDB would calculate the declaration context of "a::Foo"
+correctly, but incorrectly calculate the declaration context of "Foo"
+from within the foo() function as "a::Foo". Adding tests to ensure this
+works correctly.
+"""
+self.build()
+target = self.createTestTarget()
+exe_module = target.GetModuleAtIndex(0)
+self.assertTrue(exe_module.IsValid())
+# Test the SBTarget and SBModule APIs for FindFirstType
+for api in [target, exe_module]:
+# We should find the "a::Foo" but not the "Foo" type in the 
function
+types = api.FindTypes("a::Foo")
+self.assertEqual(types.GetSize(), 1)
+type_str0 = str(types.GetTypeAtIndex(0))
+self.assertIn('struct Foo', type_str0)
+
+# When we search by type basename, we should find any type whose
+# basename matches "Foo", so "a::Foo" and the "Foo" type in the
+# function.
+types = api.FindTypes("Foo")
+self.assertEqual(types.GetSize(), 2)
+type_str0 = str(types.GetTypeAtIndex(0))
+type_str1 = str(types.GetTypeAtIndex(1))
+# We don't know which order the types will come back as, so
+if 'struct Foo' in type_str0:
+self.assertIn('typedef Foo', type_str1)
+else:
+self.assertIn('struct Foo', type_str1)
+
+# When we search by type basename with "::" prepended, we should
+# only types in the root namespace which means only "Foo" type in

labath wrote:

> So I don't know of anyone that would use the function name when asking for a 
> type, even if that type was defined in a function.

It's certainly not valid C++, if that's what you mean (c++ just doesn't give 
you the option of referring to a local type from outside of the function), but 
I don't think this kind of qualification is a revolutionary concept. For 
example if the aforementioned local struct had a constructor, it's mangled name 
would be `_ZZN1a3fooEvEN3FooC1Ev `, that is `a::foo()::Foo::Foo()`. Gdb 
(AFAICT) does not support these kinds of lookups, but it also does not return 
the type for query like `::Foo`. (instead gdb supports searching for a type in 
a specific block/function, which is kind of similar to this)

> To make the expression parser work, we should be able to find all of the 
> types using a context like just "Foo", where we would find both `::Foo` and 
> the `Foo` from `a::foo()`, but sort the results based on the 
> CompilerDeclContext of each type that was returned. If we are evaluating an 
> expression in `a::foo()`, the `Foo` type from that function could tell us the 
> CompilerDeclContext it was defined in and we would find the closest match to 
> where the expression is being run.

This approach has the problem that a type (or really, anything) defined with 
the same name at a class level would take precedence over a local declaration.

For example with code like:
```
struct Foo { int one; }
class Class {
  struct Foo { int two; }
  void Method() {
struct Foo { int three; }
  }
};
```
if we were stopped in `Class::Method`, then the name `Foo` should refer to the 
third definition with that name, but I believe this will end up using the 
second one because clang will see the class-level declaration first and will 
not bother asking for other possible definitions. I think we used to use the 
same approach for local variables, but then we changed the implementation 
because of the same shadowing problem.

https://github.com/llvm/llvm-project/pull/94846
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb-dap] Added "port" property to vscode "attach" command. (PR #91570)

[Pavel Labath via lldb-commits]

https://github.com/labath approved this pull request.

My concerns have been addressed (*), so I'm removing my request for changes. It 
looks like the only way to do that is to Approve the PR, but I'd suggest 
getting another approval from a lldb-dap owner as well.

(*) There is still the question of the single "url" setting (to rule them all), 
but I don't consider myself an authority there.

https://github.com/llvm/llvm-project/pull/91570
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb-dap] Added "port" property to vscode "attach" command. (PR #91570)

[Pavel Labath via lldb-commits]

@@ -672,9 +672,14 @@ void request_attach(const llvm::json::Object ) {
   lldb::SBError error;
   FillResponse(request, response);
   lldb::SBAttachInfo attach_info;
+  const int invalid_port = 0;
   auto arguments = request.getObject("arguments");
   const lldb::pid_t pid =
   GetUnsigned(arguments, "pid", LLDB_INVALID_PROCESS_ID);
+  const auto gdb_remote_port =
+  GetUnsigned(arguments, "gdb-remote-port", invalid_port);
+  llvm::StringRef gdb_remote_hostname =
+  GetString(arguments, "gdb-remote-hostname", "localhost");

labath wrote:

There are environments (Google's (default) test environment is like that) that 
don't have an IPv4 stack configured (just v6), so `localhost` would be an 
advantage there. That said, this definitely wouldn't be the first instance of 
`127.0.0.1 in lldb.

FWIW, I'm of the opinion that anyone who configures `localhost` to point to 
something other than their local host deserves whatever is coming their way,

https://github.com/llvm/llvm-project/pull/91570
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb-dap] Added "port" property to vscode "attach" command. (PR #91570)

[Pavel Labath via lldb-commits]

https://github.com/labath edited https://github.com/llvm/llvm-project/pull/91570
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb-dap] Added "port" property to vscode "attach" command. (PR #91570)

[Pavel Labath via lldb-commits]

@@ -0,0 +1,202 @@
+"""
+Test lldb-dap "port" configuration to "attach" request
+"""
+
+
+import dap_server
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+from lldbsuite.test import lldbplatformutil
+import lldbdap_testcase
+import os
+import shutil
+import subprocess
+import tempfile
+import threading
+import sys
+import socket
+import select
+
+
+# A class representing a pipe for communicating with debug server.
+# This class includes menthods to open the pipe and read the port number from 
it.
+class Pipe(object):
+def __init__(self, prefix):
+self.name = os.path.join(prefix, "stub_port_number")
+os.mkfifo(self.name)
+self._fd = os.open(self.name, os.O_RDONLY | os.O_NONBLOCK)
+
+def finish_connection(self, timeout):
+pass
+
+def read(self, size, timeout):
+(readers, _, _) = select.select([self._fd], [], [], timeout)
+if self._fd not in readers:
+raise TimeoutError
+return os.read(self._fd, size)
+
+def close(self):
+os.close(self._fd)

labath wrote:

Thanks. Glad we got that resolved.

https://github.com/llvm/llvm-project/pull/91570
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb] Fix TestModuleLoadedNotifys API test to work correctly on most of Linux targets (PR #94672)

[Pavel Labath via lldb-commits]

https://github.com/labath approved this pull request.

Thanks.

https://github.com/llvm/llvm-project/pull/94672
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb] Gracefully down TestCoroutineHandle test in case the 'coroutine' feature is missing (PR #94903)

[Pavel Labath via lldb-commits]

https://github.com/labath approved this pull request.


https://github.com/llvm/llvm-project/pull/94903
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb] Fix TestModuleLoadedNotifys API test to work correctly on most of Linux targets (PR #94672)

[Pavel Labath via lldb-commits]

https://github.com/labath edited https://github.com/llvm/llvm-project/pull/94672
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb] Fix TestModuleLoadedNotifys API test to work correctly on most of Linux targets (PR #94672)

[Pavel Labath via lldb-commits]

@@ -118,6 +118,6 @@ def test_launch_notifications(self):
 # On Linux we get events for ld.so, [vdso], the binary and then all 
libraries.
 
 avg_solibs_added_per_event = round(
-float(total_solibs_added) / float(total_modules_added_events)
+10.0 * float(total_solibs_added) / 
float(total_modules_added_events)

labath wrote:

> > You can't really call it "number of libraries per event" anymore if you 
> > multiply by ten.
> 
> I have updated the patch.
> 
> > Maybe you could just remove the round call, and compare to 1.0
> 
> No, because assertGreater() expects int parameters.

That's strange because `assertGreater(47.0001, 47.)` works just fine for 
me. So does `assertLessEqual(set(["a", "b"]), set(["a", "b"]))`.

Could it be that you're still running on the version of lldb that has the 
ancient checked in copy of `unittest`?


https://github.com/llvm/llvm-project/pull/94672
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb] Fix TestModuleLoadedNotifys API test to work correctly on most of Linux targets (PR #94672)

[Pavel Labath via lldb-commits]

@@ -115,9 +151,10 @@ def test_launch_notifications(self):
 # binaries in batches.  Check that we got back more than 1 solib per 
event.
 # In practice on Darwin today, we get back two events for a do-nothing 
c
 # program: a.out and dyld, and then all the rest of the system 
libraries.
-# On Linux we get events for ld.so, [vdso], the binary and then all 
libraries.
-
-avg_solibs_added_per_event = round(
-float(total_solibs_added) / float(total_modules_added_events)
+# On Linux we get events for ld.so, [vdso], the binary and then all 
libraries,
+# but the different configurations could load a different number of 
.so modules
+# per event.
+self.assertGreaterEqual(
+len(set(max_solib_chunk_per_event).intersection(expected_solibs)),
+len(expected_solibs),

labath wrote:

I think this could be `assertLess(set(expected_solibs), 
set(max_solib_chunk_per_event))` (with the advantage being that you'd bet a 
better error message than `3 is not less than 4` when it fails.

https://github.com/llvm/llvm-project/pull/94672
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb] Fix TestModuleLoadedNotifys API test to work correctly on most of Linux targets (PR #94672)

[Pavel Labath via lldb-commits]

@@ -9,22 +9,51 @@
 from lldbsuite.test import lldbutil
 
 
+@skipUnlessPlatform(["linux"] + lldbplatformutil.getDarwinOSTriples())
 class ModuleLoadedNotifysTestCase(TestBase):
 NO_DEBUG_INFO_TESTCASE = True
 
 # At least DynamicLoaderDarwin and DynamicLoaderPOSIXDYLD should batch up
 # notifications about newly added/removed libraries.  Other DynamicLoaders 
may
 # not be written this way.
-@skipUnlessPlatform(["linux"] + lldbplatformutil.getDarwinOSTriples())
 def setUp(self):
 # Call super's setUp().
 TestBase.setUp(self)
 # Find the line number to break inside main().
 self.line = line_number("main.cpp", "// breakpoint")
 
+def setup_test(self, solibs):
+if lldb.remote_platform:
+path = lldb.remote_platform.GetWorkingDirectory()
+for f in solibs:
+lldbutil.install_to_target(self, self.getBuildArtifact(f))
+else:
+path = self.getBuildDir()
+if self.dylibPath in os.environ:
+sep = self.platformContext.shlib_path_separator
+path = os.environ[self.dylibPath] + sep + path
+self.runCmd(
+"settings append target.env-vars '{}={}'".format(self.dylibPath, 
path)
+)
+self.default_path = path
+
 def test_launch_notifications(self):
 """Test that lldb broadcasts newly loaded libraries in batches."""
+
+ext = "so"
+if self.platformIsDarwin():
+ext = "dylib"

labath wrote:

You should be able to get this as `self.platformContext.shlib_extension`

https://github.com/llvm/llvm-project/pull/94672
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb] Fix TestModuleLoadedNotifys API test to work correctly on most of Linux targets (PR #94672)

[Pavel Labath via lldb-commits]

@@ -1,3 +1,23 @@
-CXX_SOURCES := main.cpp
-
-include Makefile.rules
+CXX_SOURCES := main.cpp
+LD_EXTRAS := -L. -lloadunload_d -lloadunload_c -lloadunload_a -lloadunload_b

labath wrote:

Please rename the modules to something else (even a simple `liba` is probably 
fine).

https://github.com/llvm/llvm-project/pull/94672
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb] Fix TestModuleLoadedNotifys API test to work correctly on most of Linux targets (PR #94672)

[Pavel Labath via lldb-commits]

https://github.com/labath commented:

This is great stuff, just a couple of details

https://github.com/llvm/llvm-project/pull/94672
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb] Fix TestModuleLoadedNotifys API test to work correctly on most of Linux targets (PR #94672)

[Pavel Labath via lldb-commits]

https://github.com/labath edited https://github.com/llvm/llvm-project/pull/94672
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb] Fix TestModuleLoadedNotifys API test to work correctly on most of Linux targets (PR #94672)

[Pavel Labath via lldb-commits]

@@ -0,0 +1,7 @@
+extern "C" int b_function();
+
+int a_init() { return 234; }
+
+int a_global = a_init();

labath wrote:

Just delete all of this stuff, as it's not relevant for what we're testing. 
(the `a_function` -> `b_function` dep is marginally interesting, as it creates 
a more interesting dependency graph).

https://github.com/llvm/llvm-project/pull/94672
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] Fix type lookup bug where wrong decl context was being used for a DIE. (PR #94846)

[Pavel Labath via lldb-commits]

@@ -0,0 +1,69 @@
+"""

labath wrote:

+1

https://github.com/llvm/llvm-project/pull/94846
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] Fix type lookup bug where wrong decl context was being used for a DIE. (PR #94846)

[Pavel Labath via lldb-commits]

@@ -0,0 +1,69 @@
+"""
+Test the SBModule and SBTarget type lookup APIs to find multiple types.
+"""
+
+import lldb
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class TypeFindFirstTestCase(TestBase):
+def test_find_first_type(self):
+"""
+Test SBTarget::FindTypes() and SBModule::FindTypes() APIs.
+
+We had issues where our declaration context when finding types was
+incorrectly calculated where a type in a namepace, and a type in a
+function that was also in the same namespace would match a lookup. For
+example:
+
+namespace a {
+  struct Foo {
+int foo;
+  };
+
+  unsigned foo() {
+typedef unsigned Foo;
+Foo foo = 12;
+return foo;
+  }
+} // namespace a
+
+
+Previously LLDB would calculate the declaration context of "a::Foo"
+correctly, but incorrectly calculate the declaration context of "Foo"
+from within the foo() function as "a::Foo". Adding tests to ensure this
+works correctly.
+"""
+self.build()
+target = self.createTestTarget()
+exe_module = target.GetModuleAtIndex(0)
+self.assertTrue(exe_module.IsValid())
+# Test the SBTarget and SBModule APIs for FindFirstType
+for api in [target, exe_module]:
+# We should find the "a::Foo" but not the "Foo" type in the 
function
+types = api.FindTypes("a::Foo")
+self.assertEqual(types.GetSize(), 1)
+type_str0 = str(types.GetTypeAtIndex(0))
+self.assertIn('struct Foo', type_str0)
+
+# When we search by type basename, we should find any type whose
+# basename matches "Foo", so "a::Foo" and the "Foo" type in the
+# function.
+types = api.FindTypes("Foo")
+self.assertEqual(types.GetSize(), 2)
+type_str0 = str(types.GetTypeAtIndex(0))
+type_str1 = str(types.GetTypeAtIndex(1))
+# We don't know which order the types will come back as, so
+if 'struct Foo' in type_str0:
+self.assertIn('typedef Foo', type_str1)
+else:
+self.assertIn('struct Foo', type_str1)
+
+# When we search by type basename with "::" prepended, we should
+# only types in the root namespace which means only "Foo" type in

labath wrote:

I would say this is actually a bug, and it's one of the reasons why I said the 
two functions should be merged (as Michael alluded to in the other comment). I 
think that a better full name for this type would be something like 
`::a::foo()::Foo`
I did look into this briefly (before being preempted). Basically, I had this 
function follow DW_AT_specification links just like it was done for 
`GetDeclContext`, but I ran into two problems:
- this just gives you a name like `::a::foo::Foo` which, while probably better 
than `::Foo` is still not fully correct as the type `Foo` in `foo(void)` can be 
completely different from type `Foo` in `foo(something_else)`.
- it prevented us from accessing the type `Foo` in the expression evaluator, 
which currently "works" (scare quotes, because it only works if there are no 
conflicts) because we pretend the type to be global. To fix this we'd need to 
implement a dedicated method for injecting local types into the expression, 
just like we do with local variables. I don't think that's particularly hard, 
but it definitely takes more time that I had at the moment.

https://github.com/llvm/llvm-project/pull/94846
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] Fix type lookup bug where wrong decl context was being used for a DIE. (PR #94846)

[Pavel Labath via lldb-commits]

@@ -491,6 +491,18 @@ static void GetTypeLookupContextImpl(DWARFDIE die,
 case DW_TAG_base_type:
   push_ctx(CompilerContextKind::Builtin, name);
   break;
+// If any of the tags below appear in the parent chain, stop the decl
+// context and return. Prior to these being in here, if a type existed in a
+// namespace "a" like "a::my_struct", but we also have a function in that
+// same namespace "a" which contained a type named "my_struct", both would
+// return "a::my_struct" as the declaration context since the
+// DW_TAG_subprogram would be skipped and its parent would be found.
+case DW_TAG_compile_unit:

labath wrote:

That's fine. The removal of the early return was unintentional, although (see 
other comment) I don't think it's totally the right thing to do either.

https://github.com/llvm/llvm-project/pull/94846
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] Fix type lookup bug where wrong decl context was being used for a DIE. (PR #94846)

[Pavel Labath via lldb-commits]

@@ -0,0 +1,69 @@
+"""
+Test the SBModule and SBTarget type lookup APIs to find multiple types.
+"""
+
+import lldb
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class TypeFindFirstTestCase(TestBase):
+def test_find_first_type(self):
+"""
+Test SBTarget::FindTypes() and SBModule::FindTypes() APIs.
+
+We had issues where our declaration context when finding types was
+incorrectly calculated where a type in a namepace, and a type in a
+function that was also in the same namespace would match a lookup. For
+example:
+
+namespace a {
+  struct Foo {
+int foo;
+  };
+
+  unsigned foo() {
+typedef unsigned Foo;
+Foo foo = 12;
+return foo;
+  }
+} // namespace a
+
+
+Previously LLDB would calculate the declaration context of "a::Foo"
+correctly, but incorrectly calculate the declaration context of "Foo"
+from within the foo() function as "a::Foo". Adding tests to ensure this
+works correctly.
+"""
+self.build()
+target = self.createTestTarget()
+exe_module = target.GetModuleAtIndex(0)
+self.assertTrue(exe_module.IsValid())
+# Test the SBTarget and SBModule APIs for FindFirstType
+for api in [target, exe_module]:
+# We should find the "a::Foo" but not the "Foo" type in the 
function
+types = api.FindTypes("a::Foo")
+self.assertEqual(types.GetSize(), 1)
+type_str0 = str(types.GetTypeAtIndex(0))
+self.assertIn('struct Foo', type_str0)
+
+# When we search by type basename, we should find any type whose
+# basename matches "Foo", so "a::Foo" and the "Foo" type in the
+# function.
+types = api.FindTypes("Foo")
+self.assertEqual(types.GetSize(), 2)
+type_str0 = str(types.GetTypeAtIndex(0))
+type_str1 = str(types.GetTypeAtIndex(1))
+# We don't know which order the types will come back as, so
+if 'struct Foo' in type_str0:
+self.assertIn('typedef Foo', type_str1)
+else:
+self.assertIn('struct Foo', type_str1)

labath wrote:

Would something like this work:
`assertEqual(set([t.GetName() for t in types]), set(["typedef Foo", "struct 
Foo"]))`

https://github.com/llvm/llvm-project/pull/94846
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb] Adjust the for loop condition to prevent unintended increments in ExpandRLE (NFC) (PR #94844)

[Pavel Labath via lldb-commits]

@@ -1316,6 +1316,7 @@ std::string GDBRemoteCommunication::ExpandRLE(std::string 
packet) {
 } else {
   decoded.push_back(*c);
 }
+c++;

labath wrote:

How does this prevent an out of bounds access? If I understand the problem 
correctly, the right fix to check for an almost-end iterator when doing the RLE 
expansion (change the condition to something like `if (*c == '*' && 
std::next(c) != packet.end())`, just less clumsy).

https://github.com/llvm/llvm-project/pull/94844
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb] Fix redundant condition in compression type check (NFC) (PR #94841)

[Pavel Labath via lldb-commits]

https://github.com/labath approved this pull request.


https://github.com/llvm/llvm-project/pull/94841
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb] Remove redundant condition in watch mask check (NFC) (PR #94842)

[Pavel Labath via lldb-commits]

https://github.com/labath approved this pull request.


https://github.com/llvm/llvm-project/pull/94842
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [llvm] Add support for using foreign type units in .debug_names. (PR #87740)

[Pavel Labath via lldb-commits]

@@ -1741,45 +1741,61 @@ lldb::ModuleSP 
SymbolFileDWARF::GetExternalModule(ConstString name) {
   return pos->second;
 }
 
-DWARFDIE
-SymbolFileDWARF::GetDIE(const DIERef _ref) {
-  // This method can be called without going through the symbol vendor so we
-  // need to lock the module.
-  std::lock_guard guard(GetModuleMutex());
-
-  SymbolFileDWARF *symbol_file = nullptr;
-
+SymbolFileDWARF *SymbolFileDWARF::GetDIERefSymbolFile(const DIERef _ref) {
   // Anytime we get a "lldb::user_id_t" from an lldb_private::SymbolFile API we
   // must make sure we use the correct DWARF file when resolving things. On
   // MacOSX, when using SymbolFileDWARFDebugMap, we will use multiple
   // SymbolFileDWARF classes, one for each .o file. We can often end up with
   // references to other DWARF objects and we must be ready to receive a
   // "lldb::user_id_t" that specifies a DIE from another SymbolFileDWARF
   // instance.
+
   std::optional file_index = die_ref.file_index();
+
+  // If the file index matches, then we have the right SymbolFileDWARF already.
+  // This will work for both .dwo file and DWARF in .o files for mac. Also if
+  // both the file indexes are invalid, then we have a match.
+  if (GetFileIndex() == file_index)
+return this;
+
+  // If we are currently in a .dwo file and our file index doesn't match we 
need
+  // to let the base symbol file handle this.
+  SymbolFileDWARFDwo *dwo = llvm::dyn_cast_or_null(this);
+  if (dwo)
+return dwo->GetBaseSymbolFile().GetDIERefSymbolFile(die_ref);

labath wrote:

A better (and more consistent) way to implement this would be to make 
`GetDIERefSymbolFile` virtual, and then have the Dwo implementation do `return 
GetBaseSymbolFile().GetDIERefSymbolFile(die_ref)`.

https://github.com/llvm/llvm-project/pull/87740
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [llvm] Add support for using foreign type units in .debug_names. (PR #87740)

[Pavel Labath via lldb-commits]

@@ -85,6 +86,31 @@ class DebugNamesDWARFIndex : public DWARFIndex {
 
   DWARFUnit *GetNonSkeletonUnit(const DebugNames::Entry ) const;
   DWARFDIE GetDIE(const DebugNames::Entry ) const;
+
+  /// Checks if an entry is a foreign TU and fetch the type unit.
+  ///
+  /// This function checks if the DebugNames::Entry refers to a foreign TU and
+  /// returns an optional with a value of the \a entry is a foreign type unit
+  /// entry. A valid pointer will be returned if this entry is from a .dwo file
+  /// or if it is from a .dwp file and it matches the type unit's originating
+  /// .dwo file by verifying that the DW_TAG_type_unit DIE has a DW_AT_dwo_name
+  /// that matches the DWO name from the originating skeleton compile unit.
+  ///
+  /// \param[in] entry
+  ///   The accelerator table entry to check.
+  ///
+  /// \returns
+  ///   A std::optional that has a value if this entry represents a foreign 
type
+  ///   unit. If the pointer is valid, then we were able to find and match the
+  ///   entry to the type unit in the .dwo or .dwp file. The returned value can
+  ///   have a valid, yet contain NULL in the following cases:
+  ///   - we were not able to load the .dwo file (missing or DWO ID mismatch)
+  ///   - we were able to load the .dwp file, but the type units DWO name
+  /// doesn't match the originating skeleton compile unit's entry
+  ///   Returns std::nullopt if this entry is not a foreign type unit entry.
+  std::optional
+  IsForeignTypeUnit(const DebugNames::Entry ) const;

labath wrote:

Nit: I would expect a function called `IsSomething` to return `bool`. Maybe 
just call it `GetForeignTypeUnit` ?

https://github.com/llvm/llvm-project/pull/87740
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [llvm] Add support for using foreign type units in .debug_names. (PR #87740)

[Pavel Labath via lldb-commits]

@@ -48,20 +60,84 @@ DebugNamesDWARFIndex::GetUnits(const DebugNames 
_names) {
   return result;
 }
 
+std::optional
+DebugNamesDWARFIndex::IsForeignTypeUnit(const DebugNames::Entry ) const {
+  std::optional type_sig = entry.getForeignTUTypeSignature();
+  if (!type_sig.has_value())
+return std::nullopt;
+  auto dwp_sp = m_debug_info.GetDwpSymbolFile();
+  if (!dwp_sp) {
+// No .dwp file, we need to load the .dwo file.
+
+// Ask the entry for the skeleton compile unit offset and fetch the .dwo
+// file from it and get the type unit by signature from there. If we find
+// the type unit in the .dwo file, we don't need to check that the
+// DW_AT_dwo_name matches because each .dwo file can have its own type 
unit.
+std::optional unit_offset = entry.getForeignTUSkeletonCUOffset();
+if (!unit_offset)
+  return nullptr; // Return NULL, this is a type unit, but couldn't find 
it.
+DWARFUnit *cu =
+m_debug_info.GetUnitAtOffset(DIERef::Section::DebugInfo, *unit_offset);
+if (!cu)
+  return nullptr; // Return NULL, this is a type unit, but couldn't find 
it.

labath wrote:

I think this part could be moved into the common (dwp and non-dwp) code.

https://github.com/llvm/llvm-project/pull/87740
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [llvm] Add support for using foreign type units in .debug_names. (PR #87740)

[Pavel Labath via lldb-commits]

@@ -48,20 +60,84 @@ DebugNamesDWARFIndex::GetUnits(const DebugNames 
_names) {
   return result;
 }
 
+std::optional
+DebugNamesDWARFIndex::IsForeignTypeUnit(const DebugNames::Entry ) const {
+  std::optional type_sig = entry.getForeignTUTypeSignature();
+  if (!type_sig.has_value())
+return std::nullopt;
+  auto dwp_sp = m_debug_info.GetDwpSymbolFile();
+  if (!dwp_sp) {
+// No .dwp file, we need to load the .dwo file.
+
+// Ask the entry for the skeleton compile unit offset and fetch the .dwo
+// file from it and get the type unit by signature from there. If we find
+// the type unit in the .dwo file, we don't need to check that the
+// DW_AT_dwo_name matches because each .dwo file can have its own type 
unit.
+std::optional unit_offset = entry.getForeignTUSkeletonCUOffset();
+if (!unit_offset)
+  return nullptr; // Return NULL, this is a type unit, but couldn't find 
it.
+DWARFUnit *cu =
+m_debug_info.GetUnitAtOffset(DIERef::Section::DebugInfo, *unit_offset);
+if (!cu)
+  return nullptr; // Return NULL, this is a type unit, but couldn't find 
it.
+DWARFUnit _cu = cu->GetNonSkeletonUnit();
+// We don't need the check if the type unit matches the .dwo file if we 
have
+// a .dwo file (not a .dwp), so we can just return the value here.
+if (!dwo_cu.IsDWOUnit())
+  return nullptr; // We weren't able to load the .dwo file.
+return dwo_cu.GetSymbolFileDWARF().DebugInfo().GetTypeUnitForHash(
+*type_sig);
+  }
+  // We have a .dwp file, just get the type unit from there. We need to verify
+  // that the type unit that ended up in the final .dwp file is the right type
+  // unit. Type units have signatures which are the same across multiple .dwo
+  // files, but only one of those type units will end up in the .dwp file. The
+  // contents of type units for the same type can be different in different 
.dwo
+  // files, which means the DIE offsets might not be the same between two
+  // different type units. So we need to determine if this accelerator table
+  // matches the type unit that ended up in the .dwp file. If it doesn't match,
+  // then we need to ignore this accelerator table entry as the type unit that
+  // is in the .dwp file will have its own index. In order to determine if the
+  // type unit that ended up in a .dwp file matches this DebugNames::Entry, we
+  // need to find the skeleton compile unit for this entry.
+  DWARFTypeUnit *foreign_tu = 
dwp_sp->DebugInfo().GetTypeUnitForHash(*type_sig);
+  if (!foreign_tu)
+return nullptr; // Return NULL, this is a type unit, but couldn't find it.
+
+  std::optional cu_offset = entry.getForeignTUSkeletonCUOffset();
+  if (cu_offset) {
+DWARFUnit *cu = m_debug_info.GetUnitAtOffset(DIERef::DebugInfo, 
*cu_offset);
+if (cu) {

labath wrote:

and merged with this.

https://github.com/llvm/llvm-project/pull/87740
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb-dap] Added "port" property to vscode "attach" command. (PR #91570)

[Pavel Labath via lldb-commits]

@@ -0,0 +1,202 @@
+"""
+Test lldb-dap "port" configuration to "attach" request
+"""
+
+
+import dap_server
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+from lldbsuite.test import lldbplatformutil
+import lldbdap_testcase
+import os
+import shutil
+import subprocess
+import tempfile
+import threading
+import sys
+import socket
+import select
+
+
+# A class representing a pipe for communicating with debug server.
+# This class includes menthods to open the pipe and read the port number from 
it.
+class Pipe(object):
+def __init__(self, prefix):
+self.name = os.path.join(prefix, "stub_port_number")
+os.mkfifo(self.name)
+self._fd = os.open(self.name, os.O_RDONLY | os.O_NONBLOCK)
+
+def finish_connection(self, timeout):
+pass
+
+def read(self, size, timeout):
+(readers, _, _) = select.select([self._fd], [], [], timeout)
+if self._fd not in readers:
+raise TimeoutError
+return os.read(self._fd, size)
+
+def close(self):
+os.close(self._fd)
+
+
+class TestDAP_attachByPortNum(lldbdap_testcase.DAPTestCaseBase):
+default_timeout = 20
+
+def set_and_hit_breakpoint(self, continueToExit=True):
+source = "main.c"
+main_source_path = os.path.join(os.getcwd(), source)
+breakpoint1_line = line_number(main_source_path, "// breakpoint 1")
+lines = [breakpoint1_line]
+# Set breakpoint in the thread function so we can step the threads
+breakpoint_ids = self.set_source_breakpoints(main_source_path, lines)
+self.assertEqual(
+len(breakpoint_ids), len(lines), "expect correct number of 
breakpoints"
+)
+self.continue_to_breakpoints(breakpoint_ids)
+if continueToExit:
+self.continue_to_exit()
+
+def get_debug_server_command_line_args(self):
+args = []
+if lldbplatformutil.getPlatform() == "linux":
+args = ["gdbserver"]
+elif lldbplatformutil.getPlatform() == "macosx":
+args = ["--listen"]
+if lldb.remote_platform:
+args += ["*:0"]
+else:
+args += ["localhost:0"]
+return args
+
+def get_debug_server_pipe(self):
+pipe = Pipe(self.getBuildDir())
+self.addTearDownHook(lambda: pipe.close())
+pipe.finish_connection(self.default_timeout)
+return pipe
+
+@skipIfWindows
+@skipIfNetBSD
+def test_by_port(self):
+"""
+Tests attaching to a process by port.
+"""
+self.build_and_create_debug_adaptor()
+program = self.getBuildArtifact("a.out")
+
+debug_server_tool = self.getBuiltinDebugServerTool()
+
+pipe = self.get_debug_server_pipe()
+args = self.get_debug_server_command_line_args()
+args += [program]
+args += ["--named-pipe", pipe.name]
+
+self.process = self.spawnSubprocess(
+debug_server_tool, args, install_remote=False
+)
+
+# Read the port number from the debug server pipe.
+port = pipe.read(10, self.default_timeout)
+# Trim null byte, convert to int
+port = int(port[:-1])
+self.assertIsNotNone(
+port, " Failed to read the port number from debug server pipe"
+)
+
+self.attach(program=program, gdbRemotePort=port, sourceInitFile=True)
+self.set_and_hit_breakpoint(continueToExit=True)
+self.process.terminate()
+
+@skipIfWindows
+@skipIfNetBSD
+def test_by_port_and_pid(self):
+"""
+Tests attaching to a process by process ID and port number.
+"""
+self.build_and_create_debug_adaptor()
+program = self.getBuildArtifact("a.out")
+
+debug_server_tool = self.getBuiltinDebugServerTool()
+pipe = self.get_debug_server_pipe()
+args = self.get_debug_server_command_line_args()
+args += [program]
+args += ["--named-pipe", pipe.name]
+
+self.process = self.spawnSubprocess(
+debug_server_tool, args, install_remote=False
+)
+
+# Read the port number from the debug server pipe.
+port = pipe.read(10, self.default_timeout)
+# Trim null byte, convert to int
+port = int(port[:-1])
+self.assertIsNotNone(
+port, " Failed to read the port number from debug server pipe"
+)
+
+pid = self.process.pid
+
+response = self.attach(
+program=program,
+pid=pid,
+gdbRemotePort=port,
+sourceInitFile=True,
+expectFailure=True,
+)
+if not (response and response["success"]):
+self.assertFalse(
+response["success"], "The user can't specify both pid and port"
+)
+self.process.terminate()
+
+@skipIfWindows
+@skipIfNetBSD
+def 

[Lldb-commits] [lldb] [lldb-dap] Added "port" property to vscode "attach" command. (PR #91570)

[Pavel Labath via lldb-commits]

@@ -0,0 +1,202 @@
+"""
+Test lldb-dap "port" configuration to "attach" request
+"""
+
+
+import dap_server
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+from lldbsuite.test import lldbplatformutil
+import lldbdap_testcase
+import os
+import shutil
+import subprocess
+import tempfile
+import threading
+import sys
+import socket
+import select
+
+
+# A class representing a pipe for communicating with debug server.
+# This class includes menthods to open the pipe and read the port number from 
it.
+class Pipe(object):
+def __init__(self, prefix):
+self.name = os.path.join(prefix, "stub_port_number")
+os.mkfifo(self.name)
+self._fd = os.open(self.name, os.O_RDONLY | os.O_NONBLOCK)
+
+def finish_connection(self, timeout):
+pass
+
+def read(self, size, timeout):
+(readers, _, _) = select.select([self._fd], [], [], timeout)
+if self._fd not in readers:
+raise TimeoutError
+return os.read(self._fd, size)
+
+def close(self):
+os.close(self._fd)
+
+
+class TestDAP_attachByPortNum(lldbdap_testcase.DAPTestCaseBase):
+default_timeout = 20
+
+def set_and_hit_breakpoint(self, continueToExit=True):
+source = "main.c"
+main_source_path = os.path.join(os.getcwd(), source)
+breakpoint1_line = line_number(main_source_path, "// breakpoint 1")
+lines = [breakpoint1_line]
+# Set breakpoint in the thread function so we can step the threads
+breakpoint_ids = self.set_source_breakpoints(main_source_path, lines)
+self.assertEqual(
+len(breakpoint_ids), len(lines), "expect correct number of 
breakpoints"
+)
+self.continue_to_breakpoints(breakpoint_ids)
+if continueToExit:
+self.continue_to_exit()
+
+def get_debug_server_command_line_args(self):
+args = []
+if lldbplatformutil.getPlatform() == "linux":
+args = ["gdbserver"]
+elif lldbplatformutil.getPlatform() == "macosx":
+args = ["--listen"]
+if lldb.remote_platform:
+args += ["*:0"]
+else:
+args += ["localhost:0"]
+return args
+
+def get_debug_server_pipe(self):
+pipe = Pipe(self.getBuildDir())
+self.addTearDownHook(lambda: pipe.close())
+pipe.finish_connection(self.default_timeout)
+return pipe
+
+@skipIfWindows
+@skipIfNetBSD
+def test_by_port(self):
+"""
+Tests attaching to a process by port.
+"""
+self.build_and_create_debug_adaptor()
+program = self.getBuildArtifact("a.out")
+
+debug_server_tool = self.getBuiltinDebugServerTool()
+
+pipe = self.get_debug_server_pipe()
+args = self.get_debug_server_command_line_args()
+args += [program]
+args += ["--named-pipe", pipe.name]
+
+self.process = self.spawnSubprocess(
+debug_server_tool, args, install_remote=False
+)
+
+# Read the port number from the debug server pipe.
+port = pipe.read(10, self.default_timeout)
+# Trim null byte, convert to int
+port = int(port[:-1])
+self.assertIsNotNone(
+port, " Failed to read the port number from debug server pipe"
+)
+
+self.attach(program=program, gdbRemotePort=port, sourceInitFile=True)
+self.set_and_hit_breakpoint(continueToExit=True)
+self.process.terminate()
+
+@skipIfWindows
+@skipIfNetBSD
+def test_by_port_and_pid(self):
+"""
+Tests attaching to a process by process ID and port number.
+"""
+self.build_and_create_debug_adaptor()
+program = self.getBuildArtifact("a.out")
+
+debug_server_tool = self.getBuiltinDebugServerTool()
+pipe = self.get_debug_server_pipe()
+args = self.get_debug_server_command_line_args()
+args += [program]
+args += ["--named-pipe", pipe.name]
+
+self.process = self.spawnSubprocess(
+debug_server_tool, args, install_remote=False
+)
+
+# Read the port number from the debug server pipe.
+port = pipe.read(10, self.default_timeout)
+# Trim null byte, convert to int
+port = int(port[:-1])

labath wrote:

I would really like to remove it as I don't think it contributes in any way to 
the test (and it's always better to have less moving parts). Like, even if 
lldb-dap somehow still ends up connecting to the lldb-server port (in addition 
to sending the error message), we would never notice it, as the test does not 
assert the lldb-server state in any way.

If you really wanted to be thorough, a better check would be to open a port (in 
python) and then verify we do *not* attempt to connect to that port.

https://github.com/llvm/llvm-project/pull/91570

[Lldb-commits] [lldb] [lldb-dap] Added "port" property to vscode "attach" command. (PR #91570)

[Pavel Labath via lldb-commits]

labath wrote:

> The code looks good, a few nits in comments.
> 
> @labath: did we resolve the port race condition testing issue? We really 
> don't want this to be a flaky test on overloaded systems. It would be nice to 
> make sure this is solid before getting this in.

The current version of the code should be fine. We just need to sort out the 
code placement issue.

https://github.com/llvm/llvm-project/pull/91570
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb-dap] Added "port" property to vscode "attach" command. (PR #91570)

[Pavel Labath via lldb-commits]

@@ -0,0 +1,202 @@
+"""
+Test lldb-dap "port" configuration to "attach" request
+"""
+
+
+import dap_server
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+from lldbsuite.test import lldbplatformutil
+import lldbdap_testcase
+import os
+import shutil
+import subprocess
+import tempfile
+import threading
+import sys
+import socket
+import select
+
+
+# A class representing a pipe for communicating with debug server.
+# This class includes menthods to open the pipe and read the port number from 
it.
+class Pipe(object):
+def __init__(self, prefix):
+self.name = os.path.join(prefix, "stub_port_number")
+os.mkfifo(self.name)
+self._fd = os.open(self.name, os.O_RDONLY | os.O_NONBLOCK)
+
+def finish_connection(self, timeout):
+pass
+
+def read(self, size, timeout):
+(readers, _, _) = select.select([self._fd], [], [], timeout)
+if self._fd not in readers:
+raise TimeoutError
+return os.read(self._fd, size)
+
+def close(self):
+os.close(self._fd)

labath wrote:

I'm not sure I follow you're reasoning (please elaborate), but I'm pretty sure 
I don't agree with your conclusion.

It is true that the Pipe class is defined differently on different (host) 
platforms. It's also true that it's interface is more complicated than if it 
was designed to be used exclusively on (e.g.) posix system. However, I don't 
think any of this is a reason to not make it common code. However, I don't 
think any of this is a reason to *not* make it common code. If anything, I 
would say it's the opposite. Abstracting platform-specific behavior behind 
common interfaces is the best thing we can do in these circumstances. Sometimes 
that means the common interface will be a bit more complicated but that's just 
the reality we live in.

In this case, somebody (disclaimer: it was me, several years ago) already 
created a common interface and implemented it on both platforms. I don't see a 
reason to not make use of that. There is no fundamental reason why this test of 
yours cannot work on windows. If someday, someone cared enough to try to make 
it work, he would have to (re)implement the pipe abstraction to make it run. 
Why make him do that if we can make use of an abstraction that's already 
available?

I'm sorry, but I just don't understand what kind of confusion you're referring 
to. I don't know if it helps, but I can certainly imagine changing the class 
name to something less generic (e.g. `HostPipe` ?)

https://github.com/llvm/llvm-project/pull/91570
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb] Skip declaration DIEs in the debug_names index (PR #94744)

[Pavel Labath via lldb-commits]

labath wrote:

I should add that I'm pretty sure the index entries pointing to signature 
declaration DIEs were not the cause of crashes in @ZequanWu CLs, after creating 
the test case (what I think is the worst possible situation) and stepping 
through the code, I realized that these DIEs would get filtered out anyway on 
line DWARFIndex.cpp:126 (because `GetDWARFDeclContext` does not work on 
DW_AT_signature DIEs). Nonetheless, I think that this change makes sense 
because the fact that `GetDWARFDeclContext` is a bug that's probably preventing 
the correct lookup of types in some type unit scenarios. The check may very 
well become load-bearing if that function is fixed.

https://github.com/llvm/llvm-project/pull/94744
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb] Skip declaration DIEs in the debug_names index (PR #94744)

[Pavel Labath via lldb-commits]

https://github.com/labath created 
https://github.com/llvm/llvm-project/pull/94744

This makes sure we try to process declaration DIEs that are erroneously present 
in the index. Until bd5c6367bd7, clang was emitting index entries for 
declaration DIEs with DW_AT_signature attributes. This makes sure to avoid 
returning those DIEs as the definitions of a type, but also makes sure to pass 
through DIEs referring to static constexpr member variables, which is a 
(probably nonconforming) extension used by dsymutil.

It adds test cases for both of the scenarios. It is essentially a recommit of 
#91808.

>From 506eeb483a81146aa4e7798ad6d325da3b83a8de Mon Sep 17 00:00:00 2001
From: Pavel Labath 
Date: Fri, 7 Jun 2024 12:32:37 +0200
Subject: [PATCH] [lldb] Skip declaration DIEs in the debug_names index

This makes sure we try to process declaration DIEs that are erroneously
present in the index. Until bd5c6367bd7, clang was emitting index
entries for declaration DIEs with DW_AT_signature attributes. This makes
sure to avoid returning those DIEs as the definitions of a type, but
also makes sure to pass through DIEs referring to static constexpr
member variables, which is a (probably nonconforming) extension used by
dsymutil.

It adds test cases for both of the scenarios. It is essentially a
recommit of #91808.
---
 .../SymbolFile/DWARF/DebugNamesDWARFIndex.cpp |   5 +
 .../DWARF/x86/debug-names-signature.s | 265 ++
 .../x86/debug-names-static-constexpr-member.s | 169 +++
 3 files changed, 439 insertions(+)
 create mode 100644 lldb/test/Shell/SymbolFile/DWARF/x86/debug-names-signature.s
 create mode 100644 
lldb/test/Shell/SymbolFile/DWARF/x86/debug-names-static-constexpr-member.s

diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.cpp 
b/lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.cpp
index 90e42be7202d8..1d17f20670eed 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.cpp
@@ -85,6 +85,11 @@ bool DebugNamesDWARFIndex::ProcessEntry(
   DWARFDIE die = GetDIE(entry);
   if (!die)
 return true;
+  // Clang used to erroneously emit index entries for declaration DIEs in case
+  // when the definition is in a type unit (llvm.org/pr77696).
+  if (die.IsStructUnionOrClass() &&
+  die.GetAttributeValueAsUnsigned(DW_AT_declaration, 0))
+return true;
   return callback(die);
 }
 
diff --git a/lldb/test/Shell/SymbolFile/DWARF/x86/debug-names-signature.s 
b/lldb/test/Shell/SymbolFile/DWARF/x86/debug-names-signature.s
new file mode 100644
index 0..7b845a72bbed4
--- /dev/null
+++ b/lldb/test/Shell/SymbolFile/DWARF/x86/debug-names-signature.s
@@ -0,0 +1,265 @@
+## Test that we can correctly complete types even if the debug_names index
+## contains entries referring to declaration dies (clang emitted entries like
+## that until bd5c6367bd7).
+##
+## This test consists of two compile units and one type unit. CU1 has the
+## definition of a variable, but only a forward-declaration of its type. When
+## attempting to find a definition, the debug_names lookup will return the DIE
+## in CU0, which is also a forward-declaration (with a reference to a type
+## unit). LLDB needs to find the definition of the type within the type unit.
+
+# RUN: llvm-mc -triple=x86_64-pc-linux -filetype=obj %s > %t
+# RUN: %lldb %t -o "target variable s" -o exit | FileCheck %s
+
+# CHECK:  (lldb) target variable s
+# CHECK-NEXT: (Struct) s = (member = 47)
+
+.data
+.p2align2, 0x0
+.long   0
+s:
+.long   47  # 0x2f
+
+.section.debug_abbrev,"",@progbits
+.byte   1   # Abbreviation Code
+.byte   65  # DW_TAG_type_unit
+.byte   1   # DW_CHILDREN_yes
+.byte   19  # DW_AT_language
+.byte   5   # DW_FORM_data2
+.byte   0   # EOM(1)
+.byte   0   # EOM(2)
+.byte   2   # Abbreviation Code
+.byte   19  # DW_TAG_structure_type
+.byte   1   # DW_CHILDREN_yes
+.byte   54  # DW_AT_calling_convention
+.byte   11  # DW_FORM_data1
+.byte   3   # DW_AT_name
+.byte   14  # DW_FORM_strp
+.byte   11  # DW_AT_byte_size
+.byte   11  # DW_FORM_data1
+.byte   0   # EOM(1)
+.byte   0   # EOM(2)
+.byte   3   # Abbreviation Code
+.byte   13 

[Lldb-commits] [lldb] [lldb] Fix TestModuleLoadedNotifys API test to work correctly on most of Linux targets (PR #94672)

[Pavel Labath via lldb-commits]

@@ -118,6 +118,6 @@ def test_launch_notifications(self):
 # On Linux we get events for ld.so, [vdso], the binary and then all 
libraries.
 
 avg_solibs_added_per_event = round(
-float(total_solibs_added) / float(total_modules_added_events)
+10.0 * float(total_solibs_added) / 
float(total_modules_added_events)

labath wrote:

You can't really call it "number of libraries per event" anymore if you 
multiply by ten. It's more like number of decilibraries/event :P. Maybe you 
could just remove the round call, and compare to 1.0, but I'm wondering if this 
is even the right metric for the test. Since what we're trying to check is that 
we don't send these notifications one by one, I think some check like "number 
of events with more than one module" or "number of total events" (or both) 
would be better. @jasonmolenda, what do you think?

Also, this test is very unhermetic in the sense that it depends on the 
environment to provide enough shared libraries to measure. In the extreme case 
we could have a totally statically linked binary and zero shared libraries. So, 
another way to make this test be more resilient is to introduce a couple of 
shared libraries of our own, so that we can be sure there is something to 
measure. You could copy the pattern from TestLoadUnload.py to link a bunch of 
shared libraries to this binary.

https://github.com/llvm/llvm-project/pull/94672
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb] Split ValueObject::CreateChildAtIndex into two functions (PR #94455)

[Pavel Labath via lldb-commits]

https://github.com/labath closed https://github.com/llvm/llvm-project/pull/94455
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] Reapply [lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. (PR #92328)

[Pavel Labath via lldb-commits]

@@ -2345,11 +2345,6 @@ bool DWARFASTParserClang::CompleteTypeFromDWARF(const 
DWARFDIE ,
 
   if (!die)
 return false;
-  ParsedDWARFTypeAttributes attrs(die);

labath wrote:

In case anyone's wondering, Putting this check back in would prevent the crash 
from 7fdbc30b445286f03203e16d0be067c25c6f0df0, but it would not actually make 
the code correct -- lldb would now just say the class has no definition instead 
of crashing.

https://github.com/llvm/llvm-project/pull/92328
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] Improve performance of .debug_names lookups when DW_IDX_parent attributes are used (PR #91808)

[Pavel Labath via lldb-commits]

labath wrote:

PSA: I've reverted Zequan's patch due to other bugs, so this change is now 
uncommitted again. Since this could go in as a separate change, I'm going to 
recreate a patch for it tomorrow (running out of time today), including the fix 
from #94400, if someone doesn't beat me to it. I'm sorry about all the back and 
forth.

https://github.com/llvm/llvm-project/pull/91808
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] 7fdbc30 - Revert "[lldb][DebugNames] Only skip processing of DW_AT_declarations for class/union types"

[Pavel Labath via lldb-commits]

Author: Pavel Labath
Date: 2024-06-06T16:08:58Z
New Revision: 7fdbc30b445286f03203e16d0be067c25c6f0df0

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

LOG: Revert "[lldb][DebugNames] Only skip processing of DW_AT_declarations for 
class/union types"

and two follow-up commits. The reason is the crash we've discovered when
processing -gsimple-template-names binaries. I'm committing a minimal
reproducer as a separate patch.

This reverts the following commits:
- 51dd4eaaa29683c16151f5168e7f8645acbd6e6c (#92328)
- 3d9d48523977af3590f7dd0edfd258454cb9e9cf (#93839)
- afe6ab7586f7078cc410f6162bd9851e48e2a286 (#94400)

Added: 


Modified: 
lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParser.h
lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h
lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.cpp
lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.h
lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDwo.cpp
lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDwo.h
lldb/source/Plugins/SymbolFile/DWARF/UniqueDWARFASTType.cpp
lldb/source/Plugins/SymbolFile/DWARF/UniqueDWARFASTType.h

Removed: 
lldb/test/Shell/SymbolFile/DWARF/delayed-definition-die-searching.test



diff  --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParser.h 
b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParser.h
index e144cf0f9bd94..66db396279e06 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParser.h
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParser.h
@@ -60,8 +60,6 @@ class DWARFASTParser {
 
   virtual ConstString GetDIEClassTemplateParams(const DWARFDIE ) = 0;
 
-  virtual lldb_private::Type *FindDefinitionTypeForDIE(const DWARFDIE ) = 
0;
-
   static std::optional
   ParseChildArrayInfo(const DWARFDIE _die,
   const ExecutionContext *exe_ctx = nullptr);

diff  --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp 
b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
index 7d7e835c3d732..579a538af3634 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
@@ -154,26 +154,6 @@ static bool TagIsRecordType(dw_tag_t tag) {
   }
 }
 
-static bool IsForwardDeclaration(const DWARFDIE ,
- const ParsedDWARFTypeAttributes ,
- LanguageType cu_language) {
-  if (attrs.is_forward_declaration)
-return true;
-
-  // Work around an issue with clang at the moment where forward
-  // declarations for objective C classes are emitted as:
-  //  DW_TAG_structure_type [2]
-  //  DW_AT_name( "ForwardObjcClass" )
-  //  DW_AT_byte_size( 0x00 )
-  //  DW_AT_decl_file( "..." )
-  //  DW_AT_decl_line( 1 )
-  //
-  // Note that there is no DW_AT_declaration and there are no children,
-  // and the byte size is zero.
-  return attrs.byte_size && *attrs.byte_size == 0 && attrs.name &&
- !die.HasChildren() && cu_language == eLanguageTypeObjC;
-}
-
 TypeSP DWARFASTParserClang::ParseTypeFromClangModule(const SymbolContext ,
  const DWARFDIE ,
  Log *log) {
@@ -269,9 +249,11 @@ static void ForcefullyCompleteType(CompilerType type) {
 /// This function serves a similar purpose as RequireCompleteType above, but it
 /// avoids completing the type if it is not immediately necessary. It only
 /// ensures we _can_ complete the type later.
-void DWARFASTParserClang::PrepareContextToReceiveMembers(
-clang::DeclContext *decl_ctx, const DWARFDIE _ctx_die,
-const DWARFDIE , const char *type_name_cstr) {
+static void PrepareContextToReceiveMembers(TypeSystemClang ,
+   ClangASTImporter _importer,
+   clang::DeclContext *decl_ctx,
+   DWARFDIE die,
+   const char *type_name_cstr) {
   auto *tag_decl_ctx = clang::dyn_cast(decl_ctx);
   if (!tag_decl_ctx)
 return; // Non-tag context are always ready.
@@ -286,8 +268,7 @@ void DWARFASTParserClang::PrepareContextToReceiveMembers(
   // gmodules case), we can complete the type by doing a full import.
 
   // If this type was not imported from an external AST, there's nothing to do.
-  CompilerType type = m_ast.GetTypeForDecl(tag_decl_ctx);
-  ClangASTImporter _importer = GetClangASTImporter();
+  CompilerType type = ast.GetTypeForDecl(tag_decl_ctx);
   if (type && 

[Lldb-commits] [lldb] de3f1b6 - [lldb] Test case for the bug in #92328

[Pavel Labath via lldb-commits]

Author: Pavel Labath
Date: 2024-06-06T16:10:22Z
New Revision: de3f1b6d68ab8a0e827db84b328803857a4f60df

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

LOG: [lldb] Test case for the bug in #92328

Added: 
lldb/test/Shell/SymbolFile/DWARF/x86/simple-template-names-context.cpp

Modified: 


Removed: 




diff  --git 
a/lldb/test/Shell/SymbolFile/DWARF/x86/simple-template-names-context.cpp 
b/lldb/test/Shell/SymbolFile/DWARF/x86/simple-template-names-context.cpp
new file mode 100644
index 0..a8a4d3b8fbd5f
--- /dev/null
+++ b/lldb/test/Shell/SymbolFile/DWARF/x86/simple-template-names-context.cpp
@@ -0,0 +1,44 @@
+// Test that we can correctly resolve forward declared types when they only
+// 
diff er in the template arguments of the surrounding context. The reproducer
+// is sensitive to the order of declarations, so we test in both directions.
+
+// REQUIRES: lld
+
+// RUN: %clang --target=x86_64-pc-linux -c %s -o %t-a.o -g 
-gsimple-template-names -DFILE_A
+// RUN: %clang --target=x86_64-pc-linux -c %s -o %t-b.o -g 
-gsimple-template-names -DFILE_B
+// RUN: ld.lld %t-a.o %t-b.o -o %t
+// RUN: %lldb %t -o "target variable --ptr-depth 1 --show-types both_a both_b" 
-o exit | FileCheck %s
+
+// CHECK: (lldb) target variable
+// CHECK-NEXT: (ReferencesBoth<'A'>) both_a = {
+// CHECK-NEXT:   (Outer<'A'>::Inner *) a = 0x{{[0-9A-Fa-f]*}} {}
+// CHECK-NEXT:   (Outer<'A'>::Inner *) b = 0x{{[0-9A-Fa-f]*}} {}
+// CHECK-NEXT: }
+// CHECK-NEXT: (ReferencesBoth<'B'>) both_b = {
+// CHECK-NEXT:   (Outer<'A'>::Inner *) a = 0x{{[0-9A-Fa-f]*}} {}
+// CHECK-NEXT:   (Outer<'B'>::Inner *) b = 0x{{[0-9A-Fa-f]*}} {}
+// CHECK-NEXT: }
+
+template
+struct Outer {
+  struct Inner {};
+};
+
+template
+struct ReferencesBoth {
+  Outer<'A'>::Inner *a;
+  Outer<'B'>::Inner *b;
+};
+
+#ifdef FILE_A
+Outer<'A'>::Inner inner_a;
+extern Outer<'B'>::Inner inner_b;
+
+ReferencesBoth<'A'> both_a{_a, _b};
+
+#else
+extern Outer<'A'>::Inner inner_a;
+Outer<'B'>::Inner inner_b;
+
+ReferencesBoth<'B'> both_b{_a, _b};
+#endif



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

[Lldb-commits] [lldb] [lldb] Split ValueObject::CreateChildAtIndex into two functions (PR #94455)

[Pavel Labath via lldb-commits]

@@ -488,66 +488,85 @@ void ValueObject::SetNumChildren(uint32_t num_children) {
   m_children.SetChildrenCount(num_children);
 }
 
-ValueObject *ValueObject::CreateChildAtIndex(size_t idx,
- bool synthetic_array_member,
- int32_t synthetic_index) {
-  ValueObject *valobj = nullptr;
-
+ValueObject *ValueObject::CreateChildAtIndex(size_t idx) {
   bool omit_empty_base_classes = true;
-  bool ignore_array_bounds = synthetic_array_member;
-  std::string child_name_str;
+  bool ignore_array_bounds = false;
+  std::string child_name;
   uint32_t child_byte_size = 0;
   int32_t child_byte_offset = 0;
   uint32_t child_bitfield_bit_size = 0;
   uint32_t child_bitfield_bit_offset = 0;
   bool child_is_base_class = false;
   bool child_is_deref_of_parent = false;
   uint64_t language_flags = 0;
-
-  const bool transparent_pointers = !synthetic_array_member;
+  const bool transparent_pointers = true;
 
   ExecutionContext exe_ctx(GetExecutionContextRef());
 
   auto child_compiler_type_or_err =
   GetCompilerType().GetChildCompilerTypeAtIndex(
   _ctx, idx, transparent_pointers, omit_empty_base_classes,
-  ignore_array_bounds, child_name_str, child_byte_size,
-  child_byte_offset, child_bitfield_bit_size, 
child_bitfield_bit_offset,
+  ignore_array_bounds, child_name, child_byte_size, child_byte_offset,
+  child_bitfield_bit_size, child_bitfield_bit_offset,
   child_is_base_class, child_is_deref_of_parent, this, language_flags);
-  CompilerType child_compiler_type;
-  if (!child_compiler_type_or_err)
+  if (!child_compiler_type_or_err || !child_compiler_type_or_err->IsValid()) {
 LLDB_LOG_ERROR(GetLog(LLDBLog::Types),
child_compiler_type_or_err.takeError(),
"could not find child: {0}");
-  else
-child_compiler_type = *child_compiler_type_or_err;
+return nullptr;
+  }
+
+  return new ValueObjectChild(
+  *this, *child_compiler_type_or_err, ConstString(child_name),
+  child_byte_size, child_byte_offset, child_bitfield_bit_size,
+  child_bitfield_bit_offset, child_is_base_class, child_is_deref_of_parent,
+  eAddressTypeInvalid, language_flags);
+}
+
+ValueObject *ValueObject::CreateSyntheticArrayMember(size_t idx) {
+  bool omit_empty_base_classes = true;
+  bool ignore_array_bounds = true;
+  std::string child_name;
+  uint32_t child_byte_size = 0;
+  int32_t child_byte_offset = 0;
+  uint32_t child_bitfield_bit_size = 0;
+  uint32_t child_bitfield_bit_offset = 0;
+  bool child_is_base_class = false;
+  bool child_is_deref_of_parent = false;
+  uint64_t language_flags = 0;
+  const bool transparent_pointers = false;
 
-  if (child_compiler_type) {
-if (synthetic_index)
-  child_byte_offset += child_byte_size * synthetic_index;
+  ExecutionContext exe_ctx(GetExecutionContextRef());
 
-ConstString child_name;
-if (!child_name_str.empty())
-  child_name.SetCString(child_name_str.c_str());

labath wrote:

It seems that one side effect of "simplifying" this is that the name of 
anonymous struct members changes from nullptr/None to "". None of lldb tests 
broke but this broke some (rather dodgy) downstream code of ours. I would be 
inclined to keep this change, as I don't think we can make a promise (and keep 
it) to preserve details like this.

https://github.com/llvm/llvm-project/pull/94455
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb] Split ValueObject::CreateChildAtIndex into two functions (PR #94455)

[Pavel Labath via lldb-commits]

https://github.com/labath updated 
https://github.com/llvm/llvm-project/pull/94455

>From d3d666886d6397bb031a3ad88f147fb9b1e2b3ed Mon Sep 17 00:00:00 2001
From: Pavel Labath 
Date: Wed, 5 Jun 2024 10:21:10 +
Subject: [PATCH] [lldb] Split ValueObject::CreateChildAtIndex into two
 functions

The the function is doing two fairly different things, depending on how
it is called. While this allows for some code reuse, it also makes it
hard to override it correctly. Possibly for this reason
ValueObjectSynthetic overerides GetChildAtIndex instead, which forces it
to reimplement some of its functionality, most notably caching of
generated children.

Splitting this up makes it easier to move the caching to a common place
(and hopefully makes the code easier to follow in general).
---
 lldb/include/lldb/Core/ValueObject.h  |   9 +-
 .../lldb/Core/ValueObjectConstResult.h|  10 +-
 .../lldb/Core/ValueObjectConstResultCast.h|  10 +-
 .../lldb/Core/ValueObjectConstResultChild.h   |  10 +-
 .../lldb/Core/ValueObjectConstResultImpl.h|   4 +-
 lldb/include/lldb/Core/ValueObjectRegister.h  |   8 +-
 lldb/include/lldb/Core/ValueObjectVTable.h|   8 +-
 lldb/source/Core/ValueObject.cpp  |  91 --
 lldb/source/Core/ValueObjectConstResult.cpp   |   6 -
 .../Core/ValueObjectConstResultCast.cpp   |   6 -
 .../Core/ValueObjectConstResultChild.cpp  |   6 -
 .../Core/ValueObjectConstResultImpl.cpp   | 115 --
 lldb/source/Core/ValueObjectRegister.cpp  |  14 +--
 lldb/source/Core/ValueObjectVTable.cpp|   6 +-
 14 files changed, 178 insertions(+), 125 deletions(-)

diff --git a/lldb/include/lldb/Core/ValueObject.h 
b/lldb/include/lldb/Core/ValueObject.h
index e7e35e2b2bffc..1e8c7c9c00536 100644
--- a/lldb/include/lldb/Core/ValueObject.h
+++ b/lldb/include/lldb/Core/ValueObject.h
@@ -959,9 +959,12 @@ class ValueObject {
   /// Should only be called by ValueObject::GetChildAtIndex().
   ///
   /// \return A ValueObject managed by this ValueObject's manager.
-  virtual ValueObject *CreateChildAtIndex(size_t idx,
-  bool synthetic_array_member,
-  int32_t synthetic_index);
+  virtual ValueObject *CreateChildAtIndex(size_t idx);
+
+  /// Should only be called by ValueObject::GetSyntheticArrayMember().
+  ///
+  /// \return A ValueObject managed by this ValueObject's manager.
+  virtual ValueObject *CreateSyntheticArrayMember(size_t idx);
 
   /// Should only be called by ValueObject::GetNumChildren().
   virtual llvm::Expected
diff --git a/lldb/include/lldb/Core/ValueObjectConstResult.h 
b/lldb/include/lldb/Core/ValueObjectConstResult.h
index 37dc0867f26c9..d3b3362bd0e9e 100644
--- a/lldb/include/lldb/Core/ValueObjectConstResult.h
+++ b/lldb/include/lldb/Core/ValueObjectConstResult.h
@@ -79,9 +79,6 @@ class ValueObjectConstResult : public ValueObject {
 
   lldb::ValueObjectSP Dereference(Status ) override;
 
-  ValueObject *CreateChildAtIndex(size_t idx, bool synthetic_array_member,
-  int32_t synthetic_index) override;
-
   lldb::ValueObjectSP GetSyntheticChildAtOffset(
   uint32_t offset, const CompilerType , bool can_create,
   ConstString name_const_str = ConstString()) override;
@@ -151,6 +148,13 @@ class ValueObjectConstResult : public ValueObject {
   ValueObjectConstResult(ExecutionContextScope *exe_scope,
  ValueObjectManager , const Status );
 
+  ValueObject *CreateChildAtIndex(size_t idx) override {
+return m_impl.CreateChildAtIndex(idx);
+  }
+  ValueObject *CreateSyntheticArrayMember(size_t idx) override {
+return m_impl.CreateSyntheticArrayMember(idx);
+  }
+
   ValueObjectConstResult(const ValueObjectConstResult &) = delete;
   const ValueObjectConstResult &
   operator=(const ValueObjectConstResult &) = delete;
diff --git a/lldb/include/lldb/Core/ValueObjectConstResultCast.h 
b/lldb/include/lldb/Core/ValueObjectConstResultCast.h
index efcbe0dc6a0bd..911a08363b393 100644
--- a/lldb/include/lldb/Core/ValueObjectConstResultCast.h
+++ b/lldb/include/lldb/Core/ValueObjectConstResultCast.h
@@ -35,9 +35,6 @@ class ValueObjectConstResultCast : public ValueObjectCast {
 
   lldb::ValueObjectSP Dereference(Status ) override;
 
-  ValueObject *CreateChildAtIndex(size_t idx, bool synthetic_array_member,
-  int32_t synthetic_index) override;
-
   virtual CompilerType GetCompilerType() {
 return ValueObjectCast::GetCompilerType();
   }
@@ -61,6 +58,13 @@ class ValueObjectConstResultCast : public ValueObjectCast {
   friend class ValueObjectConstResult;
   friend class ValueObjectConstResultImpl;
 
+  ValueObject *CreateChildAtIndex(size_t idx) override {
+return m_impl.CreateChildAtIndex(idx);
+  }
+  ValueObject *CreateSyntheticArrayMember(size_t idx) override {
+return m_impl.CreateSyntheticArrayMember(idx);
+  }
+
   

[Lldb-commits] [lldb] [lldb] Split ValueObject::CreateChildAtIndex into two functions (PR #94455)

[Pavel Labath via lldb-commits]

https://github.com/labath created 
https://github.com/llvm/llvm-project/pull/94455

The the function is doing two fairly different things, depending on how it is 
called. While this allows for some code reuse, it also makes it hard to 
override it correctly. Possibly for this reason ValueObjectSynthetic overerides 
GetChildAtIndex instead, which forces it to reimplement some of its 
functionality, most notably caching of generated children.

Splitting this up makes it easier to move the caching to a common place (and 
hopefully makes the code easier to follow in general).

>From 130389992ae560366893637f70132d75e31aa04b Mon Sep 17 00:00:00 2001
From: Pavel Labath 
Date: Wed, 5 Jun 2024 10:21:10 +
Subject: [PATCH] [lldb] Split ValueObject::CreateChildAtIndex into two
 functions

The the function is doing two fairly different things, depending on how
it is called. While this allows for some code reuse, it also makes it
hard to override it correctly. Possibly for this reason
ValueObjectSynthetic overerides GetChildAtIndex instead, which forces it
to reimplement some of its functionality, most notably caching of
generated children.

Splitting this up makes it easier to move the caching to a common place
(and hopefully makes the code easier to follow in general).
---
 lldb/include/lldb/Core/ValueObject.h  |   9 +-
 .../lldb/Core/ValueObjectConstResult.h|  10 +-
 .../lldb/Core/ValueObjectConstResultCast.h|  10 +-
 .../lldb/Core/ValueObjectConstResultChild.h   |  10 +-
 .../lldb/Core/ValueObjectConstResultImpl.h|   4 +-
 lldb/include/lldb/Core/ValueObjectRegister.h  |   8 +-
 lldb/include/lldb/Core/ValueObjectVTable.h|   8 +-
 lldb/source/Core/ValueObject.cpp  |  85 -
 lldb/source/Core/ValueObjectConstResult.cpp   |   6 -
 .../Core/ValueObjectConstResultCast.cpp   |   6 -
 .../Core/ValueObjectConstResultChild.cpp  |   6 -
 .../Core/ValueObjectConstResultImpl.cpp   | 115 --
 lldb/source/Core/ValueObjectRegister.cpp  |  14 +--
 lldb/source/Core/ValueObjectVTable.cpp|   6 +-
 14 files changed, 175 insertions(+), 122 deletions(-)

diff --git a/lldb/include/lldb/Core/ValueObject.h 
b/lldb/include/lldb/Core/ValueObject.h
index e7e35e2b2bffc..1e8c7c9c00536 100644
--- a/lldb/include/lldb/Core/ValueObject.h
+++ b/lldb/include/lldb/Core/ValueObject.h
@@ -959,9 +959,12 @@ class ValueObject {
   /// Should only be called by ValueObject::GetChildAtIndex().
   ///
   /// \return A ValueObject managed by this ValueObject's manager.
-  virtual ValueObject *CreateChildAtIndex(size_t idx,
-  bool synthetic_array_member,
-  int32_t synthetic_index);
+  virtual ValueObject *CreateChildAtIndex(size_t idx);
+
+  /// Should only be called by ValueObject::GetSyntheticArrayMember().
+  ///
+  /// \return A ValueObject managed by this ValueObject's manager.
+  virtual ValueObject *CreateSyntheticArrayMember(size_t idx);
 
   /// Should only be called by ValueObject::GetNumChildren().
   virtual llvm::Expected
diff --git a/lldb/include/lldb/Core/ValueObjectConstResult.h 
b/lldb/include/lldb/Core/ValueObjectConstResult.h
index 37dc0867f26c9..d3b3362bd0e9e 100644
--- a/lldb/include/lldb/Core/ValueObjectConstResult.h
+++ b/lldb/include/lldb/Core/ValueObjectConstResult.h
@@ -79,9 +79,6 @@ class ValueObjectConstResult : public ValueObject {
 
   lldb::ValueObjectSP Dereference(Status ) override;
 
-  ValueObject *CreateChildAtIndex(size_t idx, bool synthetic_array_member,
-  int32_t synthetic_index) override;
-
   lldb::ValueObjectSP GetSyntheticChildAtOffset(
   uint32_t offset, const CompilerType , bool can_create,
   ConstString name_const_str = ConstString()) override;
@@ -151,6 +148,13 @@ class ValueObjectConstResult : public ValueObject {
   ValueObjectConstResult(ExecutionContextScope *exe_scope,
  ValueObjectManager , const Status );
 
+  ValueObject *CreateChildAtIndex(size_t idx) override {
+return m_impl.CreateChildAtIndex(idx);
+  }
+  ValueObject *CreateSyntheticArrayMember(size_t idx) override {
+return m_impl.CreateSyntheticArrayMember(idx);
+  }
+
   ValueObjectConstResult(const ValueObjectConstResult &) = delete;
   const ValueObjectConstResult &
   operator=(const ValueObjectConstResult &) = delete;
diff --git a/lldb/include/lldb/Core/ValueObjectConstResultCast.h 
b/lldb/include/lldb/Core/ValueObjectConstResultCast.h
index efcbe0dc6a0bd..911a08363b393 100644
--- a/lldb/include/lldb/Core/ValueObjectConstResultCast.h
+++ b/lldb/include/lldb/Core/ValueObjectConstResultCast.h
@@ -35,9 +35,6 @@ class ValueObjectConstResultCast : public ValueObjectCast {
 
   lldb::ValueObjectSP Dereference(Status ) override;
 
-  ValueObject *CreateChildAtIndex(size_t idx, bool synthetic_array_member,
-  int32_t synthetic_index) override;
-
   virtual CompilerType 

[Lldb-commits] [lldb] [lldb-dap] Added "port" property to vscode "attach" command. (PR #91570)

[Pavel Labath via lldb-commits]

@@ -0,0 +1,202 @@
+"""
+Test lldb-dap "port" configuration to "attach" request
+"""
+
+
+import dap_server
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+from lldbsuite.test import lldbplatformutil
+import lldbdap_testcase
+import os
+import shutil
+import subprocess
+import tempfile
+import threading
+import sys
+import socket
+import select
+
+
+# A class representing a pipe for communicating with debug server.
+# This class includes menthods to open the pipe and read the port number from 
it.
+class Pipe(object):
+def __init__(self, prefix):
+self.name = os.path.join(prefix, "stub_port_number")
+os.mkfifo(self.name)
+self._fd = os.open(self.name, os.O_RDONLY | os.O_NONBLOCK)
+
+def finish_connection(self, timeout):
+pass
+
+def read(self, size, timeout):
+(readers, _, _) = select.select([self._fd], [], [], timeout)
+if self._fd not in readers:
+raise TimeoutError
+return os.read(self._fd, size)
+
+def close(self):
+os.close(self._fd)
+
+
+class TestDAP_attachByPortNum(lldbdap_testcase.DAPTestCaseBase):
+default_timeout = 20
+
+def set_and_hit_breakpoint(self, continueToExit=True):
+source = "main.c"
+main_source_path = os.path.join(os.getcwd(), source)
+breakpoint1_line = line_number(main_source_path, "// breakpoint 1")
+lines = [breakpoint1_line]
+# Set breakpoint in the thread function so we can step the threads
+breakpoint_ids = self.set_source_breakpoints(main_source_path, lines)
+self.assertEqual(
+len(breakpoint_ids), len(lines), "expect correct number of 
breakpoints"
+)
+self.continue_to_breakpoints(breakpoint_ids)
+if continueToExit:
+self.continue_to_exit()
+
+def get_debug_server_command_line_args(self):
+args = []
+if lldbplatformutil.getPlatform() == "linux":
+args = ["gdbserver"]
+elif lldbplatformutil.getPlatform() == "macosx":
+args = ["--listen"]
+if lldb.remote_platform:
+args += ["*:0"]
+else:
+args += ["localhost:0"]
+return args
+
+def get_debug_server_pipe(self):
+pipe = Pipe(self.getBuildDir())
+self.addTearDownHook(lambda: pipe.close())
+pipe.finish_connection(self.default_timeout)
+return pipe
+
+@skipIfWindows
+@skipIfNetBSD
+def test_by_port(self):
+"""
+Tests attaching to a process by port.
+"""
+self.build_and_create_debug_adaptor()
+program = self.getBuildArtifact("a.out")
+
+debug_server_tool = self.getBuiltinDebugServerTool()
+
+pipe = self.get_debug_server_pipe()
+args = self.get_debug_server_command_line_args()
+args += [program]
+args += ["--named-pipe", pipe.name]
+
+self.process = self.spawnSubprocess(
+debug_server_tool, args, install_remote=False
+)
+
+# Read the port number from the debug server pipe.
+port = pipe.read(10, self.default_timeout)
+# Trim null byte, convert to int
+port = int(port[:-1])
+self.assertIsNotNone(
+port, " Failed to read the port number from debug server pipe"
+)
+
+self.attach(program=program, gdbRemotePort=port, sourceInitFile=True)
+self.set_and_hit_breakpoint(continueToExit=True)
+self.process.terminate()
+
+@skipIfWindows
+@skipIfNetBSD
+def test_by_port_and_pid(self):
+"""
+Tests attaching to a process by process ID and port number.
+"""
+self.build_and_create_debug_adaptor()
+program = self.getBuildArtifact("a.out")
+
+debug_server_tool = self.getBuiltinDebugServerTool()
+pipe = self.get_debug_server_pipe()
+args = self.get_debug_server_command_line_args()
+args += [program]
+args += ["--named-pipe", pipe.name]
+
+self.process = self.spawnSubprocess(
+debug_server_tool, args, install_remote=False
+)
+
+# Read the port number from the debug server pipe.
+port = pipe.read(10, self.default_timeout)
+# Trim null byte, convert to int
+port = int(port[:-1])
+self.assertIsNotNone(
+port, " Failed to read the port number from debug server pipe"
+)
+
+pid = self.process.pid
+
+response = self.attach(
+program=program,
+pid=pid,
+gdbRemotePort=port,
+sourceInitFile=True,
+expectFailure=True,
+)
+if not (response and response["success"]):
+self.assertFalse(
+response["success"], "The user can't specify both pid and port"
+)
+self.process.terminate()
+
+@skipIfWindows
+@skipIfNetBSD
+def 

[Lldb-commits] [lldb] [lldb-dap] Added "port" property to vscode "attach" command. (PR #91570)

[Pavel Labath via lldb-commits]

@@ -0,0 +1,202 @@
+"""
+Test lldb-dap "port" configuration to "attach" request
+"""
+
+
+import dap_server
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+from lldbsuite.test import lldbplatformutil
+import lldbdap_testcase
+import os
+import shutil
+import subprocess
+import tempfile
+import threading
+import sys
+import socket
+import select
+
+
+# A class representing a pipe for communicating with debug server.
+# This class includes menthods to open the pipe and read the port number from 
it.
+class Pipe(object):
+def __init__(self, prefix):
+self.name = os.path.join(prefix, "stub_port_number")
+os.mkfifo(self.name)
+self._fd = os.open(self.name, os.O_RDONLY | os.O_NONBLOCK)
+
+def finish_connection(self, timeout):
+pass
+
+def read(self, size, timeout):
+(readers, _, _) = select.select([self._fd], [], [], timeout)
+if self._fd not in readers:
+raise TimeoutError
+return os.read(self._fd, size)
+
+def close(self):
+os.close(self._fd)
+
+
+class TestDAP_attachByPortNum(lldbdap_testcase.DAPTestCaseBase):
+default_timeout = 20
+
+def set_and_hit_breakpoint(self, continueToExit=True):
+source = "main.c"
+main_source_path = os.path.join(os.getcwd(), source)
+breakpoint1_line = line_number(main_source_path, "// breakpoint 1")
+lines = [breakpoint1_line]
+# Set breakpoint in the thread function so we can step the threads
+breakpoint_ids = self.set_source_breakpoints(main_source_path, lines)
+self.assertEqual(
+len(breakpoint_ids), len(lines), "expect correct number of 
breakpoints"
+)
+self.continue_to_breakpoints(breakpoint_ids)
+if continueToExit:
+self.continue_to_exit()
+
+def get_debug_server_command_line_args(self):
+args = []
+if lldbplatformutil.getPlatform() == "linux":
+args = ["gdbserver"]
+elif lldbplatformutil.getPlatform() == "macosx":
+args = ["--listen"]
+if lldb.remote_platform:
+args += ["*:0"]
+else:
+args += ["localhost:0"]
+return args
+
+def get_debug_server_pipe(self):
+pipe = Pipe(self.getBuildDir())
+self.addTearDownHook(lambda: pipe.close())
+pipe.finish_connection(self.default_timeout)
+return pipe
+
+@skipIfWindows
+@skipIfNetBSD
+def test_by_port(self):
+"""
+Tests attaching to a process by port.
+"""
+self.build_and_create_debug_adaptor()
+program = self.getBuildArtifact("a.out")
+
+debug_server_tool = self.getBuiltinDebugServerTool()
+
+pipe = self.get_debug_server_pipe()
+args = self.get_debug_server_command_line_args()
+args += [program]
+args += ["--named-pipe", pipe.name]
+
+self.process = self.spawnSubprocess(
+debug_server_tool, args, install_remote=False
+)
+
+# Read the port number from the debug server pipe.
+port = pipe.read(10, self.default_timeout)
+# Trim null byte, convert to int
+port = int(port[:-1])
+self.assertIsNotNone(
+port, " Failed to read the port number from debug server pipe"
+)
+
+self.attach(program=program, gdbRemotePort=port, sourceInitFile=True)
+self.set_and_hit_breakpoint(continueToExit=True)
+self.process.terminate()
+
+@skipIfWindows
+@skipIfNetBSD
+def test_by_port_and_pid(self):
+"""
+Tests attaching to a process by process ID and port number.
+"""
+self.build_and_create_debug_adaptor()
+program = self.getBuildArtifact("a.out")
+
+debug_server_tool = self.getBuiltinDebugServerTool()
+pipe = self.get_debug_server_pipe()
+args = self.get_debug_server_command_line_args()
+args += [program]
+args += ["--named-pipe", pipe.name]
+
+self.process = self.spawnSubprocess(
+debug_server_tool, args, install_remote=False
+)
+
+# Read the port number from the debug server pipe.
+port = pipe.read(10, self.default_timeout)
+# Trim null byte, convert to int
+port = int(port[:-1])

labath wrote:

You don't actually need to launch lldb server just to check that this returns 
an error, right? Since noone will be actually connecting to the port (or 
attaching to the pid), you could just pass `1`, or something like that..

https://github.com/llvm/llvm-project/pull/91570
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb-dap] Added "port" property to vscode "attach" command. (PR #91570)

[Pavel Labath via lldb-commits]

@@ -0,0 +1,202 @@
+"""
+Test lldb-dap "port" configuration to "attach" request
+"""
+
+
+import dap_server
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+from lldbsuite.test import lldbplatformutil
+import lldbdap_testcase
+import os
+import shutil
+import subprocess
+import tempfile
+import threading
+import sys
+import socket
+import select
+
+
+# A class representing a pipe for communicating with debug server.
+# This class includes menthods to open the pipe and read the port number from 
it.
+class Pipe(object):
+def __init__(self, prefix):
+self.name = os.path.join(prefix, "stub_port_number")
+os.mkfifo(self.name)
+self._fd = os.open(self.name, os.O_RDONLY | os.O_NONBLOCK)
+
+def finish_connection(self, timeout):
+pass
+
+def read(self, size, timeout):
+(readers, _, _) = select.select([self._fd], [], [], timeout)
+if self._fd not in readers:
+raise TimeoutError
+return os.read(self._fd, size)
+
+def close(self):
+os.close(self._fd)

labath wrote:

Instead of copying the implementation, please move it somewhere where it can be 
accessed from both tests (`lldbgdbserverutils` is probabable a fine place for 
it).

https://github.com/llvm/llvm-project/pull/91570
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb-dap] Added "port" property to vscode "attach" command. (PR #91570)

[Pavel Labath via lldb-commits]

labath wrote:

> > FWIW, I think this feature would be more useful and easier to implement if 
> > there was just a single setting called "connection url" or something, which 
> > accepted all of the usual lldb connection urls (unix-abstract-connect://, 
> > listen://, ...).
> > Since a simple tcp port connection is definitely going to be the most 
> > commonly used connection, you can, if you want, treat a simple number (or 
> > `:number`) as an alias to `connect://localhost:number`. lldb-server does 
> > something similar.
> > PS: I'm clicking request changes because I don't believe the test for the 
> > feature can go in in this form. I don't consider myself an owner for the 
> > rest, so you can consider my comments as suggestions.
> 
> I apologize for missing your earlier comment. To clarify. Using the URL 
> "connect://localhost:12345" or "connect://:12345" with the "connect" scheme 
> helps invoke the appropriate function to establish a connection. Pls ref: 
> https://github.com/llvm/llvm-project/blob/main/lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp#L148
>  $gdb-remote number - would work from the command-line, because lldb is 
> taking care of adding "connect://".
> 
> Please let me know if my understating is incorrect.

Your understanding of the code is correct. What I am suggesting is basically to 
emulate this command line behavior in vscode. 

Basically, instead of two host+port settings, you could have one setting with a 
sufficiently vague name ("connection url", "remote address", ...) to cover the 
expanded scope. Then, you implementation could take the user-provided string 
and fill in the blanks:
- if the user types "1234", you'd convert it to `connect://localhost:1234`
- if the user types "my-host:1234", you'd convert it to 
`connect://localhost:1234`
- if the user types "unix-abstract-connect://..." (or any other scheme 
supported by lldb), you'd pass the value unmodified

The advantage of this is that you'd automatically support every connection 
protocol that lldb understands with almost no effort. If we stick to the 
current implementation, then if someone later needs to connect through unix 
sockets (or whatever), he would have to go back and add *another* setting to 
support connecting through that.

Now, I do see the appeal of having an explicit host/port fields (they're much 
easier to understand for the average user), which is why I'm not insisting on 
this approach. Nonetheless, I think this is an option worth considering, and I 
think we could avoid most of the confusion by carefully crafting the 
description of this field to guide the user to the most common scenarios.

https://github.com/llvm/llvm-project/pull/91570
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] Reapply [lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. (PR #92328)

[Pavel Labath via lldb-commits]

labath wrote:

(Zequan is OOO for three weeks, I can take over this (tomorrow) if needed.)

https://github.com/llvm/llvm-project/pull/92328
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb] Support reading DW_OP_piece from file address (PR #94026)

[Pavel Labath via lldb-commits]

https://github.com/labath approved this pull request.


https://github.com/llvm/llvm-project/pull/94026
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb] Add documentation for the max_children argument (PR #94192)

[Pavel Labath via lldb-commits]

https://github.com/labath closed https://github.com/llvm/llvm-project/pull/94192
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb] Fixed TestEchoCommands.test running on a remote target (PR #94127)

[Pavel Labath via lldb-commits]

labath wrote:

> > > * LLDB_PLATFORM_URL
> > 
> > 
> > Is that an environment variable or a cmake variable? I don't see a single 
> > instance of that string in the entire lldb source tree. Are you sure that's 
> > not some downstream feature?
> 
> Sorry for the confusion, the PR for the remote run of Shell tests is coming 
> soon. This PR will be relevant after that.

Ok, I think we should start with that first. And maybe instead of a PR, 
starting with an RFC to the dev list might be in order, as I'd like to hear how 
do you plan to implement it, and what do you hope to gain through this 
functionality.

FWIW, my opinion is that we should not have this functionality, as I don't want 
to duplicate the remote execution logic for the different kind of tests, and 
also because I think that most of the tests would not actually be useful in 
this setup (for example, because a lot of them use hard-coded assembly specific 
to a single target -- that's one of the criteria I use when choosing what kind 
of test to write).

If you think there are some tests (classes of tests) that are particularly 
useful to be run in a remote scenario, then we can consider converting them to 
an API test.

https://github.com/llvm/llvm-project/pull/94127
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb] Improve identification of Dlang mangled names (PR #93881)

[Pavel Labath via lldb-commits]

labath wrote:

> Fix is #94196.
> 
> There isn't anything to log really, the function just didn't have a symbol on 
> Windows.

AIUI, symtabs just aren't a thing on windows. You either have debug info, or 
you have the exported symbols (aka .dynsym). There's no inbetween state.


https://github.com/llvm/llvm-project/pull/93881
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb] Support reading DW_OP_piece from file address (PR #94026)

[Pavel Labath via lldb-commits]

@@ -768,3 +768,63 @@ TEST(DWARFExpression, ExtensionsDWO) {
 
   testExpressionVendorExtensions(dwo_module_sp, *dwo_dwarf_unit);
 }
+
+TEST_F(DWARFExpressionMockProcessTest, DW_OP_piece_file_addr) {
+  struct MockTarget : Target {

labath wrote:

The code I linked to ([this 
one](https://github.com/llvm/llvm-project/blob/main/lldb/unittests/TestingSupport/Host/NativeProcessTestUtils.h#L73))
 shows one way to do it. Basically, you implement the real function in terms on 
some other interface -- one which is easier to mock -- and then you mock *that*.

https://github.com/llvm/llvm-project/pull/94026
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb] Fixed TestEchoCommands.test running on a remote target (PR #94127)

[Pavel Labath via lldb-commits]

labath wrote:

> * LLDB_PLATFORM_URL

Is that an environment variable or a cmake variable? I don't see a single 
instance of that string in the entire lldb source tree. Are you sure that's not 
some downstream feature?

https://github.com/llvm/llvm-project/pull/94127
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] d00731c - [lldb] s/assertEquals/assertEqual in TestDAP_variables_children

[Pavel Labath via lldb-commits]

Author: Pavel Labath
Date: 2024-06-03T11:23:51+02:00
New Revision: d00731cb7fcc91047531069e029964a39935a5bb

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

LOG: [lldb] s/assertEquals/assertEqual in TestDAP_variables_children

Added: 


Modified: 

lldb/test/API/tools/lldb-dap/variables/children/TestDAP_variables_children.py

Removed: 




diff  --git 
a/lldb/test/API/tools/lldb-dap/variables/children/TestDAP_variables_children.py 
b/lldb/test/API/tools/lldb-dap/variables/children/TestDAP_variables_children.py
index 54fb318289aec..805e88ddf8f70 100644
--- 
a/lldb/test/API/tools/lldb-dap/variables/children/TestDAP_variables_children.py
+++ 
b/lldb/test/API/tools/lldb-dap/variables/children/TestDAP_variables_children.py
@@ -31,7 +31,7 @@ def test_get_num_children(self):
 indexed = next(filter(lambda x: x["name"] == "indexed", local_vars))
 not_indexed = next(filter(lambda x: x["name"] == "not_indexed", 
local_vars))
 self.assertIn("indexedVariables", indexed)
-self.assertEquals(indexed["indexedVariables"], 1)
+self.assertEqual(indexed["indexedVariables"], 1)
 self.assertNotIn("indexedVariables", not_indexed)
 
 self.assertIn(



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

[Lldb-commits] [lldb] [lldb] Fixed TestEchoCommands.test running on a remote target (PR #94127)

[Pavel Labath via lldb-commits]

labath wrote:

I'd like to try this myself. How can I configure my build to run this sort of 
thing?

https://github.com/llvm/llvm-project/pull/94127
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb] Use packaging module instead of pkg_resources (PR #93712)

[Pavel Labath via lldb-commits]

labath wrote:

> > That said, using a package designed (AIUI) for python versions for parsing 
> > versions of gcc/clang does strike me as somewhat... unusual, even if it 
> > does work, so _**maybe**_ there is case to be made for writing something 
> > custom (I'm not sure if we really need anything more elaborate than 
> > `tuple([int(part) for part in version.split(".")])`)
> 
> Agreed. The first time I took a stab at this, that was the first thing I 
> tried, but I quickly discovered that we had at least one tool (I think it was 
> Python itself?) that contained alphabetical characters (something like 
> `1.2.3rc` or `1.2.3.dev`) and I didn't feel like dealing with all the 
> possible edge cases.

Ah yes, I guess *one* of the versions we are parsing is the version of python 
itself..

> We have other packages the test suite relies on (pexpect, psutil, etc) so it 
> seemed reasonable, but if folks feel strongly about it we can maintain our 
> own "version parsing".

While I don't think we should be adding deps willy-nilly, I don't think this 
one is more problematic than others in that they are all available through 
common package management systems. OTOH, it has two things going against it:
- it's a hard dep breaking the ability to run any test (whereas e.g. pexpect  
would only break pexpect-based tests)
- it should be relatively easy to replace

This does not constitute an endorsement of one direction of the other, I'm just 
thinking out loud.

https://github.com/llvm/llvm-project/pull/93712
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb] Support reading DW_OP_piece from file address (PR #94026)

[Pavel Labath via lldb-commits]

https://github.com/labath commented:

Can't say I'm thrilled by the addition of a random virtual method, but I don't 
want to stand in the way. I'll just note that there is an alternative - in the 
form of writing a minimal real target in assembly. In fact, I think 
`test/Shell/SymbolFile/DWARF/x86/DW_OP_piece-struct.s` has all of the 
boilerplate, so all you'd need is to replace `DW_OP_stack_value` with 
`DW_OP_addr`.

https://github.com/llvm/llvm-project/pull/94026
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb] Support reading DW_OP_piece from file address (PR #94026)

[Pavel Labath via lldb-commits]

https://github.com/labath edited https://github.com/llvm/llvm-project/pull/94026
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb] Support reading DW_OP_piece from file address (PR #94026)

[Pavel Labath via lldb-commits]

@@ -768,3 +768,63 @@ TEST(DWARFExpression, ExtensionsDWO) {
 
   testExpressionVendorExtensions(dwo_module_sp, *dwo_dwarf_unit);
 }
+
+TEST_F(DWARFExpressionMockProcessTest, DW_OP_piece_file_addr) {
+  struct MockTarget : Target {

labath wrote:

You could consider using gmock with [this 
kind](https://github.com/llvm/llvm-project/blob/main/lldb/unittests/TestingSupport/Host/NativeProcessTestUtils.h#L73)
 of a pattern that would enable you to write `EXPECT_CALL(target, 
ReadMemory(0x40, 1)).WillOnce(Return(ByMove(std::vector{0x11})));`

https://github.com/llvm/llvm-project/pull/94026
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb] Support reading DW_OP_piece from file address (PR #94026)

[Pavel Labath via lldb-commits]

@@ -2153,26 +2152,42 @@ bool DWARFExpression::Evaluate(
 }
 break;
 
-  case Value::ValueType::FileAddress:
-  case Value::ValueType::HostAddress:
-if (error_ptr) {
-  lldb::addr_t addr = 
curr_piece_source_value.GetScalar().ULongLong(
-  LLDB_INVALID_ADDRESS);
+  case Value::ValueType::FileAddress: {

labath wrote:

I'm wondering if it would be possible to use a single implementation for these 
two. Target::ReadMemory will call Process::ReadMemory if it was a process 
available (and the `force_live_memory` precisely so that one can require this 
behavior).

https://github.com/llvm/llvm-project/pull/94026
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb] Support reading DW_OP_piece from file address (PR #94026)

[Pavel Labath via lldb-commits]

@@ -768,3 +768,63 @@ TEST(DWARFExpression, ExtensionsDWO) {
 
   testExpressionVendorExtensions(dwo_module_sp, *dwo_dwarf_unit);
 }
+
+TEST_F(DWARFExpressionMockProcessTest, DW_OP_piece_file_addr) {
+  struct MockTarget : Target {
+MockTarget(Debugger , const ArchSpec _arch,
+   const lldb::PlatformSP _sp)
+: Target(debugger, target_arch, platform_sp, true) {}
+
+size_t ReadMemory(const Address , void *dst, size_t dst_len,
+  Status , bool force_live_memory = false,
+  lldb::addr_t *load_addr_ptr = nullptr) override {
+  // We expected to be called in a very specific way.
+  assert(dst_len == 1);
+  assert(addr.GetOffset() == 0x40 || addr.GetOffset() == 0x50);
+
+  if (addr.GetOffset() == 0x40)
+((uint8_t *)dst)[0] = 0x11;
+
+  if (addr.GetOffset() == 0x50)
+((uint8_t *)dst)[0] = 0x22;
+
+  return 1;
+}
+  };
+
+  // Set up a mock process.
+  ArchSpec arch("i386-pc-linux");
+  Platform::SetHostPlatform(
+  platform_linux::PlatformLinux::CreateInstance(true, ));
+  lldb::DebuggerSP debugger_sp = Debugger::CreateInstance();
+  ASSERT_TRUE(debugger_sp);
+  lldb::PlatformSP platform_sp;
+  lldb::TargetSP target_sp =
+  std::make_shared(*debugger_sp, arch, platform_sp);
+  ASSERT_TRUE(target_sp);
+  ASSERT_TRUE(target_sp->GetArchitecture().IsValid());
+
+  ExecutionContext exe_ctx(target_sp, false);
+
+  uint8_t expr[] = {DW_OP_addr, 0x40, 0x0, 0x0, 0x0, DW_OP_piece, 1,
+DW_OP_addr, 0x50, 0x0, 0x0, 0x0, DW_OP_piece, 1};
+  DataExtractor extractor(expr, sizeof(expr), lldb::eByteOrderLittle,
+  /*addr_size*/ 4);
+  Value result;
+  Status status;
+  ASSERT_TRUE(DWARFExpression::Evaluate(
+  _ctx, /*reg_ctx*/ nullptr, /*module_sp*/ {}, extractor,
+  /*unit*/ nullptr, lldb::eRegisterKindLLDB,
+  /*initial_value_ptr*/ nullptr,
+  /*object_address_ptr*/ nullptr, result, ))
+  << status.ToError();
+
+  ASSERT_EQ(result.GetValueType(), Value::ValueType::HostAddress);
+
+  DataBufferHeap  = result.GetBuffer();
+  ASSERT_EQ(buf.GetByteSize(), 2U);
+
+  const uint8_t *bytes = buf.GetBytes();
+  EXPECT_EQ(bytes[0], 0x11);
+  EXPECT_EQ(bytes[1], 0x22);

labath wrote:

`ASSERT_THAT(result.GetBuffer().GetData(), testing::ElementsAre(0x11, 0x22));` 
(or something along those lines, may need some massaging to compile)

https://github.com/llvm/llvm-project/pull/94026
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb] Avoid (unlimited) GetNumChildren calls when printing values (PR #93946)

[Pavel Labath via lldb-commits]

https://github.com/labath closed https://github.com/llvm/llvm-project/pull/93946
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb] Add documentation for the max_children argument (PR #94192)

[Pavel Labath via lldb-commits]

@@ -930,40 +930,55 @@ be implemented by the Python class):
 
class SyntheticChildrenProvider:
   def __init__(self, valobj, internal_dict):
- this call should initialize the Python object using valobj as the 
variable to provide synthetic children for
-  def num_children(self):
- this call should return the number of children that you want your 
object to have
+ this call should initialize the Python object using valobj as the
+ variable to provide synthetic children for
+  def num_children(self, max_children):
+ this call should return the number of children that you want your
+ object to have[1]
   def get_child_index(self,name):
- this call should return the index of the synthetic child whose name 
is given as argument
+ this call should return the index of the synthetic child whose name is
+ given as argument
   def get_child_at_index(self,index):
- this call should return a new LLDB SBValue object representing the 
child at the index given as argument
+ this call should return a new LLDB SBValue object representing the

labath wrote:

Reformatted because code blocks are not automatically wrapped (and the lines 
don't fit on one screen).

https://github.com/llvm/llvm-project/pull/94192
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb] Avoid (unlimited) GetNumChildren calls when printing values (PR #93946)

[Pavel Labath via lldb-commits]

labath wrote:

> This seems reasonable. However, I note that on the page where we show how to 
> write synthetic child providers, we say:
> 
> def num_children(self): this call should return the number of children that 
> you want your object to have
> 
> That's actually not true - we pass the max_children argument on to the 
> num_children method, and in fact some of the tests do use the max parameter. 
> But since you're making that actually useful, can you fix the docs so people 
> will know to take advantage of this?

Sounds good. I've created a separate 
[pr](https://github.com/llvm/llvm-project/pull/94192) for that.


> BTW, because the max number of children is probably getting ignored in the 
> wild, this has to be advisory, you can't require that:
> 
> val.GetNumChildren(max_children) <= max_children
> 
> I don't think you do that but it might be good to note the fact.

Yes, I am keeping that in mind.

https://github.com/llvm/llvm-project/pull/93946
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb] Avoid (unlimited) GetNumChildren calls when printing values (PR #93946)

[Pavel Labath via lldb-commits]

@@ -442,16 +443,19 @@ lldb::Format 
FormatManager::GetSingleItemFormat(lldb::Format vector_format) {
 }
 
 bool FormatManager::ShouldPrintAsOneLiner(ValueObject ) {
+  TargetSP target_sp = valobj.GetTargetSP();
   // if settings say no oneline whatsoever
-  if (valobj.GetTargetSP().get() &&
-  !valobj.GetTargetSP()->GetDebugger().GetAutoOneLineSummaries())
+  if (target_sp && !target_sp->GetDebugger().GetAutoOneLineSummaries())
 return false; // then don't oneline
 
   // if this object has a summary, then ask the summary
   if (valobj.GetSummaryFormat().get() != nullptr)
 return valobj.GetSummaryFormat()->IsOneLiner();
 
-  auto num_children = valobj.GetNumChildren();
+  size_t max_num_children =

labath wrote:

Done

https://github.com/llvm/llvm-project/pull/93946
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb] Avoid (unlimited) GetNumChildren calls when printing values (PR #93946)

[Pavel Labath via lldb-commits]

https://github.com/labath updated 
https://github.com/llvm/llvm-project/pull/93946

>From 1e25ef1cc5ff4d12a3e5b85c8ea7cd30a3e0908e Mon Sep 17 00:00:00 2001
From: Pavel Labath 
Date: Fri, 31 May 2024 10:06:19 +
Subject: [PATCH 1/2] [lldb] Avoid (unlimited) GetNumChildren calls when
 printing values

For some data formatters, even getting the number of children can be an
expensive operations (e.g., needing to walk a linked list to determine
the number of elements). This is then wasted work when we know we will
be printing only small number of them.

This patch replaces the calls to GetNumChildren (at least those on the
"frame var" path) with the calls to the capped version, passing the
value of `max-children-count` setting (plus one)
---
 lldb/source/DataFormatters/FormatManager.cpp  | 10 ---
 .../DataFormatters/ValueObjectPrinter.cpp | 27 ++-
 .../synthcapping/TestSyntheticCapping.py  | 11 
 .../synthcapping/fooSynthProvider.py  | 15 ++-
 4 files changed, 46 insertions(+), 17 deletions(-)

diff --git a/lldb/source/DataFormatters/FormatManager.cpp 
b/lldb/source/DataFormatters/FormatManager.cpp
index d7ba5b4b70c94..84c0c7eed1431 100644
--- a/lldb/source/DataFormatters/FormatManager.cpp
+++ b/lldb/source/DataFormatters/FormatManager.cpp
@@ -9,6 +9,7 @@
 #include "lldb/DataFormatters/FormatManager.h"
 
 #include "lldb/Core/Debugger.h"
+#include "lldb/Core/ValueObject.h"
 #include "lldb/DataFormatters/FormattersHelpers.h"
 #include "lldb/DataFormatters/LanguageCategory.h"
 #include "lldb/Interpreter/ScriptInterpreter.h"
@@ -442,16 +443,19 @@ lldb::Format 
FormatManager::GetSingleItemFormat(lldb::Format vector_format) {
 }
 
 bool FormatManager::ShouldPrintAsOneLiner(ValueObject ) {
+  TargetSP target_sp = valobj.GetTargetSP();
   // if settings say no oneline whatsoever
-  if (valobj.GetTargetSP().get() &&
-  !valobj.GetTargetSP()->GetDebugger().GetAutoOneLineSummaries())
+  if (target_sp && !target_sp->GetDebugger().GetAutoOneLineSummaries())
 return false; // then don't oneline
 
   // if this object has a summary, then ask the summary
   if (valobj.GetSummaryFormat().get() != nullptr)
 return valobj.GetSummaryFormat()->IsOneLiner();
 
-  auto num_children = valobj.GetNumChildren();
+  size_t max_num_children =
+  (target_sp ? *target_sp : Target::GetGlobalProperties())
+  .GetMaximumNumberOfChildrenToDisplay();
+  auto num_children = valobj.GetNumChildren(max_num_children);
   if (!num_children) {
 llvm::consumeError(num_children.takeError());
 return true;
diff --git a/lldb/source/DataFormatters/ValueObjectPrinter.cpp 
b/lldb/source/DataFormatters/ValueObjectPrinter.cpp
index bbdc2a9981570..c2933d8574583 100644
--- a/lldb/source/DataFormatters/ValueObjectPrinter.cpp
+++ b/lldb/source/DataFormatters/ValueObjectPrinter.cpp
@@ -14,6 +14,8 @@
 #include "lldb/Target/Language.h"
 #include "lldb/Target/Target.h"
 #include "lldb/Utility/Stream.h"
+#include "llvm/Support/MathExtras.h"
+#include 
 
 using namespace lldb;
 using namespace lldb_private;
@@ -628,22 +630,21 @@ ValueObjectPrinter::GetMaxNumChildrenToPrint(bool 
_dotdotdot) {
   if (m_options.m_pointer_as_array)
 return m_options.m_pointer_as_array.m_element_count;
 
-  auto num_children_or_err = synth_valobj.GetNumChildren();
+  const uint32_t max_num_children =
+  m_options.m_ignore_cap ? UINT32_MAX
+ : GetMostSpecializedValue()
+   .GetTargetSP()
+   ->GetMaximumNumberOfChildrenToDisplay();
+  // Ask for one more child than the maximum to see if we should print "...".
+  auto num_children_or_err = synth_valobj.GetNumChildren(
+  llvm::SaturatingAdd(max_num_children, uint32_t(1)));
   if (!num_children_or_err)
 return num_children_or_err;
-  uint32_t num_children = *num_children_or_err;
-  print_dotdotdot = false;
-  if (num_children) {
-const size_t max_num_children = GetMostSpecializedValue()
-.GetTargetSP()
-
->GetMaximumNumberOfChildrenToDisplay();
-
-if (num_children > max_num_children && !m_options.m_ignore_cap) {
-  print_dotdotdot = true;
-  return max_num_children;
-}
+  if (*num_children_or_err > max_num_children) {
+print_dotdotdot = true;
+return max_num_children;
   }
-  return num_children;
+  return num_children_or_err;
 }
 
 void ValueObjectPrinter::PrintChildrenPostamble(bool print_dotdotdot) {
diff --git 
a/lldb/test/API/functionalities/data-formatter/synthcapping/TestSyntheticCapping.py
 
b/lldb/test/API/functionalities/data-formatter/synthcapping/TestSyntheticCapping.py
index d53dadef836e5..9ca232abefa03 100644
--- 
a/lldb/test/API/functionalities/data-formatter/synthcapping/TestSyntheticCapping.py
+++ 
b/lldb/test/API/functionalities/data-formatter/synthcapping/TestSyntheticCapping.py
@@ -68,6 +68,11 @@ def cleanup():

[Lldb-commits] [lldb] [lldb] Add documentation for the max_children argument (PR #94192)

[Pavel Labath via lldb-commits]

https://github.com/labath created 
https://github.com/llvm/llvm-project/pull/94192

None

>From 8bb7817d913cd8843fe8c90dcd7c8b70a0366994 Mon Sep 17 00:00:00 2001
From: Pavel Labath 
Date: Mon, 3 Jun 2024 09:58:47 +0200
Subject: [PATCH] [lldb] Add documentation for the max_children argument

---
 lldb/docs/use/variable.rst  | 37 +++--
 lldb/include/lldb/API/SBValue.h | 14 +
 2 files changed, 40 insertions(+), 11 deletions(-)

diff --git a/lldb/docs/use/variable.rst b/lldb/docs/use/variable.rst
index e9175b25336ba..22c2bee73fa59 100644
--- a/lldb/docs/use/variable.rst
+++ b/lldb/docs/use/variable.rst
@@ -930,20 +930,27 @@ be implemented by the Python class):
 
class SyntheticChildrenProvider:
   def __init__(self, valobj, internal_dict):
- this call should initialize the Python object using valobj as the 
variable to provide synthetic children for
-  def num_children(self):
- this call should return the number of children that you want your 
object to have
+ this call should initialize the Python object using valobj as the
+ variable to provide synthetic children for
+  def num_children(self, max_children):
+ this call should return the number of children that you want your
+ object to have[1]
   def get_child_index(self,name):
- this call should return the index of the synthetic child whose name 
is given as argument
+ this call should return the index of the synthetic child whose name is
+ given as argument
   def get_child_at_index(self,index):
- this call should return a new LLDB SBValue object representing the 
child at the index given as argument
+ this call should return a new LLDB SBValue object representing the
+ child at the index given as argument
   def update(self):
- this call should be used to update the internal state of this Python 
object whenever the state of the variables in LLDB changes.[1]
+ this call should be used to update the internal state of this Python
+ object whenever the state of the variables in LLDB changes.[2]
  Also, this method is invoked before any other method in the interface.
   def has_children(self):
- this call should return True if this object might have children, and 
False if this object can be guaranteed not to have children.[2]
+ this call should return True if this object might have children, and
+ False if this object can be guaranteed not to have children.[3]
   def get_value(self):
- this call can return an SBValue to be presented as the value of the 
synthetic value under consideration.[3]
+ this call can return an SBValue to be presented as the value of the
+ synthetic value under consideration.[4]
 
 As a warning, exceptions that are thrown by python formatters are caught
 silently by LLDB and should be handled appropriately by the formatter itself.
@@ -951,7 +958,15 @@ Being more specific, in case of exceptions, LLDB might 
assume that the given
 object has no children or it might skip printing some children, as they are
 printed one by one.
 
-[1] This method is optional. Also, a boolean value must be returned (since lldb
+[1] The `max_children` argument is optional (since lldb 3.8.0) and indicates 
the
+maximum number of children that lldb is interested in (at this moment). If the
+computation of the number of children is expensive (for example, requires
+travesing a linked list to determine its size) your implementation may return
+`max_children` rather than the actual number. If the computation is cheap 
(e.g., the
+number is stored as a field of the object), then you can always return the true
+number of children (that is, ignore the `max_children` argument).
+
+[2] This method is optional. Also, a boolean value must be returned (since lldb
 3.1.0). If ``False`` is returned, then whenever the process reaches a new stop,
 this method will be invoked again to generate an updated list of the children
 for a given variable. Otherwise, if ``True`` is returned, then the value is
@@ -959,11 +974,11 @@ cached and this method won't be called again, effectively 
freezing the state of
 the value in subsequent stops. Beware that returning ``True`` incorrectly could
 show misleading information to the user.
 
-[2] This method is optional (since lldb 3.2.0). While implementing it in terms
+[3] This method is optional (since lldb 3.2.0). While implementing it in terms
 of num_children is acceptable, implementors are encouraged to look for
 optimized coding alternatives whenever reasonable.
 
-[3] This method is optional (since lldb 3.5.2). The `SBValue` you return here
+[4] This method is optional (since lldb 3.5.2). The `SBValue` you return here
 will most likely be a numeric type (int, float, ...) as its value bytes will be
 used as-if they were the value of the root `SBValue` proper.  As a shortcut for
 

[Lldb-commits] [lldb] [lldb][test] Disable PIE for some API tests (PR #93808)

[Pavel Labath via lldb-commits]

@@ -3,4 +3,8 @@
 # C_SOURCES := b.c
 # EXE := b.out
 
+ifndef PIE
+   LDFLAGS := -no-pie

labath wrote:

Setting LD_EXTRAS is the typical way to pass linker flags from here (the same 
for other files)

https://github.com/llvm/llvm-project/pull/93808
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][test] Disable PIE for some API tests (PR #93808)

[Pavel Labath via lldb-commits]

https://github.com/labath edited https://github.com/llvm/llvm-project/pull/93808
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][test] Disable PIE for some API tests (PR #93808)

[Pavel Labath via lldb-commits]

https://github.com/labath approved this pull request.


https://github.com/llvm/llvm-project/pull/93808
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][test] Disable PIE for some API tests (PR #93808)

[Pavel Labath via lldb-commits]

labath wrote:

> > Ok, I see now. It's an ld.bfd vs ld.lld thing. You probably have your clang 
> > configured to use lld. LLD does not put relocation addends into the data 
> > section (on both arm and intel). ld.bfd does, which is why this sort of 
> > happens to work there. Was your intention to test with LLD?
> 
> Yep, I run it with lld built together with clang and lldb. Should the 
> condition be narrowed to affect only builds with lld?

No, I think this is fine. I don't believe we have the ability to detect the 
linker used at the moment, and I'd like to avoid adding new dimensions to the 
test suite. Plus, the `PIE` flag captures the problem (that lldb depends on 
linker-specific relocation behavior, at least for ELF) these test expose fairly 
well. To fix this, we'd need to change ObjectFileELF::RelocateSection to 
relocate non-debug info sections as well. It may be worth linking these 
comments to a bug that provides more context.

It's possible this could break some targets (like android) that do not support 
building/running non-PIE executables, but if that happens, we should just get 
those targets to set the PIE flag.

https://github.com/llvm/llvm-project/pull/93808
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][test] Add --target-os argument to dotest.py (PR #93887)

[Pavel Labath via lldb-commits]

labath wrote:

Agreed. The reason code is looking like it does is because at some point in 
(now, pretty distant) past it was possible to `cd` into the test directory, 
type `make` and get the test binary out. Among other problems, this has caused 
duplicate code for determining various properties of the build between python 
and make. Since we no longer support these workflow anyway, there's no reason 
to keep the makefile code.

https://github.com/llvm/llvm-project/pull/93887
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb] Fixed TestEchoCommands.test running on a remote target (PR #94127)

[Pavel Labath via lldb-commits]

labath wrote:

I'm somewhat surprised to see this. I haven't been keeping track of all of the 
latest developments, but do I understand correctly that this implies that we 
support running (all?) shell tests on a remote system?

https://github.com/llvm/llvm-project/pull/94127
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb] Avoid (unlimited) GetNumChildren calls when printing values (PR #93946)

[Pavel Labath via lldb-commits]

https://github.com/labath updated 
https://github.com/llvm/llvm-project/pull/93946

>From 1e25ef1cc5ff4d12a3e5b85c8ea7cd30a3e0908e Mon Sep 17 00:00:00 2001
From: Pavel Labath 
Date: Fri, 31 May 2024 10:06:19 +
Subject: [PATCH] [lldb] Avoid (unlimited) GetNumChildren calls when printing
 values

For some data formatters, even getting the number of children can be an
expensive operations (e.g., needing to walk a linked list to determine
the number of elements). This is then wasted work when we know we will
be printing only small number of them.

This patch replaces the calls to GetNumChildren (at least those on the
"frame var" path) with the calls to the capped version, passing the
value of `max-children-count` setting (plus one)
---
 lldb/source/DataFormatters/FormatManager.cpp  | 10 ---
 .../DataFormatters/ValueObjectPrinter.cpp | 27 ++-
 .../synthcapping/TestSyntheticCapping.py  | 11 
 .../synthcapping/fooSynthProvider.py  | 15 ++-
 4 files changed, 46 insertions(+), 17 deletions(-)

diff --git a/lldb/source/DataFormatters/FormatManager.cpp 
b/lldb/source/DataFormatters/FormatManager.cpp
index d7ba5b4b70c94..84c0c7eed1431 100644
--- a/lldb/source/DataFormatters/FormatManager.cpp
+++ b/lldb/source/DataFormatters/FormatManager.cpp
@@ -9,6 +9,7 @@
 #include "lldb/DataFormatters/FormatManager.h"
 
 #include "lldb/Core/Debugger.h"
+#include "lldb/Core/ValueObject.h"
 #include "lldb/DataFormatters/FormattersHelpers.h"
 #include "lldb/DataFormatters/LanguageCategory.h"
 #include "lldb/Interpreter/ScriptInterpreter.h"
@@ -442,16 +443,19 @@ lldb::Format 
FormatManager::GetSingleItemFormat(lldb::Format vector_format) {
 }
 
 bool FormatManager::ShouldPrintAsOneLiner(ValueObject ) {
+  TargetSP target_sp = valobj.GetTargetSP();
   // if settings say no oneline whatsoever
-  if (valobj.GetTargetSP().get() &&
-  !valobj.GetTargetSP()->GetDebugger().GetAutoOneLineSummaries())
+  if (target_sp && !target_sp->GetDebugger().GetAutoOneLineSummaries())
 return false; // then don't oneline
 
   // if this object has a summary, then ask the summary
   if (valobj.GetSummaryFormat().get() != nullptr)
 return valobj.GetSummaryFormat()->IsOneLiner();
 
-  auto num_children = valobj.GetNumChildren();
+  size_t max_num_children =
+  (target_sp ? *target_sp : Target::GetGlobalProperties())
+  .GetMaximumNumberOfChildrenToDisplay();
+  auto num_children = valobj.GetNumChildren(max_num_children);
   if (!num_children) {
 llvm::consumeError(num_children.takeError());
 return true;
diff --git a/lldb/source/DataFormatters/ValueObjectPrinter.cpp 
b/lldb/source/DataFormatters/ValueObjectPrinter.cpp
index bbdc2a9981570..c2933d8574583 100644
--- a/lldb/source/DataFormatters/ValueObjectPrinter.cpp
+++ b/lldb/source/DataFormatters/ValueObjectPrinter.cpp
@@ -14,6 +14,8 @@
 #include "lldb/Target/Language.h"
 #include "lldb/Target/Target.h"
 #include "lldb/Utility/Stream.h"
+#include "llvm/Support/MathExtras.h"
+#include 
 
 using namespace lldb;
 using namespace lldb_private;
@@ -628,22 +630,21 @@ ValueObjectPrinter::GetMaxNumChildrenToPrint(bool 
_dotdotdot) {
   if (m_options.m_pointer_as_array)
 return m_options.m_pointer_as_array.m_element_count;
 
-  auto num_children_or_err = synth_valobj.GetNumChildren();
+  const uint32_t max_num_children =
+  m_options.m_ignore_cap ? UINT32_MAX
+ : GetMostSpecializedValue()
+   .GetTargetSP()
+   ->GetMaximumNumberOfChildrenToDisplay();
+  // Ask for one more child than the maximum to see if we should print "...".
+  auto num_children_or_err = synth_valobj.GetNumChildren(
+  llvm::SaturatingAdd(max_num_children, uint32_t(1)));
   if (!num_children_or_err)
 return num_children_or_err;
-  uint32_t num_children = *num_children_or_err;
-  print_dotdotdot = false;
-  if (num_children) {
-const size_t max_num_children = GetMostSpecializedValue()
-.GetTargetSP()
-
->GetMaximumNumberOfChildrenToDisplay();
-
-if (num_children > max_num_children && !m_options.m_ignore_cap) {
-  print_dotdotdot = true;
-  return max_num_children;
-}
+  if (*num_children_or_err > max_num_children) {
+print_dotdotdot = true;
+return max_num_children;
   }
-  return num_children;
+  return num_children_or_err;
 }
 
 void ValueObjectPrinter::PrintChildrenPostamble(bool print_dotdotdot) {
diff --git 
a/lldb/test/API/functionalities/data-formatter/synthcapping/TestSyntheticCapping.py
 
b/lldb/test/API/functionalities/data-formatter/synthcapping/TestSyntheticCapping.py
index d53dadef836e5..9ca232abefa03 100644
--- 
a/lldb/test/API/functionalities/data-formatter/synthcapping/TestSyntheticCapping.py
+++ 
b/lldb/test/API/functionalities/data-formatter/synthcapping/TestSyntheticCapping.py
@@ -68,6 +68,11 @@ def cleanup():
 "r 

[Lldb-commits] [lldb] [lldb] Avoid (unlimited) GetNumChildren calls when printing values (PR #93946)

[Pavel Labath via lldb-commits]

https://github.com/labath created 
https://github.com/llvm/llvm-project/pull/93946

For some data formatters, even getting the number of children can be an 
expensive operations (e.g., needing to walk a linked list to determine the 
number of elements). This is then wasted work when we know we will be printing 
only small number of them.

This patch replaces the calls to GetNumChildren (at least those on the "frame 
var" path) with the calls to the capped version, passing the value of 
`max-children-count` setting (plus one)

>From 957f0a85a0e0d2de9f34d00e28ba932e5affce86 Mon Sep 17 00:00:00 2001
From: Pavel Labath 
Date: Fri, 31 May 2024 10:06:19 +
Subject: [PATCH] [lldb] Avoid (unlimited) GetNumChildren calls when printing
 values

For some data formatters, even getting the number of children can be an
expensive operations (e.g., needing to walk a linked list to determine
the number of elements). This is then wasted work when we know we will
be printing only small number of them.

This patch replaces the calls to GetNumChildren (at least those on the
"frame var" path) with the calls to the capped version, passing the
value of `max-children-count` setting (plus one)
---
 lldb/source/DataFormatters/FormatManager.cpp  | 10 ---
 .../DataFormatters/ValueObjectPrinter.cpp | 27 ++-
 .../synthcapping/TestSyntheticCapping.py  | 11 
 .../synthcapping/fooSynthProvider.py  | 16 ++-
 4 files changed, 47 insertions(+), 17 deletions(-)

diff --git a/lldb/source/DataFormatters/FormatManager.cpp 
b/lldb/source/DataFormatters/FormatManager.cpp
index d7ba5b4b70c94..84c0c7eed1431 100644
--- a/lldb/source/DataFormatters/FormatManager.cpp
+++ b/lldb/source/DataFormatters/FormatManager.cpp
@@ -9,6 +9,7 @@
 #include "lldb/DataFormatters/FormatManager.h"
 
 #include "lldb/Core/Debugger.h"
+#include "lldb/Core/ValueObject.h"
 #include "lldb/DataFormatters/FormattersHelpers.h"
 #include "lldb/DataFormatters/LanguageCategory.h"
 #include "lldb/Interpreter/ScriptInterpreter.h"
@@ -442,16 +443,19 @@ lldb::Format 
FormatManager::GetSingleItemFormat(lldb::Format vector_format) {
 }
 
 bool FormatManager::ShouldPrintAsOneLiner(ValueObject ) {
+  TargetSP target_sp = valobj.GetTargetSP();
   // if settings say no oneline whatsoever
-  if (valobj.GetTargetSP().get() &&
-  !valobj.GetTargetSP()->GetDebugger().GetAutoOneLineSummaries())
+  if (target_sp && !target_sp->GetDebugger().GetAutoOneLineSummaries())
 return false; // then don't oneline
 
   // if this object has a summary, then ask the summary
   if (valobj.GetSummaryFormat().get() != nullptr)
 return valobj.GetSummaryFormat()->IsOneLiner();
 
-  auto num_children = valobj.GetNumChildren();
+  size_t max_num_children =
+  (target_sp ? *target_sp : Target::GetGlobalProperties())
+  .GetMaximumNumberOfChildrenToDisplay();
+  auto num_children = valobj.GetNumChildren(max_num_children);
   if (!num_children) {
 llvm::consumeError(num_children.takeError());
 return true;
diff --git a/lldb/source/DataFormatters/ValueObjectPrinter.cpp 
b/lldb/source/DataFormatters/ValueObjectPrinter.cpp
index bbdc2a9981570..c2933d8574583 100644
--- a/lldb/source/DataFormatters/ValueObjectPrinter.cpp
+++ b/lldb/source/DataFormatters/ValueObjectPrinter.cpp
@@ -14,6 +14,8 @@
 #include "lldb/Target/Language.h"
 #include "lldb/Target/Target.h"
 #include "lldb/Utility/Stream.h"
+#include "llvm/Support/MathExtras.h"
+#include 
 
 using namespace lldb;
 using namespace lldb_private;
@@ -628,22 +630,21 @@ ValueObjectPrinter::GetMaxNumChildrenToPrint(bool 
_dotdotdot) {
   if (m_options.m_pointer_as_array)
 return m_options.m_pointer_as_array.m_element_count;
 
-  auto num_children_or_err = synth_valobj.GetNumChildren();
+  const uint32_t max_num_children =
+  m_options.m_ignore_cap ? UINT32_MAX
+ : GetMostSpecializedValue()
+   .GetTargetSP()
+   ->GetMaximumNumberOfChildrenToDisplay();
+  // Ask for one more child than the maximum to see if we should print "...".
+  auto num_children_or_err = synth_valobj.GetNumChildren(
+  llvm::SaturatingAdd(max_num_children, uint32_t(1)));
   if (!num_children_or_err)
 return num_children_or_err;
-  uint32_t num_children = *num_children_or_err;
-  print_dotdotdot = false;
-  if (num_children) {
-const size_t max_num_children = GetMostSpecializedValue()
-.GetTargetSP()
-
->GetMaximumNumberOfChildrenToDisplay();
-
-if (num_children > max_num_children && !m_options.m_ignore_cap) {
-  print_dotdotdot = true;
-  return max_num_children;
-}
+  if (*num_children_or_err > max_num_children) {
+print_dotdotdot = true;
+return max_num_children;
   }
-  return num_children;
+  return num_children_or_err;
 }
 
 void ValueObjectPrinter::PrintChildrenPostamble(bool print_dotdotdot) {
diff 

[Lldb-commits] [lldb] [lldb][test] Disable PIE for some API tests (PR #93808)

[Pavel Labath via lldb-commits]

labath wrote:

Ok, I see now. It's an ld.bfd vs ld.lld thing. You probably have your clang 
configured to use lld. LLD does not put relocation addends into the data 
section (on both arm and intel). ld.bfd does, which is why this sort of happens 
to work there. Was your intention to test with LLD?

https://github.com/llvm/llvm-project/pull/93808
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][test] Add --sysroot argument to dotest.py (PR #93885)

[Pavel Labath via lldb-commits]

https://github.com/labath approved this pull request.


https://github.com/llvm/llvm-project/pull/93885
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][test] Add --target-os argument to dotest.py (PR #93887)

[Pavel Labath via lldb-commits]

labath wrote:

Would it be possible to pass this automatically from the python process? 
Basically set `OS` to the value of `lldbplatformutil.getPlatform()` ? We could 
keep the existing exception for Android, although I don't think that anyone 
tests Android these days (your PRs sort of demonstrate that)...

https://github.com/llvm/llvm-project/pull/93887
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][test] Add --make argument to dotest.py (PR #93883)

[Pavel Labath via lldb-commits]

@@ -40,7 +40,9 @@ def getMake(self, test_subdir, test_name):
 """Returns the invocation for GNU make.
 The first argument is a tuple of the relative path to the testcase
 and its filename stem."""
-if platform.system() == "FreeBSD" or platform.system() == "NetBSD":
+if configuration.make_path is not None:
+make = configuration.make_path
+elif platform.system() == "FreeBSD" or platform.system() == "NetBSD":
 make = "gmake"
 else:
 make = "make"

labath wrote:

I think it'd be nicer to move this code to dotest.py, as that's where we deal 
with locating the compiler, and all the other tools.

https://github.com/llvm/llvm-project/pull/93883
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][test] Add --make argument to dotest.py (PR #93883)

[Pavel Labath via lldb-commits]

https://github.com/labath edited https://github.com/llvm/llvm-project/pull/93883
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][test] Add --make argument to dotest.py (PR #93883)

[Pavel Labath via lldb-commits]

https://github.com/labath approved this pull request.


https://github.com/llvm/llvm-project/pull/93883
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb] Use packaging module instead of pkg_resources (PR #93712)

[Pavel Labath via lldb-commits]

labath wrote:

> @labath Does your ✅ mean the 
> [debian](https://lab.llvm.org/buildbot/#/builders/68) bot has this package 
> installed?

Affirmative.

> Did a bit of digging around; it looks like at the very least the Arch Linux 
> python package and the Python docker container don't contain the `packaging` 
> library. It's included in Arch's `python-pip` package, but having `pip` as a 
> dependency to test LLDB seems odd.

FWIW, there appears to be an actual arch package for this 
, so it shouldn't 
be (I think -- I'm not an Arch user) necessary to install `pip`.

That said, using a package designed (AIUI) for python versions for parsing 
versions of gcc/clang does strike me as somewhat... unusual, even if it does 
work, so ***maybe*** there is case to be made for writing something custom (I'm 
not sure if  we really need anything more elaborate than `tuple([int(part) for 
part in version.split(".")])`)

https://github.com/llvm/llvm-project/pull/93712
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][test] Add flags useful for remote cross-platform testing to dotest.py (PR #93800)

[Pavel Labath via lldb-commits]

labath wrote:

It might be better to split this up per flag. For example, I don't see any 
issues with the --make flag, but I would like to understand more about why you 
need the --os flag (like, how it differs/why can't that be deduced from the 
platform name?) The sysroot arg is probably also fine, but it'd be better 
viewed in isolation.

https://github.com/llvm/llvm-project/pull/93800
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb-dap] Added "port" property to vscode "attach" command. (PR #91570)

[Pavel Labath via lldb-commits]

@@ -0,0 +1,142 @@
+"""
+Test lldb-dap "port" configuration to "attach" request
+"""
+
+
+import dap_server
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+from lldbsuite.test import lldbplatformutil
+import lldbgdbserverutils
+import lldbdap_testcase
+import os
+import shutil
+import subprocess
+import tempfile
+import threading
+import time
+import sys
+import socket
+
+
+class TestDAP_attachByPortNum(lldbdap_testcase.DAPTestCaseBase):
+def get_free_port(self):
+s = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
+s.bind(("", 0))
+port = s.getsockname()[1]
+s.close()
+return port

labath wrote:

Fair enough. Let me try to rephrase.

The thing I want to avoid is races in port allocation. Your current algorithm 
works like this:
1. open a socket
2. use it to get a free port (number) from the OS
3. close the socket
4. pass the port number to lldb-server
5. have lldb-server listen on that port
6. connect to that port

This algorithm is racy for two reasons. One, because somewhere between the 
steps 3 and 5, someone else could start listening on that port. The second is 
because there's no sequencing between steps 5 and 6. Even if noone claims that 
port, if lldb-server just happens to be slower for whatever reason (that tends 
to happen more than you'd think when you're running many tests in parallel), 
the test can attempt to connect to the port before it gets opened.

Now, compare this to the algorithm in 
test/API/tools/lldb-server/commandline/TestGdbRemoteConnection.py 
(test_named_pipe)
1. lldb-server opens a socket
2. lldb-server uses it to get a free port from the OS
3. lldb-server starts listening on the port
4. lldb-server sends the port number to the test (through the named pipe)
5. the test connects to that port

There's no race here. The socket that's used for getting the port number is the 
same that's used for listening for incoming connections, so there's no 
possibility for someone to snatch it from under us. And because the test has to 
wait for the port number before it can connect (and because the lldb-server 
does steps 3 and 4 in this order), we can be sure the lldb-server will be 
listening by the time we connect.

That's not the only way to implement these things, but my suggestion is to do 
this, because the code is already there, and it shouldn't be too hard to make 
it usable in your test as well.

https://github.com/llvm/llvm-project/pull/91570
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][test] Disable PIE for some API tests (PR #93808)

[Pavel Labath via lldb-commits]

labath wrote:

What do the failures look like? I tried to force -pie on some of the tests that 
you modified, and I couldn't get them to fail. Is this somehow specific to 
remote execution?

https://github.com/llvm/llvm-project/pull/93808
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb] Fix 'session save' command on Windows (PR #93833)

[Pavel Labath via lldb-commits]

@@ -61,8 +61,14 @@ def test_session_save(self):
 self.assertFalse(res.Succeeded())
 raw += self.raw_transcript_builder(cmd, res)
 
-tf = tempfile.NamedTemporaryFile()
-output_file = tf.name
+fd, output_file = tempfile.mkstemp()

labath wrote:

I'd just put the file into the build directory of the test 
(`self.getBuildArtifact("my_session")`). Then you don't need to worry about 
cleanups or collisions, as that directory is nuked before running the test.

https://github.com/llvm/llvm-project/pull/93833
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits