[Support] Better error msg when cache dir can't be created. #69575
Conversation
tru
requested review from
mysterymath,
ZequanWu,
teresajohnson and
sylvain-audi
|
@llvm/pr-subscribers-lld @llvm/pr-subscribers-llvm-support Author: Tobias Hieta (tru) ChangesOn windows if you passed /lldltocache:D:\tmp to lld and you didn't have D: mounted it fail to create the cache dir D:\tmp, but the error message is pretty hard to understand: Which lead one of our users to report this as a crash. I have just added a bit better message so it now says: I am not sure this is a fatal error because it's not something that really should be reported as a bug to LLVM. But at least this gives a bit more visibility on what to change. Full diff: https://github.com/llvm/llvm-project/pull/69575.diff 1 Files Affected:
diff --git a/llvm/lib/Support/Caching.cpp b/llvm/lib/Support/Caching.cpp
index f20f08a865c76ff..722746e7b440390 100644
--- a/llvm/lib/Support/Caching.cpp
+++ b/llvm/lib/Support/Caching.cpp
@@ -145,7 +145,7 @@ Expected<FileCache> llvm::localCache(const Twine &CacheNameRef,
// ensures the filesystem isn't mutated until the cache is.
if (std::error_code EC = sys::fs::create_directories(
CacheDirectoryPath, /*IgnoreExisting=*/true))
- return errorCodeToError(EC);
+ return createStringError(EC, Twine("Can't create cache directory: ") + CacheDirectoryPath);
// Write to a temporary to avoid race condition
SmallString<64> TempFilenameModel;
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
There was a problem hiding this comment.
Change looks fine but can you add a test? Looks like unfortunately the other error conditions here are not tested anywhere, but let's just start by adding one for the error modified here. There are some existing caching tests in llvm/test/ThinLTO/X86/ (look for the cache-dir flag) that can be used as examples.
|
I can, but I wasn't sure we usually tested error strings. Guess I was under the impression that we had tests for the error condition. I'll look into it tomorrow. what do you think about the fact that it calls a fatal error here? Wouldn't a normal error be enough so that you don't get a scary trace back just for a missing directory? |
I don't think the caching code itself is reporting it as a fatal error, it is simply returning an error from localCache() and I guess the linker client is reporting as a fatal error? I'm not sure what the linker typically does for errors like this. |
llvm/lib/Support/Caching.cpp
Outdated
There was a problem hiding this comment.
Usually error messages are all lower case, I think?
There was a problem hiding this comment.
(& any test coverage for this?)
There was a problem hiding this comment.
That might be true for other parts, but this file has all error messages capitalized. I think I'll follow the current convention in the file for now and someone can happily do a NFC for fixing that later.
That's true, the fatal error actually comes from the LTO backend: https://github.com/llvm/llvm-project/blob/main/llvm/lib/LTO/LTOBackend.cpp#L407 |
|
I pushed a test, but it's not yet working 100% - I am struggling with this a bit. It seems since we are "dying" of report_fatal_error the normal Lit outputAnother thing that bothered me is that the method of setting the permissions of the directory to make it fail only works on Linux, this this would have to be disabled on Windows and I was not able to find any prior art for how to handle this. |
Ah ok there, not in the linker. In any case, given the layering here, I think it is harder to have the client distinguish between errors that should be fatal vs not. So let's just leave as is for now with your improved message. Open to suggestions as to how to improve this though.
I think you need "not --crash".
I'm not very familiar with Windows, but I think for these purposes a linux only test is fine. |
llvm/lib/Support/Caching.cpp
Outdated
There was a problem hiding this comment.
https://llvm.org/docs/CodingStandards.html#error-and-warning-messages The majority of places prefer no capitalization.
On windows if you passed /lldltocache:D:\tmp to lld and you didn't have D: mounted it fail to create the cache dir D:\tmp, but the error message is pretty hard to understand: ``` c:\code\llvm\llvm-project\out\debug>bin\lld-link.exe /lldltocache:D:\tmp hello.obj LLVM ERROR: no such file or directory PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace. Exception Code: 0xC000001D ``` Which lead one of our users to report this as a crash. I have just added a bit better message so it now says: ``` c:\code\llvm\llvm-project\out\debug>bin\lld-link.exe /lldltocache:D:\tmp hello.obj LLVM ERROR: Can't create cache directory: D:\tmp PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace. ``` I am not sure this is a fatal error because it's not something that really should be reported as a bug to LLVM. But at least this gives a bit more visibility on what to change.
|
I think this is ready now. |
tru
force-pushed
the
thieta/cache_error_msg
branch
from
7e0fe4e to
6c12034
Compare
|
@teresajohnson @MaskRay Ok to land this now? |
lld/test/COFF/lto-cache-errors.ll
Outdated
| ; RUN: chmod 444 %t.cache | ||
|
|
||
| ;; Check emit warnings when we can't create the cache dir | ||
| ; RUN: not --crash lld-link /lldltocache:%t.cache/cache /out:%t3 /entry:main %t2.o %t.o 2>&1 | FileCheck %s |
There was a problem hiding this comment.
make the cache dir name something weirder like nonexistant and check that it appears below like can't create cache directory: {{.*}}nonexistant
lld/test/COFF/lto-cache-errors.ll
Outdated
| ; RUN: chmod 444 %t.cache | ||
|
|
||
| ;; Check emit warnings when we can't create the cache dir | ||
| ; RUN: not --crash lld-link /lldltocache:%t.cache/cache /out:%t3 /entry:main %t2.o %t.o 2>&1 | FileCheck %s |
There was a problem hiding this comment.
also the --crash is worrying, I'm guessing there's some report_fatal_error somewhere where we should be passing false as the second param to not treat it as a crash but a user error, and we should have a followup change to fix that?
There was a problem hiding this comment.
Yeah we did some discussion on the report_fatal_error above, I was under the impression that it always "crashed", but if it's just a bool to that function that's probably a good fix. I can follow up on that in a new PR if everyone agrees that this is a good way to handle that. cc @teresajohnson @dwblaikie
| @@ -0,0 +1,20 @@ | |||
| ; REQUIRES: x86 | |||
There was a problem hiding this comment.
https://maskray.me/blog/2021-08-08-toolchain-testing#the-test-checks-at-the-wrong-layer
A better test is llvm-lto2 --cache-dir in llvm/test/ThinLTO/X86
There was a problem hiding this comment.
My first iteration was there - but it doesn't seem to follow the same code path, I never got the error message that I got from lld-link.
I will merge this now, since a test is better than no test - but I might follow up with a test in the llvm layer when I do the changes to report_fatal_error if people think that's a good change.
On windows if you passed /lldltocache:D:\tmp to lld and you didn't have D: mounted it fail to create the cache dir D:\tmp, but the error message is pretty hard to understand: ``` c:\code\llvm\llvm-project\out\debug>bin\lld-link.exe /lldltocache:D:\tmp hello.obj LLVM ERROR: no such file or directory PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace. Exception Code: 0xC000001D ``` Which lead one of our users to report this as a crash. I have just added a bit better message so it now says: ``` c:\code\llvm\llvm-project\out\debug>bin\lld-link.exe /lldltocache:D:\tmp hello.obj LLVM ERROR: Can't create cache directory: D:\tmp PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace. ``` I am not sure this is a fatal error because it's not something that really should be reported as a bug to LLVM. But at least this gives a bit more visibility on what to change.
On windows if you passed /lldltocache:D:\tmp to lld and you didn't have D: mounted it fail to create the cache dir D:\tmp, but the error message is pretty hard to understand:
Which lead one of our users to report this as a crash. I have just added a bit better message so it now says:
I am not sure this is a fatal error because it's not something that really should be reported as a bug to LLVM. But at least this gives a bit more visibility on what to change.