Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove LOG(FATAL) #3164

Merged
merged 1 commit into from
May 23, 2024
Merged

Remove LOG(FATAL) #3164

merged 1 commit into from
May 23, 2024

Conversation

jordalgo
Copy link
Contributor

Replace these with either LOG(BUG), LOG(ERROR) and exit(1) in main.cpp, or throw std::runtime_error.

This is to avoid calling abort for user errors and removes the confusion between LOG(ERROR) and LOG(FATAL).

Issue: #3163

Checklist
  • Language changes are updated in man/adoc/bpftrace.adoc
  • User-visible and non-trivial changes updated in CHANGELOG.md
  • The new behaviour is covered by tests

github-advanced-security[bot]
github-advanced-security bot found potential problems May 15, 2024
View reviewed changes
src/attached_probe.cpp Outdated
@@ -1248,18 +1249,20 @@
// FIXME when iovisor/bcc#2064 is merged, optionally pass probe_.path
ctx = bcc_usdt_new_frompid(pid, nullptr);
if (!ctx)
LOG(FATAL) << "Error initializing context for probe: " + probe_.name +
", for PID: " + std::to_string(pid);
throw std::runtime_error(

Check notice

Code scanning / CodeQL

FIXME comment Note

FIXME comment: when iovisor/bcc#2064 is merged, optionally pass probe_.path
@danobi
Copy link
Member

danobi commented May 15, 2024

Can LOG(FATAL) throw a custom exception rather than runtime_error? Having a central choke point is nice. Also it's a bit more consistent to read/write.

@ajor ajor linked an issue May 15, 2024 that may be closed by this pull request
Logging errors results in abnormal termination (abort) #3163
Closed
@jordalgo
Copy link
Contributor Author

Can LOG(FATAL) throw a custom exception rather than runtime_error? Having a central choke point is nice. Also it's a bit more consistent to read/write.

I think creating a custom exception is useful but I don't like the idea of LOG(FATAL) being responsible for throwing it as it obscures what's going on to someone reading the code. If some part of the code throws then it's immediately clear that the caller needs to handle this or let it bubble up. I also never liked the combination of LOG(FATAL) and LOG(ERROR) as it's confusing which one to use and when.

@jordalgo
Copy link
Contributor Author

Additionally, the way log.cpp works now we can't catch errors thrown in the destructor e.g.

[[noreturn]] LogStreamBug::~LogStreamBug()
{
  throw std::runtime_error(buf_.str());
  //sink_.take_input(type_, loc_, out_, internal_location() + buf_.str());
  //abort();
}

danobi
danobi reviewed May 18, 2024
View reviewed changes
src/attached_probe.cpp Outdated Show resolved Hide resolved
src/attached_probe.cpp Outdated Show resolved Hide resolved
src/attached_probe.cpp Outdated Show resolved Hide resolved
src/attached_probe.cpp Outdated Show resolved Hide resolved
src/attached_probe.cpp Outdated Show resolved Hide resolved
src/attached_probe.cpp Outdated Show resolved Hide resolved
@jordalgo jordalgo force-pushed the log-fatal branch 2 times, most recently from c33a8de to 71adb27 Compare May 18, 2024 21:34
viktormalik
viktormalik reviewed May 21, 2024
View reviewed changes
src/utils.cpp Outdated Show resolved Hide resolved
jordalgo
jordalgo commented May 21, 2024
View reviewed changes
src/ast/irbuilderbpf.cpp
@@ -253,8 +253,7 @@ llvm::Type *IRBuilderBPF::GetType(const SizedType &stype)
ty = getInt8Ty();
break;
default:
LOG(FATAL) << stype.GetSize()
<< " is not a valid type size for GetType";
LOG(BUG) << stype.GetSize() << " is not a valid type size for GetType";
Copy link
Contributor Author

@jordalgo jordalgo May 21, 2024 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These errors irbuilderbpf all seemed like internal bugs. Please correct me if I'm wrong.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that you're right.

viktormalik
viktormalik approved these changes May 22, 2024
View reviewed changes
Copy link
Contributor

@viktormalik viktormalik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

src/ast/irbuilderbpf.cpp
@@ -253,8 +253,7 @@ llvm::Type *IRBuilderBPF::GetType(const SizedType &stype)
ty = getInt8Ty();
break;
default:
LOG(FATAL) << stype.GetSize()
<< " is not a valid type size for GetType";
LOG(BUG) << stype.GetSize() << " is not a valid type size for GetType";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that you're right.

Remove LOG(FATAL)
b01d47e 
Replace these with either LOG(BUG),
LOG(ERROR) and exit(1) in main.cpp, or
`throw FatalUserException`.

This is to avoid calling `abort` for user
errors and removes the confusion between
`LOG(ERROR)` and `LOG(FATAL)`.

Issue: bpftrace#3163
@jordalgo jordalgo merged commit c316904 into bpftrace:master May 23, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danobi danobi danobi left review comments

@viktormalik viktormalik viktormalik approved these changes

@ajor ajor Awaiting requested review from ajor ajor is a code owner

@fbs fbs Awaiting requested review from fbs fbs is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Logging errors results in abnormal termination (abort)
3 participants
@jordalgo @danobi @viktormalik