dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.43k stars 4.76k forks source link

Expression Interpreter Writeback can be incorrect in the face of aliasing. #19286

Open JonHanna opened 8 years ago

JonHanna commented 8 years ago

Consider a byref method such as this constructor:

private class ByRefNewType
{
    public ByRefNewType(ref int a, ref int b)
    {
        ++a;
        b *= a;
    }
 }

And a variable int x = 3 Calling new ByRefNewType(ref x, ref x) is equivalent to running ++x; x*=x and hence x is 16. With an expression-created delegate:

var del = Expression.Lambda<ByRefNewFactory2>(
    Expression.New(
        typeof(ByRefNewType).GetConstructors()[0], pX, pY),
        pX, pY).Compile();
int x = 3;
del(byref x, byref x);

Then with the compiler x will be 16 as expected, by with the interpreter y will be 12 (it could conceivably end up being 4).

The cause is that the interpreter doesn't have true reference types and so the aliasing is broken.

I'm not sure this is feasibly fixable. It may have to be merely noted as a limitation.

JonHanna commented 8 years ago

There's also a failure to write-back if an exception happens after the assignment to the ref parameter but within the method. This might be another consequence of the same thing, or might be separate.

VSadov commented 7 years ago

I think this cannot be truly fixed and should be documented that aliasing nature of refs could be observably not preserved in interpretation mode.

This is not something new. VB resorts to copy-backs in scenarios like passing properties by references. Even CLI standard reserves the right to break aliasing in scenarios such as remoting and fall back on copy backs

ViktorHofer commented 7 years ago

@Jiayili1 which test is disabled here?

@VSadov @JonHanna If you say it cannot be truly fixed I assume the documentation part is the only missing piece here? If yes can you propose a documentation change/addition and where it should go? Thanks!

JonHanna commented 7 years ago

There's talk of some ref-returning/using additions to reflection, which could give us more flexibility in dealing with this. I think it may be worth waiting an seeing what happens there.

(On the note of documentation, is there any good documentation anywhere about the different types of compilation? It's not something I knew about until I saw the source).

ViktorHofer commented 7 years ago

@JonHanna is the ref-returning available till 2.0?

JonHanna commented 7 years ago

I'm not in the loop on that, but I would doubt it.

ViktorHofer commented 7 years ago

If you feel we can do something here for 2.0 then leave it as it is but if not then please set it to a future milestone.

JonHanna commented 7 years ago

I think future would be more appropriate, if you can change the milestone

KristinXie1 commented 7 years ago

@ViktorHofer https://github.com/dotnet/corefx/blob/master/src/System.Linq.Expressions/tests/New/NewWithByRefParameterTests.cs#L69