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.

Sign up for GitHub

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

Jump to bottom

Serialise IL #79

Merged

l-kent merged 4 commits into main from serialise-il

Sep 22, 2023

Contributor

ailrst commented Sep 20, 2023 •

edited

Loading

For debugging and regression detection it is good to have a text output of the intermediate language, this is a basic implementation of that, aiming to make it look close to the internal datastructures with some explanatory syntax.

E.g. for secret_write:

Procedure("1980:main", in(Parameter(Register(R0, bv64)), Parameter(Register(R1, bv64))), out(Parameter(Register(R1, bv64)), Parameter(Register(R0, bv64))), blocks(
  Block("1980:lmain",
    statements(
      LocalAssign(LocalVar(#5, bv64) := Expr(BinaryExpr(Expr(Register(R31, bv64)), Expr(18446744073709551584bv64))))
      MemoryAssign(MemoryStore(Memory(Memory(mem, 64, 8))[Expr(LocalVar(#5, bv64))] := Expr(Register(R29, bv64))))
      MemoryAssign(MemoryStore(Memory(Memory(mem, 64, 8))[Expr(BinaryExpr(Expr(LocalVar(#5, bv64)), Expr(8bv64)))] := Expr(Register(R30, bv64))))
      LocalAssign(Register(R31, bv64) := Expr(LocalVar(#5, bv64)))
      LocalAssign(Register(R29, bv64) := Expr(Register(R31, bv64)))
      LocalAssign(Register(R0, bv64) := Expr(65536bv64))
      LocalAssign(Register(R0, bv64) := Expr(MemoryLoad(Memory(Memory(mem, 64, 8)), [Expr(BinaryExpr(Expr(Register(R0, bv64)), Expr(4056bv64)))])))
      MemoryAssign(MemoryStore(Memory(Memory(mem, 64, 8))[Expr(BinaryExpr(Expr(Register(R31, bv64)), Expr(24bv64)))] := Expr(Register(R0, bv64))))
      LocalAssign(Register(R0, bv64) := Expr(0bv64))
      LocalAssign(Register(R0, bv64) := Expr(BinaryExpr(Expr(Register(R0, bv64)), Expr(2088bv64))))
      LocalAssign(Register(R1, bv64) := Expr(65536bv64))
      LocalAssign(Register(R1, bv64) := Expr(MemoryLoad(Memory(Memory(mem, 64, 8)), [Expr(BinaryExpr(Expr(Register(R1, bv64)), Expr(4072bv64)))])))
      LocalAssign(Register(R30, bv64) := Expr(2020bv64))
    ),
    jumps(
      IndirectCall(Register(R1, bv64), condition())
    )
  )
  Block("2020:l000003af",
    statements(
      LocalAssign(Register(R0, bv64) := Expr(MemoryLoad(Memory(Memory(mem, 64, 8)), [Expr(BinaryExpr(Expr(Register(R31, bv64)), Expr(24bv64)))])))
      LocalAssign(Register(R30, bv64) := Expr(2028bv64))
    ),
    jumps(
      IndirectCall(Register(R0, bv64), condition())
    )
  )
  Block("2028:l000003be",
    statements(
      LocalAssign(Register(R0, bv64) := Expr(0bv64))
      LocalAssign(Register(R29, bv64) := Expr(MemoryLoad(Memory(Memory(mem, 64, 8)), [Expr(Register(R31, bv64))])))
      LocalAssign(Register(R30, bv64) := Expr(MemoryLoad(Memory(Memory(mem, 64, 8)), [Expr(BinaryExpr(Expr(Register(R31, bv64)), Expr(8bv64)))])))
      LocalAssign(Register(R31, bv64) := Expr(BinaryExpr(Expr(Register(R31, bv64)), Expr(32bv64))))
    ),
    jumps(
      IndirectCall(Register(R30, bv64), condition())
    )
  )
)),

ailrst added 2 commits

September 20, 2023 17:02


          add basic human readable il output

c9a6dc7


          minor cleanup

e3c3573

l-kent self-requested a review

September 21, 2023 00:48

l-kent reviewed

View reviewed changes

src/main/scala/translating/ILtoIL.scala Outdated

Comment on lines 243 to 245

+                override def visitMemory(node: Memory): Memory = {
+                  program ++= "Memory("
+                  program ++= node.toString()

Contributor

l-kent Sep 21, 2023

This results in outputting Memory(Memory( since Memory's toString also includes Memory(

l-kent reviewed

View reviewed changes

src/main/scala/translating/ILtoIL.scala Outdated

Comment on lines 207 to 221

+                override def visitUnaryExpr(node: UnaryExpr): Expr = {
+                  program ++= "UnaryExpr("
+                  node.arg = visitExpr(node.arg)
+                  program ++= ")"
+                  node
+                }
+                override def visitBinaryExpr(node: BinaryExpr): Expr = {
+                  program ++= "BinaryExpr("
+                  node.arg1 = visitExpr(node.arg1)
+                  program ++= ", "
+                  node.arg2 = visitExpr(node.arg2)
+                  program ++= ")"
+                  node
+                }

Contributor

l-kent Sep 21, 2023

You should include the operators too.

l-kent reviewed

View reviewed changes

src/main/scala/translating/ILtoIL.scala Outdated

		import ir._


		private class ILSerialiser extends Visitor {

Contributor

l-kent Sep 21, 2023 •

edited

Loading

probably better to extend ReadOnlyVisitor since this isn't editing the IR? You should use that as a base instead of the regular Visitor which is designed for mutating the IR.

Contributor Author

ailrst Sep 21, 2023

Thanks 🤦

l-kent reviewed

View reviewed changes

src/main/scala/translating/ILtoIL.scala Outdated

Comment on lines 179 to 205

+                override def visitExtract(node: Extract): Expr = {
+                  program ++= "Extract("
+                  node.body = visitExpr(node.body)
+                  program ++= ")"
+                  node
+                }
+                override def visitRepeat(node: Repeat): Expr = {
+                  program ++= "Repeat("
+                  node.body = visitExpr(node.body)
+                  program ++= ")"
+                  node
+                }
+                override def visitZeroExtend(node: ZeroExtend): Expr = {
+                  program ++= "ZeroExtend("
+                  node.body = visitExpr(node.body)
+                  program ++= ")"
+                  node
+                }
+                override def visitSignExtend(node: SignExtend): Expr = {
+                  program ++= "SignExtend("
+                  node.body = visitExpr(node.body)
+                  program ++= ")"
+                  node
+                }

Contributor

l-kent Sep 21, 2023 •

edited

Loading

You should include more than just the body for all of these, and don't need to be updating node.body for anything since you aren't mutating the IR.

l-kent reviewed

View reviewed changes

src/main/scala/translating/ILtoIL.scala Outdated

Comment on lines 167 to 177

+                override def visitProgram(node: Program): Program = {
+                  for (i <- node.procedures.indices) {
+                    val updatedProcedure = visitProcedure(node.procedures(i))
+                    val targetProcedure = node.procedures(i)
+                    if (targetProcedure == node.mainProcedure) {
+                      node.mainProcedure = updatedProcedure
+                    }
+                    node.procedures(i) = updatedProcedure
+                  }
+                  node
+                }

Contributor

l-kent Sep 21, 2023

You don't need to copy in methods from the base class that haven't been changed, but using this version which is designed for mutating the IR is incorrect.

Contributor

l-kent commented Sep 21, 2023

This is good to have, but you're missing a bunch of things that weren't in the Visitor (because they're just integers etc. and don't need to be visited themselves), and you should have used ReadOnlyVisitor as a base instead, as you don't need all those node.body = visitExpr(node.body) etc. which exist in the Visitor implementation to allow for mutating the IR.


          use readonly, add operators, fix memory

cd4d74c

Contributor Author

ailrst commented Sep 21, 2023

Thanks for the review,

you're missing a bunch of things that weren't in the Visitor (because they're just integers etc. and don't need to be visited themselves),

I'm not sure what's missing, e.g. Literal just forwards to toString which should give a meaningful output for the kind of trivial expression you mention I think?

Contributor

l-kent commented Sep 21, 2023

Oh, I should have been more specific - things such as the Extract etc. parameters and the BinaryExpr operator that I highlighted.

Contributor

l-kent commented Sep 22, 2023

Looking at this again I think the main issue left is that the Expr(s are redundant - we don't need to put that around every subclass of Expr.

Contributor Author

ailrst commented Sep 22, 2023 •

edited

Loading

Looking at this again I think the main issue left is that the Expr(s are redundant - we don't need to put that around every subclass of Expr.

Yeah I wondered about that, I was thinking it might be better to include everything represented in the IL, e.g. its not automatically obvious that Register is an expression, but I decided to omit things like Literal() so leaving out Expr is probably better for readability & consistency; its a subclass so its not actually an expression wrapping the inner expression though.


          remove redundant Expr

c20c1ba

l-kent merged commit 50c80a6 into main

ailrst deleted the serialise-il branch

November 6, 2023 23:52

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

Labels

None yet