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

Add SkipLocalsInitAttribute #454

Open
wants to merge 5 commits into
base: master
from
Open

Add SkipLocalsInitAttribute #454

wants to merge 5 commits into from

Conversation

@agocke
Copy link
Contributor

agocke commented Dec 2, 2019

This attribute supports a new compiler feature that allows a user to
specify that they don't want to emit the CLR .locals init portion of
the header in methods. This can sometimes produce a performance
improvement but, in some cases, can reveal uninitialized memory to the
application.

Ported from dotnet/coreclr#20093

This attribute supports a new compiler feature that allows a user to
specify that they don't want to emit the CLR `.locals init` portion of
the header in methods. This can sometimes produce a performance
improvement but, in some cases, can reveal uninitialized memory to the
application.

Ported from dotnet/coreclr#20093
Copy link
Member

stephentoub left a comment

Thanks, @agocke.

@jkotas

This comment has been minimized.

Copy link
Member

jkotas commented Dec 2, 2019

@jkotas
jkotas approved these changes Dec 2, 2019
@agocke

This comment has been minimized.

Copy link
Contributor Author

agocke commented Dec 2, 2019

yeah, sorry, that was my mistake in cloning the repo, future PRs will be in my fork.

@tannergooding

This comment has been minimized.

Copy link
Member

tannergooding commented Dec 2, 2019

CC. @TIHan, @cartermp for F# (in case you want to start respecting the attribute as well 😄)

@@ -6914,6 +6914,13 @@ public sealed partial class RuntimeWrappedException : System.Exception
public object WrappedException { get { throw null; } }
public override void GetObjectData(System.Runtime.Serialization.SerializationInfo info, System.Runtime.Serialization.StreamingContext context) { }
}
[AttributeUsage(AttributeTargets.Module | AttributeTargets.Class | AttributeTargets.Struct | AttributeTargets.Constructor | AttributeTargets.Method | AttributeTargets.Property | AttributeTargets.Event, Inherited = false)]

This comment has been minimized.

Copy link
@tannergooding

tannergooding Dec 2, 2019

Member

These should be fully qualified, e.g. System.AttributeUsage and System.AttributeTargets.Module. The class should also be partial.

I believe that generating the System.Runtime reference assembly is still a manual process for the most part (CC. @safern), but its still good to avoid the conflicts for when someone does auto-generate it later.

This comment has been minimized.

Copy link
@safern

safern Dec 3, 2019

Member

For System.Runtime you can add --follow-typeforwards parameter to the GenAPI call in the target that we have in order to generate the reference assembly.

Basically just append it here:

<Exec Command="$(_GenAPICmd)" />

Then you can call dotnet msbuild /t:GenerateReferenceSource in the System.Runtime src project and it should update ref/System.Runtime.cs.

System.Runtime.cs has a lot of manual updates so there might be some other stuff changed that you'll need to ignore.

This comment has been minimized.

Copy link
@agocke

agocke Dec 5, 2019

Author Contributor

I tried this, and the build failed with

C:\Users\angocke\code\runtime\src\libraries\Directory.Build.targets(151,5): error MSB3073: The command " "C:\Users\angocke\code\runtime\artifacts\obj\System.Reflection.TestModule\netstandard2.0-Debug\System.Reflection.TestModule.dll" --lib-path "C:\Users\angocke\code\runtime\artifacts\bin\ref\netstandard2.0" --out "C:\Users\angocke\code\runtime\src\libraries\System.Runtime\tests\ref\System.Reflection.TestModule.cs" --exclude-attributes-list "C:\Users\angocke\code\runtime\eng\DefaultGenApiDocIds.txt" --header-file "C:\Users\angocke\code\runtime\eng\LicenseHeader.txt" --lang-version "latest" --follow-type-forwards" exited with code 3. [C:\Users\angocke\code\runtime\src\libraries\System.Runtime\tests\TestModule\System.Reflection.TestModule.ilproj]

The system cannot find the path specified.

This comment has been minimized.

Copy link
@safern

safern Dec 5, 2019

Member

It seems like you ran it against the solution file. Try running it under the project src directory directly so that it runs on the src csproj only.

This comment has been minimized.

Copy link
@agocke

agocke Dec 5, 2019

Author Contributor

OK, did that, and no errors, but it basically just deleted most of the System.Runtime.cs file.

This comment has been minimized.

Copy link
@agocke

agocke Dec 5, 2019

Author Contributor

Also, will this fix the "seed assembly" problem I mentioned below?

This comment has been minimized.

Copy link
@safern

safern Dec 5, 2019

Member

OK, did that, and no errors, but it basically just deleted most of the System.Runtime.cs file.

Hmm weird. Did you build against a locally built System.Private.CoreLib? Could you paste the command that it generated when invoking GenAPI? You can get it via a binlog.

Also, will this fix the "seed assembly" problem I mentioned below?

That is a GenFacades issue. @ericstj or @Anipik would be able to help you best.

This comment has been minimized.

Copy link
@safern

safern Dec 5, 2019

Member

Also, will this fix the "seed assembly" problem I mentioned below?

Probably it is because you didn't built against an updated S.P.CoreLib which contains your new type.

/// applies to that method and all nested functions (lambdas, local
/// functions) below it. If applied to a type or module, it applies to all
/// methods nested inside.
/// </summary>

This comment has been minimized.

Copy link
@stephentoub

stephentoub Dec 4, 2019

Member

Nit: this is a large summary, e.g. it'd be a lot of text showing up in IntelliSense. Can everything after the first sentence be moved to remarks?

@agocke

This comment has been minimized.

Copy link
Contributor Author

agocke commented Dec 5, 2019

I'm getting the error:

##[error].packages\microsoft.dotnet.genfacades\5.0.0-beta.19577.4\build\Microsoft.DotNet.GenPartialFacadeSource.targets(30,5): error : Did not find type 'System.Runtime.CompilerServices.SkipLocalsInitAttribute' in any of the seed assemblies.

What's a seed assembly?

@agocke

This comment has been minimized.

Copy link
Contributor Author

agocke commented Dec 6, 2019

@ericstj Any idea what I'm doing wrong here?

@safern

This comment has been minimized.

Copy link
Member

safern commented Dec 6, 2019

Did you build agains a live CoreCLR?

@agocke

This comment has been minimized.

Copy link
Contributor Author

agocke commented Dec 6, 2019

I did nothing except run build in the root

@safern

This comment has been minimized.

Copy link
Member

safern commented Dec 6, 2019

I did nothing except run build in the root.

I see. So actually that will build libraries against 2 week old version of coreclr which is the last transport package produced before we consolidated the repo. In order to be able to build locally against a live build of coreclr you need to first build:

coreclr.cmd you could just build S.P.CoreLib for this purpose so, coreclr.cmd -windowsmscorlib.

Then you want to build libraries against that by running:
libraries.cmd /p:CoreCLROverridePath=<path-to-folder-containing-S.P.CoreLib>

In PR builds we're already building against a live CoreCLR as of yesterday morning, so we could retrigger CI and you will not see this issue in your PR builds. However, until: #494 is merged we shouldn't be merging cross repo changes as it would break local people development, so I'm adding NO-MERGE to this PR in the meantime.

@safern

This comment has been minimized.

Copy link
Member

safern commented Dec 6, 2019

/azp run runtime-libraries

@safern safern added the * NO MERGE * label Dec 6, 2019
@azure-pipelines

This comment has been minimized.

Copy link

azure-pipelines bot commented Dec 6, 2019

Azure Pipelines successfully started running 1 pipeline(s).
@agocke

This comment has been minimized.

Copy link
Contributor Author

agocke commented Dec 10, 2019

@safern Looks like the build is stalling out

@agocke agocke closed this Dec 10, 2019
@agocke agocke reopened this Dec 10, 2019
@safern

This comment has been minimized.

Copy link
Member

safern commented Dec 13, 2019

This can now be merged as we've merged the PR to build live-live locally. However I'd like to re-run CI just to make sure is up-to-date and green.

@safern safern closed this Dec 13, 2019
@safern safern reopened this Dec 13, 2019
@safern safern removed the * NO MERGE * label Dec 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.