Fix compilation errors with arm-none-eabi-gcc 7 #7502
Conversation
This prevents the compiler from optimising pdump. The error was: Firmware/src/drivers/boards/common/board_crashdump.c:41:2: error: 'memset' writing 3240 bytes into a region of size 4 overflows the destination [-Werror=stringop-overflow=] memset(pdump, 0, sizeof(fullcontext_s));
jlecoeur
force-pushed
the
lis/arm-none-eabi-gcc7
branch
from
da3d7ec to
ad40899
Compare
|
@davids5 Part of the Nuttx patch was already in upstream master. I made a PR to NuttX with the remaining warning (-Wimplicit-fallthrough) here: https://bitbucket.org/nuttx/nuttx/pull-requests/424/fix-warning-implicit-fallthrough-on-with/diff. Let's see if they accept it. |
|
Ok, the patch was merged upstream, that was quick! |
|
Great! No. Keep the patch on master. But list it as a '30-BACKPORT-fix-compilation-errors-arm-none-eabi-gcc-7.patch' (there are some other in the quere) PX4 master is tied to V7.18+ plus patches. |
|
I renamed the patch as suggested. There was another error in NuttX/apps so I added another patch. This error is - I think - not in upstream anymore: the file |
|
@davids5 can you review again? |
|
If I follow you the '00031-PENDING-fix-unused-variable-arm-none-eabi-gcc-7.patch' is not applicable upstream. So it should be named |
|
@jlecoeur On further investigation I see nsh_syscmds.c upstream. Would you please see if it has the same issue? I would just replace our version with the file. If if does not build because of other incompatibilities we would keep the REJECTED patch. If it does build but has the issue then, the submit the PR upstream and then backport the whole file as a BACKPORT |
|
Thanks for the heads-up @davids5 I do not know how I missed it. @bkueng The problem is that compatibility with gcc7 might be broken again as it is not enforced by CI. What do you think of installing a gcc 7 in parallel to the current version on travis (or other), to test against it? |
jlecoeur
changed the title
jlecoeur
force-pushed
the
lis/arm-none-eabi-gcc7
branch
from
ad0c111 to
974ba9d
Compare
|
@jlecoeur |
I have nothing against that, we just have to make sure CI does not slow down too much. @dagar ? |
|
If someone would like to get a gcc 7 docker image together I'll get it into the build system. |
|
@jlecoeur I noticed a "patch unexpectedly ends in middle of line" FYI @dagar ci is not failing. |
|
Is that usually a CRLF issue? The patch command would need to return an error to fail the build in this case, but it appears to only be a (harmless?) warning. |
|
This was indeed a warning because of bad patch format. For reference I used |
|
@dagar I found this dockerfile for gcc 7.1.0 https://hub.docker.com/_/gcc/ |
BMP280: fix -Werror=implicit-fallthrough on arm-none-eabi-gcc 7 gnss: fix -Werror=implicit-fallthrough on arm-none-eabi-gcc 7 fmu: fix -Werror=implicit-fallthrough on arm-none-eabi-gcc 7 timer.c: fix -Werror=implicit-fallthrough on arm-none-eabi-gcc 7 px4cannode_led: fix -Werror=implicit-fallthrough on arm-none-eabi-gcc 7 Fix -Werror=implicit-fallthrough on gcc7
The error was:
Firmware/src/systemcmds/hardfault_log/hardfault_log.c:312:7: error: specified bound 30 equals the size of the destination [-Werror=stringop-overflow=]
strncat(marker, sp_name, sizeof(marker));
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
…ings Rename nuttx patch Add nuttx patch for unused variable error Pending nuttx patch Backport nuttx fix for unused variables in nsh_proccmds.c Fix Patch format Modify pending patch to match new nuttx PR Move accepted nuttx changes from pending patch to backport patch
jlecoeur
force-pushed
the
lis/arm-none-eabi-gcc7
branch
from
7de721e to
020ebac
Compare
jlecoeur
changed the title
|
@davids5 It is ready, all NuttX patches are upstream, and I cleaned the commit history. Can you review again please? |
|
Awesome! @PX4TestFlights Can you take this to a test? |
|
couple of flights on a pixracer (V4) stable; no issues |
|
@santiago3dr Is this with the current build from CI or did you install the arm-none-eabi-gcc 7.1.0 tools? @jlecoeur - So we can all get on the same version, would please post the output of |
|
current build, will install tools and re-compile |
|
I am using the version from Arch repository: If that can help, here are a few builds: https://gist.github.com/jlecoeur/f6e8128deb9d7a28227bb313637ef940 |
|
couple of flight with pixracer (V4) compiled with arm-none-eabi-gcc 7.1.0 tools http://logs.px4.io/plot_app?log=a0354f3c-9cfc-4318-9651-5000eb108eb2 |
|
@santiago3dr Thanks for testing this. |
|
@jlecoeur we used the builds from your link |
|
Thanks @r0gelion @santiago3dr the flights look fine. I see this warning: |
jlecoeur
force-pushed
the
lis/arm-none-eabi-gcc7
branch
from
020ebac to
f5105e0
Compare
|
I think it is the rounding. But the allocation needs to be increased by 8-16 bytes. |
Test flights reported the warning `[load_mon] log_writer_file low on stack! (292 bytes left)` Increase stack size from 1060 to 1072 (=8 + 1060 rounded to next multiple of 8).
|
I need this as well. |
There was a problem hiding this comment.
The changes look fine, I thought we were waiting on a CI build for the new compiler in parallel.
@dagar Please advise.
Once that is done we need to vet this. The concern being it does not stem from the ARM team, and we may not see a loss of "special tweak" on the surface. CPU load, code size, jitter needs to all be looked at characterized or at least understood.
I just need it for the host compilation, for SITL. I agree, once we raise the ARM GCC version, we need to vet all of that. |
With newer versions of GCC, new warnings are emitted and prevent building on master.
Associated NuttX fixes: PX4/NuttX#3