Re: [PATCH] fix a couple of bugs in const string folding (PR 86532)
On 07/21/18 00:15, Martin Sebor wrote:
> On 07/20/2018 08:51 AM, Bernd Edlinger wrote:
>> Martin,
>>
>> when I look at tree-ssa-forwprop.c:
>>
>> str1 = string_constant (src1, &off1);
>> if (str1 == NULL_TREE)
>> break;
>> if (!tree_fits_uhwi_p (off1)
>> || compare_tree_int (off1, TREE_STRING_LENGTH (str1) - 1)
>> > 0
>> || compare_tree_int (len1, TREE_STRING_LENGTH (str1)
>> - tree_to_uhwi (off1)) > 0
>> || TREE_CODE (TREE_TYPE (str1)) != ARRAY_TYPE
>> || TYPE_MODE (TREE_TYPE (TREE_TYPE (str1)))
>> != TYPE_MODE (char_type_node))
>> break;
>>
>> I don't think it is a good idea to let string_constant return
>> strings which have not necessarily valid content after the first
>> NUL byte. It looks like the content has to be valid up to
>> TREE_STRING_LENGTH.
>
> I'm not sure I understand when this could happen. From the patch
> below it sounds like you are concerned about cases like:
>
> const char a[4] = "abc\000\000";
>
> where the STRING_CST is bigger than the array. But the string
> constant has valid content here up to TREE_STRING_LENGTH. Am
> I missing something? (A test case would be helpful.)
>
No, I mean something like:
$ cat y.c
const char a[2][3] = { "1234", "xyz" };
char b[6];
int main ()
{
__builtin_memcpy(b, a, 4);
__builtin_memset(b + 4, 'a', 2);
__builtin_printf("%.6s\n", b);
}
$ gcc y.c
y.c:1:24: warning: initializer-string for array of chars is too long
const char a[2][3] = { "1234", "xyz" };
^~
y.c:1:24: note: (near initialization for 'a[0]')
$ ./a.out
1234aa
but expected would be "123xaa".
> In any case, unless the concern is specifically related to
> this bug fix let's discuss it separately so I can fix this
> bug. I'm not opposed to making further changes here (in
> fact, I have one in the queue and two others that I raised
> in Bugzilla in response to our discussion so far), I just
> want to avoid mission creep.
>
I think when you touch a function you should fix it completely
or restrict it to the subset that is known to be safe, and not
leave lots of already known bugs behind.
Fixing this bug by bug will soon exceed my ability to reverse
engineer test cases for the remaining bugs.
Bernd.
> Martin
>
>>
>> So may I suggest to do something like the following
>> (diff to your last patch):
>>
>> --- expr.c.jj 2018-07-20 10:51:51.983605588 +0200
>> +++ expr.c 2018-07-20 15:07:29.769423479 +0200
>> @@ -11277,6 +11277,7 @@ tree
>> string_constant (tree arg, tree *ptr_offset)
>> {
>> tree array;
>> + tree array_size;
>> STRIP_NOPS (arg);
>>
>> /* Non-constant index into the character array in an ARRAY_REF
>> @@ -11295,9 +11296,11 @@ string_constant (tree arg, tree *ptr_off
>>
>> arg = TREE_OPERAND (arg, 0);
>> tree ref = arg;
>> + tree reftype = TREE_TYPE (ref);
>> if (TREE_CODE (arg) == ARRAY_REF)
>> {
>> tree idx = TREE_OPERAND (arg, 1);
>> + reftype = TREE_TYPE (TREE_OPERAND (arg, 0));
>> if (TREE_CODE (idx) != INTEGER_CST
>> && TREE_CODE (argtype) == POINTER_TYPE)
>> {
>> @@ -11313,6 +11316,11 @@ string_constant (tree arg, tree *ptr_off
>> return NULL_TREE;
>> }
>> }
>> + if (TREE_CODE (reftype) != ARRAY_TYPE)
>> + return NULL_TREE;
>> + while (TREE_CODE (TREE_TYPE (reftype)) == ARRAY_TYPE)
>> + reftype = TREE_TYPE (reftype);
>> + array_size = TYPE_SIZE_UNIT (reftype);
>> array = get_addr_base_and_unit_offset (ref, &base_off);
>> if (!array
>> || (TREE_CODE (array) != VAR_DECL
>> @@ -11345,7 +11353,10 @@ string_constant (tree arg, tree *ptr_off
>> return NULL_TREE;
>> }
>> else if (DECL_P (arg))
>> - array = arg;
>> + {
>> + array = arg;
>> + array_size = DECL_SIZE_UNIT (array);
>> + }
>> else
>> return NULL_TREE;
>>
>> @@ -11410,24 +11421,18 @@ string_constant (tree arg, tree *ptr_off
>> if (!init || TREE_CODE (init) != STRING_CST)
>> return NULL_TREE;
>>
>> - tree array_size = DECL_SIZE_UNIT (array);
>> if (!array_size || TREE_CODE (array_size) != INTEGER_CST)
>> return NULL_TREE;
>>
>> /* Avoid returning a string that doesn't fit in the array
>> - it is stored in, like
>> + it is stored in, like:
>> const char a[4] = "abcde";
>> - but do handle those that fit even if they have excess
>> - initializers, such as in
>> - const char a[4] = "abc\000\000";
>> - The excess elements contribute to TREE_STRING_LENGTH()
>> - but not to strlen(). */
>> - unsigned HOST_WIDE_INT charsize
>> - = tree_to_uhwi (TYPE_SIZE_UNIT (TREE_TYPE (TREE_TYPE (init;
>> + ... or:
>> + const char a[4] = "abc\000";
>> + ... because some cal
Re: [wwwdocs] Mention LTO link-time issue fixed in gcc 8.2
On Sat, 21 Jul 2018, Gerald Pfeifer wrote: > Thanks, Jan, this looks fine to me. Oops, I missed closing an ; just followed up on your commit to fix that. Gerald Index: changes.html === RCS file: /cvs/gcc/wwwdocs/htdocs/gcc-8/changes.html,v retrieving revision 1.90 diff -u -r1.90 changes.html --- changes.html21 Jul 2018 19:55:10 - 1.90 +++ changes.html22 Jul 2018 08:25:46 - @@ -1321,7 +1321,7 @@ complete (that is, it is possible that some PRs that have been fixed are not listed here). -General Improvements +General Improvements Fixed LTO link-time performance problems caused by an overflow in the partitioning algorithm while building large binaries.
Re: [PATCH 1/4] Clean up of new format of -falign-FOO.
On Wed, 18 Jul 2018, Martin Sebor wrote: > I'm seeing lots of warnings for this file: > > /ssd/src/gcc/svn/gcc/align.h:53:32: warning: extended initializer lists only > available with -std=c++11 or -std=gnu++11 With clang version 3.4 (system compiler on FreeBSD 10.x) this is even a hard error and GCC failed to build. So thanks for fixing this, Martin! Gerald
Re: [PATCH 8/8] Enhance documentation of gcov.
On Fri, 28 Apr 2017, Martin Sebor wrote:
>> 2017-04-27 Martin Liska
>>
>> * doc/gcov.texi: Enhance documentation of gcov.
> Since I started picking on this change set I might as well keep
> at it ;) Just a tiny nit: the sentence is missing a preposition:
>
> depending on whether a basic block is reachable.
This adds a bit more. Applied.
(This section still needs a bit more love, ideally by a native
speaker.)
Gerald
2018-07-22 Gerald Pfeifer
* doc/gcov.texi (Invoking Gcov): Editorial changes.
Index: doc/gcov.texi
===
--- doc/gcov.texi (revision 262161)
+++ doc/gcov.texi (working copy)
@@ -378,12 +378,12 @@
containing no code. Unexecuted lines are marked @samp{#} or
@samp{=}, depending on whether they are reachable by
non-exceptional paths or only exceptional paths such as C++ exception
-handlers, respectively. Given @samp{-a} option, unexecuted blocks are
+handlers, respectively. Given the @samp{-a} option, unexecuted blocks are
marked @samp{$} or @samp{%}, depending on whether a basic block
is reachable via non-exceptional or exceptional paths.
Executed basic blocks having a statement with zero @var{execution_count}
-end with @samp{*} character and are colored with magenta color with @option{-k}
-option. The functionality is not supported in Ada.
+end with @samp{*} character and are colored with magenta color with
+the @option{-k} option. This functionality is not supported in Ada.
Note that GCC can completely remove the bodies of functions that are
not needed -- for instance if they are inlined everywhere. Such functions
Re: [PATCH] fix a couple of bugs in const string folding (PR 86532)
On 07/21/18 01:58, Martin Sebor wrote:
> On 07/20/2018 03:11 PM, Bernd Edlinger wrote:
>> I think I have found a valid test case where the latest patch
>> does generate invalid code:
>
> This is due to another bug in string_constant() that's latent
> on trunk but that's exposed by the patch. Trunk only "works"
> because of a bug/limitation in c_strlen() that the patch fixes
> or removes.
>
I am not sure if that falls under the definition of "latent" bug.
A latent bug would be something unexpected in a completely different
area of the compiler that is triggered by an improved optimization
or a fixed bug elsewhere.
> The underlying root cause is the handling of POINTER_PLUS
> expressions in string_constant(). The original code (before
> the handling of aggregates was added) just dealt with string
> constants. The new code does much more but doesn't get it
> quite right in these cases.
>
I think you should be much more careful with the expressions
you evaluate in string_constant. For instance when you look
at get_addr_base_and_unit_offset, you'll see it walks through
all kinds of type casts, VIEW_CONVERT_EXPR and MEM_REF with
nonzero displacement. When you see something like that do
not fold that on the assumption that the source code does not
have undefined behavior.
So I'd say you should add a check that there is no type
cast in the expression you parse. If that is the case,
your optimization will certainly be wrong.
As Richard already repeatedly said, and I can only emphasize
this once more:
The middle end does not follow the "C" standard too closely,
and it is also the C++, fortran, Ada and Go middle-end.
Even if the source code does not exhibit undefined behavior
the folding in the middle-end can introduce it.
Bernd.
> It shouldn't be too difficult to fix, but as valuable as
> the testing you are doing is, I'd really prefer to focus
> on one problem at a time and make incremental progress.
> It will make it easier to track down and fix regressions
> than lumping multiple fixes into a larger patch.
>
> Martin
>
>>
>> $ cat part.c
>> static const char a[3][8] = { "1234", "12345", "123456" };
>>
>> int main ()
>> {
>> volatile int i = 1;
>> int n = __builtin_strlen (*(&a[1]+i));
>>
>> if (n != 6)
>> __builtin_abort ();
>> }
>> $ gcc part.c -fdump-tree-original
>> $ ./a.out
>> Aborted (core dumped)
>> $ cat part.c.004t.original
>>
>> ;; Function main (null)
>> ;; enabled by -tree-original
>>
>>
>> {
>> volatile int i = 1;
>> int n = (ssizetype) (SAVE_EXPR <(long unsigned int) i * 8>) <= 5 ? (int)
>> (5 - (unsigned int) (SAVE_EXPR <(long unsigned int) i * 8>)) : 0;
>>
>> volatile int i = 1;
>> int n = (ssizetype) (SAVE_EXPR <(long unsigned int) i * 8>) <= 5 ?
>> (int) (5 - (unsigned int) (SAVE_EXPR <(long unsigned int) i * 8>)) : 0;
>> if (n != 6)
>> {
>> __builtin_abort ();
>> }
>> }
>> return 0;
>>
>>
>> string_constant is called with arg = (const char *) (&a[1] + (sizetype)
>> ((long unsigned int) i * 8))
>
New Swedish PO file for 'gcc' (version 8.1.0)
Hello, gentle maintainer. This is a message from the Translation Project robot. A revised PO file for textual domain 'gcc' has been submitted by the Swedish team of translators. The file is available at: http://translationproject.org/latest/gcc/sv.po (This file, 'gcc-8.1.0.sv.po', has just now been sent to you in a separate email.) All other PO files for your package are available in: http://translationproject.org/latest/gcc/ Please consider including all of these in your next release, whether official or a pretest. Whenever you have a new distribution with a new version number ready, containing a newer POT file, please send the URL of that distribution tarball to the address below. The tarball may be just a pretest or a snapshot, it does not even have to compile. It is just used by the translators when they need some extra translation context. The following HTML page has been updated: http://translationproject.org/domain/gcc.html If any question arises, please contact the translation coordinator. Thank you for all your work, The Translation Project robot, in the name of your translation coordinator.
Re: [PATCH 1/4] Clean up of new format of -falign-FOO.
On Sun, 22 Jul 2018, Gerald Pfeifer wrote: > With clang version 3.4 (system compiler on FreeBSD 10.x) this is > even a hard error and GCC failed to build. So thanks for fixing > this, Martin! Unfortunately it appears there was another bootstrap failure hidden behind that one: In file included from /scratch/tmp/gerald/GCC-HEAD/gcc/system.h:691, from /scratch/tmp/gerald/GCC-HEAD/gcc/tree-vect-slp.c:23: /scratch/tmp/gerald/GCC-HEAD/gcc/tree-vect-slp.c: In function ‘_slp_tree* vect_build_slp_tree_2(vec_info*, vec, unsigned int, poly_uint64*, vec<_slp_tree*>*, bool*, unsigned int*, unsigned int*, unsigned int)’: /scratch/tmp/gerald/GCC-HEAD/gcc/../include/libiberty.h:722:36: error: ‘alloca’ bound is unknown [-Werror=alloca-larger-than=] # define alloca(x) __builtin_alloca(x) ^~~ /scratch/tmp/gerald/GCC-HEAD/gcc/../include/libiberty.h:356:33: note: in expansion of macro ‘alloca’ #define XALLOCAVEC(T, N) ((T *) alloca (sizeof (T) * (N))) ^~ /scratch/tmp/gerald/GCC-HEAD/gcc/tree-vect-slp.c:1437:16: note: in expansion of macro ‘XALLOCAVEC’ bool *tem = XALLOCAVEC (bool, group_size); ^~ cc1plus: all warnings being treated as errors gmake[3]: *** [Makefile:1112: tree-vect-slp.o] Error 1 gmake[3]: Leaving directory '/scratch/tmp/gerald/OBJ-0722-0939/gcc' gmake[2]: *** [Makefile:4644: all-stage2-gcc] Error 2 This is on FreeBSD 10.4 which features clang 3.4.1 as system compiler; FreeBSD 11.2 with clang 6.0.0 does not trigger that. Gerald
Re: [PATCH 1/4] Clean up of new format of -falign-FOO.
Hi Gerald, > On Sun, 22 Jul 2018, Gerald Pfeifer wrote: >> With clang version 3.4 (system compiler on FreeBSD 10.x) this is >> even a hard error and GCC failed to build. So thanks for fixing >> this, Martin! > > Unfortunately it appears there was another bootstrap failure hidden > behind that one: > > > In file included from /scratch/tmp/gerald/GCC-HEAD/gcc/system.h:691, > from /scratch/tmp/gerald/GCC-HEAD/gcc/tree-vect-slp.c:23: > /scratch/tmp/gerald/GCC-HEAD/gcc/tree-vect-slp.c: In function > ‘_slp_tree* vect_build_slp_tree_2(vec_info*, vec, unsigned int, > poly_uint64*, vec<_slp_tree*>*, bool*, unsigned int*, unsigned int*, > unsigned int)’: > /scratch/tmp/gerald/GCC-HEAD/gcc/../include/libiberty.h:722:36: error: > ‘alloca’ bound is unknown [-Werror=alloca-larger-than=] > # define alloca(x) __builtin_alloca(x) > ^~~ > /scratch/tmp/gerald/GCC-HEAD/gcc/../include/libiberty.h:356:33: note: in > expansion of macro ‘alloca’ > #define XALLOCAVEC(T, N) ((T *) alloca (sizeof (T) * (N))) > ^~ > /scratch/tmp/gerald/GCC-HEAD/gcc/tree-vect-slp.c:1437:16: note: in expansion > of macro ‘XALLOCAVEC’ > bool *tem = XALLOCAVEC (bool, group_size); > ^~ > cc1plus: all warnings being treated as errors > gmake[3]: *** [Makefile:1112: tree-vect-slp.o] Error 1 > gmake[3]: Leaving directory '/scratch/tmp/gerald/OBJ-0722-0939/gcc' > gmake[2]: *** [Makefile:4644: all-stage2-gcc] Error 2 > > > This is on FreeBSD 10.4 which features clang 3.4.1 as system compiler; > FreeBSD 11.2 with clang 6.0.0 does not trigger that. known issue: PR bootstrap/86621. Rainer -- - Rainer Orth, Center for Biotechnology, Bielefeld University
[PATCH] Avoid out of scope access in hsa-dump.c
Hi,
this fixes an use of a buffer after the block scope
in hsa-dump.c: "buf" is assigned to "name" and used after
the scope ends in a fprintf.
I have not done any real checks, except boot-strapping with
all languages.
Is it OK for trunk?
Thanks
Bernd.
2018-07-22 Bernd Edlinger
hsa-dump.c (dump_hsa_symbol): Avoid out of scope access to buf.
Index: gcc/hsa-dump.c
===
--- gcc/hsa-dump.c (revision 262904)
+++ gcc/hsa-dump.c (working copy)
@@ -776,11 +776,11 @@ static void
dump_hsa_symbol (FILE *f, hsa_symbol *symbol)
{
const char *name;
+ char buf[64];
if (symbol->m_name)
name = symbol->m_name;
else
{
- char buf[64];
sprintf (buf, "__%s_%i", hsa_seg_name (symbol->m_segment),
symbol->m_name_number);
[C++ PATCH] Avoid code generation changes with -Wnonnull-compare vs. -Wno-nonnull-compare (PR c++/86569)
Hi!
A couple of -fcompare-debug=-Wsomething PRs have been filed recently, this
fixes one of them. Changing code generation based on TREE_NO_WARNING flag
which usually gets set only in the warning code, or on warn_* option is
undesirable. This patch prevents that kind of folding regardless of the
warning options, the GIMPLE optimizers can handle it fine (but it makes a
difference with -O0).
Bootstrapped/regtested on x86_64-linux (only, i686-linux bootstrap is
broken), ok for trunk?
2018-07-22 Jakub Jelinek
PR c++/86569
* cp-gimplify.c (cp_fold): Don't fold comparisons into other kind
of expressions other than INTEGER_CST regardless of TREE_NO_WARNING
or warn_nonnull_compare.
* g++.dg/warn/Wnonnull-compare-9.C: New test.
--- gcc/cp/cp-gimplify.c.jj 2018-07-16 09:43:03.237023048 +0200
+++ gcc/cp/cp-gimplify.c2018-07-19 11:44:24.881759294 +0200
@@ -2381,21 +2381,26 @@ cp_fold (tree x)
else
x = fold (x);
- if (TREE_NO_WARNING (org_x)
- && warn_nonnull_compare
- && COMPARISON_CLASS_P (org_x))
+ /* This is only needed for -Wnonnull-compare and only if
+TREE_NO_WARNING (org_x), but to avoid that option affecting code
+generation, we do it always. */
+ if (COMPARISON_CLASS_P (org_x))
{
if (x == error_mark_node || TREE_CODE (x) == INTEGER_CST)
;
else if (COMPARISON_CLASS_P (x))
- TREE_NO_WARNING (x) = 1;
+ {
+ if (TREE_NO_WARNING (org_x) && warn_nonnull_compare)
+ TREE_NO_WARNING (x) = 1;
+ }
/* Otherwise give up on optimizing these, let GIMPLE folders
optimize those later on. */
else if (op0 != TREE_OPERAND (org_x, 0)
|| op1 != TREE_OPERAND (org_x, 1))
{
x = build2_loc (loc, code, TREE_TYPE (org_x), op0, op1);
- TREE_NO_WARNING (x) = 1;
+ if (TREE_NO_WARNING (org_x) && warn_nonnull_compare)
+ TREE_NO_WARNING (x) = 1;
}
else
x = org_x;
--- gcc/testsuite/g++.dg/warn/Wnonnull-compare-9.C.jj 2018-07-19
11:57:41.909727597 +0200
+++ gcc/testsuite/g++.dg/warn/Wnonnull-compare-9.C 2018-07-19
11:57:14.627696497 +0200
@@ -0,0 +1,11 @@
+// PR c++/86569
+// { dg-do compile }
+// { dg-options "-fcompare-debug=-Wnonnull-compare" }
+
+bool b;
+
+int
+main ()
+{
+ return ((!b) != 0);
+}
Jakub
Re: [C++ PATCH] Avoid code generation changes with -Wnonnull-compare vs. -Wno-nonnull-compare (PR c++/86569)
On July 22, 2018 9:15:50 PM GMT+02:00, Jakub Jelinek wrote:
>Hi!
>
>A couple of -fcompare-debug=-Wsomething PRs have been filed recently,
>this
>fixes one of them. Changing code generation based on TREE_NO_WARNING
>flag
>which usually gets set only in the warning code, or on warn_* option is
>undesirable. This patch prevents that kind of folding regardless of
>the
>warning options, the GIMPLE optimizers can handle it fine (but it makes
>a
>difference with -O0).
>
>Bootstrapped/regtested on x86_64-linux (only, i686-linux bootstrap is
>broken), ok for trunk?
OK.
Richard.
>2018-07-22 Jakub Jelinek
>
> PR c++/86569
> * cp-gimplify.c (cp_fold): Don't fold comparisons into other kind
> of expressions other than INTEGER_CST regardless of TREE_NO_WARNING
> or warn_nonnull_compare.
>
> * g++.dg/warn/Wnonnull-compare-9.C: New test.
>
>--- gcc/cp/cp-gimplify.c.jj2018-07-16 09:43:03.237023048 +0200
>+++ gcc/cp/cp-gimplify.c 2018-07-19 11:44:24.881759294 +0200
>@@ -2381,21 +2381,26 @@ cp_fold (tree x)
> else
> x = fold (x);
>
>- if (TREE_NO_WARNING (org_x)
>-&& warn_nonnull_compare
>-&& COMPARISON_CLASS_P (org_x))
>+ /* This is only needed for -Wnonnull-compare and only if
>+ TREE_NO_WARNING (org_x), but to avoid that option affecting code
>+ generation, we do it always. */
>+ if (COMPARISON_CLASS_P (org_x))
> {
> if (x == error_mark_node || TREE_CODE (x) == INTEGER_CST)
> ;
> else if (COMPARISON_CLASS_P (x))
>- TREE_NO_WARNING (x) = 1;
>+ {
>+if (TREE_NO_WARNING (org_x) && warn_nonnull_compare)
>+ TREE_NO_WARNING (x) = 1;
>+ }
> /* Otherwise give up on optimizing these, let GIMPLE folders
>optimize those later on. */
> else if (op0 != TREE_OPERAND (org_x, 0)
> || op1 != TREE_OPERAND (org_x, 1))
> {
> x = build2_loc (loc, code, TREE_TYPE (org_x), op0, op1);
>-TREE_NO_WARNING (x) = 1;
>+if (TREE_NO_WARNING (org_x) && warn_nonnull_compare)
>+ TREE_NO_WARNING (x) = 1;
> }
> else
> x = org_x;
>--- gcc/testsuite/g++.dg/warn/Wnonnull-compare-9.C.jj 2018-07-19
>11:57:41.909727597 +0200
>+++ gcc/testsuite/g++.dg/warn/Wnonnull-compare-9.C 2018-07-19
>11:57:14.627696497 +0200
>@@ -0,0 +1,11 @@
>+// PR c++/86569
>+// { dg-do compile }
>+// { dg-options "-fcompare-debug=-Wnonnull-compare" }
>+
>+bool b;
>+
>+int
>+main ()
>+{
>+ return ((!b) != 0);
>+}
>
> Jakub
[C++ PATCH] Implement P0595R1 - so far as __builtin_is_constant_evaluated rather than std::is_constant_evaluated magic builtin
Hi! As part of the PR86590 discussions that the current libstdc++ __constant_string is extremely costly, because we don't fold the __builtin_constant_p in the loop early enough and because Richard doesn't want __builtin_early_constant_p, this patch is an attempt to implement P0595R1 as a builtin (which will be likely needed anyway, so that libstdc++ will be able to use it even without -std=c++2a). When evaluating (outermost) constant expressions with !ctx->quiet (i.e. when we require constant expressions) as well in the special case of reference initializers, or const non-volatile decl initializers, or TREE_STATIC decl initializers, the builtin is folded into true (for the ctx->quiet special cases if the constexpr evaluation doesn't turn a constant expression, we retry without the special flag), otherwise folding of it is deferred and it is flagged as non-constant expression, and folding during gimplification folds it into false. Not really sure about potential_constant_expression, for if it attempts to evaluate the condition with ctx->quiet true and the patch doesn't set the magic flag to fold the builtin to true in that case; it is unclear to me if in case the condition includes std::is_constant_evaluated () call we need to fold it also to true or not depending on what will we be evaluating later on. Bootstrapped/regtested on x86_64-linux. 2018-07-22 Jakub Jelinek P0595R1 - is_constant_evaluated * builtins.def (BUILT_IN_IS_CONSTANT_EVALUATED): New C++ builtin. * builtins.c (fold_builtin_0): Fold BUILT_IN_IS_CONSTANT_EVALUATED to 0. cp/ * cp-tree.h (maybe_constant_init): Add const_evaluated argument. * typeck2.c (store_init_value): Pass true as new argument to maybe_constant_init. * constexpr.c (struct constexpr_ctx): Add const_evaluated field. (cxx_eval_builtin_function_call): Handle BUILT_IN_IS_CONSTANT_EVALUATED. (cxx_eval_outermost_constant_expr): Add const_evaluated argument, initialize const_evaluated field in ctx. If the result is TREE_CONSTANT and non_constant_p, retry with const_evaluated false if it was true. (is_sub_constant_expr): Initialize const_evaluated_field in ctx. (cxx_constant_value): Pass true as const_evaluated to cxx_eval_outermost_constant_expr. (maybe_constant_value): Pass false as const_evaluated to cxx_eval_outermost_constant_expr. (fold_non_dependent_expr): Likewise. (maybe_constant_init_1): Add const_evaluated argument, pass it down to cxx_eval_outermost_constant_expr. Pass !allow_non_constant instead of false as strict to cxx_eval_outermost_constant_expr. (maybe_constant_init): Add const_evaluated argument, pass it down to maybe_constant_init_1. (cxx_constant_init): Pass true as const_evaluated to maybe_constant_init_1. * cp-gimplify.c (cp_fold): Don't fold BUILT_IN_IS_CONSTANT_EVALUATED calls. lto/ * lto-lang.c (c_dialect_cxx): Define. ada/ * gcc-interface/utils.c (c_dialect_cxx): Define. brig/ * brig-lang.c (c_dialect_cxx): Define. testsuite/ * g++.dg/cpp2a/is-constant-evaluated1.C: New test. --- gcc/builtins.def.jj 2018-06-20 08:15:34.179862153 +0200 +++ gcc/builtins.def2018-07-20 12:03:10.254453811 +0200 @@ -974,6 +974,11 @@ DEF_EXT_LIB_BUILTIN(BUILT_IN_PRINTF_ DEF_EXT_LIB_BUILTIN(BUILT_IN_VFPRINTF_CHK, "__vfprintf_chk", BT_FN_INT_FILEPTR_INT_CONST_STRING_VALIST_ARG, ATTR_NONNULL_1_FORMAT_PRINTF_3_0) DEF_EXT_LIB_BUILTIN(BUILT_IN_VPRINTF_CHK, "__vprintf_chk", BT_FN_INT_INT_CONST_STRING_VALIST_ARG, ATTR_FORMAT_PRINTF_2_0) +/* C++ __builtin_is_constant_evaluated. */ +DEF_BUILTIN (BUILT_IN_IS_CONSTANT_EVALUATED, "__builtin_is_constant_evaluated", +BUILT_IN_NORMAL, BT_FN_BOOL, BT_LAST, false, false, false, +ATTR_CONST_NOTHROW_LEAF_LIST, true, c_dialect_cxx ()) + /* Profiling hooks. */ DEF_BUILTIN (BUILT_IN_PROFILE_FUNC_ENTER, "__cyg_profile_func_enter", BUILT_IN_NORMAL, BT_FN_VOID_PTR_PTR, BT_LAST, false, false, false, ATTR_NULL, true, true) --- gcc/builtins.c.jj 2018-07-16 23:24:51.306429546 +0200 +++ gcc/builtins.c 2018-07-20 12:09:13.278818768 +0200 @@ -9104,6 +9104,10 @@ fold_builtin_0 (location_t loc, tree fnd case BUILT_IN_CLASSIFY_TYPE: return fold_builtin_classify_type (NULL_TREE); +case BUILT_IN_IS_CONSTANT_EVALUATED: + /* The C++ FE can evaluate this to something other than false. */ + return boolean_false_node; + default: break; } --- gcc/cp/cp-tree.h.jj 2018-07-18 22:57:10.657780293 +0200 +++ gcc/cp/cp-tree.h2018-07-20 18:10:33.416409798 +0200 @@ -7536,7 +7536,7 @@ extern bool require_potential_rvalue_con extern tree cxx_constant_value (tree, tree = NULL_TREE); extern tree cxx_constant_init (tree, tree = NULL_TREE); extern tree maybe_c
Re: [PATCH] specify large command line option arguments (PR 82063)
On 07/21/2018 06:42 PM, H.J. Lu wrote: On Fri, Jul 20, 2018 at 1:57 PM, Martin Sebor wrote: On 07/19/2018 04:31 PM, Jeff Law wrote: On 06/24/2018 03:05 PM, Martin Sebor wrote: Storing integer command line option arguments in type int limits options such as -Wlarger-than= or -Walloca-larger-than to at most INT_MAX (see bug 71905). Larger values wrap around zero. The value zero is considered to disable the option, making it impossible to specify a zero limit. To get around these limitations, the -Walloc-size-larger-than= option accepts a string argument that it then parses itself and interprets as HOST_WIDE_INT. The option also accepts byte size suffixes like KB, MB, GiB, etc. to make it convenient to specify very large limits. The int limitation is obviously less than ideal in a 64-bit world. The treatment of zero as a toggle is just a minor wart. The special treatment to make it work for just a single option makes option handling inconsistent. It should be possible for any option that takes an integer argument to use the same logic. The attached patch enhances GCC option processing to do that. It changes the storage type of option arguments from int to HOST_WIDE_INT and extends the existing (although undocumented) option property Host_Wide_Int to specify wide option arguments. It also introduces the ByteSize property for options for which specifying the byte-size suffix makes sense. To make it possible to consider zero as a meaningful argument value rather than a flag indicating that an option is disabled the patch also adds a CLVC_SIZE enumerator to the cl_var_type enumeration, and modifies how options of the kind are handled. Warning options that take large byte-size arguments can be disabled by specifying a value equal to or greater than HOST_WIDE_INT_M1U. For convenience, aliases in the form of -Wno-xxx-larger-than have been provided for all the affected options. In the patch all the existing -larger-than options are set to PTRDIFF_MAX. This makes them effectively enabled, but because the setting is exceedingly permissive, and because some of the existing warnings are already set to the same value and some other checks detect and reject such exceedingly large values with errors, this change shouldn't noticeably affect what constructs are diagnosed. Although all the options are set to PTRDIFF_MAX, I think it would make sense to consider setting some of them lower, say to PTRDIFF_MAX / 2. I'd like to propose that in a followup patch. To minimize observable changes the -Walloca-larger-than and -Wvla-larger-than warnings required more extensive work to make of the new mechanism because of the "unbounded" argument handling (the warnings trigger for arguments that are not visibly constrained), and because of the zero handling (the warnings also trigger Martin gcc-82063.diff PR middle-end/82063 - issues with arguments enabled by -Wall gcc/ada/ChangeLog: PR middle-end/82063 * gcc-interface/misc.c (gnat_handle_option): Change function argument to HOST_WIDE_INT. gcc/brig/ChangeLog: * brig/brig-lang.c (brig_langhook_handle_option): Change function argument to HOST_WIDE_INT. gcc/c-family/ChangeLog: PR middle-end/82063 * c-common.h (c_common_handle_option): Change function argument to HOST_WIDE_INT. * c-opts.c (c_common_init_options): Same. (c_common_handle_option): Same. Remove special handling of OPT_Walloca_larger_than_ and OPT_Wvla_larger_than_. * c.opt (-Walloc-size-larger-than, -Walloca-larger-than): Change options to take a HOST_WIDE_INT argument and accept a byte-size suffix. Initialize. (-Wvla-larger-than): Same. (-Wno-alloc-size-larger-than, -Wno-alloca-larger-than): New. (-Wno-vla-larger-than): Same. gcc/fortran/ChangeLog: PR middle-end/82063 * gfortran.h (gfc_handle_option): Change function argument to HOST_WIDE_INT. * options.c (gfc_handle_option): Same. gcc/go/ChangeLog: PR middle-end/82063 * go-lang.c (go_langhook_handle_option): Change function argument to HOST_WIDE_INT. gcc/lto/ChangeLog: PR middle-end/82063 * lto-lang.c (lto_handle_option): Change function argument to HOST_WIDE_INT. gcc/testsuite/ChangeLog: PR middle-end/82063 * gcc.dg/Walloc-size-larger-than-16.c: Adjust. * gcc.dg/Walloca-larger-than.c: New test. * gcc.dg/Wframe-larger-than-2.c: New test. * gcc.dg/Wlarger-than3.c: New test. * gcc.dg/Wvla-larger-than-3.c: New test. gcc/ChangeLog: PR middle-end/82063 * builtins.c (expand_builtin_alloca): Adjust. * calls.c (alloc_max_size): Simplify. * cgraphunit.c (cgraph_node::expand): Adjust. * common.opt (larger_than_size, warn_frame_larger_than): Remove variables. (frame_larger_than_size): Same. (-Wframe-larger-than, -Wlarger
Re: [PATCH] fix a couple of bugs in const string folding (PR 86532)
On 07/22/2018 02:00 AM, Bernd Edlinger wrote:
On 07/21/18 00:15, Martin Sebor wrote:
On 07/20/2018 08:51 AM, Bernd Edlinger wrote:
Martin,
when I look at tree-ssa-forwprop.c:
str1 = string_constant (src1, &off1);
if (str1 == NULL_TREE)
break;
if (!tree_fits_uhwi_p (off1)
|| compare_tree_int (off1, TREE_STRING_LENGTH (str1) - 1) > 0
|| compare_tree_int (len1, TREE_STRING_LENGTH (str1)
- tree_to_uhwi (off1)) > 0
|| TREE_CODE (TREE_TYPE (str1)) != ARRAY_TYPE
|| TYPE_MODE (TREE_TYPE (TREE_TYPE (str1)))
!= TYPE_MODE (char_type_node))
break;
I don't think it is a good idea to let string_constant return
strings which have not necessarily valid content after the first
NUL byte. It looks like the content has to be valid up to
TREE_STRING_LENGTH.
I'm not sure I understand when this could happen. From the patch
below it sounds like you are concerned about cases like:
const char a[4] = "abc\000\000";
where the STRING_CST is bigger than the array. But the string
constant has valid content here up to TREE_STRING_LENGTH. Am
I missing something? (A test case would be helpful.)
No, I mean something like:
$ cat y.c
const char a[2][3] = { "1234", "xyz" };
char b[6];
int main ()
{
__builtin_memcpy(b, a, 4);
__builtin_memset(b + 4, 'a', 2);
__builtin_printf("%.6s\n", b);
}
$ gcc y.c
y.c:1:24: warning: initializer-string for array of chars is too long
const char a[2][3] = { "1234", "xyz" };
^~
y.c:1:24: note: (near initialization for 'a[0]')
$ ./a.out
1234aa
but expected would be "123xaa".
Hmm. I assumed this was undefined in C but after double
checking I'm not sure. If it's in fact valid and the excess
elements are required to be ignored I'll of course fix it in
a subsequent patch. Let me find out.
In any case, unless the concern is specifically related to
this bug fix let's discuss it separately so I can fix this
bug. I'm not opposed to making further changes here (in
fact, I have one in the queue and two others that I raised
in Bugzilla in response to our discussion so far), I just
want to avoid mission creep.
I think when you touch a function you should fix it completely
or restrict it to the subset that is known to be safe, and not
leave lots of already known bugs behind.
I find it preferable to open a new bug for separate problems
and tackle each on its own. That makes it possible to track
separate bugs independently, make sure each fix is adequately
tested, and backport patches to release branches as necessary.
Wholesale changes tend to make this more difficult.
If the code in your test case is in fact well-defined then
it's especially important that a test case be added for it
because none exists yet, and opening a separate bug makes
sure this happens.
Martin
Re: [PATCH] fix a couple of bugs in const string folding (PR 86532)
On 07/22/2018 03:03 AM, Bernd Edlinger wrote: On 07/21/18 01:58, Martin Sebor wrote: On 07/20/2018 03:11 PM, Bernd Edlinger wrote: I think I have found a valid test case where the latest patch does generate invalid code: This is due to another bug in string_constant() that's latent on trunk but that's exposed by the patch. Trunk only "works" because of a bug/limitation in c_strlen() that the patch fixes or removes. I am not sure if that falls under the definition of "latent" bug. A latent bug would be something unexpected in a completely different area of the compiler that is triggered by an improved optimization or a fixed bug elsewhere. The underlying root cause is the handling of POINTER_PLUS expressions in string_constant(). The original code (before the handling of aggregates was added) just dealt with string constants. The new code does much more but doesn't get it quite right in these cases. I think you should be much more careful with the expressions you evaluate in string_constant. For instance when you look at get_addr_base_and_unit_offset, you'll see it walks through all kinds of type casts, VIEW_CONVERT_EXPR and MEM_REF with nonzero displacement. When you see something like that do not fold that on the assumption that the source code does not have undefined behavior. So I'd say you should add a check that there is no type cast in the expression you parse. If that is the case, your optimization will certainly be wrong. There are no casts here. The pointer to array case is just something I didn't think of, that's all. The bug is in not rejecting those. As I explained, the original code handled just string literals and constant character arrays. Current trunk deals with all kinds of constants and doesn't get all the constraints right. Martin
Re: [PATCH] fix a couple of bugs in const string folding (PR 86532)
On 07/22/2018 04:47 PM, Martin Sebor wrote:
On 07/22/2018 02:00 AM, Bernd Edlinger wrote:
On 07/21/18 00:15, Martin Sebor wrote:
On 07/20/2018 08:51 AM, Bernd Edlinger wrote:
Martin,
when I look at tree-ssa-forwprop.c:
str1 = string_constant (src1, &off1);
if (str1 == NULL_TREE)
break;
if (!tree_fits_uhwi_p (off1)
|| compare_tree_int (off1, TREE_STRING_LENGTH
(str1) - 1) > 0
|| compare_tree_int (len1, TREE_STRING_LENGTH (str1)
- tree_to_uhwi (off1))
> 0
|| TREE_CODE (TREE_TYPE (str1)) != ARRAY_TYPE
|| TYPE_MODE (TREE_TYPE (TREE_TYPE (str1)))
!= TYPE_MODE (char_type_node))
break;
I don't think it is a good idea to let string_constant return
strings which have not necessarily valid content after the first
NUL byte. It looks like the content has to be valid up to
TREE_STRING_LENGTH.
I'm not sure I understand when this could happen. From the patch
below it sounds like you are concerned about cases like:
const char a[4] = "abc\000\000";
where the STRING_CST is bigger than the array. But the string
constant has valid content here up to TREE_STRING_LENGTH. Am
I missing something? (A test case would be helpful.)
No, I mean something like:
$ cat y.c
const char a[2][3] = { "1234", "xyz" };
char b[6];
int main ()
{
__builtin_memcpy(b, a, 4);
__builtin_memset(b + 4, 'a', 2);
__builtin_printf("%.6s\n", b);
}
$ gcc y.c
y.c:1:24: warning: initializer-string for array of chars is too long
const char a[2][3] = { "1234", "xyz" };
^~
y.c:1:24: note: (near initialization for 'a[0]')
$ ./a.out
1234aa
but expected would be "123xaa".
Hmm. I assumed this was undefined in C but after double
checking I'm not sure. If it's in fact valid and the excess
elements are required to be ignored I'll of course fix it in
a subsequent patch. Let me find out.
The WG14 convener confirmed that this is indeed undefined.
The words that apply to this case (as well as all the others)
are in 6.7.9, p2:
No initializer shall attempt to provide a value for an object
not contained within the entity being initialized.
If GCC wants to make this well-defined and guarantee that
the excess elements are stripped (I'm not opposed to it)
we need to treat it should be treated as an enhancement
request, so the feature can be documented and properly
tested (I'm not opposed to it but I'm not going to champion
it either.) Otherwise, I see no reason for a change.
Martin
