[cfe-commits] [PATCH] llvm.annotation support

[John McCall]

On Jun 14, 2011, at 12:14 AM, Julien Lerouge wrote:
> Attached is a patch that brings back annotation support to the state it
> was (or close) with llvm-gcc. The patch adds the __builtin_annotation
> and support for annotating struct fields and local variables (support
> for globals was already in there).
> 
> I moved all the code related to those annotations in CGAnnotations.h/cpp
> The other changes are minor tweaks for the annotate attribute and a
> bunch of tests.

As a general note, this looks pretty good;  I just have a few organizational
and data-structure comments.

diff --git a/lib/CodeGen/CGAnnotations.cpp b/lib/CodeGen/CGAnnotations.cpp

+static const char *AnnotationSection = "llvm.metadata";

This should be a #define;  every single place you use this in the file ends up having to call strlen completely unnecessarily.

(it could be a const llvm::StringRef, except that would potentially leave a global ctor around, unfortunately)

+llvm::Constant *CGAnnotations::EmitAnnotationString(llvm::StringRef Str) const {
+  llvm::Module &M = CGM.getModule();
+  llvm::LLVMContext &VMContext = CGM.getLLVMContext();
+
+  llvm::Constant *s = llvm::ConstantArray::get(VMContext, Str, true);
+  for (llvm::Value::use_iterator i = s->use_begin(), e = s->use_end(); i != e;
+    ++i) {
+    llvm::GlobalVariable *gv = dyn_cast<llvm::GlobalVariable>(*i);
+    if (!gv || gv->getSection() != AnnotationSection)
+      continue;
+    return gv;
+  }
+
+  // Not found yet, create a new global.
+  llvm::GlobalValue *gv = new llvm::GlobalVariable(M, s->getType(), true,
+    llvm::GlobalValue::PrivateLinkage, s, ".str");
+  gv->setSection(AnnotationSection);
+  gv->setUnnamedAddr(true);
+  return gv;
+}

Building a ConstantArray from a string is very expensive.  Unless you
expect annotations to almost never collide — and since you generate
these strings for each filename, that seems pretty unlikely — or you
need to make sure you unique them properly with user variables
placed specifically in the annotations section, please use an
llvm::StringMap<llvm::Constant*>.

+llvm::Value *CGAnnotations::EmitAnnotationCall(llvm::Value *AnnotationFn,
+                                               llvm::Value *AnnotatedVal,
+                                               llvm::StringRef AnnotationStr,
+                                               SourceLocation Location,
+                                               CGBuilderTy &Builder) const {
+  llvm::Value *Args[4];
+  Args[0] = AnnotatedVal;
+  Args[1] = Builder.CreateBitCast(EmitAnnotationString(AnnotationStr),
+                                  CGM.Int8PtrTy);
+  Args[2] = Builder.CreateBitCast(EmitAnnotationUnit(Location), CGM.Int8PtrTy);
+  Args[3] = EmitAnnotationLineNo(Location);
+  return Builder.CreateCall(AnnotationFn, Args, Args + 4);
+}

This should probably be a method of CodeGenFunction.

+llvm::Value *CGAnnotations::EmitBuiltinAnnotation(CodeGenFunction &CGF,
+                                                  const CallExpr *E) const {

-> CodeGenFunction

+  CGBuilderTy &Builder = CGF.Builder;
+  llvm::Value *AnnVal = CGF.EmitScalarExpr(E->getArg(0));
+
+  // Get the intrinsic.
+  const llvm::Type *ArgTy = AnnVal->getType();
+  llvm::Value *F = CGM.getIntrinsic(llvm::Intrinsic::annotation, &ArgTy, 1);
+
+  // Get the anntotation string.
+  const Expr *AnnotationStrExpr = E->getArg(1);
+  while (const CastExpr *CE = dyn_cast<CastExpr>(AnnotationStrExpr))
+    AnnotationStrExpr = CE->getSubExpr();

A better way to do this is with E->getArg(1)->IgnoreParenCasts(), unless for
some reason you really don't want to ignore parentheses.  However, the
validation code for this in Sema rejects anything that isn't directly a
StringLiteral, so it looks like either that should be made more lenient or
this code can be made more strict.

+  llvm::StringRef Str = cast<StringLiteral>(AnnotationStrExpr)->getString();
+
+  return EmitAnnotationCall(F, AnnVal, Str, E->getExprLoc(), Builder);
+}

+void CGAnnotations::EmitVarAnnotations(const VarDecl *D, llvm::Value *V,
+                                       CGBuilderTy &Builder) const {

-> CodeGenFunction

+  assert(D->hasAttr<AnnotateAttr>() && "no annotate attribute");
+  for (ann_iterator ai = ann_begin(D), ae = ann_end(D); ai != ae; ++ai)
+    EmitAnnotationCall(CGM.getIntrinsic(llvm::Intrinsic::var_annotation),
+                       Builder.CreateBitCast(V, CGM.Int8PtrTy, V->getName()),

Hoist this bitcast out of the loop, please.

+                       (*ai)->getAnnotation(), D->getLocation(), Builder);
+}

+llvm::Value *CGAnnotations::EmitFieldAnnotations(const FieldDecl *D,
+                                                 llvm::Value *V,
+                                                 CGBuilderTy &Builder) const {

-> CodeGenFunction

+  assert(D->hasAttr<AnnotateAttr>() && "no annotate attribute");
+  const llvm::Type *ArgTy = CGM.Int8PtrTy, *VTy = V->getType();
+  llvm::Value *F = CGM.getIntrinsic(llvm::Intrinsic::ptr_annotation, &ArgTy, 1);
+
+  for (ann_iterator ai = ann_begin(D), ae = ann_end(D); ai != ae; ++ai) {
+    // Always emit the cast inst so we can differentiate between annotation on
+    // the first field of a struct and annotation on the strut itself.

I guess this is how this has "traditionally" been represented, but there has
got to be a better way of encoding that in the IR.

+    if (V->getType() != ArgTy)
+      V = Builder.Insert(new llvm::BitCastInst(V, ArgTy));

Please continue using CGF.Int8PtrTy here, it's much clearer.

diff --git a/lib/CodeGen/CGAnnotations.h b/lib/CodeGen/CGAnnotations.h

+class CGAnnotations {

I think I've basically asked you to take almost all the methods here to
CodeGenFunction, so you might as well put the rest (and the data
members) on CodeGenModule, which probably kills this entire header.
I don't really care about avoiding the initialization of a couple of data
structures.

Otherwise, this all looks great, thanks!

John.