When clang crashes it emits many temp files, this changes clang's behavior to aggregate all temp files into a single tar file.
Details
Diff Detail
Event Timeline
For users on Windows, would this cause extra trouble if they wanted to see what was included?
It's available on Windows 10 and later by default, but otherwise .zip is the native archive format on Windows.
We DO support older versions of Windows (Windows Server 2003 is still quite popular!), so presumably this requirement wouldn't be particularly acceptable.
My feeling is: we have a TAR file writer, we don't have a ZIP file writer. While it'd be nice for us to produce zip files as those are more widely supported on all the platforms Clang runs on, it's a pretty heavy lift for this feature, especially given that newer versions of Windows come with a sufficiently useful tar program to handle them. So I'm fine with us producing TAR files -- if we decide to add support for ZIP in the future, we can allow the user to pick via a command line flag (or we could pick based on the host platform) and we can set the default behavior of Clang with a CMake flag.
Ah! I took the tests using 'tar' directly to mean we were offloading this work to 'tar', not that we had our own implementation of 'tar'. In that case, I am ok with that as well; the burden is only on compiler-engineers to have a copy of 'tar' on their windows machine which is a requirement I'm OK with making.
Yup, I'm okay with that compiler engineer requirement; I think our existing requirements on Windows already pull in a useful tar program even though it doesn't seem like we have any tests relying on it being installed yet.
There are actually a couple tests in lld that use tar https://github.com/llvm/llvm-project/blob/main/lld/test/ELF/reproduce-backslash.s#L8
Looks like there are some test failures on Windows my guess is their tar doesn't support the --wildcards flag. I'd rather not make these UNSUPORTED: windows, but am unsure how to move forward otherwise.
As a developer who often deals with crashes locally this is more annoying; currently I can just point tools at the shell script and C file in /tmp and let them go to work reducing, but now I have to also extract the files
+1.
I can see this being helpful for end-users who now just have to provide developers with a single file, but it does make things more awkward for me as a developer. How about an environment variable and/or a CMake option to skip archiving that can be used locally.
I think the idea of a cmake option makes a lot of sense. It may even be worth disabling the feature by default when configuring a debug build under the assumption those are primarily for compiler developers.
- Add new build option CLANG_CRASH_SAVE_TEMPS which defaults to Off, or On if CMAKE_BUILD_TYPE is Debug. It retains the old behavior alongside the tar file.
- Mark tests which use tar(1) as UNSUPPORTED: windows, it seems either the Windows bots don't have tar or it doesn't support --wildcards
clang/CMakeLists.txt | ||
---|---|---|
520 | Could you add this to the config.h header instead? | |
clang/lib/Driver/Driver.cpp | ||
1429 | This could be simplified with #cmakedefine01 in the config header | |
clang/test/Driver/crash-report-clang-cl.cpp | ||
3 | I think it would be cleaner to add a new feature if tar is available in $PATH instead of ignoring all windows bots. Some might have tar installed. | |
7 | Alternatively, if we had an env var/command line option to keep the cpp/sh, we could use that to avoid the dependency on tar in this test. | |
clang/test/Driver/crash-report-save-temps.c | ||
15 | Is this line intentional? |
- Define CLANG_CRASH_SAVE_TEMPS in config.h and not with add_definitions
- Add CLANG_CRASH_SAVE_TEMPS env var to force emitting individual preprocessed sources
clang/lib/Driver/Driver.cpp | ||
---|---|---|
1429 | Thanks I like that a lot better. | |
clang/test/Driver/crash-report-clang-cl.cpp | ||
3 | It's not clear to me what the Windows builders don't like. tar(1) is used in some lld tests. My guess is that the tar shipped on Windows doesn't support --wildcards which is seemingly necessary here because the tar file will have a random name, and it's member files are prefixed with that name. I don't have easy access to a Windows machine to test that theory though. I've gone with your suggestion to have an env var. Do you think I should remove UNSUPPORTED: windows on all tests or just this one? | |
7 | Sure. | |
clang/test/Driver/crash-report-save-temps.c | ||
15 | Nope, thanks. |
Could you add this to the config.h header instead?