Fix Fortran size_t parameter passing

Message ID 0be9bd9b-4efb-47da-e4ce-2c8ccb6f7f9a@codesourcery.com
State New
Headers show
Series
  • Fix Fortran size_t parameter passing
Related show

Commit Message

Andrew Stubbs May 22, 2019, 10:54 a.m.
This patch fixes a bug observed on amdgcn in which the Fortran frontend 
creates function calls using the 32-bit parameters where they ought to 
be 64-bit, resulting in UB.

The issue is caused by the use of "integer_zero_node" where the 
definition of the function calls for "size_zero_node". I presume this 
works on other architectures because the types are the same size, or 
else because parameters are always 64-bit wide.

OK to commit?

Andrew

Comments

Janne Blomqvist May 22, 2019, 11:35 a.m. | #1
On Wed, May 22, 2019 at 1:54 PM Andrew Stubbs <ams@codesourcery.com> wrote:
>

> This patch fixes a bug observed on amdgcn in which the Fortran frontend

> creates function calls using the 32-bit parameters where they ought to

> be 64-bit, resulting in UB.

>

> The issue is caused by the use of "integer_zero_node" where the

> definition of the function calls for "size_zero_node". I presume this

> works on other architectures because the types are the same size, or

> else because parameters are always 64-bit wide.

>

> OK to commit?

>

> Andrew


Thanks for the catch. Though for size_t you should use build_zero_cst
(size_type_node). size_zero_node is a zero constant of type sizetype,
which is not the same as size_type_node (size_t in C). Ok with that
change.


-- 
Janne Blomqvist
Andrew Stubbs May 22, 2019, 12:20 p.m. | #2
On 22/05/2019 12:35, Janne Blomqvist wrote:
> Thanks for the catch. Though for size_t you should use build_zero_cst

> (size_type_node). size_zero_node is a zero constant of type sizetype,

> which is not the same as size_type_node (size_t in C). Ok with that

> change.


So, integer_zero_node is compatible with integer_type_node, but 
size_zero_node is not (necessarily) compatible with size_type_node? 
Well, that's just asking for trouble. :-(

Just to confirm, is the attached what you mean?

Thanks

Andrew
Fix fortran size_type_node parameter passing.

2019-05-22  Andrew Stubbs  <ams@codesourcery.com>

	gcc/fortran/
	* trans-stmt.c (gfc_trans_critical): Use size_type_node for
	gfor_fndecl_caf_lock and gfor_fndecl_caf_unlock calls.
	(gfc_trans_allocate): Use size_type_node for gfor_fndecl_caf_sync_all
	call.

diff --git a/gcc/fortran/trans-stmt.c b/gcc/fortran/trans-stmt.c
index 5fa182bf05a..7c365634085 100644
--- a/gcc/fortran/trans-stmt.c
+++ b/gcc/fortran/trans-stmt.c
@@ -1576,12 +1576,13 @@ gfc_trans_critical (gfc_code *code)
 
   if (flag_coarray == GFC_FCOARRAY_LIB)
     {
+      tree zero_size = build_zero_cst (size_type_node);
       token = gfc_get_symbol_decl (code->resolved_sym);
       token = GFC_TYPE_ARRAY_CAF_TOKEN (TREE_TYPE (token));
       tmp = build_call_expr_loc (input_location, gfor_fndecl_caf_lock, 7,
-				 token, integer_zero_node, integer_one_node,
+				 token, zero_size, integer_one_node,
 				 null_pointer_node, null_pointer_node,
-				 null_pointer_node, integer_zero_node);
+				 null_pointer_node, zero_size);
       gfc_add_expr_to_block (&block, tmp);
 
       /* It guarantees memory consistency within the same segment */
@@ -1601,10 +1602,11 @@ gfc_trans_critical (gfc_code *code)
 
   if (flag_coarray == GFC_FCOARRAY_LIB)
     {
+      tree zero_size = build_zero_cst (size_type_node);
       tmp = build_call_expr_loc (input_location, gfor_fndecl_caf_unlock, 6,
-				 token, integer_zero_node, integer_one_node,
+				 token, zero_size, integer_one_node,
 				 null_pointer_node, null_pointer_node,
-				 integer_zero_node);
+				 zero_size);
       gfc_add_expr_to_block (&block, tmp);
 
       /* It guarantees memory consistency within the same segment */
@@ -6772,9 +6774,10 @@ gfc_trans_allocate (gfc_code * code)
   if (needs_caf_sync)
     {
       /* Add a sync all after the allocation has been executed.  */
+      tree zero_size = build_zero_cst (size_type_node);
       tmp = build_call_expr_loc (input_location, gfor_fndecl_caf_sync_all,
 				 3, null_pointer_node, null_pointer_node,
-				 integer_zero_node);
+				 zero_size);
       gfc_add_expr_to_block (&post, tmp);
     }
Janne Blomqvist May 22, 2019, 12:28 p.m. | #3
On Wed, May 22, 2019 at 3:20 PM Andrew Stubbs <ams@codesourcery.com> wrote:
>

> On 22/05/2019 12:35, Janne Blomqvist wrote:

> > Thanks for the catch. Though for size_t you should use build_zero_cst

> > (size_type_node). size_zero_node is a zero constant of type sizetype,

> > which is not the same as size_type_node (size_t in C). Ok with that

> > change.

>

> So, integer_zero_node is compatible with integer_type_node, but

> size_zero_node is not (necessarily) compatible with size_type_node?

> Well, that's just asking for trouble. :-(


Indeed it is. IIRC the main difference is that while both are unsigned
types, sizetype has undefined behavior on overflow whereas
size_type_node wraps around. And sizetype is an internal
implementation detail, so should not be used in ABI-visible places.

> Just to confirm, is the attached what you mean?


Yes, looks good.

-- 
Janne Blomqvist
Andrew Stubbs May 22, 2019, 1:13 p.m. | #4
On 22/05/2019 13:28, Janne Blomqvist wrote:
>> Just to confirm, is the attached what you mean?

> 

> Yes, looks good.


Thanks, now committed.

Andrew

Patch

Fix fortran size_t parameter passing.

2019-05-22  Andrew Stubbs  <ams@codesourcery.com>

	gcc/fortran/
	* trans-stmt.c (gfc_trans_critical): Use size_zero_node for
	gfor_fndecl_caf_lock and gfor_fndecl_caf_unlock calls.
	(gfc_trans_allocate): Use size_zero_node for gfor_fndecl_caf_sync_all
	call.

diff --git a/gcc/fortran/trans-stmt.c b/gcc/fortran/trans-stmt.c
index 5fa182bf05a..4314a1edb90 100644
--- a/gcc/fortran/trans-stmt.c
+++ b/gcc/fortran/trans-stmt.c
@@ -1579,9 +1579,9 @@  gfc_trans_critical (gfc_code *code)
       token = gfc_get_symbol_decl (code->resolved_sym);
       token = GFC_TYPE_ARRAY_CAF_TOKEN (TREE_TYPE (token));
       tmp = build_call_expr_loc (input_location, gfor_fndecl_caf_lock, 7,
-				 token, integer_zero_node, integer_one_node,
+				 token, size_zero_node, integer_one_node,
 				 null_pointer_node, null_pointer_node,
-				 null_pointer_node, integer_zero_node);
+				 null_pointer_node, size_zero_node);
       gfc_add_expr_to_block (&block, tmp);
 
       /* It guarantees memory consistency within the same segment */
@@ -1602,9 +1602,9 @@  gfc_trans_critical (gfc_code *code)
   if (flag_coarray == GFC_FCOARRAY_LIB)
     {
       tmp = build_call_expr_loc (input_location, gfor_fndecl_caf_unlock, 6,
-				 token, integer_zero_node, integer_one_node,
+				 token, size_zero_node, integer_one_node,
 				 null_pointer_node, null_pointer_node,
-				 integer_zero_node);
+				 size_zero_node);
       gfc_add_expr_to_block (&block, tmp);
 
       /* It guarantees memory consistency within the same segment */
@@ -6774,7 +6774,7 @@  gfc_trans_allocate (gfc_code * code)
       /* Add a sync all after the allocation has been executed.  */
       tmp = build_call_expr_loc (input_location, gfor_fndecl_caf_sync_all,
 				 3, null_pointer_node, null_pointer_node,
-				 integer_zero_node);
+				 size_zero_node);
       gfc_add_expr_to_block (&post, tmp);
     }