Sema: diagnose PMFs passed through registers to inline assembly

[compnerd]

Summary

The itanium ABI represents the PMF as a pair of pointers. As such the
structure cannot be passed through a single register. Diagnose such
cases in the frontend rather than trying to generate IR to perform this
operation.

Fixes: 59033

[rnk]

clang/lib/Sema/SemaStmtAsm.cpp

381

This is too narrow, there are lots of other ways to do this:

struct Foo { void method(); };
void f() {
  auto pmf = &Foo::method;
  asm volatile ("" : : "r"(pmf));
}

I think it makes sense to check for:

• An expression with a member pointer type
• Where the size of the type is larger than the size of a pointer, or word, or whatever proxy we normally use for the size of a general purpose register

In the Microsoft ABI, member function pointers are only sometimes pointer-sized. If the class uses the multiple inheritance model, it will be bigger and include the this-adjuster field. See the inheritance keyword docs to learn more:
https://learn.microsoft.com/en-us/cpp/cpp/inheritance-keywords?view=msvc-170

This also handles large pointers to data members in the MS ABI, which also has a wacky aggregate representation.

[aaron.ballman]

clang/include/clang/Basic/DiagnosticSemaKinds.td

8845–8846

No idea why Phab won't let me suggest this as an edit, but:

def err_asm_pmf_in_input : Error<
  "cannot pass pointer-to-member through a register on this ABI">;

clang/lib/Sema/SemaStmtAsm.cpp

381

That sounds like a less fragile approach to me, thanks for that suggestion!

clang/test/Sema/gnu-asm-pmf.cpp

1–34

[compnerd]

clang/lib/Sema/SemaStmtAsm.cpp

381

Thanks @rnk for pointing out the oversight on handling a PMF through a VarDecl and the pointer to the MSDN docs on how to form the adjusted reference. This actually is more interesting. Consider now the following:

struct s {
    __UINTPTR_TYPE__ i, j;
};

void f() {
    __asm__ __volatile__("" : : "r"(s{0,0}));
}

struct u { virtual void f(); };
struct v { virtual void operator()(); };
struct w: u, v {
};

void g() {
    __asm__ __volatile__("" : : "r"(&w::operator()));
}

GCC does accept the input, but clang fails due to the backend failing to form the indirect passing for the aggregate (Don't know how to handle indirect register inputs yet for constraint 'r'). Interestingly enough, when targeting x86_64-unknown-windows-msvc instead of x86_64-unknown-linux-gnu, we represent the PMF as an aggregate and trigger the same type of aggregate passing failure. On Linux though we attempt to lower the aggregate passing and then fail to select the copy due to the aggregate being lowered into a single register rather than being passed as indirect. One of the issues with the indirect passing is that GCC will also splat the "indirect" value but not give you the register that the remaining structure is splatted into. So ultimately, I think that filtering out any attempt to pass the PMF here is something that we can/should diagnose. But should we do that for any aggregate type is questionable, and there is still the question of how you identify that the representation that the PMF will be lowered to is an aggregate.

[compnerd]

clang/test/Sema/gnu-asm-pmf.cpp

1–34

This is pretty amazing, I didn't know that -verify took an argument!

[rnk]

clang/lib/Sema/SemaStmtAsm.cpp

381

But should we do that for any aggregate type is questionable,

Yes, GCC seems to go out of its way to make a two-int aggregate work, but we can't do that yet, and so far as I can tell, nobody is asking for that ability, so I think it's OK to leave them alone for now.

there is still the question of how you identify that the representation that the PMF will be lowered to is an aggregate.

Right, I'm proposing we use the size of a pointer as a proxy for that. The Microsoft ABI logic is quite complicated, and we wouldn't want to reimplement it here. See the details:
https://github.com/llvm/llvm-project/blob/main/clang/lib/AST/MicrosoftCXXABI.cpp#L252

[compnerd]

Address feedback from review

[compnerd]

clang/lib/Sema/SemaStmtAsm.cpp

381

Right, I'm proposing we use the size of a pointer as a proxy for that. The Microsoft ABI logic is quite complicated, and we wouldn't want to reimplement it here. See the details:

Am I reading that correctly in that member fields and not only member pointers can require the adjustment? If not, we can tighten this up to isMemberFunctionPointerType(). For now, this will simply not permit any PMF. Happy to refine this further if you think that we should be a bit more generous here.

[compnerd]

Add a test case for member data.

[aaron.ballman]

Generally LGTM but I'd appreciate a second pass by @rnk in case I've missed something.

clang/include/clang/Basic/DiagnosticSemaKinds.td

8845–8846

Slight formatting and wording tweaks:

def err_asm_pmf_through_constraint_not_permitted : Error
  "cannot pass a pointer-to-member through a register-constrained inline "
  "assembly parameter">;

[compnerd]

(Accepting Revision for Closing)
I missed the trailing number of the differential revision, and it failed to tie it to this. The commit is at https://github.com/llvm/llvm-project/commit/707cc06e1570b5966efcd6a9124191c80fa7a754
@rnk is going to be out for a while, so any concerns can be addressed in a follow up.

clang/include/clang/Basic/DiagnosticSemaKinds.td

 def warn_redefine_extname_not_applied : Warning<
   "#pragma redefine_extname is applicable to external C declarations only; "
   "not applied to %select{function|variable}0 %1">,
   InGroup<Pragmas>;
 } // End of general sema category.
 
 // inline asm.
 let CategoryName = "Inline Assembly Issue" in {
+  def err_asm_pmf_in_input
+    : Error<"cannot pass pointer-to-member through a register on this ABI">;
   def err_asm_invalid_lvalue_in_output : Error<"invalid lvalue in asm output">;
   def err_asm_invalid_output_constraint : Error<
     "invalid output constraint '%0' in asm">;
   def err_asm_invalid_lvalue_in_input : Error<
     "invalid lvalue in asm input for constraint '%0'">;
   def err_asm_invalid_input_constraint : Error<
     "invalid input constraint '%0' in asm">;
   def err_asm_tying_incompatible_types : Error<

clang/lib/Sema/SemaStmtAsm.cpp

for (unsigned i = NumOutputs, e = NumOutputs + NumInputs; i != e; i++) {
 
     ExprResult ER = CheckPlaceholderExpr(Exprs[i]);
     if (ER.isInvalid())
       return StmtError();
     Exprs[i] = ER.get();
 
     Expr *InputExpr = Exprs[i];
 
+    if (!Context.getTargetInfo().getCXXABI().isMicrosoft())
+      if (const auto *UO = dyn_cast<UnaryOperator>(InputExpr))
+        if (UO->getOpcode() == UO_AddrOf)
+          if (const auto *DRE = dyn_cast<DeclRefExpr>(UO->getSubExpr()))
+            if (const auto *CMD = dyn_cast<CXXMethodDecl>(DRE->getDecl()))
+              if (CMD->isInstance())
+                return StmtError(Diag(InputExpr->getBeginLoc(),
+                                      diag::err_asm_pmf_in_input)
+                                 << InputExpr->getSourceRange());
+
     // Referring to parameters is not allowed in naked functions.
     if (CheckNakedParmReference(InputExpr, *this))
       return StmtError();
 
     // Check that the input expression is compatible with memory constraint.
     if (Info.allowsMemory() &&
         checkExprMemoryConstraintCompat(*this, InputExpr, Info, true))
       return StmtError();

clang/test/Sema/gnu-asm-pmf.cpp

+// RUN: %clang_cc1 -triple x86_64-unknown-windows-msvc -std=c++2b -fsyntax-only -verify %s -DMICROSOFT_ABI
+// RUN: %clang_cc1 -triple x86_64-unknown-windows-itanium -std=c++2b -fsyntax-only -verify %s -DITANIUM_ABI
+
+#if defined(MICROSOFT_ABI)
+// expected-no-diagnostics
+#endif
+
+struct S {
+  void operator()() {}
+};
+
+struct T {
+  virtual void operator()() {}
+};
+
+struct U {
+  static void operator()() {}
+};
+
+auto L = [](){};
+
+void f() {
+  __asm__ __volatile__ ("" : : "r"(&decltype(L)::operator()));
+#if defined(ITANIUM_ABI)
+  // expected-error@-2{{cannot pass pointer-to-member through a register on this ABI}}
+#endif
+  __asm__ __volatile__ ("" : : "r"(&S::operator()));
+#if defined(ITANIUM_ABI)
+  // expected-error@-2{{cannot pass pointer-to-member through a register on this ABI}}
+#endif
+  __asm__ __volatile__ ("" : : "r"(&T::operator()));
+#if defined(ITANIUM_ABI)
+  // expected-error@-2{{cannot pass pointer-to-member through a register on this ABI}}
+#endif
+  __asm__ __volatile__ ("" : : "r"(U::operator()));
+}