compnerd added inline comments.
================ Comment at: clang/lib/Sema/SemaStmtAsm.cpp:381 + if (!Context.getTargetInfo().getCXXABI().isMicrosoft()) + if (const auto *UO = dyn_cast<UnaryOperator>(InputExpr)) + if (UO->getOpcode() == UO_AddrOf) ---------------- rnk wrote: > compnerd wrote: > > aaron.ballman wrote: > > > rnk wrote: > > > > 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. > > > That sounds like a less fragile approach to me, thanks for that > > > suggestion! > > 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. > > 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 > > > 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. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D138514/new/ https://reviews.llvm.org/D138514 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits