(Re)Enable NativeVarargs for CoreCLR#18373

jashook · 2018-06-08T17:58:14Z

This will allow coreclr run programs which have native varargs.
It also re-enables the ArgIterator type for managed to managed native
vararg calls which will allow the use of the __arglist keyword.

This change also deletes a test which expects an invalid program exception
for native varargs usage.

Limitations:

NYI statements have been added for all Unix Targets.

With this change __arglist (native varargs) will be supported, but not
yet tested on:

Amd64 Windows
x86 Windows
Arm32 Windows
Arm64 Windows

This change does not address re-enablabling native vararg tests. This will be done
in a seperate change.

See #18377 for on initial thoughts on varargs bringup on
unix platforms.

CarolEidt

LGTM

janvorli · 2018-06-08T21:01:22Z

src/jit/lclvars.cpp

+#ifdef _TARGET_UNIX_
+            // Currently native varargs is not implemented on non windows targets.
+            //
+            // Note that some targets like Arm64 Unix should not need much work as


The ABI is not the same for ARM64 Unix - we've discussed it together some time ago. And with Amd64, things are even more complex on unix - the variable args can be passed in registers, the va_list is a special structure containing the indices of the gen purpose / fp registers, etc.
Am I missing something?

The comment is addressing code generation. For arm64 unix the ABI for varargs is not different from the regular calling convention.

Ah, makes sense, I was somehow mislead into thinking you were comparing the actual windows and Unix abis.

janvorli · 2018-06-08T21:02:22Z

src/vm/callingconvention.h

-        PORTABILITY_ASSERT("ArgIteratorTemplate::IsVarArgPassedByRef");                
        return FALSE;
-#else // UNIX_AMD64_ABI
+#else // !UNIX_AMD64_ABI


The comment was correct - the convention in the VM is to always use the condition from the #ifdef, not the negation.

Ok will address.

jkotas · 2018-06-08T21:39:25Z

src/System.Private.CoreLib/src/System/ArgIterator.cs

        }
-#else
-        public ArgIterator(RuntimeArgumentHandle arglist)
-        {


You should keep this throwing implementation for Unix until/if the varargs are actually implemented for Unix

Is there a platform specific define that can be used here?

PLATFORM_UNIX or PLATFORM_WINDOWS

jkotas · 2018-06-08T21:41:58Z

src/vm/jitinterface.cpp

        IfFailThrow(sig.GetCallingConvInfo(&data));
        sigRet->callConv = (CorInfoCallConv) data;

-        if ((isCallConv(sigRet->callConv, IMAGE_CEE_CS_CALLCONV_VARARG)) ||


We should continue throwing here for unix

jkotas · 2018-06-09T00:19:41Z

tests/src/baseservices/varargs/varargsupport.il

@@ -1,114 +0,0 @@
-// Licensed to the .NET Foundation under one or more agreements.


Should we keep these tests for Unix?

Personally I do not see the value of the test and conditionally excluding it by platform will be more work to undo later. If there are strong opinions to keep them then I can add them back.

This test would have caught the wrong ifdef that I have commented on...

Are we going to implement varargs support on Unix for next release? If yes, I do not have a problem with deleting these tests now. If not, we should keep them.

Fair point will add them back thank you.

jkotas · 2018-06-10T10:55:27Z

src/vm/jitinterface.cpp

        IfFailThrow(sig.GetCallingConvInfo(&data));
        sigRet->callConv = (CorInfoCallConv) data;

+#ifdef _TARGET_UNIX_


_TARGET_UNIX_ is JIT-specific define. Outside of JIT, we have PLATFORM_UNIX.

Thank you for catching this.

This will allow coreclr run programs which have native varargs. It also re-enables the ArgIterator type for managed to managed native vararg calls. This will allow the use of the __arglist keyword. Limitations: NYI statements have been added for all Unix Targets. With this change __arglist (native varargs) will be supported, but not yet tested on: Amd64 Windows x86 Windows Arm32 Windows Arm64 Windows This change does not re-enable native vararg tests. This will be done in a seperate change.

briansull

Looks Good
( with minor comments )

briansull · 2018-06-11T18:21:23Z

src/jit/morph.cpp

+        // Note that some targets like Arm64 Unix should not need much work as
+        // the ABI is the same. While other targets may only need small changes
+        // such as amd64 Unix, which just expects RAX to pass numFPArguments.
+        NYI("Morhing Vararg call not yet implemented on non Windows targets.");


Spelling: Morhing => Morphing

briansull · 2018-06-11T18:22:19Z

src/jit/morph.cpp

+        // such as amd64 Unix, which just expects RAX to pass numFPArguments.
+        NYI("Morhing Vararg call not yet implemented on non Windows targets.");
+    }
 #endif


// TARGET_UNIX

* Enable NativeVarargs for CoreCLR This will allow coreclr run programs which have native varargs. It also re-enables the ArgIterator type for managed to managed native vararg calls. This will allow the use of the __arglist keyword. Limitations: NYI statements have been added for all Unix Targets. With this change __arglist (native varargs) will be supported, but not yet tested on: Amd64 Windows x86 Windows Arm32 Windows Arm64 Windows This change does not re-enable native vararg tests. This will be done in a separate change. Commit migrated from dotnet/coreclr@e600470

jashook force-pushed the enable_native_varargs branch 2 times, most recently from a1dcf8c to ce2fe51 Compare June 8, 2018 18:35

jashook requested review from CarolEidt, adityamandaleeka, briansull and janvorli June 8, 2018 18:36

CarolEidt approved these changes Jun 8, 2018

View reviewed changes

jashook requested a review from davidwrighton June 8, 2018 20:46

jashook force-pushed the enable_native_varargs branch from ce2fe51 to eaa9b20 Compare June 8, 2018 20:51

janvorli reviewed Jun 8, 2018

View reviewed changes

jkotas suggested changes Jun 8, 2018

View reviewed changes

jkotas reviewed Jun 8, 2018

View reviewed changes

jashook force-pushed the enable_native_varargs branch 2 times, most recently from d1974c4 to 17ec636 Compare June 8, 2018 22:51

jkotas reviewed Jun 9, 2018

View reviewed changes

jkotas approved these changes Jun 9, 2018

View reviewed changes

jashook force-pushed the enable_native_varargs branch from 17ec636 to 0dff2d0 Compare June 10, 2018 05:30

jkotas reviewed Jun 10, 2018

View reviewed changes

jashook force-pushed the enable_native_varargs branch from 0dff2d0 to 890cd77 Compare June 10, 2018 17:19

jashook closed this Jun 11, 2018

jashook reopened this Jun 11, 2018

briansull approved these changes Jun 11, 2018

View reviewed changes

Add comments

b7d394c

jashook merged commit e600470 into dotnet:master Jun 11, 2018

jashook deleted the enable_native_varargs branch June 11, 2018 18:28

jashook mentioned this pull request Jan 31, 2020

Initial thoughts on Native Vararg support on Unix platforms for RyuJit dotnet/runtime#10478

Open

jkotas mentioned this pull request May 1, 2020

Reduce usage of Win7 and Win8 queues, move to CI and add CI runs in Win arm/arm64 dotnet/runtime#35690

Merged

This was referenced Feb 25, 2021

Method to forward variadic arguments to varargs methods dotnet/runtime#9702

Closed

Introduce mechanism to indicate arguments are to be marshalled as native varargs dotnet/runtime#48796

Open

		@@ -1,114 +0,0 @@
		// Licensed to the .NET Foundation under one or more agreements.

Comments

Conversation

jashook commented Jun 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CarolEidt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

briansull left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jashook commented Jun 8, 2018 •

edited

Loading