[llvm] r185020 - Debug Info: clean up usage of Verify.
Manman Ren
mren at apple.com
Wed Jun 26 17:10:28 PDT 2013
On Jun 26, 2013, at 2:37 PM, David Blaikie wrote:
> On Wed, Jun 26, 2013 at 2:26 PM, Manman Ren <mren at apple.com> wrote:
>> Author: mren
>> Date: Wed Jun 26 16:26:10 2013
>> New Revision: 185020
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=185020&view=rev
>> Log:
>> Debug Info: clean up usage of Verify.
>>
>> No functionality change.
>> It should suffice to check the type of a debug info metadata, instead of
>> calling Verify.
>>
>> Modified:
>> llvm/trunk/lib/Target/NVPTX/NVPTXAsmPrinter.cpp
>> llvm/trunk/lib/Transforms/Instrumentation/GCOVProfiling.cpp
>> llvm/trunk/lib/Transforms/Utils/Local.cpp
>> llvm/trunk/tools/opt/opt.cpp
>>
>> Modified: llvm/trunk/lib/Target/NVPTX/NVPTXAsmPrinter.cpp
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/NVPTX/NVPTXAsmPrinter.cpp?rev=185020&r1=185019&r2=185020&view=diff
>> ==============================================================================
>> --- llvm/trunk/lib/Target/NVPTX/NVPTXAsmPrinter.cpp (original)
>> +++ llvm/trunk/lib/Target/NVPTX/NVPTXAsmPrinter.cpp Wed Jun 26 16:26:10 2013
>> @@ -279,7 +279,7 @@ void NVPTXAsmPrinter::emitLineNumberAsDo
>> const LLVMContext &ctx = MF->getFunction()->getContext();
>> DIScope Scope(curLoc.getScope(ctx));
>>
>> - if (!Scope.Verify())
>> + if (!Scope.isScope())
>> return;
>
> Could this one just be an assert? (all debug location descriptions
> should have a scope, right? (or do ones at the top level not have any
> scope? - in that case maybe this should just be a "is not null" check
> (& then a "isScope" assert) rather than an isScope check)
>
>>
>> StringRef fileName(Scope.getFilename());
>>
>> Modified: llvm/trunk/lib/Transforms/Instrumentation/GCOVProfiling.cpp
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Instrumentation/GCOVProfiling.cpp?rev=185020&r1=185019&r2=185020&view=diff
>> ==============================================================================
>> --- llvm/trunk/lib/Transforms/Instrumentation/GCOVProfiling.cpp (original)
>> +++ llvm/trunk/lib/Transforms/Instrumentation/GCOVProfiling.cpp Wed Jun 26 16:26:10 2013
>> @@ -434,7 +434,7 @@ void GCOVProfiler::emitProfileNotes() {
>> DIArray SPs = CU.getSubprograms();
>> for (unsigned i = 0, e = SPs.getNumElements(); i != e; ++i) {
>> DISubprogram SP(SPs.getElement(i));
>> - if (!SP.Verify()) continue;
>> + assert(SP.isSubprogram());
>
> This one may be problematic for TUs with no subprograms - since
> metadata cannot have zero element entries, the DIArray (if you look in
> the metadata you'll see this regularly) is has a single i32 0 element.
> It might be necessary to check for & skip that particular case in some
> way (there are a few ways we could skip this special case).
>
>>
>> Function *F = SP.getFunction();
>> if (!F) continue;
>> @@ -483,7 +483,7 @@ bool GCOVProfiler::emitProfileArcs() {
>> SmallVector<std::pair<GlobalVariable *, MDNode *>, 8> CountersBySP;
>> for (unsigned i = 0, e = SPs.getNumElements(); i != e; ++i) {
>> DISubprogram SP(SPs.getElement(i));
>> - if (!SP.Verify()) continue;
>> + assert(SP.isSubprogram());
>> Function *F = SP.getFunction();
>> if (!F) continue;
>> if (!Result) Result = true;
>>
>> Modified: llvm/trunk/lib/Transforms/Utils/Local.cpp
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Utils/Local.cpp?rev=185020&r1=185019&r2=185020&view=diff
>> ==============================================================================
>> --- llvm/trunk/lib/Transforms/Utils/Local.cpp (original)
>> +++ llvm/trunk/lib/Transforms/Utils/Local.cpp Wed Jun 26 16:26:10 2013
>> @@ -854,7 +854,7 @@ static bool LdStHasDebugValue(DIVariable
>> bool llvm::ConvertDebugDeclareToDebugValue(DbgDeclareInst *DDI,
>> StoreInst *SI, DIBuilder &Builder) {
>> DIVariable DIVar(DDI->getVariable());
>> - if (!DIVar.Verify())
>> + if (!DIVar.isVariable())
>
> This seems like it should just be an assert, no?
The bot failed because of the above change, there is an error in the testing case itself:
test/Transforms/InstCombine/debuginfo.ll
!10 = metadata !{i32 589846, metadata !3, metadata !"size_t", metadata !2, i32 80, i64 0, i64 0, i64 0, i32 0, metadata !11} ; [ DW_TAG_typedef ]
!3 = metadata !{i32 786449, i32 0, i32 12, metadata !26, metadata !"clang version 3.0 (trunk 127710)", i1 true, metadata !"", i32 0, null, null, metadata !24, null, null} ; [ DW_TAG_compile_unit ]
!2 = metadata !{i32 786473, metadata !27} ; [ DW_TAG_file_type ]
The format of a typedef should be "tag, file node, context, name …", we have "tag, context, name, file node ..." in the testing case.
Manman
>
>> return false;
>>
>> if (LdStHasDebugValue(DIVar, SI))
>> @@ -888,7 +888,7 @@ bool llvm::ConvertDebugDeclareToDebugVal
>> bool llvm::ConvertDebugDeclareToDebugValue(DbgDeclareInst *DDI,
>> LoadInst *LI, DIBuilder &Builder) {
>> DIVariable DIVar(DDI->getVariable());
>> - if (!DIVar.Verify())
>> + if (!DIVar.isVariable())
>
> And here.
>
>> return false;
>>
>> if (LdStHasDebugValue(DIVar, LI))
>> @@ -961,7 +961,7 @@ bool llvm::replaceDbgDeclareForAlloca(Al
>> if (!DDI)
>> return false;
>> DIVariable DIVar(DDI->getVariable());
>> - if (!DIVar.Verify())
>> + if (!DIVar.isVariable())
>
> And here (are we expecting anything other than a variable? Or is it
> possible that there's no variable attached? In which case maybe a
> non-null check (if (!DIVar)) would be more appropriate (& an assert
> that it's actually a variable))
>
>> return false;
>>
>> // Create a copy of the original DIDescriptor for user variable, appending
>>
>> Modified: llvm/trunk/tools/opt/opt.cpp
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/tools/opt/opt.cpp?rev=185020&r1=185019&r2=185020&view=diff
>> ==============================================================================
>> --- llvm/trunk/tools/opt/opt.cpp (original)
>> +++ llvm/trunk/tools/opt/opt.cpp Wed Jun 26 16:26:10 2013
>> @@ -389,8 +389,8 @@ struct BreakpointPrinter : public Module
>> for (unsigned i = 0, e = NMD->getNumOperands(); i != e; ++i) {
>> std::string Name;
>> DISubprogram SP(NMD->getOperand(i));
>> - if (SP.Verify())
>> - getContextName(SP.getContext(), Name);
>
> This may have the same problem as the previous loop I mentioned - the
> empty array case may still have an i32 0 that needs to be ignored.
> (don't take my word for it - please create test cases for these
> situations derived from actual clang output & demonstrated assertion
> failures)
>
>> + assert(SP.isSubprogram());
>> + getContextName(SP.getContext(), Name);
>> Name = Name + SP.getDisplayName().str();
>> if (!Name.empty() && Processed.insert(Name)) {
>> Out << Name << "\n";
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
More information about the llvm-commits
mailing list