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

Eric Christopher echristo at gmail.com
Wed Jun 26 17:41:41 PDT 2013 

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

Thought I had here was to use the FindDebugInfo code (that's used in a
pass to print out debug info) to verify some of it - basically doing
some of the same things you're talking about though. Specific walk
over to verify the validity of the debug info.

Your idea is also a nice incremental improvement as well.

*shrug*

-eric

> 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