[PATCH] D138514: Sema: diagnose PMFs passed through registers to inline assembly

[Saleem Abdulrasool via Phabricator via cfe-commits]

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