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

Update ExprTk #2825

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ArashPartow
Copy link
Contributor

@ArashPartow ArashPartow commented Nov 4, 2024

  1. Update ExprTk to 0.0.3 Release notes: https://www.partow.net/programming/exprtk/exprtk_release_notes_v0.0.3.txt
  2. Adaptor update
  3. Minor fixes and cleanups in exprtk, scalar, computed functions

Signed-off-by: Arash Partow <[email protected]>
Signed-off-by: Arash Partow <[email protected]>
@ArashPartow ArashPartow force-pushed the arashpartow/update_exprtk_0.0.3 branch from a8d3a1c to 0687b1d Compare November 5, 2024 05:34
@ArashPartow
Copy link
Contributor Author

ArashPartow commented Nov 5, 2024

@texodus Looks like a couple of adaptor methods and free functions need to be added to perspective::t_tscalar:

  1. to_uint64
  2. cast operator to uint64 from type

I'm ok with adding them, would you want it all in the same PR or two PRs, initially the additional adaptor methods and then this PR?

@texodus
Copy link
Member

texodus commented Nov 6, 2024

One PR would be ideal.

@ArashPartow ArashPartow force-pushed the arashpartow/update_exprtk_0.0.3 branch from 7658f2e to ccb4b1c Compare November 6, 2024 23:19
@texodus
Copy link
Member

texodus commented Nov 7, 2024

I'm not sure if you can see our CI logs externally, but this PR does not compile for different reasons on every architecture we support.

I have not had time to test this on anything but my development machine yet, but I've rebased your PR and fixed the local compilation errors here, which you can cherry pick.

@ArashPartow ArashPartow force-pushed the arashpartow/update_exprtk_0.0.3 branch 2 times, most recently from d3ce42f to a2c2161 Compare November 7, 2024 04:06
@ArashPartow ArashPartow marked this pull request as draft November 7, 2024 04:12
@ArashPartow ArashPartow force-pushed the arashpartow/update_exprtk_0.0.3 branch from a2c2161 to faba9b9 Compare November 7, 2024 04:19
@ArashPartow ArashPartow marked this pull request as ready for review November 7, 2024 04:52
@ArashPartow ArashPartow force-pushed the arashpartow/update_exprtk_0.0.3 branch 6 times, most recently from baebcb1 to 05e8878 Compare November 11, 2024 22:51
@ArashPartow ArashPartow marked this pull request as draft November 11, 2024 22:58
@ArashPartow
Copy link
Contributor Author

Finally got all the tests passing. Minor issues some of the test results, which revealed a parsing issue in ExprTk.

That being said there is a change I couldn't resolve:

-    struct random : public exprtk::igeneric_function<t_tscalar> {
+    struct random final : public exprtk::igeneric_function<t_tscalar> {
         random();
-        ~random();
+       ~random();

-        t_tscalar operator()(t_parameter_list parameters);
+        t_tscalar operator()(t_parameter_list parameters) override;

         // faster unit lookups, since we are calling this lookup in a tight
         // loop.
-        static std::default_random_engine RANDOM_ENGINE;
-        static std::uniform_real_distribution<double> DISTRIBUTION;
+        std::mt19937 generator;
+        std::uniform_real_distribution<double> distribution{ 0.0, 1.0 };
     };
 -// Set up random number generator
-std::default_random_engine random::RANDOM_ENGINE = std::default_random_engine();
-std::uniform_real_distribution<double> random::DISTRIBUTION =
-    std::uniform_real_distribution<double>(0, 1);
-
-random::random() : exprtk::igeneric_function<t_tscalar>("Z") {}
+random::random()
+: exprtk::igeneric_function<t_tscalar>("Z")
+{
+    std::random_device device;
+    std::array<unsigned int,std::mt19937::state_size> seed;
+    std::generate_n(seed.data(), seed.size(), std::ref(device));
+    std::seed_seq seq(std::begin(seed), std::end(seed));
+    generator.seed(seq);
+}

It seems the use of std::random_device in a WASM/NODERAWFS context is the issue. The implementation of std::random_device is typically done by using /dev/random or /dev/urandom (at least in a posix context). However if those devices are not available, this leads to UB which resulted in spurious crashes during the js tests. I tried several approaches to getting it to work, however was not successful.

The main reason for this particular change was to properly seed the engine, as the way it is currently being setup is not correct. It will use an internal state based on whatever is the memory contents of the allocation. Which could be random or all zeros etc.

@ArashPartow ArashPartow marked this pull request as ready for review November 12, 2024 08:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants