[Clang] Emit DW_TAG_template_alias for template aliases by OCHyams · Pull Request #87623 · llvm/llvm-project (original) (raw)

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation72 Commits19 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

OCHyams

Fix issue #54624

Add front end flags -gtemplate-alias (also a cc1 flag) and -gno-template-alias to enable/disable usage of the feature. It's enabled by default in the front end for SCE debugger tuning only. GCC emits DW_TAG_typedef for template aliases, and GDB and LLDB appear not to support DW_TAG_template_alias.

The -Xclang option -gsimple-template-names=mangled is treated the same as =full, which is not a regression from current behaviour for type aliases.

Use DICompositeType to represnt the template alias, using its extraData field as a tuple of DITemplateParameter to describe the template parameters.

New tests:

  template-alias.cpp - Check the metadata construction & interaction with
                       -gsimple-template-names.
  template-alias.ll  - Check DWARF emission.

Modified tests:

  debug-options.c    - Check Clang generates correct cc1 flags.
  frame-types.s      - Check llvm-symbolizer understands the DIE.

@llvmbot

@llvm/pr-subscribers-llvm-ir
@llvm/pr-subscribers-clang-driver
@llvm/pr-subscribers-llvm-binary-utilities
@llvm/pr-subscribers-debuginfo

@llvm/pr-subscribers-clang-codegen

Author: Orlando Cazalet-Hyams (OCHyams)

Changes

Fix issue #86529

Add front end flags -gtemplate-alias (also a cc1 flag) and -gno-template-alias to enable/disable usage of the feature. It's enabled by default in the front end for SCE debugger tuning only. GCC emits DW_TAG_typedef for template aliases, and GDB and LLDB appear not to support DW_TAG_template_alias.

The -Xclang option -gsimple-template-names=mangled is treated the same as =full, which is not a regression from current behaviour for type aliases.

Use DICompositeType to represnt the template alias, using its extraData field as a tuple of DITemplateParameter to describe the template parameters.

New tests:

  template-alias.cpp - Check the metadata construction & interaction with
                       -gsimple-template-names.
  template-alias.ll  - Check DWARF emission.

Modified tests:

  debug-options.c    - Check Clang generates correct cc1 flags.
  frame-types.s      - Check llvm-symbolizer understands the DIE.

Patch is 79.70 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/87623.diff

22 Files Affected:

diff --git a/clang/include/clang/Basic/DebugOptions.def b/clang/include/clang/Basic/DebugOptions.def index 7cd3edf08a17ea..b94f6aef9ac60b 100644 --- a/clang/include/clang/Basic/DebugOptions.def +++ b/clang/include/clang/Basic/DebugOptions.def @@ -129,6 +129,9 @@ DEBUGOPT(CodeViewCommandLine, 1, 0) /// Whether emit extra debug info for sample pgo profile collection. DEBUGOPT(DebugInfoForProfiling, 1, 0) +/// Whether to emit DW_TAG_template_alias for template aliases. +DEBUGOPT(DebugTemplateAlias, 1, 0) + /// Whether to emit .debug_gnu_pubnames section instead of .debug_pubnames. DEBUGOPT(DebugNameTable, 2, 0) diff --git a/clang/include/clang/Basic/DiagnosticDriverKinds.td b/clang/include/clang/Basic/DiagnosticDriverKinds.td index e33a1f4c45b949..862a6c72a769b2 100644 --- a/clang/include/clang/Basic/DiagnosticDriverKinds.td +++ b/clang/include/clang/Basic/DiagnosticDriverKinds.td @@ -393,6 +393,9 @@ def warn_ignored_clang_option : Warning<"the flag '%0' has been deprecated and w def warn_drv_unsupported_opt_for_target : Warning< "optimization flag '%0' is not supported for target '%1'">, InGroup; +def warn_drv_dwarf_feature_requires_version : Warning< + "debug information option '%0' is not supported and will be ignored; requires at least DWARF-%1 but " + "-gdwarf-%2 has been specified">, InGroup; def warn_drv_unsupported_debug_info_opt_for_target : Warning< "debug information option '%0' is not supported for target '%1'">, InGroup; diff --git a/clang/include/clang/Driver/Options.td b/clang/include/clang/Driver/Options.td index 29066ea14280c2..ec95e3077dc15c 100644 --- a/clang/include/clang/Driver/Options.td +++ b/clang/include/clang/Driver/Options.td @@ -4273,6 +4273,8 @@ def gsplit_dwarf_EQ : Joined<["-"], "gsplit-dwarf=">, Group, Values<"split,single">; def gno_split_dwarf : Flag<["-"], "gno-split-dwarf">, Group, Visibility<[ClangOption, CLOption, DXCOption]>; +def gtemplate_alias : Flag<["-"], "gtemplate-alias">, Group, Visibility<[ClangOption, CC1Option]>; +def gno_template_alias : Flag<["-"], "gno-template-alias">, Group, Visibility<[ClangOption]>; def gsimple_template_names : Flag<["-"], "gsimple-template-names">, Group; def gsimple_template_names_EQ : Joined<["-"], "gsimple-template-names=">, diff --git a/clang/lib/CodeGen/CGDebugInfo.cpp b/clang/lib/CodeGen/CGDebugInfo.cpp index 7453ed14aef414..1667f948db61d7 100644 --- a/clang/lib/CodeGen/CGDebugInfo.cpp +++ b/clang/lib/CodeGen/CGDebugInfo.cpp @@ -1332,6 +1332,30 @@ llvm::DIType *CGDebugInfo::CreateType(const TemplateSpecializationType *Ty, auto PP = getPrintingPolicy(); Ty->getTemplateName().print(OS, PP, TemplateName::Qualified::None); + SourceLocation Loc = AliasDecl->getLocation(); + + if (CGM.getCodeGenOpts().DebugTemplateAlias) { + TemplateArgs Args = {TD->getTemplateParameters(), Ty->template_arguments()}; + + // FIXME: Respect DebugTemplateNameKind::Mangled, e.g. by using GetName. + // Note we can't use GetName without additional work: TypeAliasTemplateDecl + // doesn't have instantiation information, so + // TypeAliasTemplateDecl::getNameForDiagnostic wouldn't have access to the + // template args. + std::string Name; + llvm::raw_string_ostream OS(Name); + TD->getNameForDiagnostic(OS, PP, /Qualified=/false); + if (CGM.getCodeGenOpts().getDebugSimpleTemplateNames() != + llvm::codegenoptions::DebugTemplateNamesKind::Simple || + !HasReconstitutableArgs(Args.Args)) + printTemplateArgumentList(OS, Args.Args, PP); + + llvm::DIDerivedType *AliasTy = DBuilder.createTemplateAlias( + Src, Name, getOrCreateFile(Loc), getLineNumber(Loc), + getDeclContextDescriptor(AliasDecl), CollectTemplateParams(Args, Unit)); + return AliasTy; + } + // Disable PrintCanonicalTypes here because we want // the DW_AT_name to benefit from the TypePrinter's ability // to skip defaulted template arguments. @@ -1343,8 +1367,6 @@ llvm::DIType *CGDebugInfo::CreateType(const TemplateSpecializationType *Ty, PP.PrintCanonicalTypes = false; printTemplateArgumentList(OS, Ty->template_arguments(), PP, TD->getTemplateParameters());

@@ -5361,7 +5383,56 @@ static bool IsReconstitutableType(QualType QT) { return T.Reconstitutable; }

-std::string CGDebugInfo::GetName(const Decl *D, bool Qualified) const { +bool CGDebugInfo::HasReconstitutableArgs(

diff --git a/clang/lib/Driver/ToolChains/Clang.cpp b/clang/lib/Driver/ToolChains/Clang.cpp index 3bcacff7724c7d..fdc7ba241dd14d 100644 --- a/clang/lib/Driver/ToolChains/Clang.cpp +++ b/clang/lib/Driver/ToolChains/Clang.cpp @@ -4584,6 +4584,32 @@ renderDebugOptions(const ToolChain &TC, const Driver &D, const llvm::Triple &T, } }

diff --git a/clang/lib/Frontend/CompilerInvocation.cpp b/clang/lib/Frontend/CompilerInvocation.cpp index 7bd91d4791ecf0..e9b171f945bdb4 100644 --- a/clang/lib/Frontend/CompilerInvocation.cpp +++ b/clang/lib/Frontend/CompilerInvocation.cpp @@ -1556,6 +1556,9 @@ void CompilerInvocationBase::GenerateCodeGenArgs(const CodeGenOptions &Opts, llvm::DICompileUnit::DebugNameTableKind::Default)) GenerateArg(Consumer, OPT_gpubnames);

@@ -1827,6 +1830,8 @@ bool CompilerInvocation::ParseCodeGenArgs(CodeGenOptions &Opts, ArgList &Args, Opts.BinutilsVersion = std::string(Args.getLastArgValue(OPT_fbinutils_version_EQ));

diff --git a/clang/test/CodeGenCXX/debug-info-alias.cpp b/clang/test/CodeGenCXX/debug-info-alias.cpp index 3d3f87ed1f6fa8..d87063e651e83d 100644 --- a/clang/test/CodeGenCXX/debug-info-alias.cpp +++ b/clang/test/CodeGenCXX/debug-info-alias.cpp @@ -1,4 +1,4 @@ -// RUN: %clang -g -std=c++11 -S -emit-llvm %s -o - | FileCheck %s +// RUN: %clang -ggdb -std=c++11 -S -emit-llvm %s -o - | FileCheck %s

template struct foo { diff --git a/clang/test/CodeGenCXX/template-alias.cpp b/clang/test/CodeGenCXX/template-alias.cpp new file mode 100644 index 00000000000000..a671f9820b408b --- /dev/null +++ b/clang/test/CodeGenCXX/template-alias.cpp @@ -0,0 +1,47 @@ +// RUN: %clang_cc1 -triple x86_64-unk-unk -o - -emit-llvm -debug-info-kind=standalone -gtemplate-alias %s -gsimple-template-names=simple
+// RUN: | FileCheck %s --check-prefixes=ALIAS-SIMPLE,ALIAS-ALL + +// RUN: %clang_cc1 -triple x86_64-unk-unk -o - -emit-llvm -debug-info-kind=standalone -gtemplate-alias %s -gsimple-template-names=mangled
+// RUN: | FileCheck %s --check-prefixes=ALIAS-MANGLED,ALIAS-ALL + +// RUN: %clang_cc1 -triple x86_64-unk-unk -o - -emit-llvm -debug-info-kind=standalone -gtemplate-alias %s
+// RUN: | FileCheck %s --check-prefixes=ALIAS-FULL,ALIAS-ALL + +// RUN: %clang_cc1 -triple x86_64-unk-unk -o - -emit-llvm -debug-info-kind=standalone %s
+// RUN: | FileCheck %s --check-prefixes=TYPEDEF + + +//// Check that -gtemplate-alias causes DW_TAG_template_alias emission for +//// template aliases, and that respects gsimple-template-names. +//// +//// Test type and value template parameters. + +template<typename Y, int Z> +struct X {

diff --git a/llvm/include/llvm/IR/DebugInfoMetadata.h b/llvm/include/llvm/IR/DebugInfoMetadata.h index 2805f6c4780578..0470ed633274bd 100644 --- a/llvm/include/llvm/IR/DebugInfoMetadata.h +++ b/... [truncated]

@llvmbot

@llvm/pr-subscribers-clang

Author: Orlando Cazalet-Hyams (OCHyams)

Changes

Fix issue #86529

Add front end flags -gtemplate-alias (also a cc1 flag) and -gno-template-alias to enable/disable usage of the feature. It's enabled by default in the front end for SCE debugger tuning only. GCC emits DW_TAG_typedef for template aliases, and GDB and LLDB appear not to support DW_TAG_template_alias.

The -Xclang option -gsimple-template-names=mangled is treated the same as =full, which is not a regression from current behaviour for type aliases.

Use DICompositeType to represnt the template alias, using its extraData field as a tuple of DITemplateParameter to describe the template parameters.

New tests:

  template-alias.cpp - Check the metadata construction &amp; interaction with
                       -gsimple-template-names.
  template-alias.ll  - Check DWARF emission.

Modified tests:

  debug-options.c    - Check Clang generates correct cc1 flags.
  frame-types.s      - Check llvm-symbolizer understands the DIE.

Patch is 79.70 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/87623.diff

22 Files Affected:

diff --git a/clang/include/clang/Basic/DebugOptions.def b/clang/include/clang/Basic/DebugOptions.def index 7cd3edf08a17ea..b94f6aef9ac60b 100644 --- a/clang/include/clang/Basic/DebugOptions.def +++ b/clang/include/clang/Basic/DebugOptions.def @@ -129,6 +129,9 @@ DEBUGOPT(CodeViewCommandLine, 1, 0) /// Whether emit extra debug info for sample pgo profile collection. DEBUGOPT(DebugInfoForProfiling, 1, 0) +/// Whether to emit DW_TAG_template_alias for template aliases. +DEBUGOPT(DebugTemplateAlias, 1, 0) + /// Whether to emit .debug_gnu_pubnames section instead of .debug_pubnames. DEBUGOPT(DebugNameTable, 2, 0) diff --git a/clang/include/clang/Basic/DiagnosticDriverKinds.td b/clang/include/clang/Basic/DiagnosticDriverKinds.td index e33a1f4c45b949..862a6c72a769b2 100644 --- a/clang/include/clang/Basic/DiagnosticDriverKinds.td +++ b/clang/include/clang/Basic/DiagnosticDriverKinds.td @@ -393,6 +393,9 @@ def warn_ignored_clang_option : Warning<"the flag '%0' has been deprecated and w def warn_drv_unsupported_opt_for_target : Warning< "optimization flag '%0' is not supported for target '%1'">, InGroup; +def warn_drv_dwarf_feature_requires_version : Warning< + "debug information option '%0' is not supported and will be ignored; requires at least DWARF-%1 but " + "-gdwarf-%2 has been specified">, InGroup; def warn_drv_unsupported_debug_info_opt_for_target : Warning< "debug information option '%0' is not supported for target '%1'">, InGroup; diff --git a/clang/include/clang/Driver/Options.td b/clang/include/clang/Driver/Options.td index 29066ea14280c2..ec95e3077dc15c 100644 --- a/clang/include/clang/Driver/Options.td +++ b/clang/include/clang/Driver/Options.td @@ -4273,6 +4273,8 @@ def gsplit_dwarf_EQ : Joined<["-"], "gsplit-dwarf=">, Group, Values<"split,single">; def gno_split_dwarf : Flag<["-"], "gno-split-dwarf">, Group, Visibility<[ClangOption, CLOption, DXCOption]>; +def gtemplate_alias : Flag<["-"], "gtemplate-alias">, Group, Visibility<[ClangOption, CC1Option]>; +def gno_template_alias : Flag<["-"], "gno-template-alias">, Group, Visibility<[ClangOption]>; def gsimple_template_names : Flag<["-"], "gsimple-template-names">, Group; def gsimple_template_names_EQ : Joined<["-"], "gsimple-template-names=">, diff --git a/clang/lib/CodeGen/CGDebugInfo.cpp b/clang/lib/CodeGen/CGDebugInfo.cpp index 7453ed14aef414..1667f948db61d7 100644 --- a/clang/lib/CodeGen/CGDebugInfo.cpp +++ b/clang/lib/CodeGen/CGDebugInfo.cpp @@ -1332,6 +1332,30 @@ llvm::DIType *CGDebugInfo::CreateType(const TemplateSpecializationType *Ty, auto PP = getPrintingPolicy(); Ty->getTemplateName().print(OS, PP, TemplateName::Qualified::None); + SourceLocation Loc = AliasDecl->getLocation(); + + if (CGM.getCodeGenOpts().DebugTemplateAlias) { + TemplateArgs Args = {TD->getTemplateParameters(), Ty->template_arguments()}; + + // FIXME: Respect DebugTemplateNameKind::Mangled, e.g. by using GetName. + // Note we can't use GetName without additional work: TypeAliasTemplateDecl + // doesn't have instantiation information, so + // TypeAliasTemplateDecl::getNameForDiagnostic wouldn't have access to the + // template args. + std::string Name; + llvm::raw_string_ostream OS(Name); + TD->getNameForDiagnostic(OS, PP, /Qualified=/false); + if (CGM.getCodeGenOpts().getDebugSimpleTemplateNames() != + llvm::codegenoptions::DebugTemplateNamesKind::Simple || + !HasReconstitutableArgs(Args.Args)) + printTemplateArgumentList(OS, Args.Args, PP); + + llvm::DIDerivedType *AliasTy = DBuilder.createTemplateAlias( + Src, Name, getOrCreateFile(Loc), getLineNumber(Loc), + getDeclContextDescriptor(AliasDecl), CollectTemplateParams(Args, Unit)); + return AliasTy; + } + // Disable PrintCanonicalTypes here because we want // the DW_AT_name to benefit from the TypePrinter's ability // to skip defaulted template arguments. @@ -1343,8 +1367,6 @@ llvm::DIType *CGDebugInfo::CreateType(const TemplateSpecializationType *Ty, PP.PrintCanonicalTypes = false; printTemplateArgumentList(OS, Ty->template_arguments(), PP, TD->getTemplateParameters());

@@ -5361,7 +5383,56 @@ static bool IsReconstitutableType(QualType QT) { return T.Reconstitutable; }

-std::string CGDebugInfo::GetName(const Decl *D, bool Qualified) const { +bool CGDebugInfo::HasReconstitutableArgs(

diff --git a/clang/lib/Driver/ToolChains/Clang.cpp b/clang/lib/Driver/ToolChains/Clang.cpp index 3bcacff7724c7d..fdc7ba241dd14d 100644 --- a/clang/lib/Driver/ToolChains/Clang.cpp +++ b/clang/lib/Driver/ToolChains/Clang.cpp @@ -4584,6 +4584,32 @@ renderDebugOptions(const ToolChain &TC, const Driver &D, const llvm::Triple &T, } }

diff --git a/clang/lib/Frontend/CompilerInvocation.cpp b/clang/lib/Frontend/CompilerInvocation.cpp index 7bd91d4791ecf0..e9b171f945bdb4 100644 --- a/clang/lib/Frontend/CompilerInvocation.cpp +++ b/clang/lib/Frontend/CompilerInvocation.cpp @@ -1556,6 +1556,9 @@ void CompilerInvocationBase::GenerateCodeGenArgs(const CodeGenOptions &Opts, llvm::DICompileUnit::DebugNameTableKind::Default)) GenerateArg(Consumer, OPT_gpubnames);

@@ -1827,6 +1830,8 @@ bool CompilerInvocation::ParseCodeGenArgs(CodeGenOptions &Opts, ArgList &Args, Opts.BinutilsVersion = std::string(Args.getLastArgValue(OPT_fbinutils_version_EQ));

diff --git a/clang/test/CodeGenCXX/debug-info-alias.cpp b/clang/test/CodeGenCXX/debug-info-alias.cpp index 3d3f87ed1f6fa8..d87063e651e83d 100644 --- a/clang/test/CodeGenCXX/debug-info-alias.cpp +++ b/clang/test/CodeGenCXX/debug-info-alias.cpp @@ -1,4 +1,4 @@ -// RUN: %clang -g -std=c++11 -S -emit-llvm %s -o - | FileCheck %s +// RUN: %clang -ggdb -std=c++11 -S -emit-llvm %s -o - | FileCheck %s

template struct foo { diff --git a/clang/test/CodeGenCXX/template-alias.cpp b/clang/test/CodeGenCXX/template-alias.cpp new file mode 100644 index 00000000000000..a671f9820b408b --- /dev/null +++ b/clang/test/CodeGenCXX/template-alias.cpp @@ -0,0 +1,47 @@ +// RUN: %clang_cc1 -triple x86_64-unk-unk -o - -emit-llvm -debug-info-kind=standalone -gtemplate-alias %s -gsimple-template-names=simple
+// RUN: | FileCheck %s --check-prefixes=ALIAS-SIMPLE,ALIAS-ALL + +// RUN: %clang_cc1 -triple x86_64-unk-unk -o - -emit-llvm -debug-info-kind=standalone -gtemplate-alias %s -gsimple-template-names=mangled
+// RUN: | FileCheck %s --check-prefixes=ALIAS-MANGLED,ALIAS-ALL + +// RUN: %clang_cc1 -triple x86_64-unk-unk -o - -emit-llvm -debug-info-kind=standalone -gtemplate-alias %s
+// RUN: | FileCheck %s --check-prefixes=ALIAS-FULL,ALIAS-ALL + +// RUN: %clang_cc1 -triple x86_64-unk-unk -o - -emit-llvm -debug-info-kind=standalone %s
+// RUN: | FileCheck %s --check-prefixes=TYPEDEF + + +//// Check that -gtemplate-alias causes DW_TAG_template_alias emission for +//// template aliases, and that respects gsimple-template-names. +//// +//// Test type and value template parameters. + +template<typename Y, int Z> +struct X {

diff --git a/llvm/include/llvm/IR/DebugInfoMetadata.h b/llvm/include/llvm/IR/DebugInfoMetadata.h index 2805f6c4780578..0470ed633274bd 100644 --- a/llvm/include/llvm/IR/DebugInfoMetadata.h +++ b/... [truncated]

@github-actions GitHub Actions

✅ With the latest revision this PR passed the C/C++ code formatter.

pogo59

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quick initial pass.

Converting frame-types.s from v4 to v5 should be done as a separate commit. Then adding the alias would be a simpler change to review. (Is symbolizer really the right place to be testing this, anyway?)

I'm a little uncomfortable with adding a new user-facing option for template aliases. Even with that in place, we should not warn and refuse to do what the user asked for, based on DWARF version. -gdwarf-2 -gsplit-dwarf generates a .dwo file claiming to be v2, for example.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

/// \param Context The surrounding context for the typedef.
/// \param TParams The template arguments.
/// \param AlignInBits Alignment. (optional)
/// \param Flags Flags to describe inheritance attribute, e.g. private

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(optional)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

Tag != dwarf::DW_TAG_restrict_type && Tag != dwarf::DW_TAG_atomic_type &&
Tag != dwarf::DW_TAG_immutable_type)
Tag != dwarf::DW_TAG_immutable_type &&
Tag != dwarf::DW_TAG_template_alias)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this got added twice?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

// info is the string name of the template. (so the template name
// itself won't benefit from any name rebuilding, but that's a
// representational limitation - maybe DWARF could be
// changed/improved to use some more structural representation)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The sentence inside the parens should be a proper sentence.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are existing comments that I've just translocated - this block has been copied from line 5390, below. I'm happy to make your suggested change, I just wanted to add this context first in case it changes your view?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, didn't notice it was a copy-paste. But it would still be nice to have the comment follow our style guide. So, up to you.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Promoted the sentence out of parens.

@OCHyams

Quick initial pass.

Much appreciated!

Converting frame-types.s from v4 to v5 should be done as a separate commit. Then adding the alias would be a simpler change to review.

Makes sense, I can do that.

(Is symbolizer really the right place to be testing this, anyway?)

I modified frame-types.s here because getTypeSizeImpl in DWARFDie.cpp is stimulated by that test, and I've made a change to that function in this patch.

I'm a little uncomfortable with adding a new user-facing option for template aliases. Even with that in place, we should not warn and refuse to do what the user asked for, based on DWARF version. -gdwarf-2 -gsplit-dwarf generates a .dwo file claiming to be v2, for example.

I don't have strong feelings about the flag setup and am happy to change it. @dwblaikie mentioned that we might not want to have the debugger tuning be the only way of controlling whether or not we emit this DIE, so adding -gtemplate-alias which has its default set based on debugger tuning seemed like a reasonable way of doing it. What would be the best way of controlling this feature in your opinion (bearing in mind that GCC doesn't emit it, and GDB and LLDB don't the DIE)?

adrian-prantl

; PUB-TYPES: "A"
target triple = "x86_64-unk-unk"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

x86_64-unknown-unkown

adrian-prantl

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LLVM IR parts look good to me.

@OCHyams

LLVM IR parts look good to me.

Thanks!

@pogo59 said:

Converting frame-types.s from v4 to v5 should be done as a separate commit. Then adding the alias would be a simpler change to review.

I've split that into two commits in this PR: upgrade to v5: 18446f2, add template alias: 49d6642. It does improve readability a bit but its still quite noisy. I am happy to commit the former separately then rebase this PR, if you'd like?

Michael137

@@ -1,4 +1,4 @@
// RUN: %clang -g -std=c++11 -S -emit-llvm %s -o - | FileCheck %s
// RUN: %clang -ggdb -std=c++11 -S -emit-llvm %s -o - | FileCheck %s

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we now lose a tiny bit of coverage for -glldb? I assume this change is to make sure the test passes on platforms that are tuned for sce? Maybe we should just add -gno-template-alias instead?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, done

Michael137

}
std::string CGDebugInfo::GetName(const Decl *D, bool Qualified,
const Type *Ty) const {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we using this extra Ty parameter anywhere? I assume your original intent was to pass the TemplateSpecializationType into GetName and get the template arguments for the alias decl that way? Any reason you didn't end up doing that?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aha, good catch. You're correct on both counts - it's not used in this patch and that was one of the approaches I tried out.

This is discussed a bit on the issue starting here #54624 (comment).

IIRC, we could pass in the TemplateSpecializationType to get the template arguments for this case, but I think we'd need to introduce a special case for building the name where we can't use D's getNameForDiagnostic on line 5461 and 5481 of the original file.

My memory is slightly blurred by the conference - I think I was erring on the side of caution given my unfamiliarity with the code, preferring to have a special case localised at the usage site rather than inside a utility function. I'd be happy to change the patch to use the TemplateSpecializationType parameter if you'd like to see what it looks like?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah thanks for the additional context. Keeping it as is seems fine to me then. I don't see a better way to get to template arguments from the alias decl. My initial thought of special-casing this in GetName isn't necessarily great either

@dwblaikie

I'm a little uncomfortable with adding a new user-facing option for template aliases. Even with that in place, we should not warn and refuse to do what the user asked for, based on DWARF version. -gdwarf-2 -gsplit-dwarf generates a .dwo file claiming to be v2, for example.

I don't have strong feelings about the flag setup and am happy to change it. @dwblaikie mentioned that we might not want to have the debugger tuning be the only way of controlling whether or not we emit this DIE, so adding -gtemplate-alias which has its default set based on debugger tuning seemed like a reasonable way of doing it. What would be the best way of controlling this feature in your opinion (bearing in mind that GCC doesn't emit it, and GDB and LLDB don't the DIE)?

@pogo59 Hmm - I thought you held a strong preference that debugger tuning never be the only way to access a certain feature, and that we should always have direct control of these features via flags (but with default (or explicit) debugger tuning setting some of these flag defaults based on the preferred content for that debugger). Perhaps i'm misremembering?

@dwblaikie

Re: code: Looks about right.

Bit unfortunate to store template parameters in different ways (in the extraData for the alias template, but in the templateParams for the composite types) - but I guess it'd be more invasive to try to represent alias templates as composite types?

@pogo59

@pogo59 Hmm - I thought you held a strong preference that debugger tuning never be the only way to access a certain feature, and that we should always have direct control of these features via flags (but with default (or explicit) debugger tuning setting some of these flag defaults based on the preferred content for that debugger). Perhaps i'm misremembering?

You are not misremembering. Ideally tuning is not gating (although we probably never expressed it that way). We probably have not fully held to that ideal, but if we're going to have knobs to tweak we should implement the knobs properly. I will be making another pass over this PR later today and pay attention to that.

@OCHyams

Thanks @dwblaikie

Bit unfortunate to store template parameters in different ways (in the extraData for the alias template, but in the templateParams for the composite types) - but I guess it'd be more invasive to try to represent alias templates as composite types?

I hadn't actually tried using DICompsiteType. I didn't choose DIDerivedType for any particularly principled reasons: typedefs use derived types which made it very easy to find places that needed updating, and an alias "felt" more like a derived type than composite type. Having looked only briefly, I don't think it'd be any more invasive to use composite types instead (not 100% sure without diving in). I'm happy to give that a go if that is your preference?

I've just found an input that causes an assertion failure in CollectTemplateParams with my implementation:

template<typename Y, typename Z>
struct X {
  Y m1;
  Z m2;
};

template<typename... Ts>
using A = X<Ts...>;

A<int, int> a;

There's a mismatch between the template parameter list and template argument list sizes, created here:

  if (CGM.getCodeGenOpts().DebugTemplateAlias) {
    TemplateArgs Args = {TD->getTemplateParameters(), Ty->template_arguments()}; // <--- here

I notice that we emit DW_TAG_GNU_template_parameter_pack for, e.g., variadic template struct instantiations. I've not figured out how to fix this just yet, but thought I'd bring it up in case there's something obvious.

@dwblaikie

Thanks @dwblaikie

Bit unfortunate to store template parameters in different ways (in the extraData for the alias template, but in the templateParams for the composite types) - but I guess it'd be more invasive to try to represent alias templates as composite types?

I hadn't actually tried using DICompsiteType. I didn't choose DIDerivedType for any particularly principled reasons: typedefs use derived types which made it very easy to find places that needed updating, and an alias "felt" more like a derived type than composite type. Having looked only briefly, I don't think it'd be any more invasive to use composite types instead (not 100% sure without diving in). I'm happy to give that a go if that is your preference?

Eh, I guess if we've already got DIGlobalVariable having template params, we aren't going to unify all 3 cases - not sure if unifying 2 out of three is particularly valuable, especially if it ends up with a different conflict (that alias templates look less like aliases & so need special casing in that direction)

But perhaps at least you could add named accessors to DIDerivedType for the template params, same as DIGlobalVariable/DICompositeType have?

(oh, and in case anyone hasn't mentioned it already - this would generally be committed in smaller pieces upstream - adding the LLVM functionality first, then adding clang patches that use that functionality)

I've just found an input that causes an assertion failure in CollectTemplateParams with my implementation:

template<typename Y, typename Z>
struct X {
 Y m1;
 Z m2;
};

template<typename... Ts>
using A = X<Ts...>;

A<int, int> a;

There's a mismatch between the template parameter list and template argument list sizes, created here:

 if (CGM.getCodeGenOpts().DebugTemplateAlias) {
   TemplateArgs Args = {TD->getTemplateParameters(), Ty->template_arguments()}; // <--- here

I notice that we emit DW_TAG_GNU_template_parameter_pack for, e.g., variadic template struct instantiations. I've not figured out how to fix this just yet, but thought I'd bring it up in case there's something obvious.

yeah, probably worth checking the APIs that we use for building the template args for classes and variable templates, they probably have some handling for this - would be good to share/reuse that code, rather than duplicating it, once you find it

pogo59

<< EffectiveDWARFVersion;
else
CmdArgs.push_back("-gtemplate-alias");
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please take out the diags here, if someone explicitly asks for something in debug info we do it.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

// info is the string name of the template. (so the template name
// itself won't benefit from any name rebuilding, but that's a
// representational limitation - maybe DWARF could be
// changed/improved to use some more structural representation)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, didn't notice it was a copy-paste. But it would still be nice to have the comment follow our style guide. So, up to you.

@@ -465,3 +465,15 @@
// MANGLED_TEMP_NAMES: error: unknown argument '-gsimple-template-names=mangled'; did you mean '-Xclang -gsimple-template-names=mangled'
// RUN: %clang -### -target x86_64 -c -g %s 2>&1 | FileCheck --check-prefix=FULL_TEMP_NAMES --implicit-check-not=debug-forward-template-params %s
// FULL_TEMP_NAMES-NOT: -gsimple-template-names
//// Test -g[no-]template-alias (enabled by default with SCE debugger tuning). Requires DWARFv5.
// RUN: %clang -### -target x86_64 -c -gdwarf-5 -gsce %s 2>&1 | FileCheck %s --check-prefixes=TEMPLATE-ALIAS

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

--target

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This gave clang: error: unknown argument '--target'; did you mean '-target'?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh Coulda sworn that was changed... okay.

@OCHyams

But perhaps at least you could add named accessors to DIDerivedType for the template params, same as DIGlobalVariable/DICompositeType have?

Done. I couldn't add an assert (getTag() == dwarf::DW_TAG_template_alias) without including Dwarf.h. I've chosen to not do that as it looks like its been avoided (but possibly just not really needed here before), but I'd be happy to add it if the extra compile time isn't worth worrying about.

(oh, and in case anyone hasn't mentioned it already - this would generally be committed in smaller pieces upstream - adding the LLVM functionality first, then adding clang patches that use that functionality)

Sure, no problem, I'll open separate pull requests once this has been accepted then.

I think I've addressed all the concerns raised now except for the variadic issue I mentioned, which I'll look at tomorrow.

@OCHyams

yeah, probably worth checking the APIs that we use for building the template args for classes and variable templates, they probably have some handling for this - would be good to share/reuse that code, rather than duplicating it, once you find it

I didn't find anything that does what (I think) we need here (but I'm not familiar with the FE, so might've just missed it if it exists. Not for lack of trying). The code I've added looks a bit ugly, but I'm not sure what else to do. See https://godbolt.org/z/nTK4bW1Yn - there doesn't seem to be a TemplateArgument pack for the template alias ClassTemplateSpecializationDecl (while there is one for the struct E in that link), so this is what I came up with instead.

@dwblaikie are you happy with the latest addition mentioned above (plus test) - that's the commits dc5a611 and c9c000f?

@pogo59 are you happy with the flag situation now?

I've separated the LLVM IR side into #88943. I can rebase this to repurpose it for the Clang side once that lands.

@OCHyams

I've rebased this so now this pull request contains only the Clang changes (LLVM side is here #88943).

There's a discussion starting here that discusses whether or not we should include defaulted arguments in the list. The very short summary is that I tried this by scraping the defaults from the template parameter list, but it turns out this is difficult in the face of parameter defaults that are dependent types and values. So the current implementation ignores defaulted arguments, i.e., doesn't include them in the argument list (and as a consequence also omits empty parameter packs that come after defaulted arguments).

This is not ideal, but not a regression from the DW_TAG_typedef names for template aliases which also do not include defaulted arg values.

Michael137

// CHECK: ![[NonDefault]] = !DITemplateTypeParameter(name: "NonDefault", type: ![[int]])
//// FIXME: Ideally, we would describe the deafulted args, like this:
// : ![[extraData]] = !{![[NonDefault:[0-9]+]], ![[T:[0-9]+]], ![[I:[0-9]+]], ![[Ts:[0-9]+]]}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: Is it worth turning these into CHECK-NOT? So when this is fixed in the future we remember that we have these tests ready

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the test will already start failing if this gets fixed because the current extraData metadata tuple contents check will fail (it'll have more than one element). I'm still happy to add CHECK-NOTs if you'd like, just thought I'd raise this in case it changes your stance.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah right, that makes sense, happy keeping it as is

Michael137

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pogo59

@OCHyams

Thanks everyone. I'll land this now then to see what the bots have to say about it.

@OCHyams

Dependent expressions strike again - https://godbolt.org/z/W381837vr

template <int>
using A = int;

template<int I>
struct S {
  using AA = A<I>;
  AA aa;
};

S<0> s;

clang++ -c test.cpp -g -gtemplate-alias
clang/lib/AST/ExprConstant.cpp:15721: bool clang::Expr::EvaluateAsRValue(clang::Expr::EvalResult&, const clang::ASTContext&, bool) const: Assertion '!isValueDependent() && "Expression evaluator can't be called on a dependent expression."' failed.

The issue is I is a dependent expression. We have an actual instantiation of S with the value of I clearly being 0 here, but I'm not sure whether that information exists in the AST for the TemplateSpecializationType *.

Looking into this now (will revert if I can't find a solution).

@OCHyams

@Michael137 said:

Btw, as a follow-up to this patch should we check that this is compatible with dsymutil (i.e., running dsymutil --verify)? I suspect it might need a fixup (given LLDB doesn't even support this tag)

dsymutil --verify seems to be happy with the tag. Upon closer inspection, I think it uses the same "verifier" code as llvm-dwarfdump so that makes sense (#89589).

There's a couple of switches in llvm/lib/DWARFLinker that look like they want a DW_TAG_template_alias:
DependencyTracker::isTypeTableCandidate in DependencyTracker.cpp
AcceleratorRecordsSaver::save in AcceleratorRecordsSaver.cpp. I'm not sure what a test for those would look like but I can look into it if you'd like?

@Michael137

@Michael137 said:

Btw, as a follow-up to this patch should we check that this is compatible with dsymutil (i.e., running dsymutil --verify)? I suspect it might need a fixup (given LLDB doesn't even support this tag)

Yup, dsymutil looks good now, thanks

There's a couple of switches in llvm/lib/DWARFLinker that look like they want a DW_TAG_template_alias:
DependencyTracker::isTypeTableCandidate in DependencyTracker.cpp
AcceleratorRecordsSaver::save in AcceleratorRecordsSaver.cpp. I'm not sure what a test for those would look like but I can look into it if you'd like?

Good question, I'm not familiar with these either, but would make sense to address eventually. Though I don't think it's urgent atm since -gtemplate-alias isn't the default

Reviewers

@adrian-prantl adrian-prantl adrian-prantl left review comments

@Michael137 Michael137 Michael137 approved these changes

@pogo59 pogo59 pogo59 left review comments

@SLTozer SLTozer Awaiting requested review from SLTozer

@dwblaikie dwblaikie Awaiting requested review from dwblaikie