tests/libc_newlib: fix pointers comparison for llvm by cladmi · Pull Request #10014 · RIOT-OS/RIOT

cladmi · 2018-09-21T17:01:00Z

Contribution description

With llvm and samr21-xpro, I could not directly do 'printf == iprintf'.
And I could not cast them to an integer value in a portable way.
So I use real function pointers for this.
However comparing printf_addr with iprintf_addr does not work if
there is not the memcmp to somehow 'force them' to be pointers...
So I used the 'memcmp' for the comparison.

Testing procedure

Run make clean flash test BOARD=samr21-xpro TOOLCHAIN=llvm and it should work.

I tested with pic32-wifire and also tested to disable newlib-nano or nano.specs for the samr21-xpro to test the error cases.

Issues/PRs references

Found by #9809

cladmi · 2018-09-21T17:04:25Z

Maybe not being able to to the comparison is a real problem, but I cannot really say. Let's already see the result of murdock in #9809

kaspar030 · 2018-09-21T20:44:56Z

This is indeed weird. The assembly looks like clang decides at compile time that the comparison is wrong.

Replacing the comparison with TEST_ASSERT((printf - iprintf) == 0); also fixes the issue.

kaspar030 · 2018-09-24T09:25:43Z

@cladmi here's the diff against master for the shorter fix: https://gist.github.com/1acb9d37be0ef35057309a6ee2d4e5da

cladmi · 2018-09-24T10:43:01Z

Replacing the comparison with TEST_ASSERT((printf - iprintf) == 0); also fixes the issue.

Nice, I will replace to use this.

miri64

Except for static tests, Murdock is happy with this PR here and and when run with #9809.

I also ran the test on both native and samr21-xpro, both compiled with either GCC and clang.

I have however some comments regarding the output messages.

You may squash when addressed (and if you like mark @kaspar030 as a co-author).

miri64 · 2018-09-25T07:37:42Z

tests/libc_newlib/main.c

 #ifdef MODULE_NEWLIB_NANO
    /* Nano maps iprintf to printf */
-    TEST_ASSERT(iprintf == printf);
+    TEST_ASSERT_MESSAGE((printf - iprintf) == 0, "iprintf == printf");


I think this output might be confusing. The message is given when iprintf != printf, so this is what should be printed.

Hm, intuitively I agreed, but a plain TEST_ASSERT(<expression>) also prints just the failed expression, not the opposite. So all the TEST_ASSERT* have an implicit assertion "<msg>" failed.

True, but with e.g. TEST_ASSERT_EQUAL_INT it tells you "This is not as expected, this is what I expect" (faked with changing an expected value in tests-core:

....................................... core_cib_tests.test_cib_get__overflow (tests/unittests/tests-core/tests-core-cib.c 48) exp 4 was 3 .................................................... run 91 failures 1

True. Thing is, we have

TEST_ASSERT(1==0) -> core_cib_tests.test_cib_get__overflow (tests/unittests/tests-core/tests-core-cib.c 48) 1==0,

but then want to tweak a custom message to essentially say the opposite, e.g.,

TEST_ASSERT_MESSAGE(1==0, "1 != 0") -> core_cib_tests.test_cib_get__overflow (tests/unittests/tests-core/tests-core-cib.c 48) 1 != 0?

(do I make sense? 😉)

(do I make sense? 😉)

Yes, at least I see (from the beginning btw) where you are coming from. I'm just arguing that for TEST_ASSERT() we can't really change what is happening. The CPP is just printing #exp, because it's the sane (and most efficient) thing to do. However, for TEST_ASSERT_MESSAGE we have more freedom, so we should use it and print a more intuitive output. But in the end it is purely based on taste of how to do it IMHO, so let's @cladmi decide, what he thinks is best for his test ;-).

I prefer keeping the "default" embunit behavior, show the condition that failed in the message on assert.

In master with the error I had:

018-09-25 14:45:58,501 - INFO # main(): This is RIOT! (Version: 2018.10-devel-855-g028bc-hal-HEAD) 2018-09-25 14:45:58,503 - INFO # Newlib/nano test 2018-09-25 14:45:58,503 - INFO # . 2018-09-25 14:45:58,509 - INFO # tests.test_newlib (tests/libc_newlib/main.c 66) iprintf == printf 2018-09-25 14:45:58,509 - INFO # 2018-09-25 14:45:58,511 - INFO # run 1 failures 1

And I only changed the condition to (iprintf- printf)==0 because it made it work for llvm but wanted to keep the same message.
With only TEST_ASSERT we would also have (iprintf - printf) == 0 so for me would feel strange to do the opposite of the normal behavior.

miri64 · 2018-09-25T07:37:50Z

tests/libc_newlib/main.c

 #else
    /* Normal newlib does not */
-    TEST_ASSERT(iprintf != printf);
+    TEST_ASSERT_MESSAGE((printf - iprintf) != 0, "iprintf != printf");


Dito (just with iprintf == printf)

@cladmi

See #10014 (comment): "[…] in the end it is purely based on taste of how to do it IMHO, so let's @cladmi decide, what he thinks is best for his test ;-)."

cladmi · 2018-09-25T12:51:20Z

@miri64 thanks for the co-author trick, I will do it.

miri64

ACK

miri64 · 2018-09-25T12:54:28Z

(please squash ;-))

With llvm and samr21-xpro, I could not directly do 'printf == iprintf'. But doing `(printf - iprintf) == 0` correctly checked if they are equal. Co-authored-by: Kaspar Schleiser <kaspar@schleiser.de>

cladmi · 2018-09-25T13:06:56Z

Squashed now waiting for the "CI: run tests".

cladmi mentioned this pull request Sep 21, 2018

murdock: add selected cortex-m boards to LLVM build #9809

Merged

cladmi added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Sep 21, 2018

RIOT-OS deleted a comment Sep 21, 2018

miri64 mentioned this pull request Sep 25, 2018

tests/gnrc_sixlowpan: fix stacksize of dummy device #10024

Merged

miri64 previously requested changes Sep 25, 2018

View reviewed changes

miri64 approved these changes Sep 25, 2018

View reviewed changes

tests/libc_newlib: fix pointers comparison for llvm

ba2a8df

With llvm and samr21-xpro, I could not directly do 'printf == iprintf'. But doing `(printf - iprintf) == 0` correctly checked if they are equal. Co-authored-by: Kaspar Schleiser <kaspar@schleiser.de>

cladmi force-pushed the pr/tests_libc/fix_for_llvm branch from 3e62363 to ba2a8df Compare September 25, 2018 13:03

miri64 merged commit f1529b8 into RIOT-OS:master Sep 25, 2018

cladmi deleted the pr/tests_libc/fix_for_llvm branch September 25, 2018 17:24

jia200x added this to the Release 2018.10 milestone Oct 4, 2018

Conversation

cladmi commented Sep 21, 2018

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

cladmi commented Sep 21, 2018

Uh oh!

kaspar030 commented Sep 21, 2018

Uh oh!

kaspar030 commented Sep 24, 2018

Uh oh!

cladmi commented Sep 24, 2018

Uh oh!

miri64 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cladmi commented Sep 25, 2018

Uh oh!

miri64 left a comment

Choose a reason for hiding this comment

Uh oh!

miri64 commented Sep 25, 2018

Uh oh!

cladmi commented Sep 25, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

miri64 left a comment •

edited

Loading