On Fri, Oct 17, 2025 at 08:00:19PM +0200, Heinrich Schuchardt wrote:
> On 10/17/25 19:08, Sean Anderson wrote:
> > On 10/15/25 12:28, Tom Rini wrote:
> > > On Thu, Oct 02, 2025 at 10:36:07AM -0400, Sean Anderson wrote:
> > > > On 10/2/25 05:52, Heinrich Schuchardt wrote:
> > > > > On 10/2/25 11:39, Andrew Goodbody wrote:
> > > > > > After calling a function that can return an error, the test to
> > > > > > detect
> > > > > > that error should use the return value not a different variable.
> > > > > > Fix it.
> > > > > >
> > > > > > This issue was found by Smatch.
> > > > > >
> > > > >
> > > > > Fixes: f676b45151c3 ("fs: Add semihosting filesystem")
> > > > >
> > > > > > Signed-off-by: Andrew Goodbody <[email protected]>
> > > > > > ---
> > > > > > fs/semihostingfs.c | 2 +-
> > > > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/fs/semihostingfs.c b/fs/semihostingfs.c
> > > > > > index
> > > > > > 77e39ca407e4d240a1fd573497c5b6b908816454..9d7a136b9ba9b035545b34b31df58e2d65de7db9
> > > > > > 100644
> > > > > > --- a/fs/semihostingfs.c
> > > > > > +++ b/fs/semihostingfs.c
> > > > > > @@ -35,7 +35,7 @@ static int smh_fs_read_at(const char *filename,
> > > > > > loff_t pos, void *buffer,
> > > > > > }
> > > > > > if (!maxsize) {
> > > > > > size = smh_flen(fd);
> > > > > > - if (ret < 0) {
> > > > > > + if (size < 0) {
> > > > >
> > > > > The ARM specification
> > > > > (https://cas5-0-urlprotect.trendmicro.com:443/wis/clicktime/v1/query?url=https%3a%2f%2fdeveloper.arm.com%2fdocumentation%2fdui0203%2fj%2fsemihosting%2fsemihosting%2doperations%2fsys%2dflen%2d%2d0x0c%2d&umid=f17ab8fc-2d9d-4cce-91e2-e7e66f117613&rct=1759398729&auth=d807158c60b7d2502abde8a2fc01f40662980862-e0799d2d4d61f93cae033d54733c8fa50ef84918)
> > > > > has:
> > > > >
> > > > > SYS_FLEN (0x0C)
> > > > >
> > > > > Returns the length of a specified file.
> > > > > On exit, R0 contains:
> > > > > the current length of the file object, if the call is successful
> > > > > -1 if an error occurs.
> > > > >
> > > > > Please, consider that the file length on 32bit systems may exceed
> > > > > 2^31 You must not consider this as an error.
> > > > >
> > > > > %s/if (size < 0)/if (size == -1L)/
> > > >
> > > > This cannot occur because on a 32-bit system SYS_FLEN only returns
> > > > 32-bits of information. The host must detect files that exceed 2 GiB in
> > > > size and return -1 (simulating EOVERFLOW).
> > >
> > > OK, but this is for 64-bit systems too, isn't it?
> > >
> >
> > Sorry, I worded that poorly. What I mean is that the spec says
> >
> > | On exit, R0 contains:
> > | the current length of the file object, if the call is successful
> > | -1 if an error occurs.
> >
> > which implies that R0 is a signed integer (otherwise they would have
>
> The only thing that is certain here is that the register will be filled with
> (unsigned long)-1L if an error occurs and that any other value is not an
> error.
>
> Everything else is your personal interpretation which does not match the
> existing implementation by Linaro in QEMU.
>
> > specified 0xffffffff or 0xffffffffffffffff as the error value). So
> > negative values (other than -1) indicate a negative file size (or are
> > unspecified).
> >
> > However, both OpenOCD and QEMU just take the return from stat(2) and
> > copy it into R0. With a 32-bit host there is no problem since stat(2)
> > will itself fail for files over 2 GiB. With a 32-bit target, this
> > produces erroneous values for files larger than 4 GiB. Files sizes
> > between 2 and 4 GiB could be recovered, but I don't think we should do
> > that. On a 64-bit target, there is again no ambiguity (at least until 8
>
> You still don't provide any reason why you don't want to support files in
> the range [2 GiB, 4GiB[ on a 32 bit system.
>
> Instead of restricting U-Boot we should add a fix in QEMU to report an error
> if the filesize exceeds ULONG_MAX - 1.
>
> This is the only way we can ensure that a 5 GiB file is not reported as 1
> GiB in U-Boot as we see it today.I think this means that at best the specification is ambiguous, so it's important to see what all, or at least the most common, implementations of the spec have done, and then what changes are needed and where. -- Tom
signature.asc
Description: PGP signature
