[Lldb-commits] [lldb] ca64f9a - [lldb][DWARFASTParserClang][NFC] Simplify unnamed bitfield condition

[Michael Buch via lldb-commits]

Author: Michael Buch
Date: 2023-05-16T11:18:09+01:00
New Revision: ca64f9af04472a27656d84c06f4e332ebbf14b4d

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

LOG: [lldb][DWARFASTParserClang][NFC] Simplify unnamed bitfield condition

Minor cleanup of redundant variable initialization and
if-condition. These are leftovers/oversights from previous
cleanup in this area:
* https://reviews.llvm.org/D72953
* https://reviews.llvm.org/D76808

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

Added: 


Modified: 
lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp

Removed: 




diff  --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp 
b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
index 1090e15370dd2..7f67d25f58ae1 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
@@ -2880,9 +2880,8 @@ void DWARFASTParserClang::ParseSingleMember(
 
 if (detect_unnamed_bitfields) {
   std::optional unnamed_field_info;
-  uint64_t last_field_end = 0;
-
-  last_field_end = last_field_info.bit_offset + last_field_info.bit_size;
+  uint64_t last_field_end =
+  last_field_info.bit_offset + last_field_info.bit_size;
 
   if (!last_field_info.IsBitfield()) {
 // The last field was not a bit-field...
@@ -2902,10 +2901,8 @@ void DWARFASTParserClang::ParseSingleMember(
   // indeed an unnamed bit-field. We currently do not have the
   // machinary to track the offset of the last field of classes we
   // have seen before, so we are not handling this case.
-  if (this_field_info.bit_offset != last_field_end &&
-  this_field_info.bit_offset > last_field_end &&
-  !(last_field_info.bit_offset == 0 &&
-last_field_info.bit_size == 0 &&
+  if (this_field_info.bit_offset > last_field_end &&
+  !(last_field_info.bit_offset == 0 && last_field_info.bit_size == 0 &&
 layout_info.base_offsets.size() != 0)) {
 unnamed_field_info = FieldInfo{};
 unnamed_field_info->bit_size =



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

[Lldb-commits] [lldb] 56eff19 - [lldb][DWARFASTParserClang][NFC] Extract condition for unnamed bitfield creation into helper function

[Michael Buch via lldb-commits]

Author: Michael Buch
Date: 2023-05-16T11:18:09+01:00
New Revision: 56eff197d0d629c768e8e48fb995e8a1d2cd6441

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

LOG: [lldb][DWARFASTParserClang][NFC] Extract condition for unnamed bitfield 
creation into helper function

This patch adds a new private helper
`DWARFASTParserClang::ShouldCreateUnnamedBitfield` which
`ParseSingleMember` whether we should fill the current gap
in a structure layout with unnamed bitfields.

Extracting this logic will allow us to add additional
conditions in upcoming patches without jeoperdizing readability
of `ParseSingleMember`.

We also store some of the boolean conditions in local variables
to make the intent more obvious.

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

Added: 


Modified: 
lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h

Removed: 




diff  --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp 
b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
index 7f67d25f58ae..db3213aebdfb 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
@@ -2892,18 +2892,8 @@ void DWARFASTParserClang::ParseSingleMember(
   last_field_end += word_width - (last_field_end % word_width);
   }
 
-  // If we have a gap between the last_field_end and the current
-  // field we have an unnamed bit-field.
-  // If we have a base class, we assume there is no unnamed
-  // bit-field if this is the first field since the gap can be
-  // attributed to the members from the base class. This assumption
-  // is not correct if the first field of the derived class is
-  // indeed an unnamed bit-field. We currently do not have the
-  // machinary to track the offset of the last field of classes we
-  // have seen before, so we are not handling this case.
-  if (this_field_info.bit_offset > last_field_end &&
-  !(last_field_info.bit_offset == 0 && last_field_info.bit_size == 0 &&
-layout_info.base_offsets.size() != 0)) {
+  if (ShouldCreateUnnamedBitfield(last_field_info, last_field_end,
+  this_field_info, layout_info)) {
 unnamed_field_info = FieldInfo{};
 unnamed_field_info->bit_size =
 this_field_info.bit_offset - last_field_end;
@@ -3698,3 +3688,29 @@ bool DWARFASTParserClang::CopyUniqueClassMethodTypes(
 
   return !failures.empty();
 }
+
+bool DWARFASTParserClang::ShouldCreateUnnamedBitfield(
+FieldInfo const &last_field_info, uint64_t last_field_end,
+FieldInfo const &this_field_info,
+lldb_private::ClangASTImporter::LayoutInfo const &layout_info) const {
+  // If we have a gap between the last_field_end and the current
+  // field we have an unnamed bit-field.
+  if (this_field_info.bit_offset <= last_field_end)
+return false;
+
+  // If we have a base class, we assume there is no unnamed
+  // bit-field if this is the first field since the gap can be
+  // attributed to the members from the base class. This assumption
+  // is not correct if the first field of the derived class is
+  // indeed an unnamed bit-field. We currently do not have the
+  // machinary to track the offset of the last field of classes we
+  // have seen before, so we are not handling this case.
+  const bool have_base = layout_info.base_offsets.size() != 0;
+  const bool this_is_first_field =
+  last_field_info.bit_offset == 0 && last_field_info.bit_size == 0;
+
+  if (have_base && this_is_first_field)
+return false;
+
+  return true;
+}

diff  --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h 
b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h
index a4613afafd54..9edf718ba921 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h
@@ -236,6 +236,22 @@ class DWARFASTParserClang : public DWARFASTParser {
 }
   };
 
+  /// Returns 'true' if we should create an unnamed bitfield
+  /// and add it to the parser's current AST.
+  ///
+  /// \param[in] last_field_info FieldInfo of the previous DW_TAG_member
+  ///we parsed.
+  /// \param[in] last_field_end Offset (in bits) where the last parsed field
+  ///ended.
+  /// \param[in] this_field_info FieldInfo of the current DW_TAG_member
+  ///being parsed.
+  /// \param[in] layout_info Layout information of all decls parsed by the
+  ///current parser.
+  bool ShouldCreateUnnamedBitfield(
+  FieldInfo const &last_field_info, uint64_t last_field_end,
+  FieldInfo const &this_field_inf

[Lldb-commits] [lldb] 3c30f22 - [lldb][DWARFASTParserClang] Don't create unnamed bitfields to account for vtable pointer

[Michael Buch via lldb-commits]

Author: Michael Buch
Date: 2023-05-16T11:18:09+01:00
New Revision: 3c30f224005e3749c89e00592bd2a43aba2aaf75

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

LOG: [lldb][DWARFASTParserClang] Don't create unnamed bitfields to account for 
vtable pointer

**Summary**

When filling out the LayoutInfo for a structure with the offsets
from DWARF, LLDB fills gaps in the layout by creating unnamed
bitfields and adding them to the AST. If we don't do this correctly
and our layout has overlapping fields, we will hat an assertion
in `clang::CGRecordLowering::lower()`. Specifically, if we have
a derived class with a VTable and a bitfield immediately following
the vtable pointer, we create a layout with overlapping fields.

This is an oversight in some of the previous cleanups done around this
area.

In `D76808`, we prevented LLDB from creating unnamed bitfields if there
was a gap between the last field of a base class and the start of a bitfield
in the derived class.

In `D112697`, we started accounting for the vtable pointer. The intention
there was to make sure the offset bookkeeping accounted for the
existence of a vtable pointer (but we didn't actually want to create
any AST nodes for it). Now that `last_field_info.bit_size` was being
set even for artifical fields, the previous fix `D76808` broke
specifically for cases where the bitfield was the first member of a
derived class with a vtable (this scenario wasn't tested so we didn't
notice it). I.e., we started creating redundant unnamed bitfields for
where the vtable pointer usually sits. This confused the lowering logic
in clang.

This patch adds a condition to `ShouldCreateUnnamedBitfield` which
checks whether the first field in the derived class is a vtable ptr.

**Testing**

* Added API test case

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

Added: 


Modified: 
lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h
lldb/test/API/lang/cpp/bitfields/TestCppBitfields.py
lldb/test/API/lang/cpp/bitfields/main.cpp

Removed: 




diff  --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp 
b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
index db3213aebdfb6..922f50d33581e 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
@@ -2934,8 +2934,10 @@ void DWARFASTParserClang::ParseSingleMember(
   // artificial member with (unnamed bitfield) padding.
   // FIXME: This check should verify that this is indeed an artificial member
   // we are supposed to ignore.
-  if (attrs.is_artificial)
+  if (attrs.is_artificial) {
+last_field_info.SetIsArtificial(true);
 return;
+  }
 
   if (!member_clang_type.IsCompleteType())
 member_clang_type.GetCompleteType();
@@ -3699,17 +3701,23 @@ bool DWARFASTParserClang::ShouldCreateUnnamedBitfield(
 return false;
 
   // If we have a base class, we assume there is no unnamed
-  // bit-field if this is the first field since the gap can be
-  // attributed to the members from the base class. This assumption
-  // is not correct if the first field of the derived class is
-  // indeed an unnamed bit-field. We currently do not have the
-  // machinary to track the offset of the last field of classes we
-  // have seen before, so we are not handling this case.
+  // bit-field if either of the following is true:
+  // (a) this is the first field since the gap can be
+  // attributed to the members from the base class.
+  // FIXME: This assumption is not correct if the first field of
+  // the derived class is indeed an unnamed bit-field. We currently
+  // do not have the machinary to track the offset of the last field
+  // of classes we have seen before, so we are not handling this case.
+  // (b) Or, the first member of the derived class was a vtable pointer.
+  // In this case we don't want to create an unnamed bitfield either
+  // since those will be inserted by clang later.
   const bool have_base = layout_info.base_offsets.size() != 0;
   const bool this_is_first_field =
   last_field_info.bit_offset == 0 && last_field_info.bit_size == 0;
+  const bool first_field_is_vptr =
+  last_field_info.bit_offset == 0 && last_field_info.IsArtificial();
 
-  if (have_base && this_is_first_field)
+  if (have_base && (this_is_first_field || first_field_is_vptr))
 return false;
 
   return true;

diff  --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h 
b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h
index 9edf718ba9215..042075b7df5f0 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserCl

[Lldb-commits] [PATCH] D150590: [lldb][DWARFASTParserClang][NFC] Extract condition for unnamed bitfield creation into helper function

[Michael Buch via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG56eff197d0d6: [lldb][DWARFASTParserClang][NFC] Extract 
condition for unnamed bitfield… (authored by Michael137).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150590

Files:
  lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h


Index: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h
===
--- lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h
+++ lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h
@@ -236,6 +236,22 @@
 }
   };
 
+  /// Returns 'true' if we should create an unnamed bitfield
+  /// and add it to the parser's current AST.
+  ///
+  /// \param[in] last_field_info FieldInfo of the previous DW_TAG_member
+  ///we parsed.
+  /// \param[in] last_field_end Offset (in bits) where the last parsed field
+  ///ended.
+  /// \param[in] this_field_info FieldInfo of the current DW_TAG_member
+  ///being parsed.
+  /// \param[in] layout_info Layout information of all decls parsed by the
+  ///current parser.
+  bool ShouldCreateUnnamedBitfield(
+  FieldInfo const &last_field_info, uint64_t last_field_end,
+  FieldInfo const &this_field_info,
+  lldb_private::ClangASTImporter::LayoutInfo const &layout_info) const;
+
   /// Parses a DW_TAG_APPLE_property DIE and appends the parsed data to the
   /// list of delayed Objective-C properties.
   ///
Index: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
@@ -2892,18 +2892,8 @@
   last_field_end += word_width - (last_field_end % word_width);
   }
 
-  // If we have a gap between the last_field_end and the current
-  // field we have an unnamed bit-field.
-  // If we have a base class, we assume there is no unnamed
-  // bit-field if this is the first field since the gap can be
-  // attributed to the members from the base class. This assumption
-  // is not correct if the first field of the derived class is
-  // indeed an unnamed bit-field. We currently do not have the
-  // machinary to track the offset of the last field of classes we
-  // have seen before, so we are not handling this case.
-  if (this_field_info.bit_offset > last_field_end &&
-  !(last_field_info.bit_offset == 0 && last_field_info.bit_size == 0 &&
-layout_info.base_offsets.size() != 0)) {
+  if (ShouldCreateUnnamedBitfield(last_field_info, last_field_end,
+  this_field_info, layout_info)) {
 unnamed_field_info = FieldInfo{};
 unnamed_field_info->bit_size =
 this_field_info.bit_offset - last_field_end;
@@ -3698,3 +3688,29 @@
 
   return !failures.empty();
 }
+
+bool DWARFASTParserClang::ShouldCreateUnnamedBitfield(
+FieldInfo const &last_field_info, uint64_t last_field_end,
+FieldInfo const &this_field_info,
+lldb_private::ClangASTImporter::LayoutInfo const &layout_info) const {
+  // If we have a gap between the last_field_end and the current
+  // field we have an unnamed bit-field.
+  if (this_field_info.bit_offset <= last_field_end)
+return false;
+
+  // If we have a base class, we assume there is no unnamed
+  // bit-field if this is the first field since the gap can be
+  // attributed to the members from the base class. This assumption
+  // is not correct if the first field of the derived class is
+  // indeed an unnamed bit-field. We currently do not have the
+  // machinary to track the offset of the last field of classes we
+  // have seen before, so we are not handling this case.
+  const bool have_base = layout_info.base_offsets.size() != 0;
+  const bool this_is_first_field =
+  last_field_info.bit_offset == 0 && last_field_info.bit_size == 0;
+
+  if (have_base && this_is_first_field)
+return false;
+
+  return true;
+}


Index: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h
===
--- lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h
+++ lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h
@@ -236,6 +236,22 @@
 }
   };
 
+  /// Returns 'true' if we should create an unnamed bitfield
+  /// and add it to the parser's current AST.
+  ///
+  /// \param[in] last_field_info FieldInfo of the previous DW_TAG_member
+  ///we parsed.
+  /// \param[in] last_field_end Offset (in bits) where the last parsed field
+  ///ended.
+  /// \param[in] this_field_info FieldInfo of the current DW_TAG_member
+  /// 

[Lldb-commits] [PATCH] D150589: [lldb][DWARFASTParserClang][NFC] Simplify unnamed bitfield condition

[Michael Buch via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGca64f9af0447: [lldb][DWARFASTParserClang][NFC] Simplify 
unnamed bitfield condition (authored by Michael137).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150589

Files:
  lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp


Index: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
@@ -2880,9 +2880,8 @@
 
 if (detect_unnamed_bitfields) {
   std::optional unnamed_field_info;
-  uint64_t last_field_end = 0;
-
-  last_field_end = last_field_info.bit_offset + last_field_info.bit_size;
+  uint64_t last_field_end =
+  last_field_info.bit_offset + last_field_info.bit_size;
 
   if (!last_field_info.IsBitfield()) {
 // The last field was not a bit-field...
@@ -2902,10 +2901,8 @@
   // indeed an unnamed bit-field. We currently do not have the
   // machinary to track the offset of the last field of classes we
   // have seen before, so we are not handling this case.
-  if (this_field_info.bit_offset != last_field_end &&
-  this_field_info.bit_offset > last_field_end &&
-  !(last_field_info.bit_offset == 0 &&
-last_field_info.bit_size == 0 &&
+  if (this_field_info.bit_offset > last_field_end &&
+  !(last_field_info.bit_offset == 0 && last_field_info.bit_size == 0 &&
 layout_info.base_offsets.size() != 0)) {
 unnamed_field_info = FieldInfo{};
 unnamed_field_info->bit_size =


Index: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
@@ -2880,9 +2880,8 @@
 
 if (detect_unnamed_bitfields) {
   std::optional unnamed_field_info;
-  uint64_t last_field_end = 0;
-
-  last_field_end = last_field_info.bit_offset + last_field_info.bit_size;
+  uint64_t last_field_end =
+  last_field_info.bit_offset + last_field_info.bit_size;
 
   if (!last_field_info.IsBitfield()) {
 // The last field was not a bit-field...
@@ -2902,10 +2901,8 @@
   // indeed an unnamed bit-field. We currently do not have the
   // machinary to track the offset of the last field of classes we
   // have seen before, so we are not handling this case.
-  if (this_field_info.bit_offset != last_field_end &&
-  this_field_info.bit_offset > last_field_end &&
-  !(last_field_info.bit_offset == 0 &&
-last_field_info.bit_size == 0 &&
+  if (this_field_info.bit_offset > last_field_end &&
+  !(last_field_info.bit_offset == 0 && last_field_info.bit_size == 0 &&
 layout_info.base_offsets.size() != 0)) {
 unnamed_field_info = FieldInfo{};
 unnamed_field_info->bit_size =
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D150591: [lldb][DWARFASTParserClang] Don't create unnamed bitfields to account for vtable pointer

[Michael Buch via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG3c30f224005e: [lldb][DWARFASTParserClang] Don't create 
unnamed bitfields to account for… (authored by Michael137).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150591

Files:
  lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h
  lldb/test/API/lang/cpp/bitfields/TestCppBitfields.py
  lldb/test/API/lang/cpp/bitfields/main.cpp

Index: lldb/test/API/lang/cpp/bitfields/main.cpp
===
--- lldb/test/API/lang/cpp/bitfields/main.cpp
+++ lldb/test/API/lang/cpp/bitfields/main.cpp
@@ -113,6 +113,12 @@
 };
 HasBaseWithVTable base_with_vtable;
 
+struct DerivedWithVTable : public Base {
+  virtual ~DerivedWithVTable() {}
+  unsigned a : 1;
+};
+DerivedWithVTable derived_with_vtable;
+
 int main(int argc, char const *argv[]) {
   lba.a = 2;
 
@@ -153,5 +159,8 @@
   base_with_vtable.b = 0;
   base_with_vtable.c = 5;
 
+  derived_with_vtable.b_a = 2;
+  derived_with_vtable.a = 1;
+
   return 0; // break here
 }
Index: lldb/test/API/lang/cpp/bitfields/TestCppBitfields.py
===
--- lldb/test/API/lang/cpp/bitfields/TestCppBitfields.py
+++ lldb/test/API/lang/cpp/bitfields/TestCppBitfields.py
@@ -168,3 +168,14 @@
  result_children=with_vtable_and_unnamed_children)
 self.expect_var_path("with_vtable_and_unnamed",
  children=with_vtable_and_unnamed_children)
+
+derived_with_vtable_children = [
+ValueCheck(name="Base", children=[
+  ValueCheck(name="b_a", value="2", type="uint32_t")
+]),
+ValueCheck(name="a", value="1", type="unsigned int:1")
+]
+self.expect_expr("derived_with_vtable",
+ result_children=derived_with_vtable_children)
+self.expect_var_path("derived_with_vtable",
+ children=derived_with_vtable_children)
Index: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h
===
--- lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h
+++ lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h
@@ -223,12 +223,16 @@
 uint64_t bit_size = 0;
 uint64_t bit_offset = 0;
 bool is_bitfield = false;
+bool is_artificial = false;
 
 FieldInfo() = default;
 
 void SetIsBitfield(bool flag) { is_bitfield = flag; }
 bool IsBitfield() { return is_bitfield; }
 
+void SetIsArtificial(bool flag) { is_artificial = flag; }
+bool IsArtificial() const { return is_artificial; }
+
 bool NextBitfieldOffsetIsValid(const uint64_t next_bit_offset) const {
   // Any subsequent bitfields must not overlap and must be at a higher
   // bit offset than any previous bitfield + size.
Index: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
@@ -2934,8 +2934,10 @@
   // artificial member with (unnamed bitfield) padding.
   // FIXME: This check should verify that this is indeed an artificial member
   // we are supposed to ignore.
-  if (attrs.is_artificial)
+  if (attrs.is_artificial) {
+last_field_info.SetIsArtificial(true);
 return;
+  }
 
   if (!member_clang_type.IsCompleteType())
 member_clang_type.GetCompleteType();
@@ -3699,17 +3701,23 @@
 return false;
 
   // If we have a base class, we assume there is no unnamed
-  // bit-field if this is the first field since the gap can be
-  // attributed to the members from the base class. This assumption
-  // is not correct if the first field of the derived class is
-  // indeed an unnamed bit-field. We currently do not have the
-  // machinary to track the offset of the last field of classes we
-  // have seen before, so we are not handling this case.
+  // bit-field if either of the following is true:
+  // (a) this is the first field since the gap can be
+  // attributed to the members from the base class.
+  // FIXME: This assumption is not correct if the first field of
+  // the derived class is indeed an unnamed bit-field. We currently
+  // do not have the machinary to track the offset of the last field
+  // of classes we have seen before, so we are not handling this case.
+  // (b) Or, the first member of the derived class was a vtable pointer.
+  // In this case we don't want to create an unnamed bitfield either
+  // since those will be inserted by clang later.
   const bool have_base = layout_info.base_offsets.size() != 0;
   const bool this_is_first_field =
   last_field_info.bit_offset == 0 && last_field

[Lldb-commits] [PATCH] D128612: RISC-V big-endian support implementation

[Alex Bradbury via Phabricator via lldb-commits]

asb added a comment.

Thanks for this patch Guy. As just discussed in the RISC-V sync-up call, it 
would be helpful from a review perspective to write down at least a simple 
plain-text description of the changes to the psABI doc needed to reflect the BE 
ABI implemented by GCC (and soon LLVM), perhaps in an issue.

I think this patch is lacking some test coverage around things like fixup 
handling (e.g. the logic to swap fixups).

In D128612#4337037 , @djtodoro wrote:

> Hi! I am wondering if someone knows what is the status of this.

I've not seen any further progress. I think it needs at a minimum a PR against 
the psABI doc for big endian that we can review against.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128612

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

[Lldb-commits] [PATCH] D146058: [lldb][gnustep] Add basic test and infrastructure for GNUstep ObjC runtime

[Stefan Gränitz via Phabricator via lldb-commits]

sgraenitz updated this revision to Diff 522582.
sgraenitz added a comment.

Rebase and re-run pre-merge checks


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146058

Files:
  lldb/cmake/modules/FindGNUstepObjC.cmake
  lldb/test/CMakeLists.txt
  lldb/test/Shell/Expr/objc-gnustep-print.m
  lldb/test/Shell/helper/build.py
  lldb/test/Shell/helper/toolchain.py
  lldb/test/Shell/lit.cfg.py
  lldb/test/Shell/lit.site.cfg.py.in

Index: lldb/test/Shell/lit.site.cfg.py.in
===
--- lldb/test/Shell/lit.site.cfg.py.in
+++ lldb/test/Shell/lit.site.cfg.py.in
@@ -16,6 +16,7 @@
 config.target_triple = "@LLVM_TARGET_TRIPLE@"
 config.python_executable = "@Python3_EXECUTABLE@"
 config.have_zlib = @LLVM_ENABLE_ZLIB@
+config.objc_gnustep_dir = "@LLDB_TEST_OBJC_GNUSTEP_DIR@"
 config.lldb_enable_lzma = @LLDB_ENABLE_LZMA@
 config.host_triple = "@LLVM_HOST_TRIPLE@"
 config.lldb_bitness = 64 if @LLDB_IS_64_BITS@ else 32
Index: lldb/test/Shell/lit.cfg.py
===
--- lldb/test/Shell/lit.cfg.py
+++ lldb/test/Shell/lit.cfg.py
@@ -24,7 +24,7 @@
 
 # suffixes: A list of file extensions to treat as test files. This is overriden
 # by individual lit.local.cfg files in the test subdirectories.
-config.suffixes = ['.test', '.cpp', '.s']
+config.suffixes = ['.test', '.cpp', '.s', '.m']
 
 # excludes: A list of directories to exclude from the testsuite. The 'Inputs'
 # subdirectories contain auxiliary inputs for various tests in their parent
@@ -135,6 +135,14 @@
 if config.have_lldb_server:
 config.available_features.add('lldb-server')
 
+if config.objc_gnustep_dir:
+config.available_features.add('objc-gnustep')
+if platform.system() == 'Windows':
+# objc.dll must be in PATH since Windows has no rpath
+config.environment['PATH'] = os.path.pathsep.join((
+os.path.join(config.objc_gnustep_dir, 'lib'),
+config.environment.get('PATH','')))
+
 # NetBSD permits setting dbregs either if one is root
 # or if user_set_dbregs is enabled
 can_set_dbregs = True
Index: lldb/test/Shell/helper/toolchain.py
===
--- lldb/test/Shell/helper/toolchain.py
+++ lldb/test/Shell/helper/toolchain.py
@@ -42,6 +42,8 @@
 build_script_args.append('--tools-dir={0}'.format(config.lldb_tools_dir))
 if config.llvm_libs_dir:
 build_script_args.append('--libs-dir={0}'.format(config.llvm_libs_dir))
+if config.objc_gnustep_dir:
+build_script_args.append('--objc-gnustep-dir="{0}"'.format(config.objc_gnustep_dir))
 
 lldb_init = _get_lldb_init_path(config)
 
Index: lldb/test/Shell/helper/build.py
===
--- lldb/test/Shell/helper/build.py
+++ lldb/test/Shell/helper/build.py
@@ -49,6 +49,18 @@
 action='append',
 help='If specified, a path to search in addition to PATH when --compiler is not an exact path')
 
+parser.add_argument('--objc-gnustep-dir',
+metavar='directory',
+dest='objc_gnustep_dir',
+required=False,
+help='If specified, a path to GNUstep libobjc2 runtime for use on Windows and Linux')
+
+parser.add_argument('--objc-gnustep',
+dest='objc_gnustep',
+action='store_true',
+default=False,
+help='Include and link GNUstep libobjc2 (Windows and Linux only)')
+
 if sys.platform == 'darwin':
 parser.add_argument('--apple-sdk',
 metavar='apple_sdk',
@@ -238,6 +250,10 @@
 self.obj_ext = obj_ext
 self.lib_paths = args.libs_dir
 self.std = args.std
+assert not args.objc_gnustep or args.objc_gnustep_dir, \
+   "--objc-gnustep specified without path to libobjc2"
+self.objc_gnustep_inc = os.path.join(args.objc_gnustep_dir, 'include') if args.objc_gnustep_dir else None
+self.objc_gnustep_lib = os.path.join(args.objc_gnustep_dir, 'lib') if args.objc_gnustep_dir else None
 
 def _exe_file_name(self):
 assert self.mode != 'compile'
@@ -656,15 +672,20 @@
 args.append('-static')
 args.append('-c')
 
-args.extend(['-o', obj])
-args.append(source)
-
 if sys.platform == 'darwin':
 args.extend(['-isysroot', self.apple_sdk])
+elif self.objc_gnustep_inc:
+if source.endswith('.m') or source.endswith('.mm'):
+args.extend(['-fobjc-runtime=gnustep-2.0', '-I', self.objc_gnustep_inc])
+if sys.platform == "win32":
+args.extend(['-Xclang', '-gcodeview', '-Xclang', '--dependent-lib=msvcrtd'])
 
 if self.std:
 args.append('-std={0}'.format(self.std))
 

[Lldb-commits] [PATCH] D150639: [lldb] Define lldbassert based on NDEBUG instead of LLDB_CONFIGURATION_DEBUG

[Jordan Rupprecht via Phabricator via lldb-commits]

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

+1 to being surprised this is not already the case

Some other places should be updated after this, e.g. 
lldb/source/Symbol/SymbolFile.cpp also has a use that can be trivially updated:

  #if defined(LLDB_CONFIGURATION_DEBUG)
// We assert that we have to module lock by trying to acquire the lock from 
a
// different thread. Note that we must abort if the result is true to
// guarantee correctness.
assert(std::async(
   std::launch::async,
   [this] {
 return this->GetModuleMutex().try_lock();
   }).get() == false &&
   "Module is not locked");
  #endif

-> just wrap it in lldbassert

In D150639#4344695 , @JDevlieghere 
wrote:

> This patch also uses `__FILE_NAME__` (Clang-specific extension that functions 
> similar to __FILE__ but only renders the last path component (the filename) 
> instead of an invocation dependent full path to that file.) when building 
> with clang.

Can you put this comment in `LLDBAssert.h` itself?


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

https://reviews.llvm.org/D150639

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

[Lldb-commits] [PATCH] D150639: [lldb] Define lldbassert based on NDEBUG instead of LLDB_CONFIGURATION_DEBUG

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

In D150639#4346009 , @rupprecht wrote:

> +1 to being surprised this is not already the case
>
> Some other places should be updated after this, e.g. 
> lldb/source/Symbol/SymbolFile.cpp also has a use that can be trivially 
> updated:
>
>   #if defined(LLDB_CONFIGURATION_DEBUG)
> // We assert that we have to module lock by trying to acquire the lock 
> from a
> // different thread. Note that we must abort if the result is true to
> // guarantee correctness.
> assert(std::async(
>std::launch::async,
>[this] {
>  return this->GetModuleMutex().try_lock();
>}).get() == false &&
>"Module is not locked");
>   #endif
>
> -> just wrap it in lldbassert

This code actually has a comment right above it explaining that it's too 
expensive to do in release builds, so this isn't a good candidate for 
`lldbassert`. But this logic can be improved: it duplicates checking the 
`LLDB_CONFIGURATION_DEBUG` in both `ASSERT_MODULE_LOCK` and `AssertModuleLock` 
and should use `NDEBUG` too. I'll address that in a separate patch.

> In D150639#4344695 , @JDevlieghere 
> wrote:
>
>> This patch also uses `__FILE_NAME__` (Clang-specific extension that 
>> functions similar to __FILE__ but only renders the last path component (the 
>> filename) instead of an invocation dependent full path to that file.) when 
>> building with clang.
>
> Can you put this comment in `LLDBAssert.h` itself?

Good idea.


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

https://reviews.llvm.org/D150639

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

[Lldb-commits] [lldb] 10a5076 - [lldb] Define lldbassert based on NDEBUG instead of LLDB_CONFIGURATION_DEBUG

[Jonas Devlieghere via lldb-commits]

Author: Jonas Devlieghere
Date: 2023-05-16T09:27:09-07:00
New Revision: 10a50762caa6ac17dd063b28863a2ec60576c6f7

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

LOG: [lldb] Define lldbassert based on NDEBUG instead of 
LLDB_CONFIGURATION_DEBUG

Whether assertions are enabled or not is orthogonal to the build type
which could lead to surprising behavior for lldbassert. Previously, when
doing a debug build with assertions disabled, lldbassert would become a
NOOP, rather than printing an error like it does in a release build. By
definining lldbassert in terms of NDEBUG, it behaves like a regular
assert when assertions are enabled, and like a soft assert.

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

Added: 


Modified: 
lldb/docs/resources/contributing.rst
lldb/include/lldb/Utility/LLDBAssert.h
lldb/source/Utility/LLDBAssert.cpp

Removed: 




diff  --git a/lldb/docs/resources/contributing.rst 
b/lldb/docs/resources/contributing.rst
index 5f4b24a8013e7..26320f6353438 100644
--- a/lldb/docs/resources/contributing.rst
+++ b/lldb/docs/resources/contributing.rst
@@ -66,8 +66,8 @@ rules of thumb:
 
 * Soft assertions.  LLDB provides ``lldbassert()`` as a soft
   alternative to cover the middle ground of situations that indicate a
-  recoverable bug in LLDB.  In a Debug configuration ``lldbassert()``
-  behaves like ``assert()``. In a Release configuration it will print a
+  recoverable bug in LLDB.  When asserts are enabled ``lldbassert()``
+  behaves like ``assert()``. When asserts are disabled, it will print a
   warning and encourage the user to file a bug report, similar to
   LLVM's crash handler, and then return execution. Use these sparingly
   and only if error handling is not otherwise feasible.  Specifically,

diff  --git a/lldb/include/lldb/Utility/LLDBAssert.h 
b/lldb/include/lldb/Utility/LLDBAssert.h
index 471a2f7e824fc..524f56fd77c80 100644
--- a/lldb/include/lldb/Utility/LLDBAssert.h
+++ b/lldb/include/lldb/Utility/LLDBAssert.h
@@ -9,13 +9,22 @@
 #ifndef LLDB_UTILITY_LLDBASSERT_H
 #define LLDB_UTILITY_LLDBASSERT_H
 
-#ifdef LLDB_CONFIGURATION_DEBUG
+#ifndef NDEBUG
 #define lldbassert(x) assert(x)
 #else
+#if defined(__clang__)
+// __FILE_NAME__ is a Clang-specific extension that functions similar to
+// __FILE__ but only renders the last path component (the filename) instead of
+// an invocation dependent full path to that file.
+#define lldbassert(x)  
\
+  lldb_private::lldb_assert(static_cast(x), #x, __FUNCTION__,
\
+__FILE_NAME__, __LINE__)
+#else
 #define lldbassert(x)  
\
   lldb_private::lldb_assert(static_cast(x), #x, __FUNCTION__, __FILE__,  
\
 __LINE__)
 #endif
+#endif
 
 namespace lldb_private {
 void lldb_assert(bool expression, const char *expr_text, const char *func,

diff  --git a/lldb/source/Utility/LLDBAssert.cpp 
b/lldb/source/Utility/LLDBAssert.cpp
index a8d8ef65a9455..17689582cdc58 100644
--- a/lldb/source/Utility/LLDBAssert.cpp
+++ b/lldb/source/Utility/LLDBAssert.cpp
@@ -25,9 +25,6 @@ void lldb_private::lldb_assert(bool expression, const char 
*expr_text,
   if (LLVM_LIKELY(expression))
 return;
 
-  // If asserts are enabled abort here.
-  assert(false && "lldb_assert failed");
-
 #if LLVM_SUPPORT_XCODE_SIGNPOSTS
   if (__builtin_available(macos 10.12, iOS 10, tvOS 10, watchOS 3, *)) {
 os_log_fault(OS_LOG_DEFAULT,
@@ -36,9 +33,8 @@ void lldb_private::lldb_assert(bool expression, const char 
*expr_text,
   }
 #endif
 
-  // In a release configuration it will print a warning and encourage the user
-  // to file a bug report, similar to LLVM’s crash handler, and then return
-  // execution.
+  // Print a warning and encourage the user to file a bug report, similar to
+  // LLVM’s crash handler, and then return execution.
   errs() << format("Assertion failed: (%s), function %s, file %s, line %u\n",
expr_text, func, file, line);
   errs() << "backtrace leading to the failure:\n";



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

[Lldb-commits] [PATCH] D150639: [lldb] Define lldbassert based on NDEBUG instead of LLDB_CONFIGURATION_DEBUG

[Jonas Devlieghere via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG10a50762caa6: [lldb] Define lldbassert based on NDEBUG 
instead of LLDB_CONFIGURATION_DEBUG (authored by JDevlieghere).
Herald added a project: LLDB.

Changed prior to commit:
  https://reviews.llvm.org/D150639?vs=522442&id=522664#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150639

Files:
  lldb/docs/resources/contributing.rst
  lldb/include/lldb/Utility/LLDBAssert.h
  lldb/source/Utility/LLDBAssert.cpp


Index: lldb/source/Utility/LLDBAssert.cpp
===
--- lldb/source/Utility/LLDBAssert.cpp
+++ lldb/source/Utility/LLDBAssert.cpp
@@ -25,9 +25,6 @@
   if (LLVM_LIKELY(expression))
 return;
 
-  // If asserts are enabled abort here.
-  assert(false && "lldb_assert failed");
-
 #if LLVM_SUPPORT_XCODE_SIGNPOSTS
   if (__builtin_available(macos 10.12, iOS 10, tvOS 10, watchOS 3, *)) {
 os_log_fault(OS_LOG_DEFAULT,
@@ -36,9 +33,8 @@
   }
 #endif
 
-  // In a release configuration it will print a warning and encourage the user
-  // to file a bug report, similar to LLVM’s crash handler, and then return
-  // execution.
+  // Print a warning and encourage the user to file a bug report, similar to
+  // LLVM’s crash handler, and then return execution.
   errs() << format("Assertion failed: (%s), function %s, file %s, line %u\n",
expr_text, func, file, line);
   errs() << "backtrace leading to the failure:\n";
Index: lldb/include/lldb/Utility/LLDBAssert.h
===
--- lldb/include/lldb/Utility/LLDBAssert.h
+++ lldb/include/lldb/Utility/LLDBAssert.h
@@ -9,13 +9,22 @@
 #ifndef LLDB_UTILITY_LLDBASSERT_H
 #define LLDB_UTILITY_LLDBASSERT_H
 
-#ifdef LLDB_CONFIGURATION_DEBUG
+#ifndef NDEBUG
 #define lldbassert(x) assert(x)
 #else
+#if defined(__clang__)
+// __FILE_NAME__ is a Clang-specific extension that functions similar to
+// __FILE__ but only renders the last path component (the filename) instead of
+// an invocation dependent full path to that file.
+#define lldbassert(x)  
\
+  lldb_private::lldb_assert(static_cast(x), #x, __FUNCTION__,
\
+__FILE_NAME__, __LINE__)
+#else
 #define lldbassert(x)  
\
   lldb_private::lldb_assert(static_cast(x), #x, __FUNCTION__, __FILE__,  
\
 __LINE__)
 #endif
+#endif
 
 namespace lldb_private {
 void lldb_assert(bool expression, const char *expr_text, const char *func,
Index: lldb/docs/resources/contributing.rst
===
--- lldb/docs/resources/contributing.rst
+++ lldb/docs/resources/contributing.rst
@@ -66,8 +66,8 @@
 
 * Soft assertions.  LLDB provides ``lldbassert()`` as a soft
   alternative to cover the middle ground of situations that indicate a
-  recoverable bug in LLDB.  In a Debug configuration ``lldbassert()``
-  behaves like ``assert()``. In a Release configuration it will print a
+  recoverable bug in LLDB.  When asserts are enabled ``lldbassert()``
+  behaves like ``assert()``. When asserts are disabled, it will print a
   warning and encourage the user to file a bug report, similar to
   LLVM's crash handler, and then return execution. Use these sparingly
   and only if error handling is not otherwise feasible.  Specifically,


Index: lldb/source/Utility/LLDBAssert.cpp
===
--- lldb/source/Utility/LLDBAssert.cpp
+++ lldb/source/Utility/LLDBAssert.cpp
@@ -25,9 +25,6 @@
   if (LLVM_LIKELY(expression))
 return;
 
-  // If asserts are enabled abort here.
-  assert(false && "lldb_assert failed");
-
 #if LLVM_SUPPORT_XCODE_SIGNPOSTS
   if (__builtin_available(macos 10.12, iOS 10, tvOS 10, watchOS 3, *)) {
 os_log_fault(OS_LOG_DEFAULT,
@@ -36,9 +33,8 @@
   }
 #endif
 
-  // In a release configuration it will print a warning and encourage the user
-  // to file a bug report, similar to LLVM’s crash handler, and then return
-  // execution.
+  // Print a warning and encourage the user to file a bug report, similar to
+  // LLVM’s crash handler, and then return execution.
   errs() << format("Assertion failed: (%s), function %s, file %s, line %u\n",
expr_text, func, file, line);
   errs() << "backtrace leading to the failure:\n";
Index: lldb/include/lldb/Utility/LLDBAssert.h
===
--- lldb/include/lldb/Utility/LLDBAssert.h
+++ lldb/include/lldb/Utility/LLDBAssert.h
@@ -9,13 +9,22 @@
 #ifndef LLDB_UTILITY_LLDBASSERT_H
 #define LLDB_UTILITY_LLDBASSERT_H
 
-#ifdef LLDB_CONFIGURATION_DEBUG
+#ifndef NDEBUG
 #define lldbassert(x) assert(x)
 #else
+#if defined(__clang_

[Lldb-commits] [PATCH] D150639: [lldb] Define lldbassert based on NDEBUG instead of LLDB_CONFIGURATION_DEBUG

[Jordan Rupprecht via Phabricator via lldb-commits]

rupprecht added a comment.

In D150639#4346649 , @JDevlieghere 
wrote:

> In D150639#4346009 , @rupprecht 
> wrote:
>
>> +1 to being surprised this is not already the case
>>
>> Some other places should be updated after this, e.g. 
>> lldb/source/Symbol/SymbolFile.cpp also has a use that can be trivially 
>> updated:
>>
>>   #if defined(LLDB_CONFIGURATION_DEBUG)
>> // We assert that we have to module lock by trying to acquire the lock 
>> from a
>> // different thread. Note that we must abort if the result is true to
>> // guarantee correctness.
>> assert(std::async(
>>std::launch::async,
>>[this] {
>>  return this->GetModuleMutex().try_lock();
>>}).get() == false &&
>>"Module is not locked");
>>   #endif
>>
>> -> just wrap it in lldbassert
>
> This code actually has a comment right above it explaining that it's too 
> expensive to do in release builds, so this isn't a good candidate for 
> `lldbassert`. But this logic can be improved: it duplicates checking the 
> `LLDB_CONFIGURATION_DEBUG` in both `ASSERT_MODULE_LOCK` and 
> `AssertModuleLock` and should use `NDEBUG` too. I'll address that in a 
> separate patch.

WDYT about just replacing `LLDB_CONFIGURATION_DEBUG` with LLVM's 
`EXPENSIVE_CHECKS` in this case?

>> In D150639#4344695 , @JDevlieghere 
>> wrote:
>>
>>> This patch also uses `__FILE_NAME__` (Clang-specific extension that 
>>> functions similar to __FILE__ but only renders the last path component (the 
>>> filename) instead of an invocation dependent full path to that file.) when 
>>> building with clang.
>>
>> Can you put this comment in `LLDBAssert.h` itself?
>
> Good idea.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150639

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

[Lldb-commits] [PATCH] D150043: [InferAddressSpaces] Handle vector of pointers type & Support intrinsic masked gather/scatter

[Matt Arsenault via Phabricator via lldb-commits]

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

LGTM


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

https://reviews.llvm.org/D150043

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

[Lldb-commits] [PATCH] D150043: [InferAddressSpaces] Handle vector of pointers type & Support intrinsic masked gather/scatter

[Matt Arsenault via Phabricator via lldb-commits]

arsenm added inline comments.



Comment at: llvm/test/Transforms/InferAddressSpaces/masked-gather-scatter.ll:3
+
+; CHECK-LABEL: @masked_gather_inferas(
+; CHECK: tail call <4 x i32> @llvm.masked.gather.v4i32.v4p1

CaprYang wrote:
> arsenm wrote:
> > Generate full checks 
> updated
These aren't generated checks? I meant use update_test_checks 


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

https://reviews.llvm.org/D150043

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

[Lldb-commits] [PATCH] D150624: [lldb] Fix lua build after 27b6a4e63afe

[Krzysztof Parzyszek via Phabricator via lldb-commits]

kparzysz added a comment.

It works now.  Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150624

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

[Lldb-commits] [lldb] d95aec2 - [lldb][NFCI] Small adjustment to Breakpoint::AddName

[Alex Langford via lldb-commits]

Author: Alex Langford
Date: 2023-05-16T10:39:44-07:00
New Revision: d95aec2de2be114b4c331783fcceae233bf49aff

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

LOG: [lldb][NFCI] Small adjustment to Breakpoint::AddName

m_name_list is a std::unordered_set, we can insert the
string directly instead of grabbing the c_str and creating yet another
one.

Added: 


Modified: 
lldb/source/Breakpoint/Breakpoint.cpp

Removed: 




diff  --git a/lldb/source/Breakpoint/Breakpoint.cpp 
b/lldb/source/Breakpoint/Breakpoint.cpp
index 54ac21ac9e485..9f00c967be439 100644
--- a/lldb/source/Breakpoint/Breakpoint.cpp
+++ b/lldb/source/Breakpoint/Breakpoint.cpp
@@ -842,7 +842,7 @@ bool Breakpoint::HasResolvedLocations() const {
 size_t Breakpoint::GetNumLocations() const { return m_locations.GetSize(); }
 
 bool Breakpoint::AddName(llvm::StringRef new_name) {
-  m_name_list.insert(new_name.str().c_str());
+  m_name_list.insert(new_name.str());
   return true;
 }
 



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

[Lldb-commits] [lldb] f8499d5 - Emit the correct flags for the PROC CodeView Debug Symbol

[Eli Friedman via lldb-commits]

Author: Daniel Paoliello
Date: 2023-05-16T10:58:10-07:00
New Revision: f8499d5709e37b4e9a6d2a39c385cfd2c00bad6e

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

LOG: Emit the correct flags for the PROC CodeView Debug Symbol

The S_LPROC32_ID and S_GPROC32_ID CodeView Debug Symbols have a flags
field which LLVM has had the values for (in the ProcSymFlags enum) but
has never actually set.

These flags are used by Microsoft-internal tooling that leverages debug
information to do binary analysis.

Modified LLVM to set the correct flags:

- ProcSymFlags::HasOptimizedDebugInfo - always set, as this indicates that
debug info is present for optimized builds (if debug info is not emitted
for optimized builds, then LLVM won't emit a debug symbol at all).
- ProcSymFlags::IsNoReturn and ProcSymFlags::IsNoInline - set if the
function has the NoReturn or NoInline attributes respectively.
- ProcSymFlags::HasFP - set if the function requires a frame pointer (per
TargetFrameLowering::hasFP).

Per discussion in review, XFAIL'ing lldb test until someone working on
lldb has a chance to look at it.

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

Added: 


Modified: 
lldb/test/Shell/SymbolFile/PDB/function-nested-block.test
llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.h
llvm/test/DebugInfo/COFF/fpo-realign-alloca.ll
llvm/test/DebugInfo/COFF/fpo-realign-vframe.ll
llvm/test/DebugInfo/COFF/frameproc-flags.ll
llvm/test/DebugInfo/COFF/function-options.ll
llvm/test/DebugInfo/COFF/inlining-header.ll
llvm/test/DebugInfo/COFF/inlining.ll
llvm/test/DebugInfo/COFF/long-name.ll
llvm/test/DebugInfo/COFF/multifunction.ll
llvm/test/DebugInfo/COFF/simple.ll
llvm/test/DebugInfo/COFF/types-array.ll
llvm/test/DebugInfo/COFF/types-basic.ll
llvm/test/MC/AArch64/coff-debug.ll

Removed: 




diff  --git a/lldb/test/Shell/SymbolFile/PDB/function-nested-block.test 
b/lldb/test/Shell/SymbolFile/PDB/function-nested-block.test
index 9057d01c25840..1cb20a4036382 100644
--- a/lldb/test/Shell/SymbolFile/PDB/function-nested-block.test
+++ b/lldb/test/Shell/SymbolFile/PDB/function-nested-block.test
@@ -2,6 +2,7 @@ REQUIRES: system-windows, lld
 RUN: %build --compiler=clang-cl --nodefaultlib --output=%t.exe 
%S/Inputs/FunctionNestedBlockTest.cpp
 RUN: lldb-test symbols -find=function -file FunctionNestedBlockTest.cpp -line 
4 %t.exe | FileCheck --check-prefix=CHECK-FUNCTION %s
 RUN: lldb-test symbols -find=block -file FunctionNestedBlockTest.cpp -line 4 
%t.exe | FileCheck --check-prefix=CHECK-BLOCK %s
+XFAIL: *
 
 CHECK-FUNCTION: Found 1 functions:
 CHECK-FUNCTION: name = "{{.*}}", mangled = "{{_?}}main"

diff  --git a/llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp 
b/llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
index ce5fe6139f918..8161de57b58e0 100644
--- a/llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
+++ b/llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
@@ -1160,7 +1160,14 @@ void CodeViewDebug::emitDebugInfoForFunction(const 
Function *GV,
 OS.AddComment("Function section index");
 OS.emitCOFFSectionIndex(Fn);
 OS.AddComment("Flags");
-OS.emitInt8(0);
+ProcSymFlags ProcFlags = ProcSymFlags::HasOptimizedDebugInfo;
+if (FI.HasFramePointer)
+  ProcFlags |= ProcSymFlags::HasFP;
+if (GV->hasFnAttribute(Attribute::NoReturn))
+  ProcFlags |= ProcSymFlags::IsNoReturn;
+if (GV->hasFnAttribute(Attribute::NoInline))
+  ProcFlags |= ProcSymFlags::IsNoInline;
+OS.emitInt8(static_cast(ProcFlags));
 // Emit the function display name as a null-terminated string.
 OS.AddComment("Function name");
 // Truncate the name so we won't overflow the record length field.
@@ -1480,6 +1487,7 @@ void CodeViewDebug::beginFunctionImpl(const 
MachineFunction *MF) {
   CurFn->EncodedLocalFramePtrReg = EncodedFramePtrReg::StackPtr;
   CurFn->EncodedParamFramePtrReg = EncodedFramePtrReg::StackPtr;
 } else {
+  CurFn->HasFramePointer = true;
   // If there is an FP, parameters are always relative to it.
   CurFn->EncodedParamFramePtrReg = EncodedFramePtrReg::FramePtr;
   if (CurFn->HasStackRealignment) {

diff  --git a/llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.h 
b/llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.h
index 495822a6e6532..29445b31e7e74 100644
--- a/llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.h
+++ b/llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.h
@@ -191,6 +191,8 @@ class LLVM_LIBRARY_VISIBILITY CodeViewDebug : public 
DebugHandlerBase {
 bool HasStackRealignment = false;
 
 bool HaveLineInfo = false;
+
+bool HasFramePointer = false;
   };
   FunctionInfo *CurFn = nullptr;
 

diff  --git a/llvm/test/DebugInfo/COFF/fpo-realign-alloca.ll 
b/llvm/test/DebugInf

[Lldb-commits] [PATCH] D148761: Emit the correct flags for the PROC CodeView Debug Symbol

[Eli Friedman via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGf8499d5709e3: Emit the correct flags for the PROC CodeView 
Debug Symbol (authored by dpaoliello, committed by efriedma).
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

Changed prior to commit:
  https://reviews.llvm.org/D148761?vs=522303&id=522711#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148761

Files:
  lldb/test/Shell/SymbolFile/PDB/function-nested-block.test
  llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
  llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.h
  llvm/test/DebugInfo/COFF/fpo-realign-alloca.ll
  llvm/test/DebugInfo/COFF/fpo-realign-vframe.ll
  llvm/test/DebugInfo/COFF/frameproc-flags.ll
  llvm/test/DebugInfo/COFF/function-options.ll
  llvm/test/DebugInfo/COFF/inlining-header.ll
  llvm/test/DebugInfo/COFF/inlining.ll
  llvm/test/DebugInfo/COFF/long-name.ll
  llvm/test/DebugInfo/COFF/multifunction.ll
  llvm/test/DebugInfo/COFF/simple.ll
  llvm/test/DebugInfo/COFF/types-array.ll
  llvm/test/DebugInfo/COFF/types-basic.ll
  llvm/test/MC/AArch64/coff-debug.ll

Index: llvm/test/MC/AArch64/coff-debug.ll
===
--- llvm/test/MC/AArch64/coff-debug.ll
+++ llvm/test/MC/AArch64/coff-debug.ll
@@ -95,7 +95,9 @@
 ; CHECK:   FunctionType: main (0x1002)
 ; CHECK:   CodeOffset: main+0x0
 ; CHECK:   Segment: 0x0
-; CHECK:   Flags [ (0x0)
+; CHECK:   Flags [ (0xC0)
+; CHECK: HasOptimizedDebugInfo (0x80)
+; CHECK: IsNoInline (0x40)
 ; CHECK:   ]
 ; CHECK:   DisplayName: main
 ; CHECK:   LinkageName: main
Index: llvm/test/DebugInfo/COFF/types-basic.ll
===
--- llvm/test/DebugInfo/COFF/types-basic.ll
+++ llvm/test/DebugInfo/COFF/types-basic.ll
@@ -221,7 +221,8 @@
 ; CHECK:   FunctionType: f (0x1002)
 ; CHECK:   CodeOffset: ?f@@YAXMN_J@Z+0x0
 ; CHECK:   Segment: 0x0
-; CHECK:   Flags [ (0x0)
+; CHECK:   Flags [ (0x80)
+; CHECK: HasOptimizedDebugInfo (0x80)
 ; CHECK:   ]
 ; CHECK:   DisplayName: f
 ; CHECK:   LinkageName: ?f@@YAXMN_J@Z
Index: llvm/test/DebugInfo/COFF/types-array.ll
===
--- llvm/test/DebugInfo/COFF/types-array.ll
+++ llvm/test/DebugInfo/COFF/types-array.ll
@@ -58,7 +58,9 @@
 ; CHECK:   FunctionType: f (0x1002)
 ; CHECK:   CodeOffset: ?f@@YAXXZ+0x0
 ; CHECK:   Segment: 0x0
-; CHECK:   Flags [ (0x0)
+; CHECK:   Flags [ (0x81)
+; CHECK: HasFP (0x1)
+; CHECK: HasOptimizedDebugInfo (0x80)
 ; CHECK:   ]
 ; CHECK:   DisplayName: f
 ; CHECK:   LinkageName: ?f@@YAXXZ
Index: llvm/test/DebugInfo/COFF/simple.ll
===
--- llvm/test/DebugInfo/COFF/simple.ll
+++ llvm/test/DebugInfo/COFF/simple.ll
@@ -58,7 +58,7 @@
 ; X86-NEXT: .long   4098
 ; X86-NEXT: .secrel32 _f
 ; X86-NEXT: .secidx _f
-; X86-NEXT: .byte   0
+; X86-NEXT: .byte   128
 ; X86-NEXT: .asciz "f"
 ; X86-NEXT: .p2align 2
 ; X86-NEXT: [[PROC_SEGMENT_END]]:
@@ -188,7 +188,7 @@
 ; X64-NEXT: .long   4098
 ; X64-NEXT: .secrel32 f
 ; X64-NEXT: .secidx f
-; X64-NEXT: .byte   0
+; X64-NEXT: .byte   128
 ; X64-NEXT: .asciz "f"
 ; X64-NEXT: .p2align 2
 ; X64-NEXT: [[PROC_SEGMENT_END]]:
Index: llvm/test/DebugInfo/COFF/multifunction.ll
===
--- llvm/test/DebugInfo/COFF/multifunction.ll
+++ llvm/test/DebugInfo/COFF/multifunction.ll
@@ -78,7 +78,7 @@
 ; X86-NEXT: .long   4098
 ; X86-NEXT: .secrel32 _x
 ; X86-NEXT: .secidx _x
-; X86-NEXT: .byte   0
+; X86-NEXT: .byte   128
 ; X86-NEXT: .asciz "x"
 ; X86-NEXT: .p2align 2
 ; X86-NEXT: [[PROC_SEGMENT_END]]:
@@ -117,7 +117,7 @@
 ; X86-NEXT: .long   4099
 ; X86-NEXT: .secrel32 _y
 ; X86-NEXT: .secidx _y
-; X86-NEXT: .byte   0
+; X86-NEXT: .byte   128
 ; X86-NEXT: .asciz "y"
 ; X86-NEXT: .p2align 2
 ; X86-NEXT: [[PROC_SEGMENT_END]]:
@@ -156,7 +156,7 @@
 ; X86-NEXT: .long   4100
 ; X86-NEXT: .secrel32 _f
 ; X86-NEXT: .secidx _f
-; X86-NEXT: .byte   0
+; X86-NEXT: .byte   128
 ; X86-NEXT: .asciz "f"
 ; X86-NEXT: .p2align 2
 ; X86-NEXT: [[PROC_SEGMENT_END]]:
@@ -390,7 +390,7 @@
 ; X64-NEXT: .long   4098
 ; X64-NEXT: .secrel32 x
 ; X64-NEXT: .secidx x
-; X64-NEXT: .byte   0
+; X64-NEXT: .byte   128
 ; X64-NEXT: .asciz "x"
 ; X64-NEXT: .p2align 2
 ; X64-NEXT: [[PROC_SEGMENT_END]]:
@@ -428,7 +428,7 @@
 ; X64-NEXT: .long   4099
 ; X64-NEXT: .secrel32 y
 ; X64-NEXT: .secidx y
-; X64-NEXT: .byte   0
+; X64-NEXT: .byte   128
 ; X64-NEXT: .asciz "y"
 ; X64-NEXT: .p2align 2
 ; X64-NEXT: [[PROC_SEGMENT_END]]:
@@ -466,7 +466,7 @@
 ; X64-NEXT: .long   4100
 ; X64-NEXT: .secrel32 f
 ; X64-NEXT: .secidx f
-; X64-NEXT: .byte   0
+; X64-NEXT: .byte   128
 ; X64-NEXT: .asciz "f"
 ; X64-NEXT: .p2align

[Lldb-commits] [PATCH] D150709: [lldb][NFCI] Change return type of Language::GetInstanceVariableName

[Alex Langford via Phabricator via lldb-commits]

bulbazord created this revision.
bulbazord added a reviewer: kastiglione.
Herald added a project: All.
bulbazord requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

I don't think this needs to be a ConstString.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D150709

Files:
  lldb/include/lldb/Symbol/SymbolContext.h
  lldb/include/lldb/Target/Language.h
  lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.h
  lldb/source/Plugins/Language/ObjC/ObjCLanguage.h
  lldb/source/Plugins/Language/ObjCPlusPlus/ObjCPlusPlusLanguage.h
  lldb/source/Symbol/SymbolContext.cpp
  lldb/source/Target/StackFrame.cpp


Index: lldb/source/Target/StackFrame.cpp
===
--- lldb/source/Target/StackFrame.cpp
+++ lldb/source/Target/StackFrame.cpp
@@ -567,8 +567,9 @@
 // Check for direct ivars access which helps us with implicit access to
 // ivars using "this" or "self".
 GetSymbolContext(eSymbolContextFunction | eSymbolContextBlock);
-if (auto instance_var_name = m_sc.GetInstanceVariableName()) {
-  var_sp = variable_list->FindVariable(instance_var_name);
+llvm::StringRef instance_var_name = m_sc.GetInstanceVariableName();
+if (!instance_var_name.empty()) {
+  var_sp = variable_list->FindVariable(ConstString(instance_var_name));
   if (var_sp) {
 separator_idx = 0;
 if (Type *var_type = var_sp->GetType())
Index: lldb/source/Symbol/SymbolContext.cpp
===
--- lldb/source/Symbol/SymbolContext.cpp
+++ lldb/source/Symbol/SymbolContext.cpp
@@ -541,7 +541,7 @@
   return nullptr;
 }
 
-ConstString SymbolContext::GetInstanceVariableName() {
+llvm::StringRef SymbolContext::GetInstanceVariableName() {
   LanguageType lang_type = eLanguageTypeUnknown;
 
   if (Block *function_block = GetFunctionBlock())
Index: lldb/source/Plugins/Language/ObjCPlusPlus/ObjCPlusPlusLanguage.h
===
--- lldb/source/Plugins/Language/ObjCPlusPlus/ObjCPlusPlusLanguage.h
+++ lldb/source/Plugins/Language/ObjCPlusPlus/ObjCPlusPlusLanguage.h
@@ -40,7 +40,7 @@
 
   static lldb_private::Language *CreateInstance(lldb::LanguageType language);
 
-  ConstString GetInstanceVariableName() override { return ConstString("self"); 
}
+  llvm::StringRef GetInstanceVariableName() override { return "self"; }
 
   static llvm::StringRef GetPluginNameStatic() { return "objcplusplus"; }
 
Index: lldb/source/Plugins/Language/ObjC/ObjCLanguage.h
===
--- lldb/source/Plugins/Language/ObjC/ObjCLanguage.h
+++ lldb/source/Plugins/Language/ObjC/ObjCLanguage.h
@@ -155,7 +155,7 @@
   return false;
   }
 
-  ConstString GetInstanceVariableName() override { return ConstString("self"); 
}
+  llvm::StringRef GetInstanceVariableName() override { return "self"; }
 
   // PluginInterface protocol
   llvm::StringRef GetPluginName() override { return GetPluginNameStatic(); }
Index: lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.h
===
--- lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.h
+++ lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.h
@@ -165,7 +165,7 @@
   ConstString FindBestAlternateFunctionMangledName(
   const Mangled mangled, const SymbolContext &sym_ctx) const override;
 
-  ConstString GetInstanceVariableName() override { return ConstString("this"); 
}
+  llvm::StringRef GetInstanceVariableName() override { return "this"; }
 
   // PluginInterface protocol
   llvm::StringRef GetPluginName() override { return GetPluginNameStatic(); }
Index: lldb/include/lldb/Target/Language.h
===
--- lldb/include/lldb/Target/Language.h
+++ lldb/include/lldb/Target/Language.h
@@ -326,7 +326,7 @@
 return ConstString();
   }
 
-  virtual ConstString GetInstanceVariableName() { return {}; }
+  virtual llvm::StringRef GetInstanceVariableName() { return {}; }
 
 protected:
   // Classes that inherit from Language can see and modify these
Index: lldb/include/lldb/Symbol/SymbolContext.h
===
--- lldb/include/lldb/Symbol/SymbolContext.h
+++ lldb/include/lldb/Symbol/SymbolContext.h
@@ -250,8 +250,8 @@
   /// For C++ the name is "this", for Objective-C the name is "self".
   ///
   /// \return
-  /// Returns a string for the name of the instance variable.
-  ConstString GetInstanceVariableName();
+  /// Returns a StringRef for the name of the instance variable.
+  llvm::StringRef GetInstanceVariableName();
 
   /// Sorts the types in TypeMap according to SymbolContext to TypeList
   ///


Index: lldb/source/Target/StackFrame.cpp
===
--- lldb/source/Ta

[Lldb-commits] [PATCH] D150630: [lldb][docs] Update SB API design document

[Jonas Devlieghere via Phabricator via lldb-commits]

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

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150630

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

[Lldb-commits] [PATCH] D150630: [lldb][docs] Update SB API design document

[Alex Langford via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGec388adbbcbf: [lldb][docs] Update SB API design document 
(authored by bulbazord).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150630

Files:
  lldb/docs/design/sbapi.rst


Index: lldb/docs/design/sbapi.rst
===
--- lldb/docs/design/sbapi.rst
+++ lldb/docs/design/sbapi.rst
@@ -7,6 +7,9 @@
 incompatibilities that C++ is so susceptible to. We've established a few rules
 to ensure that this happens.
 
+Extending the SB API
+
+
 The classes in the SB API's are all called SB, where SomeName is in
 CamelCase starting with an upper case letter. The method names are all
 CamelCase with initial capital letter as well.
@@ -45,14 +48,29 @@
 
 Another piece of the SB API infrastructure is the Python (or other script
 interpreter) customization. SWIG allows you to add property access, iterators
-and documentation to classes, but to do that you have to use a Swig interface
-file in place of the .h file. Those files have a different format than a
-straight C++ header file. These files are called SB.i, and live in
-"scripts/interface". They are constructed by starting with the associated .h
-file, and adding documentation and the Python decorations, etc. We do this in a
-decidedly low-tech way, by maintaining the two files in parallel. That
-simplifies the build process, but it does mean that if you add a method to the
-C++ API's for an SB class, you have to copy the interface to the .i file.
+and documentation to classes. We place the property accessors and iterators in
+a file dedicated to extensions to existing SB classes at
+"bindings/interface/SBExtensions.i". The documentation is similarly
+located at "bindings/interface/SBDocstrings.i". These two files, in
+addition to the actual header SB.h, forms the interface that lldb
+exposes to users through the scripting languages.
+
+There are some situations where you may want to add functionality to the SB API
+only for use in C++. To prevent SWIG from generating bindings to these
+functions, you can use a C macro guard, like so:
+
+::
+
+  #ifndef SWIG
+  int GetResourceCPPOnly() const;
+  #endif
+
+In this case, ``GetResourceCPPOnly`` will not be generated for Python or other
+scripting languages. If you wanted to add a resource specifically only for the
+SWIG case, you can invert the condition and use ``#ifdef SWIG`` instead. When
+building the LLDB framework for macOS, the headers are processed with
+``unifdef`` prior to being copied into the framework bundle to remove macros
+involving SWIG.
 
 API Instrumentation
 ---


Index: lldb/docs/design/sbapi.rst
===
--- lldb/docs/design/sbapi.rst
+++ lldb/docs/design/sbapi.rst
@@ -7,6 +7,9 @@
 incompatibilities that C++ is so susceptible to. We've established a few rules
 to ensure that this happens.
 
+Extending the SB API
+
+
 The classes in the SB API's are all called SB, where SomeName is in
 CamelCase starting with an upper case letter. The method names are all
 CamelCase with initial capital letter as well.
@@ -45,14 +48,29 @@
 
 Another piece of the SB API infrastructure is the Python (or other script
 interpreter) customization. SWIG allows you to add property access, iterators
-and documentation to classes, but to do that you have to use a Swig interface
-file in place of the .h file. Those files have a different format than a
-straight C++ header file. These files are called SB.i, and live in
-"scripts/interface". They are constructed by starting with the associated .h
-file, and adding documentation and the Python decorations, etc. We do this in a
-decidedly low-tech way, by maintaining the two files in parallel. That
-simplifies the build process, but it does mean that if you add a method to the
-C++ API's for an SB class, you have to copy the interface to the .i file.
+and documentation to classes. We place the property accessors and iterators in
+a file dedicated to extensions to existing SB classes at
+"bindings/interface/SBExtensions.i". The documentation is similarly
+located at "bindings/interface/SBDocstrings.i". These two files, in
+addition to the actual header SB.h, forms the interface that lldb
+exposes to users through the scripting languages.
+
+There are some situations where you may want to add functionality to the SB API
+only for use in C++. To prevent SWIG from generating bindings to these
+functions, you can use a C macro guard, like so:
+
+::
+
+  #ifndef SWIG
+  int GetResourceCPPOnly() const;
+  #endif
+
+In this case, ``GetResourceCPPOnly`` will not be generated for Python or other
+scripting languages. If you wanted to add a resource specifically only for the
+SWIG case, you can invert the condition and use ``#ifdef SWIG`` instead.

[Lldb-commits] [lldb] ec388ad - [lldb][docs] Update SB API design document

[Alex Langford via lldb-commits]

Author: Alex Langford
Date: 2023-05-16T12:58:54-07:00
New Revision: ec388adbbcbf2cf7f2743464ef5b3c5c540a2d23

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

LOG: [lldb][docs] Update SB API design document

The documentation should have been updated in 662548c82683.
This updates it to be more accurate with the current design.

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

Added: 


Modified: 
lldb/docs/design/sbapi.rst

Removed: 




diff  --git a/lldb/docs/design/sbapi.rst b/lldb/docs/design/sbapi.rst
index f4a7ca271be63..f2de0e7ae60ca 100644
--- a/lldb/docs/design/sbapi.rst
+++ b/lldb/docs/design/sbapi.rst
@@ -7,6 +7,9 @@ lldb. As such it is important that they not suffer from the 
binary
 incompatibilities that C++ is so susceptible to. We've established a few rules
 to ensure that this happens.
 
+Extending the SB API
+
+
 The classes in the SB API's are all called SB, where SomeName is in
 CamelCase starting with an upper case letter. The method names are all
 CamelCase with initial capital letter as well.
@@ -45,14 +48,29 @@ classes to report whether the object is empty or not.
 
 Another piece of the SB API infrastructure is the Python (or other script
 interpreter) customization. SWIG allows you to add property access, iterators
-and documentation to classes, but to do that you have to use a Swig interface
-file in place of the .h file. Those files have a 
diff erent format than a
-straight C++ header file. These files are called SB.i, and live in
-"scripts/interface". They are constructed by starting with the associated .h
-file, and adding documentation and the Python decorations, etc. We do this in a
-decidedly low-tech way, by maintaining the two files in parallel. That
-simplifies the build process, but it does mean that if you add a method to the
-C++ API's for an SB class, you have to copy the interface to the .i file.
+and documentation to classes. We place the property accessors and iterators in
+a file dedicated to extensions to existing SB classes at
+"bindings/interface/SBExtensions.i". The documentation is similarly
+located at "bindings/interface/SBDocstrings.i". These two files, in
+addition to the actual header SB.h, forms the interface that lldb
+exposes to users through the scripting languages.
+
+There are some situations where you may want to add functionality to the SB API
+only for use in C++. To prevent SWIG from generating bindings to these
+functions, you can use a C macro guard, like so:
+
+::
+
+  #ifndef SWIG
+  int GetResourceCPPOnly() const;
+  #endif
+
+In this case, ``GetResourceCPPOnly`` will not be generated for Python or other
+scripting languages. If you wanted to add a resource specifically only for the
+SWIG case, you can invert the condition and use ``#ifdef SWIG`` instead. When
+building the LLDB framework for macOS, the headers are processed with
+``unifdef`` prior to being copied into the framework bundle to remove macros
+involving SWIG.
 
 API Instrumentation
 ---



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

[Lldb-commits] [PATCH] D144509: [CMake] Bumps minimum version to 3.20.0.

[Mike Hommey via Phabricator via lldb-commits]

glandium added a comment.
Herald added subscribers: bviyer, ekilmer, jplehr.

FYI, 65429b9af6a2c99d340ab2dcddd41dab201f399c 
 is 
causing problems on Windows compiler-rt for some reason I haven't identified 
yet (with cmake 3.25.1). Which suggests for a same version of cmake, this is 
actually altering its behavior, which I wouldn't have expected...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144509

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

[Lldb-commits] [PATCH] D144509: [CMake] Bumps minimum version to 3.20.0.

[Martin Storsjö via Phabricator via lldb-commits]

mstorsjo added a comment.

In D144509#4347562 , @glandium wrote:

> FYI, 65429b9af6a2c99d340ab2dcddd41dab201f399c 
>  is 
> causing problems on Windows compiler-rt for some reason I haven't identified 
> yet (with cmake 3.25.1). Which suggests for a same version of cmake, this is 
> actually altering its behavior, which I wouldn't have expected...

See D150688  - I believe that might fix the 
issue you're seeing, as that one mentions compiler-rt.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144509

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

[Lldb-commits] [PATCH] D150716: [lldb][NFCI] Switch to using llvm::DWARFAbbreviationDeclaration

[Alex Langford via Phabricator via lldb-commits]

bulbazord created this revision.
bulbazord added reviewers: aprantl, JDevlieghere, fdeazeve, rastogishubham.
Herald added a project: All.
bulbazord requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

Both LLVM and LLDB implement DWARFAbbreviationDeclaration. As of
631ff46cbf51 
, llvm's 
implementation of
DWARFAbbreviationDeclaration::extract behaves the same as LLDB's
implementation, making it easier to merge the implementations.

The only major difference between LLDB's implementation and LLVM's
implementation is that LLVM's DWARFAbbreviationDeclaration is slightly
larger. Specifically, it has some metadata that keeps track of the size
of a declaration (if it has a fixed size) so that it can potentially
optimize extraction in some scenarios. I think this increase in size
should be acceptable and possibly useful on the LLDB side.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D150716

Files:
  lldb/source/Plugins/SymbolFile/DWARF/CMakeLists.txt
  lldb/source/Plugins/SymbolFile/DWARF/DWARFAbbreviationDeclaration.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFAbbreviationDeclaration.h
  lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugAbbrev.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugAbbrev.h
  lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.h
  lldb/source/Plugins/SymbolFile/DWARF/DWARFDefines.h
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
  lldb/unittests/SymbolFile/DWARF/SymbolFileDWARFTests.cpp

Index: lldb/unittests/SymbolFile/DWARF/SymbolFileDWARFTests.cpp
===
--- lldb/unittests/SymbolFile/DWARF/SymbolFileDWARFTests.cpp
+++ lldb/unittests/SymbolFile/DWARF/SymbolFileDWARFTests.cpp
@@ -15,7 +15,6 @@
 #include "llvm/Support/Path.h"
 
 #include "Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.h"
-#include "Plugins/SymbolFile/DWARF/DWARFAbbreviationDeclaration.h"
 #include "Plugins/SymbolFile/DWARF/DWARFDataExtractor.h"
 #include "Plugins/SymbolFile/DWARF/DWARFDebugAbbrev.h"
 #include "Plugins/SymbolFile/DWARF/DWARFDebugArangeSet.h"
@@ -105,13 +104,13 @@
   EXPECT_EQ(abbrev_set.GetIndexOffset(), 1u);
 
   auto abbrev1 = abbrev_set.GetAbbreviationDeclaration(1);
-  EXPECT_EQ(abbrev1->Tag(), DW_TAG_compile_unit);
-  EXPECT_TRUE(abbrev1->HasChildren());
-  EXPECT_EQ(abbrev1->NumAttributes(), 1u);
+  EXPECT_EQ(abbrev1->getTag(), DW_TAG_compile_unit);
+  EXPECT_TRUE(abbrev1->hasChildren());
+  EXPECT_EQ(abbrev1->getNumAttributes(), 1u);
   auto abbrev2 = abbrev_set.GetAbbreviationDeclaration(2);
-  EXPECT_EQ(abbrev2->Tag(), DW_TAG_subprogram);
-  EXPECT_FALSE(abbrev2->HasChildren());
-  EXPECT_EQ(abbrev2->NumAttributes(), 1u);
+  EXPECT_EQ(abbrev2->getTag(), DW_TAG_subprogram);
+  EXPECT_FALSE(abbrev2->hasChildren());
+  EXPECT_EQ(abbrev2->getNumAttributes(), 1u);
 }
 
 TEST_F(SymbolFileDWARFTests, TestAbbrevOrder1Start5) {
@@ -150,13 +149,13 @@
   EXPECT_EQ(abbrev_set.GetIndexOffset(), 5u);
 
   auto abbrev1 = abbrev_set.GetAbbreviationDeclaration(5);
-  EXPECT_EQ(abbrev1->Tag(), DW_TAG_compile_unit);
-  EXPECT_TRUE(abbrev1->HasChildren());
-  EXPECT_EQ(abbrev1->NumAttributes(), 1u);
+  EXPECT_EQ(abbrev1->getTag(), DW_TAG_compile_unit);
+  EXPECT_TRUE(abbrev1->hasChildren());
+  EXPECT_EQ(abbrev1->getNumAttributes(), 1u);
   auto abbrev2 = abbrev_set.GetAbbreviationDeclaration(6);
-  EXPECT_EQ(abbrev2->Tag(), DW_TAG_subprogram);
-  EXPECT_FALSE(abbrev2->HasChildren());
-  EXPECT_EQ(abbrev2->NumAttributes(), 1u);
+  EXPECT_EQ(abbrev2->getTag(), DW_TAG_subprogram);
+  EXPECT_FALSE(abbrev2->hasChildren());
+  EXPECT_EQ(abbrev2->getNumAttributes(), 1u);
 }
 
 TEST_F(SymbolFileDWARFTests, TestAbbrevOutOfOrder) {
@@ -195,13 +194,13 @@
   EXPECT_EQ(abbrev_set.GetIndexOffset(), UINT32_MAX);
 
   auto abbrev1 = abbrev_set.GetAbbreviationDeclaration(2);
-  EXPECT_EQ(abbrev1->Tag(), DW_TAG_compile_unit);
-  EXPECT_TRUE(abbrev1->HasChildren());
-  EXPECT_EQ(abbrev1->NumAttributes(), 1u);
+  EXPECT_EQ(abbrev1->getTag(), DW_TAG_compile_unit);
+  EXPECT_TRUE(abbrev1->hasChildren());
+  EXPECT_EQ(abbrev1->getNumAttributes(), 1u);
   auto abbrev2 = abbrev_set.GetAbbreviationDeclaration(1);
-  EXPECT_EQ(abbrev2->Tag(), DW_TAG_subprogram);
-  EXPECT_FALSE(abbrev2->HasChildren());
-  EXPECT_EQ(abbrev2->NumAttributes(), 1u);
+  EXPECT_EQ(abbrev2->getTag(), DW_TAG_subprogram);
+  EXPECT_FALSE(abbrev2->hasChildren());
+  EXPECT_EQ(abbrev2->getNumAttributes(), 1u);
 }
 
 TEST_F(SymbolFileDWARFTests, TestAbbrevInvalidNULLTag) {
@@ -226,9 +225,8 @@
   llvm::Error error = abbrev_set.extract(data, &data_offset);
   // Verify we get an error
   EXPECT_TRUE(bool(error));
-  EXPECT_EQ("abbrev decl requires non-null tag.",
+  EXPECT_EQ("abbreviation declaration requires a non-null tag",
 llvm::toString(std::move(error)));
-
 }
 
 TEST_F(Sym

[Lldb-commits] [PATCH] D144509: [CMake] Bumps minimum version to 3.20.0.

[Mike Hommey via Phabricator via lldb-commits]

glandium added a comment.

In D144509#4347604 , @mstorsjo wrote:

> See D150688  - I believe that might fix the 
> issue you're seeing, as that one mentions compiler-rt.

Unfortunately, it doesn't.

FWIW, the errors looks like:

  lld-link: error: undefined symbol: __declspec(dllimport) _getpid
  >>> referenced by 
clang_rt.profile-i386.lib(InstrProfilingFile.c.obj):(_getCurFilename)
  >>> referenced by 
clang_rt.profile-i386.lib(InstrProfilingFile.c.obj):(_parseAndSetFilename)
  >>> referenced by oldnames.lib(getpid.obi)

etc.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144509

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

[Lldb-commits] [PATCH] D144509: [CMake] Bumps minimum version to 3.20.0.

[Mike Hommey via Phabricator via lldb-commits]

glandium added a comment.

It's related, though, because now that I look at my build logs, the difference 
between when it works and when it doesn't is `/MT` vs `-MD` when compiler-rt is 
compiled. The main peculiarity on our end, though, is that the Windows 
compiler-rt is cross-compiled.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144509

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

[Lldb-commits] [PATCH] D150621: lldb PlatformDarwinKernel, delay local filesystem scan until needed

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda updated this revision to Diff 522813.
jasonmolenda added a comment.

Update patch to use a std::once operation instead of hand-rolling it, as 
suggested by Jonas and Alex.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150621

Files:
  lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp
  lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.h


Index: lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.h
===
--- lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.h
+++ lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.h
@@ -158,6 +158,8 @@
   bool LoadPlatformBinaryAndSetup(Process *process, lldb::addr_t addr,
   bool notify) override;
 
+  void UpdateKextandKernelsLocalScan();
+
   // Most of the ivars are assembled under FileSystem::EnumerateDirectory calls
   // where the function being called for each file/directory must be static.
   // We'll pass a this pointer as a baton and access the ivars directly.
@@ -194,6 +196,8 @@
 
   LazyBool m_ios_debug_session;
 
+  std::once_flag m_kext_scan_flag;
+
   PlatformDarwinKernel(const PlatformDarwinKernel &) = delete;
   const PlatformDarwinKernel &operator=(const PlatformDarwinKernel &) = delete;
 };
Index: lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp
===
--- lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp
+++ lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp
@@ -229,12 +229,8 @@
   m_name_to_kext_path_map_without_dsyms(), m_search_directories(),
   m_search_directories_no_recursing(), m_kernel_binaries_with_dsyms(),
   m_kernel_binaries_without_dsyms(), m_kernel_dsyms_no_binaries(),
-  m_kernel_dsyms_yaas(), m_ios_debug_session(is_ios_debug_session)
-
-{
-  CollectKextAndKernelDirectories();
-  SearchForKextsAndKernelsRecursively();
-}
+  m_kernel_dsyms_yaas(), m_ios_debug_session(is_ios_debug_session),
+  m_kext_scan_flag() {}
 
 /// Destructor.
 ///
@@ -243,6 +239,7 @@
 PlatformDarwinKernel::~PlatformDarwinKernel() = default;
 
 void PlatformDarwinKernel::GetStatus(Stream &strm) {
+  UpdateKextandKernelsLocalScan();
   Platform::GetStatus(strm);
   strm.Printf(" Debug session type: ");
   if (m_ios_debug_session == eLazyBoolYes)
@@ -709,6 +706,13 @@
   return results;
 }
 
+void PlatformDarwinKernel::UpdateKextandKernelsLocalScan() {
+  std::call_once(m_kext_scan_flag, [this]() {
+CollectKextAndKernelDirectories();
+SearchForKextsAndKernelsRecursively();
+  });
+}
+
 Status PlatformDarwinKernel::GetSharedModule(
 const ModuleSpec &module_spec, Process *process, ModuleSP &module_sp,
 const FileSpecList *module_search_paths_ptr,
@@ -789,6 +793,7 @@
 llvm::SmallVectorImpl *old_modules, bool *did_create_ptr) {
   Status error;
   module_sp.reset();
+  UpdateKextandKernelsLocalScan();
 
   // First try all kernel binaries that have a dSYM next to them
   for (auto possible_kernel : m_kernel_binaries_with_dsyms) {


Index: lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.h
===
--- lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.h
+++ lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.h
@@ -158,6 +158,8 @@
   bool LoadPlatformBinaryAndSetup(Process *process, lldb::addr_t addr,
   bool notify) override;
 
+  void UpdateKextandKernelsLocalScan();
+
   // Most of the ivars are assembled under FileSystem::EnumerateDirectory calls
   // where the function being called for each file/directory must be static.
   // We'll pass a this pointer as a baton and access the ivars directly.
@@ -194,6 +196,8 @@
 
   LazyBool m_ios_debug_session;
 
+  std::once_flag m_kext_scan_flag;
+
   PlatformDarwinKernel(const PlatformDarwinKernel &) = delete;
   const PlatformDarwinKernel &operator=(const PlatformDarwinKernel &) = delete;
 };
Index: lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp
===
--- lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp
+++ lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp
@@ -229,12 +229,8 @@
   m_name_to_kext_path_map_without_dsyms(), m_search_directories(),
   m_search_directories_no_recursing(), m_kernel_binaries_with_dsyms(),
   m_kernel_binaries_without_dsyms(), m_kernel_dsyms_no_binaries(),
-  m_kernel_dsyms_yaas(), m_ios_debug_session(is_ios_debug_session)
-
-{
-  CollectKextAndKernelDirectories();
-  SearchForKextsAndKernelsRecursively();
-}
+  m_kernel_dsyms_yaas(), m_ios_debug_session(is_ios_debug_session),
+  m_kext_scan_flag() {}
 
 /// Destructor.
 ///
@@ -243,6 +239,7 @@
 PlatformDarwinKernel::~PlatformDarwinKernel() = default;
 
 void Pla

[Lldb-commits] [PATCH] D144509: [CMake] Bumps minimum version to 3.20.0.

[Mike Hommey via Phabricator via lldb-commits]

glandium added a comment.

So the problem is that `CMakePolicy.cmake` is not included in 
`compiler-rt/CMakeLists.txt`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144509

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

[Lldb-commits] [PATCH] D150619: [lldb] Delay removal of persistent results

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

This seems fine in general, with one quibble:

IIUC, the "indirect" removal of persistent results which you are trying to 
avoid happens here:

  lldb::ExpressionResults
  UserExpression::Execute(DiagnosticManager &diagnostic_manager,
  ExecutionContext &exe_ctx,
  const EvaluateExpressionOptions &options,
  lldb::UserExpressionSP &shared_ptr_to_me,
  lldb::ExpressionVariableSP &result_var) {
lldb::ExpressionResults expr_result = DoExecute(
diagnostic_manager, exe_ctx, options, shared_ptr_to_me, result_var);
Target *target = exe_ctx.GetTargetPtr();
if (options.GetSuppressPersistentResult() && result_var && target) {
  if (auto *persistent_state =
  target->GetPersistentExpressionStateForLanguage(m_language))
persistent_state->RemovePersistentVariable(result_var);
}
return expr_result;
  }

So to succeed in preserving the persistent result, your patch relies on 
SuppressPersistentResult being false in the EvaluateExpressionOptions you pass 
to the expression.  However, you derive the expression options from the command 
options, for instance in:

  const EvaluateExpressionOptions eval_options =
  m_command_options.GetEvaluateExpressionOptions(target, m_varobj_options);

so you don't actually know what the value of the SuppressPersistentResults is 
going to be.  Since you rely on this having a particular value regardless of 
the user's intentions, you should do SetSuppressPersistentResults explicitly 
(with an appropriate comment) after you've fetch the eval_options in the 
`dwim-print` and `expr` commands.

A much smaller quibble is that it seems a little weird to ask the expression 
options or the frame which language in the 
PersistentExpressionResultsForLanguage map the result variable was stuffed into 
when you have the Result ValueObject on hand.  That seems like something the 
ValueObject should tell you.  When we Persist ValueObjects we use 
ValueObject::GetPreferredDisplayLanguage.  That seems more trustworthy - if it 
isn't we should make it so...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150619

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

[Lldb-commits] [PATCH] D150621: lldb PlatformDarwinKernel, delay local filesystem scan until needed

[Alex Langford via Phabricator via lldb-commits]

bulbazord accepted this revision.
bulbazord added a comment.

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150621

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

[Lldb-commits] [lldb] e3227e7 - lldb PlatformDarwinKernel make local filesystem scan lazy

[Jason Molenda via lldb-commits]

Author: Jason Molenda
Date: 2023-05-16T18:15:59-07:00
New Revision: e3227e74e3bfab7c5aed07c20b515202275ce7c4

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

LOG: lldb PlatformDarwinKernel make local filesystem scan lazy

Instead of doing the local filesystem scan for kexts and kernels
when the PlatformDarwinKernel is constructed, delay doing it until
the scan is needed.

Differential Revision: https://reviews.llvm.org/D150621
rdar://109186357

Added: 


Modified: 
lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp
lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.h

Removed: 




diff  --git a/lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp 
b/lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp
index c29007fb9a340..d120ae05c82bc 100644
--- a/lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp
+++ b/lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp
@@ -229,12 +229,8 @@ PlatformDarwinKernel::PlatformDarwinKernel(
   m_name_to_kext_path_map_without_dsyms(), m_search_directories(),
   m_search_directories_no_recursing(), m_kernel_binaries_with_dsyms(),
   m_kernel_binaries_without_dsyms(), m_kernel_dsyms_no_binaries(),
-  m_kernel_dsyms_yaas(), m_ios_debug_session(is_ios_debug_session)
-
-{
-  CollectKextAndKernelDirectories();
-  SearchForKextsAndKernelsRecursively();
-}
+  m_kernel_dsyms_yaas(), m_ios_debug_session(is_ios_debug_session),
+  m_kext_scan_flag() {}
 
 /// Destructor.
 ///
@@ -243,6 +239,7 @@ PlatformDarwinKernel::PlatformDarwinKernel(
 PlatformDarwinKernel::~PlatformDarwinKernel() = default;
 
 void PlatformDarwinKernel::GetStatus(Stream &strm) {
+  UpdateKextandKernelsLocalScan();
   Platform::GetStatus(strm);
   strm.Printf(" Debug session type: ");
   if (m_ios_debug_session == eLazyBoolYes)
@@ -709,6 +706,13 @@ PlatformDarwinKernel::GetDWARFBinaryInDSYMBundle(const 
FileSpec &dsym_bundle) {
   return results;
 }
 
+void PlatformDarwinKernel::UpdateKextandKernelsLocalScan() {
+  std::call_once(m_kext_scan_flag, [this]() {
+CollectKextAndKernelDirectories();
+SearchForKextsAndKernelsRecursively();
+  });
+}
+
 Status PlatformDarwinKernel::GetSharedModule(
 const ModuleSpec &module_spec, Process *process, ModuleSP &module_sp,
 const FileSpecList *module_search_paths_ptr,
@@ -789,6 +793,7 @@ Status PlatformDarwinKernel::GetSharedModuleKernel(
 llvm::SmallVectorImpl *old_modules, bool *did_create_ptr) {
   Status error;
   module_sp.reset();
+  UpdateKextandKernelsLocalScan();
 
   // First try all kernel binaries that have a dSYM next to them
   for (auto possible_kernel : m_kernel_binaries_with_dsyms) {

diff  --git a/lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.h 
b/lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.h
index e28f77cd44d55..9db9c0065613d 100644
--- a/lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.h
+++ b/lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.h
@@ -158,6 +158,8 @@ class PlatformDarwinKernel : public PlatformDarwin {
   bool LoadPlatformBinaryAndSetup(Process *process, lldb::addr_t addr,
   bool notify) override;
 
+  void UpdateKextandKernelsLocalScan();
+
   // Most of the ivars are assembled under FileSystem::EnumerateDirectory calls
   // where the function being called for each file/directory must be static.
   // We'll pass a this pointer as a baton and access the ivars directly.
@@ -194,6 +196,8 @@ class PlatformDarwinKernel : public PlatformDarwin {
 
   LazyBool m_ios_debug_session;
 
+  std::once_flag m_kext_scan_flag;
+
   PlatformDarwinKernel(const PlatformDarwinKernel &) = delete;
   const PlatformDarwinKernel &operator=(const PlatformDarwinKernel &) = delete;
 };



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

[Lldb-commits] [PATCH] D150621: lldb PlatformDarwinKernel, delay local filesystem scan until needed

[Jason Molenda via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGe3227e74e3bf: lldb PlatformDarwinKernel make local 
filesystem scan lazy (authored by jasonmolenda).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150621

Files:
  lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp
  lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.h


Index: lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.h
===
--- lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.h
+++ lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.h
@@ -158,6 +158,8 @@
   bool LoadPlatformBinaryAndSetup(Process *process, lldb::addr_t addr,
   bool notify) override;
 
+  void UpdateKextandKernelsLocalScan();
+
   // Most of the ivars are assembled under FileSystem::EnumerateDirectory calls
   // where the function being called for each file/directory must be static.
   // We'll pass a this pointer as a baton and access the ivars directly.
@@ -194,6 +196,8 @@
 
   LazyBool m_ios_debug_session;
 
+  std::once_flag m_kext_scan_flag;
+
   PlatformDarwinKernel(const PlatformDarwinKernel &) = delete;
   const PlatformDarwinKernel &operator=(const PlatformDarwinKernel &) = delete;
 };
Index: lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp
===
--- lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp
+++ lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp
@@ -229,12 +229,8 @@
   m_name_to_kext_path_map_without_dsyms(), m_search_directories(),
   m_search_directories_no_recursing(), m_kernel_binaries_with_dsyms(),
   m_kernel_binaries_without_dsyms(), m_kernel_dsyms_no_binaries(),
-  m_kernel_dsyms_yaas(), m_ios_debug_session(is_ios_debug_session)
-
-{
-  CollectKextAndKernelDirectories();
-  SearchForKextsAndKernelsRecursively();
-}
+  m_kernel_dsyms_yaas(), m_ios_debug_session(is_ios_debug_session),
+  m_kext_scan_flag() {}
 
 /// Destructor.
 ///
@@ -243,6 +239,7 @@
 PlatformDarwinKernel::~PlatformDarwinKernel() = default;
 
 void PlatformDarwinKernel::GetStatus(Stream &strm) {
+  UpdateKextandKernelsLocalScan();
   Platform::GetStatus(strm);
   strm.Printf(" Debug session type: ");
   if (m_ios_debug_session == eLazyBoolYes)
@@ -709,6 +706,13 @@
   return results;
 }
 
+void PlatformDarwinKernel::UpdateKextandKernelsLocalScan() {
+  std::call_once(m_kext_scan_flag, [this]() {
+CollectKextAndKernelDirectories();
+SearchForKextsAndKernelsRecursively();
+  });
+}
+
 Status PlatformDarwinKernel::GetSharedModule(
 const ModuleSpec &module_spec, Process *process, ModuleSP &module_sp,
 const FileSpecList *module_search_paths_ptr,
@@ -789,6 +793,7 @@
 llvm::SmallVectorImpl *old_modules, bool *did_create_ptr) {
   Status error;
   module_sp.reset();
+  UpdateKextandKernelsLocalScan();
 
   // First try all kernel binaries that have a dSYM next to them
   for (auto possible_kernel : m_kernel_binaries_with_dsyms) {


Index: lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.h
===
--- lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.h
+++ lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.h
@@ -158,6 +158,8 @@
   bool LoadPlatformBinaryAndSetup(Process *process, lldb::addr_t addr,
   bool notify) override;
 
+  void UpdateKextandKernelsLocalScan();
+
   // Most of the ivars are assembled under FileSystem::EnumerateDirectory calls
   // where the function being called for each file/directory must be static.
   // We'll pass a this pointer as a baton and access the ivars directly.
@@ -194,6 +196,8 @@
 
   LazyBool m_ios_debug_session;
 
+  std::once_flag m_kext_scan_flag;
+
   PlatformDarwinKernel(const PlatformDarwinKernel &) = delete;
   const PlatformDarwinKernel &operator=(const PlatformDarwinKernel &) = delete;
 };
Index: lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp
===
--- lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp
+++ lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp
@@ -229,12 +229,8 @@
   m_name_to_kext_path_map_without_dsyms(), m_search_directories(),
   m_search_directories_no_recursing(), m_kernel_binaries_with_dsyms(),
   m_kernel_binaries_without_dsyms(), m_kernel_dsyms_no_binaries(),
-  m_kernel_dsyms_yaas(), m_ios_debug_session(is_ios_debug_session)
-
-{
-  CollectKextAndKernelDirectories();
-  SearchForKextsAndKernelsRecursively();
-}
+  m_kernel_dsyms_yaas(), m_ios_debug_session(is_ios_debug_session),
+  m_kext_scan_flag() {}
 
 /// Destructor.
 ///
@@ -243,6 +239,7 @@
 PlatformDarwinKernel::~PlatformDarwinKernel() = default;
 
 

[Lldb-commits] [PATCH] D150619: [lldb] Delay removal of persistent results

[Dave Lee via Phabricator via lldb-commits]

kastiglione updated this revision to Diff 522875.
kastiglione added a comment.

- Explicitly control expression evaluation options for suppressing persistent 
result
- Use the ValueObject's display language instead of the CU or frame's language


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150619

Files:
  lldb/source/Commands/CommandObjectDWIMPrint.cpp
  lldb/source/Commands/CommandObjectExpression.cpp
  lldb/source/Commands/CommandObjectExpression.h

Index: lldb/source/Commands/CommandObjectExpression.h
===
--- lldb/source/Commands/CommandObjectExpression.h
+++ lldb/source/Commands/CommandObjectExpression.h
@@ -41,6 +41,9 @@
 const Target &target,
 const OptionGroupValueObjectDisplay &display_opts);
 
+bool ShouldSuppressResult(
+const OptionGroupValueObjectDisplay &display_opts) const;
+
 bool top_level;
 bool unwind_on_error;
 bool ignore_breakpoints;
Index: lldb/source/Commands/CommandObjectExpression.cpp
===
--- lldb/source/Commands/CommandObjectExpression.cpp
+++ lldb/source/Commands/CommandObjectExpression.cpp
@@ -10,6 +10,7 @@
 
 #include "CommandObjectExpression.h"
 #include "lldb/Core/Debugger.h"
+#include "lldb/Expression/ExpressionVariable.h"
 #include "lldb/Expression/REPL.h"
 #include "lldb/Expression/UserExpression.h"
 #include "lldb/Host/OptionParser.h"
@@ -21,6 +22,7 @@
 #include "lldb/Target/Process.h"
 #include "lldb/Target/StackFrame.h"
 #include "lldb/Target/Target.h"
+#include "lldb/lldb-enumerations.h"
 #include "lldb/lldb-private-enumerations.h"
 
 using namespace lldb;
@@ -200,13 +202,6 @@
 const Target &target, const OptionGroupValueObjectDisplay &display_opts) {
   EvaluateExpressionOptions options;
   options.SetCoerceToId(display_opts.use_objc);
-  // Explicitly disabling persistent results takes precedence over the
-  // m_verbosity/use_objc logic.
-  if (suppress_persistent_result != eLazyBoolCalculate)
-options.SetSuppressPersistentResult(suppress_persistent_result ==
-eLazyBoolYes);
-  else if (m_verbosity == eLanguageRuntimeDescriptionDisplayVerbosityCompact)
-options.SetSuppressPersistentResult(display_opts.use_objc);
   options.SetUnwindOnError(unwind_on_error);
   options.SetIgnoreBreakpoints(ignore_breakpoints);
   options.SetKeepInMemory(true);
@@ -242,6 +237,17 @@
   return options;
 }
 
+bool CommandObjectExpression::CommandOptions::ShouldSuppressResult(
+const OptionGroupValueObjectDisplay &display_opts) const {
+  // Explicitly disabling persistent results takes precedence over the
+  // m_verbosity/use_objc logic.
+  if (suppress_persistent_result != eLazyBoolCalculate)
+return suppress_persistent_result == eLazyBoolYes;
+
+  return display_opts.use_objc &&
+ m_verbosity == eLanguageRuntimeDescriptionDisplayVerbosityCompact;
+}
+
 CommandObjectExpression::CommandObjectExpression(
 CommandInterpreter &interpreter)
 : CommandObjectRaw(interpreter, "expression",
@@ -424,8 +430,12 @@
 return false;
   }
 
-  const EvaluateExpressionOptions eval_options =
+  EvaluateExpressionOptions eval_options =
   m_command_options.GetEvaluateExpressionOptions(target, m_varobj_options);
+  // This command manually removes the result variable, make sure expression
+  // evaluation doesn't do it first.
+  eval_options.SetSuppressPersistentResult(false);
+
   ExpressionResults success = target.EvaluateExpression(
   expr, frame, result_valobj_sp, eval_options, &m_fixed_expression);
 
@@ -454,14 +464,25 @@
   }
 }
 
+bool suppress_result =
+m_command_options.ShouldSuppressResult(m_varobj_options);
+
 DumpValueObjectOptions options(m_varobj_options.GetAsDumpOptions(
 m_command_options.m_verbosity, format));
-options.SetHideRootName(eval_options.GetSuppressPersistentResult());
+options.SetHideRootName(suppress_result);
 options.SetVariableFormatDisplayLanguage(
 result_valobj_sp->GetPreferredDisplayLanguage());
 
 result_valobj_sp->Dump(output_stream, options);
 
+if (suppress_result)
+  if (auto result_var_sp =
+  target.GetPersistentVariable(result_valobj_sp->GetName())) {
+auto language = result_valobj_sp->GetPreferredDisplayLanguage();
+if (auto *persistent_state =
+target.GetPersistentExpressionStateForLanguage(language))
+  persistent_state->RemovePersistentVariable(result_var_sp);
+  }
 result.SetStatus(eReturnStatusSuccessFinishResult);
   }
 } else {
Index: lldb/source/Commands/CommandObjectDWIMPrint.cpp
===
--- lldb/source/Commands/CommandObjectDWIMPrint.cpp
+++ lldb/

[Lldb-commits] [PATCH] D150619: [lldb] Delay removal of persistent results

[Dave Lee via Phabricator via lldb-commits]

kastiglione added a comment.

thank you @jingham, that is great feedback!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150619

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