Page MenuHomePhabricator

Handle some fs::remove failures
AcceptedPublic

Authored by jfb on Jul 31 2019, 3:29 PM.

Details

Summary

We have data showing that some modules builds fail in rare cases. We're therefore interested in handling sources of failure better, especially when it comes to modules. This patch takes us a small step closer to this by handling the return code of fs::remove in code that seems like it should. I haven't updated all ignored instances of fs::remove, I therefore can't mark it LLVM_NODISCARD for now.

This previous patch helps propagate errors: https://reviews.llvm.org/D63518

Event Timeline

jfb created this revision.Jul 31 2019, 3:29 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJul 31 2019, 3:29 PM
vsapsai added inline comments.Jul 31 2019, 5:25 PM
clang/lib/Frontend/CompilerInstance.cpp
648–650

Does the same logic as in ASTUnit.cpp apply here? I.e. if we failed to rename a file and emitted a message about it, should we also have a message about the failure to remove a file?

1447–1449

Why are you interrupting the loop when cannot remove a file? Don't know which option is the right one, just want to know your reasons.

clang/lib/Frontend/PrecompiledPreamble.cpp
111–118

Clangd uses PrecompiledPreamble but not TemporaryFiles as far as I can tell. report_fatal_error can be really disruptive for clangd, so it would be good to get an opinion from somebody working on it if this change would impact them.

clang/lib/Serialization/GlobalModuleIndex.cpp
935–938

Don't have a strong opinion but it looks odd that here in createStringError you are using string concatenation and a few lines lower %s.

llvm/include/llvm/Support/FileUtilities.h
55–61

For this change opinion of LLDB developers can be useful as it changes existing FileRemover behavior.

llvm/lib/Support/GraphWriter.cpp
102

Please add an extra space before \"

llvm/lib/Support/LockFileManager.cpp
58

Do you plan to keep using report_fatal_error in LockFileManager? It should help with discovering problems with modules but making it a fatal error forever seems a little bit scary.

214

Do you need to start the string with a space to separate a filename and "also"?

298–300

Here too, extra space before \"

jfb updated this revision to Diff 212899.Aug 1 2019, 1:51 PM
jfb marked 11 inline comments as done.
  • Return llvm::Error from ASTUnit::Save
  • Update per comments.
jkorous added inline comments.Aug 1 2019, 3:18 PM
clang/lib/Frontend/PrecompiledPreamble.cpp
111–118

Since client code in general might have different error handling/reporting strategy and can't do much about failure in destructor here, I'd consider just some kind of logging or assert here.

jfb added inline comments.Aug 1 2019, 4:07 PM
clang/lib/Frontend/CompilerInstance.cpp
648–650

I've updated ASTUnit to be a bit better, and removed the logging here.

1447–1449

The loops already bail when EC is set, and here I figure if we can't remove the base file we shouldn't try to remove its corresponding timestamp.

clang/lib/Serialization/GlobalModuleIndex.cpp
935–938

Yeah this part of createStringError bugs me... It only takes const char* for %s, and here I have a Twine. I changed it to %s.

llvm/lib/Support/LockFileManager.cpp
58

For some of the other destructors I was thinking that failing to remove a file could be non-fatal, but for lock files we can't really do much if they stay around. I think those are probably better as fatal than any other one. That, or return Expected, but combining with Optional<std::pair<std::string, int>> is... ew...

vsapsai added inline comments.Aug 2 2019, 5:00 PM
clang/lib/Frontend/ASTUnit.cpp
2318–2319

Will Out.error() still work after Out.clear_error()?

clang/lib/Frontend/CompilerInstance.cpp
1447–1449

I was thinking about using continue instead of break. After some thinking I believe continue is better because this way we'll try to remove all stale module files that we can. Otherwise one bad file can prevent the module cache pruning from working.

And I agree with your logic about not removing the timestamp when cannot remove the base file.

llvm/include/llvm/Support/Error.h
1173 ↗(On Diff #212899)

I might be missing something but should this function be static in the header?

llvm/lib/Support/LockFileManager.cpp
58

OK, I agree that a failure to remove a lock file is bad enough to justify report_fatal_error. We have some recovery code around locking (see unsafeRemoveLockFile) but we can try this approach until we have evidence it causes problems.

I haven't updated all ignored instances of fs::remove, I therefore can't mark it LLVM_NODISCARD for now.

I think I remember reading that casting a [[nodiscard]] functions return to void was broken on some compilers, ie still warns. You cast fs::remove to void a lot here which is a great way to show intent (I wish we mentioned this in the style guide) but if I'm not just making this up, it might not be worth the trouble to give fs::remove the nodiscard attribute. Maybe something to look into if you ever do this.

jfb updated this revision to Diff 213497.Aug 5 2019, 5:05 PM
jfb marked 8 inline comments as done.
  • Return llvm::Error from ASTUnit::Save
  • Update per comments.
  • Address Volodymyr's comments.
clang/lib/Frontend/CompilerInstance.cpp
1447–1449

continue will hit File != FileEnd && !EC, so it's equivalent to break. Your thinking makes sense, but I think we'd want to do it in a separate patch.

clang/lib/Frontend/PrecompiledPreamble.cpp
111–118

I changed it to dbgs().

bruno added a comment.Aug 5 2019, 8:12 PM

Thanks for working on this JF!

clang/lib/Frontend/PrecompiledPreamble.cpp
111–118

This probably means we won't see the message in a non-debug build, right?

jfb marked 2 inline comments as done.Aug 5 2019, 10:28 PM
jfb added inline comments.
clang/lib/Frontend/PrecompiledPreamble.cpp
111–118

Yeah, so it'll be diagnosable but won't affect release builds (which I understand to be the concern expressed w.r.t. clangd).

I have no other comments but for the fatal error in FileRemover I'd like to loop in Jonas as he was touching module cache in LLDB fairly recently.

clang/lib/Frontend/CompilerInstance.cpp
1447–1449

OK, I see now. Thanks for explanation.

Thanks for looping me in, @vsapsai!

llvm/include/llvm/Support/FileUtilities.h
55–61

Aborting isn't acceptable for LLDB. Can we turn this into an assert instead?

jfb updated this revision to Diff 213980.Aug 7 2019, 1:24 PM
jfb marked 3 inline comments as done.
  • Don't report_fatal_error in code that can be called from LLDB
jfb added a comment.Aug 7 2019, 1:24 PM

Updated.

jfb marked an inline comment as done.Aug 7 2019, 1:24 PM
vsapsai accepted this revision.Aug 9 2019, 3:39 PM

Looks good to me. If in some situations abort on error turns out to be too aggressive, we can change it later.

This revision is now accepted and ready to land.Aug 9 2019, 3:39 PM
bruno accepted this revision.Aug 16 2019, 2:32 PM

LGTM with one minor change.

clang/lib/Frontend/PrecompiledPreamble.cpp
38

pch-preamble is more accurate here.