fizzy icon indicating copy to clipboard operation
fizzy copied to clipboard

Public C API

Open gumb0 opened this issue 5 years ago • 10 comments
trafficstars

  • [x] Validate #530
    • [x] returning error code/message #772
  • [x] Parse #576
    • [x] returning error code/message #772
  • [x] Instantiate #576
    • [x] Providing imports
      • [x] functions #576 #557
      • [x] globals #624
      • [x] memory #631
      • [x] table #628
    • [x] Providing memory pages limit #780
    • [x] returning error code/message #772
    • [x] With metering enabled #799
  • [x] Execute #576
  • [ ] Module inspection
    • [x] types #675
    • [x] imports #683
    • [x] function types #557
    • [ ] functions (get type index by function index)
    • [ ] table
      • [x] has_table #684
      • [ ] limits
    • [ ] memory
      • [x] has_memory #684
      • [ ] limits
    • [x] global types #675
    • [ ] exports (get index by name)
      • [x] functions #601
    • [ ] exports - get export by index
    • [ ] start function
      • [x] has_start_function #685
      • [ ] get index
    • [ ] element, data, code?
    • [ ] custom sections
  • [x] Clone module https://github.com/wasmx/fizzy/pull/674
  • [x] Instance inspection
    • [x] memory
    • [ ] table
    • [ ] globals
  • [x] Exports access (find by name)
    • [x] function #633
    • [x] table #628
    • [x] memory #631
    • [x] globals #627
  • [ ] Resolving imports
    • [x] functions #608
    • [ ] table
    • [ ] memory
    • [x] globals #660

gumb0 avatar Sep 29 '20 17:09 gumb0

Can a roadmap item be added for all-around better error handling? The existing API has zero error handling, and though I'm moving to Fizzy from wasm3 due to some convoluted blocking bugs, wasm3 has much better error handling in its API that I'm really missing.

Right now, it's just "welp, it failed" if some of the resulting pointers are NULL - there's nothing to help understand why something failed.

Qix- avatar Mar 12 '21 16:03 Qix-

You are absolutely right. I was integrating fizzy into something else the other day and the lack of error message propagation is annoying. We'll think about a good API for this for the C layer, but if you have some ideas please do not be afraid to share them.

axic avatar Mar 12 '21 17:03 axic

@Axic an extra parameter that accepts a pointer to an error code would be great.

Alternatively, changing the return types to be an error code (or success code of course) and having the pointer results be returned via pointer-to-pointer arguments.

For example:

FizzyModule *fz_mod;

int r = fizzy_parse(
	&fz_mod,
	mod_bytes.data(),
	mod_bytes.size()
);

if (r != 0) {
    fprintf(stderr, "error: failed to parse module: %s\n", fizzy_strerr(r)); 
} else {
    assert(fz_mod != NULL);
    /* fz_mod is valid */
}

Qix- avatar Mar 12 '21 18:03 Qix-

I think we want to go beyond error codes to have the capability of returning messages. While error codes work well for many cases, they do not for telling about import resolutions problems (i.e. including namespace, module, etc.)

There are two problems with returning messages though: a) who manages the memory allocated for them; b) can we avoid allocating memory.

In the following option FizzyError is just a nice container for a fixed-size buffer. In this case the error message buffer lives on the stack of the caller:

FizzyError err;
fizzy_parse(..., &err);
... = fizzy_error_msg(&err); // returns const char *

In the following option we use a global buffer, which can be queried. The downside is that subsequent calls overwrite it and it is not thread safe.

fizzy_parse(...);
... = fizzy_error(); // returns const char*

Lastly, we return a message object which has to be explicitly free'd by the caller:

FizzyError* err;
fizzy_parse(..., &err);
... = fizzy_error_msg(err); // returns const char *
fizzy_error_free(err);

I'm sure @chfast and @gumb0 have some better ideas.

axic avatar Mar 17 '21 20:03 axic

@Qix- another question: should we have a public C++ API, would that be preferable? I'm not saying we'll have it anytime soon, but it is only missing due to lack of time.

axic avatar Mar 17 '21 20:03 axic

Something like this seems ok (just a sketch), but happy to review C API design from other libraries.

struct FizzyError { int error_code; const char* message; }

FizzyError err;
fizzy_parse(..., &err);
if (err.error_code != 0)
    printf("%s", err.message) && exit(err.error_code);
fizzy_instantiate(..., &err);
if (err.error_code != 0)
    printf("%s", err.message) && exit(err.error_code);
fizzy_free_error(&err);

So the FizzyError is reusable and optional (you can pass NULL meaning I don't care about error.

chfast avatar Mar 17 '21 20:03 chfast

Yes, that matches my first idea. Though I was wondering if the struct should be opaque or not.

However, when it is reusable then subsequent users must check if err.message is not null and free it.

axic avatar Mar 17 '21 21:03 axic

However, when it is reusable then subsequent users must check if err.message is not null and free it.

No. This can be handled inside Fizzy. Only after the last use the manual free is needed.

Alternatively, we can adopt this Store think present in many wasm implementation, including wasm-c-api.

chfast avatar Mar 17 '21 21:03 chfast

However, when it is reusable then subsequent users must check if err.message is not null and free it.

No. This can be handled inside Fizzy. Only after the last use the manual free is needed.

My wording was unclear, by "subsequent users" I meant subsequent calls. So I agree :wink:

axic avatar Mar 17 '21 21:03 axic

The problem with error strings is two-fold:

  • They are not (easily) localizable.
  • They are not guaranteed to be programmatically switchable (e.g. if (errcode == FIZZY_ENOSUCHIMPORT) { /* ignore */ }) even if you use extern strings.

This is why many C libraries provide a const char * strerror(int) function that translates error codes to statically allocated messages (e.g. POSIX C, libuv, etc.) allowing for other consumers of the library to localize if need-be.

I would recommend doing it this way.


In the event you do decide to go with strings, please under no circumstances perform an allocation. This could cause a cascade of failures that makes debugging (especially in production) very difficult (a memory error would cause a memory-related crash, masking the originating Fizzy-related error - or, many error codes could be generated that are ultimately ignored, but a forgotten free() causes a memory leak over time, etc.)

Any error-related strings should be strictly statically allocated.

Qix- avatar Mar 18 '21 02:03 Qix-