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

System.NotSupportedException: 'Illegal one-byte branch at position: 233. Requested branch was: 204.' #86

Open
suriyadi15 opened this issue Jan 7, 2021 · 4 comments

Comments

@suriyadi15
Copy link

I use .NET Core 2.0 and this is the code:

class Program
{
    static void Main(string[] args)
    {
        ExpressionContext context = new ExpressionContext();

        context.Imports.AddType(typeof(AggregationOperation));

        //this not work
        string processedFormula = "IF(2.1<>2.1,IF(2.1>2.1,2.1,IF(AND(2.1>2.1,2.1<=2.1),2.1,IF(AND(2.1>2.1,2.1<=2.1),2.1,2.1))),IF(2.1>2.1,2.1,IF(AND(2.1>2.1,2.1<=2.1),2.1,IF(AND(2.1>2.1,2.1<=2.1),2.1,2.1))))";

        //this work
        //string processedFormula = "IF(2<>2,IF(2>2,2,IF(AND(2>2,2<=2),2,IF(AND(2>2,2<=2),2,2))),IF(2>2,2,IF(AND(2>2,2<=2),2,IF(AND(2>2,2<=2),2,2))))";

        processedFormula = Regex.Replace(processedFormula, @"and\(", "andF(", RegexOptions.IgnoreCase);

        var e = context.CompileDynamic(processedFormula);
        object result = e.Evaluate();

        System.Console.ReadKey();
    }
}

public static class AggregationOperation
{
    public static bool andF(params bool[] conditions)
    {
        bool result = true;

        foreach (var item in conditions)
        {
            result = result && item;
        }

        return result;
    }
}

The exception throw

System.NotSupportedException
  HResult=0x80131515
  Message=Illegal one-byte branch at position: 233. Requested branch was: 204.
  Source=System.Private.CoreLib
  StackTrace:
   at System.Reflection.Emit.ILGenerator.BakeByteArray()
   at System.Reflection.Emit.DynamicResolver..ctor(DynamicILGenerator ilGenerator)
   at System.Reflection.Emit.DynamicILGenerator.GetCallableMethod(RuntimeModule module, DynamicMethod dm)
   at System.Reflection.Emit.DynamicMethod.GetMethodDescriptor()
   at System.Reflection.Emit.DynamicMethod.CreateDelegate(Type delegateType)
   at Flee.InternalTypes.Expression`1.Compile(String expression, ExpressionOptions options) in D:\Code\Flee\src\Flee.NetStandard20\InternalTypes\Expression.cs:line 100
   at Flee.InternalTypes.Expression`1..ctor(String expression, ExpressionContext context, Boolean isGeneric) in D:\Code\Flee\src\Flee.NetStandard20\InternalTypes\Expression.cs:line 49
   at Flee.PublicTypes.ExpressionContext.CompileDynamic(String expression) in D:\Code\Flee\src\Flee.NetStandard20\PublicTypes\ExpressionContext.cs:line 198
   at Flee.Console.Program.Main(String[] args) in D:\Code\Flee\test\Flee.Console\Program.cs:line 27

I just changed 2 to 2.1 and it doesn't work. How to solve this?

@suriyadi15
Copy link
Author

This problem has been solved. Change this code error code with solved code.
This problem because use IF function, and I replace it with a custom function.

@hunkydoryrepair
Copy link
Contributor

I wonder if this could be the same reason IIS is crashing for some people on 64 bit.
Your function will have worse performance as all the expressions need to be evaluated whereas the built-in only evaluates the needed expresion.

@hunkydoryrepair
Copy link
Contributor

I reproduced this on .Net Framework 4.7.2 without the AND function:
"IF(2.1<>2.1,IF(2.1>2.1,2.1,IF(2.1>2.1 AND 2.1<=2.1,2.1,IF(2.1>2.1 AND 2.1<=2.1,2.1,2.1))),IF(2.1>2.1,2.1,IF(2.1>2.1 AND 2.1<=2.1,2.1,IF(2.1>2.1 AND 2.1<=2.1,2.1,2.1))))"

I believe the problem is in BranchManager.ComputeBranches(). This is where the branch addresses are adjusted for long branches vs short branches, but there is an edge case that isn't handled. It seems to only support one depth level of branches going from a short to a long branch. So you if have a long branch that starts inside a short branch, and the extra bytes from that branch cause the short branch to become a long branch, that is handled, But if the short branch that becomes a long ALSO starts within a short branch, that 2nd level short branch also needs to be extended and may also need to become a long branch. But that adjustment is never made.

I'm working on it to see if I can find a fix.

@hunkydoryrepair
Copy link
Contributor

Turns out this is messier than I hoped. ComputeBranches is never called with more than 2 branches, so the possible problems I could see won't manifest.

However, the problem is definitely similar to what I described: nested long branches. Nested conditionals don't generate their correct length until the final time they are compiled.

In order to detect long branches, Flee generates the IL in a temporary mode, will all short branches. But as it breaks the code down it doesn't gather ALL the branches into one array and test them, it on tests at the top of the current level. The offsets are wrong because if there is a long branch inside a conditional expression (a conditional in a conditional in a conditional....) it doesn't do the adjustment for the branches until the 2nd time through, but that time the top level expects the size to be the same, but it isn't so the long branch detection fails.

I have implemented a fix, but it involves outputting NOP in the temp mode generation. This reduces the work of generating code at the lower levels over what exists in the current repo, but it could be better.

Another approach which would track all branches from the very top level, and then fix up all branches and compile only twice the entire expression. Currently, nested expressions can be compiled more than twice. However, since ComputeBranches is only suited for 2 branches, this would require a lot more work and a lot more structural changes maintain a single list of branches. (I think ComputeBranches could work if the branches were sorted in reverse order by start location)

My patch is available in this repo: https://github.com/hunkydoryrepair/Flee

I can submit a pull request unless somebody wants to invest into a better solution.

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

No branches or pull requests

2 participants