Skip to content
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.

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

Initial WebAssembly support. #175

Draft
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

waywardmonkeys
Copy link
Contributor

Fixes #168.

@waywardmonkeys
Copy link
Contributor Author

I meant for this to be a draft. I will be pushing or force-pushing to it over the coming days / weeks.

@rptb1 rptb1 marked this pull request as draft February 24, 2023 08:14
When compiling with `-s USE_PTHREADS` in emscripten, Posix threads
and locks are available (assuming the WebAssembly VM supports this
extension).

This can be detected at compile time by checking for the preprocessor
definition `__EMSCRIPTEN_PTHREADS__`.

In the absence of this definition, we request a single threaded
configuration which falls back to the implementation in `lockan.c`.

This works when building `mps.c`, but does not work when
using the older Makefile-based build system as when building
`lockix.c` as a standalone file, `mpstd.h` is not included prior
to the relevant portion of `config.h` being processed.
This uses GitHub actions and installs the Emscripten SDK's
latest version and activates it. At some point, this should
probably change to a fixed version that is periodically
incremented once some baseline support level is determined.

This is just compiling and running what's needed for `amsss`
for now until other pieces are in place for building and
running more of everything.
This fails still due to threaded tests being built and that
failing.
@waywardmonkeys
Copy link
Contributor Author

This is a more interesting build error, haven't looked into it yet, requires changes I haven't pushed yet to get to this point:

eventtxt.c:162:3: error: shift count >= width of type [-Werror,-Wshift-count-overflow]
  EVENT_CLOCK_MAKE(val, low, high);
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
./clock.h:162:35: note: expanded from macro 'EVENT_CLOCK_MAKE'
  ((lvalue) = ((EventClock)(high) << 32) + ((EventClock)(low) & (0xfffffffful)))
                                  ^  ~~
1 error generated.

Note that this is only valid as noted in a fixme comment when
threads are enabled in emscripten (which is not the default).
@rptb1
Copy link
Member

rptb1 commented Feb 27, 2023

This is a more interesting build error

Are you not getting a 64-bit unsigned long long?

__extension__ typedef unsigned long long EventClock;

@waywardmonkeys
Copy link
Contributor Author

That's line 111, the error was on line 162, which is the mps_clock_t code path ... mps_clock_t is mps_word_t

@rptb1
Copy link
Member

rptb1 commented Feb 27, 2023

Ah yes. Looks like a 64-bit assumption has crept in here

mps/code/clock.h

Lines 156 to 162 in 142999b

/* no fast clock, use plinth, probably from the C library */
#ifndef EVENT_CLOCK
typedef mps_clock_t EventClock;
#define EVENT_CLOCK_MAKE(lvalue, low, high) \
((lvalue) = ((EventClock)(high) << 32) + ((EventClock)(low) & (0xfffffffful)))

but since

mps/code/mps.h

Line 77 in 142999b

typedef mps_word_t mps_clock_t; /* processor time */

we could switch here on MPS_WORD_WIDTH or define MPS_CLOCK_WIDTH to be MPS_WORD_WIDTH and switch on that.

@rptb1
Copy link
Member

rptb1 commented Feb 27, 2023

EVENT_CLOCK_MAKE is only used in

mps/code/eventtxt.c

Lines 152 to 166 in 142999b

static EventClock parseClock(char **pInOut)
{
EventClock val;
int i, l;
unsigned long low, high;
char *p = *pInOut;
i = sscanf(p, "%08lX%08lX%n", &high, &low, &l);
if (i != 2)
everror("Couldn't read a clock from '%s'", p);
EVENT_CLOCK_MAKE(val, low, high);
*pInOut = p + l;
return val;
}
which is intended to only be run on the same platform as the MPS that generated the telemetry, though we might be able to make it more flexible.

WebAssembly doesn't support `mprotect()` or signals. This will
make it hard to support multithreaded or incremental collection
for now.
@thejayps
Copy link
Contributor

JPH+RB: This needs CI before merging but currently not a high priority to merge, not currently impacting customers. Assigning importance "optional".

@thejayps thejayps added the optional Will cause failures / of benefit. Worth assigning resources. label Mar 20, 2023
@waywardmonkeys
Copy link
Contributor Author

JPH+RB: This needs CI before merging but currently not a high priority to merge

FWIW, it does have some CI... but needs more. But there was an issue with the CI for it because it was building the threaded tests as well ...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
optional Will cause failures / of benefit. Worth assigning resources.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MPS does not support WebAssembly
3 participants