[Bug c/46488] New: server/core_filters.c from apache httpd 2.2.17 does not produce good code with optimizations (-O) used on Solaris Sparc 64-bit

[Bug c/46488] New: server/core_filters.c from apache httpd 2.2.17 does not produce good code with optimizations (-O) used on Solaris Sparc 64-bit

[lk0946 at att dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46488

           Summary: server/core_filters.c from apache httpd 2.2.17 does
                    not produce good code with optimizations (-O) used on
                    Solaris Sparc 64-bit
           Product: gcc
           Version: 4.5.1
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: lk0946@att.com


Created attachment 22403
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=22403
Preprocessed server/core_filters.c

Compiling the latest Apache httpd (2.2.17) using GCC 4.5.1 on Solaris Sparc and
64-bit produces a non-functioning web server.

Attempts to connect to httpd result in an immediate hang-up with no logs.

Recompiling server/core_filters.c without -O[2,3] produces a working httpd.

GCC was built with the following options:
../configure --with-gnu-as --with-as=/sites/compiler/bin/as \
        --without-gnu-ld \
        --with-ld=/usr/ccs/bin/ld \
        --enable-shared --enable-languages=c,c++ \
        --with-gmp=/sites/compiler/64bit --with-mpfr=/sites/compiler/64bit \
        --prefix=/sites/compiler/64bit \
        --build=sparc64-sun-solaris2.10

GNU C (GCC) version 4.5.1 (sparc64-sun-solaris2.10)
        compiled by GNU C version 4.5.1, GMP version 4.3.2, MPFR version 2.4.2,
MPC version 0.8.1
GGC heuristics: --param ggc-min-expand=100 --param ggc-min-heapsize=131072

There are no compiler warnings when compiling httpd.

httpd configure command:

./configure --prefix=/sites/httpd-2.2.17-worker-niagara-64
--enable-mods-shared=all --with-mpm=worker --enable-logio --enable-headers
--disable-autoindex --enable-cgi --disable-userdir --enable-so --enable-ssl
--disable-ipv6 --with-ssl=/sites/OPENSSL/openssl-1.0.0-niagara-64 --enable-info
--enable-nonportable-atomics=yes --enable-deflate --enable-unique-id
--with-pcre=/sites/utils --enable-proxy --build=sparc64-sun-solaris2.10
CFLAGS are:
CFLAGS='-m64 -mcpu=niagara -mtune=niagara -O3'

Pre-processed file is attached.

[Bug middle-end/46488] [4.5 regression] server/core_filters.c from apache httpd 2.2.17 miscompiled with optimization

[ebotcazou at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46488

Eric Botcazou <ebotcazou at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Target|                            |sparc64-sun-solaris2.*
                 CC|                            |ebotcazou at gcc dot
                   |                            |gnu.org
               Host|                            |sparc64-sun-solaris2.*
   Target Milestone|---                         |4.5.2
            Summary|server/core_filters.c from  |[4.5 regression]
                   |apache httpd 2.2.17 does    |server/core_filters.c from
                   |not produce good code with  |apache httpd 2.2.17
                   |optimizations (-O) used on  |miscompiled with
                   |Solaris Sparc 64-bit        |optimization
              Build|                            |sparc64-sun-solaris2.*

--- Comment #1 from Eric Botcazou <ebotcazou at gcc dot gnu.org> 2010-11-16 08:28:32 UTC ---
Any clue as to which function/which lines are miscompiled exactly?  Does it
work if you compile the file with -O -fno-delayed-branch?

[Bug middle-end/46488] [4.5 regression] server/core_filters.c from apache httpd 2.2.17 miscompiled with optimization

[lk0946 at att dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46488

--- Comment #2 from Lance Kinley <lk0946 at att dot com> 2010-11-16 16:37:20 UTC ---
Adding -fno-delayed-branch does not resolve the issue.

I don't know which function is the culprit.  I'll try to dig into it more.

[Bug middle-end/46488] [4.5 regression] server/core_filters.c from apache httpd 2.2.17 miscompiled with optimization

[lk0946 at att dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46488

--- Comment #3 from Lance Kinley <lk0946 at att dot com> 2010-11-16 16:48:11 UTC ---
My apologies, -O -fno-delayed-branch does fix it.

-O3 -fno-delayed-branch does not work, however.

[Bug middle-end/46488] [4.5 regression] server/core_filters.c from apache httpd 2.2.17 miscompiled with optimization

[ebotcazou at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46488

Eric Botcazou <ebotcazou at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |WAITING
   Last reconfirmed|                            |2010.11.16 17:11:11
     Ever Confirmed|0                           |1

--- Comment #4 from Eric Botcazou <ebotcazou at gcc dot gnu.org> 2010-11-16 17:11:11 UTC ---
> My apologies, -O -fno-delayed-branch does fix it.
> 
> -O3 -fno-delayed-branch does not work, however.

There might be several issues then.  Could you try the latest 4.5.x snapshot?
At least a couple of issues related to -fdelayed-branch were fixed in between.

[Bug middle-end/46488] [4.5 regression] server/core_filters.c from apache httpd 2.2.17 miscompiled with optimization

[lk0946 at att dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46488

--- Comment #5 from Lance Kinley <lk0946 at att dot com> 2010-11-16 23:52:24 UTC ---
I compiled the latest snapshot:
gcc (GCC) 4.5.2 20101111 (prerelease)

and tried -O3 -fno-delayed-branch again on server/core_filters.c and that still
doesn't work.

I retried -O -fno-delayed-branch and that continues to work with the new gcc
build.

[Bug middle-end/46488] [4.5 regression] server/core_filters.c from apache httpd 2.2.17 miscompiled with optimization

[ebotcazou at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46488

--- Comment #6 from Eric Botcazou <ebotcazou at gcc dot gnu.org> 2010-11-17 00:09:42 UTC ---
> I compiled the latest snapshot:
> gcc (GCC) 4.5.2 20101111 (prerelease)
> 
> and tried -O3 -fno-delayed-branch again on server/core_filters.c and that still
> doesn't work.
> 
> I retried -O -fno-delayed-branch and that continues to work with the new gcc
> build.

-fno-delayed-branch is supposed to be useless with this snapshot though.

[Bug middle-end/46488] [4.5 regression] server/core_filters.c from apache httpd 2.2.17 miscompiled with optimization

[lk0946 at att dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46488

--- Comment #7 from Lance Kinley <lk0946 at att dot com> 2010-11-17 06:15:36 UTC ---
I compiled and used the latest SVN snapshot:
gcc (GCC) 4.6.0 20101116 (experimental) [trunk revision 166840]

And got the same exact results as before with regard to both -O and -O3 in
combination with -fno-delayed-branch

[Bug middle-end/46488] [4.5 regression] server/core_filters.c from apache httpd 2.2.17 miscompiled with optimization

[ebotcazou at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46488

--- Comment #8 from Eric Botcazou <ebotcazou at gcc dot gnu.org> 2010-11-17 07:41:44 UTC ---
> And got the same exact results as before with regard to both -O and -O3 in
> combination with -fno-delayed-branch

Sorry for being unclear: starting with the 4.5.2 snapshot, issues related to
-fno-delayed-branch should be fixed so you should drop this workaround
entirely.

[Bug middle-end/46488] [4.5 regression] server/core_filters.c from apache httpd 2.2.17 miscompiled with optimization

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46488

--- Comment #9 from Richard Guenther <rguenth at gcc dot gnu.org> 2010-11-25 15:59:18 UTC ---
Works with GCC version ... ?

[Bug middle-end/46488] [4.5 regression] server/core_filters.c from apache httpd 2.2.17 miscompiled with optimization

[ebotcazou at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46488

--- Comment #10 from Eric Botcazou <ebotcazou at gcc dot gnu.org> 2010-11-25 16:34:32 UTC ---
Does the problem only happen at -O3 with the 4.5.x snapshot, i.e. do
compilations with bare -O or -O2 generate working executables?

[Bug middle-end/46488] [4.5 regression] server/core_filters.c from apache httpd 2.2.17 miscompiled with optimization

[lk0946 at att dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46488

--- Comment #11 from Lance Kinley <lk0946 at att dot com> 2010-11-26 22:10:16 UTC ---
Works with -O and -O2 on the 4.5 snapshot and the 4.6 snapshot.

-O3 is the culprit.

I have not tested with any other versions other than 4.5.1 and the two
snapshots mentioned.

[Bug middle-end/46488] server/core_filters.c from apache httpd 2.2.17 miscompiled with optimization -O3

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46488

Richard Guenther <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|4.5.2                       |---
            Summary|[4.5 regression]            |server/core_filters.c from
                   |server/core_filters.c from  |apache httpd 2.2.17
                   |apache httpd 2.2.17         |miscompiled with
                   |miscompiled with            |optimization -O3
                   |optimization                |
      Known to fail|                            |4.5.1, 4.6.0

--- Comment #12 from Richard Guenther <rguenth at gcc dot gnu.org> 2010-11-26 22:22:12 UTC ---
I you can't confirm that it worked with 4.4.x or earlier versions with the
options that break with 4.5 and 4.6 this isn't a regression.

Btw, in the original report it sounds like -O2 is broken as well, but in
reality only -O3 is broken?  Both for 4.5 and 4.6 snapshots?

[Bug middle-end/46488] server/core_filters.c from apache httpd 2.2.17 miscompiled with optimization -O3

[lk0946 at att dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46488

--- Comment #13 from Lance Kinley <lk0946 at att dot com> 2010-11-26 22:57:08 UTC ---
-O and -O2 are broken in 4.5.1 but work in the 4.5 and 4.6 snapshots.

[Bug middle-end/46488] server/core_filters.c from apache httpd 2.2.17 miscompiled with optimization -O3

[ebotcazou at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46488

Eric Botcazou <ebotcazou at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|WAITING                     |NEW
      Known to fail|4.5.1                       |4.5.2

--- Comment #14 from Eric Botcazou <ebotcazou at gcc dot gnu.org> 2010-11-27 12:19:09 UTC ---
> Works with -O and -O2 on the 4.5 snapshot and the 4.6 snapshot.
> 
> -O3 is the culprit.

OK, thanks for confirming.  Any idea as to which part of the code is
miscompiled?

[Bug middle-end/46488] server/core_filters.c from apache httpd 2.2.17 miscompiled with optimization -O3

[LpSolit at netscape dot net]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46488

Frédéric Buclin <LpSolit at netscape dot net> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |LpSolit at netscape dot net

--- Comment #15 from Frédéric Buclin <LpSolit at netscape dot net> 2010-11-27 23:18:58 UTC ---
At least OpenSUSE and Mandriva Linux are affected by this issue, see:

 https://qa.mandriva.com/show_bug.cgi?id=61384
 https://issues.apache.org/bugzilla/show_bug.cgi?id=50190

Also, the 2nd report says that it was working fine with gcc 4.4, so it really
looks like a regression in gcc 4.5.x.

[Bug middle-end/46488] [4.5 regression] server/core_filters.c from apache httpd 2.2.17 miscompiled at -O3

[ebotcazou at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46488

Eric Botcazou <ebotcazou at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Target|sparc64-sun-solaris2.*      |
      Known to work|                            |4.4.3
               Host|sparc64-sun-solaris2.*      |
   Target Milestone|---                         |4.5.2
            Summary|server/core_filters.c from  |[4.5 regression]
                   |apache httpd 2.2.17         |server/core_filters.c from
                   |miscompiled with            |apache httpd 2.2.17
                   |optimization -O3            |miscompiled at -O3
              Build|sparc64-sun-solaris2.*      |
           Severity|normal                      |major

--- Comment #16 from Eric Botcazou <ebotcazou at gcc dot gnu.org> 2010-11-28 08:58:47 UTC ---
> At least OpenSUSE and Mandriva Linux are affected by this issue, see:
> 
>  https://qa.mandriva.com/show_bug.cgi?id=61384
>  https://issues.apache.org/bugzilla/show_bug.cgi?id=50190

This looks indeed similar, thus recategorizing.

[Bug middle-end/46488] [4.5 regression] server/core_filters.c from apache httpd 2.2.17 miscompiled at -O3

[ebotcazou at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46488

--- Comment #17 from Eric Botcazou <ebotcazou at gcc dot gnu.org> 2010-11-28 12:28:27 UTC ---
It appears that inlining brigade_move into ap_core_input_filter and
ap_core_output_filter at -O3 causes (one of) the latter functions to be
miscompiled with -fstrict-aliasing.

The code uses clever type punning which seems to break C aliasing rules:

/**
 * The Ring Sentinel
 *
 * This is the magic pointer value that occurs before the first and
 * after the last elements in the ring, computed from the address of
 * the ring's head.  The head itself isn't an element, but in order to
 * get rid of all the special cases when dealing with the ends of the
 * ring, we play typecasting games to make it look like one.

[...]

#define APR_RING_SENTINEL(hp, elem, link)                \
    (struct elem *)((char *)(&(hp)->next) - APR_OFFSETOF(struct elem, link))

(hp)->next is struct elem* but it is accessed (its link field) as a struct elem
through various macros:

#define APR_RING_NEXT(ep, link)    (ep)->link.next
#define APR_RING_PREV(ep, link)    (ep)->link.prev

[Bug middle-end/46488] [4.5 regression] server/core_filters.c from apache httpd 2.2.17 miscompiled at -O3

[ebotcazou at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46488

--- Comment #18 from Eric Botcazou <ebotcazou at gcc dot gnu.org> 2010-11-28 13:21:51 UTC ---
Using

#define APR_RING_SENTINEL(hp, elem, link)                \
    (struct elem *)((char *)(hp) - APR_OFFSETOF(struct elem, link))

should be safer wrt strict aliasing.

[Bug middle-end/46488] [4.5 regression] server/core_filters.c from apache httpd 2.2.17 miscompiled at -O3

[jorton at redhat dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46488

Joe Orton <jorton at redhat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jorton at redhat dot com

--- Comment #19 from Joe Orton <jorton at redhat dot com> 2010-11-29 11:02:26 UTC ---
(In reply to comment #18)
> Using
> 
> #define APR_RING_SENTINEL(hp, elem, link)                \
>     (struct elem *)((char *)(hp) - APR_OFFSETOF(struct elem, link))
> 
> should be safer wrt strict aliasing.

We changed the macro from that to the current definition to avoid strict
aliasing warnings from gcc:

  http://svn.apache.org/viewvc?view=revision&revision=662299

Does the cast through a char * not have the desired effect of allowing the
pointer to alias any other pointer regardless of type?  That is one of the
exceptions in C99 6.5/par 7.

Why specifically does this result in an C99 aliasing violation anyway?  The
pointers to which this macro evaluates are never dereferenced, only compared
for equality.

[Bug middle-end/46488] [4.5 regression] server/core_filters.c from apache httpd 2.2.17 miscompiled at -O3

[ebotcazou at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46488

--- Comment #20 from Eric Botcazou <ebotcazou at gcc dot gnu.org> 2010-11-29 11:15:58 UTC ---
> We changed the macro from that to the current definition to avoid strict
> aliasing warnings from gcc:
> 
>   http://svn.apache.org/viewvc?view=revision&revision=662299

Bummer. :-)

> Does the cast through a char * not have the desired effect of allowing the
> pointer to alias any other pointer regardless of type?  That is one of the
> exceptions in C99 6.5/par 7.

Classical misconception.  What matters is the type of the lvalue used to access
the object, not the types through which the pointer value is casted, i.e:

  float f = 0.0;
  int t;

  t = *(int *)(char *)&f;

is as erroneous as:

  float f = 0.0;
  int t;

  t = *(int *)&f;

> Why specifically does this result in an C99 aliasing violation anyway?  The
> pointers to which this macro evaluates are never dereferenced, only compared
> for equality.

I think they are dereferenced by the macro APR_RING_{NEXT,PREV} in some cases.

[Bug middle-end/46488] [4.5 regression] server/core_filters.c from apache httpd 2.2.17 miscompiled at -O3

[jorton at redhat dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46488

--- Comment #21 from Joe Orton <jorton at redhat dot com> 2010-11-29 11:38:44 UTC ---
Thanks for the explanation.

(In reply to comment #20)
> > Why specifically does this result in an C99 aliasing violation anyway?  The
> > pointers to which this macro evaluates are never dereferenced, only compared
> > for equality.
> 
> I think they are dereferenced by the macro APR_RING_{NEXT,PREV} in some cases.

The pointers are constructed explicitly to never be dereferenced, only compared
for equality; if a dereference exists it would be a bug, but I don't see one.

In the absence of such a smoking gun, can an C99 aliasing issue occur merely in
handling pointer equivalence?

[Bug middle-end/46488] [4.5 regression] server/core_filters.c from apache httpd 2.2.17 miscompiled at -O3

[ebotcazou at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46488

--- Comment #22 from Eric Botcazou <ebotcazou at gcc dot gnu.org> 2010-11-29 12:09:30 UTC ---
> The pointers are constructed explicitly to never be dereferenced, only
> compared for equality; if a dereference exists it would be a bug, but
> I don't see one.

APR_RING_SPLICE_HEAD does such a dereference as far I can see:

#define APR_RING_SPLICE_HEAD(hp, ep1, epN, elem, link)            \
    APR_RING_SPLICE_AFTER(APR_RING_SENTINEL((hp), elem, link),    \
                 (ep1), (epN), link)

#define APR_RING_SPLICE_AFTER(lep, ep1, epN, link) do {            \
    APR_RING_PREV((ep1), link) = (lep);                \
    APR_RING_NEXT((epN), link) = APR_RING_NEXT((lep), link);    \
    APR_RING_PREV(APR_RING_NEXT((lep), link), link) = (epN);    \
    APR_RING_NEXT((lep), link) = (ep1);                \
    } while (0)

#define APR_RING_NEXT(ep, link)    (ep)->link.next
#define APR_RING_PREV(ep, link)    (ep)->link.prev

> In the absence of such a smoking gun, can an C99 aliasing issue occur merely
> in handling pointer equivalence?

That would be a real compiler bug. :-)


You seem to be quite familiar with the httpd code.  Can you pinpoint what is
miscompiled exactly in ap_core_input_filter or ap_core_output_filter?  I'm not
saying that the miscompilation comes from this strict aliasing issue, only that
it might at this point.

[Bug middle-end/46488] [4.5 regression] server/core_filters.c from apache httpd 2.2.17 miscompiled at -O3

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46488

Richard Guenther <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |rguenth at gcc dot gnu.org

--- Comment #23 from Richard Guenther <rguenth at gcc dot gnu.org> 2010-11-29 13:36:20 UTC ---
(In reply to comment #22)
> > The pointers are constructed explicitly to never be dereferenced, only
> > compared for equality; if a dereference exists it would be a bug, but
> > I don't see one.
> 
> APR_RING_SPLICE_HEAD does such a dereference as far I can see:
> 
> #define APR_RING_SPLICE_HEAD(hp, ep1, epN, elem, link)            \
>     APR_RING_SPLICE_AFTER(APR_RING_SENTINEL((hp), elem, link),    \
>                  (ep1), (epN), link)
> 
> #define APR_RING_SPLICE_AFTER(lep, ep1, epN, link) do {            \
>     APR_RING_PREV((ep1), link) = (lep);                \
>     APR_RING_NEXT((epN), link) = APR_RING_NEXT((lep), link);    \
>     APR_RING_PREV(APR_RING_NEXT((lep), link), link) = (epN);    \
>     APR_RING_NEXT((lep), link) = (ep1);                \
>     } while (0)
> 
> #define APR_RING_NEXT(ep, link)    (ep)->link.next
> #define APR_RING_PREV(ep, link)    (ep)->link.prev
> 
> > In the absence of such a smoking gun, can an C99 aliasing issue occur merely
> > in handling pointer equivalence?
> 
> That would be a real compiler bug. :-)
> 
> 
> You seem to be quite familiar with the httpd code.  Can you pinpoint what is
> miscompiled exactly in ap_core_input_filter or ap_core_output_filter?  I'm not
> saying that the miscompilation comes from this strict aliasing issue, only that
> it might at this point.

Btw, in 4.6 this was likely "fixed" by making all pointers inherit the
same alias set as void * (as this is a very common misconception that
you can alias all pointers with void *, or that all pointers do alias
in general):

2010-08-25  Richard Guenther  <rguenther@suse.de>

        * alias.c (get_alias_set): Assign a single alias-set to all pointers.
        * gimple.c (gimple_get_alias_set): Remove special handling
        for pointers.

it doesn't make the type-punning valid, of course, but in 4.6 we try
harder to not break users code just because we are allowed to.

[Bug middle-end/46488] [4.5 regression] server/core_filters.c from apache httpd 2.2.17 miscompiled at -O3

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46488

--- Comment #24 from Richard Guenther <rguenth at gcc dot gnu.org> 2010-11-29 13:39:02 UTC ---
(In reply to comment #18)
> Using
> 
> #define APR_RING_SENTINEL(hp, elem, link)                \
>     (struct elem *)((char *)(hp) - APR_OFFSETOF(struct elem, link))
> 
> should be safer wrt strict aliasing.

not really.  But using

typedef struct elem elem_ __attribute__((may_alias));
#define APR_RING_SENTINEL(hp, elem, link) \
    (struct elem_ *)((char *)(hp) - APR_OFFSETOF(struct elem, link))

would be safe wrt strict aliasing (if the pointer is only dereferenced
directly and not casted to another pointer type before, of course).

[Bug middle-end/46488] [4.5 regression] server/core_filters.c from apache httpd 2.2.17 miscompiled at -O3

[jorton at redhat dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46488

--- Comment #25 from Joe Orton <jorton at redhat dot com> 2010-11-29 14:03:40 UTC ---
(In reply to comment #22)
> APR_RING_SPLICE_HEAD does such a dereference as far I can see:

You are right, sorry, I had forgotten exactly how this code works.

(In reply to comment #24)
> not really.  But using
> 
> typedef struct elem elem_ __attribute__((may_alias));
> #define APR_RING_SENTINEL(hp, elem, link) \
>     (struct elem_ *)((char *)(hp) - APR_OFFSETOF(struct elem, link))
> 
> would be safe wrt strict aliasing (if the pointer is only dereferenced
> directly and not casted to another pointer type before, of course).

I don't think we can guarantee this with the API unchanged; the sentinel
pointer could well be stored in a "struct elem *" pointer.

[Bug middle-end/46488] [4.5 regression] server/core_filters.c from apache httpd 2.2.17 miscompiled at -O3

[rguenther at suse dot de]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46488

--- Comment #26 from rguenther at suse dot de <rguenther at suse dot de> 2010-11-29 14:20:45 UTC ---
On Mon, 29 Nov 2010, jorton at redhat dot com wrote:

> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46488
> 
> --- Comment #25 from Joe Orton <jorton at redhat dot com> 2010-11-29 14:03:40 UTC ---
> (In reply to comment #22)
> > APR_RING_SPLICE_HEAD does such a dereference as far I can see:
> 
> You are right, sorry, I had forgotten exactly how this code works.
> 
> (In reply to comment #24)
> > not really.  But using
> > 
> > typedef struct elem elem_ __attribute__((may_alias));
> > #define APR_RING_SENTINEL(hp, elem, link) \
> >     (struct elem_ *)((char *)(hp) - APR_OFFSETOF(struct elem, link))
> > 
> > would be safe wrt strict aliasing (if the pointer is only dereferenced
> > directly and not casted to another pointer type before, of course).
> 
> I don't think we can guarantee this with the API unchanged; the sentinel
> pointer could well be stored in a "struct elem *" pointer.

You could stick the attribute on the elem struct declaration itself then.

Richard.

[Bug middle-end/46488] [4.5 regression] server/core_filters.c from apache httpd 2.2.17 miscompiled at -O3

[pcpa at mandriva dot com.br]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46488

--- Comment #27 from Paulo César Pereira de Andrade <pcpa at mandriva dot com.br> 2010-11-29 21:26:32 UTC ---
Created attachment 22572
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=22572
test case

I learned most of apache/apr/apr-util internals while debugging this problem in
mandriva cooker,
so, maybe the test case can be made simpler... The crash in mandriva is only
confirmed to happen
in i586. Sample run of the test case:

[pcpa@localhost ~]$ gcc -O0 -fstrict-aliasing -D_LARGEFILE64_SOURCE
-I/usr/include/apr-1 -g3 apr-test.c; LC_ALL=C ./a.out
It works!
[pcpa@localhost ~]$ gcc -O3 -fstrict-aliasing -D_LARGEFILE64_SOURCE
-I/usr/include/apr-1 -g3 apr-test.c; LC_ALL=C ./a.out
a.out: apr-test.c:59: check: Assertion `e->link.next->link.prev == e' failed.
Abortado
[pcpa@localhost ~]$ gcc -O3 -fno-strict-aliasing -D_LARGEFILE64_SOURCE
-I/usr/include/apr-1 -g3 apr-test.c; LC_ALL=C ./a.out
It works!

[pardon choosing the "It works!" string :-)]

It also appears to correct the issue with -fstrict-aliasing by adding
temporaries, e.g:
-#define APR_RING_SPLICE_HEAD(hp, ep1, epN, elem, link)                 \
-       APR_RING_SPLICE_AFTER(APR_RING_SENTINEL((hp), elem, link),      \
-                            (ep1), (epN), link)
+#define APR_RING_SPLICE_HEAD(hp, ep1, epN, elem, link) do {            \
+       struct elem *lep = APR_RING_SENTINEL((hp), elem, link);         \
+       APR_RING_SPLICE_AFTER(lep, (ep1), (epN), link);                 \
+    } while (0)

The attribute noinline in the test case was to "somehow" make gcc not
understand where
the pointers come from, otherwise, if removing any of them, it will also work.

[Bug middle-end/46488] [4.5 regression] server/core_filters.c from apache httpd 2.2.17 miscompiled at -O3

[ebotcazou at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46488

Eric Botcazou <ebotcazou at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
         AssignedTo|unassigned at gcc dot       |ebotcazou at gcc dot
                   |gnu.org                     |gnu.org

--- Comment #28 from Eric Botcazou <ebotcazou at gcc dot gnu.org> 2010-11-30 07:40:00 UTC ---
> I learned most of apache/apr/apr-util internals while debugging this problem in
> mandriva cooker,
> so, maybe the test case can be made simpler... The crash in mandriva is only
> confirmed to happen in i586.

Thanks a lot, I can reproduce with 4.5.x and -O2 on i586.  Investigating.

[Bug middle-end/46488] [4.5 regression] server/core_filters.c from apache httpd 2.2.17 miscompiled at -O3

[ebotcazou at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46488

Eric Botcazou <ebotcazou at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #22403|0                           |1
        is obsolete|                            |
  Attachment #22572|0                           |1
        is obsolete|                            |

--- Comment #29 from Eric Botcazou <ebotcazou at gcc dot gnu.org> 2010-11-30 08:14:30 UTC ---
Created attachment 22575
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=22575
Stand-alone testcase

Fails at -O2 with 4.5.x on x86, doesn't fail with -fno-strict-aliasing or
4.6.0.

[Bug middle-end/46488] [4.5 regression] server/core_filters.c from apache httpd 2.2.17 miscompiled at -O3

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46488

--- Comment #30 from Richard Guenther <rguenth at gcc dot gnu.org> 2010-11-30 10:15:02 UTC ---
>From a quick look I can see that with -fstrict-aliasing we will never consider
ptr->link.next to alias ptr->list.next (so if they are made to alias via
casting the testcase is invalid).

Which I can see here:

brigade_move(apr_bucket_brigade *b, apr_bucket_brigade *a, apr_bucket *e)
{
    apr_bucket *f;

    if (e != (struct apr_bucket *)((char *)(&(&(b)->list)->next) - ((long)
(((char *) (&(((struct apr_bucket*)0)->link))) - ((char *) 0))))) 

(struct apr_bucket *)((char *)(&(&(b)->list)->next) does exactly this
(would be nice to rewrite the testcase to use offsetof btw).

[Bug middle-end/46488] [4.5 regression] server/core_filters.c from apache httpd 2.2.17 miscompiled at -O3

[ebotcazou at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46488

--- Comment #31 from Eric Botcazou <ebotcazou at gcc dot gnu.org> 2010-11-30 12:57:29 UTC ---
> From a quick look I can see that with -fstrict-aliasing we will never consider
> ptr->link.next to alias ptr->list.next (so if they are made to alias via
> casting the testcase is invalid).

Yes, that's it.  The miscompilation happens very late (equiv memory location
handling during register allocation) but ultimately indirect_refs_may_alias_p
decides that:

(gdb) p debug_generic_expr(ref1)
D.2012_2->list.prev

(gdb) p debug_generic_expr(ref2)
D.2136_50->link.prev

don't alias by virtue of TBAA:

  /* Do type-based disambiguation.  */
  if (base1_alias_set != base2_alias_set
      && !alias_sets_conflict_p (base1_alias_set, base2_alias_set))
    return false;

PTR1 points to apr_bucket_brigade and PTR2 points to apr_bucket.

The aliasing violation is the dereference of APR_RING_SENTINEL which happens in
APR_RING_SPLICE_HEAD and accesses an apr_bucket_brigade as an apr_bucket.

The violation is "sophisticated": the memory location have the same alias set

(gdb) p debug_rtx(mem)
(mem/s/v/f:SI (plus:SI (reg/f:SI 84 [ D.2136 ])
        (const_int 4 [0x4])) [2 D.2136_50->link.prev+0 S4 A32])

(gdb) p debug_rtx(x)
(mem/s/f:SI (plus:SI (reg/f:SI 97)
        (const_int 4 [0x4])) [2 D.2012_2->list.prev+0 S4 A32])

This may explain why the compiler doesn't warn with -Wstrict-aliasing.  It does
warn with the more natural:

#define APR_RING_SENTINEL(hp, elem, link)                \
    (struct elem *)((char *)(hp) - APR_OFFSETOF(struct elem, link))

pr46488.c: In function 'brigade_move':
pr46488.c:107:2: warning: dereferencing type-punned pointer will break
strict-aliasing rules
pr46488.c:107:2: warning: dereferencing type-punned pointer will break
strict-aliasing rules
pr46488.c:107:2: warning: dereferencing type-punned pointer will break
strict-aliasing rules
pr46488.c: In function 'test':
pr46488.c:158:5: warning: dereferencing type-punned pointer will break
strict-aliasing rules
pr46488.c:158:5: warning: dereferencing type-punned pointer will break
strict-aliasing rules
pr46488.c:158:5: warning: dereferencing type-punned pointer will break
strict-aliasing rules
pr46488.c:159:5: warning: dereferencing type-punned pointer will break
strict-aliasing rules
pr46488.c:159:5: warning: dereferencing type-punned pointer will break
strict-aliasing rules
pr46488.c:159:5: warning: dereferencing type-punned pointer will break
strict-aliasing rules

[Bug middle-end/46488] [4.5 regression] server/core_filters.c from apache httpd 2.2.17 miscompiled at -O3

[ebotcazou at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46488

--- Comment #32 from Eric Botcazou <ebotcazou at gcc dot gnu.org> 2010-11-30 15:10:58 UTC ---
The problem appears to be deeply rooted in the Ring construct, more precisely
in the HEAD trick.  IIUC the idea is to "attach" a doubly-linked list to
another structure by means of a "virtual" member overlaid on top of the
structure; the only thing they actually share is a special APR_RING_ENTRY (the
APR_RING_HEAD).  But this overlay fundamentally violates the aliasing rules
even if one try to narrow the accesses to just the shared part.

Richard, is that how the aliasing rules are implemented in the 4.5.x series? 
Has this been changed in 4.6.0?

[Bug middle-end/46488] [4.5 regression] server/core_filters.c from apache httpd 2.2.17 miscompiled at -O3

[rguenther at suse dot de]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46488

--- Comment #33 from rguenther at suse dot de <rguenther at suse dot de> 2010-11-30 15:41:36 UTC ---
On Tue, 30 Nov 2010, ebotcazou at gcc dot gnu.org wrote:

> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46488
> 
> --- Comment #32 from Eric Botcazou <ebotcazou at gcc dot gnu.org> 2010-11-30 15:10:58 UTC ---
> The problem appears to be deeply rooted in the Ring construct, more precisely
> in the HEAD trick.  IIUC the idea is to "attach" a doubly-linked list to
> another structure by means of a "virtual" member overlaid on top of the
> structure; the only thing they actually share is a special APR_RING_ENTRY (the
> APR_RING_HEAD).  But this overlay fundamentally violates the aliasing rules
> even if one try to narrow the accesses to just the shared part.
> 
> Richard, is that how the aliasing rules are implemented in the 4.5.x series? 

Yes, also in 4.4 and 4.3, but maybe you need to be more lucky to trigger
the problem there.

> Has this been changed in 4.6.0?

No.  But with 4.6 we can ignore pointer types when doing copy-propagation
and thus we probably see that they must-alias (in which case we will
not apply TBAA to be more nice to our users).

Richard.

[Bug middle-end/46488] [4.5 regression] server/core_filters.c from apache httpd 2.2.17 miscompiled at -O3

[pcpa at mandriva dot com.br]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46488

Paulo César Pereira de Andrade <pcpa at mandriva dot com.br> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |pcpa at mandriva dot com.br

--- Comment #34 from Paulo César Pereira de Andrade <pcpa at mandriva dot com.br> 2010-11-30 23:15:16 UTC ---
(In reply to comment #32)
> The problem appears to be deeply rooted in the Ring construct, more precisely
> in the HEAD trick.  IIUC the idea is to "attach" a doubly-linked list to
> another structure by means of a "virtual" member overlaid on top of the
> structure; the only thing they actually share is a special APR_RING_ENTRY (the
> APR_RING_HEAD).  But this overlay fundamentally violates the aliasing rules
> even if one try to narrow the accesses to just the shared part.
> 
> Richard, is that how the aliasing rules are implemented in the 4.5.x series? 
> Has this been changed in 4.6.0?

I said in my comment that adding temporaries could correct the issue,
but they do not. I had some different test trees, and reviewing them,
what actually "corrected" in one of the builds was adding a volatile
modifier to APR_RING_HEAD, as in:

#define APR_RING_HEAD(head, elem)                    \
    struct head {                            \
    struct elem * volatile next;                    \
    struct elem * volatile prev;                    \
    }

to better match APR_RING_ENTRY.

[Bug middle-end/46488] [4.5 regression] server/core_filters.c from apache httpd 2.2.17 miscompiled at -O3

[ebotcazou at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46488

--- Comment #35 from Eric Botcazou <ebotcazou at gcc dot gnu.org> 2010-11-30 23:25:57 UTC ---
> I said in my comment that adding temporaries could correct the issue,
> but they do not. I had some different test trees, and reviewing them,
> what actually "corrected" in one of the builds was adding a volatile
> modifier to APR_RING_HEAD, as in:
> 
> #define APR_RING_HEAD(head, elem)                    \
>     struct head {                            \
>     struct elem * volatile next;                    \
>     struct elem * volatile prev;                    \
>     }
> 
> to better match APR_RING_ENTRY.

Probably a reasonable workaround.  Having 2 different constructs APR_RING_ENTRY
and APR_RING_HEAD for the same object is strange in any case.

[Bug middle-end/46488] [4.5 regression] server/core_filters.c from apache httpd 2.2.17 miscompiled at -O3

[rguenther at suse dot de]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46488

--- Comment #36 from rguenther at suse dot de <rguenther at suse dot de> 2010-12-01 10:12:21 UTC ---
On Tue, 30 Nov 2010, ebotcazou at gcc dot gnu.org wrote:

> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46488
> 
> --- Comment #35 from Eric Botcazou <ebotcazou at gcc dot gnu.org> 2010-11-30 23:25:57 UTC ---
> > I said in my comment that adding temporaries could correct the issue,
> > but they do not. I had some different test trees, and reviewing them,
> > what actually "corrected" in one of the builds was adding a volatile
> > modifier to APR_RING_HEAD, as in:
> > 
> > #define APR_RING_HEAD(head, elem)                    \
> >     struct head {                            \
> >     struct elem * volatile next;                    \
> >     struct elem * volatile prev;                    \
> >     }
> > 
> > to better match APR_RING_ENTRY.
> 
> Probably a reasonable workaround.  Having 2 different constructs APR_RING_ENTRY
> and APR_RING_HEAD for the same object is strange in any case.

If you'd re-use the same type the issue would go away as well (I think).

Richard.

[Bug middle-end/46488] [4.5 regression] server/core_filters.c from apache httpd 2.2.17 miscompiled at -O3

[ebotcazou at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46488

Eric Botcazou <ebotcazou at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|                            |INVALID

--- Comment #37 from Eric Botcazou <ebotcazou at gcc dot gnu.org> 2010-12-01 12:15:02 UTC ---
> If you'd re-use the same type the issue would go away as well (I think).

Yes, this works for the testcase, but probably only because the problematic
fields are now volatile too (APR_RING_ENTRY has volatile fields, APR_RING_HEAD
doesn't).  The warning is still emitted for the more natural:

#define APR_RING_SENTINEL(hp, elem, link)                \
    (struct elem *)((char *)(hp) - APR_OFFSETOF(struct elem, link))

with -Wstrict-aliasing so the underlying aliasing issue is probably still
there.

Closing as "invalid" since the original SPARC issues were fixed elsewhere and
the -O3 issue is an aliasing violation in the source code.

[Bug middle-end/46488] [4.5 regression] server/core_filters.c from apache httpd 2.2.17 miscompiled at -O3

[pcpa at mandriva dot com.br]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46488

--- Comment #38 from Paulo César Pereira de Andrade <pcpa at mandriva dot com.br> 2010-12-01 18:23:36 UTC ---
(In reply to comment #37)
> > If you'd re-use the same type the issue would go away as well (I think).
> 
> Yes, this works for the testcase, but probably only because the problematic
> fields are now volatile too (APR_RING_ENTRY has volatile fields, APR_RING_HEAD
> doesn't).  The warning is still emitted for the more natural:
> 
> #define APR_RING_SENTINEL(hp, elem, link)                \
>     (struct elem *)((char *)(hp) - APR_OFFSETOF(struct elem, link))
> 
> with -Wstrict-aliasing so the underlying aliasing issue is probably still
> there.
> 
> Closing as "invalid" since the original SPARC issues were fixed elsewhere and
> the -O3 issue is an aliasing violation in the source code.

  Maybe this should this be considered a problem in the sense that it
does not warn with -Wstrict-aliasing ? As in this case, httpd source
was patched to "correct" the warning, but the problem remained.
  I mean, otherwise it is a kind of "please use -fno-strict-aliasing"
for everything you do not want to risk having incorrect code generation.

[Bug middle-end/46488] [4.5 regression] server/core_filters.c from apache httpd 2.2.17 miscompiled at -O3

[pcpa at mandriva dot com.br]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46488

--- Comment #39 from Paulo César Pereira de Andrade <pcpa at mandriva dot com.br> 2010-12-01 18:25:50 UTC ---
(In reply to comment #38)
> (In reply to comment #37)
> > > If you'd re-use the same type the issue would go away as well (I think).
> > 
> > Yes, this works for the testcase, but probably only because the problematic
> > fields are now volatile too (APR_RING_ENTRY has volatile fields, APR_RING_HEAD
> > doesn't).  The warning is still emitted for the more natural:
> > 
> > #define APR_RING_SENTINEL(hp, elem, link)                \
> >     (struct elem *)((char *)(hp) - APR_OFFSETOF(struct elem, link))
> > 
> > with -Wstrict-aliasing so the underlying aliasing issue is probably still
> > there.
> > 
> > Closing as "invalid" since the original SPARC issues were fixed elsewhere and
> > the -O3 issue is an aliasing violation in the source code.
> 
>   Maybe this should this be considered a problem in the sense that it
> does not warn with -Wstrict-aliasing ? As in this case, httpd source
> was patched to "correct" the warning, but the problem remained.
>   I mean, otherwise it is a kind of "please use -fno-strict-aliasing"
> for everything you do not want to risk having incorrect code generation.

Damn, sorry for quickly replying to myself. It warns with both
-Wstrict-aliasing=1 and -Wstrict-aliasing=2

[Bug middle-end/46488] [4.5 regression] server/core_filters.c from apache httpd 2.2.17 miscompiled at -O3

[ebotcazou at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46488

--- Comment #40 from Eric Botcazou <ebotcazou at gcc dot gnu.org> 2010-12-01 18:55:28 UTC ---
>   Maybe this should this be considered a problem in the sense that it
> does not warn with -Wstrict-aliasing ? As in this case, httpd source
> was patched to "correct" the warning, but the problem remained.

The level 3 warning isn't guaranteed to catch all the cases, especially when
one explicitly obfuscates the issue. :-)  But, yes, worth taking a look.

[Bug middle-end/46488] [4.5 regression] server/core_filters.c from apache httpd 2.2.17 miscompiled at -O3

[ebotcazou at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46488

--- Comment #41 from Eric Botcazou <ebotcazou at gcc dot gnu.org> 2010-12-01 21:53:23 UTC ---
The warning machinery is faced with:

  (struct apr_bucket *) &a->list.next

It doesn't warn because, although &a->list has a different type than struct
apr_bucket, its field 'next' has the same type as the 'next' sub-field of the
'link' field of struct apr_bucket.  Too much sophistication for the machinery.
One would probably need to mimic the disambiguation rules of tree-ssa-alias.c.

Richard, any idea?

[Bug middle-end/46488] [4.5 regression] server/core_filters.c from apache httpd 2.2.17 miscompiled at -O3

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46488

--- Comment #42 from Richard Guenther <rguenth at gcc dot gnu.org> 2010-12-01 22:14:23 UTC ---
(In reply to comment #41)
> The warning machinery is faced with:
> 
>   (struct apr_bucket *) &a->list.next
> 
> It doesn't warn because, although &a->list has a different type than struct
> apr_bucket, its field 'next' has the same type as the 'next' sub-field of the
> 'link' field of struct apr_bucket.  Too much sophistication for the machinery.
> One would probably need to mimic the disambiguation rules of tree-ssa-alias.c.
> 
> Richard, any idea?

The frontend machinery uses get_alias_set on types but it should use it
on the references (but it also warns at places that are not memory references
but just pointer casts, which of course will result in false positives and
us not being able to do real analysis).

I am repeatedly about to remove all but the memory-reference cases from
the frontend warning code as with the many false positives people only
learn how to circumvent it via casts which is never a real fix.

If you want to experiment try using get_alias_set on the reference
or get_deref_alias_set on the ADDR_EXPRs.

The warn_strict_aliasing == 2 && !alias_sets_must_conflict_p check doesn't
make sense at all btw. and the alias_sets_conflict_p check should really
be set1 != set2 && !alias_set_subset_of (set2, set1).

The interface of strict_aliasing_warning is also not good.  Callers seem
to check the warning level dependent on if they are in a dereference or
just a casting context, instead they should pass a flag.  And really
the frontend should only warn about the dereference case.

We can do a warning in the middle-end whenever we disambiguate using
TBAA and could not do with only points-to info.   We could restrict this
to type-punned pointers if we can mark them somehow.  But again this
would result in very many false positives.  Warning about TBAA
violations is hard - you have to be better at alias-analysis than
the optimizer (which otherwise would see the alias and not optimize).