Bug 92294 - alias attribute generates incorrect code
Summary: alias attribute generates incorrect code
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: rtl-optimization (show other bugs)
Version: 4.8.4
: P3 normal
Target Milestone: 10.3
Assignee: rsandifo@gcc.gnu.org
URL: https://gcc.gnu.org/pipermail/gcc-pat...
Keywords: wrong-code
Depends on:
Blocks: 92932
  Show dependency treegraph
 
Reported: 2019-10-30 18:47 UTC by Wilco
Modified: 2021-01-19 17:53 UTC (History)
2 users (show)

See Also:
Host:
Target: aarch64
Build:
Known to work:
Known to fail:
Last reconfirmed: 2019-10-31 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Wilco 2019-10-30 18:47:13 UTC
The following example (from gcc.c-torture/execute/alias-2.c) always calls abort on any AArch64 compiler with -O1 or -O2:

static int a[10];
extern int b[10] __attribute__ ((alias("a")));
int off = 0;
void f(void)
{
  b[off]=1;
  a[off]=2;
  if (b[off]!=2)
   __builtin_abort ();
}

Using extern linkage for 'a' avoids the problem, as is doing off = 1 or static int off = 0. It may only affect targets which use section anchors since -fno-section-anchors avoids the issue.
Comment 1 Richard Earnshaw 2019-10-31 17:36:52 UTC
Things go wrong in the forward-prop 1 pass.
Comment 2 Wilco 2019-10-31 17:44:11 UTC
Confirmed then
Comment 3 rsandifo@gcc.gnu.org 2020-01-24 12:49:21 UTC
About to post a patch.
Comment 4 Jakub Jelinek 2020-05-07 11:56:19 UTC
GCC 10.1 has been released.
Comment 5 Richard Biener 2020-07-23 06:51:54 UTC
GCC 10.2 is released, adjusting target milestone.
Comment 6 Marius Hillenbrand 2020-11-13 09:30:24 UTC
Same behavior on s390x: the testcase always calls abort(). As on aarch64, -fno-section-anchors avoids the issue.

the first cse pass already makes a mistake -- on both aarch64 and s390x, the compare looses its memory-read for the current value of b and instead reuses the constant 1 assigned to b earlier.

on aarch64 that looks like

(insn # # # 2 (set (reg:SI 110)
        (mem:SI (plus:DI (mult:DI (reg:DI 109)
                    (const_int 4 [0x4]))
                (reg/f:DI 107)) [1 b[off.0_1]+0 S4 A32])) "alias-2.c":9:6# {*movsi_aarch64}
     (nil))
(insn # # # 2 (set (reg:CC 66 cc)
        (compare:CC (reg:SI 110)
            (const_int 2 [0x2]))) "alias-2.c":9:6# {cmpsi}
     (nil))

... becomes ...

(insn # # # 2 (set (reg:SI 100)
        (const_int 1 [0x1])) "alias-2.c":7:9# {*movsi_aarch64}
     (nil)) [from the write to b]
...
(insn # # # 2 (set (reg:CC 66 cc)
        (compare:CC (reg:SI 100)
            (const_int 2 [0x2]))) "alias-2.c":9:6# {cmpsi}
     (expr_list:REG_DEAD (reg:SI 110 [ b[off.0_1] ])
        (nil)))
Comment 7 Marius Hillenbrand 2020-11-19 11:03:23 UTC
A simpler example derived from alias-2.c reproduces this issue on aarch64, ppc64, and s390x. 

int a;
extern int b __attribute__ ((alias("a")));
int off;

int foo()
{
  /* make sure off is ahead of a and b in .bss, so that a has a non-negative
   * offset relative to the anchor. */
  return off;
}

main()
{
  b=1;
  a=2;
  if (b!=2)
   __builtin_abort ();
  return 0;
}

Note that gcc.c-torture/execute/alias-2.c did not reproduce this issue on ppc64(le) for me. Removing the array indexing reduces the complexity of the RTL dumps somewhat.
Comment 8 Marius Hillenbrand 2020-11-19 13:02:42 UTC
From my current understanding, gcc addresses a and b in two different ways, which is not handled correctly by the dependency analysis / alias analysis while employed by the cse1 pass, and then causes cse1 to incorrectly eliminate the read from b (reusing the const 1 from b=1).

the rtl addresses a via the section anchor yet b via its symbol reference:

b = 1 becomes
(set (reg/f:DI 62) (symbol_ref:DI ("b") [flags 0x602]  <var_decl .. b>))
(set (mem/c:SI (reg/f:DI 62) [1 b+0 S4 A32]) (const_int 1 [0x1]))

vs

a=2 becomes
(set (reg/f:DI 64) (symbol_ref:DI ("*.LANCHOR0") [flags 0x182]))
(set (mem/c:SI (plus:DI (reg/f:DI 64) (const_int 4 [0x4])) [1 a+0 S4 A32]) (const_int 2 [0x2]))

when visiting the write to a, cse1 should identify the aliasing and thus drop the "remembered" b. for that, cse iterates its hash table and compares all present MEM rtx to the new address (cse.c:invalidate()).

that step compares (canonicalized) addresses
(const:DI (plus:DI (symbol_ref:DI ("*.LANCHOR0") [flags 0x182]) (const_int 4 [0x4])))
and (symbol_ref:DI ("b") [flags 0x602]  <var_decl b>)

the comparison of these two (in alias.c:write_dependence_p()) falsely claims that there is no anti-dependence between the two.

I am not certain yet how that comparison is supposed to work with section anchors. It looks like we either fail to reconnect (anchor + 4) to the symbol_ref of "a" (so we could identify both symbol_refs as equivalent) or lose the offset relative to the anchor (so we could see that both have the same offset relative to the anchor).
Comment 9 rsandifo@gcc.gnu.org 2020-11-23 20:49:21 UTC
Oops, I never linked the patch, it's:
https://gcc.gnu.org/pipermail/gcc-patches/2020-February/539763.html
(following on from an earlier discussion in January).

Reading the thread back now, the reception wasn't as negative
as I'd remembered.  I'd been vaguely hoping to find time to try
removing find_base_term & co. for GCC 11, but that obviously
never happened.  (Turns out that writing new code is more fun. ;-))

Maybe we could consider going for the linked patch for GCC 11.
Comment 10 CVS Commits 2021-01-19 17:51:17 UTC
The master branch has been updated by Richard Sandiford <rsandifo@gcc.gnu.org>:

https://gcc.gnu.org/g:6a2a38620cf178b53b217051f32d1d7bbba86fc9

commit r11-6796-g6a2a38620cf178b53b217051f32d1d7bbba86fc9
Author: Richard Sandiford <richard.sandiford@arm.com>
Date:   Tue Jan 19 17:50:53 2021 +0000

    alias: Fix offset checks involving section anchors [PR92294]
    
    memrefs_conflict_p assumes that:
    
      [XB + XO, XB + XO + XS)
    
    does not alias
    
      [YB + YO, YB + YO + YS)
    
    whenever:
    
      [XO, XO + XS)
    
    does not intersect
    
      [YO, YO + YS)
    
    In other words, the accesses can alias only if XB == YB at runtime.
    
    However, this doesn't cope correctly with section anchors.
    For example, if XB is an anchor symbol and YB is at offset
    XO from the anchor, then:
    
      [XB + XO, XB + XO + XS)
    
    overlaps
    
      [YB, YB + YS)
    
    whatever the value of XO is.  In other words, when doing the
    alias check for two symbols whose local definitions are in
    the same block, we should apply the known difference between
    their block offsets to the intersection test above.
    
    gcc/
            PR rtl-optimization/92294
            * alias.c (compare_base_symbol_refs): Take an extra parameter
            and add the distance between two symbols to it.  Enshrine in
            comments that -1 means "either 0 or 1, but we can't tell
            which at compile time".
            (memrefs_conflict_p): Update call accordingly.
            (rtx_equal_for_memref_p): Likewise.  Take the distance between symbols
            into account.
Comment 11 rsandifo@gcc.gnu.org 2021-01-19 17:53:26 UTC
Fixed on trunk.  I'm not sure it's suitable for backports.