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

[Lawrence Crowl <crowl at google dot com>]

On 10/20/11, Gabriel Charette <gcharette1@gmail.com> wrote:
> I just thought about something..
>
> Earlier I said that ALL line_table issues were resolved after this
> patch (as it ignores the re-included headers that were guarded, as the
> non-pph compiler does naturally).
>
> One problem remains however, I'm pretty sure that re-included
> non-pph'ed header's line_table entries are still showing up multiple
> times (as the direct non-pph childs of a given pph_include have their
> line_table entries copied one by one from the pph file).
>
> I think we were talking about somehow remembering guards context in
> which DECLs were declared and then ignoring DECLs streamed in if they
> belong to a given "header guard type" that was previously seen in a
> prior include using the same non-pph header, allowing us to ignore
> those DECLs that are re-declared when they should have been guarded
> out the second time.
>
> I'm not sure whether there is machinery to handle non-pph re-includes
> yet... but... at the very least, I'm pretty sure those non-pph entries
> still show up multiple times in the line_table.
>
> Now, we can't just remove/ignore those entries either... doing so
> would alter the expected location offset (pph_loc_offset) applied to
> all tokens streamed in directly from the pph header.
>
> What we could potentially do is:
> - ignore the repeated non-pph entry
> - remember the number of locations this entry was "supposed" to take
> (call that pph_loc_ignored_offset)
> - then for DECLs imported after it we would then need an offset of
> pph_loc_offset - pph_loc_ignored_offset, to compensate for the missing
> entries in the line_table
>
> The problem here obviously is that I don't think we have a way of
> knowing which DECLs come before, inside, and after a given non-pph
> header included in the parent pph header which we are currently
> reading.
>
> Furthermore, a DECL coming after the non-pph header could potentially
> refer to something inside the ignored non-pph header and the
> source_location of the referred token would now be invalid (although
> that might already be fixed by the cache hit which would redirect that
> token reference to the same token in the first included copy of that
> same header which wasn't actually skipped as it was first and which is
> valid)
>
>
> On Tue, Oct 11, 2011 at 4:26 PM, Diego Novillo <dnovillo@google.com> wrote:
>> @@ -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;
>>
>
> Also, if we do ignore some non-pph entries, the included_from
> calculation is going to need some trickier logic as well (it's fine
> for the pph includes though as each child calculates it's own offset)
>
>> -         gcc_assert (lm->included_from < (int) line_table->used);
>> -
>
> Also, I think this slipped in my previous comment, but I don't see how
> this assert could trigger in the current code. If it did trigger
> something was definitely wrong as it asserts the offseted
> included_from is referring to an entry that is actually in the
> line_table...
>
>>          lm->start_location += pph_loc_offset;

I'm wondering if we shouldn't just whitelist the problematic cases
that we know about in the system/standard headers.  It seems that all
others we could reasonably complain to the maintainers of the code.

-- 
Lawrence Crowl