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

Eric Christopher echristo at gmail.com
Thu Jun 27 10:22:03 PDT 2013 

On Thu, Jun 27, 2013 at 10:03 AM, Manman Ren <mren at apple.com> wrote:
>
> 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.
>

Should be at least a start down the path.

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

Whee. Not surprising, mind you.

-eric

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