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 3 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
104 changes: 103 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,107 @@ 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.None, theParameter.Flags);

// 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 constantHandle = peModule.Module.MetadataReader.GetParameter(theParameter.Handle).GetDefaultValue();
Assert.True(constantHandle.IsNil);
};

var comp = CompileAndVerify(source, additionalRefs: new[] { SystemRef }, symbolValidator: verifier);
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 +3278,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
13 changes: 11 additions & 2 deletions src/Compilers/Core/Portable/MetadataReader/PEModule.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1046,7 +1046,16 @@ internal bool HasDateTimeConstantAttribute(EntityHandle token, out ConstantValue
AttributeInfo info = FindLastTargetAttribute(token, AttributeDescription.DateTimeConstantAttribute);
if (info.HasValue && TryExtractLongValueFromAttribute(info.Handle, out value))
{
defaultValue = ConstantValue.Create(new DateTime(value));
// if value is outside this range, DateTime would throw when constructed
if (value < DateTime.MinValue.Ticks || value > DateTime.MaxValue.Ticks)
{
defaultValue = ConstantValue.Bad;
}
else
{
defaultValue = ConstantValue.Create(new DateTime(value));
}

return true;
}

Expand Down Expand Up @@ -1242,7 +1251,7 @@ internal bool TryExtractStringValueFromAttribute(CustomAttributeHandle handle, o
return TryExtractValueFromAttribute(handle, out value, s_attributeStringValueExtractor);
}

private bool TryExtractLongValueFromAttribute(CustomAttributeHandle handle, out long value)
internal bool TryExtractLongValueFromAttribute(CustomAttributeHandle handle, out long value)
{
return TryExtractValueFromAttribute(handle, out value, s_attributeLongValueExtractor);
}
Expand Down
11 changes: 10 additions & 1 deletion src/Compilers/Core/Portable/Symbols/Attributes/CommonAttributeData.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,16 @@ internal ConstantValue DecodeDecimalConstantValue()

internal ConstantValue DecodeDateTimeConstantValue()
{
return ConstantValue.Create(new DateTime(this.CommonConstructorArguments[0].DecodeValue<long>(SpecialType.System_Int64)));
long value = this.CommonConstructorArguments[0].DecodeValue<long>(SpecialType.System_Int64);

// if value is outside this range, DateTime would throw when constructed
if (value < DateTime.MinValue.Ticks || value > DateTime.MaxValue.Ticks)
{
// Treat as-is parameter doesn't have default
return ConstantValue.Unset;
}

return ConstantValue.Create(new DateTime(value));
}

#endregion
Expand Down
174 changes: 170 additions & 4 deletions src/Compilers/VisualBasic/Test/Emit/Attributes/AttributeTests_WellKnownAttributes.vb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,16 @@
' Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

Imports Microsoft.CodeAnalysis.Test.Utilities
Imports Microsoft.CodeAnalysis.VisualBasic.Symbols
Imports Microsoft.CodeAnalysis.VisualBasic.Symbols.Metadata.PE
Imports Roslyn.Test.Utilities
Imports System.Collections.Immutable
Imports System.Reflection
Imports System.Reflection.Metadata
Imports System.Reflection.Metadata.Ecma335
Imports System.Runtime.InteropServices
Imports System.Text
Imports Microsoft.CodeAnalysis
Imports Microsoft.CodeAnalysis.Test.Utilities
Imports Microsoft.CodeAnalysis.VisualBasic.Symbols
Imports Microsoft.CodeAnalysis.VisualBasic.Symbols.Metadata.PE
Imports Roslyn.Test.Utilities

Namespace Microsoft.CodeAnalysis.VisualBasic.UnitTests.Semantics
Public Class AttributeTests_WellKnownAttributes
Expand Down Expand Up @@ -433,6 +434,171 @@ End Class
CompileAndVerify(source, sourceSymbolValidator:=attributeValidator)
End Sub

<WorkItem(217740, "https://devdiv.visualstudio.com/DefaultCollection/DevDiv/_workitems?id=217740")>
<Fact()>
Public Sub DateTimeConstantAttribute()
Dim source =
<compilation>
<file name="attr.vb"><![CDATA[
Imports System
Imports System.Runtime.CompilerServices

Public Class Bar
Sub Method(<DateTimeConstant(-1)> p1 As DateTime)
End Sub
End Class
]]>
</file>
</compilation>

Dim symValidator As Action(Of ModuleSymbol) =
Sub(peModule)

Dim bar = peModule.GlobalNamespace.GetMember(Of NamedTypeSymbol)("Bar")
Dim method = bar.GetMember(Of MethodSymbol)("Method")
Dim parameters = method.Parameters
Dim theParameter = DirectCast(parameters(0), PEParameterSymbol)
Dim peModuleSymbol = DirectCast(peModule, PEModuleSymbol)

Assert.Equal(ParameterAttributes.None, theParameter.ParamFlags)

' let's find the attribute in the PE metadata
Dim attributeInfo = CodeAnalysis.PEModule.FindTargetAttribute(peModuleSymbol.Module.MetadataReader, theParameter.Handle, AttributeDescription.DateTimeConstantAttribute)
Assert.True(attributeInfo.HasValue)

Dim attributeValue As Long
Assert.True(peModuleSymbol.Module.TryExtractLongValueFromAttribute(attributeInfo.Handle, attributeValue))
Assert.Equal(-1L, attributeValue)

' check .param has no value
Dim constantHandle = peModuleSymbol.Module.MetadataReader.GetParameter(theParameter.Handle).GetDefaultValue()
Assert.True(constantHandle.IsNil)
End Sub

CompileAndVerify(source, symbolValidator:=symValidator)
End Sub

<WorkItem(217740, "https://devdiv.visualstudio.com/DefaultCollection/DevDiv/_workitems?id=217740")>
<Fact()>
Public Sub LoadingDateTimeConstantWithBadValueOnField()
Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn't look like this test is actually using the field.

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 the intent is to use the field in the compilation that references the IL.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed. Thanks

Dim ilSource = <![CDATA[
.class public auto ansi C
extends [mscorlib]System.Object
{
.field private static initonly 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 )
.method public 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

.method public instance void Method() cil managed
{
// Code size 29 (0x1d)
.maxstack 2
.locals init (valuetype [mscorlib]System.DateTime V_0,
valuetype [mscorlib]System.DateTime V_1,
valuetype [mscorlib]System.DateTime V_2,
valuetype [mscorlib]System.DateTime V_3,
valuetype [mscorlib]System.DateTime V_4,
valuetype [mscorlib]System.DateTime V_5,
valuetype [mscorlib]System.DateTime V_6,
valuetype [mscorlib]System.DateTime V_7,
valuetype [mscorlib]System.DateTime V_8,
valuetype [mscorlib]System.DateTime V_9,
valuetype [mscorlib]System.DateTime V_10,
valuetype [mscorlib]System.DateTime V_11,
valuetype [mscorlib]System.DateTime V_12)
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like only one local is actually used, consider deleting the rest.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.
This was actually emitted by the native VB compiler (with lots of un-necessary locals). I verified that Roslyn only emits one.

IL_0000: ldc.i8 0x8a037f6423e7380
IL_0009: newobj instance void [mscorlib]System.DateTime::.ctor(int64)
IL_000e: stloc.s V_12
IL_0010: ldloca.s V_12
IL_0012: call instance int64 [mscorlib]System.DateTime::get_Ticks()
IL_0017: call void [mscorlib]System.Console::WriteLine(int64)
IL_001c: ret
} // end of method C::Method

} // end of class C
]]>

Dim source =
<compilation>
<file name="attr.vb"><![CDATA[
Public Class D
Shared Sub Main()
Dim test = New C()
test.Method()
End Sub
End Class
]]>
</file>
</compilation>

Dim ilReference = CompileIL(ilSource.Value)
CompileAndVerify(source, expectedOutput:="621558279390000000", additionalRefs:={ilReference})
' This behavior hasn't changed from native VB compiler
End Sub

<WorkItem(217740, "https://devdiv.visualstudio.com/DefaultCollection/DevDiv/_workitems?id=217740")>
<Fact()>
Public Sub LoadingDateTimeConstantWithBadValue()
Dim ilSource = <![CDATA[
.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
]]>

Dim source =
<compilation>
<file name="attr.vb"><![CDATA[
Public Class D

Shared Sub Main()
System.Console.WriteLine(New C().Method().Ticks)
End Sub
End Class
]]>
</file>
</compilation>

Dim ilReference = CompileIL(ilSource.Value)
CompileAndVerify(source, expectedOutput:="0", additionalRefs:={ilReference})
' The native compiler would produce a working exe, but that exe would fail at runtime
End Sub

<WorkItem(531121, "http://vstfdevdiv:8080/DevDiv2/DevDiv/_workitems/edit/531121")>
<Fact()>
Public Sub TestDecimalConstantAttribute()
Expand Down