[AST] Incorrectly qualified unscoped enumeration as template actual parameter.

[CarlosAlbertoEnciso]

Summary

An unscoped enumeration used as template argument, should not
have any qualified information about its enclosing scope, as
its visibility is global.

In the case of scoped enumerations, they must include information
about their enclosing scope.

For the below test:

enum unscoped { a };
enum class scoped { b };

template<unscoped U> struct tmpl_u { };
template<scoped S> struct tmpl_s { };

tmpl_u<unscoped::a> tmpl_unscoped;
tmpl_s<scoped::b> tmpl_scoped;

gives the following encoded names:

DW_TAG_structure_type "tmpl_u<unscoped::a>"
DW_TAG_structure_type "tmpl_s<scoped::b>"

The incorrectly qualified name causes a debugger to show qualified
names for both scoped and unscoped enumerations.

[probinson]

Have you tried this change against the GDB and LLDB test suites? If they have failures then we should think about whether those tests are over-specific or whether we should put this under a tuning option.

[CarlosAlbertoEnciso]

In D39239#905641, @probinson wrote:

Have you tried this change against the GDB and LLDB test suites? If they have failures then we should think about whether those tests are over-specific or whether we should put this under a tuning option.

LLDB test suite:
There are some specific tests that use enums and templates:

tools/lldb/packages/Python/lldbsuite/test/lang/cpp/template

but they cover only the case of scoped enums and are not affected by this patch. May be the LLDB test suite should be updated to cover scoped and unscoped enums.

I will check the GDB test suite and include the results.

[CarlosAlbertoEnciso]

In D39239#905641, @probinson wrote:

Have you tried this change against the GDB and LLDB test suites? If they have failures then we should think about whether those tests are over-specific or whether we should put this under a tuning option.

I double check the LLDB test suite and there are 3 cases that fail with this patch. But the DWARF generated is correct, as the template name is encoded correctly:

DW_TAG_compile_unit "main.cpp"
  DW_AT_producer "clang version 6.0.0 (trunk 316571)"
  DW_AT_comp_dir "/home/carlos/llvm-root/llvm/tools/lldb/packages/Python/lldbsuite/test/lang/cpp/template"
  ...
  DW_TAG_enumeration_type "EnumType"
    DW_AT_enum_class DW_FORM_flag_present
    DW_TAG_enumerator "Member"
    DW_TAG_enumerator "Subclass"
    ...
  DW_TAG_class_type "EnumTemplate<EnumType::Member>"
    ...
  DW_TAG_class_type "EnumTemplate<EnumType::Subclass>"
    ...

I will investigate the issue, as the test case should pass as it does not use unscope enums, which is what the patch should affect.

[CarlosAlbertoEnciso]

Hi Tamas,

I have added you to this review, as I think I have found something odd with the LLDB.

Basically after this intended patch to the compiler (to emit scoped information only for scoped enums) the following test fail:

tools/lldb/packages/Python/lldbsuite/test/lang/cpp/template/TestTemplateArgs.py

I have saved the objects generated before and after the change for both 32/64bits mode. The test case is checking for scoped information which is present in the debug information.

def test_enum_args(self):
        frame = self.prepareProcess()

        # Make sure "member" can be displayed and also used in an expression
        # correctly
        member = frame.FindVariable('member')
        self.assertTrue(
            member.IsValid(),
            'make sure we find a local variabble named "member"')
        
        self.assertTrue(member.GetType().GetName() ==
                        'EnumTemplate<EnumType::Member>')

After some debugging, 'TestTemplateArgs.py' fails when retrieving the member type name, which is expected to have the 'EnumTemplate<EnumType::Member>'. As per my previous comment, that string is present; but the expression 'member.GetType().GetName()' returns just 'EnumTemplate<Member>' causing the test to fail.

I would appreciate it very much your feedback.

Thanks

[tberghammer]

2 fairly random ideas without looking into it too much:

• Can you do a diff of the debug_info dump before and after your change? Understanding what have changed should give us a pretty good clue about the issue.
• My first guess is that after your change we emit DW_TAG_enumeration_type for scoped enums (what seems to be the correct thing to do) instead of something else (not sure what) and you have to update https://github.com/llvm-mirror/lldb/blob/master/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp#L1512 to handle it correctly.

[CarlosAlbertoEnciso]

Hi Tamas,

Thanks very much for your message.

In D39239#910797, @tberghammer wrote:

• Can you do a diff of the debug_info dump before and after your change? Understanding what have changed should give us a pretty good clue about the issue.

For this specific case, the debug_info is the same before and after my change, as the patch affects only unscoped enums.

DW_TAG_compile_unit "main.cpp"
  DW_AT_producer "clang version 6.0.0 (trunk 316983)"
  DW_AT_comp_dir "/home/carlos/llvm-root/llvm/tools/lldb/packages/Python/lldbsuite/test/lang/cpp/template"
  ...
  DW_TAG_enumeration_type "EnumType"
    DW_AT_enum_class DW_FORM_flag_present
    DW_TAG_enumerator "Member"
    DW_TAG_enumerator "Subclass"
    ...
  DW_TAG_class_type "EnumTemplate<EnumType::Member>"
    ...
  DW_TAG_class_type "EnumTemplate<EnumType::Subclass>"
    ...

The DW_AT_name string for the templates are correctly generated, including the scoped information.

• My first guess is that after your change we emit DW_TAG_enumeration_type for scoped enums (what seems to be the correct thing to do) instead of something else (not sure what) and you have to update https://github.com/llvm-mirror/lldb/blob/master/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp#L1512 to handle it correctly.

Thanks for the link.

Looking at:
https://github.com/llvm-mirror/lldb/blob/master/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp#L1512

const char *DWARFDebugInfoEntry::GetQualifiedName(
    SymbolFileDWARF *dwarf2Data, DWARFCompileUnit *cu,
    const DWARFAttributes &attributes, std::string &storage) const {

  const char *name = GetName(dwarf2Data, cu);
  ...
  return storage.c_str();
}

The values for 'name' and 'storage' are correct and include the full qualified name: 'EnumTemplate<EnumType::Member>'.

https://github.com/llvm-mirror/lldb/blob/master/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp#L926

During the processing of DW_TAG_enumeration_type, it seems that 'DW_AT_enum_class' is not processed.

DW_TAG_enumeration_type "EnumType"
  ...
  DW_AT_enum_class DW_FORM_flag_present  
  ...
  DW_TAG_enumerator "Member"
  DW_TAG_enumerator "Subclass"

which at the end can have an impact on the name generated for the enumerators to be:

"EnumTemplate<EnumType::Member>"

or

"EnumTemplate<Member>"

Thanks.

[probinson]

Have you tried the GDB suite yet? If it has no problems, and we can make LLDB happy, I'm okay with it.

[tberghammer]

In D39239#911696, @CarlosAlbertoEnciso wrote:

Hi Tamas,

Thanks very much for your message.

In D39239#910797, @tberghammer wrote:

• Can you do a diff of the debug_info dump before and after your change? Understanding what have changed should give us a pretty good clue about the issue.

For this specific case, the debug_info is the same before and after my change, as the patch affects only unscoped enums.

If this patch doesn't effect the debug info generated for this specific case then my guess is that when LLDB builds the EnumDecl for the scoped enumeration it will incorrectly build one for an unscoped enum instead. Looking at the code at https://github.com/llvm-mirror/lldb/blob/master/source/Symbol/ClangASTContext.cpp#L2170 we even have a TODO in the LLDB to fix handling for this case. Can you try out your patch with https://reviews.llvm.org/D39545 what supposed to fix the problem on the LLDB side (haven't tested it at all)?

[tberghammer]

I verified that D39545 will be fixing the problem on the LLDB side (previously we had no proper scoped enum support in LLDB)

[CarlosAlbertoEnciso]

In D39239#912837, @probinson wrote:

Have you tried the GDB suite yet? If it has no problems, and we can make LLDB happy, I'm okay with it.

Hi Paul,

Following the instructions from David Blaikie on how to build/run the GDB suite:

"Directions for how to run it would be divined from the buildbot configuration.

https://github.com/llvm-mirror/zorg/blob/master/zorg/buildbot/builders/ClangBuilder.py#L883"

1. I have managed to build the GDB suite using clang as the default compiler.
2. Run the test before and after my changes and the results are the same.

		=== gdb Summary ===

# of expected passes		20501
# of unexpected failures	1481
# of expected failures		262
# of unknown successes		2
# of known failures		78
# of unresolved testcases	9
# of untested testcases		48
# of unsupported tests		36

The unexpected failures falls into the categories:

• Invalid option '-w' to the ADA compiler
• Timeout issues
• Threading and attaching issues

Possible incorrect settings for the GDB suite.

[CarlosAlbertoEnciso]

In D39239#912837, @probinson wrote:

Have you tried the GDB suite yet? If it has no problems, and we can make LLDB happy, I'm okay with it.

These are the results when using GCC as the default compiler:

		=== gdb Summary ===

# of expected passes		21368
# of unexpected failures	1788
# of expected failures		213
# of unknown successes		2
# of known failures		75
# of unresolved testcases	10
# of untested testcases		29
# of unsupported tests		34

The unexpected failures falls into the categories (same as with clang):

• Invalid option '-w' to the ADA compiler
• Timeout issues
• Threading and attaching issues

[probinson]

With the GDB test results and LLDB able to handle it, this LGTM.
Carlos, do you have commit access?

[CarlosAlbertoEnciso]

In D39239#961367, @probinson wrote:

With the GDB test results and LLDB able to handle it, this LGTM.
Carlos, do you have commit access?

Hi Paul,

Thanks for the LGTM.

I do not have commit access.

lib/AST/Decl.cpp

if (const auto *Spec = dyn_cast<ClassTemplateSpecializationDecl>(DC)) {
         }
       }
       OS << ')';
     } else if (const auto *ED = dyn_cast<EnumDecl>(DC)) {
       // C++ [dcl.enum]p10: Each enum-name and each unscoped
       // enumerator is declared in the scope that immediately contains
       // the enum-specifier. Each scoped enumerator is declared in the
       // scope of the enumeration.
-      if (ED->isScoped() || ED->getIdentifier())
+      // For the case of unscoped enumerator, do not include in the qualified
+      // name any information about its enum enclosing scope, as is visibility
+      // is global.
+      if (ED->isScoped())
         OS << *ED;
       else
         continue;
     } else {
       OS << *cast<NamedDecl>(DC);
     }
     OS << "::";
   }

test/Modules/odr.cpp

 
 // Trigger the declarations from a and b to be imported.
 int x = f() + g();
 
 // expected-note@a.h:5 {{definition has no member 'e2'}}
 // expected-note@a.h:3 {{declaration of 'f' does not match}}
 // expected-note@a.h:1 {{definition has no member 'm'}}
 
-// expected-error@b.h:5 {{'E::e2' from module 'b' is not present in definition of 'E' in module 'a'}}
+// expected-error@b.h:5 {{'e2' from module 'b' is not present in definition of 'E' in module 'a'}}
 // expected-error@b.h:3 {{'Y::f' from module 'b' is not present in definition of 'Y' in module 'a'}}
 // expected-error@b.h:2 {{'Y::m' from module 'b' is not present in definition of 'Y' in module 'a'}}

test/SemaCXX/return-noreturn.cpp

template <PR9412_MatchType type> int PR9412_t() {
   switch (type) {
     case PR9412_Exact:
     default:
         break;
   }
 } // expected-warning {{control reaches end of non-void function}}
 
 void PR9412_f() {
-    PR9412_t<PR9412_Exact>(); // expected-note {{in instantiation of function template specialization 'PR9412_t<PR9412_MatchType::PR9412_Exact>' requested here}}
+    PR9412_t<PR9412_Exact>(); // expected-note {{in instantiation of function template specialization 'PR9412_t<PR9412_Exact>' requested here}}
 }
 
 struct NoReturn {
   ~NoReturn() __attribute__((noreturn));
   operator bool() const;
 };
 struct Return {
   ~Return();

test/SemaTemplate/temp_arg_enum_printing.cpp

 // RUN: %clang_cc1 -fsyntax-only -ast-print %s | FileCheck %s
 
 namespace NamedEnumNS
 {
   
 enum NamedEnum
 {
   Val0,
   Val1
 };
   
 template <NamedEnum E>
 void foo();
   
 void test() {
-  // CHECK: template<> void foo<NamedEnumNS::NamedEnum::Val0>()
+  // CHECK: template<> void foo<NamedEnumNS::Val0>()
   NamedEnumNS::foo<Val0>();
-  // CHECK: template<> void foo<NamedEnumNS::NamedEnum::Val1>()
+  // CHECK: template<> void foo<NamedEnumNS::Val1>()
   NamedEnumNS::foo<(NamedEnum)1>();
   // CHECK: template<> void foo<2>()
   NamedEnumNS::foo<(NamedEnum)2>();
 }
   
 } // NamedEnumNS

test/SemaTemplate/temp_arg_enum_printing_more.cpp

+// RUN: %clang_cc1 -fsyntax-only -ast-print %s -std=c++11 | FileCheck %s
+
+// Make sure that for template value arguments that are unscoped enumerators,
+// no qualified enum information is included in their name, as their visibility
+// is global. In the case of scoped enumerators, they must include information
+// about their enum enclosing scope.
+
+enum E1 { e1 };
+template<E1 v> struct tmpl_1 {};
+// CHECK: template<> struct tmpl_1<e1>
+tmpl_1<E1::e1> TMPL_1;                      // Name must be 'e1'.
+
+namespace nsp_1 { enum E2 { e2 }; }
+template<nsp_1::E2 v> struct tmpl_2 {};
+// CHECK: template<> struct tmpl_2<nsp_1::e2>
+tmpl_2<nsp_1::E2::e2> TMPL_2;               // Name must be 'nsp_1::e2'.
+
+enum class E3 { e3 };
+template<E3 v> struct tmpl_3 {};
+// CHECK: template<> struct tmpl_3<E3::e3>
+tmpl_3<E3::e3> TMPL_3;                      // Name must be 'E3::e3'.
+
+namespace nsp_2 { enum class E4 { e4 }; }
+template<nsp_2::E4 v> struct tmpl_4 {};
+// CHECK: template<> struct tmpl_4<nsp_2::E4::e4>
+tmpl_4<nsp_2::E4::e4> TMPL_4;               // Name must be 'nsp_2::E4::e4'.

unittests/AST/NamedDeclPrinterTest.cpp

ASSERT_TRUE(PrintedWrittenNamedDeclCXX11Matches(
     "A",
     "A"));
 }
 
 TEST(NamedDeclPrinter, TestNamedEnum) {
   ASSERT_TRUE(PrintedWrittenNamedDeclCXX11Matches(
     "enum X { A };",
     "A",
-    "X::A"));
+    "A"));
 }
 
 TEST(NamedDeclPrinter, TestScopedNamedEnum) {
   ASSERT_TRUE(PrintedWrittenNamedDeclCXX11Matches(
     "enum class X { A };",
     "A",
     "X::A"));
 }
 
 TEST(NamedDeclPrinter, TestClassWithUnscopedUnnamedEnum) {
   ASSERT_TRUE(PrintedWrittenNamedDeclCXX11Matches(
     "class X { enum { A }; };",
     "A",
     "X::A"));
 }
 
 TEST(NamedDeclPrinter, TestClassWithUnscopedNamedEnum) {
   ASSERT_TRUE(PrintedWrittenNamedDeclCXX11Matches(
     "class X { enum Y { A }; };",
     "A",
-    "X::Y::A"));
+    "X::A"));
 }
 
 TEST(NamedDeclPrinter, TestClassWithScopedNamedEnum) {
   ASSERT_TRUE(PrintedWrittenNamedDeclCXX11Matches(
     "class X { enum class Y { A }; };",
     "A",
     "X::Y::A"));
 }