Message ID | 87k3afhsua.fsf@rasmusvillemoes.dk |
---|---|
State | New |
Headers | show |
Rasmus Villemoes <rv@rasmusvillemoes.dk> writes:
> The sole varargs argument to open and friends has type mode_t, not int.
Varargs use the promoted type, but mode_t traditionally has been
unsigned short.
Andreas.
Andreas Schwab <schwab@linux-m68k.org> writes: > Rasmus Villemoes <rv@rasmusvillemoes.dk> writes: > >> The sole varargs argument to open and friends has type mode_t, not int. > > Varargs use the promoted type, but mode_t traditionally has been > unsigned short. Makes sense. But then sysdeps/unix/sysv/linux/mq_open.c and sysdeps/unix/sysv/linux/openat.c are inconsistent with the other files in sysdeps/unix/sysv/linux/. (Also, mode_t is actually unsigned int on linux/x86_64.) Rasmus
gnulib attacks this problem by defining a macro PROMOTED_MODE_T that expands to 'int' on platforms where mode_t is narrower than int, and to 'mode_t' otherwise. This supports 'mode = va_arg (ap, PROMOTED_MODE_T);'. glibc could do something similar.
On Thu, Apr 24, 2014 at 07:06:58AM -0700, Paul Eggert wrote: > gnulib attacks this problem by defining a macro PROMOTED_MODE_T that > expands to 'int' on platforms where mode_t is narrower than int, and > to 'mode_t' otherwise. This supports 'mode = va_arg (ap, > PROMOTED_MODE_T);'. glibc could do something similar. I thought glibc always had mode_t with rank >= int, but if not, something like __typeof__(+(mode_t)0) would work just fine, since glibc depends on GNU C extensions like __typeof__ anyway. Rich
On Thu, Apr 24, 2014 at 02:30:21PM +0200, Andreas Schwab wrote: > Rasmus Villemoes <rv@rasmusvillemoes.dk> writes: > > > The sole varargs argument to open and friends has type mode_t, not int. > > Varargs use the promoted type, but mode_t traditionally has been > unsigned short. > So which solution do you prefer? Adding a custom type for that as suggested in sibling threads? What about just adding a cast like mode = (mode_t) va_arg (arg, int);
On 05/03/2014 10:28 AM, Ondřej Bílka wrote: > On Thu, Apr 24, 2014 at 02:30:21PM +0200, Andreas Schwab wrote: >> Rasmus Villemoes <rv@rasmusvillemoes.dk> writes: >> >>> The sole varargs argument to open and friends has type mode_t, not int. >> >> Varargs use the promoted type, but mode_t traditionally has been >> unsigned short. >> > So which solution do you prefer? Adding a custom type for that as > suggested in sibling threads? What about just adding a cast like > > mode = (mode_t) va_arg (arg, int); > Might be overkill for this instance alone, but another option would be letting the compiler do the work. It avoids having to care whether mode_t might be wider than int, or unsigned. Can be done with C11's _Generic, or GCC's __builtin_choose_expr/__builtin_types_compatible_p. Here's how it might look with the latter: mode = __unpromoted_va_arg (arg, mode_t); #define __maybe_va_arg(list, type, unpromoted, promoted, else_expr) \ __builtin_choose_expr (__builtin_types_compatible_p (type, unpromoted), \ va_arg (list, promoted), \ else_expr) /* Like va_arg, but accepts unpromoted types. */ #define __promoted_va_arg(list, type) \ (type) __maybe_va_arg (list, type, float, double, \ __maybe_va_arg (list, type, char, int, \ __maybe_va_arg (list, type, short int, int, \ __maybe_va_arg (list, type, unsigned char, unsigned int, \ __maybe_va_arg (list, type, unsigned short int, unsigned int, \ va_arg (list, type))))))
Pedro Alves <palves@redhat.com> writes: > On 05/03/2014 10:28 AM, Ondřej Bílka wrote: >> On Thu, Apr 24, 2014 at 02:30:21PM +0200, Andreas Schwab wrote: >>> Rasmus Villemoes <rv@rasmusvillemoes.dk> writes: >>> >>>> The sole varargs argument to open and friends has type mode_t, not int. >>> >>> Varargs use the promoted type, but mode_t traditionally has been >>> unsigned short. >>> >> So which solution do you prefer? Adding a custom type for that as >> suggested in sibling threads? What about just adding a cast like >> >> mode = (mode_t) va_arg (arg, int); >> That requires sizeof(mode_t) <= sizeof(int) - which (I think) is the case on all current and probably also all future platforms, but nothing guarantees it. > Might be overkill for this instance alone, but another option > would be letting the compiler do the work. Yes, that is definitely the best way. It would be nice if gcc exposed something like __builtin_promoted_type(T) so one could simply say va_arg(arg, __builtin_promoted_type(mode_t)); or even better if gcc had a switch to simply DTRT when it encounters a va_arg with a non-self-promoting type. gcc certainly already contains the infrastructure to do this. > Can be done with C11's _Generic, or GCC's > __builtin_choose_expr/__builtin_types_compatible_p. > > Here's how it might look with the latter: > > mode = __unpromoted_va_arg (arg, mode_t); > > #define __maybe_va_arg(list, type, unpromoted, promoted, else_expr) \ > __builtin_choose_expr (__builtin_types_compatible_p (type, unpromoted), \ > va_arg (list, promoted), \ > else_expr) > > /* Like va_arg, but accepts unpromoted types. */ > #define __promoted_va_arg(list, type) \ > (type) __maybe_va_arg (list, type, float, double, \ > __maybe_va_arg (list, type, char, int, \ > __maybe_va_arg (list, type, short int, int, \ > __maybe_va_arg (list, type, unsigned char, unsigned int, \ > __maybe_va_arg (list, type, unsigned short int, unsigned int, \ > va_arg (list, type)))))) This seems to solve it, but as you say, it may be overkill. I don't think there are functions other than open and derivatives with this legacy problem. Since mode_t is guaranteed to be an integer type, I think mode = __builtin_choose_expr(sizeof(mode_t) < sizeof(int), va_arg(ap, int), va_arg(ap, mode_t)); would be sufficient. Rasmus
On Tue, May 06, 2014 at 11:36:53AM +0200, Rasmus Villemoes wrote: > Pedro Alves <palves@redhat.com> writes: > > > On 05/03/2014 10:28 AM, Ondřej Bílka wrote: > >> On Thu, Apr 24, 2014 at 02:30:21PM +0200, Andreas Schwab wrote: > >>> Rasmus Villemoes <rv@rasmusvillemoes.dk> writes: > >>> > >>>> The sole varargs argument to open and friends has type mode_t, not int. > >>> > >>> Varargs use the promoted type, but mode_t traditionally has been > >>> unsigned short. > >>> > >> So which solution do you prefer? Adding a custom type for that as > >> suggested in sibling threads? What about just adding a cast like > >> > >> mode = (mode_t) va_arg (arg, int); > >> > > That requires sizeof(mode_t) <= sizeof(int) - which (I think) is the > case on all current and probably also all future platforms, but nothing > guarantees it. > > > Might be overkill for this instance alone, but another option > > would be letting the compiler do the work. > > Yes, that is definitely the best way. It would be nice if gcc exposed > something like __builtin_promoted_type(T) so one could simply say > va_arg(arg, __builtin_promoted_type(mode_t)); or even better if gcc had > a switch to simply DTRT when it encounters a va_arg with a > non-self-promoting type. gcc certainly already contains the > infrastructure to do this. > Could you prepare it as v2 of this patch?
logistically speaking, have you signed FSF copyright papers for glibc ? -mike
Mike Frysinger <vapier@gentoo.org> writes: > logistically speaking, have you signed FSF copyright papers for glibc > ? No. Where and how do I do that? Will scan+email be sufficient, or does the process involve snailmail? Rasmus
On Sun 03 Aug 2014 17:57:27 Rasmus Villemoes wrote: > Mike Frysinger <vapier@gentoo.org> writes: > > logistically speaking, have you signed FSF copyright papers for glibc > > ? > > No. Where and how do I do that? Will scan+email be sufficient, or does > the process involve snailmail? they're always changing the process ;). the bootstrap is def over e-mail, and the assignment people will provide direction after that. start here: https://sourceware.org/glibc/wiki/Contribution%20checklist#FSF_copyright_Assignment you'll want the request-assign.future. just fill that out and e-mail it to assign@gnu.org. please make sure to read over the papers they send you so you're comfortable with what it entails. the scope should be limited to changes to the glibc project in case that helps. -mike
On Mon, Aug 04 2014, Mike Frysinger <vapier@gentoo.org> wrote: > On Sun 03 Aug 2014 17:57:27 Rasmus Villemoes wrote: >> Mike Frysinger <vapier@gentoo.org> writes: >> > logistically speaking, have you signed FSF copyright papers for glibc >> > ? >> >> No. Where and how do I do that? Will scan+email be sufficient, or does >> the process involve snailmail? > > they're always changing the process ;). the bootstrap is def over e-mail, and > the assignment people will provide direction after that. > > start here: > https://sourceware.org/glibc/wiki/Contribution%20checklist#FSF_copyright_Assignment > you'll want the request-assign.future. just fill that out and e-mail it to > assign@gnu.org. Bootstrapping went fine (almost immediate response). I then sent the paperwork to FSF, by now almost a month ago, but then haven't heard anything. Nor have they replied to my email where I asked if the snail mail had arrived. What would be the next logical step? Rasmus
On Tue, Sep 02 2014, Rasmus Villemoes <rv@rasmusvillemoes.dk> wrote: > Bootstrapping went fine (almost immediate response). I then sent the > paperwork to FSF, by now almost a month ago, but then haven't heard > anything. Nor have they replied to my email where I asked if the snail > mail had arrived. What would be the next logical step? That helped. The paperwork should now be in order, so can I get someone to look at http://patchwork.sourceware.org/patch/2292/ (latest version mode_t patch) http://patchwork.sourceware.org/patch/1306/ (sendmmsg prototype fix) http://patchwork.sourceware.org/patch/1048/ (sys/time.h + test case) http://thread.gmane.org/gmane.comp.lib.glibc.alpha/41725 (eventfd prototype fix) Thanks, Rasmus
diff --git a/io/open.c b/io/open.c index 24aa380..5be57a1 100644 --- a/io/open.c +++ b/io/open.c @@ -30,7 +30,7 @@ __libc_open (file, oflag) const char *file; int oflag; { - int mode; + mode_t mode; if (file == NULL) { @@ -42,7 +42,7 @@ __libc_open (file, oflag) { va_list arg; va_start(arg, oflag); - mode = va_arg(arg, int); + mode = va_arg(arg, mode_t); va_end(arg); } diff --git a/io/open64.c b/io/open64.c index 3f3d2e8..b740ab6 100644 --- a/io/open64.c +++ b/io/open64.c @@ -28,7 +28,7 @@ __libc_open64 (file, oflag) const char *file; int oflag; { - int mode; + mode_t mode; if (file == NULL) { @@ -40,7 +40,7 @@ __libc_open64 (file, oflag) { va_list arg; va_start (arg, oflag); - mode = va_arg (arg, int); + mode = va_arg (arg, mode_t); va_end (arg); } diff --git a/io/openat.c b/io/openat.c index 2d82270..aac603c 100644 --- a/io/openat.c +++ b/io/openat.c @@ -38,7 +38,7 @@ __openat (fd, file, oflag) const char *file; int oflag; { - int mode; + mode_t mode; if (file == NULL) { @@ -64,7 +64,7 @@ __openat (fd, file, oflag) { va_list arg; va_start (arg, oflag); - mode = va_arg (arg, int); + mode = va_arg (arg, mode_t); va_end (arg); } diff --git a/io/openat64.c b/io/openat64.c index c0c4e19..1f126fc 100644 --- a/io/openat64.c +++ b/io/openat64.c @@ -31,7 +31,7 @@ __openat64 (fd, file, oflag) const char *file; int oflag; { - int mode; + mode_t mode; if (file == NULL) { @@ -57,7 +57,7 @@ __openat64 (fd, file, oflag) { va_list arg; va_start (arg, oflag); - mode = va_arg (arg, int); + mode = va_arg (arg, mode_t); va_end (arg); } diff --git a/sysdeps/posix/open64.c b/sysdeps/posix/open64.c index 64d192a..9364454 100644 --- a/sysdeps/posix/open64.c +++ b/sysdeps/posix/open64.c @@ -24,13 +24,13 @@ int __libc_open64 (const char *file, int oflag, ...) { - int mode = 0; + mode_t mode = 0; if (oflag & O_CREAT) { va_list arg; va_start (arg, oflag); - mode = va_arg (arg, int); + mode = va_arg (arg, mode_t); va_end (arg); } diff --git a/sysdeps/unix/sysv/linux/generic/open.c b/sysdeps/unix/sysv/linux/generic/open.c index 4f73fa0..d243933 100644 --- a/sysdeps/unix/sysv/linux/generic/open.c +++ b/sysdeps/unix/sysv/linux/generic/open.c @@ -27,13 +27,13 @@ int __libc_open (const char *file, int oflag, ...) { - int mode = 0; + mode_t mode = 0; if (oflag & O_CREAT) { va_list arg; va_start (arg, oflag); - mode = va_arg (arg, int); + mode = va_arg (arg, mode_t); va_end (arg); } @@ -57,13 +57,13 @@ weak_alias (__libc_open, open) int __open_nocancel (const char *file, int oflag, ...) { - int mode = 0; + mode_t mode = 0; if (oflag & O_CREAT) { va_list arg; va_start (arg, oflag); - mode = va_arg (arg, int); + mode = va_arg (arg, mode_t); va_end (arg); } diff --git a/sysdeps/unix/sysv/linux/generic/open64.c b/sysdeps/unix/sysv/linux/generic/open64.c index 93d79e3..e367e12 100644 --- a/sysdeps/unix/sysv/linux/generic/open64.c +++ b/sysdeps/unix/sysv/linux/generic/open64.c @@ -27,13 +27,13 @@ int __libc_open64 (const char *file, int oflag, ...) { - int mode = 0; + mode_t mode = 0; if (oflag & O_CREAT) { va_list arg; va_start (arg, oflag); - mode = va_arg (arg, int); + mode = va_arg (arg, mode_t); va_end (arg); } diff --git a/sysdeps/unix/sysv/linux/open64.c b/sysdeps/unix/sysv/linux/open64.c index 0d63806..442a2ef 100644 --- a/sysdeps/unix/sysv/linux/open64.c +++ b/sysdeps/unix/sysv/linux/open64.c @@ -26,13 +26,13 @@ int __libc_open64 (const char *file, int oflag, ...) { - int mode = 0; + mode_t mode = 0; if (oflag & O_CREAT) { va_list arg; va_start (arg, oflag); - mode = va_arg (arg, int); + mode = va_arg (arg, mode_t); va_end (arg); }
The sole varargs argument to open and friends has type mode_t, not int. Signed-off-by: Rasmus Villemoes <rv@rasmusvillemoes.dk> --- io/open.c | 4 ++-- io/open64.c | 4 ++-- io/openat.c | 4 ++-- io/openat64.c | 4 ++-- sysdeps/posix/open64.c | 4 ++-- sysdeps/unix/sysv/linux/generic/open.c | 8 ++++---- sysdeps/unix/sysv/linux/generic/open64.c | 4 ++-- sysdeps/unix/sysv/linux/open64.c | 4 ++-- 8 files changed, 18 insertions(+), 18 deletions(-)