lib: libc: minimal: Define off_t as intptr_t

[stephanosio]

The off_t type, which is specified by the POSIX standard as a signed
integer type representing file sizes, was defined as long or int
depending on the target architecture without a clear explanation on why
it was defined as such.

While the POSIX standard does not specify the size requirement of the
off_t type, it generally corresponds to the size of a pointer in
practice, mainly because the optimal file handling size is closely tied
to the native pointer size.

For this reason, this commit removes the per-architecture off_t
definition and defines it as intptr_t such that its size always
matches the native pointer size.

Note that the toolchain-defined __INTPTR_TYPE__ macro is used instead
of the intptr_t typedef as per the common convention used in the C
standard library headers.

Signed-off-by: Stephanos Ioannidis [email protected]

NOTE: It was a deliberate decision to not implement the support for _FILE_OFFSET_BITS because it is a Linux/glibc-specific construct, and neither the newlib nor the picolibc supports it. It may be desirable to implement the support for this macro (or something similar) in the future when zephyrproject-rtos/sdk-ng#350 is implemented.

[stephanosio]

INFO - 1/8 nrf52840dk_nrf52840 sample.bootloader.mcuboot.serial_recovery FAILED Build failure (build)
...
/__w/zephyr/bootloader/mcuboot/boot/boot_serial/src/boot_serial.c:304:18: error: format '%x' expects argument of type 'unsigned int', but argument 2 has type 'off_t' {aka 'long int'} [-Werror=format=]
304 | BOOT_LOG_INF("Erasing range 0x%x:0x%x", start, start + size - 1);
| ^~~~~~~~~~~~~~~~~~~~~~~~~ ~~~~~
| |
| off_t {aka long int}

It looks like mcuboot incorrectly assumes that off_t is always int. Since printf does not have a native format specifier for the off_t type (because it is not part of the C standard and is only part of the POSIX standard), we will need to cast it to intmax_t and print it using %jd.

[stephanosio]

It looks like mcuboot incorrectly assumes that off_t is always int.

Opened mcu-tools/mcuboot#1413 fixing this issue to the upstream mcuboot repository.

@nvlsianpu I assume you will handle the Zephyr mcuboot fork sync and west.yml update once that PR is merged upstream?

[keith-packard]

I wrote a comment about this in #46443 (comment) (sorry for missing this PR when reviewing that series).

In short -- I'd probably use a bare long here; that matches the history of off_t and gives the result you want on all existing Zephyr targets, and as __INTPTR_TYPE__ is just a #define for long int in zephyr_stdint.h, I'm not seeing a huge benefit in adding a layer of indirection here.

[ngboonkhai]

Hi @stephanosio, can you help to reopen this PR and resolve the issue?

I adding the patch at the QSPI PR was to check if it fix the issue.

[stephanosio]

as __INTPTR_TYPE__ is just a #define for long int in zephyr_stdint.h, I'm not seeing a huge benefit in adding a layer of indirection here.

The plan is to get rid of the abomination that zephyr_stdint.h is: #46708 (comment)

Hard-coding off_t as (and assuming off_t to be) a particular integer type is a very flawed approach -- in that case, what is the point of having off_t? we might as well just use long everywhere.

Ideally, the size of the off_t type should be configurable by the developer, and any code making use of it should never assume the type of off_t to be defined in terms of a particular integer type (e.g. long). But, as noted in #47612 (comment), making off_t configurable is beyond the scope of this PR:

It was a deliberate decision to not implement the support for _FILE_OFFSET_BITS because it is a Linux/glibc-specific construct, and neither the newlib nor the picolibc supports it. It may be desirable to implement the support for this macro (or something similar) in the future when zephyrproject-rtos/sdk-ng#350 is implemented.

So given that whatever we do here is going to be flawed in one way or another, and this is approaching the realm of personal preferences, let us settle this by a vote:

• 👍 = off_t as intptr_t
• 👎 = off_t as long

[andyross]

I'm inclined to pick @stephanosio 's side here. Definitely we should pick one type for simplicity reasons. I'm not a fan of "bare long" as long as the win64 ABI still exists in the world (where "long" is 32 bits!); intptr_t is going to be identical in practice on all our existing systems and has the advantage of not breaking in surprising ways if someone decides to build with a weird-but-still-popular toolchain like MSVC.

I'm sensitive to the fact that this would change the size of the type on existing 64 bit platforms, but that seems mostly tolerable given that existing Zephyr filesystem APIs are still very rarely used. I wouldn't cry if we just picked int32_t or whatever either.

[keith-packard]

I'm inclined to pick @stephanosio 's side here. Definitely we should pick one type for simplicity reasons. I'm not a fan of "bare long" as long as the win64 ABI still exists in the world (where "long" is 32 bits!); intptr_t is going to be identical in practice on all our existing systems and has the advantage of not breaking in surprising ways if someone decides to build with a weird-but-still-popular toolchain like MSVC.

As you've said, it really boils down to a matter of taste -- pick whichever you like best. I think the main question is whether the minimal C library should use something which maps to long or int on existing systems -- long/intptr_t matches POSIX best, int/int32_t means ILP32 and LP64 targets would use the same size. I'd suggest long/intptr_t as that makes porting POSIX code from other environments easier, but that has ABI/API consequences on ILP32/LP64 targets which you might prefer to avoid.

[ngboonkhai]

Any updates? can we wrap up and get this fixed?
@stephanosio

[stephanosio]

Rebased

[keith-packard]

in case more review helps here.