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

Reply via email to