On 08/02/2017 10:58 AM, Jeff Law wrote:
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);
Sure. I've added something close to the latest patch.
+@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.
No, you're right. It's me who's been confused, either thinking
of strncpy or -Wstringop-overflow. The difference between the
two warnings is just one byte in some cases and I got them mixed
up. Sorry about that and thanks for spotting this silly mistake!
I've updated the code to issue -Wstringop-overflow here and added
a better example to the manual.
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.
I expect the checking and the optimization to be done either
in the same function, or by calling a new function from the
one that implements the optimization, so I changed the names
as you suggest.
I've also done some more testing with Binutils, GDB, and Glibc
and found a couple of reasonable use cases that the warning
shouldn't trigger on so I added more code to handle it as well.
1) In the following, the strncpy call would normally trigger
-Wstringop-truncation because of the possible missing terminating
NUL, but because the immediately following statement inserts the
NUL the call is safe.
strncpy (d, s, sizeof d); // possible missing NUL
d[sizeof d - 1] = '\0'; // okay, NUL add here
2) Building Glibc made me realize that in my effort to detect
the (common) misuses of strncpy I neglected the original (and
only intended but now rare) use case of filling a buffer
without necessarily NUL-terminating it (as in struct dirent::
d_name). To allow it the patch adds a new attribute that can
be used to annotate char arrays and pointers that are intended
not to be NUL-terminated. This suppresses the truncation
warning. When the patch is approved I'll propose the (trivial)
Glibc changes. In the future, the attribute will also let GCC
warn when passing such objects to functions that expect a NUL-
terminated string argument (e.g., strlen or strcpy).
3) Finally, to add inlining context to diagnostics issued by
the middle end, I've added a new %G directive to complement
%K by accepting a gcall* argument.
To make the patch easier to review I've broken it up into
four parts:
1. Add %G.
2. Add attribute nostring.
3. Implement -Wstringop-truncation and enhance -Wstringop-
overflow (the meat of the patch).
4. Fix up GCC to compile with the new and enhanced warnings.
Martin
