Correctly handle line table entries without filenames during AST serialization

[hans]

Summary

The current code would hit an assert in ASTWriter when trying to write out the filename for a line table entry that didn't have any.

(Chromium runs into this when building with the latest MS SDK.)

[rsmith]

The intent is to use a FilenameID of -1 to represent this situation; see the documentation of the LineEntry::FilenameID member. Users of that field are expected to deal with that value (see the handling of that case in SourceManager::getPresumedLoc for example). I think we need to handle it that way, rather than mapping to the main file name as this patch does, for correctness in the PCH case -- we don't actually *know* the correct main file name at the point where the line directive appears.

I imagine the cause of the crash is that the AST writer blindly calls LineTableInfo::getFilename(LE.FilenameID) without checking for this case?

[hans]

In D40746#942824, @rsmith wrote:

The intent is to use a FilenameID of -1 to represent this situation; see the documentation of the LineEntry::FilenameID member. Users of that field are expected to deal with that value (see the handling of that case in SourceManager::getPresumedLoc for example). I think we need to handle it that way, rather than mapping to the main file name as this patch does, for correctness in the PCH case -- we don't actually *know* the correct main file name at the point where the line directive appears.

I imagine the cause of the crash is that the AST writer blindly calls LineTableInfo::getFilename(LE.FilenameID) without checking for this case?

Yes, exactly.

I suppose the right thing to do is to just make the -1 values survive serialization, and the correct filename can be figured out when the deserialized pch gets used? Patch coming up.

[rsmith]

LGTM, thanks!

[yvvan]

Can we still have it in 5.0?

cfe/trunk/lib/Serialization/ASTReader.cpp

 /// \returns true if there was an error.
 bool ASTReader::ParseLineTable(ModuleFile &F,
                                const RecordData &Record) {
   unsigned Idx = 0;
   LineTableInfo &LineTable = SourceMgr.getLineTable();
 
   // Parse the file names
   std::map<int, int> FileIDs;
+  FileIDs[-1] = -1; // For unspecified filenames.
   for (unsigned I = 0; Record[Idx]; ++I) {
     // Extract the file name
     auto Filename = ReadPath(F, Record, Idx);
     FileIDs[I] = LineTable.getLineTableFilenameID(Filename);
   }
   ++Idx;
 
   // Parse the line entries

cfe/trunk/lib/Serialization/ASTWriter.cpp

void ASTWriter::WriteSourceManagerBlock(SourceManager &SourceMgr,
   // after the source location offsets.
   if (SourceMgr.hasLineTable()) {
     LineTableInfo &LineTable = SourceMgr.getLineTable();
 
     Record.clear();
 
     // Emit the needed file names.
     llvm::DenseMap<int, int> FilenameMap;
+    FilenameMap[-1] = -1; // For unspecified filenames.
     for (const auto &L : LineTable) {
       if (L.first.ID < 0)
         continue;
       for (auto &LE : L.second) {
         if (FilenameMap.insert(std::make_pair(LE.FilenameID,
-                                              FilenameMap.size())).second)
+                                              FilenameMap.size() - 1)).second)
           AddPath(LineTable.getFilename(LE.FilenameID), Record);
       }
     }
     Record.push_back(0);
 
     // Emit the line entries
     for (const auto &L : LineTable) {
       // Only emit entries for local files.

cfe/trunk/test/PCH/line-directive-nofilename.h

+#line 42
+int foo; // This should appear as at line-directive-nofilename.h:42
+
+#line 100 "foobar.h"
+int bar; // This should appear as at foobar.h:100

cfe/trunk/test/PCH/line-directive-nofilename.c

+// RUN: %clang_cc1 -emit-pch -o %t %S/line-directive-nofilename.h
+// RUN: not %clang_cc1 -include-pch %t -fsyntax-only %s 2>&1 | FileCheck %s
+
+// This causes an "error: redefinition" diagnostic. The notes will have the
+// locations of the declarations from the PCH file.
+double foo, bar;
+
+// CHECK: line-directive-nofilename.h:42:5: note: previous definition is here
+// CHECK: foobar.h:100:5: note: previous definition is here