Fix LTO streaming of BUILTINS_LOCATION

Fix LTO streaming of BUILTINS_LOCATION

[Jan Hubicka]

Hi,
currently we stream BUILTINS_LOCATION by expanding it and streaming resulting
filename/line/col tripplet.  That is a nonsense and breaks some logic
that special case it.

This patch fixes it by special casing it same way as we do UNKNOWN_LOCATION
(we have precisely 2 special location codes, so doing compound bitpack is
not needed)

Bootstrapped/regtested ppc64le-linux, OK?

Honza

	* lto-streamer-out.c (lto_output_location): Correctly stream
	BUILTINS_LOCATION
	* lto-streamer-in (lto_input_location): Likewise.
Index: lto-streamer-out.c
===================================================================
--- lto-streamer-out.c	(revision 224201)
+++ lto-streamer-out.c	(working copy)
@@ -205,6 +205,9 @@
   bp_pack_value (bp, loc == UNKNOWN_LOCATION, 1);
   if (loc == UNKNOWN_LOCATION)
     return;
+  bp_pack_value (bp, loc == BUILTINS_LOCATION, 1);
+  if (loc == BUILTINS_LOCATION)
+    return;
 
   xloc = expand_location (loc);
 
Index: lto-streamer-in.c
===================================================================
--- lto-streamer-in.c	(revision 224201)
+++ lto-streamer-in.c	(working copy)
@@ -283,6 +283,11 @@
       *loc = UNKNOWN_LOCATION;
       return;
     }
+  if (bp_unpack_value (bp, 1))
+    {
+      *loc = BUILTINS_LOCATION;
+      return;
+    }
   *loc = BUILTINS_LOCATION + 1;
 
   file_change = bp_unpack_value (bp, 1);

Re: Fix LTO streaming of BUILTINS_LOCATION

[Richard Biener]

On Mon, 8 Jun 2015, Jan Hubicka wrote:

> Hi,
> currently we stream BUILTINS_LOCATION by expanding it and streaming resulting
> filename/line/col tripplet.  That is a nonsense and breaks some logic
> that special case it.
> 
> This patch fixes it by special casing it same way as we do UNKNOWN_LOCATION
> (we have precisely 2 special location codes, so doing compound bitpack is
> not needed)
> 
> Bootstrapped/regtested ppc64le-linux, OK?
> 
> Honza
> 
> 	* lto-streamer-out.c (lto_output_location): Correctly stream
> 	BUILTINS_LOCATION
> 	* lto-streamer-in (lto_input_location): Likewise.
> Index: lto-streamer-out.c
> ===================================================================
> --- lto-streamer-out.c	(revision 224201)
> +++ lto-streamer-out.c	(working copy)
> @@ -205,6 +205,9 @@
>    bp_pack_value (bp, loc == UNKNOWN_LOCATION, 1);
>    if (loc == UNKNOWN_LOCATION)
>      return;
> +  bp_pack_value (bp, loc == BUILTINS_LOCATION, 1);
> +  if (loc == BUILTINS_LOCATION)
> +    return;

Hmm, with this and

#define DECL_IS_BUILTIN(DECL) \
  (LOCATION_LOCUS (DECL_SOURCE_LOCATION (DECL)) <= BUILTINS_LOCATION)

shouldn't we rather stream all locations <= BUILTINS_LOCATION literally?
That is, instead of two bits stream a [0, BUILTINS_LOCATION+1] 'enum'
here?  Btw, line-map.h has RESERVED_LOCATION_COUNT for the locations
that are "special" (currently two, so your patch will work in practice).
  
>    xloc = expand_location (loc);
>  
> Index: lto-streamer-in.c
> ===================================================================
> --- lto-streamer-in.c	(revision 224201)
> +++ lto-streamer-in.c	(working copy)
> @@ -283,6 +283,11 @@
>        *loc = UNKNOWN_LOCATION;
>        return;
>      }
> +  if (bp_unpack_value (bp, 1))
> +    {
> +      *loc = BUILTINS_LOCATION;
> +      return;
> +    }
>    *loc = BUILTINS_LOCATION + 1;

Btw, this assignment to *loc looks odd (I suppose it's to make
location caching work).

Richard.

>    file_change = bp_unpack_value (bp, 1);
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Dilip Upmanyu, Graham Norton, HRB 21284 (AG Nuernberg)

Re: Fix LTO streaming of BUILTINS_LOCATION

[Jan Hubicka]

> > +  bp_pack_value (bp, loc == BUILTINS_LOCATION, 1);
> > +  if (loc == BUILTINS_LOCATION)
> > +    return;
> 
> Hmm, with this and
> 
> #define DECL_IS_BUILTIN(DECL) \
>   (LOCATION_LOCUS (DECL_SOURCE_LOCATION (DECL)) <= BUILTINS_LOCATION)
> 
> shouldn't we rather stream all locations <= BUILTINS_LOCATION literally?
> That is, instead of two bits stream a [0, BUILTINS_LOCATION+1] 'enum'
> here?  Btw, line-map.h has RESERVED_LOCATION_COUNT for the locations
> that are "special" (currently two, so your patch will work in practice).

Yep, i considered that.  Because we have precisely two special locations (ATM)
and UNKNOWN_LOCATION is quite common, that would waste one extra bit on that
case.  

Probably not a big deal, so if you think streaming all locations up to
BUILTINS_LOCATION literarly is more robust, i will update the patch.
>   
> >    xloc = expand_location (loc);
> >  
> > Index: lto-streamer-in.c
> > ===================================================================
> > --- lto-streamer-in.c	(revision 224201)
> > +++ lto-streamer-in.c	(working copy)
> > @@ -283,6 +283,11 @@
> >        *loc = UNKNOWN_LOCATION;
> >        return;
> >      }
> > +  if (bp_unpack_value (bp, 1))
> > +    {
> > +      *loc = BUILTINS_LOCATION;
> > +      return;
> > +    }
> >    *loc = BUILTINS_LOCATION + 1;
> 
> Btw, this assignment to *loc looks odd (I suppose it's to make
> location caching work).

*loc is set to UNKNOWN_LOCATION/BUILTINS_LOCATION for those locations that are
not cached and all others get BUILTINS_LOCATION + 1 which quite safely triggers
ICE in line_map lookup though I do not recall why.  I originally used 
UNKNOWN_LOCATION for cached values but that did not work as it confused
DECL_IS_BUILTIN.

We could extend API by adding INVALID_LOCATION and set it to INT_MAX or something
that would also ICE.

Honza
> 
> Richard.
> 
> >    file_change = bp_unpack_value (bp, 1);
> > 
> > 
> 
> -- 
> Richard Biener <rguenther@suse.de>
> SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Dilip Upmanyu, Graham Norton, HRB 21284 (AG Nuernberg)

Re: Fix LTO streaming of BUILTINS_LOCATION

[Richard Biener]

On Mon, 8 Jun 2015, Jan Hubicka wrote:

> > > +  bp_pack_value (bp, loc == BUILTINS_LOCATION, 1);
> > > +  if (loc == BUILTINS_LOCATION)
> > > +    return;
> > 
> > Hmm, with this and
> > 
> > #define DECL_IS_BUILTIN(DECL) \
> >   (LOCATION_LOCUS (DECL_SOURCE_LOCATION (DECL)) <= BUILTINS_LOCATION)
> > 
> > shouldn't we rather stream all locations <= BUILTINS_LOCATION literally?
> > That is, instead of two bits stream a [0, BUILTINS_LOCATION+1] 'enum'
> > here?  Btw, line-map.h has RESERVED_LOCATION_COUNT for the locations
> > that are "special" (currently two, so your patch will work in practice).
> 
> Yep, i considered that.  Because we have precisely two special locations (ATM)
> and UNKNOWN_LOCATION is quite common, that would waste one extra bit on that
> case.  
> 
> Probably not a big deal, so if you think streaming all locations up to
> BUILTINS_LOCATION literarly is more robust, i will update the patch.

Yeah, I think streaming all locations up to RESERVED_LOCATION_COUNT
literally is more robust.  Thus do

  bp_pack_int_in_range (bp, 0, RESERVED_LOCATION_COUNT,
			loc < RESERVED_LOCATION_COUNT ? loc : 
RESERVED_LOCATION_COUNT);
  if (loc < RESERVED_LOCATION_COUNT)
    return;

and on unpacking use the special RESERVED_LOCATION_COUNT value
to fall thru to unpacked location handling.

Richard.

> >   
> > >    xloc = expand_location (loc);
> > >  
> > > Index: lto-streamer-in.c
> > > ===================================================================
> > > --- lto-streamer-in.c	(revision 224201)
> > > +++ lto-streamer-in.c	(working copy)
> > > @@ -283,6 +283,11 @@
> > >        *loc = UNKNOWN_LOCATION;
> > >        return;
> > >      }
> > > +  if (bp_unpack_value (bp, 1))
> > > +    {
> > > +      *loc = BUILTINS_LOCATION;
> > > +      return;
> > > +    }
> > >    *loc = BUILTINS_LOCATION + 1;
> > 
> > Btw, this assignment to *loc looks odd (I suppose it's to make
> > location caching work).
> 
> *loc is set to UNKNOWN_LOCATION/BUILTINS_LOCATION for those locations that are
> not cached and all others get BUILTINS_LOCATION + 1 which quite safely triggers
> ICE in line_map lookup though I do not recall why.  I originally used 
> UNKNOWN_LOCATION for cached values but that did not work as it confused
> DECL_IS_BUILTIN.
> 
> We could extend API by adding INVALID_LOCATION and set it to INT_MAX or something
> that would also ICE.
> 
> Honza
> > 
> > Richard.
> > 
> > >    file_change = bp_unpack_value (bp, 1);
> > > 
> > > 
> > 
> > -- 
> > Richard Biener <rguenther@suse.de>
> > SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Dilip Upmanyu, Graham Norton, HRB 21284 (AG Nuernberg)
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Dilip Upmanyu, Graham Norton, HRB 21284 (AG Nuernberg)

Re: Fix LTO streaming of BUILTINS_LOCATION

[Jan Hubicka]

> 
> Yeah, I think streaming all locations up to RESERVED_LOCATION_COUNT
> literally is more robust.  Thus do
> 
>   bp_pack_int_in_range (bp, 0, RESERVED_LOCATION_COUNT,
> 			loc < RESERVED_LOCATION_COUNT ? loc : 
> RESERVED_LOCATION_COUNT);
>   if (loc < RESERVED_LOCATION_COUNT)
>     return;

Yep, I did that in meantime (did not notice we have RESERVED_LOCATION_COUNT)
This is what I commited.

2015-06-08  Jan Hubicka  <hubicka@ucw.cz>

	* lto-streamer-out.c (lto_output_location): Stream
	reserved locations correctly.
	* lto-streamer-in.c (lto_output_location): Likewise.
Index: lto-streamer-out.c
===================================================================
--- lto-streamer-out.c	(revision 224248)
+++ lto-streamer-out.c	(working copy)
@@ -202,8 +202,10 @@
   expanded_location xloc;
 
   loc = LOCATION_LOCUS (loc);
-  bp_pack_value (bp, loc == UNKNOWN_LOCATION, 1);
-  if (loc == UNKNOWN_LOCATION)
+  bp_pack_int_in_range (bp, 0, RESERVED_LOCATION_COUNT,
+		        loc < RESERVED_LOCATION_COUNT
+			? loc : RESERVED_LOCATION_COUNT);
+  if (loc < RESERVED_LOCATION_COUNT)
     return;
 
   xloc = expand_location (loc);
Index: lto-streamer-in.c
===================================================================
--- lto-streamer-in.c	(revision 224248)
+++ lto-streamer-in.c	(working copy)
@@ -278,13 +278,14 @@
 
   gcc_assert (current_cache == this);
 
-  if (bp_unpack_value (bp, 1))
-    {
-      *loc = UNKNOWN_LOCATION;
-      return;
-    }
-  *loc = BUILTINS_LOCATION + 1;
+  *loc = bp_unpack_int_in_range (bp, "location", 0, RESERVED_LOCATION_COUNT);
 
+  if (*loc < RESERVED_LOCATION_COUNT)
+    return;
+
+  /* Keep value RESERVED_LOCATION_COUNT in *loc as linemap lookups will
+     ICE on it.  */
+
   file_change = bp_unpack_value (bp, 1);
   line_change = bp_unpack_value (bp, 1);
   column_change = bp_unpack_value (bp, 1);