[LLDB] Ignore actual-needed artificial members in DWARFASTParserClang::ParseSingleMember #70779

hokein · 2023-10-31T10:33:08Z

Address the FIXME, this will allow lldb to print all fields of the generated coroutine frame structure.

Fixes #69309.

github-actions · 2023-10-31T10:44:48Z

✅ With the latest revision this PR passed the C/C++ code formatter.

vogelsgesang

I can't provide feedback on the lldb integration aspects, as I don't know the lldb code base well enough

vogelsgesang · 2023-10-31T11:19:45Z

lldb/source/Plugins/LanguageRuntime/CPlusPlus/CPPLanguageRuntime.cpp

+  // FIXME: use a list when the list grows more.
+  return name == g_this ||
+         name == ConstString("__promise") ||
+         name == ConstString("__coro_frame");


I am not sure if I would expose the __coro_frame variable. Maybe I would only expose the __promise variable by default. The __coro_frame is usually out-of-sync and does not hold the up-to-date coroutine state while the coroutine is in flight

Any chance we can get the Type from the Variable in CommandObjectFrameVariable and can re-use the IsCoroutineFrameType check?

I am not sure if I would expose the __coro_frame variable.
For completeness, I'm leaning towards exposing __coro_frame as well, even though it is not that useful.

Any chance we can get the Type from the Variable in CommandObjectFrameVariable and can re-use the IsCoroutineFrameType check?

Yes, we can get the Type in CommandObjectFrameVariable by calling valobj_sp->GetCompilerType();.
However it only works for the coroutine-frame variables, other variables like __promise (whose type is a user-defined promise_type) are still not available. One idea is to make the IsCoroutineFrameType more generic to check coroutine-relevant types (promise_type, coroutine_frame_type).

I removed this change from this pull request now (will sent as a followup). Let's focus on addressing the ignore-artificial-fields issue in DWARFASTParserClang.cpp.

Michael137 · 2023-11-01T01:43:10Z

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

@@ -3061,7 +3061,8 @@ 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 &&
+      !TypeSystemClang::IsCoroutineFrameType(class_clang_type)) {


I do still wonder if we should just make a ShouldIgnoreArtificialField API that returns true for known fields we want to skip. The only ones that comes to mind are artificial variables that start with _vptr$

Not sure if there are some Objective-C artificial vars we want to skip. Maybe @jimingham or @adrian-prantl would know

I changed to ShouldIgnoreArtificialField per suggest (unite tests are passed). It looks like the vtable pointer is the only case for C/C++.

hokein

Thanks for the review.

hokein · 2023-11-03T10:33:26Z

lldb/source/Plugins/LanguageRuntime/CPlusPlus/CPPLanguageRuntime.cpp

+  // FIXME: use a list when the list grows more.
+  return name == g_this ||
+         name == ConstString("__promise") ||
+         name == ConstString("__coro_frame");


I am not sure if I would expose the __coro_frame variable.
For completeness, I'm leaning towards exposing __coro_frame as well, even though it is not that useful.

Any chance we can get the Type from the Variable in CommandObjectFrameVariable and can re-use the IsCoroutineFrameType check?

Yes, we can get the Type in CommandObjectFrameVariable by calling valobj_sp->GetCompilerType();.
However it only works for the coroutine-frame variables, other variables like __promise (whose type is a user-defined promise_type) are still not available. One idea is to make the IsCoroutineFrameType more generic to check coroutine-relevant types (promise_type, coroutine_frame_type).

hokein · 2023-11-03T11:06:05Z

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

@@ -3061,7 +3061,8 @@ 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 &&
+      !TypeSystemClang::IsCoroutineFrameType(class_clang_type)) {


I changed to ShouldIgnoreArtificialField per suggest (unite tests are passed). It looks like the vtable pointer is the only case for C/C++.

Michael137 · 2023-11-07T12:15:59Z

lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp

@@ -771,6 +771,10 @@ TypeSystemClang *TypeSystemClang::GetASTContext(clang::ASTContext *ast) {
  return clang_ast;
 }

+bool TypeSystemClang::ShouldIgnoreArtificialField(llvm::StringRef Name) {
+  return Name.starts_with("_vptr$");


On GCC I see we get a slightly different name:

< 2><0x00000166> DW_TAG_member DW_AT_name _vptr.Base DW_AT_type <0x000001cc> DW_AT_data_member_location 0 DW_AT_artificial yes(1)

Thanks, good catch. Handled the gdb case and added a test case (I thought the Clang name in the filename and path name indicates that these part of code only cares about clang-generated code, but it looks like we use Clang type system to parse the DWARF debug info).

Do we care about the msvc-built program as well? I think? probably not, as we use a different debug info format PDB on Windows. And this class is only used in parsing DWARF.

TypeSystemClang should be agnostic to the debug-info format. The various debug-info->AST parsers (e.g., DWARFASTParserClang/PDBASTParser/npdb::PdbAstBuilder, the latter two being the MSVC debug-info parsers) use TypeSystemClang after they've parsed debug-info to construct clang AST nodes. But I'm not sure how the PDB parsers handle the vptr case and artificial variables. From a brief glance it doesn't look like it cares about artificial member variables at all.

Are you still planning on using this API from the CPPLanguageRuntime? If not, you could just put it inside of DWARFASTParserClang.cpp.

TypeSystemClang should be agnostic to the debug-info format. The various debug-info->AST parsers (e.g., DWARFASTParserClang/PDBASTParser/npdb::PdbAstBuilder, the latter two being the MSVC debug-info parsers) use TypeSystemClang after they've parsed debug-info to construct clang AST nodes.

Thanks for the clarification.

That makes sense. I don't have any plan to use this API. It is better to move DWARFASTParserClang.cpp (better layering).

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

* always populate all fields for the generated coroutine frame type; * make the artificial variables `__promise`, `__coro_frame` visible, so that they are present in the `frame var` command;

…tificial members we should ignore.

and remove the change in CPPLanguageRuntim.cpp.

llvmbot · 2023-11-09T13:33:44Z

@llvm/pr-subscribers-lldb

Author: Haojian Wu (hokein)

Changes

Part of the fixes #69309.

Address the FIXME, this will allow the lldb print all fields of the generated coroutine frame structure.

Full diff: https://github.com/llvm/llvm-project/pull/70779.diff

4 Files Affected:

(modified) lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp (+2-3)
(modified) lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp (+6)
(modified) lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h (+4)
(added) lldb/test/Shell/SymbolFile/DWARF/ignored_artificial_fields.test (+17)

diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
index 3174c18c97d888c..63260f28a7da85d 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
@@ -3059,9 +3059,8 @@ void DWARFASTParserClang::ParseSingleMember(
   // This needs to be done after updating FieldInfo which keeps track of where
   // field start/end so we don't later try to fill the space of this
   // 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 &&
+      TypeSystemClang::ShouldIgnoreArtificialField(attrs.name)) {
     last_field_info.SetIsArtificial(true);
     return;
   }
diff --git a/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp b/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
index 6f65587c4acedd1..e81fdc7bdfd8e40 100644
--- a/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
+++ b/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
@@ -771,6 +771,12 @@ TypeSystemClang *TypeSystemClang::GetASTContext(clang::ASTContext *ast) {
   return clang_ast;
 }
 
+bool TypeSystemClang::ShouldIgnoreArtificialField(llvm::StringRef Name) {
+  return Name.starts_with("_vptr$")
+         // gdb emit vtable pointer as "_vptr.classname"
+         || Name.starts_with("_vptr.");
+}
+
 clang::MangleContext *TypeSystemClang::getMangleContext() {
   if (m_mangle_ctx_up == nullptr)
     m_mangle_ctx_up.reset(getASTContext().createMangleContext());
diff --git a/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h b/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
index 0ec2d026e996105..477b655bb7c86ea 100644
--- a/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
+++ b/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
@@ -154,6 +154,10 @@ class TypeSystemClang : public TypeSystem {
 
   static TypeSystemClang *GetASTContext(clang::ASTContext *ast_ctx);
 
+  // Returns true if the given artificial field name should be ignored when
+  // parsing the DWARF.
+  static bool ShouldIgnoreArtificialField(llvm::StringRef FieldName);
+
   /// Returns the display name of this TypeSystemClang that indicates what
   /// purpose it serves in LLDB. Used for example in logs.
   llvm::StringRef getDisplayName() const { return m_display_name; }
diff --git a/lldb/test/Shell/SymbolFile/DWARF/ignored_artificial_fields.test b/lldb/test/Shell/SymbolFile/DWARF/ignored_artificial_fields.test
new file mode 100644
index 000000000000000..e7d3bc4b796224a
--- /dev/null
+++ b/lldb/test/Shell/SymbolFile/DWARF/ignored_artificial_fields.test
@@ -0,0 +1,17 @@
+# UNSUPPORTED: system-darwin, system-windows
+
+# Make sure the artifical field `vptr.ClassName` from gcc debug info is ignored.
+# RUN: %build --compiler=gcc %S/Inputs/debug-types-expressions.cpp -o %t
+# RUN: %lldb %t -s %s -o exit | FileCheck %s
+
+breakpoint set -n foo
+process launch
+
+# CHECK: Process {{.*}} stopped
+
+frame variable *a
+# CHECK-LABEL: frame variable *a
+# CHECK:      (B) *a = {
+# CHECK-NEXT:   A = (i = 47)
+# CHECK-NEXT:   j = 42
+# CHECK-NEXT: }

hokein · 2023-11-09T13:34:09Z

Thanks for the comment, I updated the patch, this is ready for another round of review.

DWARFASTParserClang.

lldb/test/Shell/SymbolFile/DWARF/ignored_artificial_fields.test

Michael137

LGTM

Are you still going to make the change to CommandObjectFrame such that frame var doesn't hide the artificial fields?

hokein · 2023-11-09T18:03:01Z

Thank you very much for the review!

Are you still going to make the change to CommandObjectFrame such that frame var doesn't hide the artificial fields?

Yes, that's my next step.

DavidSpickett · 2023-11-10T12:27:53Z

Hi, this is failing on Linaro's Linux LLDB bots: https://lab.llvm.org/buildbot/#/builders/96/builds/48277

Looks like the -m64 (or -m32) is being passed to an AArch64 (or ARM) g++, and those are X86 only flags.

DavidSpickett · 2023-11-10T12:48:36Z

I think I have a fix for this, will commit it soon.

hokein · 2023-11-10T12:50:37Z

Hi, this is failing on Linaro's Linux LLDB bots: https://lab.llvm.org/buildbot/#/builders/96/builds/48277

Looks like the -m64 (or -m32) is being passed to an AArch64 (or ARM) g++, and those are X86 only flags.

Sorry for the breakage. I removed this test temporarily in 343eb4b to make the builtbots happy again while doing the investigation.

hokein · 2023-11-10T12:52:27Z

I think I have a fix for this, will commit it soon.

Ah, I just saw your new message. If you can fix that, that would be fantastic (I don't know too much about lldb).

DavidSpickett · 2023-11-10T12:54:04Z

I just pushed a fix for the build script (7c3603e), I'll put the test case back once I know the script change is good.

hokein · 2023-11-10T13:01:43Z

Thank you very much for the fix!

DavidSpickett · 2023-11-10T15:54:46Z

All fixed now.

Caslyn · 2023-11-10T19:13:36Z

Hi @hokein - ignored_artificial_fields.test breaks if gcc is unavailable. For ex we purposely exclude gcc on our downstream bots so as not to implicitly depend on it. Maybe all that is needed is adding something like # REQUIRES: gcc (though I'm not sure if that is the exact LIT filter to use with gcc).

Would you be able to revert the test and/or patch it to only run when gcc is present?

hokein · 2023-11-10T19:58:12Z

@Caslyn , I added the # REQUIRES: gcc to the test in 81a7690

…::ParseSingleMember (llvm#70779) Address the FIXME, this will allow lldb to print all fields of the generated coroutine frame structure. Fixes llvm#69309.

hokein requested review from vogelsgesang and Michael137 October 31, 2023 10:33

hokein mentioned this pull request Oct 31, 2023

[lldb] No pretty print for the coroutine frame type. #69309

Closed

vogelsgesang reviewed Oct 31, 2023

View reviewed changes

Michael137 reviewed Nov 1, 2023

View reviewed changes

hokein commented Nov 3, 2023

View reviewed changes

Michael137 reviewed Nov 7, 2023

View reviewed changes

lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp Outdated Show resolved Hide resolved

hokein added 3 commits November 9, 2023 10:05

[LLDB] Don't ignore artificial variables and members for coroutines

2ff56f1

* always populate all fields for the generated coroutine frame type; * make the artificial variables `__promise`, `__coro_frame` visible, so that they are present in the `frame var` command;

Add and use ShouldIgnoreArtificialField API to ignore the indeed ar…

ac813b4

…tificial members we should ignore.

Handle the vptr gdb case, and add test.

e74494c

and remove the change in CPPLanguageRuntim.cpp.

hokein force-pushed the lldb-coroutines branch from c145045 to e74494c Compare November 9, 2023 13:23

hokein changed the title ~~[LLDB] Don't ignore artificial variables and members for coroutines~~ [LLDB] Ignore actual-needed artificial members in DWARFASTParserClang::ParseSingleMember Nov 9, 2023

hokein marked this pull request as ready for review November 9, 2023 13:33

hokein requested a review from JDevlieghere as a code owner November 9, 2023 13:33

llvmbot added the lldb label Nov 9, 2023

Move the ShouldIgnoreArtificialField from TypeSystemClang to

04318e3

DWARFASTParserClang.

Michael137 reviewed Nov 9, 2023

View reviewed changes

lldb/test/Shell/SymbolFile/DWARF/ignored_artificial_fields.test Show resolved Hide resolved

Michael137 approved these changes Nov 9, 2023

View reviewed changes

hokein merged commit 66acd1e into llvm:main Nov 10, 2023

hokein deleted the lldb-coroutines branch November 10, 2023 09:53

[LLDB] Ignore actual-needed artificial members in DWARFASTParserClang::ParseSingleMember #70779

[LLDB] Ignore actual-needed artificial members in DWARFASTParserClang::ParseSingleMember #70779

Uh oh!

Conversation

hokein commented Oct 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Oct 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vogelsgesang left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hokein left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

llvmbot commented Nov 9, 2023

Uh oh!

hokein commented Nov 9, 2023

Uh oh!

Uh oh!

Michael137 left a comment

Choose a reason for hiding this comment

Uh oh!

hokein commented Nov 9, 2023

Uh oh!

DavidSpickett commented Nov 10, 2023

Uh oh!

DavidSpickett commented Nov 10, 2023

Uh oh!

hokein commented Nov 10, 2023

Uh oh!

hokein commented Nov 10, 2023

Uh oh!

DavidSpickett commented Nov 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hokein commented Nov 10, 2023

Uh oh!

DavidSpickett commented Nov 10, 2023

Uh oh!

Caslyn commented Nov 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hokein commented Nov 10, 2023

Uh oh!

Uh oh!

hokein commented Oct 31, 2023 •

edited

Loading

github-actions bot commented Oct 31, 2023 •

edited

Loading

DavidSpickett commented Nov 10, 2023 •

edited

Loading

Caslyn commented Nov 10, 2023 •

edited

Loading