[ItaniumMangle] Fix cp versus cl call expression mangling for block scope

[hubert-reinterpretcast]

This patch updates to use the cl mangling when a declaration at block
scope (suppressing ADL) is encountered. This improves compatibility with
GCC and is consistent with the wording proposed by
itanium-cxx-abi/cxx-abi#196.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Hubert Tong (hubert-reinterpretcast)

Changes

---

Full diff: https://github.com/llvm/llvm-project/pull/114884.diff

2 Files Affected:

• (modified) clang/lib/AST/ItaniumMangle.cpp (+12)
• (modified) clang/test/CodeGenCXX/mangle-exprs.cpp (+24-1)

diff --git a/clang/lib/AST/ItaniumMangle.cpp b/clang/lib/AST/ItaniumMangle.cpp
index 14bc260d0245fb..321b0cb6830ea0 100644
--- a/clang/lib/AST/ItaniumMangle.cpp
+++ b/clang/lib/AST/ItaniumMangle.cpp
@@ -4628,6 +4628,18 @@ static bool isParenthesizedADLCallee(const CallExpr *call) {
       (*lookup->decls_begin())->isCXXClassMember())
     return false;
 
+  // Must not have found a block scope declaration other than declarations
+  // resulting from using declarations.
+  for (const auto &decl : lookup->decls()) {
+    if (isa<UsingShadowDecl>(*decl))
+      continue;
+    if (decl->isLocalExternDecl())
+      return false;
+    // If, ignoring using declarations, a non-block scope declaration was
+    // found, then there are no block-scope declarations.
+    break;
+  }
+
   // Otherwise, ADL would have been triggered.
   return true;
 }
diff --git a/clang/test/CodeGenCXX/mangle-exprs.cpp b/clang/test/CodeGenCXX/mangle-exprs.cpp
index b666eaadf45768..ca30de6fa668d9 100644
--- a/clang/test/CodeGenCXX/mangle-exprs.cpp
+++ b/clang/test/CodeGenCXX/mangle-exprs.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -std=c++11 -fclang-abi-compat=latest -emit-llvm %s -o - -triple=x86_64-apple-darwin9 | FileCheck %s
+// RUN: %clang_cc1 -std=c++14 -fclang-abi-compat=latest -emit-llvm %s -o - -triple=x86_64-apple-darwin9 | FileCheck %s
 
 namespace std {
   typedef decltype(sizeof(int)) size_t;
@@ -118,9 +118,32 @@ namespace test1 {
   // CHECK-LABEL: define linkonce_odr noundef signext i16 @_ZN5test11bIsEEDTcp3foocvT__EEES1_(
   template <class T> auto b(T t) -> decltype((foo)(T())) { return (foo)(t); }
 
+  // CHECK-LABEL: define {{.*}} @_ZN5test11cEs(
+  inline short *c(short s) {
+    using test1::foo;
+    void *foo(void *);
+    // CHECK: = call {{.*}} @_ZZN5test11cEsENKUlT_E_clIsEEPDTcl3foofp_EES0_(
+    return [](auto t) -> decltype((foo)(t)) * {
+      static auto x = (foo)(t);
+      return &x;
+    }(s);
+  }
+
+  // CHECK-LABEL: define {{.*}} @_ZN5test11dEs(
+  inline short *d(short s) {
+    using test1::foo;
+    // CHECK: = call {{.*}} @_ZZN5test11dEsENKUlT_E_clIsEEPDTcp3foodeadfp_EES0_(
+    return [](auto t) -> decltype((foo)(*&t)) * {
+      static auto x = (foo)(t);
+      return &x;
+    }(s);
+  }
+
   void test(short s) {
     a(s);
     b(s);
+    c(s);
+    d(s);
   }
 }
 

[hubert-reinterpretcast]

The test uses different expressions for the two added cases because of an existing issue with Clang where cases with different manglings have the same type because they are considered equivalent, see CWG 2946.

[hubert-reinterpretcast]

Updates to the release notes will be added once the patch is otherwise ready to avoid git conflict churn.

[rjmccall]

Can we find a way to re-use the code between this and the actual lookup code? Feels like we could have some sort of predicate like doesLookupResultSuppressADL(NamedDecl*). Or are we forced to use slightly different predicates for some compatibility reason?

[zygoloid]

Is it worth adding ClangABICompat support for this? (Do we think this will affect any real code, or is this more just formal correctness?)

[hubert-reinterpretcast]

Is it worth adding ClangABICompat support for this? (Do we think this will affect any real code, or is this more just formal correctness?)

The change was motivated by formal correctness (and the work led to the discovery of CWG 2946). I have some doubts that this change affects real code as the combination of (func)(args ...) in the signature of a lambda in an ODR context seems rare to me.

That said, if we don't add a ClangABICompat option now, then changing the meaning of -fclang-abi-compat=19 in the future (if it is needed) could lead to a need for -fclang-abi-compat=19-compat-under-20 or other such "compat compat" options.

[zygoloid]

That said, if we don't add a ClangABICompat option now, then changing the meaning of -fclang-abi-compat=19 in the future (if it is needed) could lead to a need for -fclang-abi-compat=19-compat-under-20 or other such "compat compat" options.

That's a horrible enough thought that I think we should just add the compat support now. :)

[hubert-reinterpretcast]

Can we find a way to re-use the code between this and the actual lookup code? Feels like we could have some sort of predicate like doesLookupResultSuppressADL(NamedDecl*). Or are we forced to use slightly different predicates for some compatibility reason?

Sema::UseArgumentDependentLookup in clang/lib/Sema/SemaExpr.cpp seems to have similar logic; but, in the context of the name mangler, we take advantage of whether Sema created an UnresolvedLookupExpr (including whether Sema applied overload resolution to a case where the call is not dependent).

Furthermore, considering the planned ClangABICompat change and the possible impact of CWG 2946 (e.g., mangling class-scope lookup results for friend declarations), I think the flow of the logic will diverge such that attempting to share a predicate now would not help.

[rjmccall]

Can we find a way to re-use the code between this and the actual lookup code? Feels like we could have some sort of predicate like doesLookupResultSuppressADL(NamedDecl*). Or are we forced to use slightly different predicates for some compatibility reason?

Sema::UseArgumentDependentLookup in clang/lib/Sema/SemaExpr.cpp seems to have similar logic; but, in the context of the name mangler, we take advantage of whether Sema created an UnresolvedLookupExpr (including whether Sema applied overload resolution to a case where the call is not dependent).

My point is that it's supposed to be the exact same logic, at least at the level of whether a specific declaration found by lookup suppresses ADL. "Member and local declarations except for local using declarations of non-local declarations" isn't a completely trivial rule; the fact that we tried to implement it independently in two places but forgot the local declarations half in the mangler is exactly how we got this bug.

To spell it out, if P(D) is the "D is a member or local..." predicate:

• Sema wants to suppress ADL if the syntax prohibits it or unqualified lookup found a declaration D such that P(D).
• The name mangler wants to use cp if the syntax prohibits ADL, except that for stability it wants to keep using cl in any situation where it doesn't matter, such as if the name is resolved or if unqualified lookup suppressed ADL anyway by finding a declaration D such that P(D). This ends up being equivalent to the callee operand being a parenthesized unqualified ULE with ADL suppressed that doesn't contain a D such that P(D).

Furthermore, considering the planned ClangABICompat change and the possible impact of CWG 2946 (e.g., mangling class-scope lookup results for friend declarations), I think the flow of the logic will diverge such that attempting to share a predicate now would not help.

I can certainly see how ABI compat might complicate re-use, but you could just pass a flag in saying whether this is for name mangling. I can't speak to the potential impact of CWG 2946, though.