[cfe-commits] [Patch 1 of 4] -verify fixes (stage 2)

[Andy Gibbs]

Hi Jordan,

On Friday, July 20, 2012 6:04 PM, Jordan Rose wrote:
> Hi, Andy. I've been delaying my review of this patch because it doesn't
> exactly sit well with me, but I think it will be the right way to go.
> Comments inline.

I do understand, but your input is hugely appreciated.

> On Jul 17, 2012, at 9:18 AM, Andy Gibbs wrote:
>> 2. As a result, rolled back a change made by Jordan relating to the old
>>   FilesWithDirectives where files were only added if directives were
>>   found.
>
> +
> +#ifndef NDEBUG
> +  if (const FileEntry *E = 
> SM.getFileEntryForID(SM.getFileID(CommentBegin)))
> +    FilesParsedForDirectives.insert(E);
> +#endif
>
> I'm still not so happy about this, because a file with no comments will
> look the same as a PCH or whatever. And it shouldn't be necessary with
> (3) -- every file with comments will have been seen by PPCallbacks. Did
> you have a specific motivating example for this?

Yes, it is fair to say that with (3) it shouldn't be necessary.  If you
would prefer then I can simply remove this whole part from HandleComments,
not just as in the above location, but also where you had it too.  To
answer your question, I left it in as a safe-guard, should (for whatever
reason) the hook via PPCallbacks fail.

I did, however, in response to your other point, include a test-case which
specifically tests that -verify works correctly even with files with no
observable comments (OTOH, "tests/Frontend/verify2.c").

>> 3. Integrated PPCallbacks so that FilesParsedForDirectives also includes
>>   files which have been seen but have no visible directives (e.g. they 
>> are
>>   all inside skipped #if blocks).
>
> More Preprocessor awkwardness, but okay. I'd like to see the Preprocessor
> made more robust some day so that it's acceptable to add and remove
> callbacks even once it's been initialized.
>
>> 4. Ensure that repeated BeginSourceFile calls are handled correctly, as 
>> is
>>   a mismatch between calls of BeginSourceFile and EndSourceFile (it turns
>>   out, the latter is often called twice while the former only once, but
>>   also it is possible for BeginSourceFile to be called more than once 
>> too).
>
> This worries me. If we have unbalanced BeginSourceFile and EndSourceFile
> calls, that might be a bug. Do you know specific circumstances where this
> is the case?

Not off the top of my head.  I'm afraid I'm at home now, so don't have the
code in front of me, plus it was a week ago that I was doing this patch.
If I was going to have a guess, I would hazard that it was inside
FrontendAction::EndSourceFile(), but it would be much better that I
go away and look it up rather than guess!...

>> 5. No longer add directives found during post-processing, but generate an
>>   error if such directives *are* found (which they shouldn't be!).
>
> +    // If a directive has been found but we're not interested
> +    // in storing the directive information, return now.
> +    if (!DL) return true;
>
> Nitpick via LLVM coding style: "return true;" should be on the next line.

My apologies, I must get better at switching coding styles!

>> 6. Only do the post-processing check in a debug/asserts build now since 
>> it
>>   is not otherwise necessary, and in an optimised build we can save the
>>   cost of this check.
>
>
> +/// FoundExpectedDiags - Lex the main source file to find all of the
> +///  expected errors and warnings.
>
> Nitpicking on the name: I don't immediately parse this as a predicate, and
> even then it's doing work. Coming from a Cocoa background, I'd prefer
> didFindExpectedDiags, but even just findExpectedDiags would work too for
> me. Or findDirectives.

Ok, findDirectives it is...

> +#ifndef NDEBUG
> +    // In a debug build, scan through any files that may have been missed
> +    // during parsing and issue a fatal error if diagnostics are 
> contained
> +    // within these files.  This should not occur, and if it does then it
> +    // implies that (some) directives are not being made available during
> +    // normal parsing and first it should be checked why this is the 
> case.
>
> The sentence here is hard to parse. Maybe break out the last clause? I'm
> not even sure what it means myself -- something like "It's possible these
> files are precompiled in some way." or similar.

The point I'm trying to make (albeit badly!) is that the problem is more
likely during the parse stage rather than in the post-processing stage
and as a result, a solution should be sought elsewhere first before making
changes to this part of the code.

How about:

// In a debug build, scan through any files that may have been missed
// during parsing and issue a fatal error if directives are contained
// within these files.  If a fatal error occurs, this suggests that
// this file is being parsed separately from the main file, for example
// it is a precompiled header.  To avoid the fatal error, the comments
// from these files should preferably be extracted at source and then
// forwarded directly to VerifyDiagnosticConsumer for proper processing.
// If this is not appropriate, then as a fallback the files should be
// explicitly ignored here.

> +      if (FoundExpectedDiags(*CurrentPreprocessor, *I))
> +        llvm::report_fatal_error(Twine("-verify directives found after 
> rather"
> +                                       " than during normal parsing of ",
> +                                 StringRef(E ? E->getName() : 
> "(unknown)")));
>
> Is it possible to get a test case for this, or is it really impossible?
> If the latter, you might as well just
> assert(!FoundExpectedDiags(*CurrentPreprocessor, *I)).

If you just apply the first patch and none of the latter patches, then
yes you will hit this fatal error.  Once all the holes are patched, you
shouldn't expect to see it until someone adds a new feature like PCH or
modules!  The reason I went for report_fatal_error over assert was because
then you can get the very informative error which tells you exactly which
source file has caused the error.  I think this has merit.


Thanks for the comments.  Does this answer your points to your
satisfaction?

Andy