* [PATCH] lto: Ensure we force a change for file/line/column after clear_line_info
@ 2020-09-04 8:39 Jakub Jelinek
2020-09-04 9:32 ` Richard Biener
0 siblings, 1 reply; 2+ messages in thread
From: Jakub Jelinek @ 2020-09-04 8:39 UTC (permalink / raw)
To: Richard Biener, Jan Hubicka; +Cc: gcc-patches
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?
2020-09-03 Jakub Jelinek <jakub@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
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [PATCH] lto: Ensure we force a change for file/line/column after clear_line_info
2020-09-04 8:39 [PATCH] lto: Ensure we force a change for file/line/column after clear_line_info Jakub Jelinek
@ 2020-09-04 9:32 ` Richard Biener
0 siblings, 0 replies; 2+ messages in thread
From: Richard Biener @ 2020-09-04 9:32 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: Jan Hubicka, gcc-patches
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 <jakub@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 <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] 2+ messages in thread
end of thread, other threads:[~2020-09-04 9:32 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-04 8:39 [PATCH] lto: Ensure we force a change for file/line/column after clear_line_info Jakub Jelinek
2020-09-04 9:32 ` 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).