Add SkipLocalsInitAttribute#454
Conversation
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
|
Nit: For future PRs, we prefer creating the branch in fork: https://github.com/dotnet/runtime/blob/master/docs/issues-pr-management.md#what-will-be-dotnetruntimes-branch-policy |
|
yeah, sorry, that was my mistake in cloning the repo, future PRs will be in my fork. |
| 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)] |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
runtime/src/libraries/Directory.Build.targets
Line 132 in 6afe96c
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
OK, did that, and no errors, but it basically just deleted most of the System.Runtime.cs file.
There was a problem hiding this comment.
Also, will this fix the "seed assembly" problem I mentioned below?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
I'm getting the error: What's a seed assembly? |
|
@ericstj Any idea what I'm doing wrong here? |
|
Did you build agains a live CoreCLR? |
|
I did nothing except run |
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:
Then you want to build libraries against that by running: 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. |
|
/azp run runtime-libraries |
safern
added
the
NO-MERGE
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@safern Looks like the build is stalling out |
|
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
removed
the
NO-MERGE
ghost
locked as resolved and limited conversation to collaborators
7 participants
This attribute supports a new compiler feature that allows a user to
specify that they don't want to emit the CLR
.locals initportion ofthe 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