[Bug middle-end/60546] New: [4.8 and 4.9] O2 & asan enable generates wrong insns

[Bug middle-end/60546] New: [4.8 and 4.9] O2 & asan enable generates wrong insns

[manjian2006 at gmail dot com]

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

            Bug ID: 60546
           Summary: [4.8 and 4.9] O2 & asan enable generates wrong insns
           Product: gcc
           Version: 4.9.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: middle-end
          Assignee: unassigned at gcc dot gnu.org
          Reporter: manjian2006 at gmail dot com

Created attachment 32366
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=32366&action=edit
source that causes bug

My attachment gives a testcase to this bug.
To compile,modify the first argument of 1.sh to the path of your GCC (>=
4.8.1).
Then run 1.sh to compile.
As you can see the differences between line #1 and #2 is just -O2 or -Os.
And ./1 reports an error,but ./2 don't.
So I am sure currently asan with -O2 will generates wrong insn in middle end.I
have not located where goes wrong, please help.

[Bug middle-end/60546] [4.8 and 4.9] O2 & asan enable generates wrong insns

[pinskia at gcc dot gnu.org]

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

--- Comment #1 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
-fno-strict-aliasing fixes the issue So there might be an aliasing issue in the
code.

[Bug middle-end/60546] [4.8 and 4.9] O2 & asan enable generates wrong insns

[pinskia at gcc dot gnu.org]

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

--- Comment #2 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
But it might be related to bug 60429.

[Bug middle-end/60546] [4.8/4.9] O2 & asan enable generates wrong insns

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |rguenth at gcc dot gnu.org
            Summary|[4.8 and 4.9] O2 & asan     |[4.8/4.9] O2 & asan enable
                   |enable generates wrong      |generates wrong insns
                   |insns                       |

--- Comment #3 from Richard Biener <rguenth at gcc dot gnu.org> ---
It's not fixed by the fix for PR60429.

But

1.cpp: In member function ‘unsigned int WTF::StringImpl::hashSlowCase() const’:
1.cpp:26260:1: warning: no return statement in function returning non-void
[-Wreturn-type]
 }
 ^

so at least there's sth fishy going on (fixing it with an obvious change
doesn't fix the abort).  Note that valgrind reports the error without
-fsanitize=address, for -O2 but not for -Os.

-fno-tree-loop-im fixes this so it might be a duplicate of PR??? as
loop invariant motion can cause the read of uninitialized memory when
applying store-motion.
>From gcc-bugs-return-446518-listarch-gcc-bugs=gcc.gnu.org@gcc.gnu.org Mon Mar 17 09:43:42 2014
Return-Path: <gcc-bugs-return-446518-listarch-gcc-bugs=gcc.gnu.org@gcc.gnu.org>
Delivered-To: listarch-gcc-bugs@gcc.gnu.org
Received: (qmail 2867 invoked by alias); 17 Mar 2014 09:43:41 -0000
Mailing-List: contact gcc-bugs-help@gcc.gnu.org; run by ezmlm
Precedence: bulk
List-Id: <gcc-bugs.gcc.gnu.org>
List-Archive: <http://gcc.gnu.org/ml/gcc-bugs/>
List-Post: <mailto:gcc-bugs@gcc.gnu.org>
List-Help: <mailto:gcc-bugs-help@gcc.gnu.org>
Sender: gcc-bugs-owner@gcc.gnu.org
Delivered-To: mailing list gcc-bugs@gcc.gnu.org
Received: (qmail 2828 invoked by uid 48); 17 Mar 2014 09:43:37 -0000
From: "rguenth at gcc dot gnu.org" <gcc-bugzilla@gcc.gnu.org>
To: gcc-bugs@gcc.gnu.org
Subject: [Bug tree-optimization/60537] Loop header copying code bloat for simple loops that don't benefit
Date: Mon, 17 Mar 2014 09:43:00 -0000
X-Bugzilla-Reason: CC
X-Bugzilla-Type: changed
X-Bugzilla-Watch-Reason: None
X-Bugzilla-Product: gcc
X-Bugzilla-Component: tree-optimization
X-Bugzilla-Version: 4.9.0
X-Bugzilla-Keywords:
X-Bugzilla-Severity: enhancement
X-Bugzilla-Who: rguenth at gcc dot gnu.org
X-Bugzilla-Status: NEW
X-Bugzilla-Priority: P3
X-Bugzilla-Assigned-To: unassigned at gcc dot gnu.org
X-Bugzilla-Target-Milestone: ---
X-Bugzilla-Flags:
X-Bugzilla-Changed-Fields: bug_status cf_reconfirmed_on short_desc everconfirmed
Message-ID: <bug-60537-4-YsvJRLClVz@http.gcc.gnu.org/bugzilla/>
In-Reply-To: <bug-60537-4@http.gcc.gnu.org/bugzilla/>
References: <bug-60537-4@http.gcc.gnu.org/bugzilla/>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
X-Bugzilla-URL: http://gcc.gnu.org/bugzilla/
Auto-Submitted: auto-generated
MIME-Version: 1.0
X-SW-Source: 2014-03/txt/msg01387.txt.bz2
Content-length: 1161

http://gcc.gnu.org/bugzilla/show_bug.cgi?id`537

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2014-03-17
            Summary|Loop optimization code      |Loop header copying code
                   |bloat for simple loops      |bloat for simple loops that
                   |                            |don't benefit
     Ever confirmed|0                           |1

--- Comment #1 from Richard Biener <rguenth at gcc dot gnu.org> ---
For -O2 we do this to enable loop optimizations which almost all require
do { } while style loops.  This canonicalization can sometimes peel an
entire iteration as you can see here, and this canonicalization is
not done at -Os unless the loop is determined as hot (so with -Os
and profile-feedback some loops may get this treatment).

It's hard to undo this transform but that's what would be needed here ...
(or make more passes deal with number-of-iterations == n or zero)

[Bug middle-end/60546] [4.8/4.9] O2 & asan enable generates wrong insns

[rguenth at gcc dot gnu.org]

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

--- Comment #4 from Richard Biener <rguenth at gcc dot gnu.org> ---
Created attachment 32370
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=32370&action=edit
patch to fix store motion issue

Fixing that doesn't fix it (or my fix doesn't work ;)).

[Bug middle-end/60546] [4.8/4.9] O2 & asan enable generates wrong insns

[manjian2006 at gmail dot com]

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

--- Comment #5 from linzj <manjian2006 at gmail dot com> ---
Well,valgind do detect invalid memory usage.That's not an asan problem then.
Since it effects from 4.8,does that mean 4.8 is not secure any more?

[Bug middle-end/60546] [4.8/4.9] O2 & asan enable generates wrong insns

[rguenth at gcc dot gnu.org]

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

--- Comment #6 from Richard Biener <rguenth at gcc dot gnu.org> ---
Meanwhile -fno-tree-dse also "fixes" this but it only prevents mayhem
downstream
(Jakub bisected this to a revision that exposed the issue, r158047).  You have
to disable both tree DSE passes btw, thus it points to a pass running after
DSE2.  It's not -fhoist-adjacent-loads, not RTL PRE nor RTL loop invariant
motion
or the scheduler.

[Bug middle-end/60546] [4.8/4.9] O2 & asan enable generates wrong insns

[rguenth at gcc dot gnu.org]

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

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

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

--- Comment #7 from Richard Biener <rguenth at gcc dot gnu.org> ---
Looks like TBAA violations to me:

  struct QualifiedNameComponents c;
  short unsigned int _316;
  short unsigned int _317;

  <bb 53>:
  # data_157 = PHI <data_329(53), &c(52)>
  _316 = MEM[base: data_157, offset: 2B];
  _317 = MEM[base: data_157, offset: 0B];
... hashing ...
  data_329 = data_157 + 4;
  if (data_329 != &MEM[(void *)&c + 12B])
    goto <bb 53>;
  else
    goto <bb 54>;

so you hash the QualifiedNameComponents pointers, not its strings.  And
you do it by reading the pointers as 'unsigned short'.  Probably via

    template<size_t length> static inline unsigned hashMemory(const void* data)
    {
        typedef int dummylength_must_be_a_multible_of_four [(!(length % 4)) ? 1
: -1];
        return computeHash<UChar>(static_cast<const UChar*>(data), length /
sizeof(UChar));
    }

which introduces the violation.  It should not use 'short' hashing but
hashing of 'char'.

[Bug middle-end/60546] [4.8/4.9] O2 & asan enable generates wrong insns

[manjian2006 at gmail dot com]

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

--- Comment #8 from linzj <manjian2006 at gmail dot com> ---
I don't think it can be mark as resolved-invalid that fast.This code is used by
WebKit for a long time and no one would say this is an illegal algorithm.

[Bug middle-end/60546] [4.8/4.9] O2 & asan enable generates wrong insns

[manjian2006 at gmail dot com]

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

--- Comment #9 from linzj <manjian2006 at gmail dot com> ---
If this is an illegal expression, it should be reported at compile time,not
generating a wrong code.

[Bug middle-end/60546] [4.8/4.9] O2 & asan enable generates wrong insns

[manjian2006 at gmail dot com]

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

linzj <manjian2006 at gmail dot com> changed:

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

--- Comment #10 from linzj <manjian2006 at gmail dot com> ---
add  __attribute__((noinline)) to computeHash "resolves" this bug.

[Bug middle-end/60546] [4.8/4.9] O2 & asan enable generates wrong insns

[rguenth at gcc dot gnu.org]

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

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

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

--- Comment #11 from Richard Biener <rguenth at gcc dot gnu.org> ---
That doesn't make this bug valid.

[Bug middle-end/60546] [4.8/4.9] O2 & asan enable generates wrong insns

[manjian2006 at gmail dot com]

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

--- Comment #12 from linzj <manjian2006 at gmail dot com> ---
Alright,should I change the algorithm to avoid this bug?

[Bug middle-end/60546] [4.8/4.9] O2 & asan enable generates wrong insns

[rguenther at suse dot de]

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

--- Comment #13 from rguenther at suse dot de <rguenther at suse dot de> ---
On Mon, 17 Mar 2014, manjian2006 at gmail dot com wrote:

> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=60546
> 
> --- Comment #12 from linzj <manjian2006 at gmail dot com> ---
> Alright,should I change the algorithm to avoid this bug?

Of course, what else ...

[Bug middle-end/60546] [4.8/4.9] O2 & asan enable generates wrong insns

[manjian2006 at gmail dot com]

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

--- Comment #14 from linzj <manjian2006 at gmail dot com> ---
Well,but I have not figured out what goes wrong in the hashing algorithm. Would
you point it out.

[Bug middle-end/60546] [4.8/4.9] O2 & asan enable generates wrong insns

[mpolacek at gcc dot gnu.org]

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

Marek Polacek <mpolacek at gcc dot gnu.org> changed:

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

--- Comment #15 from Marek Polacek <mpolacek at gcc dot gnu.org> ---
"It should not use 'short' hashing but hashing of 'char'." doesn't help?

[Bug middle-end/60546] [4.8/4.9] O2 & asan enable generates wrong insns

[manjian2006 at gmail dot com]

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

--- Comment #16 from linzj <manjian2006 at gmail dot com> ---
Yes,that may work.But what exactly go wrong in the original algorithm? I can't
change a correct algorithm just because it volatiles TBBA and make the compiler
generate wrong code.Because it's CORRECT logically.

[Bug middle-end/60546] [4.8/4.9] O2 & asan enable generates wrong insns

[manjian2006 at gmail dot com]

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

--- Comment #18 from linzj <manjian2006 at gmail dot com> ---
(In reply to rguenther@suse.de from comment #17)
> On Mon, 17 Mar 2014, manjian2006 at gmail dot com wrote:
> 
> > http://gcc.gnu.org/bugzilla/show_bug.cgi?id=60546
> > 
> > --- Comment #16 from linzj <manjian2006 at gmail dot com> ---
> > Yes,that may work.But what exactly go wrong in the original algorithm? I can't
> > change a correct algorithm just because it volatiles TBBA and make the compiler
> > generate wrong code.Because it's CORRECT logically.
> 
> You have to fix the memory reads from data[] to not read 'short's but
> to read 'char's.

Just out of curiosity , why this behavior causes tbba failure?And is that a
logical error?I am sure it's not an alignment error.
Interpreting a block of raw data from a temporary stack space as a short array
does not seem like any error to me.

[Bug middle-end/60546] [4.8/4.9] O2 & asan enable generates wrong insns

[pinskia at gcc dot gnu.org]

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

--- Comment #19 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to linzj from comment #18)
> (In reply to rguenther@suse.de from comment #17)
> > On Mon, 17 Mar 2014, manjian2006 at gmail dot com wrote:
> > 
> > > http://gcc.gnu.org/bugzilla/show_bug.cgi?id=60546
> > > 
> > > --- Comment #16 from linzj <manjian2006 at gmail dot com> ---
> > > Yes,that may work.But what exactly go wrong in the original algorithm? I can't
> > > change a correct algorithm just because it volatiles TBBA and make the compiler
> > > generate wrong code.Because it's CORRECT logically.
> > 
> > You have to fix the memory reads from data[] to not read 'short's but
> > to read 'char's.
> 
> Just out of curiosity , why this behavior causes tbba failure?And is that a
> logical error?I am sure it's not an alignment error.
> Interpreting a block of raw data from a temporary stack space as a short
> array does not seem like any error to me.

Except short cannot alias char.  That is the point of type based alias
analysis.

[Bug middle-end/60546] [4.8/4.9] O2 & asan enable generates wrong insns

[manjian2006 at gmail dot com]

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

--- Comment #20 from linzj <manjian2006 at gmail dot com> ---
That's really not satisfying. I am going to dig deeper.Check the expanded RTL.

[Bug middle-end/60546] [4.8/4.9] O2 & asan enable generates wrong insns

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

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

--- Comment #21 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
What is not satisfying?

struct QualifiedNameComponents {
    StringImpl* m_prefix;
    StringImpl* m_localName;
    StringImpl* m_namespace;
};
...
    template<typename T, UChar Converter(T)> static inline unsigned
computeHash(const T* data, unsigned length)
    {
        StringHasher hasher;
        bool rem = length & 1;
        length >>= 1;
        while (length--) {
            hasher.addCharacters(Converter(data[0]), Converter(data[1]));
            data += 2;
        }
        if (rem)
            hasher.addCharacter(Converter(*data));
        return hasher.hash();
    }
...
    template<size_t length> static inline unsigned hashMemory(const void* data)
    {
        typedef int dummylength_must_be_a_multible_of_four [(!(length % 4)) ? 1
: -1];
        return computeHash<UChar>(static_cast<const UChar*>(data), length /
sizeof(UChar));
    }
...
inline unsigned hashComponents(const QualifiedNameComponents& buf)
{
    return StringHasher::hashMemory<sizeof(QualifiedNameComponents)>(&buf);
}
struct QualifiedNameHash {
    static unsigned hash(const QualifiedName& name) { return hash(name.impl());
}
    static unsigned hash(const QualifiedName::QualifiedNameImpl* name)
    {
        QualifiedNameComponents c = { name->m_prefix.impl(),
name->m_localName.impl(), name->m_namespace.impl() };
        return hashComponents(c);
    }
    static bool equal(const QualifiedName& a, const QualifiedName& b) { return
a == b; }
    static bool equal(const QualifiedName::QualifiedNameImpl* a, const
QualifiedName::QualifiedNameImpl* b) { return a == b; }
    static const bool safeToCompareToEmptyOrDeleted = false;
};

is a clear aliasing violation and thus undefined behavior when called.
The `c' object has object type QualifiedNameComponents, whose subobjects have
type pointer to StringImpl.  In computeHash you are then reading the object
using the UChar (unsigned short) effective type, only char or unsigned char
types can be used for that, see e.g. [basic.types]/2, or read -fstrict-aliasing
documentation in info gcc.
For g++, you could read the object through say typedef UChar UCharMayAlias
__attribute__((__may_alias__));, or you can say std::memcpy the object into an
correspondigly sized array of UChar and hash that rather than the original
object, etc.

[Bug middle-end/60546] [4.8/4.9] O2 & asan enable generates wrong insns

[manjian2006 at gmail dot com]

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

--- Comment #22 from linzj <manjian2006 at gmail dot com> ---
(In reply to Jakub Jelinek from comment #21)
> What is not satisfying?
> 
> struct QualifiedNameComponents {
>     StringImpl* m_prefix;
>     StringImpl* m_localName;
>     StringImpl* m_namespace;
> };
> ...
>     template<typename T, UChar Converter(T)> static inline unsigned
> computeHash(const T* data, unsigned length)
>     {
>         StringHasher hasher;
>         bool rem = length & 1;
>         length >>= 1;
>         while (length--) {
>             hasher.addCharacters(Converter(data[0]), Converter(data[1]));
>             data += 2;
>         }
>         if (rem)
>             hasher.addCharacter(Converter(*data));
>         return hasher.hash();
>     }
> ...
>     template<size_t length> static inline unsigned hashMemory(const void*
> data)
>     {
>         typedef int dummylength_must_be_a_multible_of_four [(!(length % 4))
> ? 1 : -1];
>         return computeHash<UChar>(static_cast<const UChar*>(data), length /
> sizeof(UChar));
>     }
> ...
> inline unsigned hashComponents(const QualifiedNameComponents& buf)
> {
>     return StringHasher::hashMemory<sizeof(QualifiedNameComponents)>(&buf);
> }
> struct QualifiedNameHash {
>     static unsigned hash(const QualifiedName& name) { return
> hash(name.impl()); }
>     static unsigned hash(const QualifiedName::QualifiedNameImpl* name)
>     {
>         QualifiedNameComponents c = { name->m_prefix.impl(),
> name->m_localName.impl(), name->m_namespace.impl() };
>         return hashComponents(c);
>     }
>     static bool equal(const QualifiedName& a, const QualifiedName& b) {
> return a == b; }
>     static bool equal(const QualifiedName::QualifiedNameImpl* a, const
> QualifiedName::QualifiedNameImpl* b) { return a == b; }
>     static const bool safeToCompareToEmptyOrDeleted = false;
> };
> 
> is a clear aliasing violation and thus undefined behavior when called.
> The `c' object has object type QualifiedNameComponents, whose subobjects
> have type pointer to StringImpl.  In computeHash you are then reading the
> object using the UChar (unsigned short) effective type, only char or
> unsigned char types can be used for that, see e.g. [basic.types]/2, or read
> -fstrict-aliasing documentation in info gcc.
> For g++, you could read the object through say typedef UChar UCharMayAlias
> __attribute__((__may_alias__));, or you can say std::memcpy the object into
> an correspondigly sized array of UChar and hash that rather than the
> original object, etc.
Thank you for your reply.It helps me a lot.
I have found a virtual register "table_26" coming from hell,no one generates
this register but use as the base of the m_table.Tree representation must have
damages because that violation.

[Bug middle-end/60546] [4.8/4.9] O2 & asan enable generates wrong insns

[cand at gmx dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=60546

Lauri Kasanen <cand at gmx dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |cand at gmx dot com

--- Comment #23 from Lauri Kasanen <cand at gmx dot com> ---
*** Bug 61721 has been marked as a duplicate of this bug. ***