This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by harlanhaskins on Jul 31 2019, 1:19 PM.

Details

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.

Event Timeline

harlanhaskins created this revision.Jul 31 2019, 1:19 PM
Herald added a reviewer: shafik. · View Herald Transcript
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript
harlanhaskins edited the summary of this revision. (Show Details)Jul 31 2019, 1:20 PM
harlanhaskins edited the summary of this revision. (Show Details)Jul 31 2019, 1:21 PM
davide added a subscriber: davide.

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

[and Raphael for the clang vendor bits]

jkorous added inline comments.Jul 31 2019, 2:27 PM
clang/include/clang/Basic/FileManager.h
143–144

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

217

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

harlanhaskins marked 2 inline comments as done.Jul 31 2019, 3:41 PM
harlanhaskins added inline comments.
clang/include/clang/Basic/FileManager.h
143–144

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

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

JDevlieghere added inline comments.Jul 31 2019, 4:19 PM
clang/include/clang/Basic/FileManager.h
217

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

harlanhaskins added inline comments.Jul 31 2019, 4:28 PM
clang/include/clang/Basic/FileManager.h
217

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

Store references instead of raw pointers in FileManger's cache

harlanhaskins marked 2 inline comments as done.Jul 31 2019, 4:32 PM
jkorous added inline comments.Jul 31 2019, 6:24 PM
clang-tools-extra/clang-tidy/ClangTidy.cpp
240

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

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

clang/lib/ARCMigrate/FileRemapper.cpp
156

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

229

Shouldn't we initialize this guy to nullptr?

clang/lib/Basic/FileManager.cpp
73

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

clang/lib/Frontend/FrontendAction.cpp
715

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

clang/lib/Lex/HeaderSearch.cpp
1448

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

clang/unittests/Basic/FileManagerTest.cpp
260

Just an idea - should we compare error codes?

martong resigned from this revision.Aug 1 2019, 2:36 AM

The error handling in LLDB seems fine to me.

harlanhaskins marked 8 inline comments as done.

Updated in response to feedback

harlanhaskins marked 6 inline comments as done and an inline comment as not done.Aug 1 2019, 11:22 AM
harlanhaskins added inline comments.
clang/lib/ARCMigrate/FileRemapper.cpp
156

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

👍 good catch!

clang/lib/Frontend/FrontendAction.cpp
715

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

clang/lib/Lex/HeaderSearch.cpp
1448

I'll make this conditional, thanks!

clang/unittests/Basic/FileManagerTest.cpp
260

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

harlanhaskins marked an inline comment as done.Aug 1 2019, 11:22 AM
jkorous accepted this revision.Aug 1 2019, 12:45 PM
jkorous marked an inline comment as done.

LGTM

Thanks for all the work here!

clang/lib/ARCMigrate/FileRemapper.cpp
156

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.

This revision is now accepted and ready to land.Aug 1 2019, 12:45 PM
harlanhaskins marked 2 inline comments as done.Aug 1 2019, 2:28 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptAug 1 2019, 2:34 PM