CosmosOS / Cosmos

Cosmos is an operating system "construction kit". Build your own OS using managed languages such as C#, VB.NET, and more!
https://www.goCosmos.org
BSD 3-Clause "New" or "Revised" License
2.91k stars 549 forks source link

Possible RAT Bug #476

Closed jp2masa closed 7 years ago

jp2masa commented 7 years ago

BitConverter.ToString() is not working properly, the last bytes converted to string look right but the strings converted from bytes before get modified and look strange.

For example, in the test code above, commenting lines 32 and 39 will cause variable "e" to be the only one looking right, while commenting only line 39 will result on variable "f" looking right, but it doesn't get printed. This allows me to say the problem is not on Console.WriteLine, but somewhere in BitConverter (which I'm not sure if it's plugged because I couldn't find a plug for it).

Test Code: http://pastebin.com/hrAEwXG3

Also, on AppVeyor a Path test is failing, and I suspect there's some problem with strings implemention, but I'm not sure what it can be or if it exists.

fanoI commented 7 years ago

No BitConverter is not plugged so it could be a problem on how arguments are passed to functions. In BCLTest should be a BitConverterTest class: https://github.com/CosmosOS/Cosmos/blob/master/Tests/Cosmos.Compiler.Tests.Bcl/System/BitConverterTest.cs are these tests passing?

jp2masa commented 7 years ago

The tests are passing but they show the bug in the output

fanoI commented 7 years ago

Then I've not understand the bug is the text: "BitConverter.ToString(floatBytes) doesn't work: result ..." that is truncated? Or the already written lines become garbage? Another thing what version of TestRunner are you using the text mode one or the GUI version?

jp2masa commented 7 years ago

No, the bug is the strange chars, in TestRunner it is something like \u0000 unicode chars and if you run the test code I've uploaded in Bochs it draws strange symbols. What I noticed is that commenting BitConverter lines causes the last BitConverter line to be the only one producing correct results.

jp2masa commented 7 years ago

I tested this bug better and what I can say is that the problem is when BitConverter.ToString() value is assigned to a variable. The value of the previous assigned variable gets changed. I'm almost sure that's what's happening. I don't know if IL op codes to assign values to variables need to be updated, or have some king of bug. It was for sure after RAT merge so I think it's an IL op code bug.

jp2masa commented 7 years ago

Other test:

string a = new string(new[]{ '0', 'A', 'F', '5', '-' });
string b = new string(new[]{ 'Z', '-', '3', 'C', '-' });

Console.WriteLine(a);
Console.WriteLine(b);

The first 2 chars in each string don't get printed.

Project-Magenta commented 7 years ago

faces?

jp2masa commented 7 years ago

The strange symbols? Yeah, they look like faces and on the TestRunner instead of the faces, some unicode numbers appear

Project-Magenta commented 7 years ago

Oh.

fanoI commented 7 years ago

Thanks to report the bug the strange thing is that it only in "rappresentation" that the String appears screwed the value (as bytes) is correct and indeed the assertions succeed.

I've talked of this issue with @charlesbetros and the idea is to remand the solution of string related issues as this after Net Core branch is merged in which we could use the Microsoft code directly (a lot of our plugs will become obsolete so makes no sense to solve their bugs).

jp2masa commented 7 years ago

For sure .net core will fix many problems but it won't fix IL op codes and i fear the issue is with some kind of char arrays bug in IL compiler

fanoI commented 7 years ago

I concord that probably the bug is caused by the RAT merge... the sad thing is it is so particular that AppVoyer said "all green" but if one would have looked to the text it was garbled so we don't have seen this.

charlesbetros commented 7 years ago

I saw it after the merge, but I gave it a lower priority for now.

On Sep 20, 2016, at 9:32 AM, fanoI notifications@github.com wrote:

I concord that probably the bug is caused by the RAT merge... the sad thing is it is so particular that AppVoyer said "all green" but if one would have looked to the text it was garbled so we don't have seen this.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.