Re: [PATCH] Fortran: use name of array component in runtime error message [PR30802]
Am 28.01.24 um 22:43 schrieb Steve Kargl: On Sun, Jan 28, 2024 at 08:56:24PM +0100, Harald Anlauf wrote: Am 28.01.24 um 12:39 schrieb Mikael Morin: Le 24/01/2024 à 22:39, Harald Anlauf a écrit : Dear all, this patch is actually only a followup fix to generate the proper name of an array reference in derived-type components for the runtime error message generated for the bounds-checking code. Without the proper part ref, not only a user may get confused: I was, too... The testcase is compile-only, as it is only important to check the strings used in the error messages. Regtested on x86_64-pc-linux-gnu. OK for mainline? the change proper looks good, and is an improvement. But I'm a little concerned by the production of references like in the test x1%vv%z which could be confusing and is strictly speaking invalid fortran (multiple non-scalar components). Did you consider generating x1%vv(?,?)%zz or x1%vv(...)%z or similar? yes, that seems very reasonable, given that this is what NAG does. We also have spurious %_data in some error messages that I'll try to get rid off. I haven't looked at the patch, but sometimes (if not always) things like _data are marked with attr.artificial. You might see if this will help with suppressing spurious messages. I was talking about the generated format strings of runtime error messages. program p implicit none type t real :: zzz(10) = 42 end type t class(t), allocatable :: xx(:) integer :: j j = 0 allocate (t :: xx(1)) print *, xx(1)% zzz(j) end This is generating the following error at runtime since at least gcc-7: Fortran runtime error: Index '0' of dimension 1 of array 'xx%_data%zzz' below lower bound of 1 I believe you were recalling bogus warnings at compile time. There are no warnings here, and there shouldn't.
Re: [PATCH] Fortran: Mark internal symbols as artificial [PR88009,PR68800]
On Wed, 17 Nov 2021 21:32:14 +0100
Harald Anlauf wrote:
> Do you have testcases/reproducers demonstrating that the patch actually
> fixes the issues you're describing?
I believe that marking artificial symbols as such is obvious and i did
use the existing tests to verify that the changes do not regress but
behave as intended. I did check that the memory leak in
gfc_find_derived_vtab is fixed with the patch.
Ok for stage 1 if the rebased regression test passes?
thanks
>
> Am 17.11.21 um 09:12 schrieb Bernhard Reutner-Fischer via Gcc-patches:
> > On Tue, 16 Nov 2021 21:46:32 +0100
> > Harald Anlauf via Fortran wrote:
> >
> >> Hi Bernhard,
> >>
> >> I'm trying to understand your patch. What does it really try to solve?
> >
> > Compiler generated symbols should be marked artificial.
> > The fix for PR88009 ( f8add009ce300f24b75e9c2e2cc5dd944a020c28 ,
> > r9-5194 ) added artificial just to the _final component and left out all
> > the rest.
> > Note that the majority of compiler generated symbols in class.c
> > already had artificial set properly.
> > The proposed patch amends the other generated symbols to be marked
> > artificial, too.
> >
> > The other parts fix memory leaks.
> >
> >>
> >> PR88009 is closed and seems to have nothing to do with this.
> >
> > Well it marked only _final as artificial and forgot to adjust the
> > others as well.
> > We can remove the reference to PR88009 if you prefer?
> >
> > thanks!
> >>
> >> Harald
> >>
> >> Am 14.11.21 um 23:17 schrieb Bernhard Reutner-Fischer via Fortran:
> >>> Hi!
> >>>
> >>> Amend fix for PR88009 to mark all these class components as artificial.
> >>>
> >>> gcc/fortran/ChangeLog:
> >>>
> >>> * class.c (gfc_build_class_symbol,
> >>> generate_finalization_wrapper,
> >>> (gfc_find_derived_vtab, find_intrinsic_vtab): Use stringpool for
> >>> names. Mark internal symbols as artificial.
> >>> * decl.c (gfc_match_decl_type_spec, gfc_match_end): Fix
> >>> indentation.
> >>> (gfc_match_derived_decl): Fix indentation. Check extension level
> >>> before incrementing refs counter.
> >>> * parse.c (parse_derived): Fix style.
> >>> * resolve.c (resolve_global_procedure): Likewise.
> >>> * symbol.c (gfc_check_conflict): Do not ignore artificial
> >>> symbols.
> >>> (gfc_add_flavor): Reorder condition, cheapest first.
> >>> (gfc_new_symbol, gfc_get_sym_tree,
> >>> generate_isocbinding_symbol): Fix style.
> >>> * trans-expr.c (gfc_trans_subcomponent_assign): Remove
> >>> restriction on !artificial.
> >>> * match.c (gfc_match_equivalence): Special-case CLASS_DATA for
> >>> warnings.
> >>>
> >>> ---
> >>> gfc_match_equivalence(), too, should not bail-out early on the first
> >>> error but should diagnose all errors. I.e. not goto cleanup but set
> >>> err=true and continue in order to diagnose all constraints of a
> >>> statement. Maybe Sandra or somebody else will eventually find time to
> >>> tweak that.
> >>>
> >>> I think it also plugs a very minor leak of name in gfc_find_derived_vtab
> >>> so i also tagged it [PR68800]. At least that was the initial
> >>> motiviation to look at that spot.
> >>> We were doing
> >>> - name = xasprintf ("__vtab_%s", tname);
> >>> ...
> >>> gfc_set_sym_referenced (vtab);
> >>> - name = xasprintf ("__vtype_%s", tname);
> >>>
> >>> Bootstrapped and regtested without regressions on x86_64-unknown-linux.
> >>> Ok for trunk?
> >>>
> >>
> >>
> >
> >
>
Re: [PATCH] Fortran: use name of array component in runtime error message [PR30802]
Am 29.01.24 um 18:25 schrieb Harald Anlauf:
I was talking about the generated format strings of runtime error
messages.
program p
implicit none
type t
real :: zzz(10) = 42
end type t
class(t), allocatable :: xx(:)
integer :: j
j = 0
allocate (t :: xx(1))
print *, xx(1)% zzz(j)
end
This is generating the following error at runtime since at least gcc-7:
Fortran runtime error: Index '0' of dimension 1 of array 'xx%_data%zzz'
below lower bound of 1
Of course this is easily suppressed by:
diff --git a/gcc/fortran/trans-array.cc b/gcc/fortran/trans-array.cc
index 1e0d698a949..fa0e00a28a6 100644
--- a/gcc/fortran/trans-array.cc
+++ b/gcc/fortran/trans-array.cc
@@ -4054,7 +4054,8 @@ gfc_conv_array_ref (gfc_se * se, gfc_array_ref *
ar, gfc_expr *expr,
{
if (ref->type == REF_ARRAY && &ref->u.ar == ar)
break;
- if (ref->type == REF_COMPONENT)
+ if (ref->type == REF_COMPONENT
+ && strcmp (ref->u.c.component->name, "_data") != 0)
{
strcat (var_name, "%%");
strcat (var_name, ref->u.c.component->name);
I have been contemplating the generation the full chain of references as
suggested by Mikael and supported by NAG. The main issue is: how do I
easily generate that call?
gfc_trans_runtime_check is a vararg function, but what I would rather
have is a function that takes either a (chained?) list of trees or
an array of trees holding the (co-)indices of the reference.
Is there an example, or a recommendation which variant to prefer?
Thanks,
Harald
Re: [PATCH] Fortran: Mark internal symbols as artificial [PR88009,PR68800]
Am 29.01.24 um 21:45 schrieb Bernhard Reutner-Fischer:
On Wed, 17 Nov 2021 21:32:14 +0100
Harald Anlauf wrote:
Do you have testcases/reproducers demonstrating that the patch actually
fixes the issues you're describing?
I believe that marking artificial symbols as such is obvious and i did
use the existing tests to verify that the changes do not regress but
behave as intended. I did check that the memory leak in
gfc_find_derived_vtab is fixed with the patch.
Ok for stage 1 if the rebased regression test passes?
thanks
Am 17.11.21 um 09:12 schrieb Bernhard Reutner-Fischer via Gcc-patches:
On Tue, 16 Nov 2021 21:46:32 +0100
Harald Anlauf via Fortran wrote:
Hi Bernhard,
I'm trying to understand your patch. What does it really try to solve?
Compiler generated symbols should be marked artificial.
The fix for PR88009 ( f8add009ce300f24b75e9c2e2cc5dd944a020c28 ,
r9-5194 ) added artificial just to the _final component and left out all the
rest.
Note that the majority of compiler generated symbols in class.c
already had artificial set properly.
The proposed patch amends the other generated symbols to be marked
artificial, too.
The other parts fix memory leaks.
PR88009 is closed and seems to have nothing to do with this.
Well it marked only _final as artificial and forgot to adjust the
others as well.
We can remove the reference to PR88009 if you prefer?
thanks!
Harald
Am 14.11.21 um 23:17 schrieb Bernhard Reutner-Fischer via Fortran:
Hi!
Amend fix for PR88009 to mark all these class components as artificial.
gcc/fortran/ChangeLog:
* class.c (gfc_build_class_symbol, generate_finalization_wrapper,
(gfc_find_derived_vtab, find_intrinsic_vtab): Use stringpool for
names. Mark internal symbols as artificial.
* decl.c (gfc_match_decl_type_spec, gfc_match_end): Fix
indentation.
(gfc_match_derived_decl): Fix indentation. Check extension level
before incrementing refs counter.
* parse.c (parse_derived): Fix style.
* resolve.c (resolve_global_procedure): Likewise.
* symbol.c (gfc_check_conflict): Do not ignore artificial symbols.
(gfc_add_flavor): Reorder condition, cheapest first.
(gfc_new_symbol, gfc_get_sym_tree,
generate_isocbinding_symbol): Fix style.
* trans-expr.c (gfc_trans_subcomponent_assign): Remove
restriction on !artificial.
* match.c (gfc_match_equivalence): Special-case CLASS_DATA for
warnings.
---
gfc_match_equivalence(), too, should not bail-out early on the first
error but should diagnose all errors. I.e. not goto cleanup but set
err=true and continue in order to diagnose all constraints of a
statement. Maybe Sandra or somebody else will eventually find time to
tweak that.
I think it also plugs a very minor leak of name in gfc_find_derived_vtab
so i also tagged it [PR68800]. At least that was the initial
motiviation to look at that spot.
We were doing
- name = xasprintf ("__vtab_%s", tname);
...
gfc_set_sym_referenced (vtab);
- name = xasprintf ("__vtype_%s", tname);
Bootstrapped and regtested without regressions on x86_64-unknown-linux.
Ok for trunk?
Can you please post the patch here so that we can review it?
Re: [PATCH] Fortran: Mark internal symbols as artificial [PR88009,PR68800]
On 29 January 2024 22:06:04 CET, Harald Anlauf wrote:
>Am 29.01.24 um 21:45 schrieb Bernhard Reutner-Fischer:
>> On Wed, 17 Nov 2021 21:32:14 +0100
>> Harald Anlauf wrote:
>>
>>> Do you have testcases/reproducers demonstrating that the patch actually
>>> fixes the issues you're describing?
>>
>> I believe that marking artificial symbols as such is obvious and i did
>> use the existing tests to verify that the changes do not regress but
>> behave as intended. I did check that the memory leak in
>> gfc_find_derived_vtab is fixed with the patch.
>>
>> Ok for stage 1 if the rebased regression test passes?
>>
>> thanks
>>
>>>
>>> Am 17.11.21 um 09:12 schrieb Bernhard Reutner-Fischer via Gcc-patches:
On Tue, 16 Nov 2021 21:46:32 +0100
Harald Anlauf via Fortran wrote:
> Hi Bernhard,
>
> I'm trying to understand your patch. What does it really try to solve?
Compiler generated symbols should be marked artificial.
The fix for PR88009 ( f8add009ce300f24b75e9c2e2cc5dd944a020c28 ,
r9-5194 ) added artificial just to the _final component and left out all
the rest.
Note that the majority of compiler generated symbols in class.c
already had artificial set properly.
The proposed patch amends the other generated symbols to be marked
artificial, too.
The other parts fix memory leaks.
>
> PR88009 is closed and seems to have nothing to do with this.
Well it marked only _final as artificial and forgot to adjust the
others as well.
We can remove the reference to PR88009 if you prefer?
thanks!
>
> Harald
>
> Am 14.11.21 um 23:17 schrieb Bernhard Reutner-Fischer via Fortran:
>> Hi!
>>
>> Amend fix for PR88009 to mark all these class components as artificial.
>>
>> gcc/fortran/ChangeLog:
>>
>>* class.c (gfc_build_class_symbol,
>> generate_finalization_wrapper,
>>(gfc_find_derived_vtab, find_intrinsic_vtab): Use stringpool
>> for
>>names. Mark internal symbols as artificial.
>>* decl.c (gfc_match_decl_type_spec, gfc_match_end): Fix
>>indentation.
>>(gfc_match_derived_decl): Fix indentation. Check extension
>> level
>>before incrementing refs counter.
>>* parse.c (parse_derived): Fix style.
>>* resolve.c (resolve_global_procedure): Likewise.
>>* symbol.c (gfc_check_conflict): Do not ignore artificial
>> symbols.
>>(gfc_add_flavor): Reorder condition, cheapest first.
>>(gfc_new_symbol, gfc_get_sym_tree,
>>generate_isocbinding_symbol): Fix style.
>>* trans-expr.c (gfc_trans_subcomponent_assign): Remove
>>restriction on !artificial.
>>* match.c (gfc_match_equivalence): Special-case CLASS_DATA for
>>warnings.
>>
>> ---
>> gfc_match_equivalence(), too, should not bail-out early on the first
>> error but should diagnose all errors. I.e. not goto cleanup but set
>> err=true and continue in order to diagnose all constraints of a
>> statement. Maybe Sandra or somebody else will eventually find time to
>> tweak that.
>>
>> I think it also plugs a very minor leak of name in gfc_find_derived_vtab
>> so i also tagged it [PR68800]. At least that was the initial
>> motiviation to look at that spot.
>> We were doing
>> - name = xasprintf ("__vtab_%s", tname);
>> ...
>> gfc_set_sym_referenced (vtab);
>> - name = xasprintf ("__vtype_%s", tname);
>>
>> Bootstrapped and regtested without regressions on x86_64-unknown-linux.
>> Ok for trunk?
>>
>
>
>>>
>>
>>
>
>Can you please post the patch here so that we can review it?
>
I'm very sorry that I missed to provide an explicit reference, the initial
patch was submitted here:
https://inbox.sourceware.org/fortran/2024231748.376086cd@nbbrfq/
But I will follow-up with a rebased, tested patch ASAP during a weekend or
vacation.
But just to give context, that's what I was talking about..
thanks
PS: IMHO a strcmp(..,"_data") is inappropriate for you should check
attr.artificial and the path exploder to give hints should ideally deal with
this transparently -- IMHO
