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

Added CompilerServices.SkipLocalsInitAttribute #20093

Closed
wants to merge 1 commit into from

Conversation

lkts
Copy link

@lkts lkts commented Sep 21, 2018

Contributes to https://github.com/dotnet/corefx/issues/29026.

I assume IsEnabled is not needed since there is no override behavior.

@tannergooding

@tannergooding
Copy link
Member

CC. @jcouv, @agocke, @jaredpar

@stephentoub
Copy link
Member

@jaredpar or @agocke, can you comment on the language/compiler feature status for this? Will C# 8 have the feature such that we should merge this now and get it exposed in corefx? Should we wait? Close this? Etc. Thanks.

@jaredpar
Copy link
Member

This will not be in C# 8.0. I think it's likely to come later though 8.1, or beyond.

@stephentoub
Copy link
Member

This will not be in C# 8.0. I think it's likely to come later though 8.1, or beyond.

Thanks, @jaredpar. Is it definitely going to happen, and when it does, is this definitely the interface it'll use?

With thanks to @lkts, I'm tempted to say we should close this and only bring it back when we're ready to pursue it in the compiler.

@jaredpar
Copy link
Member

@stephentoub

Is it definitely going to happen, and when it does, is this definitely the interface it'll use?

I think it's about as certain as we can be about a language feature. The attribute has already gotten LDM approval hence it is fairly likely.

But with all language features it's not shipped until it's shipped. My 2 cents: i'd hold off on merging this until we can get the resources together to commit to getting it into a C# release.

@stephentoub
Copy link
Member

Thanks, @jaredpar.

@lkts, thanks for submitting this. Given the status of the feature, I'm going to close this for now. We can re-open it when the compiler moves ahead with it as well.

@lkts lkts deleted the expose-skiplocalinit branch March 20, 2019 08:02
@agocke
Copy link
Member

agocke commented Sep 16, 2019

I think it's time to re-open this, since we're committing SkipLocalsInit for the next compiler release.

@tannergooding
Copy link
Member

@agocke, does the given API match what the compiler will be adding for the next release?

Is this something that we can track internally and reopen as soon as the compiler support is merged or is the compiler wanting the framework API before it starts the work (I think, normally, the compiler just supports manually declared attributes for these kinds of things, so I would guess we could wait for compiler support to be merged).

@agocke
Copy link
Member

agocke commented Sep 16, 2019

This looks like the right surface area, to me. Notably, Assembly is not supported as a target in the compiler, so it should not appear on the attribute, which seems to be what's implemented.

@tannergooding
Copy link
Member

To be clear, Assembly is not supported but Module is?

@stephentoub
Copy link
Member

Why wouldn't Assembly be supported by the compiler? Assuming we'd use this in corelib/corefx instead of illink, I think we'd want that.

@agocke
Copy link
Member

agocke commented Sep 16, 2019

I can't really find why we did it, but it seems intentional. I think it might have been caution about net modules? If you put the attribute on an assembly and compile to a netmodule, then the attribute will get lifted to the assembly level when the netmodule is compiled, I think, but the linker that's including the netmodule may not respect the assembly level attribute.

Is the module target that much different? Do you have multi-module assemblies?

@stephentoub
Copy link
Member

Shouldn't this also be allowed on interfaces, now that we have default interface methods?

Is the module target that much different? Do you have multi-module assemblies?

I don't think we do in corefx, though I don't know for sure.

@tannergooding
Copy link
Member

Shouldn't this also be allowed on interfaces, now that we have default interface methods?

I would imagine yes. There may actually be other attributes in the framework which are allowed on class/struct today that would also apply to default interface methods... Perhaps those should be audited?

@agocke
Copy link
Member

agocke commented Sep 17, 2019

Shouldn't this also be allowed on interfaces, now that we have default interface methods?

Yes, this makes sense. I will add test for it.

agocke added a commit to dotnet/runtime that referenced this pull request Nov 21, 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
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants