On 07/31/2017 01:42 PM, Martin Sebor wrote:
>> So I *think* TYPE_SIZE_UNIT isn't necessarily guaranteed to be a
>> INTEGER_CST, it could be a non-constant expression for the size. Are
>> the callers of compute_objsize prepared to handle that? Just to be
>> clear, I'd prefer to return TYPE_SIZE_UNIT even if it's not an
>> INTEGER_CST, I'm just worried about the callers ability to handle that
>> correctly.
>
> They should be prepared for it. If not, it's a bug. I've added
> a few more test cases though I'm not sure the case you're concerned
> about actually arises (VLA sizes are represented as gimple calls to
> __builtin_alloca_with_align so the code doesn't get this far).
It may in the end be a non-issue because VLAs are still represented as
alloca calls at this point. BUt it looks like the result is typically
passed down into check_sizes which assumes the result is an INTEGER_CST
based on a quick scan.
So just to be future safe perhaps this:
if (TREE_CODE (type) == ARRAY_TYPE
&& TREE_CODE (TYPE_SIZE_UNIT (type)) == INTEGER_CST)
return TYPE_SIZE_UNIT (type);
>>> +
>>> +@smallexample
>>> +void append (char *d)
>>> +@{
>>> + strncat (d, ".txt", 4);
>>> +@}
>>> +@end smallexample
>> Sorry. I don't follow this particular example. Where's the truncation
>> when strlen (SRC) == N for strncat? In that case aren't we going to
>> append SRC without the nul terminator, then nul terminate the result?
>> Isn't that the same as appending SRC with its nul terminator? Am I
>> missing something here?
>
> You're right that there is no truncation and the effect is
> the same but only in the unlikely case when the destination
> is empty. Otherwise the result is truncated.
Maybe this is where I'm confused. How does the destination play into
truncation issues? I've always been under the impression that the
destination has to be large enough to hold the result, but that it
doesn't affect truncation of the source.
>
>>
>>> @@ -2512,6 +2651,19 @@ strlen_optimize_stmt (gimple_stmt_iterator *gsi)
>>> case BUILT_IN_STPCPY_CHK_CHKP:
>>> handle_builtin_strcpy (DECL_FUNCTION_CODE (callee), gsi);
>>> break;
>>> +
>>> + case BUILT_IN_STRNCAT:
>>> + case BUILT_IN_STRNCAT_CHK:
>>> + check_builtin_strncat (DECL_FUNCTION_CODE (callee), gsi);
>>> + break;
>>> +
>>> + case BUILT_IN_STPNCPY:
>>> + case BUILT_IN_STPNCPY_CHK:
>>> + case BUILT_IN_STRNCPY:
>>> + case BUILT_IN_STRNCPY_CHK:
>>> + check_builtin_stxncpy (DECL_FUNCTION_CODE (callee), gsi);
>>> + break;
>>> +
>> So we've got calls to check the arguments, but not optimize here. But
>> the containing function is "strlen_optimize_stmt".
>>
>> Would it make sense to first call strlen_optimize_stmt to handle the
>> optimization cases, then to call a new separate function to handle
>> warnings for the strn* family?
>
> tree-ssa-strlen doesn't handle strncat or strncpy (for the latter
> I'm tracking the enhancement in bug 81433). When the handling is
> added I expect check_builtin_{strncat,stxncpy} will be renamed to
> handle_builtin_{strncat,stxncpy} to match the existing functions,
> and the optimization done there.
>
> Or did you have something else in mind and I missed it?
So do you envision doing both optimization and checking together? If
so, then I think we'd want to rename strlen_optimize_stmt. If
optimization and checking are expected to remain separate we'd want a
new function to handle checking of strncat stpncpy, strncpy and the
caller would look something like this:
for (gimple_stmt_iterator gsi = gsi_start_bb (bb); !gsi_end_p (gsi); )
{
check_strstar_arguments (gsi); // Or whatever we named it
if (strlen_optimize_stmt (&gsi))
gsi_next (&gsi);
}
jeff
