[clang] Change FileManager to use llvm::ErrorOr instead of null on failure

[harlanhaskins]

Summary

Currently, clang's FileManager uses NULL as an indicator that a particular file
did not exist, but would not propagate errors like permission issues. Instead,
teach FileManager to use llvm::ErrorOr internally and return rich errors for
failures.

This patch updates the existing API and updates the callers throughout clang.
I tried to replicate the existing semantics, obeying the callers that handled
NULL specially.

[davide]

Really on the lldb side, Jonas is the right person to review this patch.

[davide]

[and Raphael for the clang vendor bits]

[jkorous]

clang/include/clang/Basic/FileManager.h

143 ↗

(On Diff #212650)

Maybe we could replace this with some type that has hard-to-use-incorrectly interface instead of using containsValue().

217 ↗

(On Diff #212650)

Does that mean that it's now safe to assume the value is != NULL in the absence of errors?

[harlanhaskins]

clang/include/clang/Basic/FileManager.h

143 ↗

(On Diff #212650)

You're right, these should store llvm::ErrorOr<DirectoryEntry &> and use std::err::no_such_file_or_directory as a placeholder instead of nullptr.

217 ↗

(On Diff #212650)

That's the intent of these changes, yes, but it should also be documented. 👍

[JDevlieghere]

clang/include/clang/Basic/FileManager.h

217 ↗

(On Diff #212650)

In line with the previous comment, should we just pass a reference then?

[harlanhaskins]

clang/include/clang/Basic/FileManager.h

217 ↗

(On Diff #212650)

I'm fine with doing that, but it would introduce a significant amount more churn into this patch.

[harlanhaskins]

Store references instead of raw pointers in FileManger's cache

[jkorous]

clang-tools-extra/clang-tidy/ClangTidy.cpp

240 ↗

(On Diff #212701)

Previously we'd hit the assert in translateFile() called from getOrCreateFileID(). This seems like a better way to handle that.

clang/include/clang/Basic/FileManager.h

217 ↗

(On Diff #212650)

I feel that this patch is already huge. It's a good idea though - maybe a separate patch?

clang/lib/ARCMigrate/FileRemapper.cpp

156 ↗

(On Diff #212701)

It seems like we're now avoiding the assert in remap() we'd hit previously. Is this fine?

229 ↗

(On Diff #212701)

Shouldn't we initialize this guy to nullptr?

clang/lib/Basic/FileManager.cpp

73 ↗

(On Diff #212701)

I think you've made the code self-documenting. Could you please remove the comment?

clang/lib/Frontend/FrontendAction.cpp

715 ↗

(On Diff #212701)

Could this actually happen? I was expecting the behavior to be consistent with getFile().

clang/lib/Lex/HeaderSearch.cpp

1448 ↗

(On Diff #212701)

Seems like we're introducing assert here. Is that fine?

clang/unittests/Basic/FileManagerTest.cpp

260 ↗

(On Diff #212701)

Just an idea - should we compare error codes?

[teemperor]

The error handling in LLDB seems fine to me.

[thakis]

Nice! This makes https://bugs.llvm.org/show_bug.cgi?id=42524#c3 easier.

[harlanhaskins]

Updated in response to feedback

[harlanhaskins]

clang/lib/ARCMigrate/FileRemapper.cpp

156 ↗

(On Diff #212701)

If we want to trap if this temp file fails, then I'm happy removing the condition check. Do you think this should trap on failure or should it check the condition?

229 ↗

(On Diff #212701)

👍 good catch!

clang/lib/Frontend/FrontendAction.cpp

715 ↗

(On Diff #212701)

Oops, debugging code that needs to be removed. Thanks for catching it!

clang/lib/Lex/HeaderSearch.cpp

1448 ↗

(On Diff #212701)

I'll make this conditional, thanks!

clang/unittests/Basic/FileManagerTest.cpp

260 ↗

(On Diff #212701)

Went ahead and added some tests ensuring we get useful error codes for a couple of common issues.

[jkorous]

LGTM

Thanks for all the work here!

clang/lib/ARCMigrate/FileRemapper.cpp

156 ↗

(On Diff #212701)

To be clear - I didn't have any opinion, just noticed there's a functional change and pointed that out.
On the other hand I assume your solution is fine.

lldb/trunk/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp

if (should_create_file) {
 
     if (temp_fd != -1) {
       lldb_private::File file(temp_fd, true);
       const size_t expr_text_len = strlen(expr_text);
       size_t bytes_written = expr_text_len;
       if (file.Write(expr_text, bytes_written).Success()) {
         if (bytes_written == expr_text_len) {
           file.Close();
+          if (auto fileEntry =
+                  m_compiler->getFileManager().getFile(result_path)) {
             source_mgr.setMainFileID(source_mgr.createFileID(
-              m_compiler->getFileManager().getFile(result_path),
+                *fileEntry,
                 SourceLocation(), SrcMgr::C_User));
             created_main_file = true;
           }
         }
       }
     }
+  }
 
   if (!created_main_file) {
     std::unique_ptr<MemoryBuffer> memory_buffer =
         MemoryBuffer::getMemBufferCopy(expr_text, "<lldb-expr>");
     source_mgr.setMainFileID(source_mgr.createFileID(std::move(memory_buffer)));
   }
 
   diag_buf->BeginSourceFile(m_compiler->getLangOpts(),

lldb/trunk/source/Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.cpp

if (!is_system_module) {
       auto error = [&]() {
         error_stream.Printf("error: No module map file in %s\n",
                             module.search_path.AsCString());
         return false;
       };
 
       bool is_system = true;
       bool is_framework = false;
-      auto *dir =
+      auto dir =
           HS.getFileMgr().getDirectory(module.search_path.GetStringRef());
       if (!dir)
         return error();
-      auto *file = HS.lookupModuleMapFile(dir, is_framework);
+      auto *file = HS.lookupModuleMapFile(*dir, is_framework);
       if (!file)
         return error();
       if (!HS.loadModuleMapFile(file, is_system))
         return error();
     }
   }
   if (!HS.lookupModule(module.path.front().GetStringRef())) {
     error_stream.Printf("error: Header search couldn't locate module %s\n",