[Bug rtl-optimization/96993] New: invalid code

[Bug rtl-optimization/96993] New: invalid code

[jan.smets at nokia dot com]

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

            Bug ID: 96993
           Summary: invalid code
           Product: gcc
           Version: 10.2.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: rtl-optimization
          Assignee: unassigned at gcc dot gnu.org
          Reporter: jan.smets at nokia dot com
  Target Milestone: ---

Created attachment 49203
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=49203&action=edit
testcase

Following testcase produces invalid code in 10.1, 10.2. Works with 9.3. 

%rdi is sometimes invalid in call to listReplaceEntry()   (is 0x8)
(or if listReplaceEntry() would always return, it would crash on mov   
0x18(%rbx),%rbx right before constructing %rdi for the call (%rbx is zero on
second loop iteration)

Adding simple code changes, like a asm("nop") before if (b_rep), makes it
produce working code.

Configured with: /usr/src/gcc/configure --build=x86_64-linux-gnu
--disable-multilib --enable-languages=c,c++,fortran,go

Compile testcase with -O2 [-fno-omit-frame-pointer]

[Bug rtl-optimization/96993] invalid code

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |wrong-code
             Target|                            |x86_64-*-*

--- Comment #1 from Richard Biener <rguenth at gcc dot gnu.org> ---
did you by any chance look at the assembly and figured what is wrong?

[Bug rtl-optimization/96993] [10/11 Regression] invalid code

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |10.3
            Summary|invalid code                |[10/11 Regression] invalid
                   |                            |code

[Bug rtl-optimization/96993] [10/11 Regression] invalid code

[jakub at gcc dot gnu.org]

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

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

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

--- Comment #2 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Can you turn it into a self-contained testcase (add noipa attributes to the
external functions and define them some way that it detects the problem, add
noipa attribute to the problematic function and add main that invokes it so
that it can reproduce)?
Without that and knowledge what to look for, it is not really possible e.g. to
bisect it and find out what changed and why.

[Bug rtl-optimization/96993] [10/11 Regression] invalid code

[jan.smets at nokia dot com]

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

--- Comment #3 from Jan Smets <jan.smets at nokia dot com> ---
The testcase has a sign-compare warning that we've traditionally been ignoring
given the ancient codebase. 

warning: operand of ‘?:’ changes signedness from ‘int’ to ‘long unsigned int’
due to unsignedness of other operand [-Wsign-compare]

Changing the testcase From:

 ((address_ != (0L)) ? (tUint8 *)(address_) - (tUint8 *)(&((struct
tmm_blk_free_head_t *)0)->ql.column_list) : (uintptr_t)(0L));

To:

 ((address_ != (0L)) ? (uintptr_t)(tUint8 *)(address_) - (uintptr_t)(tUint8
*)(&((struct tmm_blk_free_head_t *)0)->ql.column_list) : (uintptr_t)(0L));

will generate different code in GCC 10, whereas it generates the same code in
GCC 9 for both forms.
 (with the added casts the generated GCC 10 code is good), 


commit 810c42c38d37317c80b57db0a8b6d8991e78ef50
Author: Richard Biener <rguenther@suse.de>
Date:   Mon May 20 12:02:35 2019 +0000

    tree-ssa-structalias.c (find_func_aliases): POINTER_DIFF_EXPR doesn't
produce pointers.

    2019-05-20  Richard Biener  <rguenther@suse.de>

            * tree-ssa-structalias.c (find_func_aliases): POINTER_DIFF_EXPR
            doesn't produce pointers.
            {TRUNC,CEIL,FLOOR,ROUND,EXACT}_{DIV,MOD}_EXPR points to what
            the first operand points to.

    From-SVN: r271414


I've been trying to get a working/crashing testcase for you, but finding the
right 'input data' to trigger the behavior for my testcase seems a bit
problematic.

[Bug rtl-optimization/96993] [10/11 Regression] invalid code

[pinskia at gcc dot gnu.org]

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

--- Comment #4 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Is there any reason why you didn't use offsetof/__builtin_offsetof here?
Instead of playing tricks like:
(tUint8 *)(&((struct tmm_blk_free_head_t *)0)->ql.column_list)

[Bug tree-optimization/96993] [10/11 Regression] invalid code

[jan.smets at nokia dot com]

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

--- Comment #5 from Jan Smets <jan.smets at nokia dot com> ---
Hi Andrew
I agree that __builtin_offsetof would of good use here. However, I believe this
code predates the availability of the builtin (was written around the time we
were using gcc3.4)

[Bug tree-optimization/96993] [10/11 Regression] invalid code

[jakub at gcc dot gnu.org]

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

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

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

--- Comment #6 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
offsetof existed since forever and is portable.
Anyway, I think Richard's change is completely fine and the testcase is broken
in many ways, most importantly in what the commit talks about, that a
difference of pointers should never produce a valid pointer.
So, instead of
(tUint8 *)(address_) - (tUint8 *)(&((struct tmm_blk_free_head_t
*)0)->ql.column_list)
you should use
(tUint8 *)(address_) - offsetof (struct tmm_blk_free_head_t, ql.column_list)
or worst case
(tUint8 *)(address_) - ((tUint8 *)(&((struct tmm_blk_free_head_t
*)0)->ql.column_list) - (tUint8 *)0), i.e. subtract from a pointer some
integer, rather than pretending that from the difference of two pointers you
get something that is a valid address and can be dereferenced.