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

[contrib] Fix single-file compilation error on Emscripten build. #2227

Merged
merged 3 commits into from
Jul 7, 2020
Merged

[contrib] Fix single-file compilation error on Emscripten build. #2227

merged 3 commits into from
Jul 7, 2020

Conversation

yoshihitoh
Copy link
Contributor

@yoshihitoh yoshihitoh commented Jul 5, 2020

The single-file library makes a compilation error on Emscripten build due to static variable names collisions, reported on #2211

  • Rename a few static variables
  • Try Emscripten build using docker if emcc command is not available

Error like this...

$ ./build_library_test.sh
Amalgamating files... this can take a while
Processing: zstd-in.c
...
Combine script: PASSED
Single file library creation script: PASSED
Compiling roundtrip.c: PASSED
Compression: PASSED (size: 3127, uncompressed: 32768)
Decompression: PASSED
Byte comparison: PASSED
Running roundtrip.c: PASSED
zstd.c:30822:21: error: redefinition of 'g_ctx' with a different type: 'COVER_ctx_t *' vs 'POOL_ctx' (aka 'struct POOL_ctx_s')
static COVER_ctx_t *g_ctx = NULL;
                    ^
zstd.c:7413:17: note: previous definition is here
static POOL_ctx g_ctx;
                ^
zstd.c:30869:26: error: passing 'POOL_ctx' (aka 'struct POOL_ctx_s') to parameter of incompatible type 'COVER_ctx_t *'
  int result = COVER_cmp(g_ctx, lp, rp);
                         ^~~~~
zstd.c:30845:35: note: passing argument to parameter 'ctx' here
static int COVER_cmp(COVER_ctx_t *ctx, const void *lp, const void *rp) {
                                  ^
zstd.c:30879:27: error: passing 'POOL_ctx' (aka 'struct POOL_ctx_s') to parameter of incompatible type 'COVER_ctx_t *'
  int result = COVER_cmp8(g_ctx, lp, rp);
                          ^~~~~
zstd.c:30853:36: note: passing argument to parameter 'ctx' here
static int COVER_cmp8(COVER_ctx_t *ctx, const void *lp, const void *rp) {
                                   ^
zstd.c:31210:11: error: assigning to 'POOL_ctx' (aka 'struct POOL_ctx_s') from incompatible type 'COVER_ctx_t *'
    g_ctx = ctx;
          ^ ~~~
4 errors generated.
emcc: error: '/Users/yoshihitoh/workspace/tools/sdk/emsdk/upstream/bin/clang -target wasm32-unknown-emscripten -D__EMSCRIPTEN_major__=1 -D__EMSCRIPTEN_minor__=39 -D__EMSCRIPTEN_tiny__=18 -D_LIBCPP_ABI_VERSION=2 -Dunix -D__unix -D__unix__ -Werror=implicit-function-declaration -Xclang -nostdsysteminc -Xclang -isystem/Users/yoshihitoh/workspace/tools/sdk/emsdk/upstream/emscripten/system/include/compat -Xclang -isystem/Users/yoshihitoh/workspace/tools/sdk/emsdk/upstream/emscripten/system/include -Xclang -isystem/Users/yoshihitoh/workspace/tools/sdk/emsdk/upstream/emscripten/system/include/libc -Xclang -isystem/Users/yoshihitoh/workspace/tools/sdk/emsdk/upstream/emscripten/system/lib/libc/musl/arch/emscripten -Xclang -isystem/Users/yoshihitoh/workspace/tools/sdk/emsdk/upstream/emscripten/system/local/include -Xclang -isystem/Users/yoshihitoh/workspace/tools/sdk/emsdk/upstream/emscripten/system/include/SSE -Xclang -isystem/Users/yoshihitoh/workspace/tools/sdk/emsdk/upstream/emscripten/cache/wasm-lto/include -DEMSCRIPTEN -fignore-exceptions -fno-inline-functions -Wall -Wextra -Werror -Os -flto -I. zstd.c -Xclang -isystem/Users/yoshihitoh/workspace/tools/sdk/emsdk/upstream/emscripten/system/include/SDL -c -o /var/folders/x7/xy9p7bj978sgz5b15_l26_4c0000gq/T/emscripten_temp_g31g5lty/zstd_0.o -flto=full' failed (1)
Compiling emscripten.c: FAILED

will be fixed like...

$ ./build_library_test.sh
Amalgamating files... this can take a while
Processing: zstd-in.c
...
Combine script: PASSED
Single file library creation script: PASSED
Compiling roundtrip.c: PASSED
Compression: PASSED (size: 3127, uncompressed: 32768)
Decompression: PASSED
Byte comparison: PASSED
Running roundtrip.c: PASSED
Compiling emscripten.c: PASSED

@Cyan4973
Copy link
Contributor

Cyan4973 commented Jul 6, 2020

Thanks @yoshihitoh !
Featuring a straightforward fix, this PR looks good to me.

I also like the care you took in adding a test, including a docker stage when emcc is not locally present,
although I note it requires trusting trzeci/emscripten as a source.
Do I understand properly that the docker image is automatically downloaded and run when emcc is not locally present ?

@yoshihitoh
Copy link
Contributor Author

yoshihitoh commented Jul 7, 2020

@Cyan4973 Thank you for taking the time to review this PR!

although I note it requires trusting trzeci/emscripten as a source.
Do I understand properly that the docker image is automatically downloaded and run when emcc is not locally present ?

Yes, this patch will make tests to trust the docker image trzeci/emscripten, and download the image if emcc command is not found on the environment.
I chose this image because trzeci/emscripten series is the most used image on Dockerhub.

If it is not a good way to trust the third-party image, I can replace it to the official image or dedicated image using local Dockerfile for this feature.

I think this is the official image:
https://hub.docker.com/r/emscripten/emsdk

published by emscripten-core/emsdk on CI build:
https://github.com/emscripten-core/emsdk/blob/master/.circleci/config.yml#L105-L124

@Cyan4973
Copy link
Contributor

Cyan4973 commented Jul 7, 2020

Thanks for very clear explanations @yoshihitoh ,

I readily admit that I'm not a specialist when it comes to docker,
but I've read reports that it can become a potential malware infection channel, due to users trusting too easily docker contents.

The docker image you selected looks reasonable,
but I would feel even better, from a user security standpoint, if it was possible to use the official docker image
(presuming it hosts enough material for the tests to work).
I suspects the security difference is negligible today, but in several years from now, well a lot of things can happen, and the "official" image is likely to remain the better trusted and maintained one.

@yoshihitoh
Copy link
Contributor Author

@Cyan4973 Thank you for the detailed explanation about the security consideration!

and the "official" image is likely to remain the better trusted and maintained one.

I agree with you, and I changed the docker image to the Emscripten's official one with an explicit latest tag, which works pretty well. I think they have not announced the official image yet because it's quite new, but I'm sure this is the correct one.

@Cyan4973 Cyan4973 merged commit 2cdd33a into facebook:dev Jul 7, 2020
@yoshihitoh yoshihitoh deleted the single-file-dict-emscripten branch July 8, 2020 00:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants