[llvm] r185020 - Debug Info: clean up usage of Verify.

Manman Ren mren at apple.com
Thu Jun 27 10:03:04 PDT 2013 

On Jun 26, 2013, at 10:07 PM, David Blaikie wrote:

> On Wed, Jun 26, 2013 at 5:36 PM, Manman Ren <mren at apple.com> wrote:
>> 
>> 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.
> 
> I'd rather not rely on Verify-like behavior to decide whether
> something is debug info metadata, it seems too prone to accidentally
> diagnosing other, unrelated metadata as debug info metadata. What I'd
> personally prefer (though I haven't tried this & there might be lots
> of good reasons not to do it this way) would be to walk the debug info
> from the same roots that the actual debug info code does - from
> dbglocs attached to instructions, from the llvm.dbg.cu root, and from
> the various debug intrinsics (declare/value).

Yes, that is more reliable, given that we don't have a special code for each kind of metadata.
And I assume DebugInfoFinder does what you suggested, even though I have not looked at the code yet.

BTW, I observed around 100 testing cases with verification errors.

Manman

> 
>> 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