Improve anonymous class heuristic in ClangASTContext::CreateRecordType

[shafik]

Summary

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.

[aprantl]

packages/Python/lldbsuite/test/expression_command/completion-crash-lambda/TestCompletionCrashInLambda.py

4 ↗

(On Diff #214933)

Can you rename that test to not contain the word "crash" and instead describe the action being performed?
(CompletionOfLambda or something like that should be fine)

source/Symbol/ClangASTContext.cpp

1505 ↗

(On Diff #214933)

rename this to has_name

1509 ↗

(On Diff #214933)

has_name ? &ast->Idents.get(name) : nullptr

1513 ↗

(On Diff #214933)

if (!has_name) {

// In C++ a lambda is also represented as an unnamed class. This is different from 
// an *anonymous class* that the user wrote. Anonymous ...
...

[aprantl]

source/Symbol/ClangASTContext.cpp

1521 ↗

(On Diff #214933)

// FIXME: An unnamed class within a class is also wrongly recognized as an anonymous struct.

1524 ↗

(On Diff #214933)

and move the excuse in the commit message.

It would be really helpful though to include tiny source code examples here to iiustrate the difference between unnames struct and anonymous structs vs. unnamed structs in structs.

[teemperor]

Could you expand the test (or add another test) for completing in an anonymous classes? Otherwise this LGTM beside Adrian comments assuming this fixes the crash.

packages/Python/lldbsuite/test/expression_command/completion-crash-lambda/TestCompletionCrashInLambda.py

4 ↗

(On Diff #214933)

You an actually completely remove the decorators import and the empty [].

[shafik]

Addressing comments:

• Rewording comments
• Moving test location
• Adding test case

[shafik]

@aprantl @teemperor Good catches, I have updated the review.

[aprantl]

source/Symbol/ClangASTContext.cpp

1505 ↗

(On Diff #215228)

The condition needs to be reversed now to match the new name.

1516 ↗

(On Diff #215228)

// anonymous class (GNU/MSVC extension)

1509 ↗

(On Diff #214933)

and then here, too.

[aprantl]

source/Symbol/ClangASTContext.cpp

1505 ↗

(On Diff #215228)

bool has_name = (name && name[0])

1505 ↗

(On Diff #215228)

Even more elegant would be changing the function to take a StringRef, but you don't have to do that....

1511 ↗

(On Diff #215228)

if (!has_name)

[shafik]

Fixing has_name to better reflect the condition.

lldb/trunk/packages/Python/lldbsuite/test/expression_command/completion-crash-lambda/TestCompletionCrashInLambda.py

-from lldbsuite.test import lldbinline
-from lldbsuite.test import decorators
-
-lldbinline.MakeInlineTest(__file__, globals(), [decorators.skipIf(bugnumber="rdar://53755023")])

lldb/trunk/packages/Python/lldbsuite/test/expression_command/completion-crash-lambda/main.cpp

-int main() {
-  []()
-  { //%self.dbg.GetCommandInterpreter().HandleCompletion("e ", len("e "), 0, -1, lldb.SBStringList())
-  }
-  ();
-}

lldb/trunk/packages/Python/lldbsuite/test/expression_command/completion-in-lambda-and-unnnamed-class/TestCompletionInLambdaAndUnnamedClass.py

+from lldbsuite.test import lldbinline
+from lldbsuite.test import decorators
+
+lldbinline.MakeInlineTest(__file__, globals(),)

lldb/trunk/packages/Python/lldbsuite/test/expression_command/completion-in-lambda-and-unnnamed-class/main.cpp

+int main() {
+  []()
+  { //%self.dbg.GetCommandInterpreter().HandleCompletion("e ", len("e "), 0, -1, lldb.SBStringList())
+  }
+  ();
+  struct {
+      void f()
+      { //%self.dbg.GetCommandInterpreter().HandleCompletion("e ", len("e "), 0, -1, lldb.SBStringList())
+      }
+  } A;
+}

lldb/trunk/source/Symbol/ClangASTContext.cpp

CompilerType ClangASTContext::CreateRecordType(DeclContext *decl_ctx,
   }
 
   // NOTE: Eventually CXXRecordDecl will be merged back into RecordDecl and
   // we will need to update this code. I was told to currently always use the
   // CXXRecordDecl class since we often don't know from debug information if
   // something is struct or a class, so we default to always use the more
   // complete definition just in case.
 
-  bool is_anonymous = (!name) || (!name[0]);
+  bool has_name = name && name[0];
 
   CXXRecordDecl *decl = CXXRecordDecl::Create(
       *ast, (TagDecl::TagKind)kind, decl_ctx, SourceLocation(),
-      SourceLocation(), is_anonymous ? nullptr : &ast->Idents.get(name));
+      SourceLocation(), has_name ?  &ast->Idents.get(name) : nullptr);
 
-  if (is_anonymous)
+  if (!has_name) {
+    // In C++ a lambda is also represented as an unnamed class. This is
+    // different from an *anonymous class* that the user wrote:
+    //
+    // struct A {
+    //  // anonymous class (GNU/MSVC extension)
+    //  struct {
+    //    int x;
+    //  };
+    //  // unnamed class within a class
+    //  struct {
+    //    int y;
+    //  } B;
+    // };
+    //
+    // void f() {
+    //    // unammed class outside of a class
+    //    struct {
+    //      int z;
+    //    } C;
+    // }
+    //
+    // 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.
+    //
+    // FIXME: An unnamed class within a class is also wrongly recognized as an
+    // anonymous struct.
+    if (CXXRecordDecl *record = dyn_cast<CXXRecordDecl>(decl_ctx)) {
       decl->setAnonymousStructOrUnion(true);
+    }
+  }
 
   if (decl) {
     if (metadata)
       SetMetadata(ast, decl, *metadata);
 
     if (access_type != eAccessNone)
       decl->setAccess(ConvertAccessTypeToAccessSpecifier(access_type));