This patch provides support for DW_AT_default_value based on template parameter is default parameter or not.
Details
- Reviewers
probinson aprantl dblaikie - Commits
- rG1cb0e01e42ca: [DebugInfo][DWARF5]: Added support for debuginfo generation for defaulted…
rG7a42babeb83e: Reland "[DebugInfo][clang][DWARF5]: Added support for debuginfo generation for…
rGc2b437d53d40: [DebugInfo][clang][DWARF5]: Added support for debuginfo generation for…
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
llvm/include/llvm/IR/DebugInfoMetadata.h | ||
---|---|---|
2158 | What about default arguments for non-type template parameters? ("template<int i = 3>" etc... ) |
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.
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? |
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). |
Hi @dblaikie, I have incorporated your suggestions (renaming variables, test case modification).
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.
@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
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. |
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. |
Hi @dblaikie, I removed the condition and put false as the third argument of PrintBool. I have also removed the updated bitcode file.
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? ; 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. |
@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)
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. |
Hi @aprantl, I have added a test that two DITemplateTypeParameter only differ in the isDefault parameter are not equal.
llvm/include/llvm/IR/DebugInfoMetadata.h | ||
---|---|---|
2187 | clang-format, please. | |
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: | |
2081 | get(Context, Name, Type, defaulted); | |
2086 | same here ... | |
2091 | This can stay. |
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 |
This change breaks lldb buildbots here:
http://lab.llvm.org:8011/builders/lldb-aarch64-ubuntu/builds/2123
http://lab.llvm.org:8011/builders/lldb-x86_64-debian/builds/5911
Please revisit this.
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
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)
...
@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.
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 )