conformtest: Add x32 XFAILs for mq_attr element types (bug 21279) [committed]

Message ID alpine.DEB.2.20.1703202130510.14934@digraph.polyomino.org.uk
State New
Headers show

Commit Message

Joseph Myers March 20, 2017, 9:31 p.m.
POSIX specifies long as the type of elements of struct mq_attr.  For
x32, they are __syscall_slong_t (i.e. long long).  This patch XFAILs
the corresponding tests for x32 in the conformtest expectations (the
bug should not be closed without an actual fix).

Tested with build-many-glibcs.py.  Committed.

2017-03-20  Joseph Myers  <joseph@codesourcery.com>

	[BZ #21279]
	* sysdeps/unix/sysv/linux/x86_64/x32/Makefile
	[$(subdir) = conform] (conformtest-xfail-conds): Update comment.
	* conform/data/mqueue.h-data (mq_attr.mq_flags): XFAIL for
	x86_64-x32-linux.
	(mq_attr.mq_maxmsg): Likewise.
	(mq_attr.mq_msgsize): Likewise.
	(mq_attr.mq_curmsgs): Likewise.

Comments

Zack Weinberg March 22, 2017, 8:27 p.m. | #1
On Mon, Mar 20, 2017 at 5:31 PM, Joseph Myers <joseph@codesourcery.com> wrote:
> POSIX specifies long as the type of elements of struct mq_attr.

I am extremely disappointed to learn that POSIX didn't just make this
mistake with struct timespec.

zw
Joseph Myers March 22, 2017, 9:06 p.m. | #2
On Wed, 22 Mar 2017, Zack Weinberg wrote:

> On Mon, Mar 20, 2017 at 5:31 PM, Joseph Myers <joseph@codesourcery.com> wrote:
> > POSIX specifies long as the type of elements of struct mq_attr.
> 
> I am extremely disappointed to learn that POSIX didn't just make this
> mistake with struct timespec.

I think it's a lot more plausibly a mistake for mq_attr than for timespec 
(and thus a lot more plausible that a future POSIX version might add 
typedefs here), since as far as I can tell valid values for these fields 
could exceed the width of long, but they clearly can't for tv_nsec.  
(Though it's arguably similarly a mistake that printf functions return 
int, with a consequent limit on the number of bytes output.)
Zack Weinberg March 23, 2017, 7:21 p.m. | #3
On Wed, Mar 22, 2017 at 5:06 PM, Joseph Myers <joseph@codesourcery.com> wrote:
> On Wed, 22 Mar 2017, Zack Weinberg wrote:
>
>> On Mon, Mar 20, 2017 at 5:31 PM, Joseph Myers <joseph@codesourcery.com> wrote:
>> > POSIX specifies long as the type of elements of struct mq_attr.
>>
>> I am extremely disappointed to learn that POSIX didn't just make this
>> mistake with struct timespec.
>
> I think it's a lot more plausibly a mistake for mq_attr than for timespec

For clarity, my position is that it is ALWAYS a design error for a
field of a structure in a C-based API spec to be given a bare
fundamental type.  Such structures always wind up needing to be passed
across an ABI discontinuity sooner or later, and you can't be sure you
are allowed to copy them, so you must instead make it possible for
caller and callee to align, and typedefs are the tool C provides to do
that.  The value range is not relevant.

(Yes, this means abstractly we ought to introduce typedefs for e.g.
sa_flags, but there doesn't seem to be any immediate problem there so
it's not urgent.)

(Typedefs named for the quantity in the field - pid_t, uid_t, time_t,
off_t, the hypothetical nsec_t - are a better choice than typedefs
that specify only the size - uintNN_t and friends - because they let
you make changes more easily.  _FILE_OFFSET_BITS=64 was only an option
because off_t already existed.)

(Having said all that, I honestly don't know what the right types are
for struct mq_attr's fields.  The obvious choice is size_t but there's
actually a case that that's too big.)

zw
Joseph Myers March 23, 2017, 9:25 p.m. | #4
On Thu, 23 Mar 2017, Zack Weinberg wrote:

> For clarity, my position is that it is ALWAYS a design error for a
> field of a structure in a C-based API spec to be given a bare
> fundamental type.  Such structures always wind up needing to be passed
> across an ABI discontinuity sooner or later, and you can't be sure you
> are allowed to copy them, so you must instead make it possible for
> caller and callee to align, and typedefs are the tool C provides to do
> that.  The value range is not relevant.

And my view is that while it might be preferably to use typedefs for new 
structures, use of bare types is not problem in an existing API unless the 
range is inadequate, and that it's not realistic to expect to pass 
structures with either typedefs (other than intN_t / uintN_t) or bare 
"long" types across an ABI discontinuity without explicit conversion code 
on at least one side that knows about the different ABIs.
Zack Weinberg March 23, 2017, 10:16 p.m. | #5
On Thu, Mar 23, 2017 at 5:25 PM, Joseph Myers <joseph@codesourcery.com> wrote:
> On Thu, 23 Mar 2017, Zack Weinberg wrote:
>
>> For clarity, my position is that it is ALWAYS a design error for a
>> field of a structure in a C-based API spec to be given a bare
>> fundamental type.  Such structures always wind up needing to be passed
>> across an ABI discontinuity sooner or later, and you can't be sure you
>> are allowed to copy them, so you must instead make it possible for
>> caller and callee to align, and typedefs are the tool C provides to do
>> that.  The value range is not relevant.
>
> And my view is that while it might be preferably to use typedefs for new
> structures, use of bare types is not problem in an existing API unless the
> range is inadequate, and that it's not realistic to expect to pass
> structures with either typedefs (other than intN_t / uintN_t) or bare
> "long" types across an ABI discontinuity without explicit conversion code
> on at least one side that knows about the different ABIs.

That's very clear, thank you.

It is my assessment that, for the specific case of tv_nsec on ILP32
platforms where time_t is 64 bits wide, explicit conversion code would
only be viable with additional language features that no one has even
tried to design let alone implement, such as a way to force padding
bits to be maintained as zero by the compiler.  Can you explain why
you disagree with this assessment?

zw
Joseph Myers March 27, 2017, 2:37 p.m. | #6
On Thu, 23 Mar 2017, Zack Weinberg wrote:

> > And my view is that while it might be preferably to use typedefs for new
> > structures, use of bare types is not problem in an existing API unless the
> > range is inadequate, and that it's not realistic to expect to pass
> > structures with either typedefs (other than intN_t / uintN_t) or bare
> > "long" types across an ABI discontinuity without explicit conversion code
> > on at least one side that knows about the different ABIs.
> 
> That's very clear, thank you.
> 
> It is my assessment that, for the specific case of tv_nsec on ILP32
> platforms where time_t is 64 bits wide, explicit conversion code would
> only be viable with additional language features that no one has even
> tried to design let alone implement, such as a way to force padding
> bits to be maintained as zero by the compiler.  Can you explain why
> you disagree with this assessment?

*Avoiding* explicit conversion code might require language features.  
*Writing* such code certainly shouldn't; it's just two integer assignments 
to copy values from a structure that represents the sender's struct 
timespec, to one that represents the receiver's struct timespec.  (When 
sending to less-privileged code you should also explicitly zero any 
padding on the receiving side.)

Patch

diff --git a/conform/data/mqueue.h-data b/conform/data/mqueue.h-data
index 76652ce..c7d40ba 100644
--- a/conform/data/mqueue.h-data
+++ b/conform/data/mqueue.h-data
@@ -10,10 +10,11 @@  element {struct sigevent} {void(*} sigev_notify_function )(union sigval)
 element {struct sigevent} {pthread_attr_t*} sigev_notify_attributes
 
 type {struct mq_attr}
-element {struct mq_attr} long mq_flags
-element {struct mq_attr} long mq_maxmsg
-element {struct mq_attr} long mq_msgsize
-element {struct mq_attr} long mq_curmsgs
+// Bug 21279: mq_attr elements have wrong type.
+xfail[x86_64-x32-linux]-element {struct mq_attr} long mq_flags
+xfail[x86_64-x32-linux]-element {struct mq_attr} long mq_maxmsg
+xfail[x86_64-x32-linux]-element {struct mq_attr} long mq_msgsize
+xfail[x86_64-x32-linux]-element {struct mq_attr} long mq_curmsgs
 
 function int mq_close (mqd_t)
 function int mq_getattr (mqd_t, struct mq_attr*)
diff --git a/sysdeps/unix/sysv/linux/x86_64/x32/Makefile b/sysdeps/unix/sysv/linux/x86_64/x32/Makefile
index 72de386..16b768d 100644
--- a/sysdeps/unix/sysv/linux/x86_64/x32/Makefile
+++ b/sysdeps/unix/sysv/linux/x86_64/x32/Makefile
@@ -6,6 +6,6 @@  sysdep_routines += arch_prctl
 endif
 
 ifeq ($(subdir),conform)
-# For bug 16437.
+# For bugs 16437 and 21279.
 conformtest-xfail-conds += x86_64-x32-linux
 endif