| Message ID | CACysShi-izXNj=Dkn3fvc3OhDyuDu6QBDPy_MXcYOBP=eP4mJw@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
Hi, On Tue, Aug 5, 2014 at 4:35 AM, Alexander Ivchenko <aivchenk@gmail.com> wrote: >> Testing for *android* is less than ideal, because of the possibility of >> configuring a *-linux* toolchain to have multilibs using various different >> C libraries (with -mandroid being used to select the Android multilib). >> So, specifying a bypass based on some relevant text that appears in the >> header would be better. >> >> -- >> Joseph S. Myers >> joseph@codesourcery.com > > I've added check for "BIONIC" keyword. Hopefully it won't disappear. "based on some relevant text" I think that's important, too (that it be relevant). "BIONIC" is just some improbable text you found in the header. My guess is that testing for '*android*' would be more selective, and certainly less obscure. Who would ever guess that "BIONIC" implies "android"? > Updated patch: > > diff --git a/fixincludes/ChangeLog b/fixincludes/ChangeLog > index f7effee..e05412e 100644 > --- a/fixincludes/ChangeLog > +++ b/fixincludes/ChangeLog > @@ -1,3 +1,10 @@ > +2014-08-04 Alexander Ivchenko <alexander.ivchenko@intel.com> > + > + * inclhack.def (stdio_va_list): Bypass fix for Bionic. > + (complier_h_tradcpp): Remove. > + * fixincl.x: Regenerate. > + * tests/base/linux/compiler.h: Remove. > + > 2014-04-22 Rainer Orth <ro@CeBiTec.Uni-Bielefeld.DE> > > * inclhack.def (math_exception): Bypass on *-*-solaris2.1[0-9]*. > diff --git a/fixincludes/fixincl.x b/fixincludes/fixincl.x [[generated text is not needed for approval]] > diff --git a/fixincludes/inclhack.def b/fixincludes/inclhack.def > index 6a1136c..bf452c6 100644 > --- a/fixincludes/inclhack.def > +++ b/fixincludes/inclhack.def > @@ -1140,20 +1140,6 @@ fix = { > }; > > /* > - * Old Linux kernel's <compiler.h> header breaks Traditional CPP > - */ > -fix = { > - hackname = complier_h_tradcpp; [[ OK ]] > @@ -3722,8 +3708,9 @@ fix = { > fix = { > hackname = stdio_va_list; > files = stdio.h; > - bypass = '__gnuc_va_list|_BSD_VA_LIST_|__DJ_va_list|_G_va_list'; > + bypass = '__gnuc_va_list|_BSD_VA_LIST_|__DJ_va_list|_G_va_list|BIONIC'; > /* > + * In Bionic va_list is always appropriately typedefed to __gnuc_va_list. And that typedef does not live in stdio.h either. If there is no better way to identify this file, then > Is it ok? It is "okay". (You may be left with little choice -- I can't see the header :).
On Tue, Aug 5, 2014 at 7:10 AM, Bruce Korb <bkorb@gnu.org> wrote: > Hi, > > On Tue, Aug 5, 2014 at 4:35 AM, Alexander Ivchenko <aivchenk@gmail.com> wrote: >>> Testing for *android* is less than ideal, because of the possibility of >>> configuring a *-linux* toolchain to have multilibs using various different >>> C libraries (with -mandroid being used to select the Android multilib). >>> So, specifying a bypass based on some relevant text that appears in the >>> header would be better. >>> >>> -- >>> Joseph S. Myers >>> joseph@codesourcery.com >> >> I've added check for "BIONIC" keyword. Hopefully it won't disappear. > > "based on some relevant text" > I think that's important, too (that it be relevant). > "BIONIC" is just some improbable text you found in the header. > My guess is that testing for '*android*' would be more selective, > and certainly less obscure. Who would ever guess that > "BIONIC" implies "android"? > >> Updated patch: >> >> diff --git a/fixincludes/ChangeLog b/fixincludes/ChangeLog >> index f7effee..e05412e 100644 >> --- a/fixincludes/ChangeLog >> +++ b/fixincludes/ChangeLog >> @@ -1,3 +1,10 @@ >> +2014-08-04 Alexander Ivchenko <alexander.ivchenko@intel.com> >> + >> + * inclhack.def (stdio_va_list): Bypass fix for Bionic. >> + (complier_h_tradcpp): Remove. >> + * fixincl.x: Regenerate. >> + * tests/base/linux/compiler.h: Remove. >> + >> 2014-04-22 Rainer Orth <ro@CeBiTec.Uni-Bielefeld.DE> >> >> * inclhack.def (math_exception): Bypass on *-*-solaris2.1[0-9]*. >> diff --git a/fixincludes/fixincl.x b/fixincludes/fixincl.x > > [[generated text is not needed for approval]] > >> diff --git a/fixincludes/inclhack.def b/fixincludes/inclhack.def >> index 6a1136c..bf452c6 100644 >> --- a/fixincludes/inclhack.def >> +++ b/fixincludes/inclhack.def >> @@ -1140,20 +1140,6 @@ fix = { >> }; >> >> /* >> - * Old Linux kernel's <compiler.h> header breaks Traditional CPP >> - */ >> -fix = { >> - hackname = complier_h_tradcpp; > > [[ OK ]] > >> @@ -3722,8 +3708,9 @@ fix = { >> fix = { >> hackname = stdio_va_list; >> files = stdio.h; >> - bypass = '__gnuc_va_list|_BSD_VA_LIST_|__DJ_va_list|_G_va_list'; >> + bypass = '__gnuc_va_list|_BSD_VA_LIST_|__DJ_va_list|_G_va_list|BIONIC'; >> /* >> + * In Bionic va_list is always appropriately typedefed to __gnuc_va_list. > > And that typedef does not live in stdio.h either. > If there is no better way to identify this file, then > >> Is it ok? > > It is "okay". (You may be left with little choice -- I can't see the header :). you can see the current version of bionic's stdio.h here: https://android.googlesource.com/platform/bionic/+/master/libc/include/stdio.h i'm happy to add any string to the header file that makes things easier. if you want 'x-gcc-no-fixincludes' or whatever in there, just say :-) (you're correct that the string 'BIONIC' is currently there only as a side-effect; our FORTIFY_SOURCE implementation uses a __BIONIC_FORTIFY_INLINE macro.)
Hi, On Tue, Aug 5, 2014 at 10:36 AM, enh <enh@google.com> wrote: > you can see the current version of bionic's stdio.h here: > > https://android.googlesource.com/platform/bionic/+/master/libc/include/stdio.h > > i'm happy to add any string to the header file that makes things > easier. if you want 'x-gcc-no-fixincludes' or whatever in there, just > say :-) That would be great, but you could also add: /* this file depends on __gnuc_va_list being used for va_list */ and not bother changing fixincludes at all. :) But either of those two comments added to the header would be preferable to looking for "BIONIC". Thank you! With one of the two changes, the patch is approved. Thanks!
does https://android-review.googlesource.com/103445 look okay? On Tue, Aug 5, 2014 at 12:01 PM, Bruce Korb <bkorb@gnu.org> wrote: > Hi, > > On Tue, Aug 5, 2014 at 10:36 AM, enh <enh@google.com> wrote: >> you can see the current version of bionic's stdio.h here: >> >> https://android.googlesource.com/platform/bionic/+/master/libc/include/stdio.h >> >> i'm happy to add any string to the header file that makes things >> easier. if you want 'x-gcc-no-fixincludes' or whatever in there, just >> say :-) > > That would be great, but you could also add: > > /* this file depends on __gnuc_va_list being used for va_list */ > > and not bother changing fixincludes at all. :) But either of those two > comments added to the header would be preferable to looking for "BIONIC". > Thank you! > > With one of the two changes, the patch is approved. Thanks!
Hi, Lines 42 & 43 are not needed for fixincludes, but it is your choice. With that change, you should not need to add that test to fixincludes because __gnuc_va_list will be found within the comment and satisfy the "bypass" expression. That was the long way of saying: Looks good to me. On Tue, Aug 5, 2014 at 5:09 PM, enh <enh@google.com> wrote: > does https://android-review.googlesource.com/103445 look okay? > > On Tue, Aug 5, 2014 at 12:01 PM, Bruce Korb <bkorb@gnu.org> wrote: >> Hi, >> >> On Tue, Aug 5, 2014 at 10:36 AM, enh <enh@google.com> wrote: >>> you can see the current version of bionic's stdio.h here: >>> >>> https://android.googlesource.com/platform/bionic/+/master/libc/include/stdio.h >>> >>> i'm happy to add any string to the header file that makes things >>> easier. if you want 'x-gcc-no-fixincludes' or whatever in there, just >>> say :-) >> >> That would be great, but you could also add: >> >> /* this file depends on __gnuc_va_list being used for va_list */ >> >> and not bother changing fixincludes at all. :) But either of those two >> comments added to the header would be preferable to looking for "BIONIC". >> Thank you! >> >> With one of the two changes, the patch is approved. Thanks!
On Tue, Aug 5, 2014 at 5:26 PM, Bruce Korb <bkorb@gnu.org> wrote: > Hi, > > Lines 42 & 43 are not needed for fixincludes, but it is your choice. > With that change, you should not need to add that test to fixincludes > because __gnuc_va_list will be found within the comment and satisfy > the "bypass" expression. okay, i'll reword to explicitly say that it's the reference to __gnuc_va_list that gets us the fixincludes behavior we want (which should also ensure that no one "cleans up" the reference to, say, __builtin_va_list). > That was the long way of saying: > Looks good to me. > > On Tue, Aug 5, 2014 at 5:09 PM, enh <enh@google.com> wrote: >> does https://android-review.googlesource.com/103445 look okay? >> >> On Tue, Aug 5, 2014 at 12:01 PM, Bruce Korb <bkorb@gnu.org> wrote: >>> Hi, >>> >>> On Tue, Aug 5, 2014 at 10:36 AM, enh <enh@google.com> wrote: >>>> you can see the current version of bionic's stdio.h here: >>>> >>>> https://android.googlesource.com/platform/bionic/+/master/libc/include/stdio.h >>>> >>>> i'm happy to add any string to the header file that makes things >>>> easier. if you want 'x-gcc-no-fixincludes' or whatever in there, just >>>> say :-) >>> >>> That would be great, but you could also add: >>> >>> /* this file depends on __gnuc_va_list being used for va_list */ >>> >>> and not bother changing fixincludes at all. :) But either of those two >>> comments added to the header would be preferable to looking for "BIONIC". >>> Thank you! >>> >>> With one of the two changes, the patch is approved. Thanks!
diff --git a/fixincludes/ChangeLog b/fixincludes/ChangeLog index f7effee..e05412e 100644 --- a/fixincludes/ChangeLog +++ b/fixincludes/ChangeLog @@ -1,3 +1,10 @@ +2014-08-04 Alexander Ivchenko <alexander.ivchenko@intel.com> + + * inclhack.def (stdio_va_list): Bypass fix for Bionic. + (complier_h_tradcpp): Remove. + * fixincl.x: Regenerate. + * tests/base/linux/compiler.h: Remove. + 2014-04-22 Rainer Orth <ro@CeBiTec.Uni-Bielefeld.DE> * inclhack.def (math_exception): Bypass on *-*-solaris2.1[0-9]*. diff --git a/fixincludes/fixincl.x b/fixincludes/fixincl.x index dd45802..d555c51 100644 --- a/fixincludes/fixincl.x +++ b/fixincludes/fixincl.x @@ -2,11 +2,11 @@ * * DO NOT EDIT THIS FILE (fixincl.x) * - * It has been AutoGen-ed Tuesday January 7, 2014 at 12:02:54 PM MET + * It has been AutoGen-ed August 5, 2014 at 02:53:14 PM by AutoGen 5.12 * From the definitions inclhack.def * and the template file fixincl */ -/* DO NOT SVN-MERGE THIS FILE, EITHER Tue Jan 7 12:02:54 MET 2014 +/* DO NOT SVN-MERGE THIS FILE, EITHER Tue Aug 5 14:53:14 MSK 2014 * * You must regenerate it. Use the ./genfixes script. * @@ -15,7 +15,7 @@ * certain ANSI-incompatible system header files which are fixed to work * correctly with ANSI C and placed in a directory that GNU C will search. * - * This file contains 224 fixup descriptions. + * This file contains 223 fixup descriptions. * * See README for more information. * @@ -2111,41 +2111,6 @@ int vfscanf(FILE *, const char *, __builtin_va_list) __asm__ (_BSD_STRING(__USER /* * * * * * * * * * * * * * * * * * * * * * * * * * * - * Description of Complier_H_Tradcpp fix - */ -tSCC zComplier_H_TradcppName[] = - "complier_h_tradcpp"; - -/* - * File name selection pattern - */ -tSCC zComplier_H_TradcppList[] = - "linux/compiler.h\0"; -/* - * Machine/OS name selection pattern - */ -#define apzComplier_H_TradcppMachs (const char**)NULL - -/* - * content selection pattern - do fix if pattern found - */ -tSCC zComplier_H_TradcppSelect0[] = - "#define __builtin_warning\\(x, y\\.\\.\\.\\) \\(1\\)"; - -#define COMPLIER_H_TRADCPP_TEST_CT 1 -static tTestDesc aComplier_H_TradcppTests[] = { - { TT_EGREP, zComplier_H_TradcppSelect0, (regex_t*)NULL }, }; - -/* - * Fix Command Arguments for Complier_H_Tradcpp - */ -static const char* apzComplier_H_TradcppPatch[] = { - "format", - "/* __builtin_warning(x, y...) is obsolete */", - (char*)NULL }; - -/* * * * * * * * * * * * * * * * * * * * * * * * * * - * * Description of Ctrl_Quotes_Def fix */ tSCC zCtrl_Quotes_DefName[] = @@ -7234,7 +7199,7 @@ tSCC* apzStdio_Va_ListMachs[] = { * content bypass pattern - skip fix if pattern found */ tSCC zStdio_Va_ListBypass0[] = - "__gnuc_va_list|_BSD_VA_LIST_|__DJ_va_list|_G_va_list"; + "__gnuc_va_list|_BSD_VA_LIST_|__DJ_va_list|_G_va_list|BIONIC"; #define STDIO_VA_LIST_TEST_CT 1 static tTestDesc aStdio_Va_ListTests[] = { @@ -9187,9 +9152,9 @@ static const char* apzX11_SprintfPatch[] = { * * List of all fixes */ -#define REGEX_COUNT 261 +#define REGEX_COUNT 260 #define MACH_LIST_SIZE_LIMIT 187 -#define FIX_COUNT 224 +#define FIX_COUNT 223 /* * Enumerate the fixes @@ -9242,7 +9207,6 @@ typedef enum { BROKEN_CABS_FIXIDX, BROKEN_NAN_FIXIDX, BSD_STDIO_ATTRS_CONFLICT_FIXIDX, - COMPLIER_H_TRADCPP_FIXIDX, CTRL_QUOTES_DEF_FIXIDX, CTRL_QUOTES_USE_FIXIDX, CXX_UNREADY_FIXIDX, @@ -9657,11 +9621,6 @@ tFixDesc fixDescList[ FIX_COUNT ] = { BSD_STDIO_ATTRS_CONFLICT_TEST_CT, FD_MACH_ONLY | FD_SUBROUTINE, aBsd_Stdio_Attrs_ConflictTests, apzBsd_Stdio_Attrs_ConflictPatch, 0 }, - { zComplier_H_TradcppName, zComplier_H_TradcppList, - apzComplier_H_TradcppMachs, - COMPLIER_H_TRADCPP_TEST_CT, FD_MACH_ONLY | FD_SUBROUTINE, - aComplier_H_TradcppTests, apzComplier_H_TradcppPatch, 0 }, - { zCtrl_Quotes_DefName, zCtrl_Quotes_DefList, apzCtrl_Quotes_DefMachs, CTRL_QUOTES_DEF_TEST_CT, FD_MACH_ONLY | FD_SUBROUTINE, diff --git a/fixincludes/inclhack.def b/fixincludes/inclhack.def index 6a1136c..bf452c6 100644 --- a/fixincludes/inclhack.def +++ b/fixincludes/inclhack.def @@ -1140,20 +1140,6 @@ fix = { }; /* - * Old Linux kernel's <compiler.h> header breaks Traditional CPP - */ -fix = { - hackname = complier_h_tradcpp; - files = linux/compiler.h; - - select = "#define __builtin_warning\\(x, y\\.\\.\\.\\) \\(1\\)"; - c_fix = format; - c_fix_arg = "/* __builtin_warning(x, y...) is obsolete */"; - - test_text = "#define __builtin_warning(x, y...) (1)"; -}; - -/* * Fix various macros used to define ioctl numbers. * The traditional syntax was: * @@ -3722,8 +3708,9 @@ fix = { fix = { hackname = stdio_va_list; files = stdio.h; - bypass = '__gnuc_va_list|_BSD_VA_LIST_|__DJ_va_list|_G_va_list'; + bypass = '__gnuc_va_list|_BSD_VA_LIST_|__DJ_va_list|_G_va_list|BIONIC'; /* + * In Bionic va_list is always appropriately typedefed to __gnuc_va_list. * On Solaris 10, the definition in * <stdio.h> is guarded appropriately by the _XPG4 feature macro; * there is therefore no need for this fix there. diff --git a/fixincludes/tests/base/linux/compiler.h b/fixincludes/tests/base/linux/compiler.h deleted file mode 100644 index 7135276..0000000 --- a/fixincludes/tests/base/linux/compiler.h +++ /dev/null @@ -1,14 +0,0 @@ -/* DO NOT EDIT THIS FILE. - - It has been auto-edited by fixincludes from: - - "fixinc/tests/inc/linux/compiler.h" - - This had to be done to correct non-standard usages in the - original, manufacturer supplied header file. */ - - - -#if defined( COMPLIER_H_TRADCPP_CHECK ) -/* __builtin_warning(x, y...) is obsolete */ -#endif /* COMPLIER_H_TRADCPP_CHECK */