[llvm] r185020 - Debug Info: clean up usage of Verify.
Manman Ren
mren at apple.com
Wed Jun 26 17:36:12 PDT 2013
On Jun 26, 2013, at 5:23 PM, David Blaikie wrote:
> 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.
For now, does it make sense to just hook up DI's Verify to our IR Verifier?
Specifically, in IR Verifier, when we are visiting a MDNode, check whether it is a DI node, if it is, call the Verify function.
Thanks,
Manman
>
> - 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