Skip to content

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

Merged
Cyan4973 merged 3 commits intofacebook:devfrom
yoshihitoh:single-file-dict-emscripten
Jul 7, 2020
Merged

[contrib] Fix single-file compilation error on Emscripten build.#2227
Cyan4973 merged 3 commits intofacebook:devfrom
yoshihitoh:single-file-dict-emscripten

Conversation

@yoshihitoh
Copy link
Contributor

@yoshihitoh yoshihitoh commented Jul 5, 2020 •
edited
Loading

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 •
edited
Loading

@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

yoshihitoh commented Jul 7, 2020

@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

Reviewers

@Cyan4973 Cyan4973 Cyan4973 approved these changes

Assignees

No one assigned

Labels

CLA Signed

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@yoshihitoh @Cyan4973 @facebook-github-bot