This is an archive of the discontinued LLVM Phabricator instance.

Fix for headers having the same name as a directory
ClosedPublic

Authored by kousikk on Sep 3 2019, 3:50 AM.

Details

Summary

Scan deps tool crashes when called on a C++ file, containing an include
that has the same name as a directory. For example:

test.cpp:
#include <dir>
int main() { return 0; }

In directory foo/ :
dir/ bar/dir(file)

Now if the compile command is:
clang++ -I foo/ -I foo/bar/ -c test.cpp

The tool crashes since it finds foo/dir and tries to read that as a file and fails.
In this change, I add a defence against that scenario in the Dependency Scanning
File system.

Event Timeline

kousikk created this revision.Sep 3 2019, 3:50 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 3 2019, 3:50 AM

Thanks for fixing this! Could you add a test case which verifies that the assertion no longer happens? Let me know if you need help coming up with a test.

clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
199

This change dropped the createFile call, and didn't fix the issue where the same could happen at the end of the function. Could you please perform this check and return inside of createFile instead? This would ensure that both uses are fixed.

Sure I'll add a test case!

clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
199

Ah sorry, I didn't mean to drop the createFile() call. Sure will fix inside createFile() instead.

kousikk updated this revision to Diff 219890.Sep 12 2019, 5:14 AM
  • Add validation inside the createFile() function
kousikk updated this revision to Diff 219891.Sep 12 2019, 5:15 AM
  • Add validation inside the createFile() function
Harbormaster completed remote builds in B38049: Diff 219891.
kousikk updated this revision to Diff 219917.Sep 12 2019, 7:41 AM
  • Add tests and remove the fix inside createFile since it fails in other cases
kousikk updated this revision to Diff 219918.Sep 12 2019, 7:43 AM
  • Add validation inside the createFile() function

Sorry about the delay on this - I was OOO (back now).

  1. I added tests.
  2. I couldn't add isDirectory() check to createFile() since that resulted in failures to normal scenarios where it was previously passing.

PTAL!

Sorry about the delay on this - I was OOO (back now).

  1. I added tests.
  2. I couldn't add isDirectory() check to createFile() since that resulted in failures to normal scenarios where it was previously passing.

PTAL!

I was unable to reproduce the failures when I moved the check to createFile. What kind of failures did you see? Did you move it right to the start of the function? I would recommend rebasing the patch and retrying.

Also, could you please rename headerwithdirname.cpp to headerwithdirname_input.cpp when copying it to ensure FileCheck can match the filename without matching the temporary path (see https://reviews.llvm.org/D67379 for test fixes that @jkorous applied).

kousikk updated this revision to Diff 219975.Sep 12 2019, 12:39 PM
  • Add validation inside the createFile() function
kousikk updated this revision to Diff 219976.Sep 12 2019, 12:42 PM
  • Add validation inside the createFile() function

I was unable to reproduce the failures when I moved the check to createFile. What kind of failures did you see? Did you move it right to the start of the function? I would recommend rebasing the patch and retrying.

I saw an assertion error - but I realize now that it was because it was not at the top of the function. I had put it after Entry->getContents() which was throwing an assertion. I now realize that for getContents() to work, the entry needs to be a file. Thanks for correcting me!

Also, could you please rename headerwithdirname.cpp to headerwithdirname_input.cpp when copying it to ensure FileCheck can match the filename without matching the temporary path (see https://reviews.llvm.org/D67379 for test fixes that @jkorous applied).

Done

arphaman accepted this revision.Sep 13 2019, 12:22 PM

LGTM, Thanks!

This revision is now accepted and ready to land.Sep 13 2019, 12:22 PM

I don't have commit access - do you mind submitting this for me? Thanks!

Sure, I can do that today.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptSep 13 2019, 3:10 PM