Re: PATCH [3/n]: Add __snseconds_t and __SNSECONDS_T_TYPE

Re: PATCH [3/n]: Add __snseconds_t and __SNSECONDS_T_TYPE

[H.J. Lu]

On Thu, Mar 15, 2012 at 3:05 PM, Paul Eggert <eggert@cs.ucla.edu> wrote:
> On 03/15/2012 01:57 PM, H.J. Lu wrote:
>> What is the real consequence of using long long on tv_nsec,
>> except for not POSIX compliant? Will it lead to wrong code?
>
> It would break applications that do anything like this:
>
>  struct timespec t;
>  long *p = &t->tv_nsec;
>
> Such applications work fine now and conform to POSIX, but would

GCC will complain about "incompatible pointer type".

> either not compile or (worse) might compile and do the
> wrong thing, if tv_nsec were wider than 'long'.

We had a discussion on Linux kernel mailing list:

https://lkml.org/lkml/2012/2/8/408

We thought it was OK to have long long on tv_nsec.

-- 
H.J.

Re: PATCH [3/n]: Add __snseconds_t and __SNSECONDS_T_TYPE

[Paul Eggert]

On 03/15/2012 03:19 PM, H.J. Lu wrote:
> https://lkml.org/lkml/2012/2/8/408

That discussion does not seem to have considered the issue
of pointers, nor the issue of printf that Russ Allbery pointed out.
Here's an example from Kerrisk's "The Linux Programming Interface"
<http://man7.org/tlpi/code/online/dist/timers/t_clock_nanosleep.c.html>

            printf("... Remaining: %ld.%09ld",
                    (long) remain.tv_sec, remain.tv_nsec);

The proposed change breaks code like this.

>>  struct timespec t;
>>  long *p = &t->tv_nsec;
>> Such applications work fine now and conform to POSIX
> 
> GCC will complain about "incompatible pointer type".

True, and admittedly taking the address of tv_nsec is rarer than
printing it.  Still, it's just a warning and GCC goes ahead and builds
the program, and such warnings are often ignored.

> timespec is used in quite a few system calls. Checking all places
> which need to sign-extend is quite complex.

Many system calls copy timespec values from the kernel to the user;
these would be unaffected.  For syscalls that copy from the user
to the kernel, one could change glibc code like this:

  /* The Linux kernel can in some situations update the timeout value.          
     We do not want that so use a local variable.  */
  struct timespec tval;
  if (timeout != NULL)
    {
      tval = *timeout;
      timeout = &tval;
    }

(taken from glibc/sysdeps/unix/sysv/linux/pselect.c) to something like this:

  /* The Linux kernel can in some situations update the timeout value,
     or require a properly sign-extended timespec.  */
  struct timespec tval;
  if (timeout != NULL)
    {
      copy_timespec (&tval, timeout);
      timeout = &tval;
    }

where copy_timespec is an inline function that merely copies on existing
platforms, and also sign-extends tv_nsec on x32.  This doesn't appear complex,
though admittedly it does slow things down slightly on x32.

Another option, perhaps, would be to change the Linux kernel to
know about x32 binaries and to sign-extend tv_nsec inside the kernel,
when copying struct timespec objects from the user to the kernel.

Yet another option, I guess, would be to change POSIX so that tv_nsec could
be of type wider than 'long'.  However, this would seem to run afoul of
POSIX's intent, which is that system types like suseconds_t should
not be wider than 'long'; see
<http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/sys_types.h.html>.
This constraint is to support uses like 'printf'.
Given the likelihood of breaking programs, it may be better simply
to conform to POSIX in this area, rather than change POSIX.

x32 and width of blksize_t, suseconds_t

[Paul Eggert]

Come to think of it, the proposed x32 patch may have issues
with blksize_t and suseconds_t as well.  POSIX says that an x32
implementation must support at least one programming environment
(presumably settable via a feature test macro) where blksize_t
and suseconds_t are no wider than 'long'; see
<http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/sys_types.h.html>.
But if I understand things correctly, x32 glibc would
define these to be 'long long'.  This issue affects system
calls such as 'stat' and 'select'.

Re: x32 and width of blksize_t, suseconds_t

[H. Peter Anvin]

On 03/16/2012 01:19 AM, Paul Eggert wrote:
> Come to think of it, the proposed x32 patch may have issues
> with blksize_t and suseconds_t as well.  POSIX says that an x32
> implementation must support at least one programming environment
> (presumably settable via a feature test macro) where blksize_t
> and suseconds_t are no wider than 'long'; see
> <http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/sys_types.h.html>.
> But if I understand things correctly, x32 glibc would
> define these to be 'long long'.  This issue affects system
> calls such as 'stat' and 'select'.

That's just not going to happen, sorry.  Why on Earth is this a
requirement?  It makes no sense whatsoever.

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: x32 and width of blksize_t, suseconds_t

[Joseph S. Myers]

On Fri, 16 Mar 2012, H. Peter Anvin wrote:

> On 03/16/2012 01:19 AM, Paul Eggert wrote:
> > Come to think of it, the proposed x32 patch may have issues
> > with blksize_t and suseconds_t as well.  POSIX says that an x32
> > implementation must support at least one programming environment
> > (presumably settable via a feature test macro) where blksize_t
> > and suseconds_t are no wider than 'long'; see
> > <http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/sys_types.h.html>.
> > But if I understand things correctly, x32 glibc would
> > define these to be 'long long'.  This issue affects system
> > calls such as 'stat' and 'select'.
> 
> That's just not going to happen, sorry.  Why on Earth is this a
> requirement?  It makes no sense whatsoever.

It's probably to support code written for C90.  Note that it's the 
POSIX-conforming system as a whole that is to support such an environment 
- but it might be an environment that uses the traditional 32-bit or 
64-bit ABIs, it needn't be one using the x32 ABI.  "getconf 
POSIX_V7_WIDTH_RESTRICTED_ENVS" must print the names of the environments 
meeting this requirement; if one is "POSIX_V7_ILP32_OFF32", for example, 
then "getconf POSIX_V7_ILP32_OFF32_CFLAGS" will print the flags (maybe 
-m32) and likewise for _LDFLAGS and _LIBS.  In glibc this means defining 
appropriate macros in bits/environment.h to give the right flags - and 
also probably changing posix/confstr.c which has comments saying 
"Currently this means all environment which the system allows." (the x32 
patch series may need to adjust this in some way).

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: PATCH [3/n]: Add __snseconds_t and __SNSECONDS_T_TYPE

[H.J. Lu]

On Fri, Mar 16, 2012 at 12:52 AM, Paul Eggert <eggert@cs.ucla.edu> wrote:
> On 03/15/2012 03:19 PM, H.J. Lu wrote:
>> https://lkml.org/lkml/2012/2/8/408
>
> That discussion does not seem to have considered the issue
> of pointers, nor the issue of printf that Russ Allbery pointed out.
> Here's an example from Kerrisk's "The Linux Programming Interface"
> <http://man7.org/tlpi/code/online/dist/timers/t_clock_nanosleep.c.html>
>
>            printf("... Remaining: %ld.%09ld",
>                    (long) remain.tv_sec, remain.tv_nsec);
>
> The proposed change breaks code like this.

This will print the lower 32bit of remain.tv_nsec since for x32 each integer
argument lakes a register or a 8byte slot.

>>>  struct timespec t;
>>>  long *p = &t->tv_nsec;
>>> Such applications work fine now and conform to POSIX
>>
>> GCC will complain about "incompatible pointer type".
>
> True, and admittedly taking the address of tv_nsec is rarer than
> printing it.  Still, it's just a warning and GCC goes ahead and builds
> the program, and such warnings are often ignored.
>
>> timespec is used in quite a few system calls. Checking all places
>> which need to sign-extend is quite complex.
>
> Many system calls copy timespec values from the kernel to the user;
> these would be unaffected.  For syscalls that copy from the user
> to the kernel, one could change glibc code like this:
>
>  /* The Linux kernel can in some situations update the timeout value.
>     We do not want that so use a local variable.  */
>  struct timespec tval;
>  if (timeout != NULL)
>    {
>      tval = *timeout;
>      timeout = &tval;
>    }
>
> (taken from glibc/sysdeps/unix/sysv/linux/pselect.c) to something like this:
>
>  /* The Linux kernel can in some situations update the timeout value,
>     or require a properly sign-extended timespec.  */
>  struct timespec tval;
>  if (timeout != NULL)
>    {
>      copy_timespec (&tval, timeout);
>      timeout = &tval;
>    }
>
> where copy_timespec is an inline function that merely copies on existing
> platforms, and also sign-extends tv_nsec on x32.  This doesn't appear complex,
> though admittedly it does slow things down slightly on x32.
>
> Another option, perhaps, would be to change the Linux kernel to
> know about x32 binaries and to sign-extend tv_nsec inside the kernel,
> when copying struct timespec objects from the user to the kernel.
>
> Yet another option, I guess, would be to change POSIX so that tv_nsec could
> be of type wider than 'long'.  However, this would seem to run afoul of
> POSIX's intent, which is that system types like suseconds_t should
> not be wider than 'long'; see
> <http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/sys_types.h.html>.
> This constraint is to support uses like 'printf'.
> Given the likelihood of breaking programs, it may be better simply
> to conform to POSIX in this area, rather than change POSIX.

I'd prefer to change POSIX. This isn't the only place where x32 isn't
100% compatible with POSIX.

-- 
H.J.

Re: x32 and width of blksize_t, suseconds_t

[H.J. Lu]

On Fri, Mar 16, 2012 at 7:45 AM, Joseph S. Myers
<joseph@codesourcery.com> wrote:
> On Fri, 16 Mar 2012, H. Peter Anvin wrote:
>
>> On 03/16/2012 01:19 AM, Paul Eggert wrote:
>> > Come to think of it, the proposed x32 patch may have issues
>> > with blksize_t and suseconds_t as well.  POSIX says that an x32
>> > implementation must support at least one programming environment
>> > (presumably settable via a feature test macro) where blksize_t
>> > and suseconds_t are no wider than 'long'; see
>> > <http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/sys_types.h.html>.
>> > But if I understand things correctly, x32 glibc would
>> > define these to be 'long long'.  This issue affects system
>> > calls such as 'stat' and 'select'.
>>
>> That's just not going to happen, sorry.  Why on Earth is this a
>> requirement?  It makes no sense whatsoever.
>
> It's probably to support code written for C90.  Note that it's the
> POSIX-conforming system as a whole that is to support such an environment
> - but it might be an environment that uses the traditional 32-bit or
> 64-bit ABIs, it needn't be one using the x32 ABI.  "getconf
> POSIX_V7_WIDTH_RESTRICTED_ENVS" must print the names of the environments
> meeting this requirement; if one is "POSIX_V7_ILP32_OFF32", for example,
> then "getconf POSIX_V7_ILP32_OFF32_CFLAGS" will print the flags (maybe
> -m32) and likewise for _LDFLAGS and _LIBS.  In glibc this means defining
> appropriate macros in bits/environment.h to give the right flags - and
> also probably changing posix/confstr.c which has comments saying
> "Currently this means all environment which the system allows." (the x32
> patch series may need to adjust this in some way).
>

I took a look at POSIX getconf.  It only supports one 32bit ABI and one 64bit
ABI.  I can't find a way to support another 32bit ABI. I decided not to change
it for x32.  If you use POSIX getconf to select a 32bit ABI, you will get IA32.

-- 
H.J.

Re: PATCH [3/n]: Add __snseconds_t and __SNSECONDS_T_TYPE

[Paul Eggert]

On 03/16/2012 08:49 AM, H.J. Lu wrote:
> I'd prefer to change POSIX. This isn't the only place where x32 isn't
> 100% compatible with POSIX.

If this approach is taken, the compatibility issues with tv_nsec,
blksize_t, and suseconds_t should be documented in the glibc manual,
in areas where programmers using the interfaces are likely to find
out about the problem.  Could you propose a patch to do that?

It should be possible to change POSIX at some point, once the POSIX folks
decide that C90 compatibility is no longer an issue.  If POSIX
conformance is not a goal of x32, then this is not an urgent matter --
which is probably just as well, as I think they still worry about C90.

In the meantime, I guess applications will just have to know not to take
the address of a tv_nsec member.  I just now added a note
about this in the Gnulib portability manual, where it talks about
glitches on systems that don't conform to POSIX:

http://git.savannah.gnu.org/cgit/gnulib.git/commit/?id=47834c92f8122f4ba5e6fca5199a4611425bfe69

Re: PATCH [3/n]: Add __snseconds_t and __SNSECONDS_T_TYPE

[H.J. Lu]

On Fri, Mar 16, 2012 at 11:54 AM, Paul Eggert <eggert@cs.ucla.edu> wrote:
> On 03/16/2012 08:49 AM, H.J. Lu wrote:
>> I'd prefer to change POSIX. This isn't the only place where x32 isn't
>> 100% compatible with POSIX.
>
> If this approach is taken, the compatibility issues with tv_nsec,
> blksize_t, and suseconds_t should be documented in the glibc manual,
> in areas where programmers using the interfaces are likely to find
> out about the problem.  Could you propose a patch to do that?

I will do that once we know the full scope for POSIX issue under
x32.

> It should be possible to change POSIX at some point, once the POSIX folks
> decide that C90 compatibility is no longer an issue.  If POSIX
> conformance is not a goal of x32, then this is not an urgent matter --
> which is probably just as well, as I think they still worry about C90.
>
> In the meantime, I guess applications will just have to know not to take
> the address of a tv_nsec member.  I just now added a note
> about this in the Gnulib portability manual, where it talks about
> glitches on systems that don't conform to POSIX:
>
> http://git.savannah.gnu.org/cgit/gnulib.git/commit/?id=47834c92f8122f4ba5e6fca5199a4611425bfe69

Thanks.


-- 
H.J.