| Message ID | ZqtNFuQm0shtuzdZ@tucnak |
|---|---|
| State | New |
| Headers | show |
| Series | fortran: Fix up paste in gfc_get_array_descr_info | expand |
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 >
--- 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);
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