Optional SIMD strlen

[ncruces]

This is a potential first step at upstreaming #580 piecemeal.

Chose strlen as it's already covered by tests. It's also one of the simplest (easiest to review) implementations.

Built and tested with:

CC=[PATH_TO]/wasi-sdk-25.0-x86_64-linux/bin/clang \
  EXTRA_CFLAGS="-O2 -DNDEBUG -msimd128 -mbulk-memory -D__wasilibc_simd_string" make
(cd test ; CC=[PATH_TO]/wasi-sdk-25.0-x86_64-linux/bin/clang make )

[ncruces]

That's odd. Do I need to guard that include? It worked with my clang, but apparently not here.

[abrown]

Yes, take a look at how Clang is set up here with main.yml; I don't see anything that would set up its include path.

[abrown]

@ncruces and I are talking offline about some of the CI issues; the easy answer for now is to just guard the extra wasm_simd.h header. To be honest, maybe some of this problem would go away if wasi-libc and wasi-sdk were merged (#427); the headers should be right there.

I will note that we probably want to check a few more corner cases here, especially since a quick search of the test directory doesn't reveal much testing of strlen (e.g., grep -Ir strlen test). I think the one concern several of us had when we discussed this is, "what happens when we run into the border of WebAssembly memory?" If the v128 loads were to "over-read" and result in a trap, that would be different behavior than the scalar version of this function. I think it would be good to add a test/src/misc/strlen.c that checks for edge cases like:

• the last byte in memory is 0 but a v128 read crosses the end of memory; should not trap
• the last byte in memory is not 0; should trap for both SIMD and scalar
• from a string starting at byte 1, especially where the previous byte is 0 to test some of the alignment logic
• from a string ending at an unaligned address; especially where the trailing bytes are some pattern of 0 and 1

Perhaps you already have some of these kinds of tests implemented elsewhere?

[ncruces]

@abrown, those cases are considered, and yes I have tests for them, just not in C (I do the setup in the host, which for me is in Go): exhaustive tests. I can try to port some of this.

The problem with testing "the last byte of memory" from inside C, is that I don't think I can force malloc to give me that memory. Any ideas?

Handling those cases is the whole purpose of aligning the pointer: an aligned load cannot trap, because the 16-bytes can't cross a page (at least until clang supports custom page sizes).

[ncruces]

Also, I don't think this one is something we can or should test:

• “the last byte in memory is not 0; should trap for both SIMD and scalar”

This is undefined behaviour, so I don't think C promises a trap. Or in fact that it can promise a trap. As soon as clang figures out that it is undefined behaviour, all bets are off.

Also, I don't think there's a change of behaviour, but (to give you an idea of the problem here) I'm reasonably sure if the module allocates the full 4GB, both the existing strlen and my SIMD version both wrap around memory to address zero (and don't trap, because dereferencing zero is allowed) if the last address of the 4GB is not zero.

[ncruces]

Added tests. I have ncruces@4251588 to test this in CI, but it uses wasi-sdk and modifies your Makefile so I'm not sure how you wanna tackle that.

[abrown]

I don't think this one is something we can or should test

Yeah, I see what you mean. Additionally, I may not be remembering correctly, but I don't think we have a way to check that a test traps (?). I think it is all structured the other way around. So, yes, let's avoid that case.

[ncruces]

Yes, I don't think so. Basically, it seems any test that writes to stdout seems to be a failure. This surprised me, I thought exit code would matter.

Let me know if you want me to add anything like the CI commit I linked in #586 (comment).

[abrown]

I think this makes sense; let's add the doc comments and I will +1. We've identified the bigger problem here (which @ncruces shouldn't have to solve) which is that we need an easier way to test this kind of functionality in CI; right now this test (a good one, thanks!) only runs in with the scalar version of strlen. I've opened an issue to figure this out (#587), but in the meantime I would be willing to merge this, especially since it is behind flags, etc..

[abrown]

Yes, I don't think so. Basically, it seems any test that writes to stdout seems to be a failure. This surprised me, I thought exit code would matter.

Let me know if you want me to add anything like the CI commit I linked in #586 (comment).

Oh, the exit code does matter:

wasi-libc/test/scripts/run-test.sh

Line 20 in 4720b34

[ $? -eq 0 ] || echo "Test failed" >> output.log

But, yeah, eventually we could and probably should relax the "no output" check:

wasi-libc/test/scripts/failed-tests.sh

Line 14 in 4720b34

FAILED=$(find $TESTDIR -type f -and -name *.log -and -not -size 0)

I can't remember exactly why that is there but my guess would be that libc-test only uses output in error cases (?). If you're interested in making that more robust, it likely wouldn't be too difficult to add expected.log files for tests that need this and then use that (if present) to compare against output.log.

[alexcrichton]

I wanted to echo again thank you @ncruces for working on this, it's much appreciated!

For the opt-in, this is currently using __wasilibc_simd_string, but if I could bikeshed that a bit maybe this could be renamed to _WASI_SIMD? Using all-caps feels a bit more idiomatic with what wasi-libc does today with other user-provided #defines such as _WASI_EMULATED_MMAN and also scoping it to just "SIMD" instead of "SIMD_STRING" helps reuse this for anywhere else in wasilibc we might want to use simd too. (under the assumption that multiple layers of opt-in aren't necessary that is)

[alexcrichton]

I'm by no means an expert on C, but this looks like it's reading data prior to an allocated object which I believe is UB. This is UB at the C level, I think, although I realize there's no issue reading at the wasm-level.

Would it be possible to do the scalar loop up until s is 16-byte aligned?

[alexcrichton]

Well, I can probably rephrase this a bit. I'm pretty certain this is UB to read data before an allocated pointer. This program:

#include <stdio.h>
#include <stdlib.h>

int main() {
  char *c = malloc(1);
  *c = 0;
  printf("%d\n", *(c - 1));
}

runs as:

$ clang foo.c -fsanitize=address && ./a.out
=================================================================
==2060611==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x50200000000f at pc 0x5a6c161867c3 bp 0x7fff90b4b5a0 sp 0x7fff90b4b598
READ of size 1 at 0x50200000000f thread T0
    #0 0x5a6c161867c2 in main (/home/alex/code/wasi-libc/a.out+0x1047c2) (BuildId: b1ea958363f739949e3f7cab54dc5a56182d8397)
    #1 0x72399122a1c9 in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16
    #2 0x72399122a28a in __libc_start_main csu/../csu/libc-start.c:360:3
    #3 0x5a6c160ad344 in _start (/home/alex/code/wasi-libc/a.out+0x2b344) (BuildId: b1ea958363f739949e3f7cab54dc5a56182d8397)
...

[ncruces]

Possible, yes. It kinda defeats the purpose, in both performance (speed) and code size (when optimizing for speed and size both).

Yes, reading before the buffer is undefined at the C level, but so is reading past the buffer, which, since the buffer is of an unknown size is impossible not to do: the existing SWAR code does it.

So, yes, probably the correct way to go, is to do this in assembly (as musl, glibc do). If that's the path you decide on I guess you could do worse than start with the code this currently generates.

Or you need to decide what kind of UB is acceptable at the C level. Does __may_alias__ fix it for the existing SWAR code? If so, using wasm_v128_load should allay your concerns. And if it doesn't, why not? It's not necessary, mind you (because the address is aligned), but it if it "hides" UB from the compiler, that's OK.

Note that the reason SWAR uses scalar loops before/after should be mostly because the SWAR equivalent to wasm_i8x16_bitmask is hard/slow, and not because of concerns about dereferencing a w that only partially covers the buffer.

[ncruces]

Also, I've used emcc with sanitizers to build the following:

#include <limits.h>
#include <stdint.h>
#include <stdio.h>
#include <stdlib.h>
#include <wasm_simd128.h>

#define ALIGN (sizeof(size_t))
#define ONES ((size_t)-1/UCHAR_MAX)
#define HIGHS (ONES * (UCHAR_MAX/2+1))
#define HASZERO(x) ((x)-ONES & ~(x) & HIGHS)

size_t strlen2(const char *s)
{
{
#ifdef __wasm_simd128__
	// strlen must stop as soon as it finds the terminator.
	// Aligning ensures loads beyond the terminator are safe.
	uintptr_t align = (uintptr_t)s % sizeof(v128_t);
	const v128_t *v = (v128_t *)(s - align);

	for (;;) {
		// Bitmask is slow on AArch64, all_true is much faster.
		if (!wasm_i8x16_all_true(*v)) {
			const v128_t cmp = wasm_i8x16_eq(*v, (v128_t){});
			// Clear the bits corresponding to alignment (little-endian)
			// so we can count trailing zeros.
			int mask = wasm_i8x16_bitmask(cmp) >> align << align;
			// At least one bit will be set, unless we cleared them.
			// Knowing this helps the compiler.
			__builtin_assume(mask || align);
			// If the mask is zero because of alignment,
			// it's as if we didn't find anything.
			if (mask) {
				// Find the offset of the first one bit (little-endian).
				return (char *)v - s + __builtin_ctz(mask);
			}
		}
		align = 0;
		v++;
	}
#endif

	const char *a = s;
#ifdef __GNUC__
	typedef size_t __attribute__((__may_alias__)) word;
	const word *w;
	for (; (uintptr_t)s % ALIGN; s++) if (!*s) return s-a;
	for (w = (const void *)s; !HASZERO(*w); w++);
	s = (const void *)w;
#endif
	for (; *s; s++);
	return s-a;

}

int main() {
  char *c = malloc(1);
  c[0] = 0;
  printf("%lu\n", strlen2(c));
  free(c);
}

And sure enough, it does trip the sanitizer:

$ emcc -msimd128 main.c -o main.html --emrun -fsanitize=address && emrun main.html 
Now listening at http://0.0.0.0:6931/
Opening in existing browser session.
=================================================================
==42==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x136007b0 at pc 0x00000d20 bp 0x129574c0 sp 0x129574cc
READ of size 16 at 0x136007b0 thread T0
    #0 0x00000d20 in wasm-function[30]+0xd20 (this.program+0xd20)

0x136007b1 is located 0 bytes after 1-byte region [0x136007b0,0x136007b1)
allocated by thread T0 here:
    #0 0x000191a5 in wasm-function[373]+0x191a5 (this.program+0x191a5)
    #1 0x00000f4c in wasm-function[31]+0xf4c (this.program+0xf4c)
    #2 0x000010dc in wasm-function[36]+0x10dc (this.program+0x10dc)
    #3 0x80000317  (JavaScript+0x317)
    #4 0x80001199 in callMain http://localhost:6931/main.js:4505:15
    #5 0x800011c3 in doRun http://localhost:6931/main.js:4547:24
    #6 0x800011ca  (JavaScript+0x11ca)

The problem with using that as a benchmark is that building the current musl SWAR code, without SIMD, trips the sanitizer in the exact same way:

$ emcc main.c -o main.html --emrun -fsanitize=address && emrun main.html 
Now listening at http://0.0.0.0:6931/
Opening in existing browser session.
=================================================================
==42==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x136007b0 at pc 0x00000e80 bp 0x12957520 sp 0x1295752c
READ of size 4 at 0x136007b0 thread T0
    #0 0x00000e80 in wasm-function[30]+0xe80 (this.program+0xe80)

0x136007b1 is located 0 bytes after 1-byte region [0x136007b0,0x136007b1)
allocated by thread T0 here:
    #0 0x000193bf in wasm-function[373]+0x193bf (this.program+0x193bf)
    #1 0x00001166 in wasm-function[31]+0x1166 (this.program+0x1166)
    #2 0x000012f6 in wasm-function[36]+0x12f6 (this.program+0x12f6)
    #3 0x80000317  (JavaScript+0x317)
    #4 0x80001199 in callMain http://localhost:6931/main.js:4505:15
    #5 0x800011c3 in doRun http://localhost:6931/main.js:4547:24
    #6 0x800011ca  (JavaScript+0x11ca)

So, I'm not sure that's really a useful standard to aim for. None of the above run into issues with the undefined sanitizer.

[ncruces]

Another possibility that may allay concerns might be to use *(volatile v128_t*)v for any loads.

[alexcrichton]

To me UB is UB and should be avoided at all times, but then again I come from a background of mostly Rust-based development. I don't review musl code but I did take a look at this PR, and I realize it can be confusing when it feels like you're being held to different standards than the rest of the codebase.

I've not interacted or heard of __may_alias__ before myself. That may mean that the error flagged in asan with emcc is a false positive, but then again it could also mean that while __may_alias__ is well-intentioned but it's still UB.

Yes, reading before the buffer is undefined at the C level, but so is reading past the buffer, which, since the buffer is of an unknown size is impossible not to do: the existing SWAR code does it.

Good point! I agree reading past the end is UB too.

Or you need to decide what kind of UB is acceptable at the C level.

Well, this is sort of my background speaking, but IMO there is no amount of UB that is acceptable. Of course reasonable folks can have different stance as well though, I'm just one opinion.

If so, using wasm_v128_load should allay your concerns. And if it doesn't, why not?

...

Another possibility that may allay concerns might be to use (volatile v128_t)v for any loads.

I'm not sure! I'm pretty sure normal reads before/after an allocation are UB, but outside of that realm in the world of volatlie and compiler intrinsics such questions would probably need to be answered by someone with deeper compiler knowledge than I. I am not nor do I want to be an arbiter of whether or not an operation is UB for wasi-libc.

[ncruces]

This is UB, all of it is; volatile, may_alias, using intrinsics, may prevent the compiler from acting on it, but that's it; in this case, volatile would work best, I think.

The compiler should also not act on it, if the function isn't inlined, because whether it is UB, depends on the pointer itself (e.g. it's not UB if the pointer points into the middle of a larger buffer). So this should be fine with separate compilation, which is why musl gets away with it.

And you hit nail on the head: if you're going to hold this code to an higher standard than musl, this is simply impossible to do. I may as well drop the whole thing, which is fine (but I'd rather avoid pouring more effort into it).

The only way then, is doing it in assembly, perhaps compiling something, then inspecting the assembly.