HomePhabricator

Reland "[Support] make report_fatal_error `abort` instead of `exit`"

Authored by ychen on Feb 10 2020, 6:33 PM.

Description

Reland "[Support] make report_fatal_error abort instead of exit"

Summary:
Reland D67847 after D73742 is committed. Replace sys::Process::Exit(1)
with abort in report_fatal_error.

After this patch, for tools turning on CrashRecoveryContext,
crash handler installed by CrashRecoveryContext is called unless
they installed a non-returning handler using llvm::install_fatal_error_handler
like cc1_main currently does.

Reviewers: rnk, MaskRay, aganea, hans, espindola, jhenderson

Subscribers: jholewinski, qcolombet, dschuff, jyknight, emaste, sdardis, nemanjai, jvesely, nhaehnle, sbc100, jgravelle-google, hiraditya, aheejin, kbarton, fedor.sergeev, asb, rbar, johnrusso, simoncook, sabuasal, niosHD, jrtc27, zzheng, edward-jones, atanasyan, steven_wu, rogfer01, MartinMosbeck, brucehoult, the_o, dexonsmith, PkmX, rupprecht, jocewei, jsji, Jim, dmgreen, lenary, s.egerton, pzheng, sameer.abuasal, apazos, luismarques, kerbowa, cfe-commits, llvm-commits

Tags: #clang, #llvm

Differential Revision: https://reviews.llvm.org/D74456

Event Timeline

arsenm added a subscriber: arsenm.Feb 14 2020, 8:19 PM

I have a problem with this change. IMO the entire reason to use report_fatal_error is to not crash, or do anything that looks like a crash. I should never see a backtrace after calling report_fatal_error, and it should be a clean exit. For example I should not see a backtrace on a legalization failure with -global-isel-abort=1

ychen added a comment.EditedFeb 15 2020, 10:30 AM

Thanks for the comments, @arsenm. I expected this would cause concerns for practical reasons. I do think it is heading the right direction though.

I have a problem with this change. IMO the entire reason to use report_fatal_error is to not crash, or do anything that looks like a crash.

6c2d233e7a8993d90fd97703ed8331f2d6e80671 introduced this method. You could tell from the deleted code that it was trying to replace abort() call. MHO is that report_fatal_error should crash (or could since crash is its default behavior, it could be overriden) as its name suggests. It is for shortcutting error propagation where it is hard to assume the compilation process could exit cleanly. So exit may not be a good choice.

I should never see a backtrace after calling report_fatal_error, and it should be a clean exit.

This part is customized by tools using install_fatal_error_handler (clang does). llc uses InitLLVM which asks for a backtrace.

For example I should not see a backtrace on a legalization failure with -global-isel-abort=1

I think you mean in this case, the crash better not to trigger a backtrace? The option name suggests it should abort.

Thanks for the comments, @arsenm. I expected this would cause concerns for practical reasons. I do think it is heading the right direction though.

I have a problem with this change. IMO the entire reason to use report_fatal_error is to not crash, or do anything that looks like a crash.

6c2d233e7a8993d90fd97703ed8331f2d6e80671 introduced this method. You could tell from the deleted code that it was trying to replace abort() call. MHO is that report_fatal_error should crash (or could since crash is its default behavior, it could be overriden) as its name suggests. It is for shortcutting error propagation where it is hard to assume the compilation process could exit cleanly. So exit may not be a good choice.

I should never see a backtrace after calling report_fatal_error, and it should be a clean exit.

This part is customized by tools using install_fatal_error_handler (clang does). llc uses InitLLVM which asks for a backtrace.

The default behavior is wrong then. All llvm tools should cleanly exit on report_fatal_error

For example I should not see a backtrace on a legalization failure with -global-isel-abort=1

I think you mean in this case, the crash better not to trigger a backtrace? The option name suggests it should abort.

No, this is a clean error

Thanks for the comments, @arsenm. I expected this would cause concerns for practical reasons. I do think it is heading the right direction though.

I have a problem with this change. IMO the entire reason to use report_fatal_error is to not crash, or do anything that looks like a crash.

6c2d233e7a8993d90fd97703ed8331f2d6e80671 introduced this method. You could tell from the deleted code that it was trying to replace abort() call. MHO is that report_fatal_error should crash (or could since crash is its default behavior, it could be overriden) as its name suggests. It is for shortcutting error propagation where it is hard to assume the compilation process could exit cleanly. So exit may not be a good choice.

I should never see a backtrace after calling report_fatal_error, and it should be a clean exit.

This part is customized by tools using install_fatal_error_handler (clang does). llc uses InitLLVM which asks for a backtrace.

For example I should not see a backtrace on a legalization failure with -global-isel-abort=1

I think you mean in this case, the crash better not to trigger a backtrace? The option name suggests it should abort.

I think there are 3 levels of error handling:

  1. User facing errors that can occur due to user error, that should use the LLVMContext error reporting
  2. Clean errors that generally should generally not be end user facing, but also should be handled gracefully and in all builds. Most cases that can only be produced by writing unrealistic MIR tests should use this form of error since it's not really worth the effort to produce a user facing error
  3. Asserts for situations that should never happen. This is what llvm_unreachable/assert, and can be deleted in release builds.

This patch blobs uses 2 and 3 together. report_fatal_error is not equivalent to an assert.

ping, this is still broken

Thanks for the comments, @arsenm. I expected this would cause concerns for practical reasons. I do think it is heading the right direction though.

I have a problem with this change. IMO the entire reason to use report_fatal_error is to not crash, or do anything that looks like a crash.

6c2d233e7a8993d90fd97703ed8331f2d6e80671 introduced this method. You could tell from the deleted code that it was trying to replace abort() call. MHO is that report_fatal_error should crash (or could since crash is its default behavior, it could be overriden) as its name suggests. It is for shortcutting error propagation where it is hard to assume the compilation process could exit cleanly. So exit may not be a good choice.

I should never see a backtrace after calling report_fatal_error, and it should be a clean exit.

This part is customized by tools using install_fatal_error_handler (clang does). llc uses InitLLVM which asks for a backtrace.

For example I should not see a backtrace on a legalization failure with -global-isel-abort=1

I think you mean in this case, the crash better not to trigger a backtrace? The option name suggests it should abort.

I think there are 3 levels of error handling:

  1. User facing errors that can occur due to user error, that should use the LLVMContext error reporting
  2. Clean errors that generally should generally not be end user facing, but also should be handled gracefully and in all builds. Most cases that can only be produced by writing unrealistic MIR tests should use this form of error since it's not really worth the effort to produce a user facing error
  3. Asserts for situations that should never happen. This is what llvm_unreachable/assert, and can be deleted in release builds.

This patch blobs uses 2 and 3 together. report_fatal_error is not equivalent to an assert.

FYI: LLVMContext error reporting is set up when assembling but otherwise is not setup by clang or llc (i.e. SrcMgr in MCContext is nullptr). This means that use of MCContext::reportError just falls back to report_fatal_error which now asserts.