Re: Preventing ISO C errors when using macros for builtin types
On Tue, 11 Jun 2019 18:01:55 -0500 Segher Boessenkool wrote: > On Tue, Jun 11, 2019 at 09:44:30PM +0100, Jozef Lawrynowicz wrote: > > --- a/gcc/lto/lto-lang.c > > +++ b/gcc/lto/lto-lang.c > > @@ -1260,9 +1260,9 @@ lto_build_c_type_nodes (void) > > if (int_n_enabled_p[i]) > > { > > char name[50]; > > - sprintf (name, "__int%d unsigned", int_n_data[i].bitsize); > > + sprintf (name, "int%d", int_n_data[i].bitsize); > > > > - if (strcmp (name, SIZE_TYPE) == 0) > > + if (strstr (SIZE_TYPE, name) != NULL) > > { > > intmax_type_node = int_n_trees[i].signed_type; > > uintmax_type_node = int_n_trees[i].unsigned_type; > > I don't think that is correct, strstr allows too much? If you want to > allow some variants, you should test for all those variants exactly? Yeah I'll fix this up in my full patch submission. > It looks great otherwise :-) > > > Segher Great, thanks for all your help! Jozef
Re: Preventing ISO C errors when using macros for builtin types
Hi Jozef, On Tue, Jun 11, 2019 at 09:44:30PM +0100, Jozef Lawrynowicz wrote: > Thanks for the pointers, they've put me on the right track I think. > > It doesn't look like we can create the new type in the msp430 backend - the > SIZE_TYPE macro is examined quite early and only a couple of basic backend > initialization functions are called before SIZE_TYPE is needed in > c_common_nodes_and_builtins(). TARGET_INIT_BUILTINS seemed most appropriate, > but by then it's too late to create the type and use it in msp430.h. > > Instead I fixed it by creating a new type "__int20__" in the middle-end, > which can then be used for SIZE_TYPE in msp430.h. > __int20__ is not really a proper type - it shares it's "RID" values with > __int20, but it means we can gate the ISO C pedwarn so it only is emitted for > __int20 and not __int20__. Ooh I like this :-) Fits in well everywhere, and it's nicely generic, too. > It's ok for __int20 and __int20__ to have identical > behavior, aside from the pedwarn, which is why sharing the RIDs should be ok. Many other keywords do the same, see asm/__asm/__asm__ for example. > I think this solution fits ok with the existing behavior related to "pedantic" > warnings, since alternate keywords beginning and ending > with "__" are considered GNU Extensions and don't pedwarn. I guess "__int20" > isn't technically a "keyword" in the same sense as "asm" for example. But > its a "reserved word" and this fix fits this pattern of surrounding with > double-underscores. > Any thoughts on this approach in my attached (rough) patch? So far I regtested > it fine with the GCC C testsuite for msp430-elf. > Also need to do the same for PTRDIFF_TYPE and make that use __int20__. > --- a/gcc/lto/lto-lang.c > +++ b/gcc/lto/lto-lang.c > @@ -1260,9 +1260,9 @@ lto_build_c_type_nodes (void) > if (int_n_enabled_p[i]) > { > char name[50]; > - sprintf (name, "__int%d unsigned", int_n_data[i].bitsize); > + sprintf (name, "int%d", int_n_data[i].bitsize); > > - if (strcmp (name, SIZE_TYPE) == 0) > + if (strstr (SIZE_TYPE, name) != NULL) > { > intmax_type_node = int_n_trees[i].signed_type; > uintmax_type_node = int_n_trees[i].unsigned_type; I don't think that is correct, strstr allows too much? If you want to allow some variants, you should test for all those variants exactly? It looks great otherwise :-) Segher
Re: Preventing ISO C errors when using macros for builtin types
On Mon, 10 Jun 2019 17:09:10 -0500 Segher Boessenkool wrote: > On Mon, Jun 10, 2019 at 08:58:00PM +0100, Jozef Lawrynowicz wrote: > > On Mon, 10 Jun 2019 13:32:42 -0500 > > Segher Boessenkool wrote: > > > That is not a fix, that is sweeping the problem under the rug. > > > > > > As a somewhat dirty hack I added > > > > > > #if __MSP430X_LARGE__ > > > #undef __SIZE_TYPE__ > > > __extension__ typedef unsigned __int20 __SIZE_TYPE__; > > > #endif > > > > > > to the start of the installed stddef.h, and that fixes the problem fine, > > > for correct programs that do not forget to include (directly > > > or indirectly), anyway. > > > But we have some 850 generic tests in gcc/testsuite that use __SIZE_TYPE__ > > without including stddef.h. They just rely on the preprocessor to expand > > this > > using the builtin macro definition. > > I did say it is a dirty hack, right? > > You could call lang_hooks.types.register_builtin_type defining the type > __SIZE_TYPE__ as the int20 partial int type, and then define SIZE_TYPE as > just __SIZE_TYPE__. That is the same effectively, just not using the > header file. > > So something like > > tree type_node = builtin_type_for_size (20, 1); > lang_hooks.types.register_builtin_type (type_node, "__SIZE_TYPE__"); > > or maybe > > tree type_node = builtin_type_for_size (POINTER_SIZE, 1); > lang_hooks.types.register_builtin_type (type_node, "__SIZE_TYPE__"); > > in msp430.c, and > > #define SIZE_TYPE "__SIZE_TYPE__" > > in msp430.h? > > > Segher Thanks for the pointers, they've put me on the right track I think. It doesn't look like we can create the new type in the msp430 backend - the SIZE_TYPE macro is examined quite early and only a couple of basic backend initialization functions are called before SIZE_TYPE is needed in c_common_nodes_and_builtins(). TARGET_INIT_BUILTINS seemed most appropriate, but by then it's too late to create the type and use it in msp430.h. Instead I fixed it by creating a new type "__int20__" in the middle-end, which can then be used for SIZE_TYPE in msp430.h. __int20__ is not really a proper type - it shares it's "RID" values with __int20, but it means we can gate the ISO C pedwarn so it only is emitted for __int20 and not __int20__. It's ok for __int20 and __int20__ to have identical behavior, aside from the pedwarn, which is why sharing the RIDs should be ok. I think this solution fits ok with the existing behavior related to "pedantic" warnings, since alternate keywords beginning and ending with "__" are considered GNU Extensions and don't pedwarn. I guess "__int20" isn't technically a "keyword" in the same sense as "asm" for example. But its a "reserved word" and this fix fits this pattern of surrounding with double-underscores. Any thoughts on this approach in my attached (rough) patch? So far I regtested it fine with the GCC C testsuite for msp430-elf. Also need to do the same for PTRDIFF_TYPE and make that use __int20__. I initially implemented this by giving the __intN__ types their own new set of RIDs so they are completely distinct from __intN, but it requires quite a lot of duplicated code whenever RID_INT_N_* or RID_{FIRST,LAST}_INT_N are used, just to handle the new RIDs. This patch is at least quite concise and gets the job done. Of course, if the __intN__ types really should have their own unique RIDs then I can do it that way instead. Thanks, Jozef diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c index 4057be3aaed..57e84b84f07 100644 --- a/gcc/c-family/c-common.c +++ b/gcc/c-family/c-common.c @@ -4024,8 +4024,14 @@ c_common_nodes_and_builtins (void) sprintf (name, "__int%d", int_n_data[i].bitsize); record_builtin_type ((enum rid)(RID_FIRST_INT_N + i), name, int_n_trees[i].signed_type); + sprintf (name, "__int%d__", int_n_data[i].bitsize); + record_builtin_type ((enum rid)(RID_FIRST_INT_N + i), name, + int_n_trees[i].signed_type); + sprintf (name, "__int%d unsigned", int_n_data[i].bitsize); record_builtin_type (RID_MAX, name, int_n_trees[i].unsigned_type); + sprintf (name, "__int%d__ unsigned", int_n_data[i].bitsize); + record_builtin_type (RID_MAX, name, int_n_trees[i].unsigned_type); } if (c_dialect_cxx ()) diff --git a/gcc/c/c-decl.c b/gcc/c/c-decl.c index 87ce853d4b7..cb2f49fa5a2 100644 --- a/gcc/c/c-decl.c +++ b/gcc/c/c-decl.c @@ -10637,7 +10637,11 @@ declspecs_add_type (location_t loc, struct c_declspecs *specs, case RID_INT_N_2: case RID_INT_N_3: specs->int_n_idx = i - RID_INT_N_0; - if (!in_system_header_at (input_location)) + if (!in_system_header_at (input_location) + /* If the INT_N type ends in "__", and so is of the format + "__intN__", don't pedwarn. */ + && (strncmp (IDENTIFIER_POINTER (type) + + (IDENTIFIER_LENGTH (type) - 2), "__", 2) != 0)) pedwarn (loc, OPT_Wpedantic, "ISO C does not support %<__int%d%> types",
Re: Preventing ISO C errors when using macros for builtin types
On Mon, Jun 10, 2019 at 08:58:00PM +0100, Jozef Lawrynowicz wrote: > On Mon, 10 Jun 2019 13:32:42 -0500 > Segher Boessenkool wrote: > > That is not a fix, that is sweeping the problem under the rug. > > > > As a somewhat dirty hack I added > > > > #if __MSP430X_LARGE__ > > #undef __SIZE_TYPE__ > > __extension__ typedef unsigned __int20 __SIZE_TYPE__; > > #endif > > > > to the start of the installed stddef.h, and that fixes the problem fine, > > for correct programs that do not forget to include (directly > > or indirectly), anyway. > But we have some 850 generic tests in gcc/testsuite that use __SIZE_TYPE__ > without including stddef.h. They just rely on the preprocessor to expand this > using the builtin macro definition. I did say it is a dirty hack, right? You could call lang_hooks.types.register_builtin_type defining the type __SIZE_TYPE__ as the int20 partial int type, and then define SIZE_TYPE as just __SIZE_TYPE__. That is the same effectively, just not using the header file. So something like tree type_node = builtin_type_for_size (20, 1); lang_hooks.types.register_builtin_type (type_node, "__SIZE_TYPE__"); or maybe tree type_node = builtin_type_for_size (POINTER_SIZE, 1); lang_hooks.types.register_builtin_type (type_node, "__SIZE_TYPE__"); in msp430.c, and #define SIZE_TYPE "__SIZE_TYPE__" in msp430.h? Segher
Re: Preventing ISO C errors when using macros for builtin types
On Mon, 10 Jun 2019 13:32:42 -0500 Segher Boessenkool wrote: > On Mon, Jun 10, 2019 at 05:20:31PM +0100, Jozef Lawrynowicz wrote: > > On Thu, 6 Jun 2019 10:09:32 +0200 > > Richard Biener wrote: > > > > > On Wed, Jun 5, 2019 at 3:26 PM Jozef Lawrynowicz > > > wrote: > > > > > > > > I would appreciate if anyone can help me decide if: > > > > - It would be OK for the use of builtin macros such as __SIZE_TYPE__ to > > > > somehow > > > > not trigger the "pedantic errors", and what a valid approach might > > > > look like > > > > > > I think that would be OK - note you could also modify your target board. > > > > I'm now realising that the most straightforward way to fix this issue will > > be > > to just modify the configuration of the DejaGNU target board, so that > > DEFAULT_CFLAGS is set there and declarations of DEFAULT_CFLAGS in the > > testsuite > > that would set -pedantic-errors are never used. > > That is not a fix, that is sweeping the problem under the rug. > > As a somewhat dirty hack I added > > #if __MSP430X_LARGE__ > #undef __SIZE_TYPE__ > __extension__ typedef unsigned __int20 __SIZE_TYPE__; > #endif > > to the start of the installed stddef.h, and that fixes the problem fine, > for correct programs that do not forget to include (directly > or indirectly), anyway. > > > Segher But we have some 850 generic tests in gcc/testsuite that use __SIZE_TYPE__ without including stddef.h. They just rely on the preprocessor to expand this using the builtin macro definition. I assumed it was standard (and preferred, for the sake of compilation time at least) for them to omit including stddef.h, which is why I was considering pursuing some way of modifying how the builtin macro is expanded. I could add something like your above code snippet to the tests that trigger the ISO C error, but this is what I was originally trying to avoid. At least, it seems like a good number of tests have the following: typedef __SIZE_TYPE__ size_t; For these the fixup is simple as we can just add __extension__ before the typedef. For others (e.g. gcc.dg/c99-const-expr-10.c), either we - add something like your snippet above and redefine __SIZE_TYPE__, or - replace uses of __SIZE_TYPE__ with size_t and add the typedef i.e. __extension__ typedef __SIZE_TYPE__ size_t; Do you have any opinion on which of these would be preferred? For modifying the individual tests, I would assume it would be to add the required typedefs at the top of the file, as this is what some tests already do. It also avoids having the target specific caveats in the test file and may help other targets with non-ISO types. Thanks, Jozef
Re: Preventing ISO C errors when using macros for builtin types
On Mon, Jun 10, 2019 at 05:20:31PM +0100, Jozef Lawrynowicz wrote: > On Thu, 6 Jun 2019 10:09:32 +0200 > Richard Biener wrote: > > > On Wed, Jun 5, 2019 at 3:26 PM Jozef Lawrynowicz > > wrote: > > > > > > I would appreciate if anyone can help me decide if: > > > - It would be OK for the use of builtin macros such as __SIZE_TYPE__ to > > > somehow > > > not trigger the "pedantic errors", and what a valid approach might look > > > like > > > > I think that would be OK - note you could also modify your target board. > > I'm now realising that the most straightforward way to fix this issue will be > to just modify the configuration of the DejaGNU target board, so that > DEFAULT_CFLAGS is set there and declarations of DEFAULT_CFLAGS in the > testsuite > that would set -pedantic-errors are never used. That is not a fix, that is sweeping the problem under the rug. As a somewhat dirty hack I added #if __MSP430X_LARGE__ #undef __SIZE_TYPE__ __extension__ typedef unsigned __int20 __SIZE_TYPE__; #endif to the start of the installed stddef.h, and that fixes the problem fine, for correct programs that do not forget to include (directly or indirectly), anyway. Segher
Re: Preventing ISO C errors when using macros for builtin types
On Thu, 6 Jun 2019 10:09:32 +0200 Richard Biener wrote: > On Wed, Jun 5, 2019 at 3:26 PM Jozef Lawrynowicz wrote: > > > > I would appreciate if anyone can help me decide if: > > - It would be OK for the use of builtin macros such as __SIZE_TYPE__ to > > somehow > > not trigger the "pedantic errors", and what a valid approach might look > > like > > I think that would be OK - note you could also modify your target board. I'm now realising that the most straightforward way to fix this issue will be to just modify the configuration of the DejaGNU target board, so that DEFAULT_CFLAGS is set there and declarations of DEFAULT_CFLAGS in the testsuite that would set -pedantic-errors are never used. Thanks, Jozef
Re: Preventing ISO C errors when using macros for builtin types
On Wed, 5 Jun 2019 17:12:25 -0500 Segher Boessenkool wrote: > On Wed, Jun 05, 2019 at 08:49:39PM +0100, Jozef Lawrynowicz wrote: > > On Wed, 5 Jun 2019 11:49:21 -0500 > > Segher Boessenkool wrote: > > > The documentation says > > > > > > '-pedantic' and other options cause warnings for many GNU C extensions. > > > You can prevent such warnings within one expression by writing > > > '__extension__' before the expression. '__extension__' has no effect > > > aside from this. > > > > > > It's not clear to me why you cannot simply put __extension__ earlier in > > > your case? > > > > If I am modifying tests on an individual basis, then indeed I can put > > __extension__ earlier in the declaration and it fixes the issue. > > Or you could fix it like this in your header file? If you're referring to the main target board header file (i.e. gcc/config/msp430/msp430.h), I cannot add __extension__ to the definition of SIZE_TYPE. It just breaks the build in too many places where a specific format for SIZE_TYPE is expected. Should I manage to coerce the build into succeeding, there would be further issues from the grammar rules regarding where __extension__ can be placed in expressions/declarations anyway. > > But for a fix within the compiler to prevent having to modify individual > > tests, > > I could add __extension__ to the beginning of the macro - but the macro may > > not end up at the beginning of a declaration in the source code. > > > > For example, say __SIZE_TYPE__ now expands to "__extension__ __int20 > > unsigned", > > then the following usage of __SIZE_TYPE__ would be ok, as __extension__ is > > at > > the beginning of the declaration: > > > > > __SIZE_TYPE__ size; > > Don't use macros for types? You could use something like > > __extension__ typedef unsigned __int20 __SIZE_TYPE__; I am only really concerned about the usage of __SIZE_TYPE__ in the GCC testsuite (since real users shouldn't be using these macros, and the user-facing typedefs do not cause ISO C errors), and it is in those tests that there is uninhibited use of __SIZE_TYPE__ directly as a type. Combined with -pedantic-errors being a default flag we see an ever expanding list of failures. So to make tracking testsuite failures easier, I was looking for a way to prevent these false errors by modifying the compiler itself. Failing that, a simple and quick modification I can make to the tests would be satisfactory - such as the aforementioned potential new DejaGNU directive to prune the -pedantic-errors option before running the test. The above typedef you suggested has in fact been similar to how I've been going about fixing these failures so far, however it occurred to me that there should be a better and more efficient way of avoiding these failures. > > I'm mainly just wondering if a change to the compiler to allow the usage of > > __extension__ within a declaration would be allowed. > > Well, how would that work? We'd need to modify the grammar to allow > __extension__ pretty much anywhere, and then change all compiler code > to not pedwarn until it has parsed a full expression (or statement, or > file, or however this will work). Or make it not warn for things after > the __extension__ only, or make it only *guaranteed* not to warn for > things *after* the __extension__. > > None of those options are very appetising, IMO. Yes I do agree with you, the mechanism for fixing this in the compiler would require significant work for this relatively niche use case. Thanks, Jozef
Re: Preventing ISO C errors when using macros for builtin types
On Wed, Jun 5, 2019 at 3:26 PM Jozef Lawrynowicz wrote: > > The MSP430 target in the large memory model uses the (non-ISO) __int20 type > for > SIZE_TYPE and PTRDIFF_TYPE. > The preprocessor therefore expands a builtin such as __SIZE_TYPE__ to > "__int20 unsigned" in user code. > When compiling with the "-pedantic-errors" flag, the use of any of these > builtin macros results in an error of the form: > > > tester.c:4:9: error: ISO C does not support '__int20' types [-Wpedantic] > > The GCC documentation does instruct users *not* to use these types directly > (cpp.texi): > > You should not use these macros directly; instead, include the > > appropriate headers and use the typedefs. > When using the typedefs (e.g. size_t) in a program compiled with > -pedantic-errors, there is no ISO C error. > > However, in the testsuite there is an ever-growing list of tests which use > the macros to avoid having to include any header files required for the > typedefs. > Since -pendantic-errors is often passed as a default flag in the testsuite, > there are many false failures when testing with -mlarge, caused by this ISO C > error. > > I would like to try to find a way to address this issue within GCC itself, so > that constant updates to the testsuite are not required to filter these types > of failures out. > > I tried one approach suggested here > https://gcc.gnu.org/ml/gcc-patches/2018-11/msg02219.html > which was to add "__extension__" to the definition of SIZE_TYPE/PTRDIFF_TYPE > in > msp430.h, however it became clear that that will not work, since the following > is not valid: > > typedef __extension__ __int20 ptrdiff_t; > > > error: expected identifier or '(' before '__extension__' > > __extension__ must be placed at the beginning of the declaration. > > I'm assuming it would not be valid to modify the behaviour of __extension__ > so it can be placed within a declaration, and not just at the > beginning. However, there is minimal documentation on this keyword (it does > not > state that it can be used in declarations, even though it can), so I wonder > what the "rules" are. > > I would appreciate if anyone can help me decide if: > - It would be OK for the use of builtin macros such as __SIZE_TYPE__ to > somehow > not trigger the "pedantic errors", and what a valid approach might look like I think that would be OK - note you could also modify your target board. Maybe we can trick libcpp to set in_system_header for those internal predefined macros so we do not warn on their expansions. We can also lookup macro expansion context and eventually see it is an internal macro. people more familiar with libcpp could say which approach is more reasonable. Richard. > * would finding a way to sandwich __extension__ into the expansion of these > macros be acceptable? > or, > - These types of failures should be continued to be fixed in the tests > themselves, for example by adding __extension__ before their usage. > > Thanks, > Jozef
Re: Preventing ISO C errors when using macros for builtin types
On Wed, Jun 05, 2019 at 08:49:39PM +0100, Jozef Lawrynowicz wrote: > On Wed, 5 Jun 2019 11:49:21 -0500 > Segher Boessenkool wrote: > > The documentation says > > > > '-pedantic' and other options cause warnings for many GNU C extensions. > > You can prevent such warnings within one expression by writing > > '__extension__' before the expression. '__extension__' has no effect > > aside from this. > > > > It's not clear to me why you cannot simply put __extension__ earlier in > > your case? > > If I am modifying tests on an individual basis, then indeed I can put > __extension__ earlier in the declaration and it fixes the issue. Or you could fix it like this in your header file? > But for a fix within the compiler to prevent having to modify individual > tests, > I could add __extension__ to the beginning of the macro - but the macro may > not end up at the beginning of a declaration in the source code. > > For example, say __SIZE_TYPE__ now expands to "__extension__ __int20 > unsigned", > then the following usage of __SIZE_TYPE__ would be ok, as __extension__ is at > the beginning of the declaration: > > > __SIZE_TYPE__ size; Don't use macros for types? You could use something like __extension__ typedef unsigned __int20 __SIZE_TYPE__; (stddef.h already uses typedefs like that, btw, for __SIZE_TYPE__ in fact). > I'm mainly just wondering if a change to the compiler to allow the usage of > __extension__ within a declaration would be allowed. Well, how would that work? We'd need to modify the grammar to allow __extension__ pretty much anywhere, and then change all compiler code to not pedwarn until it has parsed a full expression (or statement, or file, or however this will work). Or make it not warn for things after the __extension__ only, or make it only *guaranteed* not to warn for things *after* the __extension__. None of those options are very appetising, IMO. Segher
Re: Preventing ISO C errors when using macros for builtin types
On Wed, 5 Jun 2019 11:49:21 -0500 Segher Boessenkool wrote: > On Wed, Jun 05, 2019 at 02:25:59PM +0100, Jozef Lawrynowicz wrote: > > I'm assuming it would not be valid to modify the behaviour of __extension__ > > so it can be placed within a declaration, and not just at the > > beginning. However, there is minimal documentation on this keyword (it does > > not > > state that it can be used in declarations, even though it can), so I wonder > > what the "rules" are. > > The documentation says > > '-pedantic' and other options cause warnings for many GNU C extensions. > You can prevent such warnings within one expression by writing > '__extension__' before the expression. '__extension__' has no effect > aside from this. > > It's not clear to me why you cannot simply put __extension__ earlier in > your case? If I am modifying tests on an individual basis, then indeed I can put __extension__ earlier in the declaration and it fixes the issue. But for a fix within the compiler to prevent having to modify individual tests, I could add __extension__ to the beginning of the macro - but the macro may not end up at the beginning of a declaration in the source code. For example, say __SIZE_TYPE__ now expands to "__extension__ __int20 unsigned", then the following usage of __SIZE_TYPE__ would be ok, as __extension__ is at the beginning of the declaration: > __SIZE_TYPE__ size; But the following would emit an error, because __extension__ is not at the beginning of the declaration: > typedef __SIZE_TYPE__ size_t; I'm mainly just wondering if a change to the compiler to allow the usage of __extension__ within a declaration would be allowed. However since there are many contexts where usage of a type such as __SIZE_TYPE__ is valid, but where __extension__ preceding the type wouldn't be valid, this is probably not worth exploring.. I'm aware of some different ways that the tests can be fixed up to prevent the ISO C errors caused by -pedantic-errors. I'm currently thinking the best way to fix these issues in the testsuite might be to implement a directive such as "dg-prune-options". This directive can then prune "-pedantic-errors" from the options to be passed to GCC when some specified condition (e.g. -mlarge is in the opt list) is matched. This would at least avoid having to re-jig tests to get the __extension__ keyword into the right places, as the dg-prune-options can just be added to the problematic tests. Thanks, Jozef
Re: Preventing ISO C errors when using macros for builtin types
On Wed, Jun 05, 2019 at 02:25:59PM +0100, Jozef Lawrynowicz wrote: > I'm assuming it would not be valid to modify the behaviour of __extension__ > so it can be placed within a declaration, and not just at the > beginning. However, there is minimal documentation on this keyword (it does > not > state that it can be used in declarations, even though it can), so I wonder > what the "rules" are. The documentation says '-pedantic' and other options cause warnings for many GNU C extensions. You can prevent such warnings within one expression by writing '__extension__' before the expression. '__extension__' has no effect aside from this. It's not clear to me why you cannot simply put __extension__ earlier in your case? Segher