[PATCH] D118777: [flang] Add runtime support for GET_COMMAND

[Jean Perier via Phabricator via llvm-commits]

jeanPerier added a comment.

The implementation looks good to me, thanks. I have the same comment as @klausler in D118776 <https://reviews.llvm.org/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.



================
Comment at: flang/runtime/command.cpp:166
+  // Also add the spaces between arguments.
+  return length + executionEnvironment.argc - 1;
+}
----------------
should this be `max(0, executionEnvironment.argc - 1)` ? I think it is highly unlikely that argc be zero, but looking at some [[ https://stackoverflow.com/questions/49817316/can-argc-be-zero-on-a-posix-system#:~:text=Yes%2C%20it%20can%20be%20zero,to%20be%20the%20program%20name. | stack overflow threads, that does not look impossible ]]. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118777/new/

https://reviews.llvm.org/D118777