[llvm] r210497 - Removing an "if (!this)" check from two print methods. The condition will

Philip Reames listmail at philipreames.com
Tue Jun 10 10:24:36 PDT 2014


Richard,

Your change looks like it will have the effect of making several 
printing paths less robust against null pointers.  While these aren't 
expected, I have seen cases when debugging that the object had gotten 
corrupted and printing the null value was helpful (instead of 
crashing).  (i.e. some internal field was set to null when it shouldn't be)

Could you extend the callers to print the null message instead of simply 
asserting?  This would preserve existing behaviour of a Debug+Asserts 
build.  (In Release, these would have been optimized away in most cases 
anyways.)

Philip

On 06/09/2014 03:53 PM, Richard Trieu wrote:
> Author: rtrieu
> Date: Mon Jun  9 17:53:16 2014
> New Revision: 210497
>
> URL: http://llvm.org/viewvc/llvm-project?rev=210497&view=rev
> Log:
> Removing an "if (!this)" check from two print methods.  The condition will
> never be true in a well-defined context.  The checking for null pointers
> has been moved into the caller logic so it does not rely on undefined behavior.
>
> Modified:
>      llvm/trunk/lib/Analysis/IPA/CallGraphSCCPass.cpp
>      llvm/trunk/lib/Analysis/IVUsers.cpp
>      llvm/trunk/lib/Analysis/LoopPass.cpp
>      llvm/trunk/lib/Analysis/RegionPass.cpp
>      llvm/trunk/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
>      llvm/trunk/lib/IR/AsmWriter.cpp
>      llvm/trunk/lib/IR/Core.cpp
>      llvm/trunk/lib/Transforms/Instrumentation/DebugIR.cpp
>
> Modified: llvm/trunk/lib/Analysis/IPA/CallGraphSCCPass.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Analysis/IPA/CallGraphSCCPass.cpp?rev=210497&r1=210496&r2=210497&view=diff
> ==============================================================================
> --- llvm/trunk/lib/Analysis/IPA/CallGraphSCCPass.cpp (original)
> +++ llvm/trunk/lib/Analysis/IPA/CallGraphSCCPass.cpp Mon Jun  9 17:53:16 2014
> @@ -602,8 +602,10 @@ namespace {
>   
>       bool runOnSCC(CallGraphSCC &SCC) override {
>         Out << Banner;
> -      for (CallGraphSCC::iterator I = SCC.begin(), E = SCC.end(); I != E; ++I)
> +      for (CallGraphSCC::iterator I = SCC.begin(), E = SCC.end(); I != E; ++I) {
> +        assert((*I)->getFunction() && "Expecting non-null Function");
>           (*I)->getFunction()->print(Out);
> +      }
>         return false;
>       }
>     };
>
> Modified: llvm/trunk/lib/Analysis/IVUsers.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Analysis/IVUsers.cpp?rev=210497&r1=210496&r2=210497&view=diff
> ==============================================================================
> --- llvm/trunk/lib/Analysis/IVUsers.cpp (original)
> +++ llvm/trunk/lib/Analysis/IVUsers.cpp Mon Jun  9 17:53:16 2014
> @@ -287,6 +287,7 @@ void IVUsers::print(raw_ostream &OS, con
>         OS << ")";
>       }
>       OS << " in  ";
> +    assert(UI->getUser() != nullptr && "Expected non-null User");
>       UI->getUser()->print(OS);
>       OS << '\n';
>     }
>
> Modified: llvm/trunk/lib/Analysis/LoopPass.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Analysis/LoopPass.cpp?rev=210497&r1=210496&r2=210497&view=diff
> ==============================================================================
> --- llvm/trunk/lib/Analysis/LoopPass.cpp (original)
> +++ llvm/trunk/lib/Analysis/LoopPass.cpp Mon Jun  9 17:53:16 2014
> @@ -45,6 +45,7 @@ public:
>       for (Loop::block_iterator b = L->block_begin(), be = L->block_end();
>            b != be;
>            ++b) {
> +      assert((*b) != nullptr && "Expecting non-null block");
>         (*b)->print(Out);
>       }
>       return false;
>
> Modified: llvm/trunk/lib/Analysis/RegionPass.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Analysis/RegionPass.cpp?rev=210497&r1=210496&r2=210497&view=diff
> ==============================================================================
> --- llvm/trunk/lib/Analysis/RegionPass.cpp (original)
> +++ llvm/trunk/lib/Analysis/RegionPass.cpp Mon Jun  9 17:53:16 2014
> @@ -195,8 +195,10 @@ public:
>   
>     bool runOnRegion(Region *R, RGPassManager &RGM) override {
>       Out << Banner;
> -    for (const auto &BB : R->blocks())
> +    for (const auto &BB : R->blocks()) {
> +      assert(BB != nullptr && "Expecting non-null Block");
>         BB->print(Out);
> +    }
>   
>       return false;
>     }
>
> Modified: llvm/trunk/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/AsmPrinter/AsmPrinter.cpp?rev=210497&r1=210496&r2=210497&view=diff
> ==============================================================================
> --- llvm/trunk/lib/CodeGen/AsmPrinter/AsmPrinter.cpp (original)
> +++ llvm/trunk/lib/CodeGen/AsmPrinter/AsmPrinter.cpp Mon Jun  9 17:53:16 2014
> @@ -1867,6 +1867,7 @@ static void emitGlobalConstantFP(const C
>       SmallString<8> StrVal;
>       CFP->getValueAPF().toString(StrVal);
>   
> +    assert(CFP->getType() != nullptr && "Expecting non-null Type");
>       CFP->getType()->print(AP.OutStreamer.GetCommentOS());
>       AP.OutStreamer.GetCommentOS() << ' ' << StrVal << '\n';
>     }
>
> Modified: llvm/trunk/lib/IR/AsmWriter.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/IR/AsmWriter.cpp?rev=210497&r1=210496&r2=210497&view=diff
> ==============================================================================
> --- llvm/trunk/lib/IR/AsmWriter.cpp (original)
> +++ llvm/trunk/lib/IR/AsmWriter.cpp Mon Jun  9 17:53:16 2014
> @@ -2156,10 +2156,6 @@ void NamedMDNode::print(raw_ostream &ROS
>   }
>   
>   void Type::print(raw_ostream &OS) const {
> -  if (!this) {
> -    OS << "<null Type>";
> -    return;
> -  }
>     TypePrinting TP;
>     TP.print(const_cast<Type*>(this), OS);
>   
> @@ -2172,10 +2168,6 @@ void Type::print(raw_ostream &OS) const
>   }
>   
>   void Value::print(raw_ostream &ROS) const {
> -  if (!this) {
> -    ROS << "printing a <null> value\n";
> -    return;
> -  }
>     formatted_raw_ostream OS(ROS);
>     if (const Instruction *I = dyn_cast<Instruction>(this)) {
>       const Function *F = I->getParent() ? I->getParent()->getParent() : nullptr;
>
> Modified: llvm/trunk/lib/IR/Core.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/IR/Core.cpp?rev=210497&r1=210496&r2=210497&view=diff
> ==============================================================================
> --- llvm/trunk/lib/IR/Core.cpp (original)
> +++ llvm/trunk/lib/IR/Core.cpp Mon Jun  9 17:53:16 2014
> @@ -281,6 +281,7 @@ char *LLVMPrintTypeToString(LLVMTypeRef
>     std::string buf;
>     raw_string_ostream os(buf);
>   
> +  assert(unwrap(Ty) != nullptr && "Expecting non-null Type");
>     unwrap(Ty)->print(os);
>     os.flush();
>   
> @@ -531,6 +532,7 @@ char* LLVMPrintValueToString(LLVMValueRe
>     std::string buf;
>     raw_string_ostream os(buf);
>   
> +  assert(unwrap(Val) != nullptr && "Expecting non-null Value");
>     unwrap(Val)->print(os);
>     os.flush();
>   
>
> Modified: llvm/trunk/lib/Transforms/Instrumentation/DebugIR.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Instrumentation/DebugIR.cpp?rev=210497&r1=210496&r2=210497&view=diff
> ==============================================================================
> --- llvm/trunk/lib/Transforms/Instrumentation/DebugIR.cpp (original)
> +++ llvm/trunk/lib/Transforms/Instrumentation/DebugIR.cpp Mon Jun  9 17:53:16 2014
> @@ -352,6 +352,7 @@ private:
>     }
>   
>     std::string getTypeName(Type *T) {
> +    assert(T != nullptr && "Expecting non-null Type");
>       std::string TypeName;
>       raw_string_ostream TypeStream(TypeName);
>       T->print(TypeStream);
>
>
> _______________________________________________
> 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