[PATCH, fortran] PR fortran/60255 Deferred character length + PR60289 Also deferred char len.

[Paul Richard Thomas]

Dear Andre,

I have applied your patch to my updated tree and have it bootstrapping
and regtesting. However, the changes to gcc mean that it is taking for
ever :-(
If I can, I will commit it tonight but I have to crash out fairly
soon. It might now have to be Wednesday since I am entertaining
visitors tomorrow evening. I will let you know.

Cheers

Paul


On 19 January 2015 at 16:36, Andre Vehreschild <vehre@gmx.de> wrote:
> Hi Paul, hi all,
>
> I am just trying to get write after approval rights on trunk, but that will
> take a few more ticks. Nevertheless is the patch for pr60289 not yet in trunk,
> while it already has been reviewed and approved.
>
> Could someone please add it for 5.0?
>
> Bootstraps and regtests ok with latest trunk on x86_64-linux-gnu.
>
> Thank you very much,
>         Andre
>
>
> On Mon, 19 Jan 2015 16:01:05 +0100
> Paul Richard Thomas <paul.richard.thomas@gmail.com> wrote:
>
>> Janus,
>>
>> Feature fetishism??? eeek! Does the doctor know how to deal with it?
>>
>> I had the same feeling about 4.9 as you but I thought that I would
>> ask. The pity is that it is likely to be the distro release for some
>> time to come.  That, said, we do make it quite easy for folks to lay
>> hands on 5.0. Stage 4 has just started for 5.0 too, so I am going to
>> miss the target for completion of PR55901, unless somebody gives me
>> the green light. I'll try to post the patch tonight.
>>
>> I am as uncomfortable with the number of regressions as you. It used
>> to be that 10 regressions would cause sleepless nights! Thus, we
>> should take advantage of the onset of stage 4 to deal with some of
>> them.
>>
>> Cheers
>>
>> Paul
>>
>>
>> On 19 January 2015 at 13:52, Janus Weil <janus@gcc.gnu.org> wrote:
>> > Hi guys,
>> >
>> >> thank you very much for your effort. I really appreciate it.
>> >
>> > thanks to both of you for taking on deferred-character polymorphism!
>> >
>> >
>> >> I would love to see all patches in 4.9. :-) I volunteer to prepare them. My
>> >> experience for 60255 is that it needs some manual moves of blocks of code,
>> >> where patch is not able to figure the correct position.
>> >>
>> >> When not all patches, then at least 60334 and 60255 (with 55901 and 64578
>> >> as dependent).
>> >
>> > Actually I would advise against backporting any of this to 4.9. The
>> > usual rule is that only regression fixes are backported to the release
>> > branches, and neither of those PRs address any regressions AFAICS.
>> > Moreover they are far from simple and do have the potential to
>> > introduce regressions themselves, which is totally not what we what on
>> > a release branch.
>> >
>> > Btw, there are around 40 open gfortran regressions right now, and
>> > that's way too much. I think we should focus a bit more on stability
>> > instead of feature fetishism right now ...
>> >
>> > Cheers,
>> > Janus
>> >
>> >
>> >
>> >> On Sun, 18 Jan 2015 23:21:20 +0100
>> >> Paul Richard Thomas <paul.richard.thomas@gmail.com> wrote:
>> >>
>> >>> Dear Andre, dear all,
>> >>>
>> >>> I committed your patch exactly as it came. I have been at it for the
>> >>> whole weekend, barring dog walking and painting an external wall.
>> >>>
>> >>> I have committed patches for PRs,60334, 60357, 61275, (55932), 57959,
>> >>> 64578, part of 55901 and, finally, 60255!
>> >>>
>> >>> I'll build on what I have committed.
>> >>>
>> >>> Opinions on committing to 4.9?
>> >>>
>> >>> Cheers
>> >>>
>> >>> Paul
>> >>>
>> >>> On 17 January 2015 at 16:14, Andre Vehreschild <vehre@gmx.de> wrote:
>> >>> > Dear Paul,
>> >>> >
>> >>> > Setting _Len to one by default should be quite simple. When I remember
>> >>> > correctly, then there is only one place where the current code sets it
>> >>> > to zero. Could be in gfc_conv_structure but that is only a guess.
>> >>> > Unfortunately am I on travel 'till Sunday and don't have my laptop with
>> >>> > me. Sorry for that.
>> >>> >
>> >>> > Regards,
>> >>> > Andre
>> >>> >
>> >>> > Andre Vehreschild * Kreuzherrenstr. 8 * 52062 Aachen
>> >>> > Tel. +49 241.9291018 * vehre@gmx.de
>> >>> >
>> >>> >
>> >>> > Paul Richard Thomas <paul.richard.thomas@gmail.com> schrieb:
>> >>> >
>> >>> > Cancel that - It should be multiplies by kind, shouldn't it ? :-) OK,
>> >>> > string length it is. We will probably have to set _len = 1 for other
>> >>> > dynamic types, though, so that the pointer arising from an array
>> >>> > reference is base_address + _len*vptr->size*index
>> >>> >
>> >>> > Cheers
>> >>> >
>> >>> > Paul
>> >>> >
>> >>> > On 17 January 2015 at 13:44, Paul Richard Thomas
>> >>> > <paul.richard.thomas@gmail.com> wrote:
>> >>> >> Dear Andre,
>> >>> >>
>> >>> >> I am open to either - what do the others think?
>> >>> >>
>> >>> >> The reason why I am for differentiating between unlimited_polymorphic
>> >>> >> objects and the deferred length character arrays is because of the
>> >>> >> difference in the way in which arrays are accessed. The former uses
>> >>> >> pointer arithmetic and the latter array references. I was trying to
>> >>> >> avoid divisions by KIND within scalarization loops. Also, I found that
>> >>> >> in developing your patch, that allocating with unlimited polymorphic
>> >>> >> sources looks neatest when the _len contains the memory size of the
>> >>> >> elements of any dynamic type, since a priori it is not known at
>> >>> >> compile time whether it is a character or not. Of course, one could
>> >>> >> interrogate the _hash field of the vtable, at the expense of more
>> >>> >> runtime code.
>> >>> >>
>> >>> >> Cheers
>> >>> >>
>> >>> >> Paul
>> >>> >>
>> >>> >> PS I have your patches for PR60357 and 61275 regtesting right now.
>> >>> >> Both look OK to me. At the risk of making potential regressions more
>> >>> >> complicated to unravel, to save my time I intend to commit both at
>> >>> >> once, unless anybody objects.
>> >>> >>
>> >>> >>
>> >>> >>
>> >>> >> On 17 January 2015 at 13:10, Andre Vehreschild <vehre@gmx.de> wrote:
>> >>> >>> Hi Paul,
>> >>> >>>
>> >>> >>> I am open on what to call the new component.
>> >>> >>>
>> >>> >>> Have you thought about my findings, that for deferred length char
>> >>> >>> arrays the
>> >>> >>> length is stored in characters and not in bytes, I.e., for a
>> >>> >>> character(kind=4, Len=:) the length is stored in number of characters
>> >>> >>> and not in bytes needed, which would be Len*4. IMHO both concepts
>> >>> >>> should be changed, or none. I favor to keep storing the string length
>> >>> >>> of both concepts
>> >>> >>> (deferred char arrays and chararrays in unlimited polymorphic
>> >>> >>> entities) interchangeable w/o computation.
>> >>> >>>
>> >>> >>> What's your opinion?
>> >>> >>>
>> >>> >>> Regards, Andre
>> >>> >>>
>> >>> >>> Andre Vehreschild * Kreuzherrenstr. 8 * 52062 Aachen
>> >>> >>> Tel. +49 241.9291018 * vehre@gmx.de
>> >>> >>>
>> >>> >>>
>> >>> >>> Paul Richard Thomas <paul.richard.thomas@gmail.com> schrieb:
>> >>> >>>
>> >>> >>>
>> >>> >>> Dear Andre,
>> >>> >>>
>> >>> >>> Perhaps, rather than calling the new component _len, we should call it
>> >>> >>> _mem_size or some such?
>> >>> >>>
>> >>> >>> Cheers
>> >>> >>>
>> >>> >>> Paul
>> >>> >>>
>> >>> >>> On 9 January 2015 at 11:52, Andre Vehreschild <vehre@gmx.de> wrote:
>> >>> >>>> Hi all, hi Paul,
>> >>> >>>>
>> >>> >>>> I started to implement the changes requested below, but I stumbled
>> >>> >>>> over an
>> >>> >>>> oddity:
>> >>> >>>>
>> >>> >>>> For a deferred length kind4 char array, the length of the string is
>> >>> >>>> stored
>> >>> >>>> without multiplication by 4 in the length variable attached. So when
>> >>> >>>> we now
>> >>> >>>> decide to store the length of the string in an unlimited polymorphic
>> >>> >>>> entity in
>> >>> >>>> bytes in the component formerly called _len and the size of each
>> >>> >>>> character
>> >>> >>>> in
>> >>> >>>> _vtype->_size then we have an inconsistency with the style deferred
>> >>> >>>> char lengths are stored. IMHO we should store this consistently,
>> >>> >>>> i.e., both 'length'-variables store either the length of the string
>> >>> >>>> ('length' = array_len)
>> >>> >>>> or the size of the memory needed ('length' = array_len * char_size).
>> >>> >>>> What
>> >>> >>>> do
>> >>> >>>> you think?
>> >>> >>>>
>> >>> >>>> Furthermore, think about debugging: When looking at an unlimited
>> >>> >>>> polymorphic
>> >>> >>>> entity storing a kind-4-char-array of length 7, then having a
>> >>> >>>> 'length' component
>> >>> >>>> set to 28 will lead to confusion. I humbly predict, that this will
>> >>> >>>> produce
>> >>> >>>> many
>> >>> >>>> entries in the bugtracker, because people don't understand that
>> >>> >>>> 'length' stores
>> >>> >>>> the product of elem_size times string_len, because all they see is an
>> >>> >>>> assignment of a length-7 char array.
>> >>> >>>>
>> >>> >>>> What do we do about it?
>> >>> >>>>
>> >>> >>>> Regards,
>> >>> >>>>         Andre
>> >>> >>>>
>> >>> >>>> On Thu, 8 Jan 2015 20:56:43 +0100
>> >>> >>>> Paul Richard Thomas <paul.richard.thomas@gmail.com> wrote:
>> >>> >>>>
>> >>> >>>>> Dear Andre,
>> >>> >>>>>
>> >>> >>>>> Thanks for the patch. As I have said to you, off list, I think that
>> >>> >>>>> the _size field in the vtable should contain the kind information
>> >>> >>>>> and that the _len field should carry the length of the string in
>> >>> >>>>> bytes. I think that it is better to optimise array access this way
>> >>> >>>>> than to avoid the division in evaluating LEN (). I am happy to
>> >>> >>>>> accept contrary opinions from the others.
>> >>> >>>>>
>> >>> >>>>> I do not believe that the bind_c issue is an issue. Your patch
>> >>> >>>>> correctly deals with it IMHO.
>> >>> >>>>>
>> >>> >>>>> Subject to the above change in the value of _len, I think that your
>> >>> >>>>> patch is OK for trunk.
>> >>> >>>>>
>> >>> >>>>> With best regards
>> >>> >>>>>
>> >>> >>>>> Paul
>> >>> >>>>>
>> >>> >>>>> On 4 January 2015 at 13:40, Andre Vehreschild <vehre@gmx.de> wrote:
>> >>> >>>>> > Hi Janus, hi Paul, hi Tobias,
>> >>> >>>>> >
>> >>> >>>>> > Janus: During code review, I found that I had the code in
>> >>> >>>>> > gfc_get_len_component() duplicated. So I now reintroduced and
>> >>> >>>>> > documented the
>> >>> >>>>> > routine making is more commonly usable and added more
>> >>> >>>>> > documentation. The
>> >>> >>>>> > call sites are now simplify.c (gfc_simplify_len) and trans-expr.c
>> >>> >>>>> > (gfc_trans_pointer_assignment). Attached is the reworked version
>> >>> >>>>> > of the
>> >>> >>>>> > patch.
>> >>> >>>>> >
>> >>> >>>>> > Paul, Tobias: Can one of you have a look at line 253 of the
>> >>> >>>>> > patch? I need
>> >>> >>>>> > some expertise on the bind_c behavior. My patch needs the check
>> >>> >>>>> > for is_bind_c added in trans_expr.c (gfc_conv_expr) to prevent
>> >>> >>>>> > mistyping an
>> >>> >>>>> > associated variable in a select type() during the conv.
>> >>> >>>>> > Background: This
>> >>> >>>>> > code fragment taken from the testcase in the patch:
>> >>> >>>>> >
>> >>> >>>>> > MODULE m
>> >>> >>>>> > contains
>> >>> >>>>> >   subroutine bar (arg, res)
>> >>> >>>>> >     class(*) :: arg
>> >>> >>>>> >     character(100) :: res
>> >>> >>>>> >     select type (w => arg)
>> >>> >>>>> >       type is (character(*))
>> >>> >>>>> >         write (res, '(I2)') len(w)
>> >>> >>>>> >     end select
>> >>> >>>>> >   end subroutine
>> >>> >>>>> > END MODULE
>> >>> >>>>> >
>> >>> >>>>> > has the conditions required for line trans-expr.c:6630 of
>> >>> >>>>> > gfc_conv_expr
>> >>> >>>>> > when
>> >>> >>>>> > the associate variable w is converted. This transforms the type of
>> >>> >>>>> > the
>> >>> >>>>> > associate variable to something unexpected in the further
>> >>> >>>>> > processing leading to some issues during fortraning. Janus told
>> >>> >>>>> > me, that the f90_type
>> >>> >>>>> > has been abused for some other things (unlimited polymorphic
>> >>> >>>>> > treatment).
>> >>> >>>>> > Although I believe that reading the comments above the if in
>> >>> >>>>> > question,
>> >>> >>>>> > the
>> >>> >>>>> > check I had to enhance is treating bind_c stuff (see the threads
>> >>> >>>>> > content
>> >>> >>>>> > for more). I would feel safer when one of you gfortran gurus can
>> >>> >>>>> > have a
>> >>> >>>>> > look and given an opinion, whether the change is problematic. I
>> >>> >>>>> > couldn't
>> >>> >>>>> > figure why w is resolved to meet the criteria (any ideas). Btw,
>> >>> >>>>> > all regtest
>> >>> >>>>> > are ok reporting no issues at all.
>> >>> >>>>> >
>> >>> >>>>> > Bootstraps and regtests ok on x86_64-linux-gnu
>> >>> >>>>> >
>> >>> >>>>> > Regards,
>> >>> >>>>> >         Andre
>> >>> >>>>> >
>> >>> >>>>> >
>> >>> >>>>> > On Sat, 3 Jan 2015 16:45:07 +0100
>> >>> >>>>> > Janus Weil <janus@gcc.gnu.org> wrote:
>> >>> >>>>> >
>> >>> >>>>> >> Hi Andre,
>> >>> >>>>> >>
>> >>> >>>>> >> >> >> For the
>> >>> >>>>> >> >> >> second one (in gfc_conv_expr), I don't directly see how
>> >>> >>>>> >> >> >> it's related
>> >>> >>>>> >> >> >> to deferred char-len. Why is this change needed?
>> >>> >>>>> >> >> >
>> >>> >>>>> >> >> > That change is needed, because in some rare case where an
>> >>> >>>>> >> >> > associated
>> >>> >>>>> >> >> > variable in a "select type ()" is used, then the type and
>> >>> >>>>> >> >> > f90_type
>> >>> >>>>> >> >> > match the condition while them not really being in a bind_c
>> >>> >>>>> >> >> > context.
>> >>> >>>>> >> >> > Therefore I have added the check for bind_c. Btw, I now have
>> >>> >>>>> >> >> > removed
>> >>> >>>>> >> >> > the TODO, because that case is covered by the regression
>> >>> >>>>> >> >> > tests.
>> >>> >>>>> >> >>
>> >>> >>>>> >> >> I don't understand how f90_type can be BT_VOID without being
>> >>> >>>>> >> >> in a BIND_C context, but I'm not really a ISO_C_BINDING
>> >>> >>>>> >> >> expert. Which test
>> >>> >>>>> >> >> case is the one that triggered this?
>> >>> >>>>> >> >
>> >>> >>>>> >> > This case is triggered by the test-case in the patch, where in
>> >>> >>>>> >> > the select
>> >>> >>>>> >> > type (w => arg) in module m routine bar the w meets the
>> >>> >>>>> >> > criteria to
>> >>> >>>>> >> > make
>> >>> >>>>> >> > the condition become true. The type of w is then "fixed" and
>> >>> >>>>> >> > gfortran
>> >>> >>>>> >> > would terminate, because the type of w would be set be and
>> >>> >>>>> >> > BT_INTEGER. I
>> >>> >>>>> >> > tried to backtrace where this is coming from, but to no
>> >>> >>>>> >> > success. In
>> >>> >>>>> >> > the
>> >>> >>>>> >> > resolve () of the select type it looks all quite ok, but in the
>> >>> >>>>> >> > trans
>> >>> >>>>> >> > stage the criteria are met. Most intriguing to me is, that in
>> >>> >>>>> >> > the condition we are talking about the type of w and f90_type
>> >>> >>>>> >> > of the derived
>> >>> >>>>> >> > class' ts (expr->ts.u.derived->ts.f90_type) of w is examined.
>> >>> >>>>> >> > But expr->ts.u.derived->ts does not describe the type of w,
>> >>> >>>>> >> > but of the class
>> >>> >>>>> >> > w is associate with __STAR...
>> >>> >>>>> >> >
>> >>> >>>>> >> > So I am not quite sure how to fix this, if this really needs
>> >>> >>>>> >> > fixing.
>> >>> >>>>> >> > When I understand you right, then f90_type should only be set
>> >>> >>>>> >> > in a bind_c context, so adding that check wouldn't hurt, right?
>> >>> >>>>> >>
>> >>> >>>>> >> Yes, in principle adding the check for attr.bind_c looks ok to me
>> >>> >>>>> >> (alternatively one could also check for
>> >>> >>>>> >> attr.unlimited_polymorphic). I
>> >>> >>>>> >> think originally BT_VOID was indeed only used in a bind_c
>> >>> >>>>> >> context, but
>> >>> >>>>> >> recently it has also been 'hijacked' for unlimited polymorphism,
>> >>> >>>>> >> e.g.
>> >>> >>>>> >> for the STAR symbol and some of the components of the intrinsic
>> >>> >>>>> >> vtabs.
>> >>> >>>>> >>
>> >>> >>>>> >> What I don't really understand is why these problems are
>> >>> >>>>> >> triggered by
>> >>> >>>>> >> your patch now and have not crept up earlier in other use-cases
>> >>> >>>>> >> of CLASS(*).
>> >>> >>>>> >>
>> >>> >>>>> >>
>> >>> >>>>> >> >> >> 3) The function 'gfc_get_len_component' that you're
>> >>> >>>>> >> >> >> introducing
>> >>> >>>>> >> >> >> is
>> >>> >>>>> >> >> >> only called in a single place. Do you expect this to be
>> >>> >>>>> >> >> >> useful in
>> >>> >>>>> >> >> >> other places in the future, or could one remove the
>> >>> >>>>> >> >> >> function and
>> >>> >>>>> >> >> >> insert the code inline?
>> >>> >>>>> >> >> >
>> >>> >>>>> >> >> > In one of the first versions it was uses from two locations.
>> >>> >>>>> >> >> > But
>> >>> >>>>> >> >> > I
>> >>> >>>>> >> >> > had to remove one call site again. I am currently not sure,
>> >>> >>>>> >> >> > if I
>> >>> >>>>> >> >> > will
>> >>> >>>>> >> >> > be using it in the patch for allocatable components when
>> >>> >>>>> >> >> > deferred
>> >>> >>>>> >> >> > char arrays are handled. So what I do I do now? Inline it
>> >>> >>>>> >> >> > and when
>> >>> >>>>> >> >> > needed make it explicit again in a future patch?
>> >>> >>>>> >> >>
>> >>> >>>>> >> >> I leave that up to you. In principle I'm fine with keeping it
>> >>> >>>>> >> >> as it
>> >>> >>>>> >> >> is. The only problem I see is that the function name sounds
>> >>> >>>>> >> >> rather
>> >>> >>>>> >> >> general, but it apparently expects the expression to be an
>> >>> >>>>> >> >> ASSOCIATE
>> >>> >>>>> >> >> symbol.
>> >>> >>>>> >> >
>> >>> >>>>> >> > I am nearly finished with the patch on allocatable scalar
>> >>> >>>>> >> > components
>> >>> >>>>> >> > and
>> >>> >>>>> >> > I don't need the code there. Therefore I have inlined the
>> >>> >>>>> >> > routine.
>> >>> >>>>> >>
>> >>> >>>>> >> Ok, good. Could you please post an updated patch?
>> >>> >>>>> >>
>> >>> >>>>> >>
>> >>> >>>>> >> > So, what do we do about the bind_c issue above? Is some bind_c
>> >>> >>>>> >> > guru
>> >>> >>>>> >> > available to have a look at this? It would be very much
>> >>> >>>>> >> > appreciated.
>> >>> >>>>> >>
>> >>> >>>>> >> From my non-guru POV, it can stay as is.
>> >>> >>>>> >>
>> >>> >>>>> >> It would be helpful if someone like Paul or Tobias could have a
>> >>> >>>>> >> look at the patch before it goes to trunk. I think it's pretty
>> >>> >>>>> >> close to being ready for prime-time. Thanks for your work!
>> >>> >>>>> >>
>> >>> >>>>> >> Cheers,
>> >>> >>>>> >> Janus
>> >>> >>>>> >
>> >>> >>>>> >
>> >>> >>>>> > --
>> >>> >>>>> > 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
>> >>> >>
>> >>> >>
>> >>> >>
>> >>> >> --
>> >>> >> Outside of a dog, a book is a man's best friend. Inside of a dog it's
>> >>> >> too dark to read.
>> >>> >>
>> >>> >> Groucho Marx
>> >>> >
>> >>> >
>> >>> >
>> >>> > --
>> >>> > Outside of a dog, a book is a man's best friend. Inside of a dog it's
>> >>> > too dark to read.
>> >>> >
>> >>> > Groucho Marx
>> >>>
>> >>>
>> >>>
>> >>
>> >>
>> >> --
>> >> 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