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

[David Blaikie]

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?

>      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