[flang] Add runtime support for GET_COMMAND

[rovka]

Summary

Implement the GET_COMMAND intrinsic.
Add 2 new parameters (sourceFile and line) so we can create
a terminator for RUNTIME_CHECKs.

[jeanPerier]

The implementation looks good to me, thanks. I have the same comment as @klausler in D118776. We recently realized that lowering was not dealing properly with argument that may be dynamically optional:

subroutine foo(command, length, errmsg, status)
  character(*), optional :: command, length, errmsg
  integer, optional :: status
  call get_command(command, length, errmsg, status)
end subroutine

It is not until the runtime that we will know which arguments are present here. So splitting the intrinsic in different entry points/returning the status by value (which made sense to me before), turns out to push some complexity in lowering here. I am not sure about get_command, but some intrinsics have requirements that at exactly one optional argument be present (RANDOM_SEED), and it is possible but cumbersome to deal with checking these constraints at runtime in inlined code. Having split entry points prevents the runtime from being able to check this. Since these intrinsics are not "hot code", I think moving to a single entry points approach and let the runtime check optionality constraints and dispatching makes more sense.

flang/runtime/command.cpp

166

should this be max(0, executionEnvironment.argc - 1) ? I think it is highly unlikely that argc be zero, but looking at some stack overflow threads, that does not look impossible.

[rovka]

I finally got around to updating this to write the length to an int descriptor.
Also:
Added the source & line parameters so we can have RUNTIME_CHECKS.
Added a test fixture with a completely empty argv.

I'm going to update the other intrinsics in the file to the same model (i.e. single entry point with int descriptors, RUNTIME_CHECKs for invalid descriptors, support for empty argv), and with that I can probably remove some redundant checks from some of the helpers. That's going to be a different patch though.

[jeanPerier]

flang/runtime/command.cpp

174

Could this be moved right after stat = CopyToDescriptor(*value, " ", 1, errmsg, offset); on line 184 ?
It seems related to it but having it after the if {} where the copy happens makes it harder for me to be sure this is related.

flang/unittests/Runtime/CommandTest.cpp

358

This test seems to fail on windows, see the build bot:

Note: Google Test filter = OnlyValidArguments.GetCommandShortLength
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from OnlyValidArguments
[ RUN      ] OnlyValidArguments.GetCommandShortLength
C:\ws\w2\llvm-project\premerge-checks\flang\unittests\Runtime\CommandTest.cpp(77): error: Expected equality of these values:
  *value->OffsetElement<std::int64_t>()
    Which is: 8589934643
  expected
    Which is: 51

I think the value->OffsetElement<std::int64_t>() in CheckDescriptorEqInt may be undefined behavior if the integer type in value is not an std::int64_t (like here where it is a short).

[rovka]

flang/unittests/Runtime/CommandTest.cpp

358

Oops, I thought I had looked at the premerge checks for this one, but it must've slipped my mind 😳 Thanks for looking into it.

[rovka]

• Replaced CopyToDescriptor with CheckAndCopyToDescriptor.
• Made DescriptorEqInt a template, so we can check with the intended integer type.

[jeanPerier]

Looks good, thanks

[jeanPerier]

This comment was removed by jeanPerier.

flang/runtime/command.cpp

 }
 
 static bool IsValidCharDescriptor(const Descriptor *value) {
   return value && value->IsAllocated() &&
       value->type() == TypeCode(TypeCategory::Character, 1) &&
       value->rank() == 0;
 }
 
-static void FillWithSpaces(const Descriptor *value) {
-  std::memset(value->OffsetElement(), ' ', value->ElementBytes());
+static void FillWithSpaces(const Descriptor &value, std::size_t offset = 0) {
+  std::memset(value.OffsetElement(offset), ' ', value.ElementBytes() - offset);
 }
 
 static std::int32_t CopyToDescriptor(const Descriptor &value,
-    const char *rawValue, std::int64_t rawValueLength,
-    const Descriptor *errmsg) {
-  std::int64_t toCopy{std::min(
-      rawValueLength, static_cast<std::int64_t>(value.ElementBytes()))};
-  std::memcpy(value.OffsetElement(), rawValue, toCopy);
+    const char *rawValue, std::int64_t rawValueLength, const Descriptor *errmsg,
+    std::size_t offset = 0) {
+  std::int64_t toCopy{std::min(rawValueLength,
+      static_cast<std::int64_t>(value.ElementBytes() - offset))};
+  std::memcpy(value.OffsetElement(offset), rawValue, toCopy);
 
   if (rawValueLength > toCopy) {
     return ToErrmsg(errmsg, StatValueTooShort);
   }
 
   return StatOk;
 }
 
+static std::int32_t CheckAndCopyToDescriptor(const Descriptor *value,
+    const char *rawValue, const Descriptor *errmsg, std::size_t &offset) {
+  bool haveValue{IsValidCharDescriptor(value)};
+
+  std::int64_t len{StringLength(rawValue)};
+  if (len <= 0) {
+    if (haveValue) {
+      FillWithSpaces(*value);
+    }
+    return ToErrmsg(errmsg, StatMissingArgument);
+  }
+
+  if (haveValue) {
+    std::int32_t stat{CopyToDescriptor(*value, rawValue, len, errmsg, offset)};
+    if (stat != StatOk) {
+      return stat;
+    }
+    offset += len;
+  }
+
+  return StatOk;
+}
+
 std::int32_t RTNAME(ArgumentValue)(
     std::int32_t n, const Descriptor *value, const Descriptor *errmsg) {
   if (IsValidCharDescriptor(value)) {
-    FillWithSpaces(value);
+    FillWithSpaces(*value);
   }
 
   if (n < 0 || n >= executionEnvironment.argc) {
     return ToErrmsg(errmsg, StatInvalidArgumentNumber);
   }
 
   if (IsValidCharDescriptor(value)) {
     const char *arg{executionEnvironment.argv[n]};
     std::int64_t argLen{StringLength(arg)};
     if (argLen <= 0) {
       return ToErrmsg(errmsg, StatMissingArgument);
     }
 
     return CopyToDescriptor(*value, arg, argLen, errmsg);
   }
 
   return StatOk;
 }
 
+std::int32_t RTNAME(CommandValue)(
+    const Descriptor *value, const Descriptor *errmsg) {
+  bool haveValue{IsValidCharDescriptor(value)};
+
+  std::size_t offset{0};
+
+  // value = argv[0]
+  std::int32_t stat{CheckAndCopyToDescriptor(
+      value, executionEnvironment.argv[0], errmsg, offset)};
+  if (stat != StatOk) {
+    return stat;
+  }
+
+  // value += " " + argv[1:n]
+  for (std::int32_t i{1}; i < executionEnvironment.argc; ++i) {
+    if (haveValue) {
+      stat = CopyToDescriptor(*value, " ", 1, errmsg, offset);
+      if (stat != StatOk) {
+        return stat;
+      }
+      offset++;
+    }
+
+    stat = CheckAndCopyToDescriptor(
+        value, executionEnvironment.argv[i], errmsg, offset);
+    if (stat != StatOk) {
+      return stat;
+    }
+  }
+
+  // value += spaces for padding
+  if (haveValue) {
+    FillWithSpaces(*value, offset);
+  }
+
+  return StatOk;
+}
+
+std::int64_t RTNAME(CommandLength)() {
+  std::int64_t length{0};
+  for (int i = 0; i < executionEnvironment.argc; i++) {
+    std::int64_t argLen{StringLength(executionEnvironment.argv[i])};
+    if (argLen <= 0) {
+      return 0;
+    }
+    length += argLen;
+  }
+  // Also add the spaces between arguments.
+  return length + executionEnvironment.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;
 }
 
 static const char *GetEnvVariableValue(
     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 nullptr;
   }
 
   Terminator terminator{sourceFile, line};
   const char *value{executionEnvironment.GetEnv(
       name.OffsetElement(), nameLength, terminator)};
   return value;
 }
 
 std::int32_t RTNAME(EnvVariableValue)(const Descriptor &name,
     const Descriptor *value, bool trim_name, const Descriptor *errmsg,
     const char *sourceFile, int line) {
   if (IsValidCharDescriptor(value)) {
-    FillWithSpaces(value);
+    FillWithSpaces(*value);
   }
 
   const char *rawValue{GetEnvVariableValue(name, trim_name, sourceFile, line)};
   if (!rawValue) {
     return ToErrmsg(errmsg, StatMissingEnvVariable);
   }
 
   if (IsValidCharDescriptor(value)) {

flang/unittests/Runtime/CommandTest.cpp

void CheckArgumentValue(const char *expectedValue, int n) const {
     SCOPED_TRACE("Checking argument:");
     CheckValue(
         [&](const Descriptor *value, const Descriptor *errmsg) {
           return RTNAME(ArgumentValue)(n, value, errmsg);
         },
         expectedValue);
   }
 
+  void CheckCommandValue(const char *args[], int n) const {
+    SCOPED_TRACE("Checking command:");
+    ASSERT_GE(n, 1);
+    std::string expectedValue{args[0]};
+    for (int i = 1; i < n; i++) {
+      expectedValue += " " + std::string{args[i]};
+    }
+    CheckValue(
+        [&](const Descriptor *value, const Descriptor *errmsg) {
+          return RTNAME(CommandValue)(value, errmsg);
+        },
+        expectedValue.c_str());
+  }
+
   void CheckEnvVarValue(
       const char *expectedValue, const char *name, bool trimName = true) const {
     SCOPED_TRACE(name);
     SCOPED_TRACE("Checking environment variable");
     CheckValue(
         [&](const Descriptor *value, const Descriptor *errmsg) {
           return RTNAME(EnvVariableValue)(*CharDescriptor(name), value,
               trimName, errmsg, /*sourceFile=*/nullptr, /*line=*/0);
void CheckMissingArgumentValue(int n, const char *errStr = nullptr) const {
     std::string spaces(value->ElementBytes(), ' ');
     CheckDescriptorEqStr(value.get(), spaces);
 
     if (errStr) {
       std::string paddedErrStr(GetPaddedStr(errStr, err->ElementBytes()));
       CheckDescriptorEqStr(err.get(), paddedErrStr);
     }
   }
+
+  void CheckMissingCommandValue(const char *errStr = nullptr) const {
+    OwningPtr<Descriptor> value{CreateEmptyCharDescriptor()};
+    ASSERT_NE(value, nullptr);
+
+    OwningPtr<Descriptor> err{errStr ? CreateEmptyCharDescriptor() : nullptr};
+
+    EXPECT_GT(RTNAME(CommandValue)(value.get(), err.get()), 0);
+
+    std::string spaces(value->ElementBytes(), ' ');
+    CheckDescriptorEqStr(value.get(), spaces);
+
+    if (errStr) {
+      std::string paddedErrStr(GetPaddedStr(errStr, err->ElementBytes()));
+      CheckDescriptorEqStr(err.get(), paddedErrStr);
+    }
+  }
 };
 
 static const char *commandOnlyArgv[]{"aProgram"};
 class ZeroArguments : public CommandFixture {
 protected:
   ZeroArguments() : CommandFixture(1, commandOnlyArgv) {}
 };
 
 TEST_F(ZeroArguments, ArgumentCount) { EXPECT_EQ(0, RTNAME(ArgumentCount)()); }
 
 TEST_F(ZeroArguments, ArgumentLength) {
   EXPECT_EQ(0, RTNAME(ArgumentLength)(-1));
   EXPECT_EQ(8, RTNAME(ArgumentLength)(0));
   EXPECT_EQ(0, RTNAME(ArgumentLength)(1));
 }
 
 TEST_F(ZeroArguments, ArgumentValue) {
   CheckArgumentValue(commandOnlyArgv[0], 0);
 }
 
+TEST_F(ZeroArguments, CommandLength) { EXPECT_EQ(8, RTNAME(CommandLength())); }
+
+TEST_F(ZeroArguments, CommandValue) { CheckCommandValue(commandOnlyArgv, 1); }
+
 static const char *oneArgArgv[]{"aProgram", "anArgumentOfLength20"};
 class OneArgument : public CommandFixture {
 protected:
   OneArgument() : CommandFixture(2, oneArgArgv) {}
 };
 
 TEST_F(OneArgument, ArgumentCount) { EXPECT_EQ(1, RTNAME(ArgumentCount)()); }
 
 TEST_F(OneArgument, ArgumentLength) {
   EXPECT_EQ(0, RTNAME(ArgumentLength)(-1));
   EXPECT_EQ(8, RTNAME(ArgumentLength)(0));
   EXPECT_EQ(20, RTNAME(ArgumentLength)(1));
   EXPECT_EQ(0, RTNAME(ArgumentLength)(2));
 }
 
 TEST_F(OneArgument, ArgumentValue) {
   CheckArgumentValue(oneArgArgv[0], 0);
   CheckArgumentValue(oneArgArgv[1], 1);
 }
 
+TEST_F(OneArgument, CommandLength) { EXPECT_EQ(29, RTNAME(CommandLength)()); }
+
+TEST_F(OneArgument, CommandValue) { CheckCommandValue(oneArgArgv, 2); }
+
 static const char *severalArgsArgv[]{
     "aProgram", "16-char-long-arg", "", "-22-character-long-arg", "o"};
 class SeveralArguments : public CommandFixture {
 protected:
   SeveralArguments()
       : CommandFixture(sizeof(severalArgsArgv) / sizeof(*severalArgsArgv),
             severalArgsArgv) {}
 };

 
 TEST_F(SeveralArguments, MissingArguments) {
   CheckMissingArgumentValue(-1, "Invalid argument number");
   CheckMissingArgumentValue(2, "Missing argument");
   CheckMissingArgumentValue(5, "Invalid argument number");
   CheckMissingArgumentValue(5);
 }
 
-TEST_F(SeveralArguments, ValueTooShort) {
+TEST_F(SeveralArguments, ArgValueTooShort) {
   OwningPtr<Descriptor> tooShort{CreateEmptyCharDescriptor<15>()};
   ASSERT_NE(tooShort, nullptr);
   EXPECT_EQ(RTNAME(ArgumentValue)(1, tooShort.get(), nullptr), -1);
   CheckDescriptorEqStr(tooShort.get(), severalArgsArgv[1]);
 
   OwningPtr<Descriptor> errMsg{CreateEmptyCharDescriptor()};
   ASSERT_NE(errMsg, nullptr);
 
   EXPECT_EQ(RTNAME(ArgumentValue)(1, tooShort.get(), errMsg.get()), -1);
 
   std::string expectedErrMsg{
       GetPaddedStr("Value too short", errMsg->ElementBytes())};
   CheckDescriptorEqStr(errMsg.get(), expectedErrMsg);
 }
 
-TEST_F(SeveralArguments, ErrMsgTooShort) {
+TEST_F(SeveralArguments, ArgErrMsgTooShort) {
   OwningPtr<Descriptor> errMsg{CreateEmptyCharDescriptor<3>()};
   EXPECT_GT(RTNAME(ArgumentValue)(-1, nullptr, errMsg.get()), 0);
   CheckDescriptorEqStr(errMsg.get(), "Inv");
 }
 
+TEST_F(SeveralArguments, CommandLength) {
+  EXPECT_EQ(0, RTNAME(CommandLength)());
+}
+
+TEST_F(SeveralArguments, CommandValue) {
+  CheckMissingCommandValue();
+  CheckMissingCommandValue("Missing argument");
+}
+
+TEST_F(SeveralArguments, CommandErrMsgTooShort) {
+  OwningPtr<Descriptor> value{CreateEmptyCharDescriptor()};
+  OwningPtr<Descriptor> errMsg{CreateEmptyCharDescriptor<3>()};
+
+  EXPECT_GT(RTNAME(CommandValue)(value.get(), errMsg.get()), 0);
+
+  std::string spaces(value->ElementBytes(), ' ');
+  CheckDescriptorEqStr(value.get(), spaces);
+  CheckDescriptorEqStr(errMsg.get(), "Mis");
+}
+
+TEST_F(SeveralArguments, CommandValueCanTakeNull) {
+  EXPECT_GT(RTNAME(CommandValue)(nullptr, nullptr), 0);
+}
+
+static const char *onlyValidArgsArgv[]{
+    "aProgram", "-f", "has/a/few/slashes", "has\\a\\few\\backslashes"};
+class OnlyValidArguments : public CommandFixture {
+protected:
+  OnlyValidArguments()
+      : CommandFixture(sizeof(onlyValidArgsArgv) / sizeof(*onlyValidArgsArgv),
+            onlyValidArgsArgv) {}
+};
+
+TEST_F(OnlyValidArguments, CommandValue) {
+  CheckCommandValue(onlyValidArgsArgv, 4);
+}
+
+TEST_F(OnlyValidArguments, CommandValueTooShort) {
+  OwningPtr<Descriptor> tooShort{CreateEmptyCharDescriptor<50>()};
+  ASSERT_NE(tooShort, nullptr);
+  EXPECT_EQ(RTNAME(CommandValue)(tooShort.get(), nullptr), -1);
+  CheckDescriptorEqStr(
+      tooShort.get(), "aProgram -f has/a/few/slashes has\\a\\few\\backslashe");
+
+  OwningPtr<Descriptor> errMsg{CreateEmptyCharDescriptor()};
+  ASSERT_NE(errMsg, nullptr);
+
+  EXPECT_EQ(-1, RTNAME(CommandValue)(tooShort.get(), errMsg.get()));
+
+  std::string expectedErrMsg{
+      GetPaddedStr("Value too short", errMsg->ElementBytes())};
+  CheckDescriptorEqStr(errMsg.get(), expectedErrMsg);
+}
+
+TEST_F(OnlyValidArguments, CommandValueCanTakeNull) {
+  EXPECT_EQ(0, RTNAME(CommandValue)(nullptr, nullptr));
+}
+
 class EnvironmentVariables : public CommandFixture {
 protected:
   EnvironmentVariables() : CommandFixture(0, nullptr) {
     SetEnv("NAME", "VALUE");
     SetEnv("EMPTY", "");
   }
 
   // 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, Nonexistent) {
   CheckMissingEnvVarValue("DOESNT_EXIST");
 
   CheckMissingEnvVarValue("      ");
   CheckMissingEnvVarValue("");
 }