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 <ja...@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 <rguent...@suse.de>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)
