[PATCH] D103452: [clang] Fix reading long doubles with va_arg on x86_64 mingw

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added inline comments.


================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:4364-4365
 
   // MS x64 ABI requirement: "Any argument that doesn't fit in 8 bytes, or is
   // not 1, 2, 4, or 8 bytes, must be passed by reference."
   if (isAggregateTypeForABI(Ty) || Ty->isMemberPointerType()) {
----------------
rnk wrote:
> I wonder if we should make the check more general. There are other cases to consider, like `__int128`. How does GCC pass other large types through varargs? It looks to me like clang passes those indirectly, so we could go ahead and check the type size directly without these aggregate checks.
> 
> Separately, I think `X86_64ABIInfo::EmitMSVAArg` has a bug, it always passes false for indirect, but it should match this code here exactly. Ideally, they would share an implementation.
I'll have a look at what GCC does for `__int128` yeah.

Yes, `X86_64ABIInfo::EmitMSVAArg` is lacking, I've got a separate patch fixing that one up to be posted after this one (but this patch fixes an actual issue I've run into, the other one is more of a correctness thing).

The open question about that one, is what to do about `long double`. If using `__attribute__((ms_abi))` and `__builtin_ms_va_list` on a non-windows platform, we don't know if they're meaning to interact with the MS or GNU ABI regarding `long double`s.

On one hand, one would mostly be using those constructs to reimplement Windows API (i.e. implementing wine or something similar), and in that case, only the MS behaviour is of interest. On the other hand, mapping `va_arg(ap, long double)` to the GNU case doesn't take anything away for the user either, because if you want a MSVC long double, you can just do `va_arg(ap, double)`. Then finally, I guess there's no guarantee that the platform where doing that even has an exactly matching long double?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D103452/new/

https://reviews.llvm.org/D103452