This is an archive of the discontinued LLVM Phabricator instance.

Sema: diagnose PMFs passed through registers to inline assembly
ClosedPublic

Authored by compnerd on Nov 22 2022, 10:55 AM.

Details

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

Diff Detail

Event Timeline

compnerd created this revision.Nov 22 2022, 10:55 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 22 2022, 10:55 AM
compnerd requested review of this revision.Nov 22 2022, 10:55 AM
rnk added a subscriber: rnk.Nov 22 2022, 4:06 PM
rnk added inline comments.
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 added inline comments.Nov 30 2022, 8:24 AM
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 added inline comments.Dec 2 2022, 10:14 AM
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 marked an inline comment as done.Dec 2 2022, 12:36 PM
compnerd added inline comments.
clang/test/Sema/gnu-asm-pmf.cpp
1–34

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

rnk added inline comments.Dec 2 2022, 12:52 PM
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 updated this revision to Diff 479752.Dec 2 2022, 2:58 PM
compnerd marked an inline comment as done.

Address feedback from review

compnerd added inline comments.Dec 2 2022, 3:03 PM
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 updated this revision to Diff 480523.Dec 6 2022, 9:25 AM

Add a test case for member data.

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 marked an inline comment as done.Dec 8 2022, 1:37 PM
compnerd accepted this revision.Dec 9 2022, 9:03 AM

(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.

This revision is now accepted and ready to land.Dec 9 2022, 9:03 AM
compnerd closed this revision.Dec 9 2022, 9:03 AM