C#: Initial port of IR for C# #1600

kiridi · 2019-07-16T17:20:43Z

This is a PR which contains a port of the C++ IR library to C#. The following functionalities have been tested:

functions: parameters, calls, return statement;
statements: if, for, while, do ... while, switch (C# 6.0 syntax), try ... catch, specific jumps
(breaks, gotos);
variables: declaration, initialization, access;
classes: inheritance, polymorphism, constructos, constructor initializers (base, this);
objects: creation via constructors, object initializers, field access;
arrays: creation, element access and initializers;
expressions: basic expressions(unary, binary, ++, --), casts (as, implicit casts), is;

This PR should be mostly reviewed as a whole, since recent commits contain more comments and tidy ups.

Tests have been added for each implemented feature (can be found in ../library-tests/ir/ir).
The generated instructions can be found in the file raw_ir.expected.
The generated instructions pass the sanity checks found is IRSanity.ql (results in the file raw_ir_sanity.expected).

ghost · 2019-07-16T17:20:52Z

All committers have signed the CLA.

dave-bartolomeo · 2019-07-18T20:43:10Z

I'd suggest deleting the ir/implementation/*ssa directories for now. They aren't required to build the "raw" stage of the IR, and they'll require additional work to port. For now, they're mostly just files that don't compile that clutter up the PR.

dave-bartolomeo

It looks like most of your changes to the shared files should be covered by the new compatibility layer. I'll attempt to apply by compatibility layer changes to your branch and see if I can back out your changes to those files.

dave-bartolomeo · 2019-07-18T20:48:26Z

csharp/ql/src/semmle/code/csharp/ir/implementation/raw/IRFunction.qll

+  /**
+   * Gets the function whose IR is represented.
+   */
+  final Callable getCallable() {


In all the places where you replaced the type Function with Callable, you also changed the name of the predicate to use Callable. If the predicate is to be shared between languages, it needs to have the same name. I'd recommend just changing the predicate names back to use Function for now. It's possible that some of the predicates that use the Function type don't actually need to be in the shared part of the IR, but I'd rather try to figure out how to factor them out later.

dave-bartolomeo · 2019-07-18T20:55:05Z

csharp/ql/src/semmle/code/csharp/ir/implementation/ValueCategory.qll

@@ -0,0 +1,21 @@
+import csharp


I don't think this file is even used anywhere. You should be able to just delete it. I'll delete it on the C++ side.

dave-bartolomeo · 2019-07-22T17:17:48Z

csharp/ql/src/semmle/code/csharp/ir/dataflow/DataFlow.qll

@@ -0,0 +1,23 @@
+/**


I'd recommend removing the DataFlow files from this PR as well. DataFlow isn't really interesting until you can build SSA, and SSA was already removed from this PR.

dave-bartolomeo

Just a few comments, most of which have auto-fixes that you can just click on in GitHub to automatically push to your branch.

Other than those simple changes, I'm happy with this PR as an initial working implementation.

Somebody from @Semmle/cs needs to review the changes to Type.qll, since they affect things beyond the IR.

csharp/ql/src/semmle/code/csharp/ir/IRSanity.ql

csharp/ql/src/semmle/code/csharp/ir/PrintIR.ql

dave-bartolomeo · 2019-08-05T08:16:31Z

csharp/ql/src/semmle/code/csharp/ir/Util.qll

+
+import csharp
+
+class Locatable extends Element { }


I'm not sure this is needed after my changes. You can just make Language::AST resolve to Element.

csharp/ql/src/semmle/code/csharp/ir/implementation/raw/IRSanity.ql

csharp/ql/src/semmle/code/csharp/ir/implementation/raw/PrintIR.ql

calumgrant

A partial review. My main observation is that there is still quite a lot of code that could be shared better. I suggest making a list of the changes that are required, and forwarding them to @dave-bartolomeo

config/identical-files.json

calumgrant · 2019-08-05T08:35:15Z

csharp/ql/src/semmle/code/csharp/ir/IR.qll

@@ -0,0 +1,4 @@
+// Most queries should operate on the aliased SSA IR, so that's what we expose


Documentation-style comment?

calumgrant · 2019-08-05T08:41:47Z

csharp/ql/src/semmle/code/csharp/ir/IRConfiguration.qll

@@ -0,0 +1,19 @@
+import csharp


File-level documentation comment is needed.

csharp/ql/src/semmle/code/csharp/ir/PrintIR.ql

csharp/ql/src/semmle/code/csharp/ir/IRConfiguration.qll

calumgrant · 2019-08-05T11:05:44Z

csharp/ql/src/semmle/code/csharp/ir/implementation/raw/internal/TranslatedCondition.qll

+  }
+
+  override Instruction getChildFalseSuccessor(TranslatedCondition child) {
+    (child = getLeftOperand() or child = getRightOperand()) and


Is there getAnOperand()? If not it might be quite good to add.

calumgrant · 2019-08-05T11:06:23Z

csharp/ql/src/semmle/code/csharp/ir/implementation/raw/internal/TranslatedCondition.qll

+  override LogicalOrExpr expr;
+
+  override Instruction getChildTrueSuccessor(TranslatedCondition child) {
+    (child = getLeftOperand() or child = getRightOperand()) and


getAnOperand()

calumgrant · 2019-08-05T11:07:03Z

csharp/ql/src/semmle/code/csharp/ir/implementation/raw/internal/TranslatedCondition.qll

+      Type resultType, boolean isLValue) {
+    tag = ValueConditionConditionalBranchTag() and
+    opcode instanceof Opcode::ConditionalBranch and
+    resultType instanceof VoidType and


We should use Language::UnknownType instead of VoidType here (and elsewhere).

Fixed in all the places where I used ObjectType.

(Language::VoidType is for instructions that don't return a result at all, including ConditionalBranch. Language::UnknownType is for instructions that return a result, but we don't know or can't represent its type)

calumgrant · 2019-08-05T11:07:21Z

csharp/ql/src/semmle/code/csharp/ir/implementation/raw/internal/TranslatedDeclarationEntry.qll

+      Type resultType, boolean isLValue) {
+    opcode instanceof Opcode::NoOp and
+    tag = OnlyInstructionTag() and
+    resultType instanceof VoidType and


Language::UnknownType

calumgrant · 2019-08-05T11:26:01Z

csharp/ql/src/semmle/code/csharp/ir/internal/IRCSharpLanguage.qll

+
+class Type = CSharp::Type;
+//REVIEW: This might not exist in the database. 
+class UnknownType = CSharp::UnknownType;


Use NullType.

csharp/ql/src/semmle/code/csharp/ir/implementation/raw/internal/InstructionTag.qll

csharp/ql/src/semmle/code/csharp/ir/implementation/raw/internal/TranslatedCall.qll

dave-bartolomeo · 2019-08-07T19:09:25Z

csharp/ql/src/semmle/code/csharp/ir/implementation/raw/internal/TranslatedCondition.qll

+      Type resultType, boolean isLValue) {
+    tag = ValueConditionConditionalBranchTag() and
+    opcode instanceof Opcode::ConditionalBranch and
+    resultType instanceof VoidType and


(Language::VoidType is for instructions that don't return a result at all, including ConditionalBranch. Language::UnknownType is for instructions that return a result, but we don't know or can't represent its type)

csharp/ql/src/semmle/code/csharp/ir/implementation/raw/internal/TranslatedDeclarationEntry.qll

dave-bartolomeo

All of my feedback has been addressed, and the C++ changes LGTM.

calumgrant

This looks like a really great start, and you have already covered many parts of the language! I have not combed the generated IR for correctness, or validated every single edge. Some of the feedback can be addressed in follow-up PRs.

csharp/ql/src/semmle/code/csharp/ir/Util.qll

csharp/ql/src/semmle/code/csharp/ir/implementation/Opcode.qll

calumgrant · 2019-08-15T16:12:46Z

csharp/ql/src/semmle/code/csharp/ir/implementation/raw/internal/TranslatedCall.qll

+
+  override Type getCallResultType() { result instanceof VoidType }
+
+  override predicate hasQualifier() { any() }


What about static constructors?

I am not quite sure when those should be called, since they do not have an exact entry point. From what I've read, they are called some time before the first access of the static field (but it is not exactly known when), though if they are not accessed the static constructor is never called.

A related question is when or how field initialization should be shown.

csharp/ql/src/semmle/code/csharp/ir/internal/IRCSharpLanguage.qll

calumgrant · 2019-08-16T11:59:47Z

csharp/ql/src/semmle/code/csharp/ir/implementation/raw/internal/TranslatedExpr.qll

+// * The IR translation of an object creation expression.
+// * Eg. Point a = new Point(1, 2);
+// */
+//class TranslatedObjectCreationExpr extends TranslatedNonConstantExpr,


Is this getting reinstated or has it been moved already?

You should consider splitting this file up further, for example in a translated/ folder.

ObjectCreation is dealt with in the TranslatedInitialization.qll file, removed the comments.
I was thinking of refactoring the file to better reflect the structure of the exprs folder in the C# library.

calumgrant · 2019-08-16T12:15:52Z

csharp/ql/src/semmle/code/csharp/ir/implementation/raw/internal/TranslatedInitialization.qll

+    result = TranslatedElementInitialization.super.getInstructionConstantValue(tag)
+    or
+    tag = getElementDefaultValueTag() and
+    result = getZeroValue(getElementType())


What about null elements?

The TranslatedElementValueInitialization will not be used (I will probably delete it when I will refactor the whole TranslatedInitialization.qll file), since when an array is created and initialized we have 2 possibilities: either the size is specified, in which case the initializer list must be that size, or the size is not specified and we initialize as many elements as the initializer list has. Should I refactor this file for this PR?

csharp/ql/src/semmle/code/csharp/ir/implementation/raw/internal/TranslatedStmt.qll

calumgrant · 2019-08-16T12:30:41Z

csharp/ql/src/semmle/code/csharp/ir/implementation/raw/internal/TranslatedStmt.qll

+  }
+}
+
+class TranslatedSwitchStmt extends TranslatedStmt {


Arguably, we could model this as a chain of ifs (unlike C++), so you could synthesise something like

switch (x) { case 1: case "foo: ... break; case 2: ... }

as

tmp = x; if (tmp == 1 || tmp == "foo") ... else if (tmp == 2) ...

Also have when conditions to consider.

Should we do this for this PR? I believe this is a good idea, since treating when and pattern matching would be easier inside conditions.

Do it later.

calumgrant · 2019-08-20T15:09:17Z

csharp/ql/test/library-tests/ir/ir/prop.cs

@@ -0,0 +1,31 @@
+class Geeks {


Misplaced {

calumgrant

Looking good. One or two small things, let me know when you're ready to merge this.

Ported basic functionalities from the C++ IR Added a simple test that passes the IR sanity check and produces sensible IR (together with the .expected files) to the C# test folder

Refactored the C++ specific names Tidied the code Updated TODOs

Objects now work, although I will refactor the code quite a bit for clarity If and while statements now produce good code Began work on try statements

Introduced 2 new tags to support multidimensional arrays Multidimensional arrays produce correct code All types of initializations for arrays work correctly

Fixed a bug in TranslatedExpr: decl + init where the rhs is a reference now work as expected Uncommented the code for the switch statement

Correct code is now generated from ObjectCreation exprs and ObjectInitializer exprs. Removed TranslatedFieldInitialization and its subclasses and further refactored TranslatedInitialization

Added tests to showcase the instructions generated for object creation and object initialization Updated raw_ir.expected PrintIR now uses the qualified name (with types) when printing the IR for more clarity

Throw statements now give correct code, apart from the case of rethrows: need to make explicit the fact that a finally block is executed even if stack unwinding happens. Added 2 new classes to TranslatedStmt.qll, one for throws that have an exception, one for rethrows. Fixed a bug in TranslatedDeclarationEntry.qll where some local declaration would be missed. Changed toString into getQualifiedName for more clarity when generating the instructions in Instruction.qll. Some general refactoring in TranslatedExpr.qll and TranslatedStmt.qll.

Correct code is now produced for increment and decrement expressions Modified producesExprResult() and TTranslatedLoad() so that no loads are done from outside the crement exprs and that the VariableAddress generated from the access of the operator variable is recognized as an expr that produces result.

Some preliminary refactoring of the TranslatedDeclaration.qll file

Added support for `ForStmt` and `DoWhileStmt` Added test cases

Broke down the class `TranslatedJump` to have more control on the IR control flow. Now GotoLabelStmt, GotoCaseStmt, GotoDefaultStmt and BreakStmt are translated separately. This also fixes an issue when having a switch as the last statement of a void function would create an incorrect CFG.

Properties and property access produce correct code. Fixed a function qualifier bug in `TranslatedCall.qll`. Added a new class to translate `ExprStmt`s whose expr is an `AssignExpr` whose lvalue is an accessor call: we translate only the accessor call in for the translated AST.

Began working o inheritance, polymorphism and constructor init. Correct code is produced for them (though some more work is needed to accurately treat conversions between classes). Removed commented code. Added classes to properly deal with constructor init and modified and refactored TranslatedFunction to accomodate for the changes.

Fixed a bug in `TranslatedArrayExpr` that would prevent the element to produce the correct instruction result, hence creating problems with loads and stores. `ElementsAddress` opcode now inherits from the `UnaryOpcode`, as it should.

Fixed some inconsistencies with casts Fixed some bugs related to which translated elements need loads Added support for IsExpr expressions

Tidied up the code for review Added more comments

Synced the files that are needed for this PR

Added some new classes for built in operations that for the moment have no effect (added to remove errors)

calumgrant · 2019-08-28T16:02:11Z

csharp/ql/src/semmle/code/csharp/ir/implementation/raw/internal/InstructionTag.qll

-private predicate fieldIsInitialized(Field field) {
-  exists(field.getInitializer())
-}
+//private predicate fieldIsInitialized(Field field) {


Might as well delete this properly.

j504 · 2020-11-18T21:00:16Z

GGreart`

kiridi requested a review from a team as a code owner July 16, 2019 17:20

calumgrant requested review from dave-bartolomeo and rdmarsh2 July 16, 2019 17:21

kiridi force-pushed the ircsharp branch 4 times, most recently from 697556c to 8a0c3cb Compare July 16, 2019 17:42

dave-bartolomeo mentioned this pull request Jul 18, 2019

C++: Start preparing IR for supporting multiple languages #1607

Merged

dave-bartolomeo reviewed Jul 18, 2019

View reviewed changes

dave-bartolomeo reviewed Jul 22, 2019

View reviewed changes

kiridi force-pushed the ircsharp branch from a3e6636 to cd7d779 Compare July 31, 2019 12:17

kiridi requested a review from a team as a code owner August 2, 2019 08:43

dave-bartolomeo reviewed Aug 5, 2019

View reviewed changes

calumgrant suggested changes Aug 5, 2019

View reviewed changes

dave-bartolomeo suggested changes Aug 7, 2019

View reviewed changes

dave-bartolomeo previously approved these changes Aug 14, 2019

View reviewed changes

rdmarsh2 mentioned this pull request Aug 14, 2019

C++: side effect IR instructions for pointer arguments #1735

Merged

calumgrant suggested changes Aug 16, 2019

View reviewed changes

kiridi dismissed dave-bartolomeo’s stale review via cac2ce9 August 20, 2019 12:16

calumgrant reviewed Aug 20, 2019

View reviewed changes

csharp/ql/test/library-tests/ir/ir/prop.cs Outdated

@@ -0,0 +1,31 @@

class Geeks {

Copy link

Contributor

calumgrant Aug 20, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Misplaced {

calumgrant reviewed Aug 20, 2019

View reviewed changes

calumgrant previously approved these changes Aug 22, 2019

View reviewed changes

kiridi dismissed calumgrant’s stale review via 048482c August 28, 2019 08:30

kiridi force-pushed the ircsharp branch 2 times, most recently from 32e3a14 to 65e7cef Compare August 28, 2019 11:14

Andrei Diaconu added 3 commits August 28, 2019 12:25

Initial copy of C++ IR with some modifications

fc69c12

Functional basic porting

c733bc0

Ported basic functionalities from the C++ IR Added a simple test that passes the IR sanity check and produces sensible IR (together with the .expected files) to the C# test folder

General tidy up and refactor

025d68f

Refactored the C++ specific names Tidied the code Updated TODOs

kiridi and others added 22 commits August 28, 2019 12:25

Applied the review comments

4977763

Synced more files

de6f547

Added support for switch stmt (CS 6.0 style)

4462bab

Initial work for objects and statements

35b028e

Objects now work, although I will refactor the code quite a bit for clarity If and while statements now produce good code Began work on try statements

Fixed and refactored code for arrays

be01b03

Introduced 2 new tags to support multidimensional arrays Multidimensional arrays produce correct code All types of initializations for arrays work correctly

Fixed bugs, updated tests

dae37e5

Fixed a bug in TranslatedExpr: decl + init where the rhs is a reference now work as expected Uncommented the code for the switch statement

Object creation and initialization + refactoring

8661074

Correct code is now generated from ObjectCreation exprs and ObjectInitializer exprs. Removed TranslatedFieldInitialization and its subclasses and further refactored TranslatedInitialization

Objects tests and fix in PrintIR

b90bc96

Added tests to showcase the instructions generated for object creation and object initialization Updated raw_ir.expected PrintIR now uses the qualified name (with types) when printing the IR for more clarity

Updated expected for crement ops

1e4b3fa

Preliminary refactoring

b6287b9

Some preliminary refactoring of the TranslatedDeclaration.qll file

Added stmts

2724075

Added support for `ForStmt` and `DoWhileStmt` Added test cases

Fixed sanity checks

34bafa7

Fixed a bug in `TranslatedArrayExpr` that would prevent the element to produce the correct instruction result, hence creating problems with loads and stores. `ElementsAddress` opcode now inherits from the `UnaryOpcode`, as it should.

Added cast exprs + deleted commented code

c8a3f6f

Casts and IsExpr

0c6ffc9

Fixed some inconsistencies with casts Fixed some bugs related to which translated elements need loads Added support for IsExpr expressions

Tidy up + more comment

7390606

Tidied up the code for review Added more comments

Synced files

c74898e

Synced the files that are needed for this PR

Fixed PR errors

66948b4

kiridi force-pushed the ircsharp branch from 65e7cef to 66948b4 Compare August 28, 2019 11:28

kiridi added 3 commits August 28, 2019 12:30

Synced files

05ae04d

Fixed errors

489dbe1

Added some new classes for built in operations that for the moment have no effect (added to remove errors)

Fixed unused predicate error

deeff07

calumgrant reviewed Aug 28, 2019

View reviewed changes

calumgrant approved these changes Aug 29, 2019

View reviewed changes

calumgrant merged commit d2bee79 into github:master Aug 29, 2019

		@@ -0,0 +1,4 @@
		// Most queries should operate on the aliased SSA IR, so that's what we expose


		override Type getCallResultType() { result instanceof VoidType }

		override predicate hasQualifier() { any() }

C#: Initial port of IR for C# #1600

C#: Initial port of IR for C# #1600

Uh oh!

Conversation

kiridi commented Jul 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Jul 16, 2019 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dave-bartolomeo commented Jul 18, 2019

Uh oh!

dave-bartolomeo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dave-bartolomeo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

calumgrant left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dave-bartolomeo left a comment

Choose a reason for hiding this comment

Uh oh!

calumgrant left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kiridi commented Jul 16, 2019 •

edited

Loading

ghost commented Jul 16, 2019 •

edited by ghost

Loading