Re: [PATCH] Avoid ARM SWI Seek when querying file position

["Richard Earnshaw (lists)" <Richard dot Earnshaw at arm dot com>]

On 31/08/18 12:30, Andy Koppe wrote:
> On 31 August 2018 at 12:10, Andy Koppe wrote:
>> Issuing an ARM semi-hosting Seek command when just querying file
>> position with SEEK_CUR and offset zero is unnecessary, because unlike
>> the lseek() Unix system call the Seek command does not actually return
>> the file position. For that reason, syscalls.c for ARM keeps track of
>> file position in the 'poslog', so we can just return that.
>>
>> Moreover, since the Seek command only accepts an absolute file position,
>> SEEK_CUR operations are implemented by adding the relative offset to the
>> position in the poslog. If the host implements non-binary files with
>> implicit carriage return characters but doesn't discount those implicit
>> CRs when implementing Seek (by just mapping straight to Windows file
>> operations), this actually ended up wrongly changing file position when
>> using SEEK_CUR with offset zero or functions like ftell() or fgetpos()
>> that are based on that.
>>
>> The ARM semi-hosting Seek command is documented at
>> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0040d/BACDFGCG.html
>>
>> (There's a wider problem in that using the position returned by the
>> likes of ftell() in a later seek operation gets you to the wrong place
>> if the host adds implicit CRs without discounting them in Seek
>> commands. Not sure there's anything that can be done about it on the
>> newlib side though.)
> 
> Slightly amended patch attached: Use off_t rather than int for the poslog.
> 
> 
> 0001-Avoid-ARM-SWI-Seek-when-querying-file-position.patch
> 
> 
> From 7fd1cd9e81fe5ab8f2c3524384431ba0d58ca151 Mon Sep 17 00:00:00 2001
> From: Andy Koppe <andy.koppe@gmail.com>
> Date: Fri, 31 Aug 2018 12:26:02 +0100
> Subject: [PATCH] Avoid ARM SWI Seek when querying file position
> 
> Issuing an ARM semi-hosting Seek command when just querying file
> position with SEEK_CUR and offset zero is unnecessary, because unlike
> the lseek() Unix system call the Seek command does not actually return
> the file position. For that reason, syscalls.c for ARM keeps track of
> file position in the 'poslog', so we can just return that.
> 
> Moreover, since the Seek command only accepts an absolute file position,
> SEEK_CUR operations are implemented by adding the relative offset to the
> position in the poslog. If the host implements non-binary files with
> implicit carriage return characters but doesn't discount those implicit
> CRs when implementing Seek (by just mapping straight to Windows file
> operations), this actually ended up wrongly changing file position when
> using SEEK_CUR with offset zero or functions like ftell() or fgetpos()
> that are based on that.
> 
> Also, use off_t rather than int for the poslog.
> ---
>  newlib/libc/sys/arm/syscalls.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/newlib/libc/sys/arm/syscalls.c b/newlib/libc/sys/arm/syscalls.c
> index e0cf0ac65..6e70467ea 100644
> --- a/newlib/libc/sys/arm/syscalls.c
> +++ b/newlib/libc/sys/arm/syscalls.c
> @@ -76,7 +76,7 @@ static int monitor_stderr;
>  typedef struct
>  {
>    int handle;
> -  int pos;
> +  off_t pos;
>  }
>  poslog;
>  
> @@ -234,9 +234,16 @@ _swilseek (int file, off_t ptr, int dir)
>  
>    if (dir == SEEK_CUR)
>      {
> +      off_t pos;
>        if (slot == MAX_OPEN_FILES)
>  	return -1;
> -      ptr = openfiles[slot].pos + ptr;
> +      pos = openfiles[slot].pos;
> +
> +      /* Avoid SWI SEEK command when just querying file position. */
> +      if (ptr == 0)
> +	return pos;
> +
> +      ptr += pos;
>        dir = SEEK_SET;
>      }
>  
> 

We probably need the same patch in libgloss/arm/syscalls.c

R.