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

tests/libc_newlib: add test for newlib-nano inclusion #9599

Merged

Conversation

cladmi
Copy link
Contributor

@cladmi cladmi commented Jul 18, 2018

Contribution description

This test verifies that when used, newlib-nano is correctly included, both
header and also linked.

This is a standalone test and is less invasive than doing a global test like what I tried in #9512

I want to use it to debug non handled developers computers.

Testing

I tested the following setups:

  • native (no newlib)
  • pic32-wifire (newlib) using the flasher PR
  • samr21-xpro (newlib-nano)

Issues/PRs references

#9512

This should not be merged before #9398 as currently llvm is not handled properly but it should not block adding tests in murdock.
I now think it should be included before #9398, I thought it would be all fixed faster.
I already have a fix for the llvm/clang issue #9513

@cladmi cladmi added Area: tests Area: tests and testing framework State: waiting for other PR State: The PR requires another PR to be merged first CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: run tests If set, CI server will run tests on hardware 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 Jul 18, 2018
@cladmi cladmi force-pushed the pr/make/newlib/add_test_for_newlibnano branch from d2cd568 to a23078e Compare July 19, 2018 12:55
@cladmi cladmi added the Area: toolchain Area: toolchains; everything related to compilation, libc, linking, … label Jul 19, 2018
@miri64
Copy link
Member

miri64 commented Jul 19, 2018

Do we really need to run tests on this PR everytime ;-)?

@miri64 miri64 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: run tests If set, CI server will run tests on hardware for the labeled PR CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jul 19, 2018
@cladmi
Copy link
Contributor Author

cladmi commented Jul 19, 2018

I was still getting a redis error from the build output.

Copy link
Contributor

@jcarrano jcarrano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good. I can pass the test, but I can't fail it (Arch linux).

About
=====

Terifies if newlib/newlib-nano is correctly included by the build system
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you include instructions on how to force a failure of this test? Then people can test the test.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The link time test (currently done at runtime) can be tested by commenting this line:

export LINKFLAGS += -specs=nano.specs

And the compile time test can be tested by commenting this one:

INCLUDES := -isystem $(NEWLIB_NANO_INCLUDE_DIR) $(INCLUDES)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested both tests and it failed as expected.

#ifdef MODULE_NEWLIB
#ifdef MODULE_NEWLIB_NANO
/* Nano maps iprintf to printf */
TEST_ASSERT(iprintf == printf);
Copy link
Contributor

@jcarrano jcarrano Jul 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be tested by binutils, so there's no need to run the application.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will try something for this.

@jcarrano
Copy link
Contributor

My test results (BOARD=stm32f4discovery, Arch Linux):

2018-07-23 15:39:23,382 - INFO # main(): This is RIOT! (Version: 2018.10-devel-27-ga2307-batman-test_newlibnano)
2018-07-23 15:39:23,383 - INFO # Newlib/nano test
2018-07-23 15:39:23,383 - INFO # .
2018-07-23 15:39:23,383 - INFO # OK (1 tests)

System

Installed compiler toolchains 
-----------------------------
             native gcc: gcc (GCC) 8.1.1 20180531
      arm-none-eabi-gcc: arm-none-eabi-gcc (Arch Repository) 8.1.0
                avr-gcc: missing
       mips-mti-elf-gcc: missing
             msp430-gcc: missing
   riscv-none-embed-gcc: missing
                  clang: clang version 6.0.1 (tags/RELEASE_601/final)

Installed compiler libs
-----------------------
   arm-none-eabi-newlib: "3.0.0"
    mips-mti-elf-newlib: missing
riscv-none-embed-newlib: missing
               avr-libc: missing (missing)

Installed development tools
---------------------------
                  cmake: cmake version 3.11.4
               cppcheck: Cppcheck 1.84
                doxygen: 1.8.14
                 flake8: 3.5.0 (mccabe: 0.6.1, pycodestyle: 2.4.0, pyflakes: 2.0.0) CPython 3.6.6 on Linux
                    git: git version 2.18.0
             coccinelle: missing

@jcarrano
Copy link
Contributor

Open question: How can we make it so that we force the user to run the test?

@cladmi
Copy link
Contributor Author

cladmi commented Jul 23, 2018

I added a test in the Makefile. Currently not documented but I should move the documentation out of main.c to the makefile.

@cladmi
Copy link
Contributor Author

cladmi commented Jul 23, 2018

I also tested wih TOOLCHAIN=llvm it currently fails during compilation because this is not fixed #9513.
I commented some parts to be able to run the test with llvm-nm at it also worked.

@cladmi cladmi added State: waiting for other PR State: The PR requires another PR to be merged first and removed State: waiting for other PR State: The PR requires another PR to be merged first labels Jul 23, 2018
@cladmi
Copy link
Contributor Author

cladmi commented Jul 23, 2018

I split the NM change in a separate PR.

@jcarrano Tell me if you like the new tests. I will update the documentation after

@miri64 I now think it should be included before #9398, I thought it would be all fixed faster.
I already have a fix for the llvm/clang issue #9513. What do you think ?
I would really like to go forward for fixing newlib for the release.

endif
endif

test-newlib: $(ELFFILE)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A dumber approach without sed (checks the number of different addresses)

# make N_PRINTFS = 2 if non-nano, 1 if nano

test-newlib: $(ELFFILE)
   PRINTFS=$$($(NM) -v $^ | grep '\( i\?printf\)'); \
   PRINTF_ADDRS=$$(echo "$${PRINTFS}" | cut -d' ' -f 1 | uniq | wc -l); \
   # ensure both symbols are there
   if test $$(echo "$${PRINTFS}" | wc -l) = 2 ; then \
       echo "[FAILED] Expected printf and iprintf to be present, found $${PRINTFS}" ; \
       exit 1; \
   elif test $${PRINTF_ADDRS} != $(N_PRINTFS) ; then \ 
       echo "[FAILED] Expected $(N_PRINTFS) different printfs"; \
       exit 1; \
   fi

BTW, could we define a standard set of shell functions to be included in all makefiles. I would like to have a 'die' function:

die() {
    echo "${1}" >&2
    exit 1
}
# one-line "do stuff and exit with error if it fails"
test -e fileXX.txt || die "file not found"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using more sub commands, and not simpler for me to understand as it uses two concepts, number of printfs + addresses instead of just comparing addresses.

sed is standard enough and is working properly.

Adding common functions makefile would be a different PR with its own problem.

Also for me echo; exit can be understood in one blink and not complex enough that I would like it replaced by another function that I would need to check. Also it can be done with oneliner as

test -e fileXX.txt || { echo "file not found"; exit 1};

@cladmi cladmi force-pushed the pr/make/newlib/add_test_for_newlibnano branch from 207e634 to 3e29c10 Compare July 24, 2018 11:51
@jcarrano
Copy link
Contributor

@cladmi The nm test is good.

I now think it should be included before #9398.

I think this PR should have a high priority, the newlib thing is a bug waiting to happen.

Copy link
Contributor

@jcarrano jcarrano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good. We need another PR to make this test compulsory.

This test verifies that when used, newlib-nano is correctly included, both
header and also linked.
@cladmi cladmi force-pushed the pr/make/newlib/add_test_for_newlibnano branch from 0a7f6a1 to db00922 Compare July 25, 2018 09:43
@cladmi cladmi added Process: needs backport Integration Process: The PR is required to be backported to a release or feature branch and removed State: waiting for other PR State: The PR requires another PR to be merged first labels Jul 25, 2018
@cladmi
Copy link
Contributor Author

cladmi commented Jul 25, 2018

Reminder, for backporting I would also need to add the NM changes.

@cladmi 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 Jul 25, 2018
@jcarrano jcarrano merged commit 0a53b61 into RIOT-OS:master Jul 25, 2018
@cladmi
Copy link
Contributor Author

cladmi commented Jul 25, 2018

Backport provided in #9633

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: tests Area: tests and testing framework Area: toolchain Area: toolchains; everything related to compilation, libc, linking, … CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Process: needs backport Integration Process: The PR is required to be backported to a release or feature branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants