Need help with vptr copy function calls

Need help with vptr copy function calls

[FX]

Hi everyone,

Long time, no see. I hope you’re all doing OK!


I am currently helping Iain Sandoe port GCC & gfortran to aarch64-apple-darwin, i.e. the new ARM-based Apple hardware for macOS. We are having one problem, that impacts a lot of Fortran class code:

$ cat associate_13.f90                                                  
  type :: mytype
    integer :: i
  end type
  type :: myderivedtype
     class(mytype), allocatable :: x
  end type
  type(mytype) :: z
  type(myderivedtype) :: toto

  z = mytype(99)
  allocate (toto%x)
  toto%x = z
end
$ ./bin/gfortran associate_13.f90 && ./a.out
Program received signal SIGBUS: Access to an undefined portion of a memory object.

You can read the full debugging there: https://github.com/iains/gcc-darwin-arm64/issues/9

The problem for this is the line: toto%x = z, which calls a copy function __copy_MAIN___Mytype. Now, the copy function itself is well defined, and the code generated is valid. However, the code generated for the call itself is not.

When the call to toto.x._vptr->_copy() is made, the type of the function is wrong:

 <function_type 0x14461be70
    type <void_type 0x14460ff18 void VOID
        align:8 warn_if_not_align:0 symtab:0 alias-set -1 canonical-type 0x14460ff18
        pointer_to_this <pointer_type 0x144617000>>
    SI
    size <integer_cst 0x144602eb8 type <integer_type 0x14460f0a8 bitsizetype> constant 32>
    unit-size <integer_cst 0x144602ed0 type <integer_type 0x14460f000 sizetype> constant 4>
    align:32 warn_if_not_align:0 symtab:0 alias-set -1 canonical-type 0x14461be70
    arg-types <tree_list 0x1446097f8 value <void_type 0x14460ff18 void>>
    pointer_to_this <pointer_type 0x14461bf18>>

i.e. it’s indicated as a function with no arg, not a function with two args, whose correct prototype would be:

__copy_MAIN___Mytype (struct mytype & restrict, struct mytype & restrict)

It appears that this is not a problem on other platforms, but on aarch64-apple-darwin the ABI for variadic functions and “normal” functions are very different, hence the bug.


Now, my gcc-fu is rusty, but I think that calling the function without the correct prototype is wrong (I remember fixing a bunch of those issues, many years back). I’m not sure exactly what a patch would look like, but I would need some help:

- can you confirm that this situation should not happen?
- do you have an idea how to fix it?

Thanks
FX

Re: Need help with vptr copy function calls

[Tobias Burnus]

Hi FX,

On 9/4/20 12:13 AM, FX via Fortran wrote:
> - can you confirm that this situation should not happen?

Concur. We recently also had an issue on PowerPC because
due to a wrong prototype, argument passing (registers vs.
stack) was mangled.

> - do you have an idea how to fix it?

In class.c, search for "/* Add component _copy.  */";
for the "if" branch (unlimited_polymorphic),
I think you want to use "void *" = "type(c_ptr),VALUE" for
the arguments; see in the "else" branch for where to place
the formal arguments.

The c_ptr you can get using something like:

  dt = generate_isocbinding_symbol ("__iso_c_binding",
                                    (iso_c_binding_symbol)ISOCBINDING_PTR,
                                    NULL, NULL, /*hidden=*/ true);

Cheers,

Tobias

-----------------
Mentor Graphics (Deutschland) GmbH, Arnulfstraße 201, 80634 München / Germany
Registergericht München HRB 106955, Geschäftsführer: Thomas Heurung, Alexander Walter

Re: Need help with vptr copy function calls

[FX]

Hi Tobias,

> In class.c, search for "/* Add component _copy.  */";
> for the "if" branch (unlimited_polymorphic),
> I think you want to use "void *" = "type(c_ptr),VALUE" for
> the arguments; see in the "else" branch for where to place
> the formal arguments.

I don’t think the problem is there. The code I am working with is a simple derived type, so the unlimited_polymorphic branch is not taken, the other one is: and that branch sets up the argument types properly.

I can see that because the function is emitted correctly:

__copy_MAIN___Mytype (struct mytype & restrict src, struct mytype & restrict dst)
{
  *dst = *src;
}

What is not correct is the type of the fndecl. In trans-expr.c’s trans_class_assignment(), you have:

  fcn = gfc_vptr_copy_get (vptr);

That fcn tree is:

 MAIN__ __copy_MAIN___Mytype <pointer_type 0x145cc3e58
    type <function_type 0x145cc3db0
        type <void_type 0x145cc0f18 void VOID
            align:8 warn_if_not_align:0 symtab:0 alias-set -1 canonical-type 0x145cc0f18
            pointer_to_this <pointer_type 0x145cc0fc0>>
        SI
        size <integer_cst 0x145c90eb8 constant 32>
        unit-size <integer_cst 0x145c90ed0 constant 4>
        align:32 warn_if_not_align:0 symtab:0 alias-set -1 canonical-type 0x145cc3db0
        arg-types <tree_list 0x145cac7f8 value <void_type 0x145cc0f18 void>>
        pointer_to_this <pointer_type 0x145cc3e58>>
    public unsigned DI
    size <integer_cst 0x145c90c78 type <integer_type 0x145cc00a8 bitsizetype> constant 64>
    unit-size <integer_cst 0x145c90c90 type <integer_type 0x145cc0000 sizetype> constant 8>
    align:64 warn_if_not_align:0 symtab:0 alias-set -1 canonical-type 0x145cc3e58>

which has an empty list as arg-types. The question I cannot answer is: where is that function decl created?

FX

Re: Need help with vptr copy function calls

[Tobias Burnus]

Hi FX,

On 9/4/20 9:19 PM, FX via Fortran wrote:
>> In class.c, search for "/* Add component _copy.  */";
>> for the "if" branch (unlimited_polymorphic),
>> I think you want to use "void *" = "type(c_ptr),VALUE" for
>> the arguments; see in the "else" branch for where to place
>> the formal arguments.
> I don’t think the problem is there. The code I am working with is a simple derived type, so the unlimited_polymorphic branch is not taken,

Okay, I looked at the "wrong" example: Issue 9 on github, which has:

|$ cat allocate_with_source_7.f08 program test type P class(*), 
allocatable :: X(:) end type Type(P) Y integer, parameter :: arr(1) = (/ 
5 /) allocate(Y%X, source=arr) end At least for that one, I see 
"class(*)". And I do think that this has to be fixed – even if it is not 
the issue you are currently seeing. |

> the other one is: and that branch sets up the argument types properly.

Then, probably the procedure-pointer is not properly setup. The component
is setup in class.c – cf. below "Add component _copy." and that's turned
into a procedure-pointer in  trans-types.c –  I think in gfc_get_ppc_type.
It seems as if there the explicit interface is not properly used.

Tobias

Re: Need help with vptr copy function calls

[FX]

Hi again,

There are apparently other functions with the same problem… so I am wondering if this is possibly a wider issue. Take this code:

------------
module m
 implicit none
 type, abstract :: t1
  logical :: l
 end type t1
 type, extends(t1), abstract :: t2
  integer :: i
 end type t2
 type, extends(t2) :: t3
  real :: x
 end type t3
contains
 subroutine s(u)
  class(t1), intent(in) :: u
   select type(u); class is(t2)
     select type(u); class is(t3)
     end select
   end select
 end subroutine s
end module m

program p
 use m
 implicit none
 type(t3) :: var = t3( l=.true. , i=2 , x=3.5 )
 call s(var)
end program p
------------

The select type construct is translated into series of “if _gfortran_is_extension_of()”. But the function call has the wrong type (well, no type) for arguments. If you look into gfc_conv_procedure_call(), after the function type is determined:

fntype = TREE_TYPE (TREE_TYPE (se->expr))

then the fntype for _gfortran_is_extension_of is:

 <function_type 0x14322da40
    type <boolean_type 0x14309b918 logical(kind=4) public unsigned SI
        size <integer_cst 0x143068eb8 constant 32>
        unit-size <integer_cst 0x143068ed0 constant 4>
        align:32 warn_if_not_align:0 symtab:0 alias-set -1 canonical-type 0x14309b918 precision:1 min <integer_cst 0x143069170 0> max <integer_cst 0x1430691a0 1>>
    SI size <integer_cst 0x143068eb8 32> unit-size <integer_cst 0x143068ed0 4>
    align:32 warn_if_not_align:0 symtab:0 alias-set -1 canonical-type 0x14322d998
    attributes <tree_list 0x143239ea0
        purpose <identifier_node 0x1430f45c8 fn spec>
        value <tree_list 0x143239e78
            value <string_cst 0x1430e6160 constant ".">>>
    arg-types <tree_list 0x1430847f8
        value <void_type 0x143098f18 void VOID
            align:8 warn_if_not_align:0 symtab:0 alias-set -1 canonical-type 0x143098f18
            pointer_to_this <pointer_type 0x143098fc0>>>
    pointer_to_this <pointer_type 0x14322dae8>>

Note the arg-types is an empty list. We’re declaring it as if the function had no arguments, although it has two.

Not all code paths in the compiler lead to this. If you compile code that explicitly calls EXTENDS_TYPE_OF, like this one:

----------
module m
 implicit none
 type, abstract :: t1
  logical :: l
 end type t1
 type, extends(t1), abstract :: t2
  integer :: i
 end type t2
contains
 subroutine s(u, v)
  class(t1), intent(in) :: u, v
  print *, extends_type_of(u, v)
 end subroutine s
end module m
-----------

then you can see the arg-types are good:

 <function_type 0x143dd9650
    type <boolean_type 0x143c47918 logical(kind=4) public unsigned SI
        size <integer_cst 0x143c14eb8 constant 32>
        unit-size <integer_cst 0x143c14ed0 constant 4>
        align:32 warn_if_not_align:0 symtab:0 alias-set -1 canonical-type 0x143c47918 precision:1 min <integer_cst 0x143c15170 0> max <integer_cst 0x143c151a0 1>>
    SI size <integer_cst 0x143c14eb8 32> unit-size <integer_cst 0x143c14ed0 4>
    align:32 warn_if_not_align:0 symtab:0 alias-set -1 canonical-type 0x143dd95a8
    attributes <tree_list 0x143de5860
        purpose <identifier_node 0x143ca05c8 fn spec>
        value <tree_list 0x143de5838
            value <string_cst 0x143c92160 constant ".rr">>>
    arg-types <tree_list 0x143de5810
        value <reference_type 0x143dd9500 type <record_type 0x143ddbbb8 __vtype_m_T1>
            unsigned restrict DI
            size <integer_cst 0x143c14c78 constant 64>
            unit-size <integer_cst 0x143c14c90 constant 8>
            align:64 warn_if_not_align:0 symtab:0 alias-set -1 canonical-type 0x143dd9500>
        chain <tree_list 0x143de57e8 value <reference_type 0x143dd9500>
            chain <tree_list 0x143c307f8 value <void_type 0x143c44f18 void>>>>
    pointer_to_this <pointer_type 0x143dd96f8>>


It’s a different bug, and yet, it feels similar. Both lead to wrong code generation on aarch64-apple-darwin, whose ABI has more differences than most between named and variadic function args.


I tried to set up the formal arg list to fix this in resolve.c’s resolve_select_type(), like this:

--- a/gcc/fortran/resolve.c
+++ b/gcc/fortran/resolve.c
@@ -9635,6 +9635,9 @@ resolve_select_type (gfc_code *code, gfc_namespace *old_ns)
          new_st->expr1->value.function.actual->next = gfc_get_actual_arglist ();
          new_st->expr1->value.function.actual->next->expr = gfc_get_variable_expr (st);
          new_st->expr1->value.function.actual->next->expr->where = code->loc;
+         /* Formal arguments.  */
+         new_st->expr1->value.function.isym->formal->ts = selector_expr->symtree->n.sym->ts;
+         new_st->expr1->value.function.isym->formal->next->ts = st->n.sym->ts;
          new_st->next = body->next;
        }
        if (default_case->next)

but this does not work because new_st->expr1->value.function.isym->formal is NULL. Sad.


FX

Re: Need help with vptr copy function calls

[FX]

> Okay, I looked at the "wrong" example: Issue 9 on github, which has:
> 
> |$ cat allocate_with_source_7.f08 program test type P class(*), allocatable :: X(:) end type Type(P) Y integer, parameter :: arr(1) = (/ 5 /) allocate(Y%X, source=arr) end At least for that one, I see "class(*)". And I do think that this has to be fixed – even if it is not the issue you are currently seeing.

So this is a can of worms. I’ve found yet another example, with select type. How is it that the front-end is calling function with wrong arg types (even worse, wrong number of arguments!) and getting away with it? Would it make sense to add a check somewhere?

FX

Re: Need help with vptr copy function calls

[FX]

[-- Attachment #1: Type: text/plain, Size: 596 bytes --]

> So this is a can of worms. I’ve found yet another example, with select type. How is it that the front-end is calling function with wrong arg types (even worse, wrong number of arguments!) and getting away with it? Would it make sense to add a check somewhere?

Here is a patch which should catch all those in trans-expr.c, as a start.
Is there a reason not to treat all these cases as errors, and add the checks systematically?

Or, asked in another way: are there cases in the Fortran front-end where we can emit calls to functions whose arguments do not match the arg list?

FX



[-- Attachment #2: check_args.txt --]
[-- Type: text/plain, Size: 3663 bytes --]

diff --git a/gcc/fortran/trans-expr.c b/gcc/fortran/trans-expr.c
index 36ff9b5cbc6..f29012ad6c5 100644
--- a/gcc/fortran/trans-expr.c
+++ b/gcc/fortran/trans-expr.c
@@ -41,6 +41,24 @@ along with GCC; see the file COPYING3.  If not see
 #include "trans-stmt.h"
 #include "dependency.h"
 #include "gimplify.h"
+#include "print-tree.h"
+
+tree
+build_call_vec_check (tree return_type, tree fn, vec<tree, va_gc> *args)
+{
+  int n = 0;
+  tree t;
+  tree fntype = TREE_TYPE (TREE_TYPE (fn));
+  for (t = TYPE_ARG_TYPES (fntype); t && t != void_list_node; t = TREE_CHAIN (t))
+    n++;
+  if (n != (int) vec_safe_length (args))
+  {
+    printf("\nSize = %d vs. %d\n", n, (int) vec_safe_length (args));
+    debug_tree(fntype);
+    fatal_error (input_location, "CHECK FAILED");
+  }
+  return build_call_vec (return_type, fn, args);
+}

 /* Convert a scalar to an array descriptor. To be used for assumed-rank
    arrays.  */
@@ -1387,7 +1405,7 @@ gfc_copy_class_to_class (tree from, tree to, tree nelems, bool unlimited)
 	  free (msg);
 	}

-      tmp = build_call_vec (fcn_type, fcn, args);
+      tmp = build_call_vec_check (fcn_type, fcn, args);

       /* Build the body of the loop.  */
       gfc_init_block (&loopbody);
@@ -1408,7 +1426,7 @@ gfc_copy_class_to_class (tree from, tree to, tree nelems, bool unlimited)
 	{
 	  vec_safe_push (args, from_len);
 	  vec_safe_push (args, to_len);
-	  tmp = build_call_vec (fcn_type, fcn, args);
+	  tmp = build_call_vec_check (fcn_type, fcn, args);
 	  /* Build the body of the loop.  */
 	  gfc_init_block (&loopbody);
 	  gfc_add_expr_to_block (&loopbody, tmp);
@@ -1444,14 +1462,14 @@ gfc_copy_class_to_class (tree from, tree to, tree nelems, bool unlimited)
       gcc_assert (!is_from_desc);
       vec_safe_push (args, from_data);
       vec_safe_push (args, to_data);
-      stdcopy = build_call_vec (fcn_type, fcn, args);
+      stdcopy = build_call_vec_check (fcn_type, fcn, args);

       /* In initialization mode from_len is a constant zero.  */
       if (unlimited && !integer_zerop (from_len))
 	{
 	  vec_safe_push (args, from_len);
 	  vec_safe_push (args, to_len);
-	  extcopy = build_call_vec (fcn_type, fcn, args);
+	  extcopy = build_call_vec_check (fcn_type, fcn, args);
 	  tmp = fold_build2_loc (input_location, GT_EXPR,
 				 logical_type_node, from_len,
 				 build_zero_cst (TREE_TYPE (from_len)));
@@ -6959,7 +6977,10 @@ gfc_conv_procedure_call (gfc_se * se, gfc_symbol * sym,
     }

   fntype = TREE_TYPE (TREE_TYPE (se->expr));
-  se->expr = build_call_vec (TREE_TYPE (fntype), se->expr, arglist);
+  //printf("#### %s ####\n", sym->name);
+  //debug_tree(fntype);
+  //printf("########\n");
+  se->expr = build_call_vec_check (TREE_TYPE (fntype), se->expr, arglist);

   /* Allocatable scalar function results must be freed and nullified
      after use. This necessitates the creation of a temporary to
@@ -10706,14 +10727,14 @@ trans_class_assignment (stmtblock_t *block, gfc_expr *lhs, gfc_expr *rhs,
       else
 	vec_safe_push (args, tmp);

-      stdcopy = build_call_vec (TREE_TYPE (TREE_TYPE (fcn)), fcn, args);
+      stdcopy = build_call_vec_check (TREE_TYPE (TREE_TYPE (fcn)), fcn, args);

       if (to_len != NULL_TREE && !integer_zerop (from_len))
 	{
 	  tree extcopy;
 	  vec_safe_push (args, from_len);
 	  vec_safe_push (args, to_len);
-	  extcopy = build_call_vec (TREE_TYPE (TREE_TYPE (fcn)), fcn, args);
+	  extcopy = build_call_vec_check (TREE_TYPE (TREE_TYPE (fcn)), fcn, args);

 	  tmp = fold_build2_loc (input_location, GT_EXPR,
 				 logical_type_node, from_len,

Re: Need help with vptr copy function calls

[Thomas Koenig]

Hi FX,

> Here is a patch which should catch all those in trans-expr.c, as a start.

I think the general idea is very good.

> Is there a reason not to treat all these cases as errors, and add the checks systematically?
I think this is a check that we should only do if CHECKING_P
is enabled.  No need to penalize the user with an internal
error for production code if it would otherwise have worked.

We also have to consider vararg functions - we call a couple
of them in the front end, runtime_error and runtime_error_at
come to mind.  IIRC, the presence or absence of void_list_node
at the end of the argument list determines this.

Finally, there is the thorny issue of -fallow-argument-mismatch.
People do have code with broken argument lists, which "work"
on many architectures like x86_64.  We (unconditionally)
warn them if we detect something like that and recommend not
to do it, but issuing a hard error for this (again) would
probably not be good.  So, if this is a user-generated function,
I would recommend to warn only. If it is an internal function,
issue an ICE.

Best regards

	Thomas

Re: Need help with vptr copy function calls

[FX]

Hi Thomas,

>> Here is a patch which should catch all those in trans-expr.c, as a start.
> 
> I think the general idea is very good.

The idea for now is to use it locally in my tree, to find out places where we are having.

If you confirm the idea is good, then I will start running the testsuite with this checking, and make a list of issues. Do you have a preference for how to handle these? Separate bugzilla issue for each? Single bugzilla issue? Mailing-list?

I would need help to fix them, my understanding of the gfortran front-end is very rusty, and the handling of classes and derived types was never something I worked in depth with.

FX

Re: Need help with vptr copy function calls

[Iain Sandoe]

FX via Fortran <fortran@gcc.gnu.org> wrote:

> Hi Thomas,
>
>>> Here is a patch which should catch all those in trans-expr.c, as a start.
>>
>> I think the general idea is very good.
>
> The idea for now is to use it locally in my tree, to find out places  
> where we are having.
>
> If you confirm the idea is good, then I will start running the testsuite  
> with this checking, and make a list of issues. Do you have a preference  
> for how to handle these? Separate bugzilla issue for each? Single  
> bugzilla issue? Mailing-list?
>
> I would need help to fix them, my understanding of the gfortran front-end  
> is very rusty, and the handling of classes and derived types was never  
> something I worked in depth with.

To clarify the nature of the issue, it’s to do with how expand_call deals  
with function signatures.

* If expand_call knows the function decl (and that is correct of course)  
all is well - we can find the callee argument list and know whether it’s  
variadic.

* If we have a function signature like (*f) () this is “K&R” - like and  
expand_call has been told “trust me! I know what I’m calling accepts these  
arguments”.  This would most likely work fine for the Fortran case, since  
we don’t expect indirect calls to the few functions that are variadic.

* However, what we have is a signature like (*f)(void) … this is not  
treated as K&R but (because it’s not expected) has the effect of  
expand_call thinking it’s a  variadic call with one named argument (it  
counts the number of arguments, without checking to see in the case of N==1  
it that’s the VOID one).

Perhaps that could be considered a bug in expand_call … but AFAICT the only  
way to get this case is by the compiler providing bad input (I didn’t  
succeed in constructing a C testcase that emulated the bug).

By pure luck, most ABIs emit calls to variadic functions sufficiently  
similarly to non-variadic, that we "get away with it" - but .. not in the  
case we’re working on.

IFF the Fortran FE was intending to exploit the K&R mechanism to avoid  
publishing the types of the entries in the vtables, then the “generic  
signature” needs to be changed from (*f)(void) ==> (*f)().

IFF none of this was intended (which is how I read Tobias’ and Thomas’  
replies) - then the vtable entries need to be built with a proper type for  
the function in each slot.

unfortunately, I don’t know my way around the Fortran FE - and have a  
drastic shortage of cycles ;)

HTH
Iain

Re: Need help with vptr copy function calls

[FX]

Just double-checking what is the value of TYPE_ARG_TYPES(func_type) in each case. Is this correct?

- if TYPE_ARG_TYPES(func_type) is NULL, then it’s K&R (*f) ()
- if TYPE_ARG_TYPES(func_type) is a single void_list_node, then it’s a (*f)(void)

Right now, the calls we emit have for the following characteristic:

   arg-types <tree_list 0x1446097f8 value <void_type 0x14460ff18 void>>

So we should make then have no arg-types instead. Providing a real function prototype is better, but at least we could have a stopgap measure with K&R-style.

FX

Re: Need help with vptr copy function calls

[Iain Sandoe]

FX <fxcoudert@gmail.com> wrote:

> Just double-checking what is the value of TYPE_ARG_TYPES(func_type) in  
> each case. Is this correct?
>
> - if TYPE_ARG_TYPES(func_type) is NULL, then it’s K&R (*f) ()
> - if TYPE_ARG_TYPES(func_type) is a single void_list_node, then it’s a  
> (*f)(void)
>
> Right now, the calls we emit have for the following characteristic:
>
>   arg-types <tree_list 0x1446097f8 value <void_type 0x14460ff18 void>>
>
> So we should make then have no arg-types instead. Providing a real  
> function prototype is better, but at least we could have a stopgap  
> measure with K&R-style.

Yes, that’s how I understand it at present.

Iain

Re: Need help with vptr copy function calls

[Thomas Koenig]

Am 05.09.20 um 16:21 schrieb FX:

 > If you confirm the idea is good, then I will start running the
 > testsuitewith this checking, and make a list of issues.

I think there is one thing you will need to change, at least.

You have

+  for (t = TYPE_ARG_TYPES (fntype); t && t != void_list_node; t = 
TREE_CHAIN (t))
+    n++;
+  if (n != (int) vec_safe_length (args))
+  {

I think this could read something like

+ bool variadic;
+ int arglist_length;
+  for (t = TYPE_ARG_TYPES (fntype); t && t != void_list_node; t = 
TREE_CHAIN (t))
+    n++;
+ variadic = t == NULL;
+ length = vec_safe_length (args);
+ if (n != arglist_length && !(variadic && n > arglist_length))
+ {

(not sure if I have conditions right here).

 > Do you have a preference for how to handle these? Separate
 > bugzilla issue for each? Single bugzilla issue? Mailing-list?

Probably a single bugzilla issue at first is best.  The test
may well result in common classes of bugs (or false positives,
or both).

 > I would need help to fix them, my understanding of the gfortran
 > front-end is very rusty, and the handling of classes and derived
 > types was never something I worked in depth with.

I will try, although I also have not been very active in this
area. Maybe Paul...?

Thanks for taking this on!

Regards

	Thomas

Re: Need help with vptr copy function calls

[Iain Sandoe]

Thomas Koenig via Fortran <fortran@gcc.gnu.org> wrote:

> Am 05.09.20 um 16:21 schrieb FX:
>
> > If you confirm the idea is good, then I will start running the
> > testsuitewith this checking, and make a list of issues.
>
> I think there is one thing you will need to change, at least.
>
> You have
>
> +  for (t = TYPE_ARG_TYPES (fntype); t && t != void_list_node; t =  
> TREE_CHAIN (t))
> +    n++;
> +  if (n != (int) vec_safe_length (args))
> +  {
> I think this could read something like
>
> + bool variadic;
> + int arglist_length;
> +  for (t = TYPE_ARG_TYPES (fntype); t && t != void_list_node; t =  
> TREE_CHAIN (t))
> +    n++;

If you are looking for the terminating VOID entry,

t && TREE_VALUE (t) != void_list_node
  ?

> + variadic = t == NULL;
> + length = vec_safe_length (args);
> + if (n != arglist_length && !(variadic && n > arglist_length))
> + {
>
> (not sure if I have conditions right here).
>
> > Do you have a preference for how to handle these? Separate
> > bugzilla issue for each? Single bugzilla issue? Mailing-list?
>
> Probably a single bugzilla issue at first is best.  The test
> may well result in common classes of bugs (or false positives,
> or both).
>
> > I would need help to fix them, my understanding of the gfortran
> > front-end is very rusty, and the handling of classes and derived
> > types was never something I worked in depth with.
>
> I will try, although I also have not been very active in this
> area. Maybe Paul...?
>
> Thanks for taking this on!
>
> Regards
>
> 	Thomas

Re: Need help with vptr copy function calls

[Iain Sandoe]

Iain Sandoe <idsandoe@googlemail.com> wrote:

> Thomas Koenig via Fortran <fortran@gcc.gnu.org> wrote:
>
>> Am 05.09.20 um 16:21 schrieb FX:
>>
>> > If you confirm the idea is good, then I will start running the
>> > testsuitewith this checking, and make a list of issues.
>>
>> I think there is one thing you will need to change, at least.
>>
>> You have
>>
>> +  for (t = TYPE_ARG_TYPES (fntype); t && t != void_list_node; t =  
>> TREE_CHAIN (t))
>> +    n++;
>> +  if (n != (int) vec_safe_length (args))
>> +  {
>> I think this could read something like
>>
>> + bool variadic;
>> + int arglist_length;
>> +  for (t = TYPE_ARG_TYPES (fntype); t && t != void_list_node; t =  
>> TREE_CHAIN (t))
>> +    n++;
>
> If you are looking for the terminating VOID entry,
>
> t && TREE_VALUE (t) != void_list_node

NM, I should read what’s there first :)

Re: Need help with vptr copy function calls

[FX]

[-- Attachment #1: Type: text/plain, Size: 2687 bytes --]

Hi Tobias, hi everyone,

I am proposing a patch below. My first patch to gfortran in a long time (one commit in 2018, a couple in 2016)!


> Then, probably the procedure-pointer is not properly setup. The component
> is setup in class.c – cf. below "Add component _copy." and that's turned
> into a procedure-pointer in  trans-types.c –  I think in gfc_get_ppc_type.
> It seems as if there the explicit interface is not properly used.

Thanks, that’s exactly what I needed. In gfc_get_ppc_type(), for the copy field of the vptr, it does not find an explicit interface, so probably this is one way we could fix it. But there is also code which is supposed to handle the case of no explicit interface, using only the return value and an unknown arg list (K&R type). It shows that it was the intent of the author of that function was to use the fact that the compiler can work with an unknown arg list. This was introduced by Paul T and Janus in 2009 (https://gcc.gnu.org/git/?p=gcc.git;a=commitdiff;h=37513ce90a7fa5730028c01850bcd983995b7063).

Except it’s not doing it correctly. It’s using:

  build_function_type_list (t, NULL_TREE)

to create the function type, but that’s wrong. That’s for a function type with a known empty arg list, i.e., in C terms:

  (*f)(void)

What we want instead is a K&R prototype, i.e., a function that has unknown arg list:

  (*f)()

which is obtained by:

build_function_type (t, NULL_TREE)

I had to read tree.c a couple of times to make sure I understood it right, so I think the potential for confusion is natural (it’s not an function call we use every day).

So… we can probably fix the fields by adding explicit interfaces to them, but… it works without. So I propose the following fix:


diff --git a/gcc/fortran/trans-types.c b/gcc/fortran/trans-types.c
index d38aa2865ae..c0a77c54f4f 100644
--- a/gcc/fortran/trans-types.c
+++ b/gcc/fortran/trans-types.c
@@ -2435,7 +2435,7 @@ gfc_get_ppc_type (gfc_component* c)
   else
     t = void_type_node;
 
-  return build_pointer_type (build_function_type_list (t, NULL_TREE));
+  return build_pointer_type (build_function_type (t, NULL_TREE));
 }
 
 
Right now this is just a “call of comment”, let me know what you think about it. I will test the patch, both on x86_64-apple-darwin, then on aarch64-apple-darwin, to make sure it fixes stuff and doesn’t break anything else. But I can already confirm that it fixes my current testcases for this specific issue!

Cheers,
FX


PS: in order to perform checking, I am using the attached patch provided by Iain, which checks function calls during lowering in the middle-end.


[-- Attachment #2: checking.diff --]
[-- Type: application/octet-stream, Size: 2017 bytes --]

diff --git a/gcc/calls.c b/gcc/calls.c
index 8ac94db6817..53bfcf8c10e 100644
--- a/gcc/calls.c
+++ b/gcc/calls.c
@@ -57,6 +57,7 @@ along with GCC; see the file COPYING3.  If not see
 #include "attribs.h"
 #include "builtins.h"
 #include "gimple-fold.h"
+#include "print-tree.h"
 
 /* Like PREFERRED_STACK_BOUNDARY but in units of bytes, not bits.  */
 #define STACK_BYTES (PREFERRED_STACK_BOUNDARY / BITS_PER_UNIT)
@@ -3843,11 +3844,15 @@ expand_call (tree exp, rtx target, int ignore)
   /* Compute number of named args.
      First, do a raw count of the args for INIT_CUMULATIVE_ARGS.  */
 
+  bool is_void_arglist_p = false;
   if (type_arg_types != 0)
-    n_named_args
-      = (list_length (type_arg_types)
-	 /* Count the struct value address, if it is passed as a parm.  */
-	 + structure_value_addr_parm);
+   {
+     n_named_args = list_length (type_arg_types);
+     if (n_named_args == 1 && TREE_VALUE (type_arg_types) == void_type_node)
+       is_void_arglist_p = true;
+     /* Count the struct value address, if it is passed as a parm.  */
+     n_named_args += structure_value_addr_parm;
+   }
   else
     /* If we know nothing, treat all args as named.  */
     n_named_args = num_actuals;
@@ -3889,6 +3894,20 @@ expand_call (tree exp, rtx target, int ignore)
   else
     /* Treat all args as named.  */
     n_named_args = num_actuals;
+  /* Unless we're in K&R mode, we shouldn't be passing more args than the
+     callee expects.  */
+  if (is_void_arglist_p && num_actuals != structure_value_addr_parm)
+  {
+    printf("\n### Function type ###\n");
+    debug_tree(fntype);
+    printf("\n### Function decl ###\n");
+    debug_tree(fndecl);
+    printf("\n###\n");
+    printf("Number of args passed: %d\n", (int) num_actuals);
+    printf("\n###\n");
+  }
+  gcc_checking_assert (!is_void_arglist_p
+                      || num_actuals == structure_value_addr_parm);
 
   /* Make a vector to hold all the information about each arg.  */
   args = XCNEWVEC (struct arg_data, num_actuals);

Re: Need help with vptr copy function calls

[FX]

> Probably a single bugzilla issue at first is best.  The test
> may well result in common classes of bugs (or false positives,
> or both).

Testsuite with arguments checking leads to:

# of expected passes		52649
# of unexpected failures	2244
# of expected failures		193
# of unresolved testcases	1043

So a total of 3287 testcases (out of 52649) regressed under argument checking. A lot of those are about vptr fields, that seem to lack proper args types: copy and final, maybe others. Fixing that with the patch I just sent (https://gcc.gnu.org/pipermail/gcc-patches/2020-September/553319.html) brings us to:

# of expected passes		54520
# of unexpected failures	472
# of expected failures		193
# of unresolved testcases	148
# of unsupported tests		82

so that’s 620 failures now. Many of them are due to _gfortran_is_extension_of (https://gcc.gnu.org/pipermail/fortran/2020-September/054999.html), but some others are coarray-related.

For example, in gfortran.dg/coarray_fail_st.f90, _gfortran_caf_fail_image is called with 1 argument but expects 0. I know absolutely nothing about the coarrays implementation, who should I contact who could help about this?

FX

Re: Need help with vptr copy function calls

[FX]

> For example, in gfortran.dg/coarray_fail_st.f90, _gfortran_caf_fail_image is called with 1 argument but expects 0. I know absolutely nothing about the coarrays implementation, who should I contact who could help about this?

OK this one is very straightforward. gfc_trans_fail_image() is generating a call with one argument (a NULL pointer):

    return build_call_expr_loc (input_location,
                                gfor_fndecl_caf_fail_image, 1,
                                build_int_cst (pchar_type_node, 0));

while the prototype for the function in libgfortran is clear that it takes no argument:

  void _gfortran_caf_fail_image (void) __attribute__ ((noreturn));

Filed as https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96947
Can someone tell me which way this is supposed to be, and I’ll submit the trivial patch to fix it.

FX

Re: Need help with vptr copy function calls

[Thomas Koenig]

Hi FX,

> For example, in gfortran.dg/coarray_fail_st.f90, _gfortran_caf_fail_image
> is called with 1 argument but expects 0. I know absolutely nothing
> about the coarrays implementation, who should I contact who could help
> about this?

It's built with

       gfor_fndecl_caf_fail_image = gfc_build_library_function_decl (
         get_identifier (PREFIX("caf_fail_image")), void_type_node, 0);
       /* CAF's FAIL doesn't return.  */
       TREE_THIS_VOLATILE (gfor_fndecl_caf_fail_image) = 1;

and the library version does not take an argument:

libcaf.h:void _gfortran_caf_fail_image (void) __attribute__ ((noreturn));
single.c:void _gfortran_caf_fail_image (void)

So, it seems that the error is not the declaration, but in
the call.  I would simply not pass the argument, but maybe
Andre (who is currently investigating Coarray errors) knows
more.

Best regards

	Thomas