Merge pull request #20385 from mppf/move-override-ld-printchplenv

Move override-ld to printchplenv

This PR addresses a future work item left by PR #18880. In particular,
historically we have saved the linker to use in a file `override-ld` and
then the LLVM backend reads this file and uses the selected linker in
order to link the binary. We recently observed some problems with this
strategy in the context of Homebrew. The Homebrew Chapel package had this
`override-ld` file referring to clang in a particular location; but the
Homebrew clang package was upgraded and clang was no longer available at
that location. (See also
Homebrew/homebrew-core#107405 ).

So, this PR moves the `override-ld` logic to something supported by the
chplenv Python scripts. One wrinkle with that is that the Makefile logic
was using `GASNET_LD_REQUIRES_MPI` but that variable is not stored in the
Gasnet .pc file that is used by the chplenv scripts in LLVM compilation.
So, this PR uses the approximation of determining
`GASNET_LD_REQUIRES_MPI` by searching for `mpi` in the `GASNET_LD`
command name.

Additionally, while debugging the issue, I noticed some odd things about
the error handling within `executeAndWait`, so this PR cleans that
function up a bit as well. It also removes some arguments to that
function that aren't really necessary. It adds a C++ dyno test of this
function to make sure the return value of the function is 0 when the
command runs and something other than 0 when the command does not exist.

The chpl-env-gen.h needed some adjustment to work with this change so I
tidied it up to stop filtering out environment variables that often have
characters that can't be in a `#define` and instead to use a more
comprehensive pattern when replacing such characters with `_`.

Future Work:
 * include https://bitbucket.org/berkeleylab/gasnet/pull-requests/554 in
   our bundled GASNet to actually use `GASNET_LD_REQUIRES_MPI` in the
   logic to match the previous behavior.

Reviewed by @arezaii 

- [x] full local testing
- [x] XC gasnet+aries testing
- [x] XC ugni testing
- [x] XC mpi testing
- [x] Mac OS X testing
- [x] testing Hello world on a real ibv system

compiler/dyno/include/chpl/util/subprocess.h

@@ -29,22 +29,24 @@ namespace chpl {
 
 
 /**
- * Launch a subprocess
- *
- * commandVec first element is the command to run, the rest are the arguments.
- * description a string representation of the command to run
- * childPidOut output parameter for the child's pid
- * ignoreStatus if true, don't abort if the child exits with a non-zero
- * quiet  if true, won't print systemCommands
- * printSystemCommands if true, print the command to be run
- * returns int as program exit status if fork successful, -1 otherwise
+ Launch a subprocess
+
+ commandVec
+   first element is the command to run, the rest are the arguments.
+
+ description
+   a string representation of the command to run
+
+ printSystemCommands if true, print the command to be run
+
+ returns
+   -1 if there was an error in fork, waitpid, or the child process was killed
+   255 if the exec call failed
+   the exit code from the subprocess (0-255) otherwise
  */
-int executeAndWait(const std::vector<std::string> commandVec,
-             std::string description,
-             pid_t&      childPidOut,
-             bool        ignoreStatus = false,
-             bool        quiet = false,
-             bool        printSystemCommands = false);
+int executeAndWait(const std::vector<std::string>& commandVec,
+                   const std::string& description,
+                   bool printSystemCommands = false);
 
 #endif
 

compiler/dyno/lib/util/subprocess.cpp

@@ -28,61 +28,80 @@
 #include <sys/wait.h>
 #include <unistd.h>
 
-
 namespace chpl {
 
 
 // TODO: body of this function should call llvm::sys::ExecuteAndWait instead
 // see: https://llvm.org/doxygen/namespacellvm_1_1sys.html#a67688ad4697f1d516a7c71e41d78c5ba
-int executeAndWait(const std::vector<std::string> commandVec,
-             std::string description,
-             pid_t&      childPidOut,
-             bool        ignoreStatus,
-             bool        quiet,
-             bool        printSystemCommands) {
+int executeAndWait(const std::vector<std::string>& commandVec,
+                   const std::string& description,
+                   bool printSystemCommands) {
 
-  // if an empty command passed, do nothing
-  if (commandVec.empty()) {
+  // if an empty command or comment is passed, do nothing
+  if (commandVec.empty() || commandVec[0].empty() || commandVec[0][0] == '#') {
     return 0;
   }
 
-  // Join the elements of commandVec into a single space separated string
-  std::string commandStr = commandVec.front();
-  for (unsigned int i = 1; i < commandVec.size(); i++) {
-    commandStr += " " + commandVec.at(i);
-  }
-
   int status = -1;
   std::vector<const char*> execArgs;
-  for (unsigned int i = 0; i < commandVec.size(); ++i) {
-    execArgs.push_back(commandVec[i].c_str());
+  for (const auto& elt : commandVec) {
+    execArgs.push_back(elt.c_str());
   }
   execArgs.push_back(NULL);
   execArgs.shrink_to_fit();
 
-  if (printSystemCommands && !quiet) {
+  if (printSystemCommands) {
+    // Join the elements of commandVec into a single space separated string
+    std::string commandStr = commandVec.front();
+    for (unsigned int i = 1; i < commandVec.size(); i++) {
+      commandStr += " " + commandVec.at(i);
+    }
+
     printf("\n# %s\n", description.c_str());
     printf("%s\n", commandStr.c_str());
     fflush(stdout);
     fflush(stderr);
   }
 
-  // Treat a '#' at the start of a line as a comment
-  if (commandStr.c_str()[0] == '#')
-    return 0;
-
-  childPidOut = fork();
+  pid_t childPid = fork();
 
-  if (childPidOut == 0) {
+  if (childPid == 0) {
     // in child process
     execvp(execArgs[0], const_cast<char* const *> (execArgs.data()));
-  } else if (childPidOut > 0 ) {
+    // if execvp returned, there was an error
+    if (printSystemCommands) {
+      printf("error in exec: %s\n", strerror(errno));
+    }
+    // indicate problem with exec by exiting the child process
+    exit(-1);
+  } else if (childPid > 0 ) {
     // in parent process
-    waitpid(childPidOut, &status, 0);
+    int w = waitpid(childPid, &status, 0);
+    if (w == -1) {
+      if (printSystemCommands) {
+        printf("error in waitpid: %s\n", strerror(errno));
+      }
+      // indicate problem with waiting
+      return -1;
+    }
+    if (!WIFEXITED(status)) {
+      // indicate program did not exit normally
+      if (printSystemCommands) {
+        printf("child process did not exit normally\n");
+      }
+      // indicate program did not exit normally
+      return -1;
+    }
     // filter status down to key bits
-    status = WEXITSTATUS(status);
+    int code = WEXITSTATUS(status);
+    if (printSystemCommands && code != 0) {
+      printf("child process exited with code %i\n", code);
+    }
+    return code;
   }
-  return status;
+
+  // this can be reached if the 'fork' failed
+  return -1;
 }
 
 

compiler/dyno/test/util/CMakeLists.txt

@@ -15,7 +15,8 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-comp_unit_test(testQuoteStrings)
-comp_unit_test(testQueryTimingAndTrace)
 comp_unit_test(testIteratorAdapters)
 comp_unit_test(testLLVMsupport)
+comp_unit_test(testQueryTimingAndTrace)
+comp_unit_test(testQuoteStrings)
+comp_unit_test(testSubprocess)

compiler/dyno/test/util/testSubprocess.cpp

@@ -0,0 +1,98 @@
+/*
+ * Copyright 2021-2022 Hewlett Packard Enterprise Development LP
+ * Other additional copyright holders may be indicated within.
+ *
+ * The entirety of this work is 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.
+ */
+
+#include "chpl/util/subprocess.h"
+
+#include <cassert>
+
+// always check assertions in this test
+#ifdef NDEBUG
+#undef NDEBUG
+#endif
+
+using namespace chpl;
+
+// check that trying to run a nonexistant command returns an error
+static void checkNonexistent() {
+  int rc;
+  std::vector<std::string> cmd;
+
+  cmd.clear();
+  cmd.push_back("./nonexistent-command");
+  rc = executeAndWait(cmd, "nonexistent-command description", true);
+  assert(rc != 0);
+
+  cmd.clear();
+  cmd.push_back("./nonexistent-command");
+  cmd.push_back("arg");
+  rc = executeAndWait(cmd, "nonexistent-command description", true);
+  assert(rc != 0);
+
+  cmd.clear();
+  cmd.push_back("/this/path/does/not/exist");
+  rc = executeAndWait(cmd, "path does not exist description", true);
+  assert(rc != 0);
+
+  cmd.clear();
+  cmd.push_back("/this/path/does/not/exist");
+  cmd.push_back("arg");
+  rc = executeAndWait(cmd, "path does not exist description", true);
+  assert(rc != 0);
+}
+
+// check that we can run /usr/bin/env and get 0 return code from it
+static void checkEnv() {
+  int rc;
+  std::vector<std::string> cmd;
+
+  cmd.clear();
+  cmd.push_back("/usr/bin/env");
+  rc = executeAndWait(cmd, "testing that env can be run", true);
+  assert(rc == 0);
+}
+
+// check that running an empty command or a comment returns 0
+static void checkEmptyOrComment() {
+  int rc;
+  std::vector<std::string> cmd;
+
+  cmd.clear();
+  rc = executeAndWait(cmd, "empty command 1", true);
+  assert(rc == 0);
+
+  cmd.clear();
+  cmd.push_back("");
+  rc = executeAndWait(cmd, "empty command 2", true);
+  assert(rc == 0);
+
+  cmd.clear();
+  cmd.push_back("# this is a comment");
+  rc = executeAndWait(cmd, "comment command", true);
+  assert(rc == 0);
+}
+
+
+int main(int argc, char** argv) {
+
+  checkNonexistent();
+  checkEnv();
+  checkEmptyOrComment();
+
+  return 0;
+}

compiler/include/driver.h

@@ -140,6 +140,7 @@ extern const char* CHPL_LLVM_CLANG_CXX;
 
 extern const char* CHPL_TARGET_BUNDLED_COMPILE_ARGS;
 extern const char* CHPL_TARGET_SYSTEM_COMPILE_ARGS;
+extern const char* CHPL_TARGET_LD;
 extern const char* CHPL_TARGET_BUNDLED_LINK_ARGS;
 extern const char* CHPL_TARGET_SYSTEM_LINK_ARGS;
 

compiler/llvm/clangUtil.cpp

@@ -4353,29 +4353,17 @@ void makeBinaryLLVM(void) {
 
   std::string options = "";
 
-
   std::string maino(CHPL_RUNTIME_LIB);
   maino += "/";
   maino += CHPL_RUNTIME_SUBDIR;
   maino += "/main.o";
 
-  // TODO: move this logic to printchplenv
-  std::string runtime_ld_override(CHPL_RUNTIME_LIB);
-  runtime_ld_override += "/";
-  runtime_ld_override += CHPL_RUNTIME_SUBDIR;
-  runtime_ld_override += "/override-ld";
-
-  std::vector<std::string> ldOverride;
-  readArgsFromFile(runtime_ld_override, ldOverride);
-  // Substitute $CHPL_HOME $CHPL_RUNTIME_LIB etc
-  expandInstallationPaths(ldOverride);
-
   std::string clangCC = clangInfo->clangCC;
   std::string clangCXX = clangInfo->clangCXX;
   std::string useLinkCXX = clangCXX;
 
-  if (ldOverride.size() > 0)
-    useLinkCXX = ldOverride[0];
+  if (CHPL_TARGET_LD != nullptr && CHPL_TARGET_LD[0] != '\0')
+    useLinkCXX = CHPL_TARGET_LD;
 
   // start with arguments from CHPL_LLVM_CLANG_C unless
   // using a non-clang compiler to link

compiler/main/driver.cpp

@@ -109,6 +109,7 @@ const char* CHPL_LLVM_CLANG_CXX = NULL;
 
 const char* CHPL_TARGET_BUNDLED_COMPILE_ARGS = NULL;
 const char* CHPL_TARGET_SYSTEM_COMPILE_ARGS = NULL;
+const char* CHPL_TARGET_LD = NULL;
 const char* CHPL_TARGET_BUNDLED_LINK_ARGS = NULL;
 const char* CHPL_TARGET_SYSTEM_LINK_ARGS = NULL;
 
@@ -1455,6 +1456,7 @@ static void setChapelEnvs() {
 
   CHPL_TARGET_BUNDLED_COMPILE_ARGS = envMap["CHPL_TARGET_BUNDLED_COMPILE_ARGS"];
   CHPL_TARGET_SYSTEM_COMPILE_ARGS = envMap["CHPL_TARGET_SYSTEM_COMPILE_ARGS"];
+  CHPL_TARGET_LD = envMap["CHPL_TARGET_LD"];
   CHPL_TARGET_BUNDLED_LINK_ARGS = envMap["CHPL_TARGET_BUNDLED_LINK_ARGS"];
   CHPL_TARGET_SYSTEM_LINK_ARGS = envMap["CHPL_TARGET_SYSTEM_LINK_ARGS"];
 

compiler/util/mysystem.cpp

@@ -37,9 +37,9 @@
 bool printSystemCommands = false;
 
 int myshell(const char* command,
-             const char* description,
-             bool        ignoreStatus,
-             bool        quiet) {
+            const char* description,
+            bool        ignoreStatus,
+            bool        quiet) {
 
   int status = 0;
 
@@ -57,7 +57,7 @@ int myshell(const char* command,
   status = system(command);
 
   if (status == -1) {
-    USR_FATAL("system() fork failed: %s", strerror(errno));
+    USR_FATAL("%s: system() fork failed: %s", description, strerror(errno));
 
   } else if (status != 0 && ignoreStatus == false) {
     USR_FATAL("%s", description);
@@ -81,17 +81,16 @@ int mysystem(const std::vector<std::string> commandVec,
              const char* description,
              bool        ignoreStatus,
              bool        quiet) {
-  pid_t childPid = 0;
-  int status = chpl::executeAndWait(commandVec, std::string(description), childPid,
-                              ignoreStatus, quiet, printSystemCommands);
-  if (childPid == 0) {
-    // if there was an error and we shouldn't ignore them, then exit
-    if (status != 0 && !ignoreStatus) {
-      USR_FATAL("%s", description);
-    }
-  // handle the case when the child couldn't be forked
-  } else if (childPid == -1) {
-    USR_FATAL("fork() failed: %s", strerror(errno));
+  int status = chpl::executeAndWait(commandVec, std::string(description),
+                                    printSystemCommands && !quiet);
+
+  // if there was an error fork/waiting
+  if (status == -1) {
+    USR_FATAL("%s", description);
+
+  } else if (status != 0 && ignoreStatus == false) {
+    USR_FATAL("%s", description);
   }
+
   return status;
 }

runtime/Makefile

@@ -58,14 +58,12 @@ $(CHPL_ENV_HEADER): $(CHPL_MAKE_HOME)/util/printchplenv $(CHPL_MAKE_HOME)/util/c
 	@echo "/* THIS FILE IS AUTOMATICALLY GENERATED */" >> $(CHPL_ENV_HEADER)
 	@echo "#ifndef _CHPL_ENV_GEN_H_" >> $(CHPL_ENV_HEADER)
 	@echo "#define _CHPL_ENV_GEN_H_" >> $(CHPL_ENV_HEADER)
+	# save output of printchplenv into #defines but munge away
+	# characters that can't be in #defines
 	@$(CHPL_MAKE_HOME)/util/printchplenv --runtime --no-tidy --anonymize --simple | \
-          grep -v CHPL_HOST_CC | \
-          grep -v CHPL_HOST_CXX | \
-          grep -v CHPL_TARGET_CC | \
-          grep -v CHPL_TARGET_CXX | \
-          sed s/[-+.\/]/_/g | \
-          sed s/\=/' '/ | \
-          awk '{ print "#define " $$1 "_" toupper($$2) }' >> $(CHPL_ENV_HEADER)
+	  sed 's/^[ \t]*//;s/[ \t]*$$//' | \
+	  sed 's/[^0-9A-Za-z]/_/g' | \
+	  awk '{ print "#define " toupper($$1) }' >> $(CHPL_ENV_HEADER)
 	@echo "#endif /* _CHPL_ENV_GEN_H_ */" >> $(CHPL_ENV_HEADER)
 
 THIRD_PARTY_PKGS = $(shell $(CHPL_MAKE_PYTHON) $(CHPL_MAKE_HOME)/util/chplenv/third-party-pkgs)