Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[flang ]GETLOG runtime and extension implementation: get login username #70917

Closed
wants to merge 23 commits into from

Conversation

yi-wu-arm
Copy link
Contributor

@yi-wu-arm yi-wu-arm commented Nov 1, 2023

Copy link

github-actions bot commented Nov 1, 2023

✅ With the latest revision this PR passed the C/C++ code formatter.

Copy link
Member

@DavidTruby DavidTruby left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with a few small comments. Please wait for @jeanPerier to approve before merging.

flang/runtime/extensions.cpp Outdated Show resolved Hide resolved
flang/runtime/extensions.cpp Outdated Show resolved Hide resolved
flang/runtime/extensions.cpp Outdated Show resolved Hide resolved
flang/runtime/extensions.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@jeanPerier jeanPerier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for the update and addressing my previous comments!

flang/runtime/extensions.cpp Outdated Show resolved Hide resolved
flang/runtime/extensions.cpp Outdated Show resolved Hide resolved
flang/runtime/extensions.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@jeanPerier jeanPerier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for addressing all my comments!

@yi-wu-arm
Copy link
Contributor Author

Hello @klausler, could you please share your thoughts or comments on this patch, particularly with regard to the Windows side? Thanks in advance.

@klausler
Copy link
Contributor

Hello @klausler, could you please share your thoughts or comments on this patch, particularly with regard to the Windows side? Thanks in advance.

You should use modern C++ braced initialization in flang/runtime.

flang/runtime/extensions.cpp Outdated Show resolved Hide resolved
flang/runtime/extensions.cpp Outdated Show resolved Hide resolved
exitstat: If sync, assigned processor-dependent exit status. Otherwise unchanged.
cmdstast: Assigned 0 as specifed in standard, if error then overwrite.
If a condition occurs that would assign a nonzero value to CMDSTAT but
the CMDSTAT variable is not present, error termination is initiated.
@yi-wu-arm
Copy link
Contributor Author

Hello @klausler, could you please share your thoughts or comments on this patch, particularly with regard to the Windows side? Thanks in advance.

You should use modern C++ braced initialization in flang/runtime.

Hi @klausler. Thanks for the comment! I have updated the runtime file with variables being braced initialized. Any other things that I should be aware of? Thanks in advance.

yi-wu-arm and others added 6 commits November 28, 2023 14:29
Work and test on Linux, both sync and async mode.
Sync mode: termination will terminate directly
Async mode: will only terminate the child/async process, no effect on parent
Standard: If a condition occurs that would assign a nonzero value to CMDSTAT,
but the CMDSTAT variable is not present, error termination is initiated.
@yi-wu-arm
Copy link
Contributor Author

Hi @klausler, thanks for the comment, I have changed the initial letters and add const to buffer in function declaration. Is there anything else I should pay attention to? Thanks in advance!

@@ -751,7 +751,7 @@ This phase currently supports all the intrinsic procedures listed above but the
| Object characteristic inquiry functions | ALLOCATED, ASSOCIATED, EXTENDS_TYPE_OF, IS_CONTIGUOUS, PRESENT, RANK, SAME_TYPE, STORAGE_SIZE |
| Type inquiry intrinsic functions | BIT_SIZE, DIGITS, EPSILON, HUGE, KIND, MAXEXPONENT, MINEXPONENT, NEW_LINE, PRECISION, RADIX, RANGE, TINY|
| Non-standard intrinsic functions | AND, OR, XOR, SHIFT, ZEXT, IZEXT, COSD, SIND, TAND, ACOSD, ASIND, ATAND, ATAN2D, COMPL, EQV, NEQV, INT8, JINT, JNINT, KNINT, QCMPLX, DREAL, DFLOAT, QEXT, QFLOAT, QREAL, DNUM, NUM, JNUM, KNUM, QNUM, RNUM, RAN, RANF, ILEN, SIZEOF, MCLOCK, SECNDS, COTAN, IBCHNG, ISHA, ISHC, ISHL, IXOR, IARG, IARGC, NARGS, GETPID, NUMARG, BADDRESS, IADDR, CACHESIZE, EOF, FP_CLASS, INT_PTR_KIND, ISNAN, MALLOC |
| Intrinsic subroutines |MVBITS (elemental), CPU_TIME, DATE_AND_TIME, EVENT_QUERY, EXECUTE_COMMAND_LINE, GET_COMMAND, GET_COMMAND_ARGUMENT, GET_ENVIRONMENT_VARIABLE, MOVE_ALLOC, RANDOM_INIT, RANDOM_NUMBER, RANDOM_SEED, SYSTEM_CLOCK |
| Intrinsic subroutines |MVBITS (elemental), CPU_TIME, DATE_AND_TIME, EVENT_QUERY, EXECUTE_COMMAND_LINE, GET_COMMAND, GET_COMMAND_ARGUMENT, GET_ENVIRONMENT_VARIABLE, GETLOG, MOVE_ALLOC, RANDOM_INIT, RANDOM_NUMBER, RANDOM_SEED, SYSTEM_CLOCK |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are not really implementing GETLOG here as an intrinsic subroutine, but as a library subroutine. (There is no interface exposed to user programs in the intrinsic procedure tables.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I create a new row in this markdown table called Library subroutine?

flang/runtime/extensions.cpp Outdated Show resolved Hide resolved
flang/runtime/time-intrinsic.cpp Outdated Show resolved Hide resolved
signal(SIGCHLD, SIG_IGN);
https://man7.org/linux/man-pages/man2/sigaction.2.html

POSIX.1-1990 disallowed setting the action for SIGCHLD to
SIG_IGN.  POSIX.1-2001 and later allow this possibility, so that
ignoring SIGCHLD can be used to prevent the creation of zombies
(see wait(2)).  Nevertheless, the historical BSD and System V
behaviors for ignoring SIGCHLD differ, so that the only
completely portable method of ensuring that terminated children
do not become zombies is to catch the SIGCHLD signal and perform
a wait(2) or similar.
@yi-wu-arm
Copy link
Contributor Author

CI didn't pass because getlogin_r fail on Linux with error code 6.

ENXIO
The calling process has no controlling terminal.

https://man.archlinux.org/man/getlogin_r.3.en#EMFILE suggested using environment variable rather than use getlogin or getlogin_r.
Thus, the implementation of getlog intrinsics has changed: get username by calling GetEnvVriable runtime function.
On WIndows, use environment variable USERNAME.
On Linux, use environment variable LOGNAME.

@klausler
Copy link
Contributor

klausler commented Dec 6, 2023

The environment is easy to spoof; perhaps you should use it only if there's no login terminal.

@yi-wu-arm
Copy link
Contributor Author

oh my..., I push the execute_command_line branch here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
flang:fir-hlfir flang:runtime flang Flang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants