Add support for DW_AT_export_symbols for anonymous structs

[shafik]

Summary

We add support for DW_AT_export_symbols to detect anonymous struct on top of the heuristics implemented in D66175

This should allow us to differentiate anonymous structs and unnamed structs.

We also fix TestTypeList.py which was incorrectly detecting an unnamed struct as an anonymous struct.

[clayborg]

Have many compilers supported DW_AT_export_symbols for a while now? If not, are there any serious issues introduced here that would change debugger behavior if this attribute is not emitted by a compiler? Or is this a new fix in clang that was recently introduced in order to fix an issue when debugging in lldb?

[shafik]

In D68961#1709708, @clayborg wrote:

Have many compilers supported DW_AT_export_symbols for a while now? If not, are there any serious issues introduced here that would change debugger behavior if this attribute is not emitted by a compiler? Or is this a new fix in clang that was recently introduced in order to fix an issue when debugging in lldb?

We don't except any regressions for code compiled with older compilers. We are fixing the case that unnamed classes are identified as anonymous. The anonymous classes cases should be caught in older revisions in ClangASTContext::AddFieldToRecordType which does a Rec->setAnonymousStructOrUnion(true) for those cases.

This was originally motivated by D66175 which only partially fixed the cases of classes being incorrectly labeled as anonymous.

[labath]

In D68961#1711407, @shafik wrote:

In D68961#1709708, @clayborg wrote:

Have many compilers supported DW_AT_export_symbols for a while now? If not, are there any serious issues introduced here that would change debugger behavior if this attribute is not emitted by a compiler? Or is this a new fix in clang that was recently introduced in order to fix an issue when debugging in lldb?

We don't except any regressions for code compiled with older compilers. We are fixing the case that unnamed classes are identified as anonymous. The anonymous classes cases should be caught in older revisions in ClangASTContext::AddFieldToRecordType which does a Rec->setAnonymousStructOrUnion(true) for those cases.

Would it make sense to write a test in asm then? That way it would be obvious exactly what kind of debug info is being tested, and you could ensure both old&new compiler behavior is covered.

[shafik]

In D68961#1711537, @labath wrote:

In D68961#1711407, @shafik wrote:

In D68961#1709708, @clayborg wrote:

Have many compilers supported DW_AT_export_symbols for a while now? If not, are there any serious issues introduced here that would change debugger behavior if this attribute is not emitted by a compiler? Or is this a new fix in clang that was recently introduced in order to fix an issue when debugging in lldb?

We don't except any regressions for code compiled with older compilers. We are fixing the case that unnamed classes are identified as anonymous. The anonymous classes cases should be caught in older revisions in ClangASTContext::AddFieldToRecordType which does a Rec->setAnonymousStructOrUnion(true) for those cases.

Would it make sense to write a test in asm then? That way it would be obvious exactly what kind of debug info is being tested, and you could ensure both old&new compiler behavior is covered.

The TestTypeList.py covers the relevant cases of both anonymous class and an unnamed class. I ran check-lldb with the feature for emitting DW_AT_export_symbols disabled and did not see any regressions in this test or others except `

We also have the lldb-cmake-matrix bot which runs the test suite using older versions of clang which is there for exactly this purpose to catch regressions due to features not supported by older compiler versions. Which would catch any regressions here.

[shafik]

Minor fixes.

[labath]

In D68961#1713566, @shafik wrote:

We also have the lldb-cmake-matrix bot which runs the test suite using older versions of clang which is there for exactly this purpose to catch regressions due to features not supported by older compiler versions. Which would catch any regressions here.

I'm afraid I have to disagree with that. Like I said the last time <D66370> when this topic came up, I don't think that this "matrix" testing is the way to test dwarf features. The "matrix" bot is very useful to catch regressions in parsing old debug info (because there will always be some quirk that you forget to think of), but I don't think there should be any piece of code that is _only_ tested this way, particularly when it is very easy to do so (like it is here).

As you're probably aware, there's an "end-to-end testing" discussion going on on the mailing lists right now. One of the main questions being debated is whether it is ok to have a test which checks the assembly generated from some c(++) source code. And there's a lot of pushback saying that this is testing too much. However, even such a test is peanuts compared to clang-ast-from-dwarf-unamed-and-anon-structs.cpp. Even though it's one of the simplest lldb test we have, it runs the compiler front end, back end *and* assembler, and then reads the output hoping that the pipeline has generated the thing it wants to test. (And btw, it will fail on windows.) It doesn't even check that the compiler has generated the thing it wants to check, so if the compiler output later changes to not include DW_AT_export_symbols, it will start to test something completely different, leaving the code added here uncovered, unless someone remembers to add a new row to the matrix bot.

Not that this is very likely to happen in this particular case, but these kinds of changes happen all the time. Just last week, llvm started generating a very different style of location lists. Both lldb <rL374769> and dsymutil <D69005> had to be fixed to handle that. Both patches had tests, which were not based on running clang to generate the debug info (the dsymutil test had a checked in binary, which I am not a fan of, but that's a different story). Even so, it's very likely that this has regressed our coverage of the previous forms of location lists because we had no tests (except some basic ones I've added two or three months ago) which tested this code directly. And we only know about this because the initial implementation was wrong/incomplete, so we noticed the breakage.

So, overall, I still think we should have an even more targeted test here. Maybe it's not fair to ask you to add this kind of a test for the "old" style structs because that is past and not really "on you". However, I think we should be trying to add these kinds of tests for new features whenever is possible (another thing -- I imagine debugging failures from the matrix bot can be hard, though I've fortunately had not had to do that yet). I've been trying to do that for the past year, and while it takes a while to get used to, after some time I've gotten fairly efficient at generating/reducing "raw" dwarf. Besides being able to generate "stable" tests, this also enables one to generate tests for "incorrect" dwarf, which is one of the things which is impossible with the "clang -g" approach.

[shafik]

This comment was removed by shafik.

[shafik]

In D68961#1714241, @labath wrote:

In D68961#1713566, @shafik wrote:

We also have the lldb-cmake-matrix bot which runs the test suite using older versions of clang which is there for exactly this purpose to catch regressions due to features not supported by older compiler versions. Which would catch any regressions here.

I'm afraid I have to disagree with that. Like I said the last time <D66370> when this topic came up, I don't think that this "matrix" testing is the way to test dwarf features. The "matrix" bot is very useful to catch regressions in parsing old debug info (because there will always be some quirk that you forget to think of), but I don't think there should be any piece of code that is _only_ tested this way, particularly when it is very easy to do so (like it is here).

As you're probably aware, there's an "end-to-end testing" discussion going on on the mailing lists right now. One of the main questions being debated is whether it is ok to have a test which checks the assembly generated from some c(++) source code. And there's a lot of pushback saying that this is testing too much. However, even such a test is peanuts compared to clang-ast-from-dwarf-unamed-and-anon-structs.cpp. Even though it's one of the simplest lldb test we have, it runs the compiler front end, back end *and* assembler, and then reads the output hoping that the pipeline has generated the thing it wants to test. (And btw, it will fail on windows.) It doesn't even check that the compiler has generated the thing it wants to check, so if the compiler output later changes to not include DW_AT_export_symbols, it will start to test something completely different, leaving the code added here uncovered, unless someone remembers to add a new row to the matrix bot.

Not that this is very likely to happen in this particular case, but these kinds of changes happen all the time. Just last week, llvm started generating a very different style of location lists. Both lldb <rL374769> and dsymutil <D69005> had to be fixed to handle that. Both patches had tests, which were not based on running clang to generate the debug info (the dsymutil test had a checked in binary, which I am not a fan of, but that's a different story). Even so, it's very likely that this has regressed our coverage of the previous forms of location lists because we had no tests (except some basic ones I've added two or three months ago) which tested this code directly. And we only know about this because the initial implementation was wrong/incomplete, so we noticed the breakage.

So, overall, I still think we should have an even more targeted test here. Maybe it's not fair to ask you to add this kind of a test for the "old" style structs because that is past and not really "on you". However, I think we should be trying to add these kinds of tests for new features whenever is possible (another thing -- I imagine debugging failures from the matrix bot can be hard, though I've fortunately had not had to do that yet). I've been trying to do that for the past year, and while it takes a while to get used to, after some time I've gotten fairly efficient at generating/reducing "raw" dwarf. Besides being able to generate "stable" tests, this also enables one to generate tests for "incorrect" dwarf, which is one of the things which is impossible with the "clang -g" approach.

When the I added the feature to the front end tests were added to verify that DW_AT_export_symbols is being generated for anonymous structs in D66605 and D66667 so if this regresses in the front-end we will catch it vis these tests. So as far I can tell we have tests at every point it can regress.

[labath]

In D68961#1717197, @shafik wrote:

When the I added the feature to the front end tests were added to verify that DW_AT_export_symbols is being generated for anonymous structs in D66605 and D66667 so if this regresses in the front-end we will catch it vis these tests. So as far I can tell we have tests at every point it can regress.

But that's a test for clang. It will make sure it clang does stop emitting this attribute accidentally, but it will not help you if the removal is a conscious decision. At that point, the clang test will be deleted/modified too, but I doubt anyone will think of lldb and the fact that some part of lldb codebase becomes untested.

In fact, I think these patches illustrate very well the point I'm trying to make. D66667 does not check that the attribute ends up in the debug info. It only adds a test to ensure that clang emits some llvm IR. It could test the actual dwarf, but it doesn't, because llvm has a strong preference for single-component unit tests. lldb's test suite is an exception in the llvm world in that nearly every lldb test is an end-to-end test.

[shafik]

-Add test to clarify what we depending wrt to debug info

[aprantl]

This comment has been deleted.

test/Shell/SymbolFile/DWARF/anon_class_w_and_wo_export_symbols.ll

1 ↗

(On Diff #226349)

de it from time to time. I think in this case LLVM IR could be superior to x86 assembler since it is much shorter and easier to understand — even if we have to upgrade it from time to time. However, since this is explicitly testing for a DWARF feature, should we check the dwarfdump output that the attribute we are looking for is actually there?

[aprantl]

I think in this case LLVM IR could be superior to x86 assembler since it is much shorter and easier to understand — even if we have to upgrade it from time to time. However, since this is explicitly testing for a DWARF feature, should we check the dwarfdump output that the attribute we are looking for is actually there?

test/Shell/SymbolFile/DWARF/anon_class_w_and_wo_export_symbols.ll

1 ↗

(On Diff #226349)

[I don't know what happened to my comment here].

[shafik]

• Updated test to add dwarfdump test

[labath]

Thanks for bearing with me. :)

test/Shell/SymbolFile/DWARF/anon_class_w_and_wo_export_symbols.ll

21 ↗

(On Diff #226366)

llvm-dwarfdump

[shafik]

Using llvm-dwarfdump instead of dwarfdump

[stella.stamenova]

This is broken on Windows. I moved the Buildbot to staging to resolve some of the issues with the move to the monorepo, so you can see the failures here:

http://lab.llvm.org:8014/builders/lldb-x64-windows-ninja/builds/0/steps/test/logs/stdio

[labath]

lldb/test/Shell/SymbolFile/DWARF/clang-ast-from-dwarf-unamed-and-anon-structs.cpp

1 ↗

(On Diff #226764)

I am seeing this test fail sporadically (mostly depending on the build config). This happens because the test output is nondeterministic (sometimes the "anonymous" structs are printed in different order. I think this is down to SymbolFileDWARF::GetTypes using pointer values (through TypeSet = std::set<Type *>) in its operation...

[aprantl]

@shafik: FYI If it isn't obvious, you can try building -DLLVM_ENABLE_REVERSE_ITERATION 1 to track it down.

include/lldb/Symbol/ClangASTContext.h

public:
   UnifyAccessSpecifiers(clang::AccessSpecifier lhs, clang::AccessSpecifier rhs);
 
   static uint32_t GetNumBaseClasses(const clang::CXXRecordDecl *cxx_record_decl,
                                     bool omit_empty_base_classes);
 
   CompilerType CreateRecordType(clang::DeclContext *decl_ctx,
                                 lldb::AccessType access_type, const char *name,
                                 int kind, lldb::LanguageType language,
-                                ClangASTMetadata *metadata = nullptr);
+                                ClangASTMetadata *metadata = nullptr,
+                                bool exports_symbols = false);
 
   class TemplateParameterInfos {
   public:
     bool IsValid() const {
       if (args.empty())
         return false;
       return args.size() == names.size() &&
         ((bool)pack_name == (bool)packed_args) &&

packages/Python/lldbsuite/test/python_api/type/TestTypeList.py

def test(self):
             self.DebugSBType(type)
             self.assertFalse(type.IsAnonymousType(), "Task is not anonymous")
             for field in type.fields:
                 if field.name == "type":
                     for enum_member in field.type.enum_members:
                         self.assertTrue(enum_member)
                         self.DebugSBType(enum_member.type)
                 elif field.name == "my_type_is_nameless":
-                    self.assertTrue(
+                    self.assertFalse(
                         field.type.IsAnonymousType(),
-                        "my_type_is_nameless has an anonymous type")
+                        "my_type_is_nameless is not an anonymous type")
                 elif field.name == "my_type_is_named":
                     self.assertFalse(
                         field.type.IsAnonymousType(),
                         "my_type_is_named has a named type")
+                elif field.name == None:
+                    self.assertTrue(
+                        field.type.IsAnonymousType(),
+                        "Nameless type is not anonymous")
 
         # Pass an empty string.  LLDB should not crash. :-)
         fuzz_types = target.FindTypes(None)
         fuzz_type = target.FindFirstType(None)
 
         # Now use the SBTarget.FindFirstType() API to find 'Task'.
         task_type = target.FindFirstType('Task')
         self.assertTrue(task_type)

packages/Python/lldbsuite/test/python_api/type/main.cpp

 class Task {
 public:
     int id;
     Task *next;
     enum {
         TASK_TYPE_1,
         TASK_TYPE_2
     } type;
+    // This struct is anonymous b/c it does not have a name
+    // and it is not unnamed class.
+    // Anonymous classes are a GNU extension.
+    struct {
+      int y;
+    };
+    // This struct is an unnamed class see [class.pre]p1
+    // http://eel.is/c++draft/class#pre-1.sentence-6
     struct {
       int x;
     } my_type_is_nameless;
     struct name {
       int x;
     } my_type_is_named;
     Task(int i, Task *n):
         id(i),

source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h

struct ParsedDWARFTypeAttributes {
   bool is_artificial = false;
   bool is_complete_objc_class = false;
   bool is_explicit = false;
   bool is_forward_declaration = false;
   bool is_inline = false;
   bool is_scoped_enum = false;
   bool is_vector = false;
   bool is_virtual = false;
+  bool exports_symbols = false;
   clang::StorageClass storage = clang::SC_None;
   const char *mangled_name = nullptr;
   lldb_private::ConstString name;
   lldb_private::Declaration decl;
   DWARFDIE object_pointer;
   DWARFFormValue abstract_origin;
   DWARFFormValue containing_type;
   DWARFFormValue signature;

source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp

for (size_t i = 0; i < num_attributes; ++i) {
 
     case DW_AT_APPLE_runtime_class:
       class_language = (LanguageType)form_value.Signed();
       break;
 
     case DW_AT_GNU_vector:
       is_vector = form_value.Boolean();
       break;
+    case DW_AT_export_symbols:
+      exports_symbols = form_value.Boolean();
+      break;
     }
   }
 }
 
 static std::string GetUnitName(const DWARFDIE &die) {
   if (DWARFUnit *unit = die.GetCU())
     return unit->GetAbsolutePath().GetPath();
   return "<missing DWARF unit path>";
if (attrs.name.GetStringRef().contains('<')) {
         m_ast.SetMetadata(class_specialization_decl, metadata);
       }
     }
 
     if (!clang_type_was_created) {
       clang_type_was_created = true;
       clang_type = m_ast.CreateRecordType(
           decl_ctx, attrs.accessibility, attrs.name.GetCString(), tag_decl_kind,
-          attrs.class_language, &metadata);
+          attrs.class_language, &metadata, attrs.exports_symbols);
     }
   }
 
   // Store a forward declaration to this class type in case any
   // parameters in any class methods need it for the clang types for
   // function prototypes.
   LinkDeclContextToDIE(m_ast.GetDeclContextForType(clang_type), die);
   type_sp = std::make_shared<Type>(die.GetID(), dwarf, attrs.name,

source/Symbol/ClangASTContext.cpp

CompilerType ClangASTContext::GetTypeForDecl(ObjCInterfaceDecl *decl) {
   if (ast)
     return CompilerType(ClangASTContext::GetASTContext(ast),
                         ast->getObjCInterfaceType(decl).getAsOpaquePtr());
   return CompilerType();
 }
 
 #pragma mark Structure, Unions, Classes
 
-CompilerType ClangASTContext::CreateRecordType(DeclContext *decl_ctx,
-                                               AccessType access_type,
-                                               const char *name, int kind,
-                                               LanguageType language,
-                                               ClangASTMetadata *metadata) {
+CompilerType ClangASTContext::CreateRecordType(
+    DeclContext *decl_ctx, AccessType access_type, const char *name, int kind,
+    LanguageType language, ClangASTMetadata *metadata, bool exports_symbols) {
   ASTContext *ast = getASTContext();
   assert(ast != nullptr);
 
   if (decl_ctx == nullptr)
     decl_ctx = ast->getTranslationUnitDecl();
 
   if (language == eLanguageTypeObjC ||
       language == eLanguageTypeObjC_plus_plus) {
if (!has_name) {
     //    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 (isa<CXXRecordDecl>(decl_ctx))
+    if (isa<CXXRecordDecl>(decl_ctx) && exports_symbols)
       decl->setAnonymousStructOrUnion(true);
   }
 
   if (decl) {
     if (metadata)
       SetMetadata(ast, decl, *metadata);
 
     if (access_type != eAccessNone)
void ClangASTContext::DumpFromSymbolFile(Stream &s,
   lldb_private::TypeList type_list;
   symfile->GetTypes(nullptr, eTypeClassAny, type_list);
   size_t ntypes = type_list.GetSize();
 
   for (size_t i = 0; i < ntypes; ++i) {
     TypeSP type = type_list.GetTypeAtIndex(i);
 
     if (!symbol_name.empty())
-      if (symbol_name.compare(type->GetName().GetStringRef()) != 0)
+      if (symbol_name != type->GetName().GetStringRef())
         continue;
 
     s << type->GetName().AsCString() << "\n";
 
     if (clang::TagDecl *tag_decl =
                  GetAsTagDecl(type->GetFullCompilerType()))
       tag_decl->dump(s.AsRawOstream());
     else if (clang::TypedefNameDecl *typedef_decl =

test/Shell/SymbolFile/DWARF/clang-ast-from-dwarf-unamed-and-anon-structs.cpp

+// Test to verify we are corectly generating anonymous flags when parsing
+// anonymous class and unnamed structs from DWARF to the a clang AST node.
+
+// RUN: %clang++ -g -c -o %t.o %s
+// RUN: lldb-test symbols -dump-clang-ast %t.o | FileCheck %s
+
+struct A {
+  struct {
+    int x;
+  };
+  struct {
+    int y;
+  } C;
+} a;
+
+// CHECK: A::(anonymous struct)
+// CHECK: |-DefinitionData is_anonymous pass_in_registers aggregate standard_layout trivially_copyable pod trivial literal
+// CHECK: A::(anonymous struct)
+// CHECK: |-DefinitionData pass_in_registers aggregate standard_layout trivially_copyable pod trivial literal