| Message ID | 20150111162105.6fb725dc@vepi2 |
|---|---|
| State | New |
| Headers | show |
Hi Andre, + if (INDIRECT_REF_P (parmse.string_length)) + /* In chains of functions/procedure calls the string_length already + is a pointer to the variable holding the length. Therefore + remove the deref on call. */ + parmse.string_length = TREE_OPERAND (parmse.string_length, 0); This is OK but I would use instead: + if (POINTER_TYPE_P (parmse.string_length)) + /* In chains of functions/procedure calls the string_length already + is a pointer to the variable holding the length. Therefore + remove the deref on call. */ + parmse.string_length = build_fold_indirect_ref (parmse.string_length); If you look in ~/gcc/fold-const.c:15751, you will see that TREE_OPERAND (parmse.string_length, 0) but that it is preceded by cleaning up of NOOPS and, in any case, its usage will preserve the standard API.... just in case the internals change :-) of course, using TREE_OPERAND (xxx, 0) in the various fortran class functions makes such an assumption ;-) Apart from that, the patch is fine. I'll have a session of doing some commits later this week and will do this patch at that time. Cheers Paul On 11 January 2015 at 16:21, Andre Vehreschild <vehre@gmx.de> wrote: > Hi Paul, > > thanks for the review. I do not have commits rights. > > Unfortunately is the patch not ok. I figured today, that it needs an extension > when function calls that return deferred char len arrays are nested. In this > special case the string length would have been lost. The attached extended > version fixes this issue. > > Sorry for the duplicate work. > > Bootstraps and regtests ok on x86_64-linux-gnu. > > Regards, > Andre > > On Sun, 11 Jan 2015 16:11:10 +0100 > Paul Richard Thomas <paul.richard.thomas@gmail.com> wrote: > >> Dear Andre, >> >> This is OK for trunk. I have not been keeping track of whether or not >> you have commit rights yet. If not, I will get to it sometime this >> week. >> >> Thanks for the patch. >> >> Paul >> >> On 10 January 2015 at 15:59, Andre Vehreschild <vehre@gmx.de> wrote: >> > Hi all, >> > >> > attached patch fixes the bug reported in pr 60334. The issue here was that >> > the function's result being (a pointer to) a deferred length char array. >> > The string length for the result value was wrapped in a local variable, >> > whose value was never written back to the string length of the result. This >> > lead the calling routine to take the length of the result to be random >> > leading to a crash. >> > >> > This patch addresses the issue by preventing the instantiation of the local >> > var and instead using a reference to the parameter. This not only saves one >> > value on the stack, but also because for small functions the compiler will >> > hold all parameters in registers for a significant level of optimization, >> > all the overhead of memory access (I hope :-). >> > >> > Bootstraps and regtests ok on x86_64-linux-gnu. >> > >> > - Andre >> > -- >> > Andre Vehreschild * Kreuzherrenstr. 8 * 52062 Aachen >> > Tel.: +49 241 9291018 * Email: vehre@gmx.de >> >> >> > > > -- > Andre Vehreschild * Kreuzherrenstr. 8 * 52062 Aachen > Tel.: +49 241 9291018 * Email: vehre@gmx.de
Hi Paul and Andre, hi all, Paul Richard Thomas wrote: ... [From Andre's patch] > + is a pointer to the variable holding the length. Therefore > + remove the deref on call. */ > + parmse.string_length = TREE_OPERAND (parmse.string_length, 0); > This is OK but I would use instead: ... + parmse.string_length = build_fold_indirect_ref (parmse.string_length); That doesn't match the comment: TREE_OPERAND(..., 0) removes the dereference, yielding a pointer, while your suggestion adds another deref. The opposite to dereferrencing is to take the address, i.e. using gfc_build_addr_expr (NULL_TREE, ...) > If you look in ~/gcc/fold-const.c:15751, you will see that > TREE_OPERAND (parmse.string_length, 0) but that it is preceded by > cleaning up of NOOPS and, in any case, its usage will preserve the > standard API.... just in case the internals change :-) I think using TREE_OPERAND directly should be fine. The condition if (INDIRECT_REF_P (parmse.string_length)) is only true if the last operator is an indirect ref; in that case, the object must be a pointer. Thus, I think TREE_OPERAND is better than gfc_build_addr_expr. The only potential issue would be that one has the wrong type; but I think that this is not possible in this case - and for function argument passing, using the wrong type would be already wrong even for pass by value (instead of by reference). [For value passing, one could use a cast/fold_convert(), for by ref not; however, there isn't one as INDIRECT_REF_P is the outermost operator.] Tobias PS: I haven't looked closer at the rest of the patch.
Hi Tobias, hi Paul, hi all, sorry, for the mess. I did not understand the meaning of build_fold_indirect_ref in the circumstances Paul pointed out. So if no objections exist, I like to propose the previous patch from: https://gcc.gnu.org/ml/fortran/2015-01/msg00056.html to address the issue in pr60334. This resolves the issue. Regards, Andre On Wed, 14 Jan 2015 14:57:57 +0100 Tobias Burnus <tobias.burnus@physik.fu-berlin.de> wrote: > Hi Paul and Andre, hi all, > > Paul Richard Thomas wrote: > ... > [From Andre's patch] > > + is a pointer to the variable holding the length. Therefore > > + remove the deref on call. */ > > + parmse.string_length = TREE_OPERAND (parmse.string_length, 0); > > > This is OK but I would use instead: > ... > + parmse.string_length = build_fold_indirect_ref > (parmse.string_length); > > That doesn't match the comment: TREE_OPERAND(..., 0) removes the dereference, > yielding a pointer, while your suggestion adds another deref. > > The opposite to dereferrencing is to take the address, i.e. using > gfc_build_addr_expr (NULL_TREE, ...) > > > > If you look in ~/gcc/fold-const.c:15751, you will see that > > TREE_OPERAND (parmse.string_length, 0) but that it is preceded by > > cleaning up of NOOPS and, in any case, its usage will preserve the > > standard API.... just in case the internals change :-) > > I think using TREE_OPERAND directly should be fine. The condition > if (INDIRECT_REF_P (parmse.string_length)) > is only true if the last operator is an indirect ref; in that case, the object > must be a pointer. > > Thus, I think TREE_OPERAND is better than gfc_build_addr_expr. > > > The only potential issue would be that one has the wrong type; but I think > that this is not possible in this case - and for function argument passing, > using the wrong type would be already wrong even for pass by value (instead > of by reference). [For value passing, one could use a cast/fold_convert(), > for by ref not; however, there isn't one as INDIRECT_REF_P is the outermost > operator.] > > Tobias > > PS: I haven't looked closer at the rest of the patch.
diff --git a/gcc/fortran/trans-decl.c b/gcc/fortran/trans-decl.c index 1e74125..86873f7 100644 --- a/gcc/fortran/trans-decl.c +++ b/gcc/fortran/trans-decl.c @@ -1333,12 +1333,30 @@ gfc_get_symbol_decl (gfc_symbol * sym) (sym->ts.u.cl->passed_length == sym->ts.u.cl->backend_decl)) sym->ts.u.cl->backend_decl = NULL_TREE; - if (sym->ts.deferred && fun_or_res - && sym->ts.u.cl->passed_length == NULL - && sym->ts.u.cl->backend_decl) + if (sym->ts.deferred && byref) { - sym->ts.u.cl->passed_length = sym->ts.u.cl->backend_decl; - sym->ts.u.cl->backend_decl = NULL_TREE; + /* The string length of a deferred char array is stored in the + parameter at sym->ts.u.cl->backend_decl as a reference and + marked as a result. Exempt this variable from generating a + temporary for it. */ + if (sym->attr.result) + { + /* We need to insert a indirect ref for param decls. */ + if (sym->ts.u.cl->backend_decl + && TREE_CODE (sym->ts.u.cl->backend_decl) == PARM_DECL) + sym->ts.u.cl->backend_decl = + build_fold_indirect_ref (sym->ts.u.cl->backend_decl); + } + /* For all other parameters make sure, that they are copied so + that the value and any modifications are local to the routine + by generating a temporary variable. */ + else if (sym->attr.function + && sym->ts.u.cl->passed_length == NULL + && sym->ts.u.cl->backend_decl) + { + sym->ts.u.cl->passed_length = sym->ts.u.cl->backend_decl; + sym->ts.u.cl->backend_decl = NULL_TREE; + } } if (sym->ts.u.cl->backend_decl == NULL_TREE) diff --git a/gcc/fortran/trans-expr.c b/gcc/fortran/trans-expr.c index 6bd3b03..a0390c1 100644 --- a/gcc/fortran/trans-expr.c +++ b/gcc/fortran/trans-expr.c @@ -5058,10 +5058,18 @@ gfc_conv_procedure_call (gfc_se * se, gfc_symbol * sym, so that the value can be returned. */ if (parmse.string_length && fsym && fsym->ts.deferred) { - tmp = parmse.string_length; - if (TREE_CODE (tmp) != VAR_DECL) - tmp = gfc_evaluate_now (parmse.string_length, &se->pre); - parmse.string_length = gfc_build_addr_expr (NULL_TREE, tmp); + if (INDIRECT_REF_P (parmse.string_length)) + /* In chains of functions/procedure calls the string_length already + is a pointer to the variable holding the length. Therefore + remove the deref on call. */ + parmse.string_length = TREE_OPERAND (parmse.string_length, 0); + else + { + tmp = parmse.string_length; + if (TREE_CODE (tmp) != VAR_DECL) + tmp = gfc_evaluate_now (parmse.string_length, &se->pre); + parmse.string_length = gfc_build_addr_expr (NULL_TREE, tmp); + } } /* Character strings are passed as two parameters, a length and a diff --git a/gcc/testsuite/gfortran.dg/deferred_type_param_6.f90 b/gcc/testsuite/gfortran.dg/deferred_type_param_6.f90 index eb00778..a2fabe8 100644 --- a/gcc/testsuite/gfortran.dg/deferred_type_param_6.f90 +++ b/gcc/testsuite/gfortran.dg/deferred_type_param_6.f90 @@ -2,15 +2,23 @@ ! ! PR fortran/51055 ! PR fortran/49110 -! +! PR fortran/60334 subroutine test() implicit none integer :: i = 5 character(len=:), allocatable :: s1 + character(len=:), pointer :: s2 + character(len=5), target :: fifeC = 'FIVEC' call sub(s1, i) if (len(s1) /= 5) call abort() if (s1 /= "ZZZZZ") call abort() + s2 => subfunc() + if (len(s2) /= 5) call abort() + if (s2 /= "FIVEC") call abort() + s1 = addPrefix(subfunc()) + if (len(s1) /= 7) call abort() + if (s1 /= "..FIVEC") call abort() contains subroutine sub(str,j) character(len=:), allocatable :: str @@ -19,6 +27,17 @@ contains if (len(str) /= 5) call abort() if (str /= "ZZZZZ") call abort() end subroutine sub + function subfunc() result(res) + character(len=:), pointer :: res + res => fifec + if (len(res) /= 5) call abort() + if (res /= "FIVEC") call abort() + end function subfunc + function addPrefix(str) result(res) + character(len=:), pointer :: str + character(len=:), allocatable :: res + res = ".." // str + end function addPrefix end subroutine test program a