[flang] runtime: Read environment variables directly

[rovka]

Summary

Add support for reading environment variables directly, via std::getenv.
This needs to allocate a C-style string to pass into std::getenv. If the
memory allocation for that fails, we terminate.

This also changes the interface for EnvVariableLength to receive the
source file and line so we can crash gracefully.

Note that we are now completely ignoring the envp pointer passed into
ProgramStart, since that could go stale if the environment is modified
during execution.

[klausler]

flang/include/flang/Runtime/memory.h

39

I'm not sure that this specialization is necessary. Does the code compile without it?

flang/runtime/environment.cpp

98

char can be used here instead of char] and would be more idiomatic.

This function might have use cases elsewhere; consider exposing it in tools.h or memory.h.

116

If GetEnvFromEnvp returns a null pointer, how do you know that it was due to a null envp pointer rather that an absent environment variable setting for the name? In the latter case, you should return a null pointer here as well, yes?

122

I think that it would be okay to crash when out of memory here. A null return from GetEnv() should signify only that the name was not found.

[klausler]

Another thing that you should consider: if the program adds or modifies its environment via the POSIX functions setenv(3) and unsetenv(3), your code may not notice the changes unless it uses std::getenv() exclusively -- the captured envp pointer may not reflect those changes and may not even still be valid.

[rovka]

In D111785#3075853, @klausler wrote:

Another thing that you should consider: if the program adds or modifies its environment via the POSIX functions setenv(3) and unsetenv(3), your code may not notice the changes unless it uses std::getenv() exclusively -- the captured envp pointer may not reflect those changes and may not even still be valid.

Yeah, to be honest I'm a bit confused about the existence of envp in the first place. Why exactly are we storing it? I was thinking it would be kind of a "quick" path, so we could skip allocating a C-style string, but if there's a possibility for envp to become stale then maybe we shouldn't have it at all? Or is the idea that when we do have an envp, then the environment cannot be modified and envp has the final word?

flang/include/flang/Runtime/memory.h

39

Nope. This is necessary for the [] operator, which I'm using to put the null at the end.

flang/runtime/environment.cpp

98

char can be used here instead of char] and would be more idiomatic.

But then we won't have operator[].

This function might have use cases elsewhere; consider exposing it in tools.h or memory.h.

Will do.

[rovka]

Updated to crash if we fail to allocate the c-style string. Introducing the crash behavior made NullTerminatedString look pretty much the same as SaveDefaultCharacter, so I used that one instead and completely removed the former, along with the specialization of OwningPtr<char []>.

I haven't changed anything with regards to envp versus getenv. My thoughts are that if envp can become stale, we should remove it completely and use only getenv, but that should probably be a separate patch. What do you think?

[klausler]

In D111785#3083551, @rovka wrote:

Updated to crash if we fail to allocate the c-style string. Introducing the crash behavior made NullTerminatedString look pretty much the same as SaveDefaultCharacter, so I used that one instead and completely removed the former, along with the specialization of OwningPtr<char []>.

I haven't changed anything with regards to envp versus getenv. My thoughts are that if envp can become stale, we should remove it completely and use only getenv, but that should probably be a separate patch. What do you think?

I think ignoring envp is the right approach, and you might as well do it now.

flang/runtime/environment.cpp

98

char can be used here instead of char] and would be more idiomatic.

But then we won't have operator[].

Yes, it'll work. In C and C++, x[y] is defined to be *((x)+(y)) so you can offset a pointer like p[1] to get the following element.

[rovka]

flang/runtime/environment.cpp

98

Yes, on the pointer, but not on the OwningPtr. For that you'd have to call .get() first, which imo doesn't look as nice as using the [] operator directly (which is only possible for OwningPtr<A[]>).

[klausler]

flang/runtime/environment.cpp

98

Yes, on the pointer, but not on the OwningPtr. For that you'd have to call .get() first, which imo doesn't look as nice as using the [] operator directly (which is only possible for OwningPtr<A[]>).

If you insist on using [] and can't get yourself to call get(), then feel free to define operator= in OwningPtr<>.

[rovka]

flang/runtime/environment.cpp

98

It doesn't actually matter anymore, I removed NullTerminatedString and all other changes to memory.h.

[klausler]

flang/runtime/environment.cpp

79

Please use RUNTIME_CHECK instead of assert, here and below.

[rovka]

Replaced assert with RUNTIME_CHECK.

flang/include/flang/Runtime/memory.h

template <typename A> void FreeMemoryAndNullify(A *&p) {
   FreeMemory(p);
   p = nullptr;
 }
 
 template <typename A> struct OwningPtrDeleter {
   void operator()(A *p) { FreeMemory(p); }
 };
 
+template <typename A> struct OwningPtrDeleter<A[]> {
+  void operator()(A *p) { FreeMemory(p); }
+};
+
 template <typename A> using OwningPtr = std::unique_ptr<A, OwningPtrDeleter<A>>;
 
 template <typename A> class SizedNew {
 public:
   explicit SizedNew(const Terminator &terminator) : terminator_{terminator} {}
   template <typename... X>
   [[nodiscard]] OwningPtr<A> operator()(std::size_t bytes, X &&...x) {
     return OwningPtr<A>{new (AllocateMemoryOrCrash(terminator_, bytes))

flang/runtime/environment.h

struct ExecutionEnvironment {
   const char *GetEnv(const char *name, std::size_t name_length);
 
   int argc;
   const char **argv;
   const char **envp;
   int listDirectedOutputLineLengthLimit;
   enum decimal::FortranRounding defaultOutputRoundingMode;
   Convert conversion;
+
+private:
+  const char *GetEnvFromEnvp(const char *name, std::size_t name_length);
 };
 extern ExecutionEnvironment executionEnvironment;
 } // namespace Fortran::runtime
 
 #endif // FORTRAN_RUNTIME_ENVIRONMENT_H_

flang/runtime/environment.cpp

 //===-- runtime/environment.cpp -------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "environment.h"
+#include "memory.h"
 #include "tools.h"
+#include <cassert>
 #include <cstdio>
 #include <cstdlib>
 #include <cstring>
 #include <limits>
 
 namespace Fortran::runtime {
 
 ExecutionEnvironment executionEnvironment;
if (auto convert{GetConvertFromString(x, std::strlen(x))}) {
       std::fprintf(
           stderr, "Fortran runtime: FORT_CONVERT=%s is invalid; ignored\n", x);
     }
   }
 
   // TODO: Set RP/ROUND='PROCESSOR_DEFINED' from environment
 }
 
-const char *ExecutionEnvironment::GetEnv(
+const char *ExecutionEnvironment::GetEnvFromEnvp(
     const char *name, std::size_t name_length) {
   if (!envp) {
-    // TODO: Ask std::getenv.
     return nullptr;
   }
 
   // envp is an array of strings of the form "name=value".
   for (const char **var{envp}; *var != nullptr; ++var) {
     const char *eq{std::strchr(*var, '=')};
     if (!eq) {
       // Found a malformed environment string, just ignore it.
       continue;
     }
     if (static_cast<std::size_t>(eq - *var) != name_length) {
       continue;
     }
     if (std::memcmp(*var, name, name_length) == 0) {
       return eq + 1;
     }
   }
+
   return nullptr;
 }
+
+static OwningPtr<char[]> NullTerminatedString(
+    const char *name, std::size_t name_length) {
+  OwningPtr<char[]> result{static_cast<char *>(std::malloc(name_length + 1))};
+  if (result) {
+    std::memcpy(result.get(), name, name_length);
+    result[name_length] = '\0';
+  }
+
+  return result;
+}
+
+const char *ExecutionEnvironment::GetEnv(
+    const char *name, std::size_t name_length) {
+  assert(name_length && "Invalid environment variable name");
+
+  const char *value{GetEnvFromEnvp(name, name_length)};
+  if (value) {
+    return value;
+  }
+
+  // Try to read the environment directly. This is especially useful when
+  // ProgramStart was not invoked at all, e.g. when the main program is not
+  // Fortran.
+  OwningPtr<char[]> cStyleName{NullTerminatedString(name, name_length)};
+  if (!cStyleName) {
+    std::fputs("Fortran runtime: out of memory for env var name\n", stderr);
+    return nullptr;
+  }
+
+  return std::getenv(cStyleName.get());
+}
 } // namespace Fortran::runtime

flang/unittests/Runtime/CommandTest.cpp

 //===-- flang/unittests/RuntimeGTest/CommandTest.cpp ----------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "flang/Runtime/command.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 #include "flang/Runtime/descriptor.h"
 #include "flang/Runtime/main.h"
+#include <cstdlib>
 
 using namespace Fortran::runtime;
 
 template <std::size_t n = 64>
 static OwningPtr<Descriptor> CreateEmptyCharDescriptor() {
   OwningPtr<Descriptor> descriptor{Descriptor::Create(
       sizeof(char), n, nullptr, 0, nullptr, CFI_attribute_allocatable)};
   if (descriptor->Allocate() != 0) {
TEST_F(EnvironmentVariables, Length) {
   EXPECT_EQ(5, RTNAME(EnvVariableLength)(*CharDescriptor("NAME")));
   EXPECT_EQ(0, RTNAME(EnvVariableLength)(*CharDescriptor("DOESNT_EXIST")));
 
   EXPECT_EQ(5, RTNAME(EnvVariableLength)(*CharDescriptor("NAME  ")));
   EXPECT_EQ(0,
       RTNAME(EnvVariableLength)(*CharDescriptor("NAME "), /*trim_name=*/false));
 
   EXPECT_EQ(0, RTNAME(EnvVariableLength)(*CharDescriptor("     ")));
+  EXPECT_EQ(0, RTNAME(EnvVariableLength)(*CharDescriptor("")));
+
+  // Test a variable that's expected to exist in the environment but isn't
+  // passed into ProgramStart's envp argument.
+  char *path{std::getenv("PATH")};
+  auto expectedLen{static_cast<int64_t>(std::strlen(path))};
+  EXPECT_EQ(expectedLen, RTNAME(EnvVariableLength)(*CharDescriptor("PATH")));
+}
+
+TEST(EnvironmentVariablesNoProgStart, Length) {
+  // Test a variable that's expected to exist in the environment.
+  char *path{std::getenv("PATH")};
+  auto expectedLen{static_cast<int64_t>(std::strlen(path))};
+  EXPECT_EQ(expectedLen, RTNAME(EnvVariableLength)(*CharDescriptor("PATH")));
 }