[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 ↗

(On Diff #379640)

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

flang/runtime/environment.cpp

84

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.

102

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?

108

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 ↗

(On Diff #379640)

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

flang/runtime/environment.cpp

84

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

84

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

84

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

84

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

84

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

[klausler]

flang/runtime/environment.cpp

75

Please use RUNTIME_CHECK instead of assert, here and below.

[rovka]

Replaced assert with RUNTIME_CHECK.

flang/include/flang/Runtime/command.h

 // Try to get the value of the environment variable specified by NAME.
 // Returns a STATUS as described in the standard.
 std::int32_t RTNAME(EnvVariableValue)(const Descriptor &name,
     const Descriptor *value = nullptr, bool trim_name = true,
     const Descriptor *errmsg = nullptr);
 
 // Try to get the significant length of the environment variable specified by
 // NAME. Returns 0 if it doesn't manage.
-std::int64_t RTNAME(EnvVariableLength)(
-    const Descriptor &name, bool trim_name = true);
+std::int64_t RTNAME(EnvVariableLength)(const Descriptor &name,
+    bool trim_name = true, const char *sourceFile = nullptr, int line = 0);
 }
 } // namespace Fortran::runtime
 
 #endif // FORTRAN_RUNTIME_COMMAND_H_

flang/runtime/command.cpp

 //===-- runtime/command.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 "environment.h"
 #include "stat.h"
+#include "terminator.h"
 #include "flang/Runtime/descriptor.h"
 #include <cstdlib>
 #include <limits>
 
 namespace Fortran::runtime {
 std::int32_t RTNAME(ArgumentCount)() {
   int argc{executionEnvironment.argc};
   if (argc > 1) {

 static std::size_t LengthWithoutTrailingSpaces(const Descriptor &d) {
   std::size_t s{d.ElementBytes() - 1};
   while (*d.OffsetElement(s) == ' ') {
     --s;
   }
   return s + 1;
 }
 
-std::int64_t RTNAME(EnvVariableLength)(const Descriptor &name, bool trim_name) {
+std::int64_t RTNAME(EnvVariableLength)(
+    const Descriptor &name, bool trim_name, const char *sourceFile, int line) {
   std::size_t nameLength{
       trim_name ? LengthWithoutTrailingSpaces(name) : name.ElementBytes()};
   if (nameLength == 0) {
     return 0;
   }
 
-  const char *value{
-      executionEnvironment.GetEnv(name.OffsetElement(), nameLength)};
+  Terminator terminator{sourceFile, line};
+  const char *value{executionEnvironment.GetEnv(
+      name.OffsetElement(), nameLength, terminator)};
   if (!value) {
     return 0;
   }
   return std::strlen(value);
 }
 } // namespace Fortran::runtime

flang/runtime/environment.h

 //===-- runtime/environment.h -----------------------------------*- C++ -*-===//
 //
 // 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
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef FORTRAN_RUNTIME_ENVIRONMENT_H_
 #define FORTRAN_RUNTIME_ENVIRONMENT_H_
 
 #include "flang/Decimal/decimal.h"
 #include <optional>
 
 namespace Fortran::runtime {
 
+class Terminator;
+
 #if FLANG_BIG_ENDIAN
 constexpr bool isHostLittleEndian{false};
 #elif FLANG_LITTLE_ENDIAN
 constexpr bool isHostLittleEndian{true};
 #else
 #error host endianness is not known
 #endif
 
 // External unformatted I/O data conversions
 enum class Convert { Unknown, Native, LittleEndian, BigEndian, Swap };
 
 std::optional<Convert> GetConvertFromString(const char *, std::size_t);
 
 struct ExecutionEnvironment {
   void Configure(int argc, const char *argv[], const char *envp[]);
-  const char *GetEnv(const char *name, std::size_t name_length);
+  const char *GetEnv(
+      const char *name, std::size_t name_length, const Terminator &terminator);
 
   int argc;
   const char **argv;
   const char **envp;
   int listDirectedOutputLineLengthLimit;
   enum decimal::FortranRounding defaultOutputRoundingMode;
   Convert conversion;
 };
 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 <cstdio>
 #include <cstdlib>
 #include <cstring>
 #include <limits>
 
 namespace Fortran::runtime {
 
if (auto convert{GetConvertFromString(x, std::strlen(x))}) {
           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 *name, std::size_t name_length) {
-  if (!envp) {
-    // TODO: Ask std::getenv.
-    return nullptr;
-  }
+    const char *name, std::size_t name_length, const Terminator &terminator) {
+  RUNTIME_CHECK(terminator, name && name_length);
 
-  // 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;
+  OwningPtr<char> cStyleName{
+      SaveDefaultCharacter(name, name_length, terminator)};
+  RUNTIME_CHECK(terminator, cStyleName);
+
+  return std::getenv(cStyleName.get());
 }
 } // namespace Fortran::runtime

flang/runtime/tools.cpp

 //===-- runtime/tools.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 "tools.h"
 #include "terminator.h"
 #include <algorithm>
 #include <cstdint>
+#include <cstdlib>
 #include <cstring>
 
 namespace Fortran::runtime {
 
 std::size_t TrimTrailingSpaces(const char *s, std::size_t n) {
   while (n > 0 && s[n - 1] == ' ') {
     --n;
   }

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) {

 }
 
 class CommandFixture : public ::testing::Test {
 protected:
   CommandFixture(int argc, const char *argv[]) {
     RTNAME(ProgramStart)(argc, argv, {});
   }
 
-  CommandFixture(const char *envp[]) { RTNAME(ProgramStart)(0, nullptr, envp); }
-
   std::string GetPaddedStr(const char *text, std::size_t len) const {
     std::string res{text};
     assert(res.length() <= len && "No room to pad");
     res.append(len - res.length(), ' ');
     return res;
   }
 
   void CheckDescriptorEqStr(

 }
 
 TEST_F(SeveralArguments, ErrMsgTooShort) {
   OwningPtr<Descriptor> errMsg{CreateEmptyCharDescriptor<3>()};
   EXPECT_GT(RTNAME(ArgumentValue)(-1, nullptr, errMsg.get()), 0);
   CheckDescriptorEqStr(errMsg.get(), "Inv");
 }
 
-static const char *env[]{"NAME=value", nullptr};
 class EnvironmentVariables : public CommandFixture {
 protected:
-  EnvironmentVariables() : CommandFixture(env) {}
+  EnvironmentVariables() : CommandFixture(0, nullptr) {
+    SetEnv("NAME", "VALUE");
+  }
+
+  // If we have access to setenv, we can run some more fine-grained tests.
+  template <typename ParamType = char>
+  void SetEnv(const ParamType *name, const ParamType *value,
+      decltype(setenv(name, value, 1)) *Enabled = nullptr) {
+    ASSERT_EQ(0, setenv(name, value, /*overwrite=*/1));
+    canSetEnv = true;
+  }
+
+  // Fallback method if setenv is not available.
+  template <typename Unused = void> void SetEnv(const void *, const void *) {}
+
+  bool EnableFineGrainedTests() const { return canSetEnv; }
+
+private:
+  bool canSetEnv{false};
 };
 
 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.
+  char *path{std::getenv("PATH")};
+  auto expectedLen{static_cast<int64_t>(std::strlen(path))};
+  EXPECT_EQ(expectedLen, RTNAME(EnvVariableLength)(*CharDescriptor("PATH")));
+
+  if (EnableFineGrainedTests()) {
+    EXPECT_EQ(5, RTNAME(EnvVariableLength)(*CharDescriptor("NAME")));
 
-  EXPECT_EQ(0, RTNAME(EnvVariableLength)(*CharDescriptor("     ")));
+    EXPECT_EQ(5, RTNAME(EnvVariableLength)(*CharDescriptor("NAME  ")));
+    EXPECT_EQ(0,
+        RTNAME(EnvVariableLength)(
+            *CharDescriptor("NAME "), /*trim_name=*/false));
+  }
 }