Skip to content

make: Makefile.include: Remove -Wno-implicit-fallthrough#9362

Merged
kaspar030 merged 3 commits intoRIOT-OS:masterfrom
jnohlgard:pr/Wimplicit-fallthrough
Aug 14, 2018
Merged

make: Makefile.include: Remove -Wno-implicit-fallthrough#9362
kaspar030 merged 3 commits intoRIOT-OS:masterfrom
jnohlgard:pr/Wimplicit-fallthrough

Conversation

@jnohlgard
Copy link
Copy Markdown
Member

Contribution description

These flags should hopefully not be necessary now.

Issues/PRs references

Introduced in #8603, #8617

@jnohlgard jnohlgard added Area: build system Area: Build system 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 Jun 14, 2018
@jnohlgard jnohlgard added this to the Release 2018.07 milestone Jun 14, 2018
@jnohlgard jnohlgard requested review from jia200x and kaspar030 June 14, 2018 23:16
@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 Jun 19, 2018
@cladmi
Copy link
Copy Markdown
Contributor

cladmi commented Jun 19, 2018

It is strange, the errors are places with "falls through" statements.
Also I have the same arm-none-eabi-gcc version as said in static-test and I do not get errors on my computer.

Could some workers have a different version than the one running static-test ? @smlng

If not, it maybe requires a more safe proof falls through comment.

@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 5, 2018
@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 17, 2018
@cladmi
Copy link
Copy Markdown
Contributor

cladmi commented Jul 17, 2018

I really do not get why it is not building… I do not get any error on my computer.

@jnohlgard
Copy link
Copy Markdown
Member Author

Same for me. Maybe some CI worker is using a different version of the toolchain? Is that possible to check?

@kaspar030
Copy link
Copy Markdown
Contributor

This is for mobi3 (failed asymcute_mqttsn on acd52832):

[kaspar@ng riot (master)]$ dwqc -q mobi3.inet.haw-hamburg.de './dist/tools/ci/print_toolchain_versions.sh'                           
Installed compiler toolchains
-----------------------------
             native gcc: gcc (Ubuntu 5.4.0-6ubuntu1~16.04.9) 5.4.0 20160609                                                          
      arm-none-eabi-gcc: arm-none-eabi-gcc (GNU Tools for Arm Embedded Processors 7-2017-q4-major) 7.2.1 20170904 (release) [ARM/embedded-7-branch revision 255204]
                avr-gcc: avr-gcc (GCC) 4.9.2
       mips-mti-elf-gcc: mips-mti-elf-gcc (Codescape GNU Tools 2016.05-03 for MIPS MTI Bare Metal) 4.9.2                             
             msp430-gcc: msp430-gcc (GCC) 4.6.3 20120301 (mspgcc LTS 20120406 unpatched)                                             
   riscv-none-embed-gcc: riscv-none-embed-gcc (GNU MCU Eclipse RISC-V Embedded GCC, 64-bits) 7.2.0                                   
                  clang: clang version 3.8.0-2ubuntu4 (tags/RELEASE_380/final)                                                       

Installed compiler libs
-----------------------
   arm-none-eabi-newlib: "2.5.0"
    mips-mti-elf-newlib: "2.1.0"
riscv-none-embed-newlib: "2.5.0"
               avr-libc: "1.8.0svn" ("20111229")

Installed development tools
---------------------------
                  cmake: cmake version 3.10.0
               cppcheck: Cppcheck 1.72
                doxygen: 1.8.11
                 flake8: missing
                    git: git version 2.7.4
             coccinelle: spatch version 1.0.4 with Python support and with PCRE support                                              
[kaspar@ng riot (master)]$ 

@kaspar030
Copy link
Copy Markdown
Contributor

This is for mobi3 (failed asymcute_mqttsn on acd52832):

well, accidentaly that is also the one printing the static test output for the last build.

@kaspar030
Copy link
Copy Markdown
Contributor

If you scroll all the way down in the build output, you'll see that the failures are spread across all workers.

@jnohlgard
Copy link
Copy Markdown
Member Author

So why does it fail? Is it a gcc 5 thing?

@jnohlgard
Copy link
Copy Markdown
Member Author

Nevermind, I looked at the wrong line. It's gcc7 on arm

@jnohlgard
Copy link
Copy Markdown
Member Author

After reading #9398 it seems like this error must be the same ccache problem that the clang builds have. The preprocessor is run separately from the compilation stage and therefore the /* falls through */ comments are not visible to the compiler and therefore we get an error. Modifying the ccache configuration to run the preprocessor twice should fix the errors seen in this PR.

@jnohlgard
Copy link
Copy Markdown
Member Author

Setting CCACHE_CPP2=yes fixed the problem with the compiler incorrectly reporting Wimplicit-fallthrough errors for intentional fall through.
Ping @cladmi @kaspar030 @miri64

@jnohlgard jnohlgard force-pushed the pr/Wimplicit-fallthrough branch from c4c0729 to dda5573 Compare August 14, 2018 08:20
@kaspar030
Copy link
Copy Markdown
Contributor

Setting CCACHE_CPP2=yes fixed the problem with the compiler incorrectly reporting Wimplicit-fallthrough errors for intentional fall through.

Nice to have found the cause, too bad we need that setting also for gcc, as this has non-negligable impact on compilation speed when using CCACHE.

@cladmi
Copy link
Copy Markdown
Contributor

cladmi commented Aug 14, 2018

It had a 2:37 minutes impact to do the CCACHE_CPP2=yes by comparing #9772 with this one. Current runtime: 13m:11s

But, in practice it should be less I think.

Now this PR is build with the cache from the other PRs that do not set CCACHE_CPP2. So basically there should be more cache miss than what would be at the end when all PRs use this one and the cache is warm everywhere.

@miri64
Copy link
Copy Markdown
Member

miri64 commented Aug 14, 2018

Nice to have found the cause, too bad we need that setting also for gcc, as this has non-negligable impact on compilation speed when using CCACHE.

Well, sometimes correctness should take precedence over speed 🤡

@kaspar030 kaspar030 changed the title Makefile.include: Remove -Wno-implicit-fallthrough make: Makefile.include: Remove -Wno-implicit-fallthrough Aug 14, 2018
@kaspar030
Copy link
Copy Markdown
Contributor

This should be good to go, right?

@kaspar030 kaspar030 merged commit 0f897fc into RIOT-OS:master Aug 14, 2018
@jnohlgard jnohlgard deleted the pr/Wimplicit-fallthrough branch September 24, 2018 09:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: build system Area: Build system CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants