Re: [PATCH] nvptx: Add _ssize_t as _READ_WRITE_RETURN_TYPE in newlib/libc/include/sys/config.h

Hi Thomas!

Sorry for the late response. I had seen your email earlier yesterday,
but thought of replying
after I got back to my laptop (which I didn't, until today). And for
some reason, Slack fails to connect from the mobile app (could be an
ISP issue because I can connect from my laptop)
so I couldn't respond from my phone.

> I did wonder why placing this new '#define' here is a good choice -- but
> then, I've just looked at the current 'config.h' file, and found that
> placement of most of its content appears to be rather disorganized, so
> this placement here indeed should be as good as (almost) any other.
> (We shall of course be happy to hear from the newlib maintainers, if
> they've got any better advice.)

Yes, and I placed it there because the #define for __AMDGCN__ was just
above that, and both
are kind of related so mainly that's why. Moreover, __nvptx__ wasn't
#define -d anywhere else
so that location seemed plausible to me. I'd of course like to hear
from the newlib maintainers,
as you said.

> But: won't that change in isolation break the build, or at least warn,
> given that we've still got 'newlib/libc/machine/nvptx/misc.c':
>
>     int
>     read(int fd, void *buf, size_t count) {
>
> ..., that is, 'int' instead of the desire 'ssize_t' return type?
>
> In 'newlib/libc/machine/nvptx/write.c' we have:
>
>     _READ_WRITE_RETURN_TYPE write (int fd, const void *buf, size_t count)
>     {

That's a very real possibility, and in fact after some speculation
it's likely to occur since the
commit does not change the return type of read() to _READ_WRITE_RETURN_TYPE (aka
_ssize_t) whereas that's the one for write() in write.c. This causes
both to have a different
function signature, which will most definitely cause a CUDA error, if
not a compile time
error or warning, as per my earlier experience.

> Apparently, there isn't any diagnostic about the 'int' vs. 'ssize_t' for
> 'read', but I suggest that you change the return types of both 'read' and
> 'write' to the actual type, 'ssize_t'.  (Actually, I don't see any
> '-W[...]' flags get used for an combined-tree GCC/newlib build, huh...)

Okay, so I change it directly to ssize_t instead of _ssize_t?
(Actually, I followed the convention
of the other #define -s for _READ_WRITE_RETURN_TYPE from config.h).

> Another thing I've now noticed: nvptx' 'read' and 'write' implementations
> correctly use 'size_t count' (see cited above) -- but in 'config.h', as
> far as I can tell, we use the default 'int':
>
>     /* Define `count' parameter of read/write routines.  In POSIX, the `count'
>        parameter is "size_t" but legacy newlib code has been using "int" for some
>        time.  If not specified, "int" is defaulted.  */
>     #ifndef _READ_WRITE_BUFSIZE_TYPE
>     #define _READ_WRITE_BUFSIZE_TYPE int
>     #endif
>
> I suggest that for nvptx, next to your new '_READ_WRITE_RETURN_TYPE', you
> also add '#define _READ_WRITE_BUFSIZE_TYPE __size_t' (double underscore,
> per 'newlib/libc/include/sys/_types.h'), what do you think?
> (I'm slightly confused, as no configuration apart from Cygwin,
> 'winsup/cygwin/include/cygwin/config.h', appears to be doing this, but it
> still seems the right thing to do?)

Yeah, __size_t is indeed correct and I verified that from _types.h as
you said. And I'll be
incorporating this new #define too, and update count as in read() and write().

> Make that either an additional commit, or as part of this one, as you
> like.

Alright.

Another thing which I was thinking is that, now that my work in
newlib/libc/machine/nvptx/misc.c
is functional (albeit, not optimized or following some expected
standards per se) could I add this
one too, in my commit? That doesn't break the build, adds some
functionality, and I can still
continue working on it and sending my commits without breaking stuff.

If the above idea isn't possible at the moment, I can also just update
the function signatures
in newlib/libc/machine/nvptx/misc.c and
newlib/libc/machine/nvptx/write.c and leave everything
as it is.

Let me know what's your take on the above.

Best regards,
Arijit