This is an archive of the discontinued LLVM Phabricator instance.

In openFileForRead don't cache erroneous entries if the error relates to them being directories. Add tests.
ClosedPublic

Authored by kousikk on Sep 29 2019, 7:53 AM.

Details

Summary

It seems that when the CachingFileSystem is first given a file to open that is actually a directory, it incorrectly
caches that path to be errenous and throws an error when subsequently a directory open call is made for the same
path.
This change makes it so that we do NOT cache a path if it turns out we asked for a file when its a directory.

Diff Detail

Event Timeline

kousikk created this revision.Sep 29 2019, 7:53 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 29 2019, 7:53 AM
dexonsmith requested changes to this revision.Sep 30 2019, 9:36 AM
dexonsmith added inline comments.
clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
21–22

Note that the file is opened here.

227–228

It seems wasteful to do an extra stat here when the file is already open in createFileEntry.

Can you instead change createFileEntry to return std::errc::is_a_directory as appropriate to avoid the extra filesystem access? (Is it possible that it's already doing that, and you just need to check for that?)

This revision now requires changes to proceed.Sep 30 2019, 9:36 AM
kousikk updated this revision to Diff 222474.Sep 30 2019, 12:17 PM

Avoid the extra stat() call

kousikk marked 3 inline comments as done.Sep 30 2019, 12:18 PM
kousikk added inline comments.
clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
227–228

Thanks for the suggestion, done.

Sorry for bouncing you around, but I just had a look at the other user of createFileEntry and I think the right thing to do is to somehow share the code between DependencyScanningWorkerFilesystem::status and this. I suggest splitting a function out of status (called getOrCreateFileSystemEntry?) that returns a CachedFileSystemEntry (or an error).

The fix I just asked you to make (to only call stat once) could be done on getOrCreateFileSystemEntry as a follow-up, using std::unique_ptr<llvm::vfs::File> and changing getFileEntry to take that instead of a filename.

kousikk updated this revision to Diff 222667.Oct 1 2019, 11:50 AM
kousikk marked an inline comment as done.

Address code-reorg comment

kousikk added a comment.EditedOct 1 2019, 11:58 AM

Sorry for bouncing you around, but I just had a look at the other user of createFileEntry and I think the right thing to do is to somehow share the code between DependencyScanningWorkerFilesystem::status and this. I suggest splitting a function out of status (called getOrCreateFileSystemEntry?) that returns a CachedFileSystemEntry (or an error).

The fix I just asked you to make (to only call stat once) could be done on getOrCreateFileSystemEntry as a follow-up, using std::unique_ptr<llvm::vfs::File> and changing getFileEntry to take that instead of a filename.

IIUC,
DependencyScanningWorkerFilesystem::status() and DependencyScanningWorkerFilesystem::openFileForRead() both inturn call into CachedFileSystemEntry::createFileEntry().

Given that, I made getOrCreateFileEntry() a private-member of DependencyScanningWorkerFilesystem and made both these functions use it first.

In a subsequent step, I passed in llvm::vfs::Status object to createFileEntry() instead of Filename. I hope this aligns with the code-organization you are looking for(happy to re-write if not).

I also ran it against a canonical set of tests I'm maintaining internally and I didn't notice a difference in either performance and didn't get new crashes of the tool, so I think there should NOT be any regressions because of this change.

kousikk updated this revision to Diff 222669.Oct 1 2019, 12:06 PM

Add missed const

arphaman added a comment.EditedOct 2 2019, 6:19 PM

I don't quite understand the specific issue you're hitting, as it sounds that the logic right now should handle it:

It seems that when the CachingFileSystem is first given a file to open that is actually a directory, it incorrectly
caches that path to be errenous and throws an error when subsequently a directory open >call is made for the same
path.

In this case, right now (without this patch) createFileEntry will return std::errc::is_a_directory. DependencyScanningWorkerFilesystem::openFileForRead will then invalidate the entry in the global cache:

if (CacheEntry.getError() == std::errc::is_a_directory) {
    // Reset the CacheEntry to avoid setting an error entry in the
    // cache for the given directory path.
    CacheEntry = CachedFileSystemEntry();

Which means that when the call to stat happens, it should be fine as the global cache doesn't have a valid entry, so it will be able to recognize a directory and won't return an error.

Does this not happen in your case?

I don't quite understand the specific issue you're hitting, as it sounds that the logic right now should handle it:

It seems that when the CachingFileSystem is first given a file to open that is actually a directory, it incorrectly
caches that path to be errenous and throws an error when subsequently a directory open >call is made for the same
path.

In this case, right now (without this patch) createFileEntry will return std::errc::is_a_directory. DependencyScanningWorkerFilesystem::openFileForRead will then invalidate the entry in the global cache:

createFileEntry does return std::errc::is_a_directory, but the cache entry is NOT invalidated from the global cache. It stays on in the global cache, as an error entry for that directory path. I think the current version is slightly better because the first time we see a path, we determine if its a file / directory and store the appropriate CachedFileSystemEntry in the global cache.

if (CacheEntry.getError() == std::errc::is_a_directory) {
    // Reset the CacheEntry to avoid setting an error entry in the
    // cache for the given directory path.
    CacheEntry = CachedFileSystemEntry();

Which means that when the call to stat happens, it should be fine as the global cache doesn't have a valid entry, so it will be able to recognize a directory and won't return an error.

Does this not happen in your case?

I think the difference is the global cache does have the entry (but its set to an Error object). The test-case I added should currently fail without the changes in this CL.

kousikk updated this revision to Diff 222953.Oct 2 2019, 7:15 PM

Update diff to include all commits

@dexonsmith when you get a chance, can you take another look at this? Thanks!

arphaman requested changes to this revision.Oct 8 2019, 12:12 PM

@kousikk Thanks, I understand your patch better now. It makes more sense for sure.

When we're opening the file we shouldn't stat before calling open, as there's a race condition introduced, where the value of the stat could change between the call between stat and open is performed. We've seen problems like this before, and it ends up in crashes and mismatch size errors as Clang is getting invalid size from the stat if the file is modified in that time. We should still call open + fstat like we used. So I would recommend not changing the createFileEntry function to take in a stat, and do the fstat after opening the file like it used to.

This revision now requires changes to proceed.Oct 8 2019, 12:12 PM

@arphaman I don't mind changing this if there are race conditions as you say, but isn't the assumption of the tool that the filesystem remains unchanged for a single run of the tool? If so, should we actually throw error conditions instead of crashing in those cases?

@arphaman I don't mind changing this if there are race conditions as you say, but isn't the assumption of the tool that the filesystem remains unchanged for a single run of the tool? If so, should we actually throw error conditions instead of crashing in those cases?

This is true for a normal build, but it is not true with modules. All file system access goes through this, even for modules. I'm currently fixing an issue where we cache the non-existence of the module cache directory, causing the subsequent module load to fail. There is also an issue due to minimization where if we return the underlying stat result, the file size after minimization is different.

kousikk updated this revision to Diff 224219.Oct 9 2019, 5:51 PM
  • Revert changes to createFileEntry since that may result in a race condition if stat is called before file open
Bigcheese added inline comments.Oct 9 2019, 6:23 PM
clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
83

Is this actually called anywhere?

kousikk updated this revision to Diff 224228.Oct 9 2019, 6:26 PM

Remove getError() function that's now unused

kousikk marked an inline comment as done.Oct 9 2019, 6:27 PM
Bigcheese accepted this revision.Oct 9 2019, 6:33 PM

lgtm, but wait for Alex or Duncan to also take a look.

dexonsmith accepted this revision.Oct 9 2019, 11:14 PM

LGTM too.

This revision is now accepted and ready to land.Oct 10 2019, 8:14 AM
This revision was automatically updated to reflect the committed changes.