posix_sockets.c: Fix 2 byte int compilation errors by mrdeep1 · Pull Request #19946 · RIOT-OS/RIOT

mrdeep1 · 2023-09-25T20:25:37Z

Contribution description

When building with POSIX_SETSOCKOPT defined (to be able to use the posix setsockopt() function to set SO_RCVTIMEO), this code fails to build with 16 bit integer compilers for some of the boards as there is an integer overflow for 1000*1000.

There is also an invalid test that the provided tv->tv_sec is less than 0 as it is a uint32_t.

Testing procedure

Murdock should fail if the following change is made to tests/sys/posix_sleep/Makefile without this PR.

diff --git a/tests/sys/posix_sleep/Makefile b/tests/sys/posix_sleep/Makefile
index 5dcb9ed74c..9e4f881492 100644
--- a/tests/sys/posix_sleep/Makefile
+++ b/tests/sys/posix_sleep/Makefile
@@ -5,4 +5,9 @@ USEMODULE += posix_sleep
 # microbit qemu failing currently
 TEST_ON_CI_BLACKLIST += microbit

+USEMODULE += gnrc_ipv6_default
+USEMODULE += sock_udp
+USEMODULE += posix_sockets
+CFLAGS += -DPOSIX_SETSOCKOPT
+
 include $(RIOTBASE)/Makefile.include

Issues/PRs references

None.

benpicco · 2023-09-26T11:02:04Z

sys/posix/sockets/posix_sockets.c

    socket_t *s;
    struct timeval *tv;
-    const uint32_t max_timeout_secs = UINT32_MAX / (1000 * 1000);
+    const uint32_t max_timeout_secs = UINT32_MAX / (1000UL * 1000);


Suggested change

const uint32_t max_timeout_secs = UINT32_MAX / (1000UL * 1000);

const uint32_t max_timeout_secs = UINT32_MAX / US_PER_SEC;

should make it clearer what's going on (from time_units.h)

Updated as suggested.

Did you push your changes?

Not yet - thinking about the time_t definition issue for avr cpus.

benpicco · 2023-09-26T11:04:51Z

sys/posix/sockets/posix_sockets.c

    tv = (struct timeval *) option_value;

-    if (tv->tv_sec < 0 || tv->tv_usec < 0) {
+    /* tv_sec is unit32_t, so never negative */


Are you sure? According to the standard, it's time_t, which is usually int64_t these days.

I agree that the standard refers to time_t, and the 3 definitions of tv_sec all refer to time_t in RIOT as per

# pri kind tag file 1 F C m tv_sec ./cpu/esp8266/include/sys/types.h struct:timespec time_t tv_sec; /* Seconds */ 2 F m tv_sec ./cpu/avr8_common/avr_libc_extra/include/sys/time.h struct:timeval time_t tv_sec; /**< seconds */ 3 F m tv_sec ./cpu/avr8_common/avr_libc_extra/include/vendor/time.h struct:timespec time_t tv_sec;

However, in RIOT, in the 3 places where time_t is defined we have

# pri kind tag file 1 F C t time_t ./cpu/esp8266/include/sys/types.h typedef _TIME_T_ time_t; 2 F t time_t ./cpu/avr8_common/avr_libc_extra/include/sys/types.h typedef uint32_t time_t; /**< Used for time in seconds */ 3 F t time_t ./cpu/avr8_common/avr_libc_extra/include/vendor/time.h typedef uint32_t time_t;

but _TIME_T_ is not defined anywhere.

So it looks like this change will have to be some sort of conditional based on an AVR build, but haven't worked out which conditional to use yet.

Ah so you are getting a build warning (that gets promited to error) on AVR with this check in place?

I'd be fine with dropping that check if it makes things easier, it might as well have been an assert().

Without this PR, the following is reported (assuming POSIX_SETSOCKOPT is defined and building for AVR)

/home/jon/RIOT/sys/posix/sockets/posix_sockets.c:1147:20: error: comparison of unsigned expression < 0 is always false [-Werror=type-limits] if (tv->tv_sec < 0 || tv->tv_usec < 0) { ^ cc1: all warnings being treated as errors

riot-ci · 2023-09-26T11:13:46Z

Murdock results

✔️ PASSED

ed64f06 posix_sockets.c: Fix 16 bit integer compilation errors

Success	Failures	Total	Runtime
7937	0	7937	15m:04s

Artifacts

Documentation preview

mrdeep1 · 2023-09-26T14:08:11Z

Changes pushed as a separate commit.

Before and after this PR change can be tested with this additional change

diff --git a/examples/posix_sockets/Makefile b/examples/posix_sockets/Makefile
index f2807361a9..32ca1e8bdf 100644
--- a/examples/posix_sockets/Makefile
+++ b/examples/posix_sockets/Makefile
@@ -15,6 +15,8 @@ USEMODULE += auto_init_gnrc_netif
 USEMODULE += gnrc_ipv6_default
 USEMODULE += sock_udp
 USEMODULE += posix_sockets
+USEMODULE += posix_select
+CFLAGS += -DPOSIX_SETSOCKOPT
 USEMODULE += posix_sleep
 USEMODULE += posix_inet
 # Add also the shell, some shell commands

and
$ BOARD=atmega256rfr2-xpro make

sys/posix/sockets/posix_sockets.c

benpicco · 2023-09-27T15:54:22Z

feel free to squash

mrdeep1 · 2023-09-27T16:20:52Z

squashed and pushed.

benpicco · 2023-09-28T11:13:53Z

bors merge

19946: posix_sockets.c: Fix 2 byte int compilation errors r=benpicco a=mrdeep1 19953: boards/esp32s3-wt32-sc01-plus: fix Kconfig r=aabadie a=gschorcht ### Contribution description This PR fixes a remaining Kconfig mismatch. It should fix the last compilation problem of the nightly. ### Testing procedure ``` python3 dist/tools/compile_test/compile_like_murdock.py -a tests/drivers/ili9341/ -b esp32s3-wt32-sc01-plus ``` should fail w/o this PR but should succeed with this PR. ### Issues/PRs references Co-authored-by: Jon Shallow <[email protected]> Co-authored-by: Gunar Schorcht <[email protected]>

bors · 2023-09-28T18:19:13Z

Build failed (retrying...):

Murdock

19946: posix_sockets.c: Fix 2 byte int compilation errors r=benpicco a=mrdeep1 Co-authored-by: Jon Shallow <[email protected]>

bors · 2023-09-28T22:02:49Z

Build failed:

Murdock

benpicco · 2023-09-29T08:36:30Z

bors merge

bors · 2023-09-29T12:26:06Z

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

github-actions bot added the Area: sys Area: System label Sep 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 Sep 26, 2023

benpicco reviewed Sep 26, 2023

View reviewed changes

benpicco added the Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) label Sep 26, 2023

benpicco reviewed Sep 27, 2023

View reviewed changes

sys/posix/sockets/posix_sockets.c Show resolved Hide resolved

posix_sockets.c: Fix 16 bit integer compilation errors

ed64f06

mrdeep1 force-pushed the posix_setsockopt branch from 619f3cb to ed64f06 Compare September 27, 2023 16:19

benpicco approved these changes Sep 27, 2023

View reviewed changes

bors bot added a commit that referenced this pull request Sep 28, 2023

Merge #19946

e242afa

19946: posix_sockets.c: Fix 2 byte int compilation errors r=benpicco a=mrdeep1 Co-authored-by: Jon Shallow <[email protected]>

bors bot merged commit 149cee4 into RIOT-OS:master Sep 29, 2023

mrdeep1 deleted the posix_setsockopt branch September 29, 2023 13:32

MrKevinWeiss added this to the Release 2023.10 milestone Nov 17, 2023

	const uint32_t max_timeout_secs = UINT32_MAX / (1000UL * 1000);
	const uint32_t max_timeout_secs = UINT32_MAX / US_PER_SEC;

Conversation

mrdeep1 commented Sep 25, 2023

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benpicco Sep 26, 2023 • 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!

Choose a reason for hiding this comment

Uh oh!

riot-ci commented Sep 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Murdock results

Artifacts

Uh oh!

mrdeep1 commented Sep 26, 2023

Uh oh!

Uh oh!

benpicco commented Sep 27, 2023

Uh oh!

mrdeep1 commented Sep 27, 2023

Uh oh!

benpicco commented Sep 28, 2023

Uh oh!

bors bot commented Sep 28, 2023

Uh oh!

bors bot commented Sep 28, 2023

Uh oh!

benpicco commented Sep 29, 2023

Uh oh!

bors bot commented Sep 29, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

benpicco Sep 26, 2023 •

edited

Loading

riot-ci commented Sep 26, 2023 •

edited

Loading