| Message ID | 20180303191852.19319-1-samuel.thibault@ens-lyon.org |
|---|---|
| State | New |
| Headers | show |
| Series | [hurd,commited] hurd: do not check Mach and Hurd headers | expand |
On Sat, 3 Mar 2018, Samuel Thibault wrote:
> as they are not standard.
That's not a sufficient reason for this change. What this script is
checking is nothing to do with whether the headers are standard; it's that
each header can be included in isolation with any supported feature test
macro defined (actually, just a few macros are tested) and in any C/C++
standards mode.
That property should apply to Hurd-specific headers installed by glibc
just as it applies to non-Hurd-specific headers (this script doesn't test
headers not installed by glibc). That is, any case of such a header
failing this test is presumptively a bug that should be fixed. (If a
header is in fact purely internal, not for direct use by users or by other
installed headers, stopping it being installed is the correct fix - this
test is specifically for installed headers. If a header is installed only
for use by other installed headers and is not meant to be included
directly by user programs, it should move into bits/; bits/ headers are
already excluded from this test.)
Hello, Joseph Myers, on sam. 03 mars 2018 22:08:49 +0000, wrote: > On Sat, 3 Mar 2018, Samuel Thibault wrote: > > > as they are not standard. > > That's not a sufficient reason for this change. What this script is > checking is nothing to do with whether the headers are standard; it's that > each header can be included in isolation with any supported feature test > macro defined (actually, just a few macros are tested) and in any C/C++ > standards mode. Well hurd & mach headers are not meant to be used without _GNU_SOURCE=1, for a start... Usual applications wouldn't use them anyway, only things like e.g. libparted, Xorg, etc. which need to interact closely with system things use them, and thus have to enable the GNU extensions. You can think of them like linux/ headers. Samuel
On Sat, 3 Mar 2018, Samuel Thibault wrote: > Well hurd & mach headers are not meant to be used without _GNU_SOURCE=1, > for a start... Usual applications wouldn't use them anyway, only things If a header requires some type T, I'd expect it to include bits/types/T.h, thereby ensuring that type T is defined regardless of feature test macros, instead of including some other header that might or might not define type T depending on feature test macros. Likewise for any other dependencies on features from other headers. (And of course these headers shouldn't themselves contain feature test macro conditionals.) > like e.g. libparted, Xorg, etc. which need to interact closely with > system things use them, and thus have to enable the GNU extensions. You > can think of them like linux/ headers. Well, all linux/*.h and asm/*h headers also ought to be includable in isolation, with any feature test macros defined. But that's the Linux kernel's problem, not ours, since it's the Linux kernel that provides those headers. Whereas this test is purely for headers installed by glibc. And every header installed by glibc should either work in isolation, or if it's not meant to work in some context should have a #error explaining the issue - like the #errors in bits/*.h headers saying not to include them directly, or the #error in sys/elf.h for x86_64 GNU/Linux, for example. I think it's dubious to have a #error requiring _GNU_SOURCE to be used, because it should always be possible to write a header in a way that doesn't have such a requirement. But in any case where, after analysis, such a #error is found to make sense (and a comment goes on the #error explaining why it makes sense), that specific header might have testing disabled in check-installed-headers.sh *only* when _GNU_SOURCE is not passed - not for other feature test macros, not using wildcards like hurd/*.h.
Hello, Joseph Myers, on sam. 03 mars 2018 22:53:23 +0000, wrote: > On Sat, 3 Mar 2018, Samuel Thibault wrote: > > like e.g. libparted, Xorg, etc. which need to interact closely with > > system things use them, and thus have to enable the GNU extensions. You > > can think of them like linux/ headers. > > Well, all linux/*.h and asm/*h headers also ought to be includable in > isolation, with any feature test macros defined. But that's the Linux > kernel's problem, not ours, since it's the Linux kernel that provides > those headers. Whereas this test is purely for headers installed by > glibc. I only mentioned Linux to point out which kind of interface we are talking about: not something for normal applications. Here, glibc is distributing pieces of the Hurd which the Linux kernel distributes. > And every header installed by glibc should either work in > isolation, or if it's not meant to work in some context should have a > #error explaining the issue - like the #errors in bits/*.h headers saying > not to include them directly, or the #error in sys/elf.h for x86_64 > GNU/Linux, for example. Ok. > But in any case where, after analysis, such a #error is found to > make sense (and a comment goes on the #error explaining why it > makes sense), that specific header might have testing disabled in > check-installed-headers.sh *only* when _GNU_SOURCE is not passed - not > for other feature test macros, not using wildcards like hurd/*.h. I have sent a patch adding support for this, and whitelisting mach headers, could you have a look? Samuel
Joseph Myers, on sam. 03 mars 2018 22:53:23 +0000, wrote: > I think it's dubious to have a #error requiring _GNU_SOURCE to be used, > because it should always be possible to write a header in a way that > doesn't have such a requirement. But in any case where, after analysis, > such a #error is found to make sense Notably, most Hurd interfaces use the error_t type, which is GNU-only. I assume this is enough to make it a #error case? Samuel
On Sun, 4 Mar 2018, Samuel Thibault wrote: > Joseph Myers, on sam. 03 mars 2018 22:53:23 +0000, wrote: > > I think it's dubious to have a #error requiring _GNU_SOURCE to be used, > > because it should always be possible to write a header in a way that > > doesn't have such a requirement. But in any case where, after analysis, > > such a #error is found to make sense > > Notably, most Hurd interfaces use the error_t type, which is GNU-only. > I assume this is enough to make it a #error case? No. Create bits/types/error_t.h (which would have a generic version and a Hurd version), which would define error_t (subject to __error_t_defined as a multiple-include guard). Then make errno.h include <bits/types/error_t.h> if __USE_GNU, while all the Hurd headers using that type would include <bits/types/error_t.h> unconditionally. (sysdeps/mach/hurd/bits/errno.h would no longer need to define error_t and wouldn't need to include <bits/types/error_t.h> either because of errno.h doing so - I'm assuming the current reason for defining error_t there is simply to get a Hurd-specific definition instead of the default from errno.h, which would be dealt with by having a Hurd-specific bits/types/error_t.h.) Having such a bits/types/*.h header is the normal way in glibc of dealing with types that different headers need under different conditions.
On Sun, 4 Mar 2018, Samuel Thibault wrote: > > But in any case where, after analysis, such a #error is found to > > make sense (and a comment goes on the #error explaining why it > > makes sense), that specific header might have testing disabled in > > check-installed-headers.sh *only* when _GNU_SOURCE is not passed - not > > for other feature test macros, not using wildcards like hurd/*.h. > > I have sent a patch adding support for this, and whitelisting mach > headers, could you have a look? It's not yet clear any such special cases in the script are needed. But if they are, they would only be for specific headers that use #error without __USE_GNU, not any wildcard such as hurd/*.h, and we should review whether the #error usage is necessary or whether there's a better approach in each particular case.
Joseph Myers, on dim. 04 mars 2018 01:33:18 +0000, wrote: > On Sun, 4 Mar 2018, Samuel Thibault wrote: > > > But in any case where, after analysis, such a #error is found to > > > make sense (and a comment goes on the #error explaining why it > > > makes sense), that specific header might have testing disabled in > > > check-installed-headers.sh *only* when _GNU_SOURCE is not passed - not > > > for other feature test macros, not using wildcards like hurd/*.h. > > > > I have sent a patch adding support for this, and whitelisting mach > > headers, could you have a look? > > It's not yet clear any such special cases in the script are needed. See the patch: mach/mach/mig_support.h explains why it needs _GNU_SOURCE. > But > if they are, they would only be for specific headers that use #error > without __USE_GNU, not any wildcard such as hurd/*.h, Sure, I _*NEVER*_ meant to do anything like that, as my patch shows. Samuel
On Sun, 4 Mar 2018, Samuel Thibault wrote: > Joseph Myers, on dim. 04 mars 2018 01:33:18 +0000, wrote: > > On Sun, 4 Mar 2018, Samuel Thibault wrote: > > > > But in any case where, after analysis, such a #error is found to > > > > make sense (and a comment goes on the #error explaining why it > > > > makes sense), that specific header might have testing disabled in > > > > check-installed-headers.sh *only* when _GNU_SOURCE is not passed - not > > > > for other feature test macros, not using wildcards like hurd/*.h. > > > > > > I have sent a patch adding support for this, and whitelisting mach > > > headers, could you have a look? > > > > It's not yet clear any such special cases in the script are needed. > > See the patch: mach/mach/mig_support.h explains why it needs > _GNU_SOURCE. Well, I disagree with the explanation there. The use of __stpncpy is in an _LIBC-conditional block, and _LIBC can reasonably be taken to imply _GNU_SOURCE, so if you remove the #error I see no reason for this header to break without _GNU_SOURCE. It just requires a few Mach-specific types, and all the headers declaring such types should do so unconditionally. (Such _LIBC-conditional code should where possible move out of installed headers and into non-installed headers used only for building glibc, but that's a separate longstanding issue. _LIBC conditionals are more appropriate for e.g. code shared with gnulib.) > > But > > if they are, they would only be for specific headers that use #error > > without __USE_GNU, not any wildcard such as hurd/*.h, > > Sure, I _*NEVER*_ meant to do anything like that, as my patch shows. My reading of <https://sourceware.org/ml/libc-alpha/2018-03/msg00074.html> is that it still has: + # Hurd headers are not standard anyway + (hurd.h | hurd/*.h | faultexc_server.h) + continue;; and as noted, I don't think hurd/*.h should be special-cased like that (or that "not standard" is a sufficient explanation for a comment here).
Joseph Myers, on dim. 04 mars 2018 01:41:43 +0000, wrote: > My reading of <https://sourceware.org/ml/libc-alpha/2018-03/msg00074.html> > is that it still has: > > + # Hurd headers are not standard anyway > + (hurd.h | hurd/*.h | faultexc_server.h) > + continue;; The reading of the patch really is all the - on mach headers. Small step to get something done instead of having to do _*ALL*_ the work at one time and never manage to find motivation to finish it all. > and as noted, I don't think hurd/*.h should be special-cased like that (or > that "not standard" is a sufficient explanation for a comment here). Sure, that was never the intent of that patch. Samuel
Joseph Myers, on dim. 04 mars 2018 01:41:43 +0000, wrote: > Well, I disagree with the explanation there. The use of __stpncpy is in > an _LIBC-conditional block, Indeed, that got in in between. /me more and more tired of fixed all kinds of code that were written by other people more than two decades ago. Samuel
On Sun, 4 Mar 2018, Samuel Thibault wrote: > Joseph Myers, on dim. 04 mars 2018 01:41:43 +0000, wrote: > > My reading of <https://sourceware.org/ml/libc-alpha/2018-03/msg00074.html> > > is that it still has: > > > > + # Hurd headers are not standard anyway > > + (hurd.h | hurd/*.h | faultexc_server.h) > > + continue;; > > The reading of the patch really is all the - on mach headers. Small > step to get something done instead of having to do _*ALL*_ the work at > one time and never manage to find motivation to finish it all. I don't actually see the problem with having some of these tests failing - no Hurd special cases in this script - unless and until we decide that a particular Hurd special case in this script is appropriate. That was how we handled many architecture-specific fixes for header problems shown by these tests for GNU/Linux (some of these tests failed on some architectures for some time); I think it's how we should handle them for Hurd. When individual headers are fixed, the number of failures may go down.
Joseph Myers, on sam. 03 mars 2018 22:53:23 +0000, wrote: > it should always be possible to write a header in a way that > doesn't have such a requirement. I'm almost there, the only remaining issue is that hurd/hurd/signal.h uses struct sigaction. I guess I should move existing definitions to a struct_sigaction.h header? Samuel
On Sat, Mar 3, 2018 at 9:55 PM, Samuel Thibault <samuel.thibault@ens-lyon.org> wrote: > Joseph Myers, on sam. 03 mars 2018 22:53:23 +0000, wrote: >> it should always be possible to write a header in a way that >> doesn't have such a requirement. > > I'm almost there, the only remaining issue is that hurd/hurd/signal.h > uses struct sigaction. I guess I should move existing definitions to a > struct_sigaction.h header? In this case, you just need a Hurd version of the existing bits/sigaction.h. (It's bits/sigaction.h rather than bits/types/struct_sigaction.h because it also defines the SA_* constants.) zw
Zack Weinberg, on dim. 04 mars 2018 11:08:00 -0500, wrote: > On Sat, Mar 3, 2018 at 9:55 PM, Samuel Thibault > <samuel.thibault@ens-lyon.org> wrote: > > Joseph Myers, on sam. 03 mars 2018 22:53:23 +0000, wrote: > >> it should always be possible to write a header in a way that > >> doesn't have such a requirement. > > > > I'm almost there, the only remaining issue is that hurd/hurd/signal.h > > uses struct sigaction. I guess I should move existing definitions to a > > struct_sigaction.h header? > > In this case, you just need a Hurd version of the existing > bits/sigaction.h. I don't understand why a different version. Can't I just make hurd/hurd/signal.h include <bits/sigaction.h>? (well, there is a technical reason: it does not have multi-inclusion guard, but that can be fixed) Samuel
On Sun, Mar 4, 2018 at 1:09 PM, Samuel Thibault <samuel.thibault@ens-lyon.org> wrote: > Zack Weinberg, on dim. 04 mars 2018 11:08:00 -0500, wrote: >> On Sat, Mar 3, 2018 at 9:55 PM, Samuel Thibault >> <samuel.thibault@ens-lyon.org> wrote: >> > Joseph Myers, on sam. 03 mars 2018 22:53:23 +0000, wrote: >> >> it should always be possible to write a header in a way that >> >> doesn't have such a requirement. >> > >> > I'm almost there, the only remaining issue is that hurd/hurd/signal.h >> > uses struct sigaction. I guess I should move existing definitions to a >> > struct_sigaction.h header? >> >> In this case, you just need a Hurd version of the existing >> bits/sigaction.h. > > I don't understand why a different version. Can't I just make > hurd/hurd/signal.h include <bits/sigaction.h>? (well, there is a > technical reason: it does not have multi-inclusion guard, but that can > be fixed) Oh, yes, if the existing generic bits/sigaction.h works for Hurd then that's fine. I thought there had to be something wrong with it, because hurd/signal.h already includes signal.h which includes bits/sigaction.h ... but now I realize that the problem is signal.h doesn't _always_ include bits/sigaction.h. zw
diff --git a/ChangeLog b/ChangeLog index 775e4c6ab3..dd65beebc6 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,7 @@ +2018-03-03 Samuel Thibault <samuel.thibault@ens-lyon.org> + + * scripts/check-installed-headers.sh: Ignore Hurd and Mach headers. + 2018-03-03 Andreas Schwab <schwab@linux-m68k.org> [BZ #22918] diff --git a/scripts/check-installed-headers.sh b/scripts/check-installed-headers.sh index 7ffd2b8e74..f7f55917f7 100644 --- a/scripts/check-installed-headers.sh +++ b/scripts/check-installed-headers.sh @@ -126,6 +126,13 @@ EOF fi ;; esac + ;; + + # Hurd and Mach headers are not standard anyway + (hurd.h | hurd/*.h | faultexc_server.h | \ + mach.h | mach_init.h | mach_error.h | mach-shortcuts.h | mach/* | \ + device/* | lock-intern.h | spin-lock.h | machine-sp.h) + continue;; esac echo :: "$header"