Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(1033)

Issue 5235061: [pph] Make libcpp symbol validation a warning (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
10 years, 7 months ago by Diego Novillo
Modified:
10 years, 7 months ago
Reviewers:
gcharette1
CC:
Lawrence Crowl, gcc-patches_gcc.gnu.org
Visibility:
Public.

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+261 lines, -171 lines) Patch
M gcc/cp/ChangeLog.pph View 1 chunk +45 lines, -0 lines 0 comments Download
M gcc/cp/pph.c View 3 chunks +15 lines, -10 lines 0 comments Download
M gcc/cp/pph-streamer.h View 8 chunks +19 lines, -18 lines 0 comments Download
M gcc/cp/pph-streamer.c View 9 chunks +91 lines, -12 lines 0 comments Download
M gcc/cp/pph-streamer-in.c View 12 chunks +46 lines, -86 lines 0 comments Download
M gcc/cp/pph-streamer-out.c View 13 chunks +32 lines, -32 lines 0 comments Download
M gcc/testsuite/ChangeLog.pph View 1 chunk +8 lines, -0 lines 0 comments Download
M gcc/testsuite/g++.dg/pph/d1symnotinc.cc View 1 chunk +1 line, -1 line 0 comments Download
M gcc/testsuite/g++.dg/pph/x5dynarray7.h View 1 chunk +0 lines, -3 lines 0 comments Download
M gcc/testsuite/g++.dg/pph/x6dynarray6.h View 1 chunk +0 lines, -3 lines 0 comments Download
M gcc/testsuite/g++.dg/pph/x7dynarray6.cc View 1 chunk +2 lines, -3 lines 0 comments Download
M gcc/testsuite/g++.dg/pph/x7dynarray7.cc View 1 chunk +2 lines, -3 lines 0 comments Download

Messages

Total messages: 8
Diego Novillo
Currently, the consistency check done on pre-processor symbols is triggering on symbols that are not ...
10 years, 7 months ago (2011-10-11 20:26:45 UTC) #1
gcharette1
Just looked at the line_table related sections, but see comments below: On Tue, Oct 11, ...
10 years, 7 months ago (2011-10-13 21:56:28 UTC) #2
Diego Novillo
On 11-10-13 17:55 , Gabriel Charette wrote: > I'm not sure exactly how you skip ...
10 years, 7 months ago (2011-10-14 12:16:07 UTC) #3
gcharette1
Yes, I understand that. But when the second 2.pph is skipped when reading foo.pph, the ...
10 years, 7 months ago (2011-10-15 00:27:41 UTC) #4
Diego Novillo
On Fri, Oct 14, 2011 at 20:27, Gabriel Charette <gcharette1@gmail.com> wrote: > Yes, I understand ...
10 years, 7 months ago (2011-10-17 13:04:14 UTC) #5
gcharette1
I just thought about something.. Earlier I said that ALL line_table issues were resolved after ...
10 years, 7 months ago (2011-10-21 03:09:03 UTC) #6
Lawrence Crowl
On 10/20/11, Gabriel Charette <gcharette1@gmail.com> wrote: > I just thought about something.. > > Earlier ...
10 years, 7 months ago (2011-10-21 18:44:00 UTC) #7
gcharette1
10 years, 7 months ago (2011-10-22 18:22:20 UTC) #8
On Fri, Oct 21, 2011 at 2:43 PM, Lawrence Crowl <crowl@google.com> wrote:

> 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.
>
>
Hmmm I'm not sure when/where you'd want to use that whitelist of headers?
Only allow non-pph headers that are whitelisted, all others would have to be
pph'ed (otherwise we'd output a warning/error)??

That would only fix (i.e. avoid) the problem described above if the only
whitelisted headers are NOT ifdef guarded, and thus re-including them does
create a second line_table entry as expected.

I like the idea of forcing people to write pph'able ifdef guarded headers as
much as possible though.

Gab
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b