drivers/at: fix invalid function pointer cast. by jcarrano · Pull Request #9740 · RIOT-OS/RIOT

jcarrano · 2018-08-08T11:31:16Z

Contribution description

Fix an invalid cast of a function pointer. ARM GCC 8.2.0 (on Arch linux) was (rightly) complaining about this.

As a note, I'd regard any explicit cast of function pointers suspicious. That's something that simply should not be done.

A-Paul

Hi @cladmi, thank you for your approach to fix this.

I have two remarks.
There are other places where the same cast is used:

paul@hedon:~/git/RIOT$ global --reference --cxref isrpipe_write_one
isrpipe_write_one   29 drivers/at/at.c      uart_init(uart, baudrate, (uart_rx_cb_t) isrpipe_write_one,
isrpipe_write_one  105 drivers/ethos/ethos.c             isrpipe_write_one(&uart_stdio_isrpipe, c);
isrpipe_write_one   65 sys/include/isrpipe.h int isrpipe_write_one(isrpipe_t *isrpipe, char c);
isrpipe_write_one   89 sys/uart_stdio/uart_stdio.c     uart_init(UART_STDIO_DEV, UART_STDIO_BAUDRATE, (uart_rx_cb_t) isrpipe_write_one, &uart_stdio_isrpipe);
isrpipe_write_one   91 sys/uart_stdio/uart_stdio.c     uart_init(ETHOS_UART, ETHOS_BAUDRATE, (uart_rx_cb_t) isrpipe_write_one, &uart_stdio_isrpipe);

I think it might be better to change them all or none, instead of getting diversification.

Code size is increasing by 8 byte. I tried inline but gained 8 bytes too.

    1002              0      20    1022     3fe xtimer_core.o (ex /xtimer.a)
-   1012              0       0    1012     3f4 at.o (ex /at.a)
+   1020              0       0    1020     3fc at.o (ex /at.a)
     768              0       0     768     300 xtimer.o (ex /xtimer.a)

jcarrano · 2018-08-20T14:25:12Z

There are other places where the same cast is used:

I'll take a look later. Is there a tool to find function pointer casts? As I mentioned before, most of the time a function cast is just wrong.

Code size is increasing by 8 byte. I tried inline but gained 8 bytes too.

It is to be expected. In fact, I'm quite surprised it is only 8 bytes. If the exact same cast is used everywhere it may make sense to share the wrapper, instead of having each module replicate it as a static function.

Btw, im not cladmi.

A-Paul · 2018-08-21T14:42:41Z

Hi @jcarrano

Btw, im not cladmi.

How did I messed this up? :D Sorry. I hope neither you or cladmi feel insulted by this mismatch. ;-)

I'll take a look later. Is there a tool to find function pointer casts?

There is a tool called global. In my previous comment is the output for all places where isrpipe_write_one is called or declared. You can see the mentioned casts are in the printed codelines.
Or, at least three of them.

jcarrano · 2018-10-18T09:42:44Z

@A-Paul Are you still there? The AT driver is still not compiling, so this fix remains relevant.

Regarding other modules, I have to investigate why it does no fail compilation? Can we get this in first?

drivers/at/at.c

jia200x · 2018-10-30T10:47:24Z

marked for the Release

jcarrano · 2018-10-30T10:50:35Z

@jia200x

marked for the Release

do you need me to do anything regarding this?

jia200x · 2018-10-30T10:57:17Z

I think it might be better to change them all or none, instead of getting diversification.

Agreed. I would still try to get this one in otherwise the Release tests fail with GCC8, and change the rest in follow ups. Maybe open a tracker would be useful.

@jcarrano if we could get this in soon-ish would be nice! As said before, maybe a tracker would be nice. But for this Release 2018.10, IMO this should be there for giving more toolchain support.

cladmi · 2018-10-30T10:58:16Z

@leandrolanzieri as you did a duplicate PR, can you review here ?

leandrolanzieri · 2018-10-30T10:58:49Z

Sure. ACK for:

Fundamentals
Code design
Code style
Documentation

@jia200x will test this now.

cladmi · 2018-10-30T11:12:38Z

I added people who wrote and maintain this module to review.

jcarrano

Auto-suggesting changes bc. I don't want to open up a real editor.

drivers/at/at.c

jia200x · 2018-10-30T11:15:50Z

In current master:

Building application "tests_driver_at" for "native" with MCU "native".

"make" -C /home/jialamos/Development/RIOT/boards/native
"make" -C /home/jialamos/Development/RIOT/boards/native/drivers
"make" -C /home/jialamos/Development/RIOT/core
"make" -C /home/jialamos/Development/RIOT/cpu/native
"make" -C /home/jialamos/Development/RIOT/cpu/native/periph
"make" -C /home/jialamos/Development/RIOT/cpu/native/vfs
"make" -C /home/jialamos/Development/RIOT/drivers
"make" -C /home/jialamos/Development/RIOT/drivers/at
/home/jialamos/Development/RIOT/drivers/at/at.c: In function ‘at_dev_init’:
/home/jialamos/Development/RIOT/drivers/at/at.c:29:31: error: cast between incompatible function types from ‘int (*)(isrpipe_t *, char)’ {aka ‘int (*)(struct <anonymous> *, char)’} to ‘void (*)(void *, uint8_t)’ {aka ‘void (*)(void *, unsigned char)’} [-Werror=cast-function-type]
     uart_init(uart, baudrate, (uart_rx_cb_t) isrpipe_write_one,
                               ^
cc1: all warnings being treated as errors
make[3]: *** [/home/jialamos/Development/RIOT/Makefile.base:83: /home/jialamos/Development/RIOT/tests/driver_at/bin/native/at/at.o] Error 1
make[2]: *** [/home/jialamos/Development/RIOT/Makefile.base:20: ALL--/home/jialamos/Development/RIOT/drivers/at] Error 2
make[1]: *** [/home/jialamos/Development/RIOT/Makefile.base:20: ALL--/home/jialamos/Development/RIOT/drivers] Error 2
make: *** [/home/jialamos/Development/RIOT/tests/driver_at/../../Makefile.include:445: /home/jialamos/Development/RIOT/tests/driver_at/bin/native/application_tests_driver_at.a] Error 2

With PR:

"make" -C /home/jialamos/Development/RIOT/boards/native
"make" -C /home/jialamos/Development/RIOT/boards/native/drivers
"make" -C /home/jialamos/Development/RIOT/core
"make" -C /home/jialamos/Development/RIOT/cpu/native
"make" -C /home/jialamos/Development/RIOT/cpu/native/periph
"make" -C /home/jialamos/Development/RIOT/cpu/native/vfs
"make" -C /home/jialamos/Development/RIOT/drivers
"make" -C /home/jialamos/Development/RIOT/drivers/at
"make" -C /home/jialamos/Development/RIOT/drivers/periph_common
"make" -C /home/jialamos/Development/RIOT/sys
"make" -C /home/jialamos/Development/RIOT/sys/auto_init
"make" -C /home/jialamos/Development/RIOT/sys/div
"make" -C /home/jialamos/Development/RIOT/sys/fmt
"make" -C /home/jialamos/Development/RIOT/sys/isrpipe
"make" -C /home/jialamos/Development/RIOT/sys/shell
"make" -C /home/jialamos/Development/RIOT/sys/tsrb
"make" -C /home/jialamos/Development/RIOT/sys/xtimer
   text    data     bss     dec     hex filename
  44929     760   49404   95093   17375 /home/jialamos/Development/RIOT/tests/driver_at/bin/native/tests_driver_at.elf

So, tested!

jia200x · 2018-10-30T20:15:52Z

@jcarrano please squash! ;)

jcarrano · 2018-10-31T09:58:10Z

@jia200x Done.

leandrolanzieri

Reviewed, tested and CI passing. So ACK.

leandrolanzieri · 2018-11-01T08:57:52Z

@jcarrano can you please provide the backport against the 2018.10-branch?

jcarrano · 2018-11-01T09:57:09Z

@leandrolanzieri Done: #10309

jcarrano added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Area: drivers Area: Device drivers labels Aug 8, 2018

cladmi mentioned this pull request Aug 9, 2018

Release 2018.07 - RC2 RIOT-OS/Release-Specs#69

Closed

69 tasks

A-Paul reviewed Aug 20, 2018

View reviewed changes

bergzand reviewed Oct 18, 2018

View reviewed changes

drivers/at/at.c Outdated Show resolved Hide resolved

cladmi mentioned this pull request Oct 30, 2018

Release 2018.10 - RC1 RIOT-OS/Release-Specs#76

Closed

54 tasks

leandrolanzieri mentioned this pull request Oct 30, 2018

drivers/at: Fix UART callback function signature #10299

Closed

jia200x added the Process: needs backport Integration Process: The PR is required to be backported to a release or feature branch label Oct 30, 2018

jcarrano added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Oct 30, 2018

leandrolanzieri self-assigned this Oct 30, 2018

cladmi requested review from kaspar030 and vincent-d October 30, 2018 11:12

jcarrano commented Oct 30, 2018

View reviewed changes

drivers/at/at.c Outdated Show resolved Hide resolved

drivers/at/at.c Outdated Show resolved Hide resolved

leandrolanzieri 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 Oct 30, 2018

jcarrano force-pushed the at_driver-fix-cast branch from a0e9ee7 to 53de520 Compare October 31, 2018 09:57

drivers/at: fix invalid function pointer cast.

53de520

leandrolanzieri approved these changes Nov 1, 2018

View reviewed changes

leandrolanzieri merged commit 8507131 into RIOT-OS:master Nov 1, 2018

jcarrano mentioned this pull request Nov 1, 2018

drivers/at: fix invalid function pointer cast [backport 2018.10] #10309

Merged

jia200x added this to the Release 2018.10 milestone Nov 1, 2018

jcarrano mentioned this pull request Nov 1, 2018

sys/stdio_uart: do not cast function pointer. #10312

Closed

jcarrano deleted the at_driver-fix-cast branch November 8, 2018 15:18

Conversation

jcarrano commented Aug 8, 2018

Contribution description

Uh oh!

A-Paul left a comment

Choose a reason for hiding this comment

Uh oh!

jcarrano commented Aug 20, 2018

Uh oh!

A-Paul commented Aug 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcarrano commented Oct 18, 2018

Uh oh!

Uh oh!

jia200x commented Oct 30, 2018

Uh oh!

jcarrano commented Oct 30, 2018

Uh oh!

jia200x commented Oct 30, 2018

Uh oh!

cladmi commented Oct 30, 2018

Uh oh!

leandrolanzieri commented Oct 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cladmi commented Oct 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcarrano left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jia200x commented Oct 30, 2018

Uh oh!

jia200x commented Oct 30, 2018

Uh oh!

jcarrano commented Oct 31, 2018

Uh oh!

leandrolanzieri left a comment

Choose a reason for hiding this comment

Uh oh!

leandrolanzieri commented Nov 1, 2018

Uh oh!

jcarrano commented Nov 1, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

A-Paul commented Aug 21, 2018 •

edited

Loading

leandrolanzieri commented Oct 30, 2018 •

edited

Loading

cladmi commented Oct 30, 2018 •

edited

Loading