| Message ID | AANLkTikPU5bB407Lvrw7TAXCRWRm46RMi8YY0wMU1do-@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
Janne Blomqvist wrote: > I've been looking a bit at improving our detection of overflow when > allocating arrays (PR 28105), and I noticed that the definition of > size_t is a bit messed up. sizetype is properly set to an unsigned > type of the right size, but the expression is calculated in a bit > needlessly complicated way. However, size_type_node is an alias for > gfc_array_index_type which is a signed type. Well spotted! > However, this patch does introduce a small regression (which the > testsuite doesn't test for), namely since size_type_node is now > changed to an unsigned type, the overflow test which checks for size< > 0 when allocating arrays is optimized away. Frankly, while this test catches some of the overflow cases, I never quite liked it. The chance that this issue occurs is relatively low, it makes the code needlessly complicated (and slow) and the chances that it works are also relatively low. For non-manual allocation, we have already removed the check, cf. PR 42958. There, the non-negative check is only done with -fcheck=mem. Thus, even though it is a regression, I am in favour of having no overflow check by default and only one - and possibly a better one - with -fcheck=mem. I think a better check would be "if (number_of_elements > (SIZE_MAX / sizeof(type)))", which assumes that the <number of element> does not already overflow. Tobias PS: If no one is faster, I will review the patch later today.
Janne Blomqvist wrote: > Regtested on x86_64-unknown-linux-gnu, Ok for trunk? OK. Thanks for the patch. Tobias > 2010-11-20 Janne Blomqvist<jb@gcc.gnu.org> > > * f95-lang.c (gfc_init_decl_processing): Set size_type_node as > unsigned int of pointer size and set sizetype based on that. > * trans-types.c (gfc_init_types): Don't set size_type_node to an > unsigned type. > >
On Sat, Nov 20, 2010 at 15:18, Tobias Burnus <burnus@net-b.de> wrote: > Janne Blomqvist wrote: >> >> Regtested on x86_64-unknown-linux-gnu, Ok for trunk? > > OK. > > Thanks for the patch. Thanks! Sending ChangeLog Sending f95-lang.c Sending trans-types.c Transmitting file data ... Committed revision 166976.
diff --git a/gcc/fortran/f95-lang.c b/gcc/fortran/f95-lang.c index 3ed500b..a3ac860 100644 --- a/gcc/fortran/f95-lang.c +++ b/gcc/fortran/f95-lang.c @@ -582,15 +582,10 @@ gfc_init_decl_processing (void) only use it for actual characters, not for INTEGER(1). Also, we want double_type_node to actually have double precision. */ build_common_tree_nodes (false); - /* x86_64 mingw32 has a sizetype of "unsigned long long", most other hosts - have a sizetype of "unsigned long". Therefore choose the correct size - in mostly target independent way. */ - if (TYPE_MODE (long_unsigned_type_node) == ptr_mode) - set_sizetype (long_unsigned_type_node); - else if (TYPE_MODE (long_long_unsigned_type_node) == ptr_mode) - set_sizetype (long_long_unsigned_type_node); - else - set_sizetype (long_unsigned_type_node); + + size_type_node = gfc_build_uint_type (POINTER_SIZE); + set_sizetype (size_type_node); + build_common_tree_nodes_2 (0); void_list_node = build_tree_list (NULL_TREE, void_type_node); diff --git a/gcc/fortran/trans-types.c b/gcc/fortran/trans-types.c index 0571bd1..66dd99e 100644 --- a/gcc/fortran/trans-types.c +++ b/gcc/fortran/trans-types.c @@ -919,8 +919,6 @@ gfc_init_types (void) gfc_max_array_element_size = build_int_cst_wide (long_unsigned_type_node, lo, hi); - size_type_node = gfc_array_index_type; - boolean_type_node = gfc_get_logical_type (gfc_default_logical_kind); boolean_true_node = build_int_cst (boolean_type_node, 1); boolean_false_node = build_int_cst (boolean_type_node, 0);
Hi, I've been looking a bit at improving our detection of overflow when allocating arrays (PR 28105), and I noticed that the definition of size_t is a bit messed up. sizetype is properly set to an unsigned type of the right size, but the expression is calculated in a bit needlessly complicated way. However, size_type_node is an alias for gfc_array_index_type which is a signed type. This is clearly wrong. The attached patch fixes this (the fix has taken inspiration from the java frontend, see java/decl.c). I went through all uses of size_type_node in the frontend, and they are all used for building calls to malloc/realloc/memset/memmove so size_type_node is the correct choice; also the openmp stuff uses it for calculating some size for reductions, this seems ok as well. However, this patch does introduce a small regression (which the testsuite doesn't test for), namely since size_type_node is now changed to an unsigned type, the overflow test which checks for size < 0 when allocating arrays is optimized away. This test is quite ad hoc, and has only a 50% chance of succeeding anyway. I assigned PR 28105 to myself and 'm working on coming up with a better way. Regtested on x86_64-unknown-linux-gnu, Ok for trunk? 2010-11-20 Janne Blomqvist <jb@gcc.gnu.org> * f95-lang.c (gfc_init_decl_processing): Set size_type_node as unsigned int of pointer size and set sizetype based on that. * trans-types.c (gfc_init_types): Don't set size_type_node to an unsigned type.