Imaginary cxx api #1

nlewycky · 2020-12-21T18:50:05Z

No description provided.

MarkMcCaskey

Looks good to me overall! I didn't fully review changes in wasm.hh and wasm-v8.cc, but it generally seems like it makes the API much more natural in C++

MarkMcCaskey · 2020-12-21T18:51:39Z

example/callback.cc

-    } break;
-  }
+auto operator<<(std::ostream &out, const wasm::Val &val) -> std::ostream& {
+  std::visit([](auto &&arg) { std::cout << arg; });


Does this do the same thing? 🤔

Why is it going to std::cout instead of out?

That's a bug, thanks.

MarkMcCaskey · 2020-12-21T18:54:29Z

example/callback.cc

-    std::cout << "> Error accessing export!" << std::endl;
+  auto run_func = instance.exported_func(0);
+  if (auto error = run_func.error()) {
+    std::cout << "> Error accessing export: " << error->what() << std::endl;
    exit(1);
  }
  auto run_func = exports[0]->func();


redefinition of run_func (previously defined on line 72), is this intentional?

Nope! Bug, thanks.

MarkMcCaskey · 2020-12-21T18:56:23Z

example/finalize.c

+  intptr_t i = (intptr_t)data;
+  if (i % (iterations / 10) == 0) printf("Finalizing #%" PRIdPTR "...\n", i);


Unrelated change, maybe we want to split this off into a separate PR. I'll leave that up to you though!

Yep, unrelated change. Removed!

MarkMcCaskey · 2020-12-21T18:58:33Z

example/finalize.cc

      exit(1);
    }
-    instance->set_host_info(reinterpret_cast<void*>(i), &finalize);
+    // ?????
+    instance->set_host_info(&i, &finalize);


// ?????

Passes ownership of arbitrary data with a destructor to some thing, in this case the instance.

MarkMcCaskey · 2020-12-21T19:00:45Z

example/global.cc

-  auto set_var_i64_import = get_export_func(exports, i++);
-  auto set_var_f32_export = get_export_func(exports, i++);
-  auto set_var_i64_export = get_export_func(exports, i++);
+  auto const_f32_export = instance->exported_global(i++);


Is it ever possible to have an exports without an Instance?

No, should it be? I don't think so, but if we should then what would create one?

MarkMcCaskey · 2020-12-21T19:13:13Z

example/imaginary-wasm.hh

+};
+struct EngineConfig {
+  EngineConfig();
+  // Implementation provides extra c'tors here.


How does this happen? By editing this file and shipping their own version of it?

MarkMcCaskey · 2020-12-21T19:21:33Z

example/imaginary-wasm.hh

+  Module compile(const std::vector<char> &);
+  Instance instantiate(const std::vector<char> &);


You know better than I do, but is char the normal way that vectors of bytes are expressed in C++? I know it's correct, just surprised there's not a fancier new type that is commonly used

It is! There is also a newer fancier type named "std::byte" which is absolutely not the right thing to use here. It's safe to say that files in the C++ sense are contiguous chars (or unsigned chars).

MarkMcCaskey · 2020-12-21T19:22:54Z

example/imaginary-wasm.hh

+  Global &operator=(float);
+  Global &operator=(double);
+  Global &operator=(int32_t);
+  Global &operator=(int64_t);


Fancy, but how will this handle 128bit numbers and other value types like references? Just make a new type for those things?

Just declare another operator= here, yes.

C++ doesn't have a native 128-bit integer type (well, rather, real compilers for C++ don't uniformly offer 128-bits, the spec specifies intX_t is X bits and lets the implementation pick which to provide). Probably we'll have to define a struct wasm::int128 { uint64_t data[2]; };

MarkMcCaskey · 2020-12-21T19:33:19Z

example/imaginary-wasm.hh

+};
+
+struct RefVal {
+  void *ptr = nullptr;


Curious what your thoughts on this vs Optional<&> or whatever it is in C++

I'm not sure what to do about RefVal until we implement refs. This guarantees that every RefVal object will contain at least one pointer (worth of data) but says nothing about what the pointer is for or what it points to, leaving that up to the implementation choice of the implementer.

I'm not sure why I'd use std::optional here? It could be std::optional<something&> but what's the something?

MarkMcCaskey · 2020-12-21T19:36:01Z

example/memory.cc


  // Grow memory.
  std::cout << "Growing memory..." << std::endl;
  check(memory->grow(1), true);
  check(memory->size(), 3u);
-  check(memory->data_size(), 0x30000u);
+  check(memory->contents()->size(), 0x30000u);


Hmm, what's the idea here? What is contents, is it type that can be used on its own?

Ideally it'd be a std::span<char> but I realized that std::span didn't make it into c++17.

Thinking about this over the weekend, I now think we should provide a span-compatible type for C++17 users but under a macro that uses just makes our wasm::span = std::span when the user has std::span.

Reference: https://en.cppreference.com/w/cpp/container/span

jubianchi · 2020-12-21T20:39:50Z

example/callback.cc

-  return nullptr;
+int32_t print_callback(int32_t v) {
+  std::cout << "Calling back..." << std::endl << "> " << v << std::endl;
+  return v;


This will prevent returning multi-values I think.

I was thinking multi-value returns would look like this:

std::tuple<int32_t, float> my_callback() { return {1, 2.0}; }

which when called from normal C++ would look like:

void foo() { auto [x, y] = my_callback(); }

Ideally we want that same syntax when C++ code calls a multi-value return wasm function.

jubianchi · 2020-12-21T20:56:32Z

example/imaginary-wasm.hh

+  template<typename Fn>
+  Func exported_func(const std::string &) const;
+  size_t exported_func_size() const;
+  bool exported_func_empty() const;


I'm afraid this will lead to having many methods here:

export_func*

export_global*

export_table*

export_mem*

DO they really belong to Instance or should we keep the exports?

Maybe? I'm not convinced that exports makes sense as a standalone object? What does it mean to have an exports but not an instance? (BTW, take a look at https://llvm.org/doxygen/classllvm_1_1Module.html sections "global variable iteration", "function iteration", etc. I was thinking we would do the same thing.)

That said, ExportedFunc has to exist even though it's not really separate from an instance, but we know why we want it to be different, we want the ability to call it.

Starting with C++20 we could use ranges, and code like "for (auto func : instance.exports() | std::views::filter(wasm::Functions)) { " becomes possible. To me that's the most convincing argument for having an exports() function I've thought of.

jubianchi · 2020-12-21T20:59:09Z

example/hello.cc

+    }
+  };
+
+  wasm::Instance instance = store.instantiate(file, imports);


I think we should keep the "standard" C++ API free from convenience method and ship them through a wrapper of our own or something like that.

nlewycky and others added 5 commits October 28, 2020 07:51

Fix warning about casting from pointer to integer of different size. (W…

b7636f6

…ebAssembly#159)

Use (void) to specify “no argument” in C. (WebAssembly#160)

89e988a

Make wasm_##type##_copy take a const pointer. (WebAssembly#162)

ac9b509

Make it possible to subclass wasm C++ API classes for the implementor. (

fd09246

WebAssembly#161)

WIP. Start of a different C++ API.

ffab74e

MarkMcCaskey reviewed Dec 21, 2020

View reviewed changes

jubianchi reviewed Dec 21, 2020

View reviewed changes

nlewycky added 3 commits December 22, 2020 00:21

Use the correct ostream.

47215cb

Remove unrelated change.

2d9a8a9

Add typed functions.

fd57a2c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Imaginary cxx api #1

Imaginary cxx api #1

nlewycky commented Dec 21, 2020

MarkMcCaskey left a comment

MarkMcCaskey Dec 21, 2020

nlewycky Dec 22, 2020

MarkMcCaskey Dec 21, 2020

nlewycky Dec 22, 2020

MarkMcCaskey Dec 21, 2020

nlewycky Dec 22, 2020

MarkMcCaskey Dec 21, 2020

MarkMcCaskey Dec 21, 2020

nlewycky Dec 21, 2020

MarkMcCaskey Dec 21, 2020

MarkMcCaskey Dec 21, 2020

nlewycky Dec 21, 2020

MarkMcCaskey Dec 21, 2020

nlewycky Dec 22, 2020

MarkMcCaskey Dec 21, 2020

nlewycky Dec 22, 2020

MarkMcCaskey Dec 21, 2020

nlewycky Dec 21, 2020

jubianchi Dec 21, 2020

nlewycky Dec 21, 2020

jubianchi Dec 21, 2020

nlewycky Dec 21, 2020

jubianchi Dec 21, 2020

		intptr_t i = (intptr_t)data;
		if (i % (iterations / 10) == 0) printf("Finalizing #%" PRIdPTR "...\n", i);

		Module compile(const std::vector<char> &);
		Instance instantiate(const std::vector<char> &);

Imaginary cxx api #1

Are you sure you want to change the base?

Imaginary cxx api #1

Conversation

nlewycky commented Dec 21, 2020

MarkMcCaskey 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment