This is an archive of the discontinued LLVM Phabricator instance.

Correctly handle line table entries without filenames during AST serialization
ClosedPublic

Authored by hans on Dec 1 2017, 1:00 PM.

Details

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.)

Diff Detail

Event Timeline

hans created this revision.Dec 1 2017, 1:00 PM
rsmith edited edge metadata.Dec 1 2017, 2:16 PM

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 added a comment.Dec 1 2017, 2:53 PM

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.

hans updated this revision to Diff 125227.Dec 1 2017, 2:55 PM
hans retitled this revision from Correctly handle line directives without filenames that come first in the file to Correctly handle line table entries without filenames during AST serialization.
hans edited the summary of this revision. (Show Details)
rsmith accepted this revision.Dec 4 2017, 1:32 PM

LGTM, thanks!

This revision is now accepted and ready to land.Dec 4 2017, 1:32 PM
This revision was automatically updated to reflect the committed changes.
This revision was automatically updated to reflect the committed changes.
yvvan added a subscriber: yvvan.Dec 11 2017, 2:35 AM

Can we still have it in 5.0?