| Message ID | AM6PR03MB45195C918D4592DF23AEC5B7E49E0@AM6PR03MB4519.eurprd03.prod.outlook.com |
|---|---|
| State | New |
| Headers | show |
| Series | Fix -Wshadow=local warnings in elfos.h | expand |
On 10/4/19 12:24 PM, Bernd Edlinger wrote: > Hi, > > these macros don't use reserved names for local variables. > This caused -Wshadow=local warnings in varasm.c IIRC. > > Fixed by using _lowercase reserved names for macro parameters. > > > Bootstrapped and reg-tested on x86_64-pc-linux-gnu. > Is it OK for trunk? > > > Thanks > Bernd. > > > > patch-wshadow-elfos.diff > > 2019-10-04 Bernd Edlinger <bernd.edlinger@hotmail.de> > > * config/elfos.h (ASM_DECLARE_OBJECT_NAME, > ASM_FINISH_DECLARE_OBJECT): Rename local vars in macro. Same objections as before. As long as we're using macros like this, we're going to have increased potential for shadowing problems and macros which touch implementation details that just happen to be available in the context where the macro is used. Convert to real functions. It avoids the shadowing problem and avoids macros touching/referencing things they shouldn't. Code in macros may have been reasonable in the 80s/90s, but we should know better by now. I'm not ranting against you Bernd, it's more a rant against the original coding style for GCC. Your changes just highlight how bad of an idea this kind of macro usage really is. We should take the opportunity to fix this stuff for real. JEff
On 10/4/19 10:26 PM, Jeff Law wrote: > On 10/4/19 12:24 PM, Bernd Edlinger wrote: >> Hi, >> >> these macros don't use reserved names for local variables. >> This caused -Wshadow=local warnings in varasm.c IIRC. >> >> Fixed by using _lowercase reserved names for macro parameters. >> >> >> Bootstrapped and reg-tested on x86_64-pc-linux-gnu. >> Is it OK for trunk? >> >> >> Thanks >> Bernd. >> >> >> >> patch-wshadow-elfos.diff >> >> 2019-10-04 Bernd Edlinger <bernd.edlinger@hotmail.de> >> >> * config/elfos.h (ASM_DECLARE_OBJECT_NAME, >> ASM_FINISH_DECLARE_OBJECT): Rename local vars in macro. > Same objections as before. As long as we're using macros like this, > we're going to have increased potential for shadowing problems and > macros which touch implementation details that just happen to be > available in the context where the macro is used. > > Convert to real functions. It avoids the shadowing problem and avoids > macros touching/referencing things they shouldn't. Code in macros may > have been reasonable in the 80s/90s, but we should know better by now. > > I'm not ranting against you Bernd, it's more a rant against the original > coding style for GCC. Your changes just highlight how bad of an idea > this kind of macro usage really is. We should take the opportunity to > fix this stuff for real. > I understand that, and would not propose to fix it this way if this was the *only* place that breaks with -Wshadow=local. I will continue to send more patches over the weekend, formally obvious ones, but the amount of changes is just huge. I would suggest I send them here, and wait for at least 24H before committing, so anybody can suggest better names, for instance, or other (small) improvements, as you like. Thanks Bernd.
On Fri, Oct 04, 2019 at 02:26:26PM -0600, Jeff Law wrote: > Same objections as before. As long as we're using macros like this, > we're going to have increased potential for shadowing problems and > macros which touch implementation details that just happen to be > available in the context where the macro is used. > > Convert to real functions. It avoids the shadowing problem and avoids > macros touching/referencing things they shouldn't. Code in macros may > have been reasonable in the 80s/90s, but we should know better by now. > > I'm not ranting against you Bernd, it's more a rant against the original > coding style for GCC. Your changes just highlight how bad of an idea > this kind of macro usage really is. We should take the opportunity to > fix this stuff for real. To get 95% of the benefit for only 2% of the pain, you can make an inline function where there was a macro before, and define that same macro to just call the function. Like #define DEFAULT_SOME_MACRO(PARMS) { lots of code } becomes #define DEFAULT_SOME_MACRO(PARMS) default_some_macro(PARMS) static inline int default_some_macro (int parm, long another) { lots of code; } The point is that all this is completely *local* changes. Segher
On 10/5/19 9:26 AM, Segher Boessenkool wrote: > On Fri, Oct 04, 2019 at 02:26:26PM -0600, Jeff Law wrote: >> Same objections as before. As long as we're using macros like this, >> we're going to have increased potential for shadowing problems and >> macros which touch implementation details that just happen to be >> available in the context where the macro is used. >> >> Convert to real functions. It avoids the shadowing problem and avoids >> macros touching/referencing things they shouldn't. Code in macros may >> have been reasonable in the 80s/90s, but we should know better by now. >> >> I'm not ranting against you Bernd, it's more a rant against the original >> coding style for GCC. Your changes just highlight how bad of an idea >> this kind of macro usage really is. We should take the opportunity to >> fix this stuff for real. > > To get 95% of the benefit for only 2% of the pain, you can make an inline > function where there was a macro before, and define that same macro to > just call the function. > > Like > > #define DEFAULT_SOME_MACRO(PARMS) { lots of code } > > becomes > > #define DEFAULT_SOME_MACRO(PARMS) default_some_macro(PARMS) > > static inline int > default_some_macro (int parm, long another) > { > lots of code; > } > > The point is that all this is completely *local* changes. > Hmm, I tried this but it does not work: Index: elfos.h =================================================================== --- elfos.h (revision 276598) +++ elfos.h (working copy) @@ -309,34 +309,35 @@ #endif #define ASM_DECLARE_OBJECT_NAME(FILE, NAME, DECL) \ - do \ - { \ - HOST_WIDE_INT size; \ - \ - /* For template static data member instantiations or \ - inline fn local statics and their guard variables, use \ - gnu_unique_object so that they will be combined even under \ - RTLD_LOCAL. Don't use gnu_unique_object for typeinfo, \ - vtables and other read-only artificial decls. */ \ - if (USE_GNU_UNIQUE_OBJECT && DECL_ONE_ONLY (DECL) \ - && (!DECL_ARTIFICIAL (DECL) || !TREE_READONLY (DECL))) \ - ASM_OUTPUT_TYPE_DIRECTIVE (FILE, NAME, "gnu_unique_object"); \ - else \ - ASM_OUTPUT_TYPE_DIRECTIVE (FILE, NAME, "object"); \ - \ - size_directive_output = 0; \ - if (!flag_inhibit_size_directive \ - && (DECL) && DECL_SIZE (DECL)) \ - { \ - size_directive_output = 1; \ - size = tree_to_uhwi (DECL_SIZE_UNIT (DECL)); \ - ASM_OUTPUT_SIZE_DIRECTIVE (FILE, NAME, size); \ - } \ - \ - ASM_OUTPUT_LABEL (FILE, NAME); \ - } \ - while (0) + elfos_asm_declare_object_name (FILE, NAME, DECL) +static inline void +elfos_asm_declare_object_name(FILE *file, const char *name, tree decl) +{ + HOST_WIDE_INT size; + + /* For template static data member instantiations or + inline fn local statics and their guard variables, use + gnu_unique_object so that they will be combined even under + RTLD_LOCAL. Don't use gnu_unique_object for typeinfo, + vtables and other read-only artificial decls. */ + if (USE_GNU_UNIQUE_OBJECT && DECL_ONE_ONLY (decl) + && (!DECL_ARTIFICIAL (decl) || !TREE_READONLY (decl))) + ASM_OUTPUT_TYPE_DIRECTIVE (file, name, "gnu_unique_object"); + else + ASM_OUTPUT_TYPE_DIRECTIVE (file, name, "object"); + + size_directive_output = 0; + if (!flag_inhibit_size_directive + && (decl) && DECL_SIZE (decl)) + { + size_directive_output = 1; + size = tree_to_uhwi (DECL_SIZE_UNIT (decl)); + ASM_OUTPUT_SIZE_DIRECTIVE (file, name, size); + } + ASM_OUTPUT_LABEL (file, name); +} + /* Output the size directive for a decl in rest_of_decl_compilation in the case where we did not do so before the initializer. Once we find the error_mark_node, we know that the value of @@ -345,24 +346,27 @@ #undef ASM_FINISH_DECLARE_OBJECT #define ASM_FINISH_DECLARE_OBJECT(FILE, DECL, TOP_LEVEL, AT_END)\ - do \ - { \ - const char *name = XSTR (XEXP (DECL_RTL (DECL), 0), 0); \ - HOST_WIDE_INT size; \ - \ - if (!flag_inhibit_size_directive \ - && DECL_SIZE (DECL) \ - && ! AT_END && TOP_LEVEL \ - && DECL_INITIAL (DECL) == error_mark_node \ - && !size_directive_output) \ - { \ - size_directive_output = 1; \ - size = tree_to_uhwi (DECL_SIZE_UNIT (DECL)); \ - ASM_OUTPUT_SIZE_DIRECTIVE (FILE, name, size); \ - } \ - } \ - while (0) + elfos_asm_finish_declare_object (FILE, DECL, TOP_LEVEL, AT_END) +static inline void +elfos_asm_finish_declare_object (FILE *file, tree decl, + int top_level, int at_end) +{ + const char *name = XSTR (XEXP (DECL_RTL (decl), 0), 0); + HOST_WIDE_INT size; + + if (!flag_inhibit_size_directive + && DECL_SIZE (decl) + && !at_end && top_level + && DECL_INITIAL (decl) == error_mark_node + && !size_directive_output) + { + size_directive_output = 1; + size = tree_to_uhwi (DECL_SIZE_UNIT (decl)); + ASM_OUTPUT_SIZE_DIRECTIVE (file, name, size); + } +} + /* This is how to declare the size of a function. */ #ifndef ASM_DECLARE_FUNCTION_SIZE #define ASM_DECLARE_FUNCTION_SIZE(FILE, FNAME, DECL) \ build/gengtype \ -S ../../gcc-trunk/gcc -I gtyp-input.list -w tmp-gtype.state g++ -std=gnu++98 -c -g -DIN_GCC -fno-exceptions -fno-rtti -fasynchronous-unwind-tables -Wall -Wextra -Wno-narrowing -Wwrite-strings -Wcast-qual -Wno-format -Wmissing-format-attribute -Woverloaded-virtual -pedantic -Wno-long-long -Wno-variadic-macros -Wno-overlength-strings -fno-common -DHAVE_CONFIG_H -DGENERATOR_FILE -fno-PIE -I. -Ibuild -I../../gcc-trunk/gcc -I../../gcc-trunk/gcc/build -I../../gcc-trunk/gcc/../include -I../../gcc-trunk/gcc/../libcpp/include \ -o build/gencheck.o ../../gcc-trunk/gcc/gencheck.c In file included from ./tm.h:30:0, from ../../gcc-trunk/gcc/gencheck.c:23: ../../gcc-trunk/gcc/config/elfos.h: In function ‘void elfos_asm_declare_object_name(FILE*, const char*, tree)’: ../../gcc-trunk/gcc/config/elfos.h:324:51: error: ‘DECL_ONE_ONLY’ was not declared in this scope if (USE_GNU_UNIQUE_OBJECT && DECL_ONE_ONLY (decl) ^ ../../gcc-trunk/gcc/config/elfos.h:325:33: error: ‘DECL_ARTIFICIAL’ was not declared in this scope && (!DECL_ARTIFICIAL (decl) || !TREE_READONLY (decl))) ^ ../../gcc-trunk/gcc/config/elfos.h:325:58: error: ‘TREE_READONLY’ was not declared in this scope && (!DECL_ARTIFICIAL (decl) || !TREE_READONLY (decl))) ^ ../../gcc-trunk/gcc/config/elfos.h:326:63: error: ‘ASM_OUTPUT_TYPE_DIRECTIVE’ was not declared in this scope ASM_OUTPUT_TYPE_DIRECTIVE (file, name, "gnu_unique_object"); ^ ../../gcc-trunk/gcc/config/elfos.h:328:52: error: ‘ASM_OUTPUT_TYPE_DIRECTIVE’ was not declared in this scope ASM_OUTPUT_TYPE_DIRECTIVE (file, name, "object"); ^ ../../gcc-trunk/gcc/config/elfos.h:330:3: error: ‘size_directive_output’ was not declared in this scope size_directive_output = 0; ^ ../../gcc-trunk/gcc/config/elfos.h:332:35: error: ‘DECL_SIZE’ was not declared in this scope && (decl) && DECL_SIZE (decl)) ^ ../../gcc-trunk/gcc/config/elfos.h:335:48: error: ‘DECL_SIZE_UNIT’ was not declared in this scope size = tree_to_uhwi (DECL_SIZE_UNIT (decl)); ^ ../../gcc-trunk/gcc/config/elfos.h:335:49: error: ‘tree_to_uhwi’ was not declared in this scope size = tree_to_uhwi (DECL_SIZE_UNIT (decl)); ^ ../../gcc-trunk/gcc/config/elfos.h:336:50: error: ‘ASM_OUTPUT_SIZE_DIRECTIVE’ was not declared in this scope ASM_OUTPUT_SIZE_DIRECTIVE (file, name, size); ^ ../../gcc-trunk/gcc/config/elfos.h:338:31: error: ‘ASM_OUTPUT_LABEL’ was not declared in this scope ASM_OUTPUT_LABEL (file, name); ^ ../../gcc-trunk/gcc/config/elfos.h: In function ‘void elfos_asm_finish_declare_object(FILE*, tree, int, int)’: ../../gcc-trunk/gcc/config/elfos.h:355:48: error: ‘DECL_RTL’ was not declared in this scope const char *name = XSTR (XEXP (DECL_RTL (decl), 0), 0); ^ ../../gcc-trunk/gcc/config/elfos.h:355:52: error: ‘XEXP’ was not declared in this scope const char *name = XSTR (XEXP (DECL_RTL (decl), 0), 0); ^ ../../gcc-trunk/gcc/config/elfos.h:355:56: error: ‘XSTR’ was not declared in this scope const char *name = XSTR (XEXP (DECL_RTL (decl), 0), 0); ^ ../../gcc-trunk/gcc/config/elfos.h:359:25: error: ‘DECL_SIZE’ was not declared in this scope && DECL_SIZE (decl) ^ ../../gcc-trunk/gcc/config/elfos.h:361:28: error: ‘DECL_INITIAL’ was not declared in this scope && DECL_INITIAL (decl) == error_mark_node ^ ../../gcc-trunk/gcc/config/elfos.h:361:33: error: ‘error_mark_node’ was not declared in this scope && DECL_INITIAL (decl) == error_mark_node ^ ../../gcc-trunk/gcc/config/elfos.h:362:11: error: ‘size_directive_output’ was not declared in this scope && !size_directive_output) ^ ../../gcc-trunk/gcc/config/elfos.h:365:48: error: ‘DECL_SIZE_UNIT’ was not declared in this scope size = tree_to_uhwi (DECL_SIZE_UNIT (decl)); ^ ../../gcc-trunk/gcc/config/elfos.h:365:49: error: ‘tree_to_uhwi’ was not declared in this scope size = tree_to_uhwi (DECL_SIZE_UNIT (decl)); ^ ../../gcc-trunk/gcc/config/elfos.h:366:50: error: ‘ASM_OUTPUT_SIZE_DIRECTIVE’ was not declared in this scope ASM_OUTPUT_SIZE_DIRECTIVE (file, name, size); ^ Now I don't know how to fix that easily. Bernd.
On Sat, Oct 05, 2019 at 08:28:26AM +0000, Bernd Edlinger wrote: > On 10/5/19 9:26 AM, Segher Boessenkool wrote: > > On Fri, Oct 04, 2019 at 02:26:26PM -0600, Jeff Law wrote: > > To get 95% of the benefit for only 2% of the pain, you can make an inline > > function where there was a macro before, and define that same macro to > > just call the function. > Hmm, I tried this but it does not work: [ snip ] > In file included from ./tm.h:30:0, > from ../../gcc-trunk/gcc/gencheck.c:23: > ../../gcc-trunk/gcc/config/elfos.h: In function ‘void elfos_asm_declare_object_name(FILE*, const char*, tree)’: > ../../gcc-trunk/gcc/config/elfos.h:324:51: error: ‘DECL_ONE_ONLY’ was not declared in this scope So not everything including elfos.h (via tm.h perhaps) includes tree.h first, but this macro / function requires that, and as function that is a compilation error. So maybe this macro / function should move to some header that isn't included everywhere, if it accesses stuff that isn't defined in all places? Or maybe it is best to make this a real hook, move the implementation to a .c that actually includes the headers it needs ;-) (elfos.h is kind of special, it is included via tm.h, this is set up in config.gcc). > Now I don't know how to fix that easily. Yeah me neither. Pity. Segher
On Sat, 5 Oct 2019, Bernd Edlinger wrote: > > Like > > > > #define DEFAULT_SOME_MACRO(PARMS) { lots of code } > > > > becomes > > > > #define DEFAULT_SOME_MACRO(PARMS) default_some_macro(PARMS) > > > > static inline int > > default_some_macro (int parm, long another) > > { > > lots of code; > > } > > > > The point is that all this is completely *local* changes. > > > > Hmm, I tried this but it does not work: The answer to that is an out-of-line function. See the default_register_move_cost function in targhooks.c, the register_move_cost hook and the REGISTER_MOVE_COST target macro, for example. There is a strategy for incremental conversion of a single target macro to a hook that goes as follows: * Define a corresponding target hook. Make the default definition in targhooks.c be one that tests whether the target macro is defined and either uses that macro or the existing default as appropriate. Make tm.texi.in document the hook alongside the macro, saying that the macro is obsolete and new targets should use the hook. * Convert all *uses* of the target macro to use the hook instead. There is no need to change any of the definitions in the various targets with non-default definitions. * Remove the default definition of the target macro, as no longer needed now targhooks.c tests whether the target macro is defined or not. At that point, you have a partial conversion of that target macro to a hook (and consequent gains regarding avoiding shadowing) without having touched any of the target-specific code. Any target can safely convert its own definition of the macro to a definition of the hook, independently of all other targets and all other hooks. Once all targets have changed, the macro can be poisoned and the documentation of the macro and support in targhooks.c for using the macro removed, in the usual fashion for removing a target macro - but that might happen a long time afterwards. It's more work than a pure local conversion to an inline function, or than just renaming variables, but significantly less than a full conversion of the macro to a hook (and in particular should not need testing on more than one target).
On 10/5/19 12:16 AM, Bernd Edlinger wrote: > > > On 10/4/19 10:26 PM, Jeff Law wrote: >> On 10/4/19 12:24 PM, Bernd Edlinger wrote: >>> Hi, >>> >>> these macros don't use reserved names for local variables. >>> This caused -Wshadow=local warnings in varasm.c IIRC. >>> >>> Fixed by using _lowercase reserved names for macro parameters. >>> >>> >>> Bootstrapped and reg-tested on x86_64-pc-linux-gnu. >>> Is it OK for trunk? >>> >>> >>> Thanks >>> Bernd. >>> >>> >>> >>> patch-wshadow-elfos.diff >>> >>> 2019-10-04 Bernd Edlinger <bernd.edlinger@hotmail.de> >>> >>> * config/elfos.h (ASM_DECLARE_OBJECT_NAME, >>> ASM_FINISH_DECLARE_OBJECT): Rename local vars in macro. >> Same objections as before. As long as we're using macros like this, >> we're going to have increased potential for shadowing problems and >> macros which touch implementation details that just happen to be >> available in the context where the macro is used. >> >> Convert to real functions. It avoids the shadowing problem and avoids >> macros touching/referencing things they shouldn't. Code in macros may >> have been reasonable in the 80s/90s, but we should know better by now. >> >> I'm not ranting against you Bernd, it's more a rant against the original >> coding style for GCC. Your changes just highlight how bad of an idea >> this kind of macro usage really is. We should take the opportunity to >> fix this stuff for real. >> > > I understand that, and would not propose to fix it this way if this > was the *only* place that breaks with -Wshadow=local. > > I will continue to send more patches over the weekend, formally obvious > ones, but the amount of changes is just huge. > > I would suggest I send them here, and wait for at least 24H before > committing, so anybody can suggest better names, for instance, > or other (small) improvements, as you like. I've been away for the last week. I hope you've seen that the concerns folks are raising are a good indicator that the current approach isn't how we want to fix these issues. Factoring/refactoring, hooks, and _sensible_ renaming of variables are the way forward. Underscores and numeric prefixes/suffixes are just making things worse. My recommendation would be to look at an opt-out style. ie, add the new warning, but explicitly disable it (in Makefile.in) for every file were we're not clean yet. There's already some mechanisms to do this in Makefile.in where we disable specific warnings for specific files. Then address one file at a time in the right way then remove that file from the opt-out list. Continue until done :-) I realize this will be a lot of work. jeff
2019-10-04 Bernd Edlinger <bernd.edlinger@hotmail.de> * config/elfos.h (ASM_DECLARE_OBJECT_NAME, ASM_FINISH_DECLARE_OBJECT): Rename local vars in macro. Index: gcc/config/elfos.h =================================================================== --- gcc/config/elfos.h (revision 276484) +++ gcc/config/elfos.h (working copy) @@ -311,7 +311,7 @@ see the files COPYING3 and COPYING.RUNTIME respect #define ASM_DECLARE_OBJECT_NAME(FILE, NAME, DECL) \ do \ { \ - HOST_WIDE_INT size; \ + HOST_WIDE_INT _size; \ \ /* For template static data member instantiations or \ inline fn local statics and their guard variables, use \ @@ -329,8 +329,8 @@ see the files COPYING3 and COPYING.RUNTIME respect && (DECL) && DECL_SIZE (DECL)) \ { \ size_directive_output = 1; \ - size = tree_to_uhwi (DECL_SIZE_UNIT (DECL)); \ - ASM_OUTPUT_SIZE_DIRECTIVE (FILE, NAME, size); \ + _size = tree_to_uhwi (DECL_SIZE_UNIT (DECL)); \ + ASM_OUTPUT_SIZE_DIRECTIVE (FILE, NAME, _size); \ } \ \ ASM_OUTPUT_LABEL (FILE, NAME); \ @@ -347,8 +347,8 @@ see the files COPYING3 and COPYING.RUNTIME respect #define ASM_FINISH_DECLARE_OBJECT(FILE, DECL, TOP_LEVEL, AT_END)\ do \ { \ - const char *name = XSTR (XEXP (DECL_RTL (DECL), 0), 0); \ - HOST_WIDE_INT size; \ + const char *_name = XSTR (XEXP (DECL_RTL (DECL), 0), 0); \ + HOST_WIDE_INT _size; \ \ if (!flag_inhibit_size_directive \ && DECL_SIZE (DECL) \ @@ -357,8 +357,8 @@ see the files COPYING3 and COPYING.RUNTIME respect && !size_directive_output) \ { \ size_directive_output = 1; \ - size = tree_to_uhwi (DECL_SIZE_UNIT (DECL)); \ - ASM_OUTPUT_SIZE_DIRECTIVE (FILE, name, size); \ + _size = tree_to_uhwi (DECL_SIZE_UNIT (DECL)); \ + ASM_OUTPUT_SIZE_DIRECTIVE (FILE, _name, _size); \ } \ } \ while (0)