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
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
EiB file sizes).
I don't think we should try to interpret negative values because it's
ambiguous in the spec, the hosts known to return such values do so
because of flaws in their implementation, and large files themselves
have dubious merit.
--Sean
