Bug 63311 - [11/12 Regression] -O1 optimization introduces valgrind warning
Summary: [11/12 Regression] -O1 optimization introduces valgrind warning
Status: NEW
Alias: None
Product: gcc
Classification: Unclassified
Component: middle-end (show other bugs)
Version: 5.0
: P2 normal
Target Milestone: 11.5
Assignee: Not yet assigned to anyone
URL:
Keywords: wrong-code
Depends on:
Blocks:
 
Reported: 2014-09-19 15:03 UTC by Joost VandeVondele
Modified: 2023-10-25 22:45 UTC (History)
7 users (show)

See Also:
Host:
Target:
Build:
Known to work: 13.2.0, 14.0, 4.8.3
Known to fail: 12.3.0, 13.1.0, 4.9.2
Last reconfirmed: 2018-02-05 00:00:00


Attachments
C testcase (676 bytes, text/x-csrc)
2014-10-31 09:30 UTC, Joost VandeVondele
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Joost VandeVondele 2014-09-19 15:03:39 UTC
The following testcase yields a valgrind error when compiled with -O1 but not at -O0. 4.8 is fine 4.9/trunk is not.

> gfortran -O1 -g bug.f90 && valgrind ./a.out 
==57092== Conditional jump or move depends on uninitialised value(s)
==57092==    at 0x4006F5: __m1_MOD_test (bug.f90:20)
==57092==    by 0x4007BB: main (bug.f90:36)
==57092== ERROR SUMMARY: 4 errors from 1 contexts (suppressed: 6 from 6)

> cat bug.f90
MODULE M1
  IMPLICIT NONE
CONTAINS
  INTEGER FUNCTION foo()
     INTEGER, VOLATILE :: v=42
     foo=v
  END FUNCTION
  SUBROUTINE test(n,flag)
    INTEGER :: n,i,j,k,l,tt
    LOGICAL :: flag
    REAL(KIND=8) :: v,t
    IF (flag) THEN
      t=42
      tt=foo()
    ENDIF
    v=0
    DO i=1,n
       v=0
       IF (flag) THEN
          IF (tt==i) v=MAX(v,t)
       ENDIF
       DO j=1,n
        DO k=1,n
            v=MAX(v,sin(REAL(j*k)))
         ENDDO
       ENDDO
    ENDDO
  END SUBROUTINE
END MODULE M1

USE M1
INTEGER :: n
LOGICAL :: flag
n=4
flag=.FALSE.
CALL test(n,flag)
END

what seems to happen is that '(tt==i)' is evaluated before IF (flag) is evaluated, also in the case where 'flag==.FALSE.', while 'tt' is only initialized in the .TRUE. case. 

I actually think generated code might nevertheless be correct, but it makes valgrind less useful on -O1 code.
Comment 1 Richard Biener 2014-09-22 10:24:37 UTC
This is probably ifcombine at work and the target splitting the combined
condition back, but the other way around.
Comment 2 Dominique d'Humieres 2014-09-22 10:34:34 UTC
Confirmed.
Comment 3 Jakub Jelinek 2014-10-09 09:45:40 UTC
Started with r208687 , so possibly a latent bug is no longer latent after the FE changes.
Comment 4 Jakub Jelinek 2014-10-30 10:40:47 UTC
GCC 4.9.2 has been released.
Comment 5 Joost VandeVondele 2014-10-31 09:28:27 UTC
(In reply to Jakub Jelinek from comment #3)
> Started with r208687 , so possibly a latent bug is no longer latent after
> the FE changes.

It also reproduces with a C instead of with a Fortran testcase (as derived from the .original dump). To be attached in a second:

> gcc -O1 -std=c11 -g PR63311.c -lm && valgrind ./a.out
==36918== Conditional jump or move depends on uninitialised value(s)
==36918==    at 0x40056C: test (PR63311.c:41)
==36918==    by 0x40061F: main (PR63311.c:130)
==36918== ERROR SUMMARY: 4 errors from 1 contexts (suppressed: 6 from 6)

> gcc -O0 -std=c11 -g PR63311.c -lm && valgrind ./a.out
==36936== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 6 from 6)
Comment 6 Joost VandeVondele 2014-10-31 09:30:30 UTC
Created attachment 33852 [details]
C testcase

warning with : gcc -O1 -std=c11 -g PR63311.c -lm && valgrind ./a.out
Comment 7 Richard Biener 2014-11-24 13:27:39 UTC
Confirmed that it is ifcombine.  Not sure if I'd call it wrong-code though.

Note that there are no default-defs involved thus ifcombine doesn't see
must-uninits and disabling its transform when maybe-uninits are seen
is IMHO bogus.

Not sure what to do about this one.  There is no way to preserve evaluation
order (no andif / orif on gimple).
Comment 8 Joost VandeVondele 2014-11-24 13:54:43 UTC
(In reply to Richard Biener from comment #7)
> Confirmed that it is ifcombine.  Not sure if I'd call it wrong-code though.
> 
> Note that there are no default-defs involved thus ifcombine doesn't see
> must-uninits and disabling its transform when maybe-uninits are seen
> is IMHO bogus.
> 
> Not sure what to do about this one.  There is no way to preserve evaluation
> order (no andif / orif on gimple).

just wondering if the 'target splitting the condition back' means if has a choice which of the two to evaluate first. Be means of heuristic, if one of the two conditions contains a 'maybe-uninit' and the other not, it is presumably best to evaluate the 'maybe-uninit' one at the second stage ?

From a usability point of view, I had to touch a couple of spots in our code to make valgrind testing possible again, so while I wouldn't call this wrong-code, it certainly is some user-visible behavior.
Comment 9 Jakub Jelinek 2015-06-26 19:57:59 UTC
GCC 4.9.3 has been released.
Comment 10 Richard Biener 2016-08-03 11:43:22 UTC
GCC 4.9 branch is being closed
Comment 11 Jakub Jelinek 2017-10-10 13:26:33 UTC
GCC 5 branch is being closed
Comment 12 Aldy Hernandez 2018-02-05 12:54:19 UTC
Reconfirmed with C testcase in comment 6.  However, as per Richard's comment in #7, is this a WONTFIX?

tor:~/bld/t/gcc$ valgrind ./a.out
==29746== Memcheck, a memory error detector
==29746== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
==29746== Using Valgrind-3.12.0 and LibVEX; rerun with -h for copyright info
==29746== Command: ./a.out
==29746== 
==29746== Conditional jump or move depends on uninitialised value(s)
==29746==    at 0x40068C: test (a.c:41)
==29746==    by 0x4006CC: main (a.c:130)
Comment 13 Richard Biener 2018-02-07 11:51:53 UTC
Please leave the bug open - it does point at an undesirable transform which would conflict with GCC ever treating the use of uninitialized vars as "more" undefined as it does right now.
Comment 14 Jakub Jelinek 2018-10-26 10:10:14 UTC
GCC 6 branch is being closed
Comment 15 Richard Biener 2019-11-14 07:56:29 UTC
The GCC 7 branch is being closed, re-targeting to GCC 8.4.
Comment 16 Jakub Jelinek 2020-03-04 09:50:26 UTC
GCC 8.4.0 has been released, adjusting target milestone.
Comment 17 Jakub Jelinek 2021-05-14 09:47:20 UTC
GCC 8 branch is being closed.
Comment 18 Richard Biener 2021-06-01 08:06:29 UTC
GCC 9.4 is being released, retargeting bugs to GCC 9.5.
Comment 19 Mark Wielaard 2022-02-15 12:09:54 UTC
This still replicates with valgrind 3.18.1 and gcc 11.2.1:

$ gcc -O1 -std=c11 -g PR63311.c -lm && valgrind --track-origins=yes ./a.out
==2836066== Memcheck, a memory error detector
==2836066== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==2836066== Using Valgrind-3.18.1 and LibVEX; rerun with -h for copyright info
==2836066== Command: ./a.out
==2836066== 
==2836066== Conditional jump or move depends on uninitialised value(s)
==2836066==    at 0x4011E2: test (PR63311.c:41)
==2836066==    by 0x401223: main (PR63311.c:130)
==2836066==  Uninitialised value was created by a stack allocation
==2836066==    at 0x40112D: test (PR63311.c:11)
==2836066== 
==2836066== 
==2836066== HEAP SUMMARY:
==2836066==     in use at exit: 0 bytes in 0 blocks
==2836066==   total heap usage: 0 allocs, 0 frees, 0 bytes allocated
==2836066== 
==2836066== All heap blocks were freed -- no leaks are possible
==2836066== 
==2836066== For lists of detected and suppressed errors, rerun with: -s
==2836066== ERROR SUMMARY: 4 errors from 1 contexts (suppressed: 0 from 0)
Comment 20 David Malcolm 2022-02-16 22:49:10 UTC
FWIW I tried running both reproducers with -fanalyzer (Fortran and C), and found that -fanalyzer generates false positives on them, which I'm tracking separately as PR analyzer/104576.
Comment 21 GCC Commits 2022-02-17 02:41:43 UTC
The master branch has been updated by David Malcolm <dmalcolm@gcc.gnu.org>:

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

commit r12-7270-g5fbcbcaff7248604e04b39464f4fbd64fbf6e43b
Author: David Malcolm <dmalcolm@redhat.com>
Date:   Wed Feb 16 18:21:58 2022 -0500

    analyzer: const functions have no side effects [PR104576]
    
    PR analyzer/104576 tracks that we issue a false positive from
    -Wanalyzer-use-of-uninitialized-value for the reproducers of PR 63311
    when optimization is disabled.
    
    The root cause is that the analyzer was considering that a call to
    __builtin_sinf could have side-effects.
    
    This patch fixes things by generalizing the handling for "pure"
    functions to also consider "const" functions.
    
    gcc/analyzer/ChangeLog:
            PR analyzer/104576
            * region-model.cc: Include "calls.h".
            (region_model::on_call_pre): Use flags_from_decl_or_type to
            generalize check for DECL_PURE_P to also check for ECF_CONST.
    
    gcc/testsuite/ChangeLog:
            PR analyzer/104576
            * gcc.dg/analyzer/torture/uninit-pr63311.c: New test.
            * gcc.dg/analyzer/uninit-pr104576.c: New test.
            * gfortran.dg/analyzer/uninit-pr63311.f90: New test.
    
    Signed-off-by: David Malcolm <dmalcolm@redhat.com>
Comment 22 Richard Biener 2022-05-27 09:35:21 UTC
GCC 9 branch is being closed
Comment 23 Jakub Jelinek 2022-06-28 10:31:06 UTC
GCC 10.4 is being released, retargeting bugs to GCC 10.5.
Comment 24 Richard Biener 2023-07-07 10:30:26 UTC
GCC 10 branch is being closed.
Comment 25 Andrew Pinski 2023-09-03 03:39:44 UTC
Fixed for GCC 13.2.0 by r13-7533-g79b6a4875f3dcc4bbca9242313b3edc1bad69660 .
And the trunk by r14-2289-gb083203f053f16 .