[Lldb-commits] [PATCH] D66175: Improve anonymous class heuristic in ClangASTContext::CreateRecordType

[Shafik Yaghmour via Phabricator via lldb-commits]

shafik created this revision.
shafik added reviewers: aprantl, teemperor.

Currently the heuristic used in `ClangASTContext::CreateRecordType` to identify an anonymous class is that there is that `name` is a `nullptr` or simply a null terminator. This heuristic is not accurate since it will also sweep up unnamed classes and lambdas. The improved heuristic relies on the requirement that an anonymous class must be contained within a class.

The motivator for this fix is that we currently see crashes during code completion inside lambdas and member functions of unnamed classes not within a class. This heuristic fix removes these cases but leaves unnamed classes within a class incorrectly labeled anonymous.

We will fix this problem long-term by implementing `DW_AT_export_symbols` which will allow us to accurately identify anonymous classes without a heuristic.


https://reviews.llvm.org/D66175

Files:
  packages/Python/lldbsuite/test/expression_command/completion-crash-lambda/TestCompletionCrashInLambda.py
  source/Symbol/ClangASTContext.cpp


Index: source/Symbol/ClangASTContext.cpp
===================================================================
--- source/Symbol/ClangASTContext.cpp
+++ source/Symbol/ClangASTContext.cpp
@@ -1508,8 +1508,25 @@
       *ast, (TagDecl::TagKind)kind, decl_ctx, SourceLocation(),
       SourceLocation(), is_anonymous ? nullptr : &ast->Idents.get(name));
 
-  if (is_anonymous)
-    decl->setAnonymousStructOrUnion(true);
+  if (is_anonymous) {
+    // The current heuristic for checking if a CXXRecordDecl is anonymous is if
+    // there is no name or if it is just a null terminator.
+    // This is not accurate since currently this can sweep up both unnamed
+    // classes and lambdas as anonymous classes, which they are not.
+    //
+    // Anonymous classes is a GNU/MSVC extension that clang supports. It
+    // requires the anonymous class be embedded within a class. So the new
+    // heuristic verifies this condition.
+    //
+    // This fix will unfortunately still mislabel unnamed classes within a class
+    // but this improves the situation greatly since getting this wrong in the
+    // other cases can lead to an assert in clang CodeCompletion since
+    // SemaAccess assumes the DeclContext of an anonymous class is a
+    // CXXRecordDecl.
+    if (CXXRecordDecl *record = dyn_cast<CXXRecordDecl>(decl_ctx)) {
+      decl->setAnonymousStructOrUnion(true);
+    }
+  }
 
   if (decl) {
     if (metadata)
Index: packages/Python/lldbsuite/test/expression_command/completion-crash-lambda/TestCompletionCrashInLambda.py
===================================================================
--- packages/Python/lldbsuite/test/expression_command/completion-crash-lambda/TestCompletionCrashInLambda.py
+++ packages/Python/lldbsuite/test/expression_command/completion-crash-lambda/TestCompletionCrashInLambda.py
@@ -1,4 +1,4 @@
 from lldbsuite.test import lldbinline
 from lldbsuite.test import decorators
 
-lldbinline.MakeInlineTest(__file__, globals(), [decorators.skipIf(bugnumber="rdar://53755023")])
+lldbinline.MakeInlineTest(__file__, globals(), [])


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D66175.214933.patch
Type: text/x-patch
Size: 2070 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20190813/dc7f01b0/attachment.bin>