Re: [Patch, fortran] PR108961 - Segfault when associating to pointer from C_F_POINTER

[Paul Richard Thomas via Fortran]

Committed as r14-2021-gcaf0892eea67349d9a1e44590c3440768136fe2b

Thanks for the pointers, Tobias and Mikael, I used them both.

Paul

On Tue, 20 Jun 2023 at 21:47, Mikael Morin  wrote:
>
> Le 20/06/2023 à 18:30, Tobias Burnus a écrit :
> > On 20.06.23 18:19, Paul Richard Thomas via Fortran wrote:
> >
> >> Is there a better way to detect a type(c_ptr) formal argument?
> > u.derived->intmod_sym_id == ISOCBINDING_PTR ?
> && u.derived->from_intmod == INTMOD_ISO_C_BINDING ?
>


-- 
"If you can't explain it simply, you don't understand it well enough"
- Albert Einstein

Re: [Patch, fortran] PR107900 Select type with intrinsic type inside associate causes ICE / Segmenation fault

[Paul Richard Thomas via Fortran]

Committed as r14-2022-g577223aebc7acdd31e62b33c1682fe54a622ae27

Thanks for the help and the review Harald. Thanks to Steve too for
picking up Neil Carlson's bugs.

Cheers

Paul

On Tue, 20 Jun 2023 at 22:57, Harald Anlauf  wrote:
>
> Hi Paul,
>
> On 6/20/23 12:54, Paul Richard Thomas via Gcc-patches wrote:
> > Hi Harald,
> >
> > Fixing the original testcase in this PR turned out to be slightly more
> > involved than I expected. However, it resulted in an open door to fix
> > some other PRs and the attached much larger patch.
> >
> > This time, I did remember to include the testcases in the .diff :-)
>
> indeed! :-)
>
> I've only had a superficial look so far although it looks very good.
> (I have to trust your experience with unlimited polymorphism.)
>
> However, I was wondering about the following helper function:
>
> +bool
> +gfc_is_ptr_fcn (gfc_expr *e)
> +{
> +  return e != NULL && e->expr_type == EXPR_FUNCTION
> + && (gfc_expr_attr (e).pointer
> + || (e->ts.type == BT_CLASS
> + && CLASS_DATA (e)->attr.class_pointer));
> +}
> +
> +
>   /* Copy a shape array.  */
>
> Is there a case where gfc_expr_attr (e).pointer returns false
> and you really need the || part?  Looking at gfc_expr_attr
> and the present context, it might just not be necessary.
>
> > I believe that, between the Change.Logs and the comments, it is
> > reasonably self-explanatory.
> >
> > OK for trunk?
>
> OK from my side.
>
> Thanks for the patch!
>
> Harald
>
> > Regards
> >
> > Paul
> >
> > Fortran: Fix some bugs in associate [PR87477]
> >
> > 2023-06-20  Paul Thomas  
> >
> > gcc/fortran
> > PR fortran/87477
> > PR fortran/88688
> > PR fortran/94380
> > PR fortran/107900
> > PR fortran/110224
> > * decl.cc (char_len_param_value): Fix memory leak.
> > (resolve_block_construct): Remove unnecessary static decls.
> > * expr.cc (gfc_is_ptr_fcn): New function.
> > (gfc_check_vardef_context): Use it to permit pointer function
> > result selectors to be used for associate names in variable
> > definition context.
> > * gfortran.h: Prototype for gfc_is_ptr_fcn.
> > * match.cc (build_associate_name): New function.
> > (gfc_match_select_type): Use the new function to replace inline
> > version and to build a new associate name for the case where
> > the supplied associate name is already used for that purpose.
> > * resolve.cc (resolve_assoc_var): Call gfc_is_ptr_fcn to allow
> > associate names with pointer function targets to be used in
> > variable definition context.
> > * trans-decl.cc (gfc_get_symbol_decl): Unlimited polymorphic
> > variables need deferred initialisation of the vptr.
> > (gfc_trans_deferred_vars): Do the vptr initialisation.
> > * trans-stmt.cc (trans_associate_var): Ensure that a pointer
> > associate name points to the target of the selector and not
> > the selector itself.
> >
> > gcc/testsuite/
> > PR fortran/87477
> > PR fortran/107900
> > * gfortran.dg/pr107900.f90 : New test
> >
> > PR fortran/110224
> > * gfortran.dg/pr110224.f90 : New test
> >
> > PR fortran/88688
> > * gfortran.dg/pr88688.f90 : New test
> >
> > PR fortran/94380
> > * gfortran.dg/pr94380.f90 : New test
> >
> > PR fortran/95398
> > * gfortran.dg/pr95398.f90 : Set -std=f2008, bump the line
> > numbers in the error tests by two and change the text in two.
>


-- 
"If you can't explain it simply, you don't understand it well enough"
- Albert Einstein

Re: [Patch, fortran] PR107900 Select type with intrinsic type inside associate causes ICE / Segmenation fault

[Steve Kargl via Fortran]

On Wed, Jun 21, 2023 at 05:12:22PM +0100, Paul Richard Thomas wrote:
> Committed as r14-2022-g577223aebc7acdd31e62b33c1682fe54a622ae27
> 
> Thanks for the help and the review Harald. Thanks to Steve too for
> picking up Neil Carlson's bugs.
> 

It's only natural.  You fix bugs in a long desired feature,
and people will start to use that feature more. 

I always look at Neil's bug reports.  They're typically
concise code snippets and have cross references to the
Fortran standard.  Unfortunately, I lack the ability to
fix them. :( 

-- 
Steve

Re: [Patch, fortran] PR107900 Select type with intrinsic type inside associate causes ICE / Segmenation fault

[Harald Anlauf via Fortran]

Hi Paul,

while I only had a minor question regarding gfc_is_ptr_fcn(),
can you still try to enlighten me why that second part
was necessary?  (I believed it to be redundant and may have
overlooked the obvious.)

Cheers,
Harald

On 6/21/23 18:12, Paul Richard Thomas via Gcc-patches wrote:

Committed as r14-2022-g577223aebc7acdd31e62b33c1682fe54a622ae27

Thanks for the help and the review Harald. Thanks to Steve too for
picking up Neil Carlson's bugs.

Cheers

Paul

On Tue, 20 Jun 2023 at 22:57, Harald Anlauf  wrote:


Hi Paul,

On 6/20/23 12:54, Paul Richard Thomas via Gcc-patches wrote:

Hi Harald,

Fixing the original testcase in this PR turned out to be slightly more
involved than I expected. However, it resulted in an open door to fix
some other PRs and the attached much larger patch.

This time, I did remember to include the testcases in the .diff :-)


indeed! :-)

I've only had a superficial look so far although it looks very good.
(I have to trust your experience with unlimited polymorphism.)

However, I was wondering about the following helper function:

+bool
+gfc_is_ptr_fcn (gfc_expr *e)
+{
+  return e != NULL && e->expr_type == EXPR_FUNCTION
+ && (gfc_expr_attr (e).pointer
+ || (e->ts.type == BT_CLASS
+ && CLASS_DATA (e)->attr.class_pointer));
+}
+
+
   /* Copy a shape array.  */

Is there a case where gfc_expr_attr (e).pointer returns false
and you really need the || part?  Looking at gfc_expr_attr
and the present context, it might just not be necessary.


I believe that, between the Change.Logs and the comments, it is
reasonably self-explanatory.

OK for trunk?


OK from my side.

Thanks for the patch!

Harald


Regards

Paul

Fortran: Fix some bugs in associate [PR87477]

2023-06-20  Paul Thomas  

gcc/fortran
PR fortran/87477
PR fortran/88688
PR fortran/94380
PR fortran/107900
PR fortran/110224
* decl.cc (char_len_param_value): Fix memory leak.
(resolve_block_construct): Remove unnecessary static decls.
* expr.cc (gfc_is_ptr_fcn): New function.
(gfc_check_vardef_context): Use it to permit pointer function
result selectors to be used for associate names in variable
definition context.
* gfortran.h: Prototype for gfc_is_ptr_fcn.
* match.cc (build_associate_name): New function.
(gfc_match_select_type): Use the new function to replace inline
version and to build a new associate name for the case where
the supplied associate name is already used for that purpose.
* resolve.cc (resolve_assoc_var): Call gfc_is_ptr_fcn to allow
associate names with pointer function targets to be used in
variable definition context.
* trans-decl.cc (gfc_get_symbol_decl): Unlimited polymorphic
variables need deferred initialisation of the vptr.
(gfc_trans_deferred_vars): Do the vptr initialisation.
* trans-stmt.cc (trans_associate_var): Ensure that a pointer
associate name points to the target of the selector and not
the selector itself.

gcc/testsuite/
PR fortran/87477
PR fortran/107900
* gfortran.dg/pr107900.f90 : New test

PR fortran/110224
* gfortran.dg/pr110224.f90 : New test

PR fortran/88688
* gfortran.dg/pr88688.f90 : New test

PR fortran/94380
* gfortran.dg/pr94380.f90 : New test

PR fortran/95398
* gfortran.dg/pr95398.f90 : Set -std=f2008, bump the line
numbers in the error tests by two and change the text in two.

Re: [Patch, fortran] PR107900 Select type with intrinsic type inside associate causes ICE / Segmenation fault

[Bernhard Reutner-Fischer via Fortran]

Hi!

First of all, many thanks for the patch!
If i may, i have a question concerning the chosen style in the error
message and one nitpick concerning a return type though, the latter
primarily prompting this reply.

On Tue, 20 Jun 2023 11:54:25 +0100
Paul Richard Thomas via Fortran  wrote:

> diff --git a/gcc/fortran/expr.cc b/gcc/fortran/expr.cc
> index d5cfbe0cc55..c960dfeabd9 100644
> --- a/gcc/fortran/expr.cc
> +++ b/gcc/fortran/expr.cc

> @@ -6470,6 +6480,22 @@ gfc_check_vardef_context (gfc_expr* e, bool pointer, 
> bool alloc_obj,
>   }
> return false;
>   }
> +  else if (context && gfc_is_ptr_fcn (assoc->target))
> + {
> +   if (!gfc_notify_std (GFC_STD_F2018, "%qs at %L associated to "
> +"pointer function target being used in a "
> +"variable definition context (%s)", name,
> +&e->where, context))

I'm curious why you decided to put context in braces and not simply use
quotes as per %qs?

> + return false;
> +   else if (gfc_has_vector_index (e))
> + {
> +   gfc_error ("%qs at %L associated to vector-indexed target"
> +  " cannot be used in a variable definition"
> +  " context (%s)",
> +  name, &e->where, context);

Ditto.

> diff --git a/gcc/fortran/match.cc b/gcc/fortran/match.cc
> index e7be7fddc64..0e4b5440393 100644
> --- a/gcc/fortran/match.cc
> +++ b/gcc/fortran/match.cc
> @@ -6377,6 +6377,39 @@ build_class_sym:
>  }
> 
> 
> +/* Build the associate name  */
> +static int
> +build_associate_name (const char *name, gfc_expr **e1, gfc_expr **e2)
> +{

> +return 1;

> +  return 0;
> +}

I've gone through the frontend recently and changed several such
boolean functions to use bool where appropriate. May i ask folks to use
narrower types in new code, please?
Iff later in the pipeline it is considered appropriate or benefical to
promote types, these will eventually be promoted.

> diff --git a/gcc/fortran/trans-decl.cc b/gcc/fortran/trans-decl.cc
> index e6a4337c0d2..18589e17843 100644
> --- a/gcc/fortran/trans-decl.cc
> +++ b/gcc/fortran/trans-decl.cc

> @@ -1906,6 +1915,7 @@ gfc_get_symbol_decl (gfc_symbol * sym)
>   gcc_assert (!sym->value || sym->value->expr_type == EXPR_NULL);
>  }
> 
> +

ISTM that the addition of vertical whitespace like here is in
contradiction with the coding style.

Please kindly excuse my comment and, again, thanks!

>gfc_finish_var_decl (decl, sym);
> 
>if (sym->ts.type == BT_CHARACTER)

Re: [PATCH] Introduce hardbool attribute for C

[Alexandre Oliva via Fortran]

Thanks for the test.

Did you mean for me to incorporate it into the patch, or do you mean to
contribute it separately, if the feature happens to be accepted?

On Jun 19, 2023, Bernhard Reutner-Fischer  wrote:

> I don't see explicit tests with _Complex nor __complex__. Would we
> want to check these here, or are they handled thought the "underlying"
> tests above?

Good question.  The notion of using complex types to hold booleans
hadn't even crossed my mind.

On the one hand, there doesn't seem to be reason to rule them out, and
that could go for literally any other type.

On the other, there doesn't seem to be any useful case for them.  Can
anyone think of one?

> I'd welcome a fortran interop note in the docs

Is there any good place for such interop notes?  I'm not sure I'm
best-suited to write them up, since Fortran is not a language I'm
very familiar with, but I suppose I could give it a try.

-- 
Alexandre Oliva, happy hackerhttps://FSFLA.org/blogs/lxo/
   Free Software Activist   GNU Toolchain Engineer
Disinformation flourishes because many people care deeply about injustice
but very few check the facts.  Ask me about 

Re: [Patch, fortran] PR107900 Select type with intrinsic type inside associate causes ICE / Segmenation fault

[Paul Richard Thomas via Fortran]

Hi Both,

> while I only had a minor question regarding gfc_is_ptr_fcn(),
> can you still try to enlighten me why that second part
> was necessary?  (I believed it to be redundant and may have
> overlooked the obvious.)

Blast! I forgot about checking that. Lurking in the back of my mind
and going back to the first days of OOP in gfortran is a distinction
between a class entity with the pointer attribute and one whose data
component has the class_pointer attribute. I'll check it out and do
whatever is needed.


> > +  else if (context && gfc_is_ptr_fcn (assoc->target))
> > + {
> > +   if (!gfc_notify_std (GFC_STD_F2018, "%qs at %L associated to "
> > +"pointer function target being used in a "
> > +"variable definition context (%s)", name,
> > +&e->where, context))
>
> I'm curious why you decided to put context in braces and not simply use
> quotes as per %qs?

That's the way it's done in the preceding errors. I had to keep the
context in context, so to speak.

> > +/* Build the associate name  */
> > +static int
> > +build_associate_name (const char *name, gfc_expr **e1, gfc_expr **e2)
> > +{
>
> > +return 1;
>
> > +  return 0;
> > +}
>
> I've gone through the frontend recently and changed several such
> boolean functions to use bool where appropriate. May i ask folks to use
> narrower types in new code, please?
> Iff later in the pipeline it is considered appropriate or benefical to
> promote types, these will eventually be promoted.
>
> > diff --git a/gcc/fortran/trans-decl.cc b/gcc/fortran/trans-decl.cc
> > index e6a4337c0d2..18589e17843 100644
> > --- a/gcc/fortran/trans-decl.cc
> > +++ b/gcc/fortran/trans-decl.cc
>
> > @@ -1906,6 +1915,7 @@ gfc_get_symbol_decl (gfc_symbol * sym)
> >   gcc_assert (!sym->value || sym->value->expr_type == EXPR_NULL);
> >  }
> >
> > +
>

'twas accidental. There had previously been another version of the fix
that I commented out and the extra line crept in when I deleted it.
Thanks for the spot.

>
> Please kindly excuse my comment and, again, thanks!
>
> >gfc_finish_var_decl (decl, sym);
> >
> >if (sym->ts.type == BT_CHARACTER)

Regards

Paul