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
* 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" } */
+ }
