Bug 217740: don't crash when emitting or loading a DateTimeConstant(-1) attribute

[jcouv]

There are two scenarios:

1. Compiling source that includes a DateTimeConstant(-1).
   This is the scenario reported by the Watson bug.
   The fix in CommonAttributeData.cs restores the behavior of the native compiler (nil .param, and DateTimeConstantAttribute(-1), as shown below).
2. Loading such attribute from metadata. Inspection of Rolsyn code found this issue, which was then confirmed to exists.
   Roslyn would crash when encountering such an attribute (and the optional flag is set on the parameter).
   In such a case, the native compilers would instead succeed, although the resulting binary would crash.
   The fix to Rolsyn in PEModule.cs makes the loading of such an attribute to load a default value instead. This change breaks back compat and will be submitted to the compat-council shortly.

                .param [1]
                .custom instance void [mscorlib]System.Runtime.CompilerServices.DateTimeConstantAttribute::.ctor(int64) = (
                                01 00 ff ff ff ff ff ff ff ff 00 00
                )

Fixes this Watson crash.

[jcouv]

@dotnet/roslyn-compiler Please review.

@MattGertz Please approve for update 3.

In the meantime, I will get approval from the compat council.

[MattGertz]

This would meet the bar, but ping me again when compat/PRs/testing all aligns.

[AlekseyTs]

I think we need to have a test that tries to consume this method without providing the argument

• in the same compilation
• in another compilation, referencing metadata produced by this one.

Is behavior for these scenarios consistent with native compiler?

[AlekseyTs]

Should test the same when default value is explicitly set through the special syntax on the parameter.
Should test similar scenarios for VB too.

void m1([DateTimeConstant(-1)] DateTime x = default(DateTime))

---

In reply to: 64497023 [](ancestors = 64497023)

[AlekseyTs]

What we need to watch out for in cases when values are also provided through special syntax is that we wouldn't try to emit two or more DateTimeConstantValue attributes.

---

In reply to: 64497476 [](ancestors = 64497476,64497023)

[jcouv]

I already manually verified the behavior when referencing metadata. The behavior is consistent, an error is reported (C# used to report error CS1501: No overload for method 'Method' takes 0 arguments and VB used to report error BC30455: Argument not specified for parameter 'p' of 'Public Function Method(p As Date = #12:00:00 AM#) As Date').
I cannot do compilation reference with native compiler.

I added a test for both scenarios (C# and VB).
I'll add test for "double attribute" concern.

[jcouv]

The "double attribute" scenario works fine in C# (only one is emitted), but VB emits two. I'll stop by.

[AlekseyTs]

Yes, it looks like C# never synthesizes the attribute.

---

In reply to: 64610028 [](ancestors = 64610028)

[AlekseyTs]

Please use AssertTheseDiagnostics API

[jcouv]

Done.

[jcouv]

@AlekseyTs The PR is updated with all the scenarios we discussed.
I wrote a new summary for the compat council.

[AlekseyTs]

Is there similar code for constant fields?

[jcouv]

Yes. I'll check it out.

Also, I noticed that some paths still check for ConstantValue.IsBad, for example DecodeDefaultParameterValueAttribute. I'll check those out too.

[jcouv]

@AlekseyTs Pushed the fix we just discussed.

[AlekseyTs]

LGTM

[jcouv]

Thanks a lot.

@dotnet/roslyn-compiler Could I have a second review for this update 3 bug?

[jcouv]

@MattGertz All checks are green. I have one review approval and one compat approval. I'll get the second one of each shortly.
Can you approve also?

[VSadov]

LGTM