On 11/23/18 6:10 PM, Martin Sebor wrote:
> On 11/23/18 8:06 AM, Martin Liška wrote:
>> Hi.
>>
>> It's patch proposal that suggests to use an enum instead of 'int endp' for
>> functions that expand memory move builtins. I've touch the code multiple
>> times
>> and it always take me time to realize what the magic numbers 0, 1, 2 mean.
>>
>> Does the suggestion make sense? Do you like enum values? Can it be installed
>> now?
>> If so I can finish the patch (few targets must be ported and ChangeLog entry
>> is missing).
>
> I think an enum will make the code much more readable.
>
> I'm not sure the name choices are the most intuitive. Maybe
> something like this instead?
>
> RETURN_BEGIN, RETURN_END, RETURN_END_MINUS_1
>
> (I suggest BEGIN rather than START because of C++ iterators, but
> I see the bigger readability gain in using RETURN over POINTER;
> IMO, it would be also nice to rename endp to retp or retmode
> or something like that).
>
> For the RETURN_END_MINUS_1 comment, I would just should say
> "return a pointer to the terminating null byte of the string,"
> rather than "to the end of it." (The latter might seem to
> contradict RETURN_END).
>
> Martin
Thank you Martin for feedback. I'm sending tested version of the patch
that survives bootstrap on x86_64-linux-gnu.
Ready for trunk?
Martin
>From 1709326b01079cfca299e2b57be18bd616954bf8 Mon Sep 17 00:00:00 2001
From: marxin <mli...@suse.cz>
Date: Fri, 23 Nov 2018 15:51:05 +0100
Subject: [PATCH] Come up with memop_ret enum instead of int endp for memory
operations.
gcc/ChangeLog:
2018-11-26 Martin Liska <mli...@suse.cz>
* asan.c (asan_emit_stack_protection): Use new enum values
instead of int constants.
* builtins.c (expand_builtin_memory_copy_args): Replace int
type with memop_ret enum type.
(expand_builtin_mempcpy_args): Likewise.
(expand_builtin_memcpy): Use new enum values
instead of int constants. Likewise.
(expand_builtin_mempcpy): Likewise.
(expand_movstr): Likewise.
(expand_builtin_strcpy_args): Likewise.
(expand_builtin_stpcpy_1): Likewise.
(expand_builtin_strncpy): Likewise.
(expand_builtin_memset_args): Likewise.
* expr.c (move_by_pieces_d::finish_endp): Rename to ...
(move_by_pieces_d::finish_retmode): ... this.
(move_by_pieces): Change last argument type to memop_ret.
(store_by_pieces): Use new enum values
instead of int constants.
(emit_block_move_hints): Likewise.
(emit_push_insn): Likewise.
(store_expr): Likewise.
* expr.h (store_by_pieces): Change int to newly added enum
type.
* rtl.h (enum memop_ret): Define.
(move_by_pieces): Use the enum type.
---
gcc/asan.c | 2 +-
gcc/builtins.c | 73 +++++++++++++++++++++++++-------------------------
gcc/expr.c | 53 +++++++++++++++++-------------------
gcc/expr.h | 2 +-
gcc/rtl.h | 17 +++++++++++-
5 files changed, 79 insertions(+), 68 deletions(-)
diff --git a/gcc/asan.c b/gcc/asan.c
index b2c41187b91..45906bf8fee 100644
--- a/gcc/asan.c
+++ b/gcc/asan.c
@@ -1484,7 +1484,7 @@ asan_emit_stack_protection (rtx base, rtx pbase, unsigned int alignb,
&& can_store_by_pieces (sz, builtin_memset_read_str, &c,
BITS_PER_UNIT, true))
store_by_pieces (shadow_mem, sz, builtin_memset_read_str, &c,
- BITS_PER_UNIT, true, 0);
+ BITS_PER_UNIT, true, RETURN_BEGIN);
else if (use_after_return_class >= 5
|| !set_storage_via_setmem (shadow_mem,
GEN_INT (sz),
diff --git a/gcc/builtins.c b/gcc/builtins.c
index ebde2db6e64..dcac49d8be1 100644
--- a/gcc/builtins.c
+++ b/gcc/builtins.c
@@ -126,10 +126,11 @@ static rtx builtin_memcpy_read_str (void *, HOST_WIDE_INT, scalar_int_mode);
static rtx expand_builtin_memchr (tree, rtx);
static rtx expand_builtin_memcpy (tree, rtx);
static rtx expand_builtin_memory_copy_args (tree dest, tree src, tree len,
- rtx target, tree exp, int endp);
+ rtx target, tree exp,
+ memop_ret retmode);
static rtx expand_builtin_memmove (tree, rtx);
static rtx expand_builtin_mempcpy (tree, rtx);
-static rtx expand_builtin_mempcpy_args (tree, tree, tree, rtx, tree, int);
+static rtx expand_builtin_mempcpy_args (tree, tree, tree, rtx, tree, memop_ret);
static rtx expand_builtin_strcat (tree, rtx);
static rtx expand_builtin_strcpy (tree, rtx);
static rtx expand_builtin_strcpy_args (tree, tree, tree, rtx);
@@ -3751,7 +3752,7 @@ expand_builtin_memcpy (tree exp, rtx target)
check_memop_access (exp, dest, src, len);
return expand_builtin_memory_copy_args (dest, src, len, target, exp,
- /*endp=*/ 0);
+ /*retmode=*/ RETURN_BEGIN);
}
/* Check a call EXP to the memmove built-in for validity.
@@ -3776,10 +3777,7 @@ expand_builtin_memmove (tree exp, rtx)
/* Expand a call EXP to the mempcpy builtin.
Return NULL_RTX if we failed; the caller should emit a normal call,
otherwise try to get the result in TARGET, if convenient (and in
- mode MODE if that's convenient). If ENDP is 0 return the
- destination pointer, if ENDP is 1 return the end pointer ala
- mempcpy, and if ENDP is 2 return the end pointer minus one ala
- stpcpy. */
+ mode MODE if that's convenient). */
static rtx
expand_builtin_mempcpy (tree exp, rtx target)
@@ -3812,20 +3810,17 @@ expand_builtin_mempcpy (tree exp, rtx target)
return NULL_RTX;
return expand_builtin_mempcpy_args (dest, src, len,
- target, exp, /*endp=*/ 1);
+ target, exp, /*retmode=*/ RETURN_END);
}
/* Helper function to do the actual work for expand of memory copy family
functions (memcpy, mempcpy, stpcpy). Expansing should assign LEN bytes
- of memory from SRC to DEST and assign to TARGET if convenient.
- If ENDP is 0 return the
- destination pointer, if ENDP is 1 return the end pointer ala
- mempcpy, and if ENDP is 2 return the end pointer minus one ala
- stpcpy. */
+ of memory from SRC to DEST and assign to TARGET if convenient. Return
+ value is based on RETMODE argument. */
static rtx
expand_builtin_memory_copy_args (tree dest, tree src, tree len,
- rtx target, tree exp, int endp)
+ rtx target, tree exp, memop_ret retmode)
{
const char *src_str;
unsigned int src_align = get_pointer_alignment (src);
@@ -3872,7 +3867,7 @@ expand_builtin_memory_copy_args (tree dest, tree src, tree len,
dest_mem = store_by_pieces (dest_mem, INTVAL (len_rtx),
builtin_memcpy_read_str,
CONST_CAST (char *, src_str),
- dest_align, false, endp);
+ dest_align, false, retmode);
dest_mem = force_operand (XEXP (dest_mem, 0), target);
dest_mem = convert_memory_address (ptr_mode, dest_mem);
return dest_mem;
@@ -3883,9 +3878,10 @@ expand_builtin_memory_copy_args (tree dest, tree src, tree len,
/* Copy word part most expediently. */
enum block_op_methods method = BLOCK_OP_NORMAL;
- if (CALL_EXPR_TAILCALL (exp) && (endp == 0 || target == const0_rtx))
+ if (CALL_EXPR_TAILCALL (exp)
+ && (retmode == RETURN_BEGIN || target == const0_rtx))
method = BLOCK_OP_TAILCALL;
- if (endp == 1 && target != const0_rtx)
+ if (retmode == RETURN_END && target != const0_rtx)
method = BLOCK_OP_NO_LIBCALL_RET;
dest_addr = emit_block_move_hints (dest_mem, src_mem, len_rtx, method,
expected_align, expected_size,
@@ -3899,11 +3895,11 @@ expand_builtin_memory_copy_args (tree dest, tree src, tree len,
dest_addr = convert_memory_address (ptr_mode, dest_addr);
}
- if (endp && target != const0_rtx)
+ if (retmode != RETURN_BEGIN && target != const0_rtx)
{
dest_addr = gen_rtx_PLUS (ptr_mode, dest_addr, len_rtx);
/* stpcpy pointer to last byte. */
- if (endp == 2)
+ if (retmode == RETURN_END_MINUS_ONE)
dest_addr = gen_rtx_MINUS (ptr_mode, dest_addr, const1_rtx);
}
@@ -3912,21 +3908,19 @@ expand_builtin_memory_copy_args (tree dest, tree src, tree len,
static rtx
expand_builtin_mempcpy_args (tree dest, tree src, tree len,
- rtx target, tree orig_exp, int endp)
+ rtx target, tree orig_exp, memop_ret retmode)
{
return expand_builtin_memory_copy_args (dest, src, len, target, orig_exp,
- endp);
+ retmode);
}
/* Expand into a movstr instruction, if one is available. Return NULL_RTX if
we failed, the caller should emit a normal call, otherwise try to
- get the result in TARGET, if convenient. If ENDP is 0 return the
- destination pointer, if ENDP is 1 return the end pointer ala
- mempcpy, and if ENDP is 2 return the end pointer minus one ala
- stpcpy. */
+ get the result in TARGET, if convenient.
+ Return value is based on RETMODE argument. */
static rtx
-expand_movstr (tree dest, tree src, rtx target, int endp)
+expand_movstr (tree dest, tree src, rtx target, memop_ret retmode)
{
struct expand_operand ops[3];
rtx dest_mem;
@@ -3937,25 +3931,25 @@ expand_movstr (tree dest, tree src, rtx target, int endp)
dest_mem = get_memory_rtx (dest, NULL);
src_mem = get_memory_rtx (src, NULL);
- if (!endp)
+ if (retmode != RETURN_BEGIN)
{
target = force_reg (Pmode, XEXP (dest_mem, 0));
dest_mem = replace_equiv_address (dest_mem, target);
}
- create_output_operand (&ops[0], endp ? target : NULL_RTX, Pmode);
+ create_output_operand (&ops[0], retmode ? target : NULL_RTX, Pmode);
create_fixed_operand (&ops[1], dest_mem);
create_fixed_operand (&ops[2], src_mem);
if (!maybe_expand_insn (targetm.code_for_movstr, 3, ops))
return NULL_RTX;
- if (endp && target != const0_rtx)
+ if (retmode != RETURN_BEGIN && target != const0_rtx)
{
target = ops[0].value;
/* movstr is supposed to set end to the address of the NUL
terminator. If the caller requested a mempcpy-like return value,
adjust it. */
- if (endp == 1)
+ if (retmode == RETURN_END)
{
rtx tem = plus_constant (GET_MODE (target),
gen_lowpart (GET_MODE (target), target), 1);
@@ -4044,7 +4038,7 @@ expand_builtin_strcpy_args (tree exp, tree dest, tree src, rtx target)
return NULL_RTX;
}
- return expand_movstr (dest, src, target, /*endp=*/0);
+ return expand_movstr (dest, src, target, /*retmode=*/ RETURN_BEGIN);
}
/* Expand a call EXP to the stpcpy builtin.
@@ -4091,14 +4085,16 @@ expand_builtin_stpcpy_1 (tree exp, rtx target, machine_mode mode)
memset (&data, 0, sizeof (c_strlen_data));
if (!c_getstr (src, NULL)
|| !(len = c_strlen (src, 0, &data, 1)))
- return expand_movstr (dst, src, target, /*endp=*/2);
+ return expand_movstr (dst, src, target,
+ /*retmode=*/ RETURN_END_MINUS_ONE);
if (data.decl && !TREE_NO_WARNING (exp))
warn_string_no_nul (EXPR_LOCATION (exp), "stpcpy", src, data.decl);
lenp1 = size_binop_loc (loc, PLUS_EXPR, len, ssize_int (1));
ret = expand_builtin_mempcpy_args (dst, src, lenp1,
- target, exp, /*endp=*/2);
+ target, exp,
+ /*retmode=*/ RETURN_END_MINUS_ONE);
if (ret)
return ret;
@@ -4132,7 +4128,8 @@ expand_builtin_stpcpy_1 (tree exp, rtx target, machine_mode mode)
}
}
- return expand_movstr (dst, src, target, /*endp=*/2);
+ return expand_movstr (dst, src, target,
+ /*retmode=*/ RETURN_END_MINUS_ONE);
}
}
@@ -4378,7 +4375,8 @@ expand_builtin_strncpy (tree exp, rtx target)
dest_mem = get_memory_rtx (dest, len);
store_by_pieces (dest_mem, tree_to_uhwi (len),
builtin_strncpy_read_str,
- CONST_CAST (char *, p), dest_align, false, 0);
+ CONST_CAST (char *, p), dest_align, false,
+ RETURN_BEGIN);
dest_mem = force_operand (XEXP (dest_mem, 0), target);
dest_mem = convert_memory_address (ptr_mode, dest_mem);
return dest_mem;
@@ -4523,7 +4521,7 @@ expand_builtin_memset_args (tree dest, tree val, tree len,
val_rtx = force_reg (val_mode, val_rtx);
store_by_pieces (dest_mem, tree_to_uhwi (len),
builtin_memset_gen_str, val_rtx, dest_align,
- true, 0);
+ true, RETURN_BEGIN);
}
else if (!set_storage_via_setmem (dest_mem, len_rtx, val_rtx,
dest_align, expected_align,
@@ -4546,7 +4544,8 @@ expand_builtin_memset_args (tree dest, tree val, tree len,
builtin_memset_read_str, &c, dest_align,
true))
store_by_pieces (dest_mem, tree_to_uhwi (len),
- builtin_memset_read_str, &c, dest_align, true, 0);
+ builtin_memset_read_str, &c, dest_align, true,
+ RETURN_BEGIN);
else if (!set_storage_via_setmem (dest_mem, len_rtx,
gen_int_mode (c, val_mode),
dest_align, expected_align,
diff --git a/gcc/expr.c b/gcc/expr.c
index 7ae3e37378c..ad0f95c90d0 100644
--- a/gcc/expr.c
+++ b/gcc/expr.c
@@ -1146,7 +1146,7 @@ class move_by_pieces_d : public op_by_pieces_d
: op_by_pieces_d (to, false, from, true, NULL, NULL, len, align)
{
}
- rtx finish_endp (int);
+ rtx finish_retmode (memop_ret);
};
/* Return true if MODE can be used for a set of copies, given an
@@ -1182,15 +1182,14 @@ move_by_pieces_d::generate (rtx op0, rtx op1,
}
/* Perform the final adjustment at the end of a string to obtain the
- correct return value for the block operation. If ENDP is 1 return
- memory at the end ala mempcpy, and if ENDP is 2 return memory the
- end minus one byte ala stpcpy. */
+ correct return value for the block operation.
+ Return value is based on RETMODE argument. */
rtx
-move_by_pieces_d::finish_endp (int endp)
+move_by_pieces_d::finish_retmode (memop_ret retmode)
{
gcc_assert (!m_reverse);
- if (endp == 2)
+ if (retmode == RETURN_END_MINUS_ONE)
{
m_to.maybe_postinc (-1);
--m_offset;
@@ -1206,13 +1205,11 @@ move_by_pieces_d::finish_endp (int endp)
ALIGN is maximum stack alignment we can assume.
- If ENDP is 0 return to, if ENDP is 1 return memory at the end ala
- mempcpy, and if ENDP is 2 return memory the end minus one byte ala
- stpcpy. */
+ Return value is based on RETMODE argument. */
rtx
move_by_pieces (rtx to, rtx from, unsigned HOST_WIDE_INT len,
- unsigned int align, int endp)
+ unsigned int align, memop_ret retmode)
{
#ifndef PUSH_ROUNDING
if (to == NULL)
@@ -1223,8 +1220,8 @@ move_by_pieces (rtx to, rtx from, unsigned HOST_WIDE_INT len,
data.run ();
- if (endp)
- return data.finish_endp (endp);
+ if (retmode)
+ return data.finish_retmode (retmode);
else
return to;
}
@@ -1244,7 +1241,7 @@ class store_by_pieces_d : public op_by_pieces_d
: op_by_pieces_d (to, false, NULL_RTX, true, cfn, cfn_data, len, align)
{
}
- rtx finish_endp (int);
+ rtx finish_retmode (memop_ret);
};
/* Return true if MODE can be used for a set of stores, given an
@@ -1272,15 +1269,14 @@ store_by_pieces_d::generate (rtx op0, rtx op1, machine_mode)
}
/* Perform the final adjustment at the end of a string to obtain the
- correct return value for the block operation. If ENDP is 1 return
- memory at the end ala mempcpy, and if ENDP is 2 return memory the
- end minus one byte ala stpcpy. */
+ correct return value for the block operation.
+ Return value is based on RETMODE argument. */
rtx
-store_by_pieces_d::finish_endp (int endp)
+store_by_pieces_d::finish_retmode (memop_ret retmode)
{
gcc_assert (!m_reverse);
- if (endp == 2)
+ if (retmode == RETURN_END_MINUS_ONE)
{
m_to.maybe_postinc (-1);
--m_offset;
@@ -1370,18 +1366,17 @@ can_store_by_pieces (unsigned HOST_WIDE_INT len,
pointer which will be passed as argument in every CONSTFUN call.
ALIGN is maximum alignment we can assume. MEMSETP is true if this is
a memset operation and false if it's a copy of a constant string.
- If ENDP is 0 return to, if ENDP is 1 return memory at the end ala
- mempcpy, and if ENDP is 2 return memory the end minus one byte ala
- stpcpy. */
+ Return value is based on RETMODE argument. */
rtx
store_by_pieces (rtx to, unsigned HOST_WIDE_INT len,
rtx (*constfun) (void *, HOST_WIDE_INT, scalar_int_mode),
- void *constfundata, unsigned int align, bool memsetp, int endp)
+ void *constfundata, unsigned int align, bool memsetp,
+ memop_ret retmode)
{
if (len == 0)
{
- gcc_assert (endp != 2);
+ gcc_assert (retmode != 2);
return to;
}
@@ -1393,8 +1388,8 @@ store_by_pieces (rtx to, unsigned HOST_WIDE_INT len,
store_by_pieces_d data (to, constfun, constfundata, len, align);
data.run ();
- if (endp)
- return data.finish_endp (endp);
+ if (retmode)
+ return data.finish_retmode (retmode);
else
return to;
}
@@ -1624,7 +1619,7 @@ emit_block_move_hints (rtx x, rtx y, rtx size, enum block_op_methods method,
}
if (CONST_INT_P (size) && can_move_by_pieces (INTVAL (size), align))
- move_by_pieces (x, y, INTVAL (size), align, 0);
+ move_by_pieces (x, y, INTVAL (size), align, RETURN_BEGIN);
else if (emit_block_move_via_movmem (x, y, size, align,
expected_align, expected_size,
min_size, max_size, probable_max_size))
@@ -4421,7 +4416,8 @@ emit_push_insn (rtx x, machine_mode mode, tree type, rtx size,
&& where_pad != stack_direction)
anti_adjust_stack (gen_int_mode (extra, Pmode));
- move_by_pieces (NULL, xinner, INTVAL (size) - used, align, 0);
+ move_by_pieces (NULL, xinner, INTVAL (size) - used, align,
+ RETURN_BEGIN);
}
else
#endif /* PUSH_ROUNDING */
@@ -5618,7 +5614,8 @@ store_expr (tree exp, rtx target, int call_param_p,
CONST_CAST (char *,
TREE_STRING_POINTER (str)),
MEM_ALIGN (target), false,
- exp_len > str_copy_len ? 1 : 0);
+ (exp_len > str_copy_len ? RETURN_END :
+ RETURN_BEGIN));
if (exp_len > str_copy_len)
clear_storage (adjust_address (dest_mem, BLKmode, 0),
GEN_INT (exp_len - str_copy_len),
diff --git a/gcc/expr.h b/gcc/expr.h
index 19028f0e6a4..6ae343d81f0 100644
--- a/gcc/expr.h
+++ b/gcc/expr.h
@@ -219,7 +219,7 @@ extern int can_store_by_pieces (unsigned HOST_WIDE_INT,
MEMSETP is true if this is a real memset/bzero, not a copy.
Returns TO + LEN. */
extern rtx store_by_pieces (rtx, unsigned HOST_WIDE_INT, by_pieces_constfn,
- void *, unsigned int, bool, int);
+ void *, unsigned int, bool, memop_ret);
/* Emit insns to set X from Y. */
extern rtx_insn *emit_move_insn (rtx, rtx);
diff --git a/gcc/rtl.h b/gcc/rtl.h
index dd3ce06295a..c2aaa9eff4b 100644
--- a/gcc/rtl.h
+++ b/gcc/rtl.h
@@ -4046,9 +4046,24 @@ extern void expand_null_return (void);
extern void expand_naked_return (void);
extern void emit_jump (rtx);
+/* Memory operation built-ins differ by return value. Mapping
+ of the enum values is following:
+ - RETURN_BEGIN - return destination, e.g. memcpy
+ - RETURN_END - return destination + n, e.g. mempcpy
+ - RETURN_END_MINUS_ONE - return a pointer to the terminating
+ null byte of the string, e.g. strcpy
+*/
+
+enum memop_ret
+{
+ RETURN_BEGIN,
+ RETURN_END,
+ RETURN_END_MINUS_ONE
+};
+
/* In expr.c */
extern rtx move_by_pieces (rtx, rtx, unsigned HOST_WIDE_INT,
- unsigned int, int);
+ unsigned int, memop_ret);
extern poly_int64 find_args_size_adjust (rtx_insn *);
extern poly_int64 fixup_args_size_notes (rtx_insn *, rtx_insn *, poly_int64);
--
2.19.1
