Don't leak implementation into headers.

[PiotrSikora]

Previously, both: embedding C/C++ application and the library had to
be built using the same DEBUG define, because vec::make_data()
and vec::free_data() were declared in the implementation when
built with DEBUG defined, and in the headers otherwise.

As a result, linking C/C++ application against library built with
different DEBUG define resulted in either: failed build or broken
make/free accounting and the library aborting at runtime.

Signed-off-by: Piotr Sikora [email protected]

[PiotrSikora]

cc @rossberg @jakobkummerow

[rossberg]

I see the problem, but it is a bit unfortunate that every vec allocation & deallocation now involves another function call.

My understanding is that inline functions have static linkage, so wouldn't it be enough to merely do the changes to the cc file? Then the functions always exist when you try to link against them with a DEBUG-mode client, but a non-DEBUG client can still omit the calls.

[PiotrSikora]

@rossberg in theory, that would work for an APIs used only by the application, but in practice, the DEBUG library uses vec<type>::make_data() and vec<type>::free_data() functions with accounting, and the non-DEBUG application uses those functions without accounting.

This means that when ownership is passed between the two, either:

1. The library will incorrectly detect leaks (DEBUG library increases made stat, but the non-DEBUG application won't decrease freed stat), e.g. in examples:

$ make run-hello-cc
mkdir -p out/example
clang++ -c -std=c++11  -Wall -Werror -ggdb -O -fsanitize=address -I. -Iv8/v8/include -I./include example/hello.cc -o out/example/hello-cc.o
clang++  -Wall -Werror -ggdb -O -fsanitize=address -fsanitize-memory-track-origins -fsanitize-memory-use-after-dtor out/example/hello-cc.o -o out/example/hello-cc \
        out/./wasm-bin.o out/./wasm-v8.o \
         \
        v8/v8/out.gn/x64.release/obj/libv8_monolith.a \
         \
        -ldl -pthread
cp example/hello.wasm out/example/hello.wasm
==== C++ hello ====
Initializing...
Loading binary...
Compiling module...
Creating callback...
Instantiating module...
Extracting export...
Calling export...
Calling back...
> Hello world!
Shutting down...
Done.
Leaked 1 instances of wasm::vec<Extern>, made 1, freed 0!
Makefile:109: recipe for target 'run-hello-cc' failed
make: *** [run-hello-cc] Error 1
rm out/example/hello-cc.o

2. The library will abort due to excessive frees (non-DEBUG application doesn't increase made stat, but the DEBUG library will decrease freed stat), e.g.:

Deleting instance of wasm::vec<ValType> when none is alive, made 879, freed 880!
src/wasm-v8.cc:115: wasm::Stats::~Stats(): Assertion `made[i][j] >= freed[i][j]' failed.

[rossberg]

I see, good point. In that case, can we somehow make linking between client and lib with different DEBUG settings fail in all cases, such that users are forced to use it consistently?

Either way, we should probably use a more specific macro name to avoid collision with other uses of DEBUG in clients.

[jakobkummerow]

My vote is to drop vec::make_data and vec::free_data entirely. If an implementation of the library wants to do accounting for debugging/development/investigation purposes, it can find some way to hack that in; for leak checking, LSan is more thorough (and convenient) than handcrafted accounting anyway.

To me, it doesn't feel like such internal debugging helpers belong in the production version of the API.

Any solution that involves forcing the same build flags for client and library makes it difficult to distribute binary versions of the library. As a variant of this, I can imagine embedders wanting to compile their own code in Debug mode while linking against the Release version of whichever library implementation they are using.

[PiotrSikora]

@rossberg sorry, this slipped my mind. I cannot think of an easy way to enforce this at build-time. We could possibly do this at run-time by passing DEBUG value to wasm::Engine::make(), but that sounds like a terrible idea.

I personally spent way more time chasing false positives, which resulted from DEBUG mismatch, than this feature could possibly save, so I agree with @jakobkummerow that we should probably drop accounting altogether.

On a side note, Xcode and Bazel (fastbuild) pass -DDEBUG to CFLAGS and CXXFLAGS, which pretty much guarantees to break anybody using this on macOS (though, this could be easily solved by replacing DEBUG with WASM_DEBUG).

[rossberg]

@jakobkummerow:

LSan is more thorough (and convenient) than handcrafted accounting anyway.

Yes, but imposes much stricter requirements on the overall program, I think. In particular, will it work when linking the lib against an app that is not (yet?) LSan-ready?

[jakobkummerow]

linking the lib against an app that is not (yet?) LSan-ready

I don't see the use case for that? When debugging the library for memory leaks, you can (and probably also want to) link it against a simple and correct embedder. When debugging an embedder, you can assume that the library is leak free. When you're not sure where the leak is, you want both/all components instrumented, and want to fix all reports anywhere.

Why would you want to taint the public header with explicit support for partially-leak-free library/embedder combinations?

[rossberg]

@jakobkummerow, the use case would be embedding V8 with foreign language bindings where the host language runtime may not be LSan-ready, but the user (of the language) has no control over it, but still is in dire need of checking the correct use of the Wasm interface -- which is hairy to get right and debug when all you have is the C interface.

Assuming we rename the flag, is there still a problem? If you are not interested in accounting, why would you set it in the first place? If you are, well, deal with possible mismatches. Having the option still seems better than having no option. Or am I missing something?

[jakobkummerow]

My reasons for preferring to drop make_data and free_data are:

• I think it's nice when library and client can be compiled independently (at different times, on different machines, with different settings, etc). Isn't that (part of) the point of having a standardized API between them?
• since vec is a template, if the library does not instantiate all required specializations that the client uses, linking will fail (unless either the client provides those functions, which defeats the point; or the library is adapted and recompiled). Since the template parameters are not restricted in any way, library authors can't predict which type specializations client applications might find useful (and clients might even define custom types).
• V8's library implementation of the API does not support vec accounting and currently has no plans to change that.

That said, renaming the flag to something that's specific enough to be unlikely to collide (WASM_VEC_ACCOUNTING?) should make it possible to pretty much ignore it, so I could live with that.