C++ PATCH for c++/91416 - GC during late parsing collects live data
This is a crash that points to a GC problem. Consider this test:
__attribute__ ((unused)) struct S {
S() { }
} s;
We're parsing a simple-declaration. While parsing the decl specs, we parse the
attribute, which means creating a TREE_LIST using ggc_alloc_*.
A function body is a complete-class context so when parsing the
member-specification of this class-specifier, we parse the bodies of the
functions we'd queued in cp_parser_late_parsing_for_member. This then leads to
this call chain:
cp_parser_function_definition_after_declarator -> expand_or_defer_fn ->
expand_or_defer_fn_1 -> maybe_clone_body -> expand_or_defer_fn ->
cgraph_node::finalize_function -> ggc_collect.
In this test, the ggc_collect call collects the TREE_LIST we had allocated, and
a crash duly ensues. We can't avoid late parsing of members in this context,
so my fix is to bump function_depth, exactly following cp_parser_lambda_body.
Since we are performing late parsing, we know we have to be nested in a class.
(We still ggc_collect later, in c_parse_final_cleanups.)
But here's the thing. This goes back to ancient r104500, at least. How has
this not broken before? All you need to trigger it is to enable GC checking
and have a class with a ctor/member function, that has an attribute. You'd
think that since we've got hundreds of those in the testsuite, at least one of
them would trigger this crash. Or that there'd be several reports about this in
the BZ already. Yet I didn't find any. Truly, I'm perplexed.
Anyway, bootstrapped/regtested on x86_64-linux, ok for trunk? How about 9.3?
I vote yes.
2019-08-11 Marek Polacek
PR c++/91416 - GC during late parsing collects live data.
* parser.c (cp_parser_late_parsing_for_member): Increment function_depth
around call to cp_parser_function_definition_after_declarator.
* g++.dg/other/gc6.C: New test.
diff --git gcc/cp/parser.c gcc/cp/parser.c
index b56cc6924f4..0d4d32e9670 100644
--- gcc/cp/parser.c
+++ gcc/cp/parser.c
@@ -28934,6 +28934,8 @@ cp_parser_late_parsing_for_member (cp_parser* parser,
tree member_function)
function_scope = current_function_decl;
if (function_scope)
push_function_context ();
+ else
+ ++function_depth;
/* Push the body of the function onto the lexer stack. */
cp_parser_push_lexer_for_tokens (parser, tokens);
@@ -28966,6 +28968,9 @@ cp_parser_late_parsing_for_member (cp_parser* parser,
tree member_function)
/* Leave the scope of the containing function. */
if (function_scope)
pop_function_context ();
+ else
+ --function_depth;
+
cp_parser_pop_lexer (parser);
}
diff --git gcc/testsuite/g++.dg/other/gc6.C gcc/testsuite/g++.dg/other/gc6.C
new file mode 100644
index 000..385be5c945e
--- /dev/null
+++ gcc/testsuite/g++.dg/other/gc6.C
@@ -0,0 +1,7 @@
+// PR c++/91416 - GC during late parsing collects live data.
+// { dg-do compile }
+// { dg-options "--param ggc-min-heapsize=0 --param ggc-min-expand=0" }
+
+__attribute__ ((unused)) struct S {
+ S() { }
+} s;
Re: Fix Ada comparison failure on SPARC
On August 11, 2019 12:20:31 PM GMT+02:00, Jakub Jelinek
wrote:
>On Sun, Aug 11, 2019 at 12:17:06PM +0200, Rainer Orth wrote:
>> 2019-08-07 Richard Sandiford
>>
>> * data-streamer.h (streamer_write_poly_uint64): Declare.
>> (streamer_read_poly_uint64): Likewise.
>> * data-streamer-in.c (streamer_read_poly_uint64): New function.
>> * data-streamer-out.c (streamer_write_poly_uint64): Likewise.
>> * ipa-predicate.h (condition::size): Turn into a poly_int64.
>> (add_condition): Take a poly_int64 size.
>> * ipa-predicate.c (add_condition): Likewise.
>> [...]
>>
>> Looking through it, I noticed this snippet
>>
>> diff --git a/gcc/ipa-predicate.c b/gcc/ipa-predicate.c
>> --- a/gcc/ipa-predicate.c
>> +++ b/gcc/ipa-predicate.c
>> @@ -549,7 +549,7 @@ add_condition (class ipa_fn_summary *sum
>>for (i = 0; vec_safe_iterate (summary->conds, i, ); i++)
>> {
>>if (c->operand_num == operand_num
>> - && c->size == size
>> + && maybe_ne (c->size, size)
>>
>>
>> where changing == to maybe_ne didn't seem right. And indeed changing
>it
>> to maybe_eq as in the following patch fixed the comparison failure.
>
>Shouldn't that be known_eq instead? Of course, it could make a
>difference
>right now solely on aarch64 SVE.
OK with using known_eq.
Richard.
> Jakub
Re: Fix Ada comparison failure on SPARC
On Sun, Aug 11, 2019 at 12:17:06PM +0200, Rainer Orth wrote:
> 2019-08-07 Richard Sandiford
>
> * data-streamer.h (streamer_write_poly_uint64): Declare.
> (streamer_read_poly_uint64): Likewise.
> * data-streamer-in.c (streamer_read_poly_uint64): New function.
> * data-streamer-out.c (streamer_write_poly_uint64): Likewise.
> * ipa-predicate.h (condition::size): Turn into a poly_int64.
> (add_condition): Take a poly_int64 size.
> * ipa-predicate.c (add_condition): Likewise.
> [...]
>
> Looking through it, I noticed this snippet
>
> diff --git a/gcc/ipa-predicate.c b/gcc/ipa-predicate.c
> --- a/gcc/ipa-predicate.c
> +++ b/gcc/ipa-predicate.c
> @@ -549,7 +549,7 @@ add_condition (class ipa_fn_summary *sum
>for (i = 0; vec_safe_iterate (summary->conds, i, ); i++)
> {
>if (c->operand_num == operand_num
> - && c->size == size
> + && maybe_ne (c->size, size)
>
>
> where changing == to maybe_ne didn't seem right. And indeed changing it
> to maybe_eq as in the following patch fixed the comparison failure.
Shouldn't that be known_eq instead? Of course, it could make a difference
right now solely on aarch64 SVE.
Jakub
Fix Ada comparison failure on SPARC
Between 20190806 (r274144) and 20190807 (r274169), Solaris/SPARC
bootstrap with Ada began to fail with a comparion failure:
Bootstrap comparison failure!
gcc/ada/bindo-graphs.o differs
The differences look like this:
-prev-gcc/ada/bindo-graphs.o.stripped: file format elf32-sparc-sol2
+gcc/ada/bindo-graphs.o.stripped: file format elf32-sparc-sol2
@@ -22716 +22716 @@
- 162d0: 81 aa 0a ca fcmped %f8, %f10
+ 162d0: 83 aa 0a ca fcmped %fcc1, %f8, %f10
@@ -22718 +22718 @@
- 162d8: 85 61 20 01 movl %fcc0, 1, %g2
+ 162d8: 85 61 28 01 movl %fcc1, 1, %g2
and a couple (ca. 40) more.
The presence or absence of the failure is quite sensitive to both the
toolchain used (32-bit gas/ld and gas/gld fail, 64-bit gas/ld and 32 and
64-bit as/ld pass) and the exact srcdir.
A reghunt identified this patch as the culprit:
2019-08-07 Richard Sandiford
* data-streamer.h (streamer_write_poly_uint64): Declare.
(streamer_read_poly_uint64): Likewise.
* data-streamer-in.c (streamer_read_poly_uint64): New function.
* data-streamer-out.c (streamer_write_poly_uint64): Likewise.
* ipa-predicate.h (condition::size): Turn into a poly_int64.
(add_condition): Take a poly_int64 size.
* ipa-predicate.c (add_condition): Likewise.
[...]
Looking through it, I noticed this snippet
diff --git a/gcc/ipa-predicate.c b/gcc/ipa-predicate.c
--- a/gcc/ipa-predicate.c
+++ b/gcc/ipa-predicate.c
@@ -549,7 +549,7 @@ add_condition (class ipa_fn_summary *sum
for (i = 0; vec_safe_iterate (summary->conds, i, ); i++)
{
if (c->operand_num == operand_num
- && c->size == size
+ && maybe_ne (c->size, size)
where changing == to maybe_ne didn't seem right. And indeed changing it
to maybe_eq as in the following patch fixed the comparison failure.
Bootstrapped without regressions on sparc-sun-solaris2.11 and
i386-pc-solaris2.11. Ok for mainline?
Rainer
--
-
Rainer Orth, Center for Biotechnology, Bielefeld University
2019-08-11 Rainer Orth
* ipa-predicate.c (add_condition): Fix typo.
# HG changeset patch
# Parent 2ca1e7660c8c156605bb706a911e971840fca6d4
Fix Ada comparison failure on SPARC
diff --git a/gcc/ipa-predicate.c b/gcc/ipa-predicate.c
--- a/gcc/ipa-predicate.c
+++ b/gcc/ipa-predicate.c
@@ -549,7 +549,7 @@ add_condition (class ipa_fn_summary *sum
for (i = 0; vec_safe_iterate (summary->conds, i, ); i++)
{
if (c->operand_num == operand_num
- && maybe_ne (c->size, size)
+ && maybe_eq (c->size, size)
&& c->code == code
&& c->val == val
&& c->agg_contents == agg_contents
Re: [PATCH,fortran] PR 91413 Generate warning when making array static
On Sat, Aug 10, 2019 at 11:57 PM Steve Kargl wrote: > > On Sat, Aug 10, 2019 at 11:34:15PM +0300, Janne Blomqvist wrote: > > When moving a local variable from the stack to static storage, the > > procedure is no longer safe to be called recursively or concurrently > > from multiple threads. Thus generate a warning when this is done. > > Also double the default limit for switching from stack to static. > > > > Regtested on x86_64-pc-linux-gnu, Ok for trunk? > > > > OK. Thanks for the quick review, committed as r274264. To be clear, this patch does not fix the PR, just makes it slightly less likely to happen, and makes some noise when it does happen. To reignite the discussion from https://gcc.gnu.org/ml/fortran/2017-12/msg00082.html , any opinions how this should properly be fixed? I can see two main options: - Use the heap instead of static storage for local variables going over the size limit set by -fmax-stack-var-size= . This is IMHO a bit ugly, going behind the back of the user like this; If the user wants to use the heap, she can use allocatable arrays. Also, it needs to be done somewhere else than in trans-decl.c (gfc_finish_var_decl) as at that point there is no access to the block, and hence one cannot add new code to malloc() and free() the variable. I'm not sure where the proper place for this would be. - Make -frecursive the default for GFC_STD_F2018 (and thus also for -std=gnu), while keeping the existing behavior for older standards. This would follow the wishes of the user, but would risk crashing applications with stack exhaustion. Any opinions which would be preferable, or any other solution to this problem? -- Janne Blomqvist
[PATCH] PR fortran/91414: Improved PRNG
Update the PRNG from xorshift1024* to xoshiro256** by the same
author. For details see
http://prng.di.unimi.it/
and the paper at
https://arxiv.org/abs/1805.01407
Also the seeding is slightly improved, by reading only 8 bytes from
the operating system and using the simple splitmix64 PRNG to fill in
the rest of the PRNG state (as recommended by the xoshiro author),
instead of reading the entire state from the OS.
Regtested on x86_64-pc-linux-gnu, Ok for trunk?
gcc/fortran/ChangeLog:
2019-08-11 Janne Blomqvist
PR fortran/91414
* check.c (gfc_check_random_seed): Reduce seed_size.
* intrinsic.texi (RANDOM_NUMBER): Update to match new PRNG.
gcc/testsuite/ChangeLog:
2019-08-11 Janne Blomqvist
PR fortran/91414
* gfortran.dg/random_seed_1.f90: Update to match new seed size.
libgfortran/ChangeLog:
2019-08-11 Janne Blomqvist
PR fortran/91414
* intrinsics/random.c (prng_state): Update state struct.
(master_state): Update to match new size.
(get_rand_state): Update to match new PRNG.
(rotl): New function.
(xorshift1024star): Replace with prng_next.
(prng_next): New function.
(jump): Update for new PRNG.
(lcg_parkmiller): Replace with splitmix64.
(splitmix64): New function.
(getosrandom): Fix return value, simplify.
(init_rand_state): Use getosrandom only to get 8 bytes, splitmix64
to fill rest of state.
(random_r4): Update to new function and struct names.
(random_r8): Likewise.
(random_r10): Likewise.
(random_r16): Likewise.
(arandom_r4): Liekwise.
(arandom_r8): Likewise.
(arandom_r10): Likwewise.
(arandom_r16): Likewise.
(xor_keys): Reduce size to match new PRNG.
(random_seed_i4): Update to new function and struct names, remove
special handling of variable p used in previous PRNG.
(random_seed_i8): Likewise.
---
gcc/fortran/check.c | 5 +-
gcc/fortran/intrinsic.texi | 10 +-
gcc/testsuite/gfortran.dg/random_seed_1.f90 | 7 +-
libgfortran/intrinsics/random.c | 213 +---
4 files changed, 110 insertions(+), 125 deletions(-)
diff --git a/gcc/fortran/check.c b/gcc/fortran/check.c
index 370a3c819f9..2bd8bc37556 100644
--- a/gcc/fortran/check.c
+++ b/gcc/fortran/check.c
@@ -6484,9 +6484,8 @@ gfc_check_random_seed (gfc_expr *size, gfc_expr *put,
gfc_expr *get)
mpz_t put_size, get_size;
/* Keep the number of bytes in sync with master_state in
- libgfortran/intrinsics/random.c. +1 due to the integer p which is
- part of the state too. */
- seed_size = 128 / gfc_default_integer_kind + 1;
+ libgfortran/intrinsics/random.c. */
+ seed_size = 32 / gfc_default_integer_kind;
if (size != NULL)
{
diff --git a/gcc/fortran/intrinsic.texi b/gcc/fortran/intrinsic.texi
index f390761dc3d..3aa068dba9d 100644
--- a/gcc/fortran/intrinsic.texi
+++ b/gcc/fortran/intrinsic.texi
@@ -11792,10 +11792,10 @@ end program test_random_seed
Returns a single pseudorandom number or an array of pseudorandom numbers
from the uniform distribution over the range @math{ 0 \leq x < 1}.
-The runtime-library implements the xorshift1024* random number
-generator (RNG). This generator has a period of @math{2^{1024} - 1},
-and when using multiple threads up to @math{2^{512}} threads can each
-generate @math{2^{512}} random numbers before any aliasing occurs.
+The runtime-library implements the xoshiro256** pseudorandom number
+generator (PRNG). This generator has a period of @math{2^{256} - 1},
+and when using multiple threads up to @math{2^{128}} threads can each
+generate @math{2^{128}} random numbers before any aliasing occurs.
Note that in a multi-threaded program (e.g. using OpenMP directives),
each thread will have its own random number state. For details of the
@@ -11852,7 +11852,7 @@ called either without arguments or with the @var{PUT}
argument, the
given seed is copied into a master seed as well as the seed of the
current thread. When a new thread uses @code{RANDOM_NUMBER} for the
first time, the seed is copied from the master seed, and forwarded
-@math{N * 2^{512}} steps to guarantee that the random stream does not
+@math{N * 2^{128}} steps to guarantee that the random stream does not
alias any other stream in the system, where @var{N} is the number of
threads that have used @code{RANDOM_NUMBER} so far during the program
execution.
diff --git a/gcc/testsuite/gfortran.dg/random_seed_1.f90
b/gcc/testsuite/gfortran.dg/random_seed_1.f90
index 39c81ce51b7..a97e53059f8 100644
--- a/gcc/testsuite/gfortran.dg/random_seed_1.f90
+++ b/gcc/testsuite/gfortran.dg/random_seed_1.f90
@@ -10,11 +10,12 @@
PROGRAM random_seed_1
IMPLICIT NONE
- INTEGER, PARAMETER :: nbytes = 128
+ ! Should match sizeof(master_state) in
+ ! libgfortran/intrinsics/random.c
+ INTEGER, PARAMETER :: nbytes
[PATCH, PR d/90601] Committed fix for ICE: gimplification failed (gimplify.c at 13436)
Hi,
The expression that caused the ICE
++(a += 1.0);
The D front-end rewrites and applies implicit type conversions so the
expression gets simplified as
(int)((double) a += 1.0) += 1
The codegen pass would subsequently generate the following invalid code
(int)(double) a = (int)((double) a + 1.0) + 1
The LHS expression `(int)(double) a', represented as a FIX_TRUNC_EXPR
being what trips as it is not a valid lvalue for assignment.
While LHS casts are stripped away, convert_expr adds a double cast
because it converts the expression to its original type before
converting it to its target type. There is no valid reason why this
is done, so it has been removed.
Bootstrapped and regression tested on x86_64-linux-gnu.
Committed to trunk as r274263.
--
Iain
---
gcc/d/ChangeLog:
PR d/90601
* d-convert.cc (convert_expr): Don't convert an expression to its
original front-end type before converting to its target type.
gcc/testsuite/ChangeLog:
PR d/90601
* gdc.dg/pr90601.d: New test.
---
diff --git a/gcc/d/d-convert.cc b/gcc/d/d-convert.cc
index b020eab902f..fd4fc3c692d 100644
--- a/gcc/d/d-convert.cc
+++ b/gcc/d/d-convert.cc
@@ -588,7 +588,6 @@ convert_expr (tree exp, Type *etype, Type *totype)
return compound_expr (exp, build_zero_cst (build_ctype (tbtype)));
}
- exp = fold_convert (build_ctype (etype), exp);
gcc_assert (TREE_CODE (exp) != STRING_CST);
break;
}
diff --git a/gcc/testsuite/gdc.dg/pr90601.d b/gcc/testsuite/gdc.dg/pr90601.d
new file mode 100644
index 000..88cdaf8c99d
--- /dev/null
+++ b/gcc/testsuite/gdc.dg/pr90601.d
@@ -0,0 +1,22 @@
+// https://gcc.gnu.org/bugzilla/show_bug.cgi?id=90601
+// { dg-do compile }
+
+int postincr(int a)
+{
+return (a += 1.0)++;
+}
+
+int postdecr(int a)
+{
+return (a -= 1.0)--;
+}
+
+int preincr(int a)
+{
+return ++(a += 1.0);
+}
+
+int predecr(int a)
+{
+return --(a -= 1.0);
+}
