Refresh System.Runtime ref from System.Private.CoreLib in Coreclr by weshaggard · Pull Request #26264 · dotnet/corefx

weshaggard · 2018-01-11T00:21:37Z

This also includes an update to the genapi tool that includes the
necessary private fields in structs to enable the compiler to
do correctness checks.

See https://github.com/dotnet/corefx/issues/6185.

The ref was generated with the updated changes in dotnet/buildtools#1859.

PTAL @jkotas @VSadov @jaredpar @gafter

Note that some of the updates are just a refresh from what is in the System.Private.CoreLib and not about the fields in structs.

This also includes an update to the genapi tool that includes the necessary private fields in structs to enable the compiler to do correctness checks. See https://github.com/dotnet/corefx/issues/6185.

jkotas · 2018-01-11T00:47:01Z

    [System.Runtime.CompilerServices.AsyncMethodBuilderAttribute(typeof(System.Runtime.CompilerServices.AsyncValueTaskMethodBuilder<>))]
    public readonly partial struct ValueTask<TResult> : System.IEquatable<System.Threading.Tasks.ValueTask<TResult>>
    {
+        internal readonly TResult _result;


Should all the fields be private?

I noticed a couple of these as well, I don't know why they showed up as internal but I will investigate.

The reason this and a couple others are internal instead of private is because for any non-public fields that are generic I just blindly include those unchanged. For now I will keep them like this so they correctly show the usage of generic parameters. If you feel there is a reason to switch them I can.

Same in Nullable<T> - internal T value . I think that is ok.

In reply to: 161028771 [](ancestors = 161028771)

jkotas · 2018-01-11T00:47:43Z

        public virtual bool IsAsync { get { throw null; } }
        public override long Length { get { throw null; } }
-        public string Name { get { throw null; } }
+        public virtual string Name { get { throw null; } }


There is a bunch of unrelated changes like this one. Are they all intentional?

Yes these are intentional. They are essentially refreshing based on what is in System.Private.CoreLib implementation. A number of changes to the API never made it to the ref.

jkotas · 2018-01-11T00:48:49Z

        public static System.ReadOnlySpan<T> DangerousCreate(object obj, ref T objectData, int length) { throw null; }
        [System.ComponentModel.EditorBrowsableAttribute((System.ComponentModel.EditorBrowsableState)(1))]
-        [System.ObsoleteAttribute("Equals() on ReadOnlySpan will always throw an exception. Use == instead.")]
+        [System.ObsoleteAttribute("Equals() on Span will always throw an exception. Use == instead.")]


The original message looks right. This looks like a bug in the implementation.

This just updated to what is in the implementation. We could definitely update both to be consistent with ReadOnlySpan.

Fix: dotnet/coreclr#15820

jkotas · 2018-01-11T01:05:11Z

    }
    public partial struct GCHandle
    {
+        private object _dummy;


I think this should be int. GCHandle does not have instance object fields.

this might be a side-effect of me not filtering out statics while doing the contains reference check. I will regenerate once I update the tool.

@jkotas the GCHandle holds a IntPtr which in turn holds a void* so based on our logic I would expect this to have an object dummy field. Is there something I'm missing?

void* should be treated as int, not as object.

OK with that change we get a bunch of updates. Can you have a look over 9288c49 to ensure they look as expected?

VSadov · 2018-01-11T18:15:59Z

    }
    public partial struct Boolean : System.IComparable, System.IComparable<bool>, System.IConvertible, System.IEquatable<bool>
    {
+        private object _dummy;


Why Boolean contains an object field?
The real one only contains a bool. #Closed

Just pushed a change that fixes this. #Closed

VSadov · 2018-01-11T18:17:28Z

    }
    public partial struct Char : System.IComparable, System.IComparable<char>, System.IConvertible, System.IEquatable<char>
    {
+        private object _dummy;


Same as Boolean - why object? #Closed

Was an issue in detecting a reference type, just fixed. #Closed

VSadov · 2018-01-11T18:20:35Z

    }
    public partial struct IntPtr : System.IEquatable<System.IntPtr>, System.Runtime.Serialization.ISerializable
    {
+        private object _dummy;


Should be int or IntPtr. #Closed

VSadov · 2018-01-11T18:27:47Z

    [System.CLSCompliantAttribute(false)]
    public partial struct UIntPtr : System.IEquatable<System.UIntPtr>, System.Runtime.Serialization.ISerializable
    {
+        private object _dummy;


should be int or UIntPtr #Closed

jkotas · 2018-01-11T18:33:22Z

        public override bool Equals(object obj) { throw null; }
        public bool Equals(System.RuntimeTypeHandle handle) { throw null; }
        public override int GetHashCode() { throw null; }
+        [System.CLSCompliantAttribute(false)]


You can delete this unnecessary CLSCompliantAttribute attribute. I have deleted it in the implementation already.

VSadov · 2018-01-11T18:40:04Z

 {
    public readonly partial struct CancellationToken
    {
+        private readonly object _dummy;


👍 This one was causing particularly annoying bug reports.

VSadov · 2018-01-11T18:41:38Z

    public static System.Span<T> DangerousCreate(object obj, ref T objectData, int length) { throw null; }

Just wondering - what happened to the DangerousGetPinnableReference ? #Closed

Refers to: src/System.Runtime/ref/System.Runtime.cs:2170 in 925866d. [](commit_id = 925866d, deletion_comment = False)

weshaggard · 2018-01-11T18:44:00Z

Just wondering - what happened to the DangerousGetPinnableReference ?

dotnet/coreclr@4d0e363

…f Span Remove unnecessary CLSCompliant(false) on GetModuleHandle

VSadov · 2018-01-11T18:58:48Z

    }
    public readonly ref partial struct Span<T>
    {
+        private readonly int _dummy;


I think .NET implementation contains an object. And even the fast implementation technically may contain an object reference.

We do not consider any generic struct as unmanaged right now, regardless of fields, but might in the future.

I think Span/ReadOnlySpan should be considered object-containing

I think Span/ReadOnlySpan should be considered object-containing

I agree. It is not something that the tool can figure out from CoreLib metadata because of Span is treated as intrinsic by the runtime. Types like these will need exceptions.

BTW: It is a bit odd that the primitive types like Int64 has 32-bit fields now. I won't be surprised if we find that it broke something somewhere and we need similar exceptions for them.

On the other hand - even if we see all the fields we would consider fast span unmanaged. (since we do not see or understand the hidden byref)
And other ones - like TypedReference were always considered unmanaged.

I guess the object inside the slow spans would be the only reason to make the spans managed. - Just to conservatively match the implementations.

In reply to: 161045136 [](ancestors = 161045136)

Yeah - I had that concern about primitives as well. Maybe it would be safer if they are self-containing, like all their implementations?

In reply to: 161048209 [](ancestors = 161048209)

I.E. - intcontains an int dummy, bool contains a bool, and so on ...

In reply to: 161049185 [](ancestors = 161049185,161048209)

And other ones - like TypedReference were always considered unmanaged.

While it actually contains managed references because of it is treated as intrinsic by the runtime...

This highlights that this has number of false positives and false negatives.

Would it make sense to relax C# to let you take pointer of anything, and demote the current error for taking pointer of "managed type" to just a warning?

Possibly. I do not know the exact rationale for not allowing pointers to managed structs. It was the case since v1.0

Perhaps to reduce chances of heap corruption when one dereferences garbage...

In reply to: 161052501 [](ancestors = 161052501)

Latest update fixes primitive types to have matching field types and manually marks Span and ReadOnlySpan as having an object type.

Are there any other special types we need to manually fix?

I think this is all that is needed.
The old-style ref-likes like TypedReference should stay as they are. At least for compat reasons.

In reply to: 161073853 [](ancestors = 161073853)

jkotas · 2018-01-11T19:19:23Z

Just wondering - what happened to the DangerousGetPinnableReference ?

It was replaced by System.Runtime.InteropServices.MemoryMarshal.GetReference, and removed from the public surface. The DangerousCreate method will be moved to MemoryMarshal as well before we ship - there is an issue about that.

Once the pattern-based fixed statement in C# is figured out, we will add it back in the right form:
dotnet/csharplang#1100 (comment)

VSadov · 2018-01-11T20:06:56Z

Re: DangerousGetPinnableReference - makes sense. Thanks!

VSadov

LGTM

jkotas · 2018-01-12T00:03:30Z

        public bool TryCopyTo(System.Span<T> destination) { throw null; }
        public ref partial struct Enumerator
        {
+            private int _dummy;


The Enumerator struct contains Span field. It should be treated same way as Span and have object field.

This applies to all structs with Span fields.

I should probably update the tool to special case Span and ReadOnlySpan.

Perhaps it is easier to hack the tool so that it thinks Span/ReadonlySpan actually contain an extra private object _dummy ?

I'm going to teach the tool to recognize "ByReference" as hold a reference.

That could add an object into TypedReference.
It is technically breaking. TypedReference* It is a very corner case and maybe fragile/broken anyways, so I am not sure how much we care.

The rule dotnet/buildtools@4b80362 produced the expected output
6ef9545.

TypedReference didn't change because it only contains an IntPtr not a ByReference.

jkotas · 2018-01-12T00:03:51Z

        public bool TryCopyTo(System.Span<T> destination) { throw null; }
        public ref partial struct Enumerator
        {
+            private int _dummy;


weshaggard · 2018-01-12T00:10:08Z

test Linux x64 Release Build
test UWP CoreCLR x64 Debug Build

weshaggard · 2018-01-12T00:15:39Z

    }
    public readonly ref partial struct ReadOnlySpan<T>
    {
+        private readonly object _dummy; //Manually set to object because tools don't know it is special


@jkotas @VSadov should this (and Span) be of type object or type T?

object , there are no fields of type T in Span.

It is similar to the ImmutableArray<T> - T is the element type, but the struct is just an indirection to some other storage, it does not contain any instances directly.

I have the changed for ImmutableArray locally and my tool generates internal T[] array; for it as it is generic underneath.

Either T[] or object would work since T[] is not T and is not a struct that contains instance field of type T .

Basically arrays are just reference types for this purpose.

This make Span and ReadOnlySpan have a reference and thus generates a private object field for the compiler to do the correct checks.

…ntimeFromCoreCLR Refresh System.Runtime ref from System.Private.CoreLib in Coreclr Commit migrated from dotnet/corefx@c84ed43

Refresh System.Runtime ref from System.Private.CoreLib in Coreclr

a2d4a32

This also includes an update to the genapi tool that includes the necessary private fields in structs to enable the compiler to do correctness checks. See https://github.com/dotnet/corefx/issues/6185.

jkotas reviewed Jan 11, 2018

View reviewed changes

Update struct private field types to not count unsafe pointers as object

9288c49

VSadov reviewed Jan 11, 2018

View reviewed changes

jkotas reviewed Jan 11, 2018

View reviewed changes

VSadov reviewed Jan 11, 2018

View reviewed changes

Fix ReadOnlySpan obsolete message to reference ReadOnlySpan instead o…

c58e40f

…f Span Remove unnecessary CLSCompliant(false) on GetModuleHandle

weshaggard force-pushed the UpdateSystemRuntimeFromCoreCLR branch from 925866d to c58e40f Compare January 11, 2018 18:47

VSadov reviewed Jan 11, 2018

View reviewed changes

Update System.Runtime ref primitive types to contain field of same type

7e39129

VSadov approved these changes Jan 11, 2018

View reviewed changes

jkotas reviewed Jan 12, 2018

View reviewed changes

weshaggard commented Jan 12, 2018

View reviewed changes

Teach the tool to understand ByReference<T> contains a reference

6ef9545

This make Span and ReadOnlySpan have a reference and thus generates a private object field for the compiler to do the correct checks.

weshaggard force-pushed the UpdateSystemRuntimeFromCoreCLR branch from ece948d to 6ef9545 Compare January 12, 2018 00:31

weshaggard merged commit c84ed43 into dotnet:master Jan 12, 2018

karelz added this to the 2.1.0 milestone Jan 20, 2018

VSadov mentioned this pull request Jan 25, 2018

Blittable design update dotnet/csharplang#1276

Merged

weshaggard deleted the UpdateSystemRuntimeFromCoreCLR branch February 9, 2018 17:54

Conversation

weshaggard commented Jan 11, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weshaggard Jan 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

VSadov Jan 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weshaggard Jan 11, 2018 • edited by VSadov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

VSadov Jan 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weshaggard Jan 11, 2018 • edited by VSadov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

VSadov Jan 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

VSadov Jan 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

VSadov commented Jan 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

weshaggard commented Jan 11, 2018

Uh oh!

VSadov Jan 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

weshaggard Jan 11, 2018 •

edited

Loading

VSadov Jan 11, 2018 •

edited

Loading

weshaggard Jan 11, 2018 •

edited by VSadov

Loading

VSadov Jan 11, 2018 •

edited

Loading

weshaggard Jan 11, 2018 •

edited by VSadov

Loading

VSadov Jan 11, 2018 •

edited

Loading

VSadov Jan 11, 2018 •

edited

Loading

VSadov commented Jan 11, 2018 •

edited

Loading

VSadov Jan 11, 2018 •

edited

Loading

jkotas Jan 11, 2018 •

edited

Loading

VSadov Jan 11, 2018 •

edited

Loading