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.

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

Add support to 'long' switch statements #86

Closed
shargon opened this issue Aug 21, 2019 · 21 comments
Closed

Add support to 'long' switch statements #86

shargon opened this issue Aug 21, 2019 · 21 comments

Comments

@shargon
Copy link
Member

shargon commented Aug 21, 2019

There is a problem with the switch instruction, it seems that works if we have less entries than 6, with more, it doesn't works.

I made a pull request for test this issue at, #87

if you comment some entries, the test will work

@erikzhang
Copy link
Member

It is because the CLR uses hash table to optimize the switch segment. And there is no native support for that hash table in NeoVM.

@shargon
Copy link
Member Author

shargon commented Aug 22, 2019

There is not possible to convert this hash table into a jump table?

Pseudocode:

// With hash table
hash= [ 0x0a, 0x0b, 0x0c ] 
JMP(hash[2]);

// Without hash table

0x00 NOP
0x01 JMP 0x0a
0x02 JMP 0x0b
0x03 JMP 0x0c

JMP 0x02

@shargon
Copy link
Member Author

shargon commented Aug 22, 2019

dnSpy decompile the source as:

using System;
using Neo.SmartContract.Framework;

namespace Neo.Compiler.MSIL.TestClasses
{
	// Token: 0x02000017 RID: 23
	internal class Contract_Switch : SmartContract
	{
		// Token: 0x0600005E RID: 94 RVA: 0x00003C78 File Offset: 0x00001E78
		public static object Main(string method, object[] args)
		{
			if (method != null)
			{
				uint num = <PrivateImplementationDetails>.ComputeStringHash(method);
				if (num <= 518729469u)
				{
					if (num <= 434841374u)
					{
						if (num <= 350953279u)
						{
							if (num != 334175660u)
							{
								if (num == 350953279u)
								{
									if (method == "19")
									{
										return 20;
									}
								}
							}
							else if (method == "18")
							{
								return 19;
							}
						}
						else if (num != 401286136u)
						{
							if (num != 418063755u)
							{
								if (num == 434841374u)
								{
									if (method == "16")
									{
										return 17;
									}
								}
							}
							else if (method == "15")
							{
								return 16;
							}
						}
						else if (method == "14")
						{
							return 15;
						}
					}
					else if (num <= 468396612u)
					{
						if (num != 451618993u)
						{
							if (num == 468396612u)
							{
								if (method == "10")
								{
									return 11;
								}
							}
						}
						else if (method == "17")
						{
							return 18;
						}
					}
					else if (num != 485174231u)
					{
						if (num != 501951850u)
						{
							if (num == 518729469u)
							{
								if (method == "13")
								{
									return 14;
								}
							}
						}
						else if (method == "12")
						{
							return 13;
						}
					}
					else if (method == "11")
					{
						return 12;
					}
				}
				else if (num <= 873244444u)
				{
					if (num <= 822911587u)
					{
						if (num != 806133968u)
						{
							if (num == 822911587u)
							{
								if (method == "4")
								{
									return 5;
								}
							}
						}
						else if (method == "5")
						{
							return 6;
						}
					}
					else if (num != 839689206u)
					{
						if (num != 856466825u)
						{
							if (num == 873244444u)
							{
								if (method == "1")
								{
									return 2;
								}
							}
						}
						else if (method == "6")
						{
							return 7;
						}
					}
					else if (method == "7")
					{
						return 8;
					}
				}
				else if (num <= 923577301u)
				{
					if (num != 890022063u)
					{
						if (num != 906799682u)
						{
							if (num == 923577301u)
							{
								if (method == "2")
								{
									return 3;
								}
							}
						}
						else if (method == "3")
						{
							return 4;
						}
					}
					else if (method == "0")
					{
						return 1;
					}
				}
				else if (num != 1007465396u)
				{
					if (num != 1024243015u)
					{
						if (num == 2381486463u)
						{
							if (method == "20")
							{
								return 21;
							}
						}
					}
					else if (method == "8")
					{
						return 9;
					}
				}
				else if (method == "9")
				{
					return 10;
				}
			}
			return 99;
		}
	}
}

@erikzhang
Copy link
Member

erikzhang commented Aug 23, 2019

You need to implement <PrivateImplementationDetails>.ComputeStringHash() in NeoVM/Interop to support switch.

@shargon
Copy link
Member Author

shargon commented Aug 23, 2019

@lightszero do you think that this is feasible?

@lightszero
Copy link
Member

lightszero commented Aug 25, 2019

@shargon

as eric said. ComputeStringHash is a sad story.

swtich xxx
{
case "aa":
}

I can't got "aa" form IL in current convert solution(current solution is use Mono.cecil to conv IL -> AVM).

One way is add a ComputeStringHash on NEOVM.
Another way for add switch support need to use another convert solution(maybe mix some rosyln part).

I will working on rosyln way,but Use rosyln to got more info need input csproj file instead input dll file.It is a big change.

I had try add a stringhash way
neo-project/neo-vm#15

@erikzhang
Copy link
Member

@lightszero We can add ComputeStringHash() on Interop.

@lightszero
Copy link
Member

C# 's stringhash is different with java,so we need many stringhash interop.

@shargon
Copy link
Member Author

shargon commented Aug 26, 2019

I understand the problem ... if we receive a compilation error (user friendly), is enough to me

@shargon
Copy link
Member Author

shargon commented Aug 26, 2019

@shargon
Copy link
Member Author

shargon commented Aug 26, 2019

And is possible to generate this method on runtime, without a syscall, and just call it?

internal static uint ComputeStringHash(string s)
	{
		uint num;
		if (s != null)
		{
			num = 2166136261u;
			for (int i = 0; i < s.Length; i++)
			{
				num = ((uint)s.get_Chars(i) ^ num) * 16777619u;
			}
		}
		return num;
	}

@shargon
Copy link
Member Author

shargon commented Aug 26, 2019

@lightszero this is something crazy?

Try to detect the call, and redirect to another one loaded in the framework
shargon#2

https://github.com/shargon/neo-devpack-dotnet/pull/2/files#diff-319fbfaba4982e6265df86aadb9e928bR769-R777

@erikzhang
Copy link
Member

Calling the generated ComputeStringHash() in the runtime is too expensive.

@shargon
Copy link
Member Author

shargon commented Aug 27, 2019

I agree with you @erikzhang
Changed #87 to detect and improve the exception and finished the unit tests to make it easy for developers to discover why this does not work

@lock9
Copy link
Contributor

lock9 commented Sep 11, 2019

@shargon will #87 close this issue?
I just checked #87 and it seems Erik has agreed to implement this. I will move this to design for two days, just to make sure.

@lock9 lock9 added the design label Sep 11, 2019
@lock9 lock9 changed the title switch issue Add support to 'long' switch statements Sep 11, 2019
@lightszero
Copy link
Member

we can add some simple method for switch.
after of all,we can touch the contract c# sourcecode.
If we can't get info from IL,then we got it from sourcecode.
we can got all string in sourcecode.and save all of it.

then we got a Hash in IL,we can knwon the string.

@lightszero
Copy link
Member

lightszero commented Oct 20, 2019

no need to touch contract c# sourcecode
I study IL switch pattern,now it worked after delete hashpart。

@shargon
Copy link
Member Author

shargon commented Oct 20, 2019

Thanks @lightszero ! amazing work!

@devhawk
Copy link
Contributor

devhawk commented Dec 11, 2019

Can we port this to master-2.x?

@shargon
Copy link
Member Author

shargon commented Dec 11, 2019

Yes, I think so

@shargon
Copy link
Member Author

shargon commented Dec 11, 2019

Also, this issue should be closed and create a new one #157

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants