Re: svn commit: r301071 - head/sys/sys

[Bruce Evans]

On Wed, 1 Jun 2016, Konstantin Belousov wrote:


On Wed, Jun 01, 2016 at 05:44:47PM +0200, Ed Schouten wrote:

 ...

 - In our implementation, struct sigevent::sigev_notify_attributes has
   type "void *" instead of "pthread_attr_t *". My guess is that this was
   done to prevent pulling in the pthread types, but this can easily be
   avoided by using the underlying structure types.


Not easily, since the tags of the underlying struct types are in the
application namespace, at least up to POSIX 2001.


Yeah, it's quite unfortunate that we use structure types starting with
'pthread'. They should have had leading underscores. But in my opinion
that's not a problem specific to this change; it's a problem with our
pthread implementation in general.


+#include 


Indeed.  The problem became larger in r146824 when these types started
to be declared unconditionally in .

Another bug in sys/_pthreadtypes.h is that it says that the prefixes
pthread_ and PTHREAD_ are reserved for use in header symbols.  Actually,
they are only reserved for use in .  This shouldn't be documented
in the general header.  The non-broken parts of general header depend on
symbols ending with _t being reserved, not on this.


This gives the following pollution (which breaks almost everything since
 includes this header:
- struct tag names pthread*
- struct member names state and mutex


Yes. It would have made so much more sense if a header like
 would have defined all pthread types as __pthread_t,
__pthread_mutex_t, etc. That way there would have been a way to expose
just pthread_t and pthread_attr_t without pulling in the rest.

No, it wouldn't.


Putting everything in sys/_types.h (or machine/_types.h) is convenient,
and it avoids proliferation of headers, but it gives a different bloat
problem: every header ends up declaring every type, but with an spelling
in the implementation namespace.  Then the number of types almost doubles
when you declare only almost all types again in the application namespace.


Replace the typedefs with the forward-struct names by the void *.
The only other change would be the libthr, where some casts might
be needed.

Use void * directly in signal.h if possible.


Pointers to incomplete structures give more type safety than void *.
Except for the problem with struct tags, just using them is best.  Unlike
for typedefs, we don't need a 5-line ifdef for every use to avoid
redeclaratiion errors.

sys/signal.h seems to use a pthread type just once.  This type can be
declared as 'struct pthread_attr *'.

Actually, signal.h already used the correct method, except this was
obfuscated using a private typedef (__pthread_t), and at some point
when POSIX apparently started explicitly requiring  to
declare pthread_t,  was polluted to match.

We use ifdefs to a fault to minimise the scope of typedefs like size_t.
Not doing this for the pthread types is inconsistent.  For POSIX headers,
it is easiest to declare all typedefs *_t in all headers.  This may
actually be more efficient by avoiding 1 layer of indirect typedefs like
__size_t and 2-3 layers of header convolutions and ifdefs.  The
indirections and convolutuons are still technically needed for non-POSIX
headers.

Bruce
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"

Re: svn commit: r301071 - head/sys/sys

[Konstantin Belousov]

On Wed, Jun 01, 2016 at 05:44:47PM +0200, Ed Schouten wrote:
> Hi Bruce,
> 
> 2016-06-01 11:31 GMT+02:00 Bruce Evans :
> >>  - This header file has always depended on pthread_t, pthread_attr_t,
> >>struct timespec, size_t and uid_t. Only as of POSIX 2008, these
> >>dependencies have been states explicitly. They should now be defined.
> >
> > Not always.  POSIX didn't have pthreads or timespecs before about 1993.
> 
> Sure. s/always/for a long time/
> 
> >>  - In our implementation, struct sigevent::sigev_notify_attributes has
> >>type "void *" instead of "pthread_attr_t *". My guess is that this was
> >>done to prevent pulling in the pthread types, but this can easily be
> >>avoided by using the underlying structure types.
> >
> > Not easily, since the tags of the underlying struct types are in the
> > application namespace, at least up to POSIX 2001.
> 
> Yeah, it's quite unfortunate that we use structure types starting with
> 'pthread'. They should have had leading underscores. But in my opinion
> that's not a problem specific to this change; it's a problem with our
> pthread implementation in general.
> 
> >> +#include 
> >
> > This gives the following pollution (which breaks almost everything since
> >  includes this header:
> > - struct tag names pthread*
> > - struct member names state and mutex
> 
> Yes. It would have made so much more sense if a header like
>  would have defined all pthread types as __pthread_t,
> __pthread_mutex_t, etc. That way there would have been a way to expose
> just pthread_t and pthread_attr_t without pulling in the rest.
No, it wouldn't.

Replace the typedefs with the forward-struct names by the void *.
The only other change would be the libthr, where some casts might
be needed.

Use void * directly in signal.h if possible.
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"

Re: svn commit: r301071 - head/sys/sys

[Ed Schouten]

Hi Bruce,

2016-06-01 11:31 GMT+02:00 Bruce Evans :
>>  - This header file has always depended on pthread_t, pthread_attr_t,
>>struct timespec, size_t and uid_t. Only as of POSIX 2008, these
>>dependencies have been states explicitly. They should now be defined.
>
> Not always.  POSIX didn't have pthreads or timespecs before about 1993.

Sure. s/always/for a long time/

>>  - In our implementation, struct sigevent::sigev_notify_attributes has
>>type "void *" instead of "pthread_attr_t *". My guess is that this was
>>done to prevent pulling in the pthread types, but this can easily be
>>avoided by using the underlying structure types.
>
> Not easily, since the tags of the underlying struct types are in the
> application namespace, at least up to POSIX 2001.

Yeah, it's quite unfortunate that we use structure types starting with
'pthread'. They should have had leading underscores. But in my opinion
that's not a problem specific to this change; it's a problem with our
pthread implementation in general.

>> +#include 
>
> This gives the following pollution (which breaks almost everything since
>  includes this header:
> - struct tag names pthread*
> - struct member names state and mutex

Yes. It would have made so much more sense if a header like
 would have defined all pthread types as __pthread_t,
__pthread_mutex_t, etc. That way there would have been a way to expose
just pthread_t and pthread_attr_t without pulling in the rest.

-- 
Ed Schouten 
Nuxi, 's-Hertogenbosch, the Netherlands
KvK-nr.: 62051717
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"

Re: svn commit: r301071 - head/sys/sys

[Bruce Evans]

On Tue, 31 May 2016, Ed Schouten wrote:


Log:
 Improve POSIX conformance of .

 - This header file has always depended on pthread_t, pthread_attr_t,
   struct timespec, size_t and uid_t. Only as of POSIX 2008, these
   dependencies have been states explicitly. They should now be defined.


Not always.  POSIX didn't have pthreads or timespecs before about 1993.


 - In our implementation, struct sigevent::sigev_notify_attributes has
   type "void *" instead of "pthread_attr_t *". My guess is that this was
   done to prevent pulling in the pthread types, but this can easily be
   avoided by using the underlying structure types.


Not easily, since the tags of the underlying struct types are in the
application namespace, at least up to POSIX 2001.


Modified:
 head/sys/sys/signal.h

Modified: head/sys/sys/signal.h
==
--- head/sys/sys/signal.h   Tue May 31 18:45:52 2016(r301070)
+++ head/sys/sys/signal.h   Tue May 31 19:05:41 2016(r301071)
@@ -45,6 +45,23 @@
#include   /* __MINSIGSTKSZ */
#include/* sig_atomic_t; trap codes; sigcontext */

+#if __POSIX_VISIBLE >= 200809
+
+#include 


This gives the following pollution (which breaks almost everything since
 includes this header:
- struct tag names pthread*
- struct member names state and mutex
POSIX could reasonably be unimproved by reserving pthread* but not ordinary
identifiers like state and mutex.


...
@@ -160,6 +177,9 @@ union sigval {
#endif

#if __POSIX_VISIBLE >= 199309
+
+struct pthread_attr;
+


The 1993 version certainly doesn't reserve pthread*.

The 1996 version has a nice table of reserved symbols for every header.
For signal.h, they are just ones with a prefix of sa_, si_, sigev_
and sival_ (these shall not be declared or #defined by the application),
and SIG_, SA_, SI_ and SIGEV_ (these may be used by the application iff
they are #undef'ed before use).  This doesn't proprtly separate optional
things.

A draft 2001 version as a not so nice table.  The rules are now too
tangled to present in a single table, so there are several tables that
are hard to parse.  The first table has sa_, uc_ (new), SIG[A-Z]
(stronger), SIG_[A-Z] (weaker).  Then it has ss_ (new) and sv_ (new)
for XSI only.  Then it has si_, SI_, sigev_, SIGEV_ and sival_ for
RTS only.  The second table has SA_, SIG_[0-9a-z_] (different/weaker),
then massive pollution: BUS_, CLD_, FPE_, ILL_, POLL_, SEGV_, SI_
(now in both tables), SS_, SV_ and TRAP_.

A draft 2007 version is like the 2001 version.  It fixes the sorting
of uc_ and makes RTS non-optional.  In the second table, it moves SS_,
SV_ and TRAP_ under XSI, and moves POLL_ under OBS XSR.

I think pthread is not reserved since it is not in these tables.  Later
versions of POSIX were broken to allow  to be pollutied
with all the symbols in , but I don't want to check what
is in that now.   is slightly simpler in POSIX but much
more polluted than  in FreeBSD.


struct sigevent {
int sigev_notify;   /* Notification type */
int sigev_signo;/* Signal number */
@@ -168,7 +188,7 @@ struct sigevent {
__lwpid_t   _threadid;


Names like _threadid are bogus.  sigev is reserved for uses like this.
Noy using a prefix makes the namespace random.


struct {
void (*_function)(union sigval);
-   void *_attribute; /* pthread_attr_t * */
+   struct pthread_attr **_attribute;


pthread is not reserved.  pthread*_t is only reserved by the general
rule that everything ending in _t is reserved.

This also has indentation errors.


} _sigev_thread;



unsigned short _kevent_flags;


Further bogus names.  At least they use a prefix.


long __spare__[8];


A more bogus name.


@@ -190,6 +210,7 @@ struct sigevent {
#define SIGEV_KEVENT3   /* Generate a kevent. */
#define SIGEV_THREAD_ID 4   /* Send signal to a kernel thread. */


It is correct to used the reserved prefix for our extensions, but this style
is inconsistent with old parts of the file.  In the old parts, we ifdef
out extensions to a fault.   This makes the code hard to read but provides
good documentation of what is portable.


#endif
+
#endif /* __POSIX_VISIBLE >= 199309 */

#if __POSIX_VISIBLE >= 199309 || __XSI_VISIBLE



Bruce
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"

svn commit: r301071 - head/sys/sys

[Ed Schouten]

Author: ed
Date: Tue May 31 19:05:41 2016
New Revision: 301071
URL: https://svnweb.freebsd.org/changeset/base/301071

Log:
  Improve POSIX conformance of .
  
  - This header file has always depended on pthread_t, pthread_attr_t,
struct timespec, size_t and uid_t. Only as of POSIX 2008, these
dependencies have been states explicitly. They should now be defined.
  
  - In our implementation, struct sigevent::sigev_notify_attributes has
type "void *" instead of "pthread_attr_t *". My guess is that this was
done to prevent pulling in the pthread types, but this can easily be
avoided by using the underlying structure types.

Modified:
  head/sys/sys/signal.h

Modified: head/sys/sys/signal.h
==
--- head/sys/sys/signal.h   Tue May 31 18:45:52 2016(r301070)
+++ head/sys/sys/signal.h   Tue May 31 19:05:41 2016(r301071)
@@ -45,6 +45,23 @@
 #include/* __MINSIGSTKSZ */
 #include /* sig_atomic_t; trap codes; sigcontext */
 
+#if __POSIX_VISIBLE >= 200809
+
+#include 
+#include 
+
+#ifndef _SIZE_T_DECLARED
+typedef__size_tsize_t;
+#define_SIZE_T_DECLARED
+#endif
+
+#ifndef _UID_T_DECLARED
+typedef__uid_t uid_t;
+#define_UID_T_DECLARED
+#endif
+
+#endif /* __POSIX_VISIBLE >= 200809 */
+
 /*
  * System defined signals.
  */
@@ -160,6 +177,9 @@ union sigval {
 #endif
 
 #if __POSIX_VISIBLE >= 199309
+
+struct pthread_attr;
+
 struct sigevent {
int sigev_notify;   /* Notification type */
int sigev_signo;/* Signal number */
@@ -168,7 +188,7 @@ struct sigevent {
__lwpid_t   _threadid;
struct {
void (*_function)(union sigval);
-   void *_attribute; /* pthread_attr_t * */
+   struct pthread_attr **_attribute;
} _sigev_thread;
unsigned short _kevent_flags;
long __spare__[8];
@@ -190,6 +210,7 @@ struct sigevent {
 #defineSIGEV_KEVENT3   /* Generate a kevent. */
 #defineSIGEV_THREAD_ID 4   /* Send signal to a kernel 
thread. */
 #endif
+
 #endif /* __POSIX_VISIBLE >= 199309 */
 
 #if __POSIX_VISIBLE >= 199309 || __XSI_VISIBLE
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"