[cfe-commits] Clang DebugInfo Patch for StopPoints.

[Chris Lattner]

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