Incomplete patch to fix build with top-of-tree GCC

  On 05/15/2015 09:47 AM, Joseph Myers wrote:
> The first question is:*is*  it OK to ignore the warnings?
I looked just at the first hunk of the patch, and it appears to me that 
it's not OK to ignore its warnings.  The type punning in the first hunk 
appears to violate the C standard; although I don't know whether GCC is 
generating "incorrect" code as a result, instead of silencing the 
warning how about fixing the code so that it doesn't have those funky 
casts?  Something like the attached (untested) patch, say.
  

On 05/15/2015 04:29 PM, Paul Eggert wrote:
> On 05/15/2015 09:47 AM, Joseph Myers wrote:
>> The first question is:*is*  it OK to ignore the warnings?
> I looked just at the first hunk of the patch, and it appears to me
> that it's not OK to ignore its warnings. The type punning in the
> first hunk appears to violate the C standard; although I don't know
> whether GCC is generating "incorrect" code as a result, instead of
> silencing the warning how about fixing the code so that it doesn't
> have those funky casts? Something like the attached (untested) patch,
> say.

Yes please.

We should be fixing this everywhere we see obvious C strict-aliasing violations.

Your changes are mechanical and look good to me.

Cheers,
Carlos.

> diff --git a/inet/rcmd.c b/inet/rcmd.c
> index acacaa0..bb5b0aa 100644
> --- a/inet/rcmd.c
> +++ b/inet/rcmd.c
> @@ -374,7 +374,11 @@ rresvport_af(alport, family)
>  	int *alport;
>  	sa_family_t family;
>  {
> -	struct sockaddr_storage ss;
> +	union {
> +	  struct sockaddr generic;
> +	  struct sockaddr_in in;
> +	  struct sockaddr_in6 in6;
> +	} ss;
>  	int s;
>  	size_t len;
>  	uint16_t *sport;
> @@ -382,11 +386,11 @@ rresvport_af(alport, family)
>  	switch(family){
>  	case AF_INET:
>  		len = sizeof(struct sockaddr_in);
> -		sport = &((struct sockaddr_in *)&ss)->sin_port;
> +		sport = &ss.in.sin_port;
>  		break;
>  	case AF_INET6:
>  		len = sizeof(struct sockaddr_in6);
> -		sport = &((struct sockaddr_in6 *)&ss)->sin6_port;
> +		sport = &ss.in6.sin6_port;
>  		break;
>  	default:
>  		__set_errno (EAFNOSUPPORT);
> @@ -398,9 +402,9 @@ rresvport_af(alport, family)
> 
>  	memset (&ss, '\0', sizeof(ss));
>  #ifdef SALEN
> -	ss.__ss_len = len;
> +	ss.generic.__ss_len = len;
>  #endif
> -	ss.ss_family = family;
> +	ss.generic.ss_family = family;
> 
>  	/* Ignore invalid values.  */
>  	if (*alport < IPPORT_RESERVED / 2)
> @@ -411,7 +415,7 @@ rresvport_af(alport, family)
>  	int start = *alport;
>  	do {
>  		*sport = htons((uint16_t) *alport);
> -		if (__bind(s, (struct sockaddr *)&ss, len) >= 0)
> +		if (__bind(s, &ss.generic, len) >= 0)
>  			return s;
>  		if (errno != EADDRINUSE) {
>  			(void)__close(s);
  

On Sat, 2015-05-16 at 00:13 -0400, Carlos O'Donell wrote:
> On 05/15/2015 04:29 PM, Paul Eggert wrote:
> > On 05/15/2015 09:47 AM, Joseph Myers wrote:
> >> The first question is:*is*  it OK to ignore the warnings?
> > I looked just at the first hunk of the patch, and it appears to me
> > that it's not OK to ignore its warnings. The type punning in the
> > first hunk appears to violate the C standard; although I don't know
> > whether GCC is generating "incorrect" code as a result, instead of
> > silencing the warning how about fixing the code so that it doesn't
> > have those funky casts? Something like the attached (untested) patch,
> > say.
> 
> Yes please.
> 
> We should be fixing this everywhere we see obvious C strict-aliasing violations.
> 
> Your changes are mechanical and look good to me.
> 
> Cheers,
> Carlos.

> >  #ifdef SALEN
> > -	ss.__ss_len = len;
> > +	ss.generic.__ss_len = len;
> >  #endif
> > -	ss.ss_family = family;
> > +	ss.generic.ss_family = family;

It looks like this last line should be 'sa_family', not 'ss_family'.  If
I change that, there are still two lines that give an error:

rcmd.c: In function 'iruserok_af':
rcmd.c:618:24: error: dereferencing type-punned pointer will break
strict-aliasing rules [-Werror=strict-aliasing]
     memcpy (&(((struct sockaddr_in *)&ra)->sin_addr), raddr,
                         ^
rcmd.c:624:24: error: dereferencing type-punned pointer will break
strict-aliasing rules [-Werror=strict-aliasing]
     memcpy (&(((struct sockaddr_in6 *)&ra)->sin6_addr), raddr,


Do we need to create another anonymous union in iruserok_af to fix
these?

Steve Ellcey
sellcey@imgtec.com