Skip to content
This repository was archived by the owner on Nov 1, 2020. It is now read-only.

[ILVerify] Implement verification of initonly fields#4914

Merged
jkotas merged 2 commits intodotnet:masterfrom
Dynatrace:initOnlyField
Nov 11, 2017
Merged

[ILVerify] Implement verification of initonly fields#4914
jkotas merged 2 commits intodotnet:masterfrom
Dynatrace:initOnlyField

Conversation

@ArztSamuel
Copy link
Copy Markdown
Collaborator

As described in ECMA II.16.1.2 Field contract attributes, initonly fields should only be mutable in their corresponding constructor. This implements this rule.

As mentioned in #4911 and noted in ECMA, a ldflda of an initonly field should always be unverifiable. However, PEVerify does not report an error for ldflda instructions in ctors. I was not sure how to deal with this case for ILVerify and have stuck to the way PEVerify does it for now, since this rule should be revisited after the proposed spec changes anyway.

This fixes #4911.

instance = null;

if (field.IsInitOnly)
Check(_method.IsStaticConstructor && field.OwningType == _method.OwningType, VerifierError.Initonly);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit - change the casing of VerifierError.Initonly to VerifierError.InitOnly to match the property name?

@jkotas
Copy link
Copy Markdown
Member

jkotas commented Nov 11, 2017

cc @jcouv @VSadov

Copy link
Copy Markdown
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ArztSamuel Thank you!

@jkotas jkotas merged commit 066cfc2 into dotnet:master Nov 11, 2017
//E_ADDR_BYREF "Address of not allowed for ByRef."
//E_ADDR_LITERAL "Address of not allowed for literal field."
//E_INITONLY "Cannot change initonly field outside its .ctor."
Initonly, // Cannot change initonly field outside its .ctor.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jkotas @ArztSamuel
Is this file the list of all checks/erros implemented by PEVerify, where all the uncommented ones have been implemented by ILVerify so far?

FYI @VSadov

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that is correct.
There are some errors that were added with ILVerify and a lot of commented errors early in the list are metadata related (as opposed to IL related).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the intention for ILVerify to only verify IL errors, or are metadata errors in scope too? Does it current enforce any metadata rules?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ILVerify does basic metadata validation as side-effect of reading the metadata and building the types out of it.

I do not see a reason why we would not want to add more in-depth metadata validation/verification to ILVerify.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not see a reason why we would not want to add more in-depth metadata validation/verification to ILVerify.

Yes, I agree. I simply concentrated on IL verifications for now, since basic metadata validation is done as a side-effect anyway, as you mentioned.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome. Thanks!

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.

Expect ILVerify error on initonly field outside its constructor

3 participants