Page MenuHomePhabricator

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

Authored by CarlosAlbertoEnciso on Oct 24 2017, 6:42 AM.

Details

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.

Diff Detail

Repository
rC Clang

Event Timeline

probinson edited edge metadata.Oct 24 2017, 1:27 PM

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.

lattner resigned from this revision.Oct 24 2017, 9:25 PM

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.

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.

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 edited edge metadata.Oct 30 2017, 9:32 AM

2 fairly random ideas without looking into it too much:

Hi Tamas,

Thanks very much for your message.

  • 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.

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.

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

Hi Tamas,

Thanks very much for your message.

  • 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)?

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

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.

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 accepted this revision.Dec 20 2017, 12:51 PM
probinson added a project: debug-info.

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

This revision is now accepted and ready to land.Dec 20 2017, 12:51 PM

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.

This revision was automatically updated to reflect the committed changes.