This is an archive of the discontinued LLVM Phabricator instance.

Make report_fatal_error respect its GenCrashDiag argument so it doesn't generate a backtrace
ClosedPublic

Authored by nlopes on May 27 2022, 8:14 AM.

Details

Summary

There are a few places where we use report_fatal_error when the input is broken. Currently, this function always crashes LLVM with an abort signal, which then triggers the backtrace printing code.
I think this is excessive, as wrong input shouldn't give a link to LLVM's github issue URL and tell users to file a bug report. We shouldn't print a stack trace either.

This patch changes report_fatal_error so it uses exit() rather than abort() when its argument GenCrashDiag=true.

As a side-effect, and the motivation for my change, this fixes 5 Alive2 reports, as our test driver doesn't propagate signal data.

Diff Detail

Event Timeline

nlopes created this revision.May 27 2022, 8:14 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 27 2022, 8:14 AM
nlopes requested review of this revision.May 27 2022, 8:14 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 27 2022, 8:14 AM
nikic accepted this revision.May 27 2022, 8:42 AM

Makes sense to me.

You probably also want to update https://github.com/llvm/llvm-project/blob/af430944b3ba8ca55c4fd6b73f53c198c469ffee/llvm/lib/Transforms/Scalar/LoopUnrollPass.cpp#L911.

This patch changes report_fatal_error so it uses exit() rather than abort() when its argument GenCrashDiag=true.

This should probably read GenCrashDiag=false.

This revision is now accepted and ready to land.May 27 2022, 8:42 AM

This patch changes report_fatal_error so it uses exit() rather than abort() when its argument GenCrashDiag=true.

GenCrashDiag=false?

MaskRay requested changes to this revision.May 29 2022, 10:39 AM
MaskRay added inline comments.
llvm/lib/Support/ErrorHandling.cpp
125

https://www.opengroup.org/austin/docs/austin_1110.pdf

"Calling exit() with a value greater than 255 or less than 0 is something that only programs
which are specifically designed to have their exit status obtained by waitid() should do
(since it does not truncate the exit status to 8 bits). ``Pure ISO C’’ programs that call
exit(EXIT_FAILURE) do not meet this design criterion."

This revision now requires changes to proceed.May 29 2022, 10:39 AM
nlopes added inline comments.May 29 2022, 10:43 AM
llvm/lib/Support/ErrorHandling.cpp
125

Man page disagrees:
"The exit() function causes normal process termination and the value of status & 0377 is returned to the parent see wait(2))."

MaskRay added inline comments.May 29 2022, 10:45 AM
llvm/lib/Support/ErrorHandling.cpp
125

A value between 1 and 125 can be used.

1 and 2 are often used for other purposes.

Perhaps use a value larger than EX__MAX in sysexits.h

MaskRay added inline comments.May 29 2022, 10:51 AM
llvm/lib/Support/ErrorHandling.cpp
125

The Linux manpage is incorrect. With waitid you may access more than the least significant 8 bites, but that's not usage in practice.

bash has this https://www.gnu.org/software/bash/manual/html_node/Exit-Status.html and zsh is similar.

nlopes updated this revision to Diff 432803.May 29 2022, 1:21 PM
MaskRay added inline comments.May 29 2022, 8:52 PM
llvm/lib/Support/ErrorHandling.cpp
125

1 and 2 are often used for other purposes. So better to use another.

nlopes added inline comments.May 30 2022, 12:07 AM
llvm/lib/Support/ErrorHandling.cpp
125

I will not bikeshed over this. Please suggest a value you like, otherwise I think 1 is just fine. Anything non-zero works.

MaskRay added inline comments.May 30 2022, 10:16 AM
llvm/lib/Support/ErrorHandling.cpp
125

1 and 2 are often used for other purposes.

Perhaps use a value larger than EX__MAX in sysexits.h

Please note that in a previous comment I mentioned this. Then I said it should be 126. A value in this range should be fine.

nlopes added inline comments.May 30 2022, 11:14 AM
llvm/lib/Support/ErrorHandling.cpp
125

1 and 2 are often used for other purposes.

Perhaps use a value larger than EX__MAX in sysexits.h

Please note that in a previous comment I mentioned this. Then I said it should be 126. A value in this range should be fine.

OK, thank you, will do.

This revision was not accepted when it landed; it landed in state Needs Review.May 30 2022, 11:19 AM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
thakis added a subscriber: thakis.May 30 2022, 11:31 AM

This seems to break all kinds of tests, see http://45.33.8.238/linux/77205/step_12.txt

Did you run tests locally before landing?

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

This seems to break all kinds of tests, see http://45.33.8.238/linux/77205/step_12.txt

Did you run tests locally before landing?

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

I'm on it, sorry for the breakage.
It seems not doesn't like exit value of 126,

MaskRay added inline comments.May 31 2022, 7:37 PM
llvm/lib/Support/ErrorHandling.cpp
125

Sorry, I made a typo as well. I should have said "between 1 and 125", as my previous comment said.

A value between 1 and 125 can be used.

MaskRay added inline comments.May 31 2022, 7:40 PM
llvm/lib/Support/ErrorHandling.cpp
125

Correction: the shell requires 1 to 125.
Many utilities use 1 and 2 for other purposes, so better to avoid them.
sysexits.h uses values up to 78.

A value satisfying all the requirement is good.

nlopes added inline comments.Jun 1 2022, 2:04 AM
llvm/lib/Support/ErrorHandling.cpp
125

Sorry, I made a typo as well. I should have said "between 1 and 125", as my previous comment said.

No worries. I had to quickly revert to 1 to fix build bots.
Feel free to change it; I really don't have any preference at all. (plus I don't have much hw resources to run LLVM tests frequently). Thank you!