On 10/17/25 14:00, 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.
It says that any other value is the current length of the file object.
And we should not support negative lengths.
> 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.
https://review.openocd.org/c/openocd/+/9173
https://lore.kernel.org/qemu-devel/[email protected]
I think the latter may be held up on the list, but hopefully it will arrive on
lore soon.
--Sean
