Patchwork [fortran] Fix sizetype and size_type_node in the Fortran frontend

login
register
mail settings
Submitter Janne Blomqvist
Date Nov. 20, 2010, 8:52 a.m.
Message ID <AANLkTikPU5bB407Lvrw7TAXCRWRm46RMi8YY0wMU1do-@mail.gmail.com>
Download mbox | patch
Permalink /patch/72346/
State New
Headers show

Comments

Janne Blomqvist - Nov. 20, 2010, 8:52 a.m.
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.
Tobias Burnus - Nov. 20, 2010, 9:34 a.m.
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.
Tobias Burnus - Nov. 20, 2010, 1:18 p.m.
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.
>
>
Janne Blomqvist - Nov. 20, 2010, 1:56 p.m.
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.

Patch

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);