newlib-nano: fix include directory by toonst · Pull Request #9216 · RIOT-OS/RIOT

toonst · 2018-05-28T15:23:18Z

Contribution description

Adding the nano.specs file to CFLAGS make sure the correct include directory is found. This way we don't have to explicitly add the include directory.

Issues/PRs references

Fixes #9214
Fixes PR #9080
Alternative to #9215

jnohlgard

Did you test this on clang as well? IIRC we needed some extra handling for that

jcarrano · 2018-05-28T15:38:26Z

Works for me on Arch. I'm closing #9215 as it is hacky.

toonst · 2018-05-28T16:06:42Z

@gebart No I did not. And you're right, it needs some extra handling. I'm looking into it.

jcarrano · 2018-05-29T08:56:52Z

@toonst Is it acceptable to fall back to what your previous PR (#9080 ) did if the toolchain is clang? And in that case, could we force a failure if nano's headers are not found, so that the error becomes more evident?

pwillem · 2018-05-29T10:06:37Z

FYI I think this only works if your newlib version >= 2.8.0 (because of https://sourceware.org/git/gitweb.cgi?p=newlib-cygwin.git;a=commit;h=be5926babbfb23ded1de2181a7524b8c3eebe34c )

smlng · 2018-05-29T15:06:39Z

works on macOS

kYc0o · 2018-05-29T20:28:42Z

CCN package is broken with this PR... I'd suggest to revert to the original state and then try another option for this?

smlng · 2018-05-30T08:06:41Z

can we get this merged, need this to compile arm based boards on macOS!!

kYc0o · 2018-05-30T08:16:48Z

@smlng have you seen Murdock's output?

smlng · 2018-05-30T08:19:51Z

ah damn, but its only ccn-lite 😬 otherwise we should think about reverting #9080?

smlng · 2018-05-30T08:29:45Z

so its a problem with all external packages?

toonst · 2018-05-30T08:50:27Z

Sorry it took this long.
I made some extra checks for the existence of a newlib.h file and error when trying to compile for native. I'm looking at getting newlib to work on linux, so if that is working we can remove the check.
Could you test if this is working properly on OSX? Suggestions welcome!

smlng · 2018-05-30T08:54:32Z

still works on macOS but also still get error when compiling ccn-lite-relay for any arm board.

jcarrano · 2018-05-30T09:28:12Z

@smlng @kYc0o What happens with CCN is that the CFLAGS get passed twice to the compiler.

kYc0o · 2018-05-30T09:33:38Z

I see, then it needs a check in the pkg makefile. @toonst do you mind to provide a patch for that also? Otherwise this will be broken and cannot be merged... It can also come in a separated PR, I can do it later if needed.

toonst · 2018-05-30T09:57:30Z

@kYc0o I don't know cmake, but if I just remove the RIOT CFLAGS it seems to be fine. You think this is OK?

smlng · 2018-05-30T10:25:47Z

@toonst note that it is not just ccn-lite, jerryscript, relic are not working, too. It seems that all apps that use external third party package won't compile, bc the CFLAGS are passed 2 times which causes the error.

smlng · 2018-05-30T10:27:34Z

also note new error reported by murdock https://ci.riot-os.org/RIOT-OS/RIOT/9216/20cf3fd8064cbcb7f7725999d5fe6cced27d7215/output/compile/tests/msg_avail/pic32-wifire.txt

jcarrano · 2018-05-30T10:40:56Z

@smlng @toonst What if -specs is made part of the CC variable instead of a CFLAG? In a way it mitght even make more sense, because even if packages could override CFLAGS, the specs flag defines lower level compilation and linking details which are not supposed to change. It would also make it easier with clang, since specs is not a valid flag for clang.

kYc0o · 2018-05-30T13:59:15Z

@toonst have you tried what @jcarrano suggests? The packages still failing 🙁

toonst · 2018-05-30T14:47:27Z

@jcarrano @kYc0o adding the specs flag to CC doesn't work for cmake:

  The CMAKE_C_COMPILER:
    arm-none-eabi-gcc -specs=nano.specs
  is not a full path and was not found in the PATH.

kYc0o · 2018-05-30T15:16:19Z

I see... What do you think then on reverting #9080 while we find a proper solution?

stddef.h and stdarg.h are not supplied by newlib, but by the compiler, so we need the standard include directories to find them.

* remove check for native board * add Werror so using specs fails for clang * remove useless comments

tcschmidt · 2018-06-13T08:32:11Z

Do all tests complete now? Are we ready to finish the review?

jnohlgard · 2018-06-13T14:59:51Z

makefiles/libc/newlib.mk

+    NEWLIB_NANO_INCLUDE_DIR ?= $(firstword $(wildcard $(addprefix $(NEWLIB_INCLUDE_DIR)/, newlib-nano nano)))
+    ifneq ($(NEWLIB_NANO_INCLUDE_DIR),)
+      # newlib-nano overrides newlib.h and its include dir should therefore go before
+      # the regular system include dirs, but after the other newlib includes.


Still not seeing why the nano include dir should go after the newlib include. newlib-nano overrides the newlib.h and therefore should go before newlib include or otherwise we will get the newlib.h from the regular newlib include directory.

You're right, I missed your first comment on it. Fixed!

jnohlgard · 2018-06-13T15:02:00Z

pkg/ccn-lite/Makefile

 .PHONY: all

-export RIOT_CFLAGS = $(CFLAGS) $(INCLUDES)
+RIOT_CFLAGS := $(INCLUDES)


Does this work? Isn't $(INCLUDES) empty at this point? Should this be = instead of :=?

As it is called with make -C pkg/PKGNAME the variable was already set from environment variable.
However, there is not reasons to set it with := either.

we don't need to search for newlib include dirs manually when using the nano.specs file

smlng · 2018-06-15T05:49:17Z

tests/rmutex/Makefile

 include ../Makefile.tests_common

+# MIPS based boards are black listed due to some issue with newlib and _Atomic
+BOARD_BLACKLIST := mips-malta pic32-clicker pic32-wifire


note: this should be removed, if someone has solution for the MIPS toolchain. It seems that some include paths are mixed up and the mips-gcc cannot find some system header files?!

The difference between master and this pr with make info-build:

diff -u info-build-master info-build-pr --- info-build-master 2018-06-19 13:47:58.983288813 +0200 +++ info-build-pr 2018-06-19 13:48:08.447320615 +0200 @@ -35,6 +35,8 @@ FEATURES_CONFLICT_MSG: INCLUDES: + -isystem + /opt/mips-mti-elf/2016.05-03/mips-mti-elf/include -I/home/harter/work/git/RIOT/core/include -I/home/harter/work/git/RIOT/drivers/include -I/home/harter/work/git/RIOT/sys/include

I also get warnings two times when doing info-build:

make -C tests/rmutex BOARD=pic32-wifire info-build > info-build-pr /home/harter/work/git/RIOT/makefiles/libc/newlib.mk:51: newlib.h not found, guessing newlib include path from gcc path /home/harter/work/git/RIOT/makefiles/libc/newlib.mk:51: newlib.h not found, guessing newlib include path from gcc path

Where is your newlib installation located? (newlib.h)
What distro?
Edit: never mind, I didn't see the context was mips-mti-elf

Found that the two times warning comes from $(MAKE) info-boards-supported being called during make info-build.

I use ubuntu 16.04 and I installed the mips-mti-elf as explained in the Wiki I think:

$ /opt/mips-mti-elf/2016.05-03/bin/mips-mti-elf-gcc --version mips-mti-elf-gcc (Codescape GNU Tools 2016.05-03 for MIPS MTI Bare Metal) 4.9.2 Copyright (C) 2014 Free Software Foundation, Inc. This is free software; see the source for copying conditions. There is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

And I have a only this newlib.h in mips-mti-elf directory, no newlib-nano directory:

/opt/mips-mti-elf/2016.05-03/mips-mti-elf/include/newlib.h

When compiling, I have errors in stdatomic.h and with the new change that puts -isystem ABC it looks like it is another stdatomic.h that is used:

I have these two:

/opt/mips-mti-elf/2016.05-03/mips-mti-elf/include/stdatomic.h /opt/mips-mti-elf/2016.05-03/lib/gcc/mips-mti-elf/4.9.2/include/stdatomic.h

cladmi · 2018-06-19T09:43:04Z

makefiles/libc/newlib.mk

-  /usr/$(TARGET_ARCH)/include \
-  /usr/local/opt/$(TARGET_ARCH)*/$(TARGET_ARCH)/include \
-  /usr/local/opt/gcc-*/$(TARGET_ARCH)/include \
-  #


The comment for the backslash terminated line was removed.

cladmi · 2018-06-19T09:46:56Z

makefiles/libc/newlib.mk

+      # newlib-nano overrides newlib.h and its include dir
+      # should therefore go before the other newlib includes.
+      NEWLIB_INCLUDES := -isystem $(NEWLIB_NANO_INCLUDE_DIR) $(NEWLIB_INCLUDES)
+    endif


What happens if there is no newlib-nano ?
Is it ok to ignore this as it was right now ?

In theory, code that depends on having newlib-nano should test for the appropriate macros, so that compilation fails right away instead if causing weird errors.

From reading around, the difference between newlib and newlib-nano, should, according to https://community.nxp.com/thread/389086, almost only be code size, so with how things are usually used, no API dependency.
So there should not be any "dependency on newlib-nano" that should be tested in the code.
I would rely on the build system to report it.

However, my knowledge on the topic is limited.

The size and layout of some internal newlib variables and structs are affected, for example sizeof(struct reent). Using the newlib.h from normal newlib when linking against newlib nano will probably work without any errors, but waste RAM. The other way around will result in buffer overflow bugs that will be difficult to debug.

@jcarrano Do you have some macros that could be checked to verify if newlib-nano is used ? I would like to verify that right now, some configuration are already broken and not using newlib-nano when requested.

@cladmi check _WANT_REENT_SMALL

cladmi · 2018-06-19T12:51:41Z

makefiles/libc/newlib.mk

-  # but Clang, when cross-compiling, needs to be told where to find the headers
-  # for the system being built.
-  # We also add -nostdinc to avoid including the host system headers by mistake
-  # in case some header is missing from the cross tool chain


This ifeq section with the gcc case was removed and it adds a wrong -isystem directory for mips-gcc 4.9.2 on my computer.

It supplies /opt/mips-mti-elf/2016.05-03/mips-mti-elf/include/ which replaces /opt/mips-mti-elf/2016.05-03/lib/gcc/mips-mti-elf/4.9.2/include/

Which breaks compilation for mips in the rmutex example.

However, re-adding the ifeq would only solve my case but maybe not llvm as it would be using the wrong directory.

Can you try if it works with the latest additions?

cladmi

I feel this is going a bit too far for a single PR and it starts having side effects. This makes it hard for me to get forward in reviewing everything.

Adding only the nano.specswhen supported in a separate PR would be already one step. Fixing the whole newlib include handling when nano.specs is not used and with llvm is another task would help the review.

I can do the split PR fixing the packages this afternoon.

smlng · 2018-06-22T07:31:25Z

this fixes (all) my issues on macOS with newlib, right now (2018-06-22) master is broken there

smlng · 2018-06-22T07:32:48Z

however, I also agree with @cladmi that we should introduces changes/fixes step-by-step, ie. nano-spec, include-dir, and llvm support.

also * give error when no newlib directory is found * add search for newlib/nano directory original author: cgundogan

toonst · 2018-06-26T15:36:29Z

Closed in favor of #9394 and #9415

toonst mentioned this pull request May 28, 2018

newlib-nano: add include directory #9080

Merged

jnohlgard reviewed May 28, 2018

View reviewed changes

jcarrano mentioned this pull request May 28, 2018

newlib-nano: Add workaround for systems without nano includes. #9215

Closed

toonst force-pushed the pr/newlib-nano_incl_dir branch from 4c5967a to 77e5081 Compare May 30, 2018 08:40

toonst force-pushed the pr/newlib-nano_incl_dir branch from 20cf3fd to 2c92bff Compare May 30, 2018 12:26

Toon Stegen and others added 5 commits June 5, 2018 15:25

newlib: allow including of standard include dirs

7eb0744

stddef.h and stdarg.h are not supplied by newlib, but by the compiler, so we need the standard include directories to find them.

newlib: only add nano include dir when it exists

77c2f26

pkg/ccn-lite: remove duplicate CFLAGS definition

ea13db8

pkg/jerryscript: remove duplicate CFLAGS definition

95c5871

tests/rmutex: blacklist MIPS-based boards

e751ac3

toonst force-pushed the pr/newlib-nano_incl_dir branch from 6d567a2 to e751ac3 Compare June 12, 2018 15:47

newlib: address comments by Joakim

7b62a8a

* remove check for native board * add Werror so using specs fails for clang * remove useless comments

jnohlgard reviewed Jun 13, 2018

View reviewed changes

jnohlgard requested changes Jun 13, 2018

View reviewed changes

Toon Stegen added 2 commits June 14, 2018 21:57

newlib: newlib-nano includes before regular newlib

7f4522d

newlib: simplify logic

1b3fa16

we don't need to search for newlib include dirs manually when using the nano.specs file

smlng reviewed Jun 15, 2018

View reviewed changes

nmeum mentioned this pull request Jun 18, 2018

makefiles/cflags.inc.mk: add -Wmissing-include-dirs flag #9243

Merged

cladmi reviewed Jun 19, 2018

View reviewed changes

newlib: fix comment for backslash terminated line

ec31eaa

cladmi reviewed Jun 19, 2018

View reviewed changes

cladmi mentioned this pull request Jun 20, 2018

newlib+llvm: Use --sysroot instead of -nostdinc #6671

Closed

cladmi suggested changes Jun 20, 2018

View reviewed changes

cgundogan mentioned this pull request Jun 20, 2018

newlib-nano: NEWLIB_INCLUDE_DIR set incorrectly #9381

Closed

This was referenced Jun 20, 2018

pkgs/cmake: fix CFLAGS given two times #9383

Merged

newlib.mk: replace the error by a warning on nano header not found #9384

Closed

smlng mentioned this pull request Jun 25, 2018

newlib: include fixes for newlib-nano #9394

Merged

Toon Stegen added 2 commits June 26, 2018 14:23

newlib: search in standard compiler include paths

cad68c0

also * give error when no newlib directory is found * add search for newlib/nano directory original author: cgundogan

newlib: add warning for missing newlib nano dir

db649c6

toonst mentioned this pull request Jun 26, 2018

newlib: support nano specs #9415

Closed

toonst closed this Jun 26, 2018

Conversation

toonst commented May 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Issues/PRs references

Uh oh!

jnohlgard left a comment

Choose a reason for hiding this comment

Uh oh!

jcarrano commented May 28, 2018

Uh oh!

toonst commented May 28, 2018

Uh oh!

jcarrano commented May 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pwillem commented May 29, 2018

Uh oh!

smlng commented May 29, 2018

Uh oh!

kYc0o commented May 29, 2018

Uh oh!

smlng commented May 30, 2018

Uh oh!

kYc0o commented May 30, 2018

Uh oh!

smlng commented May 30, 2018

Uh oh!

smlng commented May 30, 2018

Uh oh!

toonst commented May 30, 2018

Uh oh!

smlng commented May 30, 2018

Uh oh!

jcarrano commented May 30, 2018

Uh oh!

kYc0o commented May 30, 2018

Uh oh!

toonst commented May 30, 2018

Uh oh!

smlng commented May 30, 2018

Uh oh!

smlng commented May 30, 2018

Uh oh!

jcarrano commented May 30, 2018

Uh oh!

kYc0o commented May 30, 2018

Uh oh!

toonst commented May 30, 2018

Uh oh!

kYc0o commented May 30, 2018

Uh oh!

tcschmidt commented Jun 13, 2018

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!

jnohlgard Jun 19, 2018 • 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!

toonst commented May 28, 2018 •

edited

Loading

jcarrano commented May 29, 2018 •

edited

Loading

jnohlgard Jun 19, 2018 •

edited

Loading

cladmi Jun 19, 2018 •

edited

Loading