Fix all crashing tests for emscripten build

[anutosh491]

Description

Go through comment #483 (comment)

Type of change

Please tick all options which are relevant.

• Bug fix
• New feature
• Requires documentation updates

Testing

Please describe the test(s) that you added and ran to verify your changes.

Checklist

• I have read the contribution guide recently

[anutosh491]

Through #483 some tests were "failing" and some were "crashing". By crash I mean you would see the following while building

1: [ RUN      ] ScopeReflectionTest.IncludeVector
1: wasm://wasm/0098eaa2:1
1: 
1: 
1: RuntimeError: memory access out of bounds
1:     at wasm://wasm/0098eaa2:wasm-function[2582]:0x177d38
1:     at wasm://wasm/0d2d5b5e:wasm-function[27063]:0x1804288
1:     at wasm://wasm/0d2d5b5e:wasm-function[27065]:0x1804457
1:     at wasm://wasm/0d2d5b5e:wasm-function[12866]:0xdf638d
1:     at wasm://wasm/0d2d5b5e:wasm-function[19256]:0x13d2325
1:     at wasm://wasm/0d2d5b5e:wasm-function[19335]:0x13e0d5f
1:     at wasm://wasm/0d2d5b5e:wasm-function[19225]:0x13c2bc9
1:     at wasm://wasm/0d2d5b5e:wasm-function[19263]:0x13d36fe
1:     at wasm://wasm/0d2d5b5e:wasm-function[19225]:0x13c6eda
1:     at wasm://wasm/0d2d5b5e:wasm-function[19225]:0x13c3915
1: 
1: Node.js v20.18.0

This pr fixes all "crashing" tests, which means

i) Either they now pass
ii) Or they now fail

But atleast they run. Crashing is a bad sign as that means we aren't able to build the wasm module (whatever test_XX.wasm is generated when we run a test)

After this PR, we can proceed with going about fixing a test or a couple of tests through each PR and have atomic improvements.

Fixing them is more approachable now as they now fail instead of crashing and we have an error message to look into !

EDIT : And on top of that we can confirm if something is really failing by running the test case in lite. For eg Construct from FunctionReflectionTest currently fails because of a nullptr check...... EXPECT_TRUE(object != nullptr);

We can replicate the exact test in lite and check it out (unfortunately return nullptr when it shouldn't be null)

[anutosh491]

Needed for tests in FunctionReflectionTest like JitCallAdvanced, GetFunctionCallWrapper etc cause otherwise we see

error: undefined symbol: _ZNK3Cpp7JitCall17AreArgumentsValidEPvNS0_7ArgListES1_ (referenced by root reference (e.g. compiled C/C++ code))
warning: To disable errors for undefined symbols use `-sERROR_ON_UNDEFINED_SYMBOLS=0`
warning: __ZNK3Cpp7JitCall17AreArgumentsValidEPvNS0_7ArgListES1_ may need to be added to EXPORTED_FUNCTIONS if it arrives from a system library
error: undefined symbol: _ZNK3Cpp7JitCall17ReportInvokeStartEPvNS0_7ArgListES1_ (referenced by root reference (e.g. compiled C/C++ code))
warning: __ZNK3Cpp7JitCall17ReportInvokeStartEPvNS0_7ArgListES1_ may need to be added to EXPORTED_FUNCTIONS if it arrives from a system library
error: undefined symbol: _ZNK3Cpp7JitCall17ReportInvokeStartEPvmi (referenced by root reference (e.g. compiled C/C++ code))
warning: __ZNK3Cpp7JitCall17ReportInvokeStartEPvmi may need to be added to EXPORTED_FUNCTIONS if it arrives from a system library
Error: Aborting compilation due to previous errors

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 72.71%. Comparing base (4fd5386) to head (15b51c8).
Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #523   +/-   ##
=======================================
  Coverage   72.71%   72.71%           
=======================================
  Files           9        9           
  Lines        3618     3618           
=======================================
  Hits         2631     2631           
  Misses        987      987           

Files with missing lines	Coverage Δ
include/clang/Interpreter/CppInterOp.h	96.29% <ø> (ø)

Files with missing lines	Coverage Δ
include/clang/Interpreter/CppInterOp.h	96.29% <ø> (ø)

🚀 New features to boost your workflow:

• ❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[anutosh491]

Not sure if the clang-format suggested changes should be respected here or not.

[anutosh491]

Here's a summary of what happens

1. ScopeReflectionTest : 4 were marked as crashing now everything passes
2. FunctionReflectionTest - 4 marked as fail, 2 marked as crashing (1st one InstantiateTemplateFunctionFromString passes, the rest fail)
3. TypeReflectionTest - 1 was marked as crashing now everything passes
4. JitTest - 1 marked as crashing , now fails
5. VariableReflectionTest - VariableOffsetsWithInheritance marked as crashing now passes, GetVariableOffset marked as fails still fails

[mcbarton]

@anutosh491 Please fix the issue raised by the clang format workflow.

[anutosh491]

cc @mcbarton

I removed -sWasm=1 as that's a default flag.

I usually keep -s EXIT_RUNTIME=1 whenever we build a main module/executable because It allows the runtime to exit cleanly after the program terminates. Basically frees up memory and cleans up resources so yeah smoother exit. We use it in xcpp.wasm and its also needed with test_xeus_cpp.wasm but yeah tests would run without it too.

So yeah got rid of it as I think you're looking minimal changes here.

And I've taken care of this review of yours #523 (comment) too .

[anutosh491]

@mcbarton so I guess the only change you need here now is an explanation behind the flags ?

[mcbarton]

@mcbarton so I guess the only change you need here now is an explanation behind the flags ?

@anutosh491 The explanation of the flags is something I would need. I would also like the publics turned to private (I know you don't think its needed, but I found things being public caused me issues when working out my automated wasm tests PR).

I have found locally running your fork that the fexceptions and assertion flags to not be necessary to fix the crashing tests so I would like these removed too.

I know the STACK_SIZE and the INITIAL_MEMORY don't need to be anywhere near the values they are from local testing. I found that STACK_SIZE=4mb and INITIAL_MEMORY=16mb are the small enough for everything not to crash.

After these changes have been made I will just double check locally that I see this error you mention here #523 (comment) . After this we should be in a position for me to approve the PR.

[anutosh491]

Okay I am addressing some of the above things.

1. I am keeping STACK_SIZE and the INITIAL_MEMORY the same as what we keep for xeus-cpp-lite.
2. I don't see a concrete reason for us to use private for a main module. You've not pasted an issue somewhere that you had because of this and I didn't have any issues and the CI passes too . I don't think it matters and I've mostly stuck public for main modules in the past.

Anyways I hope we're still sticking to Vassil's idealogy of "Better is enemy of good"

I think these summary of results here is very good (#523 (comment))

I am not keen on making any more improvements on this PR after this. If you see any improvements, you can raise them later. My only concern here is to fix the crashing tests.