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

Jordan Rose jordan_rose at apple.com
Tue Jul 24 11:19:36 PDT 2012


On Jul 23, 2012, at 14:10 , Andy Gibbs <andyg1001 at hotmail.co.uk> wrote:

> On Friday, July 20, 2012 8:59 PM, Andy Gibbs wrote:
>> On Friday, July 20, 2012 6:04 PM, Jordan Rose wrote:
>>> On Jul 17, 2012, at 9:18 AM, Andy Gibbs wrote:
>>>> 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!...
> 
> No, I was wrong.  It is FrontendActciont::BeginSourceFile where the
> problem exists.  What happens is there is a "goto failure" error handler
> which calls "EndSourceFile" even if "BeginSourceFile" hasn't yet been
> called.  Below is a proposed patch.
> 
> I can then change my previous patch to assert on mismatch of BeginSourceFile
> and EndSourceFile.  Were you happy with my other responses to your
> reservations?  If you, shall I now post a revised patch incorporating all
> those changes?

You missed a few BeginSourceFile calls, but the basic approach seems right. And I really wouldn't track the mismatch of BeginSourceFile and EndSourceFile—it's not really VerifyDiagnosticConsumer's job. But I like this fix a lot—we should have been doing the right thing all along.


Back to your first replies...

> 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.

Better, but still confusing. I would actually just cut everything after "for example [if] it is a precompiled header". That's probably enough to help anyone looking at this code later, and people who make the mistake probably won't look here right away to see what's going on. And after all, the usual fix isn't to forward the comments, it's to move the directives to the main source file.


>> +      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.


Okay, makes sense.

Please do send a revised patch now! And thanks, as always.
Jordan

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120724/5708c86f/attachment.html>


More information about the cfe-commits mailing list