Bug 106558

With -fno-toplevel-reorder, it can be detected.
I can't figure out why there is a difference really.

Might be related to PR 82501.

Looks like a bug in the sanopt pass.
For -O2, we have before sanopt in main:
  b.0_1 = b;
  e.2_3 = e;
  c.5_4 = c;
  .ASAN_CHECK (7, c.5_4, 8, 8);
  *c.5_4 = e.2_3;
  b.7_5 = b;
  .ASAN_CHECK (7, b.7_5, 4, 4);
  *b.7_5 = 2;
  .ASAN_CHECK (7, b.0_1, 4, 4);
  *b.0_1 = 2;
  return 2;
and in sanopt:
Leaving: .ASAN_CHECK (7, c.5_4, 8, 8);
Leaving: .ASAN_CHECK (7, b.7_5, 4, 4);
Optimizing out: .ASAN_CHECK (7, b.0_1, 4, 4);
Expanded: .ASAN_CHECK (7, c.5_4, 8, 8);
Expanded: .ASAN_CHECK (7, b.7_5, 4, 4);
Even that is incorrect, we don't generally know what b points to before the *c store and after it, so neither of the stores is redundant because *c store can change the value of b.
At -O1 we have before sanopt:
  b.0_1 = b;
  e.2_6 = e;
  e.3_7 = (long int) e.2_6;
  _9 = (int) e.3_7;
  .ASAN_CHECK (7, b.0_1, 4, 4);
  *b.0_1 = _9;
  c.5_10 = c;
  e.6_11 = e;
  .ASAN_CHECK (7, c.5_10, 8, 8);
  *c.5_10 = e.6_11;
  b.7_12 = b;
  .ASAN_CHECK (7, b.7_12, 4, 4);
  *b.7_12 = 2;
  *b.0_1 = 2;
  b.1_2 = b;
  .ASAN_CHECK (6, b.1_2, 4, 4);
  _5 = *b.1_2;
  return _5;
because we optimize less at that optimization level, and sanopt:
Leaving: .ASAN_CHECK (7, b.0_1, 4, 4);
Leaving: .ASAN_CHECK (7, c.5_10, 8, 8);
Optimizing out: .ASAN_CHECK (7, b.7_12, 4, 4);
Optimizing out: .ASAN_CHECK (6, b.1_2, 4, 4);
Expanded: .ASAN_CHECK (7, b.0_1, 4, 4);
Expanded: .ASAN_CHECK (7, c.5_10, 8, 8);
The b.1_2 .ASAN_CHECK is IMHO redundant (b couldn't change between b.7_12 = b and b.1_2 = b;) but the b.7_12 .ASAN_CHECK is not redundant.

This is due to the https://gcc.gnu.org/legacy-ml/gcc-patches/2014-12/msg00242.html
r5-5530-ge28f2090dbbb5072 optimization, which is incorrect.
If we want to track pointers which live in memory, we'd need to ask the alias oracle on each store or call whether the store or call couldn't change the value of such a pointer and if yes, invalidate them.
Not really sure it is worth it though.

Ok, seems my 2014 patch will need to be reverted then. Do you want me to submit a PR?

Or perhaps could we ask the alias oracle in can_remove_asan_check
for the *base_checks case if base_addr lives in memory whether base_addr could change in between the stmt in the vector and current stmt, with some low limit on  how much we walk to find that out?

Perhaps either a quick check that for base ptrs that live in memory gimple_vuse is the same for both statements or if not, do walk_aliased_vdefs with low constant limit?
We'd want to stop if we reach the vdef of the stmt in base_checks vector (then we didn't find anything that could clobber it and can therefore use the cached check) or when we see a stmt that may clobber it (then we can't use the cached check).

(In reply to Jakub Jelinek from comment #7)

I've started work on this but I'll probly only have enough time to cook a patch on weekend.

> Perhaps either a quick check that for base ptrs that live in memory

A silly question, such cases (base_addrs living in memory) can be identified by
  gimple *g = SSA_NAME_DEF_STMT (t);
in maybe_get_single_definition having vuses?

If maybe_get_single_definition returns a SSA_NAME or is_gimple_min_invariant,
then it is ok as is and doesn't need anything new.
Otherwise I think we need to ask the alias oracle.

Created attachment 53458 [details]
Very draft patch

(In reply to Jakub Jelinek from comment #7)
> Perhaps either a quick check that for base ptrs that live in memory
> gimple_vuse is the same for both statements or if not, do walk_aliased_vdefs
> with low constant limit?
> We'd want to stop if we reach the vdef of the stmt in base_checks vector
> (then we didn't find anything that could clobber it and can therefore use
> the cached check) or when we see a stmt that may clobber it (then we can't
> use the cached check).

Something like this? It does not help with b.1_2 in attached reprocase though, because alias oracle considers
  *b.0_1 = 2;
to clobber it.

I'm trying to collect statistics how many checks this optimization removes during bootstrap-asan but I'm getting crashes when asan-bootstrapping on unchanged trunk. Is this possible?

Created attachment 53493 [details]
Updated patch

Here is an updated patch which passes bootstrap-asan (I haven't run the testsuite yet).

It results in only small 0.15% reduction of optimized checks (146062 -> 145824). But more importantly the complicated alias oracle check does not seem to contribute anything - removing it from same_value_p function (the name is ugly, I know) does not change the number of optimized checks. So I'm not sure it makes sense to keep it in final patch.

Created attachment 53530 [details]
Final patch

Attached patch passes bootstrap (regular and asan) and regtesting and, as explained above, results in very small <1% reduction of optimizations. If there are no objections, I'll post it to gcc-patches.

Posted to gcc-patches: https://gcc.gnu.org/pipermail/gcc-patches/2022-September/601041.html

Hello, is this patch going to be pushed to the trunk?

(In reply to Li Shaohua from comment #14)
> Hello, is this patch going to be pushed to the trunk?

Not yet. The patch is under review process.

*** Bug 107746 has been marked as a duplicate of this bug. ***

Fix has been approved (https://gcc.gnu.org/pipermail/gcc-patches/2022-November/606858.html), I hope to merge it soon.

*** Bug 107806 has been marked as a duplicate of this bug. ***

The master branch has been updated by Martin Liska <marxin@gcc.gnu.org>:

https://gcc.gnu.org/g:5dd4d2e93e3de60d4ef1068b6dfd06b6b9fff16e

commit r13-4354-g5dd4d2e93e3de60d4ef1068b6dfd06b6b9fff16e
Author: Yuri Gribov <y.gribov@samsung.com>
Date:   Sun Aug 14 08:42:44 2022 +0300

    asan: fix unsafe optimization of Asan checks.
    
            PR sanitizer/106558
    
    gcc/
            * sanopt.cc: Do not optimize out checks for non-SSA addresses.
    
    gcc/testsuite/
            * c-c++-common/asan/pr106558.c: New test.

Fixed on master. Do we want to do a backport?

*** Bug 107698 has been marked as a duplicate of this bug. ***