[Clang] Extend -gen-reproducer flag

[abrachet]

Summary

-gen-reproducer causes crash reproduction to be emitted even when clang didn't crash, and now can optionally take an argument of never, on-crash (default), on-error and always.

[dblaikie]

This comment was removed by dblaikie.

[dblaikie]

What's the purpose of this? (it'll need test coverage before it's committed, but some design discussion could probably come before that)

[abrachet]

In D120201#3334217, @dblaikie wrote:

What's the purpose of this? (it'll need test coverage before it's committed, but some design discussion could probably come before that)

I've tried to give more context in the description, hopefully it elaborates enough.

What other tests would you reckon are necessary here?

[phosek]

In D120201#3334217, @dblaikie wrote:

What's the purpose of this? (it'll need test coverage before it's committed, but some design discussion could probably come before that)

This is something that came up repeatedly when debugging compiler errors that show up in our CI. Currently, that usually requires reproducing the exact build so you can replay the compiler invocation and try various options to debug the issue. That process can take significant amount of time. In comparison, for Clang crashes debugging is much simpler because we automatically upload compiler reproducers to a cloud storage location and so reproducing those is simply a matter of downloading the reproducer and rerunning it locally. This got us thinking, could we make reproducing compiler errors as easy as reproducing compiler crashes, hence this feature.

[phosek]

In D120201#3338757, @phosek wrote:

In D120201#3334217, @dblaikie wrote:

What's the purpose of this? (it'll need test coverage before it's committed, but some design discussion could probably come before that)

This is something that came up repeatedly when debugging compiler errors that show up in our CI. Currently, that usually requires reproducing the exact build so you can replay the compiler invocation and try various options to debug the issue. That process can take significant amount of time. In comparison, for Clang crashes debugging is much simpler because we automatically upload compiler reproducers to a cloud storage location and so reproducing those is simply a matter of downloading the reproducer and rerunning it locally. This got us thinking, could we make reproducing compiler errors as easy as reproducing compiler crashes, hence this feature.

I forgot to mention, we would welcome other ideas or suggestions, this is just the direction that made most sense to us based on our experience.

[xbolva00]

clang/include/clang/Driver/Options.td

1380

diagnostics

1382

source

[abrachet]

Fixed typos

[abrachet]

clang/include/clang/Driver/Options.td

1380

Thanks!

[dblaikie]

In D120201#3338763, @phosek wrote:

In D120201#3338757, @phosek wrote:

In D120201#3334217, @dblaikie wrote:

What's the purpose of this? (it'll need test coverage before it's committed, but some design discussion could probably come before that)

This is something that came up repeatedly when debugging compiler errors that show up in our CI. Currently, that usually requires reproducing the exact build so you can replay the compiler invocation and try various options to debug the issue. That process can take significant amount of time. In comparison, for Clang crashes debugging is much simpler because we automatically upload compiler reproducers to a cloud storage location and so reproducing those is simply a matter of downloading the reproducer and rerunning it locally. This got us thinking, could we make reproducing compiler errors as easy as reproducing compiler crashes, hence this feature.

I forgot to mention, we would welcome other ideas or suggestions, this is just the direction that made most sense to us based on our experience.

Fair enough - yeah, I can see the appeal, though does feel somewhat awkward. Maybe a more generic tool would be suitable? (I guess you might want to reproduce compilations that don't error too, for instance - how'd did I get an object file with this feature, why isn't this symbol produced here, etc) So maybe some sort of --save_temps mode would be suitable?

[aaron.ballman]

In D120201#3338827, @dblaikie wrote:

In D120201#3338763, @phosek wrote:

In D120201#3338757, @phosek wrote:

In D120201#3334217, @dblaikie wrote:

What's the purpose of this? (it'll need test coverage before it's committed, but some design discussion could probably come before that)

This is something that came up repeatedly when debugging compiler errors that show up in our CI. Currently, that usually requires reproducing the exact build so you can replay the compiler invocation and try various options to debug the issue. That process can take significant amount of time. In comparison, for Clang crashes debugging is much simpler because we automatically upload compiler reproducers to a cloud storage location and so reproducing those is simply a matter of downloading the reproducer and rerunning it locally. This got us thinking, could we make reproducing compiler errors as easy as reproducing compiler crashes, hence this feature.

I forgot to mention, we would welcome other ideas or suggestions, this is just the direction that made most sense to us based on our experience.

Fair enough - yeah, I can see the appeal, though does feel somewhat awkward. Maybe a more generic tool would be suitable? (I guess you might want to reproduce compilations that don't error too, for instance - how'd did I get an object file with this feature, why isn't this symbol produced here, etc) So maybe some sort of --save_temps mode would be suitable?

I can also see the appeal, but I agree with David... doesn't the same situation arise for warnings as it does for errors? With errors, I can understand "crash on the first error" as being reasonable behavior, but for warnings, I think it's very plausible for users to want to crash on a *specific* warning, but I also think that applies to errors to a lesser extent. So I'm not certain how we extend this for those kinds of use cases. With warnings, we could perhaps find a way to support -fcrash-diagnostics-on-warning=strict-prototypes, but for errors, I don't know how we'd do that. (It could be that we punt on that until later, but if there's a more general design we can figure out up front, that would be better IMO.)

[abrachet]

In D120201#3341270, @aaron.ballman wrote:

I can also see the appeal, but I agree with David... doesn't the same situation arise for warnings as it does for errors? With errors, I can understand "crash on the first error" as being reasonable behavior, but for warnings, I think it's very plausible for users to want to crash on a *specific* warning, but I also think that applies to errors to a lesser extent. So I'm not certain how we extend this for those kinds of use cases. With warnings, we could perhaps find a way to support -fcrash-diagnostics-on-warning=strict-prototypes, but for errors, I don't know how we'd do that. (It could be that we punt on that until later, but if there's a more general design we can figure out up front, that would be better IMO.)

To be clear this doesn't crash on the first error. It just emits diagnostics if the command failed, as if it crashed. To have the same behavior with warnings -Werror should work well enough.

I agree with the two of you that this kind of feature could be designed better, at present it's just a quick way to get these files using an existing mechanism. Do either of you have any ideas on a better direction? lld has a really cool --reproduce flag which writes every file it opened to an output tar file. That would be very useful in clang for reproducing errors, but it's presumably a large undertaking.

[dblaikie]

In D120201#3341333, @abrachet wrote:

In D120201#3341270, @aaron.ballman wrote:

I can also see the appeal, but I agree with David... doesn't the same situation arise for warnings as it does for errors? With errors, I can understand "crash on the first error" as being reasonable behavior, but for warnings, I think it's very plausible for users to want to crash on a *specific* warning, but I also think that applies to errors to a lesser extent. So I'm not certain how we extend this for those kinds of use cases. With warnings, we could perhaps find a way to support -fcrash-diagnostics-on-warning=strict-prototypes, but for errors, I don't know how we'd do that. (It could be that we punt on that until later, but if there's a more general design we can figure out up front, that would be better IMO.)

To be clear this doesn't crash on the first error. It just emits diagnostics if the command failed, as if it crashed. To have the same behavior with warnings -Werror should work well enough.

I agree with the two of you that this kind of feature could be designed better, at present it's just a quick way to get these files using an existing mechanism. Do either of you have any ideas on a better direction? lld has a really cool --reproduce flag which writes every file it opened to an output tar file. That would be very useful in clang for reproducing errors, but it's presumably a large undertaking.

I'd have thought something like -save-temps could be used? Fleshed out to include the crash script-like command line reproduction, the preprocessed source, etc.

[phosek]

In D120201#3341333, @abrachet wrote:

In D120201#3341270, @aaron.ballman wrote:

I can also see the appeal, but I agree with David... doesn't the same situation arise for warnings as it does for errors? With errors, I can understand "crash on the first error" as being reasonable behavior, but for warnings, I think it's very plausible for users to want to crash on a *specific* warning, but I also think that applies to errors to a lesser extent. So I'm not certain how we extend this for those kinds of use cases. With warnings, we could perhaps find a way to support -fcrash-diagnostics-on-warning=strict-prototypes, but for errors, I don't know how we'd do that. (It could be that we punt on that until later, but if there's a more general design we can figure out up front, that would be better IMO.)

To be clear this doesn't crash on the first error. It just emits diagnostics if the command failed, as if it crashed. To have the same behavior with warnings -Werror should work well enough.

I agree with the two of you that this kind of feature could be designed better, at present it's just a quick way to get these files using an existing mechanism. Do either of you have any ideas on a better direction? lld has a really cool --reproduce flag which writes every file it opened to an output tar file. That would be very useful in clang for reproducing errors, but it's presumably a large undertaking.

Clang has -gen-reproducer option introduced in D27604.

[phosek]

In D120201#3342204, @phosek wrote:

In D120201#3341333, @abrachet wrote:

In D120201#3341270, @aaron.ballman wrote:

I can also see the appeal, but I agree with David... doesn't the same situation arise for warnings as it does for errors? With errors, I can understand "crash on the first error" as being reasonable behavior, but for warnings, I think it's very plausible for users to want to crash on a *specific* warning, but I also think that applies to errors to a lesser extent. So I'm not certain how we extend this for those kinds of use cases. With warnings, we could perhaps find a way to support -fcrash-diagnostics-on-warning=strict-prototypes, but for errors, I don't know how we'd do that. (It could be that we punt on that until later, but if there's a more general design we can figure out up front, that would be better IMO.)

To be clear this doesn't crash on the first error. It just emits diagnostics if the command failed, as if it crashed. To have the same behavior with warnings -Werror should work well enough.

I agree with the two of you that this kind of feature could be designed better, at present it's just a quick way to get these files using an existing mechanism. Do either of you have any ideas on a better direction? lld has a really cool --reproduce flag which writes every file it opened to an output tar file. That would be very useful in clang for reproducing errors, but it's presumably a large undertaking.

Clang has -gen-reproducer option introduced in D27604.

One possible direction would be to extend -gen-reproducer to take a value so you could specify -gen-reproducer=error (or perhaps -gen-reproducer=on-error).

[dblaikie]

I'm perhaps missing why this is desirable to be "on error" especially - is your use case to have this enabled by default in a distributed build scenario, so end users can locally reproduce the failure for further investigation? (Because the distributed build has too much overhead to iterate efficiently)

I was thinking this was more for compiler developers to investigate - so they could opt into the flag only when investigating (I guess on-error would mean that if they passed the flag to the whole build it would only dump output on the erroring compilations, not every compilation action in the build - is that the issue this is intended to address? Would some more general build feature (Bazel has this, not sure about other build systems) to pass flags only to particular actions be useful for this and other things?)

[abrachet]

In D120201#3343509, @dblaikie wrote:

I'm perhaps missing why this is desirable to be "on error" especially - is your use case to have this enabled by default in a distributed build scenario, so end users can locally reproduce the failure for further investigation? (Because the distributed build has too much overhead to iterate efficiently)

I was thinking this was more for compiler developers to investigate - so they could opt into the flag only when investigating (I guess on-error would mean that if they passed the flag to the whole build it would only dump output on the erroring compilations, not every compilation action in the build - is that the issue this is intended to address? Would some more general build feature (Bazel has this, not sure about other build systems) to pass flags only to particular actions be useful for this and other things?)

What do you think about this kind of direction?

[dblaikie]

This direction sounds better to me - could the tar file support be added independntly, perhaps as an improvement to the existing crash reproduction infrastructure? (ideally making this new functionality as small/simple as possible - "just give me the same thing the crash reproducer does, because I want it/even if it's not crashing")

[xbolva00]

+1, useful.

[abrachet]

In D120201#3377387, @dblaikie wrote:

This direction sounds better to me - could the tar file support be added independntly, perhaps as an improvement to the existing crash reproduction infrastructure?

I'm weary about changing the default behavior of the crash reproduction infrastructure. There's a lot of subtly and platform specific (mostly Darwin) handling. Although I think something like this is better than the current crash reproducer. It's more generic and could be used for any crashing job not just the compiler, and one tar file is a lot easier to upload when submitting a bug than multiple files.

Adding folks who added the Apple specific handling if they want to chime in. @bogner @bruno @t.p.northover

(ideally making this new functionality as small/simple as possible - "just give me the same thing the crash reproducer does, because I want it/even if it's not crashing")

Agreed, this was the initial plan, albeit on error and not always.

In D120201#3343509, @dblaikie wrote:

I'm perhaps missing why this is desirable to be "on error" especially - is your use case to have this enabled by default in a distributed build scenario, so end users can locally reproduce the failure for further investigation? (Because the distributed build has too much overhead to iterate efficiently)

I was thinking this was more for compiler developers to investigate - so they could opt into the flag only when investigating

That was not the initial intention, although where this patch is now, a developer could use -femit-reproducer=always for that purpose.

(I guess on-error would mean that if they passed the flag to the whole build it would only dump output on the erroring compilations, not every compilation action in the build - is that the issue this is intended to address?

Yes, we think this has value specifically when the compilation is happening asynchronously, like on bots, where you don't have the luxury of just rerunning a failed build step with a new flag. Being able to easily get the preprocessed source file is super useful in this case where having to go find all the includes on your own could be a huge pain. We specifically compile our code base with a ToT llvm, for us a lot of errors could be something like libcxx changed some configuration, and being able to see the preprocessed source file in these cases is helpful.

FWIW, most errors are going to be straightforward enough that the error message alone should be enough. But having an opt-in flag you can set globally that only affects failing compilations to help debug why they failed I think has value.

Would some more general build feature (Bazel has this, not sure about other build systems) to pass flags only to particular actions be useful for this and other things?)

Could you expand on this? Does Bazel have a way to rerun a failed action?

[phosek]

In D120201#3377514, @abrachet wrote:

(I guess on-error would mean that if they passed the flag to the whole build it would only dump output on the erroring compilations, not every compilation action in the build - is that the issue this is intended to address?

Yes, we think this has value specifically when the compilation is happening asynchronously, like on bots, where you don't have the luxury of just rerunning a failed build step with a new flag. Being able to easily get the preprocessed source file is super useful in this case where having to go find all the includes on your own could be a huge pain. We specifically compile our code base with a ToT llvm, for us a lot of errors could be something like libcxx changed some configuration, and being able to see the preprocessed source file in these cases is helpful.

FWIW, most errors are going to be straightforward enough that the error message alone should be enough. But having an opt-in flag you can set globally that only affects failing compilations to help debug why they failed I think has value.

To provide another use case, we have bots that cover large number of build configurations whereas most developers build only the most common ones locally. Sometimes, we see build errors that only impact some builders that use the more exotic configurations and we have to replicate the build locally to reproduce the issue which can take non-trivial effort.

With this feature, we would like to simplify the process by automatically collecting reproducers on errors so developers don't need to replicate the full build in order to reproduce a specific build error.

I don't think this use case is specific only to Fuchsia, the same is true for other projects. In LLVM, we often see build errors only on less common bot configurations and having reproducers available for those cases might help LLVM developers as well.

[aaron.ballman]

I like this direction, thank you! I think you should also add a release note for the new functionality.

clang/include/clang/Driver/Options.td

1383

[xbolva00]

clang/tools/driver/driver.cpp

492

Make default value configurable with cmake variable?

[dblaikie]

In D120201#3377514, @abrachet wrote:

In D120201#3377387, @dblaikie wrote:

This direction sounds better to me - could the tar file support be added independntly, perhaps as an improvement to the existing crash reproduction infrastructure?

I'm weary about changing the default behavior of the crash reproduction infrastructure. There's a lot of subtly and platform specific (mostly Darwin) handling. Although I think something like this is better than the current crash reproducer. It's more generic and could be used for any crashing job not just the compiler, and one tar file is a lot easier to upload when submitting a bug than multiple files.

I appreciate that, but would also like to avoid building subtly different-yet-similar functionality (especially for things that are off-by-default and so might be under-exercised if they have their own standalone implementation). I think it's worth trying to reconcile the functionality as much as possible.

Another thing that might be good is if you could work to enable this by default on LLVM buildbots (if there's a practical way to make these reproducers available to developers via the buildbot infrastructure somehow) - that'd add value to LLVM developers and ensure the feature is exercised regularly.

maybe the crash reproducer functionality could be the default here - could have = {none, crash, error, always} and "crash" being the default (& I guess then "error" has to be "error or crash") but maybe that's not helpful - perhaps no one would actually want to turn it off entirely?

Adding folks who added the Apple specific handling if they want to chime in. @bogner @bruno @t.p.northover

Yep, happy to get some more perspective for sure.

(ideally making this new functionality as small/simple as possible - "just give me the same thing the crash reproducer does, because I want it/even if it's not crashing")

Agreed, this was the initial plan, albeit on error and not always.

Ah, right.

In D120201#3343509, @dblaikie wrote:

I'm perhaps missing why this is desirable to be "on error" especially - is your use case to have this enabled by default in a distributed build scenario, so end users can locally reproduce the failure for further investigation? (Because the distributed build has too much overhead to iterate efficiently)

I was thinking this was more for compiler developers to investigate - so they could opt into the flag only when investigating

That was not the initial intention, although where this patch is now, a developer could use -femit-reproducer=always for that purpose.

(I guess on-error would mean that if they passed the flag to the whole build it would only dump output on the erroring compilations, not every compilation action in the build - is that the issue this is intended to address?

Yes, we think this has value specifically when the compilation is happening asynchronously, like on bots, where you don't have the luxury of just rerunning a failed build step with a new flag. Being able to easily get the preprocessed source file is super useful in this case where having to go find all the includes on your own could be a huge pain. We specifically compile our code base with a ToT llvm, for us a lot of errors could be something like libcxx changed some configuration, and being able to see the preprocessed source file in these cases is helpful.

FWIW, most errors are going to be straightforward enough that the error message alone should be enough. But having an opt-in flag you can set globally that only affects failing compilations to help debug why they failed I think has value.

Would some more general build feature (Bazel has this, not sure about other build systems) to pass flags only to particular actions be useful for this and other things?)

Could you expand on this? Does Bazel have a way to rerun a failed action?

No, not that I know of - but it does have this: https://docs.bazel.build/versions/main/user-manual.html#flag--per_file_copt - which could be used on a manual rerun. (I assume that most of bazel's custom configuration is done on the bazel command line rather than in a pre-configure step? (at least that's mostly how it works internally) so mostly you could copy the bazel command from the buildbot, add a per_file_copt to dump the reproducer, and reproduce the build locally? but I guess if you could already copy/paste the bazel command then you wouldn't need the reproducer feature... so nevermind that I guess)

[rnk]

It seems like this change is doing two things:

• producing reproducer files on any error, not just crashes
• producing tar file reproducers

Both of those seem like reasonable features, but I think it would be nice to separate them, and make sure that they integrate into all of the existing crash reproducer functionality, like freproducer-dir (I see it handles this already).

I can imagine that, depending on the build failure mode, producing a pre-processed reproducer may work better than producing a tar file. It would be nice to let the user choose.

The tar file logic doesn't seem complete. The set of files needed to reproduce a command is far more than just the input files listed on the command line.

In D120201#3378354, @phosek wrote:

To provide another use case, we have bots that cover large number of build configurations whereas most developers build only the most common ones locally. Sometimes, we see build errors that only impact some builders that use the more exotic configurations and we have to replicate the build locally to reproduce the issue which can take non-trivial effort.

With this feature, we would like to simplify the process by automatically collecting reproducers on errors so developers don't need to replicate the full build in order to reproduce a specific build error.

I don't think this use case is specific only to Fuchsia, the same is true for other projects. In LLVM, we often see build errors only on less common bot configurations and having reproducers available for those cases might help LLVM developers as well.

I'm not sure I understand who the user of the reproducer file is here. Are we talking about compiler developers or compiler users? I don't see how a user would have a use case for these reproducer tar files, except perhaps a as a way to report non-crash bugs (error-on-valid). This feature mainly seems useful as a way for compiler developers to automatically collect compiler inputs from complex build systems. This could be especially useful when modules are involved.

clang/test/Driver/reproduce.c

3 ↗

(On Diff #414811)

This test seems pretty shell-y. Make sure it passes the Windows presubmit tests.

[abrachet]

Emit reproducer tar file unconditionally on crashes.

[abrachet]

In D120201#3381444, @dblaikie wrote:

I appreciate that, but would also like to avoid building subtly different-yet-similar functionality (especially for things that are off-by-default and so might be under-exercised if they have their own standalone implementation). I think it's worth trying to reconcile the functionality as much as possible.

Sure that makes sense. I'm thinking we could do a soft transition. So for now we have the reproducer file included alongside the other files that are currently emitted. Then maybe send out a thread on the mailing list that the intention is to remove the individual files in lieu of just the tar file in the near future.

Another thing that might be good is if you could work to enable this by default on LLVM buildbots (if there's a practical way to make these reproducers available to developers via the buildbot infrastructure somehow) - that'd add value to LLVM developers and ensure the feature is exercised regularly.

Indeed, that sounds like a good plan, I will look into that.

In D120201#3387159, @rnk wrote:

It seems like this change is doing two things:

• producing reproducer files on any error, not just crashes
• producing tar file reproducers

Both of those seem like reasonable features, but I think it would be nice to separate them, and make sure that they integrate into all of the existing crash reproducer functionality, like freproducer-dir (I see it handles this already).

I can imagine that, depending on the build failure mode, producing a pre-processed reproducer may work better than producing a tar file. It would be nice to let the user choose.

The tar file logic doesn't seem complete. The set of files needed to reproduce a command is far more than just the input files listed on the command line.

It isn't meant to be a completely comprehensive way to reproduce a compilation action. Libraries implicitly added to the link will of course not be here, but header files will because the preprocessed sources will be emited.

Is there anything in particular you think is missing?

In D120201#3378354, @phosek wrote:

To provide another use case, we have bots that cover large number of build configurations whereas most developers build only the most common ones locally. Sometimes, we see build errors that only impact some builders that use the more exotic configurations and we have to replicate the build locally to reproduce the issue which can take non-trivial effort.

With this feature, we would like to simplify the process by automatically collecting reproducers on errors so developers don't need to replicate the full build in order to reproduce a specific build error.

I don't think this use case is specific only to Fuchsia, the same is true for other projects. In LLVM, we often see build errors only on less common bot configurations and having reproducers available for those cases might help LLVM developers as well.

I'm not sure I understand who the user of the reproducer file is here. Are we talking about compiler developers or compiler users? I don't see how a user would have a use case for these reproducer tar files, except perhaps a as a way to report non-crash bugs (error-on-valid). This feature mainly seems useful as a way for compiler developers to automatically collect compiler inputs from complex build systems. This could be especially useful when modules are involved.

I think it can be useful for both. Our primary motivation here was to get most of the files necessary to reproduce a failed compilation very easily.

In D120201#3379185, @aaron.ballman wrote:

I like this direction, thank you! I think you should also add a release note for the new functionality.

Ack. I will do this when we get closer to consensus.

clang/test/Driver/reproduce.c

3 ↗

(On Diff #414811)

Thanks. Looks like stat was not available.

[bruno]

This is pretty cool, I enjoy the idea of getting a tar out of a crash. I'm also a +1 for having this group of behaviors as a more official -femit-reproducer=<option> flag. In future work, do you plan to change the default crash mode to output a tar instead of multiple files?

For this specific patch: it's fine that both -gen-reproducer and FORCE_CLANG_DIAGNOSTICS_CRASH have their own specific meanings and the driver should parse both in terms of -femit-reproducer=always + TURN_OFF_TAR, so I'd prefer if the patch steers a bit more into that direction. More comments inline.

clang/tools/driver/driver.cpp

486

Can you relate this to a enum here already? Perhaps use some bits for the level style and one to track using TAR?

492

This is the same path as -femit-reproducer=always minus a few specific things, this path could be unified.

[dblaikie]

I tihnk I've lost track of the variants here - if you're waiting on review feedback, at least for me, it'd be helpful to have a high level description of the state of the patch, what it does/doesn't do in terms of how it interacts with existing crash reporting and what new functionality it exposes - and the planned direction (things you intend to do in a relatively timely fashion, versus "things someone could do at some point in the future/if they were so inclined").

Soft transition has some value, though some risk (that it's never finished and stays in a hybrid state) - I'd rather avoid it if it's not too hard - post something to the forums and see if anyone would be particularly broken by moving to a tar file maybe? But if other folks feel more strongly about a slower roll out, that's OK.

[abrachet]

In D120201#3400286, @dblaikie wrote:

I tihnk I've lost track of the variants here - if you're waiting on review feedback, at least for me, it'd be helpful to have a high level description of the state of the patch, what it does/doesn't do in terms of how it interacts with existing crash reporting and what new functionality it exposes - and the planned direction (things you intend to do in a relatively timely fashion, versus "things someone could do at some point in the future/if they were so inclined").

I've split this patch up, as suggested by @rnk. This patch now just adds the -femit-reproducer= option and D122335 changes crash reproduction into a tar file.

Soft transition has some value, though some risk (that it's never finished and stays in a hybrid state) - I'd rather avoid it if it's not too hard - post something to the forums and see if anyone would be particularly broken by moving to a tar file maybe? But if other folks feel more strongly about a slower roll out, that's OK.

Sure, I've gone with a hard transition and have posted about it on the mailing list https://discourse.llvm.org/t/changing-clangs-crash-reproduction-feature/61171

[rnk]

Thanks, I like this approach. I haven't had a chance to do more detailed code review in the last week, if someone else can.

[MaskRay]

clang/test/Driver/emit-reproducer.c

25 ↗

(On Diff #417696)

This is the first time tar is used in a test. I am unsure whether all Windows bots will provide it. Probably prepared that you may need UNSUPPORTED: system-windows

41 ↗

(On Diff #417696)

No newline at end of file

clang/tools/driver/driver.cpp

486

Consider enum class to not add common names like Off Always to the class.

[hans]

clang/test/Driver/emit-reproducer.c

25 ↗

(On Diff #417696)

Many lld tests use tar already, also on Windows. (e.g. lld/test/ELF/reproduce-error.s which doesn't have any special requirements), so hopefully it will work for Clang tests too.

[abrachet]

D122335 was a requested feature for emitting reproducers as a single tar file, but it seems to have stalled. What do folks think about moving forward with this patch without emitting the reproducer as a tar file?

[abrachet]

This patch no longer depends on D122335 which created the reproducer as a tar file. Now this will just emit the crash reproduction information as usual, but under other circumstances given the value of -femit-reproducer.

[hans]

In D120201#3531499, @abrachet wrote:

This patch no longer depends on D122335 which created the reproducer as a tar file. Now this will just emit the crash reproduction information as usual, but under other circumstances given the value of -femit-reproducer.

Thanks, I think the current functionality makes sense to me, and the patch seems very focused now.

My only high-level comment is that it seems this overlaps with the existing -gen-reproducer flag and FORCE_CLANG_DIAGNOSTICS_CRASH environment variable. Could the new functionality be implemented as new arguments to the existing flag/variable?

[abrachet]

In D120201#3534534, @hans wrote:

In D120201#3531499, @abrachet wrote:

This patch no longer depends on D122335 which created the reproducer as a tar file. Now this will just emit the crash reproduction information as usual, but under other circumstances given the value of -femit-reproducer.

Thanks, I think the current functionality makes sense to me, and the patch seems very focused now.

My only high-level comment is that it seems this overlaps with the existing -gen-reproducer flag and FORCE_CLANG_DIAGNOSTICS_CRASH environment variable. Could the new functionality be implemented as new arguments to the existing flag/variable?

Good call. I've moved away from the new flag in favor of reusing -gen-reproducer for this.

[hans]

clang/include/clang/Driver/Driver.h

482

I think the more typical clang name for this would be maybeGenerateCompilationDiagnostics

clang/include/clang/Driver/Options.td

558

Should we just drop the "on-" prefixes? It makes them a little less clear, but shorter and cleaner (and hyphens in arguments seems unusual).

clang/lib/Driver/Driver.cpp

1172

Would it make sense to move this to driver.cpp with the rest of the logic? That way we could drop the variable too.

clang/tools/driver/driver.cpp

488

The old code seems to pretend every command failed?

493

Should we reject or at least warn about invalid arguments here?

[abrachet]

clang/tools/driver/driver.cpp

488

It later looped through all failing commands and then if it was a crash, which it made them seem, and then would emit the reproducer and then break out of the loop. So it essentially only acted as if the first command failed.

[hans]

lgtm

clang/test/Driver/emit-reproducer.c

1 ↗

(On Diff #432047)

I'm not sure if lit handles that semicolon, or if it hands this over to the shell, in which case it won't work on windows.
Instead, rm -rf %t && mkdir %t seems common among clang tests.

Otherwise, this is a nice test file :-)

[abrachet]

clang/test/Driver/emit-reproducer.c

1 ↗

(On Diff #432047)

Thanks :) updated it in the commit.

[thakis]

Looks like this breaks tests on (at least) Mac and window, see eg http://45.33.8.238/macm1/36198/step_7.txt (passes on my Linux bit though).

Please take a look and revert for now if it takes a while to fix.

[abrachet]

Fix tests on macOS and compile test with -fsyntax-only

[dyung]

The test you added seems to be failing on the PS4 Windows bot. A quick glance seems to suggest that you aren't properly escaping the path separators somewhere. Can you take a look and revert if you need time to investigate?

https://lab.llvm.org/buildbot/#/builders/216/builds/5164

[abrachet]

In D120201#3547834, @dyung wrote:

The test you added seems to be failing on the PS4 Windows bot. A quick glance seems to suggest that you aren't properly escaping the path separators somewhere. Can you take a look and revert if you need time to investigate?

https://lab.llvm.org/buildbot/#/builders/216/builds/5164

Should be fixed by https://github.com/llvm/llvm-project/commit/a0ef52cc102504c4282dec7001664ee020396681

[dyung]

In D120201#3547835, @abrachet wrote:

In D120201#3547834, @dyung wrote:

The test you added seems to be failing on the PS4 Windows bot. A quick glance seems to suggest that you aren't properly escaping the path separators somewhere. Can you take a look and revert if you need time to investigate?

https://lab.llvm.org/buildbot/#/builders/216/builds/5164

Should be fixed by https://github.com/llvm/llvm-project/commit/a0ef52cc102504c4282dec7001664ee020396681

Indeed it has, thanks for the quick action!

[vitalybuka]

This patch breaks msan bots: https://lab.llvm.org/buildbot/#/builders/5/builds/24307 and https://lab.llvm.org/buildbot/#/builders/74

https://lab.llvm.org/buildbot/#/builders/5/builds/24335 is (last green build https://lab.llvm.org/buildbot/#/builders/5/builds/24306 + this patch)

FYI @browneee

clang/include/clang/Driver/Driver.h

public:
   /// generateCompilationDiagnostics - Generate diagnostics information
   /// including preprocessed source file(s).
   ///
   void generateCompilationDiagnostics(
       Compilation &C, const Command &FailingCommand,
       StringRef AdditionalInformation = "",
       CompilationDiagnosticReport *GeneratedReport = nullptr);
 
+  bool shouldEmitDiagnosticsOnError(Compilation &C) const;
+
   /// @}
   /// @name Helper Methods
   /// @{
 
   /// PrintActions - Print the list of actions.
   void PrintActions(const Compilation &C) const;
 
   /// PrintHelp - Print the help text.
   ///
   /// \param ShowHidden - Show hidden options.
   void PrintHelp(bool ShowHidden) const;
 
   /// PrintVersion - Print the driver version.
   void PrintVersion(const Compilation &C, raw_ostream &OS) const;
 
   /// GetFilePath - Lookup \p Name in the list of file search paths.
   ///
   /// \param TC - The tool chain for additional information on
   /// directories to search.
   //

clang/include/clang/Driver/Options.td

def ccc_arcmt_migrate : Separate<["-"], "ccc-arcmt-migrate">, InternalDriverOpt,
   HelpText<"Apply modifications and produces temporary files that conform to ARC">;
 def arcmt_migrate_report_output : Separate<["-"], "arcmt-migrate-report-output">,
   HelpText<"Output path for the plist report">,  Flags<[CC1Option]>,
   MarshallingInfoString<FrontendOpts<"ARCMTMigrateReportOut">>;
 def arcmt_migrate_emit_arc_errors : Flag<["-"], "arcmt-migrate-emit-errors">,
   HelpText<"Emit ARC errors even if the migrator can fix them">, Flags<[CC1Option]>,
   MarshallingInfoFlag<FrontendOpts<"ARCMTMigrateEmitARCErrors">>;
 def gen_reproducer: Flag<["-"], "gen-reproducer">, InternalDebugOpt,
   HelpText<"Auto-generates preprocessed source files and a reproduction script">;
 def gen_cdb_fragment_path: Separate<["-"], "gen-cdb-fragment-path">, InternalDebugOpt,
   HelpText<"Emit a compilation database fragment to the specified directory">;
 
 def round_trip_args : Flag<["-"], "round-trip-args">, Flags<[CC1Option, NoDriverOption]>,
   HelpText<"Enable command line arguments round-trip.">;
 def no_round_trip_args : Flag<["-"], "no-round-trip-args">, Flags<[CC1Option, NoDriverOption]>,
   HelpText<"Disable command line arguments round-trip.">;
 
def fexperimental_new_constant_interpreter : Flag<["-"], "fexperimental-new-constant-interpreter">, Group<f_Group>,
   MarshallingInfoFlag<LangOpts<"EnableNewConstInterp">>;
 def fconstexpr_backtrace_limit_EQ : Joined<["-"], "fconstexpr-backtrace-limit=">,
                                     Group<f_Group>;
 def fno_crash_diagnostics : Flag<["-"], "fno-crash-diagnostics">, Group<f_clang_Group>, Flags<[NoArgumentUnused, CoreOption]>,
   HelpText<"Disable auto-generation of preprocessed source files and a script for reproduction during a clang crash">;
 def fcrash_diagnostics_dir : Joined<["-"], "fcrash-diagnostics-dir=">,
   Group<f_clang_Group>, Flags<[NoArgumentUnused, CoreOption]>,
   HelpText<"Put crash-report files in <dir>">, MetaVarName<"<dir>">;
+def fcrash_diagnostics_on_err : Flag<["-"], "fcrash-diagnostics-on-error">,
+  Group<f_clang_Group>, Flags<[NoArgumentUnused, CoreOption]>,
+  HelpText<"Emit preprocessed source files and a script for reproduction on any tool failure">;
 def fcreate_profile : Flag<["-"], "fcreate-profile">, Group<f_Group>;
 defm cxx_exceptions: BoolFOption<"cxx-exceptions",
   LangOpts<"CXXExceptions">, DefaultFalse,
   PosFlag<SetTrue, [CC1Option], "Enable C++ exceptions">, NegFlag<SetFalse>>;
 defm async_exceptions: BoolFOption<"async-exceptions",
   LangOpts<"EHAsynch">, DefaultFalse,
   PosFlag<SetTrue, [CC1Option], "Enable EH Asynchronous exceptions">, NegFlag<SetFalse>>;
 defm cxx_modules : BoolFOption<"cxx-modules",
   LangOpts<"CPlusPlusModules">, Default<cpp20.KeyPath>,

clang/lib/Driver/Driver.cpp

Compilation *Driver::BuildCompilation(ArrayRef<const char *> ArgList) {
   // options, either by introducing new ones or by overloading gcc ones like -V
   // or -b.
   CCCPrintPhases = Args.hasArg(options::OPT_ccc_print_phases);
   CCCPrintBindings = Args.hasArg(options::OPT_ccc_print_bindings);
   if (const Arg *A = Args.getLastArg(options::OPT_ccc_gcc_name))
     CCCGenericGCCName = A->getValue();
   GenReproducer = Args.hasFlag(options::OPT_gen_reproducer,
                                options::OPT_fno_crash_diagnostics,
                                !!::getenv("FORCE_CLANG_DIAGNOSTICS_CRASH"));
 
   // Process -fproc-stat-report options.
   if (const Arg *A = Args.getLastArg(options::OPT_fproc_stat_report_EQ)) {
     CCPrintProcessStats = true;
     CCPrintStatReportFilename = A->getValue();
   }
   if (Args.hasArg(options::OPT_fproc_stat_report))
     CCPrintProcessStats = true;
if (!CrashFilePath.empty()) {
     if (EC)
       return false;
     return true;
   }
 
   return false;
 }
 
+bool Driver::shouldEmitDiagnosticsOnError(Compilation &C) const {
+  return C.getArgs().hasArg(options::OPT_fcrash_diagnostics_on_err);
+}
+
 // When clang crashes, produce diagnostic information including the fully
 // preprocessed source file(s).  Request that the developer attach the
 // diagnostic information to a bug report.
 void Driver::generateCompilationDiagnostics(
     Compilation &C, const Command &FailingCommand,
     StringRef AdditionalInformation, CompilationDiagnosticReport *Report) {
   if (C.getArgs().hasArg(options::OPT_fno_crash_diagnostics))
     return;

clang/test/Driver/crash-report.cpp

 // RUN: cat %t/crash-report-*.sh | FileCheck --check-prefix=CHECKSH %s
 
 // RUN: env TMPDIR=%t TEMP=%t TMP=%t RC_DEBUG_OPTIONS=1                  \
 // RUN:  CC_PRINT_HEADERS=1 CC_LOG_DIAGNOSTICS=1                         \
 // RUN:  not %clang %s @%t.rsp -DFATAL 2>&1 | FileCheck %s
 // RUN: cat %t/crash-report-*.cpp | FileCheck --check-prefix=CHECKSRC %s
 // RUN: cat %t/crash-report-*.sh | FileCheck --check-prefix=CHECKSH %s
 
+// RUN: env TMPDIR=%t TEMP=%t TMP=%t RC_DEBUG_OPTIONS=1                  \
+// RUN:  CC_PRINT_HEADERS=1 CC_LOG_DIAGNOSTICS=1                         \
+// RUN:  not %clang %s @%t.rsp -fcrash-diagnostics-on-error 2>&1 |        \
+// RUN:  FileCheck %s
+// RUN: cat %t/crash-report-*.cpp | FileCheck --check-prefix=CHECKSRC %s
+// RUN: cat %t/crash-report-*.sh | FileCheck --check-prefix=CHECKSH %s
+
 // REQUIRES: crash-recovery
 
 #ifdef PARSER
 #pragma clang __debug parser_crash
 #elif CRASH
 #pragma clang __debug crash
 #elif FATAL
 #pragma clang __debug llvm_fatal_error

clang/tools/driver/driver.cpp

int main(int Argc, const char **Argv) {
   if (!UseNewCC1Process) {
     TheDriver.CC1Main = &ExecuteCC1Tool;
     // Ensure the CC1Command actually catches cc1 crashes
     llvm::CrashRecoveryContext::Enable();
   }
 
   std::unique_ptr<Compilation> C(TheDriver.BuildCompilation(Args));
   int Res = 1;
   bool IsCrash = false;
   if (C && !C->containsError()) {
     SmallVector<std::pair<int, const Command *>, 4> FailingCommands;
     Res = TheDriver.ExecuteCompilation(*C, FailingCommands);
 
     // Force a crash to test the diagnostics.
     if (TheDriver.GenReproducer) {
       Diags.Report(diag::err_drv_force_crash)
         << !::getenv("FORCE_CLANG_DIAGNOSTICS_CRASH");
 
       // Pretend that every command failed.
       FailingCommands.clear();
       for (const auto &J : C->getJobs())
         if (const Command *C = dyn_cast<Command>(&J))
           FailingCommands.push_back(std::make_pair(-1, C));
 
#if LLVM_ON_UNIX
       // the same codes as if the program crashed. See section "Exit Status for
       // Commands":
       // https://pubs.opengroup.org/onlinepubs/9699919799/xrat/V4_xcu_chap02.html
       IsCrash |= CommandRes > 128;
 #endif
       if (IsCrash) {
         TheDriver.generateCompilationDiagnostics(*C, *FailingCommand);
         break;
+      } else if (TheDriver.shouldEmitDiagnosticsOnError(*C)) {
+        // Hack to ensure that diagnostic notes get emitted.
+        Diags.setLastDiagnosticIgnored(false);
+        TheDriver.generateCompilationDiagnostics(*C, *FailingCommand);
       }
     }
   }
 
   Diags.getClient()->finish();
 
   if (!UseNewCC1Process && IsCrash) {
     // When crashing in -fintegrated-cc1 mode, bury the timer pointers, because