| Message ID | 5ae29f94-956a-9c0c-20d7-241cee370162@netcologne.de |
|---|---|
| State | New |
| Headers | show |
| Series | [fortran] Fix pointers not escaping via C_PTR | expand |
On Thu, Feb 28, 2019 at 09:14:48PM +0100, Thomas Koenig wrote: > > the attached patch fixes a wrong-code bug for gfortran where pointers > were not marked as escaping. A C_PTR can be stashed away and reused > later (at least as long as the variable it points to remains active). > > This is not a regression, but IMHO a bad wrong-code bug. The chances > of this patch introducing regressions are really, really low. > > Regression-tested. OK for trunk? > Is this a wrong code bug or a clever user wandering off into undefined behavior? > subroutine init() > use, intrinsic :: iso_c_binding > implicit none > integer(c_int), pointer :: a > allocate(a) > call save_cptr(c_loc(a)) > a = 100 > end subroutine init Of particular note, 'a' is an unsaved local variable. When init() returns, 'a' goes out-of-scope. Fortran normally deallocates an unsaved allocatable entity, but 'a' is an unsaved local variable with the pointer attribute. For lack of a better term, 'allocate(a)' allocates an anonymous target and associates the anonymous target with the pointer 'a'. save_ptr() caches the address of the anonymous target. When init() returns does the anonymous target get deallocated or does the program leak memory? In addition, when init() returns, what happens to the pointer association of 'a'? I think it becomes undefined, which means the anonymous memory is inaccessible at least by Fortran means as there are no pointers associated with the anonymous target. The Fortran standard does not what happens to leaked memory. In particular, the Fortran standard does not guarantee that leaked memory is not reaped by some garbage collection or that it must retain value 100.
Hi Steve, > On Thu, Feb 28, 2019 at 09:14:48PM +0100, Thomas Koenig wrote: >> >> the attached patch fixes a wrong-code bug for gfortran where pointers >> were not marked as escaping. A C_PTR can be stashed away and reused >> later (at least as long as the variable it points to remains active). >> >> This is not a regression, but IMHO a bad wrong-code bug. The chances >> of this patch introducing regressions are really, really low. >> >> Regression-tested. OK for trunk? >> > > Is this a wrong code bug or a clever user wandering off into > undefined behavior? > >> subroutine init() >> use, intrinsic :: iso_c_binding >> implicit none >> integer(c_int), pointer :: a >> allocate(a) >> call save_cptr(c_loc(a)) >> a = 100 >> end subroutine init > > Of particular note, 'a' is an unsaved local variable. When init() > returns, 'a' goes out-of-scope. Fortran normally deallocates > an unsaved allocatable entity, but 'a' is an unsaved local variable > with the pointer attribute. For lack of a better term, 'allocate(a)' > allocates an anonymous target and associates the anonymous target > with the pointer 'a'. save_ptr() caches the address of the anonymous > target. When init() returns does the anonymous target get deallocated > or does the program leak memory? Neither. If there were no C_PTR pointing to the memory, it would leak memory. > In addition, when init() returns, > what happens to the pointer association of 'a'? 'a' becomes undefined, hence it is no longer associated with the memory. > I think it becomes > undefined, which means the anonymous memory is inaccessible at least > by Fortran means as there are no pointers associated with the > anonymous target. That is not correct. Looking at F2018, 18.2.3.3, by using C_F_POINTER, you can " Associate a data pointer with the target of a C pointer and specify its shape. " First, this talks about a C pointer having a target. Second, you can re-estabilsh the association to a different pointer to the Fortran program. Regards Thomas
I wrote: > First, this talks about a C pointer having a target. Second, you can > re-estabilsh the association to a different pointer to the Fortran > program. There is another point to consider: This is interoperability with C we are dealing with, so we also have to follow C semantics. And, love it or hate it, C pointers escape. So, OK for trunk? Regards Thomas
On 2/28/19 12:14 PM, Thomas Koenig wrote: > Hello world, > > the attached patch fixes a wrong-code bug for gfortran where pointers > were not marked as escaping. A C_PTR can be stashed away and reused > later (at least as long as the variable it points to remains active). > > This is not a regression, but IMHO a bad wrong-code bug. The chances > of this patch introducing regressions are really, really low. > > Regression-tested. OK for trunk? > This is Ok except the test case. You are using dg-run but are not testing for any condition. I think you want to to IF .... STOP instead of printing some test values or dou you just want dg-conpile? Fix test case and OK for trunk. Jerry
Hi Jerry, > I think you want to to IF .... STOP instead of printing some test values > or dou you just want dg-conpile? You're correct, I wanted a dg-do run. Here is the version of the test case that I committed. Thanks for the review (and the other one). Regards Thomas ! { dg-do run } ! PR 71544 - this failed with some optimization options due to a ! pointer not being marked as escaping. module store_cptr use, intrinsic :: iso_c_binding implicit none public type(c_ptr), save :: cptr end module store_cptr subroutine init() use, intrinsic :: iso_c_binding implicit none integer(c_int), pointer :: a allocate(a) call save_cptr(c_loc(a)) a = 100 end subroutine init subroutine save_cptr(cptr_in) use store_cptr implicit none type(c_ptr), intent(in) :: cptr_in cptr = cptr_in end subroutine save_cptr program init_fails use store_cptr implicit none integer(c_int), pointer :: val call init() call c_f_pointer(cptr,val) if (val /= 100) stop 1 end program init_fails
Index: trans-types.c =================================================================== --- trans-types.c (Revision 269260) +++ trans-types.c (Arbeitskopie) @@ -1176,7 +1176,8 @@ gfc_typenode_for_spec (gfc_typespec * spec, int co { spec->type = BT_INTEGER; spec->kind = gfc_index_integer_kind; - spec->f90_type = BT_VOID; + spec->f90_type = BT_VOID; + spec->is_c_interop = 1; /* Mark as escaping later. */ } break; case BT_VOID: @@ -2957,7 +2958,8 @@ create_fn_spec (gfc_symbol *sym, tree fntype) || f->sym->ts.u.derived->attr.pointer_comp)) || (f->sym->ts.type == BT_CLASS && (CLASS_DATA (f->sym)->ts.u.derived->attr.proc_pointer_comp - || CLASS_DATA (f->sym)->ts.u.derived->attr.pointer_comp))) + || CLASS_DATA (f->sym)->ts.u.derived->attr.pointer_comp)) + || (f->sym->ts.type == BT_INTEGER && f->sym->ts.is_c_interop)) spec[spec_len++] = '.'; else if (f->sym->attr.intent == INTENT_IN) spec[spec_len++] = 'r';
Hello world, the attached patch fixes a wrong-code bug for gfortran where pointers were not marked as escaping. A C_PTR can be stashed away and reused later (at least as long as the variable it points to remains active). This is not a regression, but IMHO a bad wrong-code bug. The chances of this patch introducing regressions are really, really low. Regression-tested. OK for trunk? Regards Thomas 2019-02-29 Thomas Koenig <tkoenig@gcc.gnu.org> PR fortran/71544 * trans-types.c (gfc_typenode_for_spec) Set ts->is_c_interop of C_PTR and C_FUNPTR. (create_fn_spec): Mark argument as escaping if ts->is_c_interop is set. 2019-02-29 Thomas Koenig <tkoenig@gcc.gnu.org> PR fortran/71544 * gfortran.dg/c_ptr_tests_19.f90: New test.