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

Jean Perier via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Mar 2 04:52:44 PST 2022


jeanPerier added inline comments.
Herald added a project: All.


================
Comment at: flang/runtime/command.cpp:189
+    }
+    offset++;
+
----------------
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.


================
Comment at: flang/unittests/Runtime/CommandTest.cpp:396
+  EXPECT_EQ(0, RTNAME(GetCommand)(nullptr, length.get(), nullptr));
+  CheckDescriptorEqInt(length.get(), 51);
+}
----------------
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).


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

https://reviews.llvm.org/D118777



More information about the llvm-commits mailing list