[clang][CodeGen] Emit annotations for function declarations.

[brendandahl]

Summary

Previously, annotations were only emitted for function definitions. With
this change annotations are also emitted for declarations. Also, emitting
function annotations is now deferred until the end so that the most
up to date declaration is used which will have any inherited annotations.

[aaron.ballman]

Adding additional reviewers for more opinions.

The changes seem reasonable to me given that the annotation attribute is used to squirrel data from the frontend to the backend (including via plugin attributes) and so its usage is pretty general. But I'd like a second opinion given that this attribute has only passed data along for function definitions for a *long* time. I'm not certain if changing this will cause surprises for the backend.

[brendandahl]

A little more context for the new reviewers. Over in this patch I added support for annotations to be emitted into WebAssembly. We'd like to use this to mark imported (declarations) and exported (definitions) of functions with special attributes.

In D156172#4531885, @aaron.ballman wrote:

The changes seem reasonable to me given that the annotation attribute is used to squirrel data from the frontend to the backend (including via plugin attributes) and so its usage is pretty general. But I'd like a second opinion given that this attribute has only passed data along for function definitions for a *long* time. I'm not certain if changing this will cause surprises for the backend.

Interestingly, I noticed the original patch that added annotations had support for merging annotations of declarations and definitions.

[efriedma]

The idea of emitting annotations on declarations seems fine. (LLVM itself doesn't consume annotations anyway; they're meant as an extension mechanism for third-party tools.)

I'm a bit concerned the way this is implemented will end up dropping annotations we would previously emit. For example:

void foo();
void *xxx = (void*)foo;
__attribute__((annotate("bar"))) void foo(){}

[brendandahl]

That example appears to still work the same with my patch:

@xxx = global ptr @foo, align 8
@.str = private unnamed_addr constant [4 x i8] c"bar\00", section "llvm.metadata"
@.str.1 = private unnamed_addr constant [8 x i8] c"main2.c\00", section "llvm.metadata"
@llvm.global.annotations = appending global [1 x { ptr, ptr, ptr, i32, ptr }] [{ ptr, ptr, ptr, i32, ptr } { ptr @foo, ptr @.str, ptr @.str.1, i32 3, ptr null }], section "llvm.metadata"

I'm not very familiar with the CodeGenModule, so I'm happy to move the call to AddGlobalAnnotations if there is a better place.

[efriedma]

Slightly messed up my example because I forgot the function was unprototyped. The following should show what I mean:

void foo(void);
void *xxx = (void*)foo;
__attribute__((annotate("bar"))) void foo(){}

In terms of where the right place is, I don't recall the exact structure of that code off the top of my head, but I think there's somewhere we handle redeclarations?

[brendandahl]

I looked into the above issue with mixing declarations and definitions and I think I can fix that by leaving the AddGlobalAnnotations call in EmitGlobalFunctionDefinition and only calling AddGlobalAnnotations from GetOrCreateLLVMFunction when it's for a definition. However, with that I'm running into some duplicate annotations being created with C++ templates that I need to fix.

While looking into the duplicate annotations, I noticed someone else already implemented annotations for declarations, but it was rolled back.

[aaron.ballman]

In D156172#4540025, @brendandahl wrote:

I looked into the above issue with mixing declarations and definitions and I think I can fix that by leaving the AddGlobalAnnotations call in EmitGlobalFunctionDefinition and only calling AddGlobalAnnotations from GetOrCreateLLVMFunction when it's for a definition. However, with that I'm running into some duplicate annotations being created with C++ templates that I need to fix.

While looking into the duplicate annotations, I noticed someone else already implemented annotations for declarations, but it was rolled back.

I *thought* this seemed familiar! :-D Be sure to add the test case from the rolled-back review to make sure your patch doesn't trip on the same issue.

[brendandahl]

Switch to emitting annotations and the end.

[efriedma]

I'm happy with this approach.

clang/lib/CodeGen/CodeGenModule.cpp

4327

This doesn't quite seem sufficient... I'd expect you'd want to update the map when you see a new declaration. Otherwise we miss annotations on something like:

void foo(void);
void *xxx = (void*)foo;
void __attribute__((annotate("bar"))) foo();

Granted, I wouldn't expect this sort of construct to show up normally, and I'm not sure how hard this is to fix.

[brendandahl]

Update deferred annotations whenever EmitGlobalDefinition is called
with a FunctionDecl and it has already been used or defined.

[brendandahl]

Cleanup some extra brackets.

[brendandahl]

@efriedma I've updated the patch to fix the decl-use-decl example and added a test for it. Anything else?

[brendandahl]

@efriedma ping

[efriedma]

LGTM

[brendandahl]

Rebase and fix unused variable warning.

[bkramer]

Reverted this in 88b7e06dcf9723d0869b0c6bee030b4140e4366d as it makes clang crash. Reduced test case in the commit description.

clang/lib/CodeGen/CodeGenModule.h

private:
 
   /// An ordered map of canonical GlobalDecls to their mangled names.
   llvm::MapVector<GlobalDecl, StringRef> MangledDeclNames;
   llvm::StringMap<GlobalDecl, llvm::BumpPtrAllocator> Manglings;
 
   /// Global annotations.
   std::vector<llvm::Constant*> Annotations;
 
+  // Store deferred function annotations so they can be emitted at the end with
+  // most up to date ValueDecl that will have all the inherited annotations.
+  llvm::DenseMap<llvm::Function *, const ValueDecl *> DeferredAnnotations;
+
   /// Map used to get unique annotation strings.
   llvm::StringMap<llvm::Constant*> AnnotationStrings;
 
   /// Used for uniquing of annotation arguments.
   llvm::DenseMap<unsigned, llvm::Constant *> AnnotationArgs;
 
   llvm::StringMap<llvm::GlobalVariable *> CFConstantStringMap;
 

clang/lib/CodeGen/CodeGenModule.cpp

for (const GlobalDecl &GD : Aliases) {
     Alias->replaceAllUsesWith(llvm::UndefValue::get(Alias->getType()));
     Alias->eraseFromParent();
   }
 }
 
 void CodeGenModule::clear() {
   DeferredDeclsToEmit.clear();
   EmittedDeferredDecls.clear();
+  DeferredAnnotations.clear();
   if (OpenMPRuntime)
     OpenMPRuntime->clear();
 }
 
 void InstrProfStats::reportDiagnostics(DiagnosticsEngine &Diags,
                                        StringRef MainFile) {
   if (!hasDiagnostics())
     return;
assert(getVTables().isVTableExternal(RD) &&
            "This queue should only contain external vtables");
     if (getCXXABI().canSpeculativelyEmitVTable(RD))
       VTables.GenerateClassData(RD);
   }
   OpportunisticVTables.clear();
 }
 
 void CodeGenModule::EmitGlobalAnnotations() {
+  for (const auto& [F, VD] : DeferredAnnotations)
+    AddGlobalAnnotations(VD, F);
+  DeferredAnnotations.clear();
+
   if (Annotations.empty())
     return;
 
   // Create a new global variable for the ConstantStruct in the Module.
   llvm::Constant *Array = llvm::ConstantArray::get(llvm::ArrayType::get(
     Annotations[0]->getType(), Annotations.size()), Annotations);
   auto *gv = new llvm::GlobalVariable(getModule(), Array->getType(), false,
                                       llvm::GlobalValue::AppendingLinkage,
if (IsForDefinition && !Entry->isDeclaration()) {
           DiagnosedConflictingDefinitions.insert(GD).second) {
         getDiags().Report(D->getLocation(), diag::err_duplicate_mangled_name)
             << MangledName;
         getDiags().Report(OtherGD.getDecl()->getLocation(),
                           diag::note_previous_definition);
       }
     }
 
+    // Store the declaration associated with this function so it is emitted at
+    // the end.
+    if (D && D->hasAttr<AnnotateAttr>() && isa<llvm::Function>(Entry))
+      DeferredAnnotations[cast<llvm::Function>(Entry)] = cast<ValueDecl>(D);
+
     if ((isa<llvm::Function>(Entry) || isa<llvm::GlobalAlias>(Entry)) &&
         (Entry->getValueType() == Ty)) {
       return Entry;
     }
 
     // Make sure the result is of the correct type.
     // (If function is requested for a definition, we always need to create a new
     // function, not just return a bitcast.)
if (isa<llvm::FunctionType>(Ty)) {
     FTy = llvm::FunctionType::get(VoidTy, false);
     IsIncompleteFunction = true;
   }
 
   llvm::Function *F =
       llvm::Function::Create(FTy, llvm::Function::ExternalLinkage,
                              Entry ? StringRef() : MangledName, &getModule());
 
+  // Update the declartion associated with this function so any new or inherited
+  // annotations are emitted.
+  if (D && D->hasAttr<AnnotateAttr>())
+    DeferredAnnotations[F] = cast<ValueDecl>(D);
+
   // If we already created a function with the same mangled name (but different
   // type) before, take its name and add it to the list of functions to be
   // replaced with F at the end of CodeGen.
   //
   // This happens if there is a prototype for a function (e.g. "int f()") and
   // then a definition of a different type (e.g. "int f(int x)").
   if (Entry) {
     F->takeName(Entry);

 /// mangled name but some other type.
 llvm::Constant *
 CodeGenModule::GetOrCreateLLVMGlobal(StringRef MangledName, llvm::Type *Ty,
                                      LangAS AddrSpace, const VarDecl *D,
                                      ForDefinition_t IsForDefinition) {
   // Lookup the entry, lazily creating it if necessary.
   llvm::GlobalValue *Entry = GetGlobalValue(MangledName);
   unsigned TargetAS = getContext().getTargetAddressSpace(AddrSpace);
+
   if (Entry) {
     if (WeakRefReferences.erase(Entry)) {
       if (D && !D->hasAttr<WeakAttr>())
         Entry->setLinkage(llvm::Function::ExternalLinkage);
     }
 
     // Handle dropped DLL attributes.
     if (D && !D->hasAttr<DLLImportAttr>() && !D->hasAttr<DLLExportAttr>() &&
void CodeGenModule::EmitGlobalFunctionDefinition(GlobalDecl GD,
 
   setNonAliasAttributes(GD, Fn);
   SetLLVMFunctionAttributesForDefinition(D, Fn);
 
   if (const ConstructorAttr *CA = D->getAttr<ConstructorAttr>())
     AddGlobalCtor(Fn, CA->getPriority());
   if (const DestructorAttr *DA = D->getAttr<DestructorAttr>())
     AddGlobalDtor(Fn, DA->getPriority(), true);
-  if (D->hasAttr<AnnotateAttr>())
-    AddGlobalAnnotations(D, Fn);
 }
 
 void CodeGenModule::EmitAliasDefinition(GlobalDecl GD) {
   const auto *D = cast<ValueDecl>(GD.getDecl());
   const AliasAttr *AA = D->getAttr<AliasAttr>();
   assert(AA && "Not an alias?");
 
   StringRef MangledName = getMangledName(GD);

clang/test/CodeGen/annotations-decl-use-define.c

+// RUN: %clang_cc1 %s -emit-llvm -o - | FileCheck %s
+
+// Test annotation attributes are still emitted when the function is used before
+// it is defined with annotations.
+
+void foo(void);
+void *xxx = (void*)foo;
+void __attribute__((annotate("bar"))) foo() {}
+
+// CHECK: target triple
+// CHECK-DAG: private unnamed_addr constant [4 x i8] c"bar\00", section "llvm.metadata"
+
+// CHECK: @llvm.global.annotations = appending global [1 x { ptr, ptr, ptr, i32, ptr }] [{
+// CHECK-SAME: { ptr @foo,
+// CHECK-SAME: }], section "llvm.metadata"
+

clang/test/CodeGen/annotations-declaration.c

+// RUN: %clang_cc1 %s -emit-llvm -o - | FileCheck %s
+
+// Test annotation attributes are emitted for declarations.
+
+__attribute__((annotate("bar"))) int foo();
+
+int main() {
+  return foo();
+}
+
+// CHECK: target triple
+// CHECK-DAG: private unnamed_addr constant [4 x i8] c"bar\00", section "llvm.metadata"
+
+// CHECK: @llvm.global.annotations = appending global [1 x { ptr, ptr, ptr, i32, ptr }] [{
+// CHECK-SAME: { ptr @foo,
+// CHECK-SAME: }], section "llvm.metadata"
+

clang/test/CodeGen/annotations-global.c

 // CHECK-DAG: private unnamed_addr constant [6 x i8] c"bar_0\00", section "llvm.metadata"
 // CHECK-DAG: private unnamed_addr constant [6 x i8] c"bar_1\00", section "llvm.metadata"
 
 // CHECK-DAG: private unnamed_addr constant [15 x i8] c"addrspace1_ann\00", section "llvm.metadata"
 
 // CHECK: @llvm.global.annotations = appending global [11 x { ptr, ptr, ptr, i32, ptr }] [{
 // CHECK-SAME: { ptr @a.bar,
 // CHECK-SAME: { ptr @a.bar,
-// CHECK-SAME: { ptr @a,
-// CHECK-SAME: { ptr @a,
-// CHECK-SAME: { ptr @a,
-// CHECK-SAME: { ptr @a,
 // CHECK-SAME: { ptr @sfoo,
 // CHECK-SAME: { ptr @sfoo,
 // CHECK-SAME: { ptr @foo,
 // CHECK-SAME: { ptr @foo,
 // CHECK-SAME: { ptr addrspacecast (ptr addrspace(1) @addrspace1_var to ptr),
+// CHECK-SAME: { ptr @a,
+// CHECK-SAME: { ptr @a,
+// CHECK-SAME: { ptr @a,
+// CHECK-SAME: { ptr @a,
 // CHECK-SAME: }], section "llvm.metadata"
 
 // AS1-GLOBALS: target datalayout = "{{.+}}-A5-G1"
 // AS1-GLOBALS: @llvm.global.annotations = appending addrspace(1) global [11 x { ptr addrspace(1), ptr addrspace(4), ptr addrspace(4), i32, ptr addrspace(4) }]
 // AS1-GLOBALS-SAME: { ptr addrspace(1) @a.bar,
 // AS1-GLOBALS-SAME: { ptr addrspace(1) @addrspace1_var,

clang/test/CodeGenCXX/attr-annotate-destructor.cpp

+// RUN: %clang_cc1 %s -S -emit-llvm -triple x86_64-unknown-linux-gnu -o - | FileCheck %s
+
+// Test annotation attributes on destructors doesn't crash.
+
+struct k {
+  ~k() __attribute__((annotate(""))) {}
+};
+void m() { k(); }
+
+// CHECK: @llvm.global.annotations = appending global [2 x { ptr, ptr, ptr, i32, ptr }] [{