[Bug rtl-optimization/103830] New: volatile optimized away

[Bug rtl-optimization/103830] New: volatile optimized away

[bernd.edlinger at hotmail dot de]

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

            Bug ID: 103830
           Summary: volatile optimized away
           Product: gcc
           Version: 12.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: rtl-optimization
          Assignee: unassigned at gcc dot gnu.org
          Reporter: bernd.edlinger at hotmail dot de
  Target Milestone: ---

the following test case is intentionally writing at address 0,
it is IMHO invalid to optimize it away (at -Og):

$ cat empty-inline.cc
/* This testcase is part of GDB, the GNU debugger.

   Copyright 2021 Free Software Foundation, Inc.

   This program is free software; you can redistribute it and/or modify
   it under the terms of the GNU General Public License as published by
   the Free Software Foundation; either version 3 of the License, or
   (at your option) any later version.

   This program is distributed in the hope that it will be useful,
   but WITHOUT ANY WARRANTY; without even the implied warranty of
   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
   GNU General Public License for more details.

   You should have received a copy of the GNU General Public License
   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */

/* PR 25987 */
struct MyClass;
struct ptr {
    MyClass* get() { return t; }     /* line 21 */
    MyClass* t;
};
struct MyClass { void call(); };
void MyClass::call() {
    *(volatile char*)(nullptr) = 1;  /* line 26 */
}
static void intermediate(ptr p) {
    p.get()->call();                 /* line 29 */
}
int main() {
    intermediate(ptr{new MyClass});
}
/* EOF */

previously this used to SIGSEGV in line 26, but no longer with
git master from December 18th at least.
I think it is rather common to have something at address zero,
for instance interrupt tables or device registers,
and therefore it's not OK to optimize those volatile accesses away.

[Bug rtl-optimization/103830] volatile optimized away

[pinskia at gcc dot gnu.org]

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

--- Comment #1 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
> I think it is rather common to have something at address zero,
> for instance interrupt tables or device registers,
> and therefore it's not OK to optimize those volatile accesses away.
-fno-delete-null-pointer-checks is designed for that use case.

[Bug ipa/103830] [12 Regression] null pointer access optimized away by removing function call at -Og

[pinskia at gcc dot gnu.org]

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

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
          Component|rtl-optimization            |ipa
   Last reconfirmed|                            |2021-12-26
   Target Milestone|---                         |12.0
            Summary|volatile optimized away     |[12 Regression] null
                   |                            |pointer access optimized
                   |                            |away by removing function
                   |                            |call at -Og
             Status|UNCONFIRMED                 |NEW
                 CC|                            |hubicka at gcc dot gnu.org,
                   |                            |marxin at gcc dot gnu.org,
                   |                            |pinskia at gcc dot gnu.org
     Ever confirmed|0                           |1

--- Comment #2 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
What was removed is not the null pointer itself but rather the function calls
to both MyClass::call and intermediate.


cddce:
Eliminating unnecessary statements:
Deleting : MyClass::call (_4);


Eliminating unnecessary statements:
Deleting : intermediate (D.2427);


>From local-const-pure:

 local analysis of void MyClass::call()/1
   NULL memory access; terminating BB
    checking previously known:
void MyClass::call()/1 is not a malloc candidate, reason: No return value.
Function is locally const.

....

The question is do we want to do const-pure at -Og really.

>it is IMHO invalid to optimize it away (at -Og):

No, it is valid to remove as it is undefined behavior, the question comes do we
want to do the removal at -Og; I suspect no.

[Bug ipa/103830] [12 Regression] null pointer access optimized away by removing function call at -Og

[jakub at gcc dot gnu.org]

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

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

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

--- Comment #3 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
The analysis, why not.  But perhaps we shouldn't DCE const or pure functions
are -Og, or CSE them etc.

[Bug ipa/103830] [12 Regression] null pointer access optimized away by removing function call at -Og

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P3                          |P1

--- Comment #4 from Richard Biener <rguenth at gcc dot gnu.org> ---
I think the recent modref change made the function const.

And no, we shouldn't DSE any volatile store and generally we don't.  It's
probably some side-effect of modref that we do.  Using -fno-ipa-pure-const
"fixes" this bug with -Og:

 local analysis of void MyClass::call()/1
   NULL memory access; terminating BB
Function is locally const.
callgraph:

so it's caused by the recent change to mitigate path-isolation damage to
modref.

[Bug ipa/103830] [12 Regression] null pointer access optimized away by removing function call at -Og

[hubicka at kam dot mff.cuni.cz]

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

--- Comment #5 from hubicka at kam dot mff.cuni.cz ---
> I think the recent modref change made the function const.
> 
> And no, we shouldn't DSE any volatile store and generally we don't.  It's
> probably some side-effect of modref that we do.  Using -fno-ipa-pure-const
> "fixes" this bug with -Og:
> 
>  local analysis of void MyClass::call()/1
>    NULL memory access; terminating BB
> Function is locally const.
> callgraph:
> 
> so it's caused by the recent change to mitigate path-isolation damage to
> modref.

The change indeed assumes that with -fdelete-null-pointer-checks the
access to NULL is invalid no matter if it is volatile or normal.  I
would expect code having exception handlers at address 0 to be always
built with -fno-delete-null-pointer-checks.

If we want to preserve user defined volaitle NULL, is there way to stick
another flag on the memory accesses synthetised by isolate-paths to mark
them as OK to be optimized this way?

[Bug ipa/103830] [12 Regression] null pointer access optimized away by removing function call at -Og

[rguenth at gcc dot gnu.org]

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

--- Comment #6 from Richard Biener <rguenth at gcc dot gnu.org> ---
To clarify myself - the testcase is invoking UB, 'volatile' doesn't make a
difference here.  That we get the DSE as a second-order effect is unfortunate
at most, in other places we try to preserve volatile qualified accesses.  But I
do not think it's worth to pessimize -Og for that.  If you declare
MyClass::call() noinline you get the same behavior with all optimization
levels:

struct MyClass;
struct ptr {
    MyClass* get() { return t; }     /* line 21 */
    MyClass* t;
};
struct MyClass { void __attribute__((noinline)) call(); };
void MyClass::call() {
    *(volatile char*)(nullptr) = 1;  /* line 26 */
}
static void intermediate(ptr p) {
    p.get()->call();                 /* line 29 */
}
int main() {
    intermediate(ptr{new MyClass});
}

so iff then we need to argue about the modref/ipa-pure-const behavior, not
about DCE of calls at -Og.

[Bug ipa/103830] [12 Regression] null pointer access optimized away by removing function call at -Og

[rguenth at gcc dot gnu.org]

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

--- Comment #7 from Richard Biener <rguenth at gcc dot gnu.org> ---
A fix in the source would be:

void MyClass::call() {
    volatile char * volatile null = nullptr;
    *null = 1;  /* line 26 */
}

which then compiles to

        movq    $0, -8(%rsp)
        movq    -8(%rsp), %rax
        movb    $1, (%rax)
        ret

(some "advanced" means using some asm() to hide the constant from the compiler
might also work)

[Bug ipa/103830] [12 Regression] null pointer access optimized away by removing function call at -Og

[rguenth at gcc dot gnu.org]

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

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

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

--- Comment #8 from Richard Biener <rguenth at gcc dot gnu.org> ---
I'd say invalid.  For -Og we might want to consider not doing pure/const
discovery (or modref) but stick to what the user declared so.

[Bug ipa/103830] [12 Regression] null pointer access optimized away by removing function call at -Og

[bernd.edlinger at hotmail dot de]

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

Bernd Edlinger <bernd.edlinger at hotmail dot de> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|INVALID                     |FIXED

--- Comment #9 from Bernd Edlinger <bernd.edlinger at hotmail dot de> ---
(In reply to Richard Biener from comment #7)
> A fix in the source would be:
> 
> void MyClass::call() {
>     volatile char * volatile null = nullptr;
>     *null = 1;  /* line 26 */
> }
> 
> which then compiles to
> 
>         movq    $0, -8(%rsp)
>         movq    -8(%rsp), %rax
>         movb    $1, (%rax)
>         ret
> 
> (some "advanced" means using some asm() to hide the constant from the
> compiler might also work)

Yes, that works, but I would prefer:

*(volatile char*)1 = 2;