| Message ID | 7577c627-25ea-1468-e93b-94b29b3618f8@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | avoid -Wstringop-truncation in Darwin bootstrap | expand |
On Nov 10, 2017, at 11:55 AM, Martin Sebor <msebor@gmail.com> wrote: > A few not incorrect but not strictly intended (according to > the function's original purpose) uses of strncpy trigger the > new -Wstringop-truncation warning because they temporarily > leave the copied string without a terminating nul. If people want to tighten the language used in gcc itself, I'm fine with this. My take, I'd leave the review of that aspect of it to those that set the language gcc uses.
On 11/10/2017 01:13 PM, Mike Stump wrote: > On Nov 10, 2017, at 11:55 AM, Martin Sebor <msebor@gmail.com> wrote: >> A few not incorrect but not strictly intended (according to >> the function's original purpose) uses of strncpy trigger the >> new -Wstringop-truncation warning because they temporarily >> leave the copied string without a terminating nul. > > If people want to tighten the language used in gcc itself, I'm fine with this. My take, I'd leave the review of that aspect of it to those that set the language gcc uses. The warning is included in -Wall and apparently causes errors in a Darwin bootstrap. I already adjusted the handful of places that did this sort of thing as I came across them in an x86_64-linux bootstrap. I see no other uses of the function in other back ends so this should be the only one. Thanks Martin
On 11/10/2017 12:55 PM, Martin Sebor wrote: > A few not incorrect but not strictly intended (according to > the function's original purpose) uses of strncpy trigger the > new -Wstringop-truncation warning because they temporarily > leave the copied string without a terminating nul. > > The attached patch replaces these uses with memcpy to avoid > the warning and make it clear (to both the reader and GCC) > that the string being constructed is, in fact, not nul > terminated until the last call to strcpy. > > Tested by cross-compiling for x86_64-darwin. For reference, > with the attached patch applied the following is the list of > outstanding warnings in the build. > > Diagnostic Count Unique Files > -Wsign-compare 4 3 3 > -Wimplicit-fallthrough= 4 1 1 > > -Wimplicit-fallthrough Instances: > gengtype-lex.c:380 > > -Wsign-compare Instances: > arlex.c:1352 > gengtype-lex.c:1367 > syslex.c:1226 > > Martin > > gcc-darwin-stringop-trunc.diff > > > gcc/ChangeLog: > > PR c/81117 > * config/darwin-c.c (framework_construct_pathname): Replace strncpy > with memcpy. > (find_subframework_file): Same. So just to be 100% crystal clear. This patch is not meant to change the semantics of the code, it merely avoids code sequences that trigger the new warning. The code as-is is valid. It's fairly common to have this kind of fallout and this looks well within what we typically fix. One could easily argue for these patches independent of the warning since they make it clearer that the strings in question are temporarily not terminated. OK. jeff
On 11/10/2017 03:18 PM, Jeff Law wrote: > On 11/10/2017 12:55 PM, Martin Sebor wrote: >> A few not incorrect but not strictly intended (according to >> the function's original purpose) uses of strncpy trigger the >> new -Wstringop-truncation warning because they temporarily >> leave the copied string without a terminating nul. >> >> The attached patch replaces these uses with memcpy to avoid >> the warning and make it clear (to both the reader and GCC) >> that the string being constructed is, in fact, not nul >> terminated until the last call to strcpy. >> >> Tested by cross-compiling for x86_64-darwin. For reference, >> with the attached patch applied the following is the list of >> outstanding warnings in the build. >> >> Diagnostic Count Unique Files >> -Wsign-compare 4 3 3 >> -Wimplicit-fallthrough= 4 1 1 >> >> -Wimplicit-fallthrough Instances: >> gengtype-lex.c:380 >> >> -Wsign-compare Instances: >> arlex.c:1352 >> gengtype-lex.c:1367 >> syslex.c:1226 >> >> Martin >> >> gcc-darwin-stringop-trunc.diff >> >> >> gcc/ChangeLog: >> >> PR c/81117 >> * config/darwin-c.c (framework_construct_pathname): Replace strncpy >> with memcpy. >> (find_subframework_file): Same. > So just to be 100% crystal clear. This patch is not meant to change the > semantics of the code, it merely avoids code sequences that trigger the > new warning. The code as-is is valid. Yes, that is correct. > It's fairly common to have this kind of fallout and this looks well > within what we typically fix. One could easily argue for these patches > independent of the warning since they make it clearer that the strings > in question are temporarily not terminated. > > OK. Committed in r254641. Thanks Martin
On Nov 10, 2017, at 12:36 PM, Martin Sebor <msebor@gmail.com> wrote: > > The warning is included in -Wall Ah, I missed that little detail the first time around. -Wall is special in that we already just sanitize the source to pass it. I assumed it was a non-wall flag someone added or wanted to add to the bootstrap.
gcc/ChangeLog: PR c/81117 * config/darwin-c.c (framework_construct_pathname): Replace strncpy with memcpy. (find_subframework_file): Same. diff --git a/gcc/config/darwin-c.c b/gcc/config/darwin-c.c index 91f08a0..bfb35b9 100644 --- a/gcc/config/darwin-c.c +++ b/gcc/config/darwin-c.c @@ -284,13 +284,13 @@ framework_construct_pathname (const char *fname, cpp_dir *dir) frname = XNEWVEC (char, strlen (fname) + dir->len + 2 + strlen(".framework/") + strlen("PrivateHeaders")); - strncpy (&frname[0], dir->name, dir->len); + memcpy (&frname[0], dir->name, dir->len); frname_len = dir->len; if (frname_len && frname[frname_len-1] != '/') frname[frname_len++] = '/'; - strncpy (&frname[frname_len], fname, fname_len); + memcpy (&frname[frname_len], fname, fname_len); frname_len += fname_len; - strncpy (&frname[frname_len], ".framework/", strlen (".framework/")); + memcpy (&frname[frname_len], ".framework/", strlen (".framework/")); frname_len += strlen (".framework/"); if (fast_dir == 0) @@ -316,7 +316,7 @@ framework_construct_pathname (const char *fname, cpp_dir *dir) /* Append framework_header_dirs and header file name */ for (i = 0; framework_header_dirs[i].dirName; i++) { - strncpy (&frname[frname_len], + memcpy (&frname[frname_len], framework_header_dirs[i].dirName, framework_header_dirs[i].dirNameLen); strcpy (&frname[frname_len + framework_header_dirs[i].dirNameLen], @@ -378,23 +378,23 @@ find_subframework_file (const char *fname, const char *pname) sfrname_len = bufptr - pname; - strncpy (&sfrname[0], pname, sfrname_len); + memcpy (&sfrname[0], pname, sfrname_len); - strncpy (&sfrname[sfrname_len], "Frameworks/", strlen ("Frameworks/")); + memcpy (&sfrname[sfrname_len], "Frameworks/", strlen ("Frameworks/")); sfrname_len += strlen("Frameworks/"); - strncpy (&sfrname[sfrname_len], fname, fname_len); + memcpy (&sfrname[sfrname_len], fname, fname_len); sfrname_len += fname_len; - strncpy (&sfrname[sfrname_len], ".framework/", strlen (".framework/")); + memcpy (&sfrname[sfrname_len], ".framework/", strlen (".framework/")); sfrname_len += strlen (".framework/"); /* Append framework_header_dirs and header file name */ for (i = 0; framework_header_dirs[i].dirName; i++) { - strncpy (&sfrname[sfrname_len], - framework_header_dirs[i].dirName, - framework_header_dirs[i].dirNameLen); + memcpy (&sfrname[sfrname_len], + framework_header_dirs[i].dirName, + framework_header_dirs[i].dirNameLen); strcpy (&sfrname[sfrname_len + framework_header_dirs[i].dirNameLen], &fname[fname_len]);