diff mbox series

[fortran] PR fortran/92621 Problems with memory handling with allocatable intent(out) arrays with bind(c)

Message ID cefb4ae0-17db-5a63-fca1-f43b023dbcee@gmail.com
State New
Headers show
Series [fortran] PR fortran/92621 Problems with memory handling with allocatable intent(out) arrays with bind(c) | expand

Commit Message

José Rui Faustino de Sousa Feb. 21, 2020, 12:56 p.m. UTC 
Hi all!

Proposed patch to solve problems with memory handling with allocatable 
intent(out) arrays with bind(c).

The patch also seems to affect PR92189.

Patch tested only on x86_64-pc-linux-gnu.

The code currently generated tries to deallocate the artificial cfi.n 
pointer before it is associated with the allocatable array.

Since the cfi.n pointer is uninitialized in some infrequent situations 
(using -static-libgfortran seems to do the trick) the pointer seems to 
contain garbage and a segmentation fault is generated.

Since the deallocation is done prior to the cfi.n pointer being 
associated with the allocatable array the memory is never freed and the 
array will be passed still allocated and consequently attempts to 
allocate it will fail.

A diff of only the main code changes without spacing changes is attached 
to facilitate human reviewing.

Thank you very much.

Best regards,
José Rui

2020-2-21  José Rui Faustino de Sousa  <jrfsousa@gmail.com>

  PR fortran/92621
  * trans-expr.c (gfc_conv_gfc_desc_to_cfi_desc): Add code to deallocate
  allocatable intent(out) dummy array arguments, slightly rearrange code.
  (gfc_conv_procedure_call): Split if conditional in two branches removes
  unnecessary checks for is_bind_c and obsolete comments from second
  branch.

2020-02-21  José Rui Faustino de Sousa  <jrfsousa@gmail.com>

  PR fortran/92621
  * bind-c-intent-out.f90: Changes dg-do compile to run, changes regex to
  match the changes in code generation.

2020-02-21  José Rui Faustino de Sousa  <jrfsousa@gmail.com>

  PR fortran/92621
  * PR92621.f90: New test.


+! { dg-final { scan-tree-dump-times "__builtin_free 
\\(cfi\\.\[0-9\]+\\);" 1 "original" } }
+! { dg-final { scan-tree-dump-times "\\(integer\\(kind\\=4\\)\\\[0:\\\] 
\\* restrict\\) a\\.data = 0B;" 1 "original" } }
+! { dg-final { scan-tree-dump-times "cfi\\.\[0-9\]+ = 0B;" 1 "original" } }

Comments

Tobias Burnus Feb. 21, 2020, 1:38 p.m. UTC | #1 
On 2/21/20 1:56 PM, José Rui Faustino de Sousa wrote:
> Since the cfi.n pointer is uninitialized in some infrequent situations 
> (using -static-libgfortran seems to do the trick) the pointer seems to 
> contain garbage and a segmentation fault is generated

Hmm, that sounds like papering over a real bug. At a glance, I do not 
see anything in the test case which is undefined behaviour – and if it 
is not undefined behaviour, it should work with all optimization options 
and with libgfortran linked both statically and dynamically.

Tobias

PS: I am woefully aware that there a still several patches to be reviewed.
José Rui Faustino de Sousa Feb. 21, 2020, 8:05 p.m. UTC | #2 
On 21/02/20 12:38, Tobias Burnus wrote:
> Hmm, that sounds like papering over a real bug. 
> 

It is possible, I tried using DECL_INITIAL to nullify cfi.n but it did 
not made any difference.

I tried to play with optimization and up to -O1 it does not seem to 
crash but it always seems to crash at -O2 (without the 
-static-libgfortran switch). Looking at the generated code I could not 
see anything obviously different...

Before the patch to PR92123 there were situations where there would be 
off bounds memory writes, but that would cause a crash at a later time.

I have no idea of what it could be...

Best regards,
José Rui
diff mbox series

Patch

diff --git a/gcc/fortran/trans-expr.c b/gcc/fortran/trans-expr.c
index 5825a4b..70dd9be 100644
--- a/gcc/fortran/trans-expr.c
+++ b/gcc/fortran/trans-expr.c
@@ -5248,6 +5248,39 @@  gfc_conv_gfc_desc_to_cfi_desc (gfc_se *parmse, gfc_expr *e, gfc_symbol *fsym)
       if (POINTER_TYPE_P (TREE_TYPE (parmse->expr)))
 	parmse->expr = build_fold_indirect_ref_loc (input_location,
 						    parmse->expr);
+    }
+  else
+    gfc_conv_expr (parmse, e);
+
+  if (POINTER_TYPE_P (TREE_TYPE (parmse->expr)))
+    parmse->expr = build_fold_indirect_ref_loc (input_location,
+						parmse->expr);
+
+  /* If an ALLOCATABLE dummy argument has INTENT(OUT) and is
+     allocated on entry, it must be deallocated.  */
+  if (fsym && fsym->attr.allocatable
+      && fsym->attr.intent == INTENT_OUT)
+    {
+      tmp = parmse->expr;
+
+      if (GFC_DESCRIPTOR_TYPE_P (TREE_TYPE (tmp)))
+	tmp = gfc_conv_descriptor_data_get (tmp);
+      tmp = gfc_deallocate_with_status (tmp, NULL_TREE, NULL_TREE,
+					NULL_TREE, NULL_TREE, true,
+					e,
+					GFC_CAF_COARRAY_NOCOARRAY);
+      if (fsym->attr.optional
+	  && e->expr_type == EXPR_VARIABLE
+	  && e->symtree->n.sym->attr.optional)
+	tmp = fold_build3_loc (input_location, COND_EXPR,
+			       void_type_node,
+			       gfc_conv_expr_present (e->symtree->n.sym),
+			       tmp, build_empty_stmt (input_location));
+      gfc_add_expr_to_block (&parmse->pre, tmp);
+    }
+	      
+  if (e->rank != 0)
+    {
       bool is_artificial = (INDIRECT_REF_P (parmse->expr)
 			    ? DECL_ARTIFICIAL (TREE_OPERAND (parmse->expr, 0))
 			    : DECL_ARTIFICIAL (parmse->expr));
@@ -5293,16 +5326,8 @@  gfc_conv_gfc_desc_to_cfi_desc (gfc_se *parmse, gfc_expr *e, gfc_symbol *fsym)
 	}
     }
   else
-    {
-      gfc_conv_expr (parmse, e);
-
-      if (POINTER_TYPE_P (TREE_TYPE (parmse->expr)))
-	parmse->expr = build_fold_indirect_ref_loc (input_location,
-						    parmse->expr);
-
     parmse->expr = gfc_conv_scalar_to_descriptor (parmse,
 						  parmse->expr, attr);
-    }
 
   /* Set the CFI attribute field through a temporary value for the
      gfc attribute.  */
@@ -6170,7 +6195,9 @@  gfc_conv_procedure_call (gfc_se * se, gfc_symbol * sym,
 		/* Implement F2018, C.12.6.1: paragraph (2).  */
 		gfc_conv_gfc_desc_to_cfi_desc (&parmse, e, fsym);
 
-	      else if (e->expr_type == EXPR_VARIABLE
+	      else
+		{
+		  if (e->expr_type == EXPR_VARIABLE
 		      && is_subref_array (e)
 		      && !(fsym && fsym->attr.pointer))
 		    /* The actual argument is a component reference to an
@@ -6219,7 +6246,7 @@  gfc_conv_procedure_call (gfc_se * se, gfc_symbol * sym,
 		  /* Unallocated allocatable arrays and unassociated pointer arrays
 		     need their dtype setting if they are argument associated with
 		     assumed rank dummies.  */
-	      if (!sym->attr.is_bind_c && e && fsym && fsym->as
+		  if (e && fsym && fsym->as
 		      && fsym->as->type == AS_ASSUMED_RANK)
 		    {
 		      if (gfc_expr_attr (e).pointer
@@ -6256,10 +6283,7 @@  gfc_conv_procedure_call (gfc_se * se, gfc_symbol * sym,
 			}
 
 		      tmp = parmse.expr;
-		  /* With bind(C), the actual argument is replaced by a bind-C
-		     descriptor; in this case, the data component arrives here,
-		     which shall not be dereferenced, but still freed and
-		     nullified.  */
+
 		      if  (TREE_TYPE(tmp) != pvoid_type_node)
 			tmp = build_fold_indirect_ref_loc (input_location,
 							   parmse.expr);
@@ -6280,6 +6304,7 @@  gfc_conv_procedure_call (gfc_se * se, gfc_symbol * sym,
 		    }
 		}
 	    }
+	}
 
       /* The case with fsym->attr.optional is that of a user subroutine
 	 with an interface indicating an optional argument.  When we call