cpu/cc26xx_cc13xx: Fix bogus array-bound warning by maribu · Pull Request #19504 · RIOT-OS/RIOT

maribu · 2023-04-25T13:36:33Z

Contribution description

GCC 12 create a bogus array out of bounds warning as it assumes that because there is special handling for uart == 0 and uart == 1, uart can indeed be 1. There is an assert(uart < UART_NUMOF) above that would blow up prior to any out of bounds access.

In any case, optimizing out the special handling of uart == 1 for when UART_NUMOF == 1 likely improves the generated code and fixes the warning.

/home/maribu/Repos/software/RIOT/cc2650/cpu/cc26xx_cc13xx/periph/uart.c:88:8: error: array subscript 1 is above array bounds of 'uart_isr_ctx_t[1]' [-Werror=array-bounds]
   88 |     ctx[uart].rx_cb = rx_cb;
      |     ~~~^~~~~~
/home/maribu/Repos/software/RIOT/cc2650/cpu/cc26xx_cc13xx/periph/uart.c:52:23: note: while referencing 'ctx'
   52 | static uart_isr_ctx_t ctx[UART_NUMOF];
      |                       ^~~
/home/maribu/Repos/software/RIOT/cc2650/cpu/cc26xx_cc13xx/periph/uart.c:89:8: error: array subscript 1 is above array bounds of 'uart_isr_ctx_t[1]' [-Werror=array-bounds]
   89 |     ctx[uart].arg = arg;
      |     ~~~^~~~~~
/home/maribu/Repos/software/RIOT/cc2650/cpu/cc26xx_cc13xx/periph/uart.c:52:23: note: while referencing 'ctx'
   52 | static uart_isr_ctx_t ctx[UART_NUMOF];
      |                       ^~~

Testing procedure

The actual change is a pretty obvious one-liner, so that code review and a green CI should be sufficient. If not, running any UART example app without regression should do.

Issues/PRs references

None

GCC 12 create a bogus array out of bounds warning as it assumes that because there is special handling for `uart == 0` and `uart == 1`, `uart` can indeed be `1`. There is an `assert(uart < UART_NUMOF)` above that would blow up prior to any out of bounds access. In any case, optimizing out the special handling of `uart == 1` for when `UART_NUMOF == 1` likely improves the generated code and fixes the warning. /home/maribu/Repos/software/RIOT/cc2650/cpu/cc26xx_cc13xx/periph/uart.c:88:8: error: array subscript 1 is above array bounds of 'uart_isr_ctx_t[1]' [-Werror=array-bounds] 88 | ctx[uart].rx_cb = rx_cb; | ~~~^~~~~~ /home/maribu/Repos/software/RIOT/cc2650/cpu/cc26xx_cc13xx/periph/uart.c:52:23: note: while referencing 'ctx' 52 | static uart_isr_ctx_t ctx[UART_NUMOF]; | ^~~ /home/maribu/Repos/software/RIOT/cc2650/cpu/cc26xx_cc13xx/periph/uart.c:89:8: error: array subscript 1 is above array bounds of 'uart_isr_ctx_t[1]' [-Werror=array-bounds] 89 | ctx[uart].arg = arg; | ~~~^~~~~~ /home/maribu/Repos/software/RIOT/cc2650/cpu/cc26xx_cc13xx/periph/uart.c:52:23: note: while referencing 'ctx' 52 | static uart_isr_ctx_t ctx[UART_NUMOF]; | ^~~

maribu · 2023-04-25T13:37:16Z

@MrKevinWeiss: Do you want a backport of this?

riot-ci · 2023-04-25T13:49:30Z

Murdock results

✔️ PASSED

d6499fa cpu/cc26xx_cc13xx: Fix bogus array-bound warning

Success	Failures	Total	Runtime
6919	0	6919	09m:11s

Artifacts

Documentation preview

MrKevinWeiss · 2023-04-25T13:56:06Z

Well I was hoping RC3 would be the one... I also guess we would not see this with docker, so I would say we leave it for now. Any opinions @jia200x?

maribu · 2023-04-25T14:09:01Z

There is also a third option: We have a history of backporting fixes to the release branch without creating new tags for it. So backporting this even after the release is certainly an option.

I'm personally a huge fan of this: It allows users affected by bugs in the release to use the release branch with the fixes instead, but with the disclaimer that there may be regressions as well that would have been caught by the release tests.

I personally don't think that this PR justifies a new RC cycle.

maribu · 2023-04-25T14:11:45Z

#19505 on the other hand may be interesting, as it fixes building rust apps when using LLVM16. Arch Linux is bound to ship that soonish.

benpicco

strange fix, but OK.
Does this also happen with DEVELHELP=1?

The assert should already make all code where uart >= UART_NUMOF unreachable and the optimizer should subsequently remove it.

maribu · 2023-04-25T14:58:33Z

Does this also happen with DEVELHELP=1?

No, then the heuristics indeed get that uart == UART_NUMOF is not possible.

benpicco · 2023-04-25T15:00:02Z

bors merge

maribu · 2023-04-25T15:00:04Z

bors merge

bors · 2023-04-25T15:00:09Z

Already running a review

18620: core: add core_mutex_debug to aid debugging deadlocks r=benpicco a=maribu ### Contribution description Adding `USEMODULE += core_mutex_debug` to your `Makefile` results in on log messages such as [mutex] waiting for thread 1 (pc = 0x800024d) being added whenever `mutex_lock()` blocks. This makes tracing down deadlocks easier. ### Testing procedure Run e.g. ```sh USEMODULE=core_mutex_debug BOARD=nucleo-f767zi make -C tests/mutex_cancel flash test ``` which should provide output such as ``` Welcome to pyterm! Type '/exit' to exit. READY s [mutex] waiting for thread 1 (pc = 0x8000f35) START main(): This is RIOT! (Version: 2022.10-devel-841-g5cc02-core/mutex/debug) Test Application for mutex_cancel / mutex_lock_cancelable ========================================================= Test without cancellation: OK Test early cancellation: OK Verify no side effects on subsequent calls: [mutex] waiting for thread 1 (pc = 0x800024d) OK Test late cancellation: [mutex] waiting for thread 1 (pc = 0x0) OK TEST PASSED ``` ```sh $ arm-none-eabi-addr2line -a 0x800024d -e tests/mutex_cancel/bin/nucleo-f767zi/tests_mutex_cancel.elf 0x0800024d /home/maribu/Repos/software/RIOT/tests/mutex_cancel/main.c:51 ``` ### Issues/PRs references Depends on and includes #18619 19296: nanocoap: allow to define CoAP resources as XFA r=benpicco a=benpicco 19504: cpu/cc26xx_cc13xx: Fix bogus array-bound warning r=benpicco a=maribu ### Contribution description GCC 12 create a bogus array out of bounds warning as it assumes that because there is special handling for `uart == 0` and `uart == 1`, `uart` can indeed be `1`. There is an `assert(uart < UART_NUMOF)` above that would blow up prior to any out of bounds access. In any case, optimizing out the special handling of `uart == 1` for when `UART_NUMOF == 1` likely improves the generated code and fixes the warning. /home/maribu/Repos/software/RIOT/cc2650/cpu/cc26xx_cc13xx/periph/uart.c:88:8: error: array subscript 1 is above array bounds of 'uart_isr_ctx_t[1]' [-Werror=array-bounds] 88 | ctx[uart].rx_cb = rx_cb; | ~~~^~~~~~ /home/maribu/Repos/software/RIOT/cc2650/cpu/cc26xx_cc13xx/periph/uart.c:52:23: note: while referencing 'ctx' 52 | static uart_isr_ctx_t ctx[UART_NUMOF]; | ^~~ /home/maribu/Repos/software/RIOT/cc2650/cpu/cc26xx_cc13xx/periph/uart.c:89:8: error: array subscript 1 is above array bounds of 'uart_isr_ctx_t[1]' [-Werror=array-bounds] 89 | ctx[uart].arg = arg; | ~~~^~~~~~ /home/maribu/Repos/software/RIOT/cc2650/cpu/cc26xx_cc13xx/periph/uart.c:52:23: note: while referencing 'ctx' 52 | static uart_isr_ctx_t ctx[UART_NUMOF]; | ^~~ ### Testing procedure The actual change is a pretty obvious one-liner, so that code review and a green CI should be sufficient. If not, running any UART example app without regression should do. ### Issues/PRs references None 19506: tools/openocd: Fix handling of OPENOCD_CMD_RESET_HALT r=maribu a=maribu ### Contribution description The OPENOCD_CMD_RESET_HALT was not longer correctly passed to the script. This fixes the issue. ### Testing procedure Flashing of e.g. the `cc2650-launchpad` with upstream OpenOCD should work again. ### Issues/PRs references The change was added to #19050 after testing the PR and before merging. I'm not sure if the fix never worked because of this, or if behavior of `target-export-variables` or GNU Make changed. 19507: cpu/cc26x0_cc13x0: Drop feature cortexm_mpu r=benpicco a=maribu ### Contribution description At least the CC2650 doesn't have an MPU, I assume this is also true for the rest of the family. The CC2652 does have an MPU according to the datasheet. So I keep the feature there in place. ### Testing procedure E.g. ``` make BOARD=cc2650-launchpad -C tests/mpu_noexec_ram flash test ``` fails. (Note: A successful test run would also crash but with a mem manage handler rather than a hardfault due to an invalid instruction on the stack being executed.) It would be nice to also test the same for a `cc2652-launchpad`, for which the MPU should work. ### Issues/PRs references None Co-authored-by: Marian Buschsieweke <[email protected]> Co-authored-by: Benjamin Valentin <[email protected]> Co-authored-by: Benjamin Valentin <[email protected]>

bors · 2023-04-25T15:03:17Z

Build failed (retrying...):

Murdock

maribu · 2023-04-25T15:05:02Z

bors r-

bors · 2023-04-25T15:05:05Z

Canceled.

maribu · 2023-04-25T15:45:00Z

bors merge

bors · 2023-04-25T19:08:22Z

Build succeeded:

maribu · 2023-04-25T20:12:55Z

Backport provided in #19509

19505: Rust: Update dependencies [backport 2023.04] r=maribu a=maribu # Backport of #19495 ### Contribution description This updates both the RIOT-specific and generic dependencies of Rust examples and modules. It also follows a deprecation from the G unit renaming originally done in #19292. ### Testing procedure * Green CI should do ### Issues/PRs references Copying from one of the commits with some sed: * riot-sys: * RIOT-OS/rust-riot-sys#26 * RIOT-OS/rust-riot-sys#31 * RIOT-OS/rust-riot-sys#30 * RIOT-OS/rust-riot-sys#28 * RIOT-OS/rust-riot-sys#27 * RIOT-OS/rust-riot-sys#25 * RIOT-OS/rust-riot-sys#23 * RIOT-OS/rust-riot-sys#22 * RIOT-OS/rust-riot-sys#21 * riot-wrappers: * RIOT-OS/rust-riot-wrappers#36 * RIOT-OS/rust-riot-wrappers#50 * RIOT-OS/rust-riot-wrappers#48 * RIOT-OS/rust-riot-wrappers#47 * RIOT-OS/rust-riot-wrappers#44 * RIOT-OS/rust-riot-wrappers#45 * RIOT-OS/rust-riot-wrappers#43 * (later, when the mistake became apparent) RIOT-OS/rust-riot-wrappers#55 ### How to do similar PRs Updating the RIOT-related dependencies (which here also updated bindgen because the dependency of riot-sys changed): ``` $ for x in **/Cargo.lock; do cargo update --manifest-path=${x%.lock}.toml --package riot-sys --package riot-wrappers; done ``` Updating everything (should never really be needed, b/c if something has a concrete dependency change it'd say so, but dependencies generally get better over time): ``` $ for x in **/Cargo.lock; do cargo update --manifest-path=${x%.lock}.toml; done ``` Creating the commit message: ``` $ git log --first-parent --format='%s (%b)' 9c29faf55d4c14d2d7f55f8df5059c52af4e5317..e4973a6ee88427f702dac41b3dce4fd6b6b9689e | sed 's/Merges: //' | sed 's/^/ * /' ``` git shortlog unfortunately doesn't show the merges the way I prefer linking them. The versions in the command line can be taken from `git diff --text` and looking for the riot-sys or riot-wrappers line, respectively. 19509: cpu/cc26xx_cc13xx: Fix bogus array-bound warning [backport 2023.04] r=maribu a=maribu # Backport of #19504 ### Contribution description GCC 12 create a bogus array out of bounds warning as it assumes that because there is special handling for `uart == 0` and `uart == 1`, `uart` can indeed be `1`. There is an `assert(uart < UART_NUMOF)` above that would blow up prior to any out of bounds access. In any case, optimizing out the special handling of `uart == 1` for when `UART_NUMOF == 1` likely improves the generated code and fixes the warning. /home/maribu/Repos/software/RIOT/cc2650/cpu/cc26xx_cc13xx/periph/uart.c:88:8: error: array subscript 1 is above array bounds of 'uart_isr_ctx_t[1]' [-Werror=array-bounds] 88 | ctx[uart].rx_cb = rx_cb; | ~~~^~~~~~ /home/maribu/Repos/software/RIOT/cc2650/cpu/cc26xx_cc13xx/periph/uart.c:52:23: note: while referencing 'ctx' 52 | static uart_isr_ctx_t ctx[UART_NUMOF]; | ^~~ /home/maribu/Repos/software/RIOT/cc2650/cpu/cc26xx_cc13xx/periph/uart.c:89:8: error: array subscript 1 is above array bounds of 'uart_isr_ctx_t[1]' [-Werror=array-bounds] 89 | ctx[uart].arg = arg; | ~~~^~~~~~ /home/maribu/Repos/software/RIOT/cc2650/cpu/cc26xx_cc13xx/periph/uart.c:52:23: note: while referencing 'ctx' 52 | static uart_isr_ctx_t ctx[UART_NUMOF]; | ^~~ ### Testing procedure The actual change is a pretty obvious one-liner, so that code review and a green CI should be sufficient. If not, running any UART example app without regression should do. ### Issues/PRs references None 19510: tools/openocd: Fix handling of OPENOCD_CMD_RESET_HALT [backport 2023.04] r=maribu a=maribu # Backport of #19506 ### Contribution description The OPENOCD_CMD_RESET_HALT was not longer correctly passed to the script. This fixes the issue. ### Testing procedure Flashing of e.g. the `cc2650-launchpad` with upstream OpenOCD should work again. ### Issues/PRs references The change was added to #19050 after testing the PR and before merging. I'm not sure if the fix never worked because of this, or if behavior of `target-export-variables` or GNU Make changed. Co-authored-by: chrysn <[email protected]> Co-authored-by: Marian Buschsieweke <[email protected]>

maribu added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer Area: cpu Area: CPU/MCU ports labels Apr 25, 2023

maribu requested review from MrKevinWeiss and jeandudey April 25, 2023 13:36

maribu requested a review from smlng as a code owner April 25, 2023 13:36

github-actions bot added the Platform: ARM Platform: This PR/issue effects ARM-based platforms label Apr 25, 2023

benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Apr 25, 2023

benpicco approved these changes Apr 25, 2023

View reviewed changes

bors bot merged commit 46af92d into RIOT-OS:master Apr 25, 2023

maribu mentioned this pull request Apr 25, 2023

cpu/cc26xx_cc13xx: Fix bogus array-bound warning [backport 2023.04] #19509

Merged

maribu deleted the cc2650-compilation-fixes branch July 3, 2023 13:22

benpicco added this to the Release 2023.07 milestone Aug 2, 2023

Conversation

maribu commented Apr 25, 2023

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

maribu commented Apr 25, 2023

Uh oh!

riot-ci commented Apr 25, 2023

Murdock results

Artifacts

Uh oh!

MrKevinWeiss commented Apr 25, 2023

Uh oh!

maribu commented Apr 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maribu commented Apr 25, 2023

Uh oh!

benpicco left a comment • 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!

maribu commented Apr 25, 2023

Uh oh!

benpicco commented Apr 25, 2023

Uh oh!

maribu commented Apr 25, 2023

Uh oh!

bors bot commented Apr 25, 2023

Uh oh!

bors bot commented Apr 25, 2023

Uh oh!

maribu commented Apr 25, 2023

Uh oh!

bors bot commented Apr 25, 2023

Uh oh!

maribu commented Apr 25, 2023

Uh oh!

bors bot commented Apr 25, 2023

Uh oh!

maribu commented Apr 25, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

maribu commented Apr 25, 2023 •

edited

Loading

benpicco left a comment •

edited

Loading