Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Method to forward variadic arguments to varargs methods #9702

Closed
IS4Code opened this issue Feb 10, 2018 · 6 comments
Closed

Method to forward variadic arguments to varargs methods #9702

IS4Code opened this issue Feb 10, 2018 · 6 comments
Labels
area-Meta needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration
Milestone

Comments

@IS4Code
Copy link

IS4Code commented Feb 10, 2018

I propose this method in the System.Runtime.CompilerServices.RuntimeHelpers class:

public static void ForwardArguments(RuntimeMethodHandle method, ArgIterator args, __arglist);

This would call the specified method (which must be variadic), passing along the remaining arguments from the ArgIterator to the variadic parameter, and the variadic arguments from the method as normal parameters. If the number of variadic arguments passed to ForwardArguments is larger than the number of arguments the method takes (throw an exception if smaller), they could be prepended before the variadic arguments passed from args.

Example usage:

public static void VarArgs(int arg, __arglist)
{
	Console.WriteLine(arg);
	
	var ai = new ArgIterator(__arglist);
	while(ai.GetRemainingCount() > 0)
	{
		Console.WriteLine(TypedReference.ToObject(ai.GetNextArg()));
	}
}

public static void Forward(__arglist)
{
	var ai = new ArgIterator(__arglist);
	int num = __refvalue(ai.GetNextArg(), int);
	
	ForwardArguments(ldtoken(VarArgs), ai, __arglist(num, "hello"));
}

Forward(__arglist(42, "world")); //outputs "42", "hello", "world"

This extracts the integer from the argument list, and passes it with the rest to VarArgs. ldtoken serves as a replacement of the ldtoken CIL instruction (for fast reflection).

I have observed the need of this method while reporting dotnet/roslyn#24348, when a bridge virtual method must be generated to implement an interface method with a non-virtual method from a base class in another assembly. In the case of a variadic method, there is no support in CIL to forward variadic arguments to another method without having to deal with complex reflection, and thus a code that would normally compile in C# for a single assembly fails if the base class is present in a different assembly.

The bridge method could be generated if the proposed method was available:

class MyVarArgs : VarArgs, IVarArgs
{
	void IVarArgs.Invoke(__arglist)
	{
		RuntimeHelpers.ForwardArguments(ldtoken(base.Invoke), new ArgIterator(__arglist), __arglist(this));
	}
}

The usefulness of this method is apparent in this case, but it can also be used for variadic reflection, which is not supported much at the moment, and other languages that use variadic methods can profit from this as well, using it either implicitly like C#, or explicitly.

There should also be variations of this method returning object and generic T and ref T, to accomodate for all return types. Another overload taking RuntimeArgumentHandle instead of ArgIterator could be also possible for additional performance (not having to construct the iterator for simple forwarding methods).

If the called method isn't variadic, the ArgIterator must be empty, but the other arguments could still be passed via normal parameters of the method, allowing invoking a method without having to construct an object array, or a delegate (and allowing passing references).

My reason for including the method in RuntimeHelpers and not MethodInfo is mainly for speed, since it would have to be instantiated every time, and it would be used mainly to implement language features.

@jkotas
Copy link
Member

jkotas commented Feb 10, 2018

ArgIterator and varargs are not part of .NET Core. This suggestion is not applicable for .NET Core unless these are added back.

Also, this mechanism technically exists today: The IL jmp instruction should be doing exactly this kind of forward. However, it is very rarely used instruction and the implementation may have bugs or not be supported on some runtimes.

@IS4Code
Copy link
Author

IS4Code commented Feb 10, 2018

I see, that's unfortunate; I hope it will be reintroduced at some point in the future. Is there maybe something like this repo for .NET Framework where I can redirect this suggestion?

I've suggested jmp in the original Roslyn issue, but there've been some issues about its verifiability and portability, considering jumping between virtual and non-virtual methods.

@jkotas
Copy link
Member

jkotas commented Feb 10, 2018

there've been some issues about its verifiability and portability, considering jumping between virtual and non-virtual methods.

Then the right way to deal with this would be to fix these issues. It does not make sense to introduce a new redundant API to achieve the same.

@jkotas
Copy link
Member

jkotas commented Feb 10, 2018

Is there maybe something like this repo for .NET Framework where I can redirect this suggestion?

The links for .NET Framework feedback are at https://github.com/dotnet/corefx/#how-to-engage-contribute-and-provide-feedback

@msftgits msftgits transferred this issue from dotnet/coreclr Jan 31, 2020
@msftgits msftgits added this to the Future milestone Jan 31, 2020
@maryamariyan maryamariyan added the untriaged New issue has not been triaged by the area owner label Feb 26, 2020
@ericstj ericstj removed the untriaged New issue has not been triaged by the area owner label Jun 25, 2020
@AaronRobinsonMSFT
Copy link
Member

@jkotas It seems that native varargs was added back in dotnet/coreclr#18373.

Does the calculus for this work change anything here? I personally don't think it does and we can likely close this issue, but looking for longer term thoughts on this proposal.

@ghost ghost added the needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration label Feb 25, 2021
@AaronRobinsonMSFT
Copy link
Member

Closing this as it doesn't seem like an area we want to invest in. See #10478 and #48752 that are more targeted.

@ghost ghost locked as resolved and limited conversation to collaborators Mar 27, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Meta needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration
Projects
No open projects
Development

No branches or pull requests

6 participants