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

[Andy Gibbs]

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?

Thanks,
Andy


Index: tools/clang/lib/Frontend/FrontendAction.cpp
==================================================================
--- tools/clang/lib/Frontend/FrontendAction.cpp
+++ tools/clang/lib/Frontend/FrontendAction.cpp
@@ -160,10 +160,11 @@
   assert(!Instance && "Already processing a source file!");
   assert(!Input.File.empty() && "Unexpected empty filename!");
   setCurrentInput(Input);
   setCompilerInstance(&CI);
 
+  bool hasBegunSourceFile = false;
   if (!BeginInvocation(CI))
     goto failure;
 
   // AST files follow a very different path, since they share objects via the
   // AST unit.
@@ -226,10 +227,11 @@
   CI.createPreprocessor();
 
   // Inform the diagnostic client we are processing a source file.
   CI.getDiagnosticClient().BeginSourceFile(CI.getLangOpts(),
                                            &CI.getPreprocessor());
+  hasBegunSourceFile = true;
 
   // Initialize the action.
   if (!BeginSourceFileAction(CI, Input.File))
     goto failure;
 
@@ -307,11 +309,12 @@
     CI.setPreprocessor(0);
     CI.setSourceManager(0);
     CI.setFileManager(0);
   }
 
-  CI.getDiagnosticClient().EndSourceFile();
+  if (hasBegunSourceFile)
+    CI.getDiagnosticClient().EndSourceFile();
   setCurrentInput(FrontendInputFile());
   setCompilerInstance(0);
   return false;
 }