Implemented verification for binary comparison and branching instructions#3474
Implemented verification for binary comparison and branching instructions#3474jkotas merged 2 commits intodotnet:ILVerifyfrom Dynatrace:ILVerify-BranchComparisonInstructionsPR
Conversation
|
@mmayr-at, It will cover your contributions to all .NET Foundation-managed open source projects. |
|
@mmayr-at, thanks for signing the contribution license agreement. We will now validate the agreement and then the pull request. |
jkotas
left a comment
There was a problem hiding this comment.
Looks great, modulo a few nits
| case ILOpcode.brfalse: | ||
| case ILOpcode.brtrue: | ||
| { | ||
| StackValue value = Pop(); |
There was a problem hiding this comment.
Nit: It would be nice to be consistent about var vs. explicit types within a method.
| var value2 = Pop(); | ||
|
|
||
| CheckIsComparable(value1, value2, opcode); | ||
| break; |
There was a problem hiding this comment.
Nit: break after } to be consistent with above?
|
|
||
| CheckIsComparable(value1, value2, opcode); | ||
| break; | ||
| } |
There was a problem hiding this comment.
We often add assert as default case to make sure that there is nothing failing through the cracks:
default:
Debug.Assert(false, "Unexpected branch opcode");
break;
| // TODO: Verification rules | ||
| switch (opcode) | ||
| { | ||
| case ILOpcode.br: break; |
There was a problem hiding this comment.
Nit: It may look better with break; on new line.
|
Thanks! |
As the title says I implemented verification for binary comparison and branching instructions according to ECMA-335.
I plan to do further development of ILVerify, so that I can use it to verify rewritten IL code as described in https://github.com/dotnet/coreclr/issues/11173.