Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 13 commits into from
May 26, 2016
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
0e97699
Bug 217740: Repro and test harness for C#
jcouv May 24, 2016
255d39d
Testing and fixing loading scenario in C# and VB
jcouv May 24, 2016
8cfaf34
Verifying VB case using attribute on field
jcouv May 24, 2016
1b0abed
Adding more tests
jcouv May 25, 2016
a2072f0
Adding VB tests
jcouv May 25, 2016
9523309
Adding tests
jcouv May 26, 2016
7744381
More tests on fields with double-attributes
jcouv May 26, 2016
aab3bb8
New test constraint
jcouv May 26, 2016
5654f9d
Fixing VB side to new test constraints
jcouv May 26, 2016
d722b1c
Fixing C# side to new test constraints
jcouv May 26, 2016
e7f5645
Fixing remaining tests
jcouv May 26, 2016
784703f
Adding comments
jcouv May 26, 2016
9da1381
Fixing VB field scenario too
jcouv May 26, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 1 addition & 2 deletions src/Compilers/CSharp/Portable/Symbols/Source/SourceComplexParameterSymbol.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -642,8 +642,7 @@ private void VerifyParamDefaultValueMatchesAttributeIfAny(ConstantValue value, C
if (data != null)
{
var attrValue = data.DefaultParameterValue;
if (!attrValue.IsBad &&
(attrValue != ConstantValue.Unset) &&
if ((attrValue != ConstantValue.Unset) &&
(value != attrValue))
{
// CS8017: The parameter has multiple distinct default values.
Expand Down
278 changes: 277 additions & 1 deletion src/Compilers/CSharp/Test/Emit/Attributes/AttributeTests_WellKnownAttributes.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
using System.Runtime.InteropServices;
using System.Text;
using Microsoft.CodeAnalysis.CSharp.Symbols;
using Microsoft.CodeAnalysis.CSharp.Symbols.Metadata.PE;
using Microsoft.CodeAnalysis.CSharp.Syntax;
using Microsoft.CodeAnalysis.CSharp.Test.Utilities;
using Microsoft.CodeAnalysis.Test.Utilities;
Expand Down Expand Up @@ -416,6 +417,281 @@ class C
CompileAndVerify(text, additionalRefs: new[] { SystemRef }, sourceSymbolValidator: attributeValidator);
}

[Fact]
[WorkItem(217740, "https://devdiv.visualstudio.com/DefaultCollection/DevDiv/_workitems?id=217740")]
public void DateTimeConstantAttribute()
{
#region "Source"
var source = @"
using System;
using System.Runtime.CompilerServices;

public class Bar
{
public void Method([DateTimeConstant(-1)]DateTime p1) { }
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

@AlekseyTs AlekseyTs May 25, 2016 •
edited

Choose a reason for hiding this comment

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

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)

Copy link
Contributor

Choose a reason for hiding this comment

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

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)

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

}
";
#endregion

// The native C# compiler emits this:
// .param[1]
// .custom instance void[mscorlib] System.Runtime.CompilerServices.DateTimeConstantAttribute::.ctor(int64) = (
// 01 00 ff ff ff ff ff ff ff ff 00 00
// )
Action<IModuleSymbol> verifier = (module) =>
{
var bar = (NamedTypeSymbol)((ModuleSymbol)module).GlobalNamespace.GetMember("Bar");
var method = (MethodSymbol)bar.GetMember("Method");
var parameters = method.GetParameters();
var theParameter = (PEParameterSymbol)parameters[0];
var peModule = (PEModuleSymbol)module;

Assert.Equal(ParameterAttributes.HasDefault, theParameter.Flags); // native compiler has None instead

// let's find the attribute in the PE metadata
var attributeInfo = PEModule.FindTargetAttribute(peModule.Module.MetadataReader, theParameter.Handle, AttributeDescription.DateTimeConstantAttribute);
Assert.True(attributeInfo.HasValue);

long attributeValue;
Assert.True(peModule.Module.TryExtractLongValueFromAttribute(attributeInfo.Handle, out attributeValue));
Assert.Equal(-1L, attributeValue); // check the attribute is constructed with a -1

// check .param has no value
var constantValue = peModule.Module.GetParamDefaultValue(theParameter.Handle);
Assert.Equal(ConstantValue.Null, constantValue);
};

var comp = CompileAndVerify(source, symbolValidator: verifier);
comp.VerifyDiagnostics();
}

[Fact]
[WorkItem(217740, "https://devdiv.visualstudio.com/DefaultCollection/DevDiv/_workitems?id=217740")]
public void DateTimeConstantAttributeReferencedViaRef()
{
#region "Source"
var source1 = @"
using System;
using System.Runtime.CompilerServices;

public class Bar
{
public void Method([DateTimeConstant(-1)]DateTime p1) { }
}
";

var source2 = @"
public class Consumer
{
public static void M()
{
new Bar().Method();
}
}
";
#endregion

var libComp = CreateCompilationWithMscorlib(source1);
var libCompRef = new CSharpCompilationReference(libComp);

var comp2 = CreateCompilationWithMscorlib(source2, new[] { libCompRef });
comp2.VerifyDiagnostics(
// (6,19): error CS7036: There is no argument given that corresponds to the required formal parameter 'p1' of 'Bar.Method(DateTime)'
// new Bar().Method();
Diagnostic(ErrorCode.ERR_NoCorrespondingArgument, "Method").WithArguments("p1", "Bar.Method(System.DateTime)").WithLocation(6, 19)
);

// The native compiler also gives an error: error CS1501: No overload for method 'Method' takes 0 arguments
var libAssemblyRef = libComp.EmitToImageReference();
var comp3 = CreateCompilationWithMscorlib(source2, new[] { libAssemblyRef });
comp3.VerifyDiagnostics(
// (6,19): error CS7036: There is no argument given that corresponds to the required formal parameter 'p1' of 'Bar.Method(DateTime)'
// new Bar().Method();
Diagnostic(ErrorCode.ERR_NoCorrespondingArgument, "Method").WithArguments("p1", "Bar.Method(System.DateTime)").WithLocation(6, 19)
);
}

[Fact]
[WorkItem(217740, "https://devdiv.visualstudio.com/DefaultCollection/DevDiv/_workitems?id=217740")]
public void DateTimeConstantAttributeWithBadDefaultValue()
{
#region "Source"
var source = @"
using System;
using System.Runtime.CompilerServices;

public class Bar
{
public DateTime M1([DateTimeConstant(-1)] DateTime x = default(DateTime)) { return x; }
public static void Main()
{
Console.WriteLine(new Bar().M1().Ticks);
}
}
";
#endregion

// The native C# compiler would succeed and emit this:
// .method public hidebysig instance void M1([opt] valuetype[mscorlib] System.DateTime x) cil managed
// {
// .param [1] = nullref
// .custom instance void[mscorlib] System.Runtime.CompilerServices.DateTimeConstantAttribute::.ctor(int64) = ( 01 00 FF FF FF FF FF FF FF FF 00 00 )

var comp = CreateCompilationWithMscorlib(source);
comp.VerifyDiagnostics(
// (7,60): error CS8017: The parameter has multiple distinct default values.
// public DateTime M1([DateTimeConstant(-1)] DateTime x = default(DateTime)) { return x; }
Diagnostic(ErrorCode.ERR_ParamDefaultValueDiffersFromAttribute, "default(DateTime)").WithLocation(7, 60)
);
}

[Fact]
[WorkItem(217740, "https://devdiv.visualstudio.com/DefaultCollection/DevDiv/_workitems?id=217740")]
public void DateTimeConstantAttributeWithValidDefaultValue()
{
#region "Source"
var source = @"
using System;
using System.Runtime.CompilerServices;

public class Bar
{
public DateTime M1([DateTimeConstant(42)] DateTime x = default(DateTime)) { return x; }
public static void Main()
{
Console.WriteLine(new Bar().M1().Ticks);
}
}
";
#endregion

// The native C# compiler emits this:
// .param [1] = nullref
// .custom instance void[mscorlib] System.Runtime.CompilerServices.DateTimeConstantAttribute::.ctor(int64) = (01 00 2A 00 00 00 00 00 00 00 00 00 )

var comp = CreateCompilationWithMscorlib(source);
comp.VerifyDiagnostics(
// (7,60): error CS8017: The parameter has multiple distinct default values.
// public DateTime M1([DateTimeConstant(42)] DateTime x = default(DateTime)) { return x; }
Diagnostic(ErrorCode.ERR_ParamDefaultValueDiffersFromAttribute, "default(DateTime)").WithLocation(7, 60)
);
}

[Fact]
[WorkItem(217740, "https://devdiv.visualstudio.com/DefaultCollection/DevDiv/_workitems?id=217740")]
public void DateTimeConstantAttributeWithBadDefaultValueOnField()
{
#region "Source"
var source = @"
using System;
using System.Runtime.CompilerServices;

public class C
{
[DateTimeConstant(-1)]
public DateTime F = default(DateTime);

public static void Main()
{
System.Console.WriteLine(new C().F.Ticks);
}
}
";
#endregion

// The native C# compiler emits this:
// .field public valuetype[mscorlib] System.DateTime F
// .custom instance void[mscorlib] System.Runtime.CompilerServices.DateTimeConstantAttribute::.ctor(int64) = ( 01 00 FF FF FF FF FF FF FF FF 00 00 )

// using the native compiler, this code outputs 0
var comp = CompileAndVerify(source, expectedOutput: "0");
comp.VerifyDiagnostics();
}

[Fact]
[WorkItem(217740, "https://devdiv.visualstudio.com/DefaultCollection/DevDiv/_workitems?id=217740")]
public void DateTimeConstantAttributeWithValidDefaultValueOnField()
{
#region "Source"
var source = @"
using System;
using System.Runtime.CompilerServices;

public class C
{
[DateTimeConstant(42)]
public DateTime F = default(DateTime);

public static void Main()
{
System.Console.WriteLine(new C().F.Ticks);
}
}
";
#endregion

// The native C# compiler emits this:
// .field public valuetype[mscorlib] System.DateTime F
// .custom instance void[mscorlib] System.Runtime.CompilerServices.DateTimeConstantAttribute::.ctor(int64) = ( 01 00 2A 00 00 00 00 00 00 00 00 00 )

// Using the native compiler, the code executes to output 0
var comp = CompileAndVerify(source, expectedOutput: "0");
comp.VerifyDiagnostics();
}

[Fact, WorkItem(217740, "https://devdiv.visualstudio.com/DefaultCollection/DevDiv/_workitems?id=217740")]
public void LoadingDateTimeConstantWithBadValue()
{
var ilsource = @"
.class public auto ansi beforefieldinit C
extends [mscorlib]System.Object
{
.method public hidebysig instance valuetype [mscorlib]System.DateTime
Method([opt] valuetype [mscorlib]System.DateTime p) cil managed
{
.param [1]
.custom instance void [mscorlib]System.Runtime.CompilerServices.DateTimeConstantAttribute::.ctor(int64) = ( 01 00 FF FF FF FF FF FF FF FF 00 00 )
// Code size 7 (0x7)
.maxstack 1
.locals init (valuetype [mscorlib]System.DateTime V_0)
IL_0000: nop
IL_0001: ldarg.1
IL_0002: stloc.0
IL_0003: br.s IL_0005

IL_0005: ldloc.0
IL_0006: ret
} // end of method C::Method

.method public hidebysig specialname rtspecialname
instance void .ctor() cil managed
{
// Code size 7 (0x7)
.maxstack 8
IL_0000: ldarg.0
IL_0001: call instance void [mscorlib]System.Object::.ctor()
IL_0006: ret
} // end of method C::.ctor

} // end of class C

";

var cssource = @"
public class D
{
public static void Main()
{
System.Console.WriteLine(new C().Method().Ticks);
}
}
";

var ilReference = CompileIL(ilsource);
CompileAndVerify(cssource, expectedOutput: "0", additionalRefs: new[] { ilReference });
// The native compiler would produce a working exe, but that exe would fail at runtime
}

[Fact]
public void TestDecimalConstantAttribute()
{
Expand Down Expand Up @@ -3176,7 +3452,7 @@ public static int Main ()

// the resulting code does not need to verify
// This is consistent with Dev10 behavior
CompileAndVerify(source, options: TestOptions.ReleaseDll, verify:false, sourceSymbolValidator: sourceValidator, symbolValidator: metadataValidator);
CompileAndVerify(source, options: TestOptions.ReleaseDll, verify: false, sourceSymbolValidator: sourceValidator, symbolValidator: metadataValidator);
}

[Fact, WorkItem(544507, "http://vstfdevdiv:8080/DevDiv2/DevDiv/_workitems/edit/544507")]
Expand Down