This is an archive of the discontinued LLVM Phabricator instance.

[flang] GET_COMMAND_ARGUMENT(VALUE) runtime implementation
ClosedPublic

Authored by rovka on Sep 15 2021, 1:35 AM.

Details

Summary

Partial implementation for the second entry point for
GET_COMMAND_ARGUMENT. It handles the VALUE and STATUS arguments, and
doesn't touch ERRMSG.

Diff Detail

Event Timeline

rovka created this revision.Sep 15 2021, 1:35 AM
rovka requested review of this revision.Sep 15 2021, 1:35 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 15 2021, 1:35 AM
rovka added a comment.Sep 15 2021, 1:50 AM

I've put ERRMSG handling in a different patch to make it easier to review, but we can also squash it into this one.

flang/runtime/command.cpp
57

We're silently ignoring 'value' if it's not allocated or not the right type - should we have an assert instead if it doesn't look the way we expect?

69

It's not very clear to me from the standard what's supposed to happen to 'value' if it's too short. I guess it could be helpful to write as much of the argument as we can fit into it, but then we'd also have to decide whether we should put a \0 at the end or not (*), and overall I don't know if it's worth the trouble. Thoughts?

(*) IIUC gfortran doesn't force a \0, which makes the code easier but the testing harder, because I think it's UB to call EXPECT_STREQ on a char string that's not null-terminated.

klausler added inline comments.Sep 16 2021, 8:42 AM
flang/runtime/command.cpp
14–15

If these types are used in the specifications of implementations of APIs then they should not be introduced here. Implementations of functions should match their prototypes in headers, apart from maybe adding names for arguments and removing any default values.

69

Truncate if short, pad if long, and never append a NUL byte.

rovka updated this revision to Diff 373521.Sep 20 2021, 2:14 AM

Truncate or pad with spaces.

rovka marked an inline comment as done and an inline comment as not done.Sep 20 2021, 2:15 AM
rovka added inline comments.
flang/runtime/command.cpp
14–15

Yeah, I introduced these as shorthand initially, but I guess it makes sense to have them in the header.

rovka updated this revision to Diff 373523.Sep 20 2021, 2:18 AM
klausler added inline comments.Sep 27 2021, 11:04 AM
flang/runtime/command.cpp
57

The value argument should be a reference, not a pointer, to a descriptor here (and in your IsValidCharDescriptor() utility above). It is not optional if ArgumentValue() is actually being called.

rovka added inline comments.Sep 27 2021, 2:08 PM
flang/runtime/command.cpp
57

Actually I was thinking it is optional. For instance a call to GET_COMMAND_ARGUMENT(NUMBER, LENGTH, STATUS, ERRMSG) would be lowered to a call to ArgumentLength() and one to ArgumentValue() without a descriptor for value, and the latter would just fill in the STATUS and ERRMSG. I guess lowering could skip the call to ArgumentValue and fill in STATUS and ERRMSG based on the return from ArgumentLength, but wouldn't that just be more complicated than necessary?

klausler added inline comments.Sep 27 2021, 2:19 PM
flang/runtime/command.cpp
57

Ok, SGTM. Please clean up the types and I'll approve.

rovka updated this revision to Diff 375427.Sep 27 2021, 3:48 PM
This revision was not accepted when it landed; it landed in state Needs Review.Sep 28 2021, 1:38 AM
This revision was automatically updated to reflect the committed changes.