Page MenuHomePhabricator

Introduce FileEntryRef and use it when handling includes to report correct dependencies when the FileManager is reused across invocations
ClosedPublic

Authored by arphaman on Aug 7 2019, 2:28 PM.

Details

Summary

This patch introduces a parallel API to FileManager's getFile: getFileEntryRef, which returns a reference to the FileEntry, and the name that was used to access the file.

The new API is adopted only in the HeaderSearch and Preprocessor for include file lookup, so that the accessed path can be propagated to SourceManager's FileInfo. SourceManager's FileInfo now can report this accessed path, using the new getName method. This API is then adopted in the dependency collector, which now correctly reports dependencies when a file is included both using a symlink and a real path in the case when the FileManager is reused across multiple Preprocessor invocations.

Note that this patch does not fix all dependency collector issues, as the same problem is still present in other cases when dependencies are obtained using FileSkipped, InclusionDirective, and HasInclude. This will be fixed in a follow-up patch.

Diff Detail

Event Timeline

arphaman created this revision.Aug 7 2019, 2:28 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 7 2019, 2:28 PM
aganea added a subscriber: aganea.Aug 7 2019, 2:47 PM
arphaman updated this revision to Diff 214010.Aug 7 2019, 2:53 PM

Fix a bug with dereferencing None File value in one call.

arphaman planned changes to this revision.Aug 7 2019, 3:04 PM

I forgot I left a FIXME that I intended to fix in there, I need to resolve that first. I'll update the patch.

arphaman updated this revision to Diff 214015.Aug 7 2019, 3:19 PM

Fix the FIXME.
The patch is now ready for review.

Bigcheese added inline comments.Aug 14 2019, 4:28 PM
clang/include/clang/Basic/FileManager.h
130

Isn't this incorrect in the case of symlinks?

259–261

LLVM_DEPRECATED()? (or w/e the name is of our depreciation attribute macro).

Bigcheese requested changes to this revision.Aug 15 2019, 3:55 PM
Bigcheese added inline comments.
clang/include/clang/Basic/SourceManager.h
1024

Invalid? I realize this is copied from above, but i'd fix the name.

clang/lib/Lex/PPDirectives.cpp
1879–1880

This lambda is huge and I only see it used once. Should this be a separate function?

This revision now requires changes to proceed.Aug 15 2019, 3:55 PM
bruno added inline comments.Aug 16 2019, 2:28 PM
clang/include/clang/Basic/FileManager.h
110

How does that work with the VFS? Will it keep the virtual path as the FileName to return with getName() or the external-content? Should it change when in face of use-external-names attribute?

259–261

Probably can't be done until we move all uses over? There's probably still enough of them that the warning would be very annoying?

clang/lib/Basic/FileManager.cpp
194

I know the surrounding functions are not consistent with capitalization, but maybe name and value here should?

arphaman updated this revision to Diff 215718.Aug 16 2019, 4:38 PM
arphaman marked 8 inline comments as done.

Address review comments and proper use-external-names support.

clang/include/clang/Basic/FileManager.h
110

Good catch. In the first patch getFileRef didn't honor use-external-names correctly (specifically once a file was opened once, for subsequent open of that file, it returned the VFS name, not the external name).

I fixed this issue by introducing a potential indirection to SeenFileEntries in the update patch. Now the getFileRef should always return the virtual path, unless use-external-names is specified, and then it will return the external path.

130

Right, I was planning to provide another way to get a directory with name. For now I'll just remove this method, so clients will have to call into FileEntry.

259–261

That's right, I can't mark the function as deprecated just yet, as you'll get a lot of warnings when building clang.

arphaman updated this revision to Diff 215719.Aug 16 2019, 4:41 PM

remove accidentally include diff's .rej file.

Bigcheese accepted this revision.Aug 21 2019, 2:01 PM
This revision is now accepted and ready to land.Aug 21 2019, 2:01 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptAug 22 2019, 11:15 AM

@arphaman you disabled this test on Windows, but did not specify exactly how it fails.
My team works on an embedded ARM compiler (most similar to arm-none-eabi), and we're now seeing failures from DependencyScannerTest. I can't find a buildbot failure for this test so I can't cross-reference to see if we have the same issue.

Does this failure look similar to what you saw on Windows, or could it be an option we're adding as part of the Compilation setup?

[ RUN      ] DependencyScanner.ScanDepsReuseFilemanager
.../clang/unittests/Tooling/DependencyScannerTest.cpp:100: Failure
      Expected: Deps[1]
      Which is: "symlink.h"
To be equal to: "/root/symlink.h"
.../clang/unittests/Tooling/DependencyScannerTest.cpp:101: Failure
      Expected: Deps[2]
      Which is: "header.h"
To be equal to: "/root/header.h"
.../clang/unittests/Tooling/DependencyScannerTest.cpp:113: Failure
      Expected: Deps[1]
      Which is: "symlink.h"
To be equal to: "/root/symlink.h"
.../clang/unittests/Tooling/DependencyScannerTest.cpp:114: Failure
      Expected: Deps[2]
      Which is: "header.h"
To be equal to: "/root/header.h"
[  FAILED  ] DependencyScanner.ScanDepsReuseFilemanager (5 ms)

@arphaman you disabled this test on Windows, but did not specify exactly how it fails.
My team works on an embedded ARM compiler (most similar to arm-none-eabi), and we're now seeing failures from DependencyScannerTest. I can't find a buildbot failure for this test so I can't cross-reference to see if we have the same issue.

Does this failure look similar to what you saw on Windows, or could it be an option we're adding as part of the Compilation setup?

[ RUN      ] DependencyScanner.ScanDepsReuseFilemanager
.../clang/unittests/Tooling/DependencyScannerTest.cpp:100: Failure
      Expected: Deps[1]
      Which is: "symlink.h"
To be equal to: "/root/symlink.h"
.../clang/unittests/Tooling/DependencyScannerTest.cpp:101: Failure
      Expected: Deps[2]
      Which is: "header.h"
To be equal to: "/root/header.h"
.../clang/unittests/Tooling/DependencyScannerTest.cpp:113: Failure
      Expected: Deps[1]
      Which is: "symlink.h"
To be equal to: "/root/symlink.h"
.../clang/unittests/Tooling/DependencyScannerTest.cpp:114: Failure
      Expected: Deps[2]
      Which is: "header.h"
To be equal to: "/root/header.h"
[  FAILED  ] DependencyScanner.ScanDepsReuseFilemanager (5 ms)

No the windows test failure was different, there were no Deps at all. I'm currently investigating it on a windows VM.

@JamesNagurne I think there's some issue with the working directory, which is not added in your case. Which platform are you running your build/test on? Which cmake options are you using?

JamesNagurne added a comment.EditedAug 23 2019, 4:01 PM

No the windows test failure was different, there were no Deps at all. I'm currently investigating it on a windows VM.

@JamesNagurne I think there's some issue with the working directory, which is not added in your case. Which platform are you running your build/test on? Which cmake options are you using?

I apologize for not giving such information in the first reply. Unfortunately this isn't an easy remote reproduction, as our ToolChain and some integral changes aren't upstreamed. This is an embedded ARM cross-compiled on Linux. Might be able to reproduce with arm-none-none-eabi.
LLVM is built as an external project. Looking at the build system, it looks like we have the CMAKE_ARGS:

-DLLVM_DEFAULT_TARGET_TRIPLE=arm-ti-none-eabi
-DLLVM_EXTERNAL_CLANG_SOURCE_DIR=${CMAKE_SOURCE_DIR}/llvm-project/clang
-DLLVM_TARGETS_TO_BUILD=ARM
-DCMAKE_BUILD_TYPE=Release
-DCMAKE_CXX_COMPILER=clang++
-DCMAKE_C_COMPILER=clang
-DLLVM_USE_LINKER=gold
-GNinja

Nothing suspicious, except maybe the default triple and ARM target.
Looking at our (not upstream) toolchain file, we do have some RTLibs, LibInternal, libcxx, and System include changes, along with a -nostdsysteminc to avoid pulling host includes into our cross compiler. However, none of this should affect general "#include" behavior, correct?
Another glance at your changes don't seem to affect any target-specific handling either, at least directly.

I might have to just bite the bullet and drop into the test with a debugger.

No the windows test failure was different, there were no Deps at all. I'm currently investigating it on a windows VM.

@JamesNagurne I think there's some issue with the working directory, which is not added in your case. Which platform are you running your build/test on? Which cmake options are you using?

I apologize for not giving such information in the first reply. Unfortunately this isn't an easy remote reproduction, as our ToolChain and some integral changes aren't upstreamed. This is an embedded ARM cross-compiled on Linux. Might be able to reproduce with arm-none-none-eabi.
LLVM is built as an external project. Looking at the build system, it looks like we have the CMAKE_ARGS:

-DLLVM_DEFAULT_TARGET_TRIPLE=arm-ti-none-eabi
-DLLVM_EXTERNAL_CLANG_SOURCE_DIR=${CMAKE_SOURCE_DIR}/llvm-project/clang
-DLLVM_TARGETS_TO_BUILD=ARM
-DCMAKE_BUILD_TYPE=Release
-DCMAKE_CXX_COMPILER=clang++
-DCMAKE_C_COMPILER=clang
-DLLVM_USE_LINKER=gold
-GNinja

Nothing suspicious, except maybe the default triple and ARM target.
Looking at our (not upstream) toolchain file, we do have some RTLibs, LibInternal, libcxx, and System include changes, along with a -nostdsysteminc to avoid pulling host includes into our cross compiler. However, none of this should affect general "#include" behavior, correct?
Another glance at your changes don't seem to affect any target-specific handling either, at least directly.

Yes, I don't see anything in your changes that is an obvious thing to blame.

I might have to just bite the bullet and drop into the test with a debugger.

I fixed the Windows issue, and it was completely unrelated to your issue. It looks like the FileManager isn't constructing absolute paths when accessing the files, which is somewhat unexpected. It would be useful to debug invocations of getFileEntryRef, to see if the InterndFilename in the function is getting changed into an absolute path or not.

No the windows test failure was different, there were no Deps at all. I'm currently investigating it on a windows VM.

@JamesNagurne I think there's some issue with the working directory, which is not added in your case. Which platform are you running your build/test on? Which cmake options are you using?

I apologize for not giving such information in the first reply. Unfortunately this isn't an easy remote reproduction, as our ToolChain and some integral changes aren't upstreamed. This is an embedded ARM cross-compiled on Linux. Might be able to reproduce with arm-none-none-eabi.
LLVM is built as an external project. Looking at the build system, it looks like we have the CMAKE_ARGS:

-DLLVM_DEFAULT_TARGET_TRIPLE=arm-ti-none-eabi
-DLLVM_EXTERNAL_CLANG_SOURCE_DIR=${CMAKE_SOURCE_DIR}/llvm-project/clang
-DLLVM_TARGETS_TO_BUILD=ARM
-DCMAKE_BUILD_TYPE=Release
-DCMAKE_CXX_COMPILER=clang++
-DCMAKE_C_COMPILER=clang
-DLLVM_USE_LINKER=gold
-GNinja

Nothing suspicious, except maybe the default triple and ARM target.
Looking at our (not upstream) toolchain file, we do have some RTLibs, LibInternal, libcxx, and System include changes, along with a -nostdsysteminc to avoid pulling host includes into our cross compiler. However, none of this should affect general "#include" behavior, correct?
Another glance at your changes don't seem to affect any target-specific handling either, at least directly.

Yes, I don't see anything in your changes that is an obvious thing to blame.

I might have to just bite the bullet and drop into the test with a debugger.

I fixed the Windows issue, and it was completely unrelated to your issue. It looks like the FileManager isn't constructing absolute paths when accessing the files, which is somewhat unexpected. It would be useful to debug invocations of getFileEntryRef, to see if the InterndFilename in the function is getting changed into an absolute path or not.

We found and fixed the issue on our end. Turns out we had a nefarious little piece of code in AddClangSystemIncludeArgs:

if (!DriverArgs.hasArg(options::OPT_nostdlibinc)) {
  SmallString<128> Dir(getDriver().SysRoot);
  llvm::sys::path::append(Dir, "include");
  addSystemInclude(DriverArgs, CC1Args, Dir.str());
}

Turns out that, as a cross compiler, we had set SysRoot to the empty string. So, we were adding "-internal-isystem" "include" to the cc1 arguments, which somehow caused the test to fail. I don't know precisely why it exposes issues in your test, but the code is clearly busted, so we've removed it.

I appreciate the responses as we've worked through the problems on our end!