Various warning fixes #3101

jnohlgard · 2015-05-28T08:28:52Z

This PR fixes all warnings that I encountered when adding with -Wall -Wextra to Makefile.include and building examples/rpl_udp for mulle and examples/default for stm32f4discovery

Most warnings were harmless but there were a few mistakes that were corrected as well:

~~Potential buffer overflow in sys/newlib/syscalls.c(_read_r())~~ split to sys/newlib/syscalls: _read_r fix potential buffer overflow #3110
Missing callback assignment in Kinetis SPI slave mode implementation
Empty definition of __set_FPSCR when building for Cortex-M4 without FPU - duplicated in Cortex-M CMSIS: Update CMSIS to version 4.3 #3128

OlegHahm · 2015-05-28T08:58:54Z

cpu/kinetis_common/random_rnga.c

@@ -55,7 +55,7 @@ int random_read(char *buf, unsigned int num)
        }
    }

-    return count;
+    return (int)count;


Without looking further into this: Isn't the logic of this prototype here somehow broken: parameter for the count is unsigned, but the returning count value is signed (and therefore only half the size for possible values)?

I think the intention is to be able to return error codes as negative values.

Yes, I assumed that. But at least theoretical this could lead to a problem, if you specify a crazy big value.

yes, and it will take a crazy long time to generate that entropy. But I think this is an actual API bug. The documentation for periph/random.h says:

Reads num or less bytes of randomness from the source, will block until random data is available.

Parameters
[in] buf destination buffer to write the bytes to
[in] num number of bytes to get from device, only values >0 are valid

Returns
the number of bytes written to buf
0 on error

so I guess we should update either the return type or the docs. My vote is for the return type.

jnohlgard · 2015-05-28T09:18:05Z

@OlegHahm I usually assign you whenever I don't know who to assign a PR, is that OK?
(you may of course reassign, I believe you have a better overview of the project resources than I do)

OlegHahm · 2015-05-28T09:35:15Z

I usually assign you whenever I don't know who to assign a PR

I noticed that. ;)

is that OK?
(you may of course reassign, I believe you have a better overview of the project resources than I do)

Yes, it's completely okay. I will re-assign whenever I don't have time or knowledge for the PR to review.

jnohlgard · 2015-05-28T11:35:36Z

what does this Travis error even mean? (BUILDTEST_MCU_GROUP=arm7)

$ ./dist/tools/travis-scripts/build_and_test.sh
The command "./dist/tools/travis-scripts/build_and_test.sh" exited with 8.

OlegHahm · 2015-05-28T11:37:47Z

I think it's a Travis problem.

OlegHahm · 2015-05-28T11:38:12Z

@PeterKietzmann, can you look at the periph stuff?

jnohlgard · 2015-05-28T14:35:40Z

Travis is green after I restarted the build.

PeterKietzmann · 2015-05-28T16:47:26Z

Will look at it tomorrow.

jnohlgard · 2015-05-28T17:54:48Z

rebased

PeterKietzmann · 2015-05-29T08:37:50Z

cpu/kinetis_common/spi.c

+#if SPI_7_EN && (SPI_7_INDEX != SPI_0_INDEX) && (SPI_7_INDEX != SPI_1_INDEX) \
+    && (SPI_7_INDEX != SPI_2_INDEX) && (SPI_7_INDEX != SPI_3_INDEX) \
+    && (SPI_7_INDEX != SPI_4_INDEX) && (SPI_7_INDEX != SPI_5_INDEX) \
+    && (SPI_7_INDEX != SPI_6_INDEX)
    [SPI_7_INDEX] = MUTEX_INIT,
 #endif


I agree, this looks strange somehow :/ . Isn't there a nicer way to check all combinations? Like an extra macro? Anyway, as it fixes warnings I won't NACK this

This was the most direct way I could imagine. The main problem is that the preprocessor can not perform loops or anything other than simple evaluations of the value of macro expressions. I believe we can refactor this if we rework the periph API as is being discussed in #3095

Yes ok, I'm fine for the moment

PeterKietzmann · 2015-05-29T08:47:09Z

cpu/kinetis_common/spi.c

@@ -1041,13 +1056,13 @@ int spi_transfer_bytes(spi_t dev, char *out, char *in, unsigned int length)
    /* Default: send idle data */
    byte_out = (uint8_t)SPI_IDLE_DATA;

-    for (i = 0; i < length; i++) {
+    for (i = 0; i < (int)length; i++) {


Maybe less changes if you initialize i as unsigned int? Anyway...

PeterKietzmann · 2015-05-29T08:53:39Z

@gebart thanks for this work. There is the question about the return values and the buffer overflow thing that you wanted to put in a separate PR.

jnohlgard · 2015-05-29T10:04:26Z

Rebased, split the buffer overflow fix into a separate PR #3110
changed return type on the find scaler functions to long

PeterKietzmann · 2015-05-29T11:38:16Z

@OlegHahm are you also fine with merging this and further discuss the open points in #3109 and #3111? I tend to give my ACK. Just one thing: isn't the first commit message a bit too long?

PeterKietzmann · 2015-05-29T11:39:52Z

BTW: I think it's reasonable to squash into 4-5 commits. Non?

It is an error to call __set_FPSCR if no FPU is present in the CPU.

- find_closest_x: sign-compare - hwtimer_arch: unused-parameter - i2c_init_slave: unused-parameter - rnga: sign-compare - rngb: sign-compare - spi_transfer_bytes: sign-compare - spi_transfer_regs: sign-compare - timer: unused-parameter

- rx_cb: unused-parameter - _write_r: sign-compare - all stubbed syscalls: unused-parameter

jnohlgard · 2015-05-29T12:23:56Z

squashed

jnohlgard · 2015-05-31T08:23:43Z

ping @PeterKietzmann
Travis is green, is the current list of commits OK?

PeterKietzmann · 2015-05-31T15:03:46Z

Sorry for the delay. I'm fine with the list of commits. ACK and go

Various warning fixes

jnohlgard added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Platform: ARM Platform: This PR/issue effects ARM-based platforms Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation labels May 28, 2015

jnohlgard assigned OlegHahm May 28, 2015

jnohlgard added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label May 28, 2015

OlegHahm reviewed May 28, 2015
View reviewed changes

OlegHahm assigned PeterKietzmann and unassigned OlegHahm May 28, 2015

jnohlgard force-pushed the pr/warning-fixes branch from 7f0919a to 101e022 Compare May 28, 2015 17:47

OlegHahm modified the milestone: Release 2015.06 May 28, 2015

PeterKietzmann reviewed May 29, 2015
View reviewed changes

jnohlgard mentioned this pull request May 29, 2015

periph/random: random_read should return unsigned int #3109

Closed

PeterKietzmann reviewed May 29, 2015
View reviewed changes

jnohlgard mentioned this pull request May 29, 2015

sys/newlib/syscalls: _read_r fix potential buffer overflow #3110

Closed

jnohlgard force-pushed the pr/warning-fixes branch from 101e022 to 2ae8055 Compare May 29, 2015 09:59

jnohlgard added the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label May 29, 2015

cortexm_common: Conditionally define __set_FPSCR

8a34b15

It is an error to call __set_FPSCR if no FPU is present in the CPU.

jnohlgard force-pushed the pr/warning-fixes branch from 2ae8055 to 00367ab Compare May 29, 2015 12:21

Joakim Gebart added 9 commits May 29, 2015 14:21

kinetis: fix warnings

d4c1436

- find_closest_x: sign-compare - hwtimer_arch: unused-parameter - i2c_init_slave: unused-parameter - rnga: sign-compare - rngb: sign-compare - spi_transfer_bytes: sign-compare - spi_transfer_regs: sign-compare - timer: unused-parameter

kinetis: spi_init_slave add missing callback assignment

ec3679f

kinetis: fix SPI double initialization warnings

697b85c

k60: fix unused-parameter warnings

12cfb8d

drivers/at86rf231: fix unused-parameter

1c855cb

sys/newlib/syscalls: fix warnings

0eaeca3

- rx_cb: unused-parameter - _write_r: sign-compare - all stubbed syscalls: unused-parameter

stm32f4: i2c_init_slave fix unused-parameter

13bdc21

stm32f4: spi_transfer_bytes fix sign-compare, reduce scope

18ea556

stm32f4: random fix sign-compare

ea597a3

jnohlgard force-pushed the pr/warning-fixes branch from 00367ab to ea597a3 Compare May 29, 2015 12:23

jnohlgard removed the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label May 30, 2015

PeterKietzmann added a commit that referenced this pull request May 31, 2015

Merge pull request #3101 from gebart/pr/warning-fixes

6dc0e78

Various warning fixes

PeterKietzmann merged commit 6dc0e78 into RIOT-OS:master May 31, 2015

jnohlgard mentioned this pull request Jun 1, 2015

sys: newlib: use double-lock scheme for uart ringbuffer #3111

Merged

jnohlgard deleted the pr/warning-fixes branch June 25, 2015 08:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Various warning fixes #3101

Various warning fixes #3101

jnohlgard commented May 28, 2015

OlegHahm May 28, 2015

jnohlgard May 28, 2015

OlegHahm May 28, 2015

jnohlgard May 28, 2015

jnohlgard commented May 28, 2015

OlegHahm commented May 28, 2015

jnohlgard commented May 28, 2015

OlegHahm commented May 28, 2015

OlegHahm commented May 28, 2015

jnohlgard commented May 28, 2015

PeterKietzmann commented May 28, 2015

jnohlgard commented May 28, 2015

PeterKietzmann May 29, 2015

jnohlgard May 29, 2015

PeterKietzmann May 29, 2015

PeterKietzmann May 29, 2015

PeterKietzmann commented May 29, 2015

jnohlgard commented May 29, 2015

PeterKietzmann commented May 29, 2015

PeterKietzmann commented May 29, 2015

jnohlgard commented May 29, 2015

jnohlgard commented May 31, 2015

PeterKietzmann commented May 31, 2015

Various warning fixes #3101

Various warning fixes #3101

Conversation

jnohlgard commented May 28, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jnohlgard commented May 28, 2015

OlegHahm commented May 28, 2015

jnohlgard commented May 28, 2015

OlegHahm commented May 28, 2015

OlegHahm commented May 28, 2015

jnohlgard commented May 28, 2015

PeterKietzmann commented May 28, 2015

jnohlgard commented May 28, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PeterKietzmann commented May 29, 2015

jnohlgard commented May 29, 2015

PeterKietzmann commented May 29, 2015

PeterKietzmann commented May 29, 2015

jnohlgard commented May 29, 2015

jnohlgard commented May 31, 2015

PeterKietzmann commented May 31, 2015