Re: [PATCH] D11297: PR17829: Functions declared extern "C" with a name matching a mangled C++ function are allowed
andreybokhanko added a comment. In http://reviews.llvm.org/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 Repository: rL LLVM http://reviews.llvm.org/D11297 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D11297: PR17829: Functions declared extern "C" with a name matching a mangled C++ function are allowed
This revision was automatically updated to reflect the committed changes.
Closed by commit rL246438: PR17829: Proper diagnostic of mangled names
conflicts (authored by asbokhan).
Changed prior to commit:
http://reviews.llvm.org/D11297?vs=33401=33577#toc
Repository:
rL LLVM
http://reviews.llvm.org/D11297
Files:
cfe/trunk/lib/CodeGen/CGCXX.cpp
cfe/trunk/lib/CodeGen/CodeGenModule.cpp
cfe/trunk/lib/CodeGen/CodeGenModule.h
cfe/trunk/lib/CodeGen/ItaniumCXXABI.cpp
cfe/trunk/test/CodeGenCXX/duplicate-mangled-name.cpp
Index: cfe/trunk/test/CodeGenCXX/duplicate-mangled-name.cpp
===
--- cfe/trunk/test/CodeGenCXX/duplicate-mangled-name.cpp
+++ cfe/trunk/test/CodeGenCXX/duplicate-mangled-name.cpp
@@ -1,4 +1,7 @@
-// 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 {
@@ -8,3 +11,34 @@
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
+
Index: cfe/trunk/lib/CodeGen/CodeGenModule.h
===
--- cfe/trunk/lib/CodeGen/CodeGenModule.h
+++ cfe/trunk/lib/CodeGen/CodeGenModule.h
@@ -342,6 +342,17 @@
typedef llvm::StringMap 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
+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 DiagnosedConflictingDefinitions;
+
/// A queue of (optional) vtables to consider emitting.
std::vector DeferredVTables;
@@ -682,18 +693,7 @@
llvm_unreachable("unknown visibility!");
}
- llvm::Constant *GetAddrOfGlobal(GlobalDecl GD) {
-if (isa(GD.getDecl()))
- return getAddrOfCXXStructor(cast(GD.getDecl()),
- getFromCtorType(GD.getCtorType()));
-else if (isa(GD.getDecl()))
- return getAddrOfCXXStructor(cast(GD.getDecl()),
- getFromDtorType(GD.getDtorType()));
-else if (isa(GD.getDecl()))
- return GetAddrOfFunction(GD);
-else
- return GetAddrOfGlobalVar(cast(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
@@ -725,7 +725,8 @@
/// 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);
@@ -847,11 +848,11 @@
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".
@@ -1122,6 +1123,8 @@
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);
@@ -1148,7
Re: [PATCH] D11297: PR17829: Functions declared extern C with a name matching a mangled C++ function are allowed
andreybokhanko added inline comments. Comment at: include/clang/Basic/DiagnosticSemaKinds.td:2323 @@ -2323,1 +2322,3 @@ + definition with same mangled name as another definition, + InGroupDuplicateMangledNames; def err_cyclic_alias : Error rjmccall wrote: 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? 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 http://reviews.llvm.org/D11297 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D11297: PR17829: Functions declared extern C with a name matching a mangled C++ function are allowed
rjmccall added a comment. Yes, please make it an error. And the obvious test changes are fine. :) http://reviews.llvm.org/D11297 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D11297: PR17829: Functions declared extern C with a name matching a mangled C++ function are allowed
andreybokhanko marked 3 inline comments as done.
andreybokhanko added a comment.
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
Comment at: lib/CodeGen/CodeGenModule.h:354
@@ +353,3 @@
+ /// call).
+ llvm::DenseSetGlobalDecl ExplicitDefinitions;
+
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.
http://reviews.llvm.org/D11297
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D11297: PR17829: Functions declared extern C with a name matching a mangled C++ function are allowed
rjmccall added inline comments.
Comment at: lib/CodeGen/CodeGenModule.h:354
@@ +353,3 @@
+ /// call).
+ llvm::DenseSetGlobalDecl ExplicitDefinitions;
+
andreybokhanko wrote:
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.
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) {
...
}
http://reviews.llvm.org/D11297
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D11297: PR17829: Functions declared extern C with a name matching a mangled C++ function are allowed
andreybokhanko updated this revision to Diff 33083.
andreybokhanko added a comment.
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
http://reviews.llvm.org/D11297
Files:
include/clang/Basic/DiagnosticGroups.td
include/clang/Basic/DiagnosticSemaKinds.td
lib/CodeGen/CGCXX.cpp
lib/CodeGen/CodeGenModule.cpp
lib/CodeGen/CodeGenModule.h
lib/CodeGen/ItaniumCXXABI.cpp
test/CodeGenCXX/duplicate-mangled-name.cpp
Index: lib/CodeGen/CodeGenModule.cpp
===
--- lib/CodeGen/CodeGenModule.cpp
+++ lib/CodeGen/CodeGenModule.cpp
@@ -237,6 +237,20 @@
}
}
+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) {
@@ -339,6 +353,7 @@
void CodeGenModule::Release() {
EmitDeferred();
+ applyGlobalValReplacements();
applyReplacements();
checkAliases();
EmitCXXGlobalInitFunc();
@@ -1108,8 +1123,19 @@
llvm::GlobalValue *GV = G.GV;
G.GV = nullptr;
-assert(!GV || GV == GetGlobalValue(getMangledName(D)));
-if (!GV)
+// Name of this global value is taken = it will be replaced, so no need to
+// emit it.
+if (GV GV != GetGlobalValue(getMangledName(D)))
+ continue;
+
+// 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 (isaFunctionDecl(D.getDecl()))
+ GV = castllvm::GlobalValue(GetAddrOfGlobal(D, /*IsForDefinition=*/true));
+else
GV = GetGlobalValue(getMangledName(D));
// Check to see if we've already emitted this. This is necessary
@@ -1541,6 +1567,140 @@
llvm_unreachable(Invalid argument to EmitGlobalDefinition());
}
+/// Replace the uses of a function that was declared with a non-proto type.
+/// We want to silently drop extra arguments from call sites
+static void replaceUsesOfNonProtoConstant(llvm::Constant *old,
+ llvm::Function *newFn) {
+ // Fast path.
+ if (old-use_empty()) return;
+
+ llvm::Type *newRetTy = newFn-getReturnType();
+ SmallVectorllvm::Value*, 4 newArgs;
+
+ for (llvm::Value::use_iterator ui = old-use_begin(), ue = old-use_end();
+ ui != ue; ) {
+llvm::Value::use_iterator use = ui++; // Increment before the use is erased.
+llvm::User *user = use-getUser();
+
+// Recognize and replace uses of bitcasts. Most calls to
+// unprototyped functions will use bitcasts.
+if (auto *bitcast = dyn_castllvm::ConstantExpr(user)) {
+ if (bitcast-getOpcode() == llvm::Instruction::BitCast)
+replaceUsesOfNonProtoConstant(bitcast, newFn);
+ continue;
+}
+
+// Recognize calls to the function.
+llvm::CallSite callSite(user);
+if (!callSite) continue;
+if (!callSite.isCallee(*use)) continue;
+
+// If the return types don't match exactly, then we can't
+// transform this call unless it's dead.
+if (callSite-getType() != newRetTy !callSite-use_empty())
+ continue;
+
+// Get the call site's attribute list.
+SmallVectorllvm::AttributeSet, 8 newAttrs;
+llvm::AttributeSet oldAttrs = callSite.getAttributes();
+
+// Collect any return attributes from the call.
+if (oldAttrs.hasAttributes(llvm::AttributeSet::ReturnIndex))
+ newAttrs.push_back(
+llvm::AttributeSet::get(newFn-getContext(),
+oldAttrs.getRetAttributes()));
+
+// If the function was passed too few arguments, don't transform.
+unsigned newNumArgs = newFn-arg_size();
+if (callSite.arg_size() newNumArgs) continue;
+
+// If extra arguments were passed, we silently drop them.
+// If any of the types mismatch, we don't transform.
+unsigned argNo = 0;
+bool dontTransform = false;
+for (llvm::Function::arg_iterator ai = newFn-arg_begin(),
+ ae = newFn-arg_end(); ai != ae; ++ai, ++argNo) {
+ if (callSite.getArgument(argNo)-getType() != ai-getType()) {
+
Re: [PATCH] D11297: PR17829: Functions declared extern C with a name matching a mangled C++ function are allowed
rjmccall added a comment.
This looks generally like what I'm looking for, thanks! Some comments.
Comment at: lib/CodeGen/CodeGenModule.cpp:1129
@@ +1128,3 @@
+if (GV GV != GetGlobalValue(getMangledName(D)))
+ continue;
+
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.
Comment at: lib/CodeGen/CodeGenModule.cpp:1573
@@ +1572,3 @@
+static void replaceUsesOfNonProtoConstant(llvm::Constant *old,
+ llvm::Function *newFn) {
+ // Fast path.
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.
Comment at: lib/CodeGen/CodeGenModule.h:349
@@ +348,3 @@
+ llvm::SmallVectorstd::pairllvm::GlobalValue *, llvm::Constant *, 8
+GlobalValReplacements;
+
Missing a period in the comment.
Comment at: lib/CodeGen/CodeGenModule.h:354
@@ +353,3 @@
+ /// call).
+ llvm::DenseSetGlobalDecl ExplicitDefinitions;
+
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.
http://reviews.llvm.org/D11297
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D11297: PR17829: Functions declared extern C with a name matching a mangled C++ function are allowed
andreybokhanko added a comment.
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
http://reviews.llvm.org/D11297
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D11297: PR17829: Functions declared extern C with a name matching a mangled C++ function are allowed
rjmccall added a comment.
In http://reviews.llvm.org/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?
http://reviews.llvm.org/D11297
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D11297: PR17829: Functions declared extern C with a name matching a mangled C++ function are allowed
rjmccall added a comment. 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. http://reviews.llvm.org/D11297 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
