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?

include/clang/Basic/SourceManagerInternals.h

public:
 
   StringRef getFilename(unsigned ID) const {
     assert(ID < FilenamesByID.size() && "Invalid FilenameID");
     return FilenamesByID[ID]->getKey();
   }
 
   unsigned getNumFilenames() const { return FilenamesByID.size(); }
 
-  void AddLineNote(FileID FID, unsigned Offset,
-                   unsigned LineNo, int FilenameID,
-                   unsigned EntryExit, SrcMgr::CharacteristicKind FileKind);
-
+  void AddLineNote(FileID FID, unsigned Offset, unsigned LineNo, int FilenameID,
+                   unsigned EntryExit, SrcMgr::CharacteristicKind FileKind,
+                   const SourceManager &SM);
 
   /// \brief Find the line entry nearest to FID that is before it.
   ///
   /// If there is no line entry before \p Offset in \p FID, returns null.
   const LineEntry *FindNearestLineEntry(FileID FID, unsigned Offset);
 
   // Low-level access
   using iterator = std::map<FileID, std::vector<LineEntry>>::iterator;

lib/Basic/SourceManager.cpp

 /// Add a line note to the line table that indicates that there is a \#line or
 /// GNU line marker at the specified FID/Offset location which changes the
 /// presumed location to LineNo/FilenameID. If EntryExit is 0, then this doesn't
 /// change the presumed \#include stack.  If it is 1, this is a file entry, if
 /// it is 2 then this is a file exit. FileKind specifies whether this is a
 /// system header or extern C system header.
 void LineTableInfo::AddLineNote(FileID FID, unsigned Offset, unsigned LineNo,
                                 int FilenameID, unsigned EntryExit,
-                                SrcMgr::CharacteristicKind FileKind) {
+                                SrcMgr::CharacteristicKind FileKind,
+                                const SourceManager &SM) {
   std::vector<LineEntry> &Entries = LineEntries[FID];
 
   // An unspecified FilenameID means use the last filename if available, or the
   // main source file otherwise.
-  if (FilenameID == -1 && !Entries.empty())
+  if (FilenameID == -1) {
+    if (!Entries.empty())
       FilenameID = Entries.back().FilenameID;
+    else
+      FilenameID = getLineTableFilenameID(SM.getFileEntryForID(FID)->getName());
+  }
+  assert(FilenameID != -1);
 
   assert((Entries.empty() || Entries.back().FileOffset < Offset) &&
          "Adding line entries out of order!");
 
   unsigned IncludeOffset = 0;
   if (EntryExit == 0) {  // No #include stack change.
     IncludeOffset = Entries.empty() ? 0 : Entries.back().IncludeOffset;
   } else if (EntryExit == 1) {
void SourceManager::AddLineNote(SourceLocation Loc, unsigned LineNo,
 
   unsigned EntryExit = 0;
   if (IsFileEntry)
     EntryExit = 1;
   else if (IsFileExit)
     EntryExit = 2;
 
   LineTable->AddLineNote(LocInfo.first, LocInfo.second, LineNo, FilenameID,
-                         EntryExit, FileKind);
+                         EntryExit, FileKind, *this);
 }
 
 LineTableInfo &SourceManager::getLineTable() {
   if (!LineTable)
     LineTable = new LineTableInfo();
   return *LineTable;
 }
 

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

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