| Message ID | AANLkTinQU9K2HTSi6TgEgRmhVnNqW=cpb=PzaoqzB5yZ@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
Hi Janne, On 01/24/2011 04:05 PM, Janne Blomqvist wrote: > the attached patch makes the GETLOG intrinsic thread safe by using the > POSIX getpwuid_r() function, if available. > Regtested on x86_64-unknown-linux-gnu, Ok for trunk? The patch looks OK except for the following: > @@ -75,7 +75,22 @@ PREFIX(getlog) (char * login, gfc_charlen_type login_len) > > memset (login, ' ', login_len); /* Blank the string. */ > [...] > p_len = strlen (p); > if (login_len< p_len) > - memcpy (login, p, login_len); > - else > - memcpy (login, p, p_len); > + p_len = login_len; > + memcpy (login, p, p_len); The change looks wrong to me. I think it will write where it shouldn't for the following program (assuming that you do not have a len=1 login name): character(len=1) :: str call getlog(str) print *, str end Tobias
On 01/25/2011 04:15 PM, Tobias Burnus wrote: >> p_len = strlen (p); >> if (login_len< p_len) >> - memcpy (login, p, login_len); >> - else >> - memcpy (login, p, p_len); >> + p_len = login_len; >> + memcpy (login, p, p_len); > > The change looks wrong to me. Scratch that - I saw one "-" too much. The patch is OK as is. Tobias
On Tue, Jan 25, 2011 at 17:17, Tobias Burnus <burnus@net-b.de> wrote: > On 01/25/2011 04:15 PM, Tobias Burnus wrote: >>> >>> p_len = strlen (p); >>> if (login_len< p_len) >>> - memcpy (login, p, login_len); >>> - else >>> - memcpy (login, p, p_len); >>> + p_len = login_len; >>> + memcpy (login, p, p_len); >> >> The change looks wrong to me. > > Scratch that - I saw one "-" too much. The patch is OK as is. Thanks, Sending libgfortran/ChangeLog Sending libgfortran/config.h.in Sending libgfortran/configure Sending libgfortran/configure.ac Sending libgfortran/intrinsics/getlog.c Transmitting file data ..... Committed revision 169243.
Janne Blomqvist <blomqvist.janne@gmail.com> writes: > the attached patch makes the GETLOG intrinsic thread safe by using the > POSIX getpwuid_r() function, if available. > > Regtested on x86_64-unknown-linux-gnu, Ok for trunk? This patch broke Solaris bootstrap: /vol/gcc/src/hg/trunk/local/libgfortran/intrinsics/getlog.c: In function '_gfortran_getlog': /vol/gcc/src/hg/trunk/local/libgfortran/intrinsics/getlog.c:89:3: error: too many arguments to function 'getpwuid_r' /usr/include/pwd.h:191:23: note: declared here /vol/gcc/src/hg/trunk/local/libgfortran/intrinsics/getlog.c:89:7: warning: assignment makes integer from pointer without a cast [enabled by default] The problem is that <pwd.h> declares two different variants of getpwuid_r: #if (_POSIX_C_SOURCE - 0 >= 199506L) || defined(_POSIX_PTHREAD_SEMANTICS) extern int getpwuid_r(uid_t, struct passwd *, char *, int, struct passwd **); #else extern struct passwd *getpwuid_r(uid_t, struct passwd *, char *, int); #endif There's this comment: /* * getpwuid_r() & getpwnam_r() prototypes are defined here. */ /* * Previous releases of Solaris, starting at 2.3, provided definitions of * various functions as specified in POSIX.1c, Draft 6. For some of these * functions, the final POSIX 1003.1c standard had a different number of * arguments and return values. * * The following segment of this header provides support for the standard * interfaces while supporting applications written under earlier * releases. The application defines appropriate values of the feature * test macros _POSIX_C_SOURCE and _POSIX_PTHREAD_SEMANTICS to indicate * whether it was written to expect the Draft 6 or standard versions of * these interfaces, before including this header. This header then * provides a mapping from the source version of the interface to an * appropriate binary interface. Such mappings permit an application * to be built from libraries and objects which have mixed expectations * of the definitions of these functions. The POSIX.1 definitions of getpwuid_r are only defined #if (_POSIX_C_SOURCE - 0 >= 199506L) || defined(_POSIX_PTHREAD_SEMANTICS) I'm not yet sure how and where best to handle this: perhaps we can do this along the lines of what g++ defines (cf. gcc/config/sol2.h): /* For C++ we need to add some additional macro \ definitions required by the C++ standard \ library. */ \ if (c_dialect_cxx ()) \ { \ builtin_define ("__STDC_VERSION__=199901L");\ builtin_define ("_XOPEN_SOURCE=600"); \ builtin_define ("_LARGEFILE_SOURCE=1"); \ builtin_define ("_LARGEFILE64_SOURCE=1"); \ builtin_define ("__EXTENSIONS__"); \ } \ For GNU/Linux, this is probably handled by -D_GNU_SOURCE in Makefile.am (AM_CPPFLAGS), but this is obviously not general. Alternatively, one could update the configure check to not only check for the existance of getpwuid_r, but for a version that takes the correct number and types of arguments. Rainer
On 01/27/2011 03:41 PM, Rainer Orth wrote: >> the attached patch makes the GETLOG intrinsic thread safe by using the >> POSIX getpwuid_r() function, if available. > This patch broke Solaris bootstrap: > > The problem is that<pwd.h> declares two different variants of > getpwuid_r: I have filled a tracking bug: PR 47491. And I have a question to you, Rainer: There are similar patches for ctime_r and for ttyname_r. Do they work as is on Solaris or is also special care required? See: http://gcc.gnu.org/ml/fortran/2011-01/msg00225.html and http://gcc.gnu.org/ml/fortran/2011-01/msg00224.html Tobias
Hi Tobias, > I have filled a tracking bug: PR 47491. Thanks. > And I have a question to you, Rainer: There are similar patches for ctime_r > and for ttyname_r. Do they work as is on Solaris or is also special care > required? > > See: http://gcc.gnu.org/ml/fortran/2011-01/msg00225.html and > http://gcc.gnu.org/ml/fortran/2011-01/msg00224.html The only difference between the two ttyname_r variants is: * full standard: extern int ttyname_r(int, char *, size_t); * draft standard: extern char *ttyname_r(int, char *, int); For ctime_r it's worse: * full standard: extern char *ctime_r(const time_t *, char *); * draft standards: extern char *ctime_r(const time_t *, char *, int); So we should simply make sure that the standard versions are used. Rainer
On Thu, Jan 27, 2011 at 17:05, Rainer Orth <ro@cebitec.uni-bielefeld.de> wrote: > Hi Tobias, > >> I have filled a tracking bug: PR 47491. > > Thanks. > >> And I have a question to you, Rainer: There are similar patches for ctime_r >> and for ttyname_r. Do they work as is on Solaris or is also special care >> required? >> >> See: http://gcc.gnu.org/ml/fortran/2011-01/msg00225.html and >> http://gcc.gnu.org/ml/fortran/2011-01/msg00224.html > > The only difference between the two ttyname_r variants is: > > * full standard: > > extern int ttyname_r(int, char *, size_t); > > * draft standard: > > extern char *ttyname_r(int, char *, int); > > For ctime_r it's worse: > > * full standard: > > extern char *ctime_r(const time_t *, char *); > > * draft standards: > > extern char *ctime_r(const time_t *, char *, int); > > So we should simply make sure that the standard versions are used. If my google-fu isn't failing me, it seems that the fix for these is more or less the same as for the getpwuid_r() issue. That is, if my currently posted patch adding AC_USE_SYSTEM_EXTENSIONS fixes the getpwuid_r() issue, it should also fix ctime_r() and ttyname_r().
Janne Blomqvist <blomqvist.janne@gmail.com> writes: > If my google-fu isn't failing me, it seems that the fix for these is > more or less the same as for the getpwuid_r() issue. That is, if my > currently posted patch adding AC_USE_SYSTEM_EXTENSIONS fixes the > getpwuid_r() issue, it should also fix ctime_r() and ttyname_r(). Exactly. Rainer
diff --git a/libgfortran/configure.ac b/libgfortran/configure.ac index 4f137e4..9b91f9a 100644 --- a/libgfortran/configure.ac +++ b/libgfortran/configure.ac @@ -249,7 +249,7 @@ AC_CHECK_FUNCS(chdir strerror getlogin gethostname kill link symlink perror) AC_CHECK_FUNCS(sleep time ttyname signal alarm ctime clock access fork execl) AC_CHECK_FUNCS(wait setmode execvp pipe dup2 close fdopen strcasestr getrlimit) AC_CHECK_FUNCS(gettimeofday stat fstat lstat getpwuid vsnprintf dup getcwd) -AC_CHECK_FUNCS(localtime_r gmtime_r strerror_r) +AC_CHECK_FUNCS(localtime_r gmtime_r strerror_r getpwuid_r) # Check for glibc backtrace functions AC_CHECK_FUNCS(backtrace backtrace_symbols) diff --git a/libgfortran/intrinsics/getlog.c b/libgfortran/intrinsics/getlog.c index e75aa1c..baacdf0 100644 --- a/libgfortran/intrinsics/getlog.c +++ b/libgfortran/intrinsics/getlog.c @@ -1,8 +1,8 @@ /* Implementation of the GETLOG g77 intrinsic. - Copyright (C) 2005, 2007, 2009 Free Software Foundation, Inc. + Copyright (C) 2005, 2007, 2009, 2011 Free Software Foundation, Inc. Contributed by François-Xavier Coudert <coudert@clipper.ens.fr> -This file is part of the GNU Fortran 95 runtime library (libgfortran). +This file is part of the GNU Fortran runtime library (libgfortran). Libgfortran is free software; you can redistribute it and/or modify it under the terms of the GNU General Public @@ -75,7 +75,22 @@ PREFIX(getlog) (char * login, gfc_charlen_type login_len) memset (login, ' ', login_len); /* Blank the string. */ -#if defined(HAVE_GETPWUID) && defined(HAVE_GETEUID) +#if defined(HAVE_GETPWUID_R) && defined(HAVE_GETEUID) + struct passwd pwd; + struct passwd *result; + char *buf; + int err; + /* To be pedantic, buflen should be determined by + sysconf(_SC_GETPW_R_SIZE_MAX), which is 1024 on some tested + targets; we do something simple in case the target doesn't + support sysconf. */ + static const size_t buflen = 1024; + buf = get_mem (buflen); + err = getpwuid_r (geteuid (), &pwd, buf, buflen, &result); + if (err != 0 || result == NULL) + goto cleanup; + p = pwd.pw_name; +#elif defined(HAVE_GETPWUID) && defined(HAVE_GETEUID) { struct passwd *pw = getpwuid (geteuid ()); if (pw) @@ -83,20 +98,22 @@ PREFIX(getlog) (char * login, gfc_charlen_type login_len) else return; } -#else -# ifdef HAVE_GETLOGIN +#elif HAVE_GETLOGIN p = getlogin(); # else return; -# endif #endif if (p == NULL) - return; + goto cleanup; p_len = strlen (p); if (login_len < p_len) - memcpy (login, p, login_len); - else - memcpy (login, p, p_len); + p_len = login_len; + memcpy (login, p, p_len); + + cleanup: +#ifdef HAVE_GETPWUID_R + free (buf); +#endif }