rust-lang / libc

Raw bindings to platform APIs for Rust
https://docs.rs/libc
Apache License 2.0
2.06k stars 1.03k forks source link

Incorrect newlib `stat` struct definition #2795

Open zetanumbers opened 2 years ago

zetanumbers commented 2 years ago

https://github.com/rust-lang/libc/blob/1a311288a8f7523bfe5f88f57c6e96724eb16842/src/unix/newlib/generic.rs#L8-L26

newlib/libc/include/sys/stat.h

struct  stat 
{
  dev_t         st_dev;
  ino_t         st_ino;
  mode_t        st_mode;
  nlink_t       st_nlink;
  uid_t         st_uid;
  gid_t         st_gid;
  dev_t         st_rdev;
  off_t         st_size;
#if defined(__svr4__) && !defined(__PPC__) && !defined(__sun__)
  time_t        st_atime;
  time_t        st_mtime;
  time_t        st_ctime;
#else
  struct timespec st_atim;
  struct timespec st_mtim;
  struct timespec st_ctim;
  blksize_t     st_blksize;
  blkcnt_t      st_blocks;
#if !defined(__rtems__)
  long          st_spare4[2];
#endif
#endif
};

#if !(defined(__svr4__) && !defined(__PPC__) && !defined(__sun__))
#define st_atime st_atim.tv_sec
#define st_ctime st_ctim.tv_sec
#define st_mtime st_mtim.tv_sec
#endif

I would like to see defined(__svr4__) && !defined(__PPC__) && !defined(__sun__) case being handled for targets without nanoseconds.

JohnTitor commented 2 years ago

@AzureMarker Could you take a look at this? A breaking change (changing some fields on some env) would be required to fix it, but I guess you're using this struct.

zetanumbers commented 2 years ago

Oh and I believe after some digging some months ago, i found no rust target that would satisfy defined(__svr4__) && !defined(__PPC__) && !defined(__sun__). But still.

AzureMarker commented 2 years ago

We're only using the struct for the "horizon" OS (Nintendo 3DS): https://github.com/rust-lang/libc/blob/1a311288a8f7523bfe5f88f57c6e96724eb16842/src/unix/newlib/horizon/mod.rs#L59-L74

I don't think we need any changes for that platform at least.

cc: @Meziu, @ian-h-chamberlain in case I miss something.

ian-h-chamberlain commented 2 years ago

Yes, I believe we added the newlib::horizon::stat struct specifically to avoid breaking anything that might have been using the existing newlib::stat struct.

Based on this:

Oh and I believe after some digging some months ago, i found no rust target that would satisfy defined(__svr4__) && !defined(__PPC__) && !defined(__sun__). But still.

Maybe we should maybe just take the horizon::stat definition and use it as the generic newlib definition, and if any platforms that meet the above criteria are added, they would need to create their own definition and use that (maybe a cfg_if! block that checks for some equivalent of __svr4__?).

Does that make sense?