Re: [PATCH] PR fortran/64124,70409 -- Reduce a charlen
On Wed, Mar 7, 2018 at 2:20 AM, Steve Kargl wrote: > All, > > As everyone knows, gfortran reads source into a trees and > and at some point she passes those trees to a resolution > phases. There are instances, for example the new tests, > where resolution fails to use the character length > parameter in declaration statements. The attach patch > seems to cure this problem. > > Previously, after reading the scalar-integer-expr, gfortran > would assign the expression to the charlen without trying > to reduce it to a constant (which should happen but doesn't > in the resolution phase). The patch now tries to reduce > the scalar-integer-expr to a constant, and then assigns > that constant to the charlen. > > Bootstrap and regression tested on 7-branch and trunk. > OK to commit? > > 2018-03-06 Steven G. Kargl > > PR fortran/64124 > PR fortran/70409 > * decl.c (gfc_match_char_spec): Try to reduce a charlen to a constant. > > 2018-03-06 Steven G. Kargl > > PR fortran/64124 > PR fortran/70409 > * gfortran.dg/pr64124.f90: New tests. > * gfortran.dg/pr70409.f90: New tests. Ok, thanks. -- Janne Blomqvist
Re: [RFA][PATCH][PR middle-end/61118] Improve tree CFG accuracy for setjmp/longjmp
On 03/06/2018 01:57 AM, Richard Biener wrote: > On Tue, Mar 6, 2018 at 4:41 AM, Jeff Law wrote: >> On 03/05/2018 12:30 PM, Michael Matz wrote: >>> Hi, >>> >>> On Mon, 5 Mar 2018, Jeff Law wrote: >>> >> The single successor test was strictly my paranoia WRT abnormal/EH >> edges. >> >> I don't immediately see why the CFG would be incorrect if the >> successor of the setjmp block has multiple preds. > > Actually, without further conditions I don't see how it would be safe > for the successor to have multiple preds. We might have this > situation: > > bb1: ret = setjmp > bb2: x0 = phi No. Can't happen -- we're still building the initial CFG. There are no PHI nodes, there are no SSA_NAMEs. >>> >>> While that is currently true I think it's short-sighted. Thinking about >>> the situation in terms of SSA names and PHI nodes clears up the mind. In >>> addition there is already code which builds (sub-)cfgs when SSA form >>> exists (gimple_find_sub_bbs). Currently that code can't ever generate >>> setjmp calls, so it's not an issue. >> It's not clearing up anything for me. Clearly you're onto something >> that I'm missing, but still trying to figure out. >> >> Certainly we have to be careful WRT the implicit set of the return value >> of the setjmp call that occurs on the longjmp path. That's worth >> investigating. I suspect that works today more by accident of having an >> incorrect CFG than by design. >> >> >>> We have two choices we can either target the setjmp itself, which is what we've been doing and is inaccurate. Or we can target the point immediately after the setjmp, which is accurate. >>> >>> Not precisely, because the setting of the return value of setjmp does >>> happen after both returns. So moving the whole second-return edge target >>> to after the setjmp call (when it includes an LHS) is not correct >>> (irrespective how the situation in the successor BBs like like). >> But it does or at least it should. It's implicitly set on the longjmp >> side. If we get this wrong I'd expect we'll see uninit uses in the PHI. >> That's easy enough to instrument and check for. >> >> This aspect of setjmp/longjmp is, in some ways, easier to see in RTL >> because the call returns its value in a hard reg which is implicitly set >> by the longjmp and we immediately copy it into a pseudo. Which would >> magically DTRT if we had the longjmp edge target the point just after >> the setjmp in RTL. > > While it's true that the hardreg is set by the callee the GIMPLE IL > indeed doesn't reflect this (and we have a similar issue with EH > where the exceptional return does _not_ include the assignment > to the LHS but the GIMPLE IL does...). > > So with your patch we should see > > ret_1 = setjmp (); >| \ >| AB dispatcher >|/ >v v > # ret_2 = PHI > ... > > even w/o a PHI. So I think we should be fine given we have that > edge from setjmp to the abnormal dispatcher. I believe so by nature that the setjmp dominates the longjmp sites and thus also dominates the dispatcher. But it's something I want to explicitly check before resubmitting. jeff
[PATCH] use attribute exclusion to reject naked vs target_clones conflicts (PR 84723)
PR 84723 is about an ICE caused by GCC accepting the mutually
exclusive attributes target_clones and naked during parsing,
only to then later crash while assuming such conflicts don't
happen.
The attached patch adds an exclusion for the two attributes
to detect when they're both specified on declarations of the
same function and avoid the ICE that way (there are other
problems with target_clones that are unrelated to other
interaction with other attributes).
As this is the first instance of an exclusion between a pair
of a front/middle-end and back-end attributes, testing the
straightforward solution of just adding an exclusion for them
exposed a restriction in the implementation that required more
extensive changes to make things work. The current (unpatched)
code requires exclusions to be symmetric (e.g., when there is
an exclusion entry for target_clones and naked, there must also
be another exclusion entry the other way around). This makes
sense for attributes in the same table (e.g., all C/C++
attributes, or all i386 back-end attributes) but not between
attributes in different tables. The patch makes additional
changes to allow asymmetric exclusions without compromising
the detection of attribute conflicts. This additional
enhancement allowed me to remove the self tests.
Since I started working on this last night Jakub has already
posted an alternate fix that doesn't make use of the exclusion
framework (plus it fixes the additional bugs/ICEs I referred
to above). This patch is thus meant to apply on top of Jakub's
(the tests will need tweaking to adjust the expected diagnostics).
If my solution is considered too invasive at this stage I'm fine
holding off on it and re-posting it in stage 1. While working
on it I noticed other opportunities for improvements in this
area that I would like to make then.
Martin
PR middle-end/84723 - ICE in create_target_clone
gcc/c-family/ChangeLog:
PR middle-end/84723
* c-attribs.c (attr_clone_exclusions, attr_noipa_exclusions): New.
(c_common_attribute_tab): Use them.
gcc/testsuite/ChangeLog:
PR middle-end/84723
* gcc.dg/Wattributes-10.c: New test.
* gcc.dg/Wattributes-11.c: New test.
* gcc.dg/Wattributes-9.c: New test.
gcc/ChangeLog:
PR middle-end/84723
* attribs.c (diag_attr_exclusions): New function.
(maybe_diag_attr_exclusions): Same.
(decl_attributes): Call it.
(namespace selftest): Remove.
* selftest-run-tests.c (selftest::run_tests): Remove call to
attribute_c_tests.
* selftest.h (attribute_c_tests): Remove.
Index: gcc/attribs.c
===
--- gcc/attribs.c (revision 258259)
+++ gcc/attribs.c (working copy)
@@ -345,6 +345,93 @@ get_attribute_namespace (const_tree attr)
return get_identifier ("gnu");
}
+
+/* Check LAST_DECL and NODE of the same symbol for attributes exclusions
+ in EXCL with respect to ATTRNAME and ATTRS when non-null or NAME
+ otherwise, diagnose them, and return true if any have been found.
+ NODE can be a DECL or a TYPE. Called from the diag_attr_exclusions
+ overloads just below. */
+
+static bool
+diag_attr_exclusions (tree last_decl, tree node, tree attrs, tree attrname,
+ tree name, const attribute_spec::exclusions *excl)
+{
+ bool found = false;
+
+ /* Iterate over the null-terminated array of exclusions. */
+ for ( ; excl->name; ++excl)
+{
+ /* The name of the mutually exclusive attribute to mention
+ in the diagnostic. */
+ const char *xatstr;
+
+ if (attrs)
+ {
+ gcc_assert (!name);
+
+ /* Avoid checking the attribute against itself. */
+ if (is_attribute_p (excl->name, attrname))
+ continue;
+
+ if (!lookup_attribute (excl->name, attrs))
+ continue;
+
+ xatstr = excl->name;
+ }
+ else
+ {
+ /* Avoid checking the attribute against itself. */
+ if (is_attribute_p (excl->name, name))
+ continue;
+
+ if (!is_attribute_p (excl->name, attrname))
+ continue;
+
+ xatstr = IDENTIFIER_POINTER (name);
+ }
+
+ /* An exclusion may apply either to a function declaration,
+ type declaration, or a field/variable declaration, or
+ any subset of the three. */
+ tree_code code = TREE_CODE (node);
+ if (code == FUNCTION_DECL && !excl->function)
+ continue;
+
+ if (code == TYPE_DECL && !excl->type)
+ continue;
+
+ if ((code == FIELD_DECL
+ || code == VAR_DECL)
+ && !excl->variable)
+ continue;
+
+ /* Print a note? */
+ bool note = last_decl != NULL_TREE;
+
+ if (TREE_CODE (node) == FUNCTION_DECL
+ && DECL_BUILT_IN (node))
+ note &= warning (OPT_Wattributes,
+ "ignoring attribute %qE in declaration of "
+ "a built-in function %qD because it conflicts "
+ "with attribute %qs",
+ attrname, node, xatstr);
+ else
+ note &= warning (OPT_Wattributes,
+ "ignoring attribute %qE because "
+ "it conflicts with attribute %qs",
+ attrname, xatstr);
+
+ if (note)
+ inform (DECL_SOURCE_LOCATION (last_decl),
+ "pre
Re: [PATCH] RISC-V: Add and document the "-mno-relax" option
On Tue, Mar 6, 2018 at 5:00 PM, Palmer Dabbelt wrote: > Thanks. How does this look? It looks OK. It still doesn't mention compression, but that is documented other places so it isn't a big deal. Jim
Re: [AARCH64] Disable pc relative literal load irrespective of TARGET_FIX_ERR_A53_84341
Hi James, This patch has to be backported to gcc-7 branch as the build error for 521.wrf with LTO is there too (for the same reason). This patch has been on trunk for some time now. So, is this OK to backport this patch gcc-7 branch? Thanks, Kugan On 30 August 2017 at 15:19, Kugan Vivekanandarajah wrote: > Hi James, > > On 29 August 2017 at 21:31, James Greenhalgh wrote: >> On Tue, Jun 27, 2017 at 11:20:02AM +1000, Kugan Vivekanandarajah wrote: >>> https://gcc.gnu.org/ml/gcc-patches/2016-03/msg00614.html added this >>> workaround to get kernel building with when TARGET_FIX_ERR_A53_843419 >>> is enabled. >>> >>> This was added to support building kernel loadable modules. In kernel, >>> when CONFIG_ARM64_ERRATUM_843419 is selected, the relocation needed >>> for ADRP/LDR (R_AARCH64_ADR_PREL_PG_HI21 and >>> R_AARCH64_ADR_PREL_PG_HI21_NC are removed from the kernel to avoid >>> loading objects with possibly offending sequence). Thus, it could only >>> support pc relative literal loads. >>> >>> However, the following patch was posted to kernel to add >>> -mpc-relative-literal-loads >>> http://www.spinics.net/lists/arm-kernel/msg476149.html >>> >>> -mpc-relative-literal-loads is unconditionally added to the kernel >>> build as can be seen from: >>> https://github.com/torvalds/linux/blob/master/arch/arm64/Makefile >>> >>> Therefore this patch removes the hunk so that applications like >>> SPECcpu2017's 521/621.wrf can be built (with LTO in this case) without >>> -mno-pc-relative-literal-loads >>> >>> Bootstrapped and regression tested on aarch64-linux-gnu with no new >>> regressions. >>> >>> Is this OK for trunk? >> >> Hi Kugan, >> >> I'm struggling a little to convince myself that this is correct. I think >> the argument is that this was a workaround for one very particular issue >> with the linux kernel module loader, which hard faults by refusing to >> handle these relocations when in a workaround mode for Erratum 843419. > > Yes. > >> Most of these relocations won't occur because the kernel builds with >> -mcmodel=large, but literals would always use a small model style >> adrp/load, unless we were using -mpc-relative-literal-loads . So, this >> workaround enabled -mpc-relative-literal-loads always when we were in >> a workaround mode, thus allowing the kernel loader to continue. >> >> The argument for removing it then, is that with the kernel now always using >> -mpc-relative-literal-loads there is no reason for this workaround to stay >> in place. The linkers which we will use will apply the workaround if needed. > > Yes. > >> Testcases and a detailed problem report of the build failures you had seen in >> 521.wrf would have helped me get closer to understanding this, and made >> review substantially easier. > > Sorry for not being clear with this. Unfortunately 521.wrf was > showing this error with LTO so I could not reproduce with a small > enough test case. > >> Am I on the right track? >> >> If so, this is OK for trunk. If not, please can you expand on what is going >> on in this patch. > > Thanks for the review. > Kugan > >> >> Thanks, >> James >> >> >>> >>> Thanks, >>> Kugan >>> >>> gcc/testsuite/ChangeLog: >>> >>> 2017-06-27 Kugan Vivekanandarajah >>> >>> * gcc.target/aarch64/pr63304_1.c: Remove-mno-fix-cortex-a53-843419. >>> >>> gcc/ChangeLog: >>> >>> 2017-06-27 Kugan Vivekanandarajah >>> >>> * config/aarch64/aarch64.c (aarch64_override_options_after_change_1): >>> Disable pc relative literal load irrespective of >>> TARGET_FIX_ERR_A53_84341 >>> for default. >> >>
Re: [PATCH] RISC-V: Add and document the "-mno-relax" option
On Fri, 02 Mar 2018 16:27:41 PST (-0800), Jim Wilson wrote:
On 03/01/2018 01:26 PM, Palmer Dabbelt wrote:
+@item -mrelax
+@itemx -mno-relax
+Take advantage of linker relaxations to reduce the number of instructions
+required to materalize symbol addresses.
materalize->materialize
This is not just number of instructions, it is also code size, since
linker relaxation can convert non-compressed instructions into
compressed instructions.
Also, this isn't clear which option is the default. If you look at the
other options descriptions, I tried to make it clear which was the
default when there was more than one choice. I don't think you should
rely on @item or @itemx to define which is the default.
Thanks. How does this look?
commit e7c570f37384d824cb9725f237920e9691e57269
gpg: Signature made Tue 06 Mar 2018 04:52:46 PM PST
gpg:using RSA key 00CE76D1834960DFCE886DF8EF4CA1502CCBAB41
gpg:issuer "pal...@dabbelt.com"
gpg: Good signature from "Palmer Dabbelt " [ultimate]
gpg: aka "Palmer Dabbelt " [ultimate]
Author: Palmer Dabbelt
Date: Thu Mar 1 12:01:06 2018 -0800
RISC-V: Add and document the "-mno-relax" option
RISC-V relies on aggressive linker relaxation to get good code size. As
a result no text symbol addresses can be known until link time, which
means that alignment must be handled during the link. This alignment
pass is essentially just another linker relaxation, so this has the
unfortunate side effect that linker relaxation is required for
correctness on many RISC-V targets.
The RISC-V assembler has supported an ".option norelax" for a long time
because there are situations in which linker relaxation is a bad idea --
the canonical example is when trying to materialize the initial value of
the global pointer into a register, which would otherwise be relaxed to
a NOP. We've been relying on users who want to disable relaxation for
an entire link to pass "-Wl,--no-relax", but that still relies on the
linker relaxing R_RISCV_ALIGN to handle alignment despite it not being
strictly necessary.
This patch adds a GCC option, "-mno-relax", that disable linker
relaxation by adding ".option norelax" to the top of every generated
assembly file. The assembler is smart enough to handle alignment at
assemble time for files that have never emitted a relaxable relocation,
so this is sufficient to really disable all relaxations in the linker,
which results in significantly faster link times for large objects.
This also has the side effect of allowing toolchains that don't support
linker relaxation (LLVM and the Linux module loader) to function
correctly. Toolchains that don't support linker relaxation should
default to "-mno-relax" and error when presented with any R_RISCV_ALIGN
relocation as those need to be handled for correctness.
gcc/ChangeLog
2018-03-01 Palmer Dabbelt
* config/riscv/riscv.opt (mrelax): New option.
* config/riscv/riscv.c (riscv_file_start): Emit ".option
"norelax" when riscv_mrelax is disabled.
* doc/invoke.texi (RISC-V): Document "-mrelax" and "-mno-relax".
diff --git a/gcc/config/riscv/riscv.c b/gcc/config/riscv/riscv.c
index c38f6c394d54..3e81874de232 100644
--- a/gcc/config/riscv/riscv.c
+++ b/gcc/config/riscv/riscv.c
@@ -3979,6 +3979,11 @@ riscv_file_start (void)
/* Instruct GAS to generate position-[in]dependent code. */
fprintf (asm_out_file, "\t.option %spic\n", (flag_pic ? "" : "no"));
+
+ /* If the user specifies "-mno-relax" on the command line then disable linker
+ relaxation in the assembler. */
+ if (! riscv_mrelax)
+fprintf (asm_out_file, "\t.option norelax\n");
}
/* Implement TARGET_ASM_OUTPUT_MI_THUNK. Generate rtl rather than asm text
diff --git a/gcc/config/riscv/riscv.opt b/gcc/config/riscv/riscv.opt
index 581a26bb5c1e..b37ac75d9bb4 100644
--- a/gcc/config/riscv/riscv.opt
+++ b/gcc/config/riscv/riscv.opt
@@ -106,6 +106,11 @@ mexplicit-relocs
Target Report Mask(EXPLICIT_RELOCS)
Use %reloc() operators, rather than assembly macros, to load addresses.
+mrelax
+Target Bool Var(riscv_mrelax) Init(1)
+Take advantage of linker relaxations to reduce the number of instructions
+required to materialize symbol addresses.
+
Mask(64BIT)
Mask(MUL)
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index 8d366c626bae..deb48af2ecad 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -1042,7 +1042,8 @@ See RS/6000 and PowerPC Options.
-msave-restore -mno-save-restore @gol
-mstrict-align -mno-strict-align @gol
-mcmodel=medlow -mcmodel=medany @gol
--mexplicit-relocs -mno-explicit-relocs @gol}
+-mexplicit-relocs -mno-explicit-relocs @gol
+-mrelax -mno-relax @gol}
@emph{RL78 Options}
@gccoptlist{-msim -mmul=none -mmul=g13 -mmul=g14 -mallregs @gol
@@ -23102,6 +23103,12 @@ Use or do not use assembler relocation operators when
dealing with symbolic
addresses. Th
[committed] hppa: Update handling of internal labels on hppa-hpux
In a recent build for hppa64-hp-hpux11.11, the build failed in stage2
due to a warning about the
format used in the ASM_GENERATE_INTERNAL_LABEL macro. This got me
looking at the implementation
in elfos.h. I also realized that there was no good reason to limit the
number of labels to and
to prefix the label numbers with zeros.
The attached patch revises the label generation on hppa-hpux to use the
same technique as used in
config/elfos.h.
Tested on hppa2.0w-hp-hpux11.11, hppa64-hp-hpux11.11 and
hppa-unknown-linux-gnu with no
observed regressions.
Committed to trunk.
Dave
--
John David Anglin dave.ang...@bell.net
2018-03-06 John David Anglin
* config/pa/pa.h (ASM_GENERATE_INTERNAL_LABEL): Revise to use
sprint_ul.
(ASM_OUTPUT_ADDR_VEC_ELT): Revise for above change.
(ASM_OUTPUT_ADDR_DIFF_ELT): Likewise.
* config/pa/pa64-hpux.h (ASM_GENERATE_INTERNAL_LABEL): Revise as above.
Index: config/pa/pa.h
===
--- config/pa/pa.h (revision 258210)
+++ config/pa/pa.h (working copy)
@@ -1114,9 +1114,19 @@
PREFIX is the class of label and NUM is the number within the class.
This is suitable for output with `assemble_name'. */
-#define ASM_GENERATE_INTERNAL_LABEL(LABEL,PREFIX,NUM) \
- sprintf (LABEL, "*%c$%s%04ld", (PREFIX)[0], (PREFIX) + 1, (long)(NUM))
+#define ASM_GENERATE_INTERNAL_LABEL(LABEL, PREFIX, NUM)\
+ do \
+{ \
+ char *__p; \
+ (LABEL)[0] = '*';\
+ (LABEL)[1] = (PREFIX)[0];\
+ (LABEL)[2] = '$';\
+ __p = stpcpy (&(LABEL)[3], &(PREFIX)[1]);\
+ sprint_ul (__p, (unsigned long) (NUM)); \
+} \
+ while (0)
+
/* Output the definition of a compiler-generated label named NAME. */
#define ASM_OUTPUT_INTERNAL_LABEL(FILE,NAME) \
@@ -1154,7 +1164,7 @@
/* This is how to output an element of a case-vector that is absolute. */
#define ASM_OUTPUT_ADDR_VEC_ELT(FILE, VALUE) \
- fprintf (FILE, "\t.word L$%04d\n", VALUE)
+ fprintf (FILE, "\t.word L$%d\n", VALUE)
/* This is how to output an element of a case-vector that is relative.
Since we always place jump tables in the text section, the difference
@@ -1161,7 +1171,7 @@
is absolute and requires no relocation. */
#define ASM_OUTPUT_ADDR_DIFF_ELT(FILE, BODY, VALUE, REL) \
- fprintf (FILE, "\t.word L$%04d-L$%04d\n", VALUE, REL)
+ fprintf (FILE, "\t.word L$%d-L$%d\n", VALUE, REL)
/* This is how to output an absolute case-vector. */
Index: config/pa/pa64-hpux.h
===
--- config/pa/pa64-hpux.h (revision 258210)
+++ config/pa/pa64-hpux.h (working copy)
@@ -245,9 +245,19 @@
/* We need to use the HP style for internal labels. */
#undef ASM_GENERATE_INTERNAL_LABEL
-#define ASM_GENERATE_INTERNAL_LABEL(LABEL, PREFIX, NUM)\
- sprintf (LABEL, "*%c$%s%04ld", (PREFIX)[0], (PREFIX) + 1, (long)(NUM))
+#define ASM_GENERATE_INTERNAL_LABEL(LABEL, PREFIX, NUM)\
+ do \
+{ \
+ char *__p; \
+ (LABEL)[0] = '*';\
+ (LABEL)[1] = (PREFIX)[0];\
+ (LABEL)[2] = '$';\
+ __p = stpcpy (&(LABEL)[3], &(PREFIX)[1]);\
+ sprint_ul (__p, (unsigned long) (NUM)); \
+} \
+ while (0)
+
#else /* USING_ELFOS_H */
/* We are not using GAS. */
[committed] hppa: Add read access checks to __canonicalize_funcptr_for_compare
The attached change fixes the build of the Debian cpputest package on
hppa. The testsuite
generates a segmentation fault canonicalizing the function pointer
0xdeadbeef. The attached
patch adds a routine from the hppa dynamic linker to check for read
access permission. So,
we are now less likely to generate a segmentation fault canonicalizing a
function pointer.
Tested on hppa-unknown-linux-gnu with no observed regressions. Committed
to trunk.
Dave
--
John David Anglin dave.ang...@bell.net
2018-03-06 John David Anglin
* config/pa/fptr.c (_dl_read_access_allowed): New.
(__canonicalize_funcptr_for_compare): Use it.
Index: config/pa/fptr.c
===
--- config/pa/fptr.c(revision 258235)
+++ config/pa/fptr.c(working copy)
@@ -52,6 +52,16 @@
typedef int (*fixup_t) (struct link_map *, unsigned int);
extern unsigned int _GLOBAL_OFFSET_TABLE_;
+static inline int
+_dl_read_access_allowed (unsigned int *addr)
+{
+ int result;
+
+ asm ("proberi (%1),3,%0" : "=r" (result) : "r" (addr) : );
+
+ return result;
+}
+
/* __canonicalize_funcptr_for_compare must be hidden so that it is not
placed in the dynamic symbol table. Like millicode functions, it
must be linked into all binaries in order access the got table of
@@ -82,6 +92,16 @@
The second word in the plabel contains the relocation offset for the
function. */
plabel = (unsigned int *) ((unsigned int) fptr & ~3);
+ if (!_dl_read_access_allowed (plabel))
+return (unsigned int) fptr;
+
+ /* Load first word of candidate descriptor. It should be a pointer
+ with word alignment and point to memory that can be read. */
+ got = (unsigned int *) plabel[0];
+ if (((unsigned int) got & 3) != 0
+ || !_dl_read_access_allowed (got))
+return (unsigned int) fptr;
+
got = (unsigned int *) (plabel[0] + GOT_FROM_PLT_STUB);
/* Return the address of the function if the plabel has been resolved. */
[Committed] PR fortran/64107 -- new test
It seems the patch for PR fortran/83633 cured
PR fortran/64107. I've added the code from
this PR to the test suite to ensure that it
doesn't get broken. Patche attached.
2018-03-06 Steven G. Kargl
PR fortran/64107
* gfortran.dg/pr64107.f90: New test.
--
Steve
Index: gcc/testsuite/gfortran.dg/pr64107.f90
===
--- gcc/testsuite/gfortran.dg/pr64107.f90 (nonexistent)
+++ gcc/testsuite/gfortran.dg/pr64107.f90 (working copy)
@@ -0,0 +1,30 @@
+! { dg-do compile }
+! PR fortran/64107
+! Code contribute by fxcoudert at gcc dot gnu dot org
+! Appears to be fixed by patch for PR fortran/83633
+module m1
+
+contains
+ pure integer function foo()
+foo = 2
+ end function
+end module
+
+subroutine test
+ use m1
+ integer :: x1(foo())
+end subroutine
+
+module m
+ use m1
+ integer :: x2(foo()) ! { dg-error "array with nonconstant bounds" }
+contains
+ subroutine sub
+integer :: x3(foo())
+ end subroutine
+end module
+
+program p
+ use m1
+ integer :: x4(foo()) ! { dg-error "array with nonconstant bounds" }
+end program
[PATCH] PR fortran/64124,70409 -- Reduce a charlen
All,
As everyone knows, gfortran reads source into a trees and
and at some point she passes those trees to a resolution
phases. There are instances, for example the new tests,
where resolution fails to use the character length
parameter in declaration statements. The attach patch
seems to cure this problem.
Previously, after reading the scalar-integer-expr, gfortran
would assign the expression to the charlen without trying
to reduce it to a constant (which should happen but doesn't
in the resolution phase). The patch now tries to reduce
the scalar-integer-expr to a constant, and then assigns
that constant to the charlen.
Bootstrap and regression tested on 7-branch and trunk.
OK to commit?
2018-03-06 Steven G. Kargl
PR fortran/64124
PR fortran/70409
* decl.c (gfc_match_char_spec): Try to reduce a charlen to a constant.
2018-03-06 Steven G. Kargl
PR fortran/64124
PR fortran/70409
* gfortran.dg/pr64124.f90: New tests.
* gfortran.dg/pr70409.f90: New tests.
--
Steve
Index: gcc/fortran/decl.c
===
--- gcc/fortran/decl.c (revision 258304)
+++ gcc/fortran/decl.c (working copy)
@@ -3147,7 +3147,24 @@ done:
if (seen_length == 0)
cl->length = gfc_get_int_expr (gfc_charlen_int_kind, NULL, 1);
else
-cl->length = len;
+{
+ /* If gfortran ends up here, then the len may be reducible to a
+ constant. Try to do that here. If it does not reduce, simply
+ assign len to the charlen. */
+ if (len && len->expr_type != EXPR_CONSTANT)
+ {
+ gfc_expr *e;
+ e = gfc_copy_expr (len);
+ gfc_reduce_init_expr (e);
+ if (e->expr_type == EXPR_CONSTANT)
+ gfc_replace_expr (len, e);
+ else
+ gfc_free_expr (e);
+ cl->length = len;
+ }
+ else
+ cl->length = len;
+}
ts->u.cl = cl;
ts->kind = kind == 0 ? gfc_default_character_kind : kind;
Index: gcc/testsuite/gfortran.dg/pr64124.f90
===
--- gcc/testsuite/gfortran.dg/pr64124.f90 (nonexistent)
+++ gcc/testsuite/gfortran.dg/pr64124.f90 (working copy)
@@ -0,0 +1,5 @@
+! { dg-do compile }
+! PR fortran/64124.f90
+ character(len=kind(1)) x
+ integer(len(x)) y
+ end
Index: gcc/testsuite/gfortran.dg/pr70409.f90
===
--- gcc/testsuite/gfortran.dg/pr70409.f90 (nonexistent)
+++ gcc/testsuite/gfortran.dg/pr70409.f90 (working copy)
@@ -0,0 +1,23 @@
+! { dg-do run }
+! PR fortran/70409
+! Contriubted by Harald Anlauf
+program foo
+ integer, parameter :: huge_1 = huge(0_1)
+ character(huge_1 ), parameter :: x = 'abc'
+ character(huge(0_1) ), parameter :: y = 'abc'
+ character(huge(0_1)+0 ), parameter :: z = 'abcdef'
+ character(huge(0_1) ):: a = 'abc'
+ integer, parameter :: huge_2 = huge(0_2)
+ character(huge_2 ), parameter :: u = 'abc'
+ character(huge(0_2) ), parameter :: v = 'abc'
+ character(int(huge(0_2),4)), parameter :: w = 'abcdef'
+ character(huge(0_2) ):: b = 'abc'
+ if (len(x) /= huge_1) stop 1
+ if (len(y) /= huge_1) stop 2
+ if (len(z) /= huge_1) stop 3
+ if (len(a) /= huge_1) stop 4
+ if (len(u) /= huge_2) stop 5
+ if (len(v) /= huge_2) stop 6
+ if (len(w) /= huge_2) stop 7
+ if (len(b) /= huge_2) stop 8
+end program foo
[C PATCH] Fix statement frontiers handling in the C FE (PR c/84721)
Hi!
The C FE in multiple spots checks building_stmt_list_p () to decide if
we are inside of parsing of functions or outside of that.
Unfortunately, that breaks with add_debug_begin_stmt which pushes
DEBUG_BEGIN_STMTs regardless of the scope it appears in; e.g. on the
testcase below it pushes DEBUG_BEGIN_STMT already for the int a
declaration in column 1 on line 5, and so with -g building_stmt_list_p ()
is pretty much always true.
Fixed by only pushing DEBUG_BEGIN_STMTs when the building_stmt_list_p ()
predicate is true, they aren't really useful outside of functions anyway.
Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
2018-03-06 Jakub Jelinek
PR c/84721
* c-parser.c (add_debug_begin_stmt): Don't add DEBUG_BEGIN_STMT if
!building_stmt_list_p ().
* gcc.dg/pr84721.c: New test.
--- gcc/c/c-parser.c.jj 2018-02-06 13:12:49.320804579 +0100
+++ gcc/c/c-parser.c2018-03-06 10:56:54.207194189 +0100
@@ -1654,7 +1654,8 @@ static void c_finish_oacc_routine (struc
static void
add_debug_begin_stmt (location_t loc)
{
- if (!MAY_HAVE_DEBUG_MARKER_STMTS)
+ /* Don't add DEBUG_BEGIN_STMTs outside of functions, see PR84721. */
+ if (!MAY_HAVE_DEBUG_MARKER_STMTS || !building_stmt_list_p ())
return;
tree stmt = build0 (DEBUG_BEGIN_STMT, void_type_node);
--- gcc/testsuite/gcc.dg/pr84721.c.jj 2018-03-06 11:03:23.798005268 +0100
+++ gcc/testsuite/gcc.dg/pr84721.c 2018-03-06 11:02:59.741016937 +0100
@@ -0,0 +1,6 @@
+/* PR c/84721 */
+/* { dg-do compile } */
+/* { dg-options "-g -O2" } */
+
+int a[({ int b })];/* { dg-error "braced-group within expression
allowed only inside a function" } */
+int c[({ int d () {}; })]; /* { dg-error "braced-group within expression
allowed only inside a function" } */
Jakub
Re: [PATCH] Avoid dumping fancy names in -fcompare-debug dumps (PR c++/84704)
On 03/06/2018 01:47 PM, Jakub Jelinek wrote: > Hi! > > As discussed, e.g. gimplification of STATEMENT_LISTs can create extra > retval.N temporaries for -g and get the counts out of sync, similarly > the SAVE_EXPR change proposed in the fix for that PR could for decls > in the statement expressions or addresses thereof get the fancy names > counters out of sync. > > The following patch marks the temporaries DECL_NAMELESS in addition to > DECL_IGNORED_P and in the dumps used for -fcompare-debug checking > prints those as D. rather than retval.234. > > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? > > 2018-03-06 Jakub Jelinek > > PR c++/84704 > * gimple-expr.c (create_tmp_var_raw): Set DECL_NAMELESS flag > on tmp_var. > * tree-pretty-print.c (dump_decl_name): For TDF_COMPARE_DEBUG, > don't print names of DECL_NAMELESS DECL_IGNORED_P decls. OK. jeff
Re: [patch, fortran] Fix PR 84697
On Tue, Mar 06, 2018 at 10:40:14PM +0100, Thomas Koenig wrote: > > the attached patch fixes a bug, partly an 8 regression, for > simplifying an expression containing minloc or maxloc. > > The underlying problem was that > > integer, dimension(0), parameter :: z=0 > > ended up as EXPR_CONSTANT even though the rank was one, which > was then passed to the simplification routines, which either > ICEd or gave wrong results. > > In doing this, I had to change the logic of the is_size_zero_array > function. Trying to call it from within the simplification rountines > led to the simplification routines to be called, and so on... until > the stack ran out. > > As soon as this is committed, I'll also look if there is anything left > in PR66128, and close that bug if appropriate I don't think there is anything of value left unless you want to investigate the old g77 compatibility functions. > > Regression-tested. OK for trunk? > OK. -- steve
Re: [C++ Patch/RFC] PR 71169 ("[7/8 Regression] ICE on invalid C++ code in pop_nested_class"), PR 71832 and more
Hi,
On 06/03/2018 21:33, Jason Merrill wrote:
Interesting, that seems like a promising idea. I'm not sure we need
to do this based on an error in a default template arg, though; can we
drop
+ || error_operand_p (TREE_PURPOSE (parameter)))
?
Good point, yes, I believe we can, isn't necessary for all the snippets
I have around neither apparently to pass the testsuite (but this is
rather straightforward, right?). As I said, I tried many different
things, some also fiddled with TREE_PURPOSE, in pt.c too, but in what I
sent I only added the check out of a reflex. Anyway, the below is in
libstdc++, so far so good.
Thanks,
Paolo.
//
Index: cp/parser.c
===
--- cp/parser.c (revision 258264)
+++ cp/parser.c (working copy)
@@ -565,6 +565,8 @@ cp_debug_parser (FILE *file, cp_parser *parser)
"local class", parser->in_function_body);
cp_debug_print_flag (file, "Auto correct a colon to a scope operator",
parser->colon_corrects_to_scope_p);
+ cp_debug_print_flag (file, "Error in template parameter list",
+ parser->error_in_template_parameter_list_p);
cp_debug_print_flag (file, "Colon doesn't start a class definition",
parser->colon_doesnt_start_class_def_p);
if (parser->type_definition_forbidden_message)
@@ -3910,6 +3912,9 @@ cp_parser_new (void)
/* We can correct until told otherwise. */
parser->colon_corrects_to_scope_p = true;
+ /* No error so far. */
+ parser->error_in_template_parameter_list_p = false;
+
/* The unparsed function queue is empty. */
push_unparsed_function_queues (parser);
@@ -10151,6 +10156,8 @@ cp_parser_lambda_expression (cp_parser* parser)
/* Inside the class, surrounding template-parameter-lists do not apply. */
unsigned int saved_num_template_parameter_lists
= parser->num_template_parameter_lists;
+bool saved_error_in_template_parameter_list_p
+= parser->error_in_template_parameter_list_p;
unsigned char in_statement = parser->in_statement;
bool in_switch_statement_p = parser->in_switch_statement_p;
bool fully_implicit_function_template_p
@@ -10161,6 +10168,7 @@ cp_parser_lambda_expression (cp_parser* parser)
= parser->auto_is_implicit_function_template_parm_p;
parser->num_template_parameter_lists = 0;
+parser->error_in_template_parameter_list_p = false;
parser->in_statement = 0;
parser->in_switch_statement_p = false;
parser->fully_implicit_function_template_p = false;
@@ -10199,6 +10207,8 @@ cp_parser_lambda_expression (cp_parser* parser)
type = finish_struct (type, /*attributes=*/NULL_TREE);
parser->num_template_parameter_lists = saved_num_template_parameter_lists;
+parser->error_in_template_parameter_list_p
+ = saved_error_in_template_parameter_list_p;
parser->in_statement = in_statement;
parser->in_switch_statement_p = in_switch_statement_p;
parser->fully_implicit_function_template_p
@@ -10624,7 +10634,8 @@ cp_parser_lambda_declarator_opt (cp_parser* parser
{
fco = finish_member_template_decl (fco);
finish_template_decl (template_param_list);
- --parser->num_template_parameter_lists;
+ if (--parser->num_template_parameter_lists == 0)
+ parser->error_in_template_parameter_list_p = false;
}
else if (parser->fully_implicit_function_template_p)
fco = finish_fully_implicit_template (parser, fco);
@@ -15065,6 +15076,10 @@ cp_parser_template_parameter_list (cp_parser* pars
parameter_list = chainon (parameter_list, err_parm);
}
+ if (parameter == error_mark_node
+ || error_operand_p (TREE_VALUE (parameter)))
+ parser->error_in_template_parameter_list_p = true;
+
/* If the next token is not a `,', we're done. */
if (cp_lexer_next_token_is_not (parser->lexer, CPP_COMMA))
break;
@@ -16673,7 +16688,8 @@ cp_parser_explicit_specialization (cp_parser* pars
if (need_lang_pop)
pop_lang_context ();
/* We're done with this parameter list. */
- --parser->num_template_parameter_lists;
+ if (--parser->num_template_parameter_lists == 0)
+parser->error_in_template_parameter_list_p = false;
}
/* Parse a type-specifier.
@@ -22438,6 +22454,7 @@ cp_parser_class_specifier_1 (cp_parser* parser)
tree attributes = NULL_TREE;
bool nested_name_specifier_p;
unsigned saved_num_template_parameter_lists;
+ bool saved_error_in_template_parameter_list_p;
bool saved_in_function_body;
unsigned char in_statement;
bool in_switch_statement_p;
@@ -22480,6 +22497,9 @@ cp_parser_class_specifier_1 (cp_parser* parser)
saved_num_template_parameter_lists
= parser->num_template_parameter_lists;
parser->num_template_parameter_lists = 0;
+ saved_error_in_template_parameter_list_p
+= parser->error_in_template_parame
[patch, fortran] Fix PR 84697
Hello world,
the attached patch fixes a bug, partly an 8 regression, for
simplifying an expression containing minloc or maxloc.
The underlying problem was that
integer, dimension(0), parameter :: z=0
ended up as EXPR_CONSTANT even though the rank was one, which
was then passed to the simplification routines, which either
ICEd or gave wrong results.
In doing this, I had to change the logic of the is_size_zero_array
function. Trying to call it from within the simplification rountines
led to the simplification routines to be called, and so on... until
the stack ran out.
As soon as this is committed, I'll also look if there is anything left
in PR66128, and close that bug if appropriate
Regression-tested. OK for trunk?
2017-03-06 Thomas Koenig
PR fortran/84697
PR fortran/66128
* expr.c (simplify_parameter_variable): If p is a size zero array
and not an ARRAY_EXPR insert an empty array constructor and
return.
* gfortran.h: Add prototype for gfc_is_size_zero_array.
* simplify.c (is_size_zero_array): Make non-static and rename into
(gfc_is_size_zero_array): Check for parameter arrays of zero
size by comparing shape and absence of constructor.
(gfc_simplify_all): Use gfc_is_size_zero_array instead of
is_size_zero_array.
(gfc_simplify_count): Likewise.
(gfc_simplify_iall): Likewise.
(gfc_simplify_iany): Likewise.
(gfc_simplify_iparity): Likewise.
(gfc_simplify_minval): Likewise.
(gfc_simplify_maxval): Likewise.
(gfc_simplify_product): Likewise.
(gfc_simplify_sum): Likewise.
2017-03-06 Thomas Koenig
PR fortran/84697
PR fortran/66128
* gfortran.dg/minmaxloc_zerosize_1.f90: New test.
Index: expr.c
===
--- expr.c (Revision 258264)
+++ expr.c (Arbeitskopie)
@@ -1857,6 +1857,22 @@ simplify_parameter_variable (gfc_expr *p, int type
gfc_expr *e;
bool t;
+ if (gfc_is_size_zero_array (p))
+{
+ if (p->expr_type == EXPR_ARRAY)
+ return true;
+
+ e = gfc_get_expr ();
+ e->expr_type = EXPR_ARRAY;
+ e->ts = p->ts;
+ e->rank = p->rank;
+ e->value.constructor = NULL;
+ e->shape = gfc_copy_shape (p->shape, p->rank);
+ e->where = p->where;
+ gfc_replace_expr (p, e);
+ return true;
+}
+
e = gfc_copy_expr (p->symtree->n.sym->value);
if (e == NULL)
return false;
Index: gfortran.h
===
--- gfortran.h (Revision 258264)
+++ gfortran.h (Arbeitskopie)
@@ -3464,6 +3464,7 @@ int gfc_code_walker (gfc_code **, walk_code_fn_t,
void gfc_convert_mpz_to_signed (mpz_t, int);
gfc_expr *gfc_simplify_ieee_functions (gfc_expr *);
+bool gfc_is_size_zero_array (gfc_expr *);
/* trans-array.c */
Index: simplify.c
===
--- simplify.c (Revision 258264)
+++ simplify.c (Arbeitskopie)
@@ -259,26 +259,28 @@ is_constant_array_expr (gfc_expr *e)
}
/* Test for a size zero array. */
-static bool
-is_size_zero_array (gfc_expr *array)
+bool
+gfc_is_size_zero_array (gfc_expr *array)
{
- gfc_expr *e;
- bool t;
- e = gfc_copy_expr (array);
- gfc_simplify_expr (e, 1);
+ if (array->rank == 0)
+return false;
- if (e->expr_type == EXPR_CONSTANT && e->rank > 0 && !e->shape)
- t = true;
- else if (e->expr_type == EXPR_ARRAY && e->rank > 0
- && !e->shape && !e->value.constructor)
- t = true;
- else
- t = false;
+ if (array->expr_type == EXPR_VARIABLE && array->rank > 0
+ && array->symtree->n.sym->attr.flavor == FL_PARAMETER
+ && array->shape != NULL)
+{
+ for (int i = 0; i < array->rank; i++)
+ if (mpz_cmp_si (array->shape[i], 0) <= 0)
+ return true;
- gfc_free_expr (e);
+ return false;
+}
- return t;
+ if (array->expr_type == EXPR_ARRAY)
+return array->value.constructor == NULL;
+
+ return false;
}
@@ -974,7 +976,7 @@ gfc_simplify_aint (gfc_expr *e, gfc_expr *k)
gfc_expr *
gfc_simplify_all (gfc_expr *mask, gfc_expr *dim)
{
- if (is_size_zero_array (mask))
+ if (gfc_is_size_zero_array (mask))
return gfc_get_logical_expr (mask->ts.kind, &mask->where, true);
return simplify_transformation (mask, dim, NULL, true, gfc_and);
@@ -1066,7 +1068,7 @@ gfc_simplify_and (gfc_expr *x, gfc_expr *y)
gfc_expr *
gfc_simplify_any (gfc_expr *mask, gfc_expr *dim)
{
- if (is_size_zero_array (mask))
+ if (gfc_is_size_zero_array (mask))
return gfc_get_logical_expr (mask->ts.kind, &mask->where, false);
return simplify_transformation (mask, dim, NULL, false, gfc_or);
@@ -1965,7 +1967,7 @@ gfc_simplify_count (gfc_expr *mask, gfc_expr *dim,
{
gfc_expr *result;
- if (is_size_zero_array (mask))
+ if (gfc_is_size_zero_array (mask))
{
int k;
k = kind ? mpz_get_si (kind->value.integer) : gfc_default_inte
Re: Fix some _GLIBCXX_DEBUG pretty printer errors
On 5 February 2018 at 06:49, François Dumont wrote:
> Hi
>
> Here is a patch to fix some pretty printer check errors when running
> those tests with _GLIBCXX_DEBUG.
>
> I introduced a special rendered for the std::forward_list iterator which
> is similar to the one used for the std::list and so used inheritance, I hope
> it is not a problem for Python 2/3 compatibility.
>
> I prefer to just rely on normal iterator rendered when we detect that
> iterator hasn't been initialized so that normal or debug modes give the same
> result.
>
> Ok to commit ?
>
> François
>
>
>@@ -575,10 +586,12 @@ class StdDebugIteratorPrinter:
> # and return the wrapped iterator value.
> def to_string (self):
> base_type = gdb.lookup_type('__gnu_debug::_Safe_iterator_base')
>+itype = self.val.type.template_argument(0)
> safe_seq = self.val.cast(base_type)['_M_sequence']
>-if not safe_seq or self.val['_M_version'] != safe_seq['_M_version']:
>+if not safe_seq:
>+return str(self.val.cast(itype))
So what's the effect of this change when we get a value-initialized
debug iterator? It prints the wrapped (value-initialized) non-debug
iterator instead?
Re: [v3 PATCH] PR libstdc++/84601
On 28/02/18 15:12 +0200, Ville Voutilainen wrote: >- // Payload for constexpr optionals. >+ // Payload for optionals with non-trivial destructor. > templatebool /*_HasTrivialDestructor*/ = >-is_trivially_destructible<_Tp>::value> >+is_trivially_destructible<_Tp>::value, >+ bool /*_HasTrivialCopyAssignment*/ = >+is_trivially_copy_assignable<_Tp>::value, >+ bool /*_HasTrivialMoveAssignment*/ = >+is_trivially_move_assignable<_Tp>::value> I'm not sure these comments are very useful, as they just repeat the info that the traits already give us. Also, you could use the _v variable templates if you wanted (doesn't make much difference though). But on the subject of redundant comments ... > struct _Optional_payload It took me a minute to figure out which conditions the primary template gets used for, to double-check the comment. Would it be helpful to use a comment like: struct _Optional_payload // or does that not really clarify anything? I suppose it doesn't tell us any more than the "non-trivial destructor" comment you already have. So OK for trunk then.
[PATCH] Reject target_clones attribute in functions that can't be cloned (PR middle-end/84723)
Hi!
The following testcases show various reasons why a function definition
with target_clones attribute can't be cloned; instead of ICEing because
node->create_version_clone_with_body returns NULL and we dereference it,
this patch just diagnoses those and ignores the attribute.
Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
Martin Sebor said he wants to work on attribute exclusion, in that case
the error messages in pr84723-{1,4,5}.c would be adjusted if needed and
const char *reason = NULL;
if (lookup_attribute ("noclone", DECL_ATTRIBUTES (node->decl)))
reason = G_("function %q+F can never be copied "
"because it has % attribute");
else
reason = copy_forbidden (DECL_STRUCT_FUNCTION (node->decl));
could be replaced just with
const char *reason = copy_forbidden (DECL_STRUCT_FUNCTION (node->decl));
but attribute exclusions can't really help for the cases where cloning
is refused because of other reasons.
2018-03-06 Jakub Jelinek
PR middle-end/84723
* multiple_target.c: Include tree-inline.h and intl.h.
(expand_target_clones): Diagnose and fail if node->definition and
!tree_versionable_function_p (node->decl).
* gcc.target/i386/pr84723-1.c: New test.
* gcc.target/i386/pr84723-2.c: New test.
* gcc.target/i386/pr84723-3.c: New test.
* gcc.target/i386/pr84723-4.c: New test.
* gcc.target/i386/pr84723-5.c: New test.
--- gcc/multiple_target.c.jj2018-01-03 10:19:54.956533925 +0100
+++ gcc/multiple_target.c 2018-03-06 11:46:09.327627941 +0100
@@ -36,6 +36,8 @@ along with GCC; see the file COPYING3.
#include "pretty-print.h"
#include "gimple-iterator.h"
#include "gimple-walk.h"
+#include "tree-inline.h"
+#include "intl.h"
/* Walker callback that replaces all FUNCTION_DECL of a function that's
going to be versioned. */
@@ -312,6 +314,22 @@ expand_target_clones (struct cgraph_node
return false;
}
+ if (node->definition
+ && !tree_versionable_function_p (node->decl))
+{
+ error_at (DECL_SOURCE_LOCATION (node->decl),
+ "clones for % attribute cannot be created");
+ const char *reason = NULL;
+ if (lookup_attribute ("noclone", DECL_ATTRIBUTES (node->decl)))
+ reason = G_("function %q+F can never be copied "
+ "because it has % attribute");
+ else
+ reason = copy_forbidden (DECL_STRUCT_FUNCTION (node->decl));
+ if (reason)
+ inform (DECL_SOURCE_LOCATION (node->decl), reason, node->decl);
+ return false;
+}
+
char *attr_str = XNEWVEC (char, attr_len);
int attrnum = get_attr_str (arglist, attr_str);
char **attrs = XNEWVEC (char *, attrnum);
--- gcc/testsuite/gcc.target/i386/pr84723-1.c.jj2018-03-06
12:00:17.533367738 +0100
+++ gcc/testsuite/gcc.target/i386/pr84723-1.c 2018-03-06 11:52:15.831511845
+0100
@@ -0,0 +1,11 @@
+/* PR middle-end/84723 */
+/* { dg-do compile } */
+/* { dg-require-ifunc } */
+/* { dg-options "-O2" } */
+
+__attribute__((target_clones ("avx", "default")))
+__attribute__((noclone))
+void
+foo (void) /* { dg-error "clones for .target_clones. attribute cannot be
created" } */
+{ /* { dg-message "function .foo. can never be copied because it
has .noclone. attribute" "" { target *-*-* } .-1 } */
+}
--- gcc/testsuite/gcc.target/i386/pr84723-2.c.jj2018-03-06
12:00:21.019367631 +0100
+++ gcc/testsuite/gcc.target/i386/pr84723-2.c 2018-03-06 11:50:30.458545226
+0100
@@ -0,0 +1,13 @@
+/* PR middle-end/84723 */
+/* { dg-do compile } */
+/* { dg-require-ifunc } */
+/* { dg-options "-O2" } */
+
+__attribute__((target_clones ("avx", "default")))
+void
+foo (void) /* { dg-error "clones for .target_clones. attribute cannot be
created" } */
+{ /* { dg-message "function .foo. can never be copied because it
saves address of local label in a static variable" "" { target *-*-* } .-1 } */
+ static void *p = &&lab;
+ asm volatile ("" : "+m" (p) : : "memory");
+lab:;
+}
--- gcc/testsuite/gcc.target/i386/pr84723-3.c.jj2018-03-06
12:00:24.397367531 +0100
+++ gcc/testsuite/gcc.target/i386/pr84723-3.c 2018-03-06 11:50:57.274536736
+0100
@@ -0,0 +1,17 @@
+/* PR middle-end/84723 */
+/* { dg-do compile } */
+/* { dg-require-ifunc } */
+/* { dg-options "-O2" } */
+
+__attribute__((target_clones ("avx", "default")))
+int
+foo (int x)/* { dg-error "clones for .target_clones. attribute cannot be
created" } */
+{ /* { dg-message "function .foo. can never be copied because it
receives a non-local goto" "" { target *-*-* } .-1 } */
+ __label__ lab;
+ __attribute__((noinline)) void bar () { goto lab; }
+ if (x == 5)
+bar ();
+ x++;
+lab:;
+ return x;
+}
--- gcc/testsuite/gcc.target/i386/pr84723-4.c.jj2018-03-06
12:00:17.533367738 +0100
+++ gcc/testsuite/gcc.target/i386/pr84723-4.c 2018-03-06 11:52:15.831511845
+0100
@@ -0,0 +1,11 @@
Re: [C PATCH] Fix statement frontiers handling in the C FE (PR c/84721)
On Tue, Mar 06, 2018 at 09:51:57PM +0100, Jakub Jelinek wrote: > Hi! > > The C FE in multiple spots checks building_stmt_list_p () to decide if > we are inside of parsing of functions or outside of that. > > Unfortunately, that breaks with add_debug_begin_stmt which pushes > DEBUG_BEGIN_STMTs regardless of the scope it appears in; e.g. on the > testcase below it pushes DEBUG_BEGIN_STMT already for the int a > declaration in column 1 on line 5, and so with -g building_stmt_list_p () > is pretty much always true. > > Fixed by only pushing DEBUG_BEGIN_STMTs when the building_stmt_list_p () > predicate is true, they aren't really useful outside of functions anyway. > > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? Ok. Marek
Re: [committed] Fix combiner.c's RTL checking failure (PR target/84710)
On Tue, Mar 06, 2018 at 09:44:07PM +0100, Jakub Jelinek wrote: > As the following testcase shows on aarch64, the SET_DEST can be a normal > subreg, not only a paradoxical one or REG. > > Fixed thusly, bootstrapped/regtested on x86_64-linux and i686-linux, > preapproved by Segher in the PR, committed to trunk. Thanks! Segher > 2018-03-06 Jakub Jelinek > > PR target/84710 > * combine.c (try_combine): Use reg_or_subregno instead of handling > just paradoxical SUBREGs and REGs. > > * gcc.dg/pr84710.c: New test.
[PATCH] Avoid dumping fancy names in -fcompare-debug dumps (PR c++/84704)
Hi!
As discussed, e.g. gimplification of STATEMENT_LISTs can create extra
retval.N temporaries for -g and get the counts out of sync, similarly
the SAVE_EXPR change proposed in the fix for that PR could for decls
in the statement expressions or addresses thereof get the fancy names
counters out of sync.
The following patch marks the temporaries DECL_NAMELESS in addition to
DECL_IGNORED_P and in the dumps used for -fcompare-debug checking
prints those as D. rather than retval.234.
Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
2018-03-06 Jakub Jelinek
PR c++/84704
* gimple-expr.c (create_tmp_var_raw): Set DECL_NAMELESS flag
on tmp_var.
* tree-pretty-print.c (dump_decl_name): For TDF_COMPARE_DEBUG,
don't print names of DECL_NAMELESS DECL_IGNORED_P decls.
--- gcc/gimple-expr.c.jj2018-01-14 17:16:55.693836137 +0100
+++ gcc/gimple-expr.c 2018-03-06 09:02:08.268188451 +0100
@@ -446,6 +446,9 @@ create_tmp_var_raw (tree type, const cha
DECL_ARTIFICIAL (tmp_var) = 1;
/* And we don't want debug info for it. */
DECL_IGNORED_P (tmp_var) = 1;
+ /* And we don't want even the fancy names of those printed in
+ -fdump-final-insns= dumps. */
+ DECL_NAMELESS (tmp_var) = 1;
/* Make the variable writable. */
TREE_READONLY (tmp_var) = 0;
--- gcc/tree-pretty-print.c.jj 2018-01-31 21:38:08.005050021 +0100
+++ gcc/tree-pretty-print.c 2018-03-06 09:13:08.026095230 +0100
@@ -247,21 +247,32 @@ dump_fancy_name (pretty_printer *pp, tre
static void
dump_decl_name (pretty_printer *pp, tree node, dump_flags_t flags)
{
- if (DECL_NAME (node))
+ tree name = DECL_NAME (node);
+ if (name)
{
if ((flags & TDF_ASMNAME)
&& HAS_DECL_ASSEMBLER_NAME_P (node)
&& DECL_ASSEMBLER_NAME_SET_P (node))
pp_tree_identifier (pp, DECL_ASSEMBLER_NAME_RAW (node));
+ /* For -fcompare-debug don't dump DECL_NAMELESS names at all,
+-g might have created more fancy names and their indexes
+could get out of sync. Usually those should be DECL_IGNORED_P
+too, SRA can create even non-DECL_IGNORED_P DECL_NAMELESS fancy
+names, let's hope those never get out of sync after doing the
+dump_fancy_name sanitization. */
+ else if ((flags & TDF_COMPARE_DEBUG)
+ && DECL_NAMELESS (node)
+ && DECL_IGNORED_P (node))
+ name = NULL_TREE;
/* For DECL_NAMELESS names look for embedded uids in the
names and sanitize them for TDF_NOUID. */
else if ((flags & TDF_NOUID) && DECL_NAMELESS (node))
- dump_fancy_name (pp, DECL_NAME (node));
+ dump_fancy_name (pp, name);
else
- pp_tree_identifier (pp, DECL_NAME (node));
+ pp_tree_identifier (pp, name);
}
char uid_sep = (flags & TDF_GIMPLE) ? '_' : '.';
- if ((flags & TDF_UID) || DECL_NAME (node) == NULL_TREE)
+ if ((flags & TDF_UID) || name == NULL_TREE)
{
if (TREE_CODE (node) == LABEL_DECL && LABEL_DECL_UID (node) != -1)
pp_printf (pp, "L%c%d", uid_sep, (int) LABEL_DECL_UID (node));
Jakub
Re: C++ PATCH for c++/84684, wrong caching when evaluating a constexpr function
On Tue, Mar 06, 2018 at 03:39:36PM -0500, Jason Merrill wrote: > On Tue, Mar 6, 2018 at 1:13 PM, Marek Polacek wrote: > > But I'm also wondering about massage_init_elt. It has > > tree t = fold_non_dependent_expr (init); > > t = maybe_constant_init (t); > > but given that fold_non_dependent_expr now calls maybe_constant_value, which > > then causes that we try to cache the calls above, this seems excessive, > > wouldn't we be better off with just calling fold_non_dependent_init as > > discussed recently? > > Probably. Do you want me to try it for GCC 8 or should we table it for GCC 9? I would think the latter since it's not a regression, just an optimization. > OK. Thanks. Marek
[committed] Fix combiner.c's RTL checking failure (PR target/84710)
Hi!
As the following testcase shows on aarch64, the SET_DEST can be a normal
subreg, not only a paradoxical one or REG.
Fixed thusly, bootstrapped/regtested on x86_64-linux and i686-linux,
preapproved by Segher in the PR, committed to trunk.
2018-03-06 Jakub Jelinek
PR target/84710
* combine.c (try_combine): Use reg_or_subregno instead of handling
just paradoxical SUBREGs and REGs.
* gcc.dg/pr84710.c: New test.
--- gcc/combine.c.jj2018-03-05 23:13:26.478215559 +0100
+++ gcc/combine.c 2018-03-06 08:50:17.756288841 +0100
@@ -4283,12 +4283,8 @@ try_combine (rtx_insn *i3, rtx_insn *i2,
if (GET_CODE (x) == PARALLEL)
x = XVECEXP (newi2pat, 0, 0);
- /* It can only be a SET of a REG or of a paradoxical SUBREG of a REG. */
- x = SET_DEST (x);
- if (paradoxical_subreg_p (x))
- x = SUBREG_REG (x);
-
- unsigned int regno = REGNO (x);
+ /* It can only be a SET of a REG or of a SUBREG of a REG. */
+ unsigned int regno = reg_or_subregno (SET_DEST (x));
bool done = false;
for (rtx_insn *insn = NEXT_INSN (i3);
--- gcc/testsuite/gcc.dg/pr84710.c.jj 2018-03-06 08:56:00.667240389 +0100
+++ gcc/testsuite/gcc.dg/pr84710.c 2018-03-06 08:55:22.637245765 +0100
@@ -0,0 +1,13 @@
+/* PR target/84710 */
+/* { dg-do compile } */
+/* { dg-options "-O -fno-forward-propagate" } */
+
+char a;
+int b;
+
+void
+foo (void)
+{
+ int d;
+ b = __builtin_mul_overflow ((char) d, 0xfe, &a);
+}
Jakub
Re: C++ PATCH for c++/84684, wrong caching when evaluating a constexpr function
On Tue, Mar 6, 2018 at 1:13 PM, Marek Polacek wrote:
> In this testcase we have a constexpr function, value_to_char_helper. Its body
> is
>for (size_t i = 0u; i < alphabet_t::value_size; ++i)
> value_to_char[i] = to_char(alphabet.assign_rank(i));
>
> which is genericized to a LOOP_EXPR looking roughly like this:
>
> while (1)
> {
> if (i > 3)
> goto out;
> value_to_char[i] = to_char (..., i, ...);
> i++;
> }
>
> Then this is what happens when evaluating the above:
>
> 1) cxx_eval_call_expression evaluates the first call to to_char
>it's not in the hash table -> copy the body and the bindings and
>then evaluate the body
>the result is 65
>bindings: alph -> {._value=0} (correct)
>save all this to the table
> 2) cxx_eval_store_expression evaluates i++
>then it replaces the value in the hash map:
> *valp = init;
>which is generally what we want, but that also overwrites {._value=0} from
>above to {._value=1} which causes problem in 3)
> 3) cxx_eval_call_expression tries to evaluate the second call to to_char
>bindings: alph -> {._value=1} (correct)
>here if the hashes match (use [1] hunk for better testing) then
>we find to_char call in the table. Then we invoke
>constexpr_call_hasher::equal() to compare the two calls: fundefs
>match, and because 2) has overwritten bindings of the previous
>to_char call, they match too, both are {._value=1}.
>That means we don't evaluate this second call and just use the cached
>result (65), and that is wrong.
>
> I think the fix is to avoid sharing the constructor in the bindings which
> is what the patch does and what we do in several places already. I think
> Jakub's patch
> wouldn't work if the bindings had more constructors.
>
> But I'm also wondering about massage_init_elt. It has
> tree t = fold_non_dependent_expr (init);
> t = maybe_constant_init (t);
> but given that fold_non_dependent_expr now calls maybe_constant_value, which
> then causes that we try to cache the calls above, this seems excessive,
> wouldn't we be better off with just calling fold_non_dependent_init as
> discussed recently?
Probably.
> I bootstrapped/regtested this patch with this hunk, too, for better testing:
>
> [1]
> @@ -1598,8 +1598,12 @@ cxx_eval_call_expression (const constexpr_ctx *ctx,
> tree t,
>constexpr_call *entry = NULL;
>if (depth_ok && !non_constant_args && ctx->strict)
> {
> +#if 0
>new_call.hash = iterative_hash_template_arg
> (new_call.bindings, constexpr_fundef_hasher::hash (new_call.fundef));
> +#else
> + new_call.hash = 0;
> +#endif
>
>/* If we have seen this call before, we are done. */
>maybe_initialize_constexpr_call_table ();
>
> Bootstrapped/regtested on x86_64-linux, ok for trunk? And likely 7.
OK.
Jason
Re: [C++ Patch/RFC] PR 71169 ("[7/8 Regression] ICE on invalid C++ code in pop_nested_class"), PR 71832 and more
Interesting, that seems like a promising idea. I'm not sure we need to do this based on an error in a default template arg, though; can we drop + || error_operand_p (TREE_PURPOSE (parameter))) ? Jason
GCC 7 backports
Hi.
I'm sending GCC 7 branch backports I've just tested and regbootstrapped.
I'm going to install that.
Thanks,
Martin
>From 10479fa4d3576b19b58d7ffc0949c2828ab6c2ff Mon Sep 17 00:00:00 2001
From: segher
Date: Fri, 23 Feb 2018 14:17:35 +
Subject: [PATCH 17/17] Backport r257932
gcc/testsuite/ChangeLog:
2018-02-23 Segher Boessenkool
PR testsuite/80551
* c-c++-common/tsan/race_on_mutex.c: Change regexp to allow
__GI___pthread_mutex_init as well.
---
gcc/testsuite/c-c++-common/tsan/race_on_mutex.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/gcc/testsuite/c-c++-common/tsan/race_on_mutex.c b/gcc/testsuite/c-c++-common/tsan/race_on_mutex.c
index def1d47de7b..2e4b7ac3cb9 100644
--- a/gcc/testsuite/c-c++-common/tsan/race_on_mutex.c
+++ b/gcc/testsuite/c-c++-common/tsan/race_on_mutex.c
@@ -42,5 +42,5 @@ int main() {
/* { dg-output "#1 Thread2.* .*(race_on_mutex.c:22|\\?{2}:0) (.*)" } */
/* { dg-output " Previous write of size \[0-9]\+ at .* by thread T1:(\n|\r\n|\r)" } */
/* { dg-output "(#0 \[^\n\r\]*(\n|\r\n|\r))?" } */
-/* { dg-output "#\[01\] (__)?pthread_mutex_init \[^\n\r\]* (.)*" } */
+/* { dg-output "#\[01\] ((__GI_)?__)?pthread_mutex_init \[^\n\r\]* (.)*" } */
/* { dg-output "#\[12\] Thread1.* .*(race_on_mutex.c:12|\\?{2}:0) .*" } */
--
2.16.2
>From cdf8aac55c7bddbb4e25bab8368f3057d1387d36 Mon Sep 17 00:00:00 2001
From: marxin
Date: Tue, 20 Feb 2018 10:04:13 +
Subject: [PATCH 16/17] Backport r257842
gcc/ChangeLog:
2018-02-20 Martin Liska
PR c/84310
PR target/79747
* final.c (shorten_branches): Build align_tab array with one
more element.
* opts.c (finish_options): Add alignment option limit check.
(MAX_CODE_ALIGN): Likewise.
(MAX_CODE_ALIGN_VALUE): Likewise.
* doc/invoke.texi: Document maximum allowed option value for
all -falign-* options.
gcc/testsuite/ChangeLog:
2018-02-20 Martin Liska
PR c/84310
PR target/79747
* gcc.target/i386/pr84310.c: New test.
* gcc.target/i386/pr84310-2.c: Likewise.
---
gcc/doc/invoke.texi | 4
gcc/final.c | 4 ++--
gcc/opts.c| 20
gcc/testsuite/gcc.target/i386/pr84310-2.c | 10 ++
gcc/testsuite/gcc.target/i386/pr84310.c | 8
5 files changed, 44 insertions(+), 2 deletions(-)
create mode 100644 gcc/testsuite/gcc.target/i386/pr84310-2.c
create mode 100644 gcc/testsuite/gcc.target/i386/pr84310.c
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index 41bb8f05216..d65bd32a092 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -8690,6 +8690,7 @@ Some assemblers only support this flag when @var{n} is a power of two;
in that case, it is rounded up.
If @var{n} is not specified or is zero, use a machine-dependent default.
+The maximum allowed @var{n} option value is 65536.
Enabled at levels @option{-O2}, @option{-O3}.
@@ -8715,6 +8716,7 @@ are greater than this value, then their values are used instead.
If @var{n} is not specified or is zero, use a machine-dependent default
which is very likely to be @samp{1}, meaning no alignment.
+The maximum allowed @var{n} option value is 65536.
Enabled at levels @option{-O2}, @option{-O3}.
@@ -8728,6 +8730,7 @@ operations.
@option{-fno-align-loops} and @option{-falign-loops=1} are
equivalent and mean that loops are not aligned.
+The maximum allowed @var{n} option value is 65536.
If @var{n} is not specified or is zero, use a machine-dependent default.
@@ -8745,6 +8748,7 @@ need be executed.
equivalent and mean that loops are not aligned.
If @var{n} is not specified or is zero, use a machine-dependent default.
+The maximum allowed @var{n} option value is 65536.
Enabled at levels @option{-O2}, @option{-O3}.
diff --git a/gcc/final.c b/gcc/final.c
index 820162b2d28..20af67816bb 100644
--- a/gcc/final.c
+++ b/gcc/final.c
@@ -906,7 +906,7 @@ shorten_branches (rtx_insn *first)
char *varying_length;
rtx body;
int uid;
- rtx align_tab[MAX_CODE_ALIGN];
+ rtx align_tab[MAX_CODE_ALIGN + 1];
/* Compute maximum UID and allocate label_align / uid_shuid. */
max_uid = get_max_uid ();
@@ -1015,7 +1015,7 @@ shorten_branches (rtx_insn *first)
alignment of n. */
uid_align = XCNEWVEC (rtx, max_uid);
- for (i = MAX_CODE_ALIGN; --i >= 0;)
+ for (i = MAX_CODE_ALIGN + 1; --i >= 0;)
align_tab[i] = NULL_RTX;
seq = get_last_insn ();
for (; seq; seq = PREV_INSN (seq))
diff --git a/gcc/opts.c b/gcc/opts.c
index f03b57aa343..e5126618f35 100644
--- a/gcc/opts.c
+++ b/gcc/opts.c
@@ -1014,6 +1014,26 @@ finish_options (struct gcc_options *opts, struct gcc_options *opts_set,
if ((opts->x_flag_sanitize & SANITIZE_KERNEL_ADDRESS) && opts->x_flag_tm)
sorry ("transactional memory is not supported with "
"%<-fsanitize=kernel-address%>");
+
+ /* Comes from final.c -- no real reason to change it. */
+#define MAX_CODE_ALIGN 16
+#define MAX_CODE_ALIGN_VALU
Re: [C++ Patch/RFC] PR 71169 ("[7/8 Regression] ICE on invalid C++ code in pop_nested_class"), PR 71832 and more
Hi again,
On 06/03/2018 19:58, Paolo Carlini wrote:
.. g, consider the patch withdrawn in its detailed form, I have a
new testcase which add some random code after that ill-formed class
and it's mishandled, it ICEs again.
The below avoids the idiotic thinko (I added a testcase for that) and is
likewise passing testing.
Thanks,
Paolo.
/
Index: cp/parser.c
===
--- cp/parser.c (revision 258264)
+++ cp/parser.c (working copy)
@@ -565,6 +565,8 @@ cp_debug_parser (FILE *file, cp_parser *parser)
"local class", parser->in_function_body);
cp_debug_print_flag (file, "Auto correct a colon to a scope operator",
parser->colon_corrects_to_scope_p);
+ cp_debug_print_flag (file, "Error in template parameter list",
+ parser->error_in_template_parameter_list_p);
cp_debug_print_flag (file, "Colon doesn't start a class definition",
parser->colon_doesnt_start_class_def_p);
if (parser->type_definition_forbidden_message)
@@ -3910,6 +3912,9 @@ cp_parser_new (void)
/* We can correct until told otherwise. */
parser->colon_corrects_to_scope_p = true;
+ /* No error so far. */
+ parser->error_in_template_parameter_list_p = false;
+
/* The unparsed function queue is empty. */
push_unparsed_function_queues (parser);
@@ -10151,6 +10156,8 @@ cp_parser_lambda_expression (cp_parser* parser)
/* Inside the class, surrounding template-parameter-lists do not apply. */
unsigned int saved_num_template_parameter_lists
= parser->num_template_parameter_lists;
+bool saved_error_in_template_parameter_list_p
+= parser->error_in_template_parameter_list_p;
unsigned char in_statement = parser->in_statement;
bool in_switch_statement_p = parser->in_switch_statement_p;
bool fully_implicit_function_template_p
@@ -10161,6 +10168,7 @@ cp_parser_lambda_expression (cp_parser* parser)
= parser->auto_is_implicit_function_template_parm_p;
parser->num_template_parameter_lists = 0;
+parser->error_in_template_parameter_list_p = false;
parser->in_statement = 0;
parser->in_switch_statement_p = false;
parser->fully_implicit_function_template_p = false;
@@ -10199,6 +10207,8 @@ cp_parser_lambda_expression (cp_parser* parser)
type = finish_struct (type, /*attributes=*/NULL_TREE);
parser->num_template_parameter_lists = saved_num_template_parameter_lists;
+parser->error_in_template_parameter_list_p
+ = saved_error_in_template_parameter_list_p;
parser->in_statement = in_statement;
parser->in_switch_statement_p = in_switch_statement_p;
parser->fully_implicit_function_template_p
@@ -10624,7 +10634,8 @@ cp_parser_lambda_declarator_opt (cp_parser* parser
{
fco = finish_member_template_decl (fco);
finish_template_decl (template_param_list);
- --parser->num_template_parameter_lists;
+ if (--parser->num_template_parameter_lists == 0)
+ parser->error_in_template_parameter_list_p = false;
}
else if (parser->fully_implicit_function_template_p)
fco = finish_fully_implicit_template (parser, fco);
@@ -15065,6 +15076,11 @@ cp_parser_template_parameter_list (cp_parser* pars
parameter_list = chainon (parameter_list, err_parm);
}
+ if (parameter == error_mark_node
+ || error_operand_p (TREE_VALUE (parameter))
+ || error_operand_p (TREE_PURPOSE (parameter)))
+ parser->error_in_template_parameter_list_p = true;
+
/* If the next token is not a `,', we're done. */
if (cp_lexer_next_token_is_not (parser->lexer, CPP_COMMA))
break;
@@ -16673,7 +16689,8 @@ cp_parser_explicit_specialization (cp_parser* pars
if (need_lang_pop)
pop_lang_context ();
/* We're done with this parameter list. */
- --parser->num_template_parameter_lists;
+ if (--parser->num_template_parameter_lists == 0)
+parser->error_in_template_parameter_list_p = false;
}
/* Parse a type-specifier.
@@ -22438,6 +22455,7 @@ cp_parser_class_specifier_1 (cp_parser* parser)
tree attributes = NULL_TREE;
bool nested_name_specifier_p;
unsigned saved_num_template_parameter_lists;
+ bool saved_error_in_template_parameter_list_p;
bool saved_in_function_body;
unsigned char in_statement;
bool in_switch_statement_p;
@@ -22480,6 +22498,9 @@ cp_parser_class_specifier_1 (cp_parser* parser)
saved_num_template_parameter_lists
= parser->num_template_parameter_lists;
parser->num_template_parameter_lists = 0;
+ saved_error_in_template_parameter_list_p
+= parser->error_in_template_parameter_list_p;
+ parser->error_in_template_parameter_list_p = false;
/* We are not in a function body. */
saved_in_function_body = parser->in_function_body;
parser->in_function_body = false;
@@ -22658,8 +22679,13 @@ cp_parser_c
Re: [C++ Patch/RFC] PR 71169 ("[7/8 Regression] ICE on invalid C++ code in pop_nested_class"), PR 71832 and more
.. g, consider the patch withdrawn in its detailed form, I have a new testcase which add some random code after that ill-formed class and it's mishandled, it ICEs again. Well, I would still appreciate some feedback about the general idea... Paolo.
[C++ Patch/RFC] PR 71169 ("[7/8 Regression] ICE on invalid C++ code in pop_nested_class"), PR 71832 and more
Hi, not considering the various rather mild error recovery regressions which we have in this area, I have been working on and off on these issues for a while, as QoI diagnostic improvements: when something wrong happens while parsing the template parameter lists we want anyway to keep track of all the parameters, erroneous and not, and we go on handling the body of the class (we are mostly considering classes here but not necessarily so in the future). Then, very often, during error recovery the hell breaks loose and we emit a ton of redundant diagnostic about member function calls, overload resolution, etc, all due to the broken template parameters. Today finally I had this very simply (but a bit invasive in this Stage!?!) idea which, of all those I tried (like, skipping completely the body, etc) is the one which so far works best, appears to strike a good balance between keeping information useful for error recovery and skipping those manipulations which, when the parameters are broken, are bound to fail anyway. Thus the idea would be adding a flag to struct cp_parser, set it when cp_parser_template_parameter returns an error and use it later in cp_parser_class_specifier_1 in order to skip parsing the queued function definitions. The solution passes the testusite as-is and, while a bit invasive, seems rather safe, because, if I'm not badly mistaken, it's about recovery, isn't that we are risking rejecting valid code. What do you think? Well, I final thing: I briefly wondered today if adding the flag is an issue from the memory footprint point of view: I don't think so, but, in case, it seems to me that cp_parser could be improved in many ways - even by just reordering the members or using bitfields? Thanks! Paolo. PS: if we want to do something super-safe about the regressions, we still have the Plan B in Comment #5 of c++/71832. /// /cp 2018-03-06 PR c++/71169 PR c++/71832 * parser.h (struct cp_parser): Add error_in_template_parameter_list_p data member. * parser.c (cp_debug_parser): Print the latter. (cp_parser_new): Initialize it. (cp_parser_lambda_expression): Save and restore it. (cp_parser_lambda_declarator_opt): Maybe reset it. (cp_parser_template_parameter_list): Set it in case of error. (cp_parser_explicit_specialization): Maybe reset it. (cp_parser_class_specifier_1): Save and restore it; if it's true do not parse the bodies of the queued function definitions. (cp_parser_class_head): Maybe reset it. (cp_parser_constructor_declarator_p): Save and restore it. (cp_parser_function_definition_after_declarator): Likewise. (cp_parser_template_declaration_after_parameters): Maybe reset it. (finish_fully_implicit_template): Likewise. /testsuite 2018-03-06 PR c++/71169 PR c++/71832 * g++.dg/cpp0x/pr71169.C: New. * g++.dg/cpp0x/pr71832.C: Likewise. Index: cp/parser.c === --- cp/parser.c (revision 258264) +++ cp/parser.c (working copy) @@ -565,6 +565,8 @@ cp_debug_parser (FILE *file, cp_parser *parser) "local class", parser->in_function_body); cp_debug_print_flag (file, "Auto correct a colon to a scope operator", parser->colon_corrects_to_scope_p); + cp_debug_print_flag (file, "Error in template parameter list", + parser->error_in_template_parameter_list_p); cp_debug_print_flag (file, "Colon doesn't start a class definition", parser->colon_doesnt_start_class_def_p); if (parser->type_definition_forbidden_message) @@ -3910,6 +3912,9 @@ cp_parser_new (void) /* We can correct until told otherwise. */ parser->colon_corrects_to_scope_p = true; + /* No error so far. */ + parser->error_in_template_parameter_list_p = false; + /* The unparsed function queue is empty. */ push_unparsed_function_queues (parser); @@ -10151,6 +10156,8 @@ cp_parser_lambda_expression (cp_parser* parser) /* Inside the class, surrounding template-parameter-lists do not apply. */ unsigned int saved_num_template_parameter_lists = parser->num_template_parameter_lists; +bool saved_error_in_template_parameter_list_p += parser->error_in_template_parameter_list_p; unsigned char in_statement = parser->in_statement; bool in_switch_statement_p = parser->in_switch_statement_p; bool fully_implicit_function_template_p @@ -10161,6 +10168,7 @@ cp_parser_lambda_expression (cp_parser* parser) = parser->auto_is_implicit_function_template_parm_p; parser->num_template_parameter_lists = 0; +parser->error_in_template_parameter_list_p = false; parser->in_statement = 0; parser->in_switch_statement_p = false; parser->fully_implicit_function_template_p =
C++ PATCH for c++/84684, wrong caching when evaluating a constexpr function
In this testcase we have a constexpr function, value_to_char_helper. Its body
is
for (size_t i = 0u; i < alphabet_t::value_size; ++i)
value_to_char[i] = to_char(alphabet.assign_rank(i));
which is genericized to a LOOP_EXPR looking roughly like this:
while (1)
{
if (i > 3)
goto out;
value_to_char[i] = to_char (..., i, ...);
i++;
}
Then this is what happens when evaluating the above:
1) cxx_eval_call_expression evaluates the first call to to_char
it's not in the hash table -> copy the body and the bindings and
then evaluate the body
the result is 65
bindings: alph -> {._value=0} (correct)
save all this to the table
2) cxx_eval_store_expression evaluates i++
then it replaces the value in the hash map:
*valp = init;
which is generally what we want, but that also overwrites {._value=0} from
above to {._value=1} which causes problem in 3)
3) cxx_eval_call_expression tries to evaluate the second call to to_char
bindings: alph -> {._value=1} (correct)
here if the hashes match (use [1] hunk for better testing) then
we find to_char call in the table. Then we invoke
constexpr_call_hasher::equal() to compare the two calls: fundefs
match, and because 2) has overwritten bindings of the previous
to_char call, they match too, both are {._value=1}.
That means we don't evaluate this second call and just use the cached
result (65), and that is wrong.
I think the fix is to avoid sharing the constructor in the bindings which
is what the patch does and what we do in several places already. I think
Jakub's patch
wouldn't work if the bindings had more constructors.
But I'm also wondering about massage_init_elt. It has
tree t = fold_non_dependent_expr (init);
t = maybe_constant_init (t);
but given that fold_non_dependent_expr now calls maybe_constant_value, which
then causes that we try to cache the calls above, this seems excessive,
wouldn't we be better off with just calling fold_non_dependent_init as
discussed recently?
I bootstrapped/regtested this patch with this hunk, too, for better testing:
[1]
@@ -1598,8 +1598,12 @@ cxx_eval_call_expression (const constexpr_ctx *ctx, tree
t,
constexpr_call *entry = NULL;
if (depth_ok && !non_constant_args && ctx->strict)
{
+#if 0
new_call.hash = iterative_hash_template_arg
(new_call.bindings, constexpr_fundef_hasher::hash (new_call.fundef));
+#else
+ new_call.hash = 0;
+#endif
/* If we have seen this call before, we are done. */
maybe_initialize_constexpr_call_table ();
Bootstrapped/regtested on x86_64-linux, ok for trunk? And likely 7.
2018-03-06 Marek Polacek
PR c++/84684
* constexpr.c (cxx_bind_parameters_in_call): Unshare evaluated
arguments.
* g++.dg/cpp1z/constexpr-84684.C: New test.
diff --git gcc/cp/constexpr.c gcc/cp/constexpr.c
index 941562ebb05..15a950187ea 100644
--- gcc/cp/constexpr.c
+++ gcc/cp/constexpr.c
@@ -1313,6 +1313,8 @@ cxx_bind_parameters_in_call (const constexpr_ctx *ctx,
tree t,
if (!*non_constant_p)
{
+ /* Don't share a CONSTRUCTOR that might be changed. */
+ arg = unshare_constructor (arg);
/* Make sure the binding has the same type as the parm. But
only for constant args. */
if (TREE_CODE (type) != REFERENCE_TYPE)
diff --git gcc/testsuite/g++.dg/cpp1z/constexpr-84684.C
gcc/testsuite/g++.dg/cpp1z/constexpr-84684.C
index e69de29bb2d..0e7912d4067 100644
--- gcc/testsuite/g++.dg/cpp1z/constexpr-84684.C
+++ gcc/testsuite/g++.dg/cpp1z/constexpr-84684.C
@@ -0,0 +1,163 @@
+// PR c++/84684
+// { dg-options -std=c++17 }
+
+typedef decltype (sizeof (0)) size_t;
+
+namespace std {
+ template
+ struct initializer_list
+ {
+typedef _E value_type;
+typedef const _E& reference;
+typedef const _E& const_reference;
+typedef size_t size_type;
+typedef const _E* iterator;
+typedef const _E* const_iterator;
+iterator _M_array;
+size_type _M_len;
+constexpr initializer_list(const_iterator __a, size_type __l) :
_M_array(__a), _M_len(__l) { }
+constexpr initializer_list() noexcept : _M_array(0), _M_len(0) { }
+constexpr size_type size() const noexcept { return _M_len; }
+constexpr const_iterator begin() const noexcept { return _M_array; }
+constexpr const_iterator end() const noexcept { return begin() + size(); }
+ };
+}
+
+template
+struct array
+{
+ constexpr E &operator[](size_t n) noexcept { return elems[n]; }
+ constexpr const E &operator[](size_t n) const noexcept { return elems[n]; }
+ constexpr size_t size() const { return N; }
+ E elems[N];
+};
+
+template
+constexpr
+inline T
+max (std::initializer_list i)
+{
+ const T *b = i.begin ();
+ const T *e = i.end ();
+ if (b == e) return *b;
+ const T *r = b;
+ while (++b != e)
+ if (*r < *b)
+r = b;
+ return *r;
+}
+
+template
Re: [patch] Fix PR target/84277
> this is the breakage of SEH on 64-bit Windows introduced by defaulting to > the -freorder-blocks-and-partition optimization. It is fixed by: > 1. Defining ASM_DECLARE_COLD_FUNCTION_NAME & > ASM_DECLARE_COLD_FUNCTION_SIZE, 2. Emitting a nop in one more case for SEH, > 3. Splitting the exception table into hot and cold parts; that's necessary > because the LSDA is referenced directly in the SEH scheme. > > Tested on x86-64/Windows, x86-64/Linux and IA-64/Linux, OK for mainline? Also tested on x86/Windows to verify that it doesn't break anything. Can anyone approve the middle-end bits? The rest effectively affects only the -freorder-blocks-and-partition optimization on x86-64/Windows, which has never worked in C++ or Ada, so I don't think that it can do any harm... -- Eric Botcazou
Re: [RFA][PATCH][PR middle-end/61118] Improve tree CFG accuracy for setjmp/longjmp
Hi, On Tue, 6 Mar 2018, Richard Biener wrote: > > bb1 > > ret = setjmp(buf) > >| \ bb-recv > >|\ > >|ret = setjmp_receiver > >|/ > > normal /---/ > > path/ > >| / > > bb-succ > > > > None of these edges would be abnormal. bb-recv would be the target for > > edges from all calls that might call longjmp(buf). Those edges might need > > to be abnormal. As the above CFG reflects all runtime effects precisely, > > but not which instructions are used to achieve them the expansion to RTL > > will be special. > > Why do you still have the edge from setjmp to the setjmp receiver? Ah, yes, that needs explanation. Without that edge the receiver hangs in the air, so to speak. But all effects that happened before the setjmp invocation also have happened before the second return, so the setjmp_receiver needs to be dominated by the setjmp call, and that requires and CFG edge. A different way of thinking about this is that both "calls" need VDEF/VUSE, and the VUSE of setjmp_receiver needs to be the VDEF of the setjmp, so again that edge needs to be there for ordering reasons. At least that's the obvious way of ordering. Thinking harder might make the edge unnecessary after all: all paths leading to longjmps need to go through a setjmp, so the call->receiver edges are already ordered behind setjmp calls (though not necessarily dominated by them), so the receiver is, and so we might be fine. I'd have to paint some pictures on our board to see how this behaves with multiple reaching setjmps. > In your scheme ret is set twice on the longjmp return path, no? No, it's set once on the first-return path, and once on the second-return path (from longjmp to setjmp_receiver, which sets ret, the set of the setjmp call isn't done on the second-return path). Which is indeed what happens in reality, the return register is set once on first-return and once on second-return. > That is, you have the same issue left as we have with EH returns from a > stmt with a LHS. I don't see that, which problem? > We currently have two outgoing edges from setjmp, one which feeds back > to right before the setjmp call via the abnormal dispatcher (so it looks > like a loop). Jeffs change will make it two outgoing edges to the same > single successor, one dispatched through the abnormal dispatcher (that > also nicely gets around the limitation of only having a single edge > between two blocks...) The crucial thing that needs to happen is that all paths from longjmp to the normal successor of the setjmp call contain an assignment to LHS. The edges out of setjmp aren't the important thing for this, the destination of edges from the dispatcher are (because that's the one targeted by the longjmp calls). And IIUC Jeffs patch makes those edges target something after the LHS-set, and this can't be right. Make the dispatcher set an LHS (and hence have one per setjmp, not one per function) and you're effectively ending up with my proposal above. Ciao, ichael.
Re: [RFC][PR82479] missing popcount builtin detection
On Mon, Mar 5, 2018 at 3:24 PM, Richard Biener
wrote:
> On Thu, Feb 8, 2018 at 1:41 AM, Kugan Vivekanandarajah
> wrote:
>> Hi Richard,
>>
>> On 1 February 2018 at 23:21, Richard Biener
>> wrote:
>>> On Thu, Feb 1, 2018 at 5:07 AM, Kugan Vivekanandarajah
>>> wrote:
Hi Richard,
On 31 January 2018 at 21:39, Richard Biener
wrote:
> On Wed, Jan 31, 2018 at 11:28 AM, Kugan Vivekanandarajah
> wrote:
>> Hi Richard,
>>
>> Thanks for the review.
>> On 25 January 2018 at 20:04, Richard Biener
>> wrote:
>>> On Wed, Jan 24, 2018 at 10:56 PM, Kugan Vivekanandarajah
>>> wrote:
Hi All,
Here is a patch for popcount builtin detection similar to LLVM. I
would like to queue this for review for next stage 1.
1. This is done part of loop-distribution and effective for -O3 and
above.
2. This does not distribute loop to detect popcount (like
memcpy/memmove). I dont think that happens in practice. Please correct
me if I am wrong.
>>>
>>> But then it has no business inside loop distribution but instead is
>>> doing final value
>>> replacement, right? You are pattern-matching the whole loop after all.
>>> I think
>>> final value replacement would already do the correct thing if you
>>> teached number of
>>> iteration analysis that niter for
>>>
>>>[local count: 955630224]:
>>> # b_11 = PHI
>>> _1 = b_11 + -1;
>>> b_8 = _1 & b_11;
>>> if (b_8 != 0)
>>> goto ; [89.00%]
>>> else
>>> goto ; [11.00%]
>>>
>>>[local count: 850510900]:
>>> goto ; [100.00%]
>>
>> I am looking into this approach. What should be the scalar evolution
>> for b_8 (i.e. b & (b -1) in a loop) should be? This is not clear to me
>> and can this be represented with the scev?
>
> No, it's not affine and thus cannot be represented. You only need the
> scalar evolution of the counting IV which is already handled and
> the number of iteration analysis needs to handle the above IV - this
> is the missing part.
Thanks for the clarification. I am now matching this loop pattern in
number_of_iterations_exit when number_of_iterations_exit_assumptions
fails. If the pattern matches, I am inserting the _builtin_popcount in
the loop preheater and setting the loop niter with this. This will be
used by the final value replacement. Is this what you wanted?
>>>
>>> No, you shouldn't insert a popcount stmt but instead the niter
>>> GENERIC tree should be a CALL_EXPR to popcount with the
>>> appropriate argument.
>>
>> Thats what I tried earlier but ran into some ICEs. I wasn't sure if
>> niter in tree_niter_desc can take such.
>>
>> Attached patch now does this. Also had to add support for CALL_EXPR in
>> few places to handle niter with CALL_EXPR. Does this look OK?
>
> Overall this looks ok - the patch includes changes in places that I don't
> think
> need changes such as chrec_convert_1 or extract_ops_from_tree.
> The expression_expensive_p change should be more specific than making
> all calls inexpensive as well.
>
> The verify_ssa change looks bogus, you do
>
> + dest = gimple_phi_result (count_phi);
> + tree var = make_ssa_name (TREE_TYPE (dest), NULL);
> + tree fn = builtin_decl_implicit (BUILT_IN_POPCOUNT);
> +
> + var = build_call_expr (fn, 1, src);
> + *niter = fold_build2 (MINUS_EXPR, TREE_TYPE (dest), var,
> + build_int_cst (TREE_TYPE (dest), 1));
>
> why do you allocate a new SSA name here? It seems unused
> as you overwrive 'var' with the CALL_EXPR immediately.
>
> I didn't review the pattern matching thoroughly nor the exact place you
> call it. But
>
> + if (check_popcount_pattern (loop, &count))
> + {
> + niter->assumptions = boolean_false_node;
> + niter->control.base = NULL_TREE;
> + niter->control.step = NULL_TREE;
> + niter->control.no_overflow = false;
> + niter->niter = count;
> + niter->assumptions = boolean_true_node;
> + niter->may_be_zero = boolean_false_node;
> + niter->max = -1;
> + niter->bound = NULL_TREE;
> + niter->cmp = ERROR_MARK;
> + return true;
> + }
>
> simply setting may_be_zero to false looks fishy. Try
> with -fno-tree-loop-ch. Also max should not be negative,
> it should be the number of bits in the IV type?
>
> A related testcase could be that we can completely peel
> a loop like the following which iterates at most 8 times:
>
> int a[8];
> void foo (unsigned char ctrl)
> {
> int c = 0;
> while (ctrl)
> {
>ctrl = ctrl & (ctrl - 1);
>a[c++] = ctrl;
> }
> }
>
> This is now stage1 material so please update and re-post. Maybe Bin has
> further suggestions as well.
Sorry for being late on this. Actually I thought about popcount in
distribution before. More l
Patch ping (Re: [PATCH] Fix aarch64_simd_reg_or_zero predicate (PR fortran/84565))
Hi!
I'd like to ping this patch, without or with the additional redundant
(match_test "op == const0_rtx")
line removal.
> Bootstrapped/regtested on aarch64-linux (scratch Fedora gcc 8 package build
> with the patch applied), ok for trunk?
>
> 2018-02-27 Jakub Jelinek
>
> PR fortran/84565
> * config/aarch64/predicates.md (aarch64_simd_reg_or_zero): Use
> aarch64_simd_or_scalar_imm_zero rather than aarch64_simd_imm_zero.
>
> * gfortran.dg/pr84565.f90: New test.
>
> --- gcc/config/aarch64/predicates.md.jj 2018-02-06 13:13:06.305751221
> +0100
> +++ gcc/config/aarch64/predicates.md 2018-02-26 16:30:01.902195208 +0100
> @@ -395,7 +395,7 @@ (define_predicate "aarch64_simd_reg_or_z
>(and (match_code "reg,subreg,const_int,const_double,const,const_vector")
> (ior (match_operand 0 "register_operand")
> (match_test "op == const0_rtx")
> - (match_operand 0 "aarch64_simd_imm_zero"
> + (match_operand 0 "aarch64_simd_or_scalar_imm_zero"
>
> (define_predicate "aarch64_simd_struct_operand"
>(and (match_code "mem")
> --- gcc/testsuite/gfortran.dg/pr84565.f90.jj 2018-02-26 16:32:49.912271950
> +0100
> +++ gcc/testsuite/gfortran.dg/pr84565.f90 2018-02-26 16:31:15.423223943
> +0100
> @@ -0,0 +1,7 @@
> +! PR fortran/84565
> +! { dg-do compile { target aarch64*-*-* } }
> +! { dg-options "-mlow-precision-sqrt -funsafe-math-optimizations" }
> +subroutine mysqrt(a)
> + real(KIND=KIND(0.0D0)) :: a
> + a=sqrt(a)
> +end subroutine
Jakub
Re: [PATCH, rs6000] Fix PR84264: ICE in rs6000_emit_le_vsx_store
On 3/5/18 8:55 AM, Segher Boessenkool wrote: > On Sat, Mar 03, 2018 at 10:55:28PM -0600, Peter Bergner wrote: >> The simple fix here is to just verify the memory_operand is not an altivec >> mem operand before calling rs6000_emit_le_vsx_move. >> >> This passed bootstrap and regtesting on powerpc64le-linux with no >> regressions. Ok for trunk? > > Yes this looks correct, okay for trunk. Thanks. But it is very > non-obvious; maybe a comment will help, or the code can be restructured > a bit? As we discussed offline, I committed the patch with an added comment. Thanks. Peter
Re: patch to fix PR81572
On 2/22/18 3:19 PM, Vladimir Makarov wrote: > The following patch fixes > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81572 > > The patch was successfully bootstrapped and tested on ppc64. Vlad approved the backporting of this patch to GCC 7. I backported his patch and bootstrap & regtesting showed no regressions. Committed to the GCC 7 branch. Peter
Re: [PR c++/84593] ice on braced init with uninit ref field
On Tue, Mar 6, 2018 at 12:42 AM, Alexandre Oliva wrote: > On Mar 2, 2018, Jason Merrill wrote: > >> On Fri, Mar 2, 2018 at 2:57 AM, Alexandre Oliva wrote: >>> + gcc_assert (TREE_CODE (type) == REFERENCE_TYPE); >>> + init = fold (convert (type, integer_zero_node)); > >> Maybe build_zero_cst? > > Sure. > > > I wonder, is there any reason to not change any of these to use > build_zero_cst, too? > > else if (TYPE_PTR_OR_PTRMEM_P (type)) > init = fold (convert (type, nullptr_node)); > else if (SCALAR_TYPE_P (type)) > init = fold (convert (type, integer_zero_node)); > > I suppose pointers to members might need an explicit conversion, which > build_zero_cst might fallback to if it doesn't consider them aggregate > types. As for scalar types, are there any C++-specific scalar types > that build_zero_cst wouldn't know how to deal with? Anyway, it's > probably not the time to change these, if it doesn't fix a regression. > Just curious. Indeed, build_zero_cst is wrong for pointers to members, but it should be right for other scalars, including regular pointers. Jason
Re: [RFA][PATCH][PR middle-end/61118] Improve tree CFG accuracy for setjmp/longjmp
On Tue, Mar 6, 2018 at 3:17 PM, Michael Matz wrote:
> Hi,
>
> On Mon, 5 Mar 2018, Jeff Law wrote:
>
>> >>> Actually, without further conditions I don't see how it would be safe
>> >>> for the successor to have multiple preds. We might have this
>> >>> situation:
>> >>>
>> >>> bb1: ret = setjmp
>> >>> bb2: x0 = phi
>> >> No. Can't happen -- we're still building the initial CFG. There are no
>> >> PHI nodes, there are no SSA_NAMEs.
>> >
>> > While that is currently true I think it's short-sighted. Thinking about
>> > the situation in terms of SSA names and PHI nodes clears up the mind. In
>> > addition there is already code which builds (sub-)cfgs when SSA form
>> > exists (gimple_find_sub_bbs). Currently that code can't ever generate
>> > setjmp calls, so it's not an issue.
>>
>> It's not clearing up anything for me. Clearly you're onto something
>> that I'm missing, but still trying to figure out.
>
> I'm saying that if there is SSA form then having the second-return edge to
> the successor block if that has multiple predecessors, then this would be
> incorrect. Do you agree? I'm also saying that if it's incorrect when in
> SSA form, then it can't be correct (perhaps only very subtly so) without
> SSA form either; I guess that's where we don't agree.
>
> I'm also saying that we currently don't have SSA form while dealing with
> setjmp by more luck and giving up than thought: inlining and va-arg
> expansion do construct sub-cfgs while in SSA. inlining setjmp calls is
> simply disabled, va-arg expansion doesn't emit setjmp calls into the
> sequence. But there is nothing inherently magic about SSA form and
> setjmp, so if we're going to fiddle with the CFG form created by setjmp to
> make it more precise, then I think we ought to do it in a way that is
> correct in all intermediate forms, especially at this devel stage, because
> that'd give confidence that it's also correct in the constrained way we're
> needing it.
>
>> Certainly we have to be careful WRT the implicit set of the return value
>> of the setjmp call that occurs on the longjmp path. That's worth
>> investigating. I suspect that works today more by accident of having an
>> incorrect CFG than by design.
>
> No, our current imprecise CFG makes it so that the setting of the return
> value is reached by the abnormal edge from the dispatcher, so in a
> data-flow sense it's executed on the first- and second-return case and all
> is well, and so the IL subsumes all the effects that happen in reality
> (and more, which is the reason for the missed optimizations). You want to
> change the CFG such that it doesn't subsume effects that don't happen in
> reality, and that makes sense, but by that you're making it not reflect
> actions which _do_ happen.
>
>> But it does or at least it should. It's implicitly set on the longjmp
>> side. If we get this wrong I'd expect we'll see uninit uses in the PHI.
>> That's easy enough to instrument and check for.
>
> Not only uninit uses but conceivably misoptimizations as well. Let's
> contrieve an example which uses gotos to demonstrate the abnormal edges
> and let's have the second-return edge to land after the setjmp call (and
> setting of return value):
>
> ret = setjmp(buf);
> second_return:
> if (ret == 0) { // first return
> inside: // see below
> call foo(buf) // might call longjmp
> maybe-goto dispatch // therefore we have an edge to dispatch
> }
> return;
> dispatch: // our abnormal dispatcher
> goto second_return; // go to second-return target
>
> Now, as far as data-flow is concerned, as soon as the ret==0 block is
> entered once, ret will never change from 0 again (there simply is no
> visible set which could change it). So an optimizer would be correct in
> threading the sequence maybe-goto-dispatch->second_return to inside (bear
> with me):
>
> ret = setjmp();
> if (ret == 0) { // first return
> inside: // see below
> call foo()// might call longjmp
> maybe-goto inside // if it does we loop, ret must still be zero ?!
> }
> return;
>
> This currently would not happen: first we don't thread through abnormal
> edges, and propagation of knowledge over abnormal edges is artificially
> limited as well. But from a pure data-flow perspective the above
> transformation would be correct, meaning that initial IL can't have been
> correct. You might say in practice even if the above would happen it
> doesn't matter, because at runtime, with a real longjmp, the "maybe-goto
> inside" would in reality land at the setjmp return again, hence setting
> ret, then the test comes along and all is well. That would probably be
> true currently, but assume foo always calls longjmp, and GCC can figure
> out it's using the same buffer as the above setjmp, then a further
> optimization might do away with the longjmp and just jump directly
Re: [PATCH] jit: fix link on OS X and Solaris (PR jit/64089 and PR jit/84288)
Hi David, Any news on that patch? Cheers, FX
Re: [RFA][PATCH][PR middle-end/61118] Improve tree CFG accuracy for setjmp/longjmp
Hi,
On Mon, 5 Mar 2018, Jeff Law wrote:
> >>> Actually, without further conditions I don't see how it would be safe
> >>> for the successor to have multiple preds. We might have this
> >>> situation:
> >>>
> >>> bb1: ret = setjmp
> >>> bb2: x0 = phi
> >> No. Can't happen -- we're still building the initial CFG. There are no
> >> PHI nodes, there are no SSA_NAMEs.
> >
> > While that is currently true I think it's short-sighted. Thinking about
> > the situation in terms of SSA names and PHI nodes clears up the mind. In
> > addition there is already code which builds (sub-)cfgs when SSA form
> > exists (gimple_find_sub_bbs). Currently that code can't ever generate
> > setjmp calls, so it's not an issue.
>
> It's not clearing up anything for me. Clearly you're onto something
> that I'm missing, but still trying to figure out.
I'm saying that if there is SSA form then having the second-return edge to
the successor block if that has multiple predecessors, then this would be
incorrect. Do you agree? I'm also saying that if it's incorrect when in
SSA form, then it can't be correct (perhaps only very subtly so) without
SSA form either; I guess that's where we don't agree.
I'm also saying that we currently don't have SSA form while dealing with
setjmp by more luck and giving up than thought: inlining and va-arg
expansion do construct sub-cfgs while in SSA. inlining setjmp calls is
simply disabled, va-arg expansion doesn't emit setjmp calls into the
sequence. But there is nothing inherently magic about SSA form and
setjmp, so if we're going to fiddle with the CFG form created by setjmp to
make it more precise, then I think we ought to do it in a way that is
correct in all intermediate forms, especially at this devel stage, because
that'd give confidence that it's also correct in the constrained way we're
needing it.
> Certainly we have to be careful WRT the implicit set of the return value
> of the setjmp call that occurs on the longjmp path. That's worth
> investigating. I suspect that works today more by accident of having an
> incorrect CFG than by design.
No, our current imprecise CFG makes it so that the setting of the return
value is reached by the abnormal edge from the dispatcher, so in a
data-flow sense it's executed on the first- and second-return case and all
is well, and so the IL subsumes all the effects that happen in reality
(and more, which is the reason for the missed optimizations). You want to
change the CFG such that it doesn't subsume effects that don't happen in
reality, and that makes sense, but by that you're making it not reflect
actions which _do_ happen.
> But it does or at least it should. It's implicitly set on the longjmp
> side. If we get this wrong I'd expect we'll see uninit uses in the PHI.
> That's easy enough to instrument and check for.
Not only uninit uses but conceivably misoptimizations as well. Let's
contrieve an example which uses gotos to demonstrate the abnormal edges
and let's have the second-return edge to land after the setjmp call (and
setting of return value):
ret = setjmp(buf);
second_return:
if (ret == 0) { // first return
inside: // see below
call foo(buf) // might call longjmp
maybe-goto dispatch // therefore we have an edge to dispatch
}
return;
dispatch: // our abnormal dispatcher
goto second_return; // go to second-return target
Now, as far as data-flow is concerned, as soon as the ret==0 block is
entered once, ret will never change from 0 again (there simply is no
visible set which could change it). So an optimizer would be correct in
threading the sequence maybe-goto-dispatch->second_return to inside (bear
with me):
ret = setjmp();
if (ret == 0) { // first return
inside: // see below
call foo()// might call longjmp
maybe-goto inside // if it does we loop, ret must still be zero ?!
}
return;
This currently would not happen: first we don't thread through abnormal
edges, and propagation of knowledge over abnormal edges is artificially
limited as well. But from a pure data-flow perspective the above
transformation would be correct, meaning that initial IL can't have been
correct. You might say in practice even if the above would happen it
doesn't matter, because at runtime, with a real longjmp, the "maybe-goto
inside" would in reality land at the setjmp return again, hence setting
ret, then the test comes along and all is well. That would probably be
true currently, but assume foo always calls longjmp, and GCC can figure
out it's using the same buffer as the above setjmp, then a further
optimization might do away with the longjmp and just jump directly to the
inside label: misoptimization achieved.
None of the above events can happen when the second-return target is
imprecise and before the setjmp.
Now that all m
[Ada] Fix incomplete narrowing FP conversion for Machine_Overflows
On platforms where System'Machine_Overflows is true, the compiler generates a check only for the lower bound of the output type in a narrowing floating- point conversion, which means that e.g. the last chance handler will not be invoked if the value overflows the upper bound of the output type. Applied on all active branches. 2018-03-06 Eric Botcazou * gcc-interface/trans.c (convert_with_check): Fix typo in the condition guarding the overflow check emitted for the upper bound of a floating- point conversion. -- Eric BotcazouIndex: gcc-interface/trans.c === --- gcc-interface/trans.c (revision 258231) +++ gcc-interface/trans.c (working copy) @@ -9381,7 +9381,7 @@ convert_with_check (Entity_Id gnat_type, ? tree_int_cst_lt (gnu_out_ub, gnu_in_ub) : (FLOAT_TYPE_P (gnu_base_type) ? real_less (&TREE_REAL_CST (gnu_out_ub), - &TREE_REAL_CST (gnu_in_lb)) + &TREE_REAL_CST (gnu_in_ub)) : 1)) gnu_cond = build_binary_op (TRUTH_ORIF_EXPR, boolean_type_node, gnu_cond,
[Ada] Fix build on x86/Windows
It has been broken on the mainline since end of December. The attached
patchlet is copy-and-pasted from the same function in the c-family dir.
Tested on x86/Windows and x86-64/Linux, applied on the mainline.
2018-03-06 Eric Botcazou
* gcc-interface/utils.c (def_builtin_1): Bail out on error_mark_node.
--
Eric BotcazouIndex: gcc-interface/utils.c
===
--- gcc-interface/utils.c (revision 258231)
+++ gcc-interface/utils.c (working copy)
@@ -6443,6 +6443,9 @@ def_builtin_1 (enum built_in_function fn
if (builtin_decl_explicit (fncode))
return;
+ if (fntype == error_mark_node)
+return;
+
gcc_assert ((!both_p && !fallback_p)
|| !strncmp (name, "__builtin_",
strlen ("__builtin_")));
RE: [PATCH] [ARC] Cleanup unused functions.
Pushed Thank you for your review. Claudiu
Re: [PATCH] Fix C++ ref op= ICE in the gimplifier with -g (PR c++/84704)
On Tue, Mar 06, 2018 at 08:56:14AM +0100, Richard Biener wrote: > > Maybe a better fix if we run into this would be to make create_tmp_var_raw > > create DECL_NAMELESS decls and make sure we don't print names of > > DECL_NAMELESS variables in -fdump-final-insns= dumps? > > They are already DECL_IGNORED_P, that should include the effects of > DECL_NAMELESS if I read both documentations in tree.h correctly. For debug info sure, but DECL_IGNORED_P names are still printed in the final dumps (like they are printed in all other dumps), which is what -fcompare-debug compares. > > I mean the fancy names are causing us trouble all the time, e.g. the SRA > > fancy names that later get concatenated to other fancy names and sometimes > > contains uids... > > Yeah... so lets not dump names of DECL_IGNORED_P | DECL_NAMELESS decls. I'll test a patch doing so then (the stabilize_expr change is still needed though even if this is changed). Jakub
Re: [RFA][PATCH][PR middle-end/61118] Improve tree CFG accuracy for setjmp/longjmp
On Tue, Mar 6, 2018 at 4:41 AM, Jeff Law wrote: > On 03/05/2018 12:30 PM, Michael Matz wrote: >> Hi, >> >> On Mon, 5 Mar 2018, Jeff Law wrote: >> > The single successor test was strictly my paranoia WRT abnormal/EH > edges. > > I don't immediately see why the CFG would be incorrect if the > successor of the setjmp block has multiple preds. Actually, without further conditions I don't see how it would be safe for the successor to have multiple preds. We might have this situation: bb1: ret = setjmp bb2: x0 = phi >>> No. Can't happen -- we're still building the initial CFG. There are no >>> PHI nodes, there are no SSA_NAMEs. >> >> While that is currently true I think it's short-sighted. Thinking about >> the situation in terms of SSA names and PHI nodes clears up the mind. In >> addition there is already code which builds (sub-)cfgs when SSA form >> exists (gimple_find_sub_bbs). Currently that code can't ever generate >> setjmp calls, so it's not an issue. > It's not clearing up anything for me. Clearly you're onto something > that I'm missing, but still trying to figure out. > > Certainly we have to be careful WRT the implicit set of the return value > of the setjmp call that occurs on the longjmp path. That's worth > investigating. I suspect that works today more by accident of having an > incorrect CFG than by design. > > >> >>> We have two choices we can either target the setjmp itself, which is >>> what we've been doing and is inaccurate. Or we can target the point >>> immediately after the setjmp, which is accurate. >> >> Not precisely, because the setting of the return value of setjmp does >> happen after both returns. So moving the whole second-return edge target >> to after the setjmp call (when it includes an LHS) is not correct >> (irrespective how the situation in the successor BBs like like). > But it does or at least it should. It's implicitly set on the longjmp > side. If we get this wrong I'd expect we'll see uninit uses in the PHI. > That's easy enough to instrument and check for. > > This aspect of setjmp/longjmp is, in some ways, easier to see in RTL > because the call returns its value in a hard reg which is implicitly set > by the longjmp and we immediately copy it into a pseudo. Which would > magically DTRT if we had the longjmp edge target the point just after > the setjmp in RTL. While it's true that the hardreg is set by the callee the GIMPLE IL indeed doesn't reflect this (and we have a similar issue with EH where the exceptional return does _not_ include the assignment to the LHS but the GIMPLE IL does...). So with your patch we should see ret_1 = setjmp (); | \ | AB dispatcher |/ v v # ret_2 = PHI ... even w/o a PHI. So I think we should be fine given we have that edge from setjmp to the abnormal dispatcher. Richard. > > > > Jeff
Re: [PATCH] Fix reg-stack error-recovery ICE (PR inline-asm/84683)
On Mon, Mar 5, 2018 at 9:42 PM, Jakub Jelinek wrote:
> Hi!
>
> If we discover some bad inline-asm during reg-stack processing and we
> error on those, we replace that inline-asm with a (use (const_int 0))
> and therefore the various assumptions of reg-stack pass may not hold.
> Seems we already have a couple of spots which are more permissive if
> any_malformed_asm is true, this patch just adds another one.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
>
> 2018-03-05 Jakub Jelinek
>
> PR inline-asm/84683
> * reg-stack.c (move_for_stack_reg): If any_malformed_asm, avoid
> assertion failure.
>
> * g++.dg/ext/pr84683.C: New test.
LGTM.
Uros.
> --- gcc/reg-stack.c.jj 2018-01-03 10:19:55.0 +0100
> +++ gcc/reg-stack.c 2018-03-05 17:41:15.558415480 +0100
> @@ -1170,7 +1170,8 @@ move_for_stack_reg (rtx_insn *insn, stac
> && XINT (SET_SRC (XVECEXP (pat, 0, 1)), 1) == UNSPEC_TAN)
> emit_swap_insn (insn, regstack, dest);
>else
> - gcc_assert (get_hard_regnum (regstack, dest) < FIRST_STACK_REG);
> + gcc_assert (get_hard_regnum (regstack, dest) < FIRST_STACK_REG
> + || any_malformed_asm);
>
>gcc_assert (regstack->top < REG_STACK_SIZE);
>
> --- gcc/testsuite/g++.dg/ext/pr84683.C.jj 2018-03-05 17:45:32.901475529
> +0100
> +++ gcc/testsuite/g++.dg/ext/pr84683.C 2018-03-05 17:44:52.527467872 +0100
> @@ -0,0 +1,13 @@
> +// PR inline-asm/84683
> +// { dg-do compile { target i?86-*-* x86_64-*-* } }
> +// { dg-options "-O2" }
> +
> +void
> +foo (float b, double c)
> +{
> + for (int e = 0; e < 2; e++)
> +{
> + asm volatile ("" : "+f" (c));// { dg-error "must specify a single
> register" }
> + asm ("" : "+rm" (c = b));
> +}
> +}
>
> Jakub
