Re: [Patch, fortran] PR69834 - Collision in derived type hashes

[Paul Richard Thomas <paul dot richard dot thomas at gmail dot com>]

Hi Andre,

Thanks for the review.  I have partially responded to your comments - see below.

Committed as revision 241450.

Cheers

Paul

On 23 October 2016 at 14:45, Andre Vehreschild <vehre@gmx.de> wrote:
> Hi Paul,
>
> here are my comments to your patch:
>
>> Index: gcc/fortran/class.c
>> ===================================================================
>> *** gcc/fortran/class.c (revision 241439)
>> --- gcc/fortran/class.c (working copy)
>> *************** add_procs_to_declared_vtab (gfc_symbol *
>> --- 2187,2219 ----
>>   gfc_symbol *
>>   gfc_find_derived_vtab (gfc_symbol *derived)
>>   {
>> !   gfc_namespace *ns = NULL;
>
> Setting this to NULL for consistency?

Indeed - =NULL eliminated. It was needed in one incarnation of the patch.


>
>> Index: gcc/fortran/dump-parse-tree.c
>> ===================================================================
>> *** gcc/fortran/dump-parse-tree.c       (revision 241439)
>> --- gcc/fortran/dump-parse-tree.c       (working copy)
>> *************** show_code_node (int level, gfc_code *c)
>> *** 1843,1848 ****
>> --- 1843,1877 ----
>
> Well, the code in this chunk is identical to the one of EXEC_SELECT, besides
> two lines where the statement's name is printed. I propose to do something like:
>
> case EXEC_SELECT:
> case EXEC_SELECT_TYPE:
>   d= ..
>   fputs ("SELECT", dumpfile);
>   if (c->op == EXEC_SELECT_TYPE)
>     fputs (" TYPE", dumpfile);
>  ...
>   // and the same for "END SELECT..."
>
> This would reduce the amount of copied code. An improvement in one
> EXEC_SELECT-dump-handler would then automagically available in the other, too.

Have done this, except for the end, where I have retained "END SELECT" for both.

>
>> Index: gcc/fortran/resolve.c
>> ===================================================================
>> *** gcc/fortran/resolve.c       (revision 241439)
>> --- gcc/fortran/resolve.c       (working copy)
> <snipp>
>> *************** resolve_select_type (gfc_code *code, gfc
> <snipp>
>> --- 8595,8641 ----
>>     else
>>       ns->code->next = new_st;
>>     code = new_st;
>> !   code->op = EXEC_SELECT_TYPE;
>>
>> +   /* Use the intrinsic LOC function to generate the an integer expression
>> +      for the vtable of the selector.  Note that the rank of the selector
>> +      expression has to be set to zero.  */
>
> double article:                                    _the an_  !!!

Corrected - thanks.

>
>> Index: gcc/fortran/trans-stmt.c
>> ===================================================================
>> *** gcc/fortran/trans-stmt.c    (revision 241439)
>> --- gcc/fortran/trans-stmt.c    (working copy)
>> *************** gfc_trans_do_while (gfc_code * code)
>> *** 2331,2336 ****
>> --- 2331,2455 ----
> <snipp>
>> +
>> +   /* Translate an assignment to a CLASS object
>> +      (pointer or ordinary assignment).  */
>> +
>> +
>
> Here is no routine the above comment could document. Left over from prior
> version?

This is in your tree, not mine :-)

>
>>   /* End of prototype trans-class.c  */
>
>
>> Index: gcc/fortran/trans-stmt.c
>> ===================================================================
>> *** gcc/fortran/trans-stmt.c    (revision 241439)
>> --- gcc/fortran/trans-stmt.c    (working copy)
>> *************** gfc_trans_do_while (gfc_code * code)
>> *** 2331,2336 ****
>> --- 2331,2455 ----
>
> Can one optimize this by using the "old style" for intrinsic types, i.e. a
> computed goto (switch-case) for them? And in the default case the if-chain on
> the derived types/classes? Would we gain any speed by this? What is your
> opinion on this?

Maybe this would gain something but I suspect that it would not amount to much.

>
>> Please find attached a revised version of the patch that corrects one
>> or two tiny wrinkles. I have removed the tidy up of vtable retrieval
>
> I haven't understood yet, what you need to do for this. Looking forward to that
> patch.

OK will submit this separately.

>
> With the above small changes the patch is ok for trunk given that Dominique
> doesn't find any issues.
>
> Beware, that my big patch on polymorphic assignment will *not* be backported
> to gcc-6. I.e., this version of your patch will most probably not be applyable.
> You rather will need to apply the old version.
>
> Thanks for the work.
>
> Regards,
>         Andre
>
>> Functionally, the patch is as described in the original submission.
>>
>> As attached, it bootstraps and regtests on FC21/x86_64.  OK for trunk
>> and, after a decent interval for 6-branch?
>>
>> Cheers
>>
>> Paul
>>
>> 2016-10-22  Paul Thomas  <pault@gcc.gnu.org>
>>
>>     PR fortran/69834
>>     * class.c (gfc_find_derived_vtab): Obtain the gsymbol for the
>>     derived type's module. If the gsymbol is present and the top
>>     level namespace corresponds to a module, use the gsymbol name
>>     space. In the search to see if the vtable exists, try the gsym
>>     namespace first.
>>     * dump-parse-tree (show_code_node): Add explicit dump for the
>>     select type construct.
>>     * resolve.c (build_loc_call): New function.
>>     (resolve_select_type): Add check for repeated type is cases.
>>     Retain selector expression and use it later instead of expr1.
>>     Exclude deferred length TYPE IS cases and emit error message.
>>     Store the address for the vtable in the 'low' expression and
>>     the hash value in the 'high' expression, for each case. Do not
>>     call resolve_select.
>>     * trans.c(trans_code) : Call gfc_trans_select_type.
>>     * trans-stmt.c (gfc_trans_select_type_cases): New function.
>>     (gfc_trans_select_type): New function.
>>     * trans-stmt.h : Add prototype for gfc_trans_select_type.
>>
>> 2016-10-22  Paul Thomas  <pault@gcc.gnu.org>
>>
>>     PR fortran/69834
>>     * gfortran.dg/select_type_1.f03: Change error for overlapping
>>     TYPE IS cases.
>>     * gfortran.dg/select_type_36.f03: New test.
>
>
> --
> Andre Vehreschild * Email: vehre ad gmx dot de



-- 
The difference between genius and stupidity is; genius has its limits.

Albert Einstein