On Fri, 4 Sep 2020, Jakub Jelinek wrote: > Hi! > > As discussed yesterday: > On the streamer out side, we call clear_line_info > in multiple spots which resets the current_* values to something, but on the > reader side, we don't have corresponding resets in the same location, just > have > the stream_* static variables that keep the current values through the > entire stream in (so across all the clear_line_info spots in a single LTO > object but also across jumping from one LTO object to another one). > Now, in an earlier version of my patch it actually broke LTO bootstrap > (and a lot of LTO testcases), so for the BLOCK case I've solved it by > clear_line_info setting current_block to something that should never appear, > which means that in the LTO stream after the clear_line_info spots including > the start of the LTO stream we force the block change bit to be set and thus > BLOCK to be streamed and therefore stream_block from earlier to be > ignored. But for the rest I think that is not the case, so I wonder if we > don't sometimes end up with wrong line/column info because of that, or > please tell me what prevents that. > clear_line_info does: > ob->current_file = NULL; > ob->current_line = 0; > ob->current_col = 0; > ob->current_sysp = false; > while I think NULL current_file is something that should likely be different > from expanded_location (...).file (UNKNOWN_LOCATION/BUILTINS_LOCATION are > handled separately and not go through the caching), I think line number 0 > can sometimes occur and especially column 0 occurs frequently if we ran out > of location_t with columns info. But then we do: > bp_pack_value (bp, ob->current_file != xloc.file, 1); > bp_pack_value (bp, ob->current_line != xloc.line, 1); > bp_pack_value (bp, ob->current_col != xloc.column, 1); > and stream the details only if the != is true. If that happens immediately > after clear_line_info and e.g. xloc.column is 0, we would stream 0 bit and > not stream the actual value, so on read-in it would reuse whatever > stream_col etc. were before. Shouldn't we set some ob->current_* new bit > that would signal we are immediately past clear_line_info which would force > all these != checks to non-zero? Either by oring something into those > tests, or perhaps: > if (ob->current_reset) > { > if (xloc.file == NULL) > ob->current_file = ""; > if (xloc.line == 0) > ob->current_line = 1; > if (xloc.column == 0) > ob->current_column = 1; > ob->current_reset = false; > } > before doing those bp_pack_value calls with a comment, effectively forcing > all 6 != comparisons to be true? > > Bootstrapped/regtested on x86_64-linux and i686-linux and lto bootstrapped > on x86_64-linux, ok for trunk?
OK. Thanks, Richard. > 2020-09-03 Jakub Jelinek <ja...@redhat.com> > > * lto-streamer.h (struct output_block): Add reset_locus member. > * lto-streamer-out.c (clear_line_info): Set reset_locus to true. > (lto_output_location_1): If reset_locus, clear it and ensure > current_{file,line,col} is different from xloc members. > > --- gcc/lto-streamer.h.jj 2020-09-03 12:50:54.990753979 +0200 > +++ gcc/lto-streamer.h 2020-09-03 17:11:51.406571519 +0200 > @@ -717,6 +717,7 @@ struct output_block > int current_line; > int current_col; > bool current_sysp; > + bool reset_locus; > tree current_block; > > /* Cache of nodes written in this section. */ > --- gcc/lto-streamer-out.c.jj 2020-09-03 12:50:54.992753950 +0200 > +++ gcc/lto-streamer-out.c 2020-09-03 17:16:44.601311584 +0200 > @@ -60,6 +60,7 @@ clear_line_info (struct output_block *ob > ob->current_line = 0; > ob->current_col = 0; > ob->current_sysp = false; > + ob->reset_locus = true; > /* Initialize to something that will never appear as block, > so that the first location with block in a function etc. > always streams a change_block bit and the first block. */ > @@ -195,6 +196,17 @@ lto_output_location_1 (struct output_blo > { > expanded_location xloc = expand_location (loc); > > + if (ob->reset_locus) > + { > + if (xloc.file == NULL) > + ob->current_file = ""; > + if (xloc.line == 0) > + ob->current_line = 1; > + if (xloc.column == 0) > + ob->current_col = 1; > + ob->reset_locus = false; > + } > + > bp_pack_value (bp, ob->current_file != xloc.file, 1); > bp_pack_value (bp, ob->current_line != xloc.line, 1); > bp_pack_value (bp, ob->current_col != xloc.column, 1); > > Jakub > > -- Richard Biener <rguent...@suse.de> SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)