Bug 51529 - [OOP] gfortran.dg/class_to_type_1.f03 is miscompiled: Uninitialized variable used
Summary: [OOP] gfortran.dg/class_to_type_1.f03 is miscompiled: Uninitialized variable ...
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: fortran (show other bugs)
Version: 4.7.0
: P3 normal
Target Milestone: 4.7.0
Assignee: Not yet assigned to anyone
URL:
Keywords: wrong-code
Depends on:
Blocks:
 
Reported: 2011-12-13 16:33 UTC by Tobias Burnus
Modified: 2016-11-16 14:26 UTC (History)
3 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Tobias Burnus 2011-12-13 16:33:26 UTC
$ gfortran -g gfortran.dg/class_to_type_1.f03
$ MALLOC_PERTURB_= ./a.out
$ MALLOC_PERTURB_=33 ./a.out

A fatal error occurred! Backtrace for this error:
...
#6  0x400929 in __copy_MAIN___T2 at class_to_type_1.f03:5
#7  0x4013D1 in MAIN__ at class_to_type_1.f03:17


That's a typical sign for code which requires that "malloc" returns '\0'-set memory, i.e. gfortran generates code which uses an uninitialized variable.
Comment 1 Paul Thomas 2011-12-13 18:14:49 UTC
(In reply to comment #0)
> $ gfortran -g gfortran.dg/class_to_type_1.f03
> $ MALLOC_PERTURB_= ./a.out
> $ MALLOC_PERTURB_=33 ./a.out
> 
> A fatal error occurred! Backtrace for this error:
> ...
> #6  0x400929 in __copy_MAIN___T2 at class_to_type_1.f03:5
> #7  0x4013D1 in MAIN__ at class_to_type_1.f03:17
> 
> 
> That's a typical sign for code which requires that "malloc" returns '\0'-set
> memory, i.e. gfortran generates code which uses an uninitialized variable.

I cannot reproduce this on x86_64/FC9.  Nor can I see anything obvious in the code.... on the other hand, there is a lot of it!

I'll try on my Debian equipped laptop tomorrow.

Paul
Comment 2 Tobias Burnus 2011-12-13 22:18:44 UTC
The problem is the following, for:
  allocate(t2 :: x(10))
one has:
  x._data.data = (void * restrict) __builtin_malloc (640);

I had now expected that one would do:
  for (i = 1; i <= 10; i++)
    __builtin_memcpy ((void *) x._data,
                      (void *) &__vtab_MAIN___T2._def_init,
                      __vtab_MAIN___T2._size)

However, the current code does:
  struct t2 D.1921;
  struct t2 t2.2;
  integer(kind=8) D.1919;
  struct t[0:] * restrict D.1918;
  D.1918 = (struct t[0:] * restrict) x._data.data;
  D.1919 = x._data.offset;
  t2.2.t.b.data = 0B;
  t2.2.z = __complex__ (3.2999999523162841796875e+0, 4.400000095367431640625e+0);
  D.1921 = t2.2;
So far so good. That's the same as __vtab_MAIN___T2._def_init. Disadvantage: Code duplication. Advantage: The information in in the same file (translation unit).

However, instead of just doing a __builtin_memcpy in a loop, one calls:
  __vtab_MAIN___T2._copy (&D.1921,
                          (struct t *) D.1918
                          + (sizetype) ((S.3 + D.1919) * (integer(kind=8))
                          x._vptr->_size));

This has several disadvantages: First, makes the advantage of having all data in the same translation unit void as one calls a function, located in another translation unit. It is also much slower as _copy does many checks which we know shouldn't matter. For MOLD= or a type-spec we know that the destination does not have allocated allocatable components.


However, I do now understand why one needs for SOURCE= to memset the source to NULL - at least as long _copy not only copies the data but also frees it. The latter could be also left to _free. - Actually, I am in favour of separating _copy and _free. As this issue shows, there are cases where one does not want to combine them, leading to work around actions (memset). I think only for polymorphic assignment, one needs _free + _copy, for allocate with SOURCE= a _copy should be enough.


The reason for the crash is:

__copy_MAIN___T2 (struct t2 & restrict src, struct t2 & restrict dst)
{
  if (dst->t.b.data != 0B)
      __builtin_free ((void *) dst->t.b.data);

where dst == x._data.data, where the latter and thus also x._data.data->t.b.data is filled with random memory.
Comment 3 paul.richard.thomas@gmail.com 2011-12-14 08:55:08 UTC
Dear Tobias,

> However, I do now understand why one needs for SOURCE= to memset the source to
> NULL - at least as long _copy not only copies the data but also frees it. The
> latter could be also left to _free. - Actually, I am in favour of separating
> _copy and _free. As this issue shows, there are cases where one does not want
> to combine them, leading to work around actions (memset). I think only for
> polymorphic assignment, one needs _free + _copy, for allocate with SOURCE= a
> _copy should be enough.

The memset came about for similar reasons with class objects with
allocatable components.  I had missed this wrinkle with the testcase.
My inclination is to restire the memset and keep the PR open.

I am am trying to clear up some issues of functionality, starting with
the failure of a%disp() to scalarize properly in class_array_3.f03.
Then I have mind to understand the failure of vector indexing in
gfc_trans_call and finally to deal with class array components and
class array subreferences.  After the tidy up :-)

Cheers

Paul
Comment 4 Tobias Burnus 2011-12-14 11:18:21 UTC
(In reply to comment #3)
> The memset came about for similar reasons with class objects with
> allocatable components.  I had missed this wrinkle with the testcase.
> My inclination is to restire the memset and keep the PR open.

I am fine with it, but state in the comment above the memset that it is needed as _copy tries to free the allocatable components of "dest" - that makes clear why the memset is need for our implementation.
Comment 5 John David Anglin 2011-12-19 00:21:44 UTC
On hppa2.0w-hp-hpux11.11, the test fails but the backtrace also fails:

A fatal error occurred! Backtrace for this error:#0  0xC1B39FE3FAIL: gfortran.dg/class_to_type_1.f03  -O0  execution test
Comment 6 Paul Thomas 2012-01-02 12:46:14 UTC
Author: pault
Date: Mon Jan  2 12:46:08 2012
New Revision: 182796

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=182796
Log:
2012-01-02  Paul Thomas  <pault@gcc.gnu.org>

	PR fortran/51529
	* trans-array.c (gfc_array_allocate): Null allocated memory of
	newly allocted class arrays.

	PR fortran/46262
	PR fortran/46328
	PR fortran/51052
	* interface.c(build_compcall_for_operator): Add a type to the
	expression.
	* trans-expr.c (conv_base_obj_fcn_val): New function.
	(gfc_conv_procedure_call): Use base_expr to detect non-variable
	base objects and, ensuring that there is a temporary variable,
	build up the typebound call using conv_base_obj_fcn_val.
	(gfc_trans_class_assign): Pick out class procedure pointer
	assignments and do the assignment with no further prcessing.
	(gfc_trans_class_array_init_assign, gfc_trans_class_init_assign
	gfc_trans_class_assign): Move to top of file.
	* gfortran.h : Add 'base_expr' field to gfc_expr.
	* resolve.c (get_declared_from_expr): Add 'types' argument to
	switch checking of derived types on or off.
	(resolve_typebound_generic_call): Set the new argument.
	(resolve_typebound_function, resolve_typebound_subroutine):
	Set 'types' argument for get_declared_from_expr appropriately.
	Identify base expression, if not a variable, in the argument
	list of class valued calls. Assign it to the 'base_expr' field
	of the final expression. Strip away all references after the
	last class reference.


2012-01-02  Paul Thomas  <pault@gcc.gnu.org>

	PR fortran/46262
	PR fortran/46328
	PR fortran/51052
	* gfortran.dg/typebound_operator_7.f03: New.
	* gfortran.dg/typebound_operator_8.f03: New.

Added:
    trunk/gcc/testsuite/gfortran.dg/typebound_operator_7.f03
    trunk/gcc/testsuite/gfortran.dg/typebound_operator_8.f03
Modified:
    trunk/gcc/fortran/ChangeLog
    trunk/gcc/fortran/dump-parse-tree.c
    trunk/gcc/fortran/gfortran.h
    trunk/gcc/fortran/interface.c
    trunk/gcc/fortran/resolve.c
    trunk/gcc/fortran/trans-array.c
    trunk/gcc/fortran/trans-expr.c
    trunk/gcc/testsuite/ChangeLog
Comment 7 Paul Thomas 2012-01-02 13:04:43 UTC
Fixed on trunk.

Thanks for the reports

Paul