[03/13] Installed-header hygiene (BZ#20366): missing inclusions A.

  These are all fallout from the change from u_intNN_t to uintNN_t; a
number of headers now need to include <stdint.h> to pick up those
types.  It is possible that __uintNN_t should be used instead.

Some of these files directly included <features.h> and/or
<sys/cdefs.h>, which I removed, as the style generally seems to be to
let <sys/types.h> do that for us.  One file included <asm/types.h>
gratuitously.

	* inet/protocols/talkd.h, resolv/arpa/nameser.h
	* sysdeps/generic/netinet/in_systm.h
	* sysdeps/gnu/netinet/ip_icmp.h, sysdeps/gnu/netinet/tcp.h
	* sysdeps/gnu/netinet/udp.h
	* sysdeps/unix/sysv/linux/net/ethernet.h
	* sysdeps/unix/sysv/linux/net/if_arp.h
	* sysdeps/unix/sysv/linux/net/if_ppp.h
	* sysdeps/unix/sysv/linux/net/if_shaper.h
	* sysdeps/unix/sysv/linux/netinet/if_fddi.h
	* sysdeps/unix/sysv/linux/netinet/if_tr.h
	* sysdeps/unix/sysv/linux/netipx/ipx.h
	* sysdeps/unix/sysv/linux/sys/acct.h
	* sysdeps/unix/sysv/linux/sys/quota.h:
	Include stdint.h for uintNN_t definitions.
	Don't include sys/cdefs.h, features.h, or asm/types.h directly.
---
 inet/protocols/talkd.h                    | 1 +
 resolv/arpa/nameser.h                     | 2 +-
 sysdeps/generic/netinet/in_systm.h        | 2 +-
 sysdeps/gnu/netinet/ip_icmp.h             | 2 +-
 sysdeps/gnu/netinet/tcp.h                 | 1 +
 sysdeps/gnu/netinet/udp.h                 | 3 +--
 sysdeps/unix/sysv/linux/net/ethernet.h    | 3 ++-
 sysdeps/unix/sysv/linux/net/if_arp.h      | 3 +--
 sysdeps/unix/sysv/linux/net/if_ppp.h      | 3 +--
 sysdeps/unix/sysv/linux/net/if_shaper.h   | 2 +-
 sysdeps/unix/sysv/linux/netinet/if_fddi.h | 4 +---
 sysdeps/unix/sysv/linux/netinet/if_tr.h   | 2 +-
 sysdeps/unix/sysv/linux/netipx/ipx.h      | 3 +--
 sysdeps/unix/sysv/linux/sys/acct.h        | 5 ++---
 sysdeps/unix/sysv/linux/sys/quota.h       | 2 +-
 15 files changed, 17 insertions(+), 21 deletions(-)
  

On Mon, 29 Aug 2016, Zack Weinberg wrote:

> These are all fallout from the change from u_intNN_t to uintNN_t; a
> number of headers now need to include <stdint.h> to pick up those
> types.  It is possible that __uintNN_t should be used instead.

I'm not clear whether this is needed to restore the glibc build / 
testsuite build, or only for installed use.  If it's needed to keep things 
building, it has to come *before* the patch that causes the header to be 
required, to keep bisectability.
  

On Mon, Aug 29, 2016 at 9:23 PM, Joseph Myers <joseph@codesourcery.com> wrote:
> On Mon, 29 Aug 2016, Zack Weinberg wrote:
>
>> These are all fallout from the change from u_intNN_t to uintNN_t; a
>> number of headers now need to include <stdint.h> to pick up those
>> types.  It is possible that __uintNN_t should be used instead.
>
> I'm not clear whether this is needed to restore the glibc build /
> testsuite build, or only for installed use.  If it's needed to keep things
> building, it has to come *before* the patch that causes the header to be
> required, to keep bisectability.

It is necessary to restore the build.  This is only separate for ease
of review; my plan was always to squash the entire series into one big
patch for landing.  Or maybe two or three big patches - the time.h and
ucontext.h changes qualify as separate ideas, probably.

zw
  

On 08/29/2016 09:16 PM, Zack Weinberg wrote:
> These are all fallout from the change from u_intNN_t to uintNN_t; a
> number of headers now need to include <stdint.h> to pick up those
> types.  It is possible that __uintNN_t should be used instead.

Why should __uintNN_t ever be used? At a minimum the uintNN_t types
should be available regardless of whatever mode you are being compiled in?

> Some of these files directly included <features.h> and/or
> <sys/cdefs.h>, which I removed, as the style generally seems to be to
> let <sys/types.h> do that for us.  One file included <asm/types.h>
> gratuitously.

Are you certain that was safe? I would be worried about arbitrarily
removing those headers without a deeper analysis.

> 	* inet/protocols/talkd.h, resolv/arpa/nameser.h
> 	* sysdeps/generic/netinet/in_systm.h
> 	* sysdeps/gnu/netinet/ip_icmp.h, sysdeps/gnu/netinet/tcp.h
> 	* sysdeps/gnu/netinet/udp.h
> 	* sysdeps/unix/sysv/linux/net/ethernet.h
> 	* sysdeps/unix/sysv/linux/net/if_arp.h
> 	* sysdeps/unix/sysv/linux/net/if_ppp.h
> 	* sysdeps/unix/sysv/linux/net/if_shaper.h
> 	* sysdeps/unix/sysv/linux/netinet/if_fddi.h
> 	* sysdeps/unix/sysv/linux/netinet/if_tr.h
> 	* sysdeps/unix/sysv/linux/netipx/ipx.h
> 	* sysdeps/unix/sysv/linux/sys/acct.h
> 	* sysdeps/unix/sysv/linux/sys/quota.h:
> 	Include stdint.h for uintNN_t definitions.
> 	Don't include sys/cdefs.h, features.h, or asm/types.h directly.

OK only if you verified that the generated code did not change due to the
removal of unneeded headers, or if you drop the unneeded header removal.
  

On 08/29/2016 11:11 PM, Zack Weinberg wrote:
> On Mon, Aug 29, 2016 at 9:23 PM, Joseph Myers <joseph@codesourcery.com> wrote:
>> On Mon, 29 Aug 2016, Zack Weinberg wrote:
>>
>>> These are all fallout from the change from u_intNN_t to uintNN_t; a
>>> number of headers now need to include <stdint.h> to pick up those
>>> types.  It is possible that __uintNN_t should be used instead.
>>
>> I'm not clear whether this is needed to restore the glibc build /
>> testsuite build, or only for installed use.  If it's needed to keep things
>> building, it has to come *before* the patch that causes the header to be
>> required, to keep bisectability.
> 
> It is necessary to restore the build.  This is only separate for ease
> of review; my plan was always to squash the entire series into one big
> patch for landing.  Or maybe two or three big patches - the time.h and
> ucontext.h changes qualify as separate ideas, probably.

That was my understanding. That this is going to be a single patch that does
the mechanical changes in one commit which can be reverted, as suggested in
patch 00/13.
  

On Wed, 21 Sep 2016, Carlos O'Donell wrote:

> On 08/29/2016 09:16 PM, Zack Weinberg wrote:
> > These are all fallout from the change from u_intNN_t to uintNN_t; a
> > number of headers now need to include <stdint.h> to pick up those
> > types.  It is possible that __uintNN_t should be used instead.
> 
> Why should __uintNN_t ever be used? At a minimum the uintNN_t types
> should be available regardless of whatever mode you are being compiled in?

ISO C doesn't reserve *_t globally, but unless any of these headers are 
ISO C headers that's not a relevant consideration (and if inappropriate 
*_t types were being added to ISO C headers that would have been detected 
by the conform/ tests).
  

On Wed, Sep 21, 2016 at 1:52 PM, Carlos O'Donell <carlos@redhat.com> wrote:
> On 08/29/2016 09:16 PM, Zack Weinberg wrote:
>> These are all fallout from the change from u_intNN_t to uintNN_t; a
>> number of headers now need to include <stdint.h> to pick up those
>> types.  It is possible that __uintNN_t should be used instead.
>
> Why should __uintNN_t ever be used? At a minimum the uintNN_t types
> should be available regardless of whatever mode you are being compiled in?

It might be inappropriate for some of these headers to (behave as-if
they) include <stdint.h>.  Joseph says that existing conform/ tests
cover this from a standards-compliance perspective, but it is my
understanding that net/* and netinet/* headers are not very well
standardized, so we might want to think about whether we should go to
extra effort here.

>> Some of these files directly included <features.h> and/or
>> <sys/cdefs.h>, which I removed, as the style generally seems to be to
>> let <sys/types.h> do that for us.  One file included <asm/types.h>
>> gratuitously.
>
> Are you certain that was safe? I would be worried about arbitrarily
> removing those headers without a deeper analysis.

sys/types.h unconditionally includes features.h, and features.h
unconditionally includes sys/cdefs.h.

None of the definitions in asm/types.h were required by the file that
was including it.

I will do a check for unchanged installed libraries as discussed elsewhere.

zw
  

On 09/21/2016 02:22 PM, Zack Weinberg wrote:
> On Wed, Sep 21, 2016 at 1:52 PM, Carlos O'Donell <carlos@redhat.com> wrote:
>> On 08/29/2016 09:16 PM, Zack Weinberg wrote:
>>> These are all fallout from the change from u_intNN_t to uintNN_t; a
>>> number of headers now need to include <stdint.h> to pick up those
>>> types.  It is possible that __uintNN_t should be used instead.
>>
>> Why should __uintNN_t ever be used? At a minimum the uintNN_t types
>> should be available regardless of whatever mode you are being compiled in?
> 
> It might be inappropriate for some of these headers to (behave as-if
> they) include <stdint.h>.  Joseph says that existing conform/ tests
> cover this from a standards-compliance perspective, but it is my
> understanding that net/* and netinet/* headers are not very well
> standardized, so we might want to think about whether we should go to
> extra effort here.

Thanks for the clarification.

Given the underspecified nature of those headers I don't see that we need
to worry. If someone comes back and has a valid complaint we could always
do a further cleanup?

>>> Some of these files directly included <features.h> and/or
>>> <sys/cdefs.h>, which I removed, as the style generally seems to be to
>>> let <sys/types.h> do that for us.  One file included <asm/types.h>
>>> gratuitously.
>>
>> Are you certain that was safe? I would be worried about arbitrarily
>> removing those headers without a deeper analysis.
> 
> sys/types.h unconditionally includes features.h, and features.h
> unconditionally includes sys/cdefs.h.
> 
> None of the definitions in asm/types.h were required by the file that
> was including it.
> 
> I will do a check for unchanged installed libraries as discussed elsewhere.

Belt-and-suspenders ;-)