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

Fix compilation with Emscripten #52

Merged
merged 12 commits into from
Dec 24, 2021
Merged

Fix compilation with Emscripten #52

merged 12 commits into from
Dec 24, 2021

Conversation

cschreib
Copy link
Contributor

@cschreib cschreib commented Dec 22, 2021

fixes #50
fixes #51

@biojppm
Copy link
Owner

biojppm commented Dec 23, 2021

Thanks for the MR. Would you mind adding an emscripten workflow to this file, taking as an example e.g. the gcc_canary workflow?

Also, I think this action will be of use to help setup emscripten.

@codecov
Copy link

codecov bot commented Dec 23, 2021

Codecov Report

Merging #52 (9275c73) into master (ac49cc7) will increase coverage by 0.16%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #52      +/-   ##
==========================================
+ Coverage   92.51%   92.67%   +0.16%     
==========================================
  Files          50       50              
  Lines        9825     9781      -44     
==========================================
- Hits         9090     9065      -25     
+ Misses        735      716      -19     
Impacted Files Coverage Δ
src/c4/charconv.hpp 97.54% <ø> (-0.03%) ⬇️
src/c4/bitmask.hpp 95.78% <100.00%> (ø)
test/test_charconv.cpp 100.00% <100.00%> (ø)
src/c4/ext/fast_float_all.h 22.14% <0.00%> (-1.54%) ⬇️
test/test_memory_resource.cpp 100.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ac49cc7...9275c73. Read the comment docs.

@cschreib
Copy link
Contributor Author

Sure! I have set it up for other projects of mine, I'll have a look at how to integrate this into your CI workflow.

@biojppm
Copy link
Owner

biojppm commented Dec 23, 2021

Thanks. If the current approach with its shell script stands in the way, feel free to do it the same way you did before. For example, this may be easier: .github/workflows/test_install.yml, as it uses cmake directly

@cschreib
Copy link
Contributor Author

cschreib commented Dec 23, 2021

Ok, I'm giving the CI a shot. I managed to get the tests building and running locally, we'll see if I got the CI scripts right!
When I run the tests locally, I get failures in the charconv test for float to hex:

===============================================================================
/home/cschreib/programming/c4core/test/test_charconv.cpp:1193:
TEST CASE:  ftoa.basic

/home/cschreib/programming/c4core/test/test_charconv.cpp:1189: ERROR: CHECK( ok ) is NOT correct!
  values: CHECK( false )
  logged: num=1.123412 precision=2'  hexa='0x1.20p+0'  hexa_alternative='0x1.20p+0'
          ret='0x1.1f8p+0

/home/cschreib/programming/c4core/test/test_charconv.cpp:1189: ERROR: CHECK( ok ) is NOT correct!
  values: CHECK( false )
  logged: num=1.123412 precision=2'  hexa='0x1.20p+0'  hexa_alternative='0x1.20p+0'
          ret='0x1.1f8p+0

/home/cschreib/programming/c4core/test/test_charconv.cpp:1189: ERROR: CHECK( ok ) is NOT correct!
  values: CHECK( false )
  logged: num=1.123412 precision=3'  hexa='0x1.1f9p+0'  hexa_alternative='0x1.1f9p+0'
          ret='0x1.1f98p+0

/home/cschreib/programming/c4core/test/test_charconv.cpp:1189: ERROR: CHECK( ok ) is NOT correct!
  values: CHECK( false )
  logged: num=1.123412 precision=3'  hexa='0x1.1f9p+0'  hexa_alternative='0x1.1f9p+0'
          ret='0x1.1f98p+0

/home/cschreib/programming/c4core/test/test_charconv.cpp:1189: ERROR: CHECK( ok ) is NOT correct!
  values: CHECK( false )
  logged: num=1.012341 precision=3'  hexa='0x1.033p+0'  hexa_alternative='0x1.032p+0'
          ret='0x1.0328p+0

/home/cschreib/programming/c4core/test/test_charconv.cpp:1189: ERROR: CHECK( ok ) is NOT correct!
  values: CHECK( false )
  logged: num=1.012341 precision=3'  hexa='0x1.033p+0'  hexa_alternative='0x1.032p+0'
          ret='0x1.0328p+0

0.004000 s: ftoa.basic

@biojppm
Copy link
Owner

biojppm commented Dec 23, 2021

I tried on my Manjaro machine, and the charconv test does succeed, with or without -m32:

$ cmake -S . -B build/emscripten -DCMAKE_TOOLCHAIN_FILE=/usr/lib/emscripten/cmake/Modules/Platform/Emscripten.cmake -DCMAKE_CROSSCOMPILING_EMULATOR=/usr/bin/node -D C4CORE_BUILD_TESTS=ON -DCMAKE_CXX_FLAGS="-m32 -s NO_DISABLE_EXCEPTION_CATCHING" && cmake --build build/emscripten/ --target c4core-test-run -j
      Start 17: c4core-test-charconv
17/21 Test #17: c4core-test-charconv .............   Passed    0.33 sec
      Start 18: c4core-test-format
18/21 Test #18: c4core-test-format ...............   Passed    0.25 sec
      Start 19: c4core-test-base64
19/21 Test #19: c4core-test-base64 ...............   Passed    0.18 sec
      Start 20: c4core-test-std_string
20/21 Test #20: c4core-test-std_string ...........   Passed    0.17 sec
      Start 21: c4core-test-std_vector
21/21 Test #21: c4core-test-std_vector ...........   Passed    0.18 sec

100% tests passed, 0 tests failed out of 21

Total Test time (real) =   4.72 sec
[100%] Built target c4core-test-run

@biojppm
Copy link
Owner

biojppm commented Dec 23, 2021

... indeed that charconv test fails in the github workflow, while it succeeds in my machine. Maybe this is a version issue? My emscripten version is this:

[jpmag@bach c4core]$ pacman -Qs emscripten
local/emscripten 3.0.0-1
    Compile C and C++ into highly-optimizable JavaScript for the web

@biojppm
Copy link
Owner

biojppm commented Dec 23, 2021

Can I split the emscripten workflow to a separate file? It's taking too long to get there...

@cschreib
Copy link
Contributor Author

Sure. The initial run does take a long time, but once the cache is setup it is normally pretty quick.

Interesting that upgrading to 3.0 fixes the tests. I didn't know this version was out, I'll have to upgrade my projects too! Still, while the Debug builds passes now, it seems Release is still having issue.

changelog/current.md Outdated Show resolved Hide resolved
@biojppm
Copy link
Owner

biojppm commented Dec 24, 2021

Right, I have found out the issue with the failure in Release; the issue was legit and it is now fixed (and I'm wondering how it is that the test ever passed!). Hopefully, all the four tests will now pass.

@biojppm
Copy link
Owner

biojppm commented Dec 24, 2021

However, I am concerned with the emscripten version constraint. The current ftoa() and dtoa() implementation is using sprintf() to achieve the asked precision in hexadecimal format, and the failures we see above to respect the precision are very likely coming from the sprintf() implementation in the standard library used by that version of emscripten, as evidenced by the fact that 3.0.0 works while 2.0.34 doesn't.

I'm guessing that 3.0.0 is fairly new and many users will not be using this version yet. I reckon it is worth keeping the tests in the earlier emscripten version. I am reinstating the 2.0.34 workflows, and will add a emscripten specific workaround in the ftoa hexadecimal test. This hexadecimal float formatting issue is sufficiently rare to warrant this.

(BTW, this is not the first time I have serious trouble coming from sprintf or sscanf; there is an accidentally quadratic bug in glibc's sscanf previously used by c4::atof() that made the rounds in the internet).

@cschreib
Copy link
Contributor Author

The main change in Emscripten 3.0 was the upgrade to their libc:

The version of musl libc used by emscripten was upgraded from v1.1.15 to v1.2.2. There could be some minor size regressions (or gains) due to changes in upstream musl code but we don't expect anything major. Since this is a fairly substantial change (at least internally) we are bumping the major version of Emscripten to 3. (#13006)

So it makes sense that sprintf could be affected.

@biojppm
Copy link
Owner

biojppm commented Dec 24, 2021

Thanks!

@biojppm biojppm merged commit db40178 into biojppm:master Dec 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants