From: Pat Thoyts <hidden> Date: 2016-06-15 22:49:58
Erik Faye-Lund [off-list ref] writes:
Here's hopefully the last iteration of this series. The previous version
only got a single complain about a typo in the subject of patch 14/15, so
it seems like most controversies have been settled.
I pulled this win32-daemon branch into my msysgit build tree and built
it. I get the following warnings:
CC daemon.o
daemon.c: In function 'service_loop':
daemon.c:674: warning: dereferencing pointer 'ss.124' does break strict-aliasing rules
daemon.c:676: warning: dereferencing pointer 'ss.124' does break strict-aliasing rules
daemon.c:681: warning: dereferencing pointer 'ss.124' does break strict-aliasing rules
daemon.c:919: note: initialized from here
daemon.c:679: warning: dereferencing pointer 'sin_addr' does break strict-aliasing rules
daemon.c:675: note: initialized from here
daemon.c:691: warning: dereferencing pointer 'sin6_addr' does break strict-aliasing rules
daemon.c:682: note: initialized from here
Otherwise it builds clean. The daemon running on Windows7 seems to be
working fine for both ipv4 and ipv6 connections (I tried both).
However, monitoring the resource usage in procexp it looks like there is
a handle leak. Each 'git ls-remote' over ipv6 is gaining 16 handles that
do not appear to be released. They're all process handles for dead
processes it looks like, so possibly there is a missing waitpid() or
something similar for the 'git daemon -serve' subprocess. Doing this
over ipv4 leaks 2 handles per request.
--
Pat Thoyts http://www.patthoyts.tk/
PGP fingerprint 2C 6E 98 07 2C 59 C8 97 10 CE 11 E6 04 E0 B9 DD
From: Erik Faye-Lund <hidden> Date: 2016-06-15 22:49:58
On Wed, Nov 3, 2010 at 10:11 PM, Pat Thoyts
[off-list ref] wrote:
Erik Faye-Lund [off-list ref] writes:
quoted
Here's hopefully the last iteration of this series. The previous version
only got a single complain about a typo in the subject of patch 14/15, so
it seems like most controversies have been settled.
I pulled this win32-daemon branch into my msysgit build tree and built
it. I get the following warnings:
CC daemon.o
daemon.c: In function 'service_loop':
daemon.c:674: warning: dereferencing pointer 'ss.124' does break strict-aliasing rules
daemon.c:676: warning: dereferencing pointer 'ss.124' does break strict-aliasing rules
daemon.c:681: warning: dereferencing pointer 'ss.124' does break strict-aliasing rules
daemon.c:919: note: initialized from here
daemon.c:679: warning: dereferencing pointer 'sin_addr' does break strict-aliasing rules
daemon.c:675: note: initialized from here
daemon.c:691: warning: dereferencing pointer 'sin6_addr' does break strict-aliasing rules
daemon.c:682: note: initialized from here
Yeah, I'm aware of these. I thought those warnings were already
present in the Linux build, but checking again I see that that's not
the case. Need to investigate.
Otherwise it builds clean. The daemon running on Windows7 seems to be
working fine for both ipv4 and ipv6 connections (I tried both).
However, monitoring the resource usage in procexp it looks like there is
a handle leak. Each 'git ls-remote' over ipv6 is gaining 16 handles that
do not appear to be released. They're all process handles for dead
processes it looks like, so possibly there is a missing waitpid() or
something similar for the 'git daemon -serve' subprocess. Doing this
over ipv4 leaks 2 handles per request.
Ah, thanks. For me it's leaking a variable amount of handles per
ls-remote, but if I apply the following patch it's down to one. Need
to find that one as well...
From: Erik Faye-Lund <hidden> Date: 2016-06-15 22:49:58
On Wed, Nov 3, 2010 at 11:18 PM, Erik Faye-Lund [off-list ref] wrote:
quoted hunk
Ah, thanks. For me it's leaking a variable amount of handles per
ls-remote, but if I apply the following patch it's down to one. Need
to find that one as well...
}
if (pid > 0 && options & WNOHANG) {
- if (WAIT_OBJECT_0 != WaitForSingleObject((HANDLE)pid, 0))
+ if (WAIT_OBJECT_0 != WaitForSingleObject((HANDLE)pid, 0)) {
AAAND the last one is right here as well:
- if (WAIT_OBJECT_0 != WaitForSingleObject((HANDLE)pid, 0))
+ if (WAIT_OBJECT_0 != WaitForSingleObject(h, 0)) {
From: Erik Faye-Lund <hidden> Date: 2016-06-15 22:49:58
On Wed, Nov 3, 2010 at 11:18 PM, Erik Faye-Lund [off-list ref] wrote:
On Wed, Nov 3, 2010 at 10:11 PM, Pat Thoyts
[off-list ref] wrote:
quoted
Erik Faye-Lund [off-list ref] writes:
quoted
Here's hopefully the last iteration of this series. The previous version
only got a single complain about a typo in the subject of patch 14/15, so
it seems like most controversies have been settled.
I pulled this win32-daemon branch into my msysgit build tree and built
it. I get the following warnings:
CC daemon.o
daemon.c: In function 'service_loop':
daemon.c:674: warning: dereferencing pointer 'ss.124' does break strict-aliasing rules
daemon.c:676: warning: dereferencing pointer 'ss.124' does break strict-aliasing rules
daemon.c:681: warning: dereferencing pointer 'ss.124' does break strict-aliasing rules
daemon.c:919: note: initialized from here
daemon.c:679: warning: dereferencing pointer 'sin_addr' does break strict-aliasing rules
daemon.c:675: note: initialized from here
daemon.c:691: warning: dereferencing pointer 'sin6_addr' does break strict-aliasing rules
daemon.c:682: note: initialized from here
Yeah, I'm aware of these. I thought those warnings were already
present in the Linux build, but checking again I see that that's not
the case. Need to investigate.
OK, it's the patch "daemon: use run-command api for async serving"
that introduce the warning. But looking closer at the patch it doesn't
seem the patch actually introduce the strict-aliasing violation, it's
there already. The patch only seems to change the code enough for GCC
to start realize there's a problem. Unless I'm misunderstanding
something vital, that is.
Anyway, here's a patch that makes it go away, I guess I'll squash it
into the next round.
From: Erik Faye-Lund <hidden> Date: 2016-06-15 22:49:58
On Wed, Nov 3, 2010 at 11:58 PM, Erik Faye-Lund [off-list ref] wrote:
On Wed, Nov 3, 2010 at 11:18 PM, Erik Faye-Lund [off-list ref] wrote:
quoted
On Wed, Nov 3, 2010 at 10:11 PM, Pat Thoyts
[off-list ref] wrote:
quoted
Erik Faye-Lund [off-list ref] writes:
quoted
Here's hopefully the last iteration of this series. The previous version
only got a single complain about a typo in the subject of patch 14/15, so
it seems like most controversies have been settled.
I pulled this win32-daemon branch into my msysgit build tree and built
it. I get the following warnings:
CC daemon.o
daemon.c: In function 'service_loop':
daemon.c:674: warning: dereferencing pointer 'ss.124' does break strict-aliasing rules
daemon.c:676: warning: dereferencing pointer 'ss.124' does break strict-aliasing rules
daemon.c:681: warning: dereferencing pointer 'ss.124' does break strict-aliasing rules
daemon.c:919: note: initialized from here
daemon.c:679: warning: dereferencing pointer 'sin_addr' does break strict-aliasing rules
daemon.c:675: note: initialized from here
daemon.c:691: warning: dereferencing pointer 'sin6_addr' does break strict-aliasing rules
daemon.c:682: note: initialized from here
Yeah, I'm aware of these. I thought those warnings were already
present in the Linux build, but checking again I see that that's not
the case. Need to investigate.
OK, it's the patch "daemon: use run-command api for async serving"
that introduce the warning. But looking closer at the patch it doesn't
seem the patch actually introduce the strict-aliasing violation, it's
there already. The patch only seems to change the code enough for GCC
to start realize there's a problem. Unless I'm misunderstanding
something vital, that is.
Anyway, here's a patch that makes it go away, I guess I'll squash it
into the next round.
I also of course need to update "daemon: get remote host address from
root-process" as well, as it introduces a new such code-path (which is
actually the one complained about here). And I guess I should use
sockaddr_in instead of sockaddr.
But my luck stops there. The resulting git-daemon.exe leaves me with a
very bizarre error:
error: unable to make a socket file descriptor: Bad file descriptor
fatal: accept returned: Bad file descriptor
This is triggered by the call to accept() in mingw_accept returning -1.
What is even stranger is that if I change the code at the error-point like this
- struct sockaddr_in sa;
+ struct sockaddr_in sa[2];
socklen_t salen = sizeof(sa);
- int incoming = accept(pfd[i].fd, (struct sockaddr *)&sa, &salen);
+ int incoming = accept(pfd[i].fd, (struct sockaddr *)sa, &salen);
the error goes away. Similarly, if I change mingw_accept's call to
Winsock's accept(), like this:
- SOCKET s2 = accept(s1, sa, sz);
+ SOCKET s2 = accept(s1, sa, NULL);
So it seems accept() somehow reacts to the value of the variable
pointed at by sz, which is 16. Strange, huh?
Perhaps it isn't -- the sockaddr_storage change seems to have been
introduced for IPv6 reasons. I'm trying to connect over IPv6, and IPv6
has a new sockaddr_in6 struct. So yeah.
Stuffing all of sockaddr, sockaddr_in and sockaddr_in6 (when built
with IPv6 support) in a union and passing that around instead does
seem to fix the issue completely. I don't find it very elegant, but
some google-searches on the issue seems to reveal that this is the
only way of getting rid of this. Any other suggestions, people?
From: Erik Faye-Lund <hidden> Date: 2016-06-15 22:49:58
On Thu, Nov 4, 2010 at 1:06 AM, Erik Faye-Lund [off-list ref] wrote:
On Wed, Nov 3, 2010 at 11:58 PM, Erik Faye-Lund [off-list ref] wrote:
quoted
On Wed, Nov 3, 2010 at 11:18 PM, Erik Faye-Lund [off-list ref] wrote:
quoted
On Wed, Nov 3, 2010 at 10:11 PM, Pat Thoyts
[off-list ref] wrote:
quoted
Erik Faye-Lund [off-list ref] writes:
quoted
Here's hopefully the last iteration of this series. The previous version
only got a single complain about a typo in the subject of patch 14/15, so
it seems like most controversies have been settled.
I pulled this win32-daemon branch into my msysgit build tree and built
it. I get the following warnings:
CC daemon.o
daemon.c: In function 'service_loop':
daemon.c:674: warning: dereferencing pointer 'ss.124' does break strict-aliasing rules
daemon.c:676: warning: dereferencing pointer 'ss.124' does break strict-aliasing rules
daemon.c:681: warning: dereferencing pointer 'ss.124' does break strict-aliasing rules
daemon.c:919: note: initialized from here
daemon.c:679: warning: dereferencing pointer 'sin_addr' does break strict-aliasing rules
daemon.c:675: note: initialized from here
daemon.c:691: warning: dereferencing pointer 'sin6_addr' does break strict-aliasing rules
daemon.c:682: note: initialized from here
Yeah, I'm aware of these. I thought those warnings were already
present in the Linux build, but checking again I see that that's not
the case. Need to investigate.
OK, it's the patch "daemon: use run-command api for async serving"
that introduce the warning. But looking closer at the patch it doesn't
seem the patch actually introduce the strict-aliasing violation, it's
there already. The patch only seems to change the code enough for GCC
to start realize there's a problem. Unless I'm misunderstanding
something vital, that is.
Anyway, here's a patch that makes it go away, I guess I'll squash it
into the next round.
Stuffing all of sockaddr, sockaddr_in and sockaddr_in6 (when built
with IPv6 support) in a union and passing that around instead does
seem to fix the issue completely. I don't find it very elegant, but
some google-searches on the issue seems to reveal that this is the
only way of getting rid of this. Any other suggestions, people?
Just for reference, this is the patch that fixes it. What do you think?
From: Martin Storsjö <hidden> Date: 2016-06-15 22:49:58
On Thu, 4 Nov 2010, Erik Faye-Lund wrote:
quoted hunk
On Thu, Nov 4, 2010 at 1:06 AM, Erik Faye-Lund [off-list ref] wrote:
quoted
On Wed, Nov 3, 2010 at 11:58 PM, Erik Faye-Lund [off-list ref] wrote:
quoted
On Wed, Nov 3, 2010 at 11:18 PM, Erik Faye-Lund [off-list ref] wrote:
quoted
On Wed, Nov 3, 2010 at 10:11 PM, Pat Thoyts
[off-list ref] wrote:
quoted
Erik Faye-Lund [off-list ref] writes:
quoted
Here's hopefully the last iteration of this series. The previous version
only got a single complain about a typo in the subject of patch 14/15, so
it seems like most controversies have been settled.
I pulled this win32-daemon branch into my msysgit build tree and built
it. I get the following warnings:
CC daemon.o
daemon.c: In function 'service_loop':
daemon.c:674: warning: dereferencing pointer 'ss.124' does break strict-aliasing rules
daemon.c:676: warning: dereferencing pointer 'ss.124' does break strict-aliasing rules
daemon.c:681: warning: dereferencing pointer 'ss.124' does break strict-aliasing rules
daemon.c:919: note: initialized from here
daemon.c:679: warning: dereferencing pointer 'sin_addr' does break strict-aliasing rules
daemon.c:675: note: initialized from here
daemon.c:691: warning: dereferencing pointer 'sin6_addr' does break strict-aliasing rules
daemon.c:682: note: initialized from here
Yeah, I'm aware of these. I thought those warnings were already
present in the Linux build, but checking again I see that that's not
the case. Need to investigate.
OK, it's the patch "daemon: use run-command api for async serving"
that introduce the warning. But looking closer at the patch it doesn't
seem the patch actually introduce the strict-aliasing violation, it's
there already. The patch only seems to change the code enough for GCC
to start realize there's a problem. Unless I'm misunderstanding
something vital, that is.
Anyway, here's a patch that makes it go away, I guess I'll squash it
into the next round.
Stuffing all of sockaddr, sockaddr_in and sockaddr_in6 (when built
with IPv6 support) in a union and passing that around instead does
seem to fix the issue completely. I don't find it very elegant, but
some google-searches on the issue seems to reveal that this is the
only way of getting rid of this. Any other suggestions, people?
Just for reference, this is the patch that fixes it. What do you think?
@@ -902,9 +903,15 @@ static int service_loop(struct socketlist *socklist)for(i=0;i<socklist->nr;i++){if(pfd[i].revents&POLLIN){-structsockaddr_storagess;+union{+structsockaddrsa;+structsockaddr_insai;+#ifndef NO_IPV6+structsockaddr_in6sai6;+#endif+}ss;unsignedintsslen=sizeof(ss);-intincoming=accept(pfd[i].fd,(structsockaddr*)&ss,&sslen);+intincoming=accept(pfd[i].fd,&ss.sa,&sslen);if(incoming<0){switch(errno){caseEAGAIN:
@@ -915,7 +922,7 @@ static int service_loop(struct socketlist *socklist)die_errno("accept returned");}}-handle(incoming,(structsockaddr*)&ss,sslen);+handle(incoming,&ss.sa,sslen);}}}
As you say yourself, it's not elegant at all - sockaddr_storage is
intended to be just that, an struct large enough to fit all the sockaddrs
you'll encounter on this platform, with all fields aligned in the same way
as all the other sockaddr structs. You're supposed to be able to cast the
sockaddr struct pointers like currently is done, although I'm not familiar
with the strict aliasing stuff well enough to know if anything else would
be required somewhere.
I didn't see any of these hacks in the v7 patchset - did the warning go
away by itself there?
FWIW, the actual warning itself isn't directly related to any of the code
worked on here, gcc just happens to realize it after some of these
changes. I'm able to trigger the same warnings on the current master, by
simply doing this change:
From: Martin Storsjö <hidden> Date: 2016-06-15 22:49:58
On Wed, 3 Nov 2010, Erik Faye-Lund wrote:
quoted hunk
On Wed, Nov 3, 2010 at 11:18 PM, Erik Faye-Lund [off-list ref] wrote:
quoted
On Wed, Nov 3, 2010 at 10:11 PM, Pat Thoyts
[off-list ref] wrote:
quoted
Erik Faye-Lund [off-list ref] writes:
quoted
Here's hopefully the last iteration of this series. The previous version
only got a single complain about a typo in the subject of patch 14/15, so
it seems like most controversies have been settled.
I pulled this win32-daemon branch into my msysgit build tree and built
it. I get the following warnings:
CC daemon.o
daemon.c: In function 'service_loop':
daemon.c:674: warning: dereferencing pointer 'ss.124' does break strict-aliasing rules
daemon.c:676: warning: dereferencing pointer 'ss.124' does break strict-aliasing rules
daemon.c:681: warning: dereferencing pointer 'ss.124' does break strict-aliasing rules
daemon.c:919: note: initialized from here
daemon.c:679: warning: dereferencing pointer 'sin_addr' does break strict-aliasing rules
daemon.c:675: note: initialized from here
daemon.c:691: warning: dereferencing pointer 'sin6_addr' does break strict-aliasing rules
daemon.c:682: note: initialized from here
Yeah, I'm aware of these. I thought those warnings were already
present in the Linux build, but checking again I see that that's not
the case. Need to investigate.
OK, it's the patch "daemon: use run-command api for async serving"
that introduce the warning. But looking closer at the patch it doesn't
seem the patch actually introduce the strict-aliasing violation, it's
there already. The patch only seems to change the code enough for GCC
to start realize there's a problem. Unless I'm misunderstanding
something vital, that is.
Anyway, here's a patch that makes it go away, I guess I'll squash it
into the next round.
@@ -1159,14 +1159,13 @@ int main(int argc, char **argv)}if(inetd_mode||serve_mode){-structsockaddr_storagess;-structsockaddr*peer=(structsockaddr*)&ss;-socklen_tslen=sizeof(ss);+structsockaddrsa;+socklen_tslen=sizeof(sa);-if(getpeername(0,peer,&slen))-peer=NULL;--returnexecute(peer);+if(getpeername(0,&sa,&slen))+returnexecute(NULL);+else+returnexecute(&sa);}if(detach){
As you noticed later yourself, this is not right. You can get any kind of
sockaddr back from getpeername. I'm not sure if any spec mandates that a
"normal" sockaddr_in should fit into the base sockaddr, or if that just
happens by coincidence or sheer luck.
// Martin
From: Erik Faye-Lund <hidden> Date: 2016-06-15 22:49:58
On Thu, Nov 4, 2010 at 9:58 AM, Martin Storsjö [off-list ref] wrote:
On Thu, 4 Nov 2010, Erik Faye-Lund wrote:
quoted
On Thu, Nov 4, 2010 at 1:06 AM, Erik Faye-Lund [off-list ref] wrote:
quoted
On Wed, Nov 3, 2010 at 11:58 PM, Erik Faye-Lund [off-list ref] wrote:
quoted
On Wed, Nov 3, 2010 at 11:18 PM, Erik Faye-Lund [off-list ref] wrote:
quoted
On Wed, Nov 3, 2010 at 10:11 PM, Pat Thoyts
[off-list ref] wrote:
quoted
Erik Faye-Lund [off-list ref] writes:
quoted
Here's hopefully the last iteration of this series. The previous version
only got a single complain about a typo in the subject of patch 14/15, so
it seems like most controversies have been settled.
I pulled this win32-daemon branch into my msysgit build tree and built
it. I get the following warnings:
CC daemon.o
daemon.c: In function 'service_loop':
daemon.c:674: warning: dereferencing pointer 'ss.124' does break strict-aliasing rules
daemon.c:676: warning: dereferencing pointer 'ss.124' does break strict-aliasing rules
daemon.c:681: warning: dereferencing pointer 'ss.124' does break strict-aliasing rules
daemon.c:919: note: initialized from here
daemon.c:679: warning: dereferencing pointer 'sin_addr' does break strict-aliasing rules
daemon.c:675: note: initialized from here
daemon.c:691: warning: dereferencing pointer 'sin6_addr' does break strict-aliasing rules
daemon.c:682: note: initialized from here
Yeah, I'm aware of these. I thought those warnings were already
present in the Linux build, but checking again I see that that's not
the case. Need to investigate.
OK, it's the patch "daemon: use run-command api for async serving"
that introduce the warning. But looking closer at the patch it doesn't
seem the patch actually introduce the strict-aliasing violation, it's
there already. The patch only seems to change the code enough for GCC
to start realize there's a problem. Unless I'm misunderstanding
something vital, that is.
Anyway, here's a patch that makes it go away, I guess I'll squash it
into the next round.
Stuffing all of sockaddr, sockaddr_in and sockaddr_in6 (when built
with IPv6 support) in a union and passing that around instead does
seem to fix the issue completely. I don't find it very elegant, but
some google-searches on the issue seems to reveal that this is the
only way of getting rid of this. Any other suggestions, people?
Just for reference, this is the patch that fixes it. What do you think?
As you say yourself, it's not elegant at all - sockaddr_storage is
intended to be just that, an struct large enough to fit all the sockaddrs
you'll encounter on this platform, with all fields aligned in the same way
as all the other sockaddr structs. You're supposed to be able to cast the
sockaddr struct pointers like currently is done, although I'm not familiar
with the strict aliasing stuff well enough to know if anything else would
be required somewhere.
Strict aliasing isn't exactly about the structure being large enough
or not, it's only being able to access a particular piece of memory
through one type only (unless specificly marked with "union").
sockaddr_storage is an attempt at fixing the storage-problem without
addressing the type punning problem, which doesn't help us much.
I didn't see any of these hacks in the v7 patchset - did the warning go
away by itself there?
The strict aliasing problem was (as I described earlier, and you
pointed out later in your email) already present in the code. All the
patch did, was modify the code enough to make GCC realize it.
Since the code is removed by a later patch ("daemon: get remote host
address from root-process"), I figured adding a union just to remove
it was just noisy. So instead I changed the code enough for the
warning to go away again. It turned out that it was the assignment of
NULL to "peer" that triggered the warning, so I made two calls to
execute() instead, one that pass NULL and one that pass "peer".
Then in "daemon: get remote host address from root-process" which
causes a similar strict-aliasing issue (this time I'm the one who
introduced it, since the handle() code-path doesn't derefence "addr"),
I fixed it with a union.
quoted hunk
FWIW, the actual warning itself isn't directly related to any of the code
worked on here, gcc just happens to realize it after some of these
changes. I'm able to trigger the same warnings on the current master, by
simply doing this change:
*addr, int
dup2(incoming, 1);
close(incoming);
- exit(execute(addr));
}
static void child_handler(int signo)
// Martin
Yeah, I already figured that one out, but thanks for making it clearer. :)
So a nice end-result of v7 is that we're good with strict aliasing,
which means that it's not safe(er) to compile git-daemon on GCC with
-O3.
From: Martin Storsjö <hidden> Date: 2016-06-15 22:49:58
On Thu, 4 Nov 2010, Erik Faye-Lund wrote:
On Thu, Nov 4, 2010 at 9:58 AM, Martin Storsjö [off-list ref] wrote:
quoted
On Thu, 4 Nov 2010, Erik Faye-Lund wrote:
quoted
On Thu, Nov 4, 2010 at 1:06 AM, Erik Faye-Lund [off-list ref] wrote:
quoted
Stuffing all of sockaddr, sockaddr_in and sockaddr_in6 (when built
with IPv6 support) in a union and passing that around instead does
seem to fix the issue completely. I don't find it very elegant, but
some google-searches on the issue seems to reveal that this is the
only way of getting rid of this. Any other suggestions, people?
Just for reference, this is the patch that fixes it. What do you think?
As you say yourself, it's not elegant at all - sockaddr_storage is
intended to be just that, an struct large enough to fit all the sockaddrs
you'll encounter on this platform, with all fields aligned in the same way
as all the other sockaddr structs. You're supposed to be able to cast the
sockaddr struct pointers like currently is done, although I'm not familiar
with the strict aliasing stuff well enough to know if anything else would
be required somewhere.
Strict aliasing isn't exactly about the structure being large enough
or not, it's only being able to access a particular piece of memory
through one type only (unless specificly marked with "union").
sockaddr_storage is an attempt at fixing the storage-problem without
addressing the type punning problem, which doesn't help us much.
Given this, does that mean that all code that uses sockaddr_storage
directly without a union, and casting pointers to this struct into
sockaddr, sockaddr_in and sockaddr_in6 is incorrect with regards to strict
aliasing?
That is, even this example from RFC 2553 is faulty:
struct sockaddr_storage __ss;
struct sockaddr_in6 *sin6;
sin6 = (struct sockaddr_in6 *) &__ss;
quoted
I didn't see any of these hacks in the v7 patchset - did the warning go
away by itself there?
The strict aliasing problem was (as I described earlier, and you
pointed out later in your email) already present in the code. All the
patch did, was modify the code enough to make GCC realize it.
Since the code is removed by a later patch ("daemon: get remote host
address from root-process"), I figured adding a union just to remove
it was just noisy. So instead I changed the code enough for the
warning to go away again. It turned out that it was the assignment of
NULL to "peer" that triggered the warning, so I made two calls to
execute() instead, one that pass NULL and one that pass "peer".
Then in "daemon: get remote host address from root-process" which
causes a similar strict-aliasing issue (this time I'm the one who
introduced it, since the handle() code-path doesn't derefence "addr"),
I fixed it with a union.
Ah, I didn't see that one. Ok, that explains it.
The union solution is correct although not pretty, while the one changing
it to a bare sockaddr was wrong, which was what caught my attention. :-)
So a nice end-result of v7 is that we're good with strict aliasing,
which means that it's not safe(er) to compile git-daemon on GCC with
-O3.
Actually, couldn't there still be similar strict aliasing violations left
that GCC hasn't realized yet?
// Martin
From: Erik Faye-Lund <hidden> Date: 2016-06-15 22:49:58
On Thu, Nov 4, 2010 at 10:35 AM, Martin Storsjö [off-list ref] wrote:
On Thu, 4 Nov 2010, Erik Faye-Lund wrote:
quoted
On Thu, Nov 4, 2010 at 9:58 AM, Martin Storsjö [off-list ref] wrote:
quoted
On Thu, 4 Nov 2010, Erik Faye-Lund wrote:
quoted
On Thu, Nov 4, 2010 at 1:06 AM, Erik Faye-Lund [off-list ref] wrote:
quoted
Stuffing all of sockaddr, sockaddr_in and sockaddr_in6 (when built
with IPv6 support) in a union and passing that around instead does
seem to fix the issue completely. I don't find it very elegant, but
some google-searches on the issue seems to reveal that this is the
only way of getting rid of this. Any other suggestions, people?
Just for reference, this is the patch that fixes it. What do you think?
As you say yourself, it's not elegant at all - sockaddr_storage is
intended to be just that, an struct large enough to fit all the sockaddrs
you'll encounter on this platform, with all fields aligned in the same way
as all the other sockaddr structs. You're supposed to be able to cast the
sockaddr struct pointers like currently is done, although I'm not familiar
with the strict aliasing stuff well enough to know if anything else would
be required somewhere.
Strict aliasing isn't exactly about the structure being large enough
or not, it's only being able to access a particular piece of memory
through one type only (unless specificly marked with "union").
sockaddr_storage is an attempt at fixing the storage-problem without
addressing the type punning problem, which doesn't help us much.
Given this, does that mean that all code that uses sockaddr_storage
directly without a union, and casting pointers to this struct into
sockaddr, sockaddr_in and sockaddr_in6 is incorrect with regards to strict
aliasing?
That is, even this example from RFC 2553 is faulty:
struct sockaddr_storage __ss;
struct sockaddr_in6 *sin6;
sin6 = (struct sockaddr_in6 *) &__ss;
Yes, at least with C99. Section 6.5, paragraph 7 of the C99 specification says:
"An object shall have its stored value accessed only by an lvalue
expression that has one of
the following types:
- a type compatible with the effective type of the object,
- a qualified version of a type compatible with the effective type of the object,
- a type that is the signed or unsigned type corresponding to the
effective type of the
object,
- a type that is the signed or unsigned type corresponding to a
qualified version of the
effective type of the object,
- an aggregate or union type that includes one of the aforementioned
types among its
members (including, recursively,amember of a subaggregate or contained
union), or
- a character type."
A type punned pointer does not meet any of those requirements.
For pre-C99 I don't have any reference other than Wikpedia, which
indicates that it's illegal in the ISO version of the standard (which
I assume must be C90):
"To enable such optimizations in a predictable manner, the ISO
standard for the C programming language (including its newer C99
edition, see section 6.5, paragraph 7) specifies that it is illegal
(with some exceptions) for pointers of different types to reference
the same memory location."
ref: http://en.wikipedia.org/wiki/Aliasing_(computing)
But the code above is incorrect in another sense: it's declaring a
symbol with a name starting with double underscore, something that is
reserved for the compiler. So I don't think I would trust the person
who wrote it to care much about standards compliance.