This is an archive of the discontinued LLVM Phabricator instance.

[clang-scan-deps] Fix for input file given as relative path in compilation database "command" entry
ClosedPublic

Authored by saudi on Nov 10 2020, 1:25 PM.

Details

Summary

The bug appeared when clang-scan-deps was run from a different directory than the one provided in the "directory" entry.

Bug: https://bugs.llvm.org/show_bug.cgi?id=47252

Diff Detail

Event Timeline

saudi created this revision.Nov 10 2020, 1:25 PM
saudi requested review of this revision.Nov 10 2020, 1:25 PM

Can we use getPhysicalFileSystem() instead of getRealFileSystem() here?

saudi updated this revision to Diff 304327.Nov 10 2020, 2:08 PM

Updated the patch.

Followed suggestion from @dexonsmith. Indeed it simplifies the code.
Also, improved the test, to also test with -j 2

saudi retitled this revision from [[clang-scan-deps] Fix for input file given as relative path in compilation database "command" entry to [clang-scan-deps] Fix for input file given as relative path in compilation database "command" entry.Nov 10 2020, 2:09 PM

Updated the patch.

Followed suggestion from @dexonsmith. Indeed it simplifies the code.
Also, improved the test, to also test with -j 2

A couple of more comments. @arphaman , can you confirm this is a good direction?

clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
216–218

I think this can just be:

RealFS = llvm::vfs::createPhysicalFileSystem();

The ProxyFileSystem wrapper is a convenience for building other file systems; it doesn't do anything on its own IIRC.

clang/test/ClangScanDeps/relative_directory.cpp
13–14
20–26

I.e., maybe this can be:

// CHECK-DAG: relative_directory_input1.o:
// CHECK-DAG:   relative_directory_input1.cpp
// CHECK-DAG:   header.h

// CHECK-DAG: relative_directory_input2.o:
// CHECK-DAG:   relative_directory_input2.cpp
// CHECK-DAG:   header.h
saudi updated this revision to Diff 304629.Nov 11 2020, 1:16 PM

Removed use of ProxyFileSystem

saudi marked 2 inline comments as not done.Nov 11 2020, 1:24 PM
saudi added inline comments.
clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
216–218

Note that createPhysicalFileSystem() returns a std::unique_ptr whereas getRealFileSystem() return type is llvm::IntrusiveRfCntPtr.

Trying to change the type of RealFS requires more changes, as DependencyScanningWorkerFilesystem is itself a ProxyFileSystem and thus requires a llvm::IntrusiveRfCntPtr object.

Should DependencyScanningWorkerFilesystem not be a ProxyFileSystem? @arphaman WDYT?
However, in this case it might fall out of the scope of this patch, I'd suggest we keep it as a separate patch.

I updated my patch for an intermediate fix, which also passes the tests:

RealFS = llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem>(llvm::vfs::createPhysicalFileSystem().release());
clang/test/ClangScanDeps/relative_directory.cpp
13–14

If I understand correctly from the doc, it would lose the order (CHECK1, CHECK1-NEXT).
We need 2 blocks of 3 lines to be present, but only the blocks can be in any order.

I did this mimicking what is done in other tests like regular_cdb.cpp.

dexonsmith added inline comments.Nov 11 2020, 1:57 PM
clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
216–218

Ah, didn't realize it returned unique_ptr. RealFS should be IntrusiveRefCntPtr, but you can be less verbose like this:

RealFS = llvm::vfs::createPhysicalFileSystem().release();
dexonsmith added inline comments.Nov 11 2020, 1:59 PM
clang/test/ClangScanDeps/relative_directory.cpp
13–14

If I understand correctly from the doc, it would lose the order (CHECK1, CHECK1-NEXT).

We need 2 blocks of 3 lines to be present, but only the blocks can be in any order.

That's correct, but I think it's safe to assume in this test that the general format of the output is correct. Best would be to add a CHECK-DAG-NEXT: feature to FileCheck but that seems above and beyond for fixing this bug.

I did this mimicking what is done in other tests like regular_cdb.cpp.

Sure, it's probably fine either way, and it might be better to match the other tests.

saudi updated this revision to Diff 304648.Nov 11 2020, 2:03 PM

Simplified the RealFS initialization, as suggested

dexonsmith accepted this revision.Nov 11 2020, 2:18 PM
dexonsmith added 1 blocking reviewer(s): arphaman.

This LGTM, but I'd like @arphaman to look as well in case he originally tried getPhysicalFileSystem and it caused a problem. I've switched him to a blocking reviewer.

arphaman accepted this revision.Nov 11 2020, 4:59 PM

This make sense to me, thank you

This revision is now accepted and ready to land.Nov 11 2020, 4:59 PM