Merge some of the PCH object support with modular codegen

[dblaikie]

Summary

I was trying to pick this up a bit when reviewing D48426 (& perhaps D69778) - in any case, looks like D48426 added a module level flag that might not be needed.

The D48426 implementation worked by setting a module level flag, then code generating contents from the PCH a special case in ASTContext::DeclMustBeEmitted would be used to delay emitting the definition of these functions if they came from a Module with this flag.

This strategy is similar to the one initially implemented for modular codegen that was removed in D29901 in favor of the modular decls list and a bit on each decl to specify whether it's homed to a module.

One major difference between PCH object support and modular code generation, other than the specific list of decls that are homed, is the compilation model: MSVC PCH modules are built into the object file for some other source file (when compiling that source file /Yc is specified to say "this compilation is where the PCH is homed"), whereas modular code generation invokes a separate compilation for the PCH alone. So the current modular code generation test of to decide if a decl should be emitted "is the module where this decl is serialized the current main file" has to be extended (as Lubos did in D69778) to also test the command line flag -building-pch-with-obj.

Otherwise the whole thing is basically streamlined down to the modular code generation path.

This even offers one extra material improvement compared to the existing divergent implementation: Homed functions are not emitted into object files that use the pch. Instead at -O0 they are not emitted into the IR at all, and at -O1 they are emitted using available_externally (existing functionality implemented for modular code generation). The pch-codegen test has been updated to reflect this new behavior.

[If possible: I'd love it if we could not have the extra MSVC-style way of accessing dllexport-pch-homing, and just do it the modular codegen way, but I understand that it might be a limitation of existing build systems. @hans / @thakis: Do either of you know if it'd be practical to move to something more similar to .pcm handling, where the pch itself is passed to the compilation, rather than homed as a side effect of compiling some other source file?]

[llunak]

The patch is incomplete, isn't it? It removes DeclIsFromPCHWithObjectFile(), but it's still called from ASTContext::DeclMustBeEmitted(). The description also mentions updating of the pch-codegen test, but that's not included.

But assuming this is intended to replace the D48426 functionality with modular codegen, I mentioned already in D69778 (starting with 'Mar 17 2020, 10:59 PM') that it's a question if this is possible. Modular codegen is a much bigger hammer than D48426 and it has also bigger possible problems: Possibly making compilation actually slower (even f63556d8b44b209e741833b0e6be3f8e72a1d91a mentions this), and possibly causing build problems because of referencing private symbols. D48426 is much smaller both in gains and in problems (=safer).

Do either of you know if it'd be practical to move to something more similar to .pcm handling, where the pch itself is passed to the compilation, rather than homed as a side effect of compiling some other source file?

Do you mean dropping compatibility with 'cl.exe /Yc'?

[hans]

In D83652#2146732, @llunak wrote:

Do either of you know if it'd be practical to move to something more similar to .pcm handling, where the pch itself is passed to the compilation, rather than homed as a side effect of compiling some other source file?

Do you mean dropping compatibility with 'cl.exe /Yc'?

I don't think we can change the interface, because we want cl.exe compatibility.

[dblaikie]

Include all the commits

[dblaikie]

In D83652#2146732, @llunak wrote:

The patch is incomplete, isn't it? It removes DeclIsFromPCHWithObjectFile(), but it's still called from ASTContext::DeclMustBeEmitted(). The description also mentions updating of the pch-codegen test, but that's not included.

Ah, right you are - arc diff only took the last commit on this branch. I've updated it/used arc correctly to get the whole branch worth of changes.

But assuming this is intended to replace the D48426 functionality with modular codegen,

Not quite - it's intended to implement the D48426 functionality using an implementation strategy that is closer to modular code generation. Removing the need for the module flag, using the MODULAR_CODEGEN_DECLS list, etc. But only putting the dllexported entities in there (when using only building-pch-with-obj without -fmodules-codegen).

The intent is not to change the behavior of D48426, but to simplify the implementation a bit by reducing the divergence between the two features.

Do either of you know if it'd be practical to move to something more similar to .pcm handling, where the pch itself is passed to the compilation, rather than homed as a side effect of compiling some other source file?

Do you mean dropping compatibility with 'cl.exe /Yc'?

Yep, that was essentially what I was wondering - since you were proposing a build mode/flags that would not be /Yc compatible, adding an explicit pch build and pch object build step - was wondering if the build system support for that was sufficiently available that it would be practical for users (like Chromium) to migrate to that kind of model and no longer need /Yc compatibility. That would further reduce some of the divergence between the two features (then it'd come down to only a "what entities do we want to home" question - the mechanics of homing them, building their object file, etc, would all be the same).

In D83652#2147097, @hans wrote:

In D83652#2146732, @llunak wrote:

Do either of you know if it'd be practical to move to something more similar to .pcm handling, where the pch itself is passed to the compilation, rather than homed as a side effect of compiling some other source file?

Do you mean dropping compatibility with 'cl.exe /Yc'?

I don't think we can change the interface, because we want cl.exe compatibility.

Fair enough - I guess the only other way to get the cl.exe behavior from the Clang driver using modular codegen-type functionality would be to use 4 steps:

• build PCH (with dllexport modular codegen)
• build PCH object file using PCH
• build source object using PCH
• ld -r the two objects together

But I guess the added driver complexity isn't worth the simplification inside Clang proper, probably. I guess we already have the "build PCH then use PCH to build object" part so /maybe/ it'd be feasible to extend it in this way.

[llunak]

In D83652#2147539, @dblaikie wrote:

Not quite - it's intended to implement the D48426 functionality using an implementation strategy that is closer to modular code generation. Removing the need for the module flag, using the MODULAR_CODEGEN_DECLS list, etc. But only putting the dllexported entities in there (when using only building-pch-with-obj without -fmodules-codegen).

I see. I still suggest testing this on a real codebase for the reasons outlined above.

Yep, that was essentially what I was wondering - since you were proposing a build mode/flags that would not be /Yc compatible, adding an explicit pch build and pch object build step - was wondering if the build system support for that was sufficiently available that it would be practical for users (like Chromium) to migrate to that kind of model and no longer need /Yc compatibility.

Not that I know for sure, but I personally doubt there's build system support for D83716. The only build system I've tried is LibreOffice's custom gbuild, where I've added the support for -fpch-codegen/debuginfo myself, and even there's I'm going to stick with -building-pch-with-obj, since it's simpler to compile yet another .cxx file with an extra flag rather than have a build step with .pch as yet another kind of input file. But /Yc is presumably supported by pretty much anything on Windows.

[dblaikie]

@hans - could you perhaps give me a quick summary of commands I could use to test this feature in Chromium (or anything else you might suggest) on a Linux box? I don't have a Windows machine, or any projects that use PCH. (or if you'd be willing to test this, that'd be great - see if the PCH object file has the same symbols before/after this patch)

[hans]

In D83652#2159585, @dblaikie wrote:

@hans - could you perhaps give me a quick summary of commands I could use to test this feature in Chromium (or anything else you might suggest) on a Linux box? I don't have a Windows machine, or any projects that use PCH. (or if you'd be willing to test this, that'd be great - see if the PCH object file has the same symbols before/after this patch)

I tried to do this today, and then realized Chromium's PCH .obj files are basically empty these days. Back when I did D48426, dllexported inline functions were emitted in the pch .obj file, but then Chromium started using /Zc:dllexportInlines- which means we don't dllexport inlines anymore, and even if I remove that flag we've moved to only including libc++ headers in our PCH, and nothing is dllexport there.

After applying and building with your patch, the PCH .obj files look the same.

I also verified that building one of Chromium's test targets succeeds with this patch.

[dblaikie]

In D83652#2161924, @hans wrote:

In D83652#2159585, @dblaikie wrote:

@hans - could you perhaps give me a quick summary of commands I could use to test this feature in Chromium (or anything else you might suggest) on a Linux box? I don't have a Windows machine, or any projects that use PCH. (or if you'd be willing to test this, that'd be great - see if the PCH object file has the same symbols before/after this patch)

I tried to do this today, and then realized Chromium's PCH .obj files are basically empty these days. Back when I did D48426, dllexported inline functions were emitted in the pch .obj file, but then Chromium started using /Zc:dllexportInlines- which means we don't dllexport inlines anymore, and even if I remove that flag we've moved to only including libc++ headers in our PCH, and nothing is dllexport there.

After applying and building with your patch, the PCH .obj files look the same.

I also verified that building one of Chromium's test targets succeeds with this patch.

Thanks for checking! So, in summary: This seems to work fine for Chromium, but Chromium isn't really exercising this functionality (only insofar as the functionality is enabled, but essentially a no-op)?

@llunak Would you be able to test this on anything you've got?

[llunak]

@llunak Would you be able to test this on anything you've got?

No, but thinking more about this, I think dllexport specifically voids the possible problems I listed. If I'm getting it right, dllexport is used only for code in the current library, so codegen won't create code referencing private symbols in other libraries, and dllexport means the code will be emitted anyway, so no unnecessary code generating. So I'm actually fine with the patch as it is.

[hans]

Thanks for checking! So, in summary: This seems to work fine for Chromium, but Chromium isn't really exercising this functionality (only insofar as the functionality is enabled, but essentially a no-op)?

Yup.

[dblaikie]

@hans @llunak - sounds like you're both fine with this? Either of you mind to give it a formal approval, if that's the case?

clang/include/clang/AST/ExternalASTSource.h

public:
   /// context is up to date.
   ///
   /// The default implementation of this function is a no-op.
   virtual void completeVisibleDeclsMap(const DeclContext *DC);
 
   /// Retrieve the module that corresponds to the given module ID.
   virtual Module *getModule(unsigned ID) { return nullptr; }
 
-  /// Determine whether D comes from a PCH which was built with a corresponding
-  /// object file.
-  virtual bool DeclIsFromPCHWithObjectFile(const Decl *D) { return false; }
-
   /// Return a descriptor for the corresponding module, if one exists.
   virtual llvm::Optional<ASTSourceDescriptor> getSourceDescriptor(unsigned ID);
 
   enum ExtKind { EK_Always, EK_Never, EK_ReplyHazy };
 
   virtual ExtKind hasExternalDefinitions(const Decl *D);
 
   /// Finds all declarations lexically contained within the given

clang/include/clang/Sema/MultiplexExternalSemaSource.h

public:
 
   /// Print any statistics that have been gathered regarding
   /// the external AST source.
   void PrintStats() override;
 
   /// Retrieve the module that corresponds to the given module ID.
   Module *getModule(unsigned ID) override;
 
-  bool DeclIsFromPCHWithObjectFile(const Decl *D) override;
-
   /// Perform layout on the given record.
   ///
   /// This routine allows the external AST source to provide an specific
   /// layout for a record, overriding the layout that would normally be
   /// constructed. It is intended for clients who receive specific layout
   /// details rather than source code (such as LLDB). The client is expected
   /// to fill in the field offsets, base offsets, virtual base offsets, and
   /// complete object size.

clang/include/clang/Serialization/ASTReader.h

public:
   ///
   Module *getSubmodule(serialization::SubmoduleID GlobalID);
 
   /// Retrieve the module that corresponds to the given module ID.
   ///
   /// Note: overrides method in ExternalASTSource
   Module *getModule(unsigned ID) override;
 
-  bool DeclIsFromPCHWithObjectFile(const Decl *D) override;
-
   /// Retrieve the module file with a given local ID within the specified
   /// ModuleFile.
   ModuleFile *getLocalModuleFile(ModuleFile &M, unsigned ID);
 
   /// Get an ID for the given module file.
   unsigned getModuleFileID(ModuleFile *M);
 
   /// Return a descriptor for the corresponding module.

clang/include/clang/Serialization/ModuleFile.h

public:
   std::string ModuleMapPath;
 
   /// Whether this precompiled header is a relocatable PCH file.
   bool RelocatablePCH = false;
 
   /// Whether timestamps are included in this module file.
   bool HasTimestamps = false;
 
-  /// Whether the PCH has a corresponding object file.
-  bool PCHHasObjectFile = false;
-
   /// Whether the top-level module has been read from the AST file.
   bool DidReadTopLevelSubmodule = false;
 
   /// The file entry for the module file.
   const FileEntry *File = nullptr;
 
   /// The signature of the module file, which may be used instead of the size
   /// and modification time to identify this particular file.

clang/lib/AST/ASTContext.cpp

else if (isa<OMPAllocateDecl>(D))
     return !D->getDeclContext()->isDependentContext();
   else if (isa<OMPDeclareReductionDecl>(D) || isa<OMPDeclareMapperDecl>(D))
     return !D->getDeclContext()->isDependentContext();
   else if (isa<ImportDecl>(D))
     return true;
   else
     return false;
 
-  if (D->isFromASTFile() && !LangOpts.BuildingPCHWithObjectFile) {
-    assert(getExternalSource() && "It's from an AST file; must have a source.");
-    // On Windows, PCH files are built together with an object file. If this
-    // declaration comes from such a PCH and DeclMustBeEmitted would return
-    // true, it would have returned true and the decl would have been emitted
-    // into that object file, so it doesn't need to be emitted here.
-    // Note that decls are still emitted if they're referenced, as usual;
-    // DeclMustBeEmitted is used to decide whether a decl must be emitted even
-    // if it's not referenced.
-    //
-    // Explicit template instantiation definitions are tricky. If there was an
-    // explicit template instantiation decl in the PCH before, it will look like
-    // the definition comes from there, even if that was just the declaration.
-    // (Explicit instantiation defs of variable templates always get emitted.)
-    bool IsExpInstDef =
-        isa<FunctionDecl>(D) &&
-        cast<FunctionDecl>(D)->getTemplateSpecializationKind() ==
-            TSK_ExplicitInstantiationDefinition;
-
-    // Implicit member function definitions, such as operator= might not be
-    // marked as template specializations, since they're not coming from a
-    // template but synthesized directly on the class.
-    IsExpInstDef |=
-        isa<CXXMethodDecl>(D) &&
-        cast<CXXMethodDecl>(D)->getParent()->getTemplateSpecializationKind() ==
-            TSK_ExplicitInstantiationDefinition;
-
-    if (getExternalSource()->DeclIsFromPCHWithObjectFile(D) && !IsExpInstDef)
-      return false;
-  }
-
   // If this is a member of a class template, we do not need to emit it.
   if (D->getDeclContext()->isDependentContext())
     return false;
 
   // Weak references don't produce any output by themselves.
   if (D->hasAttr<WeakRefAttr>())
     return false;
 

clang/lib/Sema/MultiplexExternalSemaSource.cpp

 
 Module *MultiplexExternalSemaSource::getModule(unsigned ID) {
   for (size_t i = 0; i < Sources.size(); ++i)
     if (auto M = Sources[i]->getModule(ID))
       return M;
   return nullptr;
 }
 
-bool MultiplexExternalSemaSource::DeclIsFromPCHWithObjectFile(const Decl *D) {
-  for (auto *S : Sources)
-    if (S->DeclIsFromPCHWithObjectFile(D))
-      return true;
-  return false;
-}
-
 bool MultiplexExternalSemaSource::layoutRecordType(const RecordDecl *Record,
                                                    uint64_t &Size,
                                                    uint64_t &Alignment,
                       llvm::DenseMap<const FieldDecl *, uint64_t> &FieldOffsets,
                   llvm::DenseMap<const CXXRecordDecl *, CharUnits> &BaseOffsets,
           llvm::DenseMap<const CXXRecordDecl *, CharUnits> &VirtualBaseOffsets){
   for(size_t i = 0; i < Sources.size(); ++i)
     if (Sources[i]->layoutRecordType(Record, Size, Alignment, FieldOffsets,

clang/lib/Serialization/ASTReader.cpp

while (true) {
     case METADATA: {
       if (Record[0] != VERSION_MAJOR && !DisableValidation) {
         if ((ClientLoadCapabilities & ARR_VersionMismatch) == 0)
           Diag(Record[0] < VERSION_MAJOR? diag::err_pch_version_too_old
                                         : diag::err_pch_version_too_new);
         return VersionMismatch;
       }
 
-      bool hasErrors = Record[7];
+      bool hasErrors = Record[6];
       if (hasErrors && !DisableValidation && !AllowASTWithCompilerErrors) {
         Diag(diag::err_pch_with_compiler_errors);
         return HadErrors;
       }
       if (hasErrors) {
         Diags.ErrorOccurred = true;
         Diags.UncompilableErrorOccurred = true;
         Diags.UnrecoverableErrorOccurred = true;
       }
 
       F.RelocatablePCH = Record[4];
       // Relative paths in a relocatable PCH are relative to our sysroot.
       if (F.RelocatablePCH)
         F.BaseDirectory = isysroot.empty() ? "/" : isysroot;
 
       F.HasTimestamps = Record[5];
 
-      F.PCHHasObjectFile = Record[6];
-
       const std::string &CurBranch = getClangFullRepositoryVersion();
       StringRef ASTBranch = Blob;
       if (StringRef(CurBranch) != ASTBranch && !DisableValidation) {
         if ((ClientLoadCapabilities & ARR_VersionMismatch) == 0)
           Diag(diag::err_pch_different_branch) << ASTBranch << CurBranch;
         return VersionMismatch;
       }
       break;
Module *ASTReader::getSubmodule(SubmoduleID GlobalID) {
 
   return SubmodulesLoaded[GlobalID - NUM_PREDEF_SUBMODULE_IDS];
 }
 
 Module *ASTReader::getModule(unsigned ID) {
   return getSubmodule(ID);
 }
 
-bool ASTReader::DeclIsFromPCHWithObjectFile(const Decl *D) {
-  ModuleFile *MF = getOwningModuleFile(D);
-  return MF && MF->PCHHasObjectFile;
-}
-
 ModuleFile *ASTReader::getLocalModuleFile(ModuleFile &F, unsigned ID) {
   if (ID & 1) {
     // It's a module, look it up by submodule ID.
     auto I = GlobalSubmoduleMap.find(getGlobalSubmoduleID(F, ID >> 1));
     return I == GlobalSubmoduleMap.end() ? nullptr : I->second;
   } else {
     // It's a prefix (preamble, PCH, ...). Look it up by index.
     unsigned IndexFromEnd = ID >> 1;

clang/lib/Serialization/ASTReaderDecl.cpp

 }
 
 uint64_t ASTDeclReader::GetCurrentCursorOffset() {
   return Loc.F->DeclsCursor.GetCurrentBitNo() + Loc.F->GlobalBitOffset;
 }
 
 void ASTDeclReader::ReadFunctionDefinition(FunctionDecl *FD) {
   if (Record.readInt()) {
-    Reader.DefinitionSource[FD] = Loc.F->Kind == ModuleKind::MK_MainFile;
-    if (Reader.getContext().getLangOpts().BuildingPCHWithObjectFile &&
-        Reader.DeclIsFromPCHWithObjectFile(FD))
-      Reader.DefinitionSource[FD] = true;
+    Reader.DefinitionSource[FD] =
+        Loc.F->Kind == ModuleKind::MK_MainFile ||
+        Reader.getContext().getLangOpts().BuildingPCHWithObjectFile;
   }
   if (auto *CD = dyn_cast<CXXConstructorDecl>(FD)) {
     CD->setNumCtorInitializers(Record.readInt());
     if (CD->getNumCtorInitializers())
       CD->CtorInitializers = ReadGlobalOffset();
   }
   // Store the offset of the body so we can lazily load it later.
   Reader.PendingBodies[FD] = GetCurrentCursorOffset();
ASTDeclReader::RedeclarableResult ASTDeclReader::VisitVarDeclImpl(VarDecl *VD) {
 
   if (VD->hasAttr<BlocksAttr>() && VD->getType()->getAsCXXRecordDecl()) {
     Expr *CopyExpr = Record.readExpr();
     if (CopyExpr)
       Reader.getContext().setBlockVarCopyInit(VD, CopyExpr, Record.readInt());
   }
 
   if (VD->getStorageDuration() == SD_Static && Record.readInt()) {
-    Reader.DefinitionSource[VD] = Loc.F->Kind == ModuleKind::MK_MainFile;
-    if (Reader.getContext().getLangOpts().BuildingPCHWithObjectFile &&
-        Reader.DeclIsFromPCHWithObjectFile(VD))
-      Reader.DefinitionSource[VD] = true;
+    Reader.DefinitionSource[VD] =
+        Loc.F->Kind == ModuleKind::MK_MainFile ||
+        Reader.getContext().getLangOpts().BuildingPCHWithObjectFile;
   }
 
   enum VarKind {
     VarNotTemplate = 0, VarTemplate, StaticDataMemberSpecialization
   };
   switch ((VarKind)Record.readInt()) {
   case VarNotTemplate:
     // Only true variables (not parameters or implicit parameters) can be
void ASTDeclReader::ReadCXXDefinitionData(
   Data.Name = Record.readInt();
   #include "clang/AST/CXXRecordDeclDefinitionBits.def"
 
   // Note: the caller has deserialized the IsLambda bit already.
   Data.ODRHash = Record.readInt();
   Data.HasODRHash = true;
 
   if (Record.readInt()) {
-    Reader.DefinitionSource[D] = Loc.F->Kind == ModuleKind::MK_MainFile;
-    if (Reader.getContext().getLangOpts().BuildingPCHWithObjectFile &&
-        Reader.DeclIsFromPCHWithObjectFile(D))
-      Reader.DefinitionSource[D] = true;
+    Reader.DefinitionSource[D] = 
+        Loc.F->Kind == ModuleKind::MK_MainFile ||
+        Reader.getContext().getLangOpts().BuildingPCHWithObjectFile;
   }
 
   Data.NumBases = Record.readInt();
   if (Data.NumBases)
     Data.Bases = ReadGlobalOffset();
   Data.NumVBases = Record.readInt();
   if (Data.NumVBases)
     Data.VBases = ReadGlobalOffset();

clang/lib/Serialization/ASTWriter.cpp

void ASTWriter::WriteControlBlock(Preprocessor &PP, ASTContext &Context,
   auto MetadataAbbrev = std::make_shared<BitCodeAbbrev>();
   MetadataAbbrev->Add(BitCodeAbbrevOp(METADATA));
   MetadataAbbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 16)); // Major
   MetadataAbbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 16)); // Minor
   MetadataAbbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 16)); // Clang maj.
   MetadataAbbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 16)); // Clang min.
   MetadataAbbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // Relocatable
   MetadataAbbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // Timestamps
-  MetadataAbbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // PCHHasObjectFile
   MetadataAbbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // Errors
   MetadataAbbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Blob)); // SVN branch/tag
   unsigned MetadataAbbrevCode = Stream.EmitAbbrev(std::move(MetadataAbbrev));
   assert((!WritingModule || isysroot.empty()) &&
          "writing module as a relocatable PCH?");
   {
     RecordData::value_type Record[] = {
         METADATA,
         VERSION_MAJOR,
         VERSION_MINOR,
         CLANG_VERSION_MAJOR,
         CLANG_VERSION_MINOR,
         !isysroot.empty(),
         IncludeTimestamps,
-        Context.getLangOpts().BuildingPCHWithObjectFile,
         ASTHasCompilerErrors};
     Stream.EmitRecordWithBlob(MetadataAbbrevCode, Record,
                               getClangFullRepositoryVersion());
   }
 
   if (WritingModule) {
     // Module name
     auto Abbrev = std::make_shared<BitCodeAbbrev>();

clang/lib/Serialization/ASTWriterDecl.cpp

if (D->getStorageDuration() == SD_Static) {
     if (Writer.WritingModule &&
         !D->getDescribedVarTemplate() && !D->getMemberSpecializationInfo() &&
         !isa<VarTemplateSpecializationDecl>(D)) {
       // When building a C++ Modules TS module interface unit, a strong
       // definition in the module interface is provided by the compilation of
       // that module interface unit, not by its users. (Inline variables are
       // still emitted in module users.)
       ModulesCodegen =
-          (Writer.WritingModule->Kind == Module::ModuleInterfaceUnit &&
-           Writer.Context->GetGVALinkageForVariable(D) == GVA_StrongExternal);
+          (Writer.WritingModule->Kind == Module::ModuleInterfaceUnit ||
+           (D->hasAttr<DLLExportAttr>() &&
+            Writer.Context->getLangOpts().BuildingPCHWithObjectFile)) &&
+          Writer.Context->GetGVALinkageForVariable(D) == GVA_StrongExternal;
     }
     Record.push_back(ModulesCodegen);
     if (ModulesCodegen)
       Writer.ModularCodegenDecls.push_back(Writer.GetDeclRef(D));
   }
 
   enum {
     VarNotTemplate = 0, VarTemplate, StaticDataMemberSpecialization
if (Writer->WritingModule &&
         Writer->WritingModule->Kind == Module::ModuleInterfaceUnit) {
       // When building a C++ Modules TS module interface unit, a strong
       // definition in the module interface is provided by the compilation of
       // that module interface unit, not by its users. (Inline functions are
       // still emitted in module users.)
       Linkage = Writer->Context->GetGVALinkageForFunction(FD);
       ModulesCodegen = *Linkage == GVA_StrongExternal;
     }
-    if (Writer->Context->getLangOpts().ModulesCodegen) {
+    if (Writer->Context->getLangOpts().ModulesCodegen ||
+        (FD->hasAttr<DLLExportAttr>() &&
+         Writer->Context->getLangOpts().BuildingPCHWithObjectFile)) {
+
       // Under -fmodules-codegen, codegen is performed for all non-internal,
       // non-always_inline functions, unless they are available elsewhere.
       if (!FD->hasAttr<AlwaysInlineAttr>()) {
         if (!Linkage)
           Linkage = Writer->Context->GetGVALinkageForFunction(FD);
         ModulesCodegen =
             *Linkage != GVA_Internal && *Linkage != GVA_AvailableExternally;
       }

clang/test/CodeGen/pch-dllexport.cpp

 // Build PCH without object file, then use it.
 // RUN: %clang_cc1 -triple i686-pc-win32 -fms-extensions -emit-pch -o %t %s
 // RUN: %clang_cc1 -triple i686-pc-win32 -fms-extensions -emit-obj -emit-llvm -include-pch %t -o - %s | FileCheck -check-prefix=PCH %s
 
 // Build PCH with object file, then use it.
-// RUN: %clang_cc1 -triple i686-pc-win32 -fms-extensions -emit-pch -building-pch-with-obj -o %t %s
-// RUN: %clang_cc1 -triple i686-pc-win32 -fms-extensions -emit-obj -emit-llvm -include-pch %t -building-pch-with-obj -o - %s | FileCheck -check-prefix=OBJ %s
-// RUN: %clang_cc1 -triple i686-pc-win32 -fms-extensions -emit-obj -emit-llvm -include-pch %t -o - %s | FileCheck -check-prefix=PCHWITHOBJ %s
+// RUN: %clang_cc1 -triple i686-pc-win32 -O1 -fms-extensions -emit-pch -building-pch-with-obj -o %t %s
+// RUN: %clang_cc1 -triple i686-pc-win32 -O1 -disable-llvm-optzns -fms-extensions -emit-obj -emit-llvm -include-pch %t -building-pch-with-obj -o - %s | FileCheck -check-prefix=OBJ %s
+// RUN: %clang_cc1 -triple i686-pc-win32 -O1 -disable-llvm-optzns -fms-extensions -emit-obj -emit-llvm -include-pch %t -o - %s | FileCheck -check-prefix=PCHWITHOBJ -check-prefix=PCHWITHOBJ-O1 %s
 
 // Check for vars separately to avoid having to reorder the check statements.
+// RUN: %clang_cc1 -triple i686-pc-win32 -O1 -disable-llvm-optzns -fms-extensions -emit-obj -emit-llvm -include-pch %t -o - %s | FileCheck -check-prefix=PCHWITHOBJVARS %s
+
+// Test the PCHWITHOBJ at -O0 where available_externally definitions are not
+// provided:
+// RUN: %clang_cc1 -triple i686-pc-win32 -fms-extensions -emit-pch -building-pch-with-obj -o %t %s
+// RUN: %clang_cc1 -triple i686-pc-win32 -fms-extensions -emit-obj -emit-llvm -include-pch %t -o - %s | FileCheck -check-prefix=PCHWITHOBJ -check-prefix=PCHWITHOBJ-O0 %s
 // RUN: %clang_cc1 -triple i686-pc-win32 -fms-extensions -emit-obj -emit-llvm -include-pch %t -o - %s | FileCheck -check-prefix=PCHWITHOBJVARS %s
 
+
 #ifndef IN_HEADER
 #define IN_HEADER
 
 inline void __declspec(dllexport) foo() {}
 // OBJ: define weak_odr dso_local dllexport void @"?foo@@YAXXZ"
 // PCH: define weak_odr dso_local dllexport void @"?foo@@YAXXZ"
 // PCHWITHOBJ-NOT: define {{.*}}foo
 
 
 // This function is referenced, so gets emitted as usual.
 inline void __declspec(dllexport) baz() {}
 // OBJ: define weak_odr dso_local dllexport void @"?baz@@YAXXZ"
 // PCH: define weak_odr dso_local dllexport void @"?baz@@YAXXZ"
-// PCHWITHOBJ: define weak_odr dso_local dllexport void @"?baz@@YAXXZ"
+// PCHWITHOBJ-O1: define available_externally dso_local void @"?baz@@YAXXZ"
+// PCHWITHOBJ-O0-NOT: define {{.*}}"?baz@@YAXXZ"
 
 
 struct __declspec(dllexport) S {
   void bar() {}
 // OBJ: define weak_odr dso_local dllexport x86_thiscallcc void @"?bar@S@@QAEXXZ"
 // PCH: define weak_odr dso_local dllexport x86_thiscallcc void @"?bar@S@@QAEXXZ"
 // PCHWITHOBJ-NOT: define {{.*}}bar
 };