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
[SEGFAULT]: root_node nullptr check #35882
[SEGFAULT]: root_node nullptr check #35882
Conversation
@gaurav1086 there are some sanity checks failing , can you please take a look. |
@rthadur , thanks a lot for the review. |
The sanity failures are due to https://support.sonatype.com/hc/en-us/articles/360041287334 This is a bazel failure, hoping it will be fixed soon |
@mihaimaruseac thanks for the clarification. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think you need to return early, LOG(FATAL) << msg
crashes the program.
Yes. You are right. In file: tensorflow/core/platform/default/logging.cc:237, it calls abort(), when the LOG_LEVEL is FATAL, if (severity_ == FATAL) { However, I think having this change wouldn't hurt as being good coding practice. |
Can you share why you think this is good coding practice? |
@sanjoy , If someone in the future by mistake removes the "LOG(FATAL) << msg" statement or changes the log level from FATAL to something else, it might crash the code by accessing the root_node(nullptr). So good to have these additional checks (without undermining performance). However it's obviously a choice. |
But there are cons to adding the IMO the right fix here is to add a clear comment that |
9203702
to
8f56248
Compare
Done. |
@@ -636,7 +636,7 @@ bool HloParserImpl::ParseInstructionList(HloComputation** computation, | |||
// the pool, which should not happen. | |||
if (root_node == nullptr) { | |||
LOG(FATAL) << "instruction " << root_name | |||
<< " was marked as ROOT but the parser has not seen it before"; | |||
<< " was marked as ROOT but the parser has not seen it before"; // abort() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for being so pedantic, but can you please use a full english sentence here? // abort()
reads like code that was accidentally commented out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, please check now. Thanks.
…ncing PiperOrigin-RevId: 291440430 Change-Id: Ie588fccf880c234f15140421cabf993c606e3017
Accessing root_node after it may possibly still be nullptr.