Witx parser extension in generator module #71

Jacarte · 2020-04-21T12:17:58Z

Generates Scala boilerplate from witx module definitions

parent 7f427a8 author Javier Cabrera <javierca@kth.se> 1584053392 +0100 committer Javier Cabrera <javierca@kth.se> 1587397289 +0200 Executing binary files Executing text format Adding tracing support Mappping app args to tracer options Starting parameters parsing Forcing entrypoint function name in the cli app Fixing help messages Renaming to cli Removing PublishModule mixing from cli project Listing available functions if no name is provided Listing available functions if no name is provided Update cli/src/InterpreterApp.scala Co-Authored-By: Lucas Satabin <lucas.satabin@gnieh.org> Improving code Wrapping side-effects Changing s name to main Removing test modules Remove pom settings for cil package Use blocker for readig text files Using the standard `Source` class is not safe, as it executes in the calling execution context. Switching to the fs2 interface makes it more stable and better. Fix examples with new text file reading API Removing unnecessary method in Engine.scala Removing missed printf method Reformating Fix examples with new text file reading API Reformating Fixing examples Fix code issues Sorting cli options Sorting cli options WIP Sorting cli options Successful external jar loading Starting parameters parsing Starting parameters parsing Fix fieldName repetition Fix fieldName repetition Adding WITX parser to create WASI interfaces Removing stdlib module Reformatting Adding snapshopt of WASI as package resource Loading types on demand Traversers for types and interfaces Generating boilerplate from witx Isolating the witx-parser

Jacarte · 2020-04-27T08:01:36Z

Refactoring and merging in progress

satabin

I know you are refactoring currently, but still have left some comments :)

satabin · 2020-04-27T09:13:49Z

generator/resources/witx_module.mustache

+
+
+@module
+abstract class Module[@effect F[_]](val mem: Memory[IO])(implicit F: Applicative[F])


Memory should be on the same type effect F than the module.

satabin · 2020-04-27T09:15:38Z

generator/resources/witx_module.mustache

+{
+    val name = "{{{moduleName}}}"
+
+    def tryToExecute(a: => errnoEnum.Value) = {


This method looks like it's WASI specific (errno and friends), and won't make sense outside of this specific use case. I am not sure it is the appropriate place to have this.

satabin · 2020-04-27T09:23:18Z

generator/src/swam/generator/witx/LoadTypeEmitTraverser.scala

+  override val basicTypeTraverser = {
+    case (_, t: BasicType) =>
+      t match {
+        case BasicType.u8     => s"$mem.readByte(${concatOffsets(offset, prev)}).unsafeRunSync()\n"


I am really not a fan of having unsafeRunSync in our code, because they are only there fo IO, and the library code should not assume it will be an IO, but some F having some typeclass (usually Sync). The Memory class has unsafe versions of the read/write operations. If you really need to run without effect type (only there for some internal performance reasons), use them instead of this.

satabin · 2020-04-27T09:37:08Z

generator/src/swam/generator/witx/TypesEmitTraverser.scala

+
+  override val enumTypeTraverser = {
+    case (_, t: EnumType) =>
+      s"object ${t.tpeName}Enum extends Enumeration { \n\t ${t.names


Scala standard enums are really not good. Can you use enumeratum instead? It is already a dependency of the core Swam module, so you have it here. You can have a look at how I did it in my WASI branch.

satabin · 2020-04-27T09:40:38Z

runtime/src/swam/runtime/trace/TracingMemory.scala

@@ -22,7 +22,7 @@ import cats._
 import java.nio.ByteBuffer

 /** A memory instance that traces all calls to the underlying memory instance. */
-private[runtime] class TracingMemory[F[_]](inner: Memory[F], tracer: Tracer)(implicit F: MonadError[F, Throwable])
+class TracingMemory[F[_]](val inner: Memory[F], tracer: Tracer)(implicit F: MonadError[F, Throwable])


This should stay internal. It is an implementation detail.

satabin · 2020-04-27T09:49:31Z

text/src/swam/witx/parser/ImportContext.scala

+    else // else search in path
+      System
+        .getenv("PATH")
+        .split(":")


This will raise a NPE if PATH is not defined. Prefer using the scala sys.env map with the appropriate get method, which returns an Option.

satabin · 2020-04-27T09:50:53Z

text/src/swam/witx/unresolved/Types.scala

+    extends BaseWitxType(name, size, pad)
+
+object BasicType {
+  case object u8 extends BasicType("u8", 2)


shouldn't the size be 1?

satabin · 2020-04-27T09:53:13Z

text/src/swam/witx/unresolved/Types.scala

+
+case class Pointer(tpe: BaseWitxType) extends BaseWitxType("Pointer", 4 * tpe.size) // 8 bytes
+
+class StructType(override val tpeName: String, val fields: Seq[Field])


AST classes should all be case classes.

satabin · 2020-04-27T09:56:27Z

text/src/swam/witx/unresolved/Types.scala

+/**
+  *@author Javier Cabrera-Arteaga on 2020-03-18
+  */
+abstract class BaseWitxType(val tpeName: String, val size: Int = 4, val pad: Int = 0)
+
+abstract class BasicType(name: String, override val size: Int, override val pad: Int = 0)
+    extends BaseWitxType(name, size, pad)


Also please don't use abstract classes as AST base classes if they are just interfaces, especially if this is to have default values. Avoid this, prefer:

Suggested change

/**

*@author Javier Cabrera-Arteaga on 2020-03-18

*/

abstract class BaseWitxType(val tpeName: String, val size: Int = 4, val pad: Int = 0)

abstract class BasicType(name: String, override val size: Int, override val pad: Int = 0)

extends BaseWitxType(name, size, pad)

sealed trait BaseWitxType {

val tpeName: String

val size: Int

val pad: Int

}

sealed trait BasicType extends BaseWitxType {

val name: String

}

satabin · 2020-04-27T09:57:19Z

update_wasi_interfaces.sh

@@ -0,0 +1 @@
+mill generator.run -x True -p wasi -i generator/resources/wasi_witx generator/resources/wasi_witx/wasi_snapshot_preview1.witx


Not sure this belongs to the Swam repository.

satabin · 2020-07-11T14:29:12Z

I think this one became obsolete with the WASI implementation. I will close it now.

Jacarte added 15 commits April 20, 2020 17:42

Removing side-effects in TypesParser

0addeff

Improving code

c30bf78

Moving basic types

e3ae5fe

Moving basic types as classes instead of str

31dacaa

Formatting scala text before dumping

cca1c15

Fixing ImportContext

c55c263

Adding bytes padding

17aa628

Refactoring and cleaning

d7dbd0a

Adding document for generator cli

7dad963

Typos

5502ab9

Removing unused test module in generator

dc7c53e

Merging with upstream

9a7cc97

Removing unused test module in generator

f2e7029

Removing unused test module in generator

7cc89f8

satabin mentioned this pull request Apr 27, 2020

Add WIT support in text parser #65

Open

satabin suggested changes Apr 27, 2020

View reviewed changes

satabin closed this Jul 11, 2020

Jacarte deleted the witx-parser branch August 19, 2020 08:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Witx parser extension in generator module #71

Witx parser extension in generator module #71

Jacarte commented Apr 21, 2020

Jacarte commented Apr 27, 2020

satabin left a comment

satabin Apr 27, 2020

satabin Apr 27, 2020

satabin Apr 27, 2020

satabin Apr 27, 2020

satabin Apr 27, 2020

satabin Apr 27, 2020

satabin Apr 27, 2020

satabin Apr 27, 2020

satabin Apr 27, 2020

satabin Apr 27, 2020

satabin commented Jul 11, 2020



		@module
		abstract class Module[@effect F[_]](val mem: Memory[IO])(implicit F: Applicative[F])


		case class Pointer(tpe: BaseWitxType) extends BaseWitxType("Pointer", 4 * tpe.size) // 8 bytes

		class StructType(override val tpeName: String, val fields: Seq[Field])

		@@ -0,0 +1 @@
		mill generator.run -x True -p wasi -i generator/resources/wasi_witx generator/resources/wasi_witx/wasi_snapshot_preview1.witx

Witx parser extension in generator module #71

Witx parser extension in generator module #71

Conversation

Jacarte commented Apr 21, 2020

Jacarte commented Apr 27, 2020

satabin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

satabin commented Jul 11, 2020