This is an archive of the discontinued LLVM Phabricator instance.

[Clang] Extend -gen-reproducer flag
ClosedPublic

Authored by abrachet on Feb 19 2022, 8:08 PM.

Details

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.

Diff Detail

Event Timeline

abrachet created this revision.Feb 19 2022, 8:08 PM
abrachet requested review of this revision.Feb 19 2022, 8:08 PM
This comment was removed by 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 edited the summary of this revision. (Show Details)Feb 22 2022, 2:28 PM

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?

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.

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 added inline comments.
clang/include/clang/Driver/Options.td
1380

diagnostics

1382

source

abrachet updated this revision to Diff 410651.Feb 22 2022, 3:07 PM
abrachet retitled this revision from [Clang] Add -fcrash-diagnostcs-on-error flag to [Clang] Add -fcrash-diagnostics-on-error flag.

Fixed typos

abrachet marked 2 inline comments as done.Feb 22 2022, 3:07 PM
abrachet added inline comments.
clang/include/clang/Driver/Options.td
1380

Thanks!

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?

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 marked an inline comment as done.Feb 23 2022, 12:52 PM

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 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.

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.

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).

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 updated this revision to Diff 414811.Mar 11 2022, 10:35 PM
abrachet retitled this revision from [Clang] Add -fcrash-diagnostics-on-error flag to [Clang] Add -femit-reproducer flag.
abrachet edited the summary of this revision. (Show Details)
Herald added a project: Restricted Project. · View Herald TranscriptMar 11 2022, 10:35 PM

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?

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")

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.

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?

(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.

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 added inline comments.Mar 14 2022, 5:47 AM
clang/tools/driver/driver.cpp
492

Make default value configurable with cmake variable?

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.

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 added a subscriber: rnk.Mar 16 2022, 1:00 PM

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.

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 updated this revision to Diff 417004.Mar 21 2022, 10:23 AM

Emit reproducer tar file unconditionally on crashes.

abrachet marked 2 inline comments as done.Mar 21 2022, 10:33 AM

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.

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?

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.

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 added a comment.Mar 21 2022, 3:02 PM

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.

bruno added a reviewer: bruno.Mar 21 2022, 3:02 PM

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 updated this revision to Diff 417696.Mar 23 2022, 11:18 AM
abrachet marked an inline comment as done.
abrachet edited the summary of this revision. (Show Details)

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 added a comment.Mar 29 2022, 1:33 PM

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 added inline comments.Mar 29 2022, 3:04 PM
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 added inline comments.Mar 30 2022, 1:28 AM
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.

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 updated this revision to Diff 431378.May 23 2022, 8:03 AM
abrachet marked 6 inline comments as done.

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 added a comment.May 24 2022, 9:02 AM

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 updated this revision to Diff 431833.May 24 2022, 4:34 PM
abrachet retitled this revision from [Clang] Add -femit-reproducer flag to [Clang] Extend -gen-reproducer flag.
abrachet edited the summary of this revision. (Show Details)

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 added inline comments.May 25 2022, 9:57 AM
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 updated this revision to Diff 432047.May 25 2022, 10:59 AM
abrachet marked 5 inline comments as done.
abrachet added inline comments.
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 accepted this revision.May 27 2022, 8:42 AM

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 :-)

This revision is now accepted and ready to land.May 27 2022, 8:42 AM
This revision was landed with ongoing or failed builds.May 27 2022, 8:50 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMay 27 2022, 8:50 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
abrachet marked an inline comment as done.May 27 2022, 8:51 AM
abrachet added inline comments.
clang/test/Driver/emit-reproducer.c
1 ↗(On Diff #432047)

Thanks :) updated it in the commit.

thakis added a subscriber: thakis.May 27 2022, 10:01 AM

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 updated this revision to Diff 433120.May 31 2022, 10:07 AM
abrachet marked an inline comment as done.

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

abrachet updated this revision to Diff 433122.May 31 2022, 10:09 AM
This revision was landed with ongoing or failed builds.May 31 2022, 10:11 AM
dyung added a subscriber: dyung.May 31 2022, 10:41 AM

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

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

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!