Incomplete patch to fix build with top-of-tree GCC
Commit Message
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.
Comments
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
@@ -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);