| Message ID | VI1PR03MB452892F64E88E21B5BC0B380E49F0@VI1PR03MB4528.eurprd03.prod.outlook.com |
|---|---|
| State | New |
| Headers | show |
| Series | Fix -Wshadow=local warnings in rtl.h | expand |
On Thu, Oct 03, 2019 at 03:17:47PM +0000, Bernd Edlinger wrote: > Hi, > > this fixes -Wshadow=local warnings in the RTL_FLAG_CHECKx macros, > which happen when this macro is used recursively in a macro > argument. The __typeof (RTX) const _rtx in the inner macro > expansions shadows the outer macro expansions. > > So reworked the macro to not use statement expressions but > use templates instead. Since the 7-argument overload is not > used anywhere removed RTL_FLAG_CHECK7 for now. What effect does this have on the cc1/cc1plus .text sizes? Does this affect debuggability of --enable-checking=yes,rtl compilers? I mean, often when we replace some macros with inlines step in GDB becomes a bigger nightmare, having to go through tons of inline frames. gdbinit.in has a lengthy list of inlines to skip in rtl.h, shouldn't this be added to that list? Not 100% sure how well it will work on rtl checking vs. non-rtl checking builds. Jakub
On 10/3/19 5:25 PM, Jakub Jelinek wrote: > On Thu, Oct 03, 2019 at 03:17:47PM +0000, Bernd Edlinger wrote: >> Hi, >> >> this fixes -Wshadow=local warnings in the RTL_FLAG_CHECKx macros, >> which happen when this macro is used recursively in a macro >> argument. The __typeof (RTX) const _rtx in the inner macro >> expansions shadows the outer macro expansions. >> >> So reworked the macro to not use statement expressions but >> use templates instead. Since the 7-argument overload is not >> used anywhere removed RTL_FLAG_CHECK7 for now. > > What effect does this have on the cc1/cc1plus .text sizes? > Does this affect debuggability of --enable-checking=yes,rtl compilers? > I mean, often when we replace some macros with inlines step in GDB > becomes a bigger nightmare, having to go through tons of inline frames. > gdbinit.in has a lengthy list of inlines to skip in rtl.h, shouldn't this be > added to that list? Not 100% sure how well it will work on rtl checking > vs. non-rtl checking builds. > > Jakub > I checked that the resulting code does not look completely stupid, but I will try to find answers to your questions above by tomorrow. Thanks Bernd.
On Thu, Oct 03, 2019 at 05:25:55PM +0200, Jakub Jelinek wrote: > On Thu, Oct 03, 2019 at 03:17:47PM +0000, Bernd Edlinger wrote: > > this fixes -Wshadow=local warnings in the RTL_FLAG_CHECKx macros, > > which happen when this macro is used recursively in a macro > > argument. The __typeof (RTX) const _rtx in the inner macro > > expansions shadows the outer macro expansions. > > > > So reworked the macro to not use statement expressions but > > use templates instead. Since the 7-argument overload is not > > used anywhere removed RTL_FLAG_CHECK7 for now. > > What effect does this have on the cc1/cc1plus .text sizes? > Does this affect debuggability of --enable-checking=yes,rtl compilers? > I mean, often when we replace some macros with inlines step in GDB > becomes a bigger nightmare, having to go through tons of inline frames. > gdbinit.in has a lengthy list of inlines to skip in rtl.h, shouldn't this be > added to that list? Not 100% sure how well it will work on rtl checking > vs. non-rtl checking builds. Also, how much slower does it make RTL checking builds? Segher
Bernd Edlinger <bernd.edlinger@hotmail.de> writes: > Hi, > > this fixes -Wshadow=local warnings in the RTL_FLAG_CHECKx macros, > which happen when this macro is used recursively in a macro > argument. The __typeof (RTX) const _rtx in the inner macro > expansions shadows the outer macro expansions. > > So reworked the macro to not use statement expressions but > use templates instead. Since the 7-argument overload is not > used anywhere removed RTL_FLAG_CHECK7 for now. > > > Bootstrapped and reg-tested on x86_64-pc-linux-gnu. > Is it OK for trunk? > > > Thanks > Bernd. > > 2019-10-03 Bernd Edlinger <bernd.edlinger@hotmail.de> > > * rtl.h (RTL_FLAG_CHECK): New variadic macro. > (RTL_FLAG_CHECK1-6): Use RTL_FLAG_CHECK. > (RTL_FLAG_CHECK7): Remove. > (rtl_flag_check, check_rtl_code): New helper functions. > > Index: gcc/rtl.h > =================================================================== > --- gcc/rtl.h (revision 276484) > +++ gcc/rtl.h (working copy) > @@ -1249,66 +1249,18 @@ extern void rtvec_check_failed_bounds (const_rtvec > #define RTX_FLAG(RTX, FLAG) ((RTX)->FLAG) > > #if defined ENABLE_RTL_FLAG_CHECKING && (GCC_VERSION >= 2007) > -#define RTL_FLAG_CHECK1(NAME, RTX, C1) __extension__ \ > -({ __typeof (RTX) const _rtx = (RTX); \ > - if (GET_CODE (_rtx) != C1) \ > - rtl_check_failed_flag (NAME, _rtx, __FILE__, __LINE__, \ > - __FUNCTION__); \ > - _rtx; }) > +#define RTL_FLAG_CHECK(NAME, RTX, ...) \ > + ((__typeof (&*(RTX))) rtl_flag_check (check_rtl_code<__VA_ARGS__>, \ > + NAME, RTX, __FILE__, __LINE__, \ > + __FUNCTION__)) Yet another comment on this one, sorry, but why not just make rtl_flag_check a template function instead of defining two versions of it and using __typeof? Is the idea is to make sure that GET_CODE is only applied to genuine rtxes rather than some random structure with a field called "code"? If so, I think we should do that in GET_CODE itself, and do it regardless of whether rtl checking is enabled. E.g. something like: #define GET_CODE(RTX) ((rtx_code) static_cast<const_rtx> (RTX)->code) (Wonder how much code that will break :-)) And if we do use templates instead of const_rtx/rtx variants, it might be simpler to keep the checking and assert together: #define RTL_FLAG_CHECK(NAME, RTX, ...) \ (rtl_flag_check<__VA_ARGS__> (NAME, RTX, __FILE__, __LINE__, \ __FUNCTION__)) template<rtx_code C1, typename T> inline T rtl_flag_check (const char *name, T rtl, const char *file, int line, const char *func) { if (GET_CODE (rtl) != C1) rtl_check_failed_flag (name, rtl, file, line, func); return rtl; } ...etc... which could also address some of the fears around cost. Personally I'm not fussed about keeping the checking and assert together though, just a suggestion. Richard
On 10/4/19 2:38 PM, Richard Sandiford wrote: > Bernd Edlinger <bernd.edlinger@hotmail.de> writes: >> Hi, >> >> this fixes -Wshadow=local warnings in the RTL_FLAG_CHECKx macros, >> which happen when this macro is used recursively in a macro >> argument. The __typeof (RTX) const _rtx in the inner macro >> expansions shadows the outer macro expansions. >> >> So reworked the macro to not use statement expressions but >> use templates instead. Since the 7-argument overload is not >> used anywhere removed RTL_FLAG_CHECK7 for now. >> >> >> Bootstrapped and reg-tested on x86_64-pc-linux-gnu. >> Is it OK for trunk? >> >> >> Thanks >> Bernd. >> >> 2019-10-03 Bernd Edlinger <bernd.edlinger@hotmail.de> >> >> * rtl.h (RTL_FLAG_CHECK): New variadic macro. >> (RTL_FLAG_CHECK1-6): Use RTL_FLAG_CHECK. >> (RTL_FLAG_CHECK7): Remove. >> (rtl_flag_check, check_rtl_code): New helper functions. >> >> Index: gcc/rtl.h >> =================================================================== >> --- gcc/rtl.h (revision 276484) >> +++ gcc/rtl.h (working copy) >> @@ -1249,66 +1249,18 @@ extern void rtvec_check_failed_bounds (const_rtvec >> #define RTX_FLAG(RTX, FLAG) ((RTX)->FLAG) >> >> #if defined ENABLE_RTL_FLAG_CHECKING && (GCC_VERSION >= 2007) >> -#define RTL_FLAG_CHECK1(NAME, RTX, C1) __extension__ \ >> -({ __typeof (RTX) const _rtx = (RTX); \ >> - if (GET_CODE (_rtx) != C1) \ >> - rtl_check_failed_flag (NAME, _rtx, __FILE__, __LINE__, \ >> - __FUNCTION__); \ >> - _rtx; }) >> +#define RTL_FLAG_CHECK(NAME, RTX, ...) \ >> + ((__typeof (&*(RTX))) rtl_flag_check (check_rtl_code<__VA_ARGS__>, \ >> + NAME, RTX, __FILE__, __LINE__, \ >> + __FUNCTION__)) > > Yet another comment on this one, sorry, but why not just make > rtl_flag_check a template function instead of defining two versions > of it and using __typeof? Is the idea is to make sure that GET_CODE > is only applied to genuine rtxes rather than some random structure > with a field called "code"? If so, I think we should do that in > GET_CODE itself, and do it regardless of whether rtl checking is > enabled. E.g. something like: > Actually I wanted to do it with a template, and invoke it using __typeof(RTX). BUT with that I ran into a lmitation of the template vs. block statements See PR#91803: We cannot instantiate a template on the type of a statement expression, only when the expression is completely written down without statement expressions. So even if we remove that limitation, it will be impossible to use templates dependent on __typeof(statement-expressions) as we need to bootstrap from rather old gcc versions. However if we are able to replace all statement-expressions in rtl.h then it will probably be possible to use a template here, without that type cast. I still need a const and a non-const version of that function because otherwise the -Werror=cast-qual warning will kill me. > #define GET_CODE(RTX) ((rtx_code) static_cast<const_rtx> (RTX)->code) > > (Wonder how much code that will break :-)) > > And if we do use templates instead of const_rtx/rtx variants, > it might be simpler to keep the checking and assert together: > > #define RTL_FLAG_CHECK(NAME, RTX, ...) \ > (rtl_flag_check<__VA_ARGS__> (NAME, RTX, __FILE__, __LINE__, \ > __FUNCTION__)) > > template<rtx_code C1, typename T> > inline T > rtl_flag_check (const char *name, T rtl, const char *file, int line, > const char *func) > { > if (GET_CODE (rtl) != C1) > rtl_check_failed_flag (name, rtl, file, line, func); > return rtl; > } > > ...etc... > I somehow expected the typename T need to be given int the template arguments like rtl_flag_check<__typeof(RTX), __VA_ARGS__> (NAME, RTX, Could be that there is way to make that work without __typeof ? > which could also address some of the fears around cost. > > Personally I'm not fussed about keeping the checking and assert > together though, just a suggestion. > Okay I'll consider that. BTW, I noticed that the patch is still incomplete and breaks with --enable-checking=yes,rtl :-/ I did not try that before, just did it after Jakub mentioned it. Thanks Bernd.
Bernd Edlinger <bernd.edlinger@hotmail.de> writes: > On 10/4/19 2:38 PM, Richard Sandiford wrote: >> Bernd Edlinger <bernd.edlinger@hotmail.de> writes: >>> Hi, >>> >>> this fixes -Wshadow=local warnings in the RTL_FLAG_CHECKx macros, >>> which happen when this macro is used recursively in a macro >>> argument. The __typeof (RTX) const _rtx in the inner macro >>> expansions shadows the outer macro expansions. >>> >>> So reworked the macro to not use statement expressions but >>> use templates instead. Since the 7-argument overload is not >>> used anywhere removed RTL_FLAG_CHECK7 for now. >>> >>> >>> Bootstrapped and reg-tested on x86_64-pc-linux-gnu. >>> Is it OK for trunk? >>> >>> >>> Thanks >>> Bernd. >>> >>> 2019-10-03 Bernd Edlinger <bernd.edlinger@hotmail.de> >>> >>> * rtl.h (RTL_FLAG_CHECK): New variadic macro. >>> (RTL_FLAG_CHECK1-6): Use RTL_FLAG_CHECK. >>> (RTL_FLAG_CHECK7): Remove. >>> (rtl_flag_check, check_rtl_code): New helper functions. >>> >>> Index: gcc/rtl.h >>> =================================================================== >>> --- gcc/rtl.h (revision 276484) >>> +++ gcc/rtl.h (working copy) >>> @@ -1249,66 +1249,18 @@ extern void rtvec_check_failed_bounds (const_rtvec >>> #define RTX_FLAG(RTX, FLAG) ((RTX)->FLAG) >>> >>> #if defined ENABLE_RTL_FLAG_CHECKING && (GCC_VERSION >= 2007) >>> -#define RTL_FLAG_CHECK1(NAME, RTX, C1) __extension__ \ >>> -({ __typeof (RTX) const _rtx = (RTX); \ >>> - if (GET_CODE (_rtx) != C1) \ >>> - rtl_check_failed_flag (NAME, _rtx, __FILE__, __LINE__, \ >>> - __FUNCTION__); \ >>> - _rtx; }) >>> +#define RTL_FLAG_CHECK(NAME, RTX, ...) \ >>> + ((__typeof (&*(RTX))) rtl_flag_check (check_rtl_code<__VA_ARGS__>, \ >>> + NAME, RTX, __FILE__, __LINE__, \ >>> + __FUNCTION__)) >> >> Yet another comment on this one, sorry, but why not just make >> rtl_flag_check a template function instead of defining two versions >> of it and using __typeof? Is the idea is to make sure that GET_CODE >> is only applied to genuine rtxes rather than some random structure >> with a field called "code"? If so, I think we should do that in >> GET_CODE itself, and do it regardless of whether rtl checking is >> enabled. E.g. something like: >> > > Actually I wanted to do it with a template, and invoke it using __typeof(RTX). > > BUT with that I ran into a lmitation of the template vs. block statements > See PR#91803: We cannot instantiate a template on the > type of a statement expression, only when the expression is completely > written down without statement expressions. > > So even if we remove that limitation, it will be impossible to use > templates dependent on __typeof(statement-expressions) as we need > to bootstrap from rather old gcc versions. > > However if we are able to replace all statement-expressions in > rtl.h then it will probably be possible to use a template here, > without that type cast. > > I still need a const and a non-const version of that function because > otherwise the -Werror=cast-qual warning will kill me. > > >> #define GET_CODE(RTX) ((rtx_code) static_cast<const_rtx> (RTX)->code) >> >> (Wonder how much code that will break :-)) >> >> And if we do use templates instead of const_rtx/rtx variants, >> it might be simpler to keep the checking and assert together: >> >> #define RTL_FLAG_CHECK(NAME, RTX, ...) \ >> (rtl_flag_check<__VA_ARGS__> (NAME, RTX, __FILE__, __LINE__, \ >> __FUNCTION__)) >> >> template<rtx_code C1, typename T> >> inline T >> rtl_flag_check (const char *name, T rtl, const char *file, int line, >> const char *func) >> { >> if (GET_CODE (rtl) != C1) >> rtl_check_failed_flag (name, rtl, file, line, func); >> return rtl; >> } >> >> ...etc... >> > > I somehow expected the typename T need to be given int the template arguments > like rtl_flag_check<__typeof(RTX), __VA_ARGS__> (NAME, RTX, > > Could be that there is way to make that work without __typeof ? Yeah, the typename will be deduced automatically in the example above. I don't think we need __typeof here. Richard
On 10/4/19 7:09 PM, Richard Sandiford wrote: > Bernd Edlinger <bernd.edlinger@hotmail.de> writes: >> >> Actually I wanted to do it with a template, and invoke it using __typeof(RTX). >> >> BUT with that I ran into a lmitation of the template vs. block statements >> See PR#91803: We cannot instantiate a template on the >> type of a statement expression, only when the expression is completely >> written down without statement expressions. >> >> So even if we remove that limitation, it will be impossible to use >> templates dependent on __typeof(statement-expressions) as we need >> to bootstrap from rather old gcc versions. >> >> However if we are able to replace all statement-expressions in >> rtl.h then it will probably be possible to use a template here, >> without that type cast. >> >> I still need a const and a non-const version of that function because >> otherwise the -Werror=cast-qual warning will kill me. >> >> >>> #define GET_CODE(RTX) ((rtx_code) static_cast<const_rtx> (RTX)->code) >>> >>> (Wonder how much code that will break :-)) >>> >>> And if we do use templates instead of const_rtx/rtx variants, >>> it might be simpler to keep the checking and assert together: >>> >>> #define RTL_FLAG_CHECK(NAME, RTX, ...) \ >>> (rtl_flag_check<__VA_ARGS__> (NAME, RTX, __FILE__, __LINE__, \ >>> __FUNCTION__)) >>> >>> template<rtx_code C1, typename T> >>> inline T >>> rtl_flag_check (const char *name, T rtl, const char *file, int line, >>> const char *func) >>> { >>> if (GET_CODE (rtl) != C1) >>> rtl_check_failed_flag (name, rtl, file, line, func); >>> return rtl; >>> } >>> >>> ...etc... >>> >> >> I somehow expected the typename T need to be given int the template arguments >> like rtl_flag_check<__typeof(RTX), __VA_ARGS__> (NAME, RTX, >> >> Could be that there is way to make that work without __typeof ? > > Yeah, the typename will be deduced automatically in the example above. > I don't think we need __typeof here. > Yes, indeed it works. The updated patch uses only one template function for rtl_flag_check. It also fixes the --enable-checking=rtl issue. I wonder if that can also be simplified into one template, unlike the rtl_flag_check the result is either a const or a non-const object but not the same type as the rtvec probably you'll have immediately an idea here? Thanks Bernd.
On 10/3/19 5:25 PM, Jakub Jelinek wrote: > On Thu, Oct 03, 2019 at 03:17:47PM +0000, Bernd Edlinger wrote: >> Hi, >> >> this fixes -Wshadow=local warnings in the RTL_FLAG_CHECKx macros, >> which happen when this macro is used recursively in a macro >> argument. The __typeof (RTX) const _rtx in the inner macro >> expansions shadows the outer macro expansions. >> >> So reworked the macro to not use statement expressions but >> use templates instead. Since the 7-argument overload is not >> used anywhere removed RTL_FLAG_CHECK7 for now. > > What effect does this have on the cc1/cc1plus .text sizes? r276457: with patch, --enable-languages=all --enable-checking=yes,rtl $ size gcc/cc1 text data bss dec hex filename 35117708 50984 1388192 36556884 22dd054 gcc/cc1 $ size gcc/cc1plus text data bss dec hex filename 37871569 54640 1391936 39318145 257f281 gcc/cc1plus unpatched, --enable-languages=all --enable-checking=yes,rtl $ size gcc/cc1 text data bss dec hex filename 36972031 50984 1388192 38411207 24a1bc7 gcc/cc1 $ size gcc/cc1plus text data bss dec hex filename 39725980 54640 1391936 41172556 2743e4c gcc/cc1plus I am a bit surprised, that the patch gives smaller code. I used not the original Version of this patch, but the one based on Richard's suggestion. > Does this affect debuggability of --enable-checking=yes,rtl compilers? > I mean, often when we replace some macros with inlines step in GDB > becomes a bigger nightmare, having to go through tons of inline frames. > gdbinit.in has a lengthy list of inlines to skip in rtl.h, shouldn't this be > added to that list? Not 100% sure how well it will work on rtl checking > vs. non-rtl checking builds. > I don't see a big problem here. If I type "s" in gdb it jumps to the check function and the next s jumps back, adding skip instructions in gdbinit.in does not seem to have any effect for me, but the debug is not that uncomfortable anyway. Interesting is that gdb also jumps in the check function when I press n. That is also Independent of the gdbinit.in, seems to be a bug due to inlining code from another line than the original macro, but nothing terrilby bad. Bernd.
On 10/4/19 10:26 AM, Segher Boessenkool wrote: > On Thu, Oct 03, 2019 at 05:25:55PM +0200, Jakub Jelinek wrote: >> On Thu, Oct 03, 2019 at 03:17:47PM +0000, Bernd Edlinger wrote: >>> this fixes -Wshadow=local warnings in the RTL_FLAG_CHECKx macros, >>> which happen when this macro is used recursively in a macro >>> argument. The __typeof (RTX) const _rtx in the inner macro >>> expansions shadows the outer macro expansions. >>> >>> So reworked the macro to not use statement expressions but >>> use templates instead. Since the 7-argument overload is not >>> used anywhere removed RTL_FLAG_CHECK7 for now. >> >> What effect does this have on the cc1/cc1plus .text sizes? >> Does this affect debuggability of --enable-checking=yes,rtl compilers? >> I mean, often when we replace some macros with inlines step in GDB >> becomes a bigger nightmare, having to go through tons of inline frames. >> gdbinit.in has a lengthy list of inlines to skip in rtl.h, shouldn't this be >> added to that list? Not 100% sure how well it will work on rtl checking >> vs. non-rtl checking builds. > > Also, how much slower does it make RTL checking builds? > Okay, I bootstrapped r276457: with patch, --enable-languages=all --enable-checking=yes,rtl 1:03:30 unpatched, --enable-languages=all --enable-checking=yes,rtl 1:03:18 unpatched, --enable-languages=all 0:58:41 The difference is only small, if at all significant. I used the later version of the patch here: https://gcc.gnu.org/ml/gcc-patches/2019-10/msg00365.html since the original version did not work with --enable-checking=rtl. Bernd.
On Sat, Oct 05, 2019 at 06:12:37AM +0000, Bernd Edlinger wrote: > On 10/3/19 5:25 PM, Jakub Jelinek wrote: > > What effect does this have on the cc1/cc1plus .text sizes? > > r276457: > > with patch, --enable-languages=all --enable-checking=yes,rtl > $ size gcc/cc1 > text data bss dec hex filename > 35117708 50984 1388192 36556884 22dd054 gcc/cc1 > $ size gcc/cc1plus > text data bss dec hex filename > 37871569 54640 1391936 39318145 257f281 gcc/cc1plus > > unpatched, --enable-languages=all --enable-checking=yes,rtl > $ size gcc/cc1 > text data bss dec hex filename > 36972031 50984 1388192 38411207 24a1bc7 gcc/cc1 > $ size gcc/cc1plus > text data bss dec hex filename > 39725980 54640 1391936 41172556 2743e4c gcc/cc1plus > > I am a bit surprised, that the patch gives smaller code. I used not the original > Version of this patch, but the one based on Richard's suggestion. In that case, can you check if all the non-failed checks are actually inlined? We do want to inline the if (x->code != 123) check_failed (...); and don't want to inline the check_failed (...). With partial inlining, it might be ok if we inline the if and don't inline the actual caller of check_failed, but I'd worry about checks for multiple codes if we inline from if (x->code != 123 && x->code != 235 && x->code != 348) inline say just x->code != 123 or x->code != 123 && x->code != 235 and leave the rest in the out of line function. > > Does this affect debuggability of --enable-checking=yes,rtl compilers? > > I mean, often when we replace some macros with inlines step in GDB > > becomes a bigger nightmare, having to go through tons of inline frames. > > gdbinit.in has a lengthy list of inlines to skip in rtl.h, shouldn't this be > > added to that list? Not 100% sure how well it will work on rtl checking > > vs. non-rtl checking builds. > > > > I don't see a big problem here. If I type "s" in gdb it jumps to the check > function and the next s jumps back, adding skip instructions in gdbinit.in > does not seem to have any effect for me, but the debug is not that uncomfortable > anyway. > > Interesting is that gdb also jumps in the check function when I press n. > That is also Independent of the gdbinit.in, seems to be a bug due to inlining > code from another line than the original macro, but nothing terrilby bad. Unfortunately, for me the above two counts as terribly bad, show stopper here. A lot of function calls in RTL are like: rtx_equal_for_memref_p (XEXP (x, 0), XEXP (y, 0)) rtx_equal_p (ENTRY_VALUE_EXP (x), ENTRY_VALUE_EXP (y)) force_operand (XEXP (dest_mem, 0), target) etc. (just random examples), during debugging one is absolutely uninterested in stepping into the implementation of XEXP, you know what it means, you want to go stright into the rtx_equal_for_memref_p etc. call. It is already bad that one has to step through the poly_int* stuff or rhs_regno for REGNO (we should add rhs_regno to gdbinit.in and some of the poly_int* stuff too). And no, one can't use n instead of s, because then the whole call is skipped. b rtx_equal_for_memref_p; n works, but it is time consuming and one needs to delete the breakpoint again. Jakub
On 10/5/19 9:24 AM, Jakub Jelinek wrote: > On Sat, Oct 05, 2019 at 06:12:37AM +0000, Bernd Edlinger wrote: >> On 10/3/19 5:25 PM, Jakub Jelinek wrote: >>> What effect does this have on the cc1/cc1plus .text sizes? >> >> r276457: >> >> with patch, --enable-languages=all --enable-checking=yes,rtl >> $ size gcc/cc1 >> text data bss dec hex filename >> 35117708 50984 1388192 36556884 22dd054 gcc/cc1 >> $ size gcc/cc1plus >> text data bss dec hex filename >> 37871569 54640 1391936 39318145 257f281 gcc/cc1plus >> >> unpatched, --enable-languages=all --enable-checking=yes,rtl >> $ size gcc/cc1 >> text data bss dec hex filename >> 36972031 50984 1388192 38411207 24a1bc7 gcc/cc1 >> $ size gcc/cc1plus >> text data bss dec hex filename >> 39725980 54640 1391936 41172556 2743e4c gcc/cc1plus >> >> I am a bit surprised, that the patch gives smaller code. I used not the original >> Version of this patch, but the one based on Richard's suggestion. > > In that case, can you check if all the non-failed checks are actually > inlined? We do want to inline the if (x->code != 123) check_failed (...); > and don't want to inline the check_failed (...). With partial inlining, it > might be ok if we inline the if and don't inline the actual caller of > check_failed, but I'd worry about checks for multiple codes if we inline > from if (x->code != 123 && x->code != 235 && x->code != 348) inline say > just x->code != 123 or x->code != 123 && x->code != 235 and leave the rest > in the out of line function. > Ah, the check functions were not inlined at all because an inline keyword was missing, that explains the very small text size. So in the attached patch, I merged all checking code into one template, added an inline keyword, and now it is inlined as expected, the code is still a bit smaller than the original implementation, though. I don't know why exactly the size is smaller, but everything looks completely correct now (numbers are again for r276457 with attached patch): $ size gcc/cc1 text data bss dec hex filename 36222783 50984 1388192 37661959 23ead07 gcc/cc1 $ size gcc/cc1plus text data bss dec hex filename 38976708 54640 1391936 40423284 268cf74 gcc/cc1plus I added a skip for all of rtl.h, since otherwise gdb would need all possible template invocations of rtl_check_code<> and rtl_check_bounds<> in the skip statement. n works now, but s only in the stage1 compiler. In the stage3 compiler s steps one line into the inlined code and then back to the called function. But that is generally the case for all inline functions, like INSN_UID and contains_struct_check which is in tree.h, should be skipped, but same here. That does not happen with the statement expression since all code is on the same line. Is that a show stopper for this patch, or just a bug in the gdb? >>> Does this affect debuggability of --enable-checking=yes,rtl compilers? >>> I mean, often when we replace some macros with inlines step in GDB >>> becomes a bigger nightmare, having to go through tons of inline frames. >>> gdbinit.in has a lengthy list of inlines to skip in rtl.h, shouldn't this be >>> added to that list? Not 100% sure how well it will work on rtl checking >>> vs. non-rtl checking builds. >>> >> >> I don't see a big problem here. If I type "s" in gdb it jumps to the check >> function and the next s jumps back, adding skip instructions in gdbinit.in >> does not seem to have any effect for me, but the debug is not that uncomfortable >> anyway. >> >> Interesting is that gdb also jumps in the check function when I press n. >> That is also Independent of the gdbinit.in, seems to be a bug due to inlining >> code from another line than the original macro, but nothing terrilby bad. > the issue with n is gone now. the s works in stage1 now, but not in stage3. Attached is an improved patch that fixes the inline issue and adds all of rtl.h (including rhs_regno) to the exclusions. Don't know if there is still a consensus to add -Wshadow=local or if the resistance is too big, then it is probably fine to keep the rtl checking code as-is. Bernd. > Unfortunately, for me the above two counts as terribly bad, show > stopper here. A lot of function calls in RTL are like: > rtx_equal_for_memref_p (XEXP (x, 0), XEXP (y, 0)) > rtx_equal_p (ENTRY_VALUE_EXP (x), ENTRY_VALUE_EXP (y)) > force_operand (XEXP (dest_mem, 0), target) > etc. (just random examples), during debugging one is absolutely > uninterested in stepping into the implementation of XEXP, you know what it > means, you want to go stright into the rtx_equal_for_memref_p etc. call. > It is already bad that one has to step through the poly_int* stuff or > rhs_regno for REGNO (we should add rhs_regno to gdbinit.in and some of the > poly_int* stuff too). And no, one can't use n instead of s, because then > the whole call is skipped. b rtx_equal_for_memref_p; n works, but it is > time consuming and one needs to delete the breakpoint again. > > Jakub >
On 10/5/19 9:24 AM, Jakub Jelinek wrote: > On Sat, Oct 05, 2019 at 06:12:37AM +0000, Bernd Edlinger wrote: >> On 10/3/19 5:25 PM, Jakub Jelinek wrote: >>> Does this affect debuggability of --enable-checking=yes,rtl compilers? >>> I mean, often when we replace some macros with inlines step in GDB >>> becomes a bigger nightmare, having to go through tons of inline frames. >>> gdbinit.in has a lengthy list of inlines to skip in rtl.h, shouldn't this be >>> added to that list? Not 100% sure how well it will work on rtl checking >>> vs. non-rtl checking builds. >>> >> >> I don't see a big problem here. If I type "s" in gdb it jumps to the check >> function and the next s jumps back, adding skip instructions in gdbinit.in >> does not seem to have any effect for me, but the debug is not that uncomfortable >> anyway. >> >> Interesting is that gdb also jumps in the check function when I press n. >> That is also Independent of the gdbinit.in, seems to be a bug due to inlining >> code from another line than the original macro, but nothing terrilby bad. > > Unfortunately, for me the above two counts as terribly bad, show > stopper here. A lot of function calls in RTL are like: > rtx_equal_for_memref_p (XEXP (x, 0), XEXP (y, 0)) > rtx_equal_p (ENTRY_VALUE_EXP (x), ENTRY_VALUE_EXP (y)) > force_operand (XEXP (dest_mem, 0), target) > etc. (just random examples), during debugging one is absolutely > uninterested in stepping into the implementation of XEXP, you know what it > means, you want to go stright into the rtx_equal_for_memref_p etc. call. > It is already bad that one has to step through the poly_int* stuff or > rhs_regno for REGNO (we should add rhs_regno to gdbinit.in and some of the > poly_int* stuff too). And no, one can't use n instead of s, because then > the whole call is skipped. b rtx_equal_for_memref_p; n works, but it is > time consuming and one needs to delete the breakpoint again. > Okay, I think I have fixed both the "s" ignoring the skip status on inlined subroutines, with a gdb-patch I posted here: http://sourceware.org/ml/gdb-patches/2019-10/msg00685.html And the "n" jumping in the bottom half if the inlined template, will be fixed by the patch "Fix dwarf-lineinfo inconsistency of inlined subroutines" which I posted here: https://gcc.gnu.org/ml/gcc-patches/2019-10/msg01459.html Those only affect optimized code, and are already there with the tree.h as you will probably know. With those two patches the rtl-checking patch creates both on non-optimized stage-1 compiler and an optimized stage-3 compiler a very consistent debug impression. I have cleaned up the rtl checking patch once more, removed the no longer used RTL_CHECKC3, XC3EXP macros, and found a way to suppress a template specialization in gdbinit.in using a regular expression matching syntax, instead of suppressing all of rtl.h, that might be more specific than suppressing per file. Suppressing all of rtl.h would work as well, if that is considered a better approach. Now I think the show-stoppers, regarding debuggability of the template-driven rtl-checking macros are no longer an issue, right? Also performance-wise it is better than what we had before, IMHO. Bootstrapped and reg-tested on x86_64-pc-linux-gnu. Is it OK for trunk? Thanks Bernd.
2019-10-03 Bernd Edlinger <bernd.edlinger@hotmail.de> * rtl.h (RTL_FLAG_CHECK): New variadic macro. (RTL_FLAG_CHECK1-6): Use RTL_FLAG_CHECK. (RTL_FLAG_CHECK7): Remove. (rtl_flag_check, check_rtl_code): New helper functions. Index: gcc/rtl.h =================================================================== --- gcc/rtl.h (revision 276484) +++ gcc/rtl.h (working copy) @@ -1249,66 +1249,18 @@ extern void rtvec_check_failed_bounds (const_rtvec #define RTX_FLAG(RTX, FLAG) ((RTX)->FLAG) #if defined ENABLE_RTL_FLAG_CHECKING && (GCC_VERSION >= 2007) -#define RTL_FLAG_CHECK1(NAME, RTX, C1) __extension__ \ -({ __typeof (RTX) const _rtx = (RTX); \ - if (GET_CODE (_rtx) != C1) \ - rtl_check_failed_flag (NAME, _rtx, __FILE__, __LINE__, \ - __FUNCTION__); \ - _rtx; }) +#define RTL_FLAG_CHECK(NAME, RTX, ...) \ + ((__typeof (&*(RTX))) rtl_flag_check (check_rtl_code<__VA_ARGS__>, \ + NAME, RTX, __FILE__, __LINE__, \ + __FUNCTION__)) -#define RTL_FLAG_CHECK2(NAME, RTX, C1, C2) __extension__ \ -({ __typeof (RTX) const _rtx = (RTX); \ - if (GET_CODE (_rtx) != C1 && GET_CODE(_rtx) != C2) \ - rtl_check_failed_flag (NAME,_rtx, __FILE__, __LINE__, \ - __FUNCTION__); \ - _rtx; }) +#define RTL_FLAG_CHECK1 RTL_FLAG_CHECK +#define RTL_FLAG_CHECK2 RTL_FLAG_CHECK +#define RTL_FLAG_CHECK3 RTL_FLAG_CHECK +#define RTL_FLAG_CHECK4 RTL_FLAG_CHECK +#define RTL_FLAG_CHECK5 RTL_FLAG_CHECK +#define RTL_FLAG_CHECK6 RTL_FLAG_CHECK -#define RTL_FLAG_CHECK3(NAME, RTX, C1, C2, C3) __extension__ \ -({ __typeof (RTX) const _rtx = (RTX); \ - if (GET_CODE (_rtx) != C1 && GET_CODE(_rtx) != C2 \ - && GET_CODE (_rtx) != C3) \ - rtl_check_failed_flag (NAME, _rtx, __FILE__, __LINE__, \ - __FUNCTION__); \ - _rtx; }) - -#define RTL_FLAG_CHECK4(NAME, RTX, C1, C2, C3, C4) __extension__ \ -({ __typeof (RTX) const _rtx = (RTX); \ - if (GET_CODE (_rtx) != C1 && GET_CODE(_rtx) != C2 \ - && GET_CODE (_rtx) != C3 && GET_CODE(_rtx) != C4) \ - rtl_check_failed_flag (NAME, _rtx, __FILE__, __LINE__, \ - __FUNCTION__); \ - _rtx; }) - -#define RTL_FLAG_CHECK5(NAME, RTX, C1, C2, C3, C4, C5) __extension__ \ -({ __typeof (RTX) const _rtx = (RTX); \ - if (GET_CODE (_rtx) != C1 && GET_CODE (_rtx) != C2 \ - && GET_CODE (_rtx) != C3 && GET_CODE (_rtx) != C4 \ - && GET_CODE (_rtx) != C5) \ - rtl_check_failed_flag (NAME, _rtx, __FILE__, __LINE__, \ - __FUNCTION__); \ - _rtx; }) - -#define RTL_FLAG_CHECK6(NAME, RTX, C1, C2, C3, C4, C5, C6) \ - __extension__ \ -({ __typeof (RTX) const _rtx = (RTX); \ - if (GET_CODE (_rtx) != C1 && GET_CODE (_rtx) != C2 \ - && GET_CODE (_rtx) != C3 && GET_CODE (_rtx) != C4 \ - && GET_CODE (_rtx) != C5 && GET_CODE (_rtx) != C6) \ - rtl_check_failed_flag (NAME,_rtx, __FILE__, __LINE__, \ - __FUNCTION__); \ - _rtx; }) - -#define RTL_FLAG_CHECK7(NAME, RTX, C1, C2, C3, C4, C5, C6, C7) \ - __extension__ \ -({ __typeof (RTX) const _rtx = (RTX); \ - if (GET_CODE (_rtx) != C1 && GET_CODE (_rtx) != C2 \ - && GET_CODE (_rtx) != C3 && GET_CODE (_rtx) != C4 \ - && GET_CODE (_rtx) != C5 && GET_CODE (_rtx) != C6 \ - && GET_CODE (_rtx) != C7) \ - rtl_check_failed_flag (NAME, _rtx, __FILE__, __LINE__, \ - __FUNCTION__); \ - _rtx; }) - #define RTL_INSN_CHAIN_FLAG_CHECK(NAME, RTX) \ __extension__ \ ({ __typeof (RTX) const _rtx = (RTX); \ @@ -1322,6 +1274,87 @@ extern void rtl_check_failed_flag (const char *, c ATTRIBUTE_NORETURN ATTRIBUTE_COLD ; +static inline rtx +rtl_flag_check (bool (*check) (const_rtx), const char *name, rtx rtl, + const char *file, int line, const char *func) +{ + if (!check (rtl)) + rtl_check_failed_flag (name, rtl, file, line, func); + return rtl; +} + +static inline const_rtx +rtl_flag_check (bool (*check) (const_rtx), const char *name, const_rtx rtl, + const char *file, int line, const char *func) +{ + if (!check (rtl)) + rtl_check_failed_flag (name, rtl, file, line, func); + return rtl; +} + +template<RTX_CODE C1> +inline bool +check_rtl_code (const_rtx rtl) +{ + if (GET_CODE (rtl) != C1) + return false; + return true; +} + +template<RTX_CODE C1, RTX_CODE C2> +inline bool +check_rtl_code (const_rtx rtl) +{ + if (GET_CODE (rtl) != C1 && GET_CODE (rtl) != C2) + return false; + return true; +} + +template<RTX_CODE C1, RTX_CODE C2, RTX_CODE C3> +inline bool +check_rtl_code (const_rtx rtl) +{ + if (GET_CODE (rtl) != C1 && GET_CODE (rtl) != C2 + && GET_CODE (rtl) != C3) + return false; + return true; +} + +template<RTX_CODE C1, RTX_CODE C2, RTX_CODE C3, + RTX_CODE C4> +inline bool +check_rtl_code (const_rtx rtl) +{ + if (GET_CODE (rtl) != C1 && GET_CODE (rtl) != C2 + && GET_CODE (rtl) != C3 && GET_CODE (rtl) != C4) + return false; + return true; +} + +template<RTX_CODE C1, RTX_CODE C2, RTX_CODE C3, + RTX_CODE C4, RTX_CODE C5> +inline bool +check_rtl_code (const_rtx rtl) +{ + if (GET_CODE (rtl) != C1 && GET_CODE (rtl) != C2 + && GET_CODE (rtl) != C3 && GET_CODE (rtl) != C4 + && GET_CODE (rtl) != C5) + return false; + return true; +} + +template<RTX_CODE C1, RTX_CODE C2, RTX_CODE C3, + RTX_CODE C4, RTX_CODE C5, RTX_CODE C6> +inline bool +check_rtl_code (const_rtx rtl) +{ + if (GET_CODE (rtl) != C1 && GET_CODE (rtl) != C2 + && GET_CODE (rtl) != C3 && GET_CODE (rtl) != C4 + && GET_CODE (rtl) != C5 && GET_CODE (rtl) != C6) + return false; + return true; +} + #else /* not ENABLE_RTL_FLAG_CHECKING */ #define RTL_FLAG_CHECK1(NAME, RTX, C1) (RTX) @@ -1330,7 +1363,6 @@ extern void rtl_check_failed_flag (const char *, c #define RTL_FLAG_CHECK4(NAME, RTX, C1, C2, C3, C4) (RTX) #define RTL_FLAG_CHECK5(NAME, RTX, C1, C2, C3, C4, C5) (RTX) #define RTL_FLAG_CHECK6(NAME, RTX, C1, C2, C3, C4, C5, C6) (RTX) -#define RTL_FLAG_CHECK7(NAME, RTX, C1, C2, C3, C4, C5, C6, C7) (RTX) #define RTL_INSN_CHAIN_FLAG_CHECK(NAME, RTX) (RTX) #endif