Page MenuHomePhabricator

[Clang][Driver] After default -fintegrated-cc1, fix report_fatal_error no longer generates preprocessed source + reproducer.sh
ClosedPublic

Authored by aganea on Jan 30 2020, 1:40 PM.

Details

Summary

As reported by @arichardson , this patch now makes llvm::report_fatal_error() to generate a preprocessed source + reproducer script again. Tested with/without the cmake flag CLANG_SPAWN_CC1. Tested in various configs, Debug/Release, with MSVC 19.24 or clang-cl 9.0.1, on Windows and Linux (with both gcc 9.2.1 and clang 9.0).

Added a test for #pragma clang __debug llvm_fatal_error to test for the original issue.
Added llvm::sys::Process::Exit() and replaced exit() in places where it was appropriate. This new function would call the current CrashRecoveryContext if one is running on the same thread; or call exit() otherwise.

This fixes PR44705.

Diff Detail

Event Timeline

aganea created this revision.Jan 30 2020, 1:40 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJan 30 2020, 1:40 PM
hans added a comment.Feb 3 2020, 6:54 AM

Evidently I can cut the patch in smaller pieces, let me know.

Yes, I think this would be good. There's a lot going on in this patch, and it would be good to separate the simple stuff from the tricky parts.

aganea added a comment.Feb 3 2020, 7:01 AM

Evidently I can cut the patch in smaller pieces, let me know.

Yes, I think this would be good. There's a lot going on in this patch, and it would be good to separate the simple stuff from the tricky parts.

@hans It was only so you can have the big picture. Does it make sense overall?

hans added a comment.Feb 3 2020, 7:16 AM

I think so, but I need to look closer at the CrashRecoveryContext changes, and it would help to do that separately.

I think so, but I need to look closer at the CrashRecoveryContext changes, and it would help to do that separately.

I split this into smaller pieces. Please take a look at D74063, D74070, D74076 and D74078.

arichardson added inline comments.Feb 7 2020, 5:12 AM
clang/tools/driver/cc1_main.cpp
73

I don't think it matters (yet?) but we should probably also use llvm::sys::Process::Exit() in cc1as_main.cpp?

aganea updated this revision to Diff 243241.Feb 7 2020, 12:46 PM
aganea marked 2 inline comments as done.
aganea edited the summary of this revision. (Show Details)

Simplified patch after landing previous incremental changes mentioned above.

@arichardson If you have the chance, could you please apply this, see if that fixes your use-case?

aganea updated this revision to Diff 243380.Feb 8 2020, 10:00 AM

Minor update -- added support & tested the VEH codepath as well.

clang/tools/driver/cc1_main.cpp
73

Changed cc1as_main.cpp as well.

hans accepted this revision.Feb 10 2020, 8:52 AM

Looks good to me, just a nit about one of the comments.

Thanks for making this easier to review by splitting it up!

llvm/include/llvm/Support/Process.h
205

Could the comment be phrased in terms of CrashRecoveryContext instead of integrated-cc1, which is a Clang thing?

This revision is now accepted and ready to land.Feb 10 2020, 8:52 AM
This revision was automatically updated to reflect the committed changes.
aganea marked 2 inline comments as done.Feb 11 2020, 7:26 AM

Side-note: Perhaps we would need to do the same as this patch, for calls to ::abort()?

llvm/include/llvm/Support/Process.h
205

Done in the commit.

I'm assuming this needs to be picked to 10.0?

aganea marked an inline comment as done.Feb 11 2020, 3:59 PM

I'm assuming this needs to be picked to 10.0?

Yes! Is it up to the authors to integrate their patches to the release branch? I'm seeing @hans is merging a lot of the patches.

I'm assuming this needs to be picked to 10.0?

Yes! Is it up to the authors to integrate their patches to the release branch? I'm seeing @hans is merging a lot of the patches.

I believe @hans takes care of the merging; I just wanted to make sure this hadn't been missed :) I'm gonna assume this is on his radar.

hans added a comment.Feb 12 2020, 1:26 AM

I'm assuming this needs to be picked to 10.0?

Yes! Is it up to the authors to integrate their patches to the release branch? I'm seeing @hans is merging a lot of the patches.

I believe @hans takes care of the merging; I just wanted to make sure this hadn't been missed :) I'm gonna assume this is on his radar.

Yup :) Pushed to 10.x as fd04cb43e1d83c6f18c932de94c1e341272ed160