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

Method calls on nullable value types prevent inlining #1251

Closed
CreateAndInject opened this issue Aug 10, 2018 · 6 comments · Fixed by #1253
Closed

Method calls on nullable value types prevent inlining #1251

CreateAndInject opened this issue Aug 10, 2018 · 6 comments · Fixed by #1253

Comments

@CreateAndInject
Copy link
Contributor

Web.zip

Web.Areas.Admin.Controllers.CostReceController.Sexport
Web.Areas.Admin.Controllers.CostReceController.ExpReportList

@siegfriedpammer
Copy link
Member

The problem are not the expression trees, but inlining of method calls on value types. In this case ToString() on decimal?.

See this example:

CostReceYing4 costReceYing13 = costReceYing;
List<CostReceYing2> list4 = list3;
CostReceYing2 obj2 = new CostReceYing2
{
	合同编号 = "合计",
	评估费 = costReceYing13.评估费,
	应收代理费 = costReceYing13.应收代理费
};
num = costReceYing13.实收代理费;
obj2.实收代理费 = num.ToString();
num = costReceYing13.实收评估费;
obj2.实收评估费 = num.ToString();
num = costReceYing13.手续费;
obj2.手续费 = num.ToString();
num = costReceYing13.实收手续费;
obj2.实收手续费 = num.ToString();
list4.Add(obj2);

if num were inlined correctly, then obj2 would form a nice object initializer and then everything else would be collapsed into expressions nicely, making it possible for the ExpressionTree transform do it's thing.

@siegfriedpammer siegfriedpammer changed the title Commit 4fab913 doesn't fix 'Expression Trees not detected' completely Method calls on nullable value types prevent inlining Aug 10, 2018
@siegfriedpammer
Copy link
Member

@CreateAndInject Just to explain a bit more, if you're interested: A significant part of the transforms in the decompiler are "statement transforms", which analyze the statements in each block from the last statement to the first.

@siegfriedpammer
Copy link
Member

Another problem (inlining again):

parameterExpression = Expression.Parameter(typeof(CostReceYing4), "a");
MemberExpression left8 = Expression.Property(parameterExpression, (MethodInfo)MethodBase.GetMethodFromHandle((RuntimeMethodHandle)/*OpCode not supported: LdMemberToken*/));
num = null;
costReceYing12.实收手续费 = source10.Sum(Expression.Lambda<Func<CostReceYing4, decimal?>>(Expression.Condition(Expression.Equal(left8, Expression.Constant(num, typeof(decimal?))), Expression.Convert(Expression.Convert(Expression.Constant(0, typeof(int)), typeof(decimal), (MethodInfo)MethodBase.GetMethodFromHandle((RuntimeMethodHandle)/*OpCode not supported: LdMemberToken*/)), typeof(decimal?)), Expression.Property(parameterExpression, (MethodInfo)MethodBase.GetMethodFromHandle((RuntimeMethodHandle)/*OpCode not supported: LdMemberToken*/))), new ParameterExpression[1]
{
	parameterExpression
}));

In this case I think, num's value should be copy-propagated into the expression tree, which would enable us to detect it properly.

@dgrunwald
Copy link
Member

If num is a value-type, it should be inlined into the .ToString() calls. Maybe there's a problem with variable splitting, so that the decompiler doesn't see the different num uses as independent?

For the second code snippet, inlining could handle that case, but will only do so if num is a stack slot, or if the aggressive flag is turned on. Is the C# compiler really generating a local for the temporary, even though it never needs to take the address of that local?
I guess we could make inlining aggressive in code that looks like it's constructing expression trees, but that feels like an ugly hack.

@siegfriedpammer
Copy link
Member

Yes, it seems variable splitting does not work as expected on num.

The num variable in the first and second snippet are both the same. First there is a default(decimal?) assignment and usage in the expression tree (the second snippet) and then there is ldloca access in the first snippet (which is executed after the second).

So the problem seems to be that the compiler is reusing one local variable slot for the expression tree constants and the value-type calls.

@siegfriedpammer
Copy link
Member

Variable splitting does not work, because SplitVariables.DetermineAddressUse returns AddressUse.Unknown for the parent callvirt ToString instruction.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants