Page MenuHomePhabricator

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

Authored by mstorsjo on Tue, Jun 1, 5:00 AM.



On x86_64 mingw, long doubles are always passed indirectly as
arguments (see an existing case in WinX86_64ABIInfo::classify); we
need to take this into account when manually reading back arguments
with va_arg too.

Diff Detail

Event Timeline

mstorsjo created this revision.Tue, Jun 1, 5:00 AM
mstorsjo requested review of this revision.Tue, Jun 1, 5:00 AM
Herald added a project: Restricted Project. · View Herald TranscriptTue, Jun 1, 5:00 AM
rnk added inline comments.Wed, Jun 2, 1:57 PM

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.

mstorsjo added inline comments.Wed, Jun 2, 3:10 PM

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 doubles.

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?

mstorsjo added inline comments.Thu, Jun 3, 12:54 AM

GCC does the same for __int128 in varargs as it does for long double, so that does indeed simplify the code a fair bit.

And I also realized it's not a problem wrt long double and the __builtin_ms_va_list; if the caller does va_arg(ap, long double), that has to be handled according to whatever the size of long double is on platform, which hopefully would match what GCC uses on windows.

mstorsjo updated this revision to Diff 349491.Thu, Jun 3, 2:09 AM

Updated to not require the types to be either isAggregateTypeForABI(Ty) or Ty->isMemberPointerType(), just check the size of the type, added a testcase for __int128 (which I tested against GCC).

mstorsjo updated this revision to Diff 349517.Thu, Jun 3, 4:26 AM

Updated the CodeGenCXX/ext-int.cpp testcase.

rnk accepted this revision.Thu, Jun 3, 9:24 AM



These tests will stop working after opaque pointers happens, which I hope comes in the next year. If you look for a load of the pointer type, that should be resilient to opaque pointers.



This revision is now accepted and ready to land.Thu, Jun 3, 9:24 AM
mstorsjo added inline comments.Fri, Jun 4, 2:21 AM

Hmm, good point, but the test feels more brittle to me in that form:

// GNU32: load x86_fp80, x86_fp80*
// GNU64: load x86_fp80*, x86_fp80**
// GNU64: load x86_fp80, x86_fp80*
// MSC64: load double, double*

That wouldn't notice if GNU32 also used an indirect pointer for this case (as it would still match the second load - but it would notice if GNU64 stopped using an indirect pointer as the first load wouldn't be matched). I guess it'd be safer if I'd use more named regex patterns in the test to follow the chain from the argp, but then we end up with a pattern covering the bits that change due to opaque pointers too.

mstorsjo added inline comments.Mon, Jun 7, 10:26 AM

@rnk - WDYT about how to write the test patterns - see above?

rnk added inline comments.Mon, Jun 7, 12:24 PM

I guess the only two ideas I have are to try out or to CHECK-NOT for the indirect load.

mstorsjo added inline comments.Mon, Jun 7, 12:29 PM

Ok, CHECK-NOT sounds like a sensible compromise in making it more robust.