Page MenuHomePhabricator

[dwarf-5] Support DebugInfo for Defaulted parameters for C++ templates
ClosedPublic

Authored by awpandey on Jan 27 2020, 5:01 AM.

Diff Detail

Event Timeline

awpandey created this revision.Jan 27 2020, 5:01 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJan 27 2020, 5:01 AM

Can you add a bitcode roundtrip test to test/Assembler?

dblaikie added inline comments.Jan 27 2020, 11:20 AM
llvm/include/llvm/IR/DebugInfoMetadata.h
2158

What about default arguments for non-type template parameters? ("template<int i = 3>" etc... )

awpandey updated this revision to Diff 241141.Jan 29 2020, 6:42 AM

Thanks @dblaikie for figuring out the missing feature. I have added support for the non-type template parameter too. Please let me know if further modifications are required. I will happily address them.

dblaikie added inline comments.Jan 29 2020, 1:25 PM
llvm/include/llvm/IR/DebugInfoMetadata.h
2134

Rename this (& the ctor/other parameters) to "IsDefault" to conform to LLVM's naming conventions ( https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly )

llvm/lib/AsmParser/LLParser.cpp
4861–4862

defaultValue should probably be rendered/encoded as a boolean, rather than a string?

(& "defaultValue" might be a misleading name (if the DITemplateValueParameter was a boolean non type template parameter, then "defaultValue: true" could look like the default value is true, not that the "value: "parameter is the default value... if that makes sense) - perhaps "defaulted" would read more easily ("defaulted: true" or "defaulted: false"))

Perhaps just "default" though that still feels a bit ambiguous between "is this the default value itself, or is it specifying that the "value:" field is the default value?".

llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
1808–1809

I don't think we should be using the DWARF version to decide on the schema - there's no other use of that technique in the parsing/writing code & I can think of some ways it might go poorly.

Better to encode it & it can be dropped during actual DWARF emission in the backend if the version doesn't support it.

llvm/test/DebugInfo/X86/debug-info-template-parameter.ll
1 ↗(On Diff #241141)

An implicit-check-not of DW_TAG and of DW_AT_default_value would probably help make the test better constrained/less likely to pass accidentally/in unintended ways.

(though the DW_TAG check not might be too much/add too much noise outside the types intended to be checked (you'd need to add every DW_TAG produced by the test... maybe there's a way to simplify the uses of the types so the usage doesn't need to create many more tags... )

5 ↗(On Diff #241141)

Perhaps you could swap one of these defaulted template parameters to be a non-type template parameter to broaden the demonstration a bit?

Or, alternatively, does having two parameters here especially broaden the test coverage in some way? Or would one be sufficient?

dblaikie added inline comments.Jan 29 2020, 1:35 PM
llvm/lib/AsmParser/LLParser.cpp
4861–4862

I see that the actual DWARFv5 feature is "DW_AT_default_value" - so I guess sticking with that is good, even though it's not a great name (& indeed elsewhere in DWARF this same attribute name is used with a value that represents the default value, not a boolean about whether the default value is elsewhere... :/)

Perhaps we should push the name further up then - into the API, (IsDefaultValue member, maybe make the accessor "isDefaultValue" rather than "getDefaultValue" to avoid some of the ambiguity/confusion, etc).

awpandey updated this revision to Diff 241391.Jan 30 2020, 3:21 AM
awpandey marked 7 inline comments as done.

Hi @dblaikie, I have incorporated your suggestions (renaming variables, test case modification).

awpandey marked an inline comment as done and an inline comment as not done.Jan 30 2020, 3:22 AM
awpandey added inline comments.
llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
1808–1809

I am doing this for making record structure consistent with previous dwarf version.

In addition to the roundtrip test, can you please also add a .bc file test containing a DITemplateTypeParameter / DITemplateValueParameter produced by today's llvm to test the bitcode upgrade code added to MetadataLoader? You'll find examples if you search for .bc files in the existing tests.

awpandey marked an inline comment as not done.Jan 30 2020, 10:41 PM

@aprantl Thanks for your valuable feedback.

I have a doubt. You need a .ll file its corresponding llvm bytecode generated by current llvm (without my patch). Doest this ll file includes any checks? In the round trip test case I have to do llvm-as followed by llvm-dis then performs checks or something else?

@aprantl Thanks for your valuable feedback.

I have a doubt. You need a .ll file its corresponding llvm bytecode generated by current llvm (without my patch). Doest this ll file includes any checks? In the round trip test case I have to do llvm-as followed by llvm-dis then performs checks or something else?

For an example of a bitcode upgrade test check out
https://github.com/llvm/llvm-project/blob/master/llvm/test/Bitcode/DIExpression-4.0.ll
https://github.com/llvm/llvm-project/blob/master/llvm/test/Bitcode/DIExpression-4.0.ll.bc

for example of a round-trip test check out
https://github.com/llvm/llvm-project/blob/master/llvm/test/Assembler/dicompileunit.ll

dblaikie added inline comments.Jan 31 2020, 10:01 AM
llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
1808–1809

There's no need/desire to make the record structure consistent with previous versions of DWARF.

The only consistency we have is the bitcode backwards compatibility itself.

Please remove these DWARFv5 tests.

awpandey updated this revision to Diff 241984.Feb 3 2020, 1:16 AM
awpandey marked 2 inline comments as done.

Thanks, @aprantl, and @dblaikie. I have added two new test cases as per your suggestions. I have to update dityperefs-3.8.ll and dityperefs-3.8.ll.bc because they are no longer compatible with this patch.

Might be easier in smaller patches - Bitcode & IR support, then LLVM/codegen emission and Clang/IR gen.

I have to update dityperefs-3.8.ll and dityperefs-3.8.ll.bc because they are no longer compatible with this patch.

The bitcode files in test/Bitcode generally shouldn't be updated - they're there to test backwards compatibility with older bitcode.

llvm/lib/IR/AsmWriter.cpp
2076–2077

Remove this condition & emit defaulted unconditionally.

Include the "default" value of false as the 3rd argument to printBool so we don't add this textual element to every DITemplateTypeParameter.

2091–2092

Same feedback as with the type parameter case.

awpandey updated this revision to Diff 242250.Feb 3 2020, 8:27 PM

Hi @dblaikie, I removed the condition and put false as the third argument of PrintBool. I have also removed the updated bitcode file.

awpandey marked 2 inline comments as done.Feb 3 2020, 8:27 PM
dblaikie added inline comments.Feb 4 2020, 7:54 PM
clang/test/CodeGenCXX/debug-info-template-parameter.cpp
30–32 ↗(On Diff #242250)

What are these three (6 if you include the fact that each foo has one default template parameter still) testing? Could it use fewer tests.

Perhaps something similar to the IR/LLVM test - one instantiation with a default type parameter and a default non-type parameter, and one instantiation with non-defaults?

llvm/lib/IR/AsmWriter.cpp
2091

This comment ("ShouldSkipFalse") is incorrect, perhaps "Default=" would be a better comment.

llvm/test/Assembler/DITemplateParameter.ll
47–51 ↗(On Diff #242250)

Probably want to check that these two don't have a "defaulted: true" (or other "defaulted: ") parameter?
Probably the easiest way is to positively check for the fields that are there:

; CHECK: = !DITemplateTypeParameter(name: "T", type: !{{[0-9]*}})
; CHECK: = !DITemplateTypeParameter(name: "i", type: !{{[0-9]*}}, value: i32 6)
llvm/test/DebugInfo/X86/debug-info-template-parameter.ll
80–81 ↗(On Diff #242250)

Could probably remove the "Defaulted: false" values here, since they're the default.

awpandey updated this revision to Diff 242514.Feb 4 2020, 9:42 PM
awpandey marked 3 inline comments as done.

@dblaikie, I have updated the test cases.

awpandey marked an inline comment as done.Feb 4 2020, 9:44 PM

@aprantl - functionality looks fine to me, could you check/sign off on the bitcode backwards compatibility stuff? (& ultimately this should probably be committed in 3 patches - one for the IR support, then one for Clang to generate that IR, and another for LLVM to use it to produce the relevant DWARF)

Thanks @dblaikie for all of your suggestions. I will commit this in 3 patches after getting the green signal from @aprantl.

Hi @aprantl , can you please inform me what extra changes should I make in this patch.

The upgrade test look good, I found a missing test in the unit tests though.

llvm/unittests/IR/MetadataTest.cpp
2090

I think you also need to add a new line here that tests that two DITemplateTypeParameter that only differ in the isDefault parameter are not equal. This tests the hashing/comparison mechanism.

2121–2127

and here.

awpandey updated this revision to Diff 244571.Feb 13 2020, 10:26 PM
awpandey marked 2 inline comments as done.

Hi @aprantl, I have added a test that two DITemplateTypeParameter only differ in the isDefault parameter are not equal.

Hi @aprantl, I have included all of your suggestions. Can I merge this?

aprantl added inline comments.Feb 19 2020, 1:23 PM
llvm/include/llvm/IR/DebugInfoMetadata.h
2187

clang-format, please.
(MDString *Name

llvm/lib/Bitcode/Reader/MetadataLoader.cpp
1685
MetadataList.assignValue(
          GET_OR_DISTINCT(DITemplateTypeParameter,
                          (Context, getMDString(Record[1]),
                           getDITypeRefOrNull(Record[2]), false)),
                         Record.size() == 4 ? getMDOrNull(Record[3]) : false),
           NextMetadataNo);
1712

Same here.

llvm/test/Bitcode/DITemplateParameter-5.0.ll
51

I think you can reduce the size of the binary bitcode file significantly by just having a raw TemplateParameter in the file, like so:

!named = !{!0, !1}
!0 = !DITemplateTypeParameter(name: "T", type: !10)
!1 = !DIBasicType(name: "char", size: 8, encoding: DW_ATE_signed_char)
llvm/unittests/IR/MetadataTest.cpp
2078–2079

For symmetry:
bool defaulted = false;

2081

get(Context, Name, Type, defaulted);

2086

same here ...

2091

This can stay.

aprantl accepted this revision.Feb 19 2020, 1:24 PM

LGTM with outstanding inline comments addressed.

This revision is now accepted and ready to land.Feb 19 2020, 1:24 PM
awpandey updated this revision to Diff 245875.Feb 21 2020, 8:28 AM
awpandey marked 7 inline comments as done.

LGTM

llvm/unittests/IR/MetadataTest.cpp
2107

For consistency, let's assign "false" to bool Defaulted = false and use that instead of hardcoding "false" here

awpandey updated this revision to Diff 247546.Mar 1 2020, 8:48 PM
awpandey marked an inline comment as done.
This revision was automatically updated to reflect the committed changes.
This revision is now accepted and ready to land.Mar 2 2020, 12:25 AM
hans added a subscriber: hans.Mar 2 2020, 12:31 AM

In addition to that, looks like the Bitcode/DITemplateParameter-5.0.ll.bc file was never checked in which is causing all bots to fail, e.g. http://lab.llvm.org:8011/builders/clang-x86_64-debian-fast/builds/24492

I've reverted in 802b22b5c8c30bebc1695a217478be02653c6b53

hans added a comment.Mar 2 2020, 1:14 AM

I also just noticed that this broke the Chromium build, see the attached reproducer at https://bugs.chromium.org/p/chromium/issues/detail?id=1057559#c1

clang++: /work/llvm.monorepo/clang/lib/AST/ExprConstant.cpp:14013: llvm::APSInt clang::Expr::EvaluateKnownConstInt(const clang::ASTContext &, SmallVectorImpl<clang::PartialDiagnosticAt> *) const: Assertion `!isValueDependent() && "Expression evaluator can't be called on a dependent expression."' failed.

#10 0x00000000046eb71d clang::Expr::EvaluateKnownConstInt(clang::ASTContext const&, llvm::SmallVectorImpl<std::pair<clang::SourceLocation, clang::PartialDiagnostic> >*) const (../../../../llvm.monorepo/build.release/bin/clang+++0x46eb71d)
#11 0x00000000027875de clang::CodeGen::CGDebugInfo::CollectTemplateParams(clang::TemplateParameterList const*, llvm::ArrayRef<clang::TemplateArgument>, llvm::DIFile*) (../../../../llvm.monorepo/build.release/bin/clang+++0x27875de)
#12 0x000000000278f67b clang::CodeGen::CGDebugInfo::CreateLimitedType(clang::RecordType const*) (../../../../llvm.monorepo/build.release/bin/clang+++0x278f67b)
#13 0x000000000278a2d1 clang::CodeGen::CGDebugInfo::getOrCreateLimitedType(clang::RecordType const*, llvm::DIFile*) (../../../../llvm.monorepo/build.release/bin/clang+++0x278a2d1)
#14 0x000000000278980b clang::CodeGen::CGDebugInfo::CreateTypeDefinition(clang::RecordType const*) (../../../../llvm.monorepo/build.release/bin/clang+++0x278980b)
#15 0x000000000277eb15 clang::CodeGen::CGDebugInfo::getOrCreateType(clang::QualType, llvm::DIFile*) (../../../../llvm.monorepo/build.release/bin/clang+++0x277eb15)
#16 0x0000000002782c58 clang::CodeGen::CGDebugInfo::CreateType(clang::TypedefType const*, llvm::DIFile*) (../../../../llvm.monorepo/build.release/bin/clang+++0x2782c58)
#17 0x000000000277eb15 clang::CodeGen::CGDebugInfo::getOrCreateType(clang::QualType, llvm::DIFile*) (../../../../llvm.monorepo/build.release/bin/clang+++0x277eb15)
#18 0x0000000002782a5f clang::CodeGen::CGDebugInfo::CreateType(clang::TypedefType const*, llvm::DIFile*) (../../../../llvm.monorepo/build.release/bin/clang+++0x2782a5f)
#19 0x000000000277eb15 clang::CodeGen::CGDebugInfo::getOrCreateType(clang::QualType, llvm::DIFile*) (../../../../llvm.monorepo/build.release/bin/clang+++0x277eb15)
#20 0x000000000279a13d clang::CodeGen::CGDebugInfo::EmitExplicitCastType(clang::QualType) (../../../../llvm.monorepo/build.release/bin/clang+++0x279a13d)
#21 0x0000000002a26632 clang::StmtVisitorBase<std::add_pointer, (anonymous namespace)::ScalarExprEmitter, llvm::Value*>::Visit(clang::Stmt*) (../../../../llvm.monorepo/build.release/bin/clang+++0x2a26632)
#22 0x0000000002a32e14 (anonymous namespace)::ScalarExprEmitter::VisitCastExpr(clang::CastExpr*) (../../../../llvm.monorepo/build.release/bin/clang+++0x2a32e14)
#23 0x0000000002a1e3c5 clang::CodeGen::CodeGenFunction::EmitScalarExpr(clang::Expr const*, bool) (../../../../llvm.monorepo/build.release/bin/clang+++0x2a1e3c5)
#24 0x00000000028e8257 clang::CodeGen::CodeGenFunction::EmitAtomicExpr(clang::AtomicExpr*) (../../../../llvm.monorepo/build.release/bin/clang+++0x28e8257)
...

This revision was automatically updated to reflect the committed changes.
vsk added a subscriber: vsk.Mar 18 2020, 11:37 AM

@awpandey @SouraVX this is still breaking the stage2 clang self-host with -gdwarf-5, see https://bugs.llvm.org/show_bug.cgi?id=45166. Do you have time to take a look?

This wasn't well committed, unfortunately.

It seems it was committed in two parts:

IR support and clang functionality: https://github.com/llvm/llvm-project/commit/7a42babeb83e3927e89e72a0e7e45be9d41b6c23 (a recommit of https://github.com/llvm/llvm-project/commit/c2b437d53d40b6dc5603c97f527398f477d9c5f1 )
LLVM functionality (lib/CodeGen/AsmPrinter) and a clang test: https://github.com/llvm/llvm-project/commit/1cb0e01e42ca5e9de44d9b7cb03d23552a9a9ae1

It's still missing the LLVM test & this was not the right division for committing them. In the future, perhaps it's best to go through separate reviews to validate the individual commits after the general review is done.

The Clang functionality and clang test should've been committed together, but separately from everything else (after the IR support was in)
The LLVM functionality should've been committed separately but with testing, which is not present at all.

The test file llvm/test/DebugInfo/X86/debug-info-template-parameter.ll that was in the review seems to have never been committed & this might've been more obvious if things were separated as intended/per protocol - please commit it at your earliest convenience.