Skip to content
This repository has been archived by the owner on Jan 11, 2024. It is now read-only.

Updating Roslyn to version 2.6.0-rdonly-ref-62111-06 #1681

Merged
merged 1 commit into from
Sep 18, 2017

Conversation

joperezr
Copy link
Member

FYI: @VSadov @weshaggard

Needed for dotnet/corefx#24076

Updating to a newer version of Roslyn that supports the compiler flag to disable validation generation.

@ericstj
Copy link
Member

ericstj commented Sep 18, 2017

that supports the compiler flag to disable validation generation.

Can you tell me more about this? It sounds like something we need to "know" to set on certain assemblies and we'll only know that if we happen to actually test those assemblies (in a particular way/codepath?) on desktop. I'd be worried about forgetting to set that flag. Should we be doing it globally, or perhaps for particular targetgroup(s) that could run on desktop?

@joperezr
Copy link
Member Author

According to this comment, this something we want to set on all assemblies that are marked as SecurityTransparent. System.Collections.Immutable is one of them. The point here is to set the flag in all of those assemblies for now in order to unblock the update to the new compiler, and at the same time log issues in corefx so that we investigate on doing appropiate fixes if required in our code so that the new validation that gets generated will not throw.

@joperezr joperezr merged commit 81773bf into dotnet:master Sep 18, 2017
@joperezr joperezr deleted the RoslynUpdate branch September 18, 2017 18:38
@VSadov
Copy link
Member

VSadov commented Sep 18, 2017

Technically this is not a bug in Collections.Immutable.

The assumption is that producing strictly verifiable code (as in "compliant with PEVerify" or when the same checks are done at JIT time) is not a common requirement nowdays.
Compiler still produces Type-Safe IL, but in some case it may no longer be accepted by desktop verification.

In some scenarios there is no choice, features like ref-returning methods are not accepted by peverify and there is no way around that.
In few other cases compiler has a choice - emit more efficient code or less efficient, but something that verification accepts. For the latter case we have the flag.

What I mean is if [SecurityTransparent] is still a thing, then setting peverify-compat flag on [SecurityTransparent] assemblies is an appropriate fix. That is basically the purpose of the flag.

@VSadov
Copy link
Member

VSadov commented Sep 18, 2017

CC @jaredpar

@jaredpar
Copy link
Member

can we just remove that attribute?

there is no investment in verification at the moment. but we would like for this package to get faster

@joperezr
Copy link
Member Author

@VSadov does this mean that we want to add this flag to all of the assembilies that are flagged as SecurityTransparent? as opposed to only System.Collections.Immutable?

@VSadov
Copy link
Member

VSadov commented Sep 18, 2017

As long as an assembly is decorated with [SecurityTransparent] it seems safer to compile it with the flag. Unless you are sure about the test coverage.
In theory the problem may happen in a method that is not covered by tests. Then, when user runs that, verification exception will be thrown.

@jaredpar
Copy link
Member

That means we wont ever be able to improve the perf of the immutable collections. That is a huge negative for us especially for such a legacy scenario

@VSadov
Copy link
Member

VSadov commented Sep 18, 2017

Also note that [SecurityTransparent] or any other feature that imply JIT-time verification is in a conflict with features like ref returns.

In the long term having [SecurityTransparent] may become a burden, so removal should definitely be considered.

@ericstj
Copy link
Member

ericstj commented Sep 18, 2017

File an issue in corefx asking for this. Someone will need to investigate if it creates a compatibility problem for folks who may have relied on it being marked transparent. I'm not sure myself and I'd hate for us to remove it then find out later that it made the assembly unusable from partial trust or something like that.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants