PR17829: Functions declared extern "C" with a name matching a mangled C++ function are allowed

[andreybokhanko]

Summary

This patch fixes compiler crash described in the following thread: http://lists.cs.uiuc.edu/pipermail/cfe-dev/2015-July/thread.html#43852. It also fixes incorrect behavior described in PR17829.

In essence, I inserted a check in GetAddrOfFunction that verifies that no two different declarations correspond to the same mangled name. Given that clang defers everything, this is the best suitable place I managed to find for the check. Also, the check makes sense only if one of declarations is a C++ method, as there are some kinds of non-C++ declarations that can correctly have the same mangled name (inline assembly inserts is an example).

Yours,

Andrey Bokhanko

Software Engineer
Intel Compiler Team
Intel

[rsmith]

lib/CodeGen/CodeGenModule.cpp

1674–1675 ↗

(On Diff #30000)

Why do you need this check? The same mangling collisions can happen with any other kind of function declaration.

1681 ↗

(On Diff #30000)

Any reason not to use an undef value of the appropriate type (Ty)? If you did that, you presumably wouldn't need special cases elsewhere.

[rjmccall]

I disagree with the conclusions in that thread (and wish you had CC'ed me; I unfortunately do not have time to keep up with cfe-dev). There is roughly zero chance that somebody is declaring an extern "C" symbol with a name that matches an Itanium mangled name with any intent other than to name that entity. We should not be rejecting this code or silently renaming one of the symbols. We should, of course, still diagnose attempts to *define* the same symbol multiple times.

The correct fix is to add bitcasts and/or replace globals as necessary so that the colliding references name the same LLVM object. We already have a type, it should be easy to bitcast to that.

[andreybokhanko]

Richard, John, thanks for the review. I'm currently on vacation, so can't respond now, but will do after getting back to work.

John, I will make sure in the future to CC you on all CodeGen-related issues, promise! :-)

Yours,
Andrey

[rjmccall]

Thanks. :)

When you do have time to work on this again, I've found that the best way to do this is to simply propagate a flag down to GetAddrOfFunction indicating whether you intend to define it. (One of these lessons that I keep meaning to back port from the Swift compiler...) The actions to take are then pretty easy:

• If the global value doesn't already exist, or it already exists and has the right type + kind, you don't need to do anything special.
• Otherwise, we have an existing global value with the wrong kind (e.g. is a GlobalVariable) or the wrong type:
  • If you're not trying to define it, just return a bitcast.
  • If you're trying to define it, and the existing value is not a definition, just claim the name for a new Function and queue up the existing one to be replaced with a bitcast. (Replacing it immediately can be dangerous because there could be outstanding references to it that aren't held in a value handle, e.g. if we're emitting the definition of something in the middle of emitting the definition of something else, which does happen sometimes.)
  • If you're trying to define it, and the existing value is a definition, diagnose the collision and return a new Function. You don't want to mess with the existing definition because it's possible that something is currently emitting code into it.

For these purposes, the existing value is a definition if it was created by a call to GetAddrOfFunction or GetAddrOfGlobalVar with a flag saying that this is for a definition. (Or if it was emitted by EmitAliasDefinition.) You have to be somewhat careful about just checking whether it's an LLVM definition because we might have created it for the purposes of defining it but not yet started that definition; this is particularly likely with global variables. (In fact, it might only be possible with them.)

[andreybokhanko]

John,
(Others are welcome to chime in as well)

I started to implement your suggestion, but faced with problems.

Consider the following test case:

1: struct T {
2:   ~T() {}
3: };
4: 
5: extern "C" void _ZN1TD1Ev();
6: 
7: int main() {
8:   _ZN1TD1Ev();
9:   T t;
10: }

First time we visit "GetOrCreateLLVMFunction" for "_ZN1TD1Ev" mangled name is when we are dealing with the call at line 8. No global values are associated with "_ZN1TD1Ev" name yet, so we simply create a new llvm::Function and add it to the list of module's functions (ParentModule->getFunctionList().push_back(this); at Function.cpp:264).

Next time we visit "GetOrCreateLLVMFunction" for the same "_ZN1TD1Ev" name is when we are dealing with the implicit t's destructor call at the end of "main" routine (say, at line 10). We already have a global value associated with "_ZN1TD1Ev" name *and* we have a function with this name in module's function list. We can't create a new llvm::Function and add it to the functions list -- since module's machinery asserts that there is only one function with a given mangled name in the list; we can't remove old Function either -- since it is referred to in the call at line 8. The best we can do is to bit cast old llvm::Function (from line 8) to destructor's type and return it. But this generates destructor call for a wrong llvm::Function which means problems and further asserts down the road.

Any hints how to resolve this?

Yours,
Andrey

P.S.: I honestly believe my test above is a case of incorrect usage of mangled names (despite no definition for "ZN1TD1Ev" function), so in my opinion the way to go would be to issue a error inside "GetOrCreateLLVMFunction". This is different from my initial patch and still compiles fine the following example -- which, as I believe, you meant as a correct use of explicitly mangled names:

struct T {
  ~T() {}
};

extern "C" void _ZN1TD1Ev(T *this_p);

int main() {
  _ZN1TD1Ev(0);
  T t;
}

(My initial patch rejected even this code, which is probably too strict indeed.)

[rjmccall]

You only have one attempt to define the function here; I don't see the problem. Recall that I said to add a flag to getOrCreateLLVMFunction that says whether the caller intends to define the function. The rule should be that only callers that pass "true" should be allowed to assume that they'll get a normal llvm::Function back. Everybody needs to be prepared to receive a bitcast. Whenever you find yourself needing to replace an existing function, just queue it up to be replaced at the end of IRGen.

I don't think we need to fall over ourselves ensuring that these "aliased" uses properly mark functions as used or instantiate templates or anything.

[andreybokhanko]

John,

Thank you for the quick reply!

Let me make sure I understand what you said, using my test as an example (BTW, sorry if this is a dumb question -- I asked our local Clang experts, but no-one seems to be 100% sure what to do):

1: struct T {
2:   ~T() {}
3: };
4: 
5: extern "C" void _ZN1TD1Ev();
6: 
7: int main() {
8:   _ZN1TD1Ev();
9:   T t;
10: }

When we deal with the call at line N8, there are no Functions created yet, so nothing to bitcast. Thus, we create a Function and the following call:

call void @_ZN1TD1Ev()

When we deal with implicit destructor call at line N10, there is already a Function with "_ZN1TD1Ev" mangled name exists. Thus, we create a bitcast and the following call:

call void bitcast (void ()* @_ZN1TD1Ev to void (%struct.T*)*)(%struct.T* %t)

At the end of IRGen we should replace all references of old _ZN1TD1Ev (one with zero arguments) with new _ZN1TD1Ev (one with a single T* argument) -- *including* adding a new bitcast (as we replace a Function with different type) in all places in IR where we do the replacement.

Is my understanding correct?

Andrey

[rjmccall]

In D11297#223622, @andreybokhanko wrote:

John,

Thank you for the quick reply!

Let me make sure I understand what you said, using my test as an example (BTW, sorry if this is a dumb question -- I asked our local Clang experts, but no-one seems to be 100% sure what to do):

1: struct T {
2:   ~T() {}
3: };
4: 
5: extern "C" void _ZN1TD1Ev();
6: 
7: int main() {
8:   _ZN1TD1Ev();
9:   T t;
10: }

When we deal with the call at line N8, there are no Functions created yet, so nothing to bitcast. Thus, we create a Function and the following call:

call void @_ZN1TD1Ev()

Yes.

When we deal with implicit destructor call at line N10, there is already a Function with "_ZN1TD1Ev" mangled name exists. Thus, we create a bitcast and the following call:

call void bitcast (void ()* @_ZN1TD1Ev to void (%struct.T*)*)(%struct.T* %t)

Yes.

At the end of IRGen we should replace all references of old _ZN1TD1Ev (one with zero arguments) with new _ZN1TD1Ev (one with a single T* argument) -- *including* adding a new bitcast (as we replace a Function with different type) in all places in IR where we do the replacement.

This is only necessary if you try to emit the definition at some point. In this case, you will, because emitting the reference to the destructor as part of the second call will require the destructor to be emitted, because it's inline.

Let me try to spell out the sequence more precisely.

1. IRGen starts out with no Function.
2. IRGen sees the declaration of the extern "C" function. It's just a declaration, not a definition, so IRGen doesn't need to do anything.
3. IRGen sees the declaration of the destructor. It's a definition, but it's a deferrable definition, so IRGen doesn't need to do anything except record that it's got a deferred definition in DeferredDecls.
4. IRGen emits a reference to the extern "C" function. This is a reference, not a definition, so it's fine for getOrCreateLLVMFunction to return an arbitrary Constant; it doesn't have to return a Function. But we don't have a Function yet, so we create one with the expected type for the extern "C" function. We also notice that we've got a deferred definition for this name, so we move that to the deferred-decls-to-emit queue.
5. IRGen emits a reference to the destructor. This is a reference, not a definition, so any kind of Constant is fine. Now, we've got a Function, but it's got the wrong type, so we need to bitcast it. We've already enqueued the deferred definition, so that's fine.
6. IRGen emits the deferred definition. We tell getOrCreateLLVMFunction that we're going to define the function, so getOrCreate has to return a Function with the right type; it's got an existing llvm::Function, but it's the wrong type, so it has to make a new llvm::Function. It creates a new Function with no name, takes the name of the existing Function (with takeName), replaces existing references with a bitcast to the new name, and queues up something for the end of emission to remove the replaced Function.
7. IRGen reaches the end of emission and sees that it's got a Function to replace. It replaces the Function with a bitcast of the new function again (if there are any remaining uses) and then deletes it.

Make sense?

[andreybokhanko]

John,

Make sense?

Yes, absolutely! -- even for me. :-)

Thanks a 1000000!

Yours,
Andrey

[andreybokhanko]

John,

I implemented precisely what you described (or so I believe :-))

Patch is updated; please re-review.

This patch implements support for functions, but not variables yet -- the patch is big enough already, so variables will come next.

Note that the biggest change in CodeGenModule.cpp is just moving of several static functions to another part of the file (to make them accessible earlier).

Yours,
Andrey

[rjmccall]

This looks generally like what I'm looking for, thanks! Some comments.

lib/CodeGen/CodeGenModule.cpp

1129 ↗

(On Diff #33083)

This is a pretty expensive extra check, and I think it only kicks in on invalid code where we've got multiple definitions of a function. Can we just eliminate it? It's not really a problem to emit the second function definition as long as we're not trying to emit it into the same llvm::Function.

1573 ↗

(On Diff #33083)

Instead of moving this function in this patch, just add a forward declaration. If you want to move it, you can do that in a separate patch that only moves the function.

lib/CodeGen/CodeGenModule.h

349 ↗

(On Diff #33083)

Missing a period in the comment.

354 ↗

(On Diff #33083)

I don't think this is necessary. I don't believe we ever need to emit a (mangled) function for definition between starting to emit another function and adding its entry block, or between starting to emit a global variable and finishing its constant initializer. So you should be able to just check whether the existing llvm::GlobalValue is a definition.

I don't think avoiding emitting the diagnostic multiple times for different globals is necessary, or even a good idea.

[andreybokhanko]

All (but one) of John McCall's comments fixed.

[andreybokhanko]

John,

Thank you for the review!

All your comments but one are fixed. See below for details on the single one I didn't manage to get fixed.

Andrey

lib/CodeGen/CodeGenModule.h

354 ↗

(On Diff #33316)

Checking that a GlobalDecl is not in ExplicitDefinitions yet is actually required to avoid printing multiple identical warnings.

In my example:

1: struct T {
2:   ~T() {}
3: };
4: 
5: extern "C" void _ZN1TD1Ev();
6: 
7: int main() {
8:   _ZN1TD1Ev();
9:   T t;
10: }

~T() is added to the list of deferred decls twice. Judging from this comment in "EmitDeferred" method:

// Check to see if we've already emitted this.  This is necessary
// for a couple of reasons: first, decls can end up in the
// deferred-decls queue multiple times, and second, decls can end
// up with definitions in unusual ways (e.g. by an extern inline
// function acquiring a strong function redefinition).  Just
// ignore these cases.

this is pretty normal ("decls can end up in the deferred-decls queue multiple times").

This means that we can call "GetOrCreateLLVMFunction"(..., /*IsForDefinition*/=true) for duplicated decls several times, which is fine in general, *but* will print the "duplicated mangled names" diagnostic multiple times as well -- unless we check that we already printed a warning on duplicated mangled names for given decl.

As for not emitting diagnostics for different globals -- this won't happen, as we will call "GetOrCreateLLVMFunction" at least once for each global with a definition, and thus, will print a warning for everyone.

I thought really hard (honestly!) on how to prevent duplicated diagnostics without usage of an additional set, but didn't found any solution. If you have any hints here, they would be much appreciated.

[rjmccall]

lib/CodeGen/CodeGenModule.h

354 ↗

(On Diff #33316)

Okay, that's fine. Can you at least make sure you only add to the set when you emit the warning?

As a minor optimization, you can add it to the set and check whether it was already there in the same operation, so that this would look like:

if (LookupRepresentativeDecl(...) && DiagnosedConflictingDefinitions.insert(GD).second) {
  ...
}

[andreybokhanko]

Fixed last note from John McCall.

John, please re-review.

Yours,
Andrey

[rjmccall]

This looks great, thanks! One last comment; if you agree with me, go ahead and fix it and then commit.

include/clang/Basic/DiagnosticSemaKinds.td

2323 ↗

(On Diff #33401)

I'm sorry to bring this up so late in the process, but is there a good reason for this to be a warning and not an error?

[andreybokhanko]

include/clang/Basic/DiagnosticSemaKinds.td

2323 ↗

(On Diff #33401)

No good reasons at all -- I thought you want this to be a warning, but looks like I misinterpreted you.

Do you want me to make this an error?

(If yes, apart of changing warn_duplicate_mangled_name to err_duplicate_mangled_name everywhere, I will have to change the test -- let me know whatever you want to review these changes or not really interested and your "go ahead and fix it and then commit" still holds true)

Andrey

[rjmccall]

Yes, please make it an error. And the obvious test changes are fine. :)

[andreybokhanko]

In D11297#235525, @rjmccall wrote:

Yes, please make it an error.

Done.

John, thank you for all your patience and explanations! -- I understand that this particular review and patch author required more than the usual measure. :-(

And the obvious test changes are fine. :)

I asked because after switching from warning to error, I had to introduce a new run line in the test -- effectively transforming it into two.

Andrey

cfe/trunk/lib/CodeGen/CGCXX.cpp

bool CodeGenModule::TryEmitDefinitionAsAlias(GlobalDecl AliasDecl,
   return false;
 }
 
 llvm::Function *CodeGenModule::codegenCXXStructor(const CXXMethodDecl *MD,
                                                   StructorType Type) {
   const CGFunctionInfo &FnInfo =
       getTypes().arrangeCXXStructorDeclaration(MD, Type);
   auto *Fn = cast<llvm::Function>(
-      getAddrOfCXXStructor(MD, Type, &FnInfo, nullptr, true));
+      getAddrOfCXXStructor(MD, Type, &FnInfo, /*FnType=*/nullptr,
+                           /*DontDefer=*/true, /*IsForDefinition=*/true));
 
   GlobalDecl GD;
   if (const auto *DD = dyn_cast<CXXDestructorDecl>(MD)) {
     GD = GlobalDecl(DD, toCXXDtorType(Type));
   } else {
     const auto *CD = cast<CXXConstructorDecl>(MD);
     GD = GlobalDecl(CD, toCXXCtorType(Type));
   }
 
   setFunctionLinkage(GD, Fn);
   setFunctionDLLStorageClass(GD, Fn);
 
   CodeGenFunction(*this).GenerateCode(GD, Fn, FnInfo);
   setFunctionDefinitionAttributes(MD, Fn);
   SetLLVMFunctionAttributesForDefinition(MD, Fn);
   return Fn;
 }
 
-llvm::GlobalValue *CodeGenModule::getAddrOfCXXStructor(
+llvm::Constant *CodeGenModule::getAddrOfCXXStructor(
     const CXXMethodDecl *MD, StructorType Type, const CGFunctionInfo *FnInfo,
-    llvm::FunctionType *FnType, bool DontDefer) {
+    llvm::FunctionType *FnType, bool DontDefer, bool IsForDefinition) {
   GlobalDecl GD;
   if (auto *CD = dyn_cast<CXXConstructorDecl>(MD)) {
     GD = GlobalDecl(CD, toCXXCtorType(Type));
   } else {
     GD = GlobalDecl(cast<CXXDestructorDecl>(MD), toCXXDtorType(Type));
   }
 
-  StringRef Name = getMangledName(GD);
-  if (llvm::GlobalValue *Existing = GetGlobalValue(Name))
-    return Existing;
-
   if (!FnType) {
     if (!FnInfo)
       FnInfo = &getTypes().arrangeCXXStructorDeclaration(MD, Type);
     FnType = getTypes().GetFunctionType(*FnInfo);
   }
 
-  return cast<llvm::Function>(GetOrCreateLLVMFunction(Name, FnType, GD,
-                                                      /*ForVTable=*/false,
-                                                      DontDefer));
+  return GetOrCreateLLVMFunction(
+      getMangledName(GD), FnType, GD, /*ForVTable=*/false, DontDefer,
+      /*isThunk=*/false, /*ExtraAttrs=*/llvm::AttributeSet(), IsForDefinition);
 }
 
 static llvm::Value *BuildAppleKextVirtualCall(CodeGenFunction &CGF,
                                               GlobalDecl GD,
                                               llvm::Type *Ty,
                                               const CXXRecordDecl *RD) {
   assert(!CGF.CGM.getTarget().getCXXABI().isMicrosoft() &&
          "No kext in Microsoft ABI");

cfe/trunk/lib/CodeGen/CodeGenModule.h

private:
 
   /// List of alias we have emitted. Used to make sure that what they point to
   /// is defined once we get to the end of the of the translation unit.
   std::vector<GlobalDecl> Aliases;
 
   typedef llvm::StringMap<llvm::TrackingVH<llvm::Constant> > ReplacementsTy;
   ReplacementsTy Replacements;
 
+  /// List of global values to be replaced with something else. Used when we
+  /// want to replace a GlobalValue but can't identify it by its mangled name
+  /// anymore (because the name is already taken).
+  llvm::SmallVector<std::pair<llvm::GlobalValue *, llvm::Constant *>, 8>
+    GlobalValReplacements;
+
+  /// Set of global decls for which we already diagnosed mangled name conflict.
+  /// Required to not issue a warning (on a mangling conflict) multiple times
+  /// for the same decl.
+  llvm::DenseSet<GlobalDecl> DiagnosedConflictingDefinitions;
+
   /// A queue of (optional) vtables to consider emitting.
   std::vector<const CXXRecordDecl*> DeferredVTables;
 
   /// List of global values which are required to be present in the object file;
   /// bitcast to i8*. This is used for forcing visibility of symbols which may
   /// otherwise be optimized out.
   std::vector<llvm::WeakVH> LLVMUsed;
   std::vector<llvm::WeakVH> LLVMCompilerUsed;
static llvm::GlobalValue::VisibilityTypes GetLLVMVisibility(Visibility V) {
     switch (V) {
     case DefaultVisibility:   return llvm::GlobalValue::DefaultVisibility;
     case HiddenVisibility:    return llvm::GlobalValue::HiddenVisibility;
     case ProtectedVisibility: return llvm::GlobalValue::ProtectedVisibility;
     }
     llvm_unreachable("unknown visibility!");
   }
 
-  llvm::Constant *GetAddrOfGlobal(GlobalDecl GD) {
-    if (isa<CXXConstructorDecl>(GD.getDecl()))
-      return getAddrOfCXXStructor(cast<CXXConstructorDecl>(GD.getDecl()),
-                                  getFromCtorType(GD.getCtorType()));
-    else if (isa<CXXDestructorDecl>(GD.getDecl()))
-      return getAddrOfCXXStructor(cast<CXXDestructorDecl>(GD.getDecl()),
-                                  getFromDtorType(GD.getDtorType()));
-    else if (isa<FunctionDecl>(GD.getDecl()))
-      return GetAddrOfFunction(GD);
-    else
-      return GetAddrOfGlobalVar(cast<VarDecl>(GD.getDecl()));
-  }
+  llvm::Constant *GetAddrOfGlobal(GlobalDecl GD, bool IsForDefinition = false);
 
   /// Will return a global variable of the given type. If a variable with a
   /// different type already exists then a new  variable with the right type
   /// will be created and all uses of the old variable will be replaced with a
   /// bitcast to the new variable.
   llvm::GlobalVariable *
   CreateOrReplaceCXXRuntimeVariable(StringRef Name, llvm::Type *Ty,
                                     llvm::GlobalValue::LinkageTypes Linkage);
public:
   /// would be.
   llvm::Constant *GetAddrOfGlobalVar(const VarDecl *D,
                                      llvm::Type *Ty = nullptr);
 
   /// Return the address of the given function. If Ty is non-null, then this
   /// function will use the specified type if it has to create it.
   llvm::Constant *GetAddrOfFunction(GlobalDecl GD, llvm::Type *Ty = 0,
                                     bool ForVTable = false,
-                                    bool DontDefer = false);
+                                    bool DontDefer = false,
+                                    bool IsForDefinition = false);
 
   /// Get the address of the RTTI descriptor for the given type.
   llvm::Constant *GetAddrOfRTTIDescriptor(QualType Ty, bool ForEH = false);
 
   llvm::Constant *getAddrOfCXXCatchHandlerType(QualType Ty,
                                                QualType CatchHandlerType);
 
   /// Get the address of a uuid descriptor .
public:
 
   // Produce code for this constructor/destructor. This method doesn't try
   // to apply any ABI rules about which other constructors/destructors
   // are needed or if they are alias to each other.
   llvm::Function *codegenCXXStructor(const CXXMethodDecl *MD,
                                      StructorType Type);
 
   /// Return the address of the constructor/destructor of the given type.
-  llvm::GlobalValue *
+  llvm::Constant *
   getAddrOfCXXStructor(const CXXMethodDecl *MD, StructorType Type,
                        const CGFunctionInfo *FnInfo = nullptr,
                        llvm::FunctionType *FnType = nullptr,
-                       bool DontDefer = false);
+                       bool DontDefer = false, bool IsForDefinition = false);
 
   /// Given a builtin id for a function like "__builtin_fabsf", return a
   /// Function* for "fabsf".
   llvm::Value *getBuiltinLibFunction(const FunctionDecl *FD,
                                      unsigned BuiltinID);
 
   llvm::Function *getIntrinsic(unsigned IID, ArrayRef<llvm::Type*> Tys = None);
 
public:
   /// Set attributes which must be preserved by an alias. This includes common
   /// attributes (i.e. it includes a call to SetCommonAttributes).
   ///
   /// NOTE: This should only be called for definitions.
   void setAliasAttributes(const Decl *D, llvm::GlobalValue *GV);
 
   void addReplacement(StringRef Name, llvm::Constant *C);
 
+  void addGlobalValReplacement(llvm::GlobalValue *GV, llvm::Constant *C);
+
   /// \brief Emit a code for threadprivate directive.
   /// \param D Threadprivate declaration.
   void EmitOMPThreadPrivateDecl(const OMPThreadPrivateDecl *D);
 
   /// Returns whether the given record is blacklisted from control flow
   /// integrity checks.
   bool IsCFIBlacklistedRecord(const CXXRecordDecl *RD);
 
public:
   /// \breif Get the declaration of std::terminate for the platform.
   llvm::Constant *getTerminateFn();
 
 private:
   llvm::Constant *
   GetOrCreateLLVMFunction(StringRef MangledName, llvm::Type *Ty, GlobalDecl D,
                           bool ForVTable, bool DontDefer = false,
                           bool IsThunk = false,
-                          llvm::AttributeSet ExtraAttrs = llvm::AttributeSet());
+                          llvm::AttributeSet ExtraAttrs = llvm::AttributeSet(),
+                          bool IsForDefinition = false);
 
   llvm::Constant *GetOrCreateLLVMGlobal(StringRef MangledName,
                                         llvm::PointerType *PTy,
                                         const VarDecl *D);
 
   void setNonAliasAttributes(const Decl *D, llvm::GlobalObject *GO);
 
   /// Set function attributes for a function declaration.
private:
   void EmitFundamentalRTTIDescriptor(QualType Type);
 
   /// Emit any needed decls for which code generation was deferred.
   void EmitDeferred();
 
   /// Call replaceAllUsesWith on all pairs in Replacements.
   void applyReplacements();
 
+  /// Call replaceAllUsesWith on all pairs in GlobalValReplacements.
+  void applyGlobalValReplacements();
+
   void checkAliases();
 
   /// Emit any vtables which we deferred and still have a use for.
   void EmitDeferredVTables();
 
   /// Emit the llvm.used and llvm.compiler.used metadata.
   void emitLLVMUsed();
 

cfe/trunk/lib/CodeGen/CodeGenModule.cpp

for (auto &I : Replacements) {
     if (NewF) {
       NewF->removeFromParent();
       OldF->getParent()->getFunctionList().insertAfter(OldF, NewF);
     }
     OldF->eraseFromParent();
   }
 }
 
+void CodeGenModule::addGlobalValReplacement(llvm::GlobalValue *GV, llvm::Constant *C) {
+  GlobalValReplacements.push_back(std::make_pair(GV, C));
+}
+
+void CodeGenModule::applyGlobalValReplacements() {
+  for (auto &I : GlobalValReplacements) {
+    llvm::GlobalValue *GV = I.first;
+    llvm::Constant *C = I.second;
+
+    GV->replaceAllUsesWith(C);
+    GV->eraseFromParent();
+  }
+}
+
 // This is only used in aliases that we created and we know they have a
 // linear structure.
 static const llvm::GlobalObject *getAliasedGlobal(const llvm::GlobalAlias &GA) {
   llvm::SmallPtrSet<const llvm::GlobalAlias*, 4> Visited;
   const llvm::Constant *C = &GA;
   for (;;) {
     C = C->stripPointerCasts();
     if (auto *GO = dyn_cast<llvm::GlobalObject>(C))
if (VisitedInMainFile > 0 && VisitedInMainFile == MissingInMainFile) {
     Diags.Report(diag::warn_profile_data_unprofiled) << MainFile;
   } else
     Diags.Report(diag::warn_profile_data_out_of_date) << Visited << Missing
                                                       << Mismatched;
 }
 
 void CodeGenModule::Release() {
   EmitDeferred();
+  applyGlobalValReplacements();
   applyReplacements();
   checkAliases();
   EmitCXXGlobalInitFunc();
   EmitCXXGlobalDtorFunc();
   EmitCXXThreadLocalInitFunc();
   if (ObjCRuntime)
     if (llvm::Function *ObjCInitFunction = ObjCRuntime->ModuleInitFunction())
       AddGlobalCtor(ObjCInitFunction);
void CodeGenModule::EmitDeferred() {
   std::vector<DeferredGlobal> CurDeclsToEmit;
   CurDeclsToEmit.swap(DeferredDeclsToEmit);
 
   for (DeferredGlobal &G : CurDeclsToEmit) {
     GlobalDecl D = G.GD;
     llvm::GlobalValue *GV = G.GV;
     G.GV = nullptr;
 
-    assert(!GV || GV == GetGlobalValue(getMangledName(D)));
+    // We should call GetAddrOfGlobal with IsForDefinition set to true in order
+    // to get GlobalValue with exactly the type we need, not something that
+    // might had been created for another decl with the same mangled name but
+    // different type.
+    // FIXME: Support for variables is not implemented yet.
+    if (isa<FunctionDecl>(D.getDecl()))
+      GV = cast<llvm::GlobalValue>(GetAddrOfGlobal(D, /*IsForDefinition=*/true));
+    else
       if (!GV)
         GV = GetGlobalValue(getMangledName(D));
 
     // Check to see if we've already emitted this.  This is necessary
     // for a couple of reasons: first, decls can end up in the
     // deferred-decls queue multiple times, and second, decls can end
     // up with definitions in unusual ways (e.g. by an extern inline
     // function acquiring a strong function redefinition).  Just
     // ignore these cases.
     if (GV && !GV->isDeclaration())
void CodeGenModule::EmitGlobalDefinition(GlobalDecl GD, llvm::GlobalValue *GV) {
   }
 
   if (const auto *VD = dyn_cast<VarDecl>(D))
     return EmitGlobalVarDefinition(VD);
   
   llvm_unreachable("Invalid argument to EmitGlobalDefinition()");
 }
 
+static void ReplaceUsesOfNonProtoTypeWithRealFunction(llvm::GlobalValue *Old,
+                                                      llvm::Function *NewFn);
+
 /// GetOrCreateLLVMFunction - If the specified mangled name is not in the
 /// module, create and return an llvm Function with the specified type. If there
 /// is something in the module with the specified name, return it potentially
 /// bitcasted to the right type.
 ///
 /// If D is non-null, it specifies a decl that correspond to this.  This is used
 /// to set the attributes on the function when it is first created.
 llvm::Constant *
 CodeGenModule::GetOrCreateLLVMFunction(StringRef MangledName,
                                        llvm::Type *Ty,
                                        GlobalDecl GD, bool ForVTable,
                                        bool DontDefer, bool IsThunk,
-                                       llvm::AttributeSet ExtraAttrs) {
+                                       llvm::AttributeSet ExtraAttrs,
+                                       bool IsForDefinition) {
   const Decl *D = GD.getDecl();
 
   // Lookup the entry, lazily creating it if necessary.
   llvm::GlobalValue *Entry = GetGlobalValue(MangledName);
   if (Entry) {
     if (WeakRefReferences.erase(Entry)) {
       const FunctionDecl *FD = cast_or_null<FunctionDecl>(D);
       if (FD && !FD->hasAttr<WeakAttr>())
         Entry->setLinkage(llvm::Function::ExternalLinkage);
     }
 
     // Handle dropped DLL attributes.
     if (D && !D->hasAttr<DLLImportAttr>() && !D->hasAttr<DLLExportAttr>())
       Entry->setDLLStorageClass(llvm::GlobalValue::DefaultStorageClass);
 
-    if (Entry->getType()->getElementType() == Ty)
+    // If there are two attempts to define the same mangled name, issue an
+    // error.
+    if (IsForDefinition && !Entry->isDeclaration()) {
+      GlobalDecl OtherGD;
+      // Check that GD is not yet in ExplicitDefinitions is required to make
+      // sure that we issue an error only once.
+      if (lookupRepresentativeDecl(MangledName, OtherGD) &&
+          (GD.getCanonicalDecl().getDecl() !=
+           OtherGD.getCanonicalDecl().getDecl()) &&
+          DiagnosedConflictingDefinitions.insert(GD).second) {
+        getDiags().Report(D->getLocation(),
+                          diag::err_duplicate_mangled_name);
+        getDiags().Report(OtherGD.getDecl()->getLocation(),
+                          diag::note_previous_definition);
+      }
+    }
+
+    if ((isa<llvm::Function>(Entry) || isa<llvm::GlobalAlias>(Entry)) &&
+        (Entry->getType()->getElementType() == 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 (!IsForDefinition)
       return llvm::ConstantExpr::getBitCast(Entry, Ty->getPointerTo());
   }
 
   // This function doesn't have a complete type (for example, the return
   // type is an incomplete struct). Use a fake type instead, and make
   // sure not to try to set attributes.
   bool IsIncompleteFunction = false;
 
   llvm::FunctionType *FTy;
   if (isa<llvm::FunctionType>(Ty)) {
     FTy = cast<llvm::FunctionType>(Ty);
   } else {
     FTy = llvm::FunctionType::get(VoidTy, false);
     IsIncompleteFunction = true;
   }
-  
-  llvm::Function *F = llvm::Function::Create(FTy,
-                                             llvm::Function::ExternalLinkage,
-                                             MangledName, &getModule());
+  llvm::Function *F =
+      llvm::Function::Create(FTy, llvm::Function::ExternalLinkage,
+                             Entry ? StringRef() : MangledName, &getModule());
+
+  // 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);
+
+    // This might be an implementation of a function without a prototype, in
+    // which case, try to do special replacement of calls which match the new
+    // prototype.  The really key thing here is that we also potentially drop
+    // arguments from the call site so as to make a direct call, which makes the
+    // inliner happier and suppresses a number of optimizer warnings (!) about
+    // dropping arguments.
+    if (!Entry->use_empty()) {
+      ReplaceUsesOfNonProtoTypeWithRealFunction(Entry, F);
+      Entry->removeDeadConstantUsers();
+    }
+
+    llvm::Constant *BC = llvm::ConstantExpr::getBitCast(
+        F, Entry->getType()->getElementType()->getPointerTo());
+    addGlobalValReplacement(Entry, BC);
+  }
+
   assert(F->getName() == MangledName && "name was uniqued!");
   if (D)
     SetFunctionAttributes(GD, F, IsIncompleteFunction, IsThunk);
   if (ExtraAttrs.hasAttributes(llvm::AttributeSet::FunctionIndex)) {
     llvm::AttrBuilder B(ExtraAttrs, llvm::AttributeSet::FunctionIndex);
     F->addAttributes(llvm::AttributeSet::FunctionIndex,
                      llvm::AttributeSet::get(VMContext,
                                              llvm::AttributeSet::FunctionIndex,

 }
 
 /// GetAddrOfFunction - Return the address of the given function.  If Ty is
 /// non-null, then this function will use the specified type if it has to
 /// create it (this occurs when we see a definition of the function).
 llvm::Constant *CodeGenModule::GetAddrOfFunction(GlobalDecl GD,
                                                  llvm::Type *Ty,
                                                  bool ForVTable,
-                                                 bool DontDefer) {
+                                                 bool DontDefer,
+                                                 bool IsForDefinition) {
   // If there was no specific requested type, just convert it now.
   if (!Ty)
     Ty = getTypes().ConvertType(cast<ValueDecl>(GD.getDecl())->getType());
-  
   StringRef MangledName = getMangledName(GD);
-  return GetOrCreateLLVMFunction(MangledName, Ty, GD, ForVTable, DontDefer);
+  return GetOrCreateLLVMFunction(MangledName, Ty, GD, ForVTable, DontDefer,
+                                 /*IsThunk=*/false, llvm::AttributeSet(),
+                                 IsForDefinition);
 }
 
 /// CreateRuntimeFunction - Create a new runtime function with the specified
 /// type and name.
 llvm::Constant *
 CodeGenModule::CreateRuntimeFunction(llvm::FunctionType *FTy,
                                      StringRef Name,
                                      llvm::AttributeSet ExtraAttrs) {
CodeGenModule::GetOrCreateLLVMGlobal(StringRef MangledName,
   }
 
   if (AddrSpace != Ty->getAddressSpace())
     return llvm::ConstantExpr::getAddrSpaceCast(GV, Ty);
 
   return GV;
 }
 
+llvm::Constant *
+CodeGenModule::GetAddrOfGlobal(GlobalDecl GD,
+                               bool IsForDefinition) {
+  if (isa<CXXConstructorDecl>(GD.getDecl()))
+    return getAddrOfCXXStructor(cast<CXXConstructorDecl>(GD.getDecl()),
+                                getFromCtorType(GD.getCtorType()),
+                                /*FnInfo=*/nullptr, /*FnType=*/nullptr,
+                                /*DontDefer=*/false, IsForDefinition);
+  else if (isa<CXXDestructorDecl>(GD.getDecl()))
+    return getAddrOfCXXStructor(cast<CXXDestructorDecl>(GD.getDecl()),
+                                getFromDtorType(GD.getDtorType()),
+                                /*FnInfo=*/nullptr, /*FnType=*/nullptr,
+                                /*DontDefer=*/false, IsForDefinition);
+  else if (isa<CXXMethodDecl>(GD.getDecl())) {
+    auto FInfo = &getTypes().arrangeCXXMethodDeclaration(
+        cast<CXXMethodDecl>(GD.getDecl()));
+    auto Ty = getTypes().GetFunctionType(*FInfo);
+    return GetAddrOfFunction(GD, Ty, /*ForVTable=*/false, /*DontDefer=*/false,
+                             IsForDefinition);
+  } else if (isa<FunctionDecl>(GD.getDecl())) {
+    const CGFunctionInfo &FI = getTypes().arrangeGlobalDeclaration(GD);
+    llvm::FunctionType *Ty = getTypes().GetFunctionType(FI);
+    return GetAddrOfFunction(GD, Ty, /*ForVTable=*/false, /*DontDefer=*/false,
+                             IsForDefinition);
+  } else
+    return GetAddrOfGlobalVar(cast<VarDecl>(GD.getDecl()));
+}
 
 llvm::GlobalVariable *
 CodeGenModule::CreateOrReplaceCXXRuntimeVariable(StringRef Name, 
                                       llvm::Type *Ty,
                                       llvm::GlobalValue::LinkageTypes Linkage) {
   llvm::GlobalVariable *GV = getModule().getNamedGlobal(Name);
   llvm::GlobalVariable *OldGV = nullptr;
 
void CodeGenModule::EmitGlobalFunctionDefinition(GlobalDecl GD,
                                                  llvm::GlobalValue *GV) {
   const auto *D = cast<FunctionDecl>(GD.getDecl());
 
   // Compute the function info and LLVM type.
   const CGFunctionInfo &FI = getTypes().arrangeGlobalDeclaration(GD);
   llvm::FunctionType *Ty = getTypes().GetFunctionType(FI);
 
   // Get or create the prototype for the function.
-  if (!GV) {
-    llvm::Constant *C =
-        GetAddrOfFunction(GD, Ty, /*ForVTable=*/false, /*DontDefer*/ true);
+  if (!GV || (GV->getType()->getElementType() != Ty))
+    GV = cast<llvm::GlobalValue>(GetAddrOfFunction(GD, Ty, /*ForVTable=*/false,
+                                                   /*DontDefer=*/true,
+                                                   /*IsForDefinition=*/true));
 
-    // Strip off a bitcast if we got one back.
-    if (auto *CE = dyn_cast<llvm::ConstantExpr>(C)) {
-      assert(CE->getOpcode() == llvm::Instruction::BitCast);
-      GV = cast<llvm::GlobalValue>(CE->getOperand(0));
-    } else {
-      GV = cast<llvm::GlobalValue>(C);
-    }
-  }
-
-  if (!GV->isDeclaration()) {
-    getDiags().Report(D->getLocation(), diag::err_duplicate_mangled_name);
-    GlobalDecl OldGD = Manglings.lookup(GV->getName());
-    if (auto *Prev = OldGD.getDecl())
-      getDiags().Report(Prev->getLocation(), diag::note_previous_definition);
+  // Already emitted.
+  if (!GV->isDeclaration())
     return;
-  }
-
-  if (GV->getType()->getElementType() != Ty) {
-    // If the types mismatch then we have to rewrite the definition.
-    assert(GV->isDeclaration() && "Shouldn't replace non-declaration");
-
-    // F is the Function* for the one with the wrong type, we must make a new
-    // Function* and update everything that used F (a declaration) with the new
-    // Function* (which will be a definition).
-    //
-    // 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)").  Move the old function aside so that it
-    // doesn't interfere with GetAddrOfFunction.
-    GV->setName(StringRef());
-    auto *NewFn = cast<llvm::Function>(GetAddrOfFunction(GD, Ty));
-
-    // This might be an implementation of a function without a
-    // prototype, in which case, try to do special replacement of
-    // calls which match the new prototype.  The really key thing here
-    // is that we also potentially drop arguments from the call site
-    // so as to make a direct call, which makes the inliner happier
-    // and suppresses a number of optimizer warnings (!) about
-    // dropping arguments.
-    if (!GV->use_empty()) {
-      ReplaceUsesOfNonProtoTypeWithRealFunction(GV, NewFn);
-      GV->removeDeadConstantUsers();
-    }
-
-    // Replace uses of F with the Function we will endow with a body.
-    if (!GV->use_empty()) {
-      llvm::Constant *NewPtrForOldDecl =
-          llvm::ConstantExpr::getBitCast(NewFn, GV->getType());
-      GV->replaceAllUsesWith(NewPtrForOldDecl);
-    }
-
-    // Ok, delete the old function now, which is dead.
-    GV->eraseFromParent();
-
-    GV = NewFn;
-  }
 
   // We need to set linkage and visibility on the function before
   // generating code for it because various parts of IR generation
   // want to propagate this information down (e.g. to local static
   // declarations).
   auto *Fn = cast<llvm::Function>(GV);
   setFunctionLinkage(GD, Fn);
   setFunctionDLLStorageClass(GD, Fn);

cfe/trunk/lib/CodeGen/ItaniumCXXABI.cpp

if (Type == StructorType::Complete) {
 
     if (CGType == StructorCodegen::Alias || CGType == StructorCodegen::COMDAT) {
       emitConstructorDestructorAlias(CGM, CompleteDecl, BaseDecl);
       return;
     }
 
     if (CGType == StructorCodegen::RAUW) {
       StringRef MangledName = CGM.getMangledName(CompleteDecl);
-      auto *Aliasee = cast<llvm::GlobalValue>(CGM.GetAddrOfGlobal(BaseDecl));
+      auto *Aliasee = CGM.GetAddrOfGlobal(BaseDecl);
       CGM.addReplacement(MangledName, Aliasee);
       return;
     }
   }
 
   // The base destructor is equivalent to the base destructor of its
   // base class if there is exactly one non-virtual base class with a
   // non-trivial destructor, there are no fields with a non-trivial

cfe/trunk/test/CodeGenCXX/duplicate-mangled-name.cpp

-// RUN: %clang_cc1 -triple %itanium_abi_triple -emit-llvm-only %s -verify
+// RUN: %clang_cc1 -triple %itanium_abi_triple -emit-llvm-only %s -verify -DTEST1
+// RUN: %clang_cc1 -triple %itanium_abi_triple -emit-llvm-only %s -verify -DTEST2
+
+#ifdef TEST1
 
 // rdar://15522601
 class MyClass {
  static void meth();
 };
 void MyClass::meth() { } // expected-note {{previous}}
 extern "C" {
   void _ZN7MyClass4methEv() { } // expected-error {{definition with same mangled name as another definition}}
 }
+
+#elif TEST2
+
+// We expect no warnings here, as there is only declaration of _ZN1TD1Ev function, no definitions.
+extern "C" void _ZN1TD1Ev();
+struct T {
+  ~T() {}
+};
+
+void foo() {
+  _ZN1TD1Ev();
+  T t;
+}
+
+extern "C" void _ZN2T2D2Ev() {}; // expected-note {{previous definition is here}}
+
+struct T2 {
+  ~T2() {} // expected-error {{definition with same mangled name as another definition}}
+};
+
+void bar() {
+  _ZN2T2D2Ev();
+  T2 t;
+}
+
+#else
+
+#error Unknwon test
+
+#endif
+