[clang] Implement setting crash_diagnostics_dir through env variable

[mizvekov]

Summary

This implements setting the equivalent of -fcrash-diagnostics-dir
through the environment variable CLANG_CRASH_DIAGNOSTICS_DIR.
If present, the flag still takes precedence.

This helps integration with test frameworks and pipelines.

With this feature, we change the libcxx bootstrapping build
pipeline to produce clang crash reproducers as artifacts.

Signed-off-by: Matheus Izvekov <[e-mail]>

[erichkeane]

I don't mind the idea, but wonder if some level of RFC/project-wide decision should be made here? WDYT @aaron.ballman ?

clang/lib/Driver/Driver.cpp

5415–5416

StringRef would be better here instead, which should mean you don't have to create the SmallString below, and could just work in Twines for everything.

[mizvekov]

clang/lib/Driver/Driver.cpp

5415–5416

It seems that llvm::sys::path::append takes twine as inputs, but it only outputs to a SmallVectorImpl.
Unless there is something else I could use?

[erichkeane]

clang/lib/Driver/Driver.cpp

5415–5416

Ah, yikes, I missed that was what was happening :/ THAT is still necessary. A Twine can be created from a stringref (and I'd still prefer one anyway), but you DO still have to make that SmallString unfortunately.

[mizvekov]

clang/lib/Driver/Driver.cpp

5415–5416

The Twine also can't take a nullptr (such as the case no flag passed and no env variable), so we would have to use something like std::optional<Twine>, and make the needed conversions here.

And the Twine to SmallString copy looks unergonomic as well.

So it seems we would need to make some little changes out of scope of this MR to get that to look nicely.

[erichkeane]

clang/lib/Driver/Driver.cpp

5415–5416

I guess i don't get the concern here? I'm suggesting just changing this from const char* to StringRef (which should work). Everything else below should 'just work' AFAIK.

[mizvekov]

clang/lib/Driver/Driver.cpp

5415–5416

Oh okay I misunderstood you! Thought you meant to make CrashDirectory a Twine.

Yeah with StringRef we only need to change the test:

if (CrashDirectory) {

--->

if (CrashDirectory.data() != nullptr) {

Everything else stays the same. It doesn't look cleaner, but would address your concerns about avoiding storing a char pointer variable?

[mizvekov]

clang/lib/Driver/Driver.cpp

5415–5416

We could add a bool conversion to StringRef, but perhaps folks could find the meaning confusing between empty and null pointer?

[erichkeane]

clang/lib/Driver/Driver.cpp

5415–5416

Hrm... do we actually have meaning here to 'empty' for these files? I guess we allow it already.... hrm... Ok, disregard the suggestion, and thanks for thinking this through with me.

[mizvekov]

clang/lib/Driver/Driver.cpp

5415–5416

Thanks for the review and suggestions!

Yeah right, I didn't want to change the behavior vs empty here, but I think if we did some change, it would make more sense to create some specific error for it, instead of treating empty the same as no argument passed.

[aaron.ballman]

In D133082#3763829, @erichkeane wrote:

I don't mind the idea, but wonder if some level of RFC/project-wide decision should be made here? WDYT @aaron.ballman ?

We use environment variables in several other places within the driver, but I am a bit skittish about adding new ones to correspond to feature flags as that seems to be somewhat novel. I think it'd be a good idea to get more buy-in with an RFC on Discourse for more broad awareness.

If we go this route, we definitely need user-facing documentation that explains what's going on. I don't think we have anything corresponding to https://gcc.gnu.org/onlinedocs/gcc/Environment-Variables.html, but having such a page for Clang would be a good idea (even if the page is incomplete).

clang/docs/ReleaseNotes.rst

136–144

clang/lib/Driver/Driver.cpp

5417

This should use llvm::sys::Process::GetEnv() so it properly handles text encodings. CC @tahonermann @cor3ntin for text encoding awareness.

[mizvekov]

In D133082#3764256, @aaron.ballman wrote:

We use environment variables in several other places within the driver, but I am a bit skittish about adding new ones to correspond to feature flags as that seems to be somewhat novel. I think it'd be a good idea to get more buy-in with an RFC on Discourse for more broad awareness.

If we go this route, we definitely need user-facing documentation that explains what's going on. I don't think we have anything corresponding to https://gcc.gnu.org/onlinedocs/gcc/Environment-Variables.html, but having such a page for Clang would be a good idea (even if the page is incomplete).

Okay, fair!

We will have an RFC about this.

clang/lib/Driver/Driver.cpp

5417

Okay, makes sense. But be aware that we have multiple places in the llvm project where we are using std::getenv to retrieve a filesystem path variable. There is another one within clang itself as well (CLANG_MODULE_CACHE_PATH, which is where I took inspiration from).

[aaron.ballman]

clang/lib/Driver/Driver.cpp

5417

Yeah, I noticed the same thing when I was looking around. Every one of those is a bug! It likely works fine for Linux where UTF-8 is common, but they will all fail on Windows where UTF-16 is used for the OS but ::getenv() uses the C locale. Any sort of non-ASCII characters in the file path are likely not going to do the right thing in that case.

[Mordante]

Thanks for working on this! It's really great to get the crash report as an artifact.

libcxx/test/libcxx/crash.sh.cpp

15 ↗

(On Diff #457159)

The libc++ build seems to be green. I assume it was intended to be red so we can validate the artifact is available in the CI.

libcxx/utils/ci/buildkite-pipeline.yml

377

Are relative directories allowed?

[mizvekov]

libcxx/test/libcxx/crash.sh.cpp

15 ↗

(On Diff #457159)

If I made it red, this test would fail in one the jobs from the early group, and we would never run the bootstraping job :(

But making it green does not stop it from exporting the artifact, you can go in there in the build results and have a look!

libcxx/utils/ci/buildkite-pipeline.yml

377

Yes, it already worked with the flag.

You can check that it's working, even though the bootstrapping pipeline is green, it still exports the artifacts.

[Mordante]

Ideally it would be documented these artifacts are now available. However there's no good place in libc++ to document that. I'm working on such a document so I will take care of documenting these artifacts.

LGTM from libc++'s PoV.

libcxx/utils/ci/buildkite-pipeline.yml

377

Thanks I see.

Just a hint you can move this job earlier in this file for testing purposes and/or remove jobs you don't want to test. I use this every now and then when I have issues with a specific CI.

[mizvekov]

In D133082#3764256, @aaron.ballman wrote:

If we go this route, we definitely need user-facing documentation that explains what's going on. I don't think we have anything corresponding to https://gcc.gnu.org/onlinedocs/gcc/Environment-Variables.html, but having such a page for Clang would be a good idea (even if the page is incomplete).

I took an attempt at this on the latest diff.
I am suggesting we put the variables close to command line flags, in the same document, as I think it makes sense to have these options that interact with each other close together.

[mizvekov]

@aaron.ballman I think all of your concerns are addressed now.

[aaron.ballman]

LGTM, thank you!

clang/docs/ReleaseNotes.rst

- Correctly diagnose a future keyword if it exist as a keyword in the higher
   language version and specifies in which version it will be a keyword. This
   supports both c and c++ language.
 - When diagnosing multi-level pack expansions of mismatched lengths, Clang will
   now, in most cases, be able to point to the relevant outer parameter.
 - no_sanitize("...") on a global variable for known but not relevant sanitizers
   is now just a warning. It now says that this will be ignored instead of
   incorrectly saying no_sanitize only applies to functions and methods.
 
 Non-comprehensive list of changes in this release
 -------------------------------------------------
+- It's now possible to set the crash diagnostics directory through
+  the environment variable ``CLANG_CRASH_DIAGNOSTICS_DIR``.
+  The ``-fcrash-diagnostics-dir`` flag takes precedence.
 
 New Compiler Flags
 ------------------
 
 Deprecated Compiler Flags
 -------------------------
 
 Modified Compiler Flags
 -----------------------
 
 Removed Compiler Flags
 -------------------------

clang/docs/UsersManual.rst

 The -fno-crash-diagnostics flag can be helpful for speeding the process
 of generating a delta reduced test case.
 
 .. option:: -fcrash-diagnostics-dir=<dir>
 
   Specify where to write the crash diagnostics files; defaults to the
   usual location for temporary files.
 
+.. envvar:: CLANG_CRASH_DIAGNOSTICS_DIR=<dir>
+
+   Like :option:`-fcrash-diagnostics-dir=<dir>`, specifies where to write the
+   crash diagnostics files, but with lower precedence than the option.
+
 Clang is also capable of generating preprocessed source file(s) and associated
 run script(s) even without a crash. This is specially useful when trying to
 generate a reproducer for warnings or errors while using modules.
 
 .. option:: -gen-reproducer
 
   Generates preprocessed source files, a reproducer script and if relevant, a
   cache containing: built module pcm's and all headers needed to rebuild the

clang/lib/Driver/Driver.cpp

if (isa<OffloadBundlingJobAction>(JA) &&
     return true;
   return false;
 }
 
 const char *Driver::CreateTempFile(Compilation &C, StringRef Prefix,
                                    StringRef Suffix, bool MultipleArchs,
                                    StringRef BoundArch) const {
   SmallString<128> TmpName;
   Arg *A = C.getArgs().getLastArg(options::OPT_fcrash_diagnostics_dir);
-  if (CCGenDiagnostics && A) {
-    SmallString<128> CrashDirectory(A->getValue());
-    if (!getVFS().exists(CrashDirectory))
-      llvm::sys::fs::create_directories(CrashDirectory);
-    llvm::sys::path::append(CrashDirectory, Prefix);
+  Optional<std::string> CrashDirectory =
+      CCGenDiagnostics && A
+          ? std::string(A->getValue())
+          : llvm::sys::Process::GetEnv("CLANG_CRASH_DIAGNOSTICS_DIR");
+  if (CrashDirectory) {
+    if (!getVFS().exists(*CrashDirectory))
+      llvm::sys::fs::create_directories(*CrashDirectory);
+    SmallString<128> Path(*CrashDirectory);
+    llvm::sys::path::append(Path, Prefix);
     const char *Middle = !Suffix.empty() ? "-%%%%%%." : "-%%%%%%";
-    std::error_code EC = llvm::sys::fs::createUniqueFile(
-        CrashDirectory + Middle + Suffix, TmpName);
-    if (EC) {
+    if (std::error_code EC =
+            llvm::sys::fs::createUniqueFile(Path + Middle + Suffix, TmpName)) {
       Diag(clang::diag::err_unable_to_make_temp) << EC.message();
       return "";
     }
   } else {
     if (MultipleArchs && !BoundArch.empty()) {
       TmpName = GetTemporaryDirectory(Prefix);
       llvm::sys::path::append(TmpName,
                               Twine(Prefix) + "-" + BoundArch + "." + Suffix);

clang/test/Driver/crash-diagnostics-dir-3.c

+// RUN: rm -rf %t
+// RUN: not env CLANG_CRASH_DIAGNOSTICS_DIR=%t %clang -c %s -o - 2>&1 | FileCheck %s
+#pragma clang __debug parser_crash
+// CHECK: Preprocessed source(s) and associated run script(s) are located at:
+// CHECK: diagnostic msg: {{.*}}{{/|\\}}crash-diagnostics-dir-3.c.tmp{{(/|\\).*}}.c

libcxx/utils/ci/buildkite-pipeline.yml

- label: "MSAN"
       timeout_in_minutes: 120
 
   # Tests with the various supported ways to build libc++.
   - label: "Bootstrapping build"
     command: "libcxx/utils/ci/run-buildbot bootstrapping-build"
     artifact_paths:
       - "**/test-results.xml"
       - "**/*.abilist"
+      - "**/crash_diagnostics/*"
     env:
         CC: "clang-${LLVM_HEAD_VERSION}"
         CXX: "clang++-${LLVM_HEAD_VERSION}"
         LLVM_SYMBOLIZER_PATH: "/usr/bin/llvm-symbolizer-${LLVM_HEAD_VERSION}"
+        CLANG_CRASH_DIAGNOSTICS_DIR: "crash_diagnostics"
     agents:
       queue: "libcxx-builders"
       os: "linux"
     retry:
       automatic:
         - exit_status: -1  # Agent was lost
           limit: 2
     timeout_in_minutes: 120