[Fortran, Patch] PR60334 - Segmentation fault on character pointer assignments

[Paul Richard Thomas]

Committed to trunk as r219798. I'll update 4.9 likewise during next week.

Cheers

Paul

On 15 January 2015 at 14:06, Andre Vehreschild <vehre@gmx.de> wrote:
> Hi Paul,
>
> I totally trust your knowledge on this. That said, I think the patch is ripe for
> commit now. May I kindly ask you to commit it?
>
> https://gcc.gnu.org/ml/fortran/2015-01/msg00056.html
>
> Regards,
>         Andre
>
>
> On Thu, 15 Jan 2015 13:54:04 +0100
> Paul Richard Thomas <paul.richard.thomas@gmail.com> wrote:
>
>> Dear Andre,
>>
>> I think that opportunities for the gfortran maintainers to screw this
>> one up are binary - zero or total catastrophe. Thus, I do not believe
>> that a gcc_assert would serve much purpose.
>>
>> Cheers
>>
>> Paul
>>
>> On 15 January 2015 at 13:23, Andre Vehreschild <vehre@gmx.de> wrote:
>> > Hi Paul,
>> >
>> > what about an assert in trans-expr.c?
>> >
>> > Something like:
>> >
>> > gcc_assert (POINTER_TYPE_P (TREE_TYPE (parmse.string_length)));
>> >
>> > after line 57 in the patch. Do you think that would be helping to prevent
>> > future failure of gfortran on that topic?
>> >
>> > Regards,
>> >         Andre
>> >
>> > On Thu, 15 Jan 2015 13:09:34 +0100
>> > Paul Richard Thomas <paul.richard.thomas@gmail.com> wrote:
>> >
>> >> Thanks,
>> >>
>> >> Following Tobias's intervention, I went back and took another look.
>> >> Mea culpa - it's me that is causing the confusion.
>> >>
>> >> Paul
>> >>
>> >> On 15 January 2015 at 11:36, Andre Vehreschild <vehre@gmx.de> wrote:
>> >> > Hi Paul, hi Tobias,
>> >> >
>> >> > I am trying to clear this up. Please keep in mind that I am German and my
>> >> > English isn't that good.
>> >> >
>> >> > The patch addresses a pseudo-fortran code like this:
>> >> >
>> >> > function foo() result(out)
>> >> >   character(len=:) :: out
>> >> >   out = "chars"
>> >> > end
>> >> >
>> >> > function bar() result(con)
>> >> >   character(len=:) :: con
>> >> >   con = "deferred " // foo()
>> >> > end
>> >> >
>> >> > because for the deferred char arrays out and con two information are
>> >> > needed: 1. the pointer to the memory for the char array and
>> >> > 2. the number of chars in the array.
>> >> > gfortran rewrites these functions to procedures with two reference
>> >> > parameters (now in simplified C-style; pre-patch-pr60334-code):
>> >> >
>> >> > void foo(char &__out, integer4 &.__out)
>> >> > // I am deliberately stripping all memory allocation and memcpy here
>> >> > *__out = "chars";
>> >> > .__out = 5; // <-- *
>> >> >
>> >> > * This line is wrong, because the pointer's address is set to memory
>> >> > location 5. This is addressed by the first part of the patch patching
>> >> > trans-decl.c which ensures, that this line becomes:
>> >> >
>> >> > *.__out = 5;
>> >> >
>> >> > The patch does this by resetting the backend_decl in sym(__out)->ts.u.cl
>> >> > to become a de-reference of the pointer defined in foo's function
>> >> > signature, i.e., the indirect_ref is added to the tree always. Because,
>> >> > when looking at the ts.u.cl->backend_decl of the deferred char array
>> >> > __out, the length of the string is of interest and not the memory
>> >> > location where that information is stored.
>> >> >
>> >> > Now to the second part of the patch, which takes care about chains of
>> >> > function calls, like in bar(). In pseudo C bar() looks like this before
>> >> > the second part of the patch is applied:
>> >> >
>> >> > void bar(char &__con, integer4 &.__con)
>> >> > // Again stripping all the malloc and memcpy to keep it simple
>> >> > character(len=:) :: tmp
>> >> > foo(tmp, *.__con); <-- **
>> >> > *__con = strcat("deferred ", tmp);
>> >> > *.__con = *.__con + 9;
>> >> >
>> >> > At ** the de-reference of .__con is used as the string-length argument to
>> >> > call foo. Remember the de-referencing operator is part of con's
>> >> > ts.u.cl->backend_decl as ensured by the first part of the patch. But foo
>> >> > expects a pointer to a memory location where it may store the length of
>> >> > the string it returns. The second part of the patch now strips the
>> >> > de-referencing operator added by the first patch from the backend_decl to
>> >> > get this pointer.
>> >> >
>> >> > I have chosen this way (adding the indirect_ref to the backend_decl and
>> >> > stripping it again in the rare case the address is needed), because it
>> >> > meant far less changes in gfortran, than checking every position where a
>> >> > string length's backend_decl could have been involved. Furthermore, does
>> >> > the patch ensure, that the cl's backend_decl returns a tree to the length
>> >> > of string and not the memory location where that length is stored.
>> >> >
>> >> > Therefore the parmse.string_length in line 53 of the patch is a deref of
>> >> > a pointer variable already. But only in this location the pointer (w/o
>> >> > deref) is needed (to be able to make a call by reference and) to get the
>> >> > return value of the function call handled in the context.
>> >> >
>> >> > I hope this explains the need for INDIRECT_REF_P in line 53 of the patch.
>> >> >
>> >> > May be we should add a gfc_assert (POINTER_TYPE_P
>> >> > (parmse.string_length)); after the line:
>> >> >
>> >> > parmse.string_length = TREE_OPERAND (parmse.string_length, 0);
>> >> >
>> >> > to make sure, that in the future an accidental change is detected
>> >> > immediately. What do you think?
>> >> >
>> >> > Regards,
>> >> >         Andre
>> >> >
>> >> > On Wed, 14 Jan 2015 21:16:16 +0100
>> >> > Paul Richard Thomas <paul.richard.thomas@gmail.com> wrote:
>> >> >
>> >> >> Dear Tobias and Andre,
>> >> >>
>> >> >> I am totally confused :-) I think that the comment correctly points
>> >> >> out that these are pointers/addresses for the string lengths
>> >> >> concerned. I wish that build_fold_indirect_reference was called
>> >> >> build_fold_de_reference or something! Certainly, the confusion with
>> >> >> INDIRECT_REF_P can come about because the tree concerned has been
>> >> >> multiply operated upon. Andre has written subsequent to your message -
>> >> >> I'll take a look at his offering.
>> >> >>
>> >> >> Cheers
>> >> >>
>> >> >> Paul
>> >> >>
>> >> >> On 14 January 2015 at 14:57, 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.
>> >> >>
>> >> >>
>> >> >>
>> >> >
>> >> >
>> >> > --
>> >> > 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
>>
>>
>>
>
>
> --
> Andre Vehreschild * Kreuzherrenstr. 8 * 52062 Aachen
> Tel.: +49 241 9291018 * Email: vehre@gmx.de



-- 
Outside of a dog, a book is a man's best friend. Inside of a dog it's
too dark to read.

Groucho Marx