[llvm] r185020 - Debug Info: clean up usage of Verify.
David Blaikie
dblaikie at gmail.com
Wed Jun 26 17:23:30 PDT 2013
On Wed, Jun 26, 2013 at 5:10 PM, Manman Ren <mren at apple.com> wrote:
>
> 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.
Don't be too surprised if you run across tests with invalid debug info
metadata - I made a bunch of schema changes (Including removing
versioning) earlier this year & only updated failing tests. At some
point we should implement a proper debug info verifier & ensure all
tests have correct/valid debug info metadata, but that hasn't happened
yet.
- David
> 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