Add unit testing of fdlibm

[akosthekiss]

The project is relying on a variant of fdlibm, which has aleady
been edited but never verified for correctness. This patch adds
unit testing of fdlibm by:

• introducing a test generator that uses a trusted libm
  implementation to calculate correct and expected results of math
  functions (tools/gen-test-fdlibm.c),
• adding tests created with the generator that stress all publicly
  exported functions of jerry's fdlibm
  (tests/unit/test-fdlibm.inc.h), and
• adding a unit test file to drive the generated tests
  (tests/unit/test-fdlibm.c).

Note: The test generator is not expected to be executed often, thus
it is not wired into the build system. If it gets edited, it must
be built locally and must be used to re-generate the .inc.h file.

During development, it turned out that tests/unit/test-common.h
included the system header math.h, which was only a bad smell until
now but became a real header conflict issue with the introduction
of the fdlibm unit test. Thus, this patch also changes the include
to fdlibm-math.h.

JerryScript-DCO-1.0-Signed-off-by: Akos Kiss akiss@inf.u-szeged.hu

[galpeter]

IMHO: we should mention here that from which file/script generated this.

[LaszloLango]

Do we really need to add the source code of the generator? There is no make target for it and I think it will be never used again. Can't just simply add generation macros for the test? BTW, the generator should be under tools/unit-test folder and we should add a generate-fdlibm-unit-tests.sh to tools

[LaszloLango]

In general I'd like to see the usage of JERRY_ASSERT like in other unit test files and avoid the usage of printf.

[akosthekiss]

@LaszloLango

Do we really need to add the source code of the generator? [...] I think it will be never used again.

The source code generator might be needed in the future if

• an error is found in fdlibm (e.g., in some corner case) and we'd like to add a regression test for it not to allow creep it back next time, or
• we'd simply like to increase the branch coverage of the existing functions with new test cases, or
• our version of fdlibm is extended to contain more math functions (note that the version we have is a heavily trimmed down one of the original) and we'd like to cover them with tests as well.

There is no make target for it

That was intentional. We shouldn't rebuild the .inc.h. file all the time, only when something changes in the .c. And we certainly shouldn't build the .c with the toolchain we use for building jerry, since jerry might be cross-built but this file is always to be built for and executed on the local developer machine. If someone has knowledge on how to achieve that within the same build system as we use for jerry, do share, please.

Can't just simply add generation macros for the test?

I cannot think of a working approach that way. We'd need to link a trusted libm and fdlibm to the same unit test to have fdlibm's math functions and that libm's math functions called and their results compared.

BTW, the generator should be under tools/unit-test folder

Then, it'd be built with the rest of the unit tests. CMakeLists has file(GLOB SOURCE_UNIT_TEST_MAIN_MODULES tests/unit/*.c). Tried and errored for me. A possible resolution might have been to change the GLOB in CMakeLists, but then I've put it under tools anyway, since other generators reside there as well (e.g., generator.sh and print-unicode-ranges.sh).

(BTW, will print-unicode-ranges.sh be re-executed ever? Considering that it's output has already been manually edited, according to lit-unicode-ranges.inc.h.)

In general I'd like to see the usage of JERRY_ASSERT like in other unit test files

The problem with the assert approach is that it exits on the very first failure and we will have no clue whether that's the only problem with the implementation or there are many more. I've tries that and it's very annoying. By returning a status code at the end, we can see all the failures that we have (at least what's revealed by the test suite).

avoid the usage of printf.

Why is that? test-api.c also uses printf. Seeing the difference (or match) between the computed and expected results is a huge help in debugging. See my reply above to @galpeter . (I'm not completely strong about this, but it's good to debug without the need to edit the test code and only have to focus on the code that has the issue.)

[LaszloLango]

Then, it'd be built with the rest of the unit tests. CMakeLists has file(GLOB SOURCE_UNIT_TEST_MAIN_MODULES tests/unit/*.c).

That is not what I suggested. I suggested tools/unit-test not tests/unit

Why is that?

Consistency. If we mix up everything, then the project would be hard to maintain at some point. Even if we talk about unit tests. I don't like JERRY_ASSERT, but in that case you should provide a better solution for the existing tests first.

[akosthekiss]

@LaszloLango

Consistency. If we mix up everything, then the project would be hard to maintain at some point. Even if we talk about unit tests. I don't like JERRY_ASSERT, but in that case you should provide a better solution for the existing tests first.

The proposed test is not the first to use exit status code to signal error. TEST_INIT in test-common.h exits main with return 1; if something is wrong with /dev/urandom. All but one unit tests (test-date-helpers.c) use it. And test-string-number.c uses exit status code only to signal pass/fail, doesn't use JERRY_ASSERT at all. The proposed test is not the first to use printf either: test-api.c prints quite some info during execution.

So, all the things the proposed test does, are already being done by other tests. I don't see how this breaks consistency. (If someone thinks that unit tests should be unified somehow to be more consistent, then it can be done before this patch or after this patch, but unification and fdlibm testing don't seem to be related to me.)

[akosthekiss]

@galpeter @LaszloLango I've updated the patch. I have to admit, it addresses the previous comments only partially:

• the generator code (gen-test-fdlibm.c) has been moved out from tools and a shell script (gen-test-fdlibm.sh) is added to tools to build and run the generator and put its output to the right place (test-fdlibm.inc.h) -- this indeed makes its use easier,
• the generated file does mention now how it was generated.

What went against the comments:

• now the generator resides in tests/unit: this way it is checked by vera and cppcheck and is also closer to its generated output (might be easier to relate them) -- this needed a slight change in CMakeLists,
• I haven't removed printfs and haven't added asserts (yet?) -- heated conversation about those is still ongoing.

How does it look now?

[LaszloLango]

Thanks for the update, I'd still prefer tools/unit-test/gen-test-fdlibm.c

[akosthekiss]

@LaszloLango updated (couldn't resist to tweak your request a bit though: the new directory is called tools/unt-tests, as plural form is used everywhere else)

[LaszloLango]

@akiss77, thanks. It looks much better.

That was intentional. We shouldn't rebuild the .inc.h. file all the time, only when something changes in the .c. And we certainly shouldn't build the .c with the toolchain we use for building jerry, since jerry might be cross-built but this file is always to be built for and executed on the local developer machine. If someone has knowledge on how to achieve that within the same build system as we use for jerry, do share, please.

Now we have a bash script that can build and generate tests/unit/test-fdlibm.inc.h. What if we call the script in test-unit target of Makefile? I think that will always call the system gcc, but may have problems if there is no gcc. :(

[LaszloLango]

LGTM

[akosthekiss]

@LaszloLango While waiting for a second approval, I've been thinking about your last comment and might have come up with a variant that addresses some issues you've raised. I've added a Makefile to tools/unit-tests, which builds and cleans the generator, and made the bash script in tools call make (not gcc directly, as before).

Moreover, the Makefile uses standard variables to specify the compiler and the reference math lib (CC=gcc and LDFLAGS=-lm), so should a project maintainer ever need to work on a machine that does not have gcc and/or use something different from libm, then the override of Makefile's defaults will be as easy as setting the environment variables with the same name.

The PR is now updated and implements the above approach.

[zherczeg]

Do I understand correctly that these values are just hand picked values? Or do they come from an available corner case testing environment?

[akosthekiss]

Neither-nor. At least, not exactly. I've been looking around for existing libm test suites but only found http://www.opensource.apple.com/source/Libm/Libm-40.11/noship.subproj/libm-test.c . It seemed to be shallow, however.

But it motivated me to test the well-known values (+-0.0, +-INF, NAN everywhere; PI, PI/2, PI/4, PI/6, ... angles for trigonometric functions; +-0.5, +-1.0 for arcus functions). And then I looked at fdlibm sources -- both the code and the doc comments -- and analyzed how the input domains are split up to ranges. I tried hard to add test inputs around both lower and upper bounds of all ranges. (E.g., acos is implemented to split the input domain at 0.5 -- among others --, thus I've added test inputs 0.4 and 0.6 to make sure that both ranges are tested.)

So, the values are manually chosen to test both normal and corner cases of fdlibm.

[zherczeg]

LGTM

[LaszloLango]

@akiss77, still LGTM.