Fix several printing segfaults. (#8700)

* fix: segfaults printing undefined OutputImageParam, ImageParam,
  and Pipeline objects in Python
* fix: segfault in Func::name()
* fix: avoid touching the mtime of Halide.h (drive-by)

Fixes #8699

Author: alexreinking

python_bindings/src/halide/halide_/PyImageParam.cpp

@@ -44,12 +44,13 @@ void define_image_param(py::module &m) {
 
             .def("__repr__", [](const OutputImageParam &im) -> std::string {
                 std::ostringstream o;
-                o << "<halide.OutputImageParam '" << im.name() << "'";
+                o << "<halide.OutputImageParam ";
                 if (!im.defined()) {
-                    o << " (undefined)";
+                    o << "OutputImageParam()";
                 } else {
-                    // TODO: add dimensions to this
-                    o << " type " << halide_type_to_string(im.type());
+                    o << "'" << im.name() << "'"
+                      << ", dims: " << im.dimensions()
+                      << ", type: " << halide_type_to_string(im.type());
                 }
                 o << ">";
                 return o.str();
@@ -79,12 +80,13 @@ void define_image_param(py::module &m) {
 
             .def("__repr__", [](const ImageParam &im) -> std::string {
                 std::ostringstream o;
-                o << "<halide.ImageParam '" << im.name() << "'";
+                o << "<halide.ImageParam ";
                 if (!im.defined()) {
-                    o << " (undefined)";
+                    o << "ImageParam()";
                 } else {
-                    // TODO: add dimensions to this
-                    o << " type " << halide_type_to_string(im.type());
+                    o << "'" << im.name() << "'"
+                      << ", dims: " << im.dimensions()
+                      << ", type: " << halide_type_to_string(im.type());
                 }
                 o << ">";
                 return o.str();

python_bindings/src/halide/halide_/PyPipeline.cpp

@@ -277,13 +277,19 @@ void define_pipeline(py::module &m) {
 
             .def("__repr__", [](const Pipeline &p) -> std::string {
                 std::ostringstream o;
-                o << "<halide.Pipeline [";
-                std::string comma;
-                for (auto &f : p.outputs()) {
-                    o << comma << "'" << f.name() << "'";
-                    comma = ",";
+                o << "<halide.Pipeline ";
+                if (!p.defined()) {
+                    o << "Pipeline()";
+                } else {
+                    o << "[";
+                    std::string comma;
+                    for (auto &f : p.outputs()) {
+                        o << comma << "'" << f.name() << "'";
+                        comma = ",";
+                    }
+                    o << "]";
                 }
-                o << "]>";
+                o << ">";
                 return o.str();  //
             });
 

python_bindings/test/correctness/basics.py

@@ -523,7 +523,6 @@ def test_unevaluated_funcref():
     g = hl.Func("g")
     g[x] = 2
 
-    print("------------------------")
     f = hl.Func("f")
     f[hl._] += g[hl._]
     assert list(f.realize([1])) == [2]
@@ -582,6 +581,23 @@ def test_implicit_update_by_float():
     assert f.realize([1])[0] == 0.5
 
 
+def test_print_ir():
+    im = hl.ImageParam()
+    assert str(im) == "<halide.ImageParam ImageParam()>"
+
+    im = hl.OutputImageParam()
+    assert str(im) == "<halide.OutputImageParam OutputImageParam()>"
+
+    im = hl.ImageParam(hl.UInt(16), 2, "input")
+    assert str(im) == "<halide.ImageParam 'input', dims: 2, type: uint16>"
+
+    r = hl.RDom()
+    assert str(r) == "<halide.RDom RDom()>"
+
+    p = hl.Pipeline()
+    assert str(p) == "<halide.Pipeline Pipeline()>"
+
+
 if __name__ == "__main__":
     test_compiletime_error()
     test_runtime_error()
@@ -605,3 +621,4 @@ def test_implicit_update_by_float():
     test_unevaluated_funcref()
     test_implicit_update_by_int()
     test_implicit_update_by_float()
+    test_print_ir()

src/CMakeLists.txt

@@ -469,7 +469,9 @@ set(LICENSE_PATH "${Halide_SOURCE_DIR}/LICENSE.txt")
 set(headers "$<TARGET_PROPERTY:Halide,HEADER_SET_private_headers>")
 add_custom_command(OUTPUT "${HALIDE_H}"
                    COMMAND ${CMAKE_COMMAND} -E make_directory "$<SHELL_PATH:${Halide_BINARY_DIR}/include>"
-                   COMMAND build_halide_h "$<SHELL_PATH:${LICENSE_PATH}>" "${headers}" > "$<SHELL_PATH:${HALIDE_H}>"
+                   COMMAND build_halide_h "$<SHELL_PATH:${LICENSE_PATH}>" "${headers}" > "$<SHELL_PATH:${HALIDE_H}>.tmp"
+                   COMMAND ${CMAKE_COMMAND} -E copy_if_different "$<SHELL_PATH:${HALIDE_H}>.tmp" "$<SHELL_PATH:${HALIDE_H}>"
+                   COMMAND ${CMAKE_COMMAND} -E rm "$<SHELL_PATH:${HALIDE_H}>.tmp"
                    DEPENDS build_halide_h "${LICENSE_PATH}" "${headers}"
                    WORKING_DIRECTORY "${CMAKE_CURRENT_LIST_DIR}"
                    COMMAND_EXPAND_LISTS

src/Func.cpp

@@ -85,6 +85,8 @@ Func::Func(const Expr &e)
 
 Func::Func(Function f)
     : func(std::move(f)) {
+    internal_assert(func.get_contents().defined())
+        << "Can't construct Func from undefined Function";
 }
 
 const string &Func::name() const {

src/Func.h

@@ -756,7 +756,7 @@ class Func {
      * not contain free variables). */
     explicit Func(const Expr &e);
 
-    /** Construct a new Func to wrap an existing, already-define
+    /** Construct a new Func to wrap an existing, already-defined
      * Function object. */
     explicit Func(Internal::Function f);
 

test/error/CMakeLists.txt

@@ -23,6 +23,7 @@ tests(GROUPS error
       bad_ring_buffer.cpp
       bad_extern_split.cpp
       bad_fold.cpp
+      bad_func_object.cpp
       bad_host_alignment.cpp
       bad_hoist_storage.cpp
       bad_partition_always.cpp

test/error/bad_func_object.cpp

@@ -0,0 +1,23 @@
+#include "Halide.h"
+using namespace Halide;
+
+int main() {
+#ifdef HALIDE_WITH_EXCEPTIONS
+    try {
+#endif
+        const Internal::Function func{};
+        const Func f{func};  // internal_assert
+
+        std::cout << f.name() << "\n";  // segfaults without above assert
+#ifdef HALIDE_WITH_EXCEPTIONS
+    } catch (const InternalError &e) {
+        // The harness rejects internal errors as they should typically not be
+        // _expected_. However, we are directly testing a constructor invariant
+        // check here, so an internal error is both expected and appropriate.
+        throw std::runtime_error(e.what());
+    }
+#endif
+
+    printf("Success!\n");
+    return 0;
+}