https://github.com/kmclaughlin-arm updated https://github.com/llvm/llvm-project/pull/107581
>From 1e6f25c517d8d1adeeaf59f826141efdcad8f05a Mon Sep 17 00:00:00 2001 From: Kerry McLaughlin <kerry.mclaugh...@arm.com> Date: Fri, 6 Sep 2024 10:13:33 +0000 Subject: [PATCH 1/5] [Clang] Emit error for duplicate mangled names within a lambda 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. --- clang/lib/CodeGen/CodeGenModule.cpp | 35 ++++++++++++++----- .../aarch64-sme-lambda-attributes.cpp | 28 +++++++++++++++ 2 files changed, 54 insertions(+), 9 deletions(-) create mode 100644 clang/test/CodeGenCXX/aarch64-sme-lambda-attributes.cpp 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 >From fc1394eec345702eea7f3c664e5fc33ce3c525d0 Mon Sep 17 00:00:00 2001 From: Kerry McLaughlin <kerry.mclaugh...@arm.com> Date: Fri, 13 Sep 2024 08:58:02 +0000 Subject: [PATCH 2/5] - Rewrite changes to GetOrCreateLLVMFunction --- clang/lib/CodeGen/CodeGenModule.cpp | 37 ++++++------------- .../aarch64-sme-lambda-attributes.cpp | 6 ++- 2 files changed, 15 insertions(+), 28 deletions(-) diff --git a/clang/lib/CodeGen/CodeGenModule.cpp b/clang/lib/CodeGen/CodeGenModule.cpp index b07090e3de6dfd..e0cfbdf57534c7 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,35 +4652,20 @@ llvm::Constant *CodeGenModule::GetOrCreateLLVMFunction( // If there are two attempts to define the same mangled name, issue an // error. - 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()) { + auto *MD = dyn_cast_or_null<CXXMethodDecl>(D); + bool IsLambda = MD && MD->getParent()->isLambda(); + if (IsForDefinition && (!Entry->isDeclaration() || IsLambda)) { + 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) { 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 index bce3e657001679..d7667bf3343e75 100644 --- a/clang/test/CodeGenCXX/aarch64-sme-lambda-attributes.cpp +++ b/clang/test/CodeGenCXX/aarch64-sme-lambda-attributes.cpp @@ -9,7 +9,8 @@ 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}} +// expected-error@+3 {{definition with same mangled name '_ZZ32function_params_normal_streamingvENK3$_0clIFiiEEEDaRT_' as another definition}} +// expected-note@+2 {{previous definition is here}} int function_params_normal_streaming() { auto a = [](auto &fn) { return fn(42); }; return a(normal_fn) + a(streaming_fn); @@ -19,7 +20,8 @@ int function_params_normal_streaming() { #ifdef TEST2 -// expected-error@+2 {{definition with same mangled name '_ZZ36function_params_streaming_compatiblevENK3$_0clIFiiEEEDaRT_' as another definition}} +// 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); >From 33443e62afea03ebe22dd27ad2815f668b8da293 Mon Sep 17 00:00:00 2001 From: Kerry McLaughlin <kerry.mclaugh...@arm.com> Date: Mon, 16 Sep 2024 10:54:03 +0000 Subject: [PATCH 3/5] - Removed second SME attributes test - Added a test using calling convention attributes --- .../aarch64-sme-lambda-attributes.cpp | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/clang/test/CodeGenCXX/aarch64-sme-lambda-attributes.cpp b/clang/test/CodeGenCXX/aarch64-sme-lambda-attributes.cpp index d7667bf3343e75..9e9a3d09de24fd 100644 --- a/clang/test/CodeGenCXX/aarch64-sme-lambda-attributes.cpp +++ b/clang/test/CodeGenCXX/aarch64-sme-lambda-attributes.cpp @@ -1,30 +1,31 @@ // 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 -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; +int vector_pcs_fn(int) __attribute__((aarch64_vector_pcs)); #ifdef TEST1 -// expected-error@+3 {{definition with same mangled name '_ZZ32function_params_normal_streamingvENK3$_0clIFiiEEEDaRT_' as another definition}} -// expected-note@+2 {{previous definition is here}} -int function_params_normal_streaming() { +// expected-error@+4 {{definition with same mangled name '_ZZ23function_pcs_attributesvENK3$_0clIFiiEEEDaRT_' as another definition}} +// expected-note@+3 {{previous definition is here}} +__attribute__((aarch64_vector_pcs)) +int function_pcs_attributes() { auto a = [](auto &fn) { return fn(42); }; - return a(normal_fn) + a(streaming_fn); + return a(normal_fn) + a(vector_pcs_fn); } #endif #ifdef TEST2 -// expected-error@+3 {{definition with same mangled name '_ZZ36function_params_streaming_compatiblevENK3$_0clIFiiEEEDaRT_' as another definition}} +// expected-error@+3 {{definition with same mangled name '_ZZ32function_params_normal_streamingvENK3$_0clIFiiEEEDaRT_' as another definition}} // expected-note@+2 {{previous definition is here}} -int function_params_streaming_compatible() { +int function_params_normal_streaming() { auto a = [](auto &fn) { return fn(42); }; - return a(streaming_fn) + a(streaming_compatible_fn); + return a(normal_fn) + a(streaming_fn); } #endif >From 4b6d327fe6b6a552fe16e27f7b8b15ce5d2055da Mon Sep 17 00:00:00 2001 From: Kerry McLaughlin <kerry.mclaugh...@arm.com> Date: Mon, 16 Sep 2024 16:22:41 +0000 Subject: [PATCH 4/5] - Renamed aarch64-sme-lambda-attributes.cpp to lambda-instantiation-mangling-conflicts.cpp - Replaced ifdefs in test with use of split-file --- ...mbda-instantiation-mangling-conflicts.cpp} | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) rename clang/test/CodeGenCXX/{aarch64-sme-lambda-attributes.cpp => lambda-instantiation-mangling-conflicts.cpp} (78%) diff --git a/clang/test/CodeGenCXX/aarch64-sme-lambda-attributes.cpp b/clang/test/CodeGenCXX/lambda-instantiation-mangling-conflicts.cpp similarity index 78% rename from clang/test/CodeGenCXX/aarch64-sme-lambda-attributes.cpp rename to clang/test/CodeGenCXX/lambda-instantiation-mangling-conflicts.cpp index 9e9a3d09de24fd..433f616c8bb637 100644 --- a/clang/test/CodeGenCXX/aarch64-sme-lambda-attributes.cpp +++ b/clang/test/CodeGenCXX/lambda-instantiation-mangling-conflicts.cpp @@ -1,14 +1,16 @@ // REQUIRES: aarch64-registered-target -// 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 +// RUN: rm -rf %t +// RUN: split-file %s %t + +// RUN: %clang_cc1 -triple aarch64 -target-feature +sme -target-feature +sme2 -emit-llvm -o - %t/a.cpp -verify +// RUN: %clang_cc1 -triple aarch64 -target-feature +sme -target-feature +sme2 -emit-llvm -o - %t/b.cpp -verify + +;--- a.cpp int normal_fn(int); -int streaming_fn(int) __arm_streaming; int vector_pcs_fn(int) __attribute__((aarch64_vector_pcs)); -#ifdef TEST1 - // expected-error@+4 {{definition with same mangled name '_ZZ23function_pcs_attributesvENK3$_0clIFiiEEEDaRT_' as another definition}} // expected-note@+3 {{previous definition is here}} __attribute__((aarch64_vector_pcs)) @@ -17,9 +19,10 @@ int function_pcs_attributes() { return a(normal_fn) + a(vector_pcs_fn); } -#endif +;--- b.cpp -#ifdef TEST2 +int normal_fn(int); +int streaming_fn(int) __arm_streaming; // expected-error@+3 {{definition with same mangled name '_ZZ32function_params_normal_streamingvENK3$_0clIFiiEEEDaRT_' as another definition}} // expected-note@+2 {{previous definition is here}} @@ -27,5 +30,3 @@ int function_params_normal_streaming() { auto a = [](auto &fn) { return fn(42); }; return a(normal_fn) + a(streaming_fn); } - -#endif >From 00e7b0494c831b3f7405d34fa9819a271b77eb50 Mon Sep 17 00:00:00 2001 From: Kerry McLaughlin <kerry.mclaugh...@arm.com> Date: Tue, 15 Oct 2024 15:37:56 +0000 Subject: [PATCH 5/5] - Moved error to EmitGlobal and removed specific check for a lambda --- clang/lib/CodeGen/CodeGenModule.cpp | 21 +++++++++++++------ ...ambda-instantiation-mangling-conflicts.cpp | 6 ++---- .../CodeGenCXX/mangle-lambdas-gh88906.cpp | 9 ++------ 3 files changed, 19 insertions(+), 17 deletions(-) diff --git a/clang/lib/CodeGen/CodeGenModule.cpp b/clang/lib/CodeGen/CodeGenModule.cpp index e0cfbdf57534c7..550454abdd1ef2 100644 --- a/clang/lib/CodeGen/CodeGenModule.cpp +++ b/clang/lib/CodeGen/CodeGenModule.cpp @@ -3911,9 +3911,20 @@ void CodeGenModule::EmitGlobal(GlobalDecl GD) { assert(!MayBeEmittedEagerly(Global)); addDeferredDeclToEmit(GD); } else { - // Otherwise, remember that we saw a deferred decl with this name. The - // first use of the mangled name will cause it to move into - // DeferredDeclsToEmit. + // Otherwise, remember that we saw a deferred decl with this name. + auto DDI = DeferredDecls.find(MangledName); + const auto *FD = dyn_cast<FunctionDecl>(Global); + if (FD && DDI != DeferredDecls.end()) { + bool IsDefinitionAvailableExternally = + getContext().GetGVALinkageForFunction(FD) == GVA_AvailableExternally; + if (!IsDefinitionAvailableExternally && !FD->isMultiVersion() && + !FD->hasAttr<OMPDeclareTargetDeclAttr>()) + getDiags().Report(Global->getLocation(), + diag::err_duplicate_mangled_name) + << MangledName; + } + // The first use of the mangled name will cause it to move + // into DeferredDeclsToEmit. DeferredDecls[MangledName] = GD; } } @@ -4652,9 +4663,7 @@ llvm::Constant *CodeGenModule::GetOrCreateLLVMFunction( // If there are two attempts to define the same mangled name, issue an // error. - auto *MD = dyn_cast_or_null<CXXMethodDecl>(D); - bool IsLambda = MD && MD->getParent()->isLambda(); - if (IsForDefinition && (!Entry->isDeclaration() || IsLambda)) { + 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. diff --git a/clang/test/CodeGenCXX/lambda-instantiation-mangling-conflicts.cpp b/clang/test/CodeGenCXX/lambda-instantiation-mangling-conflicts.cpp index 433f616c8bb637..3a5adc5cb7c7d0 100644 --- a/clang/test/CodeGenCXX/lambda-instantiation-mangling-conflicts.cpp +++ b/clang/test/CodeGenCXX/lambda-instantiation-mangling-conflicts.cpp @@ -11,8 +11,7 @@ int normal_fn(int); int vector_pcs_fn(int) __attribute__((aarch64_vector_pcs)); -// expected-error@+4 {{definition with same mangled name '_ZZ23function_pcs_attributesvENK3$_0clIFiiEEEDaRT_' as another definition}} -// expected-note@+3 {{previous definition is here}} +// expected-error@+3 {{definition with same mangled name '_ZZ23function_pcs_attributesvENK3$_0clIFiiEEEDaRT_' as another definition}} __attribute__((aarch64_vector_pcs)) int function_pcs_attributes() { auto a = [](auto &fn) { return fn(42); }; @@ -24,8 +23,7 @@ int function_pcs_attributes() { int normal_fn(int); int streaming_fn(int) __arm_streaming; -// expected-error@+3 {{definition with same mangled name '_ZZ32function_params_normal_streamingvENK3$_0clIFiiEEEDaRT_' as another definition}} -// expected-note@+2 {{previous definition is here}} +// 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); diff --git a/clang/test/CodeGenCXX/mangle-lambdas-gh88906.cpp b/clang/test/CodeGenCXX/mangle-lambdas-gh88906.cpp index e7592cec5da776..6130756af0e618 100644 --- a/clang/test/CodeGenCXX/mangle-lambdas-gh88906.cpp +++ b/clang/test/CodeGenCXX/mangle-lambdas-gh88906.cpp @@ -1,5 +1,5 @@ // RUN: %clang_cc1 -triple x86_64-linux-gnu %s -emit-llvm -mconstructor-aliases -o - | FileCheck %s -// RUN: %clang_cc1 -triple x86_64-linux-gnu -fclang-abi-compat=18 %s -emit-llvm -mconstructor-aliases -o - | FileCheck --check-prefix=CLANG18 %s +// RUN: not %clang_cc1 -triple x86_64-linux-gnu -fclang-abi-compat=18 %s -emit-llvm -mconstructor-aliases 2>&1 | FileCheck --check-prefix=CLANG18 %s // RUN: %clang_cc1 -triple i386-pc-win32 %s -emit-llvm -mconstructor-aliases -o - | FileCheck --check-prefix=MSABI %s @@ -29,12 +29,7 @@ void GH88906(){ // CHECK-LABEL: define internal void @_ZN4funcC2IN7GH889064Test1bMUlvE_EEET_ // CHECK-LABEL: define internal void @_ZN4funcC2IN7GH889064Test1cMUlvE_EEET_ -// CLANG18-LABEL: define internal void @_ZZ7GH88906vEN4TestC2Ev -// CLANG18: call void @_ZN4funcC2IZ7GH88906vEN4TestUlvE_EZ7GH88906vENS1_UlvE0_EEET_T0_ -// CLANG18: call void @_ZN4funcC2IZ7GH88906vEN4TestUlvE_EEET_ -// CLANG18: call void @_ZN4funcC2IZ7GH88906vEN4TestUlvE_EEET_ - - +// CLANG18: error: definition with same mangled name '_ZN4funcC1IZ7GH88906vEN4TestUlvE_EEET_' as another definition // MSABI-LABEL: define internal x86_thiscallcc noundef ptr @"??0Test@?1??GH88906@@YAXXZ@QAE@XZ" // MSABI: call x86_thiscallcc noundef ptr @"??$?0V<lambda_1>@a@Test@?1??GH88906@@YAXXZ@V<lambda_2>@12?1??3@YAXXZ@@func@@QAE@V<lambda_1>@a@Test@?1??GH88906@@YAXXZ@V<lambda_2>@23?1??4@YAXXZ@@Z" _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits