[Serialization] Place command line defines in the correct file

[john.brawn]

Summary

Fix two problems happening during deserialization causing command line defines to be reported as being built-in defines:

• When serializing or deserializing the <built-in> and <command line> files don't convert them into absolute paths.
• When deserializing SM_SLOC_BUFFER_ENTRY we need to call setHasLineDirectives in the same way as we do for SM_SLOC_FILE_ENTRY.
• When created suggested predefines based on the current command line options we need to add line markers in the same way that InitializePreprocessor does.
• Adjust a place in clangd where it was implicitly relying on command line defines being treated as builtin.

[john.brawn]

Fixed a couple of things causing failures in clangd tests (path being prepended in front of <command line>, and a bit of code assuming that command-line macros were builtin).

[john.brawn]

Ran clang-format.

[john.brawn]

Ping.

[mstorsjo]

Adding @aaron.ballman as fallback Clang reviewer here. While I did touch code in the vicinity of this area recently, I'm not familiar enough with the whole area to take on reviewing it at the moment.

[aaron.ballman]

LGTM, though you should also add a release note for the fix.

clang/lib/Serialization/ASTReader.cpp

1593

No need to repeat the type.

[fdeazeve]

According to git-bisect, this patch is causing the LLDB bots to crash.
https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/52690/console

(They were failing for other reasons before, which is why it took a while for this to be identified)

Clang is crashing with: Assertion failed: (0 && "Invalid SLocOffset or bad function choice"

In particular, this is true for the test lldb-api :: commands/expression/import_builtin_fileid/TestImportBuiltinFileID.py
I'm currently investigating the other failures reported by the bot, but we've also seen this assert fire internally in some other flows.

[john.brawn]

In D144651#4210787, @fdeazeve wrote:

According to git-bisect, this patch is causing the LLDB bots to crash.
https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/52690/console

(They were failing for other reasons before, which is why it took a while for this to be identified)

Clang is crashing with: Assertion failed: (0 && "Invalid SLocOffset or bad function choice"

In particular, this is true for the test lldb-api :: commands/expression/import_builtin_fileid/TestImportBuiltinFileID.py
I'm currently investigating the other failures reported by the bot, but we've also seen this assert fire internally in some other flows.

It looks like green.lab.llvm.org is down, so can't look at that log, but doing a local build and test of lldb (on a macbook, as this test is only enabled on macos) I don't see this failure, or any other tests failing with this assertion.

[fdeazeve]

It looks like green.lab.llvm.org is down, so can't look at that log, but doing a local build and test of lldb (on a macbook, as this test is only enabled on macos) I don't see this failure, or any other tests failing with this assertion.

It should be back online now: https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/52729/console

Unresolved Tests (20):
  lldb-api :: commands/expression/import-std-module/array/TestArrayFromStdModule.py
  lldb-api :: commands/expression/import-std-module/deque-basic/TestDequeFromStdModule.py
  lldb-api :: commands/expression/import-std-module/deque-dbg-info-content/TestDbgInfoContentDequeFromStdModule.py
  lldb-api :: commands/expression/import-std-module/forward_list-dbg-info-content/TestDbgInfoContentForwardListFromStdModule.py
  lldb-api :: commands/expression/import-std-module/forward_list/TestForwardListFromStdModule.py
  lldb-api :: commands/expression/import-std-module/iterator/TestIteratorFromStdModule.py
  lldb-api :: commands/expression/import-std-module/list-dbg-info-content/TestDbgInfoContentListFromStdModule.py
  lldb-api :: commands/expression/import-std-module/list/TestListFromStdModule.py
  lldb-api :: commands/expression/import-std-module/non-module-type-separation/TestNonModuleTypeSeparation.py
  lldb-api :: commands/expression/import-std-module/queue/TestQueueFromStdModule.py
  lldb-api :: commands/expression/import-std-module/retry-with-std-module/TestRetryWithStdModule.py
  lldb-api :: commands/expression/import-std-module/shared_ptr-dbg-info-content/TestSharedPtrDbgInfoContentFromStdModule.py
  lldb-api :: commands/expression/import-std-module/shared_ptr/TestSharedPtrFromStdModule.py
  lldb-api :: commands/expression/import-std-module/unique_ptr-dbg-info-content/TestUniquePtrDbgInfoContent.py
  lldb-api :: commands/expression/import-std-module/unique_ptr/TestUniquePtrFromStdModule.py
  lldb-api :: commands/expression/import-std-module/vector-dbg-info-content/TestDbgInfoContentVectorFromStdModule.py
  lldb-api :: commands/expression/import-std-module/vector-of-vectors/TestVectorOfVectorsFromStdModule.py
  lldb-api :: commands/expression/import-std-module/weak_ptr-dbg-info-content/TestDbgInfoContentWeakPtrFromStdModule.py
  lldb-api :: commands/expression/import-std-module/weak_ptr/TestWeakPtrFromStdModule.py
  lldb-api :: commands/expression/import_builtin_fileid/TestImportBuiltinFileID.py

Locally, I can make the last test on this list crash reliably. I've also confirmed that, on the bot, reverting this patch fixes the other tests as well.
Could you share your CMake invocation? This is what I used:

build_type=Debug
build_dir=build_$build_type
cmake \
  -S llvm/ -B $build_dir/ \
  -G Ninja \
  -DLLVM_ENABLE_PROJECTS="clang;llvm;lldb" \
  -DLLVM_ENABLE_RUNTIMES="libcxx;libcxxabi" \
  -DLLVM_OPTIMIZED_TABLEGEN=TRUE \
  -DLLVM_ENABLE_ASSERTIONS:BOOL=TRUE \
  -DLLVM_TARGETS_TO_BUILD="X86;ARM;AArch64" \
  -DCMAKE_BUILD_TYPE=$build_type

cmake --build $build_dir
build_Debug/bin/llvm-lit -v lldb/test/API/commands/expression/import_builtin_fileid/TestImportBuiltinFileID.py

This also works for a simpler repro:

build=build_Debug
rm -rf artifacts
mkdir artifacts
$build/bin/clang -g -O0 -fmodules -gmodules lldb/test/API/commands/expression/import_builtin_fileid/main.m -o artifacts/a.out
$build/bin/lldb artifacts/a.out -o 'b 4' -o run -o 'expr int (*DBG_CGImageGetRenderingIntent)(void *) = ((int (*)(void *))CGImageGetRenderingIntent);'

If you have lldb in your machine, you can also run lldb itself on lldb by prefixing that last command with lldb --

[john.brawn]

Unfortunately I still can't reproduce this even using exactly the same cmake command as you gave. Looking at above cmake log some possible causes of difference are:

• I'm doing this on an M1 macbook with host triple arm64-apple-darwin21.6.0 but the host triple on that bot is x86_64-apple-darwin20.6.0
• I'm using python 3.9, bot is using python 3.10
• I'm doing a build from clean whereas the bot is doing an incremental build

Also if I try to run the simpler reproducer you give then I get the error

error: expression failed to parse:
error: Header search couldn't locate module Cocoa

I'm now on holiday, I won't be able to get back to this until 2023-04-03.

[aaron.ballman]

In D144651#4217750, @john.brawn wrote:

Unfortunately I still can't reproduce this even using exactly the same cmake command as you gave. Looking at above cmake log some possible causes of difference are:

• I'm doing this on an M1 macbook with host triple arm64-apple-darwin21.6.0 but the host triple on that bot is x86_64-apple-darwin20.6.0
• I'm using python 3.9, bot is using python 3.10
• I'm doing a build from clean whereas the bot is doing an incremental build

Also if I try to run the simpler reproducer you give then I get the error

error: expression failed to parse:
error: Header search couldn't locate module Cocoa

I'm now on holiday, I won't be able to get back to this until 2023-04-03.

The build lab's lldb builders are all green, and greendragon's standalone build is also green. Could this be a problem with incremental builds being in a bad state somehow?

[fdeazeve]

In D144651#4219340, @aaron.ballman wrote:

The build lab's lldb builders are all green, and greendragon's standalone build is also green. Could this be a problem with incremental builds being in a bad state somehow?

I don't think so, as I can reliably reproduce this on my machine. I believe the _contents_ of the SDK headers affect what is fed to Clang, and therefore we see the crash with some SDKs but not others.

[aaron.ballman]

In D144651#4219633, @fdeazeve wrote:

In D144651#4219340, @aaron.ballman wrote:

The build lab's lldb builders are all green, and greendragon's standalone build is also green. Could this be a problem with incremental builds being in a bad state somehow?

I don't think so, as I can reliably reproduce this on my machine. I believe the _contents_ of the SDK headers affect what is fed to Clang, and therefore we see the crash with some SDKs but not others.

Ouch. :-( I think we're going to need some help getting a reproducer that doesn't rely on the SDK headers. In the meantime, because this is causing disruption by introducing a crash, I think it's fine to revert the changes until @john.brawn returns from vacation, but I'd appreciate it if we had such a reproducer in hand fairly shortly (I don't want to stick John in a position where his patch was reverted and he has no reasonable way in which to address the issue so it can be re-landed).

[Michael137]

FWIW here's some info on how we get to the assert. Maybe someone with more understanding of the clang::SourceManager and how it interacts with .pcms will be able to spot something.

Here's a bit more of the stack trace:

frame #41: 0x000000013844c7e0 liblldb.17.0.0git.dylib`clang::Sema::LookupParsedName(this=0x0000000177def200, R=0x000000016fdea010, S=0x0000000164a6e300, SS=0x000000016fdeb1b8, AllowBuiltinCreation=true, EnteringContext=false) at SemaLookup.cpp:2741:10
frame #40: 0x000000013844afd0 liblldb.17.0.0git.dylib`clang::Sema::LookupName(this=0x0000000177def200, R=0x000000016fdea010, S=0x0000000164a6e300, AllowBuiltinCreation=true, ForceNoCPlusPlus=false) at SemaLookup.cpp:2267:9
frame #39: 0x00000001384468a0 liblldb.17.0.0git.dylib`clang::Sema::CppLookupName(this=0x0000000177def200, R=0x000000016fdea010, S=0x0000000164a6df70) at SemaLookup.cpp:1495:15
frame #38: 0x00000001384474e4 liblldb.17.0.0git.dylib`CppNamespaceLookup(S=0x0000000177def200, R=0x000000016fdea010, Context=0x0000000177de2c00, NS=0x0000000177deb830, UDirs=0x000000016fde9a78)::UnqualUsingDirectiveSet&) at SemaLookup.cpp:1207:16
frame #37: 0x000000013844b208 liblldb.17.0.0git.dylib`LookupDirect(S=0x0000000177def200, R=0x000000016fdea010, DC=0x0000000177deb830) at SemaLookup.cpp:1109:39
frame #36: 0x0000000139512644 liblldb.17.0.0git.dylib`clang::DeclContext::lookup(this=0x0000000177deb830, Name=(Ptr = 5968768496)) const at DeclBase.cpp:1743:17
frame #35: 0x0000000131c48b88 liblldb.17.0.0git.dylib`lldb_private::ClangASTSource::ClangASTSourceProxy::FindExternalVisibleDeclsByName(this=0x0000600000344060, DC=0x0000000177deb830, Name=(Ptr = 5968768496)) at ClangASTSource.h:216:25
frame #34: 0x0000000134f0ab40 liblldb.17.0.0git.dylib`lldb_private::ClangASTSource::FindExternalVisibleDeclsByName(this=0x0000600003b3e140, decl_ctx=0x0000000177deb830, clang_decl_name=(Ptr = 5968768496)) at ClangASTSource.cpp:180:3
frame #33: 0x0000000134f3b9d8 liblldb.17.0.0git.dylib`lldb_private::ClangExpressionDeclMap::FindExternalVisibleDecls(this=0x0000600003b3e140, context=0x000000016fde9008) at ClangExpressionDeclMap.cpp:727:5
frame #32: 0x0000000134f3c1d8 liblldb.17.0.0git.dylib`lldb_private::ClangExpressionDeclMap::FindExternalVisibleDecls(this=0x0000600003b3e140, context=0x000000016fde9008, module_sp=nullptr, namespace_decl=0x000000016fde8c90) at ClangExpressionDeclMap.cpp:1450:3
frame #31: 0x0000000134f3fb80 liblldb.17.0.0git.dylib`lldb_private::ClangExpressionDeclMap::LookupFunction(this=0x0000600003b3e140, context=0x000000016fde9008, module_sp=nullptr, name=(m_string = "CGImageGetRenderingIntent"), namespace_decl=0x000000016fde8c90) at ClangExpressionDeclMap.cpp:1333:48
frame #30: 0x0000000134f0e2c0 liblldb.17.0.0git.dylib`lldb_private::ClangASTSource::CopyDecl(this=0x0000600003b3e140, src_decl=0x000000016462a480) at ClangASTSource.cpp:1728:29
frame #29: 0x0000000134eedd58 liblldb.17.0.0git.dylib`lldb_private::ClangASTImporter::CopyDecl(this=0x00000001050683a8, dst_ast=0x0000000177de2c00, decl=0x000000016462a480) at ClangASTImporter.cpp:78:55
frame #28: 0x000000013924cbd4 liblldb.17.0.0git.dylib`clang::ASTImporter::Import(this=0x000000010b420000, FromD=0x000000016462a480) at ASTImporter.cpp:9036:27
frame #27: 0x0000000134ef2a84 liblldb.17.0.0git.dylib`lldb_private::ClangASTImporter::ASTImporterDelegate::ImportImpl(this=0x000000010b420000, From=0x000000016462a480) at ClangASTImporter.cpp:892:23
frame #26: 0x00000001392748dc liblldb.17.0.0git.dylib`clang::ASTImporter::ImportImpl(this=0x000000010b420000, FromD=0x000000016462a480) at ASTImporter.cpp:8633:19
frame #25: 0x0000000139274dc0 liblldb.17.0.0git.dylib`clang::declvisitor::Base<std::__1::add_pointer, clang::ASTNodeImporter, llvm::Expected<clang::Decl*>>::Visit(this=0x000000016fde7a40, D=0x000000016462a480) at DeclNodes.inc:433:1
frame #24: 0x000000013923e90c liblldb.17.0.0git.dylib`clang::ASTNodeImporter::VisitFunctionDecl(this=0x000000016fde7a40, D=0x000000016462a480) at ASTImporter.cpp:3603:44
frame #23: 0x000000013924026c liblldb.17.0.0git.dylib`std::__1::conditional<std::is_base_of_v<clang::Type, clang::ParmVarDecl>, llvm::Expected<clang::ParmVarDecl const*>, llvm::Expected<clang::ParmVarDecl*>>::type clang::ASTNodeImporter::import<clang::ParmVarDecl>(this=0x000000016fde7a40, From=0x00000001035d6e80) at AS
frame #22: 0x000000013924cbd4 liblldb.17.0.0git.dylib`clang::ASTImporter::Import(this=0x000000010b420000, FromD=0x00000001035d6e80) at ASTImporter.cpp:9036:27
frame #21: 0x0000000134ef2a84 liblldb.17.0.0git.dylib`lldb_private::ClangASTImporter::ASTImporterDelegate::ImportImpl(this=0x000000010b420000, From=0x00000001035d6e80) at ClangASTImporter.cpp:892:23
frame #20: 0x00000001392748dc liblldb.17.0.0git.dylib`clang::ASTImporter::ImportImpl(this=0x000000010b420000, FromD=0x00000001035d6e80) at ASTImporter.cpp:8633:19
frame #19: 0x0000000139274eb0 liblldb.17.0.0git.dylib`clang::declvisitor::Base<std::__1::add_pointer, clang::ASTNodeImporter, llvm::Expected<clang::Decl*>>::Visit(this=0x000000016fde6530, D=0x00000001035d6e80) at DeclNodes.inc:507:1
frame #18: 0x0000000139246aec liblldb.17.0.0git.dylib`clang::ASTNodeImporter::VisitParmVarDecl(this=0x000000016fde6530, D=0x00000001035d6e80) at ASTImporter.cpp:4394:26
frame #17: 0x0000000139235924 liblldb.17.0.0git.dylib`clang::SourceLocation clang::ASTNodeImporter::importChecked<clang::SourceLocation>(this=0x000000016fde6530, Err=0x000000016fde6480, From=0x000000016fde6458) at ASTImporter.cpp:696:30
frame #16: 0x0000000139221da0 liblldb.17.0.0git.dylib`llvm::Expected<clang::SourceLocation> clang::ASTNodeImporter::import<clang::SourceLocation>(this=0x000000016fde6530, From=0x000000016fde6458) at ASTImporter.cpp:218:23
frame #15: 0x00000001392764bc liblldb.17.0.0git.dylib`clang::ASTImporter::Import(this=0x000000010b420000, FromLoc=(ID = 4217149690)) at ASTImporter.cpp:9511:36
frame #14: 0x0000000139280544 liblldb.17.0.0git.dylib`clang::ASTImporter::Import(this=0x000000010b420000, FromID=(ID = -265130), IsBuiltin=false) at ASTImporter.cpp:9541:28
frame #13: 0x00000001392764bc liblldb.17.0.0git.dylib`clang::ASTImporter::Import(this=0x000000010b420000, FromLoc=(ID = 2068864938)) at ASTImporter.cpp:9511:36
frame #12: 0x0000000139280884 liblldb.17.0.0git.dylib`clang::ASTImporter::Import(this=0x000000010b420000, FromID=(ID = -276169), IsBuiltin=false) at ASTImporter.cpp:9564:35
frame #11: 0x0000000139276468 liblldb.17.0.0git.dylib`clang::ASTImporter::Import(this=0x000000010b420000, FromLoc=(ID = 2356792)) at ASTImporter.cpp:9501:27
frame #10: 0x000000013928034c liblldb.17.0.0git.dylib`clang::SourceManager::isWrittenInBuiltinFile(this=0x0000000105070470, Loc=(ID = 2356792)) const at SourceManager.h:1480:28
frame #9: 0x000000013b6ef804 liblldb.17.0.0git.dylib`clang::SourceManager::getPresumedLoc(this=0x0000000105070470, Loc=(ID = 2356792), UseLineDirectives=true) const at SourceManager.cpp:1528:41
frame #8: 0x000000013b6e8140 liblldb.17.0.0git.dylib`clang::SourceManager::getDecomposedExpansionLoc(this=0x0000000105070470, Loc=(ID = 2356792)) const at SourceManager.h:1259:18
frame #7: 0x000000013910f0d4 liblldb.17.0.0git.dylib`clang::SourceManager::getFileID(this=0x0000000105070470, SpellingLoc=(ID = 2356792)) const at SourceManager.h:1119:12
frame #6: 0x000000013910f200 liblldb.17.0.0git.dylib`clang::SourceManager::getFileID(this=0x0000000105070470, SLocOffset=2356792) const at SourceManager.h:1834:12
frame #5: 0x000000013b6ed2d0 liblldb.17.0.0git.dylib`clang::SourceManager::getFileIDSlow(this=0x0000000105070470, SLocOffset=2356792) const at SourceManager.cpp:781:10
frame #4: 0x000000013b6ed620 liblldb.17.0.0git.dylib`clang::SourceManager::getFileIDLoaded(this=0x0000000105070470, SLocOffset=2356792) const at SourceManager.cpp:870:5

So this happens when we try clang::ASTImport a SourceLocation. The particular decl we're importing is:

ParmVarDecl 0x164d43880 </Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/System/Library/Frameworks/CoreGraphics.framework/<built-in>:408:20, /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.0.sdk/System/Library/Frameworks/CoreGraphics.framework/Headers/CGImage.h:275:83> col:83 imported in CoreGraphics.CGImage image 'CGImageRef _Nullable':'struct CGImage *'

The <built-in> in question is __nullable, which is only present on Darwin platforms.

Without the patch the decl looks like this:

ParmVarDecl 0x16a827a80 <<built-in>:409:20, /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/System/Library/Frameworks/CoreGraphics.framework/Headers/CGImage.h:275:83> col:83 imported in CoreGraphics.CGImage image 'CGImageRef _Nullable':'struct CGImage *'

(note how the location for the <built-in> doesn't have the directory prefix and the line number differs, which we think is the root of the crash)

The crashing import gets started as such:

// frame #12
Expected<FileID> ASTImporter::Import(FileID FromID, bool IsBuiltin) {
    ....
    if (!IsBuiltin && !Cache->BufferOverridden) {                            
      // Include location of this file.                                      
      ExpectedSLoc ToIncludeLoc = Import(FromSLoc.getFile().getIncludeLoc()); <<< Crashes
      if (!ToIncludeLoc)                                                     
        return ToIncludeLoc.takeError();                             
     ....

In frame #11 the IsBuiltin gets derived as such:

bool IsBuiltin = FromSM.isWrittenInBuiltinFile(FromLoc);

...which uses the PresumedLoc filename to determine whether the source location is a built-in:

bool isWrittenInBuiltinFile(SourceLocation Loc) const { 
  PresumedLoc Presumed = getPresumedLoc(Loc);           
  if (Presumed.isInvalid())                             
    return false;                                       
  StringRef Filename(Presumed.getFilename());           
  return Filename.equals("<built-in>");                 
}

But after this patch, the filename is set to:

(lldb) p FromSM.getPresumedLoc(FromLoc, true)
(clang::PresumedLoc) $6 = {
  Filename = 0x0000000107f8d168 "/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/System/Library/Frameworks/CoreGraphics.framework/<built-in>"
  ID = (ID = 0)
  Line = 408
  Col = 20
  IncludeLoc = (ID = 2356792)
}

Since this patch started setting the hasLineDirectives, getPresumedLoc now goes down following codepath:

...
PresumedLoc SourceManager::getPresumedLoc(SourceLocation Loc,            
                                          bool UseLineDirectives) const {
    if (UseLineDirectives && FI.hasLineDirectives()) {                         
    ....
      if (const LineEntry *Entry =                                             
            LineTable->FindNearestLineEntry(LocInfo.first, LocInfo.second)) {  
        // If the LineEntry indicates a filename, use it.                      
        if (Entry->FilenameID != -1) {                                         
          Filename = LineTable->getFilename(Entry->FilenameID);                
          // The contents of files referenced by #line are not in the          
          // SourceManager                                                     
          FID = FileID::get(0);                                                
    }                                                                      

  ...

I can confirm that the filename stored in the LineTable does have that prefix.

The prefixes in the line-table get written in:

void ASTReader::ParseLineTable(ModuleFile &F, const RecordData &Record) { 
     ...
    for (unsigned I = 0; Record[Idx]; ++I) {                    
      // Extract the file name                                  
      FileIDs[I] = LineTable.getLineTableFilenameID(ReadPath(F, Record, Idx));  
    }
    ...
}

Inside ReadPath we call into:

/// If we are loading a relocatable PCH or module file, and the filename      
/// is not an absolute path, add the system or module root to the beginning of
/// the file name.                                                            
void ASTReader::ResolveImportedPath(ModuleFile &M, std::string &Filename) {

If you dump the filenames that we create here you can see that even before the patch, we would add filenames into the line table of the form: /some/path/<built-in> and /some/path/<command-line>

But now that hasLineDirectives is being set, we end up reading these out. I wonder if we should just not add the root paths to <built-in> filenames.

I can confirm that something like the following:

@@ -2526,6 +2527,10 @@ void ASTReader::ResolveImportedPath(std::string &Filename, StringRef Prefix) {
   if (Filename.empty() || llvm::sys::path::is_absolute(Filename))                                   
     return;                                                                                         
                                                                                                     
+  if (Filename == "<built-in>"                                                                      
+      || Filename == "<command-line>")                                                              
+    return;

would resolve the crash. But I've not spent much time around this code so I don't know what the implications are here. But it does mimic the changes that were made to ASTWriter in this patch.

[fdeazeve]

I think given Michael's investigation, we'll go ahead and revert.
That said, some feedback on his suggestion for:

I wonder if we should just not add the root paths to <built-in> filenames.

Would allows us to make progress in restoring this patch

[john.brawn]

New version that checks for special filenames in ResolveImportedPath. I spent a while trying to come up with a test using just clang where not doing this fails, but couldn't as it looks like the failure is specific to objective-c module handing in lldb.

[fdeazeve]

I can check out a copy of this patch and help test it

[fdeazeve]

I did some local testing, and this no longer seems to crash for me. Hopefully the bots will be fine too

[john.brawn]

Ping.

[aaron.ballman]

LGTM

[john.brawn]

I accidentally pushed the old version of this patch in rG524ed4b1ba51, I've pushed a change to match what was reviewed here in rG78086af43ade.

clang-tools-extra/clangd/index/SymbolCollector.cpp

bool SymbolCollector::handleMacroOccurrence(const IdentifierInfo *Name,
                                             SourceLocation Loc) {
   assert(PP);
   // Builtin macros don't have useful locations and aren't needed in completion.
   if (MI->isBuiltinMacro())
     return true;
 
   const auto &SM = PP->getSourceManager();
   auto DefLoc = MI->getDefinitionLoc();
-  // Also avoid storing predefined macros like __DBL_MIN__.
+  // Also avoid storing macros that aren't defined in any file, i.e. predefined
+  // macros like __DBL_MIN__ and those defined on the command line.
   if (SM.isWrittenInBuiltinFile(DefLoc) ||
+      SM.isWrittenInCommandLineFile(DefLoc) ||
       Name->getName() == "__GCC_HAVE_DWARF2_CFI_ASM")
     return true;
 
   auto ID = getSymbolIDCached(Name->getName(), MI, SM);
   if (!ID)
     return true;
 
   auto SpellingLoc = SM.getSpellingLoc(Loc);

clang/docs/ReleaseNotes.rst

 - Clang now warns by default for C++20 and later about deprecated capture of
   ``this`` with a capture default of ``=``. This warning can be disabled with
   ``-Wno-deprecated-this-capture``.
 - Clang had failed to emit some ``-Wundefined-internal`` for members of a local
   class if that class was first introduced with a forward declaration.
 - Diagnostic notes and fix-its are now generated for ``ifunc``/``alias`` attributes
   which point to functions whose names are mangled.
 - Diagnostics relating to macros on the command line of a preprocessed assembly
-  file are now reported as coming from the file ``<command line>`` instead of
-  ``<built-in>``.
+  file or precompiled header are now reported as coming from the file
+  ``<command line>`` instead of ``<built-in>``.
 - Clang constexpr evaluator now provides a more concise diagnostic when calling
   function pointer that is known to be null.
 - Clang now avoids duplicate warnings on unreachable ``[[fallthrough]];`` statements
   previously issued from ``-Wunreachable-code`` and ``-Wunreachable-code-fallthrough``
   by prioritizing ``-Wunreachable-code-fallthrough``.
 
 Bug Fixes in This Version
 -------------------------

clang/lib/Serialization/ASTReader.cpp

static bool checkPreprocessorOptions(
     OptionValidation Validation = OptionValidateContradictions) {
   // Check macro definitions.
   MacroDefinitionsMap ASTFileMacros;
   collectMacroDefinitions(PPOpts, ASTFileMacros);
   MacroDefinitionsMap ExistingMacros;
   SmallVector<StringRef, 4> ExistingMacroNames;
   collectMacroDefinitions(ExistingPPOpts, ExistingMacros, &ExistingMacroNames);
 
+  // Use a line marker to enter the <command line> file, as the defines and
+  // undefines here will have come from the command line.
+  SuggestedPredefines += "# 1 \"<command line>\" 1\n";
+
   for (unsigned I = 0, N = ExistingMacroNames.size(); I != N; ++I) {
     // Dig out the macro definition in the existing preprocessor options.
     StringRef MacroName = ExistingMacroNames[I];
     std::pair<StringRef, bool> Existing = ExistingMacros[MacroName];
 
     // Check whether we know anything about this macro name or not.
     llvm::StringMap<std::pair<StringRef, bool /*IsUndef*/>>::iterator Known =
         ASTFileMacros.find(MacroName);
for (unsigned I = 0, N = ExistingMacroNames.size(); I != N; ++I) {
 
     // The macro bodies differ; complain.
     if (Diags) {
       Diags->Report(diag::err_pch_macro_def_conflict)
         << MacroName << Known->second.first << Existing.first;
     }
     return true;
   }
+
+  // Leave the <command line> file and return to <built-in>.
+  SuggestedPredefines += "# 1 \"<built-in>\" 2\n";
+
   if (Validation == OptionValidateStrictMatches) {
     // If strict matches are requested, don't tolerate any extra defines in
     // the AST file that are missing on the command line.
     for (const auto &MacroName : ASTFileMacros.keys()) {
       if (Diags) {
         Diags->Report(diag::err_pch_macro_def_undef) << MacroName << false;
       }
       return true;
case SM_SLOC_BUFFER_ENTRY: {
     SourceLocation IncludeLoc = ReadSourceLocation(*F, Record[1]);
     if (IncludeLoc.isInvalid() && F->isModule()) {
       IncludeLoc = getImportLocation(F);
     }
 
     auto Buffer = ReadBuffer(SLocEntryCursor, Name);
     if (!Buffer)
       return true;
-    SourceMgr.createFileID(std::move(Buffer), FileCharacter, ID,
+    FileID FID = SourceMgr.createFileID(std::move(Buffer), FileCharacter, ID,
                                         BaseOffset + Offset, IncludeLoc);
+    if (Record[3]) {
+      auto &FileInfo =
+          const_cast<SrcMgr::FileInfo &>(SourceMgr.getSLocEntry(FID).getFile());
+      FileInfo.setHasLineDirectives();
+    }
     break;
   }
 
   case SM_SLOC_EXPANSION_ENTRY: {
     LocSeq::State Seq;
     SourceLocation SpellingLoc = ReadSourceLocation(*F, Record[1], Seq);
     SourceLocation ExpansionBegin = ReadSourceLocation(*F, Record[2], Seq);
     SourceLocation ExpansionEnd = ReadSourceLocation(*F, Record[3], Seq);

clang/lib/Serialization/ASTWriter.cpp

 void ASTWriter::AddString(StringRef Str, RecordDataImpl &Record) {
   Record.push_back(Str.size());
   Record.insert(Record.end(), Str.begin(), Str.end());
 }
 
 bool ASTWriter::PreparePathForOutput(SmallVectorImpl<char> &Path) {
   assert(Context && "should have context when outputting path");
 
+  // Leave special file names as they are.
+  StringRef PathStr(Path.data(), Path.size());
+  if (PathStr == "<built-in>" || PathStr == "<command line>")
+    return false;
+
   bool Changed =
       cleanPathForOutput(Context->getSourceManager().getFileManager(), Path);
 
   // Remove a prefix to make the path relative, if relevant.
   const char *PathBegin = Path.data();
   const char *PathPtr =
       adjustFilenameForRelocatableAST(PathBegin, BaseDirectory);
   if (PathPtr != PathBegin) {

clang/test/PCH/macro-cmdline.c

+// RUN: %clang_cc1 %s -emit-pch -o %t1.pch -DMACRO1=1
+// RUN: %clang_cc1 -fsyntax-only %s -include-pch %t1.pch -DMACRO2=1 2>&1 | FileCheck %s
+
+#ifndef HEADER
+#define HEADER
+#else
+#define MACRO1 2
+// CHECK: macro-cmdline.c{{.*}}'MACRO1' macro redefined
+// CHECK: <command line>{{.*}}previous definition is here
+#define MACRO2 2
+// CHECK: macro-cmdline.c{{.*}}'MACRO2' macro redefined
+// CHECK: <command line>{{.*}}previous definition is here
+#endif

clang/test/PCH/ms-pch-macro.c

 
 #if defined(MSEXT) && !defined(BAR)
 #  error BAR was not defined
 #endif
 
 // CHECK-FOO: definition of macro 'FOO' differs between the precompiled header ('1') and the command line ('blah')
 // CHECK-NOFOO: macro 'FOO' was defined in the precompiled header but undef'd on the command line
 
-// expected-warning@1 {{definition of macro 'BAR' does not match definition in precompiled header}}
+// expected-warning@2 {{definition of macro 'BAR' does not match definition in precompiled header}}