Re: PING^1: [PATCH] Set start_location to 0 if we ran out of line map space
On Fri, Aug 24, 2018 at 4:05 PM, David Malcolm wrote: > On Wed, 2018-08-22 at 12:48 -0700, H.J. Lu wrote: >> PING. > > [stripping out-of-date address for Tom Tromey from recipients] > > Sorry for the belated response. > >> -- Forwarded message -- >> From: H.J. Lu >> Date: Wed, Aug 15, 2018 at 4:33 AM >> Subject: [PATCH] Set start_location to 0 if we ran out of line map >> space >> To: gcc-patches@gcc.gnu.org >> >> >> With profiledbootstrap and --with-build-config=bootstrap-lto, >> linemap_add >> may create a macro map when we run out of line map space. This patch >> changes start_location to UNKNOWN_LOCATION (0) in this case. >> >> Tested with profiledbootstrap and --with-build-config=bootstrap-lto >> on >> Linux/x86-64. >> >> PR bootstrap/86872 >> * line-map.c (pure_location_p): Return true if linemap_lookup >> returns NULL. >> (linemap_add): Set start_location to 0 if we run out of line >> map >> space. >> --- >> libcpp/line-map.c | 7 +++ >> 1 file changed, 7 insertions(+) >> >> diff --git a/libcpp/line-map.c b/libcpp/line-map.c >> index 555cd129a9c..cafe42273eb 100644 >> --- a/libcpp/line-map.c >> +++ b/libcpp/line-map.c >> @@ -304,6 +304,8 @@ pure_location_p (line_maps *set, source_location >> loc) >> return false; >> >>const line_map *map = linemap_lookup (set, loc); >> + if (map == NULL) >> +return true; >>const line_map_ordinary *ordmap = linemap_check_ordinary (map); > > I was wondering why this is needed, but it's presumably due to this > clause: > > if (set == NULL || line < RESERVED_LOCATION_COUNT) > return NULL; > > in linemap_ordinary_map_lookup. and /* Any ordinary locations ought to be "pure" at this point: no compressed ranges. */ linemap_assert (locus < RESERVED_LOCATION_COUNT || locus >= LINE_MAP_MAX_LOCATION_WITH_PACKED_RANGES || locus >= LINEMAPS_MACRO_LOWEST_LOCATION (set) || pure_location_p (set, locus)); /* Consider short-range optimization. */ if (can_be_stored_compactly_p (set, locus, src_range, data)) { /* The low bits ought to be clear. */ linemap_assert (pure_location_p (set, locus)); > Normally, the ordinary_maps are monotonically increasing by start > location, but with the next hunk to linemap_add there will be a map at > the end for location 0 which thus isn't monotonic. > >>if (loc & ((1U << ordmap->m_range_bits) - 1)) >> @@ -492,6 +494,11 @@ linemap_add (struct line_maps *set, enum >> lc_reason reason, >> } >> >>linemap_assert (reason != LC_ENTER_MACRO); >> + >> + if (start_location >= LINE_MAP_MAX_LOCATION) >> +/* We ran out of line map space. */ >> +start_location = 0; >> + > > If I'm reading this right, this means that we create an ordinary map > that starts at location 0. The caller, linemap_line_start should then > bail out, returning location 0, and subsequent calls to > linemap_line_start should return 0 without attempting to create another > map. > > My first thought was that instead linemap_add ought to return NULL for > this case, and linemap_line_start should check for NULL return and bail > out, but I believe this approach would mean doing slightly more work > per line. Out of interest, did you try this other approach? Yes, I did. But it didn't work since callers of inemap_add expect non-NULL return value for this case. > I wonder if there's a sane way to reproduce this failure using > gcc.dg/plugin/location_overflow_plugin.c ? > >>line_map_ordinary *map >> = linemap_check_ordinary (new_linemap (set, start_location)); >>map->reason = reason; >> -- >> 2.17.1 > > Given the testing this has had, and the awkwardness of reproducing, > this is OK for trunk as-is. > > Thanks > Dave > I will check it in. Thanks. -- H.J.
Re: PING^1: [PATCH] Set start_location to 0 if we ran out of line map space
On Wed, 2018-08-22 at 12:48 -0700, H.J. Lu wrote: > PING. [stripping out-of-date address for Tom Tromey from recipients] Sorry for the belated response. > -- Forwarded message -- > From: H.J. Lu > Date: Wed, Aug 15, 2018 at 4:33 AM > Subject: [PATCH] Set start_location to 0 if we ran out of line map > space > To: gcc-patches@gcc.gnu.org > > > With profiledbootstrap and --with-build-config=bootstrap-lto, > linemap_add > may create a macro map when we run out of line map space. This patch > changes start_location to UNKNOWN_LOCATION (0) in this case. > > Tested with profiledbootstrap and --with-build-config=bootstrap-lto > on > Linux/x86-64. > > PR bootstrap/86872 > * line-map.c (pure_location_p): Return true if linemap_lookup > returns NULL. > (linemap_add): Set start_location to 0 if we run out of line > map > space. > --- > libcpp/line-map.c | 7 +++ > 1 file changed, 7 insertions(+) > > diff --git a/libcpp/line-map.c b/libcpp/line-map.c > index 555cd129a9c..cafe42273eb 100644 > --- a/libcpp/line-map.c > +++ b/libcpp/line-map.c > @@ -304,6 +304,8 @@ pure_location_p (line_maps *set, source_location > loc) > return false; > >const line_map *map = linemap_lookup (set, loc); > + if (map == NULL) > +return true; >const line_map_ordinary *ordmap = linemap_check_ordinary (map); I was wondering why this is needed, but it's presumably due to this clause: if (set == NULL || line < RESERVED_LOCATION_COUNT) return NULL; in linemap_ordinary_map_lookup. Normally, the ordinary_maps are monotonically increasing by start location, but with the next hunk to linemap_add there will be a map at the end for location 0 which thus isn't monotonic. >if (loc & ((1U << ordmap->m_range_bits) - 1)) > @@ -492,6 +494,11 @@ linemap_add (struct line_maps *set, enum > lc_reason reason, > } > >linemap_assert (reason != LC_ENTER_MACRO); > + > + if (start_location >= LINE_MAP_MAX_LOCATION) > +/* We ran out of line map space. */ > +start_location = 0; > + If I'm reading this right, this means that we create an ordinary map that starts at location 0. The caller, linemap_line_start should then bail out, returning location 0, and subsequent calls to linemap_line_start should return 0 without attempting to create another map. My first thought was that instead linemap_add ought to return NULL for this case, and linemap_line_start should check for NULL return and bail out, but I believe this approach would mean doing slightly more work per line. Out of interest, did you try this other approach? I wonder if there's a sane way to reproduce this failure using gcc.dg/plugin/location_overflow_plugin.c ? >line_map_ordinary *map > = linemap_check_ordinary (new_linemap (set, start_location)); >map->reason = reason; > -- > 2.17.1 Given the testing this has had, and the awkwardness of reproducing, this is OK for trunk as-is. Thanks Dave
PING^1: [PATCH] Set start_location to 0 if we ran out of line map space
PING. -- Forwarded message -- From: H.J. Lu Date: Wed, Aug 15, 2018 at 4:33 AM Subject: [PATCH] Set start_location to 0 if we ran out of line map space To: gcc-patches@gcc.gnu.org With profiledbootstrap and --with-build-config=bootstrap-lto, linemap_add may create a macro map when we run out of line map space. This patch changes start_location to UNKNOWN_LOCATION (0) in this case. Tested with profiledbootstrap and --with-build-config=bootstrap-lto on Linux/x86-64. PR bootstrap/86872 * line-map.c (pure_location_p): Return true if linemap_lookup returns NULL. (linemap_add): Set start_location to 0 if we run out of line map space. --- libcpp/line-map.c | 7 +++ 1 file changed, 7 insertions(+) diff --git a/libcpp/line-map.c b/libcpp/line-map.c index 555cd129a9c..cafe42273eb 100644 --- a/libcpp/line-map.c +++ b/libcpp/line-map.c @@ -304,6 +304,8 @@ pure_location_p (line_maps *set, source_location loc) return false; const line_map *map = linemap_lookup (set, loc); + if (map == NULL) +return true; const line_map_ordinary *ordmap = linemap_check_ordinary (map); if (loc & ((1U << ordmap->m_range_bits) - 1)) @@ -492,6 +494,11 @@ linemap_add (struct line_maps *set, enum lc_reason reason, } linemap_assert (reason != LC_ENTER_MACRO); + + if (start_location >= LINE_MAP_MAX_LOCATION) +/* We ran out of line map space. */ +start_location = 0; + line_map_ordinary *map = linemap_check_ordinary (new_linemap (set, start_location)); map->reason = reason; -- 2.17.1 -- H.J.