[Bug middle-end/56301] New: [4.7 Regression] wrong code with the fix for PR53844

[Bug middle-end/56301] New: [4.7 Regression] wrong code with the fix for PR53844

[doko at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56301

             Bug #: 56301
           Summary: [4.7 Regression] wrong code with the fix for PR53844
    Classification: Unclassified
           Product: gcc
           Version: 4.7.3
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: middle-end
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: doko@gcc.gnu.org


works with 4.6 branch, 4.7.2 and the trunk. introduced in r195708 by the fix
for PR53844.

I don't have a reduced testcase yet. the unreduced test case is that the
upstart build in Ubuntu raring hangs in the testsuite with:

  Testing job_deserialise() ptrace handling
  BAD: wrong value for job->goal, expected 1 got 0
   at tests/test_job.c:7331 (deserialise_ptrace_next).

still trying to isolate the failure.

afaics, it is not architecture specific.

Even if backports are tested together, it would help if independent patches
could be committed separately to the branches.

[Bug middle-end/56301] [4.7 Regression] wrong code with the fix for PR53844

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56301

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P3                          |P1
             Status|UNCONFIRMED                 |WAITING
   Last reconfirmed|                            |2013-02-13
   Target Milestone|---                         |4.7.3
     Ever Confirmed|0                           |1

[Bug middle-end/56301] [4.7 Regression] wrong code with the fix for PR53844

[doko at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56301

--- Comment #1 from Matthias Klose <doko at gcc dot gnu.org> 2013-02-18 12:31:31 UTC ---
Created attachment 29482
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=29482
test case

no reduced test case yet, but there is a diff in the tree dump with
-fdump-tree-dse in the test_deserialise_ptrace function:

diff -u <working version> <not-working version>

--- revert/upstart-1.6.1/init/test_job.c.132t.dse2
+++ norevert/upstart-1.6.1/init/test_job.c.132t.dse2
@@ -367,12 +367,8 @@
   abort ();

 <bb 44>:
-  job_64->goal = 1;
-  job_64->state = 3;
   D.9269_65 = job_64->pid;
   *D.9269_65 = pid_49;
-  job_64->trace_forks = 0;
-  job_64->trace_state = 3;

 <bb 45>:
   __ret_66 = nih_str_array_new (0B);

[Bug middle-end/56301] [4.7 Regression] wrong code with the fix for PR53844

[doko at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56301

Matthias Klose <doko at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|WAITING                     |NEW

--- Comment #2 from Matthias Klose <doko at gcc dot gnu.org> 2013-02-18 12:37:50 UTC ---
with -fdump-tree-dse-details:

--- revert/upstart-1.6.1/init/test_job.c.132t.dse2
+++ norevert/upstart-1.6.1/init/test_job.c.132t.dse2
@@ -35,6 +35,14 @@

 ;; Function test_deserialise_ptrace (test_deserialise_ptrace, funcdef_no=60,
decl_uid=9022, cgraph_uid=60)

+  Deleted dead store 'job_64->trace_state = 3;
+'
+  Deleted dead store 'job_64->trace_forks = 0;
+'
+  Deleted dead store 'job_64->state = 3;
+'
+  Deleted dead store 'job_64->goal = 1;
+'
 test_deserialise_ptrace ()
 {
   long unsigned int D.9415;
@@ -367,12 +375,8 @@
   abort ();

 <bb 44>:
-  job_64->goal = 1;
-  job_64->state = 3;
   D.9269_65 = job_64->pid;
   *D.9269_65 = pid_49;
-  job_64->trace_forks = 0;
-  job_64->trace_state = 3;

 <bb 45>:
   __ret_66 = nih_str_array_new (0B);

[Bug middle-end/56301] [4.7 Regression] wrong code with the fix for PR53844

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56301

--- Comment #3 from Richard Biener <rguenth at gcc dot gnu.org> 2013-02-18 12:48:13 UTC ---
I don't see how this is a bug.

  job_64 = job_new (class_39, "");
  # DEBUG job => job_64
  if (job_64 == 0B)
    goto <bb 43>;
  else
    goto <bb 44>;

<bb 43>:
  # DEBUG __fmt => "BAD: wrong value for %s, got unexpected %p\n\tat %s:%d
(%s).\n"
  __printf_chk (1, "BAD: wrong value for %s, got unexpected %p\n\tat %s:%d
(%s).\n", "job", 0B, "tests/test_job.c", 111, &__FUNCTION__);
  abort ();

<bb 44>:
  D.9269_65 = job_64->pid;
  *D.9269_65 = pid_49;

that was the last use of the memory pointed to by job_64 (job_new is marked as
malloc).  In fact, job_64->pid is uninitialized!  To quote the documentation
of the malloc attribute:

@item malloc
@cindex @code{malloc} attribute
The @code{malloc} attribute is used to tell the compiler that a function
may be treated as if any non-@code{NULL} pointer it returns cannot
alias any other pointer valid when the function returns and that the memory
has undefined content.
This will often improve optimization.
Standard functions with this property include @code{malloc} and
@code{calloc}.  @code{realloc}-like functions do not have this
property as the memory pointed to does not have undefined content.

it appears that the returned memory does not have undefined contents.

[Bug middle-end/56301] [4.7 Regression] wrong code with the fix for PR53844

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56301

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

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

--- Comment #4 from Richard Biener <rguenth at gcc dot gnu.org> 2013-02-18 12:48:48 UTC ---
Thus, invalid.

[Bug middle-end/56301] [4.7 Regression] wrong code with the fix for PR53844

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56301

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

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

--- Comment #5 from Jakub Jelinek <jakub at gcc dot gnu.org> 2013-02-18 13:31:28 UTC ---
Well, perhaps we need to improve documentation, because for calloc the memory
doesn't have undefined contents either, it is well defined to be all zeros.

[Bug middle-end/56301] [4.7 Regression] wrong code with the fix for PR53844

[rguenther at suse dot de]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56301

--- Comment #6 from rguenther at suse dot de <rguenther at suse dot de> 2013-02-18 13:34:31 UTC ---
On Mon, 18 Feb 2013, jakub at gcc dot gnu.org wrote:

> 
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56301
> 
> Jakub Jelinek <jakub at gcc dot gnu.org> changed:
> 
>            What    |Removed                     |Added
> ----------------------------------------------------------------------------
>                  CC|                            |jakub at gcc dot gnu.org
> 
> --- Comment #5 from Jakub Jelinek <jakub at gcc dot gnu.org> 2013-02-18 13:31:28 UTC ---
> Well, perhaps we need to improve documentation, because for calloc the memory
> doesn't have undefined contents either, it is well defined to be all zeros.

Well, it points to nothing ;)  The bug here is that probably
job_new links the allocated memory into some global list or so,
so it's not about initializing the memory but the fact that it
_is_ aliased by other things.

Yes, we can probably give a few examples of what is not appropriate
use of 'malloc'.

Do you think I should revert the patch on the branch nevertheless?
(it was a fix for a missed-optimization regression only ...)

[Bug middle-end/56301] [4.7 Regression] wrong code with the fix for PR53844

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56301

--- Comment #7 from Jakub Jelinek <jakub at gcc dot gnu.org> 2013-02-18 13:42:24 UTC ---
(In reply to comment #6)
> Do you think I should revert the patch on the branch nevertheless?
> (it was a fix for a missed-optimization regression only ...)

Yeah, missed-optimization regression can wait for 4.8, but just the
tree-ssa-dse.c part + related testcase, not all the other fixes.

[Bug middle-end/56301] [4.7 Regression] wrong code with the fix for PR53844

[doko at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56301

--- Comment #8 from Matthias Klose <doko at gcc dot gnu.org> 2013-02-19 13:17:55 UTC ---
> so it's not about initializing the memory but the fact that it
> _is_ aliased by other things.

yes, the value returned was saved in a caching data structure.