Testers needed for ISO C Binding patch

[Tobias Schlüter]

Between all the whitespace changes and no ChangeLog, I don't yet have a 
high-level overview of what the patch does, but here are a few local 
remarks.

Steve Kargl wrote:
> @@ -334,8 +334,8 @@ gfc_compare_derived_types (gfc_symbol *d
>    /* Special case for comparing derived types across namespaces.  If the
>       true names and module names are the same and the module name is
>       nonnull, then they are equal.  */
> -  if (strcmp (derived1->name, derived2->name) == 0
> -      && derived1 != NULL && derived2 != NULL
> +  if (derived1 != NULL && derived2 != NULL
> +      && strcmp (derived1->name, derived2->name) == 0
>        && derived1->module != NULL && derived2->module != NULL
>        && strcmp (derived1->module, derived2->module) == 0)
>      return 1;

I don't really understand under which circumstance both pointers could 
be NULL, but these strings are allocated via gfc_get_string, so you 
could just compare the pointers instead of using strcmp.

> +  if (expr != NULL && expr->ts.type == BT_DERIVED
> +      && expr->ts.is_iso_c && expr->ts.derived)
> +    if (expr->symtree->n.sym->intmod_sym_id == ISOCBINDING_NULL_PTR
> +        || expr->symtree->n.sym->intmod_sym_id == ISOCBINDING_NULL_FUNPTR)
> +      expr = gfc_int_expr (0);
> +  

A single if would be sufficient.

> +  /* We need to convert the expressions for the iso_c_binding derived types.
> +     C_NULL_PTR and C_NULL_FUNPTR will be made EXPR_NULL, which evaluates to
> +     null_pointer_node.  C_PTR and C_FUNPTR are converted to match the
> +     typespec for the C_PTR and C_FUNPTR symbols, which has already been
> +     updated to be an integer with a kind equal to the size of a (void *).  */
> +  if (expr->ts.type == BT_DERIVED && expr->ts.derived
> +      && expr->ts.derived->attr.is_iso_c)
> +    {
> +      if (expr->symtree->n.sym->intmod_sym_id == ISOCBINDING_NULL_PTR
> +          || expr->symtree->n.sym->intmod_sym_id == ISOCBINDING_NULL_FUNPTR)
> +        {
> +          /* Try simply setting expr_type to EXPR_NULL, which should result
> +             in null_pointer_node being used below.  */
> +          expr->expr_type = EXPR_NULL;

So, does it work or not? :-)

> +  if (sym->attr.is_bind_c == 1)
> +    {
> +      /* BIND(C) variables should not be implicitly declared.  */
> +      gfc_warning_now ("Implicitly declared BIND(C) variable '%s' at %L may "
> +                       "not be C interoperable", sym->name, &sym->declared_at);
> +      sym->ts.f90_type = sym->ts.type;
> +    }

I think "BIND(C)" is redundant in that message.

> +
> +  if (sym->attr.dummy != 0)
> +    {
> +      if (sym->ns->proc_name != NULL
> +	  && (sym->ns->proc_name->attr.subroutine != 0
> +	      || sym->ns->proc_name->attr.function != 0)
> +	  && sym->ns->proc_name->attr.is_bind_c != 0)
> +        {
> +          /* Dummy args to a BIND(C) routine may not be interoperable if
> +             they are implicitly typed.  */
> +          gfc_warning_now ("Implicity declared variable '%s' at %L may not "
> +                           "be C interoperable but it is a dummy argument to "
> +                           "the BIND(C) procedure '%s' at %L", sym->name,
> +                           &(sym->declared_at), sym->ns->proc_name->name,
> +                           &(sym->ns->proc_name->declared_at));
> +          sym->ts.f90_type = sym->ts.type;
> +        }
> +    }

Again, a single if is sufficient?  I'm also not a fan of comparing 
against zero.  I.e. I'd prefer
   if (sym->attr.dummy && sym->ns->proc_name && ...)
In the rest of the patch there are several places where 
sym->attr.something == 1 or != 1 is checked.  I think this is actually 
confusing.  They can only take the values 0 or 1, but the check looks 
like a vlaue of 2 might actually mean something different.

> @@ -596,6 +636,8 @@ check_conflict (symbol_attribute * attr,
>        conf2 (value);
>        conf2 (volatile_);
>        conf2 (threadprivate);
> +      /* TODO: hmm, double check this.  */
> +      conf2 (value);
>        break;
>  
>      default:

This condition is already checked in the first line of this hunk.

> @@ -637,8 +679,9 @@ conflict_std:
>  /* Mark a symbol as referenced.  */
>  
>  void
> -gfc_set_sym_referenced (gfc_symbol * sym)
> +gfc_set_sym_referenced (gfc_symbol *sym)
>  {
> +
>    if (sym->attr.referenced)
>      return;
>  

We don't require empty lines in the beginning of functions with no 
variable declarations.  In the interest of keeping the merge simple and 
since Brooks already merged these changes, I believe we should remove 
them at a later time.

> @@ -1435,6 +1514,19 @@ gfc_copy_attr (symbol_attribute * dest, 
>    if (src->intrinsic)
>      dest->intrinsic = 1;
>  
> +  is_proc_lang_bind_spec = (src->flavor == FL_PROCEDURE ? 1 : 0);

No need for the ternary here.

> @@ -1937,13 +2032,13 @@ compare_symtree (void * _st1, void * _st
>  /* Allocate a new symtree node and associate it with the new symbol.  */
>  
>  gfc_symtree *
> -gfc_new_symtree (gfc_symtree ** root, const char *name)
> +gfc_new_symtree (gfc_symtree **root, const char *name)
>  {
>    gfc_symtree *st;
>  
>    st = gfc_getmem (sizeof (gfc_symtree));
>    st->name = gfc_get_string (name);
> -
> +  

This looks like whitespace damage.

> @@ -2074,6 +2169,16 @@ gfc_new_symbol (const char *name, gfc_na
>      gfc_internal_error ("new_symbol(): Symbol name too long");
>  
>    p->name = gfc_get_string (name);
> +
> +  /* Make sure flags for symbol being C bound are clear initially.  */
> +  p->attr.is_bind_c = 0;
> +  p->attr.is_iso_c = 0;

These are handled by gfc_clear_attr ().

> +try
> +verify_bind_c_derived_type (gfc_symbol *derived_sym)
> +{
> +  gfc_component *curr_comp = NULL;
> +  try is_c_interop = FAILURE;
> +  try retval = SUCCESS;
> +   
> +  if (derived_sym == NULL)
> +    gfc_internal_error ("verify_bind_c_derived_type(): Given symbol is "
> +                        "unexpectedly NULL");
> +
> +  /* The derived type must have the BIND attribute to be interoperable
> +     J3/04-007, Section 15.2.3.  */
> +  if (derived_sym->attr.is_bind_c != 1)
> +    {
> +      derived_sym->ts.is_c_interop = 0;
> +      gfc_error_now ("Derived type '%s' declared at %L must have the BIND "
> +                     "attribute to be C interoperable", derived_sym->name,
> +                     &(derived_sym->declared_at));

In other places it's called the BIND(C) attribute.

> +  /* Initialize the derived type as being C interoperable.
> +     If we find an error in the components, this will be set false.  */
> +  derived_sym->ts.is_c_interop = 1;
> +  
> +  /* Loop through the list of components to verify that the kind of
> +     each is a C interoperable type.  */
> +  do
> +    {
> +      /* BIND(C) derived types can't have derived types in them unless
> +	 they're c_ptr or c_funptr. J3/04-007, Section 15.2.3, C1502.  */
> +      if (curr_comp->ts.type == BT_DERIVED
> +	  && curr_comp->ts.derived->ts.is_iso_c != 1)
> +        {
> +          /* This should be allowed; the draft says a derived-type can not
> +             have type parameters if it is has the BIND attribute.  Type
> +             parameters seem to be for making parameterized derived types.
> +             There's no need to verify the type if it is c_ptr/c_funptr.  */
> +          retval = verify_bind_c_derived_type (curr_comp->ts.derived);

This will break for POINTER components, unless I'm mistaken.
type t
   type (t), pointer :: next
end type t
-> infinite recursion
I also believe this will break for ALLOCATABLE array components -- they 
are not C interoperable, but I don't see it checked here.

Hm, reading forther I realize that zou have the appropriate checks, just 
not in place where they're are performed for derived type components: 
these two should be moved to hte beginning of the loop.

> +	  /* The components can not be pointers (fortran sense).  
> +	     J3/04-007, Section 15.2.3, C1505.	*/
> +	  if (curr_comp->pointer != 0)
> +	    {
> +	      gfc_error ("Component '%s' at %L cannot have the "
> +                         "POINTER attribute because it is a member "
> +                         "of the BIND(C) derived type '%s' at %L",
> +                         curr_comp->name, &(curr_comp->loc),
> +                         derived_sym->name, &(derived_sym->declared_at));
> +	      retval = FAILURE;
> +	    }
> +
> +	  /* The components can not be allocatable.
> +	     J3/04-007, Section 15.2.3, C1505.	*/
> +	  if (curr_comp->allocatable != 0)
> +	    {
> +	      gfc_error ("Component '%s' at %L cannot have the "
> +			 "ALLOCATABLE attribute because it is a member "
> +			 "of the BIND(C) derived type '%s' at %L",
> +			 curr_comp->name, &(curr_comp->loc),
> +			 derived_sym->name, &(derived_sym->declared_at));
> +	      retval = FAILURE;
> +	    }
> +	}
> +      
> +      curr_comp = curr_comp->next;
> +    } while (curr_comp != NULL); 

I'd prefer
   }
while (curr_comp = curr_comp->next);

Also note the whitespace change.

> +/* Generate symbols for the named constants c_null_ptr and c_null_funptr.  */
> +
> +static try
> +gen_special_c_interop_ptr (int ptr_id, const char *ptr_name,
...
> +      generate_isocbinding_symbol (module_name, ptr_id == ISOCBINDING_NULL_PTR 
> +                                   ? ISOCBINDING_PTR : ISOCBINDING_FUNPTR,
> +                                   (char *) (ptr_id == ISOCBINDING_NULL_PTR 
> +				   ? "_gfortran_iso_c_binding_c_ptr"
> +				   : "_gfortran_iso_c_binding_c_funptr"));

The cast to char * is wrong.  Instead, use const char * for 
generate_isocbinding_symbol's last argument and its name variable.

> +  /* Set the binding_label.  */
> +  sprintf (tmp_sym->binding_label, "%s_%s", module_name, tmp_sym->name);

This can overflow the target string by one character :-) 
gfc_symbol->binding_label should be a const char *, and you should be using
   tmp_sym->binding_label = gfc_get_string ("%s_%s", module_name,
                                            tmp_sym->name);
instead.  Be sure to remove the declaration of GFC_MAX_BINDING_LABEL_LEN 
at the same time.


I'll have to stop here for the moment.  I'll try to give the patch a 
thorough and complete look during the weekend.  Looking at my xterm I 
have bad news to report: my build on darwin-i386 failed with the 
following error:
libtool: compile: unable to infer tagged configuration
libtool: compile: specify a tag with `--tag'
make[6]: *** [iso_c_generated_procs.lo] Error 1
make[6]: *** Waiting for unfinished jobs....
libtool: compile: unable to infer tagged configuration
libtool: compile: specify a tag with `--tag'
make[6]: *** [iso_c_binding.lo] Error 1
make[5]: *** [all] Error 2
make[4]: *** [multi-do] Error 1
make[3]: *** [all-multi] Error 2
make[2]: *** [all] Error 2
make[1]: *** [all-target-libgfortran] Error 2
make: *** [all] Error 2


I'll try again with a clean build dir, and will report back.

Cheers,
- Tobi