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.

Sign up for GitHub

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

Jump to bottom

Makefile.cflags: Build with -Wall -Werror by default (including fixes to correct all existing warnings) #3319

Merged
merged 44 commits into from Jul 14, 2015
Merged

Makefile.cflags: Build with -Wall -Werror by default (including fixes to correct all existing warnings) #3319

merged 44 commits into from Jul 14, 2015

Conversation

jnohlgard
Copy link
Member

Fixes a lot of warnings. This is a stepping stone towards #1121.

make buildtest with CFLAGS += -Wall -Werror (not -Wextra) added to Makefile.include now completes without errors on examples/hello-world, examples/default, examples/rpl_udp, tests/unittests all tests

Worth extra notice:

  • openmote GPIO periph_conf has been redefined to allow all pins as GPIOs (before only the few first were defined)
  • redbee-econotag does not set CFLAGS += -Wextra in the board Makefile anymore
  • msp430 now has a wrapper stdlib.h header for including malloc et al. as expected from <stdlib.h>

@jnohlgard jnohlgard added Platform: MSP Platform: This PR/issue effects MSP-based platforms Platform: ARM Platform: This PR/issue effects ARM-based platforms Area: network Area: Networking Area: tests Area: tests and testing framework Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jul 5, 2015
Kijewski
Kijewski reviewed Jul 5, 2015
View reviewed changes
cpu/cortexm_common/cortexm_init.c
@@ -46,7 +46,7 @@ void cortexm_init(void)
/* set SVC interrupt to same priority as the rest */
NVIC_SetPriority(SVCall_IRQn, CPU_DEFAULT_IRQ_PRIO);
/* initialize all vendor specific interrupts with the same value */
for (int i = 0; i < CPU_IRQ_NUMOF; i++) {
NVIC_SetPriority(i, CPU_DEFAULT_IRQ_PRIO);
for (int i = 0; i < (int) CPU_IRQ_NUMOF; i++) {
Copy link
Contributor

Choose a reason for hiding this comment

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

s/int i/IRQn_Type i/?

Copy link
Member Author

Choose a reason for hiding this comment

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

addressed

@jnohlgard
Copy link
Member Author

Added -Wall -Werror as an experiment, waiting for Travis.

@jnohlgard jnohlgard force-pushed the pr/warning-fixes-3 branch 5 times, most recently from 59e1aae to 414de24 Compare July 6, 2015 23:01
@jnohlgard
Copy link
Member Author

Added a whole bunch of fixes.

Interesting parts:

  • msp430 getchar implementations for all msp430 boards except chronos
  • Blacklist MSP430 boards for libfixmath_unittests because of some missing math.h functions.
  • Blacklist MSP430 from CCN lite examples because the API seem to be designed with the assumption that int is larger than 16 bits wide.
  • Added libfixmath patches for fixing warnings inside the library.

@Kijewski Kijewski added the PR-award-nominee Deprecated. Will be removed soon. label Jul 7, 2015
@jnohlgard jnohlgard changed the title More warning fixes Makefile.cflags: Build with -Wall -Werror by default (including fixes to correct all existing warnings) Jul 7, 2015
@jnohlgard
Copy link
Member Author

there's a problem with the qemu-i386 port and PRId64 being defined as d instead of lld because of a missing declaration of __have_long_long_64. I am guessing that a toolchain update might fix it but I am unfamiliar with the toolchain script.

@OlegHahm
Copy link
Member

OlegHahm commented Jul 7, 2015

Awesome work! Thanks a lot!

I am guessing that a toolchain update might fix it but I am unfamiliar with the toolchain script.

Which toolchain script are you referring to?

@jnohlgard
Copy link
Member Author

@OlegHahm I was referring to the script in dist/tools/toolchain/build_x86.sh.

boards/x86-multiboot-common/Makefile.include downloads a precompiled and patched newlib from http://download.riot-os.org/i586-newlib_2.1.0_tlsf-1254.tar.bz2. I am assuming this newlib was built via the build_x86.sh script.
The build_x86.sh script in turn has a bunch of cryptic patches without explanation what they are for or what they do, see https://github.com/RIOT-OS/RIOT/blob/master/dist/tools/toolchains/build_x86.sh#L30

I think the problem would be solved by an upgrade to newlib-2.2.0, which has some updates for the PRI* macros.

@OlegHahm
Copy link
Member

OlegHahm commented Jul 7, 2015

@Kijewski, can you shed some light on this?

@Kijewski
Copy link
Contributor

Kijewski commented Jul 7, 2015

Yes, the changes are:

  • supplying my own malloc
  • un-inlining (f)putc
  • fixing some minor warnings

@jnohlgard
Copy link
Member Author

nevermind my comment about newlib versions.
The real problem is that boards/x86-multiboot-common/Makefile.include is adding -isystem /usr/lib/gcc/x86_64-pc-linux-gnu/4.9.3/include before the newlib includes, which means that my x86_64 libgcc stdint.h is picked instead of newlib's stdint.h. GCC does not supply the macros __have_long64 etc.

When supplying -ffreestanding as an argument to GCC, the compiler will define __STDC_HOSTED__ as 0 and include its own headers instead of the libc's (newlib's)

What is the correct procedure here? Should newlib's includes be added before GCC's on the command line? Is it possible to build a QEMU kernel without -ffreestanding?

@jnohlgard
Copy link
Member Author

Right after I posted my last comment I found this mailing list post:

http://comments.gmane.org/gmane.comp.lib.newlib/9373

(The change discussed in the post is included in newlib 2.2.0, but not in newlib 2.1.0)

@jnohlgard
Copy link
Member Author

Might need an even newer newlib:
https://sourceware.org/ml/newlib/2015/msg00406.html

@jnohlgard
Copy link
Member Author

Rebased on latest master.

  • x86: Rename stack_t -> stack_info_t because recent newlib defines stack_t inside <sys/signal.h>.

See https://sourceware.org/ml/newlib-cvs/2015-q2/msg00001.html

The qemu-i386 tests work now if I replace the files in toolchain/x86/i586-none-eabi/include by the include files from my installed newlib (from git some weeks old). All PRId64, PRIu32 bugs have been fixed in newlib now.

OlegHahm
OlegHahm reviewed Jul 7, 2015
View reviewed changes
sys/net/include/etx_beaconing.h
@@ -51,13 +50,13 @@ extern "C" {
* Should be divisible through 2 (For ETX_DEF_JIT_CORRECT)
* and 5 (For ETX_MAX_JITTER) unless those values are adjusted too.
*/
#define ETX_INTERVAL (1000)
#define ETX_INTERVAL (1000ul)
Copy link
Member

Choose a reason for hiding this comment

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

Why not replacing with MS_IN_USEC while you're at it anyway?

@jnohlgard
Copy link
Member Author

squashed, waiting for Travis

jnohlgard pushed a commit that referenced this pull request Jul 14, 2015
Merge pull request #3319 from gebart/pr/warning-fixes-3
fa4e059 
Makefile.cflags: Build with -Wall -Werror by default (including fixes to correct all existing warnings)
@jnohlgard jnohlgard merged commit fa4e059 into RIOT-OS:master Jul 14, 2015
@jnohlgard jnohlgard deleted the pr/warning-fixes-3 branch July 14, 2015 06:50
@jnohlgard jnohlgard mentioned this pull request Jul 14, 2015
Closed
4 tasks
@OlegHahm
Copy link
Member

Woohoo!

@thomaseichinger
Copy link
Member

This broke building for OS X and FreeBSD. Again 😢

@thomaseichinger thomaseichinger mentioned this pull request Jul 14, 2015
Merged
@jnohlgard
Copy link
Member Author

This broke building for OS X and FreeBSD. Again 😢

Sorry about that!

@jnohlgard
Copy link
Member Author

@thomaseichinger is FreeBSD and OSX sufficiently similar to assume that any build fixes for FreeBSD usually gets it working on OSX as well?

@thomaseichinger
Copy link
Member

No, sadly not.

@miri64
Copy link
Member

miri64 commented Jul 14, 2015

It's about time, that Millhouse (Mac@FU) and Stedten (FreeBSD@FU) are included in the build tests :D

@thomaseichinger
Copy link
Member

I fear we wont fix this for OS X, as the used ucontext API used by native is marked as deprecated.

@jnohlgard
Copy link
Member Author

@thomaseichinger we could add an ifeq check for OSX in the Makefile and defer fixing this until the next OSX release where the deprecated stuff is removed.

@thomaseichinger
Copy link
Member

I'm on it.

@jnohlgard
Copy link
Member Author

@thomaseichinger does -Wno-error=deprecated get the correct result?

@thomaseichinger
Copy link
Member

@gebart I went with -Wno-deprecated-declarations.

@thomaseichinger thomaseichinger mentioned this pull request Jul 14, 2015
Merged
@miri64 miri64 mentioned this pull request Jul 19, 2015
Closed
@jnohlgard jnohlgard mentioned this pull request Jul 23, 2015
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@OlegHahm OlegHahm

Labels
Area: network Area: Networking Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: ARM Platform: This PR/issue effects ARM-based platforms Platform: MSP Platform: This PR/issue effects MSP-based platforms PR-award-nominee Deprecated. Will be removed soon. Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@jnohlgard @OlegHahm @Kijewski @miri64 @thomaseichinger @kaspar030