diff mbox series

Fortran: Skip bound conv in gfc_conv_gfc_desc_to_cfi_desc with intent(out) ptr [PR108621]

Message ID 07be8524-0755-6b77-49bd-af5c688404d5@codesourcery.com
State New
Headers show
Series Fortran: Skip bound conv in gfc_conv_gfc_desc_to_cfi_desc with intent(out) ptr [PR108621] | expand

Commit Message

Tobias Burnus Feb. 24, 2023, 11:31 a.m. UTC
[The following is about Fortran pointers as actual argument to a CFI taking procedure.]

The issue has been marked as 12/13 regression but the issue is just a diagnostic one.

To disentangle:

(A) Bogus warning
[Now tracked as middle-end https://gcc.gnu.org/PR108906 ]
Assume:
    nullify(p)
    call bind_c_proc(p)

For some reasons, the compiler does not propagate the NULL and thus assumes that
if (addr != NULL) could be true. This leads to a may-be-uninit warning with '-Wall -O0'.
(And no warning with -Og/-Os/-O1.)

We could silence it on the gfortran side, I think, by tweaking some tree properties,
but I am not sure we want to to it.

(The same kind of code is in GCC 11 but as it is hidden in libgfortran; hence, there are
no warnings when compiling user code. Since GCC 12, the compiler does the conversion
in place when generating the code.)



(B) The attached patch:

With 'intent(out)' there is no reason to do the conversions. While for nullified
pointers the bounds-conversion loop is skipped, it may still be executed for undefined
pointers. (Which is usually harmless.) In either case, not generating this code makes
sense.

OK for mainline?

Regarding GCC 12:  I am not really sure as it is no real regression. Besides bogus
warnings, there might be an issue for undefined pointers and -fsanitize=undefined, namely
if 'ubound - lbound' evaluated on random numbers overflows (such as for ubound = huge(..)
and lbound = -huge(..)). But that looks like a rather special case. - Thoughts?

Tobias
-----------------
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955

Comments

Harald Anlauf Feb. 24, 2023, 9:25 p.m. UTC | #1
Hi Tobias,

Am 24.02.23 um 12:31 schrieb Tobias Burnus:
> (B) The attached patch:
> 
> With 'intent(out)' there is no reason to do the conversions. While for 
> nullified
> pointers the bounds-conversion loop is skipped, it may still be executed 
> for undefined
> pointers. (Which is usually harmless.) In either case, not generating 
> this code makes
> sense.
> 
> OK for mainline?

LGTM.

I was pondering whether one should keep the testcase closer to the
one in the PR, but the essence of the bug and the fix is well
represented in the reduced version, and also the tree dump tells
the whole story anyway.

> Regarding GCC 12:  I am not really sure as it is no real regression. 
> Besides bogus
> warnings, there might be an issue for undefined pointers and 
> -fsanitize=undefined, namely
> if 'ubound - lbound' evaluated on random numbers overflows (such as for 
> ubound = huge(..)
> and lbound = -huge(..)). But that looks like a rather special case. - 
> Thoughts?

I'd rather consider the case of undefined pointers as of practical
importance.  It's up to you or others to decide whether it should
be backported.  I would not oppose.

Thanks for the patch!

Harald

> Tobias
> -----------------
> Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 
> 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: 
> Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; 
> Registergericht München, HRB 106955
diff mbox series

Patch

Fortran: Skip bound conv in gfc_conv_gfc_desc_to_cfi_desc with intent(out) ptr [PR108621]

When the dummy argument of the bind(C) proc is 'pointer, intent(out)', the conversion
of the GFC to the CFI bounds can be skipped: it is not needed and avoids issues with
noninit memory.


Note that the 'cfi->base_addr = gfc->addr' assignment is kept as the C code of a user
might assume that a nullified pointer arrives as NULL (or even a specific value).
For instance, gfortran.dg/c-interop/section-{1,2}.f90 assumes the value NULL.

Note 2: The PR is about a may-be-uninitialized warning with intent(out). In the PR's
testcase, the pointer was nullified and should not have produced that warning.
That is a diagnostic issue, now tracked as PR middle-end/108906 as the issue in principle
still exists (e.g. with 'intent(inout)'). [But no longer for intent(out).]

Note 3: With undefined pointers and no 'intent', accessing uninit memory is unavoidable
on the caller side as the compiler cannot know what the C function does (but this usage
determines whether the pointer is permitted be undefined or whether the bounds must be
gfc-to-cfi converted).


gcc/fortran/ChangeLog:

	PR fortran/108621
	* trans-expr.cc (gfc_conv_gfc_desc_to_cfi_desc):

gcc/testsuite/ChangeLog:

	PR fortran/108621
	* gfortran.dg/c-interop/fc-descriptor-pr108621.f90: New test.

 gcc/fortran/trans-expr.cc                          |  6 ++
 .../c-interop/fc-descriptor-pr108621.f90           | 65 ++++++++++++++++++++++
 2 files changed, 71 insertions(+)

diff --git a/gcc/fortran/trans-expr.cc b/gcc/fortran/trans-expr.cc
index e85b53fae85..045c8b00b90 100644
--- a/gcc/fortran/trans-expr.cc
+++ b/gcc/fortran/trans-expr.cc
@@ -5673,6 +5673,9 @@  gfc_conv_gfc_desc_to_cfi_desc (gfc_se *parmse, gfc_expr *e, gfc_symbol *fsym)
   gfc_add_modify (&block, tmp,
 		  build_int_cst (TREE_TYPE (tmp), attr));
 
+  /* The cfi-base_addr assignment could be skipped for 'pointer, intent(out)'.
+     That is very sensible for undefined pointers, but the C code might assume
+     that the pointer retains the value, in particular, if it was NULL.  */
   if (e->rank == 0)
     {
       tmp = gfc_get_cfi_desc_base_addr (cfi);
@@ -5695,6 +5698,9 @@  gfc_conv_gfc_desc_to_cfi_desc (gfc_se *parmse, gfc_expr *e, gfc_symbol *fsym)
       gfc_add_modify (&block, tmp2, fold_convert (TREE_TYPE (tmp2), tmp));
     }
 
+  if (fsym->attr.pointer && fsym->attr.intent == INTENT_OUT)
+    goto done;
+
   /* When allocatable + intent out, free the cfi descriptor.  */
   if (fsym->attr.allocatable && fsym->attr.intent == INTENT_OUT)
     {
diff --git a/gcc/testsuite/gfortran.dg/c-interop/fc-descriptor-pr108621.f90 b/gcc/testsuite/gfortran.dg/c-interop/fc-descriptor-pr108621.f90
new file mode 100644
index 00000000000..9c9062bd62d
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/c-interop/fc-descriptor-pr108621.f90
@@ -0,0 +1,65 @@ 
+! { dg-do compile }
+! { dg-additional-options "-fdump-tree-original" }
+!
+! PR fortran/108621
+!
+! If the bind(C) procedure's dummy argument is a POINTER with INTENT(OUT),
+! avoid converting the array bounds for the CFI descriptor before the call.
+!
+! Rational: Fewer code and, esp. for undefined pointers, there might be a
+! compile-time warning or a runtime error due to the 'extent' arithmentic
+! and integer overflows (i.e. random values and -fsanitize=undefined).
+!
+! (For disassociated pointers, it would/should be only pointless code as
+! the bound setting is guarded by a != NULL condtion. However, as the PR shows,
+! a bogus may-use-uninitialized-memory warning might still be shown in that case.)
+!
+! Without 'intent' (but still intent(out) internally), the same applies but
+! there is nothing the compiler can do on the caller side.
+! Still, as only uninit memory and not invalid memory it accessed, it should still
+! work (at least when run-time checking is turned off).
+!
+subroutine demo(f)
+use, intrinsic :: iso_c_binding, only : c_int
+implicit none
+
+interface
+  subroutine fun(f_p) bind(c)
+    import c_int
+    integer(c_int), pointer, intent(out) :: f_p(:)
+  end subroutine
+end interface
+
+integer(c_int), pointer :: f(:)
+
+call fun(f)
+end
+
+! The following ones must be present even with intent(out):
+!
+! { dg-final { scan-tree-dump "cfi...version = 1;" "original" } }
+! { dg-final { scan-tree-dump "cfi...rank = 1;" "original" } }
+! { dg-final { scan-tree-dump "cfi...type = 1025;" "original" } }
+! { dg-final { scan-tree-dump "cfi...attribute = 0;" "original" } }
+! { dg-final { scan-tree-dump "cfi...elem_len = 4;" "original" } }
+
+
+! The following is not needed - but user code might expect that an incoming pointer is NULL
+! in this case. - At least the GCC testsuite expects this in the C code at
+!   gfortran.dg/c-interop/section-{1,2}.f90 
+! Thus, it is kept as it does not cause any harm:
+!
+! { dg-final { scan-tree-dump "cfi...base_addr = f->data;" "original" } }
+
+
+! The following ones are not need with intent(out) and, therefore, shouldn't be there:
+!
+!     cfi.0.dim[idx.1].lower_bound = f->dim[idx.1].lbound;
+!     cfi.0.dim[idx.1].extent = (f->dim[idx.1].ubound - f->dim[idx.1].lbound) + 1;
+!     cfi.0.dim[idx.1].sm = f->dim[idx.1].stride * f->span;
+!
+! Now match those - but using a rather generic pattern as it is a ...-not scan:
+!
+! { dg-final { scan-tree-dump-not "lower_bound = " "original" } }
+! { dg-final { scan-tree-dump-not "extent = " "original" } }
+! { dg-final { scan-tree-dump-not "sm = " "original" } }