[Bug c++/111028] New: Incorrect optimization with -o1,-o2

[Bug c++/111028] New: Incorrect optimization with -o1,-o2

[zhaiqiming at baidu dot com]

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

            Bug ID: 111028
           Summary: Incorrect optimization with -o1,-o2
           Product: gcc
           Version: 12.1.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: zhaiqiming at baidu dot com
  Target Milestone: ---

======

[summary]

When I upgraded from gcc82 to gcc12, I found that list_t::del execution did not
meet expectations. 
Looking at .s , i noticed that the assembly statements corresponding to the
following two lines have disappeared.

(node.*inner_list_node).next->prev = (node.*inner_list_node).prev;
(node.*inner_list_node).prev->next = (node.*inner_list_node).next;


======

[environment-gcc12]
Reading specs from
/home/opt/compiler/gcc-12/bin/../lib/gcc/x86_64-pc-linux-gnu/12.1.0/specs
COLLECT_GCC=/home/opt/compiler/gcc-12/bin/gcc
COLLECT_LTO_WRAPPER=/home/opt/compiler/gcc-12/bin/../libexec/gcc/x86_64-pc-linux-gnu/12.1.0/lto-wrapper
Target: x86_64-pc-linux-gnu
Configured with: ../configure --prefix=/opt/compiler/gcc-12
--with-local-prefix=/opt/compiler/gcc-12
--with-native-system-header-dir=/opt/compiler/gcc-12/include
--enable-languages=c,c++ --disable-libstdcxx-pch --disable-multilib
--with-default-libstdcxx-abi=gcc4-compatible --disable-bootstrap
--with-sysroot=/ --build=x86_64-pc-linux-gnu --target=x86_64-pc-linux-gnu
--enable-gold --enable-lto
Thread model: posix
Supported LTO compression algorithms: zlib
gcc version 12.1.0 (GCC) 

[environment-gcc82]
Using built-in specs.
COLLECT_GCC=/home/opt/compiler/gcc-8.2/gcc-8.2/bin/gcc
COLLECT_LTO_WRAPPER=/home/opt/compiler/gcc-8.2/gcc-8.2/bin/../libexec/gcc/x86_64-pc-linux-gnu/8.2.0/lto-wrapper
Target: x86_64-pc-linux-gnu
Configured with: ../configure --prefix=/opt/compiler/gcc-8.2
--with-local-prefix=/opt/compiler/gcc-8.2
--with-native-system-header-dir=/opt/compiler/gcc-8.2/include
--enable-languages=c,c++ --disable-libstdcxx-pch --disable-multilib
--disable-bootstrap --with-default-libstdcxx-abi=gcc4-compatible
Thread model: posix
gcc version 8.2.0 (GCC) 

======

[source code]

#include <stdlib.h>
#include <vector>
#include <iostream>
#include <pthread.h>

struct list_node_t
{
        list_node_t *next;
        list_node_t *prev;
};

template <typename T, list_node_t T::*inner_list_node>
class list_t
{
        public:
                list_t() { _head.next = _head.prev = &_head; }
                bool is_empty() const { return _head.next == &_head; }
                T* entry(list_node_t &node) const { return &node == &_head ?
NULL : (T*)((char*)&node - (char*)_node_offset); }

                void add(T &node)
                {
                        _head.next->prev = &(node.*inner_list_node);
                        (node.*inner_list_node).next = _head.next;
                        (node.*inner_list_node).prev = &_head;
                        _head.next = &(node.*inner_list_node);
                }


                static void del(T &node)
                {
            int slot_1 = 10000;
            printf("slot_1 %d\n", slot_1);
                        (node.*inner_list_node).next->prev =
(node.*inner_list_node).prev;
                        (node.*inner_list_node).prev->next =
(node.*inner_list_node).next;
                        int slot_2 = 20000;
            printf("slot_2 %d\n", slot_2);
                }

        protected:
                static list_node_t const * const _node_offset;
                list_node_t _head;
};
template <typename T, list_node_t T::*inner_list_node>
list_node_t const * const list_t<T, inner_list_node>::_node_offset = &(((T
*)0)->*inner_list_node);

template <typename T, list_node_t T::*inner_list_node>
class safe_list_t: public list_t<T, inner_list_node>
{
        public:
                safe_list_t(): _alive(1),_num(0)
        {
                pthread_mutex_init(&_mutex, NULL);
                pthread_cond_init(&_cond, NULL);
        }

                ~safe_list_t()
                {
                        pthread_cond_destroy(&_cond);
                        pthread_mutex_destroy(&_mutex);
                }

                int len()
                {
                        return _num;
                }

                void put(T &node)
                {
                        pthread_mutex_lock(&_mutex);
                        if (_alive)
                        {
                this->add(node);
                                ++_num;
                        }
                        pthread_mutex_unlock(&_mutex);
                        pthread_cond_signal(&_cond);
                }

                T* get()
                {
                        T *ret;
                        pthread_mutex_lock(&_mutex);
                        while (_alive && list_t<T,
inner_list_node>::is_empty())
                                pthread_cond_wait(&_cond, &_mutex);
                        if (_alive)
                        {
                ret = this->entry(*list_t<T, inner_list_node>::_head.prev);
                this->del(*ret);
                                --_num;
                        }
                        else
                        {
                                ret = NULL;
                        }
                        pthread_mutex_unlock(&_mutex);
                        return ret;
                }

        protected:
                pthread_mutex_t _mutex;
                pthread_cond_t _cond;
                int _alive;
                int _num;
};

template <class T>
class worker_t
{
public:
    T *t;
    list_node_t inner_task_list_node;

public:
    worker_t():t(NULL)
    {
        t = new T;
    }
    worker_t(T *tt):t(tt)
    {
    }

    ~worker_t()
    {
        delete t;
    }

private:
    worker_t(const worker_t& rhs);
    worker_t& operator=(const worker_t& rhs);
};

int main() {
    safe_list_t<worker_t<size_t>, &worker_t<size_t>::inner_task_list_node>  
_serialize_merge_scan_move_list;

    auto * worker_0 = new worker_t<size_t>;
    *worker_0->t = 100;
    _serialize_merge_scan_move_list.put(*worker_0);

    auto * worker_1 = new worker_t<size_t>;
    *worker_1->t = 200;
    _serialize_merge_scan_move_list.put(*worker_1);

    auto* temp_0 = _serialize_merge_scan_move_list.get();
    printf("temp_0->t = %zu\n", *temp_0->t);
    delete temp_0;


    auto* temp_1 = _serialize_merge_scan_move_list.get();
    printf("temp_1->t = %zu\n", *temp_1->t);
    delete temp_1;
}


=====

[.s]

[important parts - gcc82]
Shell command: /home/opt/compiler/gcc-8.2/gcc-8.2/bin/gcc  -S -ohello82-use.s  
-save-temps -std=c++11 -O2 -pipe  -Wall -Wextra  -W  -fPIC
-Werror=return-local-addr -Werror=return-type test_main.cpp -lstdc++  -lpthread
-lssl -lcrypto -lrt -lz -ldl


.L8:
        .cfi_restore_state
        movq    8(%rbx), %r12
        cmpq    %rbx, %r12
        je      .L15
        movq   
_ZN6list_tI8worker_tImEXadL_ZNS1_20inner_task_list_nodeEEEE12_node_offsetE@GOTPCREL(%rip),
%rax
        movl    $10000, %esi
        leaq    .LC0(%rip), %rdi
        subq    (%rax), %r12
        xorl    %eax, %eax
        call    printf@PLT
        movq    8(%r12), %rdx
        movq    16(%r12), %rax
        movl    $20000, %esi
        leaq    .LC1(%rip), %rdi
        movq    %rax, 8(%rdx)
        movq    %rdx, (%rax)
        xorl    %eax, %eax
        call    printf@PLT
        subl    $1, 108(%rbx)
        movq    %rbp, %rdi
        call    pthread_mutex_unlock@PLT
        movq    %r12, %rax
        popq    %rbx
        .cfi_remember_state
        .cfi_def_cfa_offset 24
        popq    %rbp
        .cfi_def_cfa_offset 16
        popq    %r12
        .cfi_def_cfa_offset 8
        ret


[important parts - gcc12]
Shell command: /home/opt/compiler/gcc-12/bin/gcc  -S -ohello12-use.s  
-save-temps -std=c++11 -O2 -pipe  -Wall -Wextra  -W  -fPIC
-Werror=return-local-addr -Werror=return-type test_main.cpp -lstdc++  -lpthread
-lssl -lcrypto -lrt -lz -ldl


.L11:
        movl    $10000, %esi
        leaq    .LC0(%rip), %rdi
        xorl    %eax, %eax
        call    printf@PLT
        movl    $20000, %esi
        leaq    .LC1(%rip), %rdi
        xorl    %eax, %eax
        call    printf@PLT
        subl    $1, 108(%rbx)
        movq    %rbp, %rdi
        call    pthread_mutex_unlock@PLT
        movq    %r12, %rax
        popq    %rbx
        .cfi_remember_state
        .cfi_def_cfa_offset 24
        popq    %rbp
        .cfi_def_cfa_offset 16
        popq    %r12
        .cfi_def_cfa_offset 8
        ret
        .p2align 4,,10
        .p2align 3

[Bug c++/111028] Incorrect optimization with -o1,-o2

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |wrong-code
      Known to work|                            |9.5.0
      Known to fail|                            |10.5.0, 12.3.0, 13.2.0

--- Comment #1 from Richard Biener <rguenth at gcc dot gnu.org> ---
I can confirm the finding, not sure if the program is valid though.  Neither
-fno-strict-aliasing nor -fno-lifetime-dse has any effect.

[Bug c++/111028] Incorrect optimization with -o1,-o2

[ppalka at gcc dot gnu.org]

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

Patrick Palka <ppalka at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |ppalka at gcc dot gnu.org
           Keywords|needs-bisection             |

--- Comment #2 from Patrick Palka <ppalka at gcc dot gnu.org> ---
Seems to have started with r10-515-g810c42c38d3731 / r271414.

The line

                T* entry(list_node_t &node) const { return &node == &_head ?
NULL : (T*)((char*)&node - (char*)_node_offset); }

seems suspiciously similar to PR96993 which was deemed invalid due to treating
the difference of two pointers as a valid pointer.

[Bug c++/111028] Incorrect optimization with -o1,-o2

[ppalka at gcc dot gnu.org]

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

--- Comment #3 from Patrick Palka <ppalka at gcc dot gnu.org> ---
Converting _node_offset from a pointer into an integer offset seems to fix the
testcase:

diff --git a/111028.C b/111028.C
index ed4106e..ef2b1be 100644
--- a/111028.C
+++ b/111028.C
@@ -15,7 +15,7 @@ class list_t
        public:
                list_t() { _head.next = _head.prev = &_head; }
                bool is_empty() const { return _head.next == &_head; }
-               T* entry(list_node_t &node) const { return &node == &_head ?
NULL : (T*)((char*)&node - (char*)_node_offset); }
+               T* entry(list_node_t &node) const { return &node == &_head ?
NULL : (T*)((char*)&node - _node_offset); }

                void add(T &node)
                {
@@ -37,11 +37,11 @@ class list_t
                }

        protected:
-               static list_node_t const * const _node_offset;
+               static size_t const _node_offset;
                list_node_t _head;
 };
 template <typename T, list_node_t T::*inner_list_node>
-list_node_t const * const list_t<T, inner_list_node>::_node_offset = &(((T
*)0)->*inner_list_node);
+size_t const list_t<T, inner_list_node>::_node_offset = size_t(&(((T
*)0)->*inner_list_node));

 template <typename T, list_node_t T::*inner_list_node>
 class safe_list_t: public list_t<T, inner_list_node>

[Bug c++/111028] Incorrect optimization with -o1,-o2

[ppalka at gcc dot gnu.org]

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

--- Comment #4 from Patrick Palka <ppalka at gcc dot gnu.org> ---
Or simply just:

diff --git a/111028.C b/111028.C
index ed4106e..85f1c2a 100644
--- a/111028.C
+++ b/111028.C
@@ -15,7 +15,7 @@ class list_t
        public:
                list_t() { _head.next = _head.prev = &_head; }
                bool is_empty() const { return _head.next == &_head; }
-               T* entry(list_node_t &node) const { return &node == &_head ?
NULL : (T*)((char*)&node - (char*)_node_offset); }
+               T* entry(list_node_t &node) const { return &node == &_head ?
NULL : (T*)((char*)&node - (size_t)_node_offset); }

                void add(T &node)
                {

[Bug c++/111028] Incorrect optimization with -o1,-o2

[zhaiqiming at baidu dot com]

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

--- Comment #5 from zhaiqiming at baidu dot com ---
(In reply to Patrick Palka from comment #4)

thanks for your plan. Because my project need -o2 optimization, i use "#pragma
GCC optimize("-O0")" to temporarily solve the problem with this function. hope
this case can make gcc-optimization better at a higher version.

[Bug c++/111028] Incorrect optimization with -o1,-o2

[rguenth at gcc dot gnu.org]

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

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

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

--- Comment #6 from Richard Biener <rguenth at gcc dot gnu.org> ---
Yeah, if you subtract two pointers like

   (char*)&node - (char*)_node_offset

then both have to point to the same object (otherwise the behavior is
undefined) and you get a difference in the number of elements.
That difference isn't a pointer.  The correct fix is as suggested, this isn't
a bug in GCC.