| Message ID | CAO2gOZXygjcsWpzem4DNzk_w54u-kLR6H53YiKqazVx797Uw9Q@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
On Fri, 21 Sep 2012, Dehao Chen wrote: > This patch moves location_adhoc_data into GC, and also rebuild the > hash table when reading in the PCH. After the patch, PCH can work as > expected. > > Bootstrapped and passed gcc regression tests on x8664_linux. If you have a moment to consider improvements for the test-instructions at <http://gcc.gnu.org/contribute.html#testing> to try and avoid this situation (introducing regressions), that'd be nice; IIUC it wasn't clear enough that the "make check" must be at the top tree? It seems to say so but maybe that's somehow in a blind spot. > > OK for trunk? > > Thanks, > Dehao > > libcpp/ChangeLog: > 2012-09-21 Dehao Chen <dehao@google.com> > > PR middle-end/54645 > * include/line-map.h (location_adhoc_data): Move location_adhoc_data > into GC. > (location_adhoc_data_map): Likewise. > (line_maps): Likewise. > (rebuild_location_adhoc_htab): New Function. > * line-map.c (+rebuild_location_adhoc_htab): new Funcion. > (get_combined_adhoc_loc): Move location_adhoc_data into GC. > (location_adhoc_data_fini): Likewise. > (linemap_init): Likewise. > (location_adhoc_data_init): Remove Function. > > gcc/ChangeLog: > 2012-09-21 Dehao Chen <dehao@google.com> > > PR middle-end/54645 > * c-family/c-pch.c (c_common_read_pch): Rebuild the location_adhoc_data > map when read in the pch. I can't say anything insightful about this patch other than the nitpick below (i.e. I see nothing wrong with it) but I'd encourage a proper review of it to resolve the PCH regressions. There's no PCH section in MAINTAINERS, but "next-of-kin" global maintainers CC:ed. I have just a nitpicking remark: please break the overlong lines. I noticed the ones with htab_create calls as my viewer helpfully broke the lines at 80 columns at the last comma. brgds, H-P
On Mon, Sep 24, 2012 at 10:23 PM, Hans-Peter Nilsson <hp@bitrange.com> wrote: > On Fri, 21 Sep 2012, Dehao Chen wrote: >> This patch moves location_adhoc_data into GC, and also rebuild the >> hash table when reading in the PCH. After the patch, PCH can work as >> expected. >> >> Bootstrapped and passed gcc regression tests on x8664_linux. > > If you have a moment to consider improvements for the > test-instructions at > <http://gcc.gnu.org/contribute.html#testing> to try and avoid > this situation (introducing regressions), that'd be nice; IIUC > it wasn't clear enough that the "make check" must be at the top > tree? It seems to say so but maybe that's somehow in a blind > spot. > >> >> OK for trunk? >> >> Thanks, >> Dehao >> >> libcpp/ChangeLog: >> 2012-09-21 Dehao Chen <dehao@google.com> >> >> PR middle-end/54645 >> * include/line-map.h (location_adhoc_data): Move location_adhoc_data >> into GC. >> (location_adhoc_data_map): Likewise. >> (line_maps): Likewise. >> (rebuild_location_adhoc_htab): New Function. >> * line-map.c (+rebuild_location_adhoc_htab): new Funcion. >> (get_combined_adhoc_loc): Move location_adhoc_data into GC. >> (location_adhoc_data_fini): Likewise. >> (linemap_init): Likewise. >> (location_adhoc_data_init): Remove Function. >> >> gcc/ChangeLog: >> 2012-09-21 Dehao Chen <dehao@google.com> >> >> PR middle-end/54645 >> * c-family/c-pch.c (c_common_read_pch): Rebuild the location_adhoc_data >> map when read in the pch. > > > I can't say anything insightful about this patch other than the > nitpick below (i.e. I see nothing wrong with it) but I'd > encourage a proper review of it to resolve the PCH regressions. > There's no PCH section in MAINTAINERS, but "next-of-kin" global > maintainers CC:ed. IMHO the PCH implementation is awkward and that is enough of a reason to remove PCH in its current form. Richard. > I have just a nitpicking remark: please break the overlong > lines. I noticed the ones with htab_create calls as my viewer > helpfully broke the lines at 80 columns at the last comma. > > brgds, H-P
> gcc/ChangeLog: > 2012-09-21 Dehao Chen <dehao@google.com> > > PR middle-end/54645 > * c-family/c-pch.c (c_common_read_pch): Rebuild the location_adhoc_data > map when read in the pch. Wrong ChangeLog file, you want gcc/c-family/ChangeLog (and remove c-family/).
Sorry, I'll change that with my next patch. Dehao On Wed, Sep 26, 2012 at 12:20 AM, Eric Botcazou <ebotcazou@adacore.com> wrote: >> gcc/ChangeLog: >> 2012-09-21 Dehao Chen <dehao@google.com> >> >> PR middle-end/54645 >> * c-family/c-pch.c (c_common_read_pch): Rebuild the location_adhoc_data >> map when read in the pch. > > Wrong ChangeLog file, you want gcc/c-family/ChangeLog (and remove c-family/). > > -- > Eric Botcazou
Index: gcc/c-family/c-pch.c =================================================================== --- gcc/c-family/c-pch.c (revision 191618) +++ gcc/c-family/c-pch.c (working copy) @@ -340,6 +340,7 @@ c_common_read_pch (cpp_reader *pfile, const char * gt_pch_restore (f); cpp_set_line_map (pfile, line_table); + rebuild_location_adhoc_htab (line_table); timevar_push (TV_PCH_CPP_RESTORE); if (cpp_read_state (pfile, name, f, smd) != 0) Index: libcpp/include/line-map.h =================================================================== --- libcpp/include/line-map.h (revision 191618) +++ libcpp/include/line-map.h (working copy) @@ -260,9 +260,9 @@ struct GTY(()) maps_info { }; /* Data structure to associate an arbitrary data to a source location. */ -struct location_adhoc_data { +struct GTY(()) location_adhoc_data { source_location locus; - void *data; + void * GTY((skip)) data; }; struct htab; @@ -277,11 +277,11 @@ struct htab; bits of the integer is used to index the location_adhoc_data array, in which the locus and associated data is stored. */ -struct location_adhoc_data_map { - struct htab *htab; +struct GTY(()) location_adhoc_data_map { + struct htab * GTY((skip)) htab; source_location curr_loc; - struct location_adhoc_data *data; unsigned int allocated; + struct location_adhoc_data GTY((length ("%h.allocated"))) *data; }; /* A set of chronological line_map structures. */ @@ -315,7 +315,7 @@ struct GTY(()) line_maps { allocated, for a certain allocation size requested. */ line_map_round_alloc_size_func round_alloc_size; - struct location_adhoc_data_map GTY((skip)) location_adhoc_data_map; + struct location_adhoc_data_map location_adhoc_data_map; }; /* Returns the pointer to the memory region where information about @@ -446,6 +446,8 @@ extern source_location get_location_from_adhoc_loc #define COMBINE_LOCATION_DATA(SET, LOC, BLOCK) \ get_combined_adhoc_loc ((SET), (LOC), (BLOCK)) +extern void rebuild_location_adhoc_htab (struct line_maps *); + /* Initialize a line map set. */ extern void linemap_init (struct line_maps *); Index: libcpp/line-map.c =================================================================== --- libcpp/line-map.c (revision 191618) +++ libcpp/line-map.c (working copy) @@ -82,6 +82,19 @@ location_adhoc_data_update (void **slot, void *dat return 1; } +/* Rebuild the hash table from the location adhoc data. */ + +void +rebuild_location_adhoc_htab (struct line_maps *set) +{ + unsigned i; + set->location_adhoc_data_map.htab = + htab_create (100, location_adhoc_data_hash, location_adhoc_data_eq, NULL); + for (i = 0; i < set->location_adhoc_data_map.curr_loc; i++) + htab_find_slot (set->location_adhoc_data_map.htab, + set->location_adhoc_data_map.data + i, INSERT); +} + /* Combine LOCUS and DATA to a combined adhoc loc. */ source_location @@ -109,14 +122,21 @@ get_combined_adhoc_loc (struct line_maps *set, { char *orig_data = (char *) set->location_adhoc_data_map.data; long long offset; - set->location_adhoc_data_map.allocated *= 2; - set->location_adhoc_data_map.data = - XRESIZEVEC (struct location_adhoc_data, - set->location_adhoc_data_map.data, - set->location_adhoc_data_map.allocated); + line_map_realloc reallocator + = set->reallocator ? set->reallocator : xrealloc; + + if (set->location_adhoc_data_map.allocated == 0) + set->location_adhoc_data_map.allocated = 128; + else + set->location_adhoc_data_map.allocated *= 2; + set->location_adhoc_data_map.data = (struct location_adhoc_data *) + reallocator (set->location_adhoc_data_map.data, + set->location_adhoc_data_map.allocated + * sizeof (struct location_adhoc_data)); offset = (char *) (set->location_adhoc_data_map.data) - orig_data; - htab_traverse (set->location_adhoc_data_map.htab, - location_adhoc_data_update, &offset); + if (set->location_adhoc_data_map.allocated > 128) + htab_traverse (set->location_adhoc_data_map.htab, + location_adhoc_data_update, &offset); } *slot = set->location_adhoc_data_map.data + set->location_adhoc_data_map.curr_loc; @@ -144,24 +164,10 @@ get_location_from_adhoc_loc (struct line_maps *set return set->location_adhoc_data_map.data[loc & MAX_SOURCE_LOCATION].locus; } -/* Initialize the location_adhoc_data structure. */ - -static void -location_adhoc_data_init (struct line_maps *set) -{ - set->location_adhoc_data_map.htab = - htab_create (100, location_adhoc_data_hash, location_adhoc_data_eq, NULL); - set->location_adhoc_data_map.curr_loc = 0; - set->location_adhoc_data_map.allocated = 100; - set->location_adhoc_data_map.data = XNEWVEC (struct location_adhoc_data, 100); -} - /* Finalize the location_adhoc_data structure. */ void location_adhoc_data_fini (struct line_maps *set) { - set->location_adhoc_data_map.allocated = 0; - XDELETEVEC (set->location_adhoc_data_map.data); htab_delete (set->location_adhoc_data_map.htab); } @@ -173,7 +179,8 @@ linemap_init (struct line_maps *set) memset (set, 0, sizeof (struct line_maps)); set->highest_location = RESERVED_LOCATION_COUNT - 1; set->highest_line = RESERVED_LOCATION_COUNT - 1; - location_adhoc_data_init (set); + set->location_adhoc_data_map.htab = + htab_create (100, location_adhoc_data_hash, location_adhoc_data_eq, NULL); } /* Check for and warn about line_maps entered but not exited. */