From 40835281112e42fd34af2aea4d3788d15b8e795a Mon Sep 17 00:00:00 2001
From: Derek Murray <mrry@google.com>
Date: Fri, 8 May 2020 16:08:20 -0700
Subject: [PATCH] [Executor] Fix segfault when using verbose logging in the
executor module.
Selective vlog-enabling for the executor module would trigger a crash in `SimplePropagatorState::DumpState()` (called when the executor executes a kernel that produces an error status). Because `SimplePropagatorState` is in a separate module, and selective vlogging might not be enabled for that module as well, the necessary state was not created, leading to a null pointer dereference.
The fix is to share the executor's vlog setting with the propagators, so that if vlogging is enabled for the executor, `SimplePropagatorState` will create the necessary debugging structures (i.e. the `SimplePropgagatorState::active_` vector).
PiperOrigin-RevId: 310646985
Change-Id: Ic4220bfdb7e0800e1ad8a5eb6d468db5886367ad
---
tensorflow/core/common_runtime/executor.cc | 2 +-
tensorflow/core/common_runtime/propagator_state.cc | 4 ++--
tensorflow/core/common_runtime/propagator_state.h | 3 ++-
tensorflow/core/common_runtime/simple_propagator_state.cc | 8 ++++----
tensorflow/core/common_runtime/simple_propagator_state.h | 5 +++--
5 files changed, 12 insertions(+), 10 deletions(-)
diff --git a/tensorflow/core/common_runtime/executor.cc b/tensorflow/core/common_runtime/executor.cc
index 74de6b28d3f..447a9e0ae77 100644
--- a/tensorflow/core/common_runtime/executor.cc
+++ b/tensorflow/core/common_runtime/executor.cc
@@ -403,7 +403,7 @@ ExecutorState<PropagatorStateType>::ExecutorState(
runner_(args.runner),
sync_on_finish_(args.sync_on_finish),
run_all_kernels_inline_(args.run_all_kernels_inline),
- propagator_(immutable_state, step_id_),
+ propagator_(immutable_state, step_id_, vlog_),
num_outstanding_ops_(0) {
if (args.user_intra_op_threadpool != nullptr) {
Device* device = immutable_state_.params().device;
diff --git a/tensorflow/core/common_runtime/propagator_state.cc b/tensorflow/core/common_runtime/propagator_state.cc
index 30529dec742..a6639b1132e 100644
--- a/tensorflow/core/common_runtime/propagator_state.cc
+++ b/tensorflow/core/common_runtime/propagator_state.cc
@@ -26,10 +26,10 @@ limitations under the License.
namespace tensorflow {
PropagatorState::PropagatorState(const ImmutableExecutorState& immutable_state,
- int64 step_id)
+ int64 step_id, bool vlog)
: immutable_state_(immutable_state),
step_id_(step_id),
- vlog_(VLOG_IS_ON(1)) {
+ vlog_(vlog || VLOG_IS_ON(1)) {
// We start the entire execution in iteration 0 of the root frame
// so let us create the root frame and the state for iteration 0.
// We assume root_frame_->frame_name.empty().
diff --git a/tensorflow/core/common_runtime/propagator_state.h b/tensorflow/core/common_runtime/propagator_state.h
index d61adeff5c4..167519ccc73 100644
--- a/tensorflow/core/common_runtime/propagator_state.h
+++ b/tensorflow/core/common_runtime/propagator_state.h
@@ -45,7 +45,8 @@ typedef gtl::InlinedVector<AllocatorAttributes, 4> AllocatorAttributeVec;
// adding them to a `TaggedNodeSeq`.
class PropagatorState {
public:
- PropagatorState(const ImmutableExecutorState& immutable_state, int64 step_id);
+ PropagatorState(const ImmutableExecutorState& immutable_state, int64 step_id,
+ bool vlog);
~PropagatorState();
private:
diff --git a/tensorflow/core/common_runtime/simple_propagator_state.cc b/tensorflow/core/common_runtime/simple_propagator_state.cc
index 48fac96dd3d..01322cc3514 100644
--- a/tensorflow/core/common_runtime/simple_propagator_state.cc
+++ b/tensorflow/core/common_runtime/simple_propagator_state.cc
@@ -23,16 +23,16 @@ limitations under the License.
namespace tensorflow {
SimplePropagatorState::SimplePropagatorState(
- const ImmutableExecutorState& immutable_state, int64 step_id)
+ const ImmutableExecutorState& immutable_state, int64 step_id, bool vlog)
: SimplePropagatorState(immutable_state, step_id,
- immutable_state.get_root_frame_info()) {}
+ immutable_state.get_root_frame_info(), vlog) {}
SimplePropagatorState::SimplePropagatorState(
const ImmutableExecutorState& immutable_state, int64 step_id,
- const ImmutableExecutorState::FrameInfo& finfo)
+ const ImmutableExecutorState::FrameInfo& finfo, bool vlog)
: immutable_state_(immutable_state),
step_id_(step_id),
- vlog_(VLOG_IS_ON(1)),
+ vlog_(vlog || VLOG_IS_ON(1)),
input_tensors_(finfo.total_inputs),
pending_(
new std::atomic<int32>[immutable_state.graph_view().num_nodes()]),
diff --git a/tensorflow/core/common_runtime/simple_propagator_state.h b/tensorflow/core/common_runtime/simple_propagator_state.h
index 1aee4c7ff2f..024341e5048 100644
--- a/tensorflow/core/common_runtime/simple_propagator_state.h
+++ b/tensorflow/core/common_runtime/simple_propagator_state.h
@@ -47,7 +47,7 @@ namespace tensorflow {
class SimplePropagatorState {
public:
SimplePropagatorState(const ImmutableExecutorState& immutable_state,
- int64 step_id);
+ int64 step_id, bool vlog);
~SimplePropagatorState();
// A `TaggedNode` corresponds to a single invocation of a node's kernel,
@@ -157,7 +157,8 @@ class SimplePropagatorState {
private:
SimplePropagatorState(const ImmutableExecutorState& immutable_state_,
int64 step_id,
- const ImmutableExecutorState::FrameInfo& finfo);
+ const ImmutableExecutorState::FrameInfo& finfo,
+ bool vlog);
const ImmutableExecutorState& immutable_state_;
const int64 step_id_;