Exclude scalbnf.c from LTO #20215
Exclude scalbnf.c from LTO #20215
Conversation
| @@ -34,6 +34,7 @@ int test_builtins() { | |||
| TEST(fmin) | |||
| TEST(fmod) | |||
| TEST(scalbn) | |||
There was a problem hiding this comment.
scalbn was already in this test it seems, so I was wondering why didn't we see an error before, and looks like we don't run the test in LTO mode. Maybe we should add a variation on the test for that?
There was a problem hiding this comment.
We do run the entire lto2 test suite on the emscripten-releases builder.
But I will add this specific test to the github CI too.
There was a problem hiding this comment.
lto2.test_float_builtins passes fine on main .. so I guess its not enough to just reference scaldnf. This is why the repo cases requires ldexp.. I'm not 100% why this is.. but something to do with the fact the symbol gets indirectly pulled in...
| @@ -34,6 +34,7 @@ int test_builtins() { | |||
| TEST(fmin) | |||
| TEST(fmod) | |||
| TEST(scalbn) | |||
| TEST(ldexp) | |||
There was a problem hiding this comment.
Why add ldexp in this PR specifically?
There was a problem hiding this comment.
Because that is reproducer of this the failure. See #19781 (comment)
|
Note also the reports on #16836 (comment) - there seems to be a number of other symbols which might need similar treatment (from the recent comments, |
Indeed, I just like to find repro cases for them so we have test coverage. If you have a program that fails with any of those that would be very useful. |
Continuation of #15497
Fixes: #19781