This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |
Other format: | [Raw text] |
On 08/18/18 06:01, Jeff Law wrote: > On 08/17/2018 03:14 AM, Bernd Edlinger wrote: >> Hi! >> >> >> After the other patch has been applied, I re-based this patch accordingly. >> >> Except the mechanical changes, there are a few notable differences to the >> previous version: >> >> In string_constant, I added a similar check for the STRING_CSTs >> because when callers don't use mem_size, they assume to be >> able to read "TREE_STRING_LENGTH (array)" bytes, but that is >> not always the case, for languages that don't always use >> zero-terminated strings, for instance hollerith strings in fortran. >> >> --- gcc/expr.c 2018-08-17 05:32:57.332211963 +0200 >> +++ gcc/expr.c 2018-08-16 23:08:23.544940795 +0200 >> @@ -11372,6 +11372,9 @@ string_constant (tree arg, tree *ptr_off >> *ptr_offset = fold_convert (sizetype, offset); >> if (mem_size) >> *mem_size = TYPE_SIZE_UNIT (TREE_TYPE (array)); >> + else if (compare_tree_int (TYPE_SIZE_UNIT (TREE_TYPE (array)), >> + TREE_STRING_LENGTH (array)) < 0) >> + return NULL_TREE; >> return array; >> } >> > Yes. I purposefully didn't include this change in its entirety as it > was dependent upon your earlier patch. Instead I twiddled your patch > so that it applied on the current trunk and didn't regress anything > while keeping the core concept you were introducing. > That's a more or less theoretical possibility that I just considered useful for symmetry, and maybe Ada/Go strings. So it should be completely impossible to have this change anything in C (hopefully!!!). If it would happen, that would probably be a bug that needs to be fixed. > > I'm also confident that change breaks one or more tests in the > testsuite. I'm not sure why you didn't see the regression. But I > certainly saw it with the variant shown above. > I tested the patch against an older version of the trunk, but today, I repeated the regression test against r263644. This time I attach the test results for your reference: gcc-trunk-263644-test.txt: unchanged r263644 gcc-trunk-263644-0-test.txt: r263644 + this patch gcc-trunk-263644-1-test.txt: r263644 + this patch without string_constant As you can see, these regressions are already in r263644: FAIL: c-c++-common/torture/builtin-arith-overflow-17.c -O0 execution test FAIL: c-c++-common/torture/builtin-arith-overflow-17.c -O2 execution test FAIL: c-c++-common/torture/builtin-arith-overflow-17.c -O2 -flto -fno-use-linker-plugin -flto-partition=none execution test FAIL: c-c++-common/torture/builtin-arith-overflow-p-17.c -O0 execution test FAIL: c-c++-common/torture/builtin-arith-overflow-p-17.c -O2 execution test FAIL: c-c++-common/torture/builtin-arith-overflow-p-17.c -O2 -flto -fno-use-linker-plugin -flto-partition=none execution test FAIL: gcc.target/i386/20040112-1.c scan-assembler testb I have never seen those before, so they are brand new. FAIL: gnat.dg/dinst.adb scan-assembler loc [0-9] 5 [0-9]( is_stmt [0-9])? discriminator 1\\n FAIL: gnat.dg/dinst.adb scan-assembler loc [0-9] 5 [0-9]( is_stmt [0-9])? discriminator 3\\n FAIL: gnat.dg/dinst.adb scan-assembler loc [0-9] 5 [0-9]( is_stmt [0-9])? discriminator 4\\n these were already there on monday, but not last week. The test result with this patch as is does not change anything. But what's surprising, is this: --- gcc-trunk-r263644-test.txt 2018-08-18 13:49:27.214851609 +0200 +++ gcc-trunk-r263644-1-test.txt 2018-08-18 13:49:27.070851601 +0200 @@ -18,6 +18,12 @@ Running target unix +FAIL: gcc.c-torture/execute/pr86714.c -O1 execution test +FAIL: gcc.c-torture/execute/pr86714.c -O2 execution test +FAIL: gcc.c-torture/execute/pr86714.c -O2 -flto -fno-use-linker-plugin -flto-partition=none execution test +FAIL: gcc.c-torture/execute/pr86714.c -O2 -flto -fuse-linker-plugin -fno-fat-lto-objects execution test +FAIL: gcc.c-torture/execute/pr86714.c -O3 -g execution test +FAIL: gcc.c-torture/execute/pr86714.c -Os execution test FAIL: c-c++-common/torture/builtin-arith-overflow-17.c -O0 execution test FAIL: c-c++-common/torture/builtin-arith-overflow-17.c -O2 execution test FAIL: c-c++-common/torture/builtin-arith-overflow-17.c -O2 -flto -fno-use-linker-plugin -flto-partiti There is another regression in gcc-trunk-r263644-1-test.txt only which is likely only noise that happens randomly and can be ignored: === libgo Summary for unix === # of expected passes 163 Running target unix/-m32 FAIL: sync The result is very interesting, and is probably what you were looking for: The test was run with both test cases, but only pr86714 failed, so the change in c_getstr Fixes pr86711, while the change in string_constant fixes pr86714 (or both). The patch could actually be split between pr86711 and pr86714. The c_getstr / p86711 should be applied before string_constant / pr86714. Thanks Bernd. > Anyway, the basic idea was to carve out the basic concept of your patch > to allow callers to specify how to count without regressing the trunk. > > That allows us to carve out an issue, resolve it and move on. The > interdependent and conflicting patches are a nightmare to sort out. > >> >> The range check in c_getstr was refined as well: >> >> This I added, because vla arrays can be initialized with string constants, >> especially since the 71625 patch was installed: >> In this case we end up with mem_size that fails to be constant. >> >> >> @@ -14606,25 +14603,17 @@ c_getstr (tree src, unsigned HOST_WIDE_I >> offset = tree_to_uhwi (offset_node); >> } >> >> + if (!tree_fits_uhwi_p (mem_size)) >> + return NULL; >> + >> /* STRING_LENGTH is the size of the string literal, including any >> embedded NULs. STRING_SIZE is the size of the array the string >> literal is stored in. */ >> >> Also the rest of the string length checks are refined, to return >> actually zero-terminated single byte strings when strlen is not given, >> and return something not necessarily zero-terminated which is >> suitable for memxxx-functions otherwise. > >> >> >> Bootstrapped and reg-tested on x86_64-pc-linux-gnu. >> Is it OK for trunk? > Not yet. There's a lot to go over here and I haven't finished reviewing > all the discussions around 86711/86714. > > > Jeff >
Attachment:
gcc-trunk-r263644-0-test.txt
Description: gcc-trunk-r263644-0-test.txt
Attachment:
gcc-trunk-r263644-1-test.txt
Description: gcc-trunk-r263644-1-test.txt
Attachment:
gcc-trunk-r263644-test.txt
Description: gcc-trunk-r263644-test.txt
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |