[prev in list] [next in list] [prev in thread] [next in thread] 

List:       gcc-patches
Subject:    Re: *revised* PATCH: named address space support (1/2:  target-independent  parts)
From:       "Joseph S. Myers" <joseph () codesourcery ! com>
Date:       2008-08-31 17:07:29
Message-ID: Pine.LNX.4.64.0808311641540.12423 () digraph ! polyomino ! org ! uk
[Download RAW message or body]

On Fri, 29 Aug 2008, Ben Elliston wrote:

> +  if (!flag_iso)
> +    {
> +      addr_space_t as1 = declspecs->address_space;
> +      addr_space_t as2 = TYPE_ADDR_SPACE (element_type);
> +
> +      if (as1 > 0 && as2 > 0 && as1 != as2)
> +	error ("incompatible address space qualifiers %qs and %qs",
> +	       targetm.addr_space_name (as1),
> +	       targetm.addr_space_name (as2));
> +    }

I see no reason for this to be !flag_iso.  If address spaces are in use, 
the error should be given for incompatible address spaces and this is 
outside the scope of ISO C.

> /* Warn about storage classes that are invalid for certain
> kinds of declarations (parameters, typenames, etc.).  */
> 
> +  if (((declarator->kind == cdk_pointer
> + 	&& (DECODE_QUAL_ADDR_SPACE (declarator->u.pointer_quals)) != 0)
> +       || addr_space_p)
> +      && targetm.addr_space_name == default_addr_space_name)
> +     {
> +       /* A mere warning is sure to result in improper semantics
> +	  at runtime.  Don't bother to allow this to compile.  */
> +       error ("extended address space not supported for this target");
> +       return 0;
> +     }

This is not the right check.  You should check for address spaces in the 
declaration specifiers and in the qualifiers in any nested pointer 
declarator, rather than arbitrarily only for a toplevel pointer 
declarator as here.  That probably means a check in the loop going down 
through the nested declarators.

> +  if (declarator->kind == cdk_pointer
> +      ? (DECODE_QUAL_ADDR_SPACE (declarator->u.pointer_quals)) != 0
> +      : addr_space_p)
> +    {
> +      const char *addrspace_name;
> +
> +      /* Either declspecs or the declarator identifies the address space.  */
> +      if (declspecs->address_space)
> +	addrspace_name = targetm.addr_space_name (declspecs->address_space);
> +      else
> +	addrspace_name = targetm.addr_space_name (DECODE_QUAL_ADDR_SPACE \
> (declarator->u.pointer_quals)); +
> +      if (decl_context == NORMAL)
> +	{
> +	  if (declarator->kind == cdk_function)
> +	    error ("%qs specified for function %qs", addrspace_name, name);

The right check is not whether the top declarator (corresponding to the 
*innermost* part of the type described by the full declarator) is a 
function declarator.  It is whether, at any level of nested declarators, 
an address space is being applied to a function type.  In each place where 
there is a pedwarn "ISO C forbids qualified function types", or in one 
place "ISO C forbids const or volatile function types", there should be a 
check for address space qualifiers (if not more generally disallowed in 
that case) and this error.  (There should be a testcase for each such 
diagnostic.)

> +	  else
> +	    {
> +	      switch (storage_class)
> +		{
> +		case csc_auto:
> +		  error ("%qs combined with %<auto%> qualifier for %qs", addrspace_name, name);
> +		  break;
> +		case csc_register:
> +		  error ("%qs combined with %<register%> qualifier for %qs", addrspace_name, \
> name); +		  break;
> +		case csc_none:
> +		  if (current_function_scope)
> + 		    {
> + 		      error ("%<__ea%> specified for auto variable %qs", name);
> + 		      break;
> + 		    }
> +		  break;

For these, again the question is whether the qualifier would finally end 
up applied to the variable.  If you have "register int **__ea x;" you 
should get the error; you should not get it for "register int *__ea *x;".  
You're checking the toplevel declarator for being a pointer, but it's the 
*innermost* declarator that ends up specifying the toplevel type, while 
the toplevel declarator specifies the innermost type.  There should be 
tests for both those examples.  I think this check needs to go later, 
somewhere at or after

  /* Now TYPE has the actual type, apart from any qualifiers in
     TYPE_QUALS.  */

so TYPE + TYPE_QUALS gives the final type that will be used.

> +      else if (decl_context == PARM
> +	       && declarator->kind != cdk_array)
> +	error ("%qs specified for parameter %qs", addrspace_name, name);
> +      else if (decl_context == FIELD)
> +	error ("%qs specified for structure field %qs", addrspace_name, name);

Likewise, you need to know if the qualifiers will end up on the parameter 
or field or not (and to have associated testcases).

> diff -N -p --exclude=spu --exclude=config/spu --exclude=ea --exclude=LAST_UPDATED \
> --exclude=REVISION --exclude=ChangeLog --exclude=ChangeLog.named -r -u \
>                 --exclude=.svn gcc-clean/gcc/c-typeck.c gcc-nas/gcc/c-typeck.c
> --- gcc-clean/gcc/c-typeck.c	2008-08-29 11:32:47.000000000 +1000
> +++ gcc-nas/gcc/c-typeck.c	2008-08-27 12:04:08.000000000 +1000

I do not see any sign of my previous comments on the changes to this file 
having been addressed, so I repeat them here:

  For both places comparison operators are handled, the relevant check is 
  not whether an address space is other than generic, it is whether the 
  address spaces overlap.  If not, an error must be given.  If they do 
  overlap, both must be converted to whichever is the larger address space 
  (not whichever is the smaller).

  Conditional expressions need similar handling.

  I don't see any changes to convert_for_assignment to check the constraints 
  on pointer assignment (and argument passing, function return, 
  initialization) and address spaces.

-- 
Joseph S. Myers
joseph@codesourcery.com


[prev in list] [next in list] [prev in thread] [next in thread]