[llvm] r185020 - Debug Info: clean up usage of Verify.
David Blaikie
dblaikie at gmail.com
Wed Jun 26 22:07:40 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.
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).
> 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