clang: Diag running out of file handles while looking for files

[thakis]

Summary

clang would only print "file not found" when it's unable to find a
header file. If the reason for that is a file handle leak, that's not a
very useful error message. For errors that aren't in a small whitelist
("file not found", "file is directory"), print an error with the
strerror() output.

This changes behavior in corner cases: If clang was out of file handles
while looking in one -I dir but then suddenly wasn't when looking in the
next -I dir, and both directories contained a file with the desired
name, previously we'd silently return the file from the second
directory. For this reason, it's important to ignore "is a directory"
for this new diag: if a file foo/foo exists and -I -Ifoo are passed, an
include of "foo" should successfully open file "foo" in directory "foo/"
instead of complaining that "./foo" is a directory.

No test since we mostly hit this when there's a handle leak somewhere,
and currently there isn't one. I manually tested this with the repro
steps in comment 2 on the bug below.

Fixes PR42524.

[rnk]

lgtm

[karies]

Similar to the concern raised at https://github.com/llvm/llvm-project/commit/50fcf7285eeb001d751eadac5d001a0e216fd701 we have received user reports about this patch: with -Ino-access-permissions -Iall-good, clang will throw an error (EACCES) even though header search goes on and will find the header in all-good. That seems a misleading an unnecessary error, especially as the header *is* found later, yet compilation "fails" because of this diagnostic.

I would propose to collect these errors, and where originally (before this patch), cling would complain "file not found" we could diagnose "while searching for this header, the following errors have been seen" and reporting the uniq-ed set of collected diags - but *only* in the case where the file cannot be found. This would prevent spurious diags during iteration through the SearchDirs when HeaderSearch actually finds the file. And I am fully aware that it's pointless to propose something without providing a differential :-/

FYI here's what we do right now to handle the EACCES case:

patch
--- a/interpreter/llvm/src/tools/clang/lib/Lex/HeaderSearch.cpp
+++ b/interpreter/llvm/src/tools/clang/lib/Lex/HeaderSearch.cpp
@@ -367,7 +367,9 @@ Optional<FileEntryRef> HeaderSearch::getFileAndSuggestModule(
     std::error_code EC = llvm::errorToErrorCode(File.takeError());
     if (EC != llvm::errc::no_such_file_or_directory &&
         EC != llvm::errc::invalid_argument &&
-        EC != llvm::errc::is_a_directory && EC != llvm::errc::not_a_directory) {
+        EC != llvm::errc::is_a_directory &&
+        EC != llvm::errc::not_a_directory &&
+        EC != llvm::errc::permission_denied) {
       Diags.Report(IncludeLoc, diag::err_cannot_open_file)
           << FileName << EC.message();
     }

cfe/trunk/lib/Lex/HeaderSearch.cpp

 }
 
 const FileEntry *HeaderSearch::getFileAndSuggestModule(
     StringRef FileName, SourceLocation IncludeLoc, const DirectoryEntry *Dir,
     bool IsSystemHeaderDir, Module *RequestingModule,
     ModuleMap::KnownHeader *SuggestedModule) {
   // If we have a module map that might map this header, load it and
   // check whether we'll have a suggestion for a module.
-  auto File = getFileMgr().getFile(FileName, /*OpenFile=*/true);
-  if (!File)
+  llvm::ErrorOr<const FileEntry *> File =
+      getFileMgr().getFile(FileName, /*OpenFile=*/true);
+  if (!File) {
+    // For rare, surprising errors (e.g. "out of file handles"), diag the EC
+    // message.
+    std::error_code EC = File.getError();
+    if (EC != std::errc::no_such_file_or_directory &&
+        EC != std::errc::is_a_directory) {
+      Diags.Report(IncludeLoc, diag::err_cannot_open_file) << EC.message();
+    }
     return nullptr;
+  }
 
   // If there is a module that corresponds to this header, suggest it.
   if (!findUsableModuleForHeader(*File, Dir ? Dir : (*File)->getDir(),
                                  RequestingModule, SuggestedModule,
                                  IsSystemHeaderDir))
     return nullptr;
 
   return *File;