Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

@llvm/pr-subscribers-lldb

@llvm/pr-subscribers-clang

Author: None (tgs-sc)

[lldb][DWARFASTParserClang] Added a check for the specialization existence

While debugging an application with incorrect dwarf information, where
DW_TAG_template_value_parameter was lost, I found that lldb does not
check that the corresponding specialization exists. As a result, at the
stage when ASTImporter works, the type is completed in such a way that
it inherits from itself. And during the calculation of layout, an
infinite recursion occurs. To catch this error, I added a corresponding check
at the stage of restoring the type from dwarf information. I also added a
trivial assert in clang to check that the class does not inherit from itself.

diff --git a/clang/lib/AST/RecordLayoutBuilder.cpp b/clang/lib/AST/RecordLayoutBuilder.cpp
index 6d819031cbef4..93571543f1c7d 100644
--- a/clang/lib/AST/RecordLayoutBuilder.cpp
+++ b/clang/lib/AST/RecordLayoutBuilder.cpp
@@ -187,6 +187,8 @@ void EmptySubobjectMap::ComputeEmptySubobjectSizes() {
   // Check the bases.
   for (const CXXBaseSpecifier &Base : Class->bases()) {
     const CXXRecordDecl *BaseDecl = Base.getType()->getAsCXXRecordDecl();
+    // Assert to prevent infinite recursion.
+    assert(BaseDecl != Class && "Class cannot inherit from itself.");
 
     CharUnits EmptySize;
     const ASTRecordLayout &Layout = Context.getASTRecordLayout(BaseDecl);
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
index c76d67b47b336..6643751cd237a 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
@@ -1873,6 +1873,22 @@ DWARFASTParserClang::ParseStructureLikeDIE(const SymbolContext &sc,
     clang_type =
         m_ast.CreateClassTemplateSpecializationType(class_specialization_decl);
 
+    // Try to find an existing specialization with these template arguments and
+    // template parameter list.
+    void *InsertPos = nullptr;
+    if (!class_template_decl->findSpecialization(template_param_infos.GetArgs(),
+                                                 InsertPos))
+      // Add this specialization to the class template.
+      class_template_decl->AddSpecialization(class_specialization_decl,
+                                             InsertPos);
+    else {
+      dwarf->GetObjectFile()->GetModule()->ReportError(
+          "SymbolFileDWARF({0:p}) - Specialization for "
+          "clang::ClassTemplateDecl({1:p}) already exists.",
+          static_cast<void *>(this), static_cast<void *>(class_template_decl));
+      return TypeSP();
+    }
+
     m_ast.SetMetadata(class_template_decl, metadata);
     m_ast.SetMetadata(class_specialization_decl, metadata);
   }

Hmm yea guarding against incorrect DWARF has been mostly best effort. Can you provide a test-case for this?

Also, please split out the clang change into a separate PR. The clang maintainers can comment there on whether that's a useful thing to have in place

Hmm yea guarding against incorrect DWARF has been mostly best effort. Can you provide a test-case for this?

Sure. Originally this bug was discovered by debugging llc, but I minimized it to such example:

--- b1.cc ---

#include <optional>
#include <iostream>

struct Type {
  Type() {
    printf("ctor\n");
  }

  Type(const Type& T) {
    printf("ctor copy\n");
  }

  Type(Type&& ) {
    printf("ctor move\n");
  }

  ~Type() {
    printf("dtor");
  }
};

std::optional<Type> var{};
std::optional<Type>* x = &var;

--- b2.cc ---

#include <iostream>

class Type;
namespace std{
template<typename T>
class optional;
}
extern std::optional<Type>* x;

int main() {
        std::cout << x << std::endl;
}

Compilation: /usr/bin/g++ b1.cc b2.cc -g -O0 -std=c++17 -o b.out. The version of g++ should be 9.4.0.
Running: lldb b.out -o "b main" -o "r" -o "p x"
The problem is going to happen in internals of std::optional (https://gcc.gnu.org/onlinedocs/gcc-12.3.0/libstdc++/api/a00152_source.html) as it has 2 specializations of _Optional_payload that have the same type and differ only with template parameters.

Also, please split out the clang change into a separate PR. The clang maintainers can comment there on whether that's a useful thing to have in place

Addressed. I made a PR (#154134) to clang with adding assert as seeing backtrace with 16k functions is not very nice.

@Michael137, is it OK?

@JDevlieghere, can you also please look at this?

This is a reproduction in godbolt: https://godbolt.org/z/aKWETszEY.

This is a reproduction in godbolt: https://godbolt.org/z/r96v6cKEs.

Thanks, we will somehow need to turn this into a test. I'll have a more detailed look later in the week