Mono.Cecil does not set the HasFieldRVA field on a FieldDefinition when the InitialData property is set to non-null #728

New issue

Closed

opened 2021-03-12 17:52:52 -06:00 by jkoritzinsky · 5 comments

jkoritzinsky commented

2021-03-12 17:52:52 -06:00

(Migrated from github.com)

I'm trying to use the FieldDefinition.InitialData property to emit a static array into an image using the .data declaration feature in ECMA-335.

However, it seems that any data assigned to this field is dropped and not emitted as a .data declaration in the image.

I know that Mono.Cecil supports round-tripping the InitialData field from a loaded assembly back to disk if it hasn't been modified, but it seems that modifying it or assigning it on a new field is unsupported.

I'm trying to use the `FieldDefinition.InitialData` property to emit a static array into an image using the `.data` declaration feature in ECMA-335. However, it seems that any data assigned to this field is dropped and not emitted as a `.data` declaration in the image. I know that Mono.Cecil supports round-tripping the `InitialData` field from a loaded assembly back to disk if it hasn't been modified, but it seems that modifying it or assigning it on a new field is unsupported.

jbevain commented

2021-03-12 18:14:46 -06:00

(Migrated from github.com)

Hi @jkoritzinsky,

Thank you for filing this. That definitely sounds like a bug, which might be annoying because from a quick look that should not happen:

https://github.com/jbevain/cecil/blob/master/Mono.Cecil/FieldDefinition.cs#L114
https://github.com/jbevain/cecil/blob/master/Mono.Cecil/AssemblyWriter.cs#L1595

Any chance you could start a PR with a failing test?

Hi @jkoritzinsky, Thank you for filing this. That definitely sounds like a bug, which might be annoying because from a quick look that should not happen: https://github.com/jbevain/cecil/blob/master/Mono.Cecil/FieldDefinition.cs#L114 https://github.com/jbevain/cecil/blob/master/Mono.Cecil/AssemblyWriter.cs#L1595 Any chance you could start a PR with a failing test?

👍 1

jkoritzinsky commented

2021-03-15 16:59:29 -05:00

(Migrated from github.com)

I'll work on putting together a failing unit test for this.

jkoritzinsky commented

2021-03-15 18:12:08 -05:00

(Migrated from github.com)

While putting together a unit test, I found the issue.

When a new field is created in Mono.Cecil and an array is assigned to the InitialValue field, Mono.Cecil correctly emits the value and will read it back in. However, Mono.Cecil does not set the HasFieldRVA flag, so other tools like ILDasm and ILSpy do not correctly recognize that an RVA has been assigned.

When I manually set the HasFieldRVA flag, the other tools recognize the RVA. So thankfully I have a workaround.

It would be nice if the FieldDefinition class would add that flag when the InitialData property is set. I'll update the issue title.

While putting together a unit test, I found the issue. When a new field is created in Mono.Cecil and an array is assigned to the `InitialValue` field, Mono.Cecil correctly emits the value and will read it back in. However, Mono.Cecil does not set the `HasFieldRVA` flag, so other tools like ILDasm and ILSpy do not correctly recognize that an RVA has been assigned. When I manually set the `HasFieldRVA` flag, the other tools recognize the RVA. So thankfully I have a workaround. It would be nice if the `FieldDefinition` class would add that flag when the `InitialData` property is set. I'll update the issue title.

jbevain commented

2021-03-15 18:38:09 -05:00

(Migrated from github.com)

Hah! That makes sense. Yes we should do that, should be has simple as:

		public byte [] InitialValue {
			// get
			set {
				initial_value = value;
				rva = 0;
				HasFieldRVA = !value.IsNullOrEmpty();
			}
		}

@jkoritzinsky want to send a PR with the test & the fix?

Hah! That makes sense. Yes we should do that, should be has simple as: ```csharp public byte [] InitialValue { // get set { initial_value = value; rva = 0; HasFieldRVA = !value.IsNullOrEmpty(); } } ``` @jkoritzinsky want to send a PR with the test & the fix?

jkoritzinsky commented

2021-03-15 18:45:03 -05:00

(Migrated from github.com)