[PATCH] D65545: Handle some fs::remove failures

[Volodymyr Sapsai via Phabricator via llvm-commits]

vsapsai added inline comments.


================
Comment at: clang/lib/Frontend/ASTUnit.cpp:2315-2316
   if (Out.has_error()) {
     Out.clear_error();
-    return true;
+    return llvm::createStringError(Out.error(), "AST serialization failed");
   }
----------------
Will `Out.error()` still work after `Out.clear_error()`?


================
Comment at: clang/lib/Frontend/CompilerInstance.cpp:1444-1445
       // Remove the file.
-      llvm::sys::fs::remove(File->path());
+      if ((EC = llvm::sys::fs::remove(File->path())))
+        break;
 
----------------
jfb wrote:
> vsapsai wrote:
> > 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.
> 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.
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.


================
Comment at: llvm/include/llvm/Support/Error.h:1173
 
+static inline Error createStringError(std::error_code EC, const Twine &S) {
+  return createStringError(EC, S.str().c_str());
----------------
I might be missing something but should this function be `static` in the header?


================
Comment at: llvm/lib/Support/LockFileManager.cpp:58
+    if (std::error_code EC = sys::fs::remove(LockFileName))
+      report_fatal_error("Unable to remove invalid lock file \"" + LockFileName + "\": " + EC.message());
     return None;
----------------
jfb wrote:
> vsapsai wrote:
> > 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.
> 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...
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65545/new/

https://reviews.llvm.org/D65545