[Bug lto/65536] LTO line number information garbled
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65536 --- Comment #55 from Jan Hubicka --- > Can the bug be marked as resolved? I think with the location cache we only made this problem less visible and for really large programs linemap still can overflow and behave funy, right?
[Bug lto/65536] LTO line number information garbled
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65536 --- Comment #54 from Martin Liška --- Can the bug be marked as resolved?
[Bug lto/65536] LTO line number information garbled
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65536 --- Comment #53 from Jan Hubicka --- > You can get an estimate of how much memory would be required to stream in/out > directly the line_table by summing up the memory reported by > dump_line_table_statistics for each TU before streaming out (perhaps using > -ftrack-macro-expansion=0 to reduce it further). This would still be an > overestimate, because one can drop one char (sysp) and one int > (included_from) > per map and one can drop all maps that are not used by LTO. > Moreover, you will > not need a cache and everything will be in order already > when you stream in. I see, the stats actually are in -fmem-report, how convenient ;)) With Firefox I get: Line Table allocations during the compilation process Number of ordinary maps used: 407k Ordinary map used size: 15M Number of ordinary maps allocated: 409k Ordinary maps allocated size: 15M Number of macro maps used: 0 Macro maps used size:0 Macro maps locations size: 0 Macro maps size: 0 Duplicated maps locations size: 0 Total allocated maps size: 15M Total used maps size: 15M after streaming in all declarations&types and Line Table allocations during the compilation process Number of ordinary maps used: 769k Ordinary map used size: 30M Number of ordinary maps allocated:1638k Ordinary maps allocated size: 63M Number of macro maps used: 0 Macro maps used size:0 Macro maps locations size: 0 Macro maps size: 0 Duplicated maps locations size: 0 Total allocated maps size: 63M Total used maps size: 30M by end of WPA stage. The extra growth is by variable initializers and function bodies we bring in on demand (for merging, ICF, devirt machinery). Those by large part bypass my cache. Individual ltrans units consume after streaming in global decls: Line Table allocations during the compilation process Number of ordinary maps used: 29k Ordinary map used size: 1189k Number of ordinary maps allocated: 102k Ordinary maps allocated size: 4095k Number of macro maps used: 0 Macro maps used size:0 Macro maps locations size: 0 Macro maps size: 0 Duplicated maps locations size: 0 Total allocated maps size:4095k Total used maps size: 1189k this does not look terrible I tried to sum the sizes for EON, and got to 92493kB and 8631 without macro expansion tracking (note that this size is basically the same with or without my proposed line-maps.c patch, so it does not seem to pesimize non-LTO builds). At WPA time (with my proposed patch, will rebuild tree without after lunch) I get: Line Table allocations during the compilation process Number of ordinary maps used: 6084 Ordinary map used size:237k Number of ordinary maps allocated:6553 Ordinary maps allocated size: 255k Number of macro maps used: 0 Macro maps used size:0 Macro maps locations size: 0 Macro maps size: 0 Duplicated maps locations size: 0 Total allocated maps size: 255k Total used maps size: 237k So about 30x smaller, even though we do have information loss here (i.e. inline stacks) So it seems we can not really directly stream linemaps - there is way too much of information that is discarded from parsing time to LTO time. (about 95% of trees that are streamed out are discarded by tree merging and thus with my cache never hits linemaps). Linemap representation is interesting. I suppose for next release we want to do kind of on-disk variant inspired by libcpp. If we arrange cache to be per-LTO-section (not randomly flushed as it is now), we can read the on-disk format into LTO own format and apply relevant part to the line-maps after tree merging.
[Bug lto/65536] LTO line number information garbled
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65536 --- Comment #52 from Manuel López-Ibáñez --- (In reply to Jan Hubicka from comment #50) > > > > +/* Do not track column numbers higher than this one. As a result, the > > + range of column_bits is [7, 18] (or 0 if column numbers are > > + disabled). */ > > +#define LINE_MAP_MAX_COLUMN_NUMBER (1U << 17) > > + > > +/* Do not track column numbers if locations get higher than this. */ > > +#define LINE_MAP_MAX_LOCATION_WITH_COLS 0x7000 > > + > > +/* Highest possible source location encoded within an ordinary or > > + macro map. */ > > +#define LINE_MAP_MAX_SOURCE_LOCATION 0x7FF0 > > I understood the limit of 0x7000 to give some buffer so we can still > record new > files but also drop the line number information? Do we really need to reserve space for 0x7FF0 - 0x7000 = 268435440 files? > Well, this is a bug in C/C++ FEs clearly, lang_type should not be declared > twice with different meanings. One needs to be renamed. You can however > safely > ignore it and get build to complete with --disable-werror I'll try that. Thanks! > I will try to add assets and reproduce it (at least with cache disabled on > gcc > bootstrap), lets see if I get any luck. I only saw it on Chromium that is > order of mangnitude bigger than GCC. Just to give you idea, GCC linktime > memory consumption is about 700MB, while Firefox takes 3GB and Chromium 9GB. > It has 11k files linked to one unit (which makes is so much bigger than > firefox > that does include many .cpp units into one to get around the bloat). > Before my changes, the linemap occupied about 1GB. You can get an estimate of how much memory would be required to stream in/out directly the line_table by summing up the memory reported by dump_line_table_statistics for each TU before streaming out (perhaps using -ftrack-macro-expansion=0 to reduce it further). This would still be an overestimate, because one can drop one char (sysp) and one int (included_from) per map and one can drop all maps that are not used by LTO. Moreover, you will not need a cache and everything will be in order already when you stream in.
[Bug lto/65536] LTO line number information garbled
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65536 --- Comment #51 from Jan Hubicka --- > > Contrary to what I said before, I think now that it really makes sense for > line-maps to return UNKNOWN_LOCATION rather than the location of something > else > when overflow occurs, but then LTO has to detect this case and decide what to > do. This would affect not only LTO code - all C++ FE's can face an overflow in exterme cases and becuase we special case everyting <=BUILTINS_LOCATION perhaps we really can just add extra special value WRONG_LOCATION that would be used to denote this? Honza
[Bug lto/65536] LTO line number information garbled
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65536 --- Comment #50 from Jan Hubicka --- > > +/* Do not track column numbers higher than this one. As a result, the > + range of column_bits is [7, 18] (or 0 if column numbers are > + disabled). */ > +#define LINE_MAP_MAX_COLUMN_NUMBER (1U << 17) > + > +/* Do not track column numbers if locations get higher than this. */ > +#define LINE_MAP_MAX_LOCATION_WITH_COLS 0x7000 > + > +/* Highest possible source location encoded within an ordinary or > + macro map. */ > +#define LINE_MAP_MAX_SOURCE_LOCATION 0x7FF0 I understood the limit of 0x7000 to give some buffer so we can still record new files but also drop the line number information? > -I/home/manuel/test1/221728/build/prev-x86_64-unknown-linux-gnu/libstdc++-v3/include > -I/home/manuel/test1/pristine/libstdc++-v3/libsupc++ > -L/home/manuel/test1/221728/build/prev-x86_64-unknown-linux-gnu/libstdc++-v3/src/.libs > -L/home/manuel/test1/221728/build/prev-x86_64-unknown-linux-gnu/libstdc++-v3/libsupc++/.libs > -g -O2 -flto=jobserver -frandom-seed=1 -DIN_GCC-fno-exceptions -fno-rtti > -fasynchronous-unwind-tables -W -Wall -Wno-narrowing -Wwrite-strings > -Wcast-qual -Wmissing-format-attribute -Woverloaded-virtual -pedantic > -Wno-long-long -Wno-variadic-macros -Wno-overlength-strings -Werror > -fno-common > -DHAVE_CONFIG_H -static-libstdc++ -static-libgcc -o cc1 c/c-lang.o > c-family/stub-objc.o attribs.o c/c-errors.o c/c-decl.o c/c-typeck.o > c/c-convert.o c/c-aux-info.o c/c-objc-common.o c/c-parser.o > c/c-array-notation.o c-family/c-common.o c-family/c-cppbuiltin.o > c-family/c-dump.o c-family/c-format.o c-family/c-gimplify.o c-family/c-lex.o > c-family/c-omp.o c-family/c-opts.o c-family/c-pch.o c-family/c-ppoutput.o > c-family/c-pragma.o c-family/c-pretty-print.o c-family/c-semantics.o > c-family/c-ada-spec.o c-family/c-cilkplus.o c-family/array-notation-common.o > c-family/cilk.o c-family/c-ubsan.o i386-c.o glibc-c.o \ > cc1-checksum.o libbackend.a main.o tree-browser.o libcommon-target.a > libcommon.a ../libcpp/libcpp.a ../libdecnumber/libdecnumber.a libcommon.a > ../libcpp/libcpp.a ../libbacktrace/.libs/libbacktrace.a > ../libiberty/libiberty.a ../libdecnumber/libdecnumber.a > -L/opt/cfarm/isl-0.12.2/lib -lisl -L/opt/cfarm/gmp-4.3.2/lib > -L/opt/cfarm/mpfr-2.4.2/lib -L/opt/cfarm/mpc-0.8.1/lib -lmpc -lmpfr -lgmp > -rdynamic -ldl -L../zlib -lz > /home/manuel/test1/pristine/gcc/c/c-lang.h:26:16: error: type ???struct > lang_type??? violates one definition rule [-Werror=odr] > struct GTY(()) lang_type { > ^ > /home/manuel/test1/pristine/gcc/cp/cp-tree.h:1566:16: note: a different type > is > defined in another translation unit > struct GTY(()) lang_type { > ^ > /home/manuel/test1/pristine/gcc/c/c-lang.h:28:72: note: the first difference > of > corresponding definitions is field ???s??? >struct sorted_fields_type * GTY ((reorder ("resort_sorted_fields"))) s; > ^ > /home/manuel/test1/pristine/gcc/cp/cp-tree.h:1572:45: note: a field with > different name is defined in another translation unit >} GTY((desc ("%h.h.is_lang_type_class"))) u; > ^ Well, this is a bug in C/C++ FEs clearly, lang_type should not be declared twice with different meanings. One needs to be renamed. You can however safely ignore it and get build to complete with --disable-werror > > > 2) there is a bug that we sometimes output wrong line number. I think it is > > caused by the logic reallocating ORDINARY_MAP_NUMBER_OF_COLUMN_BITS as > > described in original email. > > Maybe but I do not see it yet. My guess is that it is caused either by > overflowing start_location in linemap_add, or when computing 'r' > linemap_position_for_column after linemap_line_start starts returning 0 or > after column_hint has been truncated (your patch fixes this), or when the > computation of 'r' overflows in linemap_line_start. Any of those cases will > trigger a silent overflow and return a location for something else. I will try to add assets and reproduce it (at least with cache disabled on gcc bootstrap), lets see if I get any luck. I only saw it on Chromium that is order of mangnitude bigger than GCC. Just to give you idea, GCC linktime memory consumption is about 700MB, while Firefox takes 3GB and Chromium 9GB. It has 11k files linked to one unit (which makes is so much bigger than firefox that does include many .cpp units into one to get around the bloat). Before my changes, the linemap occupied about 1GB. > > I do not think typical usage is pesimized here. Yes, code is trying to avoid > > case where we skip too many location indexes because we entered lines 1...30 > > and now we want to insert line 1000. We do not want to fast forward by > > 970*80. > > Yes, we want to. Because then set->highest_location goes up just by one and > the > next
[Bug lto/65536] LTO line number information garbled
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65536 --- Comment #49 from Manuel López-Ibáñez --- (In reply to Jan Hubicka from comment #46) > Manuel, > I will hopefully commit the cache patch today or tomorrow morning. It does > not solve full issue. What we have is > 1) we still drop columns for firefox&chromium pretty early I think the current limits are too conservative. In particular, the limit in linemap_position_for_column does not make sense to me. I would suggest trying something like: Index: line-map.c === --- line-map.c (revision 221728) +++ line-map.c (working copy) @@ -24,10 +24,22 @@ along with this program; see the file CO #include "line-map.h" #include "cpplib.h" #include "internal.h" #include "hashtab.h" +/* Do not track column numbers higher than this one. As a result, the + range of column_bits is [7, 18] (or 0 if column numbers are + disabled). */ +#define LINE_MAP_MAX_COLUMN_NUMBER (1U << 17) + +/* Do not track column numbers if locations get higher than this. */ +#define LINE_MAP_MAX_LOCATION_WITH_COLS 0x7000 + +/* Highest possible source location encoded within an ordinary or + macro map. */ +#define LINE_MAP_MAX_SOURCE_LOCATION 0x7FF0 + static void trace_include (const struct line_maps *, const struct line_map *); static const struct line_map * linemap_ordinary_map_lookup (struct line_maps *, source_location); static const struct line_map* linemap_macro_map_lookup (struct line_maps *, source_location); @@ -528,26 +543,28 @@ linemap_line_start (struct line_maps *se || (line_delta > 10 && line_delta * ORDINARY_MAP_NUMBER_OF_COLUMN_BITS (map) > 1000) || (max_column_hint >= (1U << ORDINARY_MAP_NUMBER_OF_COLUMN_BITS (map))) || (max_column_hint <= 80 && ORDINARY_MAP_NUMBER_OF_COLUMN_BITS (map) >= 10) - || (highest > 0x6000 - && (set->max_column_hint || highest > 0x7000))) + || (highest > LINE_MAP_MAX_LOCATION_WITH_COLS + && (set->max_column_hint || highest >= LINE_MAP_MAX_SOURCE_LOCATION))) add_map = true; else max_column_hint = set->max_column_hint; if (add_map) { int column_bits; - if (max_column_hint > 10 || highest > 0x6000) + if (max_column_hint > LINE_MAP_MAX_COLUMN_NUMBER + || highest > LINE_MAP_MAX_LOCATION_WITH_COLS) { /* If the column number is ridiculous or we've allocated a huge number of source_locations, give up on column numbers. */ max_column_hint = 0; - if (highest > 0x7000) - return 0; column_bits = 0; + + if (set->highest_location >= LINEMAPS_MACRO_LOWEST_LOCATION (set) - 1) + return 0; } else { column_bits = 7; while (max_column_hint >= (1U << column_bits)) @@ -598,19 +628,25 @@ linemap_position_for_column (struct line linemap_assert (!linemap_macro_expansion_map_p (LINEMAPS_LAST_ORDINARY_MAP (set))); if (to_column >= set->max_column_hint) { - if (r >= 0xC00 || to_column > 10) + if (r > LINE_MAP_MAX_LOCATION_WITH_COLS + || to_column > LINE_MAP_MAX_COLUMN_NUMBER) { /* Running low on source_locations - disable column numbers. */ + if (r >= LINEMAPS_MACRO_LOWEST_LOCATION (set) - 1) + return 0; return r; } else { struct line_map *map = LINEMAPS_LAST_ORDINARY_MAP (set); r = linemap_line_start (set, SOURCE_LINE (map, r), to_column + 50); + /* We just got to overflow; disable column numbers. */ + if (to_column >= set->max_column_hint) + return r; } } r = r + to_column; if (r >= set->highest_location) set->highest_location = r; Unfortunately, I cannot get a pristine lto-boostrap to succeed, it fails with: /home/manuel/test1/221728/build/./prev-gcc/xg++ -B/home/manuel/test1/221728/build/./prev-gcc/ -B/home/manuel/test1/./221728/install/x86_64-unknown-linux-gnu/bin/ -nostdinc++ -B/home/manuel/test1/221728/build/prev-x86_64-unknown-linux-gnu/libstdc++-v3/src/.libs -B/home/manuel/test1/221728/build/prev-x86_64-unknown-linux-gnu/libstdc++-v3/libsupc++/.libs -I/home/manuel/test1/221728/build/prev-x86_64-unknown-linux-gnu/libstdc++-v3/include/x86_64-unknown-linux-gnu -I/home/manuel/test1/221728/build/prev-x86_64-unknown-linux-gnu/libstdc++-v3/include -I/home/manuel/test1/pristine/libstdc++-v3/libsupc++ -L/home/manuel/test1/221728/build/prev-x86_64-unknown-linux-gnu/libstdc++-v3/src/.libs -L/home/manuel/test1/221728/build/prev-x86_64-unknown-linux-gnu/libstdc++-v3/libsupc++/.libs -g -O2 -flto=jobserver -frandom-seed=1 -DIN_GCC-fno-exceptions -fno-rtti -fasynchronous-unwind-tables -W -Wall -Wno-narrowing -Wwrite-strings -Wcast-qu
[Bug lto/65536] LTO line number information garbled
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65536 --- Comment #48 from Jan Hubicka --- I run memory statistics with the cache and my patch. I will run stats with cache w/o the libcpp patch tomorrow. I would like to get this problem solved, but perhaps we want to only track down the reason for wrong line numbers and do more tuning next stage1 & backport. I suppose I can implement checking that will lookup location after every insert to double check lines are not garbled. Dropped columns are I guess acceptable (it would be nice to drop the wrong carrets then) So for memory use: realloc_for_line_map is now at 1681: 0.9% of GGC memory use pre-IPA. This is definitly improvement. Gimple streaming however push it up to 75497496:4.2% I guess this suggest that caching blocks and gimple stmt locators would make a lot of difference. ltrans report 4194328: 5.4% post main decl streaming, but before bodies are read in.
[Bug lto/65536] LTO line number information garbled
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65536 --- Comment #47 from Jan Hubicka --- Author: hubicka Date: Fri Mar 27 06:58:59 2015 New Revision: 221720 URL: https://gcc.gnu.org/viewcvs?rev=221720&root=gcc&view=rev Log: PR lto/65536 * lto-streamer.h (class lto_location_cache): New. (struct data_in): Add location_cache. (lto_input_location): Update prototype. (stream_input_location_now): New. * streamer-hooks.h (struct streamer_hooks): Make input_location to take pointer to location. (stream_input_location): Update. * ipa-devirt.c: Include streamer-hooks.h and lto-streamer.h (warn_odr): Apply location cache before warning. (lto_input_location): Update prototype. * gimple-streamer-in.c (input_phi, input_gimple_stmt): Use stream_input_location_now. * lto/lto.c (unify_scc): Revert location cache when unification suceeded. (lto_read_decls): Accept location cache after sucess; apply location cache before calling debug hooks. * lto-streamer-in.c (lto_location_cache::current_cache): New static variable. (lto_location_cache::cmp_loc): New function. (lto_location_cache::apply_location_cache): New function. (lto_location_cache::accept_location_cache): New function. (lto_location_cache::revert_location_cache): New function. (lto_location_cache::input_location): New function. (lto_input_location): Do location caching. (stream_input_location_now): New function. (input_eh_region, input_struct_function_base): Use stream_input_location_now. (lto_data_in_create): use new. (lto_data_in_delete): Use delete. * tree-streamer-in.c (unpack_ts_block_value_fields, unpack_ts_omp_clause_value_fields, streamer_read_tree_bitfields, lto_input_ts_exp_tree_pointers): Update for cached location api. Modified: trunk/gcc/gimple-streamer-in.c trunk/gcc/ipa-devirt.c trunk/gcc/lto-streamer-in.c trunk/gcc/lto-streamer.h trunk/gcc/lto/ChangeLog trunk/gcc/lto/lto.c trunk/gcc/streamer-hooks.h trunk/gcc/tree-streamer-in.c
[Bug lto/65536] LTO line number information garbled
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65536 --- Comment #46 from Jan Hubicka --- Manuel, I will hopefully commit the cache patch today or tomorrow morning. It does not solve full issue. What we have is 1) we still drop columns for firefox&chromium pretty early 2) there is a bug that we sometimes output wrong line number. I think it is caused by the logic reallocating ORDINARY_MAP_NUMBER_OF_COLUMN_BITS as described in original email. It would be nice to fix those. > My guess is that the motivation here is that, if there is a large gap, it > means that we didn't get asked any source_location since a lot of lines ago. > thus it would save a lot of source_locations to simply start a new map now. > Of course, this does not work for out-of-order, but why should we pessimize > typical usage for something that should be fixed in LTO? I do not think typical usage is pesimized here. Yes, code is trying to avoid case where we skip too many location indexes because we entered lines 1...30 and now we want to insert line 1000. We do not want to fast forward by 970*80. My code keeps this logic, only it allows ordering lines backward. > > 2) (line_delta > 10 && line_delta * ORDINARY_MAP_NUMBER_OF_COLUMN_BITS (map) > > > 1000) > >ORDINARY_MAP_NUMBER_OF_COLUMN_BITS (map) is in range 7... 15, so it never > >gets high enough to make this conditional trigger. I changed it to: > > I don't understand this. A line_delta of 67 (1000/15) will already trigger it. You are right, I misread the condition. > > 4) the code deciding whether to do reuse seems worng: > > if (line_delta < 0 > > || last_line != ORDINARY_MAP_STARTING_LINE_NUMBER (map) > > || SOURCE_COLUMN (map, highest) >= (1U << column_bits)) > > > >line_delta really should be base_line_delta, we do not need to give up > >when map's line is 1, SOURCE_LINE (map, set->highest_line) is 5 > >and we are requested to switch to line 3. > > If you insert a line out of order without creating a new map, then > linemap_position_for_column will return the wrong value. I do not see why. If we insert first line 1 (80 columns), then we create a map 1. Now we insert line 3, we do not create new map, but we offset highest_line by 80*2. Now if we start line 2, all we need is to offset highest_line by -80 and linemap_position_for_column will still work well. Of course when we get request for column that is out of range, we need to trigger creation of new map entry. What I am missing here?
[Bug lto/65536] LTO line number information garbled
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65536 --- Comment #45 from Manuel López-Ibáñez --- (In reply to Jan Hubicka from comment #42) > Hi, > I read linemap_line_start and I think I noticed few issues with respect > to overflows and lines being added randomly. I honestly think this is too sensitive for stage3. I'm also not very much in favour for optimizing out-of-order creating of line-maps. That is not the typical case outside LTO and I would argue it should not be the typical case in LTO either. Of course, fixing overflow problems is orthogonal and desirable and should be submitted as a separate patch. > 1) line_delta is computed as to_line SOURCE_LINE (map, set->highest_line) >I think the last inserted line is not very releavnt. What we care about > is >the base of the last location and to keep thing dense how much we are >stretching the value range from highest inserted element (inserting into > middle >is cheap). My guess is that the motivation here is that, if there is a large gap, it means that we didn't get asked any source_location since a lot of lines ago. thus it would save a lot of source_locations to simply start a new map now. Of course, this does not work for out-of-order, but why should we pessimize typical usage for something that should be fixed in LTO? > 2) (line_delta > 10 && line_delta * ORDINARY_MAP_NUMBER_OF_COLUMN_BITS (map) > > 1000) >ORDINARY_MAP_NUMBER_OF_COLUMN_BITS (map) is in range 7... 15, so it never >gets high enough to make this conditional trigger. I changed it to: I don't understand this. A line_delta of 67 (1000/15) will already trigger it. > 4) the code deciding whether to do reuse seems worng: > if (line_delta < 0 > || last_line != ORDINARY_MAP_STARTING_LINE_NUMBER (map) > || SOURCE_COLUMN (map, highest) >= (1U << column_bits)) > >line_delta really should be base_line_delta, we do not need to give up >when map's line is 1, SOURCE_LINE (map, set->highest_line) is 5 >and we are requested to switch to line 3. If you insert a line out of order without creating a new map, then linemap_position_for_column will return the wrong value.
[Bug lto/65536] LTO line number information garbled
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65536 --- Comment #44 from Jan Hubicka --- > > Ah, ok, it seems to work now. It just takes ages to print the lto1 line and it > get printed way after the lto1 process is running already. Yep, really anoying property that the stderr output is all buffered and output only at once. Something to do with libiberty. > > There is a path in linemap_add_line that does not increase locator, but it > > is bit bogus. I am playing with patch to improve this. > > Which path? > > What for sure happens is that when linemap_start_line starts returning 0, then > linemap_postion_for_column will always return the same location until > linemap_add is called directly (because of a change of file in the case of > LTO). Ah yes, you are right, it returns 0. I think this is bug too. We probably want to return last map or have a dedicated location for out of range? GCC uses: #define DECL_IS_BUILTIN(DECL) (LOCATION_LOCUS (DECL_SOURCE_LOCATION (DECL)) <= BUILTINS_LOCATION) so I think returning 0 will make us to believe that declaration with out of range location is builtin that will definitly lead to wrong code. Honza
[Bug lto/65536] LTO line number information garbled
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65536 --- Comment #43 from Manuel López-Ibáñez --- (In reply to Jan Hubicka from comment #40) > Manuel, > the following way you get the lto1 invocation: Ah, ok, it seems to work now. It just takes ages to print the lto1 line and it get printed way after the lto1 process is running already. > For proper LTO you need a linker with plugin support check: That seems correct. Thanks. > There is a path in linemap_add_line that does not increase locator, but it > is bit bogus. I am playing with patch to improve this. Which path? What for sure happens is that when linemap_start_line starts returning 0, then linemap_postion_for_column will always return the same location until linemap_add is called directly (because of a change of file in the case of LTO).
[Bug lto/65536] LTO line number information garbled
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65536 --- Comment #42 from Jan Hubicka --- Hi, I read linemap_line_start and I think I noticed few issues with respect to overflows and lines being added randomly. 1) line_delta is computed as to_line SOURCE_LINE (map, set->highest_line) I think the last inserted line is not very releavnt. What we care about is the base of the last location and to keep thing dense how much we are stretching the value range from highest inserted element (inserting into middle is cheap). For this reason I added base_line_delta and changed line_delta to be to_line - SOURCE_LINE (map, set->highest_location). Because things go in randomly, highest_line, which really is last inserted line, may be somewhere in between. 2) (line_delta > 10 && line_delta * ORDINARY_MAP_NUMBER_OF_COLUMN_BITS (map) > 1000) ORDINARY_MAP_NUMBER_OF_COLUMN_BITS (map) is in range 7... 15, so it never gets high enough to make this conditional trigger. I changed it to: || line_delta > 1000 || (line_delta << ORDINARY_MAP_NUMBER_OF_COLUMN_BITS (map)) > 1000 I.e. we do not want to skip more than 1000 unused entries since highest inserted location. 3) (max_column_hint <= 80 && ORDINARY_MAP_NUMBER_OF_COLUMN_BITS (map) >= 10) seems to intend to reduce the column range when it is no longer needed. Again, this is not really good idea when line inserted is not last. 4) the code deciding whether to do reuse seems worng: if (line_delta < 0 || last_line != ORDINARY_MAP_STARTING_LINE_NUMBER (map) || SOURCE_COLUMN (map, highest) >= (1U << column_bits)) line_delta really should be base_line_delta, we do not need to give up when map's line is 1, SOURCE_LINE (map, set->highest_line) is 5 and we are requested to switch to line 3. Second last_line != ORDINARY_MAP_STARTING_LINE_NUMBER (map) tests whether location has only one line that does not work (at least with my changes) because we may switch to next line and back. This conditoinal also seems to be completely missing hanlding of overflows. The following patch makes all line info and all but one carret to to be right on chromium warnings * line-map.c (linemap_line_start): Correct overflow tests. Index: line-map.c === --- line-map.c(revision 221568) +++ line-map.c(working copy) @@ -519,25 +519,38 @@ linemap_line_start (struct line_maps *se struct line_map *map = LINEMAPS_LAST_ORDINARY_MAP (set); source_location highest = set->highest_location; source_location r; - linenum_type last_line = -SOURCE_LINE (map, set->highest_line); - int line_delta = to_line - last_line; + int base_line_delta = to_line - ORDINARY_MAP_STARTING_LINE_NUMBER (map); + int line_delta = to_line - SOURCE_LINE (map, set->highest_location); bool add_map = false; - if (line_delta < 0 - || (line_delta > 10 - && line_delta * ORDINARY_MAP_NUMBER_OF_COLUMN_BITS (map) > 1000) - || (max_column_hint >= (1U << ORDINARY_MAP_NUMBER_OF_COLUMN_BITS (map))) + /* Single MAP entry can be used to encode multiple source lines. + Look for situations when this is impossible or undesriable. */ + if (base_line_delta < 0 + /* We want to keep maps resonably dense, so do not increase the range + of this linemap entry by more than 1000. */ + || line_delta > 1000 + || (line_delta << ORDINARY_MAP_NUMBER_OF_COLUMN_BITS (map)) > 1000 + /* If the max column is out of range and we are still not dropping line + info. */ + || (max_column_hint >= (1U << ORDINARY_MAP_NUMBER_OF_COLUMN_BITS (map)) + && highest < 0x6000) + /* If the prevoius line was long. Ignore this problem is we already + re-used the map for lines with greater indexes. */ || (max_column_hint <= 80 - && ORDINARY_MAP_NUMBER_OF_COLUMN_BITS (map) >= 10) + && ORDINARY_MAP_NUMBER_OF_COLUMN_BITS (map) >= 10 && line_delta > 0) + /* If we are just started running out of locations (which makes us to drop + column info), but current line map still has column info, create fresh + one. */ || (highest > 0x6000 - && (set->max_column_hint || highest > 0x7000))) + && (ORDINARY_MAP_NUMBER_OF_COLUMN_BITS (map) + || highest > 0x7000))) add_map = true; else max_column_hint = set->max_column_hint; if (add_map) { int column_bits; + bool reuse_map = true; if (max_column_hint > 10 || highest > 0x6000) { /* If the column number is ridiculous or we've allocated a huge @@ -554,11 +567,38 @@ linemap_line_start (struct line_maps *se column_bits++; max_column_hint = 1U << column_bits; } + /* Allocate the new line_map. However, if the current map only has a single line we can sometimes just increase its column_bits instead. */ - if (line_delta < 0 - || last_line
[Bug lto/65536] LTO line number information garbled
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65536 --- Comment #41 from Manuel López-Ibáñez --- (In reply to Manuel López-Ibáñez from comment #37) > But now that I think about it. If linemap_star_line returns UNKNOWN_LOCATION > because set->highest_location > 0x7000, but then you call > linemap_position_for_column and set->highest_location < 0xC000, then > linemap_line_start will return 0 again, thus linemap_position_for_column > will return 0 + to_column and that would be bad. Can you follow me? Do you > also see a potential bug there? Ignore the above, I just noticed that linemap_position_for_column tests for 0xC00 not for 0xC000, thus the above cannot happen.
[Bug lto/65536] LTO line number information garbled
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65536 --- Comment #40 from Jan Hubicka --- Manuel, the following way you get the lto1 invocation: jan@linux-qos1:~> gcc t.c -flto -O2 -c jan@linux-qos1:~> gcc t.o -flto -O2 --verbose -save-temps 2>&1 | grep lto1 /usr/lib64/gcc/x86_64-suse-linux/4.8/lto1 -quiet -dumpbase t.o -mtune=generic -march=x86-64 -mtune=generic -march=x86-64 -auxbase t -O2 -version -fltrans-output-list=/tmp/ccbfSrzR.ltrans.out -fwpa -fresolution=t.res @/tmp/ccSRKmvR /usr/lib64/gcc/x86_64-suse-linux/4.8/lto1 -quiet -dumpbase ccbfSrzR.ltrans0.o -mtune=generic -march=x86-64 -mtune=generic -march=x86-64 -auxbase-strip /tmp/ccbfSrzR.ltrans0.ltrans.o -O2 -version -fltrans-output-list=/tmp/ccbfSrzR.ltrans.out -fltrans @/tmp/cccTxW0P -o ccbfSrzR.ltrans0.s For proper LTO you need a linker with plugin support check: jan@linux-qos1:~> ld --help | grep plugin -plugin PLUGIN Load named plugin -plugin-opt ARG Send arg to last-loaded plugin There is a path in linemap_add_line that does not increase locator, but it is bit bogus. I am playing with patch to improve this.
[Bug lto/65536] LTO line number information garbled
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65536 --- Comment #39 from Manuel López-Ibáñez --- (In reply to Jan Hubicka from comment #36) > Manuel, > I returned back looking for reason lines are going out wrong when we get > short on locators. I do not understand the following code: I just noticed that linemap_add increases the highest_location by one everytime it is called, but there is no overflow check there. Thus, the source_locations may indeed overflow.
[Bug lto/65536] LTO line number information garbled
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65536 --- Comment #38 from Manuel López-Ibáñez --- (In reply to Jan Hubicka from comment #27) > I simply use --save-temps --verbose and use lto1 invocation from there. It > is easy ;) When I add those two options to an invokation of xg++, I get back the call to collect2 but no call to lto1, although I can see that lto1 is running.
[Bug lto/65536] LTO line number information garbled
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65536 --- Comment #37 from Manuel López-Ibáñez --- (In reply to Jan Hubicka from comment #36) > Here we seem to sometimes affect ORDINARY_MAP_NUMBER_OF_COLUMN_BITS of an > existing line map. How that can work? We already have locators that do > depends on the previous COLUMN_BITS value. The condition is checking that this map has more than one line || last_line != ORDINARY_MAP_STARTING_LINE_NUMBER (map) or we have assigned a location_t for a column larger than what we can address if we reduce the column bits of this map || SOURCE_COLUMN (map, highest) >= (1U << column_bits)) If none of those are true, we can reduce the column bits without affecting an already assigned columns, because the location_t value for those columns is simply the location_t value of the line + column. > I would expect the ORDINARY_MPA_NUMBER_OF_COLUMN_BITS set to be part of the > conditional and the computation bellow use map's column_bits. > Also I think the conditoinal guarding creation may want to check how far r > will be bumped and force creation when we are running short of locators. I don't understand. Which conditional? Force creating of what? In fact, when we are running short of locators, || (highest > 0x6000 && (set->max_column_hint || highest > 0x7000))) you see that add_map=true; then later if (max_column_hint > 10 || highest > 0x6000) { /* If the column number is ridiculous or we've allocated a huge number of source_locations, give up on column numbers. */ max_column_hint = 0; if (highest > 0x7000) return 0; column_bits = 0; which will give up on column numbers and force creating a new map that does not track columns (since SOURCE_COLUMN (map, highest) >= (1U << column_bits) will be always true), or if we are really short on source_locations, then give up completely and return UNKNOWN_LOCATION. But now that I think about it. If linemap_star_line returns UNKNOWN_LOCATION because set->highest_location > 0x7000, but then you call linemap_position_for_column and set->highest_location < 0xC000, then linemap_line_start will return 0 again, thus linemap_position_for_column will return 0 + to_column and that would be bad. Can you follow me? Do you also see a potential bug there?
[Bug lto/65536] LTO line number information garbled
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65536 Jan Hubicka changed: What|Removed |Added Status|UNCONFIRMED |NEW Last reconfirmed||2015-03-25 Ever confirmed|0 |1 --- Comment #36 from Jan Hubicka --- Manuel, I returned back looking for reason lines are going out wrong when we get short on locators. I do not understand the following code: if (line_delta < 0 || (line_delta > 10 && line_delta * ORDINARY_MAP_NUMBER_OF_COLUMN_BITS (map) > 1000) || (max_column_hint >= (1U << ORDINARY_MAP_NUMBER_OF_COLUMN_BITS (map))) || (max_column_hint <= 80 && ORDINARY_MAP_NUMBER_OF_COLUMN_BITS (map) >= 10) || (highest > 0x6000 && (set->max_column_hint || highest > 0x7000))) add_map = true; else max_column_hint = set->max_column_hint; here we seem to decide about adding new map and we always do that when we are running tight on locators... then if (add_map) { int column_bits; if (max_column_hint > 10 || highest > 0x6000) { /* If the column number is ridiculous or we've allocated a huge number of source_locations, give up on column numbers. */ max_column_hint = 0; if (highest > 0x7000) return 0; I will add abort to highest and check if we reach it for chromium. We probably can starting slowing down and allocating new linemap for every new line, not try the heuristics with 1000 above. However... column_bits = 0; } else { column_bits = 7; while (max_column_hint >= (1U << column_bits)) column_bits++; max_column_hint = 1U << column_bits; } /* Allocate the new line_map. However, if the current map only has a single line we can sometimes just increase its column_bits instead. */ if (line_delta < 0 || last_line != ORDINARY_MAP_STARTING_LINE_NUMBER (map) || SOURCE_COLUMN (map, highest) >= (1U << column_bits)) map = (struct line_map *) linemap_add (set, LC_RENAME, ORDINARY_MAP_IN_SYSTEM_HEADER_P (map), ORDINARY_MAP_FILE_NAME (map), to_line); ORDINARY_MAP_NUMBER_OF_COLUMN_BITS (map) = column_bits; r = (MAP_START_LOCATION (map) + ((to_line - ORDINARY_MAP_STARTING_LINE_NUMBER (map)) << column_bits)); Here we seem to sometimes affect ORDINARY_MAP_NUMBER_OF_COLUMN_BITS of an existing line map. How that can work? We already have locators that do depends on the previous COLUMN_BITS value. I would expect the ORDINARY_MPA_NUMBER_OF_COLUMN_BITS set to be part of the conditional and the computation bellow use map's column_bits. Also I think the conditoinal guarding creation may want to check how far r will be bumped and force creation when we are running short of locators. Honza
[Bug lto/65536] LTO line number information garbled
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65536 --- Comment #35 from Jan Hubicka --- > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65536 > > --- Comment #31 from Martin Liška --- > Beginning of the difference of ODR warnings: > > 210,211c210,211 > < gen/blink/core/CSSPropertyNames.cpp:2330:0: warning: type ???struct > stringpool_t??? violates one definition rule [-Wodr] > < static const short lookup[] = > --- > > gen/blink/core/CSSPropertyNames.cpp:1019:0: warning: type ???struct > > stringpool_t??? violates one definition rule [-Wodr] > > struct stringpool_t > 213,214c213,214 > < gen/blink/core/CSSValueKeywords.cpp:4309:0: note: a different type is > defined > in another translation unit >--- > > gen/blink/core/CSSValueKeywords.cpp:1850:0: note: a different type is > > defined in another translation unit > > struct stringpool_t > 216,217c216,217 > < gen/blink/core/CSSPropertyNames.cpp:2330:0: note: the first difference of > corresponding definitions is field ???stringpool_str0??? > < static const short lookup[] = This looks good, the messup in between stringppol_t and lookup was first one I noticed. Honza
[Bug lto/65536] LTO line number information garbled
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65536 --- Comment #34 from Manuel López-Ibáñez --- (In reply to Martin Liška from comment #30) > Created attachment 35132 [details] > Chrome: ODR warnings with patch from #c21 @Martin: The previous comment where the ICE is mentioned seems to refer to Jan's latest patch (with the cache), but this comment appears to refer to my patch in comment 21. Is that right? I don't see how you can get that backtrace with Jan's patch: linemap_line_start should only be called when applying the location cache, no? @Jan: I think: + linemap_line_start (line_table, loc.line, + MAX (81, loc.col + 1)) should simply be: + linemap_line_start (line_table, loc.line, + max + 1) For what is worth, I think your patch should already help a lot in GCC 5, it seems a move in the right direction. Perhaps the WPA sorting time is offset by the memory savings and lower number of maps, which should reduce lookup times. It would really be interesting to compute the sum of dump_line_table_statistics() for each input file before stream out and for the LTO line_table after stream in. It will give you some idea of how much you can expect to save in streamed out data and LTO memory by streaming out the line_tables directly.
[Bug lto/65536] LTO line number information garbled
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65536 --- Comment #33 from Richard Biener --- (In reply to Jan Hubicka from comment #26) > Created attachment 35130 [details] > linemap > > this is a proof of concept patch that makes streamer in to collect locations > into a "cache" and apply them in sorted order (looking up correct max_column > hints) at the end of handling of a given section. It also has facility to > throw away locations of trees that are freed. > while sorting at stream in time is not cool, it does not show top in the > profiles > and memory use of the cache is actually dominated by other stuff we read, > so this seems to work quite well in practice. Main problem would be if > someone > copied/used the locator before cache is applied. > > This helps to get all lines and most of carrets right on firefox. I killed my > Chromium tree so can't test it there (Martin, perhaps you could try?) > > I did not have much time today to test the patch. It also saves quite a lot > of > memory, about 400MB on firefox I guess. Looks sensible to me (apart from sorting at WPA time and missing TLC like adding function/code comments and removing no-op hunks). Richard. > Honza
[Bug lto/65536] LTO line number information garbled
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65536 --- Comment #32 from Richard Biener --- (In reply to Jan Hubicka from comment #16) > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65536 > > > > --- Comment #2 from Richard Biener --- > > The main issue with LTO is that it re-creates a combined linemap but in > > (most > > of the time) quite awkward ordering (simply registering random-ordered > > file:line:column entries by switching files/lines "appropriately"). > > > > I've argued that it would be better to stream those file:line:columns > > separately so we can "sort" them before handing them over to libcpp for > > linemap re-creation. > > Yep, we discussed this few times. One issue is when we sort them. > We can keep track of location as they are streamed out (and directly assign > them new IDs in the awkward ordering as they come) and then produce the > separate linemap section that would be read at once by WPA, sorted and > fed into linemaps. > > Sorting at compile time is bit tricky as we probably do not want to patch > existing pickled tree stream with the new locator IDs in the sorted sequence > and we do not really know what locations we will need ahead of time. Well, the idea was to stream another index representing the sorted order so we can sort and merge at WPA in O(n) and re-map the index on stream-in time. I don't think we want to sort at WPA time. > Other easier to implement idea for GCC 5 may be to simply collect locaiton > info and pointers to trees per SCC component andonly if it survived merging > feed it into linemap and in memory patch the new trees. This still should > save quite some memory given that most of trees read are discarded by merging > and may be quite easy to implement. > > Martin, the ICE by my patch is caused by libcpp getting out of memory? Btw, we don't have to fix this for GCC 5. We can of course backport a good solution later.
[Bug lto/65536] LTO line number information garbled
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65536 --- Comment #31 from Martin Liška --- Beginning of the difference of ODR warnings: 210,211c210,211 < gen/blink/core/CSSPropertyNames.cpp:2330:0: warning: type ‘struct stringpool_t’ violates one definition rule [-Wodr] < static const short lookup[] = --- > gen/blink/core/CSSPropertyNames.cpp:1019:0: warning: type ‘struct > stringpool_t’ violates one definition rule [-Wodr] > struct stringpool_t 213,214c213,214 < gen/blink/core/CSSValueKeywords.cpp:4309:0: note: a different type is defined in another translation unitgen/blink/core/CSSValueKeywords.cpp:1850:0: note: a different type is defined > in another translation unit > struct stringpool_t 216,217c216,217 < gen/blink/core/CSSPropertyNames.cpp:2330:0: note: the first difference of corresponding definitions is field ‘stringpool_str0’ < static const short lookup[] = --- > gen/blink/core/CSSPropertyNames.cpp:1021:0: note: the first difference of > corresponding definitions is field ‘stringpool_str0’ > char stringpool_str0[sizeof("grid")]; 219,220c219,220 < gen/blink/core/CSSValueKeywords.cpp:4309:0: note: a field of same name but different type is defined in another translation unit gen/blink/core/CSSValueKeywords.cpp:1852:0: note: a field of same name but > different type is defined in another translation unit > char stringpool_str0[sizeof("dot")]; 230,232c230,232 < ../../../../out/Release/gen/blink/core/XPathGrammar.cpp:241:0: note: a different type is defined in another translation unit < ../../../../out/Release/gen/blink/core/CSSGrammar.cpp:557:0: note: the first difference of corresponding definitions is field ‘yyvs_alloc’ < ../../../../out/Release/gen/blink/core/XPathGrammar.cpp:241:0: note: a field of same name but different type is defined in another translation unit --- > ../../../../out/Release/gen/blink/core/XPathGrammar.cpp:373:0: note: a > different type is defined in another translation unit > ../../../../out/Release/gen/blink/core/CSSGrammar.cpp:560:0: note: the first > difference of corresponding definitions is field ‘yyvs_alloc’ > ../../../../out/Release/gen/blink/core/XPathGrammar.cpp:376:0: note: a field > of same name but different type is defined in another translation unit ...
[Bug lto/65536] LTO line number information garbled
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65536 --- Comment #29 from Martin Liška --- Created attachment 35131 --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=35131&action=edit Chrome: ODR warnings before
[Bug lto/65536] LTO line number information garbled
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65536 --- Comment #30 from Martin Liška --- Created attachment 35132 --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=35132&action=edit Chrome: ODR warnings with patch from #c21
[Bug lto/65536] LTO line number information garbled
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65536 --- Comment #28 from Martin Liška --- (In reply to Jan Hubicka from comment #26) > Created attachment 35130 [details] > linemap > > this is a proof of concept patch that makes streamer in to collect locations > into a "cache" and apply them in sorted order (looking up correct max_column > hints) at the end of handling of a given section. It also has facility to > throw away locations of trees that are freed. > while sorting at stream in time is not cool, it does not show top in the > profiles > and memory use of the cache is actually dominated by other stuff we read, > so this seems to work quite well in practice. Main problem would be if > someone > copied/used the locator before cache is applied. > > This helps to get all lines and most of carrets right on firefox. I killed my > Chromium tree so can't test it there (Martin, perhaps you could try?) Sure. Unfortunately, there's still ICE, which is caused by a function body load that happens in IPA ICF: In function ‘Create’: lto1: internal compiler error: Segmentation fault 0x9037df crash_signal ../../gcc/toplev.c:383 0xed909c new_linemap ../../libcpp/line-map.c:265 0xed9467 linemap_add(line_maps*, lc_reason, unsigned int, char const*, unsigned int) ../../libcpp/line-map.c:314 0xed98d0 linemap_line_start(line_maps*, unsigned int, unsigned int) ../../libcpp/line-map.c:566 0xa8b190 unpack_ts_block_value_fields ../../gcc/tree-streamer-in.c:414 0xa8b190 streamer_read_tree_bitfields(lto_input_block*, data_in*, tree_node*) ../../gcc/tree-streamer-in.c:538 0x80a791 lto_read_tree_1 ../../gcc/lto-streamer-in.c:1197 0x80ab04 lto_read_tree ../../gcc/lto-streamer-in.c:1234 0x80ab04 lto_input_tree_1(lto_input_block*, data_in*, LTO_tags, unsigned int) ../../gcc/lto-streamer-in.c:1353 0x80ae01 lto_input_scc(lto_input_block*, data_in*, unsigned int*, unsigned int*) ../../gcc/lto-streamer-in.c:1258 0x80ae64 lto_input_tree(lto_input_block*, data_in*) ../../gcc/lto-streamer-in.c:1368 0xe26ff1 input_gimple_stmt ../../gcc/gimple-streamer-in.c:186 0xe26ff1 input_bb(lto_input_block*, LTO_tags, data_in*, function*, int) ../../gcc/gimple-streamer-in.c:303 0x80c457 input_function ../../gcc/lto-streamer-in.c:992 0x80c457 lto_read_body_or_constructor ../../gcc/lto-streamer-in.c:1129 0x614d5d cgraph_node::get_untransformed_body() ../../gcc/cgraph.c:3225 0xe6e580 ipa_icf::sem_function::init() ../../gcc/ipa-icf.c:1167 0xe6b730 ipa_icf::sem_item_optimizer::parse_nonsingleton_classes() ../../gcc/ipa-icf.c:2617 0xe72b9a ipa_icf::sem_item_optimizer::execute() ../../gcc/ipa-icf.c:2419 0xe74386 ipa_icf_driver ../../gcc/ipa-icf.c:3304 I also attach ODR warnings before and after the patch. Diff > > I did not have much time today to test the patch. It also saves quite a lot > of > memory, about 400MB on firefox I guess. > > Honza
[Bug lto/65536] LTO line number information garbled
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65536 --- Comment #27 from Jan Hubicka --- > I have in fact considered it already, since I think it should be fairly easy > and it can be done incrementally. However, as always in GCC, things are never > as trivial as they seem. I just tried following > https://gcc.gnu.org/wiki/DebuggingGCC#Debugging_LTO to see what > dump_line_table_statistics reports before and after my patch above, but it > doesn't print the lto1 invokation. The debugx scripts > (https://gcc.gnu.org/ml/gcc/2004-03/msg01195.html) do not seem to work with > lto1 (why those scripts are not in contrib/?). I feel like a new contributor > again! :) I simply use --save-temps --verbose and use lto1 invocation from there. It is easy ;) Having direct way to invoke lto1 in gdb from gcc wrapper or script may come handy, but it is not that hard to do by hand. Honza
[Bug lto/65536] LTO line number information garbled
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65536 --- Comment #26 from Jan Hubicka --- this is a proof of concept patch that makes streamer in to collect locations into a "cache" and apply them in sorted order (looking up correct max_column hints) at the end of handling of a given section. It also has facility to throw away locations of trees that are freed. while sorting at stream in time is not cool, it does not show top in the profiles and memory use of the cache is actually dominated by other stuff we read, so this seems to work quite well in practice. Main problem would be if someone copied/used the locator before cache is applied. This helps to get all lines and most of carrets right on firefox. I killed my Chromium tree so can't test it there (Martin, perhaps you could try?) I did not have much time today to test the patch. It also saves quite a lot of memory, about 400MB on firefox I guess. Honza
[Bug lto/65536] LTO line number information garbled
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65536 --- Comment #25 from Manuel López-Ibáñez --- (In reply to Jan Hubicka from comment #24) > Manuel, you may be right person to implement the streaming of linemaps then I have in fact considered it already, since I think it should be fairly easy and it can be done incrementally. However, as always in GCC, things are never as trivial as they seem. I just tried following https://gcc.gnu.org/wiki/DebuggingGCC#Debugging_LTO to see what dump_line_table_statistics reports before and after my patch above, but it doesn't print the lto1 invokation. The debugx scripts (https://gcc.gnu.org/ml/gcc/2004-03/msg01195.html) do not seem to work with lto1 (why those scripts are not in contrib/?). I feel like a new contributor again! :) Thus, sorry I don't really have the necessary free time to dedicate to this. I'm too busy with my real job at the moment. If/When I have time to dedicate to develop GCC, I will finish the transition of Fortran to the common diagnostics machinery, and then I will focus on C/C++ diagnostics. I still think that a patch similar to the one in comment #21 (+libcpp parts in comment #17) may improve the situation for GCC 5 and fix PR54962.
[Bug lto/65536] LTO line number information garbled
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65536 --- Comment #24 from Jan Hubicka --- Manuel, you may be right person to implement the streaming of linemaps then :) I suppose we do care about inline stacks in longer term to get proper warnings. We even may want to preserve macro expansion and all other info we have at compile time. In addition to that we ought to be able to tell what compilation unit the line infromation originate from https://gcc.gnu.org/ml/gcc-patches/2014-07/msg01876.html because same location in different units may have (and often has) different context. Not for GCC 5 though. I will still experiment with the idea of adding locations to linemaps only once tree merging found the tree location new. On average we read every tree about 20 times for Firefox, so this ought to reduce the waste of linemap space quite considerably perhaps pushing it to non-issue for GCC 5. honza
[Bug lto/65536] LTO line number information garbled
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65536 --- Comment #23 from Manuel López-Ibáñez --- (In reply to Manuel López-Ibáñez from comment #15) > (In reply to Jakub Jelinek from comment #14) > > By streaming the line_table directly you'd stream lots of location_t that > > are not actually used for anything that is streamed into the LTO. > > I don't understand why the tables would be huge, the string would of course > > use normal LTO section string sharing, and if we stream right now every > > location_t as the triplet, it would mean in case a single location_t is > > never used by more than one tree we'd stream an integer extra to what we > > stream now (just the triplets would be streamed in a different place), but > > in the more common case where one location_t is used by many trees, we'd > > stream less than what we stream now, due to the additional indirection. > [...] > Of course, if you are really unlucky, each location_t used by LTO may belong > to a different map, thus you cannot remove anything and then it would have > been more efficient to use a file:line:column table. In this worst case, the > overhead would be approximately sizeof(struct line_map) * number of rows in > the file:line:column table. In fact, looking at struct GTY(()) line_map_ordinary { const char *to_file; linenum_type to_line; /* An index into the set that gives the line mapping at whose end the current one was included. File(s) at the bottom of the include stack have this set to -1. */ int included_from; /* SYSP is one for a system header, two for a C system header file that therefore needs to be extern "C" protected in C++, and zero otherwise. This field isn't really needed now that it's in cpp_buffer. */ unsigned char sysp; /* Number of the low-order source_location bits used for a column number. */ unsigned int column_bits : 8; }; If we do not care about sysp and included_from, then even in the worst case, it will be more efficient to stream out the line_table than the file:line:column table. And it has the benefit that it is already sorted and the location_t values within trees/gimple can be streamed out directly without calling linemap_lookup to get the file:line:column for each of them. And since a location_t takes 1/3 of space that file:line:column, that would reduce the streamed out data even further.
[Bug lto/65536] LTO line number information garbled
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65536 --- Comment #22 from Manuel López-Ibáñez --- (In reply to Jan Hubicka from comment #20) > It seems that manuel just commented out the actual insertion to linemap, > columns are still streamed and ignored. > > Doing so will clearly make ODR waring more difficult to follow, so perhaps > we could try to go for it only after reading sufficiently large portion of > program? The problem is that by creating the line_table with physical locations out-of-order, LTO is 1) creating a lot of maps, even for small column changes. Thus, consuming a lot of extra memory. 2) increasing location_t values rapidly, thus probably triggering an overflow. The line-maps.c code already dynamically disables column numbers when location_t values get very large (I think this is the reason that there are no column numbers in the examples you show). If the location_t numbers get even larger, it will start returning UNKNOWN_LOCATION. Now I think that if you see wrong line numbers is most probably because of wrong location info in the front-ends, not because linemaps overflowed. There is also some mismatch between the limits used by linemap_line_start(): if (max_column_hint > LINE_MAP_MAX_COLUMN_NUMBER || highest > 0x6000) { /* If the column number is ridiculous or we've allocated a huge number of source_locations, give up on column numbers. */ max_column_hint = 0; if (highest > 0x7000) return 0; /* Give up completely and return UKNOWN_LOCATION */ column_bits = 0; } and the limits used by linemap_position_for_column: if (to_column >= set->max_column_hint) { if (r >= 0xC00 || to_column > LINE_MAP_MAX_COLUMN_NUMBER) { /* Running low on source_locations - disable column numbers. */ return r; } else { struct line_map *map = LINEMAPS_LAST_ORDINARY_MAP (set); r = linemap_line_start (set, SOURCE_LINE (map, r), to_column + 50); } } I'm not sure if the gap may lead to some problem, but it seems unlikely. In the worst case, it should return 0, thus UNKNOWN_LOCATION (I can see some instances of this in your examples).
[Bug lto/65536] LTO line number information garbled
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65536 --- Comment #21 from Manuel López-Ibáñez --- (In reply to Jakub Jelinek from comment #19) > Guess that can't really pass make check or lto bootstrap, because the LTO > streamer needs to be in sync between out and in, so commenting out > streaming in of the column number, but not doing the same for streaming it > out will break things completely. I was trying to silence the unused-but-set warning. Maybe this works: --- src/gcc/lto-streamer-in.c (revision 221557) +++ src/gcc/lto-streamer-in.c (working copy) @@ -198,19 +198,23 @@ lto_input_location (struct bitpack_d *bp current_line = bp_unpack_var_len_unsigned (bp); if (column_change) current_col = bp_unpack_var_len_unsigned (bp); - if (file_change) + /* FIXME: Disable columns for now until streaming of locations is reimplemented. */ + /* Enough column_hint to disable columns */ + current_col = LINE_MAP_MAX_COLUMN_NUMBER + 1; + + if (file_change) { if (prev_file) - linemap_add (line_table, LC_LEAVE, false, NULL, 0); - - linemap_add (line_table, LC_ENTER, false, current_file, current_line); + linemap_add (line_table, LC_RENAME, false, current_file, current_line); + else + linemap_add (line_table, LC_ENTER, false, current_file, current_line); } else if (line_change) -linemap_line_start (line_table, current_line, current_col); +return linemap_line_start (line_table, current_line, current_col); return linemap_position_for_column (line_table, current_col); } Note that using column_hint == 0, like in this patch: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=45375#c175 does not actually disable columns as it should, in fact, it triggers a new map allocation. (I think this is a bug in line-maps.c.)
[Bug lto/65536] LTO line number information garbled
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65536 --- Comment #20 from Jan Hubicka --- It seems that manuel just commented out the actual insertion to linemap, columns are still streamed and ignored. Doing so will clearly make ODR waring more difficult to follow, so perhaps we could try to go for it only after reading sufficiently large portion of program? Note that about 80-90% of code is removed by merging, so perhaps just applying linemaps after merging fails is way to get enough buffer for chromium sized programs?
[Bug lto/65536] LTO line number information garbled
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65536 Jakub Jelinek changed: What|Removed |Added CC||jakub at gcc dot gnu.org --- Comment #19 from Jakub Jelinek --- Guess that can't really pass make check or lto bootstrap, because the LTO streamer needs to be in sync between out and in, so commenting out streaming in of the column number, but not doing the same for streaming it out will break things completely.
[Bug lto/65536] LTO line number information garbled
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65536 --- Comment #18 from Manuel López-Ibáñez --- (In reply to Manuel López-Ibáñez from comment #17) > Something like this (not even compiled): This version compiles Index: src/libcpp/include/line-map.h === --- src/libcpp/include/line-map.h (revision 221557) +++ src/libcpp/include/line-map.h (working copy) @@ -88,10 +88,14 @@ struct GTY(()) line_map_ordinary { /* This is the highest possible source location encoded within an ordinary or macro map. */ #define MAX_SOURCE_LOCATION 0x7FFF +/* We do not track column numbers higher than this one. */ +#define LINE_MAP_MAX_COLUMN_NUMBER 10 + + struct cpp_hashnode; /* A macro line map encodes location of tokens coming from a macro expansion. Index: src/libcpp/line-map.c === --- src/libcpp/line-map.c (revision 221557) +++ src/libcpp/line-map.c (working copy) @@ -536,11 +536,12 @@ linemap_line_start (struct line_maps *se else max_column_hint = set->max_column_hint; if (add_map) { int column_bits; - if (max_column_hint > 10 || highest > 0x6000) + if (max_column_hint > LINE_MAP_MAX_COLUMN_NUMBER + || highest > 0x6000) { /* If the column number is ridiculous or we've allocated a huge number of source_locations, give up on column numbers. */ max_column_hint = 0; if (highest > 0x7000) @@ -598,11 +599,11 @@ linemap_position_for_column (struct line linemap_assert (!linemap_macro_expansion_map_p (LINEMAPS_LAST_ORDINARY_MAP (set))); if (to_column >= set->max_column_hint) { - if (r >= 0xC00 || to_column > 10) + if (r >= 0xC00 || to_column > LINE_MAP_MAX_COLUMN_NUMBER) { /* Running low on source_locations - disable column numbers. */ return r; } else Index: src/gcc/lto-streamer-in.c === --- src/gcc/lto-streamer-in.c (revision 221557) +++ src/gcc/lto-streamer-in.c (working copy) @@ -178,11 +178,10 @@ canon_file_name (const char *string) location_t lto_input_location (struct bitpack_d *bp, struct data_in *data_in) { static const char *current_file; static int current_line; - static int current_col; bool file_change, line_change, column_change; bool prev_file = current_file != NULL; if (bp_unpack_value (bp, 1)) return UNKNOWN_LOCATION; @@ -195,24 +194,31 @@ lto_input_location (struct bitpack_d *bp current_file = canon_file_name (bp_unpack_string (data_in, bp)); if (line_change) current_line = bp_unpack_var_len_unsigned (bp); + /* FIXME: Disable columns for now until streaming of locations is reimplemented. + static int current_col; if (column_change) current_col = bp_unpack_var_len_unsigned (bp); + */ - if (file_change) + if (file_change) { if (prev_file) - linemap_add (line_table, LC_LEAVE, false, NULL, 0); - - linemap_add (line_table, LC_ENTER, false, current_file, current_line); + linemap_add (line_table, LC_RENAME, false, current_file, current_line); + else + linemap_add (line_table, LC_ENTER, false, current_file, current_line); } else if (line_change) -linemap_line_start (line_table, current_line, current_col); - - return linemap_position_for_column (line_table, current_col); +/* Enough column_hint to disable columns */ +return linemap_line_start (line_table, current_line, + LINE_MAP_MAX_COLUMN_NUMBER + 1); + + /* Enough column_hint to disable columns */ + return linemap_position_for_column (line_table, + LINE_MAP_MAX_COLUMN_NUMBER + 1); } /* Read a reference to a tree node from DATA_IN using input block IB. TAG is the expected node that should be found in IB, if TAG belongs
[Bug lto/65536] LTO line number information garbled
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65536 --- Comment #17 from Manuel López-Ibáñez --- (In reply to Jan Hubicka from comment #16) > Other easier to implement idea for GCC 5 may be to simply collect locaiton For GCC 5 you should just disable column numbers in line-maps. This will save you a lot of source_locations before overflowing. If you don't overflow the source_locations themselves, then the line numbers at least should come out right. Something like this (not even compiled): Index: src/libcpp/include/line-map.h === --- src/libcpp/include/line-map.h (revision 221557) +++ src/libcpp/include/line-map.h (working copy) @@ -88,10 +88,14 @@ struct GTY(()) line_map_ordinary { /* This is the highest possible source location encoded within an ordinary or macro map. */ #define MAX_SOURCE_LOCATION 0x7FFF +/* We do not track column numbers higher than this one. */ +#define LINE_MAP_MAX_COLUMN_NUMBER 10 + + struct cpp_hashnode; /* A macro line map encodes location of tokens coming from a macro expansion. Index: src/libcpp/line-map.c === --- src/libcpp/line-map.c (revision 221557) +++ src/libcpp/line-map.c (working copy) @@ -536,11 +536,11 @@ linemap_line_start (struct line_maps *se else max_column_hint = set->max_column_hint; if (add_map) { int column_bits; - if (max_column_hint > 10 || highest > 0x6000) + if (max_column_hint > LINE_MAP_MAX_COLUMN_NUMBER || highest > 0x6000) { /* If the column number is ridiculous or we've allocated a huge number of source_locations, give up on column numbers. */ max_column_hint = 0; if (highest > 0x7000) @@ -598,11 +598,11 @@ linemap_position_for_column (struct line linemap_assert (!linemap_macro_expansion_map_p (LINEMAPS_LAST_ORDINARY_MAP (set))); if (to_column >= set->max_column_hint) { - if (r >= 0xC00 || to_column > 10) + if (r >= 0xC00 || to_column > LINE_MAP_MAX_COLUMN_NUMBER) { /* Running low on source_locations - disable column numbers. */ return r; } else Index: src/gcc/lto-streamer-in.c === --- src/gcc/lto-streamer-in.c (revision 221557) +++ src/gcc/lto-streamer-in.c (working copy) @@ -198,21 +198,23 @@ lto_input_location (struct bitpack_d *bp current_line = bp_unpack_var_len_unsigned (bp); if (column_change) current_col = bp_unpack_var_len_unsigned (bp); - if (file_change) + if (file_change) { if (prev_file) - linemap_add (line_table, LC_LEAVE, false, NULL, 0); - - linemap_add (line_table, LC_ENTER, false, current_file, current_line); + linemap_add (line_table, LC_RENAME, false, current_file, current_line); + else + linemap_add (line_table, LC_ENTER, false, current_file, current_line); } else if (line_change) -linemap_line_start (line_table, current_line, current_col); +/* Enough column_hint to disable columns */ +return linemap_line_start (line_table, current_line, LINE_MAP_MAX_COLUMN_NUMBER + 1); - return linemap_position_for_column (line_table, current_col); + /* Enough column_hint to disable columns */ + return linemap_position_for_column (line_table, LINE_MAP_MAX_COLUMN_NUMBER + 1); } /* Read a reference to a tree node from DATA_IN using input block IB. TAG is the expected node that should be found in IB, if TAG belongs
[Bug lto/65536] LTO line number information garbled
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65536 --- Comment #16 from Jan Hubicka --- > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65536 > > --- Comment #2 from Richard Biener --- > The main issue with LTO is that it re-creates a combined linemap but in (most > of the time) quite awkward ordering (simply registering random-ordered > file:line:column entries by switching files/lines "appropriately"). > > I've argued that it would be better to stream those file:line:columns > separately so we can "sort" them before handing them over to libcpp for > linemap re-creation. Yep, we discussed this few times. One issue is when we sort them. We can keep track of location as they are streamed out (and directly assign them new IDs in the awkward ordering as they come) and then produce the separate linemap section that would be read at once by WPA, sorted and fed into linemaps. Sorting at compile time is bit tricky as we probably do not want to patch existing pickled tree stream with the new locator IDs in the sorted sequence and we do not really know what locations we will need ahead of time. Other easier to implement idea for GCC 5 may be to simply collect locaiton info and pointers to trees per SCC component andonly if it survived merging feed it into linemap and in memory patch the new trees. This still should save quite some memory given that most of trees read are discarded by merging and may be quite easy to implement. Martin, the ICE by my patch is caused by libcpp getting out of memory?
[Bug lto/65536] LTO line number information garbled
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65536 --- Comment #15 from Manuel López-Ibáñez --- (In reply to Jakub Jelinek from comment #14) > By streaming the line_table directly you'd stream lots of location_t that > are not actually used for anything that is streamed into the LTO. > I don't understand why the tables would be huge, the string would of course > use normal LTO section string sharing, and if we stream right now every > location_t as the triplet, it would mean in case a single location_t is > never used by more than one tree we'd stream an integer extra to what we > stream now (just the triplets would be streamed in a different place), but > in the more common case where one location_t is used by many trees, we'd > stream less than what we stream now, due to the additional indirection. The line_table does not store every possible location_t, only the ones that start new maps, it should not even store one location_t per line! But it is true that a whole file:line:column table for all locations stored in the object file might be smaller than the line_table for the whole TU. It depends a lot how much of the input file ends up in the object file. But I think it should be easy to estimate the size of the line_table (see dump_line_table_statistics). I have no idea how large a file:line:column table would be for a typical object file. If on-disk size is a concern, one can also replace virtual (macro) locations with spelling/expansion point locations and not stream out any macro maps. Moreover, when streaming out, because of how the line_table is encoded as a contiguous array, one can mark the maps that are not used by LTO and simply not stream them out, which may reduce the size of the line_table significantly (and speed up any linemap merging/lookup later). This cannot be much slower than what you are doing already, since expanding "file:line:column" requires finding the map that holds this info. Of course, if you are really unlucky, each location_t used by LTO may belong to a different map, thus you cannot remove anything and then it would have been more efficient to use a file:line:column table. In this worst case, the overhead would be approximately sizeof(struct line_map) * number of rows in the file:line:column table. Even in this worst case, my intuition says that one could re-optimize the line_table to make it more efficient (since one location_t per map is the most inefficient case possible) before streaming out without actually changing the location_t values, but I would need to think a bit more whether this re-optimization is actually possible. Perhaps Dodji can figure out a problem with it.
[Bug lto/65536] LTO line number information garbled
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65536 --- Comment #14 from Jakub Jelinek --- By streaming the line_table directly you'd stream lots of location_t that are not actually used for anything that is streamed into the LTO. I don't understand why the tables would be huge, the string would of course use normal LTO section string sharing, and if we stream right now every location_t as the triplet, it would mean in case a single location_t is never used by more than one tree we'd stream an integer extra to what we stream now (just the triplets would be streamed in a different place), but in the more common case where one location_t is used by many trees, we'd stream less than what we stream now, due to the additional indirection.
[Bug lto/65536] LTO line number information garbled
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65536 --- Comment #13 from Manuel López-Ibáñez --- (In reply to Manuel López-Ibáñez from comment #12) > When reading, it is possible to merge different TU line_tables into a single > one quite efficiently (in the order of the maps and not in the order of > triplets or maximum location_t). When adding each new TU line_table to the > unified one, one keeps track of the last location_t assigned, and uses this > later to convert out of order the old location_t from each TU line_table to > the new location_t of the new unified line_table. If the merge function is implemented within line-map.c, it could be extremely efficient in terms of memory re-allocations. I think with the other approaches we end up reallocating the whole table many times because we do not know how many maps we need in advance (admittedly, anything else would be better than the current approach, because it seems LTO is creating a lot of empty maps).
[Bug lto/65536] LTO line number information garbled
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65536 --- Comment #12 from Manuel López-Ibáñez --- (In reply to Jakub Jelinek from comment #10) > You could just stream another table, containing the { file, line, column } > triplets, and stream location_t as indexes into this table (with 0/1 being > special for UNKNOWN_LOCATION and BUILTINS_LOCATION), or as pairs { > location_t, block }. Then on LTO reading, you could perhaps read the > location tables from multiple source files, merge them and for each of the > TUs build a hash_map mapping from the table indexes to location_t values. That is a bit of waste of memory, no? The "{ file, line, column }" table might be huge, presumably much larger than the actual line_tables. Thus, why not just stream the line_tables directly? Then, you can stream the location_t as they are. When reading, it is possible to merge different TU line_tables into a single one quite efficiently (in the order of the maps and not in the order of triplets or maximum location_t). When adding each new TU line_table to the unified one, one keeps track of the last location_t assigned, and uses this later to convert out of order the old location_t from each TU line_table to the new location_t of the new unified line_table. Perhaps I'm missing some detail and Dodji can comment on this idea.
[Bug lto/65536] LTO line number information garbled
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65536 --- Comment #9 from Manuel López-Ibáñez --- (In reply to Richard Biener from comment #2) > if (file_change) > { > if (prev_file) > linemap_add (line_table, LC_LEAVE, false, NULL, 0); > > linemap_add (line_table, LC_ENTER, false, current_file, current_line); > } > else if (line_change) > linemap_line_start (line_table, current_line, current_col); > > return linemap_position_for_column (line_table, current_col); By the way, just doing else if (line_change) - linemap_line_start (line_table, current_line, current_col); + linemap_line_start (line_table, current_line, current_col + 1); return linemap_position_for_column (line_table, current_col); should reduce memory consumption (and perhaps speed) of the line_table by a lot. You are basically saying: I need a map not bigger than current_col. Then saying, give me a position for current_col, which triggers another map allocation. And my guess is that: else if (line_change) - linemap_line_start (line_table, current_line, current_col); + linemap_line_start (line_table, current_line, 81); return linemap_position_for_column (line_table, current_col); may reduce it even further if at least the file:line are mostly in order (otherwise, the column number does not matter because you will create a new map for every call to linemap_line_start).
[Bug lto/65536] LTO line number information garbled
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65536 --- Comment #11 from Manuel López-Ibáñez --- (In reply to Manuel López-Ibáñez from comment #9) > should reduce memory consumption (and perhaps speed) of the line_table by a I meant "increase speed".
[Bug lto/65536] LTO line number information garbled
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65536 --- Comment #10 from Jakub Jelinek --- You could just stream another table, containing the { file, line, column } triplets, and stream location_t as indexes into this table (with 0/1 being special for UNKNOWN_LOCATION and BUILTINS_LOCATION), or as pairs { location_t, block }. Then on LTO reading, you could perhaps read the location tables from multiple source files, merge them and for each of the TUs build a hash_map mapping from the table indexes to location_t values.
[Bug lto/65536] LTO line number information garbled
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65536 --- Comment #8 from Manuel López-Ibáñez --- (In reply to Richard Biener from comment #7) > (In reply to Manuel López-Ibáñez from comment #5) > > (In reply to Richard Biener from comment #2) > > > The main issue with LTO is that it re-creates a combined linemap but in > > > (most > > > of the time) quite awkward ordering (simply registering random-ordered > > > file:line:column entries by switching files/lines "appropriately"). > > > > > > I've argued that it would be better to stream those file:line:columns > > > separately so we can "sort" them before handing them over to libcpp for > > > linemap re-creation. Do you mean build the line_table first in order, then when reading in the trees/ statements, find the location_t corresponding to each given file:line:column? Unfortunately, there is no fast way to find the maps that correspond to a given file:line:column. You could collect all physical locations (file:line:columns), perhaps having some sorted list of files that points to a sorted list of line:column pairs. Then go through each file in order and build the line_table incrementally, once done with a file, LC_RENAME for the next file and continue building the line table and so on. Then, we streaming tree, find the location_t for a given file:line:column by doing a (binary?) search in the list of maps. This last step seems slow, not sure how it could be sped up. > I see. An issue is that we don't keep track of whether we entered a file > already (and after re-computing line-maps we lose any "nesting" of includes, > that is, we always have enter/leave pairs). But if LC_RENAME works even > when a file was never LC_ENTERed before then we can use it. > > Not sure if it will make a difference though. I think not much because line-map.c already has code to deal with this. Look for: /* So this _should_ mean we are leaving the main file -- effectively ending the compilation unit. But to_file not being NULL means the caller thinks we are leaving to another file. This is an erroneous behaviour but we'll try to recover from it. Let's pretend we are not leaving the main file. */ > > How are the original locations stored on disk? > > As string for file and two ints for line and column. Thus they are > stored "expanded". But they are interleaved with the trees / stmts they > are associated to. I wonder if it would not be easier to store directly the line_table and location_t per object file, then re-encode location_t when reading to be an index into an array of line_tables, plus the actual location_t. It just needs to be careful to undo the re-encoding and use the correct line_table before passing the info to the line-map functions.
[Bug lto/65536] LTO line number information garbled
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65536 --- Comment #7 from Richard Biener --- (In reply to Manuel López-Ibáñez from comment #5) > (In reply to Richard Biener from comment #2) > > The main issue with LTO is that it re-creates a combined linemap but in > > (most > > of the time) quite awkward ordering (simply registering random-ordered > > file:line:column entries by switching files/lines "appropriately"). > > > > I've argued that it would be better to stream those file:line:columns > > separately so we can "sort" them before handing them over to libcpp for > > linemap re-creation. > > If the lines or columns are out of order, then all bets are off. Line-map > locations have to be allocated incrementally. If you switch between files a > lot, then you are wasting a lot of memory unnecessarily, since switching > requires creating a new line-map. > > How difficult would be to track on which file you are and simply have a > line_table per file and switch the current line_table appropriately? > > > So currently we do, for each "file:line:column" location we stream in: > > > > if (file_change) > > { > > if (prev_file) > > linemap_add (line_table, LC_LEAVE, false, NULL, 0); > > > > linemap_add (line_table, LC_ENTER, false, current_file, current_line); > > } > > else if (line_change) > > linemap_line_start (line_table, current_line, current_col); > > > > return linemap_position_for_column (line_table, current_col); > > > > which of course puts a heavy load on the linemap encoding... > > According to line-map.h: > > /* Reason for creating a new line map with linemap_add. LC_ENTER is >when including a new file, e.g. a #include directive in C. >LC_LEAVE is when reaching a file's end. LC_RENAME is when a file >name or line number changes for neither of the above reasons >(e.g. a #line directive in C); > > Thus I'm not sure the above is really correct. If a single line_table is > desirable, it probably should use LC_RENAME for different main files (as if > they were concatenated and using #line). LC_LEAVE/LC_ENTER only makes sense > for included files, which I presume are slightly different than concatenated > files. I see. An issue is that we don't keep track of whether we entered a file already (and after re-computing line-maps we lose any "nesting" of includes, that is, we always have enter/leave pairs). But if LC_RENAME works even when a file was never LC_ENTERed before then we can use it. Not sure if it will make a difference though. Index: gcc/lto-streamer-in.c === --- gcc/lto-streamer-in.c (revision 221619) +++ gcc/lto-streamer-in.c (working copy) @@ -182,7 +182,6 @@ lto_input_location (struct bitpack_d *bp static int current_line; static int current_col; bool file_change, line_change, column_change; - bool prev_file = current_file != NULL; if (bp_unpack_value (bp, 1)) return UNKNOWN_LOCATION; @@ -201,12 +200,7 @@ lto_input_location (struct bitpack_d *bp current_col = bp_unpack_var_len_unsigned (bp); if (file_change) -{ - if (prev_file) - linemap_add (line_table, LC_LEAVE, false, NULL, 0); - - linemap_add (line_table, LC_ENTER, false, current_file, current_line); -} +linemap_add (line_table, LC_RENAME, false, current_file, current_line); else if (line_change) linemap_line_start (line_table, current_line, current_col); > line-map.c has code to make some erroneous cases "work": I think they should > be replaced by linemap_assert_fails(), such that they trigger an assert when > checking but they try to recover gracefully otherwise. > > How are the original locations stored on disk? As string for file and two ints for line and column. Thus they are stored "expanded". But they are interleaved with the trees / stmts they are associated to.
[Bug lto/65536] LTO line number information garbled
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65536 --- Comment #6 from Manuel López-Ibáñez --- (In reply to Jan Hubicka from comment #0) > One obvious thing is that linemap_line_start takes argument 3 to be max > column hint, but we pass current_col that is not cool. If I remember correctly, the column hint is only used to try to optimize memory. That is, if you provide a bad hint, then memory will be wasted (a too short hint may require additional maps for larger columns; a too large hint requires additional maps to encode the same number of lines). However, unless you pass a value of 10 or more, it should not affect the actual behavior.
[Bug lto/65536] LTO line number information garbled
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65536 --- Comment #5 from Manuel López-Ibáñez --- (In reply to Richard Biener from comment #2) > The main issue with LTO is that it re-creates a combined linemap but in (most > of the time) quite awkward ordering (simply registering random-ordered > file:line:column entries by switching files/lines "appropriately"). > > I've argued that it would be better to stream those file:line:columns > separately so we can "sort" them before handing them over to libcpp for > linemap re-creation. If the lines or columns are out of order, then all bets are off. Line-map locations have to be allocated incrementally. If you switch between files a lot, then you are wasting a lot of memory unnecessarily, since switching requires creating a new line-map. How difficult would be to track on which file you are and simply have a line_table per file and switch the current line_table appropriately? > So currently we do, for each "file:line:column" location we stream in: > > if (file_change) > { > if (prev_file) > linemap_add (line_table, LC_LEAVE, false, NULL, 0); > > linemap_add (line_table, LC_ENTER, false, current_file, current_line); > } > else if (line_change) > linemap_line_start (line_table, current_line, current_col); > > return linemap_position_for_column (line_table, current_col); > > which of course puts a heavy load on the linemap encoding... According to line-map.h: /* Reason for creating a new line map with linemap_add. LC_ENTER is when including a new file, e.g. a #include directive in C. LC_LEAVE is when reaching a file's end. LC_RENAME is when a file name or line number changes for neither of the above reasons (e.g. a #line directive in C); Thus I'm not sure the above is really correct. If a single line_table is desirable, it probably should use LC_RENAME for different main files (as if they were concatenated and using #line). LC_LEAVE/LC_ENTER only makes sense for included files, which I presume are slightly different than concatenated files. line-map.c has code to make some erroneous cases "work": I think they should be replaced by linemap_assert_fails(), such that they trigger an assert when checking but they try to recover gracefully otherwise. How are the original locations stored on disk? > But it should definitely work (not sure what it does on line or file > "overflow"). I'm not sure lines or files can really overflow, but location_t can, and the only "fix" is that column numbers get disabled (highest location > 0x6000), but there is nothing stopping it from really overflowing after that.
[Bug lto/65536] LTO line number information garbled
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65536 Martin Liška changed: What|Removed |Added CC||marxin at gcc dot gnu.org --- Comment #4 from Martin Liška --- (In reply to Jan Hubicka from comment #0) > As shown in https://gcc.gnu.org/ml/gcc-patches/2015-03/msg01151.html > warnings seems to come out wrong with large programs. I did not manage to > reproduce it with small testcase. Columns tends to be 0 and line numbers > (somewhat) off usually pointing to the begging of type definition instead of > the field in question but sometimes they just point in further distance. > > This reproduce both on Fireofx and Chromium > > The problem goes away with: > > Index: lto-streamer-in.c > === > --- lto-streamer-in.c (revision 221582) > +++ lto-streamer-in.c (working copy) > @@ -200,7 +200,7 @@ lto_input_location (struct bitpack_d *bp >if (column_change) > current_col = bp_unpack_var_len_unsigned (bp); > > - if (file_change) > + if (file_change || 1) > { >if (prev_file) > linemap_add (line_table, LC_LEAVE, false, NULL, 0); > > (which also quite significantly increases memory use). The warnings seems > to be right on beggining and gets worse at end, so I suspect it is some kind > of overflow in libcpp. > > The problem stays with: > > Index: lto-streamer-in.c > === > --- lto-streamer-in.c (revision 221582) > +++ lto-streamer-in.c (working copy) > @@ -207,7 +207,7 @@ lto_input_location (struct bitpack_d *bp > >linemap_add (line_table, LC_ENTER, false, current_file, current_line); > } > - else if (line_change) > + else if (line_change || 1) > linemap_line_start (line_table, current_line, current_col); > >return linemap_position_for_column (line_table, current_col); > > One obvious thing is that linemap_line_start takes argument 3 to be max > column hint, but we pass current_col that is not cool. > > I see that libcpp seems to drop the column info after some threshold (that > is clearly too small for LTO) but why the line numbers are off? Unfortunately, following patch causes SEGFAULT: Program received signal SIGSEGV, Segmentation fault. memset () at ../sysdeps/x86_64/memset.S:80 80../sysdeps/x86_64/memset.S: No such file or directory. (gdb) bt #0 memset () at ../sysdeps/x86_64/memset.S:80 #1 0x00ed8ebd in new_linemap (set=set@entry=0x77fee000, reason=reason@entry=LC_ENTER) at ../../libcpp/line-map.c:265 #2 0x00ed9288 in linemap_add (set=0x77fee000, reason=reason@entry=LC_ENTER, sysp=sysp@entry=0, to_file=0x12d245f0 "../../ui/views/controls/image_view.h", to_line=92) at ../../libcpp/line-map.c:314 #3 0x0080d439 in lto_input_location (bp=, data_in=) at ../../gcc/lto-streamer-in.c:208 #4 0x00a8ba06 in streamer_read_tree_bitfields (ib=ib@entry=0x7fffd620, data_in=data_in@entry=0xc3281b0, expr=expr@entry=0x7fff163d05e8) at ../../gcc/tree-streamer-in.c:506 #5 0x0080a572 in lto_read_tree_1 (ib=0x7fffd620, data_in=0xc3281b0, expr=0x7fff163d05e8) at ../../gcc/lto-streamer-in.c:1193 #6 0x0080a8e5 in lto_read_tree (hash=399977849, tag=, data_in=0xc3281b0, ib=0x7fffd620) at ../../gcc/lto-streamer-in.c:1230 #7 lto_input_tree_1 (ib=0x7fffd620, data_in=0xc3281b0, tag=, hash=399977849) at ../../gcc/lto-streamer-in.c:1349 #8 0x0080abe2 in lto_input_scc (ib=ib@entry=0x7fffd620, data_in=data_in@entry=0xc3281b0, len=len@entry=0x7fffd618, entry_len=entry_len@entry=0x7fffd61c) at ../../gcc/lto-streamer-in.c:1254 #9 0x005b39b7 in lto_read_decls (decl_data=decl_data@entry=0x7fff1b99f000, data=data@entry=0x151de970, resolutions=..., resolutions@entry=...) at ../../gcc/lto/lto.c:1902 #10 0x005b52ac in lto_file_finalize (file=, file_data=0x7fff1b99f000) at ../../gcc/lto/lto.c:2236 #11 lto_create_files_from_ids (count=, file_data=0x7fff1b99f000, file=0xbf4b5f0) at ../../gcc/lto/lto.c:2246 #12 lto_file_read (count=, resolution_file=, file=) at ../../gcc/lto/lto.c:2287 #13 read_cgraph_and_symbols (fnames=, nfiles=) at ../../gcc/lto/lto.c:2992 #14 lto_main () at ../../gcc/lto/lto.c:3462 #15 0x00903592 in compile_file () at ../../gcc/toplev.c:594 #16 0x0058f271 in do_compile () at ../../gcc/toplev.c:2076 #17 toplev::main (this=this@entry=0x7fffd7f0, argc=13846, argc@entry=34, argv=0x192f9f0, argv@entry=0x7fffd8f8) at ../../gcc/toplev.c:2174 #18 0x0058fa6a in main (argc=34, argv=0x7fffd8f8) at ../../gcc/main.c:39 More precisely, I used patched compiler just for WPA phase, should also this set-up work? Martin
[Bug lto/65536] LTO line number information garbled
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65536 Richard Biener changed: What|Removed |Added Summary|[5 regression] LTO line |LTO line number information |number information garbled |garbled --- Comment #3 from Richard Biener --- Btw, this can't be a regression in GCC 5 only (or even a regression at all?). Maybe with macro expansion locations and better column tracking we now more likely overflow tables, but well...