Fix for preprocessor CommentHandler bug

Clang Frontend
#1

Hi,

Following hot on the heels of my previous preprocessor patch, here is one
that solves issues related to the fact that the CommentHandler interface is
not invoked if comments follow #else and #endif preprocessor directives,
since it can be that the preprocessor is still in skip (i.e. raw) mode while
processing the directive line.

This fix solves this problem by bringing the preprocessor out of raw mode
while it processes to the end of the directive line.

The patch causes no test-suite regressions.

Cheers
Andy

commenthandler.diff (1.75 KB)

#2

Following hot on the heels of my previous preprocessor patch, here is one
that solves issues related to the fact that the CommentHandler interface is
not invoked if comments follow #else and #endif preprocessor directives,
since it can be that the preprocessor is still in skip (i.e. raw) mode while
processing the directive line.

This fix solves this problem by bringing the preprocessor out of raw mode
while it processes to the end of the directive line.

The patch causes no test-suite regressions.

<clippy>It looks like you're trying to fix a bug - would you like to
include a test case?</clippy>

& is this a fix for: http://llvm.org/bugs/show_bug.cgi?id=13065 by
chance? I'm curious what caused the bug as I'd assumed it was due to
my commit ( https://llvm.org/svn/llvm-project/cfe/trunk@158093 ). But
judging by the fix, that might not be the case.

- David

#3

No, the bug was uncovered while making an enhancement patch for the "-verify" feature, where I changed from the use of the separate parse step to using the CommentHandler interface. Unfortunately, the CommentHandler interface was not invoked in some cases where comments followed #else or #endif directives, and this meant I needed to alter some pre-existing test-cases to enable them to work with my patch. This fix was directed at removing this necessity.

In reply, then, to the missing test-cases, I have test-cases as part of my patch for the new "-verify" feature, since this actually uses the CommentHandler interface and can directly test it. I accept that this is a weak reply, but my hope is that with the approval of this and my other recent preprocessor patch, then my "-verify" patch can also meet with approval -- in which case the test-cases will appear as part of that.

Alternatively, is there a better way to test the CommentHandler interface? If so, I can supply some specific test-cases for this patch.

Thanks
Andy

#4

Hi,

Could I just bring this patch back into the light?

Here is the link to the original patch:
http://lists.cs.uiuc.edu/pipermail/cfe-dev/attachments/20120612/2b7041e4/attachment.obj

Are there any objections to getting this committed?

Thanks,
Andy

#5

Apologies for the delay!

There are two ways in which you might consider testing CommentHandler:

1) Add a gunit test for it (you should be able to mirror the basic
structure of unittests/Tooling/RecursiveASTVisitorTest.cpp). Your
FrontendAction can add a CommentHandler in an override of
BeginInvocation.

2) Add a tool to tools/ which runs -cc1 over a file and dumps out the
comments found. Then we can use FileCheck to check the results.
c-index-test almost gives us this ability already, by allowing parsing
in comment retention mode, but that doesn't find the same set of
comments as the CommentHandler does (and in particular, it doesn't
find comments in preprocessing directives, which makes it
inappropriate for your purposes).

(1) seems like the simpler option.

For the patch itself, would it make sense to move the LexingRawMode
alteration into CheckEndOfDirective?

It also looks like you've changed the behavior of this code:

#if 0
#if 1
#endif junk
#endif

... from producing a warning to being silently accepted. This seems
like an independent bugfix, which should have an accompanying test,
and should probably be committed separately, along with a test that
this:

#if 0
#endif junk

... still warns.

#6

Apologies for the delay!

No problem! Its only because I'm hoping to get my other patch accepted ;o)
(By the way, I am also splitting this up, as requested...)

There are two ways in which you might consider testing
CommentHandler:

1) Add a gunit test for it (you should be able to mirror
the basic structure of unittests/Tooling/RecursiveASTVisitorTest.cpp).
Your FrontendAction can add a CommentHandler in an
override of BeginInvocation.

This is the route I've taken. I should have a new patch together tomorrow
(I've run out of time for today, unfortunately).

For the patch itself, would it make sense to move the
LexingRawMode alteration into CheckEndOfDirective?

Yes, this might make more sense. I'll have a look into it tomorrow.

It also looks like you've changed the behavior of this
code:

#if 0
#if 1
#endif junk
#endif

Well caught! But it is also the case that other #directives inside an #if 0
will also not provide these warnings. Take the following snippet:

#if 0 junk
#if 1 junk
#else junk
#endif junk
#endif junk

Even without my patch, only the first #if and both #endifs produce
diagnostics. The way the preprocessor seems to be designed, in most cases
it simply discards the whole line (comments included) following the
preprocessor directive when inside a skipped block. With the change I made,
one might argue, the diagnostics from the preprocessor are moving towards
more consistenty: i.e. inside a skipped #if block, there are (or should be)
no diagnostics generated. This matches nicely with the fact that we ignore
comment handling inside skipped #if blocks (thinking ahead to the -verify
enhancement...)

But on the other hand, of course, I can easily revert the regression.

What do you think?

Thanks
Andy

#7

It also looks like you've changed the behavior of this
code:

#if 0
#if 1
#endif junk
#endif

Well caught! But it is also the case that other #directives inside an #if 0
will also not provide these warnings. Take the following snippet:

#if 0 junk
#if 1 junk
#else junk
#endif junk
#endif junk

Even without my patch, only the first #if and both #endifs produce
diagnostics. The way the preprocessor seems to be designed, in most cases
it simply discards the whole line (comments included) following the
preprocessor directive when inside a skipped block. With the change I made,
one might argue, the diagnostics from the preprocessor are moving towards
more consistenty: i.e. inside a skipped #if block, there are (or should be)
no diagnostics generated. This matches nicely with the fact that we ignore
comment handling inside skipped #if blocks (thinking ahead to the -verify
enhancement...)

But on the other hand, of course, I can easily revert the regression.

This is a bugfix, not a regression -- the new behavior is the one
mandated by all relevant standards:

"Each directive’s condition is checked in order. If it evaluates to
false (zero), the group that it controls is
skipped: directives are processed only through the name that
determines the directive in order to keep track
of the level of nested conditionals; the rest of the directives’
preprocessing tokens are ignored, as are the other
preprocessing tokens in the group."

What do you think?

I'd like this split out into a separate patch with a testcase.

#8

Sorry, I misunderstood what you had said: I had thought you meant that the
change was a bug needing fixing, not the bugfix!

Anyway, attached is a patch and test-case for this bugfix. I'll re-post the
initial patch with test-case in a few minutes.

Andy

disabled-cond-diags.diff (1.92 KB)

#9

Thanks for splitting this out, I've checked it in as r158883.

#10

Thanks, any chance of also committing:

http://lists.cs.uiuc.edu/pipermail/cfe-dev/2012-June/022290.html

which is the other part.

I can then post a split-up version of my -verify patch and then once we've
got through that, I can post my patch for bug 12850, although that is also
waiting on a fix to 12896 for the full test-case to work.

Thanks,
Andy

#11

Done (with some 80-column fixes) in r159119.