Page MenuHomePhabricator

[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.

Diff Detail

Repository
rL LLVM

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 ↗(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 marked 2 inline comments as done.Jul 31 2019, 3:41 PM
harlanhaskins added inline comments.
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 added inline comments.Jul 31 2019, 4:19 PM
clang/include/clang/Basic/FileManager.h
217 ↗(On Diff #212650)

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 ↗(On Diff #212650)

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 ↗(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?

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 ↗(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.

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 ↗(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.

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