[Clang] Emit error for duplicate mangled names within a lambda #107581
Conversation
When functions are passed as arguments to a lambda, it's possible for
the mangled names of these functions to be the same despite the prototypes
being different. For example:
int non_streaming_fn(int);
int streaming_fn(int) __arm_streaming;
auto lambda_fn = [](const auto &f) { return f(); };
return lambda_fn(non_streaming_fn) + lambda_fn(streaming_fn);
Only one function will be generated for the lambda above and the __arm_streaming
attribute from streaming_fn will be incorrectly applied to non_streaming_fn.
With this change, an error will be emitted when a duplicate mangled name is
found but the function prototypes do not match.
llvmbot
added
clang
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-codegen Author: Kerry McLaughlin (kmclaughlin-arm) ChangesWhen functions are passed as arguments to a lambda, it's possible for Only one function will be generated for the lambda above and the __arm_streaming Full diff: https://github.com/llvm/llvm-project/pull/107581.diff 2 Files Affected:
diff --git a/clang/lib/CodeGen/CodeGenModule.cpp b/clang/lib/CodeGen/CodeGenModule.cpp
index df4c13c9ad97aa..b07090e3de6dfd 100644
--- a/clang/lib/CodeGen/CodeGenModule.cpp
+++ b/clang/lib/CodeGen/CodeGenModule.cpp
@@ -4638,8 +4638,8 @@ llvm::Constant *CodeGenModule::GetOrCreateLLVMFunction(
// Lookup the entry, lazily creating it if necessary.
llvm::GlobalValue *Entry = GetGlobalValue(MangledName);
if (Entry) {
+ const FunctionDecl *FD = cast_or_null<FunctionDecl>(D);
if (WeakRefReferences.erase(Entry)) {
- const FunctionDecl *FD = cast_or_null<FunctionDecl>(D);
if (FD && !FD->hasAttr<WeakAttr>())
Entry->setLinkage(llvm::Function::ExternalLinkage);
}
@@ -4652,18 +4652,35 @@ llvm::Constant *CodeGenModule::GetOrCreateLLVMFunction(
// 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 DiagnosedConflictingDefinitions 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) {
+ GlobalDecl OtherGD;
+ // Check that GD is not yet in DiagnosedConflictingDefinitions is required
+ // to make sure that we issue an error only once.
+ if (GD && lookupRepresentativeDecl(MangledName, OtherGD) &&
+ (GD.getCanonicalDecl().getDecl() !=
+ OtherGD.getCanonicalDecl().getDecl()) &&
+ DiagnosedConflictingDefinitions.insert(GD).second) {
+ if (IsForDefinition && !Entry->isDeclaration()) {
getDiags().Report(D->getLocation(), diag::err_duplicate_mangled_name)
<< MangledName;
getDiags().Report(OtherGD.getDecl()->getLocation(),
diag::note_previous_definition);
+ } else {
+ // For lambdas, it's possible to create the same mangled name from
+ // different function prototypes. For example, two FPTs may have
+ // identical types but incompatible function attributes which we should
+ // not allow.
+ auto *MD = dyn_cast<CXXMethodDecl>(D);
+ if (MD && MD->getParent()->isLambda()) {
+ const FunctionDecl *OtherFD =
+ cast_or_null<FunctionDecl>(OtherGD.getDecl());
+ if (FD && FD->hasPrototype() && OtherFD && OtherFD->hasPrototype()) {
+ if (FD->getType()->getAs<FunctionProtoType>() !=
+ OtherFD->getType()->getAs<FunctionProtoType>())
+ getDiags().Report(D->getLocation(),
+ diag::err_duplicate_mangled_name)
+ << MangledName;
+ }
+ }
}
}
diff --git a/clang/test/CodeGenCXX/aarch64-sme-lambda-attributes.cpp b/clang/test/CodeGenCXX/aarch64-sme-lambda-attributes.cpp
new file mode 100644
index 00000000000000..bce3e657001679
--- /dev/null
+++ b/clang/test/CodeGenCXX/aarch64-sme-lambda-attributes.cpp
@@ -0,0 +1,28 @@
+// REQUIRES: aarch64-registered-target
+
+// RUN: %clang_cc1 -triple aarch64 -target-feature +sme -target-feature +sme2 -emit-llvm -o - %s -verify -DTEST1
+// RUN: %clang_cc1 -triple aarch64 -target-feature +sme -target-feature +sme2 -emit-llvm -o - %s -verify -DTEST2
+
+int normal_fn(int);
+int streaming_fn(int) __arm_streaming;
+int streaming_compatible_fn(int) __arm_streaming_compatible;
+
+#ifdef TEST1
+
+// expected-error@+2 {{definition with same mangled name '_ZZ32function_params_normal_streamingvENK3$_0clIFiiEEEDaRT_' as another definition}}
+int function_params_normal_streaming() {
+ auto a = [](auto &fn) { return fn(42); };
+ return a(normal_fn) + a(streaming_fn);
+}
+
+#endif
+
+#ifdef TEST2
+
+// expected-error@+2 {{definition with same mangled name '_ZZ36function_params_streaming_compatiblevENK3$_0clIFiiEEEDaRT_' as another definition}}
+int function_params_streaming_compatible() {
+ auto a = [](auto &fn) { return fn(42); };
+ return a(streaming_fn) + a(streaming_compatible_fn);
+}
+
+#endif
|
kmclaughlin-arm
requested review from
SamTebbs33,
huntergr-arm and
sdesmalen-arm
|
Isn't this a bug in the mangler? I mean, it's better to print an error rather than silently miscompile, but this doesn't really solve the issue. |
Hi @efriedma-quic, |
If |
|
(You shouldn't need to special-case lambdas; any use of the type needs the appropriate mangling, except maybe the function declaration itself.) |
I agree that it would be better for the SME attributes to be represented in the type mangling, although changing the mangling at this point would be an ABI break. I'll probe and follow this up! Nevertheless I believe there is merit to this patch because Clang should not silently generate wrong code. For other instantiations Clang just emits a diagnostic, e.g. Results in a diagnostic |
|
Sure, it makes sense to print a diagnostic for lambdas. I'm having a bit of trouble understanding the way the new code is structured. What makes the definition of lambda call operators special here? Do we not call GetOrCreateLLVMFunction with IsForDefinition set? |
When I added this error I incorrectly thought GetOrCreateLLVMFunction is not called with IsForDefinition set. Now that I've taken another look at it, I see that it is set and the reason the existing error is not emitted is because of the I have removed most of my changes and instead extended the |
clang/lib/CodeGen/CodeGenModule.cpp
Outdated
| const FunctionDecl *OtherFD = | ||
| cast_or_null<FunctionDecl>(OtherGD.getDecl()); | ||
| if (FD && FD->hasPrototype() && OtherFD && OtherFD->hasPrototype()) { | ||
| if (FD->getType()->getAs<FunctionProtoType>() != | ||
| OtherFD->getType()->getAs<FunctionProtoType>()) |
There was a problem hiding this comment.
I don't understand why there's a need to check the FunctionProtoTypes here, because the check GD.getCanonicalDecl().getDecl() != OtherGD.getCanonicalDecl().getDecl() has already checked if the declarations and types are the same.
Note that I tried a smaller change like this:
bool IsLambdaInstantiation =
D && isa<CXXMethodDecl>(D) &&
cast<CXXMethodDecl>(D)->getParent()->isLambda();
if (IsForDefinition && (!Entry->isDeclaration() || IsLambdaInstantiation)) {
And got the desired result. Would the above be sufficient or is there something this is not covering?
There was a problem hiding this comment.
(I see you implemented the same suggestion, not sure why Github didn't publish my comment)
| #ifdef TEST2 | ||
|
|
||
| // expected-error@+3 {{definition with same mangled name '_ZZ36function_params_streaming_compatiblevENK3$_0clIFiiEEEDaRT_' as another definition}} | ||
| // expected-note@+2 {{previous definition is here}} | ||
| int function_params_streaming_compatible() { | ||
| auto a = [](auto &fn) { return fn(42); }; | ||
| return a(streaming_fn) + a(streaming_compatible_fn); | ||
| } | ||
|
|
||
| #endif |
There was a problem hiding this comment.
This test adds much value over the other test above, because the logic is no different for streaming and streaming-compatible attributes, so I think you can remove it.
There was a problem hiding this comment.
Can you add a separate test for a non-SME attribute that has a similar issue?
Type attributes for the calling convention are also not part of the type mangling, e.g. https://godbolt.org/z/sEdsGr53K which would (without your change) miscompile.
- Added a test using calling convention attributes
| // RUN: %clang_cc1 -triple aarch64 -emit-llvm -o - %s -verify -DTEST1 | ||
| // RUN: %clang_cc1 -triple aarch64 -target-feature +sme -target-feature +sme2 -emit-llvm -o - %s -verify -DTEST2 |
There was a problem hiding this comment.
Can you now change the filename, since it's now wider scope than just the SME attributes. Maybe lambda-instantiation-require-different-mangled-names.cpp or something?
The same issue exists for more generic ways to specify the calling convention, e.g. __attribute__((preserve_none)), which also affects other targets. For these, the compiler could select the worst-case calling convention for its instantiation (i.e. assume everything is clobbered), rather than emitting a diagnostic. But that would require explicit logic to select the right calling convention, which is out of scope of this PR.
nit: rather than using ifdef's, I'd recommend using the split-file tool instead (it does mean duplicating the normal_fn declaration)
There was a problem hiding this comment.
I've removed the ifdefs & renamed this to lambda-instantiation-mangling-conflicts.cpp as it was a bit shorter, but I can rename to lambda-instantiation-require-different-mangled-names.cpp if preferred.
For non-lambda methods, the way this works it that we call GetOrCreateLLVMFunction for both methods... for the first method, the |
…angling-conflicts.cpp - Replaced ifdefs in test with use of split-file
I don't think lambda methods are any different to what you have described, but I think my previous explanation was unclear. What I found is that IsForDefinition is set only the second time GetOrCreateLLVMFunction is called, where |
|
In the example in #107581 (comment) , there are four relevant calls to GetOrCreateLLVMFunction: one for each function with IsInDefinition false, and one for each function with IsInDefinition true. The last of those calls triggers the diagnostic For the lambda example, there are only three relevant calls to GetOrCreateLLVMFunction; one for each function with IsInDefinition false, but then only one with IsInDefinition true. It's not clear to me why the two cases are different, and I don't really want to add lambda-specific check without knowing why lambdas are special here. Maybe the right condition to check is something else. |
From what I've observed comparing the lambda and template test cases, the difference comes from how we create the list of Decls which can be deferred. Both functions For the template case this happens first, before calling EmitGlobal for Although we'll set DeferredDecls with the mangled name from the lambda twice, it will only appear in DeferredDeclsToEmit once. In the other example, it will be added to the DeferredDeclsToEmit list with addDeferredDeclToEmit both times. When EmitDeferred is called later it uses this list and calls GetOrCreateLLVMFunction with IsForDefinition set to true. I'm not sure adding a check specificially for lambdas is the right thing to do because of this. I considered trying to emit the error somewhere in lib/Sema, however I think this is too early to check whether functions will have the same mangled name. Something I'm considering instead is to try and emit the error in EmitGlobal, where we set DeferredDecls for a given MangledName (around I will also be away for the next two weeks but plan to pick this up again when I return. |
|
Oh, that makes sense... so the issue is generally with functions we emit lazily? I'd say it's reasonable to emit an error if we have two definitions for the same symbol, even if we don't end up emitting them because they're deferred. Lazy emission is an optimization, and we don't guarantee whether a particular odr-used function will be emitted. |
Yes, I believe so. I've moved the error to EmitGlobal at the point where DeferredDecls is updated if a duplicate mangled name is found. I've tried to exclude the cases where this can be expected (such as for multiversioning where the mangled name may be changed later by UpdateMultiVersionNames), but there is nothing specific to lambdas now.
Following on from this, I am also working on changes to name mangling of types which have SME attributes (ARM-software/abi-aa#290) |
|
Can you add a testcase that doesn't involve lambdas? Maybe something like the following: |
When functions are passed as arguments to a lambda, it's possible for
the mangled names of these functions to be the same despite the prototypes
being different. For example:
Only one function will be generated for the lambda above and the __arm_streaming
attribute from streaming_fn will be incorrectly applied to the non_streaming_fn call.
With this change, an error will be emitted when a duplicate mangled name is found
but the function prototypes do not match.