[Bug c++/96717] New: -flifetime-dse=2 breaks webkit-gtk-2.28.4

[Bug c++/96717] New: -flifetime-dse=2 breaks webkit-gtk-2.28.4

[slyfox at gcc dot gnu.org]

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

            Bug ID: 96717
           Summary: -flifetime-dse=2 breaks webkit-gtk-2.28.4
           Product: gcc
           Version: 11.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: slyfox at gcc dot gnu.org
  Target Milestone: ---

Created attachment 49084
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=49084&action=edit
part-of-webkit-gtk-2.28.4.tar.gz

Initially I noticed the bug on liferea which uses webkit-gtk-2.28.4 as a
rendering engine.

I was not able to extract minimal example because I got lost in SFINATE
instance selections. I am not sure if it's a webkit bug or gcc bug.

I'm leaning to gcc bug. gcc-10 used to work. gcc-11 generates code that hits
'ud2'.

The webkit client code looks simple: we create a hash set of raw pointers and
add pointers there to see how hashset resizes:

"""
#define USE_SYSTEM_MALLOC 1 /* avoid bmalloc */

#include "wtf/HashSet.h"
#include "wtf/Threading.h"

using namespace WTF;

int main() {
    HashSet<Thread*> hst;
    for (int i = 1; i < 1000; ++i) {
        Thread * v = (Thread*)(long)(i * 128);
        hst.add(v);
    }
}
"""

But headers take almost 2 megabytes.

Building the example with -fno-lifetime-dse produces working code. Building
without breaks on first hash table resize:

$ ./mk.bash

./a
./mk.bash: line 50: 1054935 Illegal instruction     (core dumped) ./a
132

./a-nodse
0

The 'ud2' is encountered in HashTable::rehash() method:
https://github.com/WebKit/webkit/blob/master/Source/WTF/wtf/HashTable.h#L1304

Attaching self-contained directory with sources and headers.

Can you help me rule out gcc's misbehaviour?

[Bug c++/96717] -flifetime-dse=2 breaks webkit-gtk-2.28.4

[redi at gcc dot gnu.org]

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

--- Comment #1 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Created attachment 49085
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=49085&action=edit
Preprocessed source for a.cc

[Bug c++/96717] -flifetime-dse=2 breaks webkit-gtk-2.28.4

[redi at gcc dot gnu.org]

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

--- Comment #2 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to Jonathan Wakely from comment #1)
> Created attachment 49085 [details]
> Preprocessed source for a.cc

This was prepared on x86_64-pc-linux-gnu.

Compile it with -O2 to see the ud2 insn in the .s output.

[Bug c++/96717] -flifetime-dse=2 breaks webkit-gtk-2.28.4

[jakub at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
     Ever confirmed|0                           |1
   Last reconfirmed|                            |2020-08-20
                 CC|                            |jakub at gcc dot gnu.org
             Status|UNCONFIRMED                 |NEW

--- Comment #3 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
The __builtin_trap is there because of:
            if (isEmptyBucket(oldEntry)) {
                do { if (!(std::addressof(oldEntry) != entry)) {
WTFReportAssertionFailure("WTF/wtf/HashTable.h", 1337, __PRETTY_FUNCTION__,
"std::
                oldTable[i].~ValueType();
                continue;
            }
where the type of oldTable[i] and oldEntry is struct Thread *, isEmptyBucket is
== NULL check and invoking a pseudo-destructor.

Playing with this, I've come up with:
typedef int *T;

void
foo (T a)
{
  if (a)
    return;
  a.~T ();
}

int
main ()
{
  int p;
  foo (&p);
  foo (nullptr);
  return 0;
}
testcase.

Starting with r11-2238-ge443d8213864ac337c29092d4767224f280d2062 we emit:
  if (a != 0B) goto <D.2343>; else goto <D.2344>;
  <D.2343>:
  // predicted unlikely by early return (on trees) predictor.
  return;
  <D.2344>:
  *a = {CLOBBER};
which doesn't look right to me, that is as if the (pseudo)destructor is run on
what the pointer points to (so if the testcase is changed to also have typedef
int V; and use a->~V (); rather than a.~T ();).
That is one thing, but I'd say in that case it ought to crash only if a is
NULL, because the pseudo destructor is invoked only in that case.
With that we have:
  if (a_2(D) != 0B)
    goto <bb 3>; [INV]
  else
    goto <bb 4>; [INV]

  <bb 3> :
  // predicted unlikely by early return (on trees) predictor.
  goto <bb 5>; [INV]

  <bb 4> :
  MEM[(int *)0B] ={v} {CLOBBER};

  <bb 5> :
  return;
before cddce1, but we then "optimize" it into:
  <bb 2> :
  MEM[(int *)0B] ={v} {CLOBBER};
  return;

So, I'd say we have two bugs here, one is that the FE since r1-2238 mishandles
pseudo-destructors of pointer types, for that use the above testcase, and then
that we mishandle clobbers in DCE or so, for which the testcase is:
struct S { int s; ~S () {} };

void
foo (S *a)
{
  if (a)
    return;
  a->~S ();
}

int
main ()
{
  S s;
  foo (&s);
  foo ((S *) 0);
}
which started to be really miscompiled with r249450, but had the questionable
unconditional
  MEM[(struct S *)0B] ={v} {CLOBBER};
as first stmt in main already since r197375.

So, in addition to fixing the C++ FE, I think we either shall declare that
clobbers never trap even if invoked with NULL pointers, then cddce can do the
optimization it does, but we shouldn't turn it into __builtin_trap later, or we
disable the optimization that happens during dce if the pointer in MEM_REF lhs
of a clobber is or might be NULL.

Now, whether fixing this (both or just one) fixes the above reported bug is
unclear.

[Bug c++/96717] -flifetime-dse=2 breaks webkit-gtk-2.28.4

[jakub at gcc dot gnu.org]

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

--- Comment #4 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
I've moved the #c3 issues into PR96721 and PR96722 as they are separate.  And
either this bug turns a dup of the former, or not.

[Bug c++/96717] -flifetime-dse=2 breaks webkit-gtk-2.28.4

[redi at gcc dot gnu.org]

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

--- Comment #5 from Jonathan Wakely <redi at gcc dot gnu.org> ---
#include <vector>

void pop_many(std::vector<int>& v, unsigned n) {
    for (unsigned i = 0; i < n; ++i) {
        v.pop_back();
    }
}

With GCC 10 at -O2 and above this was optimized to simply subtract n from the
v.end() pointer:

_Z8pop_manyRSt6vectorIiSaIiEEm:
.LFB883:
        testq   %rsi, %rsi
        je      .L1
        salq    $2, %rsi
        subq    %rsi, 8(%rdi)
.L1:
        ret


But since r11-2238 it decrements the pointer n times:

_Z8pop_manyRSt6vectorIiSaIiEEm:
.LFB883:
        testq   %rsi, %rsi
        je      .L1
        movq    8(%rdi), %rdx
        xorl    %eax, %eax
        .p2align 4,,10
        .p2align 3
.L4:
        subq    $4, %rdx
        addq    $1, %rax
        movq    %rdx, 8(%rdi)
        cmpq    %rax, %rsi
        jne     .L4
.L1:
        ret


This regression is still present after the fixes for PR96721 and PR96722.
Should it be marked as [11/12 Regression] ?

[Bug c++/96717] -flifetime-dse=2 breaks webkit-gtk-2.28.4

[redi at gcc dot gnu.org]

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

--- Comment #6 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Reduced:

template<typename T>
struct vector
{
  using pointer = T*;

  pointer begin, end, end_of_storage;

  void pop_back()
  {
    (end--)->~T();
  }
};

using size_t = decltype(sizeof(0));

void pop_many(vector<int>& v, size_t n) {
    for (size_t i = 0; i < n; ++i) {
        v.pop_back();
    }
}


With -O2 -fno-lifetime-dse we get the expected code back.

[Bug c++/96717] -flifetime-dse=2 breaks webkit-gtk-2.28.4

[pinskia at gcc dot gnu.org]

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

--- Comment #7 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to Jonathan Wakely from comment #6)
> 
> With -O2 -fno-lifetime-dse we get the expected code back.

I just saw another bug report filed about code quality with lifetime dse
enabled in the last week.

[Bug c++/96717] -flifetime-dse=2 breaks webkit-gtk-2.28.4

[jakub at gcc dot gnu.org]

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

--- Comment #8 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
PR104515 ?

[Bug c++/96717] -flifetime-dse=2 breaks webkit-gtk-2.28.4

[redi at gcc dot gnu.org]

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

Jonathan Wakely <redi at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           See Also|                            |https://gcc.gnu.org/bugzill
                   |                            |a/show_bug.cgi?id=104515

--- Comment #9 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Yes indeed, somebody on reddit just pointed me to that. It's the same as
comment 6.