| Message ID | 50357EC8.3000709@gnu.org |
|---|---|
| State | New |
| Headers | show |
On 8/22/2012 8:52 PM, Bruce Korb wrote: > On 08/22/12 17:05, rbmj wrote: >> Hello Everyone, >> >> I have ten patches which are approved or obvious but waiting on commit > > > The include fixing stuff looks fine to me. > However I think it might be simpler to tweak mkfixinc.sh to > > sed '/if test -s .{MACRO_LIST}/s/$/ && false/' \ > ${srcdir}/fixinc.in > ${target} > > for vxworks rather than all that configury rigmarole. > That would eliminate changes to gcc/configure.ac and gcc/Makefile.in. I didn't even think of that. I was probably doing all my fixes in the Makefile because that was where I was looking with the original problem. However, I'm looking at the result of that sed command, and I don't think that the && is going to work, as when I run that sed command I just get whitespace there. IIRC, sed handles '&' differently so it probably needs to be escaped. I'll see if I get a chance to test that later on. > Basically degrading the entire first patch to this: > > $ svn diff mkfixinc.sh > Index: mkfixinc.sh > =================================================================== > > 1 == '-u' > 2 == '-L' > 3 == 'mkfixinc.sh (revision 190448)' > 4 == '-L' > 5 == 'mkfixinc.sh (working copy)' > 6 == '.svn/text-base/mkfixinc.sh.svn-base' > 7 == 'mkfixinc.sh' > --- mkfixinc.sh (revision 190448) > +++ mkfixinc.sh (working copy) > @@ -15,7 +15,6 @@ > i?86-*-mingw32* | \ > x86_64-*-mingw32* | \ > i?86-*-interix* | \ > - *-*-vxworks* | \ > powerpc-*-eabisim* | \ > powerpc-*-eabi* | \ > powerpc-*-rtems* | \ > @@ -26,6 +25,11 @@ > (echo "#! /bin/sh" ; echo "exit 0" ) > ${target} > ;; > > + *-*-vxworks* ) > + sed '/if test -s .{MACRO_LIST}/s/$/ && false/' \ > + ${srcdir}/fixinc.in > ${target} > + ;; > + > *) > cat < ${srcdir}/fixinc.in > ${target} || exit 1 > ;; > > Assuming that when I test everything works and I can get a successful build that works for me. Probably won't get a chance to test as I don't have very much time until the weekend and right now there's another regression in rs6000 (issue with one of the macros) that's breaking that I have to fix before I get to that part of the build process. Thanks for your input. That's certainly a simpler fix. -- Robert Mason
Il 23/08/2012 04:27, rbmj ha scritto: >> >> sed '/if test -s .{MACRO_LIST}/s/$/ && false/' \ >> ${srcdir}/fixinc.in > ${target} >> >> for vxworks rather than all that configury rigmarole. >> That would eliminate changes to gcc/configure.ac and gcc/Makefile.in. > > I didn't even think of that. I was probably doing all my fixes in the > Makefile because that was where I was looking with the original > problem. However, I'm looking at the result of that sed command, and I > don't think that the && is going to work, as when I run that sed command > I just get whitespace there. IIRC, sed handles '&' differently so it > probably needs to be escaped. I'll see if I get a chance to test that > later on. Yes, & is equivalent to \0. You need \&\& instead. Some comments on the patches: > + c_fix_arg = "%0\n" > + "#define ioctl(fd, func, arg) ((ioctl)((fd), (func), ((int)(arg))))\n"; This can be simply #define ioctl(fd, func, arg) ioctl(fd, func, (int)arg) thanks to C and cpp precedence rules. > > + c_fix_arg = "%0\n" > + "#ifdef IN_GCC\n" > + "#define mkdir(dir, mode) ((mode), (mkdir)(dir))\n" > + "#endif\n"; Are you sure about the #ifdef/#endif? In fact, you definitely do not want a _global_ include to have a dependency on a user symbol. > Add fix to make write() const correct on VxWorks > > VxWorks' write() takes its second argument as non-const, so the > compiler complains if one tries to pass a const pointer to it. I think this does not need to be VxWorks-specific, but I'm not sure of the standards for fixincludes. Bruce? > Subject: [PATCH 10/10] Make open() call more compatible in gcc/gcov-io.c > > In gcc/gcov-io.c, the call to open() only has two arguments. This > is fine, as long as the system open() is standards compliant. So you have to add another fixincludes hack, adding a macro indirection like the one you have for ioctl: #define open(a, b, ...) __open(a, b , ##__VA_ARGS__, 0660) #define __open(a, b, c, ...) (open)(a, b, c) Please make these adjustments and resubmit. Thanks! Paolo
On 23 August 2012 09:24, Paolo Bonzini <bonzini@gnu.org> wrote: > Il 23/08/2012 04:27, rbmj ha scritto: >> + c_fix_arg = "%0\n" >> + "#define ioctl(fd, func, arg) ((ioctl)((fd), (func), ((int)(arg))))\n"; > > This can be simply > > #define ioctl(fd, func, arg) ioctl(fd, func, (int)arg) "(int)(arg)", surely. Jay.
Il 23/08/2012 10:35, Jay Foad ha scritto: >> > This can be simply >> > >> > #define ioctl(fd, func, arg) ioctl(fd, func, (int)arg) > "(int)(arg)", surely. Right, only the other parentheses are unnecessary. (Cut-and-paste mistake). Paolo
On 8/23/2012 4:24 AM, Paolo Bonzini wrote: > Some comments on the patches: > >> + c_fix_arg = "%0\n" >> + "#define ioctl(fd, func, arg) ((ioctl)((fd), (func), ((int)(arg))))\n"; > This can be simply > > #define ioctl(fd, func, arg) ioctl(fd, func, (int)arg) > > thanks to C and cpp precedence rules. OK. I just over-parenthesize all function-macros by default because I can never remember the rules exactly and worry about not anticipating what someone puts in a macro. >> + c_fix_arg = "%0\n" >> + "#ifdef IN_GCC\n" >> + "#define mkdir(dir, mode) ((mode), (mkdir)(dir))\n" >> + "#endif\n"; > Are you sure about the #ifdef/#endif? In fact, you definitely do not > want a _global_ include to have a dependency on a user symbol. The idea is I don't want to break existing code, so I only want this macro to take effect inside of GCC proper, as (AFAIK) anything built with the new compiler gets the fixed includes, and there's lots of VxWorks code that relies on single-argument mkdir. The alternative is varaidic macros, but I'm no expert on the nuances of c89 vs c99 variadic macros. > >> Add fix to make write() const correct on VxWorks >> >> VxWorks' write() takes its second argument as non-const, so the >> compiler complains if one tries to pass a const pointer to it. > I think this does not need to be VxWorks-specific, but I'm not sure of > the standards for fixincludes. Bruce? I set it to only vxworks for now as that's the only platform that I 1. know has the issue and 2. know that it's safe to do this change. > >> Subject: [PATCH 10/10] Make open() call more compatible in gcc/gcov-io.c >> >> In gcc/gcov-io.c, the call to open() only has two arguments. This >> is fine, as long as the system open() is standards compliant. > So you have to add another fixincludes hack, adding a macro indirection > like the one you have for ioctl: > > #define open(a, b, ...) __open(a, b , ##__VA_ARGS__, 0660) > #define __open(a, b, c, ...) (open)(a, b, c) Again, just not sure about variadic macro compatibility. If that will work for both c89 and c99 and c++, then that looks good to me. -- Robert Mason
Il 23/08/2012 13:46, rbmj ha scritto: > On 8/23/2012 4:24 AM, Paolo Bonzini wrote: >> Some comments on the patches: >> >>> + c_fix_arg = "%0\n" >>> + "#define ioctl(fd, func, arg) ((ioctl)((fd), (func), >>> ((int)(arg))))\n"; >> This can be simply >> >> #define ioctl(fd, func, arg) ioctl(fd, func, (int)arg) >> >> thanks to C and cpp precedence rules. > > OK. I just over-parenthesize all function-macros by default because I > can never remember the rules exactly and worry about not anticipating > what someone puts in a macro. >>> + c_fix_arg = "%0\n" >>> + "#ifdef IN_GCC\n" >>> + "#define mkdir(dir, mode) ((mode), (mkdir)(dir))\n" >>> + "#endif\n"; >> Are you sure about the #ifdef/#endif? In fact, you definitely do not >> want a _global_ include to have a dependency on a user symbol. > The idea is I don't want to break existing code, so I only want this > macro to take effect inside of GCC proper, as (AFAIK) anything built > with the new compiler gets the fixed includes, and there's lots of > VxWorks code that relies on single-argument mkdir. > The alternative is varaidic macros, but I'm no expert on the nuances of > c89 vs c99 variadic macros. Something like this should work: #define mkdir(dir, ...) ((void)0, ##__VA_ARGS, (mkdir)(dir)) >>> Subject: [PATCH 10/10] Make open() call more compatible in gcc/gcov-io.c >>> >>> In gcc/gcov-io.c, the call to open() only has two arguments. This >>> is fine, as long as the system open() is standards compliant. >> So you have to add another fixincludes hack, adding a macro indirection >> like the one you have for ioctl: >> >> #define open(a, b, ...) __open(a, b , ##__VA_ARGS__, 0660) >> #define __open(a, b, c, ...) (open)(a, b, c) > > Again, just not sure about variadic macro compatibility. If that will > work for both c89 and c99 and c++, then that looks good to me. Yes, GCC has variadic macros as an extension in C89 mode too. You need to experiment a bit with -pedantic and/or -ansi and/or -std=c89, though. Paolo
> In gcc/gcov-io.c, the call to open() only has two arguments. This > is fine, as long as the system open() is standards compliant. >>> So you have to add another fixincludes hack, adding a macro indirection >>> like the one you have for ioctl: >>> >>> #define open(a, b, ...) __open(a, b , ##__VA_ARGS__, 0660) >>> #define __open(a, b, c, ...) (open)(a, b, c) Also forgot to note: I've seen passing the extra argument unconditionally (even though it's for a read-only open) other places in GCC sources, so that seems to be accepted practice. -- Robert Mason
Il 23/08/2012 21:37, rbmj ha scritto: >> In gcc/gcov-io.c, the call to open() only has two arguments. This >> is fine, as long as the system open() is standards compliant. >>>> So you have to add another fixincludes hack, adding a macro indirection >>>> like the one you have for ioctl: >>>> >>>> #define open(a, b, ...) __open(a, b , ##__VA_ARGS__, 0660) >>>> #define __open(a, b, c, ...) (open)(a, b, c) > Also forgot to note: I've seen passing the extra argument > unconditionally (even though it's for a read-only open) other places in > GCC sources, so that seems to be accepted practice. It doesn't really seem to be the case, they look more like cut-and-paste: With: ./c-family/c-pch.c: fd = open (name, O_RDONLY | O_BINARY, 0666); ./mips-tfile.c: in_fd = open (object_name, O_RDONLY, 0666); ./gcc.c: desc = open (filename, O_RDONLY, 0); Without: ./mips-tdump.c: tfile_fd = open (argv[optind], O_RDONLY); ./ggc-page.c: G.dev_zero_fd = open ("/dev/zero", O_RDONLY); ./lto/lto.c: fd = open (file_data->file_name, O_RDONLY|O_BINARY); ./ggc-zone.c: G.dev_zero_fd = open ("/dev/zero", O_RDONLY); ./gcov-io.c: fd = open (name, O_RDONLY); ./collect2-aix.c: ldfile->fd = open (filename, O_RDONLY); ./java/resource.c: fd = open (filename, O_RDONLY | O_BINARY); ./java/win32-host.c: return open (filename, oflag); ./java/jcf-io.c: fd = open (zipfile, O_RDONLY | O_BINARY); ./java/jcf-io.c: int fd = open (filename, O_RDONLY | O_BINARY); ./gcc.c: int fd = open (cmpfile[i], O_RDONLY); ./config/rs6000/driver-rs6000.c: fd = open ("/proc/self/auxv", O_RDONLY); ./config/rs6000/driver-rs6000.c: fd = open ("/proc/self/auxv", O_RDONLY); ./config/alpha/host-osf.c: procfd = open (pname, O_RDONLY); Paolo
On 8/22/2012 8:52 PM, Bruce Korb wrote: > However I think it might be simpler to tweak mkfixinc.sh to > > sed '/if test -s .{MACRO_LIST}/s/$/ && false/' \ > ${srcdir}/fixinc.in > ${target} > > for vxworks rather than all that configury rigmarole. > That would eliminate changes to gcc/configure.ac and gcc/Makefile.in. OK. One question though: Why not just have a case statement inside fixinc.in? Just running the script through sed seems like a kludgier solution. e.g. case ${target_canonical} in *-*-vxworks*) # Disable the machine name fix as it breaks things machine_name_override='OVERRIDE' ;; esac if test -s ${MACRO_LIST} && test -z "${machine_name_override}" -- Robert Mason
On 08/24/12 11:50, rbmj wrote: > On 8/22/2012 8:52 PM, Bruce Korb wrote: >> However I think it might be simpler to tweak mkfixinc.sh to >> >> sed '/if test -s .{MACRO_LIST}/s/$/ && false/' \ >> ${srcdir}/fixinc.in > ${target} >> >> for vxworks rather than all that configury rigmarole. >> That would eliminate changes to gcc/configure.ac and gcc/Makefile.in. > OK. One question though: Why not just have a case statement inside fixinc.in? > e.g. > > case ${target_canonical} in > *-*-vxworks*) > # Disable the machine name fix as it breaks things > machine_name_override='OVERRIDE' > ;; > esac > > if test -s ${MACRO_LIST} && test -z "${machine_name_override}" There are many ways of accomplishing the same thing. I just thought of the fact that the mkfixinc.sh script constructed the result script either by spinning it from thin air or by copying a source file, so why not just edit that source file when it wasn't doing the right thing? Your solution is fine, too. You could also make the sed expression simpler: sed '/^if test -s .{MACRO_LIST}/s/if .*/if false/' sed '/^if test -s .{MACRO_LIST}/,/^fi/d' or replace ``machine_name_override='OVERRIDE'' with ``test -f ${MACRO_LIST} && rm -f ${MACRO_LIST}'' I will approve any reasonably simple solution. :) Thank you for doing this, by the way! Regards, Bruce
On 8/23/2012 7:54 AM, Paolo Bonzini wrote: > Il 23/08/2012 13:46, rbmj ha scritto: >> On 8/23/2012 4:24 AM, Paolo Bonzini wrote: >>>> Subject: [PATCH 10/10] Make open() call more compatible in gcc/gcov-io.c >>>> >>>> In gcc/gcov-io.c, the call to open() only has two arguments. This >>>> is fine, as long as the system open() is standards compliant. >>> So you have to add another fixincludes hack, adding a macro indirection >>> like the one you have for ioctl: >>> >>> #define open(a, b, ...) __open(a, b , ##__VA_ARGS__, 0660) >>> #define __open(a, b, c, ...) (open)(a, b, c) >> >> Again, just not sure about variadic macro compatibility. If that will >> work for both c89 and c99 and c++, then that looks good to me. > > Yes, GCC has variadic macros as an extension in C89 mode too. You need > to experiment a bit with -pedantic and/or -ansi and/or -std=c89, though. > So the variadic macros work for compiling GCC itself. However, I run into problems when compiling libstdc++-v3. The problem is that basic_file.cc defines __basic_file<char>::open(), and the macro is substituting for this as well. So AFAICT the original solution (just passing unconditionally) is necessary. I don't see any pitfalls associated with this - do we really care *that* much about passing one extra int? Though it looks weird, it's clearly not unprecedented (as you said, it's not the rule, but it has certainly been done in other places). I don't see a way to use a macro that will not break the declaration. Is there a way that a macro can work that I'm missing? -- rbmj
Index: mkfixinc.sh =================================================================== 1 == '-u' 2 == '-L' 3 == 'mkfixinc.sh (revision 190448)' 4 == '-L' 5 == 'mkfixinc.sh (working copy)' 6 == '.svn/text-base/mkfixinc.sh.svn-base' 7 == 'mkfixinc.sh' --- mkfixinc.sh (revision 190448) +++ mkfixinc.sh (working copy) @@ -15,7 +15,6 @@ i?86-*-mingw32* | \ x86_64-*-mingw32* | \ i?86-*-interix* | \ - *-*-vxworks* | \ powerpc-*-eabisim* | \ powerpc-*-eabi* | \ powerpc-*-rtems* | \ @@ -26,6 +25,11 @@ (echo "#! /bin/sh" ; echo "exit 0" ) > ${target} ;; + *-*-vxworks* ) + sed '/if test -s .{MACRO_LIST}/s/$/ && false/' \ + ${srcdir}/fixinc.in > ${target} + ;; + *) cat < ${srcdir}/fixinc.in > ${target} || exit 1 ;;