Merge pull request #36026 from tensorflow/mm-r2.0-patch

Fix segfault when attempting to convert string to float16.

tensorflow/python/BUILD

@@ -1378,6 +1378,16 @@ py_library(
     ],
 )
 
+tf_py_test(
+    name = "framework_constant_op_test",
+    size = "small",
+    srcs = ["framework/constant_op_test.py"],
+    additional_deps = [
+        ":constant_op",
+    ],
+    main = "framework/constant_op_test.py",
+)
+
 tf_py_test(
     name = "framework_registry_test",
     size = "small",

tensorflow/python/framework/constant_op_test.py

@@ -0,0 +1,61 @@
+# Copyright 2020 The TensorFlow Authors. All Rights Reserved.
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+# ==============================================================================
+"""Tests for tensorflow.python.framework.constant_op."""
+
+from __future__ import absolute_import
+from __future__ import division
+from __future__ import print_function
+
+from absl.testing import parameterized
+
+from tensorflow.python.framework import constant_op
+from tensorflow.python.framework import dtypes
+from tensorflow.python.framework import ops
+from tensorflow.python.platform import test
+
+
+class ConstantOpTest(test.TestCase, parameterized.TestCase):
+
+  @parameterized.parameters(
+      dtypes.bfloat16,
+      dtypes.complex128,
+      dtypes.complex64,
+      dtypes.double,
+      dtypes.float16,
+      dtypes.float32,
+      dtypes.float64,
+      dtypes.half,
+      dtypes.int16,
+      dtypes.int32,
+      dtypes.int64,
+      dtypes.int8,
+      dtypes.qint16,
+      dtypes.qint32,
+      dtypes.qint8,
+      dtypes.quint16,
+      dtypes.quint8,
+      dtypes.uint16,
+      dtypes.uint32,
+      dtypes.uint64,
+      dtypes.uint8,
+  )
+  def test_convert_string_to_number(self, dtype):
+    with self.assertRaises(TypeError):
+      constant_op.constant("hello", dtype)
+
+
+if __name__ == "__main__":
+  ops.enable_eager_execution()
+  test.main()

tensorflow/python/lib/core/py_seq_tensor.cc

@@ -21,6 +21,7 @@ limitations under the License.
 #include "tensorflow/core/lib/core/errors.h"
 #include "tensorflow/core/lib/core/stringpiece.h"
 #include "tensorflow/core/lib/strings/str_util.h"
+#include "tensorflow/core/platform/macros.h"
 #include "tensorflow/core/platform/types.h"
 #include "tensorflow/python/lib/core/numpy.h"
 #include "tensorflow/python/lib/core/py_util.h"
@@ -375,37 +376,43 @@ DEFINE_HELPER(ConvertInt32, int32, DT_INT32, ConvertOneInt32);
 
 // Floating-point support
 
+// Returns `true` if `out` overflows when converted from `as_double`.
+template <class T>
+static inline bool CheckForOverflow(double as_double, T* out) {
+  return (sizeof(T) < sizeof(double) && std::isinf(*out) &&
+          std::isfinite(as_double));
+}
+
+// There is no `std::isinf` that takes `Eigen::half` as argument but Eigen
+// provides `Eigen::half_impl::isinf` instead.
+template <>
+inline bool CheckForOverflow<Eigen::half>(double as_double, Eigen::half* out) {
+  return (sizeof(Eigen::half) < sizeof(double) &&
+          Eigen::half_impl::isinf(*out) && std::isfinite(as_double));
+}
+
 template <class T>
 const char* ConvertOneFloat(PyObject* v, T* out) {
   if (PyErr_Occurred()) {
     return nullptr;
   }
   if (TF_PREDICT_TRUE(PyFloat_Check(v))) {
-    double as_double = PyFloat_AsDouble(v);
-    // Handle infinity.
-    if (as_double == std::numeric_limits<double>::infinity()) {
-      *out = std::numeric_limits<T>::infinity();
-      return nullptr;
-    } else if (as_double == -1 * std::numeric_limits<double>::infinity()) {
-      *out = -1 * std::numeric_limits<T>::infinity();
-      return nullptr;
-    }
+    const double as_double = PyFloat_AS_DOUBLE(v);
+    *out = static_cast<T>(as_double);
     // Check for overflow.
-    if (as_double > std::numeric_limits<T>::max() ||
-        as_double < std::numeric_limits<T>::lowest()) {
+    if (TF_PREDICT_FALSE(CheckForOverflow<T>(as_double, out))) {
       return ErrorOutOfRangeDouble;
     }
-    *out = static_cast<T>(as_double);
     return nullptr;
   }
 #if PY_MAJOR_VERSION < 3
   if (PyInt_Check(v)) {
-    *out = PyInt_AS_LONG(v);
+    *out = static_cast<T>(PyInt_AS_LONG(v));
     return nullptr;
   }
 #endif
   if (PyLong_Check(v)) {
-    *out = PyLong_AsDouble(v);
+    *out = static_cast<T>(PyLong_AsDouble(v));
     if (PyErr_Occurred()) return ErrorOutOfRangeDouble;
     return nullptr;
   }
@@ -432,17 +439,7 @@ const char* ConvertOneFloat(PyObject* v, T* out) {
 
 DEFINE_HELPER(ConvertDouble, double, DT_DOUBLE, ConvertOneFloat<double>);
 DEFINE_HELPER(ConvertFloat, float, DT_FLOAT, ConvertOneFloat<float>);
-
-const char* ConvertOneNumpyHalf(PyObject* v, Eigen::half* out) {
-  // NOTE(nareshmodi): Is there a way to convert to C double without the
-  // intermediate Python double? This will help with ConvertOneFloat as well.
-  Safe_PyObjectPtr as_float = make_safe(PyNumber_Float(v));
-  double v_double = PyFloat_AS_DOUBLE(as_float.get());
-  *out = Eigen::half(v_double);
-
-  return nullptr;
-}
-DEFINE_HELPER(ConvertNumpyHalf, Eigen::half, DT_HALF, ConvertOneNumpyHalf);
+DEFINE_HELPER(ConvertNumpyHalf, Eigen::half, DT_HALF, ConvertOneFloat<Eigen::half>);
 
 // String support
 
@@ -537,7 +534,8 @@ Status PySeqToTensor(PyObject* obj, DataType dtype, Tensor* ret) {
       break;
 
     case DT_HALF:
-      RETURN_STRING_AS_STATUS(ConvertNumpyHalf(obj, shape, ret));
+      if (ConvertNumpyHalf(obj, shape, ret) == nullptr) return Status::OK();
+      break;
 
     case DT_INT64:
       if (ConvertInt64(obj, shape, ret) == nullptr) return Status::OK();