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.

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

Restore "Check failure stack trace" message on LOG(FATAL) #1110

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

AustinSchuh
Copy link

#1074 refactored some of the code to enable the failure function to throw. This made it so when the LogMessageFatal class was used, the error ended up being printed differently.

Before:
LOG_AT_LEVEL(google::LogSeverity::FATAL) << "Crash: Hello world!"; ->

F20240621 18:12:44.710584 139620827212672 log_demo.cc:16] Crash: Hello world!
*** Check failure stack trace: ***
@ 0x559e2704711a
@ 0x7efc01fac24a
@ 0x7efc01fac305
@ 0x559e27046dd5
Aborted

LOG(FATAL) << "Crash: Hello world!"; ->

F20240621 18:13:05.760556 140518290856832 log_demo.cc:16] Crash: Hello world!
@ 0x55cdc2475130
@ 0x7fccf6fb324a
@ 0x7fccf6fb3305
@ 0x55cdc2474df5
Aborted

With this patch, they both produce the same output.

@codecov-commenter
Copy link

codecov-commenter commented Jun 22, 2024

Codecov Report

Attention: Patch coverage is 50.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 63.65%. Comparing base (45f99f5) to head (039298a).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1110      +/-   ##
==========================================
+ Coverage   63.57%   63.65%   +0.07%     
==========================================
  Files          20       20              
  Lines        2578     2578              
  Branches      894      907      +13     
==========================================
+ Hits         1639     1641       +2     
  Misses        671      671              
+ Partials      268      266       -2     
Files Coverage Δ
src/logging.cc 70.28% <50.00%> (+0.15%) ⬆️

Copy link
Collaborator

@sergiud sergiud left a comment

Choose a reason for hiding this comment

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

Thank you for the PR.

Unfortunately, the CI builds are now failing. Please inspect the runner output to identify the reason.

// We really want [[noreturn]] on the destructor so the compiler can use it.
// We really just want to reuse the parent class's destructor since it has all
// the right logic in it.
LogMessage::~LogMessage();
Copy link
Collaborator

Choose a reason for hiding this comment

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

The destructors in a class hierarchy are generally called in the reverse order of construction. Explicitly invoking the base class destructor means that the base class destructor will be called a second time. However, at that point the object no longer exists. I suspect this is UB territory.

src/logging_unittest.cc Outdated Show resolved Hide resolved
google#1074 refactored some of the code to
enable the failure function to throw.  This made it so when
the LogMessageFatal class was used, the error ended up being printed
differently.

Before:
  LOG_AT_LEVEL(google::LogSeverity::FATAL) << "Crash: Hello world!"; ->

  F20240621 18:12:44.710584 139620827212672 log_demo.cc:16] Crash: Hello world!
  *** Check failure stack trace: ***
      @     0x559e2704711a
      @     0x7efc01fac24a
      @     0x7efc01fac305
      @     0x559e27046dd5
  Aborted

  LOG(FATAL) << "Crash: Hello world!"; ->

  F20240621 18:13:05.760556 140518290856832 log_demo.cc:16] Crash: Hello world!
      @     0x55cdc2475130
      @     0x7fccf6fb324a
      @     0x7fccf6fb3305
      @     0x55cdc2474df5
  Aborted

With this patch, they both produce the same output.

Signed-off-by: Austin Schuh <austin.linux@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants