wrong usage of the `malloc' function attribute

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.

Created attachment 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);

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);

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.

Thus, invalid.

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.

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 ...)

(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.

> 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.