[Bug lto/65536] LTO line number information garbled

[manu at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65536

--- Comment #49 from Manuel López-Ibáñez <manu at gcc dot gnu.org> ---
(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 0x70000000
+
+/* Highest possible source location encoded within an ordinary or
+   macro map.  */
+#define LINE_MAP_MAX_SOURCE_LOCATION 0x7FFFFFF0
+
 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 > 0x60000000
-         && (set->max_column_hint || highest > 0x70000000)))
+      || (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 > 100000 || highest > 0x60000000)
+      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 > 0x70000000)
-           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 >= 0xC000000 || to_column > 100000)
+      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-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;
                                             ^

> 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.

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.

> > 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.

Yes, we want to. Because then set->highest_location goes up just by one and the
next map starts from that, thus we do not need to consume source_locations for
those 970*80 entries which will never be addressed (we jump directly from 30:80
to 1000:1).

> > > 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.

But then you do not know which is the source_location of the highest line
encoded in the line_table, which is used in several places. I'm not saying it
is not worth it or possible, but it is not as trivial as just offsetting
highest_line (and then, it should not be called highest_line but perhaps
last_line_location).

Note that I'm not discussing all this to be negative. I'm very much in favour
of supporting some kind of out-of-order insertion (in particular for PR52952),
but we should be careful to not pessimize the in-order case and we should get
all the details right. The line-maps are tricky and getting the details wrong
could be disaster.