Fwd: [cxx-mem-model] Don't over process __sync_mem parameters.

Fwd: [cxx-mem-model] Don't over process __sync_mem parameters.

[Andrew MacLeod]

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

  This patch implements a reduced type massaging for the new 
'__sync_mem' built-in functions, as well as verifying the number of 
parameters are correct.

The gory details from the wiki TODO :

    The parameters are massaged in
    c-family/c-common.c::resolve_overloaded_builtin,
    sync_resolve_return, and sync_resolve_params.

    Previous to this, all the __sync were of the format T __Sync (T *,
    T, T, ...) so these 3 routines figured out what type T was and cast
    the return and all parameters to T and then to the type of the
    builtins's argument, usually BT_I{1,2,4,8,16}, which is an unsigned
    value. ie if it the builtin type was 1 byte, BT_I1, we'd see
    something like (BT_I1)(char)actual_param where BT_I1 maps to
    type_for_size(1), which I think is unsigned char.

    The new __sync_mem routines are similar, but the memory model
    parameter is a BT_INT. This means that currently the memory model
    parameter on a call for 16 bytes values would be cast to an
    __int128_t, and then back to an int. Which is quite ugly and silly.

    The right thing would be to change these routines to look at all the
    arguments and only do these casts when the underlying type of the
    builtin argument is the same size as the real base (T) (picked up
    from the pointer in parameter 1). Since the memory_model argument is
    a BT_INT, we'll only get the cast on the memory model parameter when
    it's size matches T (either long or int).. and then its a harmless cast.

    Extra parameters can be thrown on the end as well and no errors are
    reported, this dates back to the variadic versions the IA64 stuff
    required. the new routines should complain if there are extra
    parameters.

    The reason for all this casting is to prevent a bunch of compiler
    warnings when passing in pointers for compare and swap and to avoid
    signed/unsigned conversions issues which may cause miscompilations.

    And now there is a bug uncovered. If type T is a bool for instance,
    the memory_model parameter will be cast to type T, which means it
    will end up being either a 0 or a 1, which is very very bad.  This
    caused a failure in one of the follow on patches when
    __sync_mem_exchange(&bool, bool, memmodel) is called.

A new testcase for checking the number of parameters on a sample call is 
also added.  Bootstraps on x86_64-unknown-linux-gnu and causes no new 
regression.

OK for branch?
Andrew




[-- Attachment #2: param.diff --]
[-- Type: text/plain, Size: 7962 bytes --]


	* c-family/c-common.c (sync_resolve_params, sync_resolve_return): Only
	tweak parameters that are the same type size.
	(resolve_overloaded_builtin): Use new param for __sync_mem builtins.
	
	* testsuite/gcc.dg/sync-mem-param.c: New testcase to check correct
	number of parameters on a sample __sync_mem builtin.


Index: c-family/c-common.c
===================================================================
*** c-family/c-common.c	(revision 178734)
--- c-family/c-common.c	(working copy)
*************** sync_resolve_size (tree function, VEC(tr
*** 9015,9021 ****
     was encountered; true on success.  */
  
  static bool
! sync_resolve_params (tree orig_function, tree function, VEC(tree, gc) *params)
  {
    function_args_iterator iter;
    tree ptype;
--- 9015,9022 ----
     was encountered; true on success.  */
  
  static bool
! sync_resolve_params (tree orig_function, tree function, VEC(tree, gc) *params,
! 		     bool orig_format)
  {
    function_args_iterator iter;
    tree ptype;
*************** sync_resolve_params (tree orig_function,
*** 9047,9063 ****
  	  return false;
  	}
  
!       /* ??? Ideally for the first conversion we'd use convert_for_assignment
! 	 so that we get warnings for anything that doesn't match the pointer
! 	 type.  This isn't portable across the C and C++ front ends atm.  */
!       val = VEC_index (tree, params, parmnum);
!       val = convert (ptype, val);
!       val = convert (arg_type, val);
!       VEC_replace (tree, params, parmnum, val);
  
        function_args_iter_next (&iter);
      }
  
    /* The definition of these primitives is variadic, with the remaining
       being "an optional list of variables protected by the memory barrier".
       No clue what that's supposed to mean, precisely, but we consider all
--- 9048,9077 ----
  	  return false;
  	}
  
!       /* Only convert parameters if the size is appropriate with new format
! 	 sync routines.  */
!       if (orig_format ||
! 	    tree_int_cst_equal (TYPE_SIZE (ptype), TYPE_SIZE (arg_type)))
! 	{
! 	  /* Ideally for the first conversion we'd use convert_for_assignment
! 	     so that we get warnings for anything that doesn't match the pointer
! 	     type.  This isn't portable across the C and C++ front ends atm.  */
! 	  val = VEC_index (tree, params, parmnum);
! 	  val = convert (ptype, val);
! 	  val = convert (arg_type, val);
! 	  VEC_replace (tree, params, parmnum, val);
! 	}
  
        function_args_iter_next (&iter);
      }
  
+   /* __sync_mem routines are not variadic.  */
+   if (!orig_format && VEC_length (tree, params) != parmnum + 1)
+     {
+       error ("too many arguments to function %qE", orig_function);
+       return false;
+     }
+ 
    /* The definition of these primitives is variadic, with the remaining
       being "an optional list of variables protected by the memory barrier".
       No clue what that's supposed to mean, precisely, but we consider all
*************** sync_resolve_params (tree orig_function,
*** 9072,9082 ****
     PARAMS.  */
  
  static tree
! sync_resolve_return (tree first_param, tree result)
  {
    tree ptype = TREE_TYPE (TREE_TYPE (first_param));
    ptype = TYPE_MAIN_VARIANT (ptype);
!   return convert (ptype, result);
  }
  
  /* Some builtin functions are placeholders for other expressions.  This
--- 9086,9102 ----
     PARAMS.  */
  
  static tree
! sync_resolve_return (tree first_param, tree result, bool orig_format)
  {
    tree ptype = TREE_TYPE (TREE_TYPE (first_param));
+   tree rtype = TREE_TYPE (result);
    ptype = TYPE_MAIN_VARIANT (ptype);
! 
!   /* New format doesn't require casting unless the types are the same size.  */
!   if (orig_format || tree_int_cst_equal (TYPE_SIZE (ptype), TYPE_SIZE (rtype)))
!     return convert (ptype, result);
!   else
!     return result;
  }
  
  /* Some builtin functions are placeholders for other expressions.  This
*************** tree
*** 9094,9099 ****
--- 9114,9121 ----
  resolve_overloaded_builtin (location_t loc, tree function, VEC(tree,gc) *params)
  {
    enum built_in_function orig_code = DECL_FUNCTION_CODE (function);
+   bool orig_format = true;
+ 
    switch (DECL_BUILT_IN_CLASS (function))
      {
      case BUILT_IN_NORMAL:
*************** resolve_overloaded_builtin (location_t l
*** 9110,9115 ****
--- 9132,9155 ----
    /* Handle BUILT_IN_NORMAL here.  */
    switch (orig_code)
      {
+     case BUILT_IN_SYNC_MEM_EXCHANGE_N:
+     case BUILT_IN_SYNC_MEM_COMPARE_EXCHANGE_N:
+     case BUILT_IN_SYNC_MEM_LOAD_N:
+     case BUILT_IN_SYNC_MEM_STORE_N:
+     case BUILT_IN_SYNC_MEM_ADD_FETCH_N:
+     case BUILT_IN_SYNC_MEM_SUB_FETCH_N:
+     case BUILT_IN_SYNC_MEM_AND_FETCH_N:
+     case BUILT_IN_SYNC_MEM_XOR_FETCH_N:
+     case BUILT_IN_SYNC_MEM_OR_FETCH_N:
+     case BUILT_IN_SYNC_MEM_FETCH_ADD_N:
+     case BUILT_IN_SYNC_MEM_FETCH_SUB_N:
+     case BUILT_IN_SYNC_MEM_FETCH_AND_N:
+     case BUILT_IN_SYNC_MEM_FETCH_XOR_N:
+     case BUILT_IN_SYNC_MEM_FETCH_OR_N:
+       {
+         orig_format = false;
+ 	/* Fallthru for parameter processing.  */
+       }
      case BUILT_IN_SYNC_FETCH_AND_ADD_N:
      case BUILT_IN_SYNC_FETCH_AND_SUB_N:
      case BUILT_IN_SYNC_FETCH_AND_OR_N:
*************** resolve_overloaded_builtin (location_t l
*** 9126,9145 ****
      case BUILT_IN_SYNC_VAL_COMPARE_AND_SWAP_N:
      case BUILT_IN_SYNC_LOCK_TEST_AND_SET_N:
      case BUILT_IN_SYNC_LOCK_RELEASE_N:
-     case BUILT_IN_SYNC_MEM_EXCHANGE_N:
-     case BUILT_IN_SYNC_MEM_COMPARE_EXCHANGE_N:
-     case BUILT_IN_SYNC_MEM_LOAD_N:
-     case BUILT_IN_SYNC_MEM_STORE_N:
-     case BUILT_IN_SYNC_MEM_ADD_FETCH_N:
-     case BUILT_IN_SYNC_MEM_SUB_FETCH_N:
-     case BUILT_IN_SYNC_MEM_AND_FETCH_N:
-     case BUILT_IN_SYNC_MEM_XOR_FETCH_N:
-     case BUILT_IN_SYNC_MEM_OR_FETCH_N:
-     case BUILT_IN_SYNC_MEM_FETCH_ADD_N:
-     case BUILT_IN_SYNC_MEM_FETCH_SUB_N:
-     case BUILT_IN_SYNC_MEM_FETCH_AND_N:
-     case BUILT_IN_SYNC_MEM_FETCH_XOR_N:
-     case BUILT_IN_SYNC_MEM_FETCH_OR_N:
        {
  	int n = sync_resolve_size (function, params);
  	tree new_function, first_param, result;
--- 9166,9171 ----
*************** resolve_overloaded_builtin (location_t l
*** 9148,9154 ****
  	  return error_mark_node;
  
  	new_function = built_in_decls[orig_code + exact_log2 (n) + 1];
! 	if (!sync_resolve_params (function, new_function, params))
  	  return error_mark_node;
  
  	first_param = VEC_index (tree, params, 0);
--- 9174,9180 ----
  	  return error_mark_node;
  
  	new_function = built_in_decls[orig_code + exact_log2 (n) + 1];
! 	if (!sync_resolve_params (function, new_function, params, orig_format))
  	  return error_mark_node;
  
  	first_param = VEC_index (tree, params, 0);
*************** resolve_overloaded_builtin (location_t l
*** 9156,9162 ****
  	if (orig_code != BUILT_IN_SYNC_BOOL_COMPARE_AND_SWAP_N
  	    && orig_code != BUILT_IN_SYNC_LOCK_RELEASE_N
  	    && orig_code != BUILT_IN_SYNC_MEM_STORE_N)
! 	  result = sync_resolve_return (first_param, result);
  
  	return result;
        }
--- 9182,9188 ----
  	if (orig_code != BUILT_IN_SYNC_BOOL_COMPARE_AND_SWAP_N
  	    && orig_code != BUILT_IN_SYNC_LOCK_RELEASE_N
  	    && orig_code != BUILT_IN_SYNC_MEM_STORE_N)
! 	  result = sync_resolve_return (first_param, result, orig_format);
  
  	return result;
        }
Index: testsuite/gcc.dg/sync-mem-param.c
===================================================================
*** testsuite/gcc.dg/sync-mem-param.c	(revision 0)
--- testsuite/gcc.dg/sync-mem-param.c	(revision 0)
***************
*** 0 ****
--- 1,13 ----
+ /* Test __sync_mem routines for invalid memory model errors. This only needs
+    to be tested on a single size.  */
+ /* { dg-do compile } */
+ /* { dg-require-effective-target sync_int_long } */
+ 
+ int i;
+ 
+ main ()
+ {
+ 
+   __sync_mem_exchange (&i, 1); /* { dg-error "too few arguments" } */
+   __sync_mem_exchange (&i, 1, __SYNC_MEM_SEQ_CST, __SYNC_MEM_SEQ_CST); /* { dg-error "too many arguments" } */
+ }

Re: Fwd: [cxx-mem-model] Don't over process __sync_mem parameters.

[Richard Henderson]

> 	* c-family/c-common.c (sync_resolve_params, sync_resolve_return): Only
> 	tweak parameters that are the same type size.
> 	(resolve_overloaded_builtin): Use new param for __sync_mem builtins.
> 	
> 	* testsuite/gcc.dg/sync-mem-param.c: New testcase to check correct
> 	number of parameters on a sample __sync_mem builtin.

Ok, except,

> +   /* __sync_mem routines are not variadic.  */
> +   if (!orig_format && VEC_length (tree, params) != parmnum + 1)
> +     {
> +       error ("too many arguments to function %qE", orig_function);
> +       return false;
> +     }

I shouldn't think you need this?  Surely because the function type
is non-variadic the c front end would diagnose this.


r~

Re: Fwd: [cxx-mem-model] Don't over process __sync_mem parameters.

[Andrew MacLeod]

On 09/15/2011 04:35 PM, Richard Henderson wrote:
>> 	* c-family/c-common.c (sync_resolve_params, sync_resolve_return): Only
>> 	tweak parameters that are the same type size.
>> 	(resolve_overloaded_builtin): Use new param for __sync_mem builtins.
>> 	
>> 	* testsuite/gcc.dg/sync-mem-param.c: New testcase to check correct
>> 	number of parameters on a sample __sync_mem builtin.
>
> Ok, except,
>
>> +   /* __sync_mem routines are not variadic.  */
>> +   if (!orig_format&&  VEC_length (tree, params) != parmnum + 1)
>> +     {
>> +       error ("too many arguments to function %qE", orig_function);
>> +       return false;
>> +     }
>
> I shouldn't think you need this?  Surely because the function type
> is non-variadic the c front end would diagnose this.
>

It didn't seem to until I added it...  The expanders handle issuing the 
message if there aren't enough parameters already... It doesnt really 
complain about types either.. maybe it just passes builtins on to be 
processed by expanders?

Andrew