rnk 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)
----------------
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
Repository:
rG LLVM Github Monorepo
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
