Page MenuHomePhabricator

[clang] Emit crash reproduction as a single tar file
AbandonedPublic

Authored by abrachet on Mar 23 2022, 11:16 AM.

Details

Summary

When clang crashes it emits many temp files, this changes clang's behavior to aggregate all temp files into a single tar file.

Diff Detail

Event Timeline

abrachet created this revision.Mar 23 2022, 11:16 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 23 2022, 11:16 AM
abrachet requested review of this revision.Mar 23 2022, 11:16 AM

For users on Windows, would this cause extra trouble if they wanted to see what was included?

For users on Windows, would this cause extra trouble if they wanted to see what was included?

Is tar(1) not available on Windows?

For users on Windows, would this cause extra trouble if they wanted to see what was included?

Is tar(1) not available on Windows?

It's available on Windows 10 and later by default, but otherwise .zip is the native archive format on Windows.

For users on Windows, would this cause extra trouble if they wanted to see what was included?

Is tar(1) not available on Windows?

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.

For users on Windows, would this cause extra trouble if they wanted to see what was included?

Is tar(1) not available on Windows?

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.

For users on Windows, would this cause extra trouble if they wanted to see what was included?

Is tar(1) not available on Windows?

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.

For users on Windows, would this cause extra trouble if they wanted to see what was included?

Is tar(1) not available on Windows?

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.

abrachet updated this revision to Diff 417754.Mar 23 2022, 2:45 PM

Fix test failures outside of Driver/

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.

jrtc27 added a subscriber: jrtc27.Mar 23 2022, 6:37 PM

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

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.

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.

hans added a subscriber: hans.Mar 25 2022, 6:24 AM
abrachet updated this revision to Diff 419014.Mar 29 2022, 5:07 PM
  • 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
arichardson added inline comments.Mar 30 2022, 1:15 AM
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?

abrachet updated this revision to Diff 419165.Mar 30 2022, 8:50 AM
abrachet marked 2 inline comments as done.
  • 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
abrachet marked 2 inline comments as done.Mar 30 2022, 8:54 AM
abrachet added inline comments.
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.

abrachet updated this revision to Diff 419170.Mar 30 2022, 8:55 AM
abrachet marked 2 inline comments as done.

reabase

abrachet updated this revision to Diff 420299.Apr 4 2022, 1:41 PM

Fixing failing tests in Modules/