[libclang] Allow skipping function bodies in preamble only

[nik]

Summary

As an addition to CXTranslationUnit_SkipFunctionBodies, provide the
new option CXTranslationUnit_LimitSkipFunctionBodiesToPreamble,
which constraints the skipping of functions bodies to the preamble
only. Function bodies in the main file are not affected if this
option is set.

Skipping function bodies only in the preamble is what clangd already
does and the introduced flag implements it for libclang clients.

[nik]

@ilya: Using SkipFunctionBodies_AllExceptTemplates for the preamble might be also useful for clangd.

[ilya-biryukov]

In D45815#1072094, @nik wrote:

@ilya: Using SkipFunctionBodies_AllExceptTemplates for the preamble might be also useful for clangd.

Unfortunately, that's also the biggest performance win you get when skipping the function bodies.
I.e. most codebases do not have function bodies in headers unless the function is template.

Have you measured the performance gain of skipping only non-template functions, but not skipping the template ones?
Would that really make a significant difference for any real codebase?

include/clang/Frontend/SkipFunctionBodies.h

18 ↗

(On Diff #143086)

Maybe make it enum class and remove the name prefix from enumerators?
I.e. enum class SkipFunctionBodiesKind { None, ....

[nik]

In D45815#1073418, @ilya-biryukov wrote:

In D45815#1072094, @nik wrote:

@ilya: Using SkipFunctionBodies_AllExceptTemplates for the preamble might be also useful for clangd.

Unfortunately, that's also the biggest performance win you get when skipping the function bodies.
I.e. most codebases do not have function bodies in headers unless the function is template.

Have you measured the performance gain of skipping only non-template functions, but not skipping the template ones?
Would that really make a significant difference for any real codebase?

Here are some rough numbers, based on LIBCLANG_TIMING=1 output: https://dpaste.de/os39/raw

Reparses of Qt Creator's texteditor.cpp
Without any skipping of function bodies in the preamble: 1.9s
Skipping all function bodies in the preamble: 1.0s
Skipping all function bodies in the preamble except function template bodies: 1.2s

Reparses of Clang's SemaDecl.cpp
Without any skipping of function bodies in the preamble: 1.7s
Skipping all function bodies in the preamble: 0.59s
Skipping all function bodies in the preamble except function template bodies: 0.64s

Hmm, that indicates that template function bodies are actually not that
dominant, which I also haven't thought. Now I'm doubting the correctness of
my patch/tests.

I'll recheck this on Tuesday. Until then, you/someone might want to give this also a shot.

include/clang/Frontend/SkipFunctionBodies.h

18 ↗

(On Diff #143086)

Will do.

[ilya-biryukov]

In D45815#1073581, @nik wrote:

Hmm, that indicates that template function bodies are actually not that
dominant, which I also haven't thought. Now I'm doubting the correctness of
my patch/tests.

The numbers are definitely interesting, we need to double-check if that it's the case. But they definitely make a good point for including this option.
It seems Qt and LLVM are exactly the types of codebases where skipping the templates doesn't bring much value, as most of the code is not templated. On the other hand, skipping inline function might potentially be a win for those.

I'm still tempted to say that we should either skip all bodies or none of them to keep the code simpler, but I see why having errors from template instantiations is preferable.

[ilya-biryukov]

include/clang/Frontend/ASTUnit.h

685

Default ctor will be generated automatically. Maybe remove explicit definition?

include/clang/Frontend/FrontendOptions.h

302 ↗

(On Diff #143086)

Maybe add a comment to match the code style of other options?

lib/Frontend/ASTUnit.cpp

1644

Maybe use LLVM's make_scope_exit from ADT/ScopeExit.h instead of creating a separate RAII class?
Or even directy set/restore the value of the flag right in the function.
Given that LLVM does not use exceptions, RAII class does not seem to buy much in terms of correctness and doesn't really make the code easier to read, IMO.

But up to you.

lib/Parse/Parser.cpp

1107 ↗

(On Diff #143086)

Requested in this name is a bit confusing, e.g. it's not clear who requested it.
Maybe rename to TrySkipFunctionBody or something similar?

[yvvan]

include/clang/Frontend/ASTUnit.h

685

or replace by constructor with parameters

[nik]

In D45815#1076434, @ilya-biryukov wrote:

In D45815#1073581, @nik wrote:

Hmm, that indicates that template function bodies are actually not that
dominant, which I also haven't thought. Now I'm doubting the correctness of
my patch/tests.

The numbers are definitely interesting, we need to double-check if that it's the case. But they definitely make a good point for including this option.
It seems Qt and LLVM are exactly the types of codebases where skipping the templates doesn't bring much value, as most of the code is not templated. On the other hand, skipping inline function might potentially be a win for those.

I'm still tempted to say that we should either skip all bodies or none of them to keep the code simpler, but I see why having errors from template instantiations is preferable.

OK, I've rechecked this change. I don't see any obvious mistake :)

In my previous tests/timings I had some modifications to the clang command line applied (extra diagnostics enabled with -Weverything and maybe others, using -isystem instead of -I). I've reverted these and made some runs/timings with only "-Wall -Wextra" (https://dpaste.de/cZgZ/raw) - The difference between skip-all-in-preamble and skip-only-non-template-in-preamble becomes even smaller now for Qt Creator's texteditor.cpp.

include/clang/Frontend/ASTUnit.h

685

The explicit definition is needed to workaround https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58328 ("SkipFunctionBodiesOptions()" is used as default value in some function signatures)

685

That would not really help in this case.

include/clang/Frontend/FrontendOptions.h

302 ↗

(On Diff #143086)

No comment here is in line with "CodeCompleteOptions CodeCompleteOpts" below. That one has also its comments at definition. Also, I can't come up with a comment that adds more value than the name itself and that does not duplicate the comment from SkipFunctionBodies.h.

lib/Frontend/ASTUnit.cpp

1644

I've added the RAII class solely to minimize duplication. Now I'm just passing the flags to getMainBufferWithPrecompiledPreamble() and set/reset there directly.

lib/Parse/Parser.cpp

1107 ↗

(On Diff #143086)

TrySkipFunctionBody somewhat collides with the function trySkippingFunctionBody(). I'm using "ShouldSkipFunctionBody" now.

[nik]

Addressed inline comments.

[erikjv]

About the timing testing: I'm not very familiar with boost, but it probably has a library that's using templates more than either LLVM or Qt. Maybe you can test one of those too?

LGTM otherwise.

[ilya-biryukov]

OK, I've rechecked this change. I don't see any obvious mistake :)

I think I got to the bottom of it. We didn't expect a big win, because we expect people to not put their non-template code into the header files. Which is probably true.
The problem with our reasoning, is that this patch also skip bodies of non-template functions inside template classes, e.g.

template <class T>
struct vector {
   template <class It>
   void append(T* pos, It begin, It end) {
        / * This function won't be skipped, it is a template. */ 
   }

   void push_back(T const&) {
        /* However, this function will be skipped! It is not a template! */
   }
};

So it's not surprising that we get a big win. Template function are (probably) much more rare than non-template functions inside templates.

However, note that we will still miss a lot of diagnostics if we skip bodies of non-template functions inside templates.
So I don't see much value in an option like this: it still compromises correctness of diagnostics even inside the main file, while still missing out some performance opportunities that come from skipping the template functions. And it makes the code more complex.

We should either have an option that does not skip functions inside template classes or keep the current boolean value.
We should definitely measure the performance impact of having such an option before adding more complexity to the code.
WDYT?

BTW, maybe split this change out into two patches: one that allows to skip function bodies only in the preamble, the other that turns SkipFunctionBodies boolean into a enum and changes parser, etc.
The ASTUnit changes LG and we could probably LGTM them sooner than the parser changes.

lib/Parse/ParseCXXInlineMethods.cpp

104 ↗

(On Diff #143738)

Can't this be a template too? Do we need to check for it here?

[nik]

In D45815#1079327, @ilya-biryukov wrote:

OK, I've rechecked this change. I don't see any obvious mistake :)

I think I got to the bottom of it. We didn't expect a big win, because we expect people to not put their non-template code into the header files. Which is probably true.
The problem with our reasoning, is that this patch also skip bodies of non-template functions inside template classes, e.g.

template <class T>
struct vector {
   template <class It>
   void append(T* pos, It begin, It end) {
        / * This function won't be skipped, it is a template. */ 
   }

   void push_back(T const&) {
        /* However, this function will be skipped! It is not a template! */
   }
};

So it's not surprising that we get a big win. Template function are (probably) much more rare than non-template functions inside templates.

Oh, right. That makes sense. I didn't have a test for this case and thus didn't notice.

However, note that we will still miss a lot of diagnostics if we skip bodies of non-template functions inside templates.
So I don't see much value in an option like this: it still compromises correctness of diagnostics even inside the main file, while still missing out some performance opportunities that come from skipping the template functions. And it makes the code more complex.

We should either have an option that does not skip functions inside template classes or keep the current boolean value.
We should definitely measure the performance impact of having such an option before adding more complexity to the code.
WDYT?

BTW, maybe split this change out into two patches: one that allows to skip function bodies only in the preamble, the other that turns SkipFunctionBodies boolean into a enum and changes parser, etc.
The ASTUnit changes LG and we could probably LGTM them sooner than the parser changes.

Agree. I'll first reduce this change to the skip-in-preamble-only functionality and then will check some numbers with the new case.

[nik]

Reduction to skip-in-preamble-only functionality.

[nik]

lib/Parse/ParseCXXInlineMethods.cpp

104 ↗

(On Diff #143738)

Correct. Here is also the point were we would check for the parent class being a template.

[nik]

OK, to skip all function bodies in the preamble except for template functions and functions within a template class, I've amended the previous diff with:

• a/lib/Parse/ParseCXXInlineMethods.cpp +++ b/lib/Parse/ParseCXXInlineMethods.cpp @@ -102,9 +102,14 @@ NamedDecl *Parser::ParseCXXInlineMethodDef(AccessSpecifier AS, }
  
  if (SkipFunctionBodies != SkipFunctionBodiesKind::None && + TemplateInfo.Kind == ParsedTemplateInfo::NonTemplate && + !isa<ClassTemplateDecl>(getCurrentClass().TagOrTemplate) && + !isa<ClassTemplateSpecializationDecl>(getCurrentClass().TagOrTemplate) && + !isa<ClassTemplatePartialSpecializationDecl>( + getCurrentClass().TagOrTemplate) && (!FnD || Actions.canSkipFunctionBody(FnD)) && trySkippingFunctionBody()) { Actions.ActOnSkippedFunctionBody(FnD);

I'm not sure whether this covers all cases, but here are the timing in relation to others:

Reparse for skipping all function bodies: 0.2s
Reparse for skipping all function bodies except template functions (previous patch set, not handling function bodies within template classes): 0.27s
Reparse for skipping all function bodies with the diff above (fixing another case + ignoring function bodies in templates): 0.44s
Reparse without skipping any function bodies in the preamble: 0.51s

As I said, I'm not sure whether this diff covered all cases, but it can get only more worse than 0.44s :) That's enough for me to stop here.

[nik]

Trying to format the diff in the previous comment:

--- a/lib/Parse/ParseCXXInlineMethods.cpp
+++ b/lib/Parse/ParseCXXInlineMethods.cpp
@@ -102,9 +102,14 @@ NamedDecl *Parser::ParseCXXInlineMethodDef(AccessSpecifier AS,
   }
 
   if (SkipFunctionBodies != SkipFunctionBodiesKind::None &&
+      TemplateInfo.Kind == ParsedTemplateInfo::NonTemplate &&
+      !isa<ClassTemplateDecl>(getCurrentClass().TagOrTemplate) &&
+      !isa<ClassTemplateSpecializationDecl>(getCurrentClass().TagOrTemplate) &&
+      !isa<ClassTemplatePartialSpecializationDecl>(
+          getCurrentClass().TagOrTemplate) &&
       (!FnD || Actions.canSkipFunctionBody(FnD)) && trySkippingFunctionBody()) {
     Actions.ActOnSkippedFunctionBody(FnD);

[nik]

Do I miss something? I've uploaded a new diff/version and state is still "(X) Requested Changes to Prior Diff".

Waiting for review.

[ilya-biryukov]

include/clang/Frontend/ASTUnit.h

107

Maybe move this out of ASTUnit? Would allow removing the first qualifier in usages outside the class itself (i.e. ASTUnit::SkipFunctionBodiesScope::Preamble will be shorter!)

370

NIT: Maybe keep the name SkipFunctionBodies? It seems that putting Scp at the end does not add any clarity.

832

This parameter seems slightly out of place.
The scope of skipped function bodies should be a property of the whole ASTUnit, changing it on every reparse would mean more complexity and wouldn't be used anyway.

I suggest storing a field of type SkipFunctionBodiesScope instead of adding parameters to Reparse and LoadFromCompilerInvocation.

[ilya-biryukov]

Sorry for the delay

[nik]

Addressed comments.

[nik]

include/clang/Frontend/ASTUnit.h

370

IIRC I've done that to better distinguish it from e.g. "getFrontendOpts().SkipFunctionBodies", but yes, they are not so often used together, removed "Scp" suffix.

832

Agree, this makes the change significantly shorter :)

[ilya-biryukov]

LGTM

include/clang-c/Index.h

enum CXTranslationUnit_Flags {
    * When fatal errors are encountered while parsing a translation unit,
    * semantic analysis is typically stopped early when compiling code. A common
    * source for fatal errors are unresolvable include files. For the
    * purposes of an IDE, this is undesirable behavior and as much information
    * as possible should be reported. Use this flag to enable this behavior.
    */
   CXTranslationUnit_KeepGoing = 0x200,
 
-  /**
-   * Sets the preprocessor in a mode for parsing a single file only.
-   */
-  CXTranslationUnit_SingleFileParse = 0x400
-};
-
-/**
+  /**
+   * Sets the preprocessor in a mode for parsing a single file only.
+   */
+  CXTranslationUnit_SingleFileParse = 0x400,
+
+  /**
+   * \brief Used in combination with CXTranslationUnit_SkipFunctionBodies to
+   * constrain the skipping of function bodies to the preamble.
+   *
+   * The function bodies of the main file are not skipped.
+   */
+  CXTranslationUnit_LimitSkipFunctionBodiesToPreamble = 0x800,
+};
+
+/**
  * Returns the set of flags that is suitable for parsing a translation
  * unit that is being edited.
  *
  * The set of flags returned provide options for \c clang_parseTranslationUnit()
  * to indicate that the translation unit is likely to be reparsed many times,
  * either explicitly (via \c clang_reparseTranslationUnit()) or implicitly
  * (e.g., by code completion (\c clang_codeCompletionAt())). The returned flag
  * set contains an unspecified set of optimizations (e.g., the precompiled 

include/clang/Frontend/ASTUnit.h

 class Preprocessor;
 class PreprocessorOptions;
 class Sema;
 class TargetInfo;
 
 namespace vfs {
 
 class FileSystem;
-
-} // namespace vfs
-
-/// Utility class for loading a ASTContext from an AST file.
-class ASTUnit {
-public:
+
+} // namespace vfs
+
+/// \brief Enumerates the available scopes for skipping function bodies.
+enum class SkipFunctionBodiesScope { None, Preamble, PreambleAndMainFile };
+
+/// Utility class for loading a ASTContext from an AST file.
+class ASTUnit {
+public:
   struct StandaloneFixIt {
     std::pair<unsigned, unsigned> RemoveRange;
     std::pair<unsigned, unsigned> InsertFromRange;
     std::string CodeToInsert;
     bool BeforePreviousInsertions;
   };
 
   struct StandaloneDiagnostic {
     unsigned ID;
     DiagnosticsEngine::Level Level;
     std::string Message;
     std::string Filename;
     unsigned LocOffset;
     std::vector<std::pair<unsigned, unsigned>> Ranges;
     std::vector<StandaloneFixIt> FixIts;
   };
 
 private:
   std::shared_ptr<LangOptions>            LangOpts;
   IntrusiveRefCntPtr<DiagnosticsEngine>   Diagnostics;
   IntrusiveRefCntPtr<FileManager>         FileMgr;
   IntrusiveRefCntPtr<SourceManager>       SourceMgr;
   IntrusiveRefCntPtr<MemoryBufferCache>   PCMCache;
   std::unique_ptr<HeaderSearch>           HeaderInfo;
   IntrusiveRefCntPtr<TargetInfo>          Target;
   std::shared_ptr<Preprocessor>           PP;
private:
   /// global code-completion cache after a rebuild of the precompiled preamble.
   unsigned PreambleTopLevelHashValue = 0;
 
   /// The current hash value for the top-level declaration and macro
   /// definition names
   unsigned CurrentTopLevelHashValue = 0;
   
   /// Bit used by CIndex to mark when a translation unit may be in an
-  /// inconsistent state, and is not safe to free.
-  unsigned UnsafeToFree : 1;
-
-  /// Cache any "global" code-completion results, so that we can avoid
-  /// recomputing them with each completion.
-  void CacheCodeCompletionResults();
+  /// inconsistent state, and is not safe to free.
+  unsigned UnsafeToFree : 1;
+
+  /// \brief Enumerator specifying the scope for skipping function bodies.
+  SkipFunctionBodiesScope SkipFunctionBodies = SkipFunctionBodiesScope::None;
+
+  /// Cache any "global" code-completion results, so that we can avoid
+  /// recomputing them with each completion.
+  void CacheCodeCompletionResults();
   
   /// Clear out and deallocate 
   void ClearCachedCompletionResults();
   
   explicit ASTUnit(bool MainFileIsAST);
 
   bool Parse(std::shared_ptr<PCHContainerOperations> PCHContainerOps,
              std::unique_ptr<llvm::MemoryBuffer> OverrideMainBuffer,
              IntrusiveRefCntPtr<vfs::FileSystem> VFS);
-
-  std::unique_ptr<llvm::MemoryBuffer> getMainBufferWithPrecompiledPreamble(
-      std::shared_ptr<PCHContainerOperations> PCHContainerOps,
-      const CompilerInvocation &PreambleInvocationIn,
-      IntrusiveRefCntPtr<vfs::FileSystem> VFS, bool AllowRebuild = true,
-      unsigned MaxLines = 0);
-  void RealizeTopLevelDeclsFromPreamble();
+
+  std::unique_ptr<llvm::MemoryBuffer> getMainBufferWithPrecompiledPreamble(
+      std::shared_ptr<PCHContainerOperations> PCHContainerOps,
+      CompilerInvocation &PreambleInvocationIn,
+      IntrusiveRefCntPtr<vfs::FileSystem> VFS, bool AllowRebuild = true,
+      unsigned MaxLines = 0);
+  void RealizeTopLevelDeclsFromPreamble();
 
   /// Transfers ownership of the objects (like SourceManager) from
   /// \param CI to this ASTUnit.
   void transferASTDataFromCompilerInstance(CompilerInstance &CI);
 
   /// Allows us to assert that ASTUnit is not being used concurrently,
   /// which is not supported.
   ///
static std::unique_ptr<ASTUnit> LoadFromASTFile(
       const std::string &Filename, const PCHContainerReader &PCHContainerRdr,
       WhatToLoad ToLoad, IntrusiveRefCntPtr<DiagnosticsEngine> Diags,
       const FileSystemOptions &FileSystemOpts, bool UseDebugInfo = false,
       bool OnlyLocalDecls = false, ArrayRef<RemappedFile> RemappedFiles = None,
       bool CaptureDiagnostics = false, bool AllowPCHWithCompilerErrors = false,
       bool UserFilesAreVolatile = false);
 
 private:
   /// Helper function for \c LoadFromCompilerInvocation() and
   /// \c LoadFromCommandLine(), which loads an AST from a compiler invocation.
   ///
   /// \param PrecompilePreambleAfterNParses After how many parses the preamble
   /// of this translation unit should be precompiled, to improve the performance
   /// of reparsing. Set to zero to disable preambles.
   ///
   /// \param VFS - A vfs::FileSystem to be used for all file accesses. Note that
   /// preamble is saved to a temporary directory on a RealFileSystem, so in order
public:
   static ASTUnit *LoadFromCommandLine(
       const char **ArgBegin, const char **ArgEnd,
       std::shared_ptr<PCHContainerOperations> PCHContainerOps,
       IntrusiveRefCntPtr<DiagnosticsEngine> Diags, StringRef ResourceFilesPath,
       bool OnlyLocalDecls = false, bool CaptureDiagnostics = false,
       ArrayRef<RemappedFile> RemappedFiles = None,
       bool RemappedFilesKeepOriginalName = true,
       unsigned PrecompilePreambleAfterNParses = 0,
-      TranslationUnitKind TUKind = TU_Complete,
-      bool CacheCodeCompletionResults = false,
-      bool IncludeBriefCommentsInCodeCompletion = false,
-      bool AllowPCHWithCompilerErrors = false, bool SkipFunctionBodies = false,
-      bool SingleFileParse = false,
-      bool UserFilesAreVolatile = false, bool ForSerialization = false,
-      llvm::Optional<StringRef> ModuleFormat = llvm::None,
-      std::unique_ptr<ASTUnit> *ErrAST = nullptr,
-      IntrusiveRefCntPtr<vfs::FileSystem> VFS = nullptr);
+      TranslationUnitKind TUKind = TU_Complete,
+      bool CacheCodeCompletionResults = false,
+      bool IncludeBriefCommentsInCodeCompletion = false,
+      bool AllowPCHWithCompilerErrors = false,
+      SkipFunctionBodiesScope SkipFunctionBodies =
+          SkipFunctionBodiesScope::None,
+      bool SingleFileParse = false, bool UserFilesAreVolatile = false,
+      bool ForSerialization = false,
+      llvm::Optional<StringRef> ModuleFormat = llvm::None,
+      std::unique_ptr<ASTUnit> *ErrAST = nullptr,
+      IntrusiveRefCntPtr<vfs::FileSystem> VFS = nullptr);
 
   /// Reparse the source files using the same command-line options that
   /// were originally used to produce this translation unit.
   ///
   /// \param VFS - A vfs::FileSystem to be used for all file accesses. Note that
   /// preamble is saved to a temporary directory on a RealFileSystem, so in order
   /// for it to be loaded correctly, VFS should give an access to this(i.e. be an
   /// overlay over RealFileSystem). FileMgr->getVirtualFileSystem() will be used if
   /// \p VFS is nullptr.
   ///
   /// \returns True if a failure occurred that causes the ASTUnit not to
   /// contain any translation-unit information, false otherwise.
   bool Reparse(std::shared_ptr<PCHContainerOperations> PCHContainerOps,
                ArrayRef<RemappedFile> RemappedFiles = None,
                IntrusiveRefCntPtr<vfs::FileSystem> VFS = nullptr);
 
   /// Free data that will be re-generated on the next parse.
   ///
   /// Preamble-related data is not affected.
   void ResetForParse();
 
   /// Perform code completion at the given file, line, and
   /// column within this translation unit.

lib/Frontend/ASTUnit.cpp

 /// out-of-date.
 ///
 /// \param MaxLines When non-zero, the maximum number of lines that
 /// can occur within the preamble.
 ///
 /// \returns If the precompiled preamble can be used, returns a newly-allocated
 /// buffer that should be used in place of the main file when doing so.
 /// Otherwise, returns a NULL pointer.
-std::unique_ptr<llvm::MemoryBuffer>
-ASTUnit::getMainBufferWithPrecompiledPreamble(
-    std::shared_ptr<PCHContainerOperations> PCHContainerOps,
-    const CompilerInvocation &PreambleInvocationIn,
-    IntrusiveRefCntPtr<vfs::FileSystem> VFS, bool AllowRebuild,
-    unsigned MaxLines) {
-  auto MainFilePath =
+std::unique_ptr<llvm::MemoryBuffer>
+ASTUnit::getMainBufferWithPrecompiledPreamble(
+    std::shared_ptr<PCHContainerOperations> PCHContainerOps,
+    CompilerInvocation &PreambleInvocationIn,
+    IntrusiveRefCntPtr<vfs::FileSystem> VFS, bool AllowRebuild,
+    unsigned MaxLines) {
+  auto MainFilePath =
       PreambleInvocationIn.getFrontendOpts().Inputs[0].getFile();
   std::unique_ptr<llvm::MemoryBuffer> MainFileBuffer =
       getBufferForFileHandlingRemapping(PreambleInvocationIn, VFS.get(),
                                         MainFilePath);
   if (!MainFileBuffer)
     return nullptr;
 
   PreambleBounds Bounds =
ASTUnit::getMainBufferWithPrecompiledPreamble(
   ASTUnitPreambleCallbacks Callbacks;
   {
     llvm::Optional<CaptureDroppedDiagnostics> Capture;
     if (CaptureDiagnostics)
       Capture.emplace(/*RequestCapture=*/true, *Diagnostics, &NewPreambleDiags,
                       &NewPreambleDiagsStandalone);
 
     // We did not previously compute a preamble, or it can't be reused anyway.
-    SimpleTimer PreambleTimer(WantTiming);
-    PreambleTimer.setOutput("Precompiling preamble");
-
-    llvm::ErrorOr<PrecompiledPreamble> NewPreamble = PrecompiledPreamble::Build(
-        PreambleInvocationIn, MainFileBuffer.get(), Bounds, *Diagnostics, VFS,
-        PCHContainerOps, /*StoreInMemory=*/false, Callbacks);
-    if (NewPreamble) {
-      Preamble = std::move(*NewPreamble);
-      PreambleRebuildCounter = 1;
+    SimpleTimer PreambleTimer(WantTiming);
+    PreambleTimer.setOutput("Precompiling preamble");
+
+    const bool PreviousSkipFunctionBodies =
+        PreambleInvocationIn.getFrontendOpts().SkipFunctionBodies;
+    if (SkipFunctionBodies == SkipFunctionBodiesScope::Preamble)
+      PreambleInvocationIn.getFrontendOpts().SkipFunctionBodies = true;
+
+    llvm::ErrorOr<PrecompiledPreamble> NewPreamble = PrecompiledPreamble::Build(
+        PreambleInvocationIn, MainFileBuffer.get(), Bounds, *Diagnostics, VFS,
+        PCHContainerOps, /*StoreInMemory=*/false, Callbacks);
+
+    PreambleInvocationIn.getFrontendOpts().SkipFunctionBodies =
+        PreviousSkipFunctionBodies;
+
+    if (NewPreamble) {
+      Preamble = std::move(*NewPreamble);
+      PreambleRebuildCounter = 1;
     } else {
       switch (static_cast<BuildPreambleError>(NewPreamble.getError().value())) {
       case BuildPreambleError::CouldntCreateTempFile:
       case BuildPreambleError::PreambleIsEmpty:
         // Try again next time.
         PreambleRebuildCounter = 1;
         return nullptr;
       case BuildPreambleError::CouldntCreateTargetInfo:
bool ASTUnit::LoadFromCompilerInvocation(
   getDiagnostics().Reset();
   ProcessWarningOptions(getDiagnostics(), Invocation->getDiagnosticOpts());
 
   std::unique_ptr<llvm::MemoryBuffer> OverrideMainBuffer;
   if (PrecompilePreambleAfterNParses > 0) {
     PreambleRebuildCounter = PrecompilePreambleAfterNParses;
     OverrideMainBuffer =
         getMainBufferWithPrecompiledPreamble(PCHContainerOps, *Invocation, VFS);
     getDiagnostics().Reset();
     ProcessWarningOptions(getDiagnostics(), Invocation->getDiagnosticOpts());
   }
 
   SimpleTimer ParsingTimer(WantTiming);
   ParsingTimer.setOutput("Parsing " + getMainFileName());
 
   // Recover resources if we crash before exiting this method.
   llvm::CrashRecoveryContextCleanupRegistrar<llvm::MemoryBuffer>
std::unique_ptr<ASTUnit> ASTUnit::LoadFromCompilerInvocation(
   return AST;
 }
 
 ASTUnit *ASTUnit::LoadFromCommandLine(
     const char **ArgBegin, const char **ArgEnd,
     std::shared_ptr<PCHContainerOperations> PCHContainerOps,
     IntrusiveRefCntPtr<DiagnosticsEngine> Diags, StringRef ResourceFilesPath,
     bool OnlyLocalDecls, bool CaptureDiagnostics,
-    ArrayRef<RemappedFile> RemappedFiles, bool RemappedFilesKeepOriginalName,
-    unsigned PrecompilePreambleAfterNParses, TranslationUnitKind TUKind,
-    bool CacheCodeCompletionResults, bool IncludeBriefCommentsInCodeCompletion,
-    bool AllowPCHWithCompilerErrors, bool SkipFunctionBodies,
-    bool SingleFileParse, bool UserFilesAreVolatile, bool ForSerialization,
-    llvm::Optional<StringRef> ModuleFormat, std::unique_ptr<ASTUnit> *ErrAST,
-    IntrusiveRefCntPtr<vfs::FileSystem> VFS) {
+    ArrayRef<RemappedFile> RemappedFiles, bool RemappedFilesKeepOriginalName,
+    unsigned PrecompilePreambleAfterNParses, TranslationUnitKind TUKind,
+    bool CacheCodeCompletionResults, bool IncludeBriefCommentsInCodeCompletion,
+    bool AllowPCHWithCompilerErrors, SkipFunctionBodiesScope SkipFunctionBodies,
+    bool SingleFileParse, bool UserFilesAreVolatile, bool ForSerialization,
+    llvm::Optional<StringRef> ModuleFormat, std::unique_ptr<ASTUnit> *ErrAST,
+    IntrusiveRefCntPtr<vfs::FileSystem> VFS) {
   assert(Diags.get() && "no DiagnosticsEngine was provided");
 
   SmallVector<StoredDiagnostic, 4> StoredDiagnostics;
 
   std::shared_ptr<CompilerInvocation> CI;
 
   {
     CaptureDroppedDiagnostics Capture(CaptureDiagnostics, *Diags,
for (const auto &RemappedFile : RemappedFiles) {
     CI->getPreprocessorOpts().addRemappedFile(RemappedFile.first,
                                               RemappedFile.second);
   }
   PreprocessorOptions &PPOpts = CI->getPreprocessorOpts();
   PPOpts.RemappedFilesKeepOriginalName = RemappedFilesKeepOriginalName;
   PPOpts.AllowPCHWithCompilerErrors = AllowPCHWithCompilerErrors;
   PPOpts.SingleFileParseMode = SingleFileParse;
 
-  // Override the resources path.
-  CI->getHeaderSearchOpts().ResourceDir = ResourceFilesPath;
-
-  CI->getFrontendOpts().SkipFunctionBodies = SkipFunctionBodies;
-
-  if (ModuleFormat)
-    CI->getHeaderSearchOpts().ModuleFormat = ModuleFormat.getValue();
+  // Override the resources path.
+  CI->getHeaderSearchOpts().ResourceDir = ResourceFilesPath;
+
+  CI->getFrontendOpts().SkipFunctionBodies =
+      SkipFunctionBodies == SkipFunctionBodiesScope::PreambleAndMainFile;
+
+  if (ModuleFormat)
+    CI->getHeaderSearchOpts().ModuleFormat = ModuleFormat.getValue();
 
   // Create the AST unit.
   std::unique_ptr<ASTUnit> AST;
   AST.reset(new ASTUnit(false));
   AST->NumStoredDiagnosticsFromDriver = StoredDiagnostics.size();
   AST->StoredDiagnostics.swap(StoredDiagnostics);
   ConfigureDiags(Diags, *AST, CaptureDiagnostics);
   AST->Diagnostics = Diags;
   AST->FileSystemOpts = CI->getFileSystemOpts();
   if (!VFS)
     VFS = vfs::getRealFileSystem();
   VFS = createVFSFromCompilerInvocation(*CI, *Diags, VFS);
   AST->FileMgr = new FileManager(AST->FileSystemOpts, VFS);
   AST->PCMCache = new MemoryBufferCache;
   AST->OnlyLocalDecls = OnlyLocalDecls;
   AST->CaptureDiagnostics = CaptureDiagnostics;
   AST->TUKind = TUKind;
   AST->ShouldCacheCodeCompletionResults = CacheCodeCompletionResults;
   AST->IncludeBriefCommentsInCodeCompletion
-    = IncludeBriefCommentsInCodeCompletion;
-  AST->UserFilesAreVolatile = UserFilesAreVolatile;
-  AST->Invocation = CI;
-  if (ForSerialization)
-    AST->WriterData.reset(new ASTWriterData(*AST->PCMCache));
-  // Zero out now to ease cleanup during crash recovery.
+    = IncludeBriefCommentsInCodeCompletion;
+  AST->UserFilesAreVolatile = UserFilesAreVolatile;
+  AST->Invocation = CI;
+  AST->SkipFunctionBodies = SkipFunctionBodies;
+  if (ForSerialization)
+    AST->WriterData.reset(new ASTWriterData(*AST->PCMCache));
+  // Zero out now to ease cleanup during crash recovery.
   CI = nullptr;
   Diags = nullptr;
 
   // Recover resources if we crash before exiting this method.
   llvm::CrashRecoveryContextCleanupRegistrar<ASTUnit>
     ASTUnitCleanup(AST.get());
 
   if (AST->LoadFromCompilerInvocation(std::move(PCHContainerOps),

test/Parser/skip-function-bodies.h

+int header1(int t) {
+  return t;
+}

test/Parser/skip-function-bodies.mm

-// RUN: env CINDEXTEST_SKIP_FUNCTION_BODIES=1 c-index-test -test-load-source all %s | FileCheck %s
-
-class A {
-  class B {};
+#include "skip-function-bodies.h"
+
+class A {
+  class B {};
 
 public:
   A() {
     struct C {
       void d() {}
     };
   }
 
   typedef B E;
 };
 
 @interface F
 - (void) G;
 @end
 @implementation F
 - (void) G {
   typedef A H;
   class I {};
 }
 @end
 
 void J() {
-  class K {};
-}
-
-// CHECK: skip-function-bodies.mm:3:7: ClassDecl=A:3:7 (Definition) Extent=[3:1 - 14:2]
-// CHECK: skip-function-bodies.mm:4:9: ClassDecl=B:4:9 (Definition) Extent=[4:3 - 4:13]
-// CHECK: skip-function-bodies.mm:6:1: CXXAccessSpecifier=:6:1 (Definition) Extent=[6:1 - 6:8]
+  class K {};
+}
+
+// RUN: env CINDEXTEST_SKIP_FUNCTION_BODIES=1 c-index-test -test-load-source all %s | FileCheck %s
+// CHECK: skip-function-bodies.mm:3:7: ClassDecl=A:3:7 (Definition) Extent=[3:1 - 14:2]
+// CHECK: skip-function-bodies.mm:4:9: ClassDecl=B:4:9 (Definition) Extent=[4:3 - 4:13]
+// CHECK: skip-function-bodies.mm:6:1: CXXAccessSpecifier=:6:1 (Definition) Extent=[6:1 - 6:8]
 // CHECK: skip-function-bodies.mm:7:3: CXXConstructor=A:7:3 (default constructor) Extent=[7:3 - 7:6]
 // CHECK-NOT: skip-function-bodies.mm:8:12: StructDecl=C:8:12 (Definition) Extent=[8:5 - 10:6]
 // CHECK-NOT: skip-function-bodies.mm:9:12: CXXMethod=d:9:12 (Definition) Extent=[9:7 - 9:18]
 // CHECK: skip-function-bodies.mm:13:13: TypedefDecl=E:13:13 (Definition) Extent=[13:3 - 13:14]
 // CHECK: skip-function-bodies.mm:13:11: TypeRef=class A::B:4:9 Extent=[13:11 - 13:12]
 // CHECK: skip-function-bodies.mm:16:12: ObjCInterfaceDecl=F:16:12 Extent=[16:1 - 18:5]
 // CHECK: skip-function-bodies.mm:17:10: ObjCInstanceMethodDecl=G:17:10 Extent=[17:1 - 17:12]
 // CHECK: skip-function-bodies.mm:19:17: ObjCImplementationDecl=F:19:17 (Definition) Extent=[19:1 - 24:2]
 // CHECK: skip-function-bodies.mm:20:10: ObjCInstanceMethodDecl=G:20:10 Extent=[20:1 - 20:13]
 // CHECK-NOT: skip-function-bodies.mm:21:13: TypedefDecl=H:21:13 (Definition) Extent=[21:3 - 21:14]
-// CHECK-NOT: skip-function-bodies.mm:21:11: TypeRef=class A:3:7 Extent=[21:11 - 21:12]
-// CHECK: skip-function-bodies.mm:26:6: FunctionDecl=J:26:6 Extent=[26:1 - 26:9]
-// CHECK-NOT: skip-function-bodies.mm:27:9: ClassDecl=K:27:9 (Definition) Extent=[27:3 - 27:13]
+// CHECK-NOT: skip-function-bodies.mm:21:11: TypeRef=class A:3:7 Extent=[21:11 - 21:12]
+// CHECK: skip-function-bodies.mm:26:6: FunctionDecl=J:26:6 Extent=[26:1 - 26:9]
+// CHECK-NOT: skip-function-bodies.mm:27:9: ClassDecl=K:27:9 (Definition) Extent=[27:3 - 27:13]
+
+// RUN: env CINDEXTEST_EDITING=1 \
+// RUN:  CINDEXTEST_CREATE_PREAMBLE_ON_FIRST_PARSE=1 \
+// RUN:  CINDEXTEST_SKIP_FUNCTION_BODIES=1 \
+// RUN:  CINDEXTEST_LIMIT_SKIP_FUNCTION_BODIES_TO_PREAMBLE=1 \
+// RUN:  c-index-test -test-load-source all %s | FileCheck -check-prefix=CHECK-PREAMBLE %s
+// CHECK-PREAMBLE: skip-function-bodies.h:1:5: FunctionDecl=header1:1:5 Extent=[1:1 - 1:19]
+// CHECK-PREAMBLE-NOT: skip-function-bodies.h:2:3: ReturnStmt= Extent=[2:3 - 2:11]
+// CHECK-PREAMBLE: skip-function-bodies.mm:8:12: StructDecl=C:8:12 (Definition) Extent=[8:5 - 10:6]
+// CHECK-PREAMBLE: skip-function-bodies.mm:9:12: CXXMethod=d:9:12 (Definition) Extent=[9:7 - 9:18]

tools/c-index-test/c-index-test.c

if (getenv("CINDEXTEST_COMPLETION_CACHING"))
     options |= CXTranslationUnit_CacheCompletionResults;
   if (getenv("CINDEXTEST_COMPLETION_NO_CACHING"))
     options &= ~CXTranslationUnit_CacheCompletionResults;
   if (getenv("CINDEXTEST_SKIP_FUNCTION_BODIES"))
     options |= CXTranslationUnit_SkipFunctionBodies;
   if (getenv("CINDEXTEST_COMPLETION_BRIEF_COMMENTS"))
     options |= CXTranslationUnit_IncludeBriefCommentsInCodeCompletion;
   if (getenv("CINDEXTEST_CREATE_PREAMBLE_ON_FIRST_PARSE"))
-    options |= CXTranslationUnit_CreatePreambleOnFirstParse;
-  if (getenv("CINDEXTEST_KEEP_GOING"))
-    options |= CXTranslationUnit_KeepGoing;
-
-  return options;
-}
+    options |= CXTranslationUnit_CreatePreambleOnFirstParse;
+  if (getenv("CINDEXTEST_KEEP_GOING"))
+    options |= CXTranslationUnit_KeepGoing;
+  if (getenv("CINDEXTEST_LIMIT_SKIP_FUNCTION_BODIES_TO_PREAMBLE"))
+    options |= CXTranslationUnit_LimitSkipFunctionBodiesToPreamble;
+
+  return options;
+}
 
 static void ModifyPrintingPolicyAccordingToEnv(CXPrintingPolicy Policy) {
   struct Mapping {
     const char *name;
     enum CXPrintingPolicyProperty property;
   };
   struct Mapping mappings[] = {
       {"CINDEXTEST_PRINTINGPOLICY_INDENTATION", CXPrintingPolicy_Indentation},

tools/libclang/CIndex.cpp

clang_parseTranslationUnit_Impl(CXIndex CIdx, const char *source_filename,
   bool PrecompilePreamble = options & CXTranslationUnit_PrecompiledPreamble;
   bool CreatePreambleOnFirstParse =
       options & CXTranslationUnit_CreatePreambleOnFirstParse;
   // FIXME: Add a flag for modules.
   TranslationUnitKind TUKind
     = (options & (CXTranslationUnit_Incomplete |
                   CXTranslationUnit_SingleFileParse))? TU_Prefix : TU_Complete;
   bool CacheCodeCompletionResults
-    = options & CXTranslationUnit_CacheCompletionResults;
-  bool IncludeBriefCommentsInCodeCompletion
-    = options & CXTranslationUnit_IncludeBriefCommentsInCodeCompletion;
-  bool SkipFunctionBodies = options & CXTranslationUnit_SkipFunctionBodies;
-  bool SingleFileParse = options & CXTranslationUnit_SingleFileParse;
-  bool ForSerialization = options & CXTranslationUnit_ForSerialization;
-
-  // Configure the diagnostics.
-  IntrusiveRefCntPtr<DiagnosticsEngine>
+    = options & CXTranslationUnit_CacheCompletionResults;
+  bool IncludeBriefCommentsInCodeCompletion
+    = options & CXTranslationUnit_IncludeBriefCommentsInCodeCompletion;
+  bool SingleFileParse = options & CXTranslationUnit_SingleFileParse;
+  bool ForSerialization = options & CXTranslationUnit_ForSerialization;
+  SkipFunctionBodiesScope SkipFunctionBodies = SkipFunctionBodiesScope::None;
+  if (options & CXTranslationUnit_SkipFunctionBodies) {
+    SkipFunctionBodies =
+        (options & CXTranslationUnit_LimitSkipFunctionBodiesToPreamble)
+            ? SkipFunctionBodiesScope::Preamble
+            : SkipFunctionBodiesScope::PreambleAndMainFile;
+  }
+
+  // Configure the diagnostics.
+  IntrusiveRefCntPtr<DiagnosticsEngine>
     Diags(CompilerInstance::createDiagnostics(new DiagnosticOptions));
 
   if (options & CXTranslationUnit_KeepGoing)
     Diags->setSuppressAfterFatalError(false);
 
   // Recover resources if we crash before exiting this function.
   llvm::CrashRecoveryContextCleanupRegistrar<DiagnosticsEngine,
     llvm::CrashRecoveryContextReleaseRefCleanup<DiagnosticsEngine> >