[cfe-commits] Clang DebugInfo Patch for StopPoints.

[Sanjiv.Gupta at microchip.com]

> -----Original Message-----
> From: Chris Lattner [mailto:clattner at apple.com] 
> Sent: Monday, May 05, 2008 12:42 PM
> To: Sanjiv Kumar Gupta - I00171
> Cc: cfe-commits at cs.uiuc.edu
> Subject: Re: [cfe-commits] Clang DebugInfo Patch for StopPoints.
> 
> 
> On May 2, 2008, at 4:39 AM, Sanjiv.Gupta at microchip.com wrote:
> 
> > Hi,
> >
> > Please find the patch attached for stoppoint debug info 
> generation in 
> > clang.
> > It also includes a command line switch called -g.
> > There are two new files to be placed in lib/CodeGen , those 
> files are 
> > attached here.
> 
> This is looking great, some thoughts:
> 
>   //
> =
> =
> =-------------------------------------------------------------
> ---------
> ===//
> +// Code Generator Options
> +//
> =
> =
> =-------------------------------------------------------------
> ---------
> ===//
> +static llvm::cl::opt<bool>
> +GenerateDebugInfo("g",
> +                  llvm::cl::desc("Generate source level debug
> information."));
> 
> It looks like clang is currently inconsistent here, but 
> option descriptions should not end with a ".".
> 
> 
> 
> +++ lib/CodeGen/CGStmt.cpp	(working copy)
> @@ -29,6 +31,16 @@
>   void CodeGenFunction::EmitStmt(const Stmt *S) {
>     assert(S && "Null statement?");
> 
> +  if (S->getStmtClass() != Stmt::CompoundStmtClass) {
> +    if (CGM.TheDebugInfo) {
> 
> Please reduce nesting by using && here for the first two predicates:  
> "if !compoundstmt && thedebuginfo"  Please add a comment 
> indicating why you explicitly exclude compound statements.  
> (they don't generate executable code?)
> 
> +      if (S->getLocStart().isValid()) {
> +        CGM.TheDebugInfo->setLocation (S->getLocStart());
> 
> No space before the '('.
> 
> +      }
> +
> +      CGM.TheDebugInfo->EmitStopPoint(CurFn, 
> Builder.GetInsertBlock());
> +    }
> +  }
> 
> Why does this pass Builder.GetInsertBlock() here?  Wouldn't 
> it be more natural to pass a reference to the builder in instead?
> 
> 
> +++ lib/CodeGen/CodeGenModule.cpp	(working copy)
> @@ -32,13 +33,19 @@
> +  // Debug Info generator.
> +  if (GenerateDebugInfo)
> 
> This comment doesn't add any value.  How about "If debug info 
> generation is enabled, create the CodeGenDebugInfo object 
> which manages this."
> 
> +    TheDebugInfo = new CodeGenDebugInfo (this);  else
> +    TheDebugInfo = NULL;
> 
> No space before (this).
> 
> 
> +  if (TheDebugInfo)
> +    delete TheDebugInfo;
> 
> No need to check for null, delete handles null pointers.
> 
> 
> +++ lib/CodeGen/CodeGenModule.h	(working copy)
> @@ -69,8 +70,10 @@
> 
>     std::vector<llvm::Function *> BuiltinFunctions;
>   public:
> +  CodeGenDebugInfo *TheDebugInfo;
> 
> Please make TheDebugInfo private and add an accessor, 
> following the lead of other instance vars in CodeGenModule.
> 
> //===--- CodeGenDebugInfo.h - DebugInfo for LLVM CodeGen 
> ----------===//
> 
> Please pad this out with '-'s to fill 80 cols.
> 
> 
> #include <map>
> #include <string>
> 
> You don't use "string" in the the header, it can be removed.
> 
> #include "llvm/CodeGen/MachineModuleInfo.h"
> 
> Is there any way to avoid bringing in MachineModuleInfo.h?  
> It is a really gross header that needs a lot of cleanup.  Can 
> you avoid #including it by forward declaring what ever you 
> need and then only #including it into CodeGenDebugInfo.cpp?  
> I think it is worth it, even if it means making 'SR' by a 
> pointer instead of holding the DISerializer by value.
> 
>    std::map<unsigned, llvm::CompileUnitDesc *> CompileUnitCache;
>                                          // Cache of 
> previously constructed
>                                          // CompileUnits.
> 
> Please comment this with a doxygen-style comment:
> 
>    /// CompileUnitCache - Cache of previously constructed 
> CompileUnits.
>    std::map<unsigned, llvm::CompileUnitDesc *> CompileUnitCache;
> 
> 
> 
> There are a couple more places where you put spaces before '('.
> 
> 
> 
>    llvm::CompileUnitDesc *getOrCreateCompileUnit(const 
> SourceLocation loc);
> 
> You pass Loc by value, so no need for it to be const.  Please 
> use 'Loc' instead of 'loc' for consistency.
> 
> 
> 
> //===--- CodeGenDebugInfo.cpp - Emit Debug Information for a 
> Module --------===//
> 
> This is one char to long.  I'm picky, huh? :)
> 
> 
> #include "CodeGenDebugInfo.h"
> 
> #include "CodeGenModule.h"
> 
> #include "llvm/Constants.h"
> #include "llvm/DerivedTypes.h"
> #include "llvm/Instructions.h"
> #include "llvm/Intrinsics.h"
> #include "llvm/Module.h"
> #include "llvm/Support/Dwarf.h"
> #include "llvm/Target/TargetMachine.h"
> #include "llvm/ADT/StringExtras.h"
> #include "llvm/ADT/SmallVector.h"
> #include "llvm/CodeGen/MachineModuleInfo.h"
> 
> #include "clang/Basic/SourceManager.h"
> #include "clang/Basic/FileManager.h"
> #include "clang/AST/ASTContext.h"
> 
> 
> 
> using namespace clang;
> using namespace clang::CodeGen;
> 
> The vertical spacing here is pretty arbitrary and doesn't add 
> much value, please eliminate it.
> 
> 
>   // FIX_ME: Do not know how to get clang version yet.
> 
> Please use "FIXME" instead of "FIX_ME" for consistency with 
> the rest of the codebase.
> 
> void
> CodeGenDebugInfo::EmitStopPoint(llvm::Function *Fn, llvm::BasicBlock
> *CurBB) {
>    // Don't bother if things are the same as last time.
>    if (CurLoc == PrevLoc && PrevBB == CurBB)
>      return;
> 
> Why are you checking for BB equality here?  Wouldn't it be 
> more reasonable to check that the line # is the same instead 
> of the full location?  This would emit multiple stop points 
> for things like:
> 
>    a = b; c = d;
> 
> right?
> 
> 
>    // Invoke llvm.dbg.stoppoint
>    llvm::Value *Args[3] = {
>      llvm::ConstantInt::get(llvm::Type::Int32Ty, CurLineNo),
>      llvm::ConstantInt::get(llvm::Type::Int32Ty, (uint64_t) 0),
>      getCastValueFor(Unit)
>    };
>    llvm::CallInst::Create(StopPointFn, Args, Args+3, "", CurBB);
> 
> This should use the LLVMBuilder and use the .CreateCall3 method.  
> Likewise for the EmitRegionStart method etc.
> 
> 
> 
>    // Provide an region stop point.
>    EmitStopPoint(Fn, CurBB);
> 
> 
> Why emit a stop point on "}"?
> 
> Please resend the updated patch.  I'm sorry for the delay, I 
> will try not to delay as much on the next iteration.  Thanks 
> for working on this!
> 
> -Chris
> 

Thanks for your inputs Chris. I am working on these.

-Sanjiv