Add API that allows the emitter to indicate presence of localloc #27589

tmat · 2018-03-01T03:32:47Z

When encoding method body header the encoder needs to decide whether to emit tiny or fat header. A small method that has no locals but contains localloc instruction and has InitLocals set to true should not be encoded with tiny header since tiny header implies InitLocals is false. The presence of localloc instruction needs to be indicated by the caller of the method body encoder similarly to max stack and other info. Hence we need to add an overload that takes an extra bool parameter.

Fixes https://github.com/dotnet/corefx/issues/26910
Implements https://github.com/dotnet/corefx/issues/27611

tmat · 2018-03-01T03:33:30Z

@nguerrera @VSadov PTAL

nguerrera · 2018-03-01T04:07:08Z

LGTM. This is technically new API needing review. @terrajobst can we fast track it?

nguerrera · 2018-03-01T04:08:28Z

...flection.Metadata/src/System/Reflection/Metadata/Ecma335/Encoding/MethodBodyStreamEncoder.cs

+        /// <exception cref="ArgumentOutOfRangeException"><paramref name="maxStack"/> is out of range [0, <see cref="ushort.MaxValue"/>].</exception>
+        /// <exception cref="InvalidOperationException">
+        /// A label targeted by a branch in the instruction stream has not been marked,
+        /// or the distance between a branch instruction and the target label is doesn't fit the size of the instruction operand.


Typo: "is doesn't"

VSadov

.

VSadov

GTM
As discussed offline, the shape of API solves solves our issue.

ahsonkhan · 2018-03-01T06:06:23Z

API changes/additions here. What's the plan?
cc @terrajobst, @karelz, @danmosemsft

danmoseley · 2018-03-01T07:05:51Z

@ahsonkhan when API is approved send shiproom brief note with reasoning for the churn.

karelz · 2018-03-01T07:07:22Z

I don't see ref changes ... should they be there as well?

agocke

This means we'll have to track two bits instead of one in the compiler, but it also means this isn't a breaking change for S.R.M.

agocke · 2018-03-01T17:17:23Z

cc @t-camaia

tmat · 2018-03-01T17:44:18Z

@karelz Created API proposal issue: https://github.com/dotnet/corefx/issues/27611

tmat · 2018-03-01T17:44:27Z

test Tizen armel Debug Build please

VSadov · 2018-03-01T19:13:46Z

@agocke - we can also always set hasDynamicStackAllocation and just twiddle the LocalsInit in the attributes. - the result is the same.
Or track the presence of stackalloc. We would have that information anyways, just would need to plumb it through.

Yes the API change is additive to not be breaking.

tmat · 2018-03-02T00:49:55Z

I don't see ref changes ... should they be there as well?

Are they not auto-generated? I run build-managed.cmd, it succeeded and don't see any other files changed in the repo.

ahsonkhan · 2018-03-02T00:53:18Z

Are they not auto-generated? I run build-managed.cmd, it succeeded and don't see any other files changed in the repo.

Try running msbuild /t:GenerateReferenceSource /p:TargetGroup=netstandard /p:ConfigurationGroup=Release (from src directory of your project - in this case https://github.com/tmat/corefx/tree/27c4627148084a29bb6839e3a39a7d9d05767de6/src/System.Reflection.Metadata/src)

This is after building the repo at the root.

tmat · 2018-03-02T01:01:43Z

@ahsonkhan Thanks. Added another commit with regenerated ref file.

This process is very much broken. My previous PR also added new APIs but I didn't know the ref files need to be manually regenerated. Yet everything passed and it got merged!
The regenerated ref file also contains some other changes that have likely been made previously without regenerating the ref file: df10420

I wonder how many bugs are there currently in CoreFX due to this ref file generation.

ahsonkhan · 2018-03-02T01:02:53Z

src/System.Reflection.Metadata/ref/System.Reflection.Metadata.cs

@@ -99,9 +99,7 @@ namespace System.Reflection.Metadata
        public System.Reflection.Metadata.StringHandle Name { get { throw null; } }
        public System.Reflection.Metadata.BlobHandle PublicKey { get { throw null; } }
        public System.Version Version { get { throw null; } }
-#if !NETSTANDARD11


You need to sanitize the changes to only include the necessary changes you want (+ formatting cleanup/spacing/etc).

Keep the ifdefs.

ahsonkhan · 2018-03-02T01:03:51Z

src/System.Reflection.Metadata/ref/System.Reflection.Metadata.cs

@@ -2039,7 +2036,7 @@ public enum PrimitiveTypeCode : byte
    }
    public readonly partial struct PropertyAccessors
    {
-        private readonly int _dummy;
+        private readonly object _dummy;


@weshaggard, is this change OK and expected (the _dummy fields going from int to object)?

Some of those definitely look suspicious. object is the fallback if we cannot determine all the fields in the struct.

Can you provide some rudimentary guidelines that we can follow to vet these changes ourselves?

ahsonkhan · 2018-03-02T01:08:19Z

src/System.Reflection.Metadata/ref/System.Reflection.Metadata.cs

@@ -3135,6 +3138,7 @@ public enum Machine : ushort
        AM33 = (ushort)467,
        Amd64 = (ushort)34404,
        Arm = (ushort)448,
+        Arm64 = (ushort)43620,


@davidwrighton, this was added here - #26243

This is expected, correct?

weshaggard · 2018-03-02T01:11:04Z

This process is very much broken. My previous PR also added new APIs but I didn't know the ref files need to be manually regenerated

The process isn't the greatest but it is the best we have. We cannot automatically generate these easily because we have a different surface area based on configuration in a lot of libraries there isn't one definitively implementation we can generate them from.

I wonder how many bugs are there currently in CoreFX due to this ref file generation.

There may be a few but I don't expect there is a lot because most folks try to test or use their new APIs and they do it via the ref assembly and if the APIs aren't present then they cannot use them.

ahsonkhan · 2018-03-02T01:11:22Z

This process is very much broken. My previous PR also added new APIs but I didn't know the ref files need to be manually regenerated. Yet everything passed and it got merged!

If you add tests that exercise the newly added APIs, the build won't pass without the APIs being added to the ref. Only if you have test gaps would this be an issue.

What does the code coverage look like for this assembly?

ahsonkhan · 2018-03-02T01:17:32Z

There may be a few but I don't expect there is a lot because most folks try to test or use their new APIs and they do it via the ref assembly and if the APIs aren't present then they cannot use them.

Precisely.

https://github.com/dotnet/corefx/blob/master/src/System.Reflection.Metadata/tests/System.Reflection.Metadata.Tests.csproj#L144

<!-- Some internal types are needed, so we reference the implementation assembly, rather than the reference assembly. -->

tmat · 2018-03-02T01:18:30Z

There may be a few but I don't expect there is a lot because most folks try to test or use their new APIs and they do it via the ref assembly and if the APIs aren't present then they cannot use them.

The unit tests pass :(

If you add tests that exercise the newly added APIs, the build won't pass without the APIs being added to the ref.

Well, it passed.

ahsonkhan · 2018-03-02T01:23:33Z

The unit tests pass :(

I wonder if we can use InternalsVisibleTo instead of referencing the implementation assembly. Would we then be able to reference the ref and circumvent this issue?

tmat · 2018-03-02T01:27:36Z

The process isn't the greatest but it is the best we have.

This is not an acceptable answer.
@Petermarcu

danmoseley · 2018-03-02T01:30:04Z

Seems the problem is not that one must manually update the ref, but that by some unknown means the unit tests passed without the ref update. @weshaggard ?

nguerrera · 2018-03-02T01:31:06Z

@danmosemsft The means has been identified: the tests reference the implementation assembly as they require access to internals.

nguerrera · 2018-03-02T01:32:19Z

I wonder if we can use InternalsVisibleTo instead of referencing the implementation assembly. Would we then be able to reference the ref and circumvent this issue?

@ahsonkhan We are using InternalsVisibleTo. Do you mean put the internals in the reference assembly? Do other assemblies do that? If this closes the test gap, then yes, we should at least do that.

tmat · 2018-03-02T01:34:15Z

Filed an issue to fix the process: https://github.com/dotnet/corefx/issues/27639

tmat · 2018-03-02T01:36:28Z

Unit tests won't validate that the ref assembly is 100% correct. E.g. ref assembly contains private dummy fields. No unit test will validate those.

tmat · 2018-03-02T02:00:36Z

I have somewhat fixed up the ref assembly. Please validate it's correct.

ahsonkhan · 2018-03-02T02:01:55Z

I have somewhat fixed up the ref assembly. Please validate it's correct.

The only thing would be to keep the #if !NETSTANDARD11 that were removed. See https://github.com/dotnet/corefx/pull/27589/files#r171741403

danmoseley · 2018-03-02T02:09:45Z

What about two test libraries for the same implementation library, one with IVT and the other going only through public API would that help?

tmat · 2018-03-02T02:14:45Z

@danmosemsft. That makes it more complicated, not less. I don't want any more complexity in this already over complicated system.

This would help:

Generate one ref file per configuration and target framework
Generate the file to intermediate directory and do not check it in
Use C# compiler ref assembly feature

Perhaps these are not applicable on all projects in CoreFX that need some magic like System.Runtime but it will work at least for SCI and SRM (and maybe for others).

tmat · 2018-03-02T23:40:43Z

Let's move the ref assembly discussion to https://github.com/dotnet/corefx/issues/27639

tmat · 2018-03-02T23:40:57Z

test OSX x64 Debug Build please

tmat · 2018-03-02T23:41:02Z

test UWP CoreCLR x64 Debug Build please

tmat · 2018-03-05T17:09:53Z

test OSX x64 Debug Build please

tmat · 2018-03-05T17:10:00Z

test UWP CoreCLR x64 Debug Build please

ahsonkhan · 2018-03-05T18:43:34Z

The OSX failure is a known issue - https://github.com/dotnet/core-eng/issues/2808

weshaggard · 2018-03-08T18:32:40Z

While working on https://github.com/dotnet/corefx/issues/27639 I regenerated the ref for System.Reflection.Metadata and also noticed the dummy fields being changed from int to object and I do think that is a correct update because those structs have ImmutableArray references which contain array references. As part of addressing https://github.com/dotnet/corefx/issues/27639 I will regenerate the ref to fix that.

…net/corefx#27589) * Add API that allows the emitter to indicate presence of localloc * Regenerate ref assembly source Commit migrated from dotnet/corefx@3fc3900

Add API that allows the emitter to indicate presence of localloc

27c4627

nguerrera approved these changes Mar 1, 2018

View reviewed changes

VSadov reviewed Mar 1, 2018

View reviewed changes

VSadov approved these changes Mar 1, 2018

View reviewed changes

ahsonkhan assigned tmat Mar 1, 2018

agocke approved these changes Mar 1, 2018

View reviewed changes

tmat mentioned this pull request Mar 1, 2018

Update System.Reflection.Metadata dependency dotnet/roslyn#24677

Closed

tmat added 2 commits March 1, 2018 16:56

Add a test

792cf4d

Regenerate ref assembly.

df10420

ahsonkhan reviewed Mar 2, 2018

View reviewed changes

Fix up ref assembly

99704d4

Add ifdefs back

be5d687

karelz added the area-System.Reflection.Metadata label Mar 8, 2018

tmat merged commit 3fc3900 into dotnet:master Mar 8, 2018

tmat deleted the LocAlloc branch March 8, 2018 18:22

karelz added this to the 2.1.0 milestone Mar 10, 2018

tmat assigned VSadov and tmat and unassigned tmat and VSadov Mar 23, 2018

Add API that allows the emitter to indicate presence of localloc #27589

Add API that allows the emitter to indicate presence of localloc #27589

Conversation

tmat commented Mar 1, 2018 • edited Loading

tmat commented Mar 1, 2018

nguerrera commented Mar 1, 2018

nguerrera Mar 1, 2018

Choose a reason for hiding this comment

tmat Mar 1, 2018

Choose a reason for hiding this comment

VSadov left a comment • edited Loading

Choose a reason for hiding this comment

VSadov left a comment

Choose a reason for hiding this comment

ahsonkhan commented Mar 1, 2018 • edited Loading

danmoseley commented Mar 1, 2018

karelz commented Mar 1, 2018

agocke left a comment

Choose a reason for hiding this comment

agocke commented Mar 1, 2018

tmat commented Mar 1, 2018

tmat commented Mar 1, 2018

VSadov commented Mar 1, 2018

tmat commented Mar 2, 2018

ahsonkhan commented Mar 2, 2018

tmat commented Mar 2, 2018

ahsonkhan Mar 2, 2018 • edited Loading

Choose a reason for hiding this comment

ahsonkhan Mar 2, 2018

Choose a reason for hiding this comment

weshaggard Mar 2, 2018

Choose a reason for hiding this comment

ahsonkhan Mar 2, 2018

Choose a reason for hiding this comment

ahsonkhan Mar 2, 2018

Choose a reason for hiding this comment

weshaggard commented Mar 2, 2018

ahsonkhan commented Mar 2, 2018 • edited Loading

ahsonkhan commented Mar 2, 2018

tmat commented Mar 2, 2018 • edited Loading

ahsonkhan commented Mar 2, 2018

tmat commented Mar 2, 2018

danmoseley commented Mar 2, 2018

nguerrera commented Mar 2, 2018

nguerrera commented Mar 2, 2018 • edited Loading

tmat commented Mar 2, 2018

tmat commented Mar 2, 2018

tmat commented Mar 2, 2018

ahsonkhan commented Mar 2, 2018

danmoseley commented Mar 2, 2018

tmat commented Mar 2, 2018 • edited Loading

tmat commented Mar 2, 2018

tmat commented Mar 2, 2018

tmat commented Mar 2, 2018

tmat commented Mar 5, 2018

tmat commented Mar 5, 2018

ahsonkhan commented Mar 5, 2018

weshaggard commented Mar 8, 2018

tmat commented Mar 1, 2018 •

edited

Loading

VSadov left a comment •

edited

Loading

ahsonkhan commented Mar 1, 2018 •

edited

Loading

ahsonkhan Mar 2, 2018 •

edited

Loading

ahsonkhan commented Mar 2, 2018 •

edited

Loading

tmat commented Mar 2, 2018 •

edited

Loading

nguerrera commented Mar 2, 2018 •

edited

Loading

tmat commented Mar 2, 2018 •

edited

Loading