Regression on 32-bit powerpc?
ot;\"%s\": ", key); // FIXME: escaping? | ^~ ../../src/gcc/json.cc:73:27: warning: unquoted sequence of 2 consecutive punctuation characters '":' in format [-Wformat-diag] 73 | pp_printf (pp, "\"%s\": ", key); // FIXME: escaping? | ^~~ ../../src/gcc/json.cc:73:30: warning: spurious trailing space in format [-Wformat-diag] 73 | pp_printf (pp, "\"%s\": ", key); // FIXME: escaping? | ^ ../../src/gcc/json.cc:73:23: warning: unterminated quote character '"' in format [-Wformat-diag] 73 | pp_printf (pp, "\"%s\": ", key); // FIXME: escaping? | ^~ /<>/build/./prev-gcc/xg++ -B/<>/build/./prev-gcc/ -B/usr/powerpc-linux-gnu/bin/ -nostdinc++ -B/<>/build/prev-powerpc-linux-gnu/libstdc++-v3/src/.libs -B/<>/build/prev-powerpc-linux-gnu/libstdc++-v3/libsupc++/.libs -I/<>/build/prev-powerpc-linux-gnu/libstdc++-v3/include/powerpc-linux-gnu -I/<>/build/prev-powerpc-linux-gnu/libstdc++-v3/include -I/<>/src/libstdc++-v3/libsupc++ -L/<>/build/prev-powerpc-linux-gnu/libstdc++-v3/src/.libs -L/<>/build/prev-powerpc-linux-gnu/libstdc++-v3/libsupc++/.libs -fno-PIE -c -DBASEVER="\"10.0.1\"" -DDATESTAMP="\" 20200304\"" -DREVISION="\" [master revision 0b0908c1f27:cb0a7e0ca53:94f7d7ec6ebef49a50da777fd71db3d03ee03aa0]\"" -DDEVPHASE="\" (experimental)\"" -DPKGVERSION="\"(Debian 10-20200304-1) \"" -DBUGURL="\"\"" -g -O2 -fno-checking -gtoggle -DIN_GCC -fno-exceptions -fno-rtti -fasynchronous-unwind-tables -W -Wall -Wno-narrowing -Wwrite-strings -Wcast-qual -Wno-error=format-diag -Wmissing-format-attribute -Woverloaded-virtual -pedantic -Wno-long-long -Wno-variadic-macros -Wno-overlength-strings -DHAVE_CONFIG_H -I. -I. -I../../src/gcc -I../../src/gcc/. -I../../src/gcc/../include -I../../src/gcc/../libcpp/include -I../../src/gcc/../libdecnumber -I../../src/gcc/../libdecnumber/dpd -I../libdecnumber -I../../src/gcc/../libbacktrace -o version.o -MT version.o -MMD -MP -MF ./.deps/version.TPo ../../src/gcc/version.c ../../src/gcc/pretty-print.c: In function 'void pp_begin_url(pretty_printer*, const char*)': ../../src/gcc/pretty-print.c:2179:21: warning: unquoted control character '\x1b' in format [-Wformat-diag] 2179 | pp_printf (pp, "\33]8;;%s\33\\", url); | ^~~ ../../src/gcc/pretty-print.c:2179:24: warning: unbalanced punctuation character ']' in format [-Wformat-diag] 2179 | pp_printf (pp, "\33]8;;%s\33\\", url); |^ ../../src/gcc/pretty-print.c:2179:26: warning: unquoted sequence of 2 consecutive punctuation characters ';;' in format [-Wformat-diag] 2179 | pp_printf (pp, "\33]8;;%s\33\\", url); | ^~ ../../src/gcc/pretty-print.c:2179:30: warning: unquoted control character '\x1b' in format [-Wformat-diag] 2179 | pp_printf (pp, "\33]8;;%s\33\\", url); | ^~~ ../../src/gcc/pretty-print.c:2179:33: warning: spurious trailing punctuation sequence '\' in format [-Wformat-diag] 2179 | pp_printf (pp, "\33]8;;%s\33\\", url); | ^~ ../../src/gcc/pretty-print.c:2182:21: warning: unquoted control character '\x1b' in format [-Wformat-diag] 2182 | pp_printf (pp, "\33]8;;%s\a", url); | ^~~ ../../src/gcc/pretty-print.c:2182:24: warning: unbalanced punctuation character ']' in format [-Wformat-diag] 2182 | pp_printf (pp, "\33]8;;%s\a", url); |^ ../../src/gcc/pretty-print.c:2182:26: warning: unquoted sequence of 2 consecutive punctuation characters ';;' in format [-Wformat-diag] 2182 | pp_printf (pp, "\33]8;;%s\a", url); | ^~ ../../src/gcc/pretty-print.c:2182:30: warning: unquoted control character '\x07' in format [-Wformat-diag] 2182 | pp_printf (pp, "\33]8;;%s\a", url); | ^~ Full log in: https://buildd.debian.org/status/fetch.php?pkg=gcc-10=powerpc=10-20200304-1=1583386777=0 Adrian -- .''`. John Paul Adrian Glaubitz : :' : Debian Developer - glaub...@debian.org `. `' Freie Universitaet Berlin - glaub...@physik.fu-berlin.de `-GPG: 62FF 8A75 84E0 2956 9546 0006 7426 3B37 F5B5 F913
Re: [PATCH] print-rtl: Fix printing of CONST_STRING in DEBUG_INSNs [PR93399]
On Thu, 5 Mar 2020, Jakub Jelinek wrote:
> Hi!
>
> The following testcase fails to assemble, as CONST_STRING in the DEBUG_INSNs
> is printed as is, so if it contains \n and/or \r, we are in trouble:
> .loc 1 14 3
> # DEBUG haystack => [si]
> # DEBUG needle => "
> "
> In the gimple dumps we print those (STRING_CSTs) as
> # DEBUG haystack => D#1
> # DEBUG needle => "\n"
> so this patch uses what we use in tree printing for the CONST_STRINGs too.
> I think it isn't really useful to print gigabytes for very long strings,
> so the patch also shrinks the string with ... at the end indicating
> continuation, but if you think that is undesirable, I can leave it out
> (though, at least theoretically, while STRING_CSTs can't be longer than
> 2GB because tree_string has int length, CONST_STRING is just a pointer
> without length and thus on 64-bit hosts could be longer, so if not capping
> at 1K, I think we need to cap at 2GB-1 or 4GB-1 (pretty_print_string
> takes unsigned nbytes). Or of course we can change it to size_t nbytes.
It looks like tree-pretty-print.c doesn't bother to truncate it
so why bother for RTL?
So OK with not pruning the string for now.
Thanks,
Richard.
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
>
> 2020-03-05 Jakub Jelinek
>
> PR middle-end/93399
> * tree-pretty-print.h (pretty_print_string): Declare.
> * tree-pretty-print.c (pretty_print_string): Remove forward
> declaration, no longer static.
> * print-rtl.c (print_value) : Use
> pretty_print_string and for shrink way too long strings.
>
> * gcc.dg/pr93399.c: New test.
>
> --- gcc/tree-pretty-print.h.jj2020-01-12 11:54:38.499381937 +0100
> +++ gcc/tree-pretty-print.h 2020-03-04 14:50:22.737021195 +0100
> @@ -47,6 +47,7 @@ extern void print_declaration (pretty_pr
> extern int op_code_prio (enum tree_code);
> extern int op_prio (const_tree);
> extern const char *op_symbol_code (enum tree_code);
> +extern void pretty_print_string (pretty_printer *, const char *, unsigned);
> extern void print_call_name (pretty_printer *, tree, dump_flags_t);
> extern void percent_K_format (text_info *, location_t, tree);
> extern void pp_tree_identifier (pretty_printer *, tree);
> --- gcc/tree-pretty-print.c.jj2020-01-30 17:55:50.411163667 +0100
> +++ gcc/tree-pretty-print.c 2020-03-04 14:49:53.673451945 +0100
> @@ -45,7 +45,6 @@ along with GCC; see the file COPYING3.
>
> /* Local functions, macros and variables. */
> static const char *op_symbol (const_tree);
> -static void pretty_print_string (pretty_printer *, const char*, unsigned);
> static void newline_and_indent (pretty_printer *, int);
> static void maybe_init_pretty_print (FILE *);
> static void print_struct_decl (pretty_printer *, const_tree, int,
> dump_flags_t);
> @@ -4216,7 +4215,7 @@ print_call_name (pretty_printer *pp, tre
> /* Print the first N characters in the array STR, replacing non-printable
> characters (including embedded nuls) with unambiguous escape sequences.
> */
>
> -static void
> +void
> pretty_print_string (pretty_printer *pp, const char *str, unsigned n)
> {
>if (str == NULL)
> --- gcc/print-rtl.c.jj2020-01-12 11:54:36.915405835 +0100
> +++ gcc/print-rtl.c 2020-03-04 14:55:55.089094999 +0100
> @@ -1685,7 +1685,15 @@ print_value (pretty_printer *pp, const_r
>pp_string (pp, tmp);
>break;
> case CONST_STRING:
> - pp_printf (pp, "\"%s\"", XSTR (x, 0));
> + pp_string (pp, "\"");
> + if (size_t nbytes = strlen (XSTR (x, 0)))
> + {
> + unsigned int n = MIN (nbytes, 1024);
> + pretty_print_string (pp, XSTR (x, 0), n);
> + if (n != nbytes)
> + pp_string (pp, "...");
> + }
> + pp_string (pp, "\"");
>break;
> case SYMBOL_REF:
>pp_printf (pp, "`%s'", XSTR (x, 0));
> --- gcc/testsuite/gcc.dg/pr93399.c.jj 2020-03-04 15:20:01.281631147 +0100
> +++ gcc/testsuite/gcc.dg/pr93399.c2020-03-04 15:19:18.426266715 +0100
> @@ -0,0 +1,17 @@
> +/* PR middle-end/93399 */
> +/* { dg-do assemble } */
> +/* { dg-options "-fverbose-asm -dA -g -O3" } */
> +
> +extern inline __attribute__ ((__always_inline__, __gnu_inline__)) char *
> +strstr (const char *haystack, const char *needle)
> +{
> + return __builtin_strstr (haystack, needle);
> +}
> +
> +int
> +main (int argc, const char **argv)
> +{
> + char *substr = strstr (argv[0], "\n");
> + char *another = strstr (argv[0], "\r\n");
> + return 0;
> +}
>
> Jakub
>
>
--
Richard Biener
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)
Re: [RFA/RFC] [tree-optimization/91890] [P1 Regression] Avoid clobbering useful location in Wrestrict code
On Thu, Mar 5, 2020 at 12:49 AM Jeff Law wrote:
>
> On Wed, 2020-03-04 at 09:22 -0700, Martin Sebor wrote:
> >
> > I don't remember why the code in -Wrestrict unconditionally overwrites
> > the statement location rather than only when it's not available, but
> > I do remember adding conditional code like in your patch in r277076
> > to deal with missing location on the statement. So either your fix
> > or something like the hunk below might be the right solution (if we
> > go with the code below, abstracting it into a utility function might
> > be nice).
> So there's several chunks that are fairly similar to what you referenced in
> maybe_warn_pointless_strcmp. Factoring all of them into a single location is
> pretty easy.
>
> That also gives us a nice place where we can experiment with "does extraction
> of
> location information from the expression ever help". The answer is, it
> doesn't,
> at least not within our testsuite when run on x86_64.
>
> I'm hesitant to remove the code that extracts the location out of the
> expression,
> but could be convinced to do so.
>
> Thoughts?
Using anything but the actual stmt location is prone to end up at random places
due to tree sharing issues, CSE and copy propagation. Simply consider
char one[50];
char two[50];
void
test_strncat (void)
{
char *p = one;
(void) __builtin_strcpy (p, "gh");
(void) __builtin_strcpy (two, "ef");
#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wstringop-overflow="
#pragma GCC diagnostic ignored "-Warray-bounds"
(void) __builtin_strncat (p, two, 99);
#pragma GCC diagnostic pop
}
where we happily forward p = [0] to both uses injecting
a "faulty" location. Well, it's actually the correct location
computing [0] but irrelevant for the actual call.
So the question is why we end up with UNKNOWN_LOCATION
for such call and if why we need to bother emit a diagnostic
at all (and why emitting it for another possibly random location is a good idea
instead of maybe simply emitting it without location).
Richard.
> Jeff
[PATCH] c++: Adjust handling of COMPOUND_EXPRs in cp_build_binary_op [PR91993]
Hi!
As the testcases shows, the -Wconversion warning behaves quite differently
when -fsanitize=undefined vs. when not sanitizing, but in the end it is
not something specific to sanitizing, if a user uses
return static_cast(static_cast((d++, a) << 1U) | b) | c;
instead of
return static_cast(static_cast(a << 1U) | b) | c;
and thus there is some COMPOUND_EXPR involved, cp_build_binary_op behaves
significantly different, e.g. shorten_binary_op will have different result
(uc for the case without COMPOUND_EXPR, int with it), but it isn't limited
to that.
The following patch attempts to handle those the same, basically ignoring
everything but the ultimately last operand of COMPOUND_EXPR(s) and treating
the other COMPOUND_EXPR(s) operand(s) just as side-effects that need to be
evaluated first. Of course, if CODE has evaluation ordering (&&, ||, and
<<, >> in C++17), we need to be more careful, but as in all of those op0 is
sequenced before op1, we can just handle op0 that way and not do it for op1.
The patch can still change evaluation order, but I believe only when it is
undefined, e.g.
(foo (), bar ()) + (baz (), qux ())
used to be evaluated as foo, bar, baz, qux and now would be foo, baz, bar,
qux, so if you think we should defer it for GCC11, fine with me.
As for compile time/memory complexity, as the patch pushes the side-effects
into a single expression that is then added, after it has been performed the
outermost COMPOUND_EXPR should contain the final result in op1 and all the
other COMPOUND_EXPR if any in op0 and so another cp_build_binary_op should
see just one COMPOUND_EXPR to handle.
Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk, or
defer for GCC11? Or do we instead want to remember the pre-side-effects
for each operand separately, perform what cp_build_binary_op does and readd
them right before constructing the final result to each?
2020-03-05 Jakub Jelinek
PR c++/91993
* typeck.c (cp_build_binary_op): If orig_op0 is COMPOUND_EXPR,
push the first operand into instrument_expr and use the second operand
as orig_op0. Similarly for orig_op1 if evaluation order is undefined.
Make sure to add instrument_expr into a new COMPOUND_EXPR before
returning. Use add_stmt_to_compound even if instrument_expr is NULL.
Formatting fix.
* g++.dg/warn/Wconversion-pr91993.C: New test.
* g++.dg/ubsan/pr91993.C: New test.
--- gcc/cp/typeck.c.jj 2020-03-05 07:57:41.759445443 +0100
+++ gcc/cp/typeck.c 2020-03-05 08:25:23.841850883 +0100
@@ -4478,6 +4478,25 @@ cp_build_binary_op (const op_location_t
/* Tree holding instrumentation expression. */
tree instrument_expr = NULL_TREE;
+ while (TREE_CODE (orig_op0) == COMPOUND_EXPR)
+{
+ instrument_expr = add_stmt_to_compound (instrument_expr,
+ TREE_OPERAND (orig_op0, 0));
+ orig_op0 = TREE_OPERAND (orig_op0, 1);
+}
+ if (code != TRUTH_ANDIF_EXPR
+ && code != TRUTH_ORIF_EXPR
+ && ((code != LSHIFT_EXPR && code != RSHIFT_EXPR)
+ || flag_strong_eval_order != 2))
+{
+ while (TREE_CODE (orig_op1) == COMPOUND_EXPR)
+ {
+ instrument_expr = add_stmt_to_compound (instrument_expr,
+ TREE_OPERAND (orig_op1, 0));
+ orig_op1 = TREE_OPERAND (orig_op1, 1);
+ }
+}
+
/* Apply default conversions. */
op0 = resolve_nondeduced_context (orig_op0, complain);
op1 = resolve_nondeduced_context (orig_op1, complain);
@@ -4566,8 +4585,8 @@ cp_build_binary_op (const op_location_t
&& !TYPE_PTR_OR_PTRMEM_P (type1)))
&& (complain & tf_warning))
{
- location_t loc =
- expansion_point_location_if_in_system_header (input_location);
+ location_t loc
+ = expansion_point_location_if_in_system_header (input_location);
warning_at (loc, OPT_Wpointer_arith, "NULL used in arithmetic");
}
@@ -4618,10 +4637,13 @@ cp_build_binary_op (const op_location_t
&& same_type_ignoring_top_level_qualifiers_p (TREE_TYPE (type0),
TREE_TYPE (type1)))
{
+ tree instrument_expr1 = NULL_TREE;
result = pointer_diff (location, op0, op1,
common_pointer_type (type0, type1), complain,
-_expr);
- if (instrument_expr != NULL)
+_expr1);
+ instrument_expr = add_stmt_to_compound (instrument_expr,
+ instrument_expr1);
+ if (instrument_expr != NULL_TREE)
result = build2 (COMPOUND_EXPR, TREE_TYPE (result),
instrument_expr, result);
@@ -4650,10 +4672,12 @@ cp_build_binary_op (const op_location_t
result_type = TREE_TYPE (ptr_operand);
break;
}
- return
Re: [PATCH v2 3/3] Keep .GCC.command.line sections of LTO objetcs.
On Wed, Mar 4, 2020 at 8:39 PM Egeyar Bagcioglu wrote: > > > > On 3/4/20 6:23 PM, Martin Liška wrote: > > On 3/4/20 4:25 PM, Egeyar Bagcioglu wrote: > >> Thanks Richard. > >> > >> I do not have write-access to the GCC repo. I'd be glad if someone > >> commits it for me. > > > > Can we please wait? I'm really convinced we do not want one another > > very similar > > functionality. > > I am sorry. I thought Richard's comment was only about the third patch, > which is also necessary for the current -frecord-gcc-switches. > Therefore, my reply was only for that patch as well. Yes, my comment / approval was only about preserving the early compile options for LTO. I'll commit that piece momentarily. Richard. > Regards > Egeyar > > > I would definitely recommend to change the semantics > > of -frecord-gcc-switches to what the patchset does. > > > > That's why I added Nick as he's the author of the original > > implementation. > > Reasoning is provided in my previous email. > > > > Martin >
Re: [PATCH v2 0/3] Introduce a new GCC option, --record-gcc-command-line
On Wed, Mar 4, 2020 at 5:28 PM Egeyar Bagcioglu wrote: > > > > On 3/4/20 1:18 AM, Fangrui Song wrote: > > On 2020-03-03, Joseph Myers wrote: > >> On Tue, 3 Mar 2020, Egeyar Bagcioglu wrote: > >> > >>> Although we discussed after the submission of the first version that > >>> there are several other options performing similar tasks, I believe we > >>> established that there is still a need for this specific functionality. > >>> Therefore, I am skipping in this email the comparison between this > >>> option and the existing options with similarities. > > > > Mentioning -frecord-gcc-switches will be much appreciated. > > > > How is the new .GCC.command.line different? > > > > Does it still have the SHF_MERGE | SHF_STRINGS flag? > > If you change the flags, the .GCC.command.line section may not play with > > another object file (generated by -frecord-gcc-switches) whose > > .GCC.command.line is > > SHF_MERGE | SHF_STRINGS. > > > > When both -frecord-gcc-switches and --record-command-line are specified, > > is it an error? > > This option is similar to -frecord-gcc-switches. However, they have > three fundamental differences: Firstly, -frecord-gcc-switches saves the > internal state after the argv is processed and passed by the driver. As > opposed to that, --record-gcc-command-line saves the command-line as > received by the driver, with the exception of extending @files first. > Secondly, -frecord-gcc-switches saves the switches as separate entries > into a mergeable string section. Therefore, the entries belonging to > different object files get mixed up after being linked. The new > --record-gcc-command-line, on the other hand, creates one entry per > invocation. By doing so, it makes it clear which options were used > together in a single gcc invocation. Lastly, --record-gcc-command-line > also adds the version of the gcc into this single entry to make it clear > which version of gcc was called with any given command line. This is > useful in cases where .comment section reports multiple versions. > > While there are also similarities between the implementations of these > two options, those implementations are completely independent. These > commands can be used separately or together without issues. I used the > same section that -frecord-gcc-switches uses on purpose, so that they > can also be used together to save both the command line given to GCC and > the internal switches passed by GCC. > > The option -grecord-gcc-switches is similar to -frecord-gcc-switches, > but saves the internal GCC switches into DWARF. Lastly, -fverbose-asm > option saves the switches into the assembly file but that information > never makes it to the object files. -grecord-gcc-switches also allows to match the options used to the actual generated code while both -frecord-gcc-switches and --record-gcc-command-line end up as ELF comment sections not associated with particular code pieces. So IMHO anything but -grecord-gcc-switches is quite useless in case options used do not match for all object files. Richard. > > > >> We're now using git-style commit messages with self-contained > >> explanation > >> / justification of the change being committed. > >> > >> This means that one of the commit messages (not just message 0, whose > >> contents don't go in a commit message) for an individual patch should > >> have > >> the explanation, which should include the self-contained > >> justification by > >> reference to comparison with other existing similar options. People > >> should be able to find the relevant information in the commit without > >> needing to search the list archives for reviews of a previous patch > >> version. > > Thanks for telling me. I will extend the above comparison according to > the questions I might receive. Then I'll add it, together with the > explanation in the cover letter, into the commit message of the second > patch. > > Regards > Egeyar
[PATCH] print-rtl: Fix printing of CONST_STRING in DEBUG_INSNs [PR93399]
Hi!
The following testcase fails to assemble, as CONST_STRING in the DEBUG_INSNs
is printed as is, so if it contains \n and/or \r, we are in trouble:
.loc 1 14 3
# DEBUG haystack => [si]
# DEBUG needle => "
"
In the gimple dumps we print those (STRING_CSTs) as
# DEBUG haystack => D#1
# DEBUG needle => "\n"
so this patch uses what we use in tree printing for the CONST_STRINGs too.
I think it isn't really useful to print gigabytes for very long strings,
so the patch also shrinks the string with ... at the end indicating
continuation, but if you think that is undesirable, I can leave it out
(though, at least theoretically, while STRING_CSTs can't be longer than
2GB because tree_string has int length, CONST_STRING is just a pointer
without length and thus on 64-bit hosts could be longer, so if not capping
at 1K, I think we need to cap at 2GB-1 or 4GB-1 (pretty_print_string
takes unsigned nbytes). Or of course we can change it to size_t nbytes.
Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
2020-03-05 Jakub Jelinek
PR middle-end/93399
* tree-pretty-print.h (pretty_print_string): Declare.
* tree-pretty-print.c (pretty_print_string): Remove forward
declaration, no longer static.
* print-rtl.c (print_value) : Use
pretty_print_string and for shrink way too long strings.
* gcc.dg/pr93399.c: New test.
--- gcc/tree-pretty-print.h.jj 2020-01-12 11:54:38.499381937 +0100
+++ gcc/tree-pretty-print.h 2020-03-04 14:50:22.737021195 +0100
@@ -47,6 +47,7 @@ extern void print_declaration (pretty_pr
extern int op_code_prio (enum tree_code);
extern int op_prio (const_tree);
extern const char *op_symbol_code (enum tree_code);
+extern void pretty_print_string (pretty_printer *, const char *, unsigned);
extern void print_call_name (pretty_printer *, tree, dump_flags_t);
extern void percent_K_format (text_info *, location_t, tree);
extern void pp_tree_identifier (pretty_printer *, tree);
--- gcc/tree-pretty-print.c.jj 2020-01-30 17:55:50.411163667 +0100
+++ gcc/tree-pretty-print.c 2020-03-04 14:49:53.673451945 +0100
@@ -45,7 +45,6 @@ along with GCC; see the file COPYING3.
/* Local functions, macros and variables. */
static const char *op_symbol (const_tree);
-static void pretty_print_string (pretty_printer *, const char*, unsigned);
static void newline_and_indent (pretty_printer *, int);
static void maybe_init_pretty_print (FILE *);
static void print_struct_decl (pretty_printer *, const_tree, int,
dump_flags_t);
@@ -4216,7 +4215,7 @@ print_call_name (pretty_printer *pp, tre
/* Print the first N characters in the array STR, replacing non-printable
characters (including embedded nuls) with unambiguous escape sequences. */
-static void
+void
pretty_print_string (pretty_printer *pp, const char *str, unsigned n)
{
if (str == NULL)
--- gcc/print-rtl.c.jj 2020-01-12 11:54:36.915405835 +0100
+++ gcc/print-rtl.c 2020-03-04 14:55:55.089094999 +0100
@@ -1685,7 +1685,15 @@ print_value (pretty_printer *pp, const_r
pp_string (pp, tmp);
break;
case CONST_STRING:
- pp_printf (pp, "\"%s\"", XSTR (x, 0));
+ pp_string (pp, "\"");
+ if (size_t nbytes = strlen (XSTR (x, 0)))
+ {
+ unsigned int n = MIN (nbytes, 1024);
+ pretty_print_string (pp, XSTR (x, 0), n);
+ if (n != nbytes)
+ pp_string (pp, "...");
+ }
+ pp_string (pp, "\"");
break;
case SYMBOL_REF:
pp_printf (pp, "`%s'", XSTR (x, 0));
--- gcc/testsuite/gcc.dg/pr93399.c.jj 2020-03-04 15:20:01.281631147 +0100
+++ gcc/testsuite/gcc.dg/pr93399.c 2020-03-04 15:19:18.426266715 +0100
@@ -0,0 +1,17 @@
+/* PR middle-end/93399 */
+/* { dg-do assemble } */
+/* { dg-options "-fverbose-asm -dA -g -O3" } */
+
+extern inline __attribute__ ((__always_inline__, __gnu_inline__)) char *
+strstr (const char *haystack, const char *needle)
+{
+ return __builtin_strstr (haystack, needle);
+}
+
+int
+main (int argc, const char **argv)
+{
+ char *substr = strstr (argv[0], "\n");
+ char *another = strstr (argv[0], "\r\n");
+ return 0;
+}
Jakub
Re: [testsuite] Fix PR94019 to allow one vector char when !vect_hw_misalign
Hi Richard,
on 2020/3/5 上午3:09, Richard Sandiford wrote:
> "Kewen.Lin" writes:
>> Hi,
>>
>>
>> --- a/gcc/testsuite/gcc.dg/vect/vect-over-widen-17.c
>> +++ b/gcc/testsuite/gcc.dg/vect/vect-over-widen-17.c
>> @@ -41,6 +41,10 @@ main (void)
>> }
>>
>> /* { dg-final { scan-tree-dump-not {vect_recog_over_widening_pattern:
>> detected} "vect" } } */
>> -/* { dg-final { scan-tree-dump-not {vector[^\n]*char} "vect" } } */
>> +/* { dg-final { scan-tree-dump-not {vector[^\n]*char} "vect" { target { { !
>> powerpc*-*-* } || { powerpc*-*-* && vect_hw_misalign } } } } }
>> +/* On Power, if there is no vect_hw_misalign support, unaligned vector
>> access
>> + adopts realign_load scheme. It requires rs6000_builtin_mask_for_load to
>> + generate mask whose return type is vector char. */
>> +/* { dg-final { scan-tree-dump-times {vector[^\n]*char} 1 "vect" { target {
>> powerpc*-*-* && { ! vect_hw_misalign } } } } } */
>
> Thanks for looking at this. The patch is OK as-is. However, since
> vect-over-widen-17.c is a negative test for generic code, there probably
> isn't much need for the new scan-tree-dump-times line, and it could start
> failing if we make different optimisation decisions in future. So the
> patch is also OK with just the change to the scan-tree-dump-not line,
> if you prefer that. (Please keep the comment either way though --
> it's really helpful.)
>
Thanks for your suggestion! The new patch is updated as below. I removed
the scan-tree-dump-times, as well as powerpc specific requirement.
Does it look good to you especially the later? Thanks in advance!
BR,
Kewen
gcc/testsuite/ChangeLog
2020-03-05 Kewen Lin
PR testsuite/94019
* gcc.dg/vect/vect-over-widen-17.c: Don't expect vector char if it's
without misaligned vector access support.
--
diff --git a/gcc/testsuite/gcc.dg/vect/vect-over-widen-17.c
b/gcc/testsuite/gcc.dg/vect/vect-over-widen-17.c
index 0448260..333d74a 100644
--- a/gcc/testsuite/gcc.dg/vect/vect-over-widen-17.c
+++ b/gcc/testsuite/gcc.dg/vect/vect-over-widen-17.c
@@ -41,6 +41,9 @@ main (void)
}
/* { dg-final { scan-tree-dump-not {vect_recog_over_widening_pattern:
detected} "vect" } } */
-/* { dg-final { scan-tree-dump-not {vector[^\n]*char} "vect" } } */
+/* On Power, if there is no vect_hw_misalign support, unaligned vector access
+ adopts realign_load scheme. It requires rs6000_builtin_mask_for_load to
+ generate mask whose return type is vector char. */
+/* { dg-final { scan-tree-dump-not {vector[^\n]*char} "vect" { target
vect_hw_misalign } } } */
/* { dg-final { scan-tree-dump-not {vector[^ ]* int} "vect" } } */
/* { dg-final { scan-tree-dump-times "vectorized 1 loop" 1 "vect" } } */
Re: [testsuite] Fix PR94019 to allow one vector char when !vect_hw_misalign
Hi Segher, on 2020/3/5 上午2:44, Segher Boessenkool wrote: > Hi! > > On Wed, Mar 04, 2020 at 03:13:51PM +0800, Kewen.Lin wrote: >> As PR94019 shows, without misaligned vector access support but with >> realign load, the vectorized loop will end up with realign scheme. >> It generates mask (control vector) with return type vector signed >> char which breaks the not check. >> >> The fix is to differentiate powerpc vect_hw_misalign and powerpc >> !vect_hw_misalign, permit one vector char occurance for powerpc >> !vect_hw_misalign and keep other targets same as before. >> >> Verified it on ppc64-redhat-linux (Power7 BE). >> >> Is it ok for trunk, and backport to GCC 9 after some burn-in time? > > Why is any of this Power-specific? Does it work on other targets if > !vect_hw_misalign? > Good question, the initial draft was not to take power as specific and just guard it with !vect_hw_misalign. But the root cause is due to builtin_mask_for_load return type, currently rs6000 port is the only user providing that hook, I thought it's better to use "times" to keep the coverage somehow. To avoid any possible new ports with that hook but different return type and then leads to unexpected vector char occurrence times, I further guarded it with powerpc specific. Considering Richard's comments, I think it's fine to remove that particularity now. BR, Kewen
[PATCH] rs6000: Check -+0 and NaN for smax/smin generation
Hi,
PR93709 mentioned regressions on maxlocval_4.f90 and minlocval_f.f90 which
relates to max of '-inf' and 'nan'. This regression occur on P9 which has
new instruction 'xsmaxcdp/xsmincdp'.
The similar issue also could be find on `a < b ? b : a` which is also
generated as `xsmaxcdp` under -O2 for P9. This instruction `xsmaxcdp`
more like C/C++ semantic (a>b?a:b). A testcase is added for this issue.
The following patch improve code to check -+0 and NaN before 'smax/smin' to
be generated for those cases.
Bootstrap/regtest on powerpc64le pass without regressions.
Is this OK for trunk and backport to GCC 9?
BR.
Jiufu
gcc/
2020-03-05 Jiufu Guo
PR target/93709
* gcc/config/rs6000/rs6000.c (rs6000_emit_p9_fp_minmax): Check
-fno-signed-zeros and -ffinite_math_only for smax/smin.
gcc/testsuite
2020-03-05 Jiufu Guo
PR target/93709
* gcc.target/powerpc/p9-minmax-3.c: New test.
---
gcc/config/rs6000/rs6000.c | 8 +++-
gcc/testsuite/gcc.target/powerpc/p9-minmax-3.c | 17 +
2 files changed, 24 insertions(+), 1 deletion(-)
create mode 100644 gcc/testsuite/gcc.target/powerpc/p9-minmax-3.c
diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c
index f34e1ba70c6..951a6c32884 100644
--- a/gcc/config/rs6000/rs6000.c
+++ b/gcc/config/rs6000/rs6000.c
@@ -14836,7 +14836,13 @@ rs6000_emit_p9_fp_minmax (rtx dest, rtx op, rtx
true_cond, rtx false_cond)
if (rtx_equal_p (op0, true_cond) && rtx_equal_p (op1, false_cond))
;
- else if (rtx_equal_p (op1, true_cond) && rtx_equal_p (op0, false_cond))
+ /* Only when -fno-signed-zeros and -ffinite_math_only are in effect,
+ `op0 < op1 ? op1 : op0` works like `op1 > op0 ? op1 : op0` which
+ could use smax;
+ `op0 > op1 ? op1 : op0` works like `op1 < op0 ? op1 : op0` which
+ could use smin. */
+ else if (rtx_equal_p (op1, true_cond) && rtx_equal_p (op0, false_cond)
+ && (flag_finite_math_only && !flag_signed_zeros))
max_p = !max_p;
else
diff --git a/gcc/testsuite/gcc.target/powerpc/p9-minmax-3.c
b/gcc/testsuite/gcc.target/powerpc/p9-minmax-3.c
new file mode 100644
index 000..141603e05b4
--- /dev/null
+++ b/gcc/testsuite/gcc.target/powerpc/p9-minmax-3.c
@@ -0,0 +1,17 @@
+/* { dg-do compile { target { powerpc*-*-* } } } */
+/* { dg-require-effective-target powerpc_p9vector_ok } */
+/* { dg-options "-mdejagnu-cpu=power9 -O2 -mpower9-minmax" } */
+/* { dg-final { scan-assembler-not "xsmaxcdp" } } */
+/* { dg-final { scan-assembler-not "xsmincdp" } } */
+
+double
+dbl_max1 (double a, double b)
+{
+ return a < b ? b : a;
+}
+
+double
+dbl_min1 (double a, double b)
+{
+ return a > b ? b : a;
+}
--
2.17.1
Re: [PATCH] [rs6000] Fix a wrong GC issue
Hi Segher, On 2020/3/5 上午2:35, Segher Boessenkool wrote: On Wed, Mar 04, 2020 at 03:08:41PM +0800, binbin wrote: * config/rs6000/rs6000.h (MAX_MACHINE_MODE): Include the header file for MAX_MACHINE_MODE. The changelog entry should say *what* file is included, and under what condition. It doesn't have to say why (that belongs in the commit message). But, can't you just include it unconditionally? Don't we already, anyway, via coretypes.h -> machmode.h -> insn-modes.h? OK, change it to uncondition. Thanks for your suggestion. What about the second part? Shouldn't it already be included anyway? If "insn-modes.h" is not included in rs6000.h, it reports error showing MAX_MACHINE_MODE’ undeclared here (not in a function) in file included from ../../host-powerpc64le-unknown-linux-gnu/gcc/tm.h:25 from ../.././libgcc/libgcc2.c:29 ../.././libgcc/../gcc/config/rs6000/rs6000.h:2495:42. Thanks. * config/rs6000/rs6000-internal.h (altivec_builtin_mask_for_load, builtin_mode_to_type[MAX_MACHINE_MODE][2]): Remove GTY(()). This changelog entry needs updating now. (And please check the rest as well.) Segher OK, updated now, and the rest were checked. Thanks. gcc/ChangeLog 2020-03-05 Bin Bin Lv * config/rs6000/rs6000-internal.h (altivec_builtin_mask_for_load, builtin_mode_to_type[MAX_MACHINE_MODE][2]): Remove the declaration. * config/rs6000/rs6000.h (altivec_builtin_mask_for_load, builtin_mode_to_type[MAX_MACHINE_MODE][2]): Add an extern GTY(()) declaration. * config/rs6000/rs6000.h (MAX_MACHINE_MODE): Include insn-modes.h. * config/rs6000/rs6000.c (altivec_builtin_mask_for_load, builtin_mode_to_type[MAX_MACHINE_MODE][2]): Remove the GTY(()) declaration and add the definition. --- gcc/config/rs6000/rs6000-internal.h | 2 -- gcc/config/rs6000/rs6000.c | 4 ++-- gcc/config/rs6000/rs6000.h | 4 3 files changed, 6 insertions(+), 4 deletions(-) diff --git a/gcc/config/rs6000/rs6000-internal.h b/gcc/config/rs6000/rs6000-internal.h index a23e956..d331b9e 100644 --- a/gcc/config/rs6000/rs6000-internal.h +++ b/gcc/config/rs6000/rs6000-internal.h @@ -187,7 +187,5 @@ extern bool rs6000_passes_long_double; extern bool rs6000_passes_vector; extern bool rs6000_returns_struct; extern bool cpu_builtin_p; -extern GTY(()) tree builtin_mode_to_type[MAX_MACHINE_MODE][2]; -extern GTY(()) tree altivec_builtin_mask_for_load; #endif diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c index 9910b27..0faf44b 100644 --- a/gcc/config/rs6000/rs6000.c +++ b/gcc/config/rs6000/rs6000.c @@ -99,7 +99,7 @@ #endif /* Support targetm.vectorize.builtin_mask_for_load. */ -GTY(()) tree altivec_builtin_mask_for_load; +tree altivec_builtin_mask_for_load; #ifdef USING_ELFOS_H /* Counter for labels which are to be placed in .fixup. */ @@ -196,7 +196,7 @@ enum reg_class rs6000_constraints[RS6000_CONSTRAINT_MAX]; int rs6000_vector_align[NUM_MACHINE_MODES]; /* Map selected modes to types for builtins. */ -GTY(()) tree builtin_mode_to_type[MAX_MACHINE_MODE][2]; +tree builtin_mode_to_type[MAX_MACHINE_MODE][2]; /* What modes to automatically generate reciprocal divide estimate (fre) and reciprocal sqrt (frsqrte) for. */ diff --git a/gcc/config/rs6000/rs6000.h b/gcc/config/rs6000/rs6000.h index 1697186..cd3d054 100644 --- a/gcc/config/rs6000/rs6000.h +++ b/gcc/config/rs6000/rs6000.h @@ -35,6 +35,8 @@ #include "config/rs6000/rs6000-modes.h" #endif +#include "insn-modes.h" + /* Definitions for the object file format. These are set at compile-time. */ @@ -2488,6 +2490,8 @@ enum rs6000_builtin_type_index extern GTY(()) tree rs6000_builtin_types[RS6000_BTI_MAX]; extern GTY(()) tree rs6000_builtin_decls[RS6000_BUILTIN_COUNT]; +extern GTY(()) tree builtin_mode_to_type[MAX_MACHINE_MODE][2]; +extern GTY(()) tree altivec_builtin_mask_for_load; #ifndef USED_FOR_TARGET /* A C structure for machine-specific, per-function data. -- 1.8.3.1
Re: [RFA/RFC] [tree-optimization/91890] [P1 Regression] Avoid clobbering useful location in Wrestrict code
On 3/4/20 4:49 PM, Jeff Law wrote: On Wed, 2020-03-04 at 09:22 -0700, Martin Sebor wrote: I don't remember why the code in -Wrestrict unconditionally overwrites the statement location rather than only when it's not available, but I do remember adding conditional code like in your patch in r277076 to deal with missing location on the statement. So either your fix or something like the hunk below might be the right solution (if we go with the code below, abstracting it into a utility function might be nice). So there's several chunks that are fairly similar to what you referenced in maybe_warn_pointless_strcmp. Factoring all of them into a single location is pretty easy. That also gives us a nice place where we can experiment with "does extraction of location information from the expression ever help". The answer is, it doesn't, at least not within our testsuite when run on x86_64. I'm hesitant to remove the code that extracts the location out of the expression, but could be convinced to do so. Thoughts? I tried disabling the two UNKNOWN_LOCATION workarounds in the strlen pass and that didn't make a difference to the test results I ran either (I only ran warning tests) so it looks like whatever reason it was added for doesn't exist anymore. I usually open bugs when I see something not working but the only bug I've raised that I can find that has to do with location is pr90735, for -Wreturn-local-addr. It has the background into the problem there. Grepping for gimple_set_location and UNKNOWN_LOCATION turns up just four instances in the middle-end: grep gimple_set_location gcc/*.c | grep UNKNOWN_) gcc/gimple-low.c: gimple_set_location (t.stmt, UNKNOWN_LOCATION); gcc/gimple-low.c: gimple_set_location (tmp_rs.stmt, UNKNOWN_LOCATION); gcc/tree-eh.c: gimple_set_location (stmt, UNKNOWN_LOCATION); gcc/tree-inline.c:gimple_set_location (stmt, UNKNOWN_LOCATION); Maybe something will jump out at you there. For GCC 10 I agree it's safest to leave it in place. For GCC 11, if we can't come up with a test case that shows it's needed, I'll look into removing it in stage 1. Martin
Re: [RFA/RFC] [tree-optimization/91890] [P1 Regression] Avoid clobbering useful location in Wrestrict code
On Wed, 2020-03-04 at 09:22 -0700, Martin Sebor wrote:
>
> I don't remember why the code in -Wrestrict unconditionally overwrites
> the statement location rather than only when it's not available, but
> I do remember adding conditional code like in your patch in r277076
> to deal with missing location on the statement. So either your fix
> or something like the hunk below might be the right solution (if we
> go with the code below, abstracting it into a utility function might
> be nice).
So there's several chunks that are fairly similar to what you referenced in
maybe_warn_pointless_strcmp. Factoring all of them into a single location is
pretty easy.
That also gives us a nice place where we can experiment with "does extraction of
location information from the expression ever help". The answer is, it doesn't,
at least not within our testsuite when run on x86_64.
I'm hesitant to remove the code that extracts the location out of the
expression,
but could be convinced to do so.
Thoughts?
Jeff
Fix location maybe_diag_overlap passes to diagnostics so that
diagnostic pragmas work better.
PR tree-optimization/91890
* gimple-ssa-warn-restrict.c (maybe_diag_overlap): Remove LOC argument.
Use gimple_or_expr_nonartificial_location.
(check_bounds_overlap): Drop LOC argument to maybe_diag_access_bounds.
Use gimple_or_expr_nonartificial_location.
* gimple.c (gimple_or_expr_nonartificial_location): New function.
* gimple.h (gimple_or_expr_nonartificial_location): Declare it.
* tree-ssa-strlen.c (maybe_warn_overflow): Use
gimple_or_expr_nonartificial_location.
(maybe_diag_stxncpy_trunc, handle_builtin_stxncpy_strncat): Likewise.
(maybe_warn_pointless_strcmp): Likewise.
* gcc.dg/pragma-diag-8.c: New test.
diff --git a/gcc/gimple-ssa-warn-restrict.c b/gcc/gimple-ssa-warn-restrict.c
index 2c582a670eb..5e7e5d41dbb 100644
--- a/gcc/gimple-ssa-warn-restrict.c
+++ b/gcc/gimple-ssa-warn-restrict.c
@@ -1692,10 +1692,11 @@ maybe_diag_overlap (location_t loc, gimple *call,
builtin_access )
has been issued, or would have been issued if DO_WARN had been true. */
static bool
-maybe_diag_access_bounds (location_t loc, gimple *call, tree func, int strict,
+maybe_diag_access_bounds (gimple *call, tree func, int strict,
const builtin_memref , offset_int wroff,
bool do_warn)
{
+ location_t loc = gimple_or_expr_nonartificial_location (call, ref.ptr);
const offset_int maxobjsize = ref.maxobjsize;
/* Check for excessive size first and regardless of warning options
@@ -1711,11 +1712,6 @@ maybe_diag_access_bounds (location_t loc, gimple *call,
tree func, int strict,
if (warn_stringop_overflow)
{
- if (EXPR_HAS_LOCATION (ref.ptr))
- loc = EXPR_LOCATION (ref.ptr);
-
- loc = expansion_point_location_if_in_system_header (loc);
-
if (ref.sizrange[0] == ref.sizrange[1])
return warning_at (loc, OPT_Wstringop_overflow_,
"%G%qD specified bound %wu "
@@ -1754,11 +1750,6 @@ maybe_diag_access_bounds (location_t loc, gimple *call,
tree func, int strict,
|| (ref.ref && TREE_NO_WARNING (ref.ref)))
return false;
- if (EXPR_HAS_LOCATION (ref.ptr))
-loc = EXPR_LOCATION (ref.ptr);
-
- loc = expansion_point_location_if_in_system_header (loc);
-
char rangestr[2][64];
if (ooboff[0] == ooboff[1]
|| (ooboff[0] != ref.offrange[0]
@@ -2018,9 +2009,6 @@ check_bounds_or_overlap (gimple *call, tree dst, tree
src, tree dstsize,
tree srcsize, bool bounds_only /* = false */,
bool do_warn /* = true */)
{
- location_t loc = gimple_nonartificial_location (call);
- loc = expansion_point_location_if_in_system_header (loc);
-
tree func = gimple_call_fndecl (call);
builtin_memref dstref (dst, dstsize);
@@ -2041,8 +2029,8 @@ check_bounds_or_overlap (gimple *call, tree dst, tree
src, tree dstsize,
/* Validate offsets to each reference before the access first to make
sure they are within the bounds of the destination object if its
size is known, or PTRDIFF_MAX otherwise. */
- if (maybe_diag_access_bounds (loc, call, func, strict, dstref, wroff,
do_warn)
- || maybe_diag_access_bounds (loc, call, func, strict, srcref, 0,
do_warn))
+ if (maybe_diag_access_bounds (call, func, strict, dstref, wroff, do_warn)
+ || maybe_diag_access_bounds (call, func, strict, srcref, 0, do_warn))
{
if (do_warn)
gimple_set_no_warning (call, true);
@@ -2066,6 +2054,7 @@ check_bounds_or_overlap (gimple *call, tree dst, tree
src, tree dstsize,
}
}
+ location_t loc = gimple_or_expr_nonartificial_location (call, dst);
if (operand_equal_p (dst, src, 0))
{
/* Issue -Wrestrict unless the pointers are null (those do
diff --git a/gcc/gimple.c b/gcc/gimple.c
index
Re: [Committed 4/4] IBM Z: zTPF: Include glibc-stdint.h
On Wed, 4 Mar 2020, Andreas Krebbel wrote: > Building a zTPF cross currently fails when building libstdc++ > complaining about the __UINTPTR_TYPE__ to be missing. > > Fixed by including the glibc-stdint.h header. To confirm: TPF provides a header, which uses the same types as glibc's does? If so, you can remove TPF from the list of targets for which bug 448 has yet to be resolved. (If TPF doesn't provide , you should set use_gcc_stdint=provide for TPF in config.gcc. If it provides it but using different types from glibc, you'll need to add a host-side header describing those types instead of using glibc-stdint.h.) -- Joseph S. Myers jos...@codesourcery.com
[committed][PR bootstrap/93962] Fix bootstrap issue on freebsd
PR 93962 reports a bootstrap failure on FreeBSD 11.3 due to a format warning.
Andrew Pinski recommended using std::abs rather than a naked abs. I
bootstrapped
and regression tested that on x86_64-linux-gnu.
I was unable to reproduce the failure in a FreeBSD VM. However Gerald (the
reporter) was able to confirm that using std::abs worked to fix the problem in
their builds.
I'm committing the change to std::abs based on Gerald's testing as well as my
own. I'm going to remove the regression marker, but keep the BZ open as Jakub
wants the CPP output to try and figure out what's going on at a deeper level.
Jeff
commit 20a235a8b443a81ea0ec6a10f260b119f2193a69
Author: Jeff Law
Date: Wed Mar 4 16:25:11 2020 -0700
Fix format warning which showed up on FreeBSD 11.3.
PR bootstrap/93962
* value-prof.c (dump_histogram_value): Use std::abs.
diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index 0df606a8211..5b2e4a83721 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,8 @@
+2020-03-04 Andrew Pinski
+
+ PR bootstrap/93962
+ * value-prof.c (dump_histogram_value): Use std::abs.
+
2020-03-04 Martin Sebor
PR tree-optimization/93986
diff --git a/gcc/value-prof.c b/gcc/value-prof.c
index 8e9f129708a..585b909096f 100644
--- a/gcc/value-prof.c
+++ b/gcc/value-prof.c
@@ -266,7 +266,7 @@ dump_histogram_value (FILE *dump_file, histogram_value hist)
if (hist->hvalue.counters)
{
fprintf (dump_file, " all: %" PRId64 "%s, values: ",
- abs ((int64_t) hist->hvalue.counters[0]),
+ std::abs ((int64_t) hist->hvalue.counters[0]),
hist->hvalue.counters[0] < 0
? " (values missing)": "");
for (unsigned i = 0; i < GCOV_TOPN_VALUES; i++)
Re: [PATCH v2 3/3] Keep .GCC.command.line sections of LTO objetcs.
On 3/4/20 6:33 PM, Jakub Jelinek wrote: On Wed, Mar 04, 2020 at 06:23:10PM +0100, Martin Liška wrote: On 3/4/20 4:25 PM, Egeyar Bagcioglu wrote: Thanks Richard. I do not have write-access to the GCC repo. I'd be glad if someone commits it for me. Can we please wait? I'm really convinced we do not want one another very similar functionality. I would definitely recommend to change the semantics of -frecord-gcc-switches to what the patchset does. That's why I added Nick as he's the author of the original implementation. Reasoning is provided in my previous email. Also, what is the reason for storing the option in a file, rather than say putting them into an environment variable from which the backend can pick it up? I needed to pass that information from one process to the other and I picked a way to do so. Moreover, passing files is what gcc does; therefore, it didn't seem unnatural to do so. Furthermore, gcc specs already has its way to create temporary file names so I didn't have to worry about what level of thread safety is acceptable. If you tell me why an environment variable is better, we can discuss and I might implement that too. I must say I don't really see advantages of this over -grecord-gcc-switches, recording all options looks very bloaty and will include mostly stuff you don't really care about (such as, e.g. the -I options without knowing what was the current directory when the source file has been compiled), on the other side will not record interesting options that -grecord-gcc-switches records (say, if some code is compiled with -march=native, this new option will record that, rather than what it really is), 1) -grecord-gcc-switches is quite similar to -frecord-gcc-switches and my first and main arguement about the latter is valid for the former: You cannot easily construct the right command line to invoke the same compilation from the switches. Switches are useful to check that the compiler is doing the expected. The command line is useful when you want the compiler to do the same thing again. 2) The output is exactly what gcc takes as command line to make that compilation. It is bloaty, surely, when so is what's provided to gcc to make it compile as desired. On the other hand, -grecord-gcc-switches is just much bloatier since it does not work alone. It's embedded in dwarf. 3) Many systems automatically strip dwarf, together with this information. I do not think there is an easy way to separate this information and keep it. Since this comes up often, I'd like to emphasize that the internal switches of gcc are different than the user-given options of gcc. The purpose of this new option is really to catch the user given options of gcc and only them, so much so that I mark it SWITCH_IGNORE within the specs to avoid it propagated to any child processes of gcc. This option is simply to retrieve how the call was made to GCC. Currently, there are no other options giving us this information. but I won't stand in a way unless such an option would be on by default. I totally agree that this should not be on by default. Best regards Egeyar
[pushed] c++: Fix [[no_unique_address]] and default mem-init [PR90432]
output_constructor doesn't like two consecutive entries with fields at the
same position; let's avoid adding the one for the empty field.
Tested x86_64-pc-linux-gnu, applying to trunk and 9.
gcc/cp/ChangeLog
2020-03-04 Jason Merrill
PR c++/90432
* init.c (perform_member_init): Don't do aggregate initialization of
empty field.
* constexpr.c (cx_check_missing_mem_inits): Don't enforce
initialization of empty field.
---
gcc/cp/constexpr.c | 3 +++
gcc/cp/init.c | 5 +
gcc/testsuite/g++.dg/cpp2a/no_unique_address3.C | 16
3 files changed, 24 insertions(+)
create mode 100644 gcc/testsuite/g++.dg/cpp2a/no_unique_address3.C
diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c
index c2d44605764..521c87f6210 100644
--- a/gcc/cp/constexpr.c
+++ b/gcc/cp/constexpr.c
@@ -831,6 +831,9 @@ cx_check_missing_mem_inits (tree ctype, tree body, bool
complain)
/* A flexible array can't be intialized here, so don't complain
that it isn't. */
continue;
+ if (DECL_SIZE (field) && integer_zerop (DECL_SIZE (field)))
+ /* An empty field doesn't need an initializer. */
+ continue;
ftype = strip_array_types (ftype);
if (type_has_constexpr_default_constructor (ftype))
{
diff --git a/gcc/cp/init.c b/gcc/cp/init.c
index 61ed3aa7e93..27623cf4db1 100644
--- a/gcc/cp/init.c
+++ b/gcc/cp/init.c
@@ -865,6 +865,11 @@ perform_member_init (tree member, tree init)
}
if (init == error_mark_node)
return;
+ if (DECL_SIZE (member) && integer_zerop (DECL_SIZE (member))
+ && !TREE_SIDE_EFFECTS (init))
+ /* Don't add trivial initialization of an empty base/field, as they
+ might not be ordered the way the back-end expects. */
+ return;
/* A FIELD_DECL doesn't really have a suitable lifetime, but
make_temporary_var_for_ref_to_temp will treat it as automatic and
set_up_extended_ref_temp wants to use the decl in a warning. */
diff --git a/gcc/testsuite/g++.dg/cpp2a/no_unique_address3.C
b/gcc/testsuite/g++.dg/cpp2a/no_unique_address3.C
new file mode 100644
index 000..07108b8b715
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp2a/no_unique_address3.C
@@ -0,0 +1,16 @@
+// PR c++/90432
+// { dg-do compile { target c++11 } }
+
+struct empty {};
+
+struct has_empty {
+ [[no_unique_address]] empty brace_or_equal_initialized{};
+};
+
+struct has_value {
+ int non_zero = 1;
+};
+
+struct pair : has_empty, has_value {};
+
+pair a;
base-commit: 10591cfe6cac200e926a73f3b8065147ce84
--
2.18.1
Re: [PATCH] c++: Backport PR90505 fix to 9
On 3/4/20 4:02 PM, Marek Polacek wrote:
While backporting our 90505 fix to 9 I noticed a bunch of concepts regressions.
Fortunately I think the following variant of the fix is safe and still fixes
the deduction issue. In 9, we want to reject 'auto' when tf_partial before
returning cp_build_qualified_type_real for TEMPLATE_TYPE_PARM and similar.
And then to fix 90505, return early before reducing the template level.
The difference is caused by the huge concepts merge, so it's tough to figure
out what specific change caused it.
Bootstrapped/regtested on x86_64-linux, ok for 9?
OK.
2020-03-04 Jason Merrill
Marek Polacek
PR c++/90505 - mismatch in template argument deduction.
* pt.c (tsubst): Don't reduce the template level of template
parameters when tf_partial.
* g++.dg/template/deduce4.C: New test.
* g++.dg/template/deduce5.C: New test.
* g++.dg/template/deduce6.C: New test.
* g++.dg/template/deduce7.C: New test.
diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c
index f233e78cc45..2de9036b647 100644
--- a/gcc/cp/pt.c
+++ b/gcc/cp/pt.c
@@ -14630,6 +14630,11 @@ tsubst (tree t, tree args, tsubst_flags_t complain,
tree in_decl)
about the template parameter in question. */
return t;
+ /* Like with 'auto', don't reduce the level of template parameters
+ to avoid mismatches when deducing their types. */
+ if (complain & tf_partial)
+ return t;
+
/* If we get here, we must have been looking at a parm for a
more deeply nested template. Make a new version of this
template parameter, but with a lower level. */
diff --git a/gcc/testsuite/g++.dg/template/deduce4.C
b/gcc/testsuite/g++.dg/template/deduce4.C
new file mode 100644
index 000..e2c165dc788
--- /dev/null
+++ b/gcc/testsuite/g++.dg/template/deduce4.C
@@ -0,0 +1,17 @@
+// PR c++/90505 - mismatch in template argument deduction.
+// { dg-do compile }
+
+template
+struct S {
+ template
+ static void foo(V) { }
+
+ void bar () { foo(10); }
+};
+
+void
+test ()
+{
+ S s;
+ s.bar ();
+}
diff --git a/gcc/testsuite/g++.dg/template/deduce5.C
b/gcc/testsuite/g++.dg/template/deduce5.C
new file mode 100644
index 000..9d382bfe03a
--- /dev/null
+++ b/gcc/testsuite/g++.dg/template/deduce5.C
@@ -0,0 +1,17 @@
+// PR c++/90505 - mismatch in template argument deduction.
+// { dg-do compile { target c++11 } }
+
+template
+struct S {
+ template
+ static void foo(U) { }
+
+ void bar () { foo(10); }
+};
+
+void
+test ()
+{
+ S s;
+ s.bar ();
+}
diff --git a/gcc/testsuite/g++.dg/template/deduce6.C
b/gcc/testsuite/g++.dg/template/deduce6.C
new file mode 100644
index 000..8fee6124f5a
--- /dev/null
+++ b/gcc/testsuite/g++.dg/template/deduce6.C
@@ -0,0 +1,17 @@
+// PR c++/90505 - mismatch in template argument deduction.
+// { dg-do compile { target c++11 } }
+
+template
+struct S {
+ template
+ static void foo(V) { }
+
+ void bar () { foo<>(10); }
+};
+
+void
+test ()
+{
+ S s;
+ s.bar ();
+}
diff --git a/gcc/testsuite/g++.dg/template/deduce7.C
b/gcc/testsuite/g++.dg/template/deduce7.C
new file mode 100644
index 000..fbc28e5150d
--- /dev/null
+++ b/gcc/testsuite/g++.dg/template/deduce7.C
@@ -0,0 +1,10 @@
+// PR c++/90505 - mismatch in template argument deduction.
+// { dg-do compile { target c++11 } }
+
+template class a {
+ using b = int;
+ using c = int;
+ b d;
+ void e() { g(d); }
+ template static void g(f...);
+};
Re: [PATCH] avoid user-constructible types in reshape_init_array (PR 90938)
On 3/4/20 4:14 PM, Martin Sebor wrote:
On 3/4/20 10:41 AM, Jason Merrill wrote:
On 2/14/20 3:06 PM, Martin Sebor wrote:
On 2/13/20 3:59 PM, Jason Merrill wrote:
On 2/12/20 9:21 PM, Martin Sebor wrote:
On 2/11/20 5:28 PM, Jason Merrill wrote:
On 2/11/20 9:00 PM, Martin Sebor wrote:
r270155, committed in GCC 9, introduced a transformation that strips
redundant trailing zero initializers from array initializer lists in
order to support string literals as template arguments.
The transformation neglected to consider the case of array elements
of trivial class types with user-defined conversion ctors and either
defaulted or deleted default ctors. (It didn't occur to me that
those qualify as trivial types despite the user-defined ctors.) As
a result, some valid initialization expressions are rejected when
the explicit zero-initializers are dropped in favor of the (deleted)
default ctor,
Hmm, a type with only a deleted default constructor is not
trivial, that should have been OK already.
For Marek's test case:
struct A { A () == delete; A (int) = delete; };
trivial_type_p() returns true (as does __is_trivial (A) in both GCC
and Clang).
[class.prop] says that
A trivial class is a class that is trivially copyable and has one
or more default constructors (10.3.4.1), all of which are either
trivial or deleted and at least one of which is not deleted.
That sounds like A above is not trivial because it doesn't have
at least one default ctor that's not deleted, but both GCC and
Clang say it is. What am I missing? Is there some other default
constructor hiding in there that I don't know about?
and others are eliminated in favor of the defaulted
ctor instead of invoking a user-defined conversion ctor, leading to
wrong code.
This seems like a bug in type_initializer_zero_p; it shouldn't
treat 0 as a zero initializer for any class.
That does fix it, and it seems like the right solution to me as well.
Thanks for the suggestion. I'm a little unsure about the condition
I put in place though.
Attached is an updated patch rested on x86_64-linux.
- if (sized_array_p && trivial_type_p (elt_type))
+ if (sized_array_p
+ && trivial_type_p (elt_type)
+ && !TYPE_NEEDS_CONSTRUCTING (elt_type))
Do we still need this change? If so, please add a comment about the
trivial_type_p bug.
The change isn't needed with my patch as it was
Let's go ahead and commit the type_initializer_zero_p hunk on both
trunk and 9 now to fix this bug for 9.3,
Just to be sure: You mean the patch at this link, correct?
https://gcc.gnu.org/ml/gcc-patches/2020-02/msg00742.html
(Without the change above that's not needed.)
Correct.
and pursue the process_init_constructor_array patch for later.
Ack.
Martin
[committed] analyzer: validate region subclasses
This patch converts region::validate to a vfunc, implementing
additional checking per subclass: verifying that various
region_id fields within map_region, array_region, stack_region and
root_region are valid, rather than just those within the base class.
Doing so caught bugs earlier in follow-up work I have on
canonicalization and purging of region_model.
Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu.
Pushed to master as 3c1645a379e405c7ce33060846fa424373b1f5f4.
gcc/analyzer/ChangeLog:
* region-model.cc (region::validate): Convert model param from ptr
to reference. Update comment to reflect that it's now a vfunc.
(map_region::validate): New vfunc implementation.
(array_region::validate): New vfunc implementation.
(stack_region::validate): New vfunc implementation.
(root_region::validate): New vfunc implementation.
(region_model::validate): Pass a reference rather than a pointer
to the region::validate vfunc.
* region-model.h (region::validate): Make virtual. Convert model
param from ptr to reference.
(map_region::validate): New vfunc decl.
(array_region::validate): New vfunc decl.
(stack_region::validate): New vfunc decl.
(root_region::validate): New vfunc decl.
---
gcc/analyzer/region-model.cc | 70
gcc/analyzer/region-model.h | 8 -
2 files changed, 70 insertions(+), 8 deletions(-)
diff --git a/gcc/analyzer/region-model.cc b/gcc/analyzer/region-model.cc
index 6813117968f..0ceeab45a02 100644
--- a/gcc/analyzer/region-model.cc
+++ b/gcc/analyzer/region-model.cc
@@ -1360,21 +1360,23 @@ region::dump_child_label (const region_model ,
}
}
-/* Assert that this object is valid. */
+/* Base implementation of region::validate vfunc.
+ Assert that the fields of "region" are valid; subclasses should
+ chain up their implementation to this one. */
void
-region::validate (const region_model *model) const
+region::validate (const region_model ) const
{
- m_parent_rid.validate (*model);
- m_sval_id.validate (*model);
+ m_parent_rid.validate (model);
+ m_sval_id.validate (model);
unsigned i;
region_id *view_rid;
FOR_EACH_VEC_ELT (m_view_rids, i, view_rid)
{
gcc_assert (!view_rid->null_p ());
- view_rid->validate (*model);
+ view_rid->validate (model);
}
- m_active_view_rid.validate (*model);
+ m_active_view_rid.validate (model);
}
/* Apply MAP to svalue_ids to this region. This updates the value
@@ -1599,6 +1601,21 @@ map_region::print_fields (const region_model ,
pp_string (pp, "}");
}
+/* Implementation of region::validate vfunc for map_region. */
+
+void
+map_region::validate (const region_model ) const
+{
+ region::validate (model);
+ for (map_t::iterator iter = m_map.begin ();
+ iter != m_map.end ();
+ ++iter)
+{
+ region_id child_rid = (*iter).second;
+ child_rid.validate (model);
+}
+}
+
/* Implementation of region::dump_dot_to_pp vfunc for map_region. */
void
@@ -2268,6 +2285,21 @@ array_region::print_fields (const region_model ,
pp_string (pp, "}");
}
+/* Implementation of region::validate vfunc for array_region. */
+
+void
+array_region::validate (const region_model ) const
+{
+ region::validate (model);
+ for (map_t::iterator iter = m_map.begin ();
+ iter != m_map.end ();
+ ++iter)
+{
+ region_id child_rid = (*iter).second;
+ child_rid.validate (model);
+}
+}
+
/* Implementation of region::dump_dot_to_pp vfunc for array_region. */
void
@@ -2544,6 +2576,18 @@ stack_region::dump_child_label (const region_model
,
pp_printf (pp, "frame for %qs: ", function_name (fun));
}
+/* Implementation of region::validate vfunc for stack_region. */
+
+void
+stack_region::validate (const region_model ) const
+{
+ region::validate (model);
+ int i;
+ region_id *frame_rid;
+ FOR_EACH_VEC_ELT (m_frame_rids, i, frame_rid)
+m_frame_rids[i].validate (model);
+}
+
/* Push FRAME_RID (for a frame_region) onto this stack. */
void
@@ -2834,6 +2878,18 @@ root_region::print_fields (const region_model ,
// TODO
}
+/* Implementation of region::validate vfunc for root_region. */
+
+void
+root_region::validate (const region_model ) const
+{
+ region::validate (model);
+ m_stack_rid.validate (model);
+ m_globals_rid.validate (model);
+ m_code_rid.validate (model);
+ m_heap_rid.validate (model);
+}
+
/* Implementation of region::dump_child_label vfunc for root_region. */
void
@@ -3714,7 +3770,7 @@ region_model::validate () const
unsigned i;
region *r;
FOR_EACH_VEC_ELT (m_regions, i, r)
-r->validate (this);
+r->validate (*this);
// TODO: anything else?
diff --git a/gcc/analyzer/region-model.h b/gcc/analyzer/region-model.h
index 6d49f00cfe3..c782e93a83d 100644
--- a/gcc/analyzer/region-model.h
+++ b/gcc/analyzer/region-model.h
@@ -891,7 +891,7 @@ public:
[committed] analyzer: add regression test for fixed ICE [PR94028]
The C++ reproducer for PR analyzer/94028 generates a similar ICE
to that of the Fortran reproducer for PR analyzer/93993 and, like
it, was fixed by r10-7023-g3d66e153b40ed000af30a9e569a05f34d5d576aa.
This patch adds the C++ reproducer as a regression test.
Successfully regrtested on x86_64-pc-linux-gnu.
Pushed to master as 4ac3eb5c5f157bea22b5ae34b0df254d729dac25.
gcc/testsuite/ChangeLog:
PR analyzer/94028
* g++.dg/analyzer/pr94028.C: New test.
---
gcc/testsuite/g++.dg/analyzer/pr94028.C | 36 +
1 file changed, 36 insertions(+)
create mode 100644 gcc/testsuite/g++.dg/analyzer/pr94028.C
diff --git a/gcc/testsuite/g++.dg/analyzer/pr94028.C
b/gcc/testsuite/g++.dg/analyzer/pr94028.C
new file mode 100644
index 000..0a222d1b991
--- /dev/null
+++ b/gcc/testsuite/g++.dg/analyzer/pr94028.C
@@ -0,0 +1,36 @@
+void *calloc (__SIZE_TYPE__, __SIZE_TYPE__);
+
+struct B
+{
+ B (short);
+ int cls;
+} k (0);
+
+void d (int);
+
+enum e {} i;
+
+struct j
+{
+ void *operator new (__SIZE_TYPE__ b)
+ {
+return calloc (b, sizeof (int)); // { dg-warning "leak" }
+ }
+ j (B *, int)
+ {
+ } // { dg-warning "leak" }
+};
+
+j *
+f (B * b, int h, bool)
+{
+ d (b->cls);
+ return new j (b, h); // { dg-warning "leak" }
+}
+
+void
+m ()
+{
+ if (i)
+f (, 0, false);
+}
--
2.21.0
Re: GLIBC libmvec status
‐‐‐ Original Message ‐‐‐ On Monday, March 2, 2020 12:14 PM, Bill Schmidt wrote: > On 3/2/20 11:10 AM, Tulio Magno Quites Machado Filho wrote: > > > Bill Schmidt writes: > > > > > One tiny nit on the document: For the "b" value, let's just say > > > "VSX" rather than > > > "VSX as defined in PowerISA v2.07)." We will plan to only change > > > values in case > > > a different vector length is defined in future. > > > > That change would have more implications: all libmvec functions would have > > to > > work on Power ISA v2.06 HW too. But half of the functions do use v2.07 > > instructions now. > > Ah, I see. Well, then language such as "VSX defined at least at the level of > PowerISA v2.07" would be appropriate. We want to define a minimum subset > without > further implied constraint. (Higher levels can be handled with ifunc without > needing to reference this in the ABI, as previously discussed.) > Changed description of 'b' ISA to exactly the quoted sentence above. Bert.
Re: [PATCH] issue -Walloca even when alloca is a system header macro [PR94004]
On Wed, Mar 04, 2020 at 02:06:58PM -0700, Martin Sebor wrote: > +#ifndef alloca > +// Simulate a definition in a system header. > +# 13 "/usr/include/alloca.h" > +# define alloca(n) __builtin_alloca (n) > +# 15 "Walloca-larger-than-3.c" > +#endif This isn't correct simulation of definition in a system header. Comment out the #include to verify. The third line should be # 1 "/usr/include/alloca.h" 1 3 (or 1 3 4 at the end) and fifth line should be # 15 "Walloca-larger-than-3.c" 2 1 means entering a header, 3 means system header (4 is about implicit extern "C") and 2 means returning back to previous source. Jakub
Re: [PATCH] avoid user-constructible types in reshape_init_array (PR 90938)
On 3/4/20 10:41 AM, Jason Merrill wrote:
On 2/14/20 3:06 PM, Martin Sebor wrote:
On 2/13/20 3:59 PM, Jason Merrill wrote:
On 2/12/20 9:21 PM, Martin Sebor wrote:
On 2/11/20 5:28 PM, Jason Merrill wrote:
On 2/11/20 9:00 PM, Martin Sebor wrote:
r270155, committed in GCC 9, introduced a transformation that strips
redundant trailing zero initializers from array initializer lists in
order to support string literals as template arguments.
The transformation neglected to consider the case of array elements
of trivial class types with user-defined conversion ctors and either
defaulted or deleted default ctors. (It didn't occur to me that
those qualify as trivial types despite the user-defined ctors.) As
a result, some valid initialization expressions are rejected when
the explicit zero-initializers are dropped in favor of the (deleted)
default ctor,
Hmm, a type with only a deleted default constructor is not trivial,
that should have been OK already.
For Marek's test case:
struct A { A () == delete; A (int) = delete; };
trivial_type_p() returns true (as does __is_trivial (A) in both GCC
and Clang).
[class.prop] says that
A trivial class is a class that is trivially copyable and has one
or more default constructors (10.3.4.1), all of which are either
trivial or deleted and at least one of which is not deleted.
That sounds like A above is not trivial because it doesn't have
at least one default ctor that's not deleted, but both GCC and
Clang say it is. What am I missing? Is there some other default
constructor hiding in there that I don't know about?
and others are eliminated in favor of the defaulted
ctor instead of invoking a user-defined conversion ctor, leading to
wrong code.
This seems like a bug in type_initializer_zero_p; it shouldn't
treat 0 as a zero initializer for any class.
That does fix it, and it seems like the right solution to me as well.
Thanks for the suggestion. I'm a little unsure about the condition
I put in place though.
Attached is an updated patch rested on x86_64-linux.
- if (sized_array_p && trivial_type_p (elt_type))
+ if (sized_array_p
+ && trivial_type_p (elt_type)
+ && !TYPE_NEEDS_CONSTRUCTING (elt_type))
Do we still need this change? If so, please add a comment about the
trivial_type_p bug.
The change isn't needed with my patch as it was
Let's go ahead and commit the type_initializer_zero_p hunk on both trunk
and 9 now to fix this bug for 9.3,
Just to be sure: You mean the patch at this link, correct?
https://gcc.gnu.org/ml/gcc-patches/2020-02/msg00742.html
(Without the change above that's not needed.)
and pursue the
process_init_constructor_array patch for later.
Ack.
Martin
[PATCH] issue -Walloca even when alloca is a system header macro [PR94004]
A 2017 change that propagated call location to GIMPLE statements
had the unexpected (though not incorrect) effect of setting
the location on calls to __builtin_alloca. When such calls are
the result of the expansion of the alloca macro defined in
a system header like , the change prevents -Walloca and
-Walloca-larger-than
warnings from being issued unless -Wsystem-headers is also used,
effectively defeating the point of the first two warnings in most
code.
The attached patch changes -Walloca to use the location of the point
of the macro expansion rather than that of its definition, restoring
the intended behavior.
In addition, to make the warnings (mainly -Wvla) in inlined code
easier to debug, the patch also arranges for the warnings to include
their inlining context.
Tested on x86_64-linux.
Martin
PR middle-end/94004 - missing -Walloca on calls to alloca due to -Wno-system-headers
gcc/testsuite/ChangeLog:
PR middle-end/94004
* gcc.dg/Walloca-larger-than-3.c: New test.
* gcc.dg/Wvla-larger-than-4.c: New test.
gcc/ChangeLog:
PR middle-end/94004
* gimple-ssa-warn-alloca.c (pass_walloca::execute): Issue warnings
even for alloca calls resulting from system macro expansion.
Include inlining context in all warnings.
diff --git a/gcc/gimple-ssa-warn-alloca.c b/gcc/gimple-ssa-warn-alloca.c
index dfe40c9c02a..9e80e5dbbd9 100644
--- a/gcc/gimple-ssa-warn-alloca.c
+++ b/gcc/gimple-ssa-warn-alloca.c
@@ -510,11 +510,12 @@ pass_walloca::execute (function *fun)
gsi_next ())
{
gimple *stmt = gsi_stmt (si);
- location_t loc = gimple_location (stmt);
-
if (!gimple_alloca_call_p (stmt))
continue;
+ location_t loc = gimple_nonartificial_location (stmt);
+ loc = expansion_point_location_if_in_system_header (loc);
+
const bool is_vla
= gimple_call_alloca_for_var_p (as_a (stmt));
@@ -528,7 +529,7 @@ pass_walloca::execute (function *fun)
}
else if (warn_alloca)
{
- warning_at (loc, OPT_Walloca, "use of %");
+ warning_at (loc, OPT_Walloca, "%Guse of %", stmt);
continue;
}
else if (warn_alloca_limit < 0)
@@ -564,10 +565,12 @@ pass_walloca::execute (function *fun)
{
auto_diagnostic_group d;
if (warning_at (loc, wcode,
-is_vla ? G_("argument to variable-length "
- "array may be too large")
-: G_("argument to % may be too "
- "large"))
+(is_vla
+ ? G_("%Gargument to variable-length "
+ "array may be too large")
+ : G_("%Gargument to % may be too "
+ "large")),
+stmt)
&& t.limit != 0)
{
print_decu (t.limit, buff);
@@ -582,47 +585,57 @@ pass_walloca::execute (function *fun)
{
auto_diagnostic_group d;
if (warning_at (loc, wcode,
-is_vla ? G_("argument to variable-length"
- " array is too large")
-: G_("argument to % is too large"))
+(is_vla
+ ? G_("%Gargument to variable-length"
+ " array is too large")
+ : G_("%Gargument to % is too large")),
+stmt)
&& t.limit != 0)
{
print_decu (t.limit, buff);
inform (loc, "limit is %wu bytes, but argument is %s",
- is_vla ? warn_vla_limit : adjusted_alloca_limit,
- buff);
+ is_vla ? warn_vla_limit : adjusted_alloca_limit,
+ buff);
}
}
break;
case ALLOCA_BOUND_UNKNOWN:
warning_at (loc, wcode,
- is_vla ? G_("variable-length array bound is unknown")
- : G_("% bound is unknown"));
+ (is_vla
+ ? G_("%Gvariable-length array bound is unknown")
+ : G_("%G% bound is unknown")),
+ stmt);
break;
case ALLOCA_UNBOUNDED:
warning_at (loc, wcode,
- is_vla ? G_("unbounded use of variable-length array")
- : G_("unbounded use of %"));
+ (is_vla
+ ? G_("%Gunbounded use of variable-length array")
+ : G_("%Gunbounded use of %")),
+ stmt);
break;
case ALLOCA_IN_LOOP:
gcc_assert (!is_vla);
- warning_at (loc, wcode, "use of % within a loop");
+ warning_at (loc, wcode,
+ "%Guse of % within a loop", stmt);
break;
case ALLOCA_CAST_FROM_SIGNED:
gcc_assert (invalid_casted_type != NULL_TREE);
warning_at (loc, wcode,
- is_vla ? G_("argument to variable-length array "
- "may be too large due to "
- "conversion from %qT to %qT")
- : G_("argument to % may be too large "
- "due to conversion from %qT to %qT"),
- invalid_casted_type, size_type_node);
+ (is_vla
+ ? G_("%Gargument to variable-length array "
+"may be too large due to "
+"conversion from %qT to %qT")
+ : G_("%Gargument to % may be too large "
+"due to conversion from %qT to %qT")),
+ stmt, invalid_casted_type, size_type_node);
break;
case ALLOCA_ARG_IS_ZERO:
warning_at (loc, wcode,
- is_vla ? G_("argument to variable-length array "
- "is zero")
- : G_("argument to % is zero"));
+
[PATCH] c++: Backport PR90505 fix to 9
While backporting our 90505 fix to 9 I noticed a bunch of concepts regressions.
Fortunately I think the following variant of the fix is safe and still fixes
the deduction issue. In 9, we want to reject 'auto' when tf_partial before
returning cp_build_qualified_type_real for TEMPLATE_TYPE_PARM and similar.
And then to fix 90505, return early before reducing the template level.
The difference is caused by the huge concepts merge, so it's tough to figure
out what specific change caused it.
Bootstrapped/regtested on x86_64-linux, ok for 9?
2020-03-04 Jason Merrill
Marek Polacek
PR c++/90505 - mismatch in template argument deduction.
* pt.c (tsubst): Don't reduce the template level of template
parameters when tf_partial.
* g++.dg/template/deduce4.C: New test.
* g++.dg/template/deduce5.C: New test.
* g++.dg/template/deduce6.C: New test.
* g++.dg/template/deduce7.C: New test.
diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c
index f233e78cc45..2de9036b647 100644
--- a/gcc/cp/pt.c
+++ b/gcc/cp/pt.c
@@ -14630,6 +14630,11 @@ tsubst (tree t, tree args, tsubst_flags_t complain,
tree in_decl)
about the template parameter in question. */
return t;
+ /* Like with 'auto', don't reduce the level of template parameters
+ to avoid mismatches when deducing their types. */
+ if (complain & tf_partial)
+ return t;
+
/* If we get here, we must have been looking at a parm for a
more deeply nested template. Make a new version of this
template parameter, but with a lower level. */
diff --git a/gcc/testsuite/g++.dg/template/deduce4.C
b/gcc/testsuite/g++.dg/template/deduce4.C
new file mode 100644
index 000..e2c165dc788
--- /dev/null
+++ b/gcc/testsuite/g++.dg/template/deduce4.C
@@ -0,0 +1,17 @@
+// PR c++/90505 - mismatch in template argument deduction.
+// { dg-do compile }
+
+template
+struct S {
+ template
+ static void foo(V) { }
+
+ void bar () { foo(10); }
+};
+
+void
+test ()
+{
+ S s;
+ s.bar ();
+}
diff --git a/gcc/testsuite/g++.dg/template/deduce5.C
b/gcc/testsuite/g++.dg/template/deduce5.C
new file mode 100644
index 000..9d382bfe03a
--- /dev/null
+++ b/gcc/testsuite/g++.dg/template/deduce5.C
@@ -0,0 +1,17 @@
+// PR c++/90505 - mismatch in template argument deduction.
+// { dg-do compile { target c++11 } }
+
+template
+struct S {
+ template
+ static void foo(U) { }
+
+ void bar () { foo(10); }
+};
+
+void
+test ()
+{
+ S s;
+ s.bar ();
+}
diff --git a/gcc/testsuite/g++.dg/template/deduce6.C
b/gcc/testsuite/g++.dg/template/deduce6.C
new file mode 100644
index 000..8fee6124f5a
--- /dev/null
+++ b/gcc/testsuite/g++.dg/template/deduce6.C
@@ -0,0 +1,17 @@
+// PR c++/90505 - mismatch in template argument deduction.
+// { dg-do compile { target c++11 } }
+
+template
+struct S {
+ template
+ static void foo(V) { }
+
+ void bar () { foo<>(10); }
+};
+
+void
+test ()
+{
+ S s;
+ s.bar ();
+}
diff --git a/gcc/testsuite/g++.dg/template/deduce7.C
b/gcc/testsuite/g++.dg/template/deduce7.C
new file mode 100644
index 000..fbc28e5150d
--- /dev/null
+++ b/gcc/testsuite/g++.dg/template/deduce7.C
@@ -0,0 +1,10 @@
+// PR c++/90505 - mismatch in template argument deduction.
+// { dg-do compile { target c++11 } }
+
+template class a {
+ using b = int;
+ using c = int;
+ b d;
+ void e() { g(d); }
+ template static void g(f...);
+};
Re: [PATCH] [rs6000] Rewrite the declaration of a variable
Hi! On Wed, Mar 04, 2020 at 06:35:23PM +0800, Kewen.Lin wrote: > Another try seems to move it into #ifndef USED_FOR_TARGET hunk. > Since "typedef union section section" is guard by #ifndef USED_FOR_TARGET > in coretypes.h. It can make them consistent. Yes, that should work, good idea. Segher
Re: [PATCH] [rs6000] Rewrite the declaration of a variable
Hi! On Wed, Mar 04, 2020 at 03:24:52PM +0800, binbin wrote: > >>+extern union GTY(()) section *toc_section; > > > >Why does this add "union"? > > If "union" is not added, it reports error showing unknown type name > ‘section’ > in file included from ../../host-powerpc64le-unknown-linux-gnu/gcc/tm.h:25, > from ../.././libgcc/generic-morestack-thread.c:29: > extern GTY(()) section *toc_section. This means that there is no declaration of "section" before this. It is not correct to implicitly declare the union like this: the scope of that declaration is the function declaration, making this union a different type from any later uses of it! (This problem is more commonly seen with struct, but it is exactly the same with union). You should make sure "section" is declared before this, instead. Segher
Re: [PATCH] Handle 'omp declare target' attribute set for both OpenACC and OpenMP 'target' [PR89433, PR93465] (was: [committed] PR89433 "Repeated use of the OpenACC 'routine' directive")
On Wed, Mar 04, 2020 at 08:27:10PM +0100, Thomas Schwinge wrote: > ... which as of PR89433 commit b48f44bf77a39fefc238a16cf1225c6464c82406 causes > an ICE. Not sure if this is actually supposed to be valid or invalid code. > Until the interactions between OpenACC and OpenMP 'target' get defined > properly, make this a compile-time error. > > gcc/ > PR middle-end/89433 > PR middle-end/93465 > * omp-general.c (oacc_verify_routine_clauses): Diagnose if > "#pragma omp declare target" has also been applied. > gcc/testsuite/ > PR middle-end/89433 > PR middle-end/93465 > * c-c++-common/goacc-gomp/pr93465-1.c: New file. Ok for trunk. Jakub
Re: [committed] analyzer: detect malloc, free, calloc within "std" [PR93959]
On Wed, 04 Mar 2020 11:16:35 -0500
David Malcolm wrote:
> On Wed, 2020-03-04 at 11:05 -0500, Marek Polacek wrote:
> > On Wed, Mar 04, 2020 at 04:54:54PM +0100, Bernhard Reutner-Fischer
> > wrote:
> > > On Mon, 2 Mar 2020 16:48:26 -0500
> > > David Malcolm wrote:
> > >
> > > > +static inline bool
> > > > +is_std_function_p (const_tree fndecl)
> > > > +{
> > > > + tree name_decl = DECL_NAME (fndecl);
> > > > + if (!name_decl)
> > > > +return false;
> > > > + if (!DECL_CONTEXT (fndecl))
> > > > +return false;
> > > > + if (TREE_CODE (DECL_CONTEXT (fndecl)) != NAMESPACE_DECL)
> > > > +return false;
> > > > + tree ns = DECL_CONTEXT (fndecl);
> > > > + if (!(DECL_CONTEXT (ns) == NULL_TREE
> > > > + || TREE_CODE (DECL_CONTEXT (ns)) == TRANSLATION_UNIT_DECL))
> > > > +return false;
> > > > + if (!DECL_NAME (ns))
> > > > +return false;
> > > > + return id_equal ("std", DECL_NAME (ns));
> > > > +}
> > >
> > > Sounds a bit elaborate, doesn't?
> > > I hope this is optimized to
> > >
> > > static inline bool
> > > is_std_function_p (const_tree fndecl)
> > > {
> > > tree name_decl = DECL_NAME (fndecl);
> > > if (!name_decl)
> > > return false;
Make the above
if (!DECL_NAME (fndecl))
return false;
> > > tree ns = DECL_CONTEXT (fndecl);
> > > if (!ns)
> > > return false;
> > > if (TREE_CODE (ns) != NAMESPACE_DECL)
> > > return false;
> > > if (!(DECL_CONTEXT (ns) == NULL_TREE
> > > || TREE_CODE (DECL_CONTEXT (ns)) == TRANSLATION_UNIT_DECL))
> > > return false;
> > > if (!DECL_NAME (ns))
> > > return false;
> > > return id_equal ("std", DECL_NAME (ns));
> > > }
> > >
> > > isn't "std" spelled out std_identifier() and is an identifier, i.e.
> > > return DECL_NAME (ns) == std_identifier; ?
> >
> > We have decl_in_std_namespace_p for that.
>
> Thanks. Indeed the comment to the function quoted above which wasn't
> quoted reads:
>
> /* Return true if FNDECL is within the namespace "std".
>Compare with cp/typeck.c: decl_in_std_namespace_p, but this doesn't
>rely on being the C++ FE (or handle inline namespaces inside of std). */
>
> since as noted in my reply to Bernhard the analyzer runs in the middle-
> end and thus can't rely on features of a specific frontend.
>
> Is there a better way to do this?
yea probably not (my tree is obviously outdated, i still had
std_identifier lying around here, sorry for that!)
Still, to the bikeshedding question above, does CSE or whatever arrive
at the same or wouldn't we want to phrase it more concisely as i tried
to imply?
thanks,
Re: [PATCH v2 3/3] Keep .GCC.command.line sections of LTO objetcs.
On 3/4/20 6:23 PM, Martin Liška wrote: On 3/4/20 4:25 PM, Egeyar Bagcioglu wrote: Thanks Richard. I do not have write-access to the GCC repo. I'd be glad if someone commits it for me. Can we please wait? I'm really convinced we do not want one another very similar functionality. I am sorry. I thought Richard's comment was only about the third patch, which is also necessary for the current -frecord-gcc-switches. Therefore, my reply was only for that patch as well. Regards Egeyar I would definitely recommend to change the semantics of -frecord-gcc-switches to what the patchset does. That's why I added Nick as he's the author of the original implementation. Reasoning is provided in my previous email. Martin
[PATCH] Handle 'omp declare target' attribute set for both OpenACC and OpenMP 'target' [PR89433, PR93465] (was: [committed] PR89433 "Repeated use of the OpenACC 'routine' directive")
-
Mentor Graphics (Deutschland) GmbH, Arnulfstraße 201, 80634 München / Germany
Registergericht München HRB 106955, Geschäftsführer: Thomas Heurung, Alexander
Walter
--- Begin Message ---
Hi!
On 2019-05-17T21:16:57+0200, I wrote:
> Now committed to trunk in [...]
> r271345 "[PR89433] Repeated use of the C/C++ OpenACC 'routine'
> directive"
> --- a/gcc/omp-general.c
> +++ b/gcc/omp-general.c
> @@ -610,11 +610,14 @@ oacc_set_fn_attrib (tree fn, tree clauses, vec
> *args)
>
> /* Verify OpenACC routine clauses.
>
> + Returns 0 if FNDECL should be marked with an OpenACC 'routine' directive,
> 1
> + if it has already been marked in compatible way, and -1 if incompatible.
> Upon returning, the chain of clauses will contain exactly one clause
> specifying the level of parallelism. */
> + tree attr
> += lookup_attribute ("omp declare target", DECL_ATTRIBUTES (fndecl));
> + if (attr != NULL_TREE)
> +{
> + /* If a "#pragma acc routine" has already been applied, just verify
> + this one for compatibility. */
> + /* Collect previous directive's clauses. */
> + tree c_level_p = NULL_TREE;
> + for (tree c = TREE_VALUE (attr); c; c = OMP_CLAUSE_CHAIN (c))
> + switch (OMP_CLAUSE_CODE (c))
> + {
> + case OMP_CLAUSE_GANG:
> + case OMP_CLAUSE_WORKER:
> + case OMP_CLAUSE_VECTOR:
> + case OMP_CLAUSE_SEQ:
> + gcc_checking_assert (c_level_p == NULL_TREE);
> + c_level_p = c;
> + break;
> + default:
> + gcc_unreachable ();
> + }
> + gcc_checking_assert (c_level_p != NULL_TREE);
As documented in , this triggers an ICE if
the 'omp declare target' attribute had already been set for '#pragma omp
declare target'. OK to deal with this situation as in the patch
attached, "Handle 'omp declare target' attribute set for both OpenACC and
OpenMP 'target' [PR89433, PR93465]"? If approving this patch, please
respond with "Reviewed-by: NAME " so that your effort will be
recorded in the commit log, see .
That's probably not worth backporting (a variant of this) to release
branches -- where also other such mixed OpenACC/OpenMP code is silently
accepted, with unclear semantics.
Grüße
Thomas
From 3f8e048f5f8d1eabde642c1c146114027bb44e79 Mon Sep 17 00:00:00 2001
From: Thomas Schwinge
Date: Wed, 4 Mar 2020 17:58:33 +0100
Subject: [PATCH] Handle 'omp declare target' attribute set for both OpenACC
and OpenMP 'target' [PR89433, PR93465]
... which as of PR89433 commit b48f44bf77a39fefc238a16cf1225c6464c82406 causes
an ICE. Not sure if this is actually supposed to be valid or invalid code.
Until the interactions between OpenACC and OpenMP 'target' get defined
properly, make this a compile-time error.
gcc/
PR middle-end/89433
PR middle-end/93465
* omp-general.c (oacc_verify_routine_clauses): Diagnose if
"#pragma omp declare target" has also been applied.
gcc/testsuite/
PR middle-end/89433
PR middle-end/93465
* c-c++-common/goacc-gomp/pr93465-1.c: New file.
---
gcc/omp-general.c | 13 +
.../c-c++-common/goacc-gomp/pr93465-1.c | 56 +++
2 files changed, 69 insertions(+)
create mode 100644 gcc/testsuite/c-c++-common/goacc-gomp/pr93465-1.c
diff --git a/gcc/omp-general.c b/gcc/omp-general.c
index f107f4c050f1..49023f42c473 100644
--- a/gcc/omp-general.c
+++ b/gcc/omp-general.c
@@ -1776,6 +1776,19 @@ oacc_verify_routine_clauses (tree fndecl, tree *clauses, location_t loc,
= lookup_attribute ("omp declare target", DECL_ATTRIBUTES (fndecl));
if (attr != NULL_TREE)
{
+ /* Diagnose if "#pragma omp declare target" has also been applied. */
+ if (TREE_VALUE (attr) == NULL_TREE)
+ {
+ /* See ; the semantics of combining
+ OpenACC and OpenMP 'target' are not clear. */
+ error_at (loc,
+ "cannot apply %<%s%> to %qD, which has also been"
+ " marked with an OpenMP 'declare target' directive",
+ routine_str, fndecl);
+ /* Incompatible. */
+ return -1;
+ }
+
/* If a "#pragma acc routine" has already been applied, just verify
this one for compatibility. */
/* Collect previous directive's clauses. */
diff --git a/gcc/testsuite/c-c++-common/goacc-gomp/pr93465-1.c b/gcc/testsuite/c-c++-common/goacc-gomp/pr93465-1.c
new file mode 100644
index ..c8b9135d9973
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/goacc-gomp/pr93465-1.c
@@ -0,0 +1,56 @@
+#pragma omp declare target
+#pragma acc routine seq /* { dg-error "cannot apply '#pragma acc routine' to '\(void \)?f1\(\\(\\)\)?', which has also been marked with an OpenMP 'declare target' directive" } */
+void f1 (void) {}
+#pragma omp end declare target
+
+#pragma omp declare target
+void f1 (void);
+
+#pragma acc routine seq /* { dg-error "cannot apply '#pragma acc routine' to '\(void \)?f1\(\\(\\)\)?', which has also been
Re: [testsuite] Fix PR94019 to allow one vector char when !vect_hw_misalign
"Kewen.Lin" writes:
> Hi,
>
> As PR94019 shows, without misaligned vector access support but with
> realign load, the vectorized loop will end up with realign scheme.
> It generates mask (control vector) with return type vector signed
> char which breaks the not check.
>
> The fix is to differentiate powerpc vect_hw_misalign and powerpc
> !vect_hw_misalign, permit one vector char occurance for powerpc
> !vect_hw_misalign and keep other targets same as before.
>
> Verified it on ppc64-redhat-linux (Power7 BE).
>
> Is it ok for trunk, and backport to GCC 9 after some burn-in time?
>
>
> BR,
> Kewen
>
>
> gcc/testsuite/ChangeLog
>
> 2020-03-04 Kewen Lin
>
> PR testsuite/94019
> * gcc.dg/vect/vect-over-widen-17.c: Expect one vector char if it's on
> POWER and without misaligned vector access support.
>
>
>
> --- a/gcc/testsuite/gcc.dg/vect/vect-over-widen-17.c
> +++ b/gcc/testsuite/gcc.dg/vect/vect-over-widen-17.c
> @@ -41,6 +41,10 @@ main (void)
> }
>
> /* { dg-final { scan-tree-dump-not {vect_recog_over_widening_pattern:
> detected} "vect" } } */
> -/* { dg-final { scan-tree-dump-not {vector[^\n]*char} "vect" } } */
> +/* { dg-final { scan-tree-dump-not {vector[^\n]*char} "vect" { target { { !
> powerpc*-*-* } || { powerpc*-*-* && vect_hw_misalign } } } } }
> +/* On Power, if there is no vect_hw_misalign support, unaligned vector access
> + adopts realign_load scheme. It requires rs6000_builtin_mask_for_load to
> + generate mask whose return type is vector char. */
> +/* { dg-final { scan-tree-dump-times {vector[^\n]*char} 1 "vect" { target {
> powerpc*-*-* && { ! vect_hw_misalign } } } } } */
Thanks for looking at this. The patch is OK as-is. However, since
vect-over-widen-17.c is a negative test for generic code, there probably
isn't much need for the new scan-tree-dump-times line, and it could start
failing if we make different optimisation decisions in future. So the
patch is also OK with just the change to the scan-tree-dump-not line,
if you prefer that. (Please keep the comment either way though --
it's really helpful.)
Richard
> /* { dg-final { scan-tree-dump-not {vector[^ ]* int} "vect" } } */
> /* { dg-final { scan-tree-dump-times "vectorized 1 loop" 1 "vect" } } */
Re: [testsuite] Fix PR94023 to guard case under vect_hw_misalign
"Kewen.Lin" writes:
> Hi,
>
> As PR94023 shows, the expected SLP requires misaligned vector access
> support. This patch is to guard the check under the target condition
> vect_hw_misalign to ensure that.
>
> Verified it on ppc64-redhat-linux (Power7 BE).
>
> Is it ok for trunk, and backport to GCC 9 after some burn-in time?
OK for both, thanks.
Richard
>
>
> BR,
> Kewen
>
>
> gcc/testsuite/ChangeLog
>
> 2020-03-04 Kewen Lin
>
> PR testsuite/94023
> * gcc.dg/vect/slp-perm-12.c: Expect loop vectorized messages only on
> vect_hw_misalign targets.
>
>
> diff --git a/gcc/testsuite/gcc.dg/vect/slp-perm-12.c
> b/gcc/testsuite/gcc.dg/vect/slp-perm-12.c
> index 4d4c534..113223a 100644
> --- a/gcc/testsuite/gcc.dg/vect/slp-perm-12.c
> +++ b/gcc/testsuite/gcc.dg/vect/slp-perm-12.c
> @@ -49,4 +49,4 @@ int main()
>return 0;
> }
>
> -/* { dg-final { scan-tree-dump-times "vectorizing stmts using SLP" 1 "vect"
> { target vect_perm } } } */
> +/* { dg-final { scan-tree-dump-times "vectorizing stmts using SLP" 1 "vect"
> { target { vect_perm && vect_hw_misalign } } } } */
Re: [testsuite] Fix PR94019 to allow one vector char when !vect_hw_misalign
Hi! On Wed, Mar 04, 2020 at 03:13:51PM +0800, Kewen.Lin wrote: > As PR94019 shows, without misaligned vector access support but with > realign load, the vectorized loop will end up with realign scheme. > It generates mask (control vector) with return type vector signed > char which breaks the not check. > > The fix is to differentiate powerpc vect_hw_misalign and powerpc > !vect_hw_misalign, permit one vector char occurance for powerpc > !vect_hw_misalign and keep other targets same as before. > > Verified it on ppc64-redhat-linux (Power7 BE). > > Is it ok for trunk, and backport to GCC 9 after some burn-in time? Why is any of this Power-specific? Does it work on other targets if !vect_hw_misalign? The patch is fine everywhere as far as the rs6000 port is concerned, but I'd like someone else who actually understands this stuff to have a look at it as well ;-) Segher
Re: [PATCH] [rs6000] Fix a wrong GC issue
On Wed, Mar 04, 2020 at 03:08:41PM +0800, binbin wrote: > >>* config/rs6000/rs6000.h (MAX_MACHINE_MODE): Include the header file > >>for MAX_MACHINE_MODE. > > > >The changelog entry should say *what* file is included, and under what > >condition. It doesn't have to say why (that belongs in the commit > >message). > > > >But, can't you just include it unconditionally? Don't we already, > >anyway, via coretypes.h -> machmode.h -> insn-modes.h? > > OK, change it to uncondition. Thanks for your suggestion. What about the second part? Shouldn't it already be included anyway? > * config/rs6000/rs6000-internal.h (altivec_builtin_mask_for_load, > builtin_mode_to_type[MAX_MACHINE_MODE][2]): Remove GTY(()). This changelog entry needs updating now. (And please check the rest as well.) Segher
Re: [PATCH v2 3/3] Keep .GCC.command.line sections of LTO objetcs.
On Wed, Mar 04, 2020 at 06:42:29PM +0100, Martin Liška wrote: > > I must say I don't really see advantages of this over > > -grecord-gcc-switches, recording all options looks very bloaty and will > > include mostly stuff you don't really care about (such as, e.g. the -I > > options without knowing what was the current directory when the source file > > has been compiled), on the other side will not record interesting options > > that -grecord-gcc-switches records (say, if some code is compiled with > > -march=native, this new option will record that, rather than what it really > > is), but I won't stand in a way unless such an option would be on by > > default. > > Yes, it's a minor disadvantage. On the other hand one can check the fortify > macros. I don't care much about them too, but what's the biggest benefit to me > is that each argument will not go into it's own mergeable section. Then > you will not see something like: Well, the fortify macro is questionable, because as a macro, it can be either specified on the command line, or e.g. defined in the source before including headers, so -g3 seems much better way to query it. > The output is useless and can't disambiguate each compiler > invocations. Sure, I'm not talking about -frecord-gcc-switches, that option is indeed not really useful. Jakub
Re: [PATCH v2 3/3] Keep .GCC.command.line sections of LTO objetcs.
On 3/4/20 6:33 PM, Jakub Jelinek wrote: On Wed, Mar 04, 2020 at 06:23:10PM +0100, Martin Liška wrote: On 3/4/20 4:25 PM, Egeyar Bagcioglu wrote: Thanks Richard. I do not have write-access to the GCC repo. I'd be glad if someone commits it for me. Can we please wait? I'm really convinced we do not want one another very similar functionality. I would definitely recommend to change the semantics of -frecord-gcc-switches to what the patchset does. That's why I added Nick as he's the author of the original implementation. Reasoning is provided in my previous email. Also, what is the reason for storing the option in a file, rather than say putting them into an environment variable from which the backend can pick it up? That's smart suggestion! It would not be the first environment variable that we use, right? I must say I don't really see advantages of this over -grecord-gcc-switches, recording all options looks very bloaty and will include mostly stuff you don't really care about (such as, e.g. the -I options without knowing what was the current directory when the source file has been compiled), on the other side will not record interesting options that -grecord-gcc-switches records (say, if some code is compiled with -march=native, this new option will record that, rather than what it really is), but I won't stand in a way unless such an option would be on by default. Yes, it's a minor disadvantage. On the other hand one can check the fortify macros. I don't care much about them too, but what's the biggest benefit to me is that each argument will not go into it's own mergeable section. Then you will not see something like: $ gcc -O0 foo.c -frecord-gcc-switches -c -g $ gcc -Ofast bar.c -frecord-gcc-switches -c -g0 $ gcc foo.o bar.o $ readelf -p .GCC.command.line a.out String dump of section '.GCC.command.line': [ 0] foo.c [ 6] -mtune=generic [15] -march=x86-64 [23] -g [26] -O0 [2a] -frecord-gcc-switches [40] bar.c [46] -g0 [4a] -Ofast The output is useless and can't disambiguate each compiler invocations. Martin Jakub
Re: [PATCH] avoid user-constructible types in reshape_init_array (PR 90938)
On 2/14/20 3:06 PM, Martin Sebor wrote:
On 2/13/20 3:59 PM, Jason Merrill wrote:
On 2/12/20 9:21 PM, Martin Sebor wrote:
On 2/11/20 5:28 PM, Jason Merrill wrote:
On 2/11/20 9:00 PM, Martin Sebor wrote:
r270155, committed in GCC 9, introduced a transformation that strips
redundant trailing zero initializers from array initializer lists in
order to support string literals as template arguments.
The transformation neglected to consider the case of array elements
of trivial class types with user-defined conversion ctors and either
defaulted or deleted default ctors. (It didn't occur to me that
those qualify as trivial types despite the user-defined ctors.) As
a result, some valid initialization expressions are rejected when
the explicit zero-initializers are dropped in favor of the (deleted)
default ctor,
Hmm, a type with only a deleted default constructor is not trivial,
that should have been OK already.
For Marek's test case:
struct A { A () == delete; A (int) = delete; };
trivial_type_p() returns true (as does __is_trivial (A) in both GCC
and Clang).
[class.prop] says that
A trivial class is a class that is trivially copyable and has one
or more default constructors (10.3.4.1), all of which are either
trivial or deleted and at least one of which is not deleted.
That sounds like A above is not trivial because it doesn't have
at least one default ctor that's not deleted, but both GCC and
Clang say it is. What am I missing? Is there some other default
constructor hiding in there that I don't know about?
and others are eliminated in favor of the defaulted
ctor instead of invoking a user-defined conversion ctor, leading to
wrong code.
This seems like a bug in type_initializer_zero_p; it shouldn't treat
0 as a zero initializer for any class.
That does fix it, and it seems like the right solution to me as well.
Thanks for the suggestion. I'm a little unsure about the condition
I put in place though.
Attached is an updated patch rested on x86_64-linux.
- if (sized_array_p && trivial_type_p (elt_type))
+ if (sized_array_p
+ && trivial_type_p (elt_type)
+ && !TYPE_NEEDS_CONSTRUCTING (elt_type))
Do we still need this change? If so, please add a comment about the
trivial_type_p bug.
The change isn't needed with my patch as it was
Let's go ahead and commit the type_initializer_zero_p hunk on both trunk
and 9 now to fix this bug for 9.3, and pursue the
process_init_constructor_array patch for later.
Jason
Re: [PATCH v2 3/3] Keep .GCC.command.line sections of LTO objetcs.
On Wed, Mar 04, 2020 at 06:23:10PM +0100, Martin Liška wrote: > On 3/4/20 4:25 PM, Egeyar Bagcioglu wrote: > > Thanks Richard. > > > > I do not have write-access to the GCC repo. I'd be glad if someone commits > > it for me. > > Can we please wait? I'm really convinced we do not want one another very > similar > functionality. I would definitely recommend to change the semantics > of -frecord-gcc-switches to what the patchset does. > > That's why I added Nick as he's the author of the original implementation. > Reasoning is provided in my previous email. Also, what is the reason for storing the option in a file, rather than say putting them into an environment variable from which the backend can pick it up? I must say I don't really see advantages of this over -grecord-gcc-switches, recording all options looks very bloaty and will include mostly stuff you don't really care about (such as, e.g. the -I options without knowing what was the current directory when the source file has been compiled), on the other side will not record interesting options that -grecord-gcc-switches records (say, if some code is compiled with -march=native, this new option will record that, rather than what it really is), but I won't stand in a way unless such an option would be on by default. Jakub
[COMMITTED] add test for false positive -Wformat-overflow in a loop [PR81401]
The bug has been fixed for a while. I added the two tests from
the report in 97bd1d6b513..3ca63e1c76b (attached).
Martin
commit 3ca63e1c76b7693b5d3f5ba2567421defc764249 (HEAD -> master)
Author: Martin Sebor
Date: Wed Mar 4 10:23:49 2020 -0700
PR middle-end/81401 - false positive -Wformat-overflow in a loop
gcc/testsuite/ChangeLog:
* gcc.dg/tree-ssa/builtin-sprintf-warn-24.c: New test.
diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog
index 83882d2ecf6..b28eb52f8a5 100644
--- a/gcc/testsuite/ChangeLog
+++ b/gcc/testsuite/ChangeLog
@@ -1,3 +1,8 @@
+2020-03-04 Martin Sebor
+
+ PR middle-end/81401
+ * gcc.dg/tree-ssa/builtin-sprintf-warn-24.c: New test.
+
2020-03-04 Will Schmidt
* gcc.target/powerpc/20050603-3.c: Remove XFAILS.
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-24.c b/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-24.c
new file mode 100644
index 000..d9109781d84
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-24.c
@@ -0,0 +1,22 @@
+/* PR middle-end/81401 - false positive -Wformat-overflow in a loop
+ { dg-do compile }
+ { dg-options "-O2 -Wall -Wformat-overflow" } */
+
+char a[3];
+
+void f (void)
+{
+ int i, i0 = 0x00;
+
+ for (i = i0; i <= 0xff; ++i)
+__builtin_sprintf (a, "%02x", i); // { dg-bogus "\\\[-Wformat-overflow" }
+}
+
+char b[2];
+
+void g (void)
+{
+ int i;
+ for (i = 0; i < 10; ++i)
+__builtin_sprintf (b, "%d", i); // { dg-bogus "\\\[-Wformat-overflow" }
+}
Re: [PATCH v2 3/3] Keep .GCC.command.line sections of LTO objetcs.
On 3/4/20 4:25 PM, Egeyar Bagcioglu wrote: Thanks Richard. I do not have write-access to the GCC repo. I'd be glad if someone commits it for me. Can we please wait? I'm really convinced we do not want one another very similar functionality. I would definitely recommend to change the semantics of -frecord-gcc-switches to what the patchset does. That's why I added Nick as he's the author of the original implementation. Reasoning is provided in my previous email. Martin
Re: [GCC][PATCH][AArch32] ACLE intrinsics bfloat16 vmmla and vfma for AArch32 AdvSIMD
Hi,
This is the latest version of the patch.
Thanks,
Delia
On 2/21/20 11:41 AM, Kyrill Tkachov wrote:
Hi Delia,
On 2/19/20 5:23 PM, Delia Burduv wrote:
Hi,
Here is the latest version of the patch. It just has some minor
formatting changes that were brought up by Richard Sandiford in the
AArch64 patches
Thanks,
Delia
On 1/31/20 3:23 PM, Delia Burduv wrote:
Here is the updated patch. The changes are minor, so let me know if
there is anything else to fix or if it can be committed.
Thank you,
Delia
On 1/30/20 2:55 PM, Kyrill Tkachov wrote:
Hi Delia,
On 1/28/20 4:44 PM, Delia Burduv wrote:
Ping.
*From:* Delia Burduv
*Sent:* 22 January 2020 17:26
*To:* gcc-patches@gcc.gnu.org
*Cc:* ni...@redhat.com ; Richard Earnshaw
; Ramana Radhakrishnan
; Kyrylo Tkachov
*Subject:* Re: [GCC][PATCH][AArch32] ACLE intrinsics bfloat16 vmmla
and vfma for AArch32 AdvSIMD
Ping.
I have read Richard Sandiford's comments on the AArch64 patches and I
will apply what is relevant to this patch as well. Particularly, I
will
change the tests to use the exact input and output registers and I
will
change the types of the rtl patterns.
Please send the updated patches so that someone can commit them for
you once they're reviewed.
Thanks,
Kyrill
On 12/20/19 6:44 PM, Delia Burduv wrote:
> This patch adds the ARMv8.6 ACLE intrinsics for vmmla, vfmab and
vfmat
> as part of the BFloat16 extension.
> (https://developer.arm.com/docs/101028/latest.)
> The intrinsics are declared in arm_neon.h and the RTL patterns are
> defined in neon.md.
> Two new tests are added to check assembler output and lane indices.
>
> This patch depends on the Arm back-end patche.
> (https://gcc.gnu.org/ml/gcc-patches/2019-12/msg01448.html)
>
> Tested for regression on arm-none-eabi and armeb-none-eabi. I
don't have
> commit rights, so if this is ok can someone please commit it for me?
>
> gcc/ChangeLog:
>
> 2019-11-12� Delia Burduv
>
>� ����* config/arm/arm_neon.h (vbfmmlaq_f32): New.
>� ����� (vbfmlalbq_f32): New.
>� ����� (vbfmlaltq_f32): New.
>� ����� (vbfmlalbq_lane_f32): New.
>� ����� (vbfmlaltq_lane_f32): New.
>� ������� (vbfmlalbq_laneq_f32): New.
>� ����� (vbfmlaltq_laneq_f32): New.
>� ����* config/arm/arm_neon_builtins.def (vbfmmla): New.
>� ��������� (vbfmab): New.
>� ��������� (vbfmat): New.
>� ��������� (vbfmab_lane): New.
>� ��������� (vbfmat_lane): New.
>� ��������� (vbfmab_laneq): New.
>� ��������� (vbfmat_laneq): New.
>� ���� * config/arm/iterators.md (BF_MA): New int iterator.
>� ��������� (bt): New int attribute.
>� ��������� (VQXBF): Copy of VQX with V8BF.
>� ��������� (V_HALF): Added V8BF.
>� ����� * config/arm/neon.md (neon_vbfmmlav8hi): New
insn.
>� ��������� (neon_vbfmav8hi): New insn.
>� ��������� (neon_vbfma_lanev8hi): New insn.
>� ��������� (neon_vbfma_laneqv8hi): New
expand.
>� ��������� (neon_vget_high): Changed
iterator to VQXBF.
>� ����* config/arm/unspecs.md (UNSPEC_BFMMLA): New UNSPEC.
>� ��������� (UNSPEC_BFMAB): New UNSPEC.
>� ��������� (UNSPEC_BFMAT): New UNSPEC.
>
> 2019-11-12� Delia Burduv
>
>� ������� * gcc.target/arm/simd/bf16_ma_1.c: New
test.
>� ������� * gcc.target/arm/simd/bf16_ma_2.c: New
test.
>� ������� * gcc.target/arm/simd/bf16_mmla_1.c: New
test.
This looks good, a few minor things though...
diff --git a/gcc/config/arm/arm_neon.h b/gcc/config/arm/arm_neon.h
index
3c78f435009ab027f92693d00ab5b40960d5419d..81f8008ea6a5fb11eb09f6685ba24bb0c54fb248
100644
--- a/gcc/config/arm/arm_neon.h
+++ b/gcc/config/arm/arm_neon.h
@@ -18742,6 +18742,64 @@ vcmlaq_rot270_laneq_f32 (float32x4_t __r,
float32x4_t __a, float32x4_t __b,
�� return __builtin_neon_vcmla_lane270v4sf (__r, __a, __b, __index);
�}
+#pragma GCC push_options
+#pragma GCC target ("arch=armv8.2-a+bf16")
+
+__extension__ extern __inline float32x4_t
+__attribute__ ((__always_inline__, __gnu_inline__, __artificial__))
+vbfmmlaq_f32 (float32x4_t __r, bfloat16x8_t __a, bfloat16x8_t __b)
+{
+� return __builtin_neon_vbfmmlav8bf (__r, __a, __b);
+}
+
+__extension__ extern __inline float32x4_t
+__attribute__ ((__always_inline__, __gnu_inline__, __artificial__))
+vbfmlalbq_f32 (float32x4_t __r, bfloat16x8_t __a, bfloat16x8_t __b)
+{
+� return __builtin_neon_vbfmabv8bf (__r, __a, __b);
+}
+
+__extension__ extern __inline float32x4_t
+__attribute__ ((__always_inline__, __gnu_inline__, __artificial__))
+vbfmlaltq_f32 (float32x4_t __r, bfloat16x8_t __a, bfloat16x8_t
Re: ACLE intrinsics: BFloat16 store (vst{q}_bf16) intrinsics for AArch32
Hi Delia,
On 3/3/20 5:23 PM, Delia Burduv wrote:
Hi,
I noticed that the patch doesn't apply cleanly. I fixed it and this is
the latest version.
Thanks,
Delia
On 3/3/20 4:23 PM, Delia Burduv wrote:
Sorry, I forgot the attachment.
On 3/3/20 4:20 PM, Delia Burduv wrote:
Hi,
I made a mistake in the previous patch. This is the latest version.
Please let me know if it is ok.
Thanks,
Delia
On 2/21/20 3:18 PM, Delia Burduv wrote:
Hi Kyrill,
The arm_bf16.h is only used for scalar operations. That is how the
aarch64 versions are implemented too.
Thanks,
Delia
On 2/21/20 2:06 PM, Kyrill Tkachov wrote:
Hi Delia,
On 2/19/20 5:25 PM, Delia Burduv wrote:
Hi,
Here is the latest version of the patch. It just has some minor
formatting changes that were brought up by Richard Sandiford in the
AArch64 patches
Thanks,
Delia
On 1/22/20 5:29 PM, Delia Burduv wrote:
> Ping.
>
> I will change the tests to use the exact input and output
registers as
> Richard Sandiford suggested for the AArch64 patches.
>
> On 12/20/19 6:46 PM, Delia Burduv wrote:
>> This patch adds the ARMv8.6 ACLE BFloat16 store intrinsics
>> vst{q}_bf16 as part of the BFloat16 extension.
>>
(https://developer.arm.com/architectures/instruction-sets/simd-isas/neon/intrinsics)
>>
>> The intrinsics are declared in arm_neon.h .
>> A new test is added to check assembler output.
>>
>> This patch depends on the Arm back-end patche.
>> (https://gcc.gnu.org/ml/gcc-patches/2019-12/msg01448.html)
>>
>> Tested for regression on arm-none-eabi and armeb-none-eabi. I
don't
>> have commit rights, so if this is ok can someone please commit
it for me?
>>
>> gcc/ChangeLog:
>>
>> 2019-11-14 Delia Burduv
>>
>> * config/arm/arm_neon.h (bfloat16_t): New typedef.
>> (bfloat16x4x2_t): New typedef.
>> (bfloat16x8x2_t): New typedef.
>> (bfloat16x4x3_t): New typedef.
>> (bfloat16x8x3_t): New typedef.
>> (bfloat16x4x4_t): New typedef.
>> (bfloat16x8x4_t): New typedef.
>> (vst2_bf16): New.
>> (vst2q_bf16): New.
>> (vst3_bf16): New.
>> (vst3q_bf16): New.
>> (vst4_bf16): New.
>> (vst4q_bf16): New.
>> * config/arm/arm-builtins.c (E_V2BFmode): New mode.
>> (VAR13): New.
>> (arm_simd_types[Bfloat16x2_t]):New type.
>> * config/arm/arm-modes.def (V2BF): New mode.
>> * config/arm/arm-simd-builtin-types.def
>> (Bfloat16x2_t): New entry.
>> * config/arm/arm_neon_builtins.def
>> (vst2): Changed to VAR13 and added v4bf, v8bf
>> (vst3): Changed to VAR13 and added v4bf, v8bf
>> (vst4): Changed to VAR13 and added v4bf, v8bf
>> * config/arm/iterators.md (VDXBF): New iterator.
>> (VQ2BF): New iterator.
>> (V_elem): Added V4BF, V8BF.
>> (V_sz_elem): Added V4BF, V8BF.
>> (V_mode_nunits): Added V4BF, V8BF.
>> (q): Added V4BF, V8BF.
>> *config/arm/neon.md (vst2): Used new iterators.
>> (vst3): Used new iterators.
>> (vst3qa): Used new iterators.
>> (vst3qb): Used new iterators.
>> (vst4): Used new iterators.
>> (vst4qa): Used new iterators.
>> (vst4qb): Used new iterators.
>>
>>
>> gcc/testsuite/ChangeLog:
>>
>> 2019-11-14 Delia Burduv
>>
>> * gcc.target/arm/simd/bf16_vstn_1.c: New test.
One thing I just noticed in this and the other arm bfloat16
patches...
diff --git a/gcc/config/arm/arm_neon.h b/gcc/config/arm/arm_neon.h
index
3c78f435009ab027f92693d00ab5b40960d5419d..fd81c18948db3a7f6e8e863d32511f75bf950e6a
100644
--- a/gcc/config/arm/arm_neon.h
+++ b/gcc/config/arm/arm_neon.h
@@ -18742,6 +18742,89 @@ vcmlaq_rot270_laneq_f32 (float32x4_t __r,
float32x4_t __a, float32x4_t __b,
return __builtin_neon_vcmla_lane270v4sf (__r, __a, __b, __index);
}
+#pragma GCC push_options
+#pragma GCC target ("arch=armv8.2-a+bf16")
+
+typedef struct bfloat16x4x2_t
+{
+ bfloat16x4_t val[2];
+} bfloat16x4x2_t;
These should be in a new arm_bf16.h file that gets included in the
main arm_neon.h file, right?
I believe the aarch64 versions are implemented that way.
Otherwise the patch looks good to me.
Thanks!
Kyrill
+
+typedef struct bfloat16x8x2_t
+{
+ bfloat16x8_t val[2];
+} bfloat16x8x2_t;
+
diff --git a/gcc/testsuite/gcc.target/arm/simd/bf16_vstn_1.c
b/gcc/testsuite/gcc.target/arm/simd/bf16_vstn_1.c
new file mode 100644
index
..b52ecfb959776fd04c7c33908cb7f8898ec3fe0b
--- /dev/null
+++ b/gcc/testsuite/gcc.target/arm/simd/bf16_vstn_1.c
@@ -0,0 +1,84 @@
+/* { dg-do assemble } */
+/* { dg-require-effective-target arm_v8_2a_bf16_neon_ok } */
+/* { dg-add-options arm_v8_2a_bf16_neon } */
+/* { dg-additional-options "-save-temps" } */
+/* { dg-final { check-function-bodies "**" "" {-O[^0]} } } */
+
I don't see the check-function-bodies checks being performed in my testing.
Changing the directives order to:
/* { dg-do
Re: ACLE intrinsics: BFloat16 load intrinsics for AArch32
Hi Delia,
On 3/4/20 2:05 PM, Delia Burduv wrote:
Hi,
The previous version of this patch shared part of its code with the
store intrinsics patch
(https://gcc.gnu.org/ml/gcc-patches/2020-03/msg00145.html) so I removed
any duplicated code. This patch now depends on the previously mentioned
store intrinsics patch.
Here is the latest version and the updated ChangeLog.
gcc/ChangeLog:
2019-03-04 Delia Burduv
* config/arm/arm_neon.h (bfloat16_t): New typedef.
(vld2_bf16): New.
(vld2q_bf16): New.
(vld3_bf16): New.
(vld3q_bf16): New.
(vld4_bf16): New.
(vld4q_bf16): New.
(vld2_dup_bf16): New.
(vld2q_dup_bf16): New.
(vld3_dup_bf16): New.
(vld3q_dup_bf16): New.
(vld4_dup_bf16): New.
(vld4q_dup_bf16): New.
* config/arm/arm_neon_builtins.def
(vld2): Changed to VAR13 and added v4bf, v8bf
(vld2_dup): Changed to VAR8 and added v4bf, v8bf
(vld3): Changed to VAR13 and added v4bf, v8bf
(vld3_dup): Changed to VAR8 and added v4bf, v8bf
(vld4): Changed to VAR13 and added v4bf, v8bf
(vld4_dup): Changed to VAR8 and added v4bf, v8bf
* config/arm/iterators.md (VDXBF): New iterator.
(VQ2BF): New iterator.
*config/arm/neon.md (vld2): Used new iterators.
(vld2_dup): Used new iterators.
(vld2_dupv8bf): New.
(vst3): Used new iterators.
(vst3qa): Used new iterators.
(vst3qb): Used new iterators.
(vld3_dup): Used new iterators.
(vld3_dupv8bf): New.
(vst4): Used new iterators.
(vst4qa): Used new iterators.
(vst4qb): Used new iterators.
(vld4_dup): Used new iterators.
(vld4_dupv8bf): New.
gcc/testsuite/ChangeLog:
2019-03-04 Delia Burduv
* gcc.target/arm/simd/bf16_vldn_1.c: New test.
Thanks,
Delia
On 2/19/20 5:25 PM, Delia Burduv wrote:
>
> Hi,
>
> Here is the latest version of the patch. It just has some minor
> formatting changes that were brought up by Richard Sandiford in the
> AArch64 patches
>
> Thanks,
> Delia
>
> On 1/22/20 5:31 PM, Delia Burduv wrote:
>> Ping.
>>
>> I will change the tests to use the exact input and output registers as
>> Richard Sandiford suggested for the AArch64 patches.
>>
>> On 12/20/19 6:48 PM, Delia Burduv wrote:
>>> This patch adds the ARMv8.6 ACLE BFloat16 load intrinsics
>>> vld{q}_bf16 as part of the BFloat16 extension.
>>>
(https://developer.arm.com/architectures/instruction-sets/simd-isas/neon/intrinsics)
>>>
>>> The intrinsics are declared in arm_neon.h .
>>> A new test is added to check assembler output.
>>>
>>> This patch depends on the Arm back-end patche.
>>> (https://gcc.gnu.org/ml/gcc-patches/2019-12/msg01448.html)
>>>
>>> Tested for regression on arm-none-eabi and armeb-none-eabi. I don't
>>> have commit rights, so if this is ok can someone please commit it for
>>> me?
>>>
>>> gcc/ChangeLog:
>>>
>>> 2019-11-14 Delia Burduv
>>>
>>> * config/arm/arm_neon.h (bfloat16_t): New typedef.
>>> (bfloat16x4x2_t): New typedef.
>>> (bfloat16x8x2_t): New typedef.
>>> (bfloat16x4x3_t): New typedef.
>>> (bfloat16x8x3_t): New typedef.
>>> (bfloat16x4x4_t): New typedef.
>>> (bfloat16x8x4_t): New typedef.
>>> (vld2_bf16): New.
>>> (vld2q_bf16): New.
>>> (vld3_bf16): New.
>>> (vld3q_bf16): New.
>>> (vld4_bf16): New.
>>> (vld4q_bf16): New.
>>> (vld2_dup_bf16): New.
>>> (vld2q_dup_bf16): New.
>>> (vld3_dup_bf16): New.
>>> (vld3q_dup_bf16): New.
>>> (vld4_dup_bf16): New.
>>> (vld4q_dup_bf16): New.
>>> * config/arm/arm-builtins.c (E_V2BFmode): New mode.
>>> (VAR13): New.
>>> (arm_simd_types[Bfloat16x2_t]):New type.
>>> * config/arm/arm-modes.def (V2BF): New mode.
>>> * config/arm/arm-simd-builtin-types.def
>>> (Bfloat16x2_t): New entry.
>>> * config/arm/arm_neon_builtins.def
>>> (vld2): Changed to VAR13 and added v4bf, v8bf
>>> (vld2_dup): Changed to VAR8 and added v4bf, v8bf
>>> (vld3): Changed to VAR13 and added v4bf, v8bf
>>> (vld3_dup): Changed to VAR8 and added v4bf, v8bf
>>> (vld4): Changed to VAR13 and added v4bf, v8bf
>>> (vld4_dup): Changed to VAR8 and added v4bf, v8bf
>>> * config/arm/iterators.md (VDXBF): New iterator.
>>> (VQ2BF): New iterator.
>>> (V_elem): Added V4BF, V8BF.
>>> (V_sz_elem): Added V4BF, V8BF.
>>> (V_mode_nunits): Added V4BF, V8BF.
>>> (q): Added V4BF, V8BF.
>>> *config/arm/neon.md (vld2): Used new iterators.
>>> (vld2_dup): Used new iterators.
>>> (vld2_dupv8bf): New.
>>> (vst3): Used new iterators.
>>> (vst3qa): Used new iterators.
>>> (vst3qb): Used new iterators.
>>> (vld3_dup): Used new iterators.
>>>
Re: [PATCH v2 0/3] Introduce a new GCC option, --record-gcc-command-line
On 3/4/20 5:28 PM, Egeyar Bagcioglu wrote: On 3/4/20 1:18 AM, Fangrui Song wrote: On 2020-03-03, Joseph Myers wrote: On Tue, 3 Mar 2020, Egeyar Bagcioglu wrote: Although we discussed after the submission of the first version that there are several other options performing similar tasks, I believe we established that there is still a need for this specific functionality. Therefore, I am skipping in this email the comparison between this option and the existing options with similarities. Mentioning -frecord-gcc-switches will be much appreciated. How is the new .GCC.command.line different? Does it still have the SHF_MERGE | SHF_STRINGS flag? If you change the flags, the .GCC.command.line section may not play with another object file (generated by -frecord-gcc-switches) whose .GCC.command.line is SHF_MERGE | SHF_STRINGS. When both -frecord-gcc-switches and --record-command-line are specified, is it an error? This option is similar to -frecord-gcc-switches. However, they have three fundamental differences: Firstly, -frecord-gcc-switches saves the internal state after the argv is processed and passed by the driver. As opposed to that, --record-gcc-command-line saves the command-line as received by the driver, with the exception of extending @files first. Secondly, -frecord-gcc-switches saves the switches as separate entries into a mergeable string section. Therefore, the entries belonging to different object files get mixed up after being linked. The new --record-gcc-command-line, on the other hand, creates one entry per invocation. By doing so, it makes it clear which options were used together in a single gcc invocation. Lastly, --record-gcc-command-line also adds the version of the gcc into this single entry to make it clear which version of gcc was called with any given command line. This is useful in cases where .comment section reports multiple versions. I should add here that both Martin Liska and Nick Clifton mentioned during the last round of discussion the importance of capturing options such as -D_FORTIFY_SOURCE which is apparently not distinguished by --frecord-gcc-switches. The option I am suggesting, --record-gcc-command-line, saves that without any special case handling. While there are also similarities between the implementations of these two options, those implementations are completely independent. These commands can be used separately or together without issues. I used the same section that -frecord-gcc-switches uses on purpose, so that they can also be used together to save both the command line given to GCC and the internal switches passed by GCC. Here is an old example from the previous discussion, calling g++ with both -frecord-gcc-switches and --record-gcc-command-line for comparison: [egeyar@localhost save-commandline]$ g++ main.c --record-gcc-command-line -frecord-gcc-switches [egeyar@localhost save-commandline]$ readelf -p .GCC.command.line a.out String dump of section '.GCC.command.line': [ 0] 10.0.0 20191025 (experimental) : g++ main.c --record-gcc-command-line -frecord-gcc-switches [ 5c] -D_GNU_SOURCE [ 6a] main.c [ 71] -mtune=generic [ 80] -march=x86-64 [ 8e] --record-gcc-command-line /tmp/ccgC4ZtS.cmdline Above, the first entry in the .GCC.command.line section is saved by --record-gcc-command-line; while the rest of the entries are added by -frecord-gcc-switches. Regards Egeyar The option -grecord-gcc-switches is similar to -frecord-gcc-switches, but saves the internal GCC switches into DWARF. Lastly, -fverbose-asm option saves the switches into the assembly file but that information never makes it to the object files. We're now using git-style commit messages with self-contained explanation / justification of the change being committed. This means that one of the commit messages (not just message 0, whose contents don't go in a commit message) for an individual patch should have the explanation, which should include the self-contained justification by reference to comparison with other existing similar options. People should be able to find the relevant information in the commit without needing to search the list archives for reviews of a previous patch version. Thanks for telling me. I will extend the above comparison according to the questions I might receive. Then I'll add it, together with the explanation in the cover letter, into the commit message of the second patch. Regards Egeyar
Re: [PATCH v2 0/3] Introduce a new GCC option, --record-gcc-command-line
On 3/4/20 1:18 AM, Fangrui Song wrote: On 2020-03-03, Joseph Myers wrote: On Tue, 3 Mar 2020, Egeyar Bagcioglu wrote: Although we discussed after the submission of the first version that there are several other options performing similar tasks, I believe we established that there is still a need for this specific functionality. Therefore, I am skipping in this email the comparison between this option and the existing options with similarities. Mentioning -frecord-gcc-switches will be much appreciated. How is the new .GCC.command.line different? Does it still have the SHF_MERGE | SHF_STRINGS flag? If you change the flags, the .GCC.command.line section may not play with another object file (generated by -frecord-gcc-switches) whose .GCC.command.line is SHF_MERGE | SHF_STRINGS. When both -frecord-gcc-switches and --record-command-line are specified, is it an error? This option is similar to -frecord-gcc-switches. However, they have three fundamental differences: Firstly, -frecord-gcc-switches saves the internal state after the argv is processed and passed by the driver. As opposed to that, --record-gcc-command-line saves the command-line as received by the driver, with the exception of extending @files first. Secondly, -frecord-gcc-switches saves the switches as separate entries into a mergeable string section. Therefore, the entries belonging to different object files get mixed up after being linked. The new --record-gcc-command-line, on the other hand, creates one entry per invocation. By doing so, it makes it clear which options were used together in a single gcc invocation. Lastly, --record-gcc-command-line also adds the version of the gcc into this single entry to make it clear which version of gcc was called with any given command line. This is useful in cases where .comment section reports multiple versions. While there are also similarities between the implementations of these two options, those implementations are completely independent. These commands can be used separately or together without issues. I used the same section that -frecord-gcc-switches uses on purpose, so that they can also be used together to save both the command line given to GCC and the internal switches passed by GCC. The option -grecord-gcc-switches is similar to -frecord-gcc-switches, but saves the internal GCC switches into DWARF. Lastly, -fverbose-asm option saves the switches into the assembly file but that information never makes it to the object files. We're now using git-style commit messages with self-contained explanation / justification of the change being committed. This means that one of the commit messages (not just message 0, whose contents don't go in a commit message) for an individual patch should have the explanation, which should include the self-contained justification by reference to comparison with other existing similar options. People should be able to find the relevant information in the commit without needing to search the list archives for reviews of a previous patch version. Thanks for telling me. I will extend the above comparison according to the questions I might receive. Then I'll add it, together with the explanation in the cover letter, into the commit message of the second patch. Regards Egeyar
Re: [RFA/RFC] [tree-optimization/91890] [P1 Regression] Avoid clobbering useful location in Wrestrict code
On 3/4/20 8:54 AM, Jeff Law wrote:
Martin, I'd like your thoughts here.
As noted in the BZ our #pragmas aren't working to suppress a warning.
I did some debugging and ultimately found that the location passed down to the
diagnostic code is indeed outside the scope of the pragmas.
Further digging uncovered that we had a reasonable location passed to
maybe_diag_access_bounds, but rather than using it, we extracted a location
attached to the builtin_memref structure.
So if we look at the test:
char one[50];
char two[50];
void
test_strncat (void)
{
(void) __builtin_strcpy (one, "gh");
(void) __builtin_strcpy (two, "ef");
#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wstringop-overflow="
#pragma GCC diagnostic ignored "-Warray-bounds"
(void) __builtin_strncat (one, two, 99);
#pragma GCC diagnostic pop
}
The location we end up passing to the diagnostic code comes from the destination
of the call (via DSTREF which is passed to maybe_diag_access_bounds and becomes
REF in that context).
(gdb) p debug_tree (ref.ptr)
BLK
size
unit-size
align:8 warn_if_not_align:0 symtab:0 alias-set -1 canonical-type
0x7fffea9372a0 domain
pointer_to_this >
unsigned DI
size
unit-size
align:64 warn_if_not_align:0 symtab:0 alias-set -1 canonical-type
0x7fffea937540>
constant
arg:0
addressable used public static read BLK j.c:2:6 size unit-size
align:256 warn_if_not_align:0 context
chain
addressable used public static read BLK j.c:3:6 size unit-size
align:256 warn_if_not_align:0 context chain >>
j.c:8:28 start: j.c:8:28 finish: j.c:8:30>
Note the location. j.c line 8 column 28-30. THat location makes absolutely no
sense (it's the first call to strcpy) given we're trying to diagnose the
strncat.
I have no idea why we're using the location from ref.ptr rather than the
location
of the statement we're analyzing (which is passed in as LOC). It's no surprise
the pragma didn't work.
Anyway, this patch seems to fix the problem and doesn't regress anything. But
I'm hesitant to go forward with it given I don't know the motivation behind
extracting the location from ref.ptr.
Another thought would be to just remove the code that pulls the location from
ref.ptr. I haven't tested that, but certainly could easily.
Martin, thoughts?
I don't remember why the code in -Wrestrict unconditionally overwrites
the statement location rather than only when it's not available, but
I do remember adding conditional code like in your patch in r277076
to deal with missing location on the statement. So either your fix
or something like the hunk below might be the right solution (if we
go with the code below, abstracting it into a utility function might
be nice).
Thanks for looking into it!
Martin
@@ -3642,7 +3670,11 @@ maybe_warn_pointless_strcmp (gimple *stmt,
HOST_WIDE_INT bound,
/* FIXME: Include a note pointing to the declaration of the smaller
array. */
- location_t stmt_loc = gimple_location (stmt);
+ location_t stmt_loc = gimple_nonartificial_location (stmt);
+ if (stmt_loc == UNKNOWN_LOCATION && EXPR_HAS_LOCATION (lhs))
+stmt_loc = tree_nonartificial_location (lhs);
+ stmt_loc = expansion_point_location_if_in_system_header (stmt_loc);
+
tree callee = gimple_call_fndecl (stmt);
bool warned = false;
if (siz <= minlen && bound == -1)
Re: [committed] analyzer: detect malloc, free, calloc within "std" [PR93959]
On Wed, 2020-03-04 at 11:05 -0500, Marek Polacek wrote:
> On Wed, Mar 04, 2020 at 04:54:54PM +0100, Bernhard Reutner-Fischer
> wrote:
> > On Mon, 2 Mar 2020 16:48:26 -0500
> > David Malcolm wrote:
> >
> > > +static inline bool
> > > +is_std_function_p (const_tree fndecl)
> > > +{
> > > + tree name_decl = DECL_NAME (fndecl);
> > > + if (!name_decl)
> > > +return false;
> > > + if (!DECL_CONTEXT (fndecl))
> > > +return false;
> > > + if (TREE_CODE (DECL_CONTEXT (fndecl)) != NAMESPACE_DECL)
> > > +return false;
> > > + tree ns = DECL_CONTEXT (fndecl);
> > > + if (!(DECL_CONTEXT (ns) == NULL_TREE
> > > + || TREE_CODE (DECL_CONTEXT (ns)) == TRANSLATION_UNIT_DECL))
> > > +return false;
> > > + if (!DECL_NAME (ns))
> > > +return false;
> > > + return id_equal ("std", DECL_NAME (ns));
> > > +}
> >
> > Sounds a bit elaborate, doesn't?
> > I hope this is optimized to
> >
> > static inline bool
> > is_std_function_p (const_tree fndecl)
> > {
> > tree name_decl = DECL_NAME (fndecl);
> > if (!name_decl)
> > return false;
> > tree ns = DECL_CONTEXT (fndecl);
> > if (!ns)
> > return false;
> > if (TREE_CODE (ns) != NAMESPACE_DECL)
> > return false;
> > if (!(DECL_CONTEXT (ns) == NULL_TREE
> > || TREE_CODE (DECL_CONTEXT (ns)) == TRANSLATION_UNIT_DECL))
> > return false;
> > if (!DECL_NAME (ns))
> > return false;
> > return id_equal ("std", DECL_NAME (ns));
> > }
> >
> > isn't "std" spelled out std_identifier() and is an identifier, i.e.
> > return DECL_NAME (ns) == std_identifier; ?
>
> We have decl_in_std_namespace_p for that.
Thanks. Indeed the comment to the function quoted above which wasn't
quoted reads:
/* Return true if FNDECL is within the namespace "std".
Compare with cp/typeck.c: decl_in_std_namespace_p, but this doesn't
rely on being the C++ FE (or handle inline namespaces inside of std). */
since as noted in my reply to Bernhard the analyzer runs in the middle-
end and thus can't rely on features of a specific frontend.
Is there a better way to do this?
Dave
Re: [committed] analyzer: detect malloc, free, calloc within "std" [PR93959]
On Wed, 2020-03-04 at 16:54 +0100, Bernhard Reutner-Fischer wrote:
> On Mon, 2 Mar 2020 16:48:26 -0500
> David Malcolm wrote:
>
> > +static inline bool
> > +is_std_function_p (const_tree fndecl)
> > +{
> > + tree name_decl = DECL_NAME (fndecl);
> > + if (!name_decl)
> > +return false;
> > + if (!DECL_CONTEXT (fndecl))
> > +return false;
> > + if (TREE_CODE (DECL_CONTEXT (fndecl)) != NAMESPACE_DECL)
> > +return false;
> > + tree ns = DECL_CONTEXT (fndecl);
> > + if (!(DECL_CONTEXT (ns) == NULL_TREE
> > + || TREE_CODE (DECL_CONTEXT (ns)) == TRANSLATION_UNIT_DECL))
> > +return false;
> > + if (!DECL_NAME (ns))
> > +return false;
> > + return id_equal ("std", DECL_NAME (ns));
> > +}
>
> Sounds a bit elaborate, doesn't?
> I hope this is optimized to
>
> static inline bool
> is_std_function_p (const_tree fndecl)
> {
> tree name_decl = DECL_NAME (fndecl);
> if (!name_decl)
> return false;
> tree ns = DECL_CONTEXT (fndecl);
> if (!ns)
> return false;
> if (TREE_CODE (ns) != NAMESPACE_DECL)
> return false;
> if (!(DECL_CONTEXT (ns) == NULL_TREE
> || TREE_CODE (DECL_CONTEXT (ns)) == TRANSLATION_UNIT_DECL))
> return false;
> if (!DECL_NAME (ns))
> return false;
> return id_equal ("std", DECL_NAME (ns));
> }
>
> isn't "std" spelled out std_identifier() and is an identifier, i.e.
> return DECL_NAME (ns) == std_identifier; ?
gcc/cp/ChangeLog-2019 has:
2019-10-23 Nathan Sidwell
* cp-tree.c (CPTI_STD_IDENTIFIER): Delete.
(std_identifier): Delete.
(DECL_NAME_SPACE_STD_P): Compare against std_node.
[...snippped...]
The current ideal way to do it in gcc/cp/cp-tree.h seems to be:
/* Nonzero if NODE is the std namespace. */
#define DECL_NAMESPACE_STD_P(NODE)\
((NODE) == std_node)
where:
#define std_node cp_global_trees[CPTI_STD]
and:
extern GTY(()) tree cp_global_trees[CPTI_MAX];
However all of the above is specific to the C++ frontend, whereas the
analyzer is part of the middle-end (and can be run from within lto1,
where any concept of "the frontend" becomes rather abstract).
I don't like the above analyzer code, but I don't see a better way to
do it.
Dave
Re: [committed] analyzer: detect malloc, free, calloc within "std" [PR93959]
On Wed, Mar 04, 2020 at 04:54:54PM +0100, Bernhard Reutner-Fischer wrote:
> On Mon, 2 Mar 2020 16:48:26 -0500
> David Malcolm wrote:
>
> > +static inline bool
> > +is_std_function_p (const_tree fndecl)
> > +{
> > + tree name_decl = DECL_NAME (fndecl);
> > + if (!name_decl)
> > +return false;
> > + if (!DECL_CONTEXT (fndecl))
> > +return false;
> > + if (TREE_CODE (DECL_CONTEXT (fndecl)) != NAMESPACE_DECL)
> > +return false;
> > + tree ns = DECL_CONTEXT (fndecl);
> > + if (!(DECL_CONTEXT (ns) == NULL_TREE
> > + || TREE_CODE (DECL_CONTEXT (ns)) == TRANSLATION_UNIT_DECL))
> > +return false;
> > + if (!DECL_NAME (ns))
> > +return false;
> > + return id_equal ("std", DECL_NAME (ns));
> > +}
>
> Sounds a bit elaborate, doesn't?
> I hope this is optimized to
>
> static inline bool
> is_std_function_p (const_tree fndecl)
> {
> tree name_decl = DECL_NAME (fndecl);
> if (!name_decl)
> return false;
> tree ns = DECL_CONTEXT (fndecl);
> if (!ns)
> return false;
> if (TREE_CODE (ns) != NAMESPACE_DECL)
> return false;
> if (!(DECL_CONTEXT (ns) == NULL_TREE
> || TREE_CODE (DECL_CONTEXT (ns)) == TRANSLATION_UNIT_DECL))
> return false;
> if (!DECL_NAME (ns))
> return false;
> return id_equal ("std", DECL_NAME (ns));
> }
>
> isn't "std" spelled out std_identifier() and is an identifier, i.e.
> return DECL_NAME (ns) == std_identifier; ?
We have decl_in_std_namespace_p for that.
Marek
[committed] analyzer: handle __builtin_expect [PR93993]
The false warning:
pr93993.f90:19:0:
19 | allocate (tm) ! { dg-warning "dereference of possibly-NULL" }
|
Warning: dereference of possibly-NULL ‘_6’ [CWE-690]
[-Wanalyzer-possible-null-dereference]
in the reproducer for PR analyzer/93993 is due to a BUILTIN_EXPECT in
the chain of SSA expressions between the malloc and the condition
guarding the edge: the analyzer didn't "know" about the relationship
between initial argument to BUILTIN_EXPECT and the return value.
This patch implements support for BUILTIN_EXPECT so that the return
value is known to be equal to the initial argument. This adds
constraints when exploring the CFG edges, eliminating the above
false positive.
Doing so also eliminated the leak warning from the reproducer. The
issue was that leaked_pvs was empty within
impl_region_model_context::on_state_leak, due to the leaking region
being a view, of type struct Pdtet_8 *, of a region of type
struct pdtet_8 *, which led region_model::get_representative_path_var to
return a NULL_TREE value.
Hence the patch also implements view support for
region_model::get_representative_path_var, restoring the leak
diagnostic, albeit changing the wording to:
Warning: leak of ‘(struct Pdtet_8) qb’ [CWE-401] [-Wanalyzer-malloc-leak]
It's not clear to me if we should emit leaks at a fortran "end program"
(currently we suppress them for leaks at the end of main).
Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu.
Pushed to master as r10-7024-ge516294a1acb28d44cfd583cc6a80354044e.
gcc/analyzer/ChangeLog:
PR analyzer/93993
* region-model.cc (region_model::on_call_pre): Handle
BUILT_IN_EXPECT and its variants.
(region_model::add_any_constraints_from_ssa_def_stmt): Split out
gassign handling into add_any_constraints_from_gassign; add gcall
handling.
(region_model::add_any_constraints_from_gassign): New function,
based on the above. Add handling for NOP_EXPR.
(region_model::add_any_constraints_from_gcall): New function.
(region_model::get_representative_path_var): Handle views.
* region-model.h
(region_model::add_any_constraints_from_ssa_def_stmt): New decl.
(region_model::add_any_constraints_from_gassign): New decl.
gcc/testsuite/ChangeLog:
PR analyzer/93993
* gcc.dg/analyzer/expect-1.c: New test.
* gcc.dg/analyzer/malloc-4.c: New test.
* gfortran.dg/analyzer/pr93993.f90: Remove xfail from dg-bogus.
Move location of leak warning and update message.
---
gcc/analyzer/region-model.cc | 71 +--
gcc/analyzer/region-model.h | 8 +++
gcc/testsuite/gcc.dg/analyzer/expect-1.c | 32 +
gcc/testsuite/gcc.dg/analyzer/malloc-4.c | 20 ++
.../gfortran.dg/analyzer/pr93993.f90 | 6 +-
5 files changed, 128 insertions(+), 9 deletions(-)
create mode 100644 gcc/testsuite/gcc.dg/analyzer/expect-1.c
create mode 100644 gcc/testsuite/gcc.dg/analyzer/malloc-4.c
diff --git a/gcc/analyzer/region-model.cc b/gcc/analyzer/region-model.cc
index b2179bd220a..6813117968f 100644
--- a/gcc/analyzer/region-model.cc
+++ b/gcc/analyzer/region-model.cc
@@ -4204,6 +4204,19 @@ region_model::on_call_pre (const gcall *call,
region_model_context *ctxt)
}
return false;
}
+ else if (gimple_call_builtin_p (call, BUILT_IN_EXPECT)
+ || gimple_call_builtin_p (call, BUILT_IN_EXPECT_WITH_PROBABILITY)
+ || gimple_call_internal_p (call, IFN_BUILTIN_EXPECT))
+ {
+ /* __builtin_expect's return value is its initial argument. */
+ if (!lhs_rid.null_p ())
+ {
+ tree initial_arg = gimple_call_arg (call, 0);
+ svalue_id sid = get_rvalue (initial_arg, ctxt);
+ set_value (lhs_rid, sid, ctxt);
+ }
+ return false;
+ }
else if (is_named_call_p (callee_fndecl, "strlen", call, 1))
{
region_id buf_rid = deref_rvalue (gimple_call_arg (call, 0), ctxt);
@@ -5447,28 +5460,46 @@ region_model::add_any_constraints_from_ssa_def_stmt
(tree lhs,
if (TREE_CODE (lhs) != SSA_NAME)
return;
- if (rhs != boolean_false_node)
+ if (!zerop (rhs))
return;
if (op != NE_EXPR && op != EQ_EXPR)
return;
+ gimple *def_stmt = SSA_NAME_DEF_STMT (lhs);
+ if (const gassign *assign = dyn_cast (def_stmt))
+add_any_constraints_from_gassign (op, rhs, assign, ctxt);
+ else if (gcall *call = dyn_cast (def_stmt))
+add_any_constraints_from_gcall (op, rhs, call, ctxt);
+}
+
+/* Add any constraints for an SSA_NAME defined by ASSIGN
+ where the result OP RHS. */
+
+void
+region_model::add_any_constraints_from_gassign (enum tree_code op,
+ tree rhs,
+ const gassign *assign,
+ region_model_context
Re: [PATCH v2 3/3] Keep .GCC.command.line sections of LTO objetcs.
On 3/4/20 4:34 PM, Andreas Schwab wrote: On Mär 04 2020, Richard Biener wrote: --record-gcc-command-line is not a FSF GCC option, there's -frecord-gcc-switches though which --record-gcc-command-line is translated to -frecord-gcc-switches by the driver. That happens for all double-dash options that match an -f option. I think there is a misunderstanding here. I have just implemented --record-gcc-command-line. I have been testing it and no it does not act as -frecord-gcc-switches. Regards Egeyar Andreas.
[committed] analyzer: fix ICE on non-lvalue in prune_for_sm_diagnostic [PR93993]
PR analyzer/93993 reports another ICE within
diagnostic_manager::prune_for_sm_diagnostic in which the expression
of interest becomes a non-lvalue (similar to PR 93544, PR 93647, and
PR 93950), due to attempting to get an lvalue for a non-lvalue with a
NULL context, leading to an ICE when the failure is reported to
make_region_for_unexpected_tree_code. The tree in question is
an ADDR_EXPR of a VAR_DECL, due to:
event 11: switching var of interest from ‘tm’ in callee to ‘’ in caller
This patch adds more bulletproofing to the routine by introducing
a tentative_region_model_context class that can be passed in such
circumstances which records that an error occurred, and then
checking to see if an error was recorded, thus avoiding the ICE.
This is papering over the problem, but a better solution seems more
like stage 1 material.
The patch also refactors the error-checking for CONSTANT_CLASS_P.
The testcase pr93993.f90 has a false positive:
pr93993.f90:19:0:
19 | allocate (tm) ! { dg-warning "dereference of possibly-NULL" }
|
Warning: dereference of possibly-NULL ‘_6’ [CWE-690]
[-Wanalyzer-possible-null-dereference]
which appears to be a pre-existing bug affecting any allocate call in
Fortran, which I will fix in a followup.
Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu.
Pushed to master as r10-7023-g3d66e153b40ed000af30a9e569a05f34d5d576aa.
gcc/analyzer/ChangeLog:
PR analyzer/93993
* checker-path.h (state_change_event::get_lvalue): Add ctxt param
and pass it to region_model::get_value call.
* diagnostic-manager.cc (get_any_origin): Pass a
tentative_region_model_context to the calls to get_lvalue and reject
the comparison if errors occur.
(can_be_expr_of_interest_p): New function.
(diagnostic_manager::prune_for_sm_diagnostic): Replace checks for
CONSTANT_CLASS_P with calls to update_for_unsuitable_sm_exprs.
Pass a tentative_region_model_context to the calls to
state_change_event::get_lvalue and reject the comparison if errors
occur.
(diagnostic_manager::update_for_unsuitable_sm_exprs): New.
* diagnostic-manager.h
(diagnostic_manager::update_for_unsuitable_sm_exprs): New decl.
* region-model.h (class tentative_region_model_context): New class.
gcc/testsuite/ChangeLog:
PR analyzer/93993
* gfortran.dg/analyzer/pr93993.f90: New test.
---
gcc/analyzer/checker-path.h | 4 +-
gcc/analyzer/diagnostic-manager.cc| 90 ---
gcc/analyzer/diagnostic-manager.h | 1 +
gcc/analyzer/region-model.h | 48 ++
.../gfortran.dg/analyzer/pr93993.f90 | 33 +++
5 files changed, 142 insertions(+), 34 deletions(-)
create mode 100644 gcc/testsuite/gfortran.dg/analyzer/pr93993.f90
diff --git a/gcc/analyzer/checker-path.h b/gcc/analyzer/checker-path.h
index 30cb43c13ba..2eead25f058 100644
--- a/gcc/analyzer/checker-path.h
+++ b/gcc/analyzer/checker-path.h
@@ -201,9 +201,9 @@ public:
label_text get_desc (bool can_colorize) const FINAL OVERRIDE;
- region_id get_lvalue (tree expr) const
+ region_id get_lvalue (tree expr, region_model_context *ctxt) const
{
-return m_dst_state.m_region_model->get_lvalue (expr, NULL);
+return m_dst_state.m_region_model->get_lvalue (expr, ctxt);
}
const supernode *m_node;
diff --git a/gcc/analyzer/diagnostic-manager.cc
b/gcc/analyzer/diagnostic-manager.cc
index 7435092e2d7..1b2c3ce68fa 100644
--- a/gcc/analyzer/diagnostic-manager.cc
+++ b/gcc/analyzer/diagnostic-manager.cc
@@ -574,9 +574,14 @@ get_any_origin (const gimple *stmt,
if (const gassign *assign = dyn_cast (stmt))
{
tree lhs = gimple_assign_lhs (assign);
- /* Use region IDs to compare lhs with DST_REP. */
- if (dst_state.m_region_model->get_lvalue (lhs, NULL)
- == dst_state.m_region_model->get_lvalue (dst_rep, NULL))
+ /* Use region IDs to compare lhs with DST_REP, bulletproofing against
+cases where they can't have lvalues by using
+tentative_region_model_context. */
+ tentative_region_model_context ctxt;
+ region_id lhs_rid = dst_state.m_region_model->get_lvalue (lhs, );
+ region_id dst_rep_rid
+ = dst_state.m_region_model->get_lvalue (dst_rep, );
+ if (lhs_rid == dst_rep_rid && !ctxt.had_errors_p ())
{
tree rhs1 = gimple_assign_rhs1 (assign);
enum tree_code op = gimple_assign_rhs_code (assign);
@@ -1059,6 +1064,25 @@ diagnostic_manager::prune_path (checker_path *path,
path->maybe_log (get_logger (), "pruned");
}
+/* A cheap test to determine if EXPR can be the expression of interest in
+ an sm-diagnostic, so that we can reject cases where we have a non-lvalue.
+ We don't have always have a model when calling this, so we can't use
+ tentative_region_model_context, so there can be false positives. */
+
+static bool
[committed] analyzer: remove unused private fields
Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu.
Pushed to master as 13e3ba14eccc0b1ccf1ba9de90443ec7e524f2a6.
gcc/analyzer/ChangeLog:
* engine.cc (worklist::worklist): Remove unused field m_eg.
(class viz_callgraph_edge): Remove unused field m_call_sedge.
(class viz_callgraph): Remove unused field m_sg.
* exploded-graph.h (worklistm_eg): Remove unused field.
---
gcc/analyzer/engine.cc| 18 +-
gcc/analyzer/exploded-graph.h | 1 -
2 files changed, 5 insertions(+), 14 deletions(-)
diff --git a/gcc/analyzer/engine.cc b/gcc/analyzer/engine.cc
index 9c3b5adc09b..e411d5b40e7 100644
--- a/gcc/analyzer/engine.cc
+++ b/gcc/analyzer/engine.cc
@@ -1654,8 +1654,7 @@ strongly_connected_components::strong_connect (unsigned
index)
/* worklist's ctor. */
worklist::worklist (const exploded_graph , const analysis_plan )
-: m_eg (eg),
- m_scc (eg.get_supergraph (), eg.get_logger ()),
+: m_scc (eg.get_supergraph (), eg.get_logger ()),
m_plan (plan),
m_queue (key_t (*this, NULL))
{
@@ -3602,10 +3601,8 @@ private:
class viz_callgraph_edge : public dedge
{
public:
- viz_callgraph_edge (viz_callgraph_node *src, viz_callgraph_node *dest,
-const call_superedge *call_sedge)
- : dedge (src, dest),
-m_call_sedge (call_sedge)
+ viz_callgraph_edge (viz_callgraph_node *src, viz_callgraph_node *dest)
+ : dedge (src, dest)
{}
void dump_dot (graphviz_out *gv, const dump_args_t &) const
@@ -3627,9 +3624,6 @@ public:
style, color, weight, constraint);
pp_printf (pp, "\"];\n");
}
-
-private:
- const call_superedge * const m_call_sedge;
};
/* Subclass of digraph representing the callgraph. */
@@ -3650,7 +3644,6 @@ public:
}
private:
- const supergraph _sg;
hash_map m_map;
};
@@ -3663,7 +3656,6 @@ class viz_callgraph_cluster : public
cluster
/* viz_callgraph's ctor. */
viz_callgraph::viz_callgraph (const supergraph )
-: m_sg (sg)
{
cgraph_node *node;
FOR_EACH_FUNCTION_WITH_GIMPLE_BODY (node)
@@ -3682,11 +3674,11 @@ viz_callgraph::viz_callgraph (const supergraph )
viz_callgraph_node *vcg_src = get_vcg_node_for_snode (sedge->m_src);
if (vcg_src->m_fun)
get_vcg_node_for_function (vcg_src->m_fun)->m_num_superedges++;
- if (const call_superedge *call_sedge = sedge->dyn_cast_call_superedge ())
+ if (sedge->dyn_cast_call_superedge ())
{
viz_callgraph_node *vcg_dest = get_vcg_node_for_snode (sedge->m_dest);
viz_callgraph_edge *vcg_edge
- = new viz_callgraph_edge (vcg_src, vcg_dest, call_sedge);
+ = new viz_callgraph_edge (vcg_src, vcg_dest);
add_edge (vcg_edge);
}
}
diff --git a/gcc/analyzer/exploded-graph.h b/gcc/analyzer/exploded-graph.h
index c3bd383e924..c0a520a9961 100644
--- a/gcc/analyzer/exploded-graph.h
+++ b/gcc/analyzer/exploded-graph.h
@@ -710,7 +710,6 @@ private:
/* The order is important here: m_scc needs to stick around
until after m_queue has finished being cleaned up (the dtor
calls the ordering fns). */
- const exploded_graph _eg;
strongly_connected_components m_scc;
const analysis_plan _plan;
--
2.21.0
Re: [committed] analyzer: detect malloc, free, calloc within "std" [PR93959]
On Mon, 2 Mar 2020 16:48:26 -0500
David Malcolm wrote:
> +static inline bool
> +is_std_function_p (const_tree fndecl)
> +{
> + tree name_decl = DECL_NAME (fndecl);
> + if (!name_decl)
> +return false;
> + if (!DECL_CONTEXT (fndecl))
> +return false;
> + if (TREE_CODE (DECL_CONTEXT (fndecl)) != NAMESPACE_DECL)
> +return false;
> + tree ns = DECL_CONTEXT (fndecl);
> + if (!(DECL_CONTEXT (ns) == NULL_TREE
> + || TREE_CODE (DECL_CONTEXT (ns)) == TRANSLATION_UNIT_DECL))
> +return false;
> + if (!DECL_NAME (ns))
> +return false;
> + return id_equal ("std", DECL_NAME (ns));
> +}
Sounds a bit elaborate, doesn't?
I hope this is optimized to
static inline bool
is_std_function_p (const_tree fndecl)
{
tree name_decl = DECL_NAME (fndecl);
if (!name_decl)
return false;
tree ns = DECL_CONTEXT (fndecl);
if (!ns)
return false;
if (TREE_CODE (ns) != NAMESPACE_DECL)
return false;
if (!(DECL_CONTEXT (ns) == NULL_TREE
|| TREE_CODE (DECL_CONTEXT (ns)) == TRANSLATION_UNIT_DECL))
return false;
if (!DECL_NAME (ns))
return false;
return id_equal ("std", DECL_NAME (ns));
}
isn't "std" spelled out std_identifier() and is an identifier, i.e.
return DECL_NAME (ns) == std_identifier; ?
thanks,
[RFA/RFC] [tree-optimization/91890] [P1 Regression] Avoid clobbering useful location in Wrestrict code
Martin, I'd like your thoughts here.
As noted in the BZ our #pragmas aren't working to suppress a warning.
I did some debugging and ultimately found that the location passed down to the
diagnostic code is indeed outside the scope of the pragmas.
Further digging uncovered that we had a reasonable location passed to
maybe_diag_access_bounds, but rather than using it, we extracted a location
attached to the builtin_memref structure.
So if we look at the test:
char one[50];
char two[50];
void
test_strncat (void)
{
(void) __builtin_strcpy (one, "gh");
(void) __builtin_strcpy (two, "ef");
#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wstringop-overflow="
#pragma GCC diagnostic ignored "-Warray-bounds"
(void) __builtin_strncat (one, two, 99);
#pragma GCC diagnostic pop
}
The location we end up passing to the diagnostic code comes from the destination
of the call (via DSTREF which is passed to maybe_diag_access_bounds and becomes
REF in that context).
> (gdb) p debug_tree (ref.ptr)
> type type char>
> BLK
> size
> unit-size
> align:8 warn_if_not_align:0 symtab:0 alias-set -1 canonical-type
> 0x7fffea9372a0 domain
> pointer_to_this >
> unsigned DI
> size
> unit-size
> align:64 warn_if_not_align:0 symtab:0 alias-set -1 canonical-type
> 0x7fffea937540>
> constant
> arg:0
> addressable used public static read BLK j.c:2:6 size 0x7fffea81c3a8 400> unit-size
> align:256 warn_if_not_align:0 context 0x7fffea80bac8 j.c>
> chain
> addressable used public static read BLK j.c:3:6 size 0x7fffea81c3a8 400> unit-size
> align:256 warn_if_not_align:0 context 0x7fffea80bac8 j.c> chain >>
> j.c:8:28 start: j.c:8:28 finish: j.c:8:30>
>
Note the location. j.c line 8 column 28-30. THat location makes absolutely no
sense (it's the first call to strcpy) given we're trying to diagnose the
strncat.
I have no idea why we're using the location from ref.ptr rather than the
location
of the statement we're analyzing (which is passed in as LOC). It's no surprise
the pragma didn't work.
Anyway, this patch seems to fix the problem and doesn't regress anything. But
I'm hesitant to go forward with it given I don't know the motivation behind
extracting the location from ref.ptr.
Another thought would be to just remove the code that pulls the location from
ref.ptr. I haven't tested that, but certainly could easily.
Martin, thoughts?
PR tree-optimization/91890
* gimple-ssa-warn-restrict.c (maybe_diag_access_bounds): Only
extract a location from the reference if there is currently
no valid location information.
* gcc.dg/pragma-diag-8.c: New test.
diff --git a/gcc/gimple-ssa-warn-restrict.c b/gcc/gimple-ssa-warn-restrict.c
index 2c582a670eb..9421d3d6899 100644
--- a/gcc/gimple-ssa-warn-restrict.c
+++ b/gcc/gimple-ssa-warn-restrict.c
@@ -1711,7 +1711,7 @@ maybe_diag_access_bounds (location_t loc, gimple *call,
tree func, int strict,
if (warn_stringop_overflow)
{
- if (EXPR_HAS_LOCATION (ref.ptr))
+ if (loc == UNKNOWN_LOCATION && EXPR_HAS_LOCATION (ref.ptr))
loc = EXPR_LOCATION (ref.ptr);
loc = expansion_point_location_if_in_system_header (loc);
@@ -1754,7 +1754,7 @@ maybe_diag_access_bounds (location_t loc, gimple *call,
tree func, int strict,
|| (ref.ref && TREE_NO_WARNING (ref.ref)))
return false;
- if (EXPR_HAS_LOCATION (ref.ptr))
+ if (loc == UNKNOWN_LOCATION && EXPR_HAS_LOCATION (ref.ptr))
loc = EXPR_LOCATION (ref.ptr);
loc = expansion_point_location_if_in_system_header (loc);
diff --git a/gcc/testsuite/gcc.dg/pragma-diag-8.c
b/gcc/testsuite/gcc.dg/pragma-diag-8.c
new file mode 100644
index 000..00780606e9b
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pragma-diag-8.c
@@ -0,0 +1,20 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -Wall" } */
+
+
+char one[50];
+char two[50];
+
+void
+test_strncat (void)
+{
+ (void) __builtin_strcpy (one, "gh");
+ (void) __builtin_strcpy (two, "ef");
+
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wstringop-overflow="
+#pragma GCC diagnostic ignored "-Warray-bounds"
+ (void) __builtin_strncat (one, two, 99);
+#pragma GCC diagnostic pop
+}
+
[PATCH] libstdc++: Workaround is_trivially_copyable (PR 94013)
Several algorithms check the is_trivially_copyable trait to decide
whether to dispatch to memmove or memcmp as an optimization. Since
r271435 (CWG DR 2094) the trait is true for volatile-qualified scalars,
but we can't use memmove or memcmp when the type is volatile. We need to
also check for volatile types.
This is complicated by the fact that in C++20 (but not earlier standards)
iterator_traits::value_type is T, so we can't just check
whether the value_type is volatile.
The solution in this patch is to introduce new traits __memcpyable and
__memcmpable which combine into a single trait the checks for pointers,
the value types being the same, and the type being trivially copyable
but not volatile-qualified.
PR libstdc++/94013
* include/bits/cpp_type_traits.h (__memcpyable, __memcmpable): New
traits to control when to use memmove and memcmp optimizations.
(__is_nonvolatile_trivially_copyable): New helper trait.
* include/bits/ranges_algo.h (__lexicographical_compare_fn): Do not
use memcmp optimization with volatile data.
* include/bits/ranges_algobase.h (__equal_fn): Use __memcmpable.
(__copy_or_move, __copy_or_move_backward): Use __memcpyable.
* include/bits/stl_algobase.h (__copy_move_a2): Use __memcpyable.
(__copy_move_backward_a2): Likewise.
(__equal_aux1): Use __memcmpable.
(__lexicographical_compare_aux): Do not use memcmp optimization with
volatile data.
* testsuite/25_algorithms/copy/94013.cc: New test.
* testsuite/25_algorithms/copy_backward/94013.cc: New test.
* testsuite/25_algorithms/equal/94013.cc: New test.
* testsuite/25_algorithms/fill/94013.cc: New test.
* testsuite/25_algorithms/lexicographical_compare/94013.cc: New test.
* testsuite/25_algorithms/move/94013.cc: New test.
* testsuite/25_algorithms/move_backward/94013.cc: New test.
I committed this yesterday but forgot to send the email. The attached
patch also contains a follow up change that I've just committed,
improving some comments.
Tested powerpc64le-linux.
commit 462f6c2041fad058abcdd5122e99a024f69a39d5
Author: Jonathan Wakely
Date: Tue Mar 3 21:38:57 2020 +
libstdc++: Workaround is_trivially_copyable (PR 94013)
Several algorithms check the is_trivially_copyable trait to decide
whether to dispatch to memmove or memcmp as an optimization. Since
r271435 (CWG DR 2094) the trait is true for volatile-qualified scalars,
but we can't use memmove or memcmp when the type is volatile. We need to
also check for volatile types.
This is complicated by the fact that in C++20 (but not earlier standards)
iterator_traits::value_type is T, so we can't just check
whether the value_type is volatile.
The solution in this patch is to introduce new traits __memcpyable and
__memcmpable which combine into a single trait the checks for pointers,
the value types being the same, and the type being trivially copyable
but not volatile-qualified.
PR libstdc++/94013
* include/bits/cpp_type_traits.h (__memcpyable, __memcmpable): New
traits to control when to use memmove and memcmp optimizations.
(__is_nonvolatile_trivially_copyable): New helper trait.
* include/bits/ranges_algo.h (__lexicographical_compare_fn): Do not
use memcmp optimization with volatile data.
* include/bits/ranges_algobase.h (__equal_fn): Use __memcmpable.
(__copy_or_move, __copy_or_move_backward): Use __memcpyable.
* include/bits/stl_algobase.h (__copy_move_a2): Use __memcpyable.
(__copy_move_backward_a2): Likewise.
(__equal_aux1): Use __memcmpable.
(__lexicographical_compare_aux): Do not use memcmp optimization with
volatile data.
* testsuite/25_algorithms/copy/94013.cc: New test.
* testsuite/25_algorithms/copy_backward/94013.cc: New test.
* testsuite/25_algorithms/equal/94013.cc: New test.
* testsuite/25_algorithms/fill/94013.cc: New test.
* testsuite/25_algorithms/lexicographical_compare/94013.cc: New
test.
* testsuite/25_algorithms/move/94013.cc: New test.
* testsuite/25_algorithms/move_backward/94013.cc: New test.
diff --git a/libstdc++-v3/include/bits/cpp_type_traits.h
b/libstdc++-v3/include/bits/cpp_type_traits.h
index 63b6d6c346f..fac6e4bbea2 100644
--- a/libstdc++-v3/include/bits/cpp_type_traits.h
+++ b/libstdc++-v3/include/bits/cpp_type_traits.h
@@ -420,6 +420,65 @@ __INT_N(__GLIBCXX_TYPE_INT_N_3)
};
#endif
+ template struct iterator_traits;
+
+ // A type that is safe for use with memcpy, memmove, memcmp etc.
+ template
+struct __is_nonvolatile_trivially_copyable
+{
+ enum { __value = __is_trivially_copyable(_Tp) };
+};
+
+ // Cannot use memcpy/memmove/memcmp on volatile
Re: [PATCH v2 3/3] Keep .GCC.command.line sections of LTO objetcs.
On Mär 04 2020, Richard Biener wrote: > --record-gcc-command-line is not a FSF GCC option, there's > -frecord-gcc-switches though which --record-gcc-command-line is translated to -frecord-gcc-switches by the driver. That happens for all double-dash options that match an -f option. Andreas. -- Andreas Schwab, SUSE Labs, sch...@suse.de GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7 "And now for something completely different."
Re: [PATCH v2 3/3] Keep .GCC.command.line sections of LTO objetcs.
On 3/4/20 10:00 AM, Richard Biener wrote: On Tue, Mar 3, 2020 at 5:41 PM Egeyar Bagcioglu wrote: This patch is for .GCC.command.line sections in LTO objects to be copied into the final objects as in the following example: [egeyar@localhost lto]$ gcc -flto -O3 demo.c -c -g --record-gcc-command-line [egeyar@localhost lto]$ gcc -flto -O2 demo2.c -c -g --record-gcc-command-line -DFORTIFY=2 [egeyar@localhost lto]$ gcc demo.o demo2.o -o a.out [egeyar@localhost lto]$ readelf -p .GCC.command.line a.out String dump of section '.GCC.command.line': [ 0] 10.0.1 20200227 (experimental) : gcc -flto -O3 demo.c -c -g --record-gcc-command-line [56] 10.0.1 20200227 (experimental) : gcc -flto -O2 demo2.c -c -g --record-gcc-command-line -DFORTIFY=2 --record-gcc-command-line is not a FSF GCC option, there's -frecord-gcc-switches though which (also) populates .GCC.command.line Right. This is also necessary for preserving the outcome of -frecord-gcc-switches option and that issue is reported to me by Martin Liska. OK. Thanks Richard. I do not have write-access to the GCC repo. I'd be glad if someone commits it for me. Regards Egeyar Thanks, Richard. Regards Egeyar libiberty: 2020-02-27 Egeyar Bagcioglu * simple-object.c (handle_lto_debug_sections): Name ".GCC.command.line" among debug sections to be copied over from lto objects. --- libiberty/simple-object.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/libiberty/simple-object.c b/libiberty/simple-object.c index d9c648a..3b3ca9c 100644 --- a/libiberty/simple-object.c +++ b/libiberty/simple-object.c @@ -298,6 +298,9 @@ handle_lto_debug_sections (const char *name, int rename) COMDAT sections in objects produced by GCC. */ else if (strcmp (name, ".comment") == 0) return strcpy (newname, name); + /* Copy over .GCC.command.line section under the same name if present. */ + else if (strcmp (name, ".GCC.command.line") == 0) +return strcpy (newname, name); free (newname); return NULL; } -- 1.8.3.1
Re: [PING][PATCH][GCC][ARM] Arm generates out of range conditional branches in Thumb2 (PR91816)
On 3/4/20 2:14 PM, Tamar Christina wrote:
Hi Kyrill,
Ok for backporting this patch to GCC 8 and GCC 9?
Ok assuming bootstrap and test shows no problems.
Thanks,
Kyrill
Thanks,
Tamar
-Original Message-
From: gcc-patches-ow...@gcc.gnu.org
On Behalf Of Kyrill Tkachov
Sent: Thursday, January 30, 2020 14:55
To: Stam Markianos-Wright ; gcc-
patc...@gcc.gnu.org
Cc: ni...@redhat.com; Ramana Radhakrishnan
; Richard Earnshaw
Subject: Re: [PING][PATCH][GCC][ARM] Arm generates out of range
conditional branches in Thumb2 (PR91816)
On 1/30/20 2:42 PM, Stam Markianos-Wright wrote:
On 1/28/20 10:35 AM, Kyrill Tkachov wrote:
Hi Stam,
On 1/8/20 3:18 PM, Stam Markianos-Wright wrote:
On 12/10/19 5:03 PM, Kyrill Tkachov wrote:
Hi Stam,
On 11/15/19 5:26 PM, Stam Markianos-Wright wrote:
Pinging with more correct maintainers this time :)
Also would need to backport to gcc7,8,9, but need to get this
approved first!
Sorry for the delay.
Same here now! Sorry totally forget about this in the lead up to Xmas!
Done the changes marked below and also removed the unnecessary
extra
#defines from the test.
This is ok with a nit on the testcase...
diff --git a/gcc/testsuite/gcc.target/arm/pr91816.c
b/gcc/testsuite/gcc.target/arm/pr91816.c
new file mode 100644
index
..757c897e9c0db32709227b3fdf
1
b4a8033428232
--- /dev/null
+++ b/gcc/testsuite/gcc.target/arm/pr91816.c
@@ -0,0 +1,61 @@
+/* { dg-do compile } */
+/* { dg-options "-march=armv7-a -mthumb -mfpu=vfpv3-d16" } */ int
+printf(const char *, ...);
+
I think this needs a couple of effective target checks like
arm_hard_vfp_ok and arm_thumb2_ok. See other tests in gcc.target/arm
that add -mthumb to the options.
Hmm, looking back at this now, is there any reason why it can't just be:
/* { dg-do compile } */
/* { dg-require-effective-target arm_thumb2_ok } */
/* { dg-additional-options "-mthumb" } */
were we don't override the march or fpu options at all, but just use
`require-effective-target arm_thumb2_ok` to make sure that thumb2 is
supported?
The attached new diff does just that.
Works for me, there are plenty of configurations run with fpu that it should
get the right coverage.
Ok (make sure commit the updated, if needed, ChangeLog as well)
Thanks!
Kyrill
Cheers :)
Stam.
Thanks,
Kyrill
RE: [PING][PATCH][GCC][ARM] Arm generates out of range conditional branches in Thumb2 (PR91816)
Hi Kyrill,
Ok for backporting this patch to GCC 8 and GCC 9?
Thanks,
Tamar
> -Original Message-
> From: gcc-patches-ow...@gcc.gnu.org
> On Behalf Of Kyrill Tkachov
> Sent: Thursday, January 30, 2020 14:55
> To: Stam Markianos-Wright ; gcc-
> patc...@gcc.gnu.org
> Cc: ni...@redhat.com; Ramana Radhakrishnan
> ; Richard Earnshaw
>
> Subject: Re: [PING][PATCH][GCC][ARM] Arm generates out of range
> conditional branches in Thumb2 (PR91816)
>
>
> On 1/30/20 2:42 PM, Stam Markianos-Wright wrote:
> >
> >
> > On 1/28/20 10:35 AM, Kyrill Tkachov wrote:
> >> Hi Stam,
> >>
> >> On 1/8/20 3:18 PM, Stam Markianos-Wright wrote:
> >>>
> >>> On 12/10/19 5:03 PM, Kyrill Tkachov wrote:
> Hi Stam,
>
> On 11/15/19 5:26 PM, Stam Markianos-Wright wrote:
> > Pinging with more correct maintainers this time :)
> >
> > Also would need to backport to gcc7,8,9, but need to get this
> > approved first!
> >
> Sorry for the delay.
> >>> Same here now! Sorry totally forget about this in the lead up to Xmas!
> >>>
> >>> Done the changes marked below and also removed the unnecessary
> extra
> >>> #defines from the test.
> >>
> >>
> >> This is ok with a nit on the testcase...
> >>
> >>
> >> diff --git a/gcc/testsuite/gcc.target/arm/pr91816.c
> >> b/gcc/testsuite/gcc.target/arm/pr91816.c
> >> new file mode 100644
> >> index
> >>
> ..757c897e9c0db32709227b3fdf
> 1
> >> b4a8033428232
> >> --- /dev/null
> >> +++ b/gcc/testsuite/gcc.target/arm/pr91816.c
> >> @@ -0,0 +1,61 @@
> >> +/* { dg-do compile } */
> >> +/* { dg-options "-march=armv7-a -mthumb -mfpu=vfpv3-d16" } */ int
> >> +printf(const char *, ...);
> >> +
> >>
> >> I think this needs a couple of effective target checks like
> >> arm_hard_vfp_ok and arm_thumb2_ok. See other tests in gcc.target/arm
> >> that add -mthumb to the options.
> >
> > Hmm, looking back at this now, is there any reason why it can't just be:
> >
> > /* { dg-do compile } */
> > /* { dg-require-effective-target arm_thumb2_ok } */
> > /* { dg-additional-options "-mthumb" } */
> >
> > were we don't override the march or fpu options at all, but just use
> > `require-effective-target arm_thumb2_ok` to make sure that thumb2 is
> > supported?
> >
> > The attached new diff does just that.
> >
>
> Works for me, there are plenty of configurations run with fpu that it should
> get the right coverage.
>
> Ok (make sure commit the updated, if needed, ChangeLog as well)
>
> Thanks!
>
> Kyrill
>
>
> > Cheers :)
> >
> > Stam.
> >
> >>
> >> Thanks,
> >> Kyrill
> >>
> >
Re: ACLE intrinsics: BFloat16 load intrinsics for AArch32
Hi,
The previous version of this patch shared part of its code with the
store intrinsics patch
(https://gcc.gnu.org/ml/gcc-patches/2020-03/msg00145.html) so I removed
any duplicated code. This patch now depends on the previously mentioned
store intrinsics patch.
Here is the latest version and the updated ChangeLog.
gcc/ChangeLog:
2019-03-04 Delia Burduv
* config/arm/arm_neon.h (bfloat16_t): New typedef.
(vld2_bf16): New.
(vld2q_bf16): New.
(vld3_bf16): New.
(vld3q_bf16): New.
(vld4_bf16): New.
(vld4q_bf16): New.
(vld2_dup_bf16): New.
(vld2q_dup_bf16): New.
(vld3_dup_bf16): New.
(vld3q_dup_bf16): New.
(vld4_dup_bf16): New.
(vld4q_dup_bf16): New.
* config/arm/arm_neon_builtins.def
(vld2): Changed to VAR13 and added v4bf, v8bf
(vld2_dup): Changed to VAR8 and added v4bf, v8bf
(vld3): Changed to VAR13 and added v4bf, v8bf
(vld3_dup): Changed to VAR8 and added v4bf, v8bf
(vld4): Changed to VAR13 and added v4bf, v8bf
(vld4_dup): Changed to VAR8 and added v4bf, v8bf
* config/arm/iterators.md (VDXBF): New iterator.
(VQ2BF): New iterator.
*config/arm/neon.md (vld2): Used new iterators.
(vld2_dup): Used new iterators.
(vld2_dupv8bf): New.
(vst3): Used new iterators.
(vst3qa): Used new iterators.
(vst3qb): Used new iterators.
(vld3_dup): Used new iterators.
(vld3_dupv8bf): New.
(vst4): Used new iterators.
(vst4qa): Used new iterators.
(vst4qb): Used new iterators.
(vld4_dup): Used new iterators.
(vld4_dupv8bf): New.
gcc/testsuite/ChangeLog:
2019-03-04 Delia Burduv
* gcc.target/arm/simd/bf16_vldn_1.c: New test.
Thanks,
Delia
On 2/19/20 5:25 PM, Delia Burduv wrote:
Hi,
Here is the latest version of the patch. It just has some minor
formatting changes that were brought up by Richard Sandiford in the
AArch64 patches
Thanks,
Delia
On 1/22/20 5:31 PM, Delia Burduv wrote:
Ping.
I will change the tests to use the exact input and output registers as
Richard Sandiford suggested for the AArch64 patches.
On 12/20/19 6:48 PM, Delia Burduv wrote:
This patch adds the ARMv8.6 ACLE BFloat16 load intrinsics
vld{q}_bf16 as part of the BFloat16 extension.
(https://developer.arm.com/architectures/instruction-sets/simd-isas/neon/intrinsics)
The intrinsics are declared in arm_neon.h .
A new test is added to check assembler output.
This patch depends on the Arm back-end patche.
(https://gcc.gnu.org/ml/gcc-patches/2019-12/msg01448.html)
Tested for regression on arm-none-eabi and armeb-none-eabi. I don't
have commit rights, so if this is ok can someone please commit it for
me?
gcc/ChangeLog:
2019-11-14 Delia Burduv
* config/arm/arm_neon.h (bfloat16_t): New typedef.
(bfloat16x4x2_t): New typedef.
(bfloat16x8x2_t): New typedef.
(bfloat16x4x3_t): New typedef.
(bfloat16x8x3_t): New typedef.
(bfloat16x4x4_t): New typedef.
(bfloat16x8x4_t): New typedef.
(vld2_bf16): New.
(vld2q_bf16): New.
(vld3_bf16): New.
(vld3q_bf16): New.
(vld4_bf16): New.
(vld4q_bf16): New.
(vld2_dup_bf16): New.
(vld2q_dup_bf16): New.
(vld3_dup_bf16): New.
(vld3q_dup_bf16): New.
(vld4_dup_bf16): New.
(vld4q_dup_bf16): New.
* config/arm/arm-builtins.c (E_V2BFmode): New mode.
(VAR13): New.
(arm_simd_types[Bfloat16x2_t]):New type.
* config/arm/arm-modes.def (V2BF): New mode.
* config/arm/arm-simd-builtin-types.def
(Bfloat16x2_t): New entry.
* config/arm/arm_neon_builtins.def
(vld2): Changed to VAR13 and added v4bf, v8bf
(vld2_dup): Changed to VAR8 and added v4bf, v8bf
(vld3): Changed to VAR13 and added v4bf, v8bf
(vld3_dup): Changed to VAR8 and added v4bf, v8bf
(vld4): Changed to VAR13 and added v4bf, v8bf
(vld4_dup): Changed to VAR8 and added v4bf, v8bf
* config/arm/iterators.md (VDXBF): New iterator.
(VQ2BF): New iterator.
(V_elem): Added V4BF, V8BF.
(V_sz_elem): Added V4BF, V8BF.
(V_mode_nunits): Added V4BF, V8BF.
(q): Added V4BF, V8BF.
*config/arm/neon.md (vld2): Used new iterators.
(vld2_dup): Used new iterators.
(vld2_dupv8bf): New.
(vst3): Used new iterators.
(vst3qa): Used new iterators.
(vst3qb): Used new iterators.
(vld3_dup): Used new iterators.
(vld3_dupv8bf): New.
(vst4): Used new iterators.
(vst4qa): Used new iterators.
(vst4qb): Used new iterators.
(vld4_dup): Used new iterators.
(vld4_dupv8bf): New.
gcc/testsuite/ChangeLog:
2019-11-14 Delia Burduv
* gcc.target/arm/simd/bf16_vldn_1.c: New test.
diff --git
RE: [PATCH] Ada: gcc-interface: fixed assertion for aliased entities
> -Original Message- > From: Eric Botcazou > Sent: March 4, 2020 6:18 AM > To: Richard Wai > Cc: gcc-patches@gcc.gnu.org > Subject: Re: [PATCH] Ada: gcc-interface: fixed assertion for aliased entities > > > I'll have to look into this.. Any pointers? This assertion is not a > > language rule assertion. > > Of course, neither of the 117 assertions in gcc-interface is, instead they are > assertions meant to prevent wrong-code generation from occuring. > Please excuse my ignorance as this is my first (and hopefully not last) patch submission.. But I don't see any testcases in the Ada testsuite except for the (outdated) ACATS tests, which doesn't cover this assertion. So I'm honestly not sure how I should go about that.. > > As you see, the assertion being modified already tests for > > "Is_Public". So the issue is precisely that this assertion wrongly > > fails for cases where the entity is not Public. > > We cannot let anything go through if there is not Is_Public set somewhere, > possibly on the Alias (gnat_entity) since it is present in your case. > My reasoning there was that at decl.c:3914 when the E_Function/E_Procedure that has alias is resolved, there is a recursive call to gnat_to_gnu_entity on the alias. So if the alias was not Public, the same assertion will be triggered on that recursive call, so there seems to be no need to check it at that point. Though I suppose that could leave some holes if the incoming entity is not a subprogram! Shall I add the Is_Public check to the Alias and resubmit the patch? Richard Wai ANNEXI-STRAYLINE
Re: [PATCH] use all same precision in wide_int arguments (PR 93986)
On Wed, Mar 4, 2020 at 1:26 AM Martin Sebor wrote: > > On 3/3/20 11:50 AM, Richard Biener wrote: > > On March 3, 2020 4:39:34 PM GMT+01:00, Martin Sebor > > wrote: > >> On 3/3/20 2:42 AM, Richard Biener wrote: > >>> On Tue, Mar 3, 2020 at 12:04 AM Martin Sebor > >> wrote: > > The wide_int APIs expect operands to have the same precision and > abort when they don't. This is especially insidious in code where > the operands normally do have the same precision but where mixed > precision arguments can come up as a result of unusual combinations > optimization options. That is also what precipitated pr93986. > >>> > >>> If you want sth like (signed) arbitrary precision arithmetic then you > >> can > >>> use widest_int instead. Or, since you're working with offsets, > >> offset_int > >>> is another good choice. > >> > >> Yes, I would much prefer not to have to do all this myself (and > >> risk getting it wrong). Unfortunately, the APIs that obtain > >> the ranges all use wide_int, so I'd have to convert them one way > >> or the other. I could change some of the APIs but not all of > >> them (e.g., get_range_info). > > > > You can convert wide_int to both offset and widest int. > > Yes, I realize that. But it seems like six of one vs half a dozen > of the other. Either way some variables need converting. I don't > really have a preference for either approach. I just copied > the solution I already used in gimple_call_alloc_size, and I chose > that one there because the get_range calls take wide_int, and > because gimple_call_alloc_size's caller (the function I'm changing > now) also uses wide_int. > > Everything could be changed to widest_int instead but I'm not sure > it would make sense (e.g., get_range_info). And unless everything > is changed, the APIs that interoperate need to convert between one > another. > > I went ahead and rewrote the patch to use widest_int. It let me get > rid of some conversions but it introduced others. Most of them look > pretty much the same between the two approaches but there are more > of them with widest_int because of the extra variables. The updated > patch is also about one and half times longer. > > Is one approach significantly better than the other or were you just > pointing out another way of doing it? I was just pointing out other ways of doing it since you weren't happy. > Either way, please choose one and approve. The widest_int one is OK, it looks slightly better to me (no explicit precisions). Thanks, Richard. > Thanks > Martin > > > > > Richard. > > > >> Martin > >> > >> > >>> > The attached patch adjusts the code to extend all wide_int operands > to the same precision to avoid the ICE. > > Besides the usual bootstrap/testing I also compiled all string tests > in gcc.dg with the same options as in the test case in pr93986 in > an effort to weed out any lingering bugs like it (found none). > > Martin > > >
[Committed 4/4] IBM Z: zTPF: Include glibc-stdint.h
Building a zTPF cross currently fails when building libstdc++
complaining about the __UINTPTR_TYPE__ to be missing.
Fixed by including the glibc-stdint.h header.
2020-03-04 Andreas Krebbel
* config.gcc: Include the glibc-stdint.h header for zTPF.
---
gcc/config.gcc | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/gcc/config.gcc b/gcc/config.gcc
index ae5a845fcce..2df4b36d190 100644
--- a/gcc/config.gcc
+++ b/gcc/config.gcc
@@ -3104,7 +3104,7 @@ s390x-*-linux*)
tmake_file="${tmake_file} s390/t-linux64 s390/t-s390"
;;
s390x-ibm-tpf*)
- tm_file="s390/s390x.h s390/s390.h dbxelf.h elfos.h s390/tpf.h"
+ tm_file="s390/s390x.h s390/s390.h dbxelf.h elfos.h glibc-stdint.h
s390/tpf.h"
tm_p_file=s390/s390-protos.h
c_target_objs="${c_target_objs} s390-c.o"
cxx_target_objs="${cxx_target_objs} s390-c.o"
--
2.24.1
[Committed 1/4] IBM Z: zTPF: Add tpf trace customization options
The zTPF OS implements a tracing facility for function entry and exit
which uses global flags and trace function addresses. The addresses of
the flags as well as the trace functions are currently hard-coded in
the zTPF specific GCC parts of the IBM Z back-end.
With this patch these addresses can be changed at compile-time using
the new command line options. For convenience one additional command
line option (-mtpf-trace-skip) implements a new set of hard-coded
addresses.
gcc/ChangeLog:
2020-03-04 Andreas Krebbel
* config/s390/s390.c (s390_emit_prologue): Specify the 2 new
operands to the prologue_tpf expander.
(s390_emit_epilogue): Likewise.
(s390_option_override_internal): Do error checking and setup for
the new options.
* config/s390/tpf.h (TPF_TRACE_PROLOGUE_CHECK)
(TPF_TRACE_EPILOGUE_CHECK, TPF_TRACE_PROLOGUE_TARGET)
(TPF_TRACE_EPILOGUE_TARGET, TPF_TRACE_PROLOGUE_SKIP_TARGET)
(TPF_TRACE_EPILOGUE_SKIP_TARGET): New macro definitions.
* config/s390/tpf.md ("prologue_tpf", "epilogue_tpf"): Add two new
operands for the check flag and the branch target.
* config/s390/tpf.opt ("mtpf-trace-hook-prologue-check")
("mtpf-trace-hook-prologue-target")
("mtpf-trace-hook-epilogue-check")
("mtpf-trace-hook-epilogue-target", "mtpf-trace-skip"): New
options.
* doc/invoke.texi: Document -mtpf-trace-skip option. The other
options are for debugging purposes and will not be documented
here.
---
gcc/config/s390/s390.c | 53 +++--
gcc/config/s390/tpf.h | 16 +
gcc/config/s390/tpf.md | 12 ++
gcc/config/s390/tpf.opt | 20
gcc/doc/invoke.texi | 13 +-
5 files changed, 96 insertions(+), 18 deletions(-)
diff --git a/gcc/config/s390/s390.c b/gcc/config/s390/s390.c
index ebba6704852..31af842eb35 100644
--- a/gcc/config/s390/s390.c
+++ b/gcc/config/s390/s390.c
@@ -11363,17 +11363,21 @@ s390_emit_prologue (void)
emit_insn (insns);
}
+#if TARGET_TPF != 0
if (TARGET_TPF_PROFILING)
{
- /* Generate a BAS instruction to serve as a function
-entry intercept to facilitate the use of tracing
-algorithms located at the branch target. */
- emit_insn (gen_prologue_tpf ());
+ /* Generate a BAS instruction to serve as a function entry
+intercept to facilitate the use of tracing algorithms located
+at the branch target. */
+ emit_insn (gen_prologue_tpf (
+ GEN_INT (s390_tpf_trace_hook_prologue_check),
+ GEN_INT (s390_tpf_trace_hook_prologue_target)));
- /* Emit a blockage here so that all code
-lies between the profiling mechanisms. */
+ /* Emit a blockage here so that all code lies between the
+profiling mechanisms. */
emit_insn (gen_blockage ());
}
+#endif
}
/* Expand the epilogue into a bunch of separate insns. */
@@ -11386,19 +11390,22 @@ s390_emit_epilogue (bool sibcall)
int next_offset;
int i;
+#if TARGET_TPF != 0
if (TARGET_TPF_PROFILING)
{
+ /* Generate a BAS instruction to serve as a function entry
+intercept to facilitate the use of tracing algorithms located
+at the branch target. */
- /* Generate a BAS instruction to serve as a function
-entry intercept to facilitate the use of tracing
-algorithms located at the branch target. */
-
- /* Emit a blockage here so that all code
-lies between the profiling mechanisms. */
+ /* Emit a blockage here so that all code lies between the
+profiling mechanisms. */
emit_insn (gen_blockage ());
- emit_insn (gen_epilogue_tpf ());
+ emit_insn (gen_epilogue_tpf (
+ GEN_INT (s390_tpf_trace_hook_epilogue_check),
+ GEN_INT (s390_tpf_trace_hook_epilogue_target)));
}
+#endif
/* Check whether to use frame or stack pointer for restore. */
@@ -15266,6 +15273,26 @@ s390_option_override_internal (struct gcc_options
*opts,
if (!DISP_IN_RANGE ((1 << param_stack_clash_protection_probe_interval)))
param_stack_clash_protection_probe_interval = 12;
+#if TARGET_TPF != 0
+ if (!CONST_OK_FOR_J (opts->x_s390_tpf_trace_hook_prologue_check))
+error ("-mtpf-trace-hook-prologue-check requires integer in range
0..4095");
+
+ if (!CONST_OK_FOR_J (opts->x_s390_tpf_trace_hook_prologue_target))
+error ("-mtpf-trace-hook-prologue-target requires integer in range
0..4095");
+
+ if (!CONST_OK_FOR_J (opts->x_s390_tpf_trace_hook_epilogue_check))
+error ("-mtpf-trace-hook-epilogue-check requires integer in range
0..4095");
+
+ if (!CONST_OK_FOR_J (opts->x_s390_tpf_trace_hook_epilogue_target))
+error ("-mtpf-trace-hook-epilogue-target requires integer in range
0..4095");
+
+ if (s390_tpf_trace_skip)
+{
+
[Committed 3/4] IBM Z: zTPF: Prevent FPR usage
For the zTPF we must not use floating point registers. gcc/ChangeLog: 2020-03-04 Andreas Krebbel * config/s390/s390.c (s390_secondary_memory_needed): Disallow direct FPR-GPR copies. (s390_register_info_gprtofpr): Disallow GPR content to be saved in FPRs. --- gcc/config/s390/s390.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/gcc/config/s390/s390.c b/gcc/config/s390/s390.c index 31af842eb35..ae2be36e65d 100644 --- a/gcc/config/s390/s390.c +++ b/gcc/config/s390/s390.c @@ -4647,7 +4647,8 @@ s390_secondary_memory_needed (machine_mode mode, && reg_classes_intersect_p (class2, GENERAL_REGS)) || (reg_classes_intersect_p (class1, GENERAL_REGS) && reg_classes_intersect_p (class2, VEC_REGS))) - && (!TARGET_DFP || !TARGET_64BIT || GET_MODE_SIZE (mode) != 8) + && (TARGET_TPF || !TARGET_DFP || !TARGET_64BIT + || GET_MODE_SIZE (mode) != 8) && (!TARGET_VX || (SCALAR_FLOAT_MODE_P (mode) && GET_MODE_SIZE (mode) > 8))); } @@ -9554,7 +9555,7 @@ s390_register_info_gprtofpr () int save_reg_slot = FPR0_REGNUM; int i, j; - if (!TARGET_Z10 || !TARGET_HARD_FLOAT || !crtl->is_leaf) + if (TARGET_TPF || !TARGET_Z10 || !TARGET_HARD_FLOAT || !crtl->is_leaf) return; /* builtin_eh_return needs to be able to modify the return address -- 2.24.1
[Committed 2/4] IBM Z: zTPF: Build libgcc with -mtpf-trace-skip
libgcc is supposed to be built with the trace skip flags and branch
targets. Add a zTPF header file fragment and add the -mtpf-trace-skip
option.
libgcc/ChangeLog:
2020-03-04 Andreas Krebbel
* config.host: Include the new makefile fragment.
* config/s390/t-tpf: New file.
---
libgcc/config.host | 2 +-
libgcc/config/s390/t-tpf | 7 +++
2 files changed, 8 insertions(+), 1 deletion(-)
create mode 100644 libgcc/config/s390/t-tpf
diff --git a/libgcc/config.host b/libgcc/config.host
index 4198dc8d95e..1ff41592337 100644
--- a/libgcc/config.host
+++ b/libgcc/config.host
@@ -1301,7 +1301,7 @@ s390x-*-linux*)
md_unwind_header=s390/linux-unwind.h
;;
s390x-ibm-tpf*)
- tmake_file="${tmake_file} s390/t-crtstuff t-libgcc-pic t-eh-dw2-dip"
+ tmake_file="${tmake_file} s390/t-crtstuff t-libgcc-pic t-eh-dw2-dip
s390/t-tpf"
extra_parts="crtbeginS.o crtendS.o"
md_unwind_header=s390/tpf-unwind.h
;;
diff --git a/libgcc/config/s390/t-tpf b/libgcc/config/s390/t-tpf
new file mode 100644
index 000..50da2239fab
--- /dev/null
+++ b/libgcc/config/s390/t-tpf
@@ -0,0 +1,7 @@
+DFP_ENABLE = true
+
+# Override t-slibgcc-elf-ver to export some libgcc symbols with
+# the symbol versions that glibc used.
+SHLIB_MAPFILES = libgcc-std.ver $(srcdir)/config/s390/libgcc-glibc.ver
+
+HOST_LIBGCC2_CFLAGS += -mlong-double-128 -mtpf-trace-skip
--
2.24.1
Re: [PATCH] sccvn: Fix handling of POINTER_PLUS_EXPR in memset offset [PR93582]
On Wed, 4 Mar 2020, Jakub Jelinek wrote:
> Hi!
>
> On Wed, Mar 04, 2020 at 11:33:06AM +0100, Richard Biener wrote:
> > > where POINTER_PLUS_EXPR last operand has sizetype type, thus unsigned,
> > > and in the testcase gimple_assign_rhs2 (def) is thus 0xf001ULL
> > > which multiplied by 8 doesn't fit into signed HWI. If it would be treated
> > > as signed offset instead, it would fit (-0xfffLL, multiplied
> > > by 8 is -0x7ff8LL). Unfortunately with the poly_int
> > > obfuscation
> > > I'm not sure how to convert it from unsigned to signed poly_int.
> >
> > mem_ref_offset provides a boiler-plate for this:
> >
> > poly_offset_int::from (wi::to_poly_wide (TREE_OPERAND (t, 1)), SIGNED);
>
> Thanks, that seems to work.
> So, is the following ok for trunk if it passes bootstrap/regtest?
> The test now works on both big-endian and little-endian.
Yes.
Thanks,
Richard.
> 2020-03-04 Richard Biener
> Jakub Jelinek
>
> PR tree-optimization/93582
> * tree-ssa-sccvn.c (vn_reference_lookup_3): Treat POINTER_PLUS_EXPR
> last operand as signed when looking for memset offset. Formatting
> fix.
>
> * gcc.dg/tree-ssa/pr93582-11.c: New test.
>
> --- gcc/tree-ssa-sccvn.c.jj 2020-03-04 12:46:09.809610729 +0100
> +++ gcc/tree-ssa-sccvn.c 2020-03-04 13:07:07.982902846 +0100
> @@ -2656,7 +2656,8 @@ vn_reference_lookup_3 (ao_ref *ref, tree
> {
> poly_int64 soff;
> if (TREE_CODE (base) != MEM_REF
> - || !(mem_ref_offset (base) << LOG2_BITS_PER_UNIT).to_shwi ())
> + || !(mem_ref_offset (base)
> +<< LOG2_BITS_PER_UNIT).to_shwi ())
> return (void *)-1;
> offset += soff;
> offset2 = 0;
> @@ -2666,10 +2667,13 @@ vn_reference_lookup_3 (ao_ref *ref, tree
> if (is_gimple_assign (def)
> && gimple_assign_rhs_code (def) == POINTER_PLUS_EXPR
> && gimple_assign_rhs1 (def) == TREE_OPERAND (base, 0)
> - && poly_int_tree_p (gimple_assign_rhs2 (def))
> - && (wi::to_poly_offset (gimple_assign_rhs2 (def))
> - << LOG2_BITS_PER_UNIT).to_shwi ())
> + && poly_int_tree_p (gimple_assign_rhs2 (def)))
> {
> + tree rhs2 = gimple_assign_rhs2 (def);
> + if (!(poly_offset_int::from (wi::to_poly_wide (rhs2),
> +SIGNED)
> + << LOG2_BITS_PER_UNIT).to_shwi ())
> + return (void *)-1;
> ref2 = gimple_assign_rhs1 (def);
> if (TREE_CODE (ref2) == SSA_NAME)
> ref2 = SSA_VAL (ref2);
> --- gcc/testsuite/gcc.dg/tree-ssa/pr93582-11.c.jj 2020-03-04
> 13:12:22.819222647 +0100
> +++ gcc/testsuite/gcc.dg/tree-ssa/pr93582-11.c2020-03-04
> 13:13:47.234968590 +0100
> @@ -0,0 +1,16 @@
> +/* PR tree-optimization/93582 */
> +/* { dg-do compile { target lp64 } } */
> +/* { dg-options "-O2 -fdump-tree-fre1" } */
> +/* { dg-final { scan-tree-dump "return 9223372036854775806;" "fre1" } } */
> +
> +union U { struct A { unsigned long long a : 1, b : 62, c : 1; } a; unsigned
> long long i; };
> +
> +unsigned long long
> +foo (char *p)
> +{
> + __builtin_memset (p - 0xfffULL, 0, 0xffeULL);
> + __builtin_memset (p + 1, 0, 0xffeULL);
> + union U *q = (union U *) (void *) (p - 4);
> + q->a.b = -1;
> + return q->i;
> +}
>
>
> Jakub
>
>
--
Richard Biener
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)
[PATCH] sccvn: Fix handling of POINTER_PLUS_EXPR in memset offset [PR93582]
Hi!
On Wed, Mar 04, 2020 at 11:33:06AM +0100, Richard Biener wrote:
> > where POINTER_PLUS_EXPR last operand has sizetype type, thus unsigned,
> > and in the testcase gimple_assign_rhs2 (def) is thus 0xf001ULL
> > which multiplied by 8 doesn't fit into signed HWI. If it would be treated
> > as signed offset instead, it would fit (-0xfffLL, multiplied
> > by 8 is -0x7ff8LL). Unfortunately with the poly_int obfuscation
> > I'm not sure how to convert it from unsigned to signed poly_int.
>
> mem_ref_offset provides a boiler-plate for this:
>
> poly_offset_int::from (wi::to_poly_wide (TREE_OPERAND (t, 1)), SIGNED);
Thanks, that seems to work.
So, is the following ok for trunk if it passes bootstrap/regtest?
The test now works on both big-endian and little-endian.
2020-03-04 Richard Biener
Jakub Jelinek
PR tree-optimization/93582
* tree-ssa-sccvn.c (vn_reference_lookup_3): Treat POINTER_PLUS_EXPR
last operand as signed when looking for memset offset. Formatting
fix.
* gcc.dg/tree-ssa/pr93582-11.c: New test.
--- gcc/tree-ssa-sccvn.c.jj 2020-03-04 12:46:09.809610729 +0100
+++ gcc/tree-ssa-sccvn.c2020-03-04 13:07:07.982902846 +0100
@@ -2656,7 +2656,8 @@ vn_reference_lookup_3 (ao_ref *ref, tree
{
poly_int64 soff;
if (TREE_CODE (base) != MEM_REF
- || !(mem_ref_offset (base) << LOG2_BITS_PER_UNIT).to_shwi ())
+ || !(mem_ref_offset (base)
+ << LOG2_BITS_PER_UNIT).to_shwi ())
return (void *)-1;
offset += soff;
offset2 = 0;
@@ -2666,10 +2667,13 @@ vn_reference_lookup_3 (ao_ref *ref, tree
if (is_gimple_assign (def)
&& gimple_assign_rhs_code (def) == POINTER_PLUS_EXPR
&& gimple_assign_rhs1 (def) == TREE_OPERAND (base, 0)
- && poly_int_tree_p (gimple_assign_rhs2 (def))
- && (wi::to_poly_offset (gimple_assign_rhs2 (def))
- << LOG2_BITS_PER_UNIT).to_shwi ())
+ && poly_int_tree_p (gimple_assign_rhs2 (def)))
{
+ tree rhs2 = gimple_assign_rhs2 (def);
+ if (!(poly_offset_int::from (wi::to_poly_wide (rhs2),
+ SIGNED)
+ << LOG2_BITS_PER_UNIT).to_shwi ())
+ return (void *)-1;
ref2 = gimple_assign_rhs1 (def);
if (TREE_CODE (ref2) == SSA_NAME)
ref2 = SSA_VAL (ref2);
--- gcc/testsuite/gcc.dg/tree-ssa/pr93582-11.c.jj 2020-03-04
13:12:22.819222647 +0100
+++ gcc/testsuite/gcc.dg/tree-ssa/pr93582-11.c 2020-03-04 13:13:47.234968590
+0100
@@ -0,0 +1,16 @@
+/* PR tree-optimization/93582 */
+/* { dg-do compile { target lp64 } } */
+/* { dg-options "-O2 -fdump-tree-fre1" } */
+/* { dg-final { scan-tree-dump "return 9223372036854775806;" "fre1" } } */
+
+union U { struct A { unsigned long long a : 1, b : 62, c : 1; } a; unsigned
long long i; };
+
+unsigned long long
+foo (char *p)
+{
+ __builtin_memset (p - 0xfffULL, 0, 0xffeULL);
+ __builtin_memset (p + 1, 0, 0xffeULL);
+ union U *q = (union U *) (void *) (p - 4);
+ q->a.b = -1;
+ return q->i;
+}
Jakub
Re: [PATCH] Add -fcommon to a test-case to re-trigger it.
On Wed, Mar 04, 2020 at 12:53:58PM +0100, Martin Liška wrote: > Hi. > > I've noticed during working on VEC_COND_EXPR, that code added in > r10-2910-g9151048d854e352a9b83b771c6711b8221c73f7e is not executed. > It's also seen on our LCOV instance: > https://users.suse.com/~mliska/lcov/gcc/optabs.c.gcov.html > line 5889. > > It started with the revision where we changed default to -fno-common. > > Ready for trunk? Wouldn't it be better to add pr91623-2.c testcase that would #include this one and have the additional -fcommon, so that we test both -fcommon and default behavior? If you think it is completely unnecessary, just go ahead with your patch. Jakub
[PATCH] Add -fcommon to a test-case to re-trigger it.
Hi.
I've noticed during working on VEC_COND_EXPR, that code added in
r10-2910-g9151048d854e352a9b83b771c6711b8221c73f7e is not executed.
It's also seen on our LCOV instance:
https://users.suse.com/~mliska/lcov/gcc/optabs.c.gcov.html
line 5889.
It started with the revision where we changed default to -fno-common.
Ready for trunk?
Thanks,
Martin
gcc/testsuite/ChangeLog:
2020-03-04 Martin Liska
* gcc.target/i386/pr91623.c: Add -fcommon in order
to re-trigger the needed code for the test-case which
was added in r10-2910-g9151048d854e352a9b83b771c6711b8221c73f7e.
---
gcc/testsuite/gcc.target/i386/pr91623.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/gcc/testsuite/gcc.target/i386/pr91623.c b/gcc/testsuite/gcc.target/i386/pr91623.c
index 94de4f91c6d..e42930d91db 100644
--- a/gcc/testsuite/gcc.target/i386/pr91623.c
+++ b/gcc/testsuite/gcc.target/i386/pr91623.c
@@ -1,6 +1,6 @@
/* PR middle-end/91623 */
/* { dg-do compile } */
-/* { dg-options "-O3 -msse4.1 -mno-sse4.2" } */
+/* { dg-options "-O3 -msse4.1 -mno-sse4.2 -fcommon" } */
typedef long long V __attribute__((__vector_size__(16)));
V e, h;
Re: [PATCH] inliner: Copy DECL_BY_REFERENCE in copy_decl_to_var [PR93888]
On March 3, 2020 9:13:01 PM GMT+01:00, Jakub Jelinek wrote:
>Hi!
>
>In the following testcase we emit wrong debug info for the karg
>parameter in the DW_TAG_inlined_subroutine into main.
>The problem is that the karg PARM_DECL is DECL_BY_REFERENCE and thus
>in the IL has const K & type, but in the source just const K.
>When the function is inlined, we create a VAR_DECL for it, but don't
>set DECL_BY_REFERENCE, so when emitting DW_AT_location, we treat it
>like
>a const K & typed variable, but it has DW_AT_abstract_origin which has
>just the const K type and thus the debugger thinks the variable has
>const K type.
>
>Fixed by copying the DECL_BY_REFERENCE flag. Not doing it in
>copy_decl_for_dup_finish, because copy_decl_no_change already copies
>that flag through copy_node and in copy_result_decl_to_var it is
>undesirable, as we handle DECL_BY_REFERENCE in that case instead
>by changing the type.
>
>Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
Ok.
Richard.
>2020-03-03 Jakub Jelinek
>
> PR debug/93888
> * tree-inline.c (copy_decl_to_var): Copy DECL_BY_REFERENCE flag.
>
> * g++.dg/guality/pr93888.C: New test.
>
>--- gcc/tree-inline.c.jj 2020-02-07 19:11:57.444981885 +0100
>+++ gcc/tree-inline.c 2020-03-03 13:27:57.811046011 +0100
>@@ -5929,6 +5929,7 @@ copy_decl_to_var (tree decl, copy_body_d
> TREE_READONLY (copy) = TREE_READONLY (decl);
> TREE_THIS_VOLATILE (copy) = TREE_THIS_VOLATILE (decl);
> DECL_GIMPLE_REG_P (copy) = DECL_GIMPLE_REG_P (decl);
>+ DECL_BY_REFERENCE (copy) = DECL_BY_REFERENCE (decl);
>
> return copy_decl_for_dup_finish (id, decl, copy);
> }
>--- gcc/testsuite/g++.dg/guality/pr93888.C.jj 2020-03-03
>13:38:16.273935942 +0100
>+++ gcc/testsuite/g++.dg/guality/pr93888.C 2020-03-03
>13:40:15.890174469 +0100
>@@ -0,0 +1,24 @@
>+// PR debug/93888
>+// { dg-do run }
>+// { dg-options "-g -fvar-tracking -fno-inline" }
>+// { dg-skip-if "" { *-*-* } { "*" } { "-O0" } }
>+
>+struct K
>+{
>+ K () {}
>+ K (K const ) { k[0] = 'C'; }
>+ char k[8] = {'B','B','B','B','B','B','B','B'};
>+};
>+
>+__attribute__((always_inline)) inline bool
>+foo (const K karg)
>+{
>+ return karg.k[0] != 'C';// { dg-final { gdb-test 16 "karg.k[0]"
>"'C'" } }
>+} // { dg-final { gdb-test 16 "karg.k[1]" "'B'" }
>}
>+
>+int
>+main ()
>+{
>+ K x;
>+ return foo (x);
>+}
>
> Jakub
Re: [PATCH] Ada: gcc-interface: fixed assertion for aliased entities
> I'll have to look into this.. Any pointers? This assertion is not a language > rule assertion. Of course, neither of the 117 assertions in gcc-interface is, instead they are assertions meant to prevent wrong-code generation from occuring. > As you see, the assertion being modified already tests for "Is_Public". So > the issue is precisely that this assertion wrongly fails for cases where the > entity is not Public. We cannot let anything go through if there is not Is_Public set somewhere, possibly on the Alias (gnat_entity) since it is present in your case. -- Eric Botcazou
Re: [PATCH] [rs6000] Rewrite the declaration of a variable
on 2020/3/4 下午3:24, binbin wrote: > Hi > > On 2020/3/4 上午8:33, Segher Boessenkool wrote: >> Hi! >> >> On Tue, Mar 03, 2020 at 10:13:56AM -0600, Bin Bin Lv wrote: >>> Rewrite the declaration of toc_section from the source file rs6000.c to its >>> header file for standardizing the code. >> >>> diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c >>> index 0faf44b..c0a6e86 100644 >>> --- a/gcc/config/rs6000/rs6000.c >>> +++ b/gcc/config/rs6000/rs6000.c >>> @@ -181,7 +181,6 @@ static GTY(()) section *tls_private_data_section; >>> static GTY(()) section *read_only_private_data_section; >>> static GTY(()) section *sdata2_section; >>> -extern GTY(()) section *toc_section; >>> section *toc_section = 0; >>> /* Describe the vector unit used for modes. */ >>> diff --git a/gcc/config/rs6000/rs6000.h b/gcc/config/rs6000/rs6000.h >>> index 3844bec..e77a84a 100644 >>> --- a/gcc/config/rs6000/rs6000.h >>> +++ b/gcc/config/rs6000/rs6000.h >>> @@ -2494,6 +2494,7 @@ extern GTY(()) tree >>> rs6000_builtin_types[RS6000_BTI_MAX]; >>> extern GTY(()) tree rs6000_builtin_decls[RS6000_BUILTIN_COUNT]; >>> extern GTY(()) tree builtin_mode_to_type[MAX_MACHINE_MODE][2]; >>> extern GTY(()) tree altivec_builtin_mask_for_load; >>> +extern union GTY(()) section *toc_section; >> >> Why does this add "union"? >> >> >> Segher >> > > If "union" is not added, it reports error showing unknown type name ‘section’ > in file included from ../../host-powerpc64le-unknown-linux-gnu/gcc/tm.h:25, > from ../.././libgcc/generic-morestack-thread.c:29: > extern GTY(()) section *toc_section. Then add "union" to solve this. Thanks. > Hi Binbin, Another try seems to move it into #ifndef USED_FOR_TARGET hunk. Since "typedef union section section" is guard by #ifndef USED_FOR_TARGET in coretypes.h. It can make them consistent. BR, Kewen
Re: [PATCH] sccvn: Avoid overflows in push_partial_def
On Tue, 3 Mar 2020, Jakub Jelinek wrote:
> Hi!
>
> The following patch attempts to avoid dangerous overflows in the various
> push_partial_def HOST_WIDE_INT computations.
> This is achieved by performing the subtraction offset2i - offseti in
> the push_partial_def function and before doing that doing some tweaks.
> If a constant store (non-CONSTRUCTOR) is too large (perhaps just
> hypothetical case), native_encode_expr would fail for it, but we don't
> necessarily need to fail right away, instead we can treat it like
> non-constant store and if it is already shadowed, we can ignore it.
> Otherwise, if it at most 64-byte and the caller ensured that there is
> a range overlap and push_partial_def ensures the load is at most 64-byte,
> I think we should be fine, offset (relative to the load)
> can be from -64*8+1 to 64*8-1 only and size at most 64*8, so no risks of
> overflowing HOST_WIDE_INT computations.
> For CONSTRUCTOR (or non-constant) stores, those can be indeed arbitrarily
> large, the caller just checks that both the absolute offset and size fit
> into signed HWI. But, we store the same bytes in that case over and over
> (both in the {} case where it is all 0, and in the hypothetical future case
> where we handle in push_partial_def also memset (, 123, )), so we can tweak
> the write range for our purposes. For {} store we could just cap it at the
> start offset and/or offset+size because all the bits are 0, but I wrote it
> in anticipation of the memset case and so the relative offset can now be
> down to -7 and similarly size can grow up to 64 bytes + 14 bits, all this
> trying to preserve the offset difference % BITS_PER_UNIT or end as well.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
OK.
> I've tried to construct a testcase and came with
> /* PR tree-optimization/93582 */
> /* { dg-do compile { target lp64 } } */
>
> union U { struct A { unsigned long long a : 1, b : 62, c : 1; } a; unsigned
> long long i; };
>
> unsigned long long
> foo (char *p)
> {
> __builtin_memset (p - 0xfffULL, 0, 0xffeULL);
> __builtin_memset (p + 1, 0, 0xffeULL);
> union U *q = (union U *) (void *) (p - 4);
> q->a.b = -1;
> return q->i;
> }
> With this testcase, one can see signed integer overflows in the compiler
> without the patch. But unfortunately even with the patch it isn't optimized
> as it should. I believe the problem is in:
> gimple *def = SSA_NAME_DEF_STMT (ref2);
> if (is_gimple_assign (def)
> && gimple_assign_rhs_code (def) == POINTER_PLUS_EXPR
> && gimple_assign_rhs1 (def) == TREE_OPERAND (base, 0)
> && poly_int_tree_p (gimple_assign_rhs2 (def))
> && (wi::to_poly_offset (gimple_assign_rhs2 (def))
> << LOG2_BITS_PER_UNIT).to_shwi ())
> {
> where POINTER_PLUS_EXPR last operand has sizetype type, thus unsigned,
> and in the testcase gimple_assign_rhs2 (def) is thus 0xf001ULL
> which multiplied by 8 doesn't fit into signed HWI. If it would be treated
> as signed offset instead, it would fit (-0xfffLL, multiplied
> by 8 is -0x7ff8LL). Unfortunately with the poly_int obfuscation
> I'm not sure how to convert it from unsigned to signed poly_int.
mem_ref_offset provides a boiler-plate for this:
poly_offset_int::from (wi::to_poly_wide (TREE_OPERAND (t, 1)), SIGNED);
Richard.
> 2020-03-03 Jakub Jelinek
>
> * tree-ssa-sccvn.c (vn_walk_cb_data::push_partial_def): Add offseti
> argument. Change pd argument so that it can be modified. Turn
> constant non-CONSTRUCTOR store into non-constant if it is too large.
> Adjust offset and size of CONSTRUCTOR or non-constant store to avoid
> overflows.
> (vn_walk_cb_data::vn_walk_cb_data, vn_reference_lookup_3): Adjust
> callers.
>
> --- gcc/tree-ssa-sccvn.c.jj 2020-03-03 11:20:52.761545034 +0100
> +++ gcc/tree-ssa-sccvn.c 2020-03-03 16:22:49.387657379 +0100
> @@ -1716,7 +1716,7 @@ struct vn_walk_cb_data
> else
> pd.offset = pos;
> pd.size = tz;
> - void *r = push_partial_def (pd, 0, 0, prec);
> + void *r = push_partial_def (pd, 0, 0, 0, prec);
> gcc_assert (r == NULL_TREE);
> }
> pos += tz;
> @@ -1733,8 +1733,9 @@ struct vn_walk_cb_data
>}
>~vn_walk_cb_data ();
>void *finish (alias_set_type, alias_set_type, tree);
> - void *push_partial_def (const pd_data& pd,
> - alias_set_type, alias_set_type, HOST_WIDE_INT);
> + void *push_partial_def (pd_data pd,
> + alias_set_type, alias_set_type, HOST_WIDE_INT,
> + HOST_WIDE_INT);
>
>vn_reference_t vr;
>ao_ref orig_ref;
> @@ -1817,8 +1818,9 @@ pd_tree_dealloc (void *, void *)
> on failure. */
>
> void *
>
[PATCH] tree-optimization/93964 - adjust ISL code generation for pointer params
Pointers eventually need intermediate conversions in code generation.
Allowing them is much easier than fending them off since niter
and scev expansion easily drag those in.
Bootstrapped / tested on x86_64-unknown-linux-gnu, pushed.
Richard.
2020-02-03 Richard Biener
PR tree-optimization/93964
* graphite-isl-ast-to-gimple.c
(gcc_expression_from_isl_ast_expr_id): Add intermediate
conversion for pointer to integer converts.
* graphite-scop-detection.c (assign_parameter_index_in_region):
Relax assert.
* gcc.dg/graphite/pr93964.c: New testcase.
---
gcc/graphite-isl-ast-to-gimple.c| 3 +++
gcc/graphite-scop-detection.c | 1 -
gcc/testsuite/gcc.dg/graphite/pr93964.c | 19 +++
3 files changed, 22 insertions(+), 1 deletion(-)
create mode 100644 gcc/testsuite/gcc.dg/graphite/pr93964.c
diff --git a/gcc/graphite-isl-ast-to-gimple.c b/gcc/graphite-isl-ast-to-gimple.c
index c08a259a80e..ef93fda2233 100644
--- a/gcc/graphite-isl-ast-to-gimple.c
+++ b/gcc/graphite-isl-ast-to-gimple.c
@@ -265,6 +265,9 @@ gcc_expression_from_isl_ast_expr_id (tree type,
tree t = res->second;
if (useless_type_conversion_p (type, TREE_TYPE (t)))
return t;
+ if (POINTER_TYPE_P (TREE_TYPE (t))
+ && !POINTER_TYPE_P (type) && !ptrofftype_p (type))
+t = fold_convert (sizetype, t);
return fold_convert (type, t);
}
diff --git a/gcc/graphite-scop-detection.c b/gcc/graphite-scop-detection.c
index ed12fea801b..75f81227f8a 100644
--- a/gcc/graphite-scop-detection.c
+++ b/gcc/graphite-scop-detection.c
@@ -1102,7 +1102,6 @@ static void
assign_parameter_index_in_region (tree name, sese_info_p region)
{
gcc_assert (TREE_CODE (name) == SSA_NAME
- && INTEGRAL_TYPE_P (TREE_TYPE (name))
&& ! defined_in_sese_p (name, region->region));
int i;
tree p;
diff --git a/gcc/testsuite/gcc.dg/graphite/pr93964.c
b/gcc/testsuite/gcc.dg/graphite/pr93964.c
new file mode 100644
index 000..80fc523b855
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/graphite/pr93964.c
@@ -0,0 +1,19 @@
+/* { dg-do compile } */
+/* { dg-options "-O -floop-nest-optimize" } */
+
+int *
+eo (void);
+
+void
+g4 (int *nt)
+{
+ int dh, t2 = (__INTPTR_TYPE__)eo ();
+
+ for (dh = 0; dh < 2; ++dh)
+{
+ int m7;
+
+ for (m7 = 0; m7 < t2; ++m7)
+nt[m7] = 0;
+}
+}
--
2.16.4
Re: [PATCH v2 0/3] Introduce a new GCC option, --record-gcc-command-line
On 3/3/20 3:44 PM, Egeyar Bagcioglu wrote: Hello, Hi. I would like to propose the second version of the patches which introduce a compile option --record-gcc-command-line. When passed to gcc, it saves the command line invoking gcc into the produced object file. The option makes it trivial to trace back with which command a file was compiled and by which version of the gcc. It helps with debugging, reproducing bugs and repeating the build process. The reviews addressed in this version include indentation changes, error handling and corner case coverage pointed out by Segher Boessenkool in the first two patches; while the new third patch is another corner case (lto) coverage requested by Martin Liska. Great, thanks for working on that. Based on the mentioned disadvantages of -frecord-gcc-switches, where each option entry is put into a mergeable section and so one can't disambiguate in between multiple command line options, I would suggest to replace -frecord-gcc-switches functionality with what --record-gcc-command-line does. Moreover, looking at clang: https://reviews.llvm.org/D54487, their current implementation of -frecord-gcc-switches does the very same: $ clang main.c -frecord-gcc-switches -c $ readelf -p .GCC.command.line main.o String dump of section '.GCC.command.line': [ 1] /usr/bin/clang-9.0.1 main.c -frecord-command-line -c One additional advantage is that driver options also catch macros (like -D_FORTIFY_SOURCE), which is also what clang does. It's question to Nick who originally implemented the option. Nick, are you fine with that? If I'm correct you're using the annobin plugin and this can help you to replace it? Although we discussed after the submission of the first version that there are several other options performing similar tasks, I believe we established that there is still a need for this specific functionality. Therefore, I am skipping in this email the comparison between this option and the existing options with similarities. This functionality operates as the following: It saves gcc's argv into a temporary file, and passes --record-gcc-command-line to cc1 or cc1plus. The functionality of the backend is implemented via a hook. This patch includes an example implementation of the hook for elf targets: elf_record_gcc_command_line function. This function reads the given file and writes gcc's version and the command line into a mergeable string section, .GCC.command.line. It creates one entry per invocation. By doing so, it makes it clear which options were used together in a single gcc invocation, even after linking. Here is an *example usage* of the option: [egeyar@localhost save-commandline]$ gcc main.c --record-gcc-command-line [egeyar@localhost save-commandline]$ readelf -p .GCC.command.line a.out String dump of section '.GCC.command.line': [ 0] 10.0.1 20200227 (experimental) : gcc main.c --record-gcc-command-line The following is a *second example* calling g++ with -save-temps and a repetition of options, where --save-temps saves the intermediate file, main.cmdline in this case. You can see that the options are recorded unprocessed: [egeyar@localhost save-commandline]$ g++ main.c -save-temps --record-gcc-command-line -O0 -O2 -O3 -DFORTIFY=2 --record-gcc-command-line [egeyar@localhost save-commandline]$ readelf -p .GCC.command.line a.out String dump of section '.GCC.command.line': [ 0] 10.0.1 20200227 (experimental) : g++ main.c -save-temps --record-gcc-command-line -O0 -O2 -O3 -DFORTIFY=2 --record-gcc-command-line The first patch of this three-patch-series only extends the testsuite machinery, while the second patch implements the functionality and adds a test case for it. The third patch that alters libiberty is to make sure the .GCC.command.line section in LTO objects survive the linking and appear in the linked object. One small nit: I would not send in 3 parts, as the first and last one are just one hunks. Martin In addition to the new test case, I built binutils as my test case after passing this option to CFLAGS. The added .GCC.command.line section of ld.bfd listed many compile commands as expected. Tested on x86_64-pc-linux-gnu. Please review the patches, let me know what you think and apply if appropriate. Regards Egeyar Egeyar Bagcioglu (3): Introduce dg-require-target-object-format Introduce the gcc option --record-gcc-command-line Keep .GCC.command.line sections of LTO objetcs. gcc/common.opt | 4 +++ gcc/config/elfos.h | 5 +++ gcc/doc/tm.texi| 22 gcc/doc/tm.texi.in | 4 +++ gcc/gcc.c | 41 ++ gcc/gcc.h | 1 + gcc/target.def | 30 gcc/target.h
Re: [PATCH 1/3] [ARC] Remove mmixed-code option.
On Wed, Mar 4, 2020 at 9:52 AM Claudiu Zissulescu wrote: > > I will rework the patches preserving the option. Shall I add a deprecate > message as well? There's no need for that I think. Richard. > //Claudiu > > From: Jeff Law > Sent: Tuesday, March 3, 2020 7:00 PM > To: Richard Biener ; Claudiu Zissulescu > > Cc: GCC Patches ; Francois Bedard > ; Claudiu Zissulescu ; Andrew > Burgess > Subject: Re: [PATCH 1/3] [ARC] Remove mmixed-code option. > > On Tue, 2020-03-03 at 10:54 +0100, Richard Biener wrote: > > On Tue, Mar 3, 2020 at 10:41 AM Claudiu Zissulescu > > wrote: > > > The mmixed-code option is obsolete, remove it. > > > > You might want to preserve the option and ignore it like we do > > for some in common.opt: > > > > fargument-alias > > Common Ignore > > Does nothing. Preserved for backward compatibility. > > > > this avoids compiler errors when updating the compiler but not > > adjusting flags. > Yea, I'd recommend this as well. > > jeff > > >
Re: [PATCH v2 3/3] Keep .GCC.command.line sections of LTO objetcs.
On Tue, Mar 3, 2020 at 5:41 PM Egeyar Bagcioglu wrote: > > This patch is for .GCC.command.line sections in LTO objects to be copied > into the final objects as in the following example: > > [egeyar@localhost lto]$ gcc -flto -O3 demo.c -c -g --record-gcc-command-line > [egeyar@localhost lto]$ gcc -flto -O2 demo2.c -c -g --record-gcc-command-line > -DFORTIFY=2 > [egeyar@localhost lto]$ gcc demo.o demo2.o -o a.out > [egeyar@localhost lto]$ readelf -p .GCC.command.line a.out > > String dump of section '.GCC.command.line': > [ 0] 10.0.1 20200227 (experimental) : gcc -flto -O3 demo.c -c -g > --record-gcc-command-line > [56] 10.0.1 20200227 (experimental) : gcc -flto -O2 demo2.c -c -g > --record-gcc-command-line -DFORTIFY=2 --record-gcc-command-line is not a FSF GCC option, there's -frecord-gcc-switches though which (also) populates .GCC.command.line OK. Thanks, Richard. > Regards > Egeyar > > libiberty: > 2020-02-27 Egeyar Bagcioglu > > * simple-object.c (handle_lto_debug_sections): Name > ".GCC.command.line" among debug sections to be copied over > from lto objects. > --- > libiberty/simple-object.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/libiberty/simple-object.c b/libiberty/simple-object.c > index d9c648a..3b3ca9c 100644 > --- a/libiberty/simple-object.c > +++ b/libiberty/simple-object.c > @@ -298,6 +298,9 @@ handle_lto_debug_sections (const char *name, int rename) > COMDAT sections in objects produced by GCC. */ >else if (strcmp (name, ".comment") == 0) > return strcpy (newname, name); > + /* Copy over .GCC.command.line section under the same name if present. */ > + else if (strcmp (name, ".GCC.command.line") == 0) > +return strcpy (newname, name); >free (newname); >return NULL; > } > -- > 1.8.3.1 >
Re: [PATCH 1/3] [ARC] Remove mmixed-code option.
I will rework the patches preserving the option. Shall I add a deprecate message as well? //Claudiu From: Jeff Law Sent: Tuesday, March 3, 2020 7:00 PM To: Richard Biener ; Claudiu Zissulescu Cc: GCC Patches ; Francois Bedard ; Claudiu Zissulescu ; Andrew Burgess Subject: Re: [PATCH 1/3] [ARC] Remove mmixed-code option. On Tue, 2020-03-03 at 10:54 +0100, Richard Biener wrote: > On Tue, Mar 3, 2020 at 10:41 AM Claudiu Zissulescu wrote: > > The mmixed-code option is obsolete, remove it. > > You might want to preserve the option and ignore it like we do > for some in common.opt: > > fargument-alias > Common Ignore > Does nothing. Preserved for backward compatibility. > > this avoids compiler errors when updating the compiler but not > adjusting flags. Yea, I'd recommend this as well. jeff >
Re: [PATCH] [COMMITTED] arc: Add ARC entry for gcc-10/changes.html
Done Thank you for your review, Claudiu From: Martin Sebor Sent: Tuesday, March 3, 2020 6:47 PM To: Claudiu Zissulescu ; gcc-patches@gcc.gnu.org Cc: ger...@pfeifer.com ; l...@redhat.com ; Francois Bedard ; Claudiu Zissulescu ; andrew.burg...@embecosm.com Subject: Re: [PATCH] [COMMITTED] arc: Add ARC entry for gcc-10/changes.html On 3/3/20 2:12 AM, Claudiu Zissulescu wrote: > Add ARC entry for gcc-10/changes.html > > --- > htdocs/gcc-10/changes.html | 14 +- > 1 file changed, 13 insertions(+), 1 deletion(-) > > diff --git a/htdocs/gcc-10/changes.html b/htdocs/gcc-10/changes.html > index 53d0ca08..4e27c05b 100644 > --- a/htdocs/gcc-10/changes.html > +++ b/htdocs/gcc-10/changes.html > @@ -557,7 +557,19 @@ a work-in-progress. > much improved. > > > - > +ARC > + > + The interrupt service routine functions saves all used ^ Just a small typo: they "save". > + registers, including extension registers and auxiliary registers > + used by Zero Overhead Loops. > + Improve code-size by using multiple short instructions instead Not terribly important but I don't think the hyphen in code size is called for (it's not used as an adjective). Martin > + of a single long mov or ior instruction when its long immediate > + constant is known. > + Fix usage of the accumulator register for ARC600. > + Fix issues with uncached attribute. > + Remove -mq-class option. > + Improve 64-bit integer addition and subtraction operations. > + > > arm > >
[PATCH PR94026] combine missed opportunity to simplify comparisons with zero
Hi,
This is a simple fix for PR94026.
With this fix, combine will try make an extraction if we are in a equality
comparison and this is an AND
with a constant which is power of two minus one. Shift here should be an
constant. For example, combine
will transform (compare (and (lshiftrt x 8) 6) 0) to (compare (zero_extract
(x 2 9)) 0).
Added one test case for this. Bootstrap and tested on both x86_64 and
aarch64 Linux platform.
Any suggestion?
Thanks,
Felix
gcc:
+2020-03-04 Felix Yang
+
+ PR rtl-optimization/94026
+ * combine.c (make_compound_operation_int): Make an extraction
+ if we are in a equality comparison and this is an AND with a
+ constant which is power of two minus one.
+
gcc/testsuite:
+2020-03-04 Felix Yang
+
+ PR rtl-optimization/94026
+ * gcc.dg/pr94026.c: New test.
+
diff --git a/gcc/combine.c b/gcc/combine.c
index 58366a6d331..c05064fc333 100644
--- a/gcc/combine.c
+++ b/gcc/combine.c
@@ -8170,14 +8170,31 @@ make_compound_operation_int (scalar_int_mode mode, rtx
*x_ptr,
if (!CONST_INT_P (XEXP (x, 1)))
break;
+ HOST_WIDE_INT pos;
+ unsigned HOST_WIDE_INT len;
+ pos = get_pos_from_mask (UINTVAL (XEXP (x, 1)), );
+
/* If the constant is a power of two minus one and the first operand
-is a logical right shift, make an extraction. */
+is a logical right shift, make an extraction.
+If we are in a equality comparison and this is an AND with a constant
+which is power of two minus one, also make an extraction. */
if (GET_CODE (XEXP (x, 0)) == LSHIFTRT
- && (i = exact_log2 (UINTVAL (XEXP (x, 1)) + 1)) >= 0)
+ && (pos == 0 || (pos > 0 && equality_comparison
+ && CONST_INT_P (XEXP (XEXP (x, 0), 1)
{
new_rtx = make_compound_operation (XEXP (XEXP (x, 0), 0), next_code);
- new_rtx = make_extraction (mode, new_rtx, 0, XEXP (XEXP (x, 0), 1),
-i, 1, 0, in_code == COMPARE);
+ if (pos == 0)
+ {
+ new_rtx = make_extraction (mode, new_rtx, 0,
+XEXP (XEXP (x, 0), 1), len, 1, 0,
+in_code == COMPARE);
+ }
+ else
+ {
+ int real_pos = pos + UINTVAL (XEXP (XEXP (x, 0), 1));
+ new_rtx = make_extraction (mode, new_rtx, real_pos, NULL_RTX,
+len, 1, 0, in_code == COMPARE);
+ }
}
/* Same as previous, but for (subreg (lshiftrt ...)) in first op. */
@@ -8186,13 +8203,25 @@ make_compound_operation_int (scalar_int_mode mode, rtx
*x_ptr,
&& is_a (GET_MODE (SUBREG_REG (XEXP (x, 0))),
_mode)
&& GET_CODE (SUBREG_REG (XEXP (x, 0))) == LSHIFTRT
- && (i = exact_log2 (UINTVAL (XEXP (x, 1)) + 1)) >= 0)
+ && (pos == 0
+ || (pos > 0 && equality_comparison
+ && CONST_INT_P (XEXP (SUBREG_REG (XEXP (x, 0)), 1)
{
rtx inner_x0 = SUBREG_REG (XEXP (x, 0));
new_rtx = make_compound_operation (XEXP (inner_x0, 0), next_code);
- new_rtx = make_extraction (inner_mode, new_rtx, 0,
-XEXP (inner_x0, 1),
-i, 1, 0, in_code == COMPARE);
+ if (pos == 0)
+ {
+ new_rtx = make_extraction (inner_mode, new_rtx, 0,
+XEXP (inner_x0, 1),
+len, 1, 0, in_code == COMPARE);
+ }
+ else
+ {
+ int real_pos = pos + UINTVAL (XEXP (inner_x0, 1));
+ new_rtx = make_extraction (inner_mode, new_rtx, real_pos,
+NULL_RTX, len, 1, 0,
+in_code == COMPARE);
+ }
/* If we narrowed the mode when dropping the subreg, then we lose. */
if (GET_MODE_SIZE (inner_mode) < GET_MODE_SIZE (mode))
@@ -8200,10 +8229,10 @@ make_compound_operation_int (scalar_int_mode mode, rtx
*x_ptr,
/* If that didn't give anything, see if the AND simplifies on
its own. */
- if (!new_rtx && i >= 0)
+ if (!new_rtx)
{
new_rtx = make_compound_operation (XEXP (x, 0), next_code);
- new_rtx = make_extraction (mode, new_rtx, 0, NULL_RTX, i, 1,
+ new_rtx = make_extraction (mode, new_rtx, pos, NULL_RTX, len, 1,
0, in_code == COMPARE);
}
}
@@ -8212,7 +8241,7 @@ make_compound_operation_int (scalar_int_mode mode, rtx
*x_ptr,
|| GET_CODE (XEXP (x, 0)) == IOR)
&& GET_CODE (XEXP (XEXP (x, 0), 0)) == LSHIFTRT
&& GET_CODE (XEXP
