diff mbox series

fortran: Fix up paste in gfc_get_array_descr_info

Message ID ZqtNFuQm0shtuzdZ@tucnak
State New
Headers
Series fortran: Fix up paste in gfc_get_array_descr_info |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gcc_build--master-aarch64 success Build passed
linaro-tcwg-bot/tcwg_gcc_build--master-arm success Build passed

Commit Message

Jakub Jelinek Aug. 1, 2024, 8:53 a.m. UTC 
  Hi!

A static analyzer found a pasto in gfc_get_array_descr_info.
The code does
      t = base_decl;
      if (!integer_zerop (dtype_off))
        t = fold_build_pointer_plus (t, dtype_off);
      dtype = TYPE_MAIN_VARIANT (get_dtype_type_node ());
      field = gfc_advance_chain (TYPE_FIELDS (dtype), GFC_DTYPE_RANK);
      rank_off = byte_position (field);
      if (!integer_zerop (dtype_off))
        t = fold_build_pointer_plus (t, rank_off);
i.e. uses the same !integer_zerop check between both, while it should
be checking rank_off in the latter case.
This actually doesn't change anything on the generated code, because
both the dtype_off and rank_off aren't zero,
typedef struct dtype_type
{
  size_t elem_len;
  int version;
  signed char rank;
  signed char type;
  signed short attribute;
}
dtype_type;
struct {
  type *base_addr;
  size_t offset;
  dtype_type dtype;
  index_type span;
  descriptor_dimension dim[];
};
dtype_off is 16 on 64-bit arches and 8 on 32-bit ones and rank_off is
12 on 64-bit arches and 8 on 32-bit arches, so this patch is just to
pacify those static analyzers or be prepared if the ABI changes in the
future.
Though, as we know that the offsets currently aren't zero, checking for
!integer_zerop isn't actually an optimization but slow things down,
fold_build_pointer_plus will handle += 0 as well, so maybe we should just
drop it for the cases where we know it isn't 0.

Anyway, the following patch just does the minimal change,
bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2024-08-01  Jakub Jelinek  <jakub@redhat.com>

	* trans-types.cc (gfc_get_array_descr_info): Add rank_off to t
	if rank_off isn't zero rather than dtype_off.


	Jakub

Comments

Mikael Morin Aug. 1, 2024, 10:12 a.m. UTC | #1 
Hello,

Le 01/08/2024 à 10:53, Jakub Jelinek a écrit :
> Hi!
> 
> A static analyzer found a pasto in gfc_get_array_descr_info.
> The code does
>        t = base_decl;
>        if (!integer_zerop (dtype_off))
>          t = fold_build_pointer_plus (t, dtype_off);
>        dtype = TYPE_MAIN_VARIANT (get_dtype_type_node ());
>        field = gfc_advance_chain (TYPE_FIELDS (dtype), GFC_DTYPE_RANK);
>        rank_off = byte_position (field);
>        if (!integer_zerop (dtype_off))
>          t = fold_build_pointer_plus (t, rank_off);
> i.e. uses the same !integer_zerop check between both, while it should
> be checking rank_off in the latter case.
> This actually doesn't change anything on the generated code, because
> both the dtype_off and rank_off aren't zero,
> typedef struct dtype_type
> {
>    size_t elem_len;
>    int version;
>    signed char rank;
>    signed char type;
>    signed short attribute;
> }
> dtype_type;
> struct {
>    type *base_addr;
>    size_t offset;
>    dtype_type dtype;
>    index_type span;
>    descriptor_dimension dim[];
> };
> dtype_off is 16 on 64-bit arches and 8 on 32-bit ones and rank_off is
> 12 on 64-bit arches and 8 on 32-bit arches, so this patch is just to
> pacify those static analyzers or be prepared if the ABI changes in the
> future.
> Though, as we know that the offsets currently aren't zero, checking for
> !integer_zerop isn't actually an optimization but slow things down,
> fold_build_pointer_plus will handle += 0 as well, so maybe we should just
> drop it for the cases where we know it isn't 0.
> 
Yes, I've always wondered how much of a win these integer_zerop checks 
were, probably not that much.  In the cases we know they are useless, 
let's remove them (patch pre-approved for gfc_get_array_descr_info).

> Anyway, the following patch just does the minimal change,
> bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> 
Looks good, but as said removing the check seems preferable.

> 2024-08-01  Jakub Jelinek  <jakub@redhat.com>
> 
> 	* trans-types.cc (gfc_get_array_descr_info): Add rank_off to t
> 	if rank_off isn't zero rather than dtype_off.
> 
> --- gcc/fortran/trans-types.cc.jj	2024-07-19 17:22:59.405097054 +0200
> +++ gcc/fortran/trans-types.cc	2024-07-31 20:48:20.546569993 +0200
> @@ -3605,7 +3605,7 @@ gfc_get_array_descr_info (const_tree typ
>         dtype = TYPE_MAIN_VARIANT (get_dtype_type_node ());
>         field = gfc_advance_chain (TYPE_FIELDS (dtype), GFC_DTYPE_RANK);
>         rank_off = byte_position (field);
> -      if (!integer_zerop (dtype_off))
> +      if (!integer_zerop (rank_off))
>   	t = fold_build_pointer_plus (t, rank_off);
>   
>         t = build1 (NOP_EXPR, build_pointer_type (TREE_TYPE (field)), t);
> 
> 	Jakub
>
diff mbox series

Patch

--- gcc/fortran/trans-types.cc.jj	2024-07-19 17:22:59.405097054 +0200
+++ gcc/fortran/trans-types.cc	2024-07-31 20:48:20.546569993 +0200
@@ -3605,7 +3605,7 @@  gfc_get_array_descr_info (const_tree typ
       dtype = TYPE_MAIN_VARIANT (get_dtype_type_node ());
       field = gfc_advance_chain (TYPE_FIELDS (dtype), GFC_DTYPE_RANK);
       rank_off = byte_position (field);
-      if (!integer_zerop (dtype_off))
+      if (!integer_zerop (rank_off))
 	t = fold_build_pointer_plus (t, rank_off);
 
       t = build1 (NOP_EXPR, build_pointer_type (TREE_TYPE (field)), t);