This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [pph] Make libcpp symbol validation a warning (issue5235061)


Just looked at the line_table related sections, but see comments below:

On Tue, Oct 11, 2011 at 4:26 PM, Diego Novillo <dnovillo@google.com> wrote:
>
> Currently, the consistency check done on pre-processor symbols is
> triggering on symbols that are not really problematic (e.g., symbols
> used for double-include guards).
>
> The problem is that in the testsuite, we are refusing to process PPH
> images that fail that test, which means we don't get to test other
> issues. ÂTo avoid this, I changed the error() call to warning(). ÂSeemed
> innocent enough, but there were more problems behind that one:
>
> 1- We do not really try to avoid reading PPH images more than once.
> Â This problem is different than the usual double-inclusion guard.
> Â For instance, suppose a file foo.pph includes 1.pph, 2.pph and
> Â 3.pph. ÂWhen generating foo.pph, we read all 3 files just once and
> Â double-include guards do not need to trigger. ÂHowever, if we are
> Â later building a TU with:
> Â Â Â Â#include 2.pph
> Â Â Â Â#include foo.pph
> Â we first read 2.pph and when reading foo.pph, we try to read 2.pph
> Â again, because it is mentioned in foo.pph's line map table.
>
> Â I added a guard in pph_stream_open() so it doesn't try to open the
> Â same file more than once, but that meant adjusting some of the
> Â assertions while reading the line table. ÂWe should not expect to
> Â find foo.pph's line map table exactly like the one we wrote.

That makes sense.

> @@ -328,8 +327,6 @@ pph_in_line_table_and_includes (pph_stream *stream)
> Â int entries_offset = line_table->used - PPH_NUM_IGNORED_LINE_TABLE_ENTRIES;
> Â enum pph_linetable_marker next_lt_marker = pph_in_linetable_marker (stream);
>
> - Âpph_reading_includes++;
> -
> Â for (first = true; next_lt_marker != PPH_LINETABLE_END;
> Â Â Â Ânext_lt_marker = pph_in_linetable_marker (stream))
> Â Â {
> @@ -373,19 +370,33 @@ pph_in_line_table_and_includes (pph_stream *stream)
> Â Â Â Â Âelse
> Â Â Â Â Â Âlm->included_from += entries_offset;
>
> - Â Â Â Â gcc_assert (lm->included_from < (int) line_table->used);
> -

This should still hold, it is impossible that included_from points to
an entry that doesn't exist (i.e. beyond line_table->used), but since
we recalculate it on the previous line, adding entries_offset, this
was just a safe check to make sure everything read makes sense.

> Â Â Â Â Âlm->start_location += pph_loc_offset;
>
> Â Â Â Â Âline_table->used++;
> Â Â Â Â}
> Â Â }
>
> - Âpph_reading_includes--;
> + Â/* We used to expect exactly the same number of entries, but files
> + Â Â included from this PPH file may sometimes not be needed. ÂFor
> + Â Â example,
> +
> + Â Â Â #include "2.pph"
> + Â Â Â #include "foo.pph"
> + Â Â Â Â +--> Â#include "1.pph"
> + Â Â Â Â Â Â Â #include "2.pph"
> + Â Â Â Â Â Â Â #include "3.pph"
> +
> + Â Â When foo.pph was originally created, the line table was built
> + Â Â with inclusions of 1.pph, 2.pph and 3.pph. ÂBut when compiling
> + Â Â the main translation unit, we include 2.pph before foo.pph, so
> + Â Â the inclusion of 2.pph from foo.pph does nothing. ÂLeaving the
> + Â Â line table in a different shape than the original compilation.
>
> + Â Â Instead of insisting on getting EXPECTED_IN entries, we expect at
> + Â Â most EXPECTED_IN entries. Â*/
> Â {
> Â Â unsigned int expected_in = pph_in_uint (stream);
> - Â Âgcc_assert (line_table->used - used_before == expected_in);
> + Â Âgcc_assert (line_table->used - used_before <= expected_in);

I'm not sure exactly how you skip headers already parsed now (we
didn't used to when I wrote this code and that was the only problem
remaining in the line_table (i.e. duplicate entries for guarded
headers in the non-pph compile)), but couldn't you count the number of
skipped entries and assert (line_table->used - used_before) +
numSkipped == expected_in) ?

I'd have to re-download the code, I've bee following through patches,
but I'm not so sure now exactly how the "guarded headers skipping" is
done, my memorized knowledge of the codebase has diverged I feel..!

A more important note: I think it could be worth having a new flag
that outputs the line_table when done parsing (as a mean to robustly
test it). My way to test it usually was to breakpoint on
varpool_assemble_decl (a random choice, but it was only called after
parsing was done...), both in pph and non-pph compiles and compare the
line_table in gdb.... However, to have a stable test in the long run,
it could be nice to have a flag that asks for an output of the
line_table and then we could checksum and compare the line_table
outputted by the pph and non-pph compiles.

A good test I had found to break in and analyze the line_table was
p4eabi.h as it pretty much had all the problems that I fixed regarding
the line_table (it also has re-includes if I remember correctly, but
that wasn't a problem before as we would not guard out re-includes as
I just mentioned above).

Having such a robust test would be important I feel as, as we saw with
previous bugs, discrepancies in the line_table can result in tricky
unique ID diffs.

If you are now guarding out re-includes the line_table should now be
perfectly identical in pph and non-pph as this was the only thing
remaining I think :)!

Cheers,
Gab

>
> --
> This patch is available for review at http://codereview.appspot.com/5235061


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]