[Debug-Info] DW_AT_export_symbols shouldn't be generated before version-5 of DWARF.

[Esme]

Summary

DW_AT_export_symbols is an attribute introduced in DWARF-5, which should not be generated under previous DWARF versions. And this unexpected behavior caused the DBX to fail.

[dblaikie]

@aprantl @probinson @jhenderson - any of you folks want this in older versions? I think this is for C++ inline namespaces - so if you used a modern C++ standard library (that uses inline namespaces like "std::__1::basic_string", etc) users would have to use these extra qualifiers when naming types, functions, etc. Not unworkable, but a bit inconvenient.

[shchenz]

Can we guard the fix only for DBX? So that other consumers will not be impacted?

[Esme]

Addressed Zheng's comments.

[jhenderson]

In D100440#2688023, @dblaikie wrote:

@aprantl @probinson @jhenderson - any of you folks want this in older versions? I think this is for C++ inline namespaces - so if you used a modern C++ standard library (that uses inline namespaces like "std::__1::basic_string", etc) users would have to use these extra qualifiers when naming types, functions, etc. Not unworkable, but a bit inconvenient.

I'm not the right person to ask, but I wouldn't expect a DWARF v4 consumer to be able to recognise a DWARF v5 attribute in general, and therefore we shouldn't be using emitting it in older cases. @probinson or maybe @jmorse will probably have a better insight here.

[teemperor]

In D100440#2688023, @dblaikie wrote:

@aprantl @probinson @jhenderson - any of you folks want this in older versions? I think this is for C++ inline namespaces - so if you used a modern C++ standard library (that uses inline namespaces like "std::__1::basic_string", etc) users would have to use these extra qualifiers when naming types, functions, etc. Not unworkable, but a bit inconvenient.

That's right. LLDB is using this attribute even when parsing DWARF v4 to model inline namespaces (such as libc++'s __1 namespace). If we don't have the attribute then we end up with a bunch of __1 in the type names we show to the user (+ some other minor changes in behaviour). It shouldn't break any LLDB scripts though as the type names used in scripts always specify inline namespaces.

If we decide to remove this for all consumers we can just special-case the namespace parsing in LLDB to infer that std::__1 is an inline namespace. It's the only namespace that is important for users (and the test suite).

[dblaikie]

In D100440#2698501, @jhenderson wrote:

In D100440#2688023, @dblaikie wrote:

@aprantl @probinson @jhenderson - any of you folks want this in older versions? I think this is for C++ inline namespaces - so if you used a modern C++ standard library (that uses inline namespaces like "std::__1::basic_string", etc) users would have to use these extra qualifiers when naming types, functions, etc. Not unworkable, but a bit inconvenient.

I'm not the right person to ask, but I wouldn't expect a DWARF v4 consumer to be able to recognise a DWARF v5 attribute in general, and therefore we shouldn't be using emitting it in older cases. @probinson or maybe @jmorse will probably have a better insight here.

Generally we figure emitting new tags/attributes is OK, because consumers are meant to ignore them if they don't recognize them - and some consumers can recognize them as an extension. (such as lldb)

This conversation's gotten fragmented over a bunch of reviews, unfortunately - but it sounds like the direction it's headed is that maybe -gstrict-dwarf will be wired up to do all this pedantry/not emit things from future standards, and DBX tuning will opt in to -gstrict-dwarf mode.

[dblaikie]

In D100440#2698675, @teemperor wrote:

In D100440#2688023, @dblaikie wrote:

@aprantl @probinson @jhenderson - any of you folks want this in older versions? I think this is for C++ inline namespaces - so if you used a modern C++ standard library (that uses inline namespaces like "std::__1::basic_string", etc) users would have to use these extra qualifiers when naming types, functions, etc. Not unworkable, but a bit inconvenient.

That's right. LLDB is using this attribute even when parsing DWARF v4 to model inline namespaces (such as libc++'s __1 namespace). If we don't have the attribute then we end up with a bunch of __1 in the type names we show to the user (+ some other minor changes in behaviour). It shouldn't break any LLDB scripts though as the type names used in scripts always specify inline namespaces.

If we decide to remove this for all consumers we can just special-case the namespace parsing in LLDB to infer that std::__1 is an inline namespace. It's the only namespace that is important for users (and the test suite).

Thanks for the context. (from other threads, sounds like we might be moving more towards DBX folks wiring up -gstrict-dwarf to implement all this stuff and enabling -gstrict-dwarf when targeting DBX by default - leaving all these future-spec-feature-in-prior-spec-mode on by default for everyone else)

[shchenz]

In D100440#2698987, @dblaikie wrote:

In D100440#2698501, @jhenderson wrote:

In D100440#2688023, @dblaikie wrote:

@aprantl @probinson @jhenderson - any of you folks want this in older versions? I think this is for C++ inline namespaces - so if you used a modern C++ standard library (that uses inline namespaces like "std::__1::basic_string", etc) users would have to use these extra qualifiers when naming types, functions, etc. Not unworkable, but a bit inconvenient.

I'm not the right person to ask, but I wouldn't expect a DWARF v4 consumer to be able to recognise a DWARF v5 attribute in general, and therefore we shouldn't be using emitting it in older cases. @probinson or maybe @jmorse will probably have a better insight here.

Generally we figure emitting new tags/attributes is OK, because consumers are meant to ignore them if they don't recognize them - and some consumers can recognize them as an extension. (such as lldb)

This conversation's gotten fragmented over a bunch of reviews, unfortunately - but it sounds like the direction it's headed is that maybe -gstrict-dwarf will be wired up to do all this pedantry/not emit things from future standards, and DBX tuning will opt in to -gstrict-dwarf mode.

Thanks for the explanation @dblaikie Yes, we just add -gstrict-dwarf support in D100809 and D100826. This option is turned off by default and only enabled for DBX.

I think we also need to change this patch accordingly. @Esme

[Esme]

use strict dwarf flag instead of DBX debugger

[probinson]

There may be an opportunity to do this more robustly, instead of scattering the checks all over the place. Note that the Dwarf.def file already knows the version for each tag and attribute; DwarfDebug already knows the version we're emitting; so if DwarfDebug also had the strict-dwarf flag, it would be easy to add a helper predicate or two that would do the checking for any tag or attribute.

Then the methods that add attributes could generally call the predicate themselves, instead of having higher-level code do it.
For tags you probably do want the higher-level code to call the predicate, but it seems like it would be a lot simpler to do something like if (canEmitTag(dwarf::DW_TAG_rvalue_reference)) than the kind of thing we're seeing in these patches.

Does that seem reasonable?

[dblaikie]

In D100440#2702203, @probinson wrote:

There may be an opportunity to do this more robustly, instead of scattering the checks all over the place. Note that the Dwarf.def file already knows the version for each tag and attribute; DwarfDebug already knows the version we're emitting; so if DwarfDebug also had the strict-dwarf flag, it would be easy to add a helper predicate or two that would do the checking for any tag or attribute.

Then the methods that add attributes could generally call the predicate themselves, instead of having higher-level code do it.
For tags you probably do want the higher-level code to call the predicate, but it seems like it would be a lot simpler to do something like if (canEmitTag(dwarf::DW_TAG_rvalue_reference)) than the kind of thing we're seeing in these patches.

Does that seem reasonable?

Agreed - brought up something similar here: https://reviews.llvm.org/D100826#2702135

(For the tags, yeah, I'm not sure - wonder if the code for emitting the DIEs could return null/empty DIE/something to indicate that the DIE wasn't created - though that might be hard to retrofit if it means revisiting every place that constructs a DIE to ensure it can handle a "nothing" return)

llvm/lib/CodeGen/AsmPrinter/DwarfUnit.cpp

for (const auto *Element : Elements) {
           constructTypeDIE(VariantPart, Composite);
         }
       }
     }
 
     if (CTy->isAppleBlockExtension())
       addFlag(Buffer, dwarf::DW_AT_APPLE_block);
 
-    if (CTy->getExportSymbols())
+    if (CTy->getExportSymbols() && DD->getDwarfVersion() >= 5)
       addFlag(Buffer, dwarf::DW_AT_export_symbols);
 
     // This is outside the DWARF spec, but GDB expects a DW_AT_containing_type
     // inside C++ composite types to point to the base class with the vtable.
     // Rust uses DW_AT_containing_type to link a vtable to the type
     // for which it was created.
     if (auto *ContainingType = CTy->getVTableHolder())
       addDIEEntry(Buffer, dwarf::DW_AT_containing_type,
DIE *DwarfUnit::getOrCreateNameSpace(const DINamespace *NS) {
 
   StringRef Name = NS->getName();
   if (!Name.empty())
     addString(NDie, dwarf::DW_AT_name, NS->getName());
   else
     Name = "(anonymous namespace)";
   DD->addAccelNamespace(*CUNode, Name, NDie);
   addGlobalName(Name, NDie, NS->getScope());
-  if (NS->getExportSymbols())
+  if (NS->getExportSymbols() && DD->getDwarfVersion() >= 5)
     addFlag(NDie, dwarf::DW_AT_export_symbols);
   return &NDie;
 }
 
 DIE *DwarfUnit::getOrCreateModule(const DIModule *M) {
   // Construct the context before querying for the existence of the DIE in case
   // such construction creates the DIE.
   DIE *ContextDIE = getOrCreateContextDIE(M->getScope());

llvm/test/DebugInfo/Generic/export-symbol-anonymous-class.ll

 !5 = !{!0}
 !6 = distinct !DICompositeType(tag: DW_TAG_structure_type, name: "A", file: !3, line: 1, size: 32, flags: DIFlagTypePassByValue, elements: !7, identifier: "_ZTS1A")
 !7 = !{!8}
 !8 = !DIDerivedType(tag: DW_TAG_member, scope: !6, file: !3, line: 2, baseType: !9, size: 32)
 !9 = distinct !DICompositeType(tag: DW_TAG_structure_type, scope: !6, file: !3, line: 2, size: 32, flags: DIFlagExportSymbols | DIFlagTypePassByValue, elements: !10, identifier: "_ZTSN1AUt_E")
 !10 = !{!11}
 !11 = !DIDerivedType(tag: DW_TAG_member, name: "y", scope: !9, file: !3, line: 3, baseType: !12, size: 32)
 !12 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
-!14 = !{i32 2, !"Dwarf Version", i32 4}
+!14 = !{i32 2, !"Dwarf Version", i32 5}
 !15 = !{i32 2, !"Debug Info Version", i32 3}
 !16 = !{!"clang version 10.0.0"}