Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[clang][DebugInfo] Revert "emit definitions for constant-initialized static data-members" #74580

Merged
merged 2 commits into from
Dec 6, 2023

Conversation

Michael137
Copy link
Member

@Michael137 Michael137 commented Dec 6, 2023

This commit reverts the changes in #71780 and all of its follow-up patches.

We got reports of the .debug_names/.debug_gnu_pubnames/gdb_index/etc. sections growing by a non-trivial amount for some large projects. While GCC emits definitions for static data member constants into the Names index, they do so only for explicitly constexpr members. We were indexing all constant-initialized const-static members, which is likely where the significant size difference comes from. However, only emitting explicitly constexpr variables into the index doesn't seem like a good way forward, since from clang's perspective const-static integrals are constexpr too, and that shouldn't be any different in the debug-info component. Also, as new code moves to constexpr instead of const static for constants, such solution would just delay the growth of the Names index.

To prevent the size regression we revert to not emitting definitions for static data-members that have no location.

To support access to such constants from LLDB we'll most likely have to have to make LLDB find the constants by looking at the containing class first.

…tialized static data-members"

This commit reverts the changes in llvm#71780
and all of its follow-up patches.

We got reports of the `.debug_names/.debug_gnu_pubnames/gdb_index/etc.`
sections growing by a non-trivial amount for some large projects. While
GCC does index definitions for static data member constants, they
do so *only* for explicitly `constexpr` members. We were indexing
*all* constant-initialized const-static members, which is likely where
the significant size difference comes from. However, only emitting explicitly
`constexpr` variables into the index doesn't seem like a good way
forward, since from clang's perspective `const`-static integrals
are `constexpr` too, and that shouldn't be any different in the debug-info
component. Also, as new code moves to `constexpr` instead of `const` static
for constants, such solution would just delay the growth of the Names index.

To prevent the size regression we revert to not emitting definitions for
static data-members that have no location.

To support access to such constants from LLDB we'll most likely have to
have to make LLDB find the constants by looking at the containing class
first.
@llvmbot llvmbot added clang Clang issues not falling into any other category lldb clang:codegen debuginfo labels Dec 6, 2023
@Michael137 Michael137 changed the title [clang][DebugInfo] Revert "emit variable definitions for constant-initialized static data-members" [clang][DebugInfo] Revert "emit definitions for constant-initialized static data-members" Dec 6, 2023
@llvmbot
Copy link
Collaborator

llvmbot commented Dec 6, 2023

@llvm/pr-subscribers-lldb
@llvm/pr-subscribers-clang-codegen

@llvm/pr-subscribers-clang

Author: Michael Buch (Michael137)

Changes

This commit reverts the changes in #71780 and all of its follow-up patches.

We got reports of the .debug_names/.debug_gnu_pubnames/gdb_index/etc. sections growing by a non-trivial amount for some large projects. While GCC does index definitions for static data member constants, they do so only for explicitly constexpr members. We were indexing all constant-initialized const-static members, which is likely where the significant size difference comes from. However, only emitting explicitly constexpr variables into the index doesn't seem like a good way forward, since from clang's perspective const-static integrals are constexpr too, and that shouldn't be any different in the debug-info component. Also, as new code moves to constexpr instead of const static for constants, such solution would just delay the growth of the Names index.

To prevent the size regression we revert to not emitting definitions for static data-members that have no location.

To support access to such constants from LLDB we'll most likely have to have to make LLDB find the constants by looking at the containing class first.


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

7 Files Affected:

  • (modified) clang/lib/CodeGen/CGDebugInfo.cpp (-73)
  • (modified) clang/lib/CodeGen/CGDebugInfo.h (-6)
  • (modified) clang/test/CodeGenCXX/debug-info-class.cpp (+4-8)
  • (removed) clang/test/CodeGenCXX/debug-info-static-inline-member.cpp (-104)
  • (modified) clang/test/CodeGenCXX/debug-info-static-member.cpp (+4-16)
  • (modified) lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp (+2-62)
  • (modified) lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h (-11)
diff --git a/clang/lib/CodeGen/CGDebugInfo.cpp b/clang/lib/CodeGen/CGDebugInfo.cpp
index 602e325e560f9..7cf661994a29c 100644
--- a/clang/lib/CodeGen/CGDebugInfo.cpp
+++ b/clang/lib/CodeGen/CGDebugInfo.cpp
@@ -69,29 +69,6 @@ static uint32_t getDeclAlignIfRequired(const Decl *D, const ASTContext &Ctx) {
   return D->hasAttr<AlignedAttr>() ? D->getMaxAlignment() : 0;
 }
 
-/// Given a VarDecl corresponding to either the definition or
-/// declaration of a C++ static data member, if it has a constant
-/// initializer and is evaluatable, return the evaluated value.
-/// Returns std::nullopt otherwise.
-static std::optional<APValue>
-evaluateConstantInitializer(const clang::VarDecl *VD,
-                            const clang::ASTContext &Ctx) {
-  assert(VD != nullptr);
-
-  if (!VD->isStaticDataMember())
-    return std::nullopt;
-
-  if (!VD->isUsableInConstantExpressions(Ctx))
-    return std::nullopt;
-
-  auto const *InitExpr = VD->getAnyInitializer();
-  Expr::EvalResult Result;
-  if (!InitExpr->EvaluateAsConstantExpr(Result, Ctx))
-    return std::nullopt;
-
-  return Result.Val;
-}
-
 CGDebugInfo::CGDebugInfo(CodeGenModule &CGM)
     : CGM(CGM), DebugKind(CGM.getCodeGenOpts().getDebugInfo()),
       DebugTypeExtRefs(CGM.getCodeGenOpts().DebugTypeExtRefs),
@@ -1724,7 +1701,6 @@ CGDebugInfo::CreateRecordStaticField(const VarDecl *Var, llvm::DIType *RecordTy,
   llvm::DIDerivedType *GV = DBuilder.createStaticMemberType(
       RecordTy, VName, VUnit, LineNumber, VTy, Flags, C, Tag, Align);
   StaticDataMemberCache[Var->getCanonicalDecl()].reset(GV);
-  StaticDataMemberDefinitionsToEmit.push_back(Var->getCanonicalDecl());
   return GV;
 }
 
@@ -5628,41 +5604,6 @@ void CGDebugInfo::EmitGlobalVariable(const ValueDecl *VD, const APValue &Init) {
       TemplateParameters, Align));
 }
 
-void CGDebugInfo::EmitGlobalVariable(const VarDecl *VD) {
-  assert(CGM.getCodeGenOpts().hasReducedDebugInfo());
-  if (VD->hasAttr<NoDebugAttr>())
-    return;
-
-  const auto CacheIt = DeclCache.find(VD);
-  if (CacheIt != DeclCache.end())
-    return;
-
-  const auto InitVal = evaluateConstantInitializer(VD, CGM.getContext());
-  if (!InitVal)
-    return;
-
-  llvm::DIFile *Unit = nullptr;
-  llvm::DIScope *DContext = nullptr;
-  unsigned LineNo;
-  StringRef DeclName, LinkageName;
-  QualType T;
-  llvm::MDTuple *TemplateParameters = nullptr;
-  collectVarDeclProps(VD, Unit, LineNo, T, DeclName, LinkageName,
-                      TemplateParameters, DContext);
-
-  auto Align = getDeclAlignIfRequired(VD, CGM.getContext());
-  llvm::DINodeArray Annotations = CollectBTFDeclTagAnnotations(VD);
-  llvm::DIExpression *InitExpr = createConstantValueExpression(VD, *InitVal);
-
-  // Omit linkage name for variable definitions that represent constants.
-  // There hasn't been a need from consumers yet to have it attached.
-  DeclCache[VD].reset(DBuilder.createGlobalVariableExpression(
-      TheCU, DeclName, /* LinkageName */ {}, Unit, LineNo,
-      getOrCreateType(T, Unit), true, true, InitExpr,
-      getOrCreateStaticDataMemberDeclarationOrNull(VD), TemplateParameters,
-      Align, Annotations));
-}
-
 void CGDebugInfo::EmitExternalVariable(llvm::GlobalVariable *Var,
                                        const VarDecl *D) {
   assert(CGM.getCodeGenOpts().hasReducedDebugInfo());
@@ -5867,20 +5808,6 @@ void CGDebugInfo::setDwoId(uint64_t Signature) {
 }
 
 void CGDebugInfo::finalize() {
-  // We can't use a for-each here because `EmitGlobalVariable`
-  // may push new decls into `StaticDataMemberDefinitionsToEmit`,
-  // which would invalidate any iterator.
-  for (size_t i = 0; i < StaticDataMemberDefinitionsToEmit.size(); ++i) {
-    auto const *VD = StaticDataMemberDefinitionsToEmit[i];
-
-    assert(VD && VD->isStaticDataMember());
-
-    if (DeclCache.contains(VD))
-      continue;
-
-    EmitGlobalVariable(VD);
-  }
-
   // Creating types might create further types - invalidating the current
   // element and the size(), so don't cache/reference them.
   for (size_t i = 0; i != ObjCInterfaceCache.size(); ++i) {
diff --git a/clang/lib/CodeGen/CGDebugInfo.h b/clang/lib/CodeGen/CGDebugInfo.h
index 3e4c133b7f2b9..7b60e94555d06 100644
--- a/clang/lib/CodeGen/CGDebugInfo.h
+++ b/clang/lib/CodeGen/CGDebugInfo.h
@@ -161,9 +161,6 @@ class CGDebugInfo {
   llvm::DenseMap<const Decl *, llvm::TypedTrackingMDRef<llvm::DIDerivedType>>
       StaticDataMemberCache;
 
-  /// Keeps track of static data members for which we should emit a definition.
-  std::vector<const VarDecl *> StaticDataMemberDefinitionsToEmit;
-
   using ParamDecl2StmtTy = llvm::DenseMap<const ParmVarDecl *, const Stmt *>;
   using Param2DILocTy =
       llvm::DenseMap<const ParmVarDecl *, llvm::DILocalVariable *>;
@@ -529,9 +526,6 @@ class CGDebugInfo {
   /// Emit a constant global variable's debug info.
   void EmitGlobalVariable(const ValueDecl *VD, const APValue &Init);
 
-  /// Emit debug-info for a variable with a constant initializer.
-  void EmitGlobalVariable(const VarDecl *VD);
-
   /// Emit information about an external variable.
   void EmitExternalVariable(llvm::GlobalVariable *GV, const VarDecl *Decl);
 
diff --git a/clang/test/CodeGenCXX/debug-info-class.cpp b/clang/test/CodeGenCXX/debug-info-class.cpp
index a3111cd7c3640..8d610ca68a9d4 100644
--- a/clang/test/CodeGenCXX/debug-info-class.cpp
+++ b/clang/test/CodeGenCXX/debug-info-class.cpp
@@ -117,18 +117,11 @@ int main(int argc, char **argv) {
 // CHECK-NOT:                             identifier:
 // CHECK-SAME:                            ){{$}}
 
-// CHECK:      !DIGlobalVariableExpression(var: ![[HDR_VAR:[0-9]+]], expr: !DIExpression(DW_OP_constu, 52, DW_OP_stack_value))
-// CHECK:      ![[HDR_VAR]] = distinct !DIGlobalVariable(name: "HdrSize",
-// CHECK-SAME:                isLocal: true, isDefinition: true, declaration: ![[HDR_VAR_DECL:[0-9]+]])
-// CHECK:      ![[INT:[0-9]+]] = !DIBasicType(name: "int"
-// CHECK:      ![[HDR_VAR_DECL]] = !DIDerivedType(tag: DW_TAG_member, name: "HdrSize"
-
-// CHECK: !DICompositeType(tag: DW_TAG_structure_type, name: "A"
-
 // CHECK: !DICompositeType(tag: DW_TAG_structure_type, name: "I"
 // CHECK-NOT:              DIFlagFwdDecl
 // CHECK-SAME:             ){{$}}
 
+// CHECK:      ![[INT:[0-9]+]] = !DIBasicType(name: "int"
 // CHECK: !DICompositeType(tag: DW_TAG_structure_type, name: "foo"
 // CHECK: !DICompositeType(tag: DW_TAG_class_type, name: "bar"
 // CHECK: !DICompositeType(tag: DW_TAG_union_type, name: "baz"
@@ -194,5 +187,8 @@ int main(int argc, char **argv) {
 // CHECK: [[G_INNER_I]] = !DIDerivedType(tag: DW_TAG_member, name: "j"
 // CHECK-SAME:                           baseType: ![[INT]]
 
+// CHECK: !DICompositeType(tag: DW_TAG_structure_type, name: "A"
+// CHECK: !DIDerivedType(tag: DW_TAG_member, name: "HdrSize"
+
 // CHECK: ![[EXCEPTLOC]] = !DILocation(line: 100,
 // CHECK: ![[RETLOC]] = !DILocation(line: 99,
diff --git a/clang/test/CodeGenCXX/debug-info-static-inline-member.cpp b/clang/test/CodeGenCXX/debug-info-static-inline-member.cpp
deleted file mode 100644
index 3230b0e0c9c60..0000000000000
--- a/clang/test/CodeGenCXX/debug-info-static-inline-member.cpp
+++ /dev/null
@@ -1,104 +0,0 @@
-// RUN: %clangxx -target arm64-apple-macosx11.0.0 -g -gdwarf-4 -debug-info-kind=standalone %s -emit-llvm -S -o - | FileCheck --check-prefixes=CHECK %s
-// RUN: %clangxx -target arm64-apple-macosx11.0.0 -g -gdwarf-4 -debug-info-kind=limited %s -emit-llvm -S -o - | FileCheck --check-prefixes=CHECK %s
-
-enum class Enum : int {
-  VAL = -1
-};
-
-struct Empty {};
-struct Fwd;
-
-constexpr auto func() { return 25; }
-
-struct Foo {
-    static constexpr int cexpr_int_with_addr = func();
-    static constexpr int cexpr_int2 = func() + 1;
-    static constexpr float cexpr_float = 2.0 + 1.0;
-    static constexpr Enum cexpr_enum = Enum::VAL;
-    static constexpr Empty cexpr_struct_with_addr{};
-
-    template<typename T, unsigned V>
-    static constexpr auto cexpr_template = V;
-
-    static const auto empty_templated = cexpr_template<Fwd, 1>;
-};
-
-int main() {
-    Foo f;
-    //Bar b;
-
-    // Force global variable definitions to be emitted.
-    (void)&Foo::cexpr_int_with_addr;
-    (void)&Foo::cexpr_struct_with_addr;
-
-    return Foo::cexpr_int_with_addr + Foo::cexpr_float
-           + (int)Foo::cexpr_enum + Foo::cexpr_template<short, 5>
-           + Foo::empty_templated;
-}
-
-// CHECK:      @{{.*}}cexpr_int_with_addr{{.*}} =
-// CHECK-SAME:   !dbg ![[INT_GLOBAL:[0-9]+]]
-
-// CHECK:      @{{.*}}cexpr_struct_with_addr{{.*}} = 
-// CHECK-SAME    !dbg ![[EMPTY_GLOBAL:[0-9]+]]
-
-// CHECK:      !DIGlobalVariableExpression(var: ![[INT_VAR:[0-9]+]], expr: !DIExpression())
-// CHECK:      ![[INT_VAR]] = distinct !DIGlobalVariable(name: "cexpr_int_with_addr", linkageName:
-// CHECK-SAME:                isLocal: false, isDefinition: true, declaration: ![[INT_DECL:[0-9]+]])
-
-// CHECK:      ![[INT_DECL]] = !DIDerivedType(tag: DW_TAG_member, name: "cexpr_int_with_addr",
-// CHECK-SAME:                 flags: DIFlagStaticMember
-// CHECK-SAME:                 extraData: i32 25
-
-// CHECK:      ![[INT_DECL2:[0-9]+]] = !DIDerivedType(tag: DW_TAG_member, name: "cexpr_int2",
-// CHECK-SAME:                         flags: DIFlagStaticMember
-// CHECK-SAME:                         extraData: i32 26
-
-// CHECK:      ![[FLOAT_DECL:[0-9]+]] = !DIDerivedType(tag: DW_TAG_member, name: "cexpr_float",
-// CHECK-SAME:                          flags: DIFlagStaticMember
-// CHECK-SAME:                          extraData: float
-
-// CHECK:      ![[ENUM_DECL:[0-9]+]] = !DIDerivedType(tag: DW_TAG_member, name: "cexpr_enum",
-// CHECK-SAME:                         flags: DIFlagStaticMember
-// CHECK-SAME:                         extraData: i32 -1
-
-// CHECK:      ![[EMPTY_DECL:[0-9]+]] = !DIDerivedType(tag: DW_TAG_member, name: "cexpr_struct_with_addr",
-// CHECK-SAME:                          flags: DIFlagStaticMember
-// CHECK-NOT:                           extraData:
-
-// CHECK:      ![[EMPTY_TEMPLATED_DECL:[0-9]+]] = !DIDerivedType(tag: DW_TAG_member, name: "empty_templated",
-// CHECK-SAME:                                    flags: DIFlagStaticMember
-// CHECK-SAME:                                    extraData: i32 1
-
-// CHECK:      ![[TEMPLATE_DECL:[0-9]+]] = !DIDerivedType(tag: DW_TAG_member, name: "cexpr_template",
-// CHECK-SAME:                             flags: DIFlagStaticMember
-// CHECK-SAME:                             extraData: i32 1
-
-// CHECK:      !DIGlobalVariableExpression(var: ![[EMPTY_VAR:[0-9]+]], expr: !DIExpression())
-// CHECK:      ![[EMPTY_VAR]] = distinct !DIGlobalVariable(name: "cexpr_struct_with_addr", linkageName:
-// CHECK-SAME:                  isLocal: false, isDefinition: true, declaration: ![[EMPTY_DECL]])
-
-// CHECK:      !DIGlobalVariableExpression(var: ![[INT_VAR2:[0-9]+]], expr: !DIExpression(DW_OP_constu, 26, DW_OP_stack_value))
-// CHECK:      ![[INT_VAR2]] = distinct !DIGlobalVariable(name: "cexpr_int2"
-// CHECK-NOT:                  linkageName:
-// CHECK-SAME:                 isLocal: true, isDefinition: true, declaration: ![[INT_DECL2]])
-
-// CHECK:      !DIGlobalVariableExpression(var: ![[FLOAT_VAR:[0-9]+]], expr: !DIExpression(DW_OP_constu, {{.*}}, DW_OP_stack_value))
-// CHECK:      ![[FLOAT_VAR]] = distinct !DIGlobalVariable(name: "cexpr_float"
-// CHECK-NOT:                   linkageName:
-// CHECK-SAME:                  isLocal: true, isDefinition: true, declaration: ![[FLOAT_DECL]])
-
-// CHECK:      !DIGlobalVariableExpression(var: ![[ENUM_VAR:[0-9]+]], expr: !DIExpression(DW_OP_constu, {{.*}}, DW_OP_stack_value))
-// CHECK:      ![[ENUM_VAR]] = distinct !DIGlobalVariable(name: "cexpr_enum"
-// CHECK-NOT:                  linkageName:
-// CHECK-SAME:                 isLocal: true, isDefinition: true, declaration: ![[ENUM_DECL]])
-
-// CHECK:      !DIGlobalVariableExpression(var: ![[EMPTY_TEMPLATED_VAR:[0-9]+]], expr: !DIExpression(DW_OP_constu, 1, DW_OP_stack_value))
-// CHECK:      ![[EMPTY_TEMPLATED_VAR]] = distinct !DIGlobalVariable(name: "empty_templated"
-// CHECK-NOT:                             linkageName:
-// CHECK-SAME:                            isLocal: true, isDefinition: true, declaration: ![[EMPTY_TEMPLATED_DECL]])
-
-// CHECK:      !DIGlobalVariableExpression(var: ![[TEMPLATE_VAR:[0-9]+]], expr: !DIExpression(DW_OP_constu, 5, DW_OP_stack_value))
-// CHECK:      ![[TEMPLATE_VAR]] = distinct !DIGlobalVariable(name: "cexpr_template"
-// CHECK-NOT:                      linkageName:
-// CHECK-SAME:                     isLocal: true, isDefinition: true, declaration: ![[TEMPLATE_DECL]], templateParams: ![[TEMPLATE_PARMS_2:[0-9]+]])
diff --git a/clang/test/CodeGenCXX/debug-info-static-member.cpp b/clang/test/CodeGenCXX/debug-info-static-member.cpp
index a111dc84b6e66..972ca62d7b267 100644
--- a/clang/test/CodeGenCXX/debug-info-static-member.cpp
+++ b/clang/test/CodeGenCXX/debug-info-static-member.cpp
@@ -1,8 +1,8 @@
-// RUN: %clangxx -target x86_64-unknown-unknown -g -gdwarf-4 %s -emit-llvm -S -o - | FileCheck --check-prefixes=CHECK,DWARF4,CPP11,NOT-MS %s
+// RUN: %clangxx -target x86_64-unknown-unknown -g -gdwarf-4 %s -emit-llvm -S -o - | FileCheck --check-prefixes=CHECK,DWARF4,NOT-MS %s
 // RUN: %clangxx -target x86_64-unknown-unknown -g -gdwarf-4 -std=c++98 %s -emit-llvm -S -o - | FileCheck --check-prefixes=CHECK,DWARF4,NOT-MS %s
-// RUN: %clangxx -target x86_64-unknown-unknown -g -gdwarf-4 -std=c++11 %s -emit-llvm -S -o - | FileCheck --check-prefixes=CHECK,DWARF4,CPP11,NOT-MS %s
-// RUN: %clangxx -target x86_64-unknown-unknown -g -gdwarf-5 -std=c++11 %s -emit-llvm -S -o - | FileCheck --check-prefixes=CHECK,DWARF5,CPP11 %s
-// RUN: %clangxx -target x86_64-windows-msvc -g -gdwarf-4 %s -emit-llvm -S -o - | FileCheck --check-prefixes=CHECK,DWARF4,CPP11 %s
+// RUN: %clangxx -target x86_64-unknown-unknown -g -gdwarf-4 -std=c++11 %s -emit-llvm -S -o - | FileCheck --check-prefixes=CHECK,DWARF4,NOT-MS %s
+// RUN: %clangxx -target x86_64-unknown-unknown -g -gdwarf-5 -std=c++11 %s -emit-llvm -S -o - | FileCheck --check-prefixes=CHECK,DWARF5 %s
+// RUN: %clangxx -target x86_64-windows-msvc -g -gdwarf-4 %s -emit-llvm -S -o - | FileCheck --check-prefixes=CHECK,DWARF4 %s
 // PR14471
 
 // CHECK: @{{.*}}a{{.*}} = dso_local global i32 4, align 4, !dbg [[A:![0-9]+]]
@@ -166,15 +166,3 @@ struct y {
 };
 int y::z;
 }
-
-// CHECK:      !DIGlobalVariableExpression(var: ![[CONST_A_VAR:[0-9]+]], expr: !DIExpression(DW_OP_constu, 1, DW_OP_stack_value))
-// CHECK:      ![[CONST_A_VAR]] = distinct !DIGlobalVariable(name: "const_a"
-// CHECK-SAME:                    isLocal: true, isDefinition: true, declaration: ![[CONST_A_DECL]])
-
-// CPP11:      !DIGlobalVariableExpression(var: ![[CONST_B_VAR:[0-9]+]], expr: !DIExpression(DW_OP_constu, {{.*}}, DW_OP_stack_value))
-// CPP11:      ![[CONST_B_VAR]] = distinct !DIGlobalVariable(name: "const_b"
-// CPP11-SAME:                    isLocal: true, isDefinition: true, declaration: ![[CONST_B_DECL]])
-
-// CHECK:      !DIGlobalVariableExpression(var: ![[CONST_C_VAR:[0-9]+]], expr: !DIExpression(DW_OP_constu, 18, DW_OP_stack_value))
-// CHECK:      ![[CONST_C_VAR]] = distinct !DIGlobalVariable(name: "const_c"
-// CHECK-SAME:                    isLocal: true, isDefinition: true, declaration: ![[CONST_C_DECL]])
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
index c3b22f889c2f4..e3c64640c7917 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
@@ -142,54 +142,6 @@ static bool ShouldIgnoreArtificialField(llvm::StringRef FieldName) {
          || FieldName.starts_with("_vptr.");
 }
 
-std::optional<DWARFFormValue>
-DWARFASTParserClang::FindConstantOnVariableDefinition(DWARFDIE die) {
-  assert(die.Tag() == DW_TAG_member || die.Tag() == DW_TAG_variable);
-
-  auto *dwarf = die.GetDWARF();
-  if (!dwarf)
-    return {};
-
-  ConstString name{die.GetName()};
-  if (!name)
-    return {};
-
-  auto *CU = die.GetCU();
-  if (!CU)
-    return {};
-
-  DWARFASTParser *dwarf_ast = dwarf->GetDWARFParser(*CU);
-  auto parent_decl_ctx = dwarf_ast->GetDeclContextContainingUIDFromDWARF(die);
-
-  // Make sure we populate the GetDieToVariable cache.
-  VariableList variables;
-  dwarf->FindGlobalVariables(name, parent_decl_ctx, UINT_MAX, variables);
-
-  // The cache contains the variable definition whose DW_AT_specification
-  // points to our declaration DIE. Look up that definition using our
-  // declaration.
-  auto const &die_to_var = dwarf->GetDIEToVariable();
-  auto it = die_to_var.find(die.GetDIE());
-  if (it == die_to_var.end())
-    return {};
-
-  auto var_sp = it->getSecond();
-  assert(var_sp != nullptr);
-
-  if (!var_sp->GetLocationIsConstantValueData())
-    return {};
-
-  auto def = dwarf->GetDIE(var_sp->GetID());
-  auto def_attrs = def.GetAttributes();
-  DWARFFormValue form_value;
-  if (!def_attrs.ExtractFormValueAtIndex(
-          def_attrs.FindAttributeIndex(llvm::dwarf::DW_AT_const_value),
-          form_value))
-    return {};
-
-  return form_value;
-}
-
 TypeSP DWARFASTParserClang::ParseTypeFromClangModule(const SymbolContext &sc,
                                                      const DWARFDIE &die,
                                                      Log *log) {
@@ -2916,23 +2868,11 @@ void DWARFASTParserClang::CreateStaticMemberVariable(
 
   bool unused;
   // TODO: Support float/double static members as well.
-  if (!ct.IsIntegerOrEnumerationType(unused))
+  if (!ct.IsIntegerOrEnumerationType(unused) || !attrs.const_value_form)
     return;
 
-  auto maybe_const_form_value = attrs.const_value_form;
-
-  // Newer versions of Clang don't emit the DW_AT_const_value
-  // on the declaration of an inline static data member. Instead
-  // it's attached to the definition DIE. If that's the case,
-  // try and fetch it.
-  if (!maybe_const_form_value) {
-    maybe_const_form_value = FindConstantOnVariableDefinition(die);
-    if (!maybe_const_form_value)
-      return;
-  }
-
   llvm::Expected<llvm::APInt> const_value_or_err =
-      ExtractIntFromFormValue(ct, *maybe_const_form_value);
+      ExtractIntFromFormValue(ct, *attrs.const_value_form);
   if (!const_value_or_err) {
     LLDB_LOG_ERROR(log, const_value_or_err.takeError(),
                    "Failed to add const value to variable {1}: {0}",
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h
index 7b495419cf324..3e28e54d6220d 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h
@@ -413,17 +413,6 @@ class DWARFASTParserClang : public lldb_private::plugin::dwarf::DWARFASTParser {
                        lldb_private::CompilerType &class_clang_type,
                        const lldb::AccessType default_accesibility,
                        lldb_private::ClangASTImporter::LayoutInfo &layout_info);
-
-  /// Tries to find the definition DW_TAG_variable DIE of the the specified
-  /// DW_TAG_member 'die'. If such definition exists, returns the
-  /// DW_AT_const_value of that definition if available. Returns std::nullopt
-  /// otherwise.
-  ///
-  /// In newer versions of clang, DW_AT_const_value attributes are not attached
-  /// to the declaration of a inline static data-member anymore, but rather on
-  /// its definition. This function is used to locate said constant.
-  std::optional<lldb_private::plugin::dwarf::DWARFFormValue>
-  FindConstantOnVariableDefinition(lldb_private::plugin::dwarf::DWARFDIE die);
 };
 
 /// Parsed form of all attributes that are relevant for type reconstruction.

@llvmbot
Copy link
Collaborator

llvmbot commented Dec 6, 2023

@llvm/pr-subscribers-debuginfo

Author: Michael Buch (Michael137)

Changes

This commit reverts the changes in #71780 and all of its follow-up patches.

We got reports of the .debug_names/.debug_gnu_pubnames/gdb_index/etc. sections growing by a non-trivial amount for some large projects. While GCC does index definitions for static data member constants, they do so only for explicitly constexpr members. We were indexing all constant-initialized const-static members, which is likely where the significant size difference comes from. However, only emitting explicitly constexpr variables into the index doesn't seem like a good way forward, since from clang's perspective const-static integrals are constexpr too, and that shouldn't be any different in the debug-info component. Also, as new code moves to constexpr instead of const static for constants, such solution would just delay the growth of the Names index.

To prevent the size regression we revert to not emitting definitions for static data-members that have no location.

To support access to such constants from LLDB we'll most likely have to have to make LLDB find the constants by looking at the containing class first.


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

7 Files Affected:

  • (modified) clang/lib/CodeGen/CGDebugInfo.cpp (-73)
  • (modified) clang/lib/CodeGen/CGDebugInfo.h (-6)
  • (modified) clang/test/CodeGenCXX/debug-info-class.cpp (+4-8)
  • (removed) clang/test/CodeGenCXX/debug-info-static-inline-member.cpp (-104)
  • (modified) clang/test/CodeGenCXX/debug-info-static-member.cpp (+4-16)
  • (modified) lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp (+2-62)
  • (modified) lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h (-11)
diff --git a/clang/lib/CodeGen/CGDebugInfo.cpp b/clang/lib/CodeGen/CGDebugInfo.cpp
index 602e325e560f9..7cf661994a29c 100644
--- a/clang/lib/CodeGen/CGDebugInfo.cpp
+++ b/clang/lib/CodeGen/CGDebugInfo.cpp
@@ -69,29 +69,6 @@ static uint32_t getDeclAlignIfRequired(const Decl *D, const ASTContext &Ctx) {
   return D->hasAttr<AlignedAttr>() ? D->getMaxAlignment() : 0;
 }
 
-/// Given a VarDecl corresponding to either the definition or
-/// declaration of a C++ static data member, if it has a constant
-/// initializer and is evaluatable, return the evaluated value.
-/// Returns std::nullopt otherwise.
-static std::optional<APValue>
-evaluateConstantInitializer(const clang::VarDecl *VD,
-                            const clang::ASTContext &Ctx) {
-  assert(VD != nullptr);
-
-  if (!VD->isStaticDataMember())
-    return std::nullopt;
-
-  if (!VD->isUsableInConstantExpressions(Ctx))
-    return std::nullopt;
-
-  auto const *InitExpr = VD->getAnyInitializer();
-  Expr::EvalResult Result;
-  if (!InitExpr->EvaluateAsConstantExpr(Result, Ctx))
-    return std::nullopt;
-
-  return Result.Val;
-}
-
 CGDebugInfo::CGDebugInfo(CodeGenModule &CGM)
     : CGM(CGM), DebugKind(CGM.getCodeGenOpts().getDebugInfo()),
       DebugTypeExtRefs(CGM.getCodeGenOpts().DebugTypeExtRefs),
@@ -1724,7 +1701,6 @@ CGDebugInfo::CreateRecordStaticField(const VarDecl *Var, llvm::DIType *RecordTy,
   llvm::DIDerivedType *GV = DBuilder.createStaticMemberType(
       RecordTy, VName, VUnit, LineNumber, VTy, Flags, C, Tag, Align);
   StaticDataMemberCache[Var->getCanonicalDecl()].reset(GV);
-  StaticDataMemberDefinitionsToEmit.push_back(Var->getCanonicalDecl());
   return GV;
 }
 
@@ -5628,41 +5604,6 @@ void CGDebugInfo::EmitGlobalVariable(const ValueDecl *VD, const APValue &Init) {
       TemplateParameters, Align));
 }
 
-void CGDebugInfo::EmitGlobalVariable(const VarDecl *VD) {
-  assert(CGM.getCodeGenOpts().hasReducedDebugInfo());
-  if (VD->hasAttr<NoDebugAttr>())
-    return;
-
-  const auto CacheIt = DeclCache.find(VD);
-  if (CacheIt != DeclCache.end())
-    return;
-
-  const auto InitVal = evaluateConstantInitializer(VD, CGM.getContext());
-  if (!InitVal)
-    return;
-
-  llvm::DIFile *Unit = nullptr;
-  llvm::DIScope *DContext = nullptr;
-  unsigned LineNo;
-  StringRef DeclName, LinkageName;
-  QualType T;
-  llvm::MDTuple *TemplateParameters = nullptr;
-  collectVarDeclProps(VD, Unit, LineNo, T, DeclName, LinkageName,
-                      TemplateParameters, DContext);
-
-  auto Align = getDeclAlignIfRequired(VD, CGM.getContext());
-  llvm::DINodeArray Annotations = CollectBTFDeclTagAnnotations(VD);
-  llvm::DIExpression *InitExpr = createConstantValueExpression(VD, *InitVal);
-
-  // Omit linkage name for variable definitions that represent constants.
-  // There hasn't been a need from consumers yet to have it attached.
-  DeclCache[VD].reset(DBuilder.createGlobalVariableExpression(
-      TheCU, DeclName, /* LinkageName */ {}, Unit, LineNo,
-      getOrCreateType(T, Unit), true, true, InitExpr,
-      getOrCreateStaticDataMemberDeclarationOrNull(VD), TemplateParameters,
-      Align, Annotations));
-}
-
 void CGDebugInfo::EmitExternalVariable(llvm::GlobalVariable *Var,
                                        const VarDecl *D) {
   assert(CGM.getCodeGenOpts().hasReducedDebugInfo());
@@ -5867,20 +5808,6 @@ void CGDebugInfo::setDwoId(uint64_t Signature) {
 }
 
 void CGDebugInfo::finalize() {
-  // We can't use a for-each here because `EmitGlobalVariable`
-  // may push new decls into `StaticDataMemberDefinitionsToEmit`,
-  // which would invalidate any iterator.
-  for (size_t i = 0; i < StaticDataMemberDefinitionsToEmit.size(); ++i) {
-    auto const *VD = StaticDataMemberDefinitionsToEmit[i];
-
-    assert(VD && VD->isStaticDataMember());
-
-    if (DeclCache.contains(VD))
-      continue;
-
-    EmitGlobalVariable(VD);
-  }
-
   // Creating types might create further types - invalidating the current
   // element and the size(), so don't cache/reference them.
   for (size_t i = 0; i != ObjCInterfaceCache.size(); ++i) {
diff --git a/clang/lib/CodeGen/CGDebugInfo.h b/clang/lib/CodeGen/CGDebugInfo.h
index 3e4c133b7f2b9..7b60e94555d06 100644
--- a/clang/lib/CodeGen/CGDebugInfo.h
+++ b/clang/lib/CodeGen/CGDebugInfo.h
@@ -161,9 +161,6 @@ class CGDebugInfo {
   llvm::DenseMap<const Decl *, llvm::TypedTrackingMDRef<llvm::DIDerivedType>>
       StaticDataMemberCache;
 
-  /// Keeps track of static data members for which we should emit a definition.
-  std::vector<const VarDecl *> StaticDataMemberDefinitionsToEmit;
-
   using ParamDecl2StmtTy = llvm::DenseMap<const ParmVarDecl *, const Stmt *>;
   using Param2DILocTy =
       llvm::DenseMap<const ParmVarDecl *, llvm::DILocalVariable *>;
@@ -529,9 +526,6 @@ class CGDebugInfo {
   /// Emit a constant global variable's debug info.
   void EmitGlobalVariable(const ValueDecl *VD, const APValue &Init);
 
-  /// Emit debug-info for a variable with a constant initializer.
-  void EmitGlobalVariable(const VarDecl *VD);
-
   /// Emit information about an external variable.
   void EmitExternalVariable(llvm::GlobalVariable *GV, const VarDecl *Decl);
 
diff --git a/clang/test/CodeGenCXX/debug-info-class.cpp b/clang/test/CodeGenCXX/debug-info-class.cpp
index a3111cd7c3640..8d610ca68a9d4 100644
--- a/clang/test/CodeGenCXX/debug-info-class.cpp
+++ b/clang/test/CodeGenCXX/debug-info-class.cpp
@@ -117,18 +117,11 @@ int main(int argc, char **argv) {
 // CHECK-NOT:                             identifier:
 // CHECK-SAME:                            ){{$}}
 
-// CHECK:      !DIGlobalVariableExpression(var: ![[HDR_VAR:[0-9]+]], expr: !DIExpression(DW_OP_constu, 52, DW_OP_stack_value))
-// CHECK:      ![[HDR_VAR]] = distinct !DIGlobalVariable(name: "HdrSize",
-// CHECK-SAME:                isLocal: true, isDefinition: true, declaration: ![[HDR_VAR_DECL:[0-9]+]])
-// CHECK:      ![[INT:[0-9]+]] = !DIBasicType(name: "int"
-// CHECK:      ![[HDR_VAR_DECL]] = !DIDerivedType(tag: DW_TAG_member, name: "HdrSize"
-
-// CHECK: !DICompositeType(tag: DW_TAG_structure_type, name: "A"
-
 // CHECK: !DICompositeType(tag: DW_TAG_structure_type, name: "I"
 // CHECK-NOT:              DIFlagFwdDecl
 // CHECK-SAME:             ){{$}}
 
+// CHECK:      ![[INT:[0-9]+]] = !DIBasicType(name: "int"
 // CHECK: !DICompositeType(tag: DW_TAG_structure_type, name: "foo"
 // CHECK: !DICompositeType(tag: DW_TAG_class_type, name: "bar"
 // CHECK: !DICompositeType(tag: DW_TAG_union_type, name: "baz"
@@ -194,5 +187,8 @@ int main(int argc, char **argv) {
 // CHECK: [[G_INNER_I]] = !DIDerivedType(tag: DW_TAG_member, name: "j"
 // CHECK-SAME:                           baseType: ![[INT]]
 
+// CHECK: !DICompositeType(tag: DW_TAG_structure_type, name: "A"
+// CHECK: !DIDerivedType(tag: DW_TAG_member, name: "HdrSize"
+
 // CHECK: ![[EXCEPTLOC]] = !DILocation(line: 100,
 // CHECK: ![[RETLOC]] = !DILocation(line: 99,
diff --git a/clang/test/CodeGenCXX/debug-info-static-inline-member.cpp b/clang/test/CodeGenCXX/debug-info-static-inline-member.cpp
deleted file mode 100644
index 3230b0e0c9c60..0000000000000
--- a/clang/test/CodeGenCXX/debug-info-static-inline-member.cpp
+++ /dev/null
@@ -1,104 +0,0 @@
-// RUN: %clangxx -target arm64-apple-macosx11.0.0 -g -gdwarf-4 -debug-info-kind=standalone %s -emit-llvm -S -o - | FileCheck --check-prefixes=CHECK %s
-// RUN: %clangxx -target arm64-apple-macosx11.0.0 -g -gdwarf-4 -debug-info-kind=limited %s -emit-llvm -S -o - | FileCheck --check-prefixes=CHECK %s
-
-enum class Enum : int {
-  VAL = -1
-};
-
-struct Empty {};
-struct Fwd;
-
-constexpr auto func() { return 25; }
-
-struct Foo {
-    static constexpr int cexpr_int_with_addr = func();
-    static constexpr int cexpr_int2 = func() + 1;
-    static constexpr float cexpr_float = 2.0 + 1.0;
-    static constexpr Enum cexpr_enum = Enum::VAL;
-    static constexpr Empty cexpr_struct_with_addr{};
-
-    template<typename T, unsigned V>
-    static constexpr auto cexpr_template = V;
-
-    static const auto empty_templated = cexpr_template<Fwd, 1>;
-};
-
-int main() {
-    Foo f;
-    //Bar b;
-
-    // Force global variable definitions to be emitted.
-    (void)&Foo::cexpr_int_with_addr;
-    (void)&Foo::cexpr_struct_with_addr;
-
-    return Foo::cexpr_int_with_addr + Foo::cexpr_float
-           + (int)Foo::cexpr_enum + Foo::cexpr_template<short, 5>
-           + Foo::empty_templated;
-}
-
-// CHECK:      @{{.*}}cexpr_int_with_addr{{.*}} =
-// CHECK-SAME:   !dbg ![[INT_GLOBAL:[0-9]+]]
-
-// CHECK:      @{{.*}}cexpr_struct_with_addr{{.*}} = 
-// CHECK-SAME    !dbg ![[EMPTY_GLOBAL:[0-9]+]]
-
-// CHECK:      !DIGlobalVariableExpression(var: ![[INT_VAR:[0-9]+]], expr: !DIExpression())
-// CHECK:      ![[INT_VAR]] = distinct !DIGlobalVariable(name: "cexpr_int_with_addr", linkageName:
-// CHECK-SAME:                isLocal: false, isDefinition: true, declaration: ![[INT_DECL:[0-9]+]])
-
-// CHECK:      ![[INT_DECL]] = !DIDerivedType(tag: DW_TAG_member, name: "cexpr_int_with_addr",
-// CHECK-SAME:                 flags: DIFlagStaticMember
-// CHECK-SAME:                 extraData: i32 25
-
-// CHECK:      ![[INT_DECL2:[0-9]+]] = !DIDerivedType(tag: DW_TAG_member, name: "cexpr_int2",
-// CHECK-SAME:                         flags: DIFlagStaticMember
-// CHECK-SAME:                         extraData: i32 26
-
-// CHECK:      ![[FLOAT_DECL:[0-9]+]] = !DIDerivedType(tag: DW_TAG_member, name: "cexpr_float",
-// CHECK-SAME:                          flags: DIFlagStaticMember
-// CHECK-SAME:                          extraData: float
-
-// CHECK:      ![[ENUM_DECL:[0-9]+]] = !DIDerivedType(tag: DW_TAG_member, name: "cexpr_enum",
-// CHECK-SAME:                         flags: DIFlagStaticMember
-// CHECK-SAME:                         extraData: i32 -1
-
-// CHECK:      ![[EMPTY_DECL:[0-9]+]] = !DIDerivedType(tag: DW_TAG_member, name: "cexpr_struct_with_addr",
-// CHECK-SAME:                          flags: DIFlagStaticMember
-// CHECK-NOT:                           extraData:
-
-// CHECK:      ![[EMPTY_TEMPLATED_DECL:[0-9]+]] = !DIDerivedType(tag: DW_TAG_member, name: "empty_templated",
-// CHECK-SAME:                                    flags: DIFlagStaticMember
-// CHECK-SAME:                                    extraData: i32 1
-
-// CHECK:      ![[TEMPLATE_DECL:[0-9]+]] = !DIDerivedType(tag: DW_TAG_member, name: "cexpr_template",
-// CHECK-SAME:                             flags: DIFlagStaticMember
-// CHECK-SAME:                             extraData: i32 1
-
-// CHECK:      !DIGlobalVariableExpression(var: ![[EMPTY_VAR:[0-9]+]], expr: !DIExpression())
-// CHECK:      ![[EMPTY_VAR]] = distinct !DIGlobalVariable(name: "cexpr_struct_with_addr", linkageName:
-// CHECK-SAME:                  isLocal: false, isDefinition: true, declaration: ![[EMPTY_DECL]])
-
-// CHECK:      !DIGlobalVariableExpression(var: ![[INT_VAR2:[0-9]+]], expr: !DIExpression(DW_OP_constu, 26, DW_OP_stack_value))
-// CHECK:      ![[INT_VAR2]] = distinct !DIGlobalVariable(name: "cexpr_int2"
-// CHECK-NOT:                  linkageName:
-// CHECK-SAME:                 isLocal: true, isDefinition: true, declaration: ![[INT_DECL2]])
-
-// CHECK:      !DIGlobalVariableExpression(var: ![[FLOAT_VAR:[0-9]+]], expr: !DIExpression(DW_OP_constu, {{.*}}, DW_OP_stack_value))
-// CHECK:      ![[FLOAT_VAR]] = distinct !DIGlobalVariable(name: "cexpr_float"
-// CHECK-NOT:                   linkageName:
-// CHECK-SAME:                  isLocal: true, isDefinition: true, declaration: ![[FLOAT_DECL]])
-
-// CHECK:      !DIGlobalVariableExpression(var: ![[ENUM_VAR:[0-9]+]], expr: !DIExpression(DW_OP_constu, {{.*}}, DW_OP_stack_value))
-// CHECK:      ![[ENUM_VAR]] = distinct !DIGlobalVariable(name: "cexpr_enum"
-// CHECK-NOT:                  linkageName:
-// CHECK-SAME:                 isLocal: true, isDefinition: true, declaration: ![[ENUM_DECL]])
-
-// CHECK:      !DIGlobalVariableExpression(var: ![[EMPTY_TEMPLATED_VAR:[0-9]+]], expr: !DIExpression(DW_OP_constu, 1, DW_OP_stack_value))
-// CHECK:      ![[EMPTY_TEMPLATED_VAR]] = distinct !DIGlobalVariable(name: "empty_templated"
-// CHECK-NOT:                             linkageName:
-// CHECK-SAME:                            isLocal: true, isDefinition: true, declaration: ![[EMPTY_TEMPLATED_DECL]])
-
-// CHECK:      !DIGlobalVariableExpression(var: ![[TEMPLATE_VAR:[0-9]+]], expr: !DIExpression(DW_OP_constu, 5, DW_OP_stack_value))
-// CHECK:      ![[TEMPLATE_VAR]] = distinct !DIGlobalVariable(name: "cexpr_template"
-// CHECK-NOT:                      linkageName:
-// CHECK-SAME:                     isLocal: true, isDefinition: true, declaration: ![[TEMPLATE_DECL]], templateParams: ![[TEMPLATE_PARMS_2:[0-9]+]])
diff --git a/clang/test/CodeGenCXX/debug-info-static-member.cpp b/clang/test/CodeGenCXX/debug-info-static-member.cpp
index a111dc84b6e66..972ca62d7b267 100644
--- a/clang/test/CodeGenCXX/debug-info-static-member.cpp
+++ b/clang/test/CodeGenCXX/debug-info-static-member.cpp
@@ -1,8 +1,8 @@
-// RUN: %clangxx -target x86_64-unknown-unknown -g -gdwarf-4 %s -emit-llvm -S -o - | FileCheck --check-prefixes=CHECK,DWARF4,CPP11,NOT-MS %s
+// RUN: %clangxx -target x86_64-unknown-unknown -g -gdwarf-4 %s -emit-llvm -S -o - | FileCheck --check-prefixes=CHECK,DWARF4,NOT-MS %s
 // RUN: %clangxx -target x86_64-unknown-unknown -g -gdwarf-4 -std=c++98 %s -emit-llvm -S -o - | FileCheck --check-prefixes=CHECK,DWARF4,NOT-MS %s
-// RUN: %clangxx -target x86_64-unknown-unknown -g -gdwarf-4 -std=c++11 %s -emit-llvm -S -o - | FileCheck --check-prefixes=CHECK,DWARF4,CPP11,NOT-MS %s
-// RUN: %clangxx -target x86_64-unknown-unknown -g -gdwarf-5 -std=c++11 %s -emit-llvm -S -o - | FileCheck --check-prefixes=CHECK,DWARF5,CPP11 %s
-// RUN: %clangxx -target x86_64-windows-msvc -g -gdwarf-4 %s -emit-llvm -S -o - | FileCheck --check-prefixes=CHECK,DWARF4,CPP11 %s
+// RUN: %clangxx -target x86_64-unknown-unknown -g -gdwarf-4 -std=c++11 %s -emit-llvm -S -o - | FileCheck --check-prefixes=CHECK,DWARF4,NOT-MS %s
+// RUN: %clangxx -target x86_64-unknown-unknown -g -gdwarf-5 -std=c++11 %s -emit-llvm -S -o - | FileCheck --check-prefixes=CHECK,DWARF5 %s
+// RUN: %clangxx -target x86_64-windows-msvc -g -gdwarf-4 %s -emit-llvm -S -o - | FileCheck --check-prefixes=CHECK,DWARF4 %s
 // PR14471
 
 // CHECK: @{{.*}}a{{.*}} = dso_local global i32 4, align 4, !dbg [[A:![0-9]+]]
@@ -166,15 +166,3 @@ struct y {
 };
 int y::z;
 }
-
-// CHECK:      !DIGlobalVariableExpression(var: ![[CONST_A_VAR:[0-9]+]], expr: !DIExpression(DW_OP_constu, 1, DW_OP_stack_value))
-// CHECK:      ![[CONST_A_VAR]] = distinct !DIGlobalVariable(name: "const_a"
-// CHECK-SAME:                    isLocal: true, isDefinition: true, declaration: ![[CONST_A_DECL]])
-
-// CPP11:      !DIGlobalVariableExpression(var: ![[CONST_B_VAR:[0-9]+]], expr: !DIExpression(DW_OP_constu, {{.*}}, DW_OP_stack_value))
-// CPP11:      ![[CONST_B_VAR]] = distinct !DIGlobalVariable(name: "const_b"
-// CPP11-SAME:                    isLocal: true, isDefinition: true, declaration: ![[CONST_B_DECL]])
-
-// CHECK:      !DIGlobalVariableExpression(var: ![[CONST_C_VAR:[0-9]+]], expr: !DIExpression(DW_OP_constu, 18, DW_OP_stack_value))
-// CHECK:      ![[CONST_C_VAR]] = distinct !DIGlobalVariable(name: "const_c"
-// CHECK-SAME:                    isLocal: true, isDefinition: true, declaration: ![[CONST_C_DECL]])
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
index c3b22f889c2f4..e3c64640c7917 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
@@ -142,54 +142,6 @@ static bool ShouldIgnoreArtificialField(llvm::StringRef FieldName) {
          || FieldName.starts_with("_vptr.");
 }
 
-std::optional<DWARFFormValue>
-DWARFASTParserClang::FindConstantOnVariableDefinition(DWARFDIE die) {
-  assert(die.Tag() == DW_TAG_member || die.Tag() == DW_TAG_variable);
-
-  auto *dwarf = die.GetDWARF();
-  if (!dwarf)
-    return {};
-
-  ConstString name{die.GetName()};
-  if (!name)
-    return {};
-
-  auto *CU = die.GetCU();
-  if (!CU)
-    return {};
-
-  DWARFASTParser *dwarf_ast = dwarf->GetDWARFParser(*CU);
-  auto parent_decl_ctx = dwarf_ast->GetDeclContextContainingUIDFromDWARF(die);
-
-  // Make sure we populate the GetDieToVariable cache.
-  VariableList variables;
-  dwarf->FindGlobalVariables(name, parent_decl_ctx, UINT_MAX, variables);
-
-  // The cache contains the variable definition whose DW_AT_specification
-  // points to our declaration DIE. Look up that definition using our
-  // declaration.
-  auto const &die_to_var = dwarf->GetDIEToVariable();
-  auto it = die_to_var.find(die.GetDIE());
-  if (it == die_to_var.end())
-    return {};
-
-  auto var_sp = it->getSecond();
-  assert(var_sp != nullptr);
-
-  if (!var_sp->GetLocationIsConstantValueData())
-    return {};
-
-  auto def = dwarf->GetDIE(var_sp->GetID());
-  auto def_attrs = def.GetAttributes();
-  DWARFFormValue form_value;
-  if (!def_attrs.ExtractFormValueAtIndex(
-          def_attrs.FindAttributeIndex(llvm::dwarf::DW_AT_const_value),
-          form_value))
-    return {};
-
-  return form_value;
-}
-
 TypeSP DWARFASTParserClang::ParseTypeFromClangModule(const SymbolContext &sc,
                                                      const DWARFDIE &die,
                                                      Log *log) {
@@ -2916,23 +2868,11 @@ void DWARFASTParserClang::CreateStaticMemberVariable(
 
   bool unused;
   // TODO: Support float/double static members as well.
-  if (!ct.IsIntegerOrEnumerationType(unused))
+  if (!ct.IsIntegerOrEnumerationType(unused) || !attrs.const_value_form)
     return;
 
-  auto maybe_const_form_value = attrs.const_value_form;
-
-  // Newer versions of Clang don't emit the DW_AT_const_value
-  // on the declaration of an inline static data member. Instead
-  // it's attached to the definition DIE. If that's the case,
-  // try and fetch it.
-  if (!maybe_const_form_value) {
-    maybe_const_form_value = FindConstantOnVariableDefinition(die);
-    if (!maybe_const_form_value)
-      return;
-  }
-
   llvm::Expected<llvm::APInt> const_value_or_err =
-      ExtractIntFromFormValue(ct, *maybe_const_form_value);
+      ExtractIntFromFormValue(ct, *attrs.const_value_form);
   if (!const_value_or_err) {
     LLDB_LOG_ERROR(log, const_value_or_err.takeError(),
                    "Failed to add const value to variable {1}: {0}",
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h
index 7b495419cf324..3e28e54d6220d 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h
@@ -413,17 +413,6 @@ class DWARFASTParserClang : public lldb_private::plugin::dwarf::DWARFASTParser {
                        lldb_private::CompilerType &class_clang_type,
                        const lldb::AccessType default_accesibility,
                        lldb_private::ClangASTImporter::LayoutInfo &layout_info);
-
-  /// Tries to find the definition DW_TAG_variable DIE of the the specified
-  /// DW_TAG_member 'die'. If such definition exists, returns the
-  /// DW_AT_const_value of that definition if available. Returns std::nullopt
-  /// otherwise.
-  ///
-  /// In newer versions of clang, DW_AT_const_value attributes are not attached
-  /// to the declaration of a inline static data-member anymore, but rather on
-  /// its definition. This function is used to locate said constant.
-  std::optional<lldb_private::plugin::dwarf::DWARFFormValue>
-  FindConstantOnVariableDefinition(lldb_private::plugin::dwarf::DWARFDIE die);
 };
 
 /// Parsed form of all attributes that are relevant for type reconstruction.

Copy link
Contributor

@cor3ntin cor3ntin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you need me to merge that for you?

@cor3ntin
Copy link
Contributor

cor3ntin commented Dec 6, 2023

See #74070

@Michael137
Copy link
Member Author

Do you need me to merge that for you?

Was going to wait until some more people had the chance to look. But if urgent, I don't think there should be any trouble merging this sooner

@dwblaikie
Copy link
Collaborator

To support access to such constants from LLDB we'll most likely have to have to make LLDB find the constants by looking at the containing class first.

Tangentially related to: https://discourse.llvm.org/t/rfc-improve-dwarf-5-debug-names-type-lookup-parsing-speed/74151/31?u=dblaikie

Clang/LLVM do know the difference between type-invariant members and type variant ones (type invariant members are in the members list of the DICompositeType and variant members have a scope that refers to the type but don't appear in the members list) - would it be reasonable to not include the invariant members in the accelerator table, but only include the variant ones? Invariant ones can be found by finding any instance of the type in the index, then looking at its members - and for variant members it'd be useful to have them in the index. (though, honestly, I'm not sure how lldb and gdb handle that situation - last time I tested it with gdb, it just saw two different copies of the type and didn't try to unify/aggregate all the variant members... but lldb only creates one copy of the type, so don't know what it does if you've got, say, two member function template instantiations for different template parameters in two different translation units/compile units)

@mstorsjo
Copy link
Member

mstorsjo commented Dec 6, 2023

Could we please land this now?

@Michael137
Copy link
Member Author

To support access to such constants from LLDB we'll most likely have to have to make LLDB find the constants by looking at the containing class first.

Tangentially related to: https://discourse.llvm.org/t/rfc-improve-dwarf-5-debug-names-type-lookup-parsing-speed/74151/31?u=dblaikie

Clang/LLVM do know the difference between type-invariant members and type variant ones (type invariant members are in the members list of the DICompositeType and variant members have a scope that refers to the type but don't appear in the members list) - would it be reasonable to not include the invariant members in the accelerator table, but only include the variant ones? Invariant ones can be found by finding any instance of the type in the index, then looking at its members - and for variant members it'd be useful to have them in the index. (though, honestly, I'm not sure how lldb and gdb handle that situation - last time I tested it with gdb, it just saw two different copies of the type and didn't try to unify/aggregate all the variant members... but lldb only creates one copy of the type, so don't know what it does if you've got, say, two member function template instantiations for different template parameters in two different translation units/compile units)

I'll merge the PR for now to unblock and address this later

@Michael137 Michael137 merged commit 4db54e6 into llvm:main Dec 6, 2023
4 checks passed
felipepiovezan added a commit that referenced this pull request Dec 6, 2023
These tests started passing after this PR landed:
#74580
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:codegen clang Clang issues not falling into any other category debuginfo lldb
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants