Hi, I can see some strange behavior for the warning -W=stringop-truncation with -O2 and current master, depending on how I enclose my code in if/else blocks or not, despite that logically speaking, the end result is always the same. See this code: EOF #include <string.h> class A { public: A(); A(const char* iCString); A& operator=(const A& iA); private: void setCString(const char* iCString); // Uncommenting the attribute silences the warning // but actually we do have a "string", not a "nonstring". /* [[gnu::nonstring]] */ char _cstring[3 + 1]; }; A::A() { _cstring[0] = '\0'; } A::A(const char* iCString) { setCString(iCString); } A& A::operator=(const A& iA) { if (this != &iA) // Commenting this line silences the warning. { setCString(iA._cstring); } return *this; } void A::setCString(const char* iCString) { // This version produces warning. if (iCString) { strncpy(_cstring, iCString, 3); } _cstring[3] = '\0'; /* // This version doesn't produce any warning, but has the same logic as above. if (iCString) { strncpy(_cstring, iCString, 3); _cstring[3] = '\0'; } else { _cstring[3] = '\0'; } */ /* // This version also produces a warning, despite having the same logic as above, // just with an extra assigment to _cstring[3] when iCString is not null. if (iCString) { strncpy(_cstring, iCString, 3); _cstring[3] = '\0'; } _cstring[3] = '\0'; */ } When compiled with -Wall -Werror -Wextra -std=gnu++17 -O2 and current master. I get: error: ‘char* strncpy(char*, const char*, size_t)’ output may be truncated copying 3 bytes from a string of length 3 [-Werror=stringop-truncation] First what I find strange is the warning itself. gcc is clever enough to see that source is 3 bytes long, that we copy in a buffer that is 4 bytes long, so why would it complain about a truncation anyway ? Or maybe I misunderstood the wording of the warning itself ? Second, why are the different variant of A::setCString I commented in the code working differently wrt the warning, while logically speaking they do have the same behavior. If we need to write our if/else block in a given way to make gcc understand it is safe, this starts to be complex for developers. Final question: do you think it would somehow make sense to introduce a "string" attribute, that would be the contrary of the newly introduced "nonstring". "nonstring" means that maybe the string is not null terminate. "string" would mean that for sure the array is always null terminated, and thus doing things like strncpy(_cstring, anotherA._cstring, 4) (note that I used 4 and not 3 for the total size), would always work ? "nonstring" correctly silences the warning, but has the exact opposite semantic meaning than I have in this particular case. Cheers, Romain
btw and this warning [1] make sense ? [1] mongoose.c: In function 'mg_resolve_async_opt': mongoose.c:10791:3: error: 'strncpy' specified bound 1024 equals destination size [-Werror=stringop-truncation] strncpy(req->name, name, sizeof(req->name));
The warning is designed to look at the next statement just after a call to strncpy() and if it's an assignment to the destination it does not trigger. Unfortunately, due to a limitation/missing feature in the optimizer (bug 84470), in the test case in comment #0, the 'if (iCString)' test prevents the warning from seeing that the next statement is an assignment. As you already noted, making the assignment to _cstring[3] in A::setCString() also conditional on iCString being nonnull avoids the warning. This will become unnecessary once the missing optimization mentioned in bug 84470 is implemented. I can confirm that the warning is a false positive in this case. Let me see if there's a way to enhance the warning to handle it until the optimization is implemented. (In reply to Sérgio Basto from comment #1) > btw and this warning [1] make sense ? > > [1] > mongoose.c: In function 'mg_resolve_async_opt': > mongoose.c:10791:3: error: 'strncpy' specified bound 1024 equals destination > size [-Werror=stringop-truncation] > strncpy(req->name, name, sizeof(req->name)); It's a common mistake to assume that strncpy() nul-terminates the copy even when it's called with the size of the destination as the last argument and a source at least as long. The warning is meant to detect cases when this assumption may be unsafe. It tries to avoid triggering for provably safe code (by looking for the subsequent assignment, or by tracking string lengths when it can) but the logic isn't perfect so it's best to call the function with a byte count that's less than the destination size and follow the call immediately by an assignment of the nul to the destination.
Ok there maybe a problem with the optimizer as you describe it in bug 84470. However why does variant 3 from comment #0 yields a warning too ? if (iCString) { strncpy(_cstring, iCString, 3); _cstring[3] = '\0'; } _cstring[3] = '\0'; It is because the optimizer finds out that "_cstring[3] = '\0';" is both in the "if branch" and also after, thus eliminates the one from the "if branch", then the strncpy silencing warning mechanism doesn't work properly anymore ? I don't know very well what is the internal gcc IR, but do you think it would be possible to go beyond branches and basic block separation to find out that the next statement in if (iCString) { strncpy(_cstring, iCString, 3); } _cstring[3] = '\0'; really is "_cstring[3] = '\0';" (ie crossing basic block boundaries) ?
Yes, the conditional assignment is eliminated in favor of the one in the next basic block. Considering the first statement in the next basic block (rather than just the current one) is also the solution I'm looking into.
Patch: https://gcc.gnu.org/ml/gcc-patches/2018-02/msg01141.html
Hi, Tried to apply this patch on top of current trunk. During my build process, I bootstrap a complete Linux/binutils/glibc/gcc toolchain following the Linux From Scratch guidelines. Without the patch, the bootstrap works fine. With the patch, the bootstrap fails when using a newly built gcc 8, I try to build the Linux headers. Here are the logs I have: + cd /workdir/src/linux-2.6.32.23 + make mrproper + make INSTALL_HDR_PATH=dest headers_install CHK include/linux/version.h UPD include/linux/version.h HOSTCC scripts/basic/fixdep HOSTCC scripts/basic/docproc HOSTCC scripts/basic/hash HOSTCC scripts/unifdef In file included from /home/jenkins/workspace/OTF_Toolchain_release_4.0-YQJ6EBM33XTPWHNNQSFSZFEGBOIWUEOX32S2OZLINN43UBSUZTJA/output/build/temporary-system/install/include/string.h:630, from scripts/unifdef.c:72: scripts/unifdef.c: In function 'Mpass': scripts/unifdef.c:377:28: internal compiler error: Segmentation fault static void Mpass (void) { strncpy(keyword, "if ", 4); Pelif(); } ^~~~~~~ scripts/unifdef.c:377:28: internal compiler error: Aborted gcc: internal compiler error: Aborted signal terminated program cc1 Please submit a full bug report, with preprocessed source if appropriate. See <https://gcc.gnu.org/bugs/> for instructions. scripts/Makefile.host:118: recipe for target 'scripts/unifdef' failed make[1]: *** [scripts/unifdef] Error 4 Makefile:1089: recipe for target '__headers' failed make: *** [__headers] Error 2 I expect that you would be able to reproduce the same error by downloading the Linux headers (https://www.kernel.org/pub/linux/kernel/v2.6/linux-2.6.32.23.tar.gz) and then: make mrproper make INSTALL_HDR_PATH=dest headers_install This file is actually pretty simple. See https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git/tree/scripts/unifdef.c?h=v2.6.32.23#n377 ... static void Pelif (void) { print(); ignoreoff(); state(IS_PASS_MIDDLE); } ... static void Mpass (void) { strncpy(keyword, "if ", 4); Pelif(); } ... Maybe the fact that the instruction that follows strncpy is a function call makes gcc seg faults ? Cheers, Romain
Thanks for the early heads up! Can you please attach the translation unit for the kernel file that GCC faults on?
I am currently testing a little variant of your patch (check that "nextbb" if not NULL before trying to use it): Index: gcc/tree-ssa-strlen.c =================================================================== --- gcc/tree-ssa-strlen.c (revision 257796) +++ gcc/tree-ssa-strlen.c (working copy) @@ -1851,8 +1851,21 @@ maybe_diag_stxncpy_trunc (gimple_stmt_iterator gsi avoid the truncation warning. */ gsi_next_nondebug (&gsi); gimple *next_stmt = gsi_stmt (gsi); + if (!next_stmt) + { + /* When there is no statement in the same basic block check + the immediate successor block. */ + basic_block bb = gimple_bb (stmt); + basic_block nextbb + = EDGE_COUNT (bb->succs) ? EDGE_SUCC (bb, 0)->dest : NULL; + if (nextbb) + { + gimple_stmt_iterator it = gsi_start_bb (nextbb); + next_stmt = gsi_stmt (it); + } + } - if (!gsi_end_p (gsi) && is_gimple_assign (next_stmt)) + if (next_stmt && is_gimple_assign (next_stmt)) { tree lhs = gimple_assign_lhs (next_stmt); tree_code code = TREE_CODE (lhs); If it doesn't work, I will provide you with the translation unit.
Ok I was able to strip down the ICE to this very simple reproducer: <<EOF #include <string.h> static char keyword[4]; static void f (void) { strncpy(keyword, "if ", 4); } EOF
Thanks, I can reproduce it with that test case. Checking for the basic block being null fixes the SEGV for me. Let me retest this and post an update for review. Index: gcc/tree-ssa-strlen.c =================================================================== --- gcc/tree-ssa-strlen.c (revision 257963) +++ gcc/tree-ssa-strlen.c (working copy) @@ -1856,8 +1856,20 @@ maybe_diag_stxncpy_trunc (gimple_stmt_iterator gsi avoid the truncation warning. */ gsi_next_nondebug (&gsi); gimple *next_stmt = gsi_stmt (gsi); + if (!next_stmt) + { + /* When there is no statement in the same basic block check + the immediate successor block. */ + if (basic_block bb = gimple_bb (stmt)) + { + basic_block nextbb + = EDGE_COUNT (bb->succs) ? EDGE_SUCC (bb, 0)->dest : NULL; + gimple_stmt_iterator it = gsi_start_bb (nextbb); + next_stmt = gsi_stmt (it); + } + } - if (!gsi_end_p (gsi) && is_gimple_assign (next_stmt)) + if (next_stmt && is_gimple_assign (next_stmt)) { tree lhs = gimple_assign_lhs (next_stmt); tree_code code = TREE_CODE (lhs);
Hi, Indeed this version of the patch doesn't have any segv. However it seems that it doesn't fix anymore the initial bug report. Does it actually passes the new tests you introduced in your patch ? Unless I am mistaken (I am pretty sure I have applied the patch), the following code extracted from your tests still emit a warning with -O2 <<EOF #define strncpy __builtin_strncpy struct A { char a[4]; }; void succ (struct A *p, const struct A *q) { /* Verify that the assignment suppresses the warning for the conditional strcnpy call. The conditional should be folded to true since the address of an array can never be null (see bug 84470). */ if (q->a) strncpy (p->a, q->a, sizeof p->a - 1); /* { dg-bogus "\\\[-Wstringop-truncation" } */ p->a[sizeof p->a - 1] = 0; } EOF
Yes, all the relevant tests pass with the patch. There is no warning for either the test case in comment #0 or the one in comment #11. The change from v1 of the patch is just the addition of test for null to avoid the ICE.
Hi, It looks like that the code in #comment 11 works when you build just with -O2, but not when you add debug symbols: -O2 -g. Do we have a way to ignore debug statements when looking for the next statement in the next basic block ? Cheers, Romain
(In reply to Romain Geissler from comment #13) Ah, right. It's not skipping over debug statements. That's easier to fix than pr84561. This should do it: Index: gcc/tree-ssa-strlen.c =================================================================== --- gcc/tree-ssa-strlen.c (revision 257963) +++ gcc/tree-ssa-strlen.c (working copy) @@ -1856,8 +1856,21 @@ maybe_diag_stxncpy_trunc (gimple_stmt_iterator gsi avoid the truncation warning. */ gsi_next_nondebug (&gsi); gimple *next_stmt = gsi_stmt (gsi); + if (!next_stmt) + { + /* When there is no statement in the same basic block check + the immediate successor block. */ + if (basic_block bb = gimple_bb (stmt)) + { + basic_block nextbb + = EDGE_COUNT (bb->succs) ? EDGE_SUCC (bb, 0)->dest : NULL; + gimple_stmt_iterator it = gsi_start_bb (nextbb); + gsi_next_nondebug (&it); + next_stmt = gsi_stmt (it); + } + } - if (!gsi_end_p (gsi) && is_gimple_assign (next_stmt)) + if (next_stmt && is_gimple_assign (next_stmt)) { tree lhs = gimple_assign_lhs (next_stmt); tree_code code = TREE_CODE (lhs);
Hi, This latest patch seems to fix the occurences I have in my own code. Thanks ;) Cheers, Romain
I still say simply walk to the next VDEF...
Author: msebor Date: Wed Mar 7 19:30:31 2018 New Revision: 258339 URL: https://gcc.gnu.org/viewcvs?rev=258339&root=gcc&view=rev Log: PR tree-optimization/84468 - bogus -Wstringop-truncation despite assignment after conditional strncpy gcc/ChangeLog: PR tree-optimization/84468 * tree-ssa-strlen.c (maybe_diag_stxncpy_trunc): Consider successor basic block when looking for nul assignment. gcc/testsuite/ChangeLog: PR tree-optimization/84468 * g++.dg/warn/Wstringop-truncation-2.C: New test. * gcc.dg/Wstringop-truncation.c: New test. * gcc.dg/Wstringop-truncation-2.c: New test. Added: trunk/gcc/testsuite/g++.dg/warn/Wstringop-truncation-2.C trunk/gcc/testsuite/gcc.dg/Wstringop-truncation-2.c trunk/gcc/testsuite/gcc.dg/Wstringop-truncation.c Modified: trunk/gcc/ChangeLog trunk/gcc/testsuite/ChangeLog trunk/gcc/testsuite/g++.dg/warn/Wstringop-truncation-1.C trunk/gcc/tree-ssa-strlen.c
Fixed with r258339 on trunk. For GCC 9 I'll look into something more sophisticated.