-
Notifications
You must be signed in to change notification settings - Fork 36
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.
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
Implement IL codes for #91, #93, fix #105 #104
base: devel
Are you sure you want to change the base?
Conversation
to resolve conflicts
Fixed causing undefined symbols related minimum requirements for il2c…
Merge devel back to implement-ilcodes
… DLL is in fact the mscorlib itself
No regression up to this point on already available unit tests, though new implementations are not tested yet |
… interface definitions
…m.Object, where there is no BaseType
No regression from unit tests. https://github.com/cyborgyn/CoreLibrary can be transpiled to C so far, however compiling it's C will fail (with 91 errors), as expected. But no syntax errors.
This is expected, as now there are multiple definitions conflicting in IL2C.Runtime and the transpiled mscorlib C sources. |
Wow! Nice, long CI pipeline. I looked into the results, and came across such ones:
I put those messages into it: "Bad MSIL, or test case!" and it means, even the .NET runtime fails to test correctly that test, before even running the actual compiled ones. In my case (why I added it at the first place) was, that I played around with pointers, which were not fixed, and sometimes GC moved the data around. But in this specific scenario, otherwise it's evaluating OK under Windows, it suggests that under .NET Core the MSIL or the BCL behaves differently, as the .NET Framework runtime. Odd, good to note! :) Also, a suggestion, this many unit tests will always fail, due to time out. Let's shorten the list the way: only execute a single test case randomly chosen per test method. Will still give a good coverage, and probably will have enough time to execute. |
Yes, regression test is important on IL2C and nice improvement for insightful message! (I'm working ports completion CI on GitHub Actions, currently broken on Windows test, please wait... #100 )
So that's it. In that case, it would be good to devise the timing of CI so that complete test and randomly select test can be separated. I'll think about it. Do you know a common way to randomly select test cases in NUnit? (I think it is necessary to implement the dynamic generation part well in order to realize it with IL2C) |
var xname = x.Name; | ||
var yname = y.Name; | ||
|
||
var rn = xname.CompareTo(yname); | ||
if (rn != 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cyborgyn Is this operator valid negative? (Equals method is same as?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since String implements IComparable, it's not a ReferenceEquals compare anymore defined on Object. It has 3 possible return values of -1, 0, 1, where 0 indicates match. In case the two strings xname and yname don't match, we do a second try, with explicit interface naming match. There were cases observed, where the Equals specifically was used, and even though it should have been match, it didn't get to the point to validate a parameter list, and returned false. The same logic was adapted to the sorting aid, Compare() method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I look to seem this expression makes short circuit evaluation:
var rn = xname.CompareTo(yname); // (a)
if (rn != 0)
{
if (x.DeclaringType.IsInterface)
{
xname = $"{x.DeclaringType.FriendlyName}.{xname}"; // (b-1)
}
if (y.DeclaringType.IsInterface)
{
yname = $"{y.DeclaringType.FriendlyName}.{yname}"; // (b-2)
}
rn = xname.CompareTo(yname); // (c)
if (rn != 0)
{
// ...
At (a) xname value and yname value is different (not 0), and contains string of last part at (b-1) and (b-2), so final result at (c) is always different...? (always not 0)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Think about the following scenario:
namespace NS {
interface Ib {
void Method();
}
class A : Ib {
void NS.Ib.Method() { ... }
}
}
The trouble was, for the class MethodInformation.Name was "NS.Ib.Method()", but in the interface definition it was just "Method()". So when searching for interface implementation on the class, it didn't find it. One of them's DeclaringType is the class, the other's is the interface. So, the Equals() always returned false, and in the CallConverters.cs, at line 100 it throw Exception at this place:
var implementationMethod = allDeclaredMethods.First(
dm => MetadataUtilities.VirtualMethodSignatureComparer.Equals(dm, m));
So, what this thing does, is just give an other chance, putting the full "namespace.interfaceTypeName." before the appropriate method name (x, or y), and does an other compare. A side effect in the case for ordering, is that all interface implementation would be packed together after each other, I guess, but it is not used for things like this, as I am aware. In the case of Equals() there is no side effect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was insight it and finally understood what is problem for ;)
See branch feature/implement-ilcodes-r1
/ 19bb2ea
There was nothing regression test for this code fragment, it's my badness....
The callvirt opcode devirtualization function try it a few conditions:
// Invoke interface method with the class-type instance and
// IL2C detected here for can cast statically.
else if (method.DeclaringType.IsInterface &&
arg0.TargetType.IsClass && // (a)
method.DeclaringType.IsAssignableFrom(arg0.TargetType))
At (a), this expression will test arg0 (this) is class
type (analysis by IL2C), but this case couldn't be realize when using C# compiler. (Maybe) C# compiler never generate these conditions.
So I added some test written IL (b159b49, 65ebbbe). These test will cause making (a) condition with true and false. And I understood your explain.
Then I thought of a way to make your method better. In this case, we can only check for method matches by symbolic name, only methods on class/value type.
For example, if the F# compiler generate an explicit interface implementation method, the method names will not necessarily match. To make sure that the methods match exactly, check if they are included in IMethodInformation.Overrides
property.
So, I made the following changes to make sure the unit test passes.
// Try strictly matching when both class/value type member method
// and explicitly implemented interface method are same.
if (!x.DeclaringType.IsInterface && y.DeclaringType.IsInterface)
{
if (x.Overrides.Contains(y))
{
return true;
}
}
if (x.DeclaringType.IsInterface && !y.DeclaringType.IsInterface)
{
if (y.Overrides.Contains(x))
{
return true;
}
}
decodeContext.CurrentCode.RawLocation, | ||
si0.TargetType.FriendlyName); | ||
|
||
if (si0.TargetType.IsInt32StackFriendlyType) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cyborgyn The IsInt32StackFriendlyType
property means "Can place evaluation stack these types, that's compatible int32 type on CLR".
Therefore use IsInt32Type
, IsInt64Type
, IsIntPtrType
and IsUIntPtrType
. By the way, can use TargetType
directly onto decodeContext.PushStack(...) argument.
We need regression test for not converter, require these topics:
- Apply not opcode each types
int32
int64
andIntPtr
. - Asserts equality not values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! This way, it will be much nicer. I am getting to know more and more of your code, and how parts are interacting with each other. I need to note, when I uncomment IntPtr unit tests, and also insert those into the IL, a UnitTestFramework fails to discover any tests, even though the produced DLL can be verified to be OK with DotPeek.
var si1 = decodeContext.PopStack(); | ||
var si0 = decodeContext.PopStack(); | ||
|
||
if (si0.TargetType.IsFloatStackFriendlyType || si0.TargetType.IsByReference || !si1.TargetType.IsInt32Type) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cyborgyn Likely not
opcode's integer type limitation.
ECMA-335: III.3.58 shl - shift integer left
section is written:
The shl instruction shifts value (int32, int64 or native int) left by the number of bits
specified by shiftAmount. shiftAmount is of type int32 or native int.
shiftAmount (si1
) tricky specification, it's excepted int64
.
We need regression test for shl
and shr
converter, require these topics:
- Apply
shl
/shr
opcode each types int32 int64 and IntPtr combination with shiftAmount by int32 and IntPtr. - Asserts equality constant values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have already added quite an amount of tests especially for shr & shr.un, as they were not that intuitive when they sign extend, and when not. I found this:
// Arithmetic Shift right (with sign)
// Warning!!! MSIL always operates for bit storage on Int32, or Int64
// and as it seems, smaller unsigned values (uint8 & uint16) will never
// be able to set the int32 highest bit => they will operate as logical shift right.
// On the other hand: uint32 and uint64 are still operated as arithmetic shift.
Will try to add the native int shift parameter as test, good point, thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately I needed to comment them out again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I will take a look on my environment, could you tell me explicit commit id reproducible?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it valid on newest commit on branch feature/implement-ilcodes
(ca5706c) ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commented: #104 (comment)
Thank you very much, will look into it soon! |
…into feature/implement-ilcodes
[TestCase(~(short)0x1234, "Not_int16", (short)0x1234)] | ||
[TestCase(~(ushort)0x1234, "Not_uint16", (ushort)0x1234)] | ||
// Breaks unit testing framework??? | ||
//[TestCase(~(sbyte)0x12, "Not_int8", (sbyte)0x12)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cyborgyn The test case result expression (TestCase attribute first argument) is object type, so (comparing) value can makes final cast operator.
Result with C# interactive window:
> (~(sbyte)0x12).GetType().FullName
"System.Int32"
Therefore:
[TestCase((sbyte)~(sbyte)0x12, "Not_int8", (sbyte)0x12)]
[TestCase(-1, "Not_UIntPtr", 0)] | ||
// If I implement in IL, whole testing system goes down. | ||
// C# compiler says: CS0023: Operator '~' cannot be applied to operand of type 'IntPtr' | ||
//[TestCase(~IntPtr.Zero, "not_intptr", 0)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IntPtr
couldn't make any constant value on .NET attribute. Because .NET attribute has to serialize argument into binary format, but IntPtr type is having to call constructor from any constant value.
In these cases, test framework will apply automatically conversion between IntPtr
/UIntPtr
and integer types at this then this.
Therefore:
[TestCase(~0, "not_intptr", 0)]
But IntPtr
value be going to cause different result on runtime environment (test case on 32/64bit, native test is only 32bit gcc).
Unfortunately all test case disabled when IntPtr
was used... I wrote this case:
// IL2C.BasicTypes.System_IntPtr
[TestCase(4, "SizeOf", Assert = TestCaseAsserts.IgnoreValidateInvokeResult)] // Unit test environment is unknown, gcc is 32bit
IgnoreValidateInvokeResult
flag will ignore regression test execution on CLR. Then test case result value can make from 32bit value.
@@ -310,4 +310,46 @@ internal sealed class ShiftLeftConverter : ShiftConverter | |||
|
|||
public override ShiftDirection Direction => ShiftDirection.Left; | |||
} | |||
|
|||
internal sealed class ShiftRightUnConverter : ShiftConverter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cyborgyn Ouch, I forgot signed integer flag :)
shr
opcode will understand signed bit on MSB:
shr 10000010
result: 11000001
It case, we can use C language shift operator with signed integer type (int32_t, int64_t and intptr_t).
But shr.un
opcode:
shr.un 10000010
result: 01000001
Strictly ignores MSB bit and insert zero. ECMA-335 III.3.60 shr.un - shift integer right, unsigned
section:
shr.un inserts a zero bit on each shift.
On IL2C side, The C language expression is having to write temporary casting unsigned integers explicitly, like:
stack0 = (int32_t)(((uint32_t)stack0) >> shiftAmount);
We need regression test for shr.un (and shr) converter, require these topics:
- Positive and negative value makes valid shift result.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did the temporary casting, if you look closely, and the tests are also walking through all of those tricky cases. One thing I forget, is negative shift (I never tried it, never even thought about it on any platform/language before). :) For the shift left, no magic happens, this is why it is omitted there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I found it on branch latest commit. (I took and read commit from bottom of branch...)
I approved it!
Bit off topic: The CLR (.NET Framework CLR and Core CLR) behavior is funny. Because shr
and shr.un
opcode behavior isn't defined with any int32
, int64
and IntPtr
on ECMA-335.
The shr.un instruction shifts value (int32, int 64 or native int) right by the number of bits
specified by shiftAmount.
But real regression test on CLR is passed...
public override ShiftDirection Direction => ShiftDirection.Right; | ||
} | ||
|
||
internal sealed class NegConverter : InlineNoneConverter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cyborgyn The 'neg' opcode calculus is serious with floating point type...
ECMA-335 III.3.50 neg - negate
:
Negation of integral values is standard twos-complement negation. In particular, negating the
most negative number (which does not have a positive counterpart) yields the most negative
number. To detect this overflow use the sub.ovf instruction instead (i.e., subtract from 0).
Negating a floating-point number cannot overflow; negating NaN returns NaN.
If we write the same expression in C language, I feel that the result may not be the same between C and IL (C#). Also, the current IL2C doesn't consider NaN
value at all, so it seems that it's time to think about it....
At this point, I think it's okay to include TODO comments and write with comment out regression test case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wrote ckfinite(float/double) opcode implementation in C to conform with these tests https://github.com/dotnet/runtime/blob/f9a2449/src/tests/JIT/IL_Conformance/Old/Base/ckfinite.il#L52.
Here is the C version: https://godbolt.org/z/vxMYrvb8d passing all conformance tests. coreclr uses the same approach by checking if all exponent bits are 1 to find out if float/double number is infinite or NaN, and emits ArithmeticException.
Was looking at the place to implement it in IL2C. Note: in this case, we need the raw single/double number in binary storage variable (e.g. uint32_t and uint64_t respectively) as it is represented in IL (IEEE 754 floating-point); without converting it to C float where it may or may not conform to IEEE 754 fp (ISO C99 is relaxed on the format...).
Feel free to implement ckfinite too, as this one is pretty simple. Otherwise, I'll take a stab at it; once this PR is merged. 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good news! I will include NaN handler and ckfinite implementation when after merged this PR. (Memoized: #112)
…itly implemented interface method. #104 #104 (review)
|
||
namespace IL2C.ILConverters | ||
{ | ||
// Arithmetic Shift right (with sign) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you inserted details!
For a few days, unfortunately I have no spare time, but will be back on the topic as soon as I can. Thank you for your efforts so far! |
No problem, keep your pace! |
For #91, investigating nanoFramework BCL (mscorlib) compilation, implement all missing opcodes to be able to compile.
Solve #93 by implementing switch IL opcode
Solve #105 by adding stop condition, and handling static constructors