[PATCH, fortran] PR fortran/60255 Deferred character length

[Janus Weil]

Hi Paul,

> Feature fetishism??? eeek! Does the doctor know how to deal with it?

I think gfortran is only lightly affected by this disease, so there
might be appropriate medication ;)

(I didn't want to offend anyone of course; just kiddin' around ...)


> 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.

Yes, I'm aware of that. But, also for this reason, it should better be stable!


> That, said, we do make it quite easy for folks to lay
> hands on 5.0.

Right. There are plenty of binaries and howtos for building versions
not shipped by your distributor. Those folks that desperately need the
new stuff will find a way to get it. (Here is an example of how people
address this problem:
http://www.astro.wisc.edu/~townsend/static.php?ref=mesasdk).


> 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 don't have a problem with attacking non-reg issues in stage 4 (we've
always been rather flexible in this respect in the Fortran front end),
as long as we also get the regression epidemic under control.


> I am as uncomfortable with the number of regressions as you. It used
> to be that 10 regressions would cause sleepless nights!

I can even remember times when the gfortran regression count was
identical to zero over significant periods of time. I guess the
increased regression count is partly price we pay for the pace of
progress during the last releases. Maybe also due to an increasing
user base, while the developer count tends to be constant.


> Thus, we
> should take advantage of the onset of stage 4 to deal with some of
> them.

Definitely. I already started with some of the regressions introduced
by the finalization code in 4.9. Feedback welcome ...

Cheers,
Janus





> 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
>
>
>
> --
> Outside of a dog, a book is a man's best friend. Inside of a dog it's
> too dark to read.
>
> Groucho Marx