samples/gettimeofday: Detect 32-bit timeval.tv_sec

[keith-packard]

First, fix the posix arch to make glibc expose 64-bit time_t and off_t types. Then, make sure time_t is never 32-bits on all architectures.

[aescolar]

What is the background/motivation for this change?

Please note:

1. If you build this sample for native_posix(_64), kconfig will, against the sample configuration, prevent the POSIX API shim from being selected (CONFIG_POSIX=n) , will default to the host libC (CONFIG_NEWLIB_LIBC=n, CONFIG_EXTERNAL_LIBC=y) and will similarly mess some of the networking configuration.
   The result, (apart from the kconfig warnings), is that the sample will not test anything in the POSIX compatibility shim subsystem, but instead call the Linux host APIs. So it will appear to work at first sight, but will not be really testing anything of use.
   It will actually be a very confusing thing, as effectively the Zephyr OS is there, but not even let to run once the application enters its main() function.

2. If on the other hand, you attempt to build this app for native_sim(_64), AND, you select CONFIG_PICOLIBC=Y you will be compiling with the POSIX API shim:
   2.1 Unfortunately with just that, the native ethernet driver will fail to build (see native drivers and backends not yet supported with embedded C libraries #60096)
   2.2 If you also disable the network stack (cmake -GNinja -DBOARD=native_sim ../samples/posix/gettimeofday/ -DCONFIG_PICOLIBC=y -DCONFIG_NETWORKING=n && ninja , the sample will build properly and run, and the POSIX API compatibility shim will be tested. Time will not be synchronized thru SNTP though.

In this 2. case, you do not need a change like the one in this PR, as glibc is not used.
In the 1. case, the sample would be of no use, so this change does not really seem helpful.

@keith-packard Did you have something else in mind?

[keith-packard]

@keith-packard Did you have something else in mind?

This is for building any of the 'posix' boards which use -m32 to compile 32-bit applications using glibc. In that mode, the default size of time_t is only 32 bits, which means the existing gettimeofday sample emits a compile-time warning when computing the upper 32-bits of the time value from this expression:

		printf("gettimeofday(): HI(tv_sec)=%d, LO(tv_sec)=%d, "
		       "tv_usec=%d\n\t%s\n", (unsigned int)(tv.tv_sec >> 32),
		       (unsigned int)tv.tv_sec, (unsigned int)tv.tv_usec,
		       asctime(&tm));

The simple fix would be to adjust this sample to avoid the right-shift on systems with 32-bit time_t, but I thought it would be better to ensure that every target board used 64-bit time_t as that's the best way to ensure all of the code is fully 2038 ready.

[aescolar]

@keith-packard Did you have something else in mind?

This is for building any of the 'posix' boards which use -m32 to compile 32-bit applications using glibc.

Ok. But, why do you want to build this application in native_posix? (This application cannot be used in native_posix.
This application cannot be built with any of the POSIX arch based boards when using the host libC.)

[keith-packard]

Ok. But, why do you want to build this application in native_posix? (This application cannot be used in native_posix. This application cannot be built with any of the POSIX arch based boards when using the host libC.)

Hrm. It was getting built by twister when I started this series, which is how I discovered this issue. And, it can be built directly:

$ west build -b native_posix samples/posix/gettimeofday/
...
[20/245] Building C object CMakeFiles/app.dir/src/main.c.obj
.../zephyr/samples/posix/gettimeofday/src/main.c: In function ‘_posix_zephyr_main’:
.../zephyr/samples/posix/gettimeofday/src/main.c:37:71: warning: right shift count >= width of type [-Wshift-count-overflow]
   37 |                        "tv_usec=%d\n\t%s\n", (unsigned int)(tv.tv_sec >> 32),
      |                                                                       ^~

In any case, it does seem like getting the native_posix target to use the 64 bit types is useful -- this found a couple of bugs in code that appears to only get tested under native_posix currently.

[de-nordic]

FS test changes ok.

[aescolar]

Some thoughts:

1. I'm not too fond of setting flags for an external C library, as they may change or the user may have another library. But if we do, we should have a longer comment here telling that these are glibc flags, and why we are doing it (including why we set _FILE_OFFSET_BITS).¨

2. Nit: I guess this could be guarded with a if (CONFIG_EXTERNAL_LIBC) (no need to set this otherwise, even though the potential harm is quite small)

[aescolar]

.. this found a couple of bugs in code that appears to only get tested under native_posix currently.

I'm very fine with fixing the typecasting issues. Though that can be sent as a separate PR.

[cfriedt]

LGTM for the gettimeofday sample

ok

[fabiobaltieri]

@keith-packard looks like the test config is skipping it right now

$ ./scripts/twister -v -p native_posix -T samples/posix
...
INFO    - 2/3 native_posix              samples/posix/gettimeofday/sample.posix.gettimeofday SKIPPED (runtime filter)

Looks like it's https://github.com/zephyrproject-rtos/zephyr/blob/main/samples/posix/gettimeofday/sample.yaml#L5, that's why we saw it in #63332

So that sample has been broken since the introduction, guess this should go in for 3.5?

[aescolar]

@fabiobaltieri

So that sample has been broken since the introduction, guess this should go in for 3.5?

For native_posix this sample does NOT work and should never be not filtered.
(independently of this PR change)

[fabiobaltieri]

For native_posix this sample does NOT work and should never be not filtered. (independently of this PR change)

Ok, does it need a platform_exclude then? https://github.com/zephyrproject-rtos/zephyr/pull/63332/files#diff-b226ecf1e2d64385ee6951d471595210c4b0a2e52e8e4f185f5a89a9d2b73728 would enable it otherwise as the current filter says nothing about the platform. Sorry if that makes no sense, I'm unfamiliar with the details. I guess, why would it not work with native_posix? :-)

[aescolar]

For native_posix this sample does NOT work and should never be not filtered. (independently of this PR change)

Ok, does it need a platform_exclude then?

Let's better discuss this in the other PR, it is a bit off-topic here :)

[keith-packard]

For native_posix this sample does NOT work and should never be not filtered. (independently of this PR change)

Ok, does it need a platform_exclude then?

Let's better discuss this in the other PR, it is a bit off-topic here :)

What would you like me to do with these patches then? Do we want to switch arch/posix to 64-bit time/offset on -m32 builds? If so, how should that deal with libc-specific definitions? Do we want to split out the minor fixes for 32-bit targets using 64-bit offset/time types? Or, should we just pend this discussion until after 3.5 and think about it then? To be clear, none of these patches are required for 3.5.0 as they do not fix any problems.

[aescolar]

What would you like me to do with these patches then?

I'd say, the patches for tests/subsys/fs/common/test_fs_basic.c
& samples/drivers/flash_shell/src/main.c
are uncontroversial fixes that should go in as is.

Wrt. samples/posix/gettimeofday/src/main.c I don't lean either way.

Wrt to arch/posix/CMakeLists.txt , if you think it is in general better to have those types match the embedded library size when building with the host library for 32bits we can take it in already as is. If you think that due to not enabling samples/gettimeofday in native_posix it is a moot change, we can just drop it. Or we can calmly talk about it.

[keith-packard]

Wrt. samples/posix/gettimeofday/src/main.c I don't lean either way.

It doesn't seem like this is the right place to test this condition anyways -- it's only getting tested on a subset of targets and only when the POSIX stuff is enabled. time_t is part of the C standard, so if we want to make sure everyone uses a 64-bit version, then we should have a test which checks that in every environment. Maybe as addition to the existing C library tests instead of here? I stuck it here because that's the code which failed (when shifting the time_t value by 32 bits).

I'll move this check to test/lib/c_lib and see what we think. It seems to fit in very nicely there. Having a simple check that avoids an obvious 2038 vulnerability seems pretty useful to me.

That's a longer series which also enables the C library tests on the native targets; #63941. That has no fixes for the time_t size, it just gets the tests building and running on the native platforms, skipping the time_t size check.

Wrt to arch/posix/CMakeLists.txt , if you think it is in general better to have those types match the embedded library size when building with the host library for 32bits we can take it in already as is. If you think that due to not enabling samples/gettimeofday in native_posix it is a moot change, we can just drop it. Or we can calmly talk about it.

It seems like using 64-bit types for time_t everywhere would be good, but I agree that the current mechanism to select that for glibc is not great. Something that does this would be required if we're testing the size of time_t as above. Figuring out something prettier than a random #define would be good.

[keith-packard]

Switched to draft until we sort out #63941 and whether we should be using 64-bit time/offset types on 32-bit native builds.

[github-actions]

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.