Re: [PATCH] libgloss/arm: report semihosted directories correctly

On 2026-08-14 11:25, Jonathan Wakely wrote:
> 
> 
> On Fri, 14 Aug 2026, 09:51 Torbjörn SVENSSON, <[email protected] <mailto:[email protected]>> wrote:
> 
>     In GCC r17-2048-gcc195f7b11a406, support for resolving /etc/localtime symlinks
>     was added.  As a consequense, when using semihosting for arm-none-eabi, the
>     resolution fails due to that semihosting unconditionally consider all paths as
>     character devices.  This commit tries to work around that by inferring as much
>     stat details as possible from available APIs in the semihosting specification.
> 
>     With this change, I see that the failure is resolved, but there are a few
>     tests that have been marked as xfail that now passes.  I will follow up
>     with a patch for GCC as soon as this change is merged.
> 
>     These are the changes when testing with GCC r17-2833-g32657f29f91871.
> 
>     Without patch:
>     PASS: 27_io/basic_filebuf/sgetn/char/1-in.cc  -std=gnu++20 (test for excess errors)
>     XFAIL: 27_io/basic_filebuf/sgetn/char/1-in.cc  -std=gnu++20 execution test
>     PASS: 27_io/basic_filebuf/sgetn/char/1-io.cc  -std=gnu++20 (test for excess errors)
>     XFAIL: 27_io/basic_filebuf/sgetn/char/1-io.cc  -std=gnu++20 execution test
>     PASS: 27_io/basic_filebuf/sgetn/char/2-in.cc  -std=gnu++20 (test for excess errors)
>     XFAIL: 27_io/basic_filebuf/sgetn/char/2-in.cc  -std=gnu++20 execution test
>     PASS: 27_io/basic_filebuf/sgetn/char/2-io.cc  -std=gnu++20 (test for excess errors)
>     XFAIL: 27_io/basic_filebuf/sgetn/char/2-io.cc  -std=gnu++20 execution test
>     PASS: 27_io/basic_filebuf/underflow/wchar_t/11603.cc  -std=gnu++20 (test for excess errors)
>     FAIL: 27_io/basic_filebuf/underflow/wchar_t/11603.cc  -std=gnu++20 execution test
>     PASS: 27_io/basic_istream/readsome/char/6746-2.cc  -std=gnu++20 (test for excess errors)
>     XFAIL: 27_io/basic_istream/readsome/char/6746-2.cc  -std=gnu++20 execution test
>     PASS: 27_io/basic_istream/readsome/wchar_t/6746-2.cc  -std=gnu++20 (test for excess errors)
>     XFAIL: 27_io/basic_istream/readsome/wchar_t/6746-2.cc  -std=gnu++20 execution test
>     PASS: std/time/tzdb/1.cc  -std=gnu++20 (test for excess errors)
>     FAIL: std/time/tzdb/1.cc  -std=gnu++20 execution test
>     PASS: std/time/zoned_time/custom.cc  -std=gnu++20 (test for excess errors)
>     FAIL: std/time/zoned_time/custom.cc  -std=gnu++20 execution test
> 
>     With patch:
>     PASS: 27_io/basic_filebuf/sgetn/char/1-in.cc  -std=gnu++20 (test for excess errors)
>     XPASS: 27_io/basic_filebuf/sgetn/char/1-in.cc  -std=gnu++20 execution test
>     PASS: 27_io/basic_filebuf/sgetn/char/1-io.cc  -std=gnu++20 (test for excess errors)
>     XPASS: 27_io/basic_filebuf/sgetn/char/1-io.cc  -std=gnu++20 execution test
>     PASS: 27_io/basic_filebuf/sgetn/char/2-in.cc  -std=gnu++20 (test for excess errors)
>     XPASS: 27_io/basic_filebuf/sgetn/char/2-in.cc  -std=gnu++20 execution test
>     PASS: 27_io/basic_filebuf/sgetn/char/2-io.cc  -std=gnu++20 (test for excess errors)
>     XPASS: 27_io/basic_filebuf/sgetn/char/2-io.cc  -std=gnu++20 execution test
>     PASS: 27_io/basic_filebuf/underflow/wchar_t/11603.cc  -std=gnu++20 (test for excess errors)
>     PASS: 27_io/basic_filebuf/underflow/wchar_t/11603.cc  -std=gnu++20 execution test
>     PASS: 27_io/basic_istream/readsome/char/6746-2.cc  -std=gnu++20 (test for excess errors)
>     XPASS: 27_io/basic_istream/readsome/char/6746-2.cc  -std=gnu++20 execution test
>     PASS: 27_io/basic_istream/readsome/wchar_t/6746-2.cc  -std=gnu++20 (test for excess errors)
>     XPASS: 27_io/basic_istream/readsome/wchar_t/6746-2.cc  -std=gnu++20 execution test
>     PASS: std/time/tzdb/1.cc  -std=gnu++20 (test for excess errors)
>     PASS: std/time/tzdb/1.cc  -std=gnu++20 execution test
>     PASS: std/time/zoned_time/custom.cc  -std=gnu++20 (test for excess errors)
>     PASS: std/time/zoned_time/custom.cc  -std=gnu++20 execution test
> 
> 
>     These is also an implementation choise to be made.
>     Would it be prefered to use stack (like I do in this patch) or would it be
>     better to allocated the temporary string in path_is_dir on the heap using
>     malloc?  Regardless of solution, semihosting is expensive, so I don't think
>     it really matters what solution is used. I choose stack based for the first
>     version as it does not pull in any extra functions.
> 
>     Ok to push to master as-is or should I change to a heap based implementation?
> 
>     Kind regards,
>     Torbjörn
> 
>     --
> 
>     Arm semihosting does not provide a real stat operation, so _stat has to
>     infer file information from the operations that are available.
> 
>     The previous code always treated an opened path as a regular file, and
>     _swistat then added S_IFCHR unconditionally.  That meant directories were
>     not reported as directories, and the file type bits could end up
>     describing more than one kind of file at once.
> 
>     Use _isatty to identify character devices, use SYS_FLEN for regular
>     files, and probe "path/." so _stat can recognize directories.
> 
>     This gives callers such as std::filesystem::canonical enough information
>     to handle semihosted paths correctly.
> 
>     Discovered, and reported, in
>     https://gcc.gnu.org/pipermail/gcc-patches/2026-August/727185.html <https://gcc.gnu.org/pipermail/gcc-patches/2026-August/727185.html>
> 
>     Signed-off-by: Torbjörn SVENSSON <[email protected] <mailto:[email protected]>>
>     ---
>       libgloss/arm/syscalls.c | 79 ++++++++++++++++++++++++++++++++++++++---
>       1 file changed, 74 insertions(+), 5 deletions(-)
> 
>     diff --git a/libgloss/arm/syscalls.c b/libgloss/arm/syscalls.c
>     index 710a741ee..162059fc0 100644
>     --- a/libgloss/arm/syscalls.c
>     +++ b/libgloss/arm/syscalls.c
>     @@ -15,6 +15,7 @@
>       #include <reent.h>
>       #include <unistd.h>
>       #include <sys/wait.h>
>     +#include <limits.h>
>       #include "swi.h"
> 
>       /* Forward prototypes.  */
>     @@ -46,6 +47,7 @@ void  initialise_monitor_handles (void);
>       static int     checkerror      (int);
>       static int     error           (int);
>       static int     get_errno       (void);
>     +static int     path_is_dir     (const char *);
> 
>       /* Semihosting utilities.  */
>       static void initialise_semihosting_exts (void);
>     @@ -346,6 +348,46 @@ checkerror (int result)
>         return result;
>       }
> 
>     +/* Check if the given path is likely a directory. */
>     +static int
>     +path_is_dir (const char *path)
>     +{
>     +  size_t path_len;
>     +  size_t dir_len;
>     +  char dir_path[PATH_MAX + 3];
>     +  int fd;
>     +
>     +  path_len = strlen (path);
>     +  if (path_len == 0)
>     +    return 0;
>     +
>     +  if (path_len + 3 > sizeof (dir_path))
>     +    {
>     +      errno = ENAMETOOLONG;
>     +      return -1;
>     +    }
>     +
>     +  /* Build a new string that ends with "/.". */
>     +  memcpy (dir_path, path, path_len);
>     +  dir_len = path_len;
>     +  if (dir_path[dir_len - 1] != '/')
> 
> 
> Just an observation...
> 
> This check means you will add /. to a path ending with /.. which is redundant, but I think that's OK.
> 
> If strlen(dir_path) == sizeof(dir_path) and it ends with /.. then there's no need to append anything, but this function will return ENAMETOOLONG. Again, I think that's OK. It's an edge case and the path is already longer than PATH_MAX.

Indeed, this is true.
The reason why I did not bother is that it makes the logic a bit more complex and I think the corner case could be dismissed as working anyway.
Another thing that hit me with your feedback is what would happen when the PC side of the semihosting is running Windows. Is it okay to mix front- and back-slashes or do we need to normalize the path too?
If we normalize to one of them, then should it be front or back?
It's also going to be hard to know what is expected on the other side for the semihosting with this code running on an Arm target...

I do not consider this as a perfect solution, but I cannot come up with something better that would not require an update of the specification and the software on the PC side.

Kind regards,
Torbjörn

> 
> 
>     +    dir_path[dir_len++] = '/';
>     +  dir_path[dir_len++] = '.';
>     +  dir_path[dir_len] = '\0';
>     +
>     +  /* Try to open the directory. */
>     +  fd = _open (dir_path, O_RDONLY);
>     +
>     +  /* Error means that path either does not exist
>     +   * or is not a directory. */
>     +  if (fd == -1)
>     +    return 0;
>     +
>     +  /* Not interested in the error.  */
>     +  _close (fd);
>     +  return 1;
>     +}
>     +
>       /* fh, is a valid internal file handle.
>          ptr, is a null terminated string.
>          len, is the length in bytes to read.
>     @@ -738,10 +780,14 @@ _swistat (int fd, struct stat * st)
>             return -1;
>           }
> 
>     -  /* Always assume a character device,
>     -     with 1024 byte blocks. */
>     -  st->st_mode |= S_IFCHR;
>         st->st_blksize = 1024;
>     +  if (_isatty (fd))
>     +    {
>     +      /* Assume character device. */
>     +      st->st_mode |= S_IFCHR;
>     +      return 0;
>     +    }
>     +
>       #ifdef ARM_RDI_MONITOR
>         res = checkerror (do_AngelSWI (AngelSWI_Reason_FLen, &pfd->handle));
>       #else
>     @@ -752,7 +798,19 @@ _swistat (int fd, struct stat * st)
>         checkerror (res);
>       #endif
>         if (res == -1)
>     -    return -1;
>     +    {
>     +      if (errno == EISDIR)
>     +       {
>     +         /* Path is a directory. */
>     +         st->st_mode |= S_IFDIR;
>     +         return 0;
>     +       }
>     +      return -1;
>     +    }
>     +
>     +  /* Assume regular file. */
>     +  st->st_mode |= S_IFREG;
>     +
>         /* Return the file size. */
>         st->st_size = res;
>         return 0;
>     @@ -770,12 +828,23 @@ _stat (const char *fname, struct stat *st)
>       {
>         int fd, res;
>         memset (st, 0, sizeof (* st));
>     +  res = path_is_dir (fname);
>     +  if (res == 1)
>     +    {
>     +      st->st_mode = S_IFDIR | S_IREAD | S_IEXEC;
>     +      st->st_blksize = 1024;
>     +      return 0;
>     +    }
>     +  else if (res == -1)
>     +    return -1;
>     +
>         /* The best we can do is try to open the file readonly.  If it exists,
>            then we can guess a few things about it.  */
>         if ((fd = _open (fname, O_RDONLY)) == -1)
>           return -1;
>     -  st->st_mode |= S_IFREG | S_IREAD;
>         res = _swistat (fd, st);
>     +  if (res == 0)
>     +    st->st_mode |= S_IREAD;
>         /* Not interested in the error.  */
>         _close (fd);
>         return res;
>     -- 
>     2.43.0
>