On 6/27/19 9:15 AM, Jeff Law wrote:
On 6/27/19 9:00 AM, Jeff Law wrote:
On 6/26/19 8:40 PM, Martin Sebor wrote:
On 6/26/19 4:31 PM, Jeff Law wrote:
On 6/25/19 5:03 PM, Martin Sebor wrote:
The caller ensures that handle_char_store is only called for stores
to arrays (MEM_REF) or single elements as wide as char.
Where? I don't see it, even after fixing the formatting in
strlen_check_and_optimize_stmt :-)
gimple *stmt = gsi_stmt (*gsi);
if (is_gimple_call (stmt))
I think we can agree that we don't have a call, so this is false.
else if (is_gimple_assign (stmt) && !gimple_clobber_p (stmt))
{
tree lhs = gimple_assign_lhs (stmt);
This should be true IIUC, so we'll go into its THEN block.
if (TREE_CODE (lhs) == SSA_NAME && POINTER_TYPE_P (TREE_TYPE
(lhs)))
Should be false.
else if (TREE_CODE (lhs) == SSA_NAME && INTEGRAL_TYPE_P
(TREE_TYPE (lhs)))
Should also be false.
else if (TREE_CODE (lhs) != SSA_NAME && !TREE_SIDE_EFFECTS (lhs))
Should be true since LHS is a MEM_REF.
{
tree type = TREE_TYPE (lhs);
if (TREE_CODE (type) == ARRAY_TYPE)
type = TREE_TYPE (type);
if (TREE_CODE (type) == INTEGER_TYPE
&& TYPE_MODE (type) == TYPE_MODE (char_type_node)
&& TYPE_PRECISION (type) == TYPE_PRECISION
(char_type_node))
{
if (! handle_char_store (gsi))
return false;
}
}
If TREE_TYPE (type) is an ARRAY_TYPE, we strip the ARRAY_TYPE. We then
verify that TYPE is a single character type. _But_ we stripped away the
ARRAY_TYPE. So ISTM that we allow either an array of chars or a single
char on the LHS.
So how does this avoid multiple character stores?!? We could have had
an ARRAY_REF on the LHS and we never check the number of elements in the
array. There's no check on the RHS either. SO I don't see how we
guarantee that we're dealing with a single character store.
What am I missing here?
Can you show me a test case where it breaks? If not, I don't know
what you want me to do. I'll just move on to something else.
THe thing to do is research what gimple accepts and what other passes
may do. Given this is a code generation bug, "just moving on" isn't
really a good option unless we're going to rip out that little bit of code.
As I was thinking about this last night, the pass we'd want to look at
would be store merging. Let's do that on the call today.
Actually it was trivial to create with store merging.
char x[20];
foo()
{
x[0] = 0x41;
x[1] = 0x42;
}
MEM <unsigned short> [(char *)&x] = 16961;
The LHS is unsigned short so handle_char_store would not be called
because of the check in the caller. You would need something like:
MEM <char[2]> [(char *)&x] = { 'a', 'b' };
or something like that where the type is an array of char. I have
no idea if something like it can happen or how. As it is, I'm
pretty sure it's not valid (but who knows).
So clearly we can get this in gimple. So we need a check of some kind,
either on the LHS or the RHS to ensure that we actually have a single
character store as opposed to something like the above.
As far as I can see that describes the check in the caller.
I never said the MEM_REF you show above can't come up in Gimple.
What I am saying is that I don't know how to get it to cause
a problem here, or even to make it into the function to begin
with. Store merging doesn't do it because it runs after strlen.
If it did run earlier as I said above, it wouldn't make it into
the function. I tried it to convince myself.
Without a test case showing how something can happen I don't know
what to do to prevent it. You're throwing out hypothetical
scenarios based on what's valid Gimple and expect me to write
code to handle them. I'm sorry but I don't work that way. I
need a test case to see a problem, understand it, and to verify
I have fixed it properly.
In this instance, I can't think of a problem here, either real
or hypothetical, so it's impossible for me to do something to
prevent it. Even if the multicharacter store did make it into
the function I don't even understand what it is you want me to
do -- we'd have to look at the value of the RHS, find a nul in
its representation if there is one, and either compute the new
string length from it or invalidate the existing length. Either
way it's not trivial and would need to be tested to make sure
it's correct. So we're back to a test case.
Martin
PS Here's a test case that can be made to fail but only if a)
store merging runs before strlen, and also b) the LHS check
above is disabled.
char b[4];
int f (const char *s)
{
__builtin_strcpy (b, "123");
int i = __builtin_strcmp (s, b);
b[0] = 'a';
b[1] = 0;
// MEM <unsigned short> [(char *)&b] = 97;
if (__builtin_strlen (b) != 1)
__builtin_abort ();
return i;
}
