Compilation fails because of undefined behaviour catched by `-Wunsequenced`
yeputons opened this issue · 7 comments
General Information
- Bug
- Improvement
- Feature
- Other
Description
There is an undefined behaviour in asmdom::addVNodes because EM_ASM is a macro which evaluates arguments twice when expanded (see corresponding issue). And evaluating startIdx++ twice is undefined behaviour.
Steps to reproduce
I use Windows (probably irrelevant) and emcc (Emscripten gcc/clang-like replacement) 1.38.38 (commit 49652605051d9faca2dcb781918a028bdfbc2703) (probably relevant).
git clone https://github.com/mbasso/asm-dom-boilerplate.git
cd asm-dom-boilerplate
npm install
# if you are using windows, you have to make a little change to the Makefile in the root of the project, just open it and follow the instructions at the top
make start
Observe the following warning:
emcc \
-O3 -Wall -Werror -Wall -Wno-deprecated -Wno-parentheses -Wno-format \
--bind \
-include node_modules/asm-dom/cpp/asm-dom.hpp \
temp/o/index.o \
node_modules/asm-dom/cpp/asm-dom.cpp \
-o temp/app.bc
In file included from node_modules/asm-dom/cpp/asm-dom.cpp:6:
node_modules/asm-dom/cpp/Patch/patch.cpp:93:43: error: multiple unsequenced modifications to 'startIdx' [-Werror,-Wunsequenced]
}, parentElm, createElm(vnodes[startIdx++]), before);
^~
C:\emsdk\upstream\emscripten\system\include\emscripten/em_asm.h:213:77: note: expanded from macro 'EM_ASM_'
#define EM_ASM_(code, ...) emscripten_asm_const_int(#code _EM_ASM_PREP_ARGS(__VA_ARGS__))
^~~~~~~~~~~
C:\emsdk\upstream\emscripten\system\include\emscripten/em_asm.h:122:42: note: expanded from macro '_EM_ASM_PREP_ARGS'
, __em_asm_sig_builder::__em_asm_sig(__VA_ARGS__).buffer, ##__VA_ARGS__
^ ~~~~~~~~~~~
1 error generated.
shared:ERROR: compiler frontend failed to generate LLVM bitcode, halting
make: *** [Makefile:95: temp/app.bc] Error 1
Suggested fix
Move the offending expression to a separate statement:
int elm = createElm(vnodes[startIdx++]);
EM_ASM_({
Module.insertBefore($0, $1, $2)
}, parentElm, elm, before);Versions
- asm-dom: current
HEAD, ea3dba7 - Browser: N/A
Hi @yeputons,
thank you for opening this issue and suggesting a fix!
I'll try to fix it and publish a new version in the next few days, I hope tomorrow. I'll let you know if it works fine!
I have some troubles updating emscripten, it seems I cannot get version after the 1.38.30... I'll fix it and I'll let you know
Unfortunately I'm not able to reproduce the issue, as you can see the CI just works fine without changes (it's red in the PR cause I stopped the deploy of the website). I have also tested it on Windows. Can you please give me more details about your C++ compiler version and so on?
Anyway, I have just created a PR to fix the problem and published asm-dom@0.6.3-alpha.0. Can you please try it and check if it works fine?
Sure. I've used emsdk to install emscripten, but I installed latest-upstream (which uses plain upstream clang for WASM) instead of latest (which uses fastcomp fork by emscripten)
cd C:\
git clone https://github.com/emscripten-core/emsdk.git
cd emsdk
emsdk install latest-upstream
emsdk activate latest-upstream
cd path-to-asm-dom
# Removed `--closure 1` parameters from Makefile because I don't have Java
make
This results in the following emcc/clang++ versions:
emcc (Emscripten gcc/clang-like replacement) 1.38.40 (commit e2110d88167bc0bd74affe35054d799f90dfa759)
Copyright (C) 2014 the Emscripten authors (see AUTHORS.txt)
This is free and open source software under the MIT license.
There is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
clang version 10.0.0 (C:�swircachegitchromium.googlesource.com-external-github.com-llvm-llvm--project 10dad95a75592717d2f7c0ebc181fb8a970a8df7)
Target: x86_64-pc-windows-msvc
Thread model: posix
InstalledDir: C:\emsdk\upstream\bin
Note: I expect this issue to be fixed in emscripten directly soon, so you won't have to change anything, see this pull request
Also note that lack of warning does not mean lack of undefined behavior in the latest version of emscripten. #28 fixes the problem for me.