libpressio version 0.60.0

Major Changes

+ ABI break: the pressio_io_read and related experimental functions no
  longer delete or steal the input, but more correctly move the input as
  documented.  Previously these functions deleted the input value, but
  this made it very difficult to safely wrap these functions on platforms
  that are garbage collected.
+ BREAKING: the `libpressio` higher-level python binding had several changes to
  how it handled configurations:
  1. bytes objects are now created as pressio_data objects this is to be
    more conistent across the interface and fix some faulty conversions
    in SZ-ExaFEL that should have worked
  2. mpi4py MPI Communicator objects can now be passed if built with
    mpi4py support
  3. Several changes to how type-inference works in the python bindings.
     For most users, these will just be improvements, and enable a
     correct implementation of numcodecs.Codec interface
    + if a template object is NOT passed, we no longer set entires with
      a `None` value.  This prevents segfaults
    + if a template object is passed, we now only pass the new options
      rather than all of the objects from the template.
    + if a template object is passed, we call pressio_options.set_type
      if a `None` value is passed with the type from the template.
  4. Several default arguments were added to make things default
    construable.
  5. `name` parameters are now only passed if they are truthy
  6. get_configurations was renamed get_compile_config() to be more
    consistently and understandably named
  7. get_config() now returns a dict which can be directly passed to
    `from_configuration` and things should just work
  8. Similar changes were made to PressioIO
+ A pretty substantial rewrite to how external_forkexec reads from its
  child process to use `select` to be more efficient and avoid deadlocks in
  poorly behaved (segfaulting) child processes.

Minor Changes

+ pressio_data::dimensions now returns a const reference
+ better debugging output in the external metric test cases

Bug Fixes

+ fixed a case where the constructor to pressio_data would segfault if
  passed a nullptr
+ fixed a number of segfaults when using distributed_manager (i.e.
  many_indepenent/many_dependent/etc...) when building with C++11 caused
  by constructing with an empty initializer list instead of compat::nullopt
+ fixed a number of compilation failures in the JSON bindings that
  incorrectly were implemented in C++17.
+ pinned the version of gtest to avoid build failures in anticipation of
  them adding an abseil dependency.

Author: robertu94

CMakeLists.txt

@@ -1,5 +1,5 @@
 cmake_minimum_required(VERSION 3.13 FATAL_ERROR)
-project(libpressio VERSION "0.59.0" LANGUAGES CXX C)
+project(libpressio VERSION "0.60.0" LANGUAGES CXX C)
 
 #correct was to set a default build type
 # https://blog.kitware.com/cmake-and-the-default-build-type/

include/libpressio_ext/cpp/data.h

@@ -339,7 +339,7 @@ struct pressio_data {
   /**
    * \returns a copy of the vector of dimensions
    */
-  std::vector<size_t> dimensions() const {
+  std::vector<size_t> const& dimensions() const {
     return dims;
   }
 

include/libpressio_ext/io/posix.h

@@ -24,7 +24,7 @@ extern "C" {
    *            If dims is not null, The user SHOULD assume that the memory pointed to by this pointer has been "moved" in a C++11 sense and the user MUST not rely on its contents.
    *            The implementation MAY return this pointer and reuse the underlying space if pressio_data_has_data(dims) returns true.
    * \param[in,out] in_file an file open for reading seeked to the beginning of the data to read in.
-   * \returns a pointer to a (possibly new) pressio data structure.
+   * \returns a pointer to a new pressio data structure.
    *
    */
   struct pressio_data* pressio_io_data_fread(struct pressio_data* dims, FILE* in_file); 
@@ -39,7 +39,7 @@ extern "C" {
    *            If dims is not null, The user SHOULD assume that the memory pointed to by this pointer has been "moved" in a C++11 sense and the user MUST not rely on its contents.
    *            The implementation MAY return this pointer and reuse the underlying space if pressio_data_has_data(ptr) returns true.
    * \param[in,out] in_filedes an file open for reading seeked to the beginning of the data to read in.  If dims is not null, only pressio_data_get_bytes(dims) bytes are read.
-   * \returns a pointer to a (possibly new) pressio data structure.
+   * \returns a pointer to a new pressio data structure.
    *
    */
   struct pressio_data* pressio_io_data_read(struct pressio_data* dims, int in_filedes); 
@@ -53,7 +53,7 @@ extern "C" {
    *            If dims is not null, The user SHOULD assume that the memory pointed to by this pointer has been "moved" in a C++11 sense and the user MUST not rely on its contents.
    *            The implementation MAY return this pointer and reuse the underlying space if pressio_data_has_data(dims) returns true.
    * \param[in,out] out_file an file open for reading seeked to the beginning of the data to read in.
-   * \returns a pointer to a (possibly new) pressio data structure.
+   * \returns a pointer to a new pressio data structure.
    *
    */
   struct pressio_data* pressio_io_data_path_read(struct pressio_data* dims, const char* out_file); 

src/plugins/io/csv.cc

@@ -46,14 +46,12 @@ namespace {
 struct csv_io : public libpressio_io_plugin
 {
   virtual struct pressio_data* read_impl(struct pressio_data* data) override {
-    if(data != nullptr) pressio_data_free(data);
-
     std::ifstream in{path};
     if(not in) {
       bad_path(path);
       return nullptr;
     }
-    size_t sizes[2] = {0,0};
+    std::array<size_t,2> sizes {0,0};
     std::vector<double> builder;
     for(std::string line; std::getline(in, line, line_delim.front()); sizes[0]++) {
       if(sizes[0] < skip_rows) continue;
@@ -65,12 +63,19 @@ struct csv_io : public libpressio_io_plugin
       sizes[1] = column;
     }
     sizes[0] -= skip_rows;
-    return pressio_data_new_copy(
-        pressio_double_dtype,
-        builder.data(),
-        2,
-        sizes
-        );
+    if(data && data->has_data() && std::equal(sizes.begin(), sizes.end(), data->dimensions().begin(), data->dimensions().end()) && data->dtype() == pressio_double_dtype) {
+      auto ret = pressio_data_new_empty(pressio_byte_dtype, 0, nullptr);
+      *ret = std::move(*data);
+      std::copy(builder.begin(), builder.end(), static_cast<double*>(ret->data()));
+      return ret;
+    } else {
+      return pressio_data_new_copy(
+          pressio_double_dtype,
+          builder.data(),
+          2,
+          sizes.data()
+          );
+    }
   }
 
   virtual int write_impl(struct pressio_data const* data) override{

src/plugins/io/hdf5.cc

@@ -241,7 +241,8 @@ struct hdf5_io: public libpressio_io_plugin {
         if(buffer == nullptr) {
           ret = pressio_data_new_owning(*dtype, pressio_size.size(), pressio_size.data());
         } else {
-          ret = buffer;
+          ret = pressio_data_new_empty(pressio_byte_dtype, 0, nullptr);
+          *ret = std::move(*buffer);
         }
         auto ptr = pressio_data_ptr(ret, nullptr);
         std::vector<hsize_t> memextents(std::begin(pressio_size), std::end(pressio_size));
@@ -297,7 +298,18 @@ struct hdf5_io: public libpressio_io_plugin {
       } else {
         char err_msg[1024];
         std::fill(err_msg, err_msg+1024, '\0');
-        strerror_r(errno, err_msg, 1024);
+
+        // gnulibc insists on providing their implementation
+        // of strerror_r when compiling with C++ which returns a char* which
+        // is declared warn_unused.  However, there is also a POSIX version
+        // which returns a int. In either case, we can't really recover
+        // from the error so capture the value in a variable and explicitly
+        // cast to (void) to silence the warning. BAD GNU...
+        //
+        // The other versions are either not thread safe or GNU extensions
+        auto rc = strerror_r(errno, err_msg, 1024);
+        (void)rc;
+
         set_error(1, err_msg);
         return 1;
       }

src/plugins/io/noop.cc

@@ -11,8 +11,7 @@
 #include "std_compat/memory.h"
 
 struct noop_io : public libpressio_io_plugin {
-  virtual struct pressio_data* read_impl(struct pressio_data* data) override {
-    if(data != nullptr) pressio_data_free(data);
+  virtual struct pressio_data* read_impl(struct pressio_data*) override {
     return nullptr;
   }
 

src/plugins/io/petsc.cc

@@ -91,7 +91,8 @@ struct petsc_io : public libpressio_io_plugin {
     if (data != nullptr && pressio_data_dtype(data) == pressio_dtype_from_type<PetscScalar>() &&
         pressio_data_num_dimensions(data) == 2 && pressio_data_get_dimension(data, 0) == sizes[0] &&
         pressio_data_get_dimension(data, 1) == sizes[1] && pressio_data_has_data(data)) {
-      ret = data;
+      ret = pressio_data_new_empty(pressio_byte_dtype, 0, nullptr);
+      *ret = std::move(*data);
       PetscScalar *data_ptr = static_cast<PetscScalar *>(pressio_data_ptr(data, nullptr));
       std::copy(raw_data, raw_data + (rows * columns), data_ptr);
     } else {

src/plugins/io/posix.cc

@@ -29,7 +29,6 @@ extern "C" {
         auto dims_v = dims->dimensions();
         ret = pressio_data_new_owning(dtype, dims_v.size(), dims_v.data());
       }
-      pressio_data_free(dims);
     } else {
       struct stat statbuf;
       if(fstat(in_filedes, &statbuf)) {

src/plugins/launch/external_forkexec.cc

@@ -4,6 +4,7 @@
 #include <unistd.h>
 #include <iterator>
 #include <sys/wait.h>
+#include <errno.h>
 #include "std_compat/memory.h"
 
 struct external_forkexec: public libpressio_launch_plugin {
@@ -36,6 +37,7 @@ extern_proc_results launch(std::vector<std::string> const& full_command) const o
         case 0:
           //in the child process
           {
+          close(STDIN_FILENO);
           close(stdout_pipe_fd[0]);
           close(stderr_pipe_fd[0]);
           dup2(stdout_pipe_fd[1], 1);
@@ -78,21 +80,72 @@ extern_proc_results launch(std::vector<std::string> const& full_command) const o
           char buffer[2048];
           std::ostringstream stdout_stream;
           std::ostringstream stderr_stream;
-          do {
-            //read the stdout[0]
-            int nread;
-            while((nread = read(stdout_pipe_fd[0], buffer, 2048)) > 0) {
-              stdout_stream.write(buffer, nread);
+          bool stdout_closed = false, stderr_closed = false;
+
+          while(true) {
+            int ready, nfds = 0;
+            ssize_t nread;
+            fd_set readfds, writefds, exceptfds;
+
+            FD_ZERO(&readfds);
+            FD_ZERO(&writefds);
+            FD_ZERO(&exceptfds);
+            if(not stdout_closed) {
+              FD_SET(stdout_pipe_fd[0], &readfds);
+              nfds = std::max(stdout_pipe_fd[0], nfds);
             }
-            
-            //read the stderr[0]
-            while((nread = read(stderr_pipe_fd[0], buffer, 2048)) > 0) {
-              stderr_stream.write(buffer, nread);
+            if(not stderr_closed) {
+              FD_SET(stderr_pipe_fd[0], &readfds);
+              nfds = std::max(stderr_pipe_fd[0], nfds);
             }
 
-            //wait for the child to complete
-            waitpid(child, &status, 0);
-          } while (not WIFEXITED(status));
+            if(stdout_closed && stderr_closed) {
+              break;
+            }
+
+            ready = select(nfds+1, &readfds, &writefds, &exceptfds, nullptr);
+
+            if(ready == -1 && errno == EINTR) {
+              continue;
+            }
+
+            if(!stdout_closed && FD_ISSET(stdout_pipe_fd[0], &readfds)) {
+              nread = read(stdout_pipe_fd[0], buffer, sizeof buffer);
+              if(nread > 0) {
+                stdout_stream.write(buffer, nread);
+              } else if (nread == 0) {
+                stdout_closed = true;
+              }
+            }
+
+            if(!stderr_closed && FD_ISSET(stderr_pipe_fd[0], &readfds)) {
+              nread = read(stderr_pipe_fd[0], buffer, sizeof buffer);
+              if(nread > 0) {
+                stderr_stream.write(buffer, nread);
+              } else if(nread == 0)  {
+                stderr_closed = true;
+              }
+            }
+          }
+
+          waitpid(child, &status, 0);
+          close(stdout_pipe_fd[0]);
+          close(stderr_pipe_fd[0]);
+//          do {
+//            //read the stdout[0]
+//            int nread;
+//            while((nread = read(stdout_pipe_fd[0], buffer, 2048)) > 0) {
+//              stdout_stream.write(buffer, nread);
+//            }
+//            
+//            //read the stderr[0]
+//            while((nread = read(stderr_pipe_fd[0], buffer, 2048)) > 0) {
+//              stderr_stream.write(buffer, nread);
+//            }
+//
+//            //wait for the child to complete
+//            waitpid(child, &status, 0);
+//          } while (not WIFEXITED(status));
 
           results.proc_stdout = stdout_stream.str();
           results.proc_stderr = stderr_stream.str();

src/pressio_distributed_manager.cc

@@ -7,13 +7,13 @@ compat::optional<std::vector<size_t>> distributed_build_groups(const unsigned in
   if(size <= 1) return std::vector<size_t>{0};
 
   //if the user tries to allocate more processes than exist report an error
-  if(size < (n_workers_groups + n_masters)) return {};
+  if(size < (n_workers_groups + n_masters)) return compat::nullopt;
 
   //if the user ties to allocate all processes as
-  if(n_workers_groups == size || n_masters == size) return {};
+  if(n_workers_groups == size || n_masters == size) return compat::nullopt;
 
   //root must be a valid rank
-  if(root >= size) return {};
+  if(root >= size) return compat::nullopt;
 
 
   //the zero rank is guaranteed the code below to be a master prior to the swap below