[Bug tree-optimization/62035] New: [4.9 Regresion] wrong code building libapache-mod-perl with -O1, works with -O1 -fno-tree-dse

[Bug tree-optimization/62035] New: [4.9 Regresion] wrong code building libapache-mod-perl with -O1, works with -O1 -fno-tree-dse

[doko at gcc dot gnu.org]

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

            Bug ID: 62035
           Summary: [4.9 Regresion] wrong code building libapache-mod-perl
                    with -O1, works with -O1 -fno-tree-dse
           Product: gcc
           Version: 4.9.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: tree-optimization
          Assignee: unassigned at gcc dot gnu.org
          Reporter: doko at gcc dot gnu.org

Created attachment 33260
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=33260&action=edit
test case

[forwarded from http://bugs.debian.org/754901]

libapache2-mod-perl2 build segfaults in the test suite when built with gcc 4.9
(r213317) with -O1 or higher, building with -O1 -fno-tree-dse works around it.
seen at least on x86_64-linux-gnu.

attributing one function with

__attribute__((optimize("-fno-tree-dse")))
modperl_enc.c (modperl_env_table_populate)

is enough to avoid the segfaults in the test suite. The diff in the assembler
is:

--- ok/modperl_env.s    2014-08-06 12:42:36.714269110 +0000
+++ bad/modperl_env.s   2014-08-06 12:40:44.441445210 +0000
@@ -118,7 +118,6 @@
 .LVL15:
        movq    %rax, %r14
 .LVL16:
-       movq    %rax, 40(%rsp)
        subq    $8, %rsp
        .cfi_def_cfa_offset 120
        pushq   $0

[Bug tree-optimization/62035] [4.9/4.10 Regression] wrong code building libapache-mod-perl with -O1, works with -O1 -fno-tree-dse

[wschmidt at gcc dot gnu.org]

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

Bill Schmidt <wschmidt at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |rguenth at gcc dot gnu.org,
                   |                            |wschmidt at gcc dot gnu.org

--- Comment #2 from Bill Schmidt <wschmidt at gcc dot gnu.org> ---
Reported to fail for powerpc64le-unknown-linux-gnu as well.  CCing middle end
maintainer.

[Bug tree-optimization/62035] [4.9/4.10 Regression] wrong code building libapache-mod-perl with -O1, works with -O1 -fno-tree-dse

[rguenth at gcc dot gnu.org]

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

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

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

--- Comment #3 from Richard Biener <rguenth at gcc dot gnu.org> ---
I can reproduce the bad assembly with -O -g -fwrapv -fstack-protector -fPIC
--param ssp-buffer-size=4 --param ssp-buffer-size=4

The only dead store deleted in modperl_env_table_populate is

  Deleted dead store 'sv = sv.5_31;

and that's ok because we have a clobber of 'sv' in this path.

  <bb 7>:
  _29 = _22->val;
  sv.5_31 = Perl_newSVpv (my_perl_7(D), _29, 0);
  sv = sv.5_31;
  _33 = _22->key;
  Perl_hv_common_key_len (my_perl_7(D), hv_10, _33, klen_26, 36, sv.5_31, 0);
  _35 = _22->key;
  Perl_sv_magic (my_perl_7(D), sv.5_31, 0B, 101, _35, klen_26);
  # DEBUG svp => &sv
  sv ={v} {CLOBBER};

  <bb 8>:
  # svp_2 = PHI <&sv(7), svp_28(6)>
  # DEBUG svp => svp_2
  _41 = my_perl_7(D)->Itainting;
  if (_41 != 0)
    goto <bb 9>;
  else
    goto <bb 10>;

  <bb 9>:
  _42 = *svp_2;
  Perl_sv_magic (my_perl_7(D), _42, 0B, 116, 0B, 0);

but a use-after-free here through *svp_2.  Prettified preprocessed souce
looks like

        SV **svp = ((SV**) Perl_hv_common_key_len(my_perl,
(hv),(elts[i].key),(klen),((0)) ? (0x20 | 0x10) : 0x20,((void *)0),0));
        if (svp) { 
          Perl_sv_setpv(my_perl, *svp,elts[i].val);
        } else { 
          SV *sv = Perl_newSVpv(my_perl, elts[i].val,0);
          (void)((SV**) Perl_hv_common_key_len(my_perl,
(hv),(elts[i].key),(klen),(0x04|0x20),(sv),((0))));
          Perl_sv_magic(my_perl, sv,(SV *)((void *)0),'e',elts[i].key,klen);
          svp = &sv;
        } 
        if (0) modperl_trace(__func__, "$ENV{%s} = \"%s\";", elts[i].key,
elts[i].val);
        (void)({ 
          if(((my_perl->Itainting))){Perl_sv_magic(my_perl, (*svp),((void
*)0),'t',((void *)0),0);

so 'sv' is declared inside the 'else' but you make its address escape
through the 'svp' variable declared in the outer block.

Invalid.

A fix is to move the declaration of SV *sv up one block.

[Bug tree-optimization/62035] [4.9/4.10 Regression] wrong code building libapache-mod-perl with -O1, works with -O1 -fno-tree-dse

[rguenth at gcc dot gnu.org]

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

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

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

--- Comment #4 from Richard Biener <rguenth at gcc dot gnu.org> ---
Marc, do you think it's possible to warn about such "return of local
addresses"?

[Bug tree-optimization/62035] [4.9/4.10 Regression] wrong code building libapache-mod-perl with -O1, works with -O1 -fno-tree-dse

[glisse at gcc dot gnu.org]

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

--- Comment #5 from Marc Glisse <glisse at gcc dot gnu.org> ---
(In reply to Richard Biener from comment #4)
> Marc, do you think it's possible to warn about such "return of local
> addresses"?

It doesn't seem easy. Here are some random thoughts.

At the exit of a block, we can say that the address of sv becomes nonsense, but
we don't have an easy way to replace it with 0 or undefined. The front end
doesn't track the leaking address. In the middle-end, we don't know anymore
that &sv is meaningless. AFAIK, in SSA, all variables live for the whole
function, the clobber only tells us that the content is nonsense, but the
address itself can perfectly well be reused to write new content. So we are
left with trying to warn about the read-after-clobber.

bb 10 ends with:
  # .MEM_37 = VDEF <.MEM_36>
  svD.22441 ={v} {CLOBBER};

bb 11 contains:
  # svp_2 = PHI <&svD.22441(10), svp_28(9)>
  # .MEM_3 = PHI <.MEM_37(10), .MEM_40(9)>
  if (_41 != 0)
    goto <bb 12>;

and bb 12 starts with:
  # VUSE <.MEM_3>
  _42 = *svp_2;

It is possible to see from this that _42 will receive garbage if we take the
path 10 -> 11 -> 12. But that's going to be an awfully specific piece of code
(and we don't want to start a walk of vdefs every time something is
dereferenced). I haven't yet studied the passes that track values in memory, so
I don't know if they can be adapted. I think there are lower hanging fruits
than this though.