* [PATCH] lto: Fix up lto location streaming
@ 2020-09-10 13:53 Jakub Jelinek
2020-09-10 14:09 ` Richard Biener
0 siblings, 1 reply; 5+ messages in thread
From: Jakub Jelinek @ 2020-09-10 13:53 UTC (permalink / raw)
To: Richard Biener; +Cc: gcc-patches
Hi!
When I've tried to backport recent LTO changes of mine, I've ran into
FAIL: g++.dg/ubsan/align-3.C -O2 -flto -fno-use-linker-plugin -flto-partition=none output pattern test
FAIL: g++.dg/ubsan/align-3.C -O2 -flto -fuse-linker-plugin -fno-fat-lto-objects output pattern test
regressions that don't for some reason show up on the trunk.
I've tracked it down to input_location_and_block being called recursively,
first on some UBSAN_NULL ifn call's location which needs to stream a BLOCK
that hasn't been streamed yet, which in turn needs to stream some locations
for the decls in the BLOCK.
Now, the problem is that both lto_output_location_1 and
input_location_and_block use static variables and/or class members to track
what the "current" location is (so that we don't stream everything all the
time), but on the writer side, lto_output_tree is pretty much the last thing
the function does, so it doesn't matter if the recursion updates those,
but on the reader side, the code relies on the stream_* static vars
and current_* class members not to change across the stream_read_tree call.
I believe we shouldn't be streaming any BLOCKs in the recursive calls,
as we only stream blocks for gimple_location, PHI locations and goto_locus.
Anyway, the following patch fixes it by forcing all those values that might
change across the call into automatic variables, so the code handles
recursion well. The lto-streamer-out.c change is actually not strictly
necessary because BLOCKs shouldn't be streamed out, and if they would be,
it is unclear how it could be handled, because on stream in we can't set
the "current" block until it is streamed in. So, maybe it is more correct
to leave that hunk out.
Tested so far with make check RUNTESTFLAGS=lto.exp, lto bootstrap in
progress, ok for trunk if it succeeds? With or without the
lto-streamer-out.c change?
2020-09-10 Jakub Jelinek <jakub@redhat.com>
* lto-streamer-in.c (lto_location_cache::input_location_and_block):
Don't use static variables nor class members across the
stream_read_tree call, as it might recurse into the
input_location_and_block method.
* lto-streamer-out.c (lto_output_location_1): Write ob->current_block
before calling lto_output_tree, as it might recurse into
lto_output_location_1.
--- gcc/lto-streamer-in.c.jj 2020-09-10 14:29:17.925466837 +0200
+++ gcc/lto-streamer-in.c 2020-09-10 15:09:57.408311917 +0200
@@ -298,7 +298,29 @@ lto_location_cache::input_location_and_b
if (column_change)
stream_col = bp_unpack_var_len_unsigned (bp);
+ /* stream_read_tree below might recurse into input_location_and_block,
+ e.g. when streaming locations of the decls in the BLOCK, although it
+ shouldn't recurse with ib non-NULL. So, avoid using the static
+ variables as well as class members across it. */
+ const char *file = stream_file;
+ int line = stream_line;
+ int col = stream_col;
+ bool sysp = stream_sysp;
tree block = NULL_TREE;
+ bool use_current_loc = false;
+
+ /* This optimization saves location cache operations during gimple
+ streaming. */
+
+ if (current_file == file
+ && current_line == line
+ && current_col == col
+ && current_sysp == sysp)
+ {
+ use_current_loc = true;
+ *loc = current_loc;
+ }
+
if (ib)
{
bool block_change = bp_unpack_value (bp, 1);
@@ -307,25 +329,19 @@ lto_location_cache::input_location_and_b
block = stream_block;
}
- /* This optimization saves location cache operations during gimple
- streaming. */
-
- if (current_file == stream_file
- && current_line == stream_line
- && current_col == stream_col
- && current_sysp == stream_sysp)
+ if (use_current_loc)
{
- if (current_block == block)
- *loc = current_loc;
- else if (block)
- *loc = set_block (current_loc, block);
- else
- *loc = LOCATION_LOCUS (current_loc);
+ if (LOCATION_BLOCK (*loc) != block)
+ {
+ if (block)
+ *loc = set_block (*loc, block);
+ else
+ *loc = LOCATION_LOCUS (*loc);
+ }
return;
}
- struct cached_location entry
- = {stream_file, loc, stream_line, stream_col, stream_sysp, block};
+ struct cached_location entry = { file, loc, line, col, sysp, block };
loc_cache.safe_push (entry);
}
--- gcc/lto-streamer-out.c.jj 2020-09-10 14:29:17.939466635 +0200
+++ gcc/lto-streamer-out.c 2020-09-10 15:12:00.948535171 +0200
@@ -231,8 +231,12 @@ lto_output_location_1 (struct output_blo
bp_pack_value (bp, ob->current_block != block, 1);
streamer_write_bitpack (bp);
if (ob->current_block != block)
- lto_output_tree (ob, block, true, true);
- ob->current_block = block;
+ {
+ ob->current_block = block;
+ /* lto_output_tree might recurse into lto_output_location_1,
+ so update ob->current_block before calling it. */
+ lto_output_tree (ob, block, true, true);
+ }
}
}
Jakub
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] lto: Fix up lto location streaming
2020-09-10 13:53 [PATCH] lto: Fix up lto location streaming Jakub Jelinek
@ 2020-09-10 14:09 ` Richard Biener
2020-09-10 15:32 ` Jakub Jelinek
0 siblings, 1 reply; 5+ messages in thread
From: Richard Biener @ 2020-09-10 14:09 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: gcc-patches
On Thu, 10 Sep 2020, Jakub Jelinek wrote:
> Hi!
>
> When I've tried to backport recent LTO changes of mine, I've ran into
> FAIL: g++.dg/ubsan/align-3.C -O2 -flto -fno-use-linker-plugin -flto-partition=none output pattern test
> FAIL: g++.dg/ubsan/align-3.C -O2 -flto -fuse-linker-plugin -fno-fat-lto-objects output pattern test
> regressions that don't for some reason show up on the trunk.
>
> I've tracked it down to input_location_and_block being called recursively,
> first on some UBSAN_NULL ifn call's location which needs to stream a BLOCK
> that hasn't been streamed yet, which in turn needs to stream some locations
> for the decls in the BLOCK.
>
> Now, the problem is that both lto_output_location_1 and
> input_location_and_block use static variables and/or class members to track
> what the "current" location is (so that we don't stream everything all the
> time), but on the writer side, lto_output_tree is pretty much the last thing
> the function does, so it doesn't matter if the recursion updates those,
> but on the reader side, the code relies on the stream_* static vars
> and current_* class members not to change across the stream_read_tree call.
>
> I believe we shouldn't be streaming any BLOCKs in the recursive calls,
> as we only stream blocks for gimple_location, PHI locations and goto_locus.
>
> Anyway, the following patch fixes it by forcing all those values that might
> change across the call into automatic variables, so the code handles
> recursion well. The lto-streamer-out.c change is actually not strictly
> necessary because BLOCKs shouldn't be streamed out, and if they would be,
> it is unclear how it could be handled, because on stream in we can't set
> the "current" block until it is streamed in. So, maybe it is more correct
> to leave that hunk out.
>
> Tested so far with make check RUNTESTFLAGS=lto.exp, lto bootstrap in
> progress, ok for trunk if it succeeds? With or without the
> lto-streamer-out.c change?
So we're usually streaming the bits and the tree portion of SCCs
separately so I really wonder how we can end up with recursion here?
For stmts they should only refer to BLOCKs also in the BLOCK tree
which is streamed in before we stream in the stmts(?).
So - isn't this an undetected stmt/tree references BLOCK not in
BLOCK tree bug? See verify_location which might or might not
handle all locations "correctly" though it looks like it obviously
verifies all stmts locations this way. So the question is whether
the function would pass verification at stream-out time ...
Richard.
> 2020-09-10 Jakub Jelinek <jakub@redhat.com>
>
> * lto-streamer-in.c (lto_location_cache::input_location_and_block):
> Don't use static variables nor class members across the
> stream_read_tree call, as it might recurse into the
> input_location_and_block method.
> * lto-streamer-out.c (lto_output_location_1): Write ob->current_block
> before calling lto_output_tree, as it might recurse into
> lto_output_location_1.
>
> --- gcc/lto-streamer-in.c.jj 2020-09-10 14:29:17.925466837 +0200
> +++ gcc/lto-streamer-in.c 2020-09-10 15:09:57.408311917 +0200
> @@ -298,7 +298,29 @@ lto_location_cache::input_location_and_b
> if (column_change)
> stream_col = bp_unpack_var_len_unsigned (bp);
>
> + /* stream_read_tree below might recurse into input_location_and_block,
> + e.g. when streaming locations of the decls in the BLOCK, although it
> + shouldn't recurse with ib non-NULL. So, avoid using the static
> + variables as well as class members across it. */
> + const char *file = stream_file;
> + int line = stream_line;
> + int col = stream_col;
> + bool sysp = stream_sysp;
> tree block = NULL_TREE;
> + bool use_current_loc = false;
> +
> + /* This optimization saves location cache operations during gimple
> + streaming. */
> +
> + if (current_file == file
> + && current_line == line
> + && current_col == col
> + && current_sysp == sysp)
> + {
> + use_current_loc = true;
> + *loc = current_loc;
> + }
> +
> if (ib)
> {
> bool block_change = bp_unpack_value (bp, 1);
> @@ -307,25 +329,19 @@ lto_location_cache::input_location_and_b
> block = stream_block;
> }
>
> - /* This optimization saves location cache operations during gimple
> - streaming. */
> -
> - if (current_file == stream_file
> - && current_line == stream_line
> - && current_col == stream_col
> - && current_sysp == stream_sysp)
> + if (use_current_loc)
> {
> - if (current_block == block)
> - *loc = current_loc;
> - else if (block)
> - *loc = set_block (current_loc, block);
> - else
> - *loc = LOCATION_LOCUS (current_loc);
> + if (LOCATION_BLOCK (*loc) != block)
> + {
> + if (block)
> + *loc = set_block (*loc, block);
> + else
> + *loc = LOCATION_LOCUS (*loc);
> + }
> return;
> }
>
> - struct cached_location entry
> - = {stream_file, loc, stream_line, stream_col, stream_sysp, block};
> + struct cached_location entry = { file, loc, line, col, sysp, block };
> loc_cache.safe_push (entry);
> }
>
> --- gcc/lto-streamer-out.c.jj 2020-09-10 14:29:17.939466635 +0200
> +++ gcc/lto-streamer-out.c 2020-09-10 15:12:00.948535171 +0200
> @@ -231,8 +231,12 @@ lto_output_location_1 (struct output_blo
> bp_pack_value (bp, ob->current_block != block, 1);
> streamer_write_bitpack (bp);
> if (ob->current_block != block)
> - lto_output_tree (ob, block, true, true);
> - ob->current_block = block;
> + {
> + ob->current_block = block;
> + /* lto_output_tree might recurse into lto_output_location_1,
> + so update ob->current_block before calling it. */
> + lto_output_tree (ob, block, true, true);
> + }
> }
> }
>
>
>
> Jakub
>
>
--
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] lto: Fix up lto location streaming
2020-09-10 14:09 ` Richard Biener
@ 2020-09-10 15:32 ` Jakub Jelinek
2020-09-10 18:39 ` Jakub Jelinek
0 siblings, 1 reply; 5+ messages in thread
From: Jakub Jelinek @ 2020-09-10 15:32 UTC (permalink / raw)
To: Richard Biener, Jan Hubicka; +Cc: gcc-patches
On Thu, Sep 10, 2020 at 04:09:24PM +0200, Richard Biener wrote:
> So we're usually streaming the bits and the tree portion of SCCs
> separately so I really wonder how we can end up with recursion here?
> For stmts they should only refer to BLOCKs also in the BLOCK tree
> which is streamed in before we stream in the stmts(?).
>
> So - isn't this an undetected stmt/tree references BLOCK not in
> BLOCK tree bug? See verify_location which might or might not
> handle all locations "correctly" though it looks like it obviously
> verifies all stmts locations this way. So the question is whether
> the function would pass verification at stream-out time ...
Looking at that align-3.C testcase on the trunk, I also see the recursive
lto_output_location_1 calls as well. First on block:
$18 = <block 0x7fffea738480>
which is in the block tree from what I can see just fine (see the end of
mail).
Now, output_function has code that should stream the BLOCK tree leafs:
/* Output DECL_INITIAL for the function, which contains the tree of
lexical scopes. */
stream_write_tree (ob, DECL_INITIAL (function), true);
/* As we do not recurse into BLOCK_SUBBLOCKS but only BLOCK_SUPERCONTEXT
collect block tree leafs and stream those. */
auto_vec<tree> block_tree_leafs;
if (DECL_INITIAL (function))
collect_block_tree_leafs (DECL_INITIAL (function), block_tree_leafs);
streamer_write_uhwi (ob, block_tree_leafs.length ());
for (unsigned i = 0; i < block_tree_leafs.length (); ++i)
stream_write_tree (ob, block_tree_leafs[i], true);
static void
collect_block_tree_leafs (tree root, vec<tree> &leafs)
{
for (root = BLOCK_SUBBLOCKS (root); root; root = BLOCK_CHAIN (root))
if (! BLOCK_SUBBLOCKS (root))
leafs.safe_push (root);
else
collect_block_tree_leafs (BLOCK_SUBBLOCKS (root), leafs);
}
but the problem is that it is broken, it doesn't cover all block leafs,
but only leafs with an odd depth from DECL_INITIAL (and only some of those).
The following patch fixes that, but I guess we are going to stream at that
point significantly more blocks than before (though I guess most of the time
we'd stream them later on when streaming the gimple_locations that refer to
them).
2020-09-10 Jakub Jelinek <jakub@redhat.com>
* lto-streamer-out.c (collect_block_tree_leafs): Recurse on
root rather than BLOCK_SUBBLOCKS (root).
--- gcc/lto-streamer-out.c.jj 2020-09-10 15:52:36.401413518 +0200
+++ gcc/lto-streamer-out.c 2020-09-10 17:14:24.934503237 +0200
@@ -2294,7 +2294,7 @@ collect_block_tree_leafs (tree root, vec
if (! BLOCK_SUBBLOCKS (root))
leafs.safe_push (root);
else
- collect_block_tree_leafs (BLOCK_SUBBLOCKS (root), leafs);
+ collect_block_tree_leafs (root, leafs);
}
/* This performs function body modifications that are needed for streaming
(gdb) p debug_tree (((tree)0x7fffea71d700)->decl_common.initial)
<block 0x7fffea6f9ea0 used tree_0
supercontext <function_decl 0x7fffea71d700 main
type <function_type 0x7fffea8259d8 type <integer_type 0x7fffea8145e8 int>
QI
size <integer_cst 0x7fffea7f5f60 constant 8>
unit-size <integer_cst 0x7fffea7f5f78 constant 1>
align:8 warn_if_not_align:0 symtab:0 alias-set -1 canonical-type 0x7fffea8259d8
arg-types <tree_list 0x7fffea80a8e8 value <void_type 0x7fffea814f18 void>>>
addressable nothrow public static function-specific-target function-specific-opt decl_5 QI /usr/src/gcc-10/gcc/testsuite/g++.dg/ubsan/align-3.C:23:1 align:8 warn_if_not_align:0 context <translation_unit_decl 0x7fffea803168 /usr/src/gcc-10/gcc/testsuite/g++.dg/ubsan/align-3.C> initial <block 0x7fffea6f9ea0>
result <result_decl 0x7fffea70de10 D.6516 type <integer_type 0x7fffea8145e8 int>
ignored SI /usr/src/gcc-10/gcc/testsuite/g++.dg/ubsan/align-3.C:23:11
size <integer_cst 0x7fffea8170c0 constant 32>
unit-size <integer_cst 0x7fffea8170d8 constant 4>
align:32 warn_if_not_align:0 context <function_decl 0x7fffea71d700 main>>
struct-function 0x7fffea714840
chain <var_decl 0x7fffea719120 s type <record_type 0x7fffea71a0a8 S>
addressable used public static read BLK /usr/src/gcc-10/gcc/testsuite/g++.dg/ubsan/align-3.C:20:69
size <integer_cst 0x7fffea817330 constant 192>
unit-size <integer_cst 0x7fffea817300 constant 24>
align:128 warn_if_not_align:0 context <translation_unit_decl 0x7fffea803168 /usr/src/gcc-10/gcc/testsuite/g++.dg/ubsan/align-3.C> chain <type_decl 0x7fffea718098 S>>>
subblocks <block 0x7fffea7384e0 used supercontext <block 0x7fffea6f9ea0>
subblocks <block 0x7fffea738540 used supercontext <block 0x7fffea7384e0>
subblocks <block 0x7fffea7385a0 used supercontext <block 0x7fffea738540>
abstract_origin <block 0x7fffea6f9a80 used supercontext <function_decl 0x7fffea713b00 __ct >>> abstract_origin <block 0x7fffea6f9a80>>
chain <block 0x7fffea7383c0 used supercontext <block 0x7fffea6f9ea0>
subblocks <block 0x7fffea738480 used supercontext <block 0x7fffea7383c0>
^^^^^^^^^^^^^^ HERE
abstract_origin <block 0x7fffea6f9c00 used supercontext <function_decl 0x7fffea713c00 __dt >>>
chain <block 0x7fffea7381e0 used supercontext <block 0x7fffea6f9ea0>
subblocks <block 0x7fffea738240 used supercontext <block 0x7fffea7381e0>
subblocks <block 0x7fffea7382a0 used supercontext <block 0x7fffea738240> abstract_origin <block 0x7fffea6f9a80>> abstract_origin <block 0x7fffea6f9a80>>
chain <block 0x7fffea6f9f00 used supercontext <block 0x7fffea6f9ea0>
subblocks <block 0x7fffea738000 used supercontext <block 0x7fffea6f9f00> abstract_origin <block 0x7fffea6f9c00>> abstract_origin <function_decl 0x7fffea717200 __dt_base >> abstract_origin <function_decl 0x7fffea713b00 __ct >> abstract_origin <function_decl 0x7fffea717200 __dt_base >>
abstract_origin <function_decl 0x7fffea713b00 __ct type <method_type 0x7fffea711498>
addressable asm_written used nothrow public static abstract external weak autoinline decl_5 QI defer-output /usr/src/gcc-10/gcc/testsuite/g++.dg/ubsan/align-3.C:14:3 align:16 warn_if_not_align:0 context <translation_unit_decl 0x7fffea803168 /usr/src/gcc-10/gcc/testsuite/g++.dg/ubsan/align-3.C> initial <error_mark 0x7fffea7f5e58> chain <function_decl 0x7fffea717000 __ct_base >>>>
Jakub
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] lto: Fix up lto location streaming
2020-09-10 15:32 ` Jakub Jelinek
@ 2020-09-10 18:39 ` Jakub Jelinek
2020-09-10 18:49 ` Richard Biener
0 siblings, 1 reply; 5+ messages in thread
From: Jakub Jelinek @ 2020-09-10 18:39 UTC (permalink / raw)
To: Richard Biener, Jan Hubicka, gcc-patches
On Thu, Sep 10, 2020 at 05:32:58PM +0200, Jakub Jelinek via Gcc-patches wrote:
> 2020-09-10 Jakub Jelinek <jakub@redhat.com>
>
> * lto-streamer-out.c (collect_block_tree_leafs): Recurse on
> root rather than BLOCK_SUBBLOCKS (root).
>
> --- gcc/lto-streamer-out.c.jj 2020-09-10 15:52:36.401413518 +0200
> +++ gcc/lto-streamer-out.c 2020-09-10 17:14:24.934503237 +0200
> @@ -2294,7 +2294,7 @@ collect_block_tree_leafs (tree root, vec
> if (! BLOCK_SUBBLOCKS (root))
> leafs.safe_push (root);
> else
> - collect_block_tree_leafs (BLOCK_SUBBLOCKS (root), leafs);
> + collect_block_tree_leafs (root, leafs);
> }
>
> /* This performs function body modifications that are needed for streaming
Successfully lto-bootstrapped/regtested on x86_64-linux. As a very rough estimate
(not exactly the same tree, 22 hours appart), *.o files in the stage3 directory grew
from 522432KB to 524760KB together (slim LTO), so 0.4% growth.
Jakub
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] lto: Fix up lto location streaming
2020-09-10 18:39 ` Jakub Jelinek
@ 2020-09-10 18:49 ` Richard Biener
0 siblings, 0 replies; 5+ messages in thread
From: Richard Biener @ 2020-09-10 18:49 UTC (permalink / raw)
To: Jakub Jelinek, Jan Hubicka, gcc-patches
On September 10, 2020 8:39:20 PM GMT+02:00, Jakub Jelinek <jakub@redhat.com> wrote:
>On Thu, Sep 10, 2020 at 05:32:58PM +0200, Jakub Jelinek via Gcc-patches
>wrote:
>> 2020-09-10 Jakub Jelinek <jakub@redhat.com>
>>
>> * lto-streamer-out.c (collect_block_tree_leafs): Recurse on
>> root rather than BLOCK_SUBBLOCKS (root).
>>
>> --- gcc/lto-streamer-out.c.jj 2020-09-10 15:52:36.401413518 +0200
>> +++ gcc/lto-streamer-out.c 2020-09-10 17:14:24.934503237 +0200
>> @@ -2294,7 +2294,7 @@ collect_block_tree_leafs (tree root, vec
>> if (! BLOCK_SUBBLOCKS (root))
>> leafs.safe_push (root);
>> else
>> - collect_block_tree_leafs (BLOCK_SUBBLOCKS (root), leafs);
>> + collect_block_tree_leafs (root, leafs);
>> }
>>
>> /* This performs function body modifications that are needed for
>streaming
>
>Successfully lto-bootstrapped/regtested on x86_64-linux. As a very
>rough estimate
>(not exactly the same tree, 22 hours appart), *.o files in the stage3
>directory grew
>from 522432KB to 524760KB together (slim LTO), so 0.4% growth.
OK.
Thanks,
Richard.
> Jakub
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-09-10 18:49 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-10 13:53 [PATCH] lto: Fix up lto location streaming Jakub Jelinek
2020-09-10 14:09 ` Richard Biener
2020-09-10 15:32 ` Jakub Jelinek
2020-09-10 18:39 ` Jakub Jelinek
2020-09-10 18:49 ` Richard Biener
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).