* [PATCH] avr: Set param_min_pagesize to 0 [PR105523] @ 2023-04-26 9:00 SenthilKumar.Selvaraj 2023-04-26 9:42 ` Richard Biener 0 siblings, 1 reply; 12+ messages in thread From: SenthilKumar.Selvaraj @ 2023-04-26 9:00 UTC (permalink / raw) To: gcc-patches Hi, This patch fixes PR 105523 by setting param_min_pagesize to 0 for the avr target. For this target, zero and offsets from zero are perfectly valid addresses, and the default value of param_min_pagesize ends up triggering warnings on valid memory accesses. Ok for trunk and backporting to 13 and 12 branches? Regards Senthil PR target/105523 gcc/ChangeLog: * config/avr/avr.cc (avr_option_override): Set param_min_pagesize to 0. gcc/testsuite/ChangeLog: * gcc.target/avr/pr105523.c: New test. diff --git a/gcc/config/avr/avr.cc b/gcc/config/avr/avr.cc index c193430cf07..3b862f4e4ac 100644 --- a/gcc/config/avr/avr.cc +++ b/gcc/config/avr/avr.cc @@ -56,6 +56,7 @@ #include "tree-pass.h" #include "print-rtl.h" #include "rtl-iter.h" +#include "opts.h" /* This file should be included last. */ #include "target-def.h" @@ -769,6 +770,9 @@ avr_option_override (void) avr_gasisr_prologues = 0; #endif + SET_OPTION_IF_UNSET (&global_options, &global_options_set, + param_min_pagesize, 0); + if (!avr_set_core_architecture()) return; diff --git a/gcc/testsuite/gcc.target/avr/pr105523.c b/gcc/testsuite/gcc.target/avr/pr105523.c new file mode 100644 index 00000000000..fbbf7bf4422 --- /dev/null +++ b/gcc/testsuite/gcc.target/avr/pr105523.c @@ -0,0 +1,14 @@ +/* { dg-do compile } */ +/* { dg-options "-Os -Wall" } */ + +/* Verify no "array subscript 0 is outside array bounds of" is generated + for accessing memory addresses in the 0-4096 range. */ + +typedef __UINT8_TYPE__ uint8_t; + +#define SREG (*(volatile uint8_t*) (0x3F + __AVR_SFR_OFFSET__ )) + +void bar (void) +{ + SREG = 0; +} ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] avr: Set param_min_pagesize to 0 [PR105523] 2023-04-26 9:00 [PATCH] avr: Set param_min_pagesize to 0 [PR105523] SenthilKumar.Selvaraj @ 2023-04-26 9:42 ` Richard Biener 2023-04-26 9:43 ` Richard Biener 0 siblings, 1 reply; 12+ messages in thread From: Richard Biener @ 2023-04-26 9:42 UTC (permalink / raw) To: SenthilKumar.Selvaraj; +Cc: gcc-patches On Wed, Apr 26, 2023 at 11:01 AM SenthilKumar.Selvaraj--- via Gcc-patches <gcc-patches@gcc.gnu.org> wrote: > > Hi, > > This patch fixes PR 105523 by setting param_min_pagesize to 0 for the > avr target. For this target, zero and offsets from zero are perfectly > valid addresses, and the default value of param_min_pagesize ends up > triggering warnings on valid memory accesses. I think the proper configuration is to have DEFAULT_ADDR_SPACE_ZERO_ADDRESS_VALID defined to something returning true instead. Richard. > Ok for trunk and backporting to 13 and 12 branches? > > Regards > Senthil > > PR target/105523 > > gcc/ChangeLog: > > * config/avr/avr.cc (avr_option_override): Set > param_min_pagesize to 0. > > gcc/testsuite/ChangeLog: > > * gcc.target/avr/pr105523.c: New test. > > diff --git a/gcc/config/avr/avr.cc b/gcc/config/avr/avr.cc > index c193430cf07..3b862f4e4ac 100644 > --- a/gcc/config/avr/avr.cc > +++ b/gcc/config/avr/avr.cc > @@ -56,6 +56,7 @@ > #include "tree-pass.h" > #include "print-rtl.h" > #include "rtl-iter.h" > +#include "opts.h" > > /* This file should be included last. */ > #include "target-def.h" > @@ -769,6 +770,9 @@ avr_option_override (void) > avr_gasisr_prologues = 0; > #endif > > + SET_OPTION_IF_UNSET (&global_options, &global_options_set, > + param_min_pagesize, 0); > + > if (!avr_set_core_architecture()) > return; > > diff --git a/gcc/testsuite/gcc.target/avr/pr105523.c b/gcc/testsuite/gcc.target/avr/pr105523.c > new file mode 100644 > index 00000000000..fbbf7bf4422 > --- /dev/null > +++ b/gcc/testsuite/gcc.target/avr/pr105523.c > @@ -0,0 +1,14 @@ > +/* { dg-do compile } */ > +/* { dg-options "-Os -Wall" } */ > + > +/* Verify no "array subscript 0 is outside array bounds of" is generated > + for accessing memory addresses in the 0-4096 range. */ > + > +typedef __UINT8_TYPE__ uint8_t; > + > +#define SREG (*(volatile uint8_t*) (0x3F + __AVR_SFR_OFFSET__ )) > + > +void bar (void) > +{ > + SREG = 0; > +} > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] avr: Set param_min_pagesize to 0 [PR105523] 2023-04-26 9:42 ` Richard Biener @ 2023-04-26 9:43 ` Richard Biener 2023-04-26 10:56 ` SenthilKumar.Selvaraj 0 siblings, 1 reply; 12+ messages in thread From: Richard Biener @ 2023-04-26 9:43 UTC (permalink / raw) To: SenthilKumar.Selvaraj; +Cc: gcc-patches On Wed, Apr 26, 2023 at 11:42 AM Richard Biener <richard.guenther@gmail.com> wrote: > > On Wed, Apr 26, 2023 at 11:01 AM SenthilKumar.Selvaraj--- via > Gcc-patches <gcc-patches@gcc.gnu.org> wrote: > > > > Hi, > > > > This patch fixes PR 105523 by setting param_min_pagesize to 0 for the > > avr target. For this target, zero and offsets from zero are perfectly > > valid addresses, and the default value of param_min_pagesize ends up > > triggering warnings on valid memory accesses. > > I think the proper configuration is to have > DEFAULT_ADDR_SPACE_ZERO_ADDRESS_VALID Err, TARGET_ADDR_SPACE_ZERO_ADDRESS_VALID > defined to something returning true instead. > > Richard. > > > Ok for trunk and backporting to 13 and 12 branches? > > > > Regards > > Senthil > > > > PR target/105523 > > > > gcc/ChangeLog: > > > > * config/avr/avr.cc (avr_option_override): Set > > param_min_pagesize to 0. > > > > gcc/testsuite/ChangeLog: > > > > * gcc.target/avr/pr105523.c: New test. > > > > diff --git a/gcc/config/avr/avr.cc b/gcc/config/avr/avr.cc > > index c193430cf07..3b862f4e4ac 100644 > > --- a/gcc/config/avr/avr.cc > > +++ b/gcc/config/avr/avr.cc > > @@ -56,6 +56,7 @@ > > #include "tree-pass.h" > > #include "print-rtl.h" > > #include "rtl-iter.h" > > +#include "opts.h" > > > > /* This file should be included last. */ > > #include "target-def.h" > > @@ -769,6 +770,9 @@ avr_option_override (void) > > avr_gasisr_prologues = 0; > > #endif > > > > + SET_OPTION_IF_UNSET (&global_options, &global_options_set, > > + param_min_pagesize, 0); > > + > > if (!avr_set_core_architecture()) > > return; > > > > diff --git a/gcc/testsuite/gcc.target/avr/pr105523.c b/gcc/testsuite/gcc.target/avr/pr105523.c > > new file mode 100644 > > index 00000000000..fbbf7bf4422 > > --- /dev/null > > +++ b/gcc/testsuite/gcc.target/avr/pr105523.c > > @@ -0,0 +1,14 @@ > > +/* { dg-do compile } */ > > +/* { dg-options "-Os -Wall" } */ > > + > > +/* Verify no "array subscript 0 is outside array bounds of" is generated > > + for accessing memory addresses in the 0-4096 range. */ > > + > > +typedef __UINT8_TYPE__ uint8_t; > > + > > +#define SREG (*(volatile uint8_t*) (0x3F + __AVR_SFR_OFFSET__ )) > > + > > +void bar (void) > > +{ > > + SREG = 0; > > +} > > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] avr: Set param_min_pagesize to 0 [PR105523] 2023-04-26 9:43 ` Richard Biener @ 2023-04-26 10:56 ` SenthilKumar.Selvaraj 2023-04-26 12:19 ` Richard Biener 0 siblings, 1 reply; 12+ messages in thread From: SenthilKumar.Selvaraj @ 2023-04-26 10:56 UTC (permalink / raw) To: richard.guenther; +Cc: gcc-patches On Wed, Apr 26, 2023 at 3:15 PM Richard Biener via Gcc-patches <gcc-patches@gcc.gnu.org> wrote: > > On Wed, Apr 26, 2023 at 11:42 AM Richard Biener > <richard.guenther@gmail.com> wrote: > > > > On Wed, Apr 26, 2023 at 11:01 AM SenthilKumar.Selvaraj--- via > > Gcc-patches <gcc-patches@gcc.gnu.org> wrote: > > > > > > Hi, > > > > > > This patch fixes PR 105523 by setting param_min_pagesize to 0 for the > > > avr target. For this target, zero and offsets from zero are perfectly > > > valid addresses, and the default value of param_min_pagesize ends up > > > triggering warnings on valid memory accesses. > > > > I think the proper configuration is to have > > DEFAULT_ADDR_SPACE_ZERO_ADDRESS_VALID > > Err, TARGET_ADDR_SPACE_ZERO_ADDRESS_VALID That worked. Ok for trunk and backporting to 13 and 12 branches (pending regression testing)? Regards, Senthil PR 105523 gcc/ChangeLog: * config/avr/avr.cc (avr_addr_space_zero_address_valid): (TARGET_ADDR_SPACE_ZERO_ADDRESS_VALID): Return true. gcc/testsuite/ChangeLog: * gcc.target/avr/pr105523.c: New test. diff --git a/gcc/config/avr/avr.cc b/gcc/config/avr/avr.cc index c193430cf07..5439eb8e55c 100644 --- a/gcc/config/avr/avr.cc +++ b/gcc/config/avr/avr.cc @@ -9788,6 +9788,16 @@ avr_addr_space_diagnose_usage (addr_space_t as, location_t loc) (void) avr_addr_space_supported_p (as, loc); } +/* Implement `TARGET_ADDR_SPACE_ZERO_ADDRESS_VALID. Zero is a valid + address in all address spaces. Even in ADDR_SPACE_FLASH1 etc., + a zero address is valid and means 0x<RAMPZ>0000, where RAMPZ is + set to the appropriate segment value. */ + +static bool +avr_addr_space_zero_address_valid (addr_space_t as) +{ + return true; +} /* Look if DECL shall be placed in program memory space by means of attribute `progmem' or some address-space qualifier. @@ -14688,6 +14698,9 @@ avr_float_lib_compare_returns_bool (machine_mode mode, enum rtx_code) #undef TARGET_ADDR_SPACE_DIAGNOSE_USAGE #define TARGET_ADDR_SPACE_DIAGNOSE_USAGE avr_addr_space_diagnose_usage +#undef TARGET_ADDR_SPACE_ZERO_ADDRESS_VALID +#define TARGET_ADDR_SPACE_ZERO_ADDRESS_VALID avr_addr_space_zero_address_valid + #undef TARGET_MODE_DEPENDENT_ADDRESS_P #define TARGET_MODE_DEPENDENT_ADDRESS_P avr_mode_dependent_address_p diff --git a/gcc/testsuite/gcc.target/avr/pr105523.c b/gcc/testsuite/gcc.target/avr/pr105523.c new file mode 100644 index 00000000000..fbbf7bf4422 --- /dev/null +++ b/gcc/testsuite/gcc.target/avr/pr105523.c @@ -0,0 +1,14 @@ +/* { dg-do compile } */ +/* { dg-options "-Os -Wall" } */ + +/* Verify no "array subscript 0 is outside array bounds of" is generated + for accessing memory addresses in the 0-4096 range. */ + +typedef __UINT8_TYPE__ uint8_t; + +#define SREG (*(volatile uint8_t*) (0x3F + __AVR_SFR_OFFSET__ )) + +void bar (void) +{ + SREG = 0; +} ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] avr: Set param_min_pagesize to 0 [PR105523] 2023-04-26 10:56 ` SenthilKumar.Selvaraj @ 2023-04-26 12:19 ` Richard Biener 2023-05-19 5:58 ` SenthilKumar.Selvaraj 0 siblings, 1 reply; 12+ messages in thread From: Richard Biener @ 2023-04-26 12:19 UTC (permalink / raw) To: SenthilKumar.Selvaraj; +Cc: gcc-patches, Denis Chertykov On Wed, Apr 26, 2023 at 12:56 PM <SenthilKumar.Selvaraj@microchip.com> wrote: > > On Wed, Apr 26, 2023 at 3:15 PM Richard Biener via Gcc-patches <gcc-patches@gcc.gnu.org> wrote: > > > > On Wed, Apr 26, 2023 at 11:42 AM Richard Biener > > <richard.guenther@gmail.com> wrote: > > > > > > On Wed, Apr 26, 2023 at 11:01 AM SenthilKumar.Selvaraj--- via > > > Gcc-patches <gcc-patches@gcc.gnu.org> wrote: > > > > > > > > Hi, > > > > > > > > This patch fixes PR 105523 by setting param_min_pagesize to 0 for the > > > > avr target. For this target, zero and offsets from zero are perfectly > > > > valid addresses, and the default value of param_min_pagesize ends up > > > > triggering warnings on valid memory accesses. > > > > > > I think the proper configuration is to have > > > DEFAULT_ADDR_SPACE_ZERO_ADDRESS_VALID > > > > Err, TARGET_ADDR_SPACE_ZERO_ADDRESS_VALID > > That worked. Ok for trunk and backporting to 13 and 12 branches > (pending regression testing)? OK, but please let Denis time to comment. Richard. > Regards, > Senthil > > PR 105523 > > gcc/ChangeLog: > > * config/avr/avr.cc (avr_addr_space_zero_address_valid): > (TARGET_ADDR_SPACE_ZERO_ADDRESS_VALID): Return true. > > gcc/testsuite/ChangeLog: > > * gcc.target/avr/pr105523.c: New test. > > > > diff --git a/gcc/config/avr/avr.cc b/gcc/config/avr/avr.cc > index c193430cf07..5439eb8e55c 100644 > --- a/gcc/config/avr/avr.cc > +++ b/gcc/config/avr/avr.cc > @@ -9788,6 +9788,16 @@ avr_addr_space_diagnose_usage (addr_space_t as, location_t loc) > (void) avr_addr_space_supported_p (as, loc); > } > > +/* Implement `TARGET_ADDR_SPACE_ZERO_ADDRESS_VALID. Zero is a valid > + address in all address spaces. Even in ADDR_SPACE_FLASH1 etc., > + a zero address is valid and means 0x<RAMPZ>0000, where RAMPZ is > + set to the appropriate segment value. */ > + > +static bool > +avr_addr_space_zero_address_valid (addr_space_t as) > +{ > + return true; > +} > > /* Look if DECL shall be placed in program memory space by > means of attribute `progmem' or some address-space qualifier. > @@ -14688,6 +14698,9 @@ avr_float_lib_compare_returns_bool (machine_mode mode, enum rtx_code) > #undef TARGET_ADDR_SPACE_DIAGNOSE_USAGE > #define TARGET_ADDR_SPACE_DIAGNOSE_USAGE avr_addr_space_diagnose_usage > > +#undef TARGET_ADDR_SPACE_ZERO_ADDRESS_VALID > +#define TARGET_ADDR_SPACE_ZERO_ADDRESS_VALID avr_addr_space_zero_address_valid > + > #undef TARGET_MODE_DEPENDENT_ADDRESS_P > #define TARGET_MODE_DEPENDENT_ADDRESS_P avr_mode_dependent_address_p > > diff --git a/gcc/testsuite/gcc.target/avr/pr105523.c b/gcc/testsuite/gcc.target/avr/pr105523.c > new file mode 100644 > index 00000000000..fbbf7bf4422 > --- /dev/null > +++ b/gcc/testsuite/gcc.target/avr/pr105523.c > @@ -0,0 +1,14 @@ > +/* { dg-do compile } */ > +/* { dg-options "-Os -Wall" } */ > + > +/* Verify no "array subscript 0 is outside array bounds of" is generated > + for accessing memory addresses in the 0-4096 range. */ > + > +typedef __UINT8_TYPE__ uint8_t; > + > +#define SREG (*(volatile uint8_t*) (0x3F + __AVR_SFR_OFFSET__ )) > + > +void bar (void) > +{ > + SREG = 0; > +} > > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] avr: Set param_min_pagesize to 0 [PR105523] 2023-04-26 12:19 ` Richard Biener @ 2023-05-19 5:58 ` SenthilKumar.Selvaraj 2023-05-19 14:02 ` Bernhard Reutner-Fischer 2023-05-22 12:05 ` Richard Biener 0 siblings, 2 replies; 12+ messages in thread From: SenthilKumar.Selvaraj @ 2023-05-19 5:58 UTC (permalink / raw) To: richard.guenther; +Cc: gcc-patches, chertykov On 26/04/23, 5:51 PM, "Richard Biener" <richard.guenther@gmail.com <mailto:richard.guenther@gmail.com>> wrote: > On Wed, Apr 26, 2023 at 12:56 PM <SenthilKumar.Selvaraj@microchip.com <mailto:SenthilKumar.Selvaraj@microchip.com>> wrote: > > > > On Wed, Apr 26, 2023 at 3:15 PM Richard Biener via Gcc-patches <gcc-patches@gcc.gnu.org <mailto:gcc-patches@gcc.gnu.org>> wrote: > > > > > > On Wed, Apr 26, 2023 at 11:42 AM Richard Biener > > > <richard.guenther@gmail.com <mailto:richard.guenther@gmail.com>> wrote: > > > > > > > > On Wed, Apr 26, 2023 at 11:01 AM SenthilKumar.Selvaraj--- via > > > > Gcc-patches <gcc-patches@gcc.gnu.org <mailto:gcc-patches@gcc.gnu.org>> wrote: > > > > > > > > > > Hi, > > > > > > > > > > This patch fixes PR 105523 by setting param_min_pagesize to 0 for the > > > > > avr target. For this target, zero and offsets from zero are perfectly > > > > > valid addresses, and the default value of param_min_pagesize ends up > > > > > triggering warnings on valid memory accesses. > > > > > > > > I think the proper configuration is to have > > > > DEFAULT_ADDR_SPACE_ZERO_ADDRESS_VALID > > > > > > Err, TARGET_ADDR_SPACE_ZERO_ADDRESS_VALID > > > > That worked. Ok for trunk and backporting to 13 and 12 branches > > (pending regression testing)? > > > OK, but please let Denis time to comment. Didn't hear from Denis. When running regression tests with this patch, I found that some tests with -fdelete-null-pointer-checks were failing. Commit 19416210b37db0584cd0b3f3b3961324b8973d25 made -fdelete-null-pointer-checks false by default, while still allowing it to be overridden from the command line (it was previously unconditionally false). To keep the same behavior, I modified the hook to report zero addresses as valid only if -fdelete-null-pointer-checks is not set. With this change, all regression tests pass. Ok for trunk and backporting to 13 and 12 branches? Regards Senthil PR 105523 gcc/ChangeLog: * config/avr/avr.cc (avr_addr_space_zero_address_valid): (TARGET_ADDR_SPACE_ZERO_ADDRESS_VALID): Return true if flag_delete_null_pointer_checks is not set. gcc/testsuite/ChangeLog: * gcc.target/avr/pr105523.c: New test. diff --git gcc/config/avr/avr.cc gcc/config/avr/avr.cc index d5af40f..4c9eb84 100644 --- gcc/config/avr/avr.cc +++ gcc/config/avr/avr.cc @@ -9787,6 +9787,18 @@ avr_addr_space_diagnose_usage (addr_space_t as, location_t loc) (void) avr_addr_space_supported_p (as, loc); } +/* Implement `TARGET_ADDR_SPACE_ZERO_ADDRESS_VALID. Zero is a valid + address in all address spaces. Even in ADDR_SPACE_FLASH1 etc.., + a zero address is valid and means 0x<RAMPZ val>0000, where RAMPZ is + set to the appropriate segment value. + If the user explicitly passes in -fdelete-null-pointer-checks though, + assume zero addresses are invalid.*/ + +static bool +avr_addr_space_zero_address_valid (addr_space_t as ATTRIBUTE_UNUSED) +{ + return flag_delete_null_pointer_checks == 0; +} /* Look if DECL shall be placed in program memory space by means of attribute `progmem' or some address-space qualifier. @@ -14687,6 +14699,9 @@ avr_float_lib_compare_returns_bool (machine_mode mode, enum rtx_code) #undef TARGET_ADDR_SPACE_DIAGNOSE_USAGE #define TARGET_ADDR_SPACE_DIAGNOSE_USAGE avr_addr_space_diagnose_usage +#undef TARGET_ADDR_SPACE_ZERO_ADDRESS_VALID +#define TARGET_ADDR_SPACE_ZERO_ADDRESS_VALID avr_addr_space_zero_address_valid + #undef TARGET_MODE_DEPENDENT_ADDRESS_P #define TARGET_MODE_DEPENDENT_ADDRESS_P avr_mode_dependent_address_p diff --git gcc/testsuite/gcc.target/avr/pr105523.c gcc/testsuite/gcc.target/avr/pr105523.c new file mode 100644 index 0000000..fbbf7bf --- /dev/null +++ gcc/testsuite/gcc.target/avr/pr105523.c @@ -0,0 +1,14 @@ +/* { dg-do compile } */ +/* { dg-options "-Os -Wall" } */ + +/* Verify no "array subscript 0 is outside array bounds of" is generated + for accessing memory addresses in the 0-4096 range. */ + +typedef __UINT8_TYPE__ uint8_t; + +#define SREG (*(volatile uint8_t*) (0x3F + __AVR_SFR_OFFSET__ )) + +void bar (void) +{ + SREG = 0; +} ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] avr: Set param_min_pagesize to 0 [PR105523] 2023-05-19 5:58 ` SenthilKumar.Selvaraj @ 2023-05-19 14:02 ` Bernhard Reutner-Fischer 2023-05-19 16:51 ` Jeff Law 2023-05-22 12:05 ` Richard Biener 1 sibling, 1 reply; 12+ messages in thread From: Bernhard Reutner-Fischer @ 2023-05-19 14:02 UTC (permalink / raw) To: SenthilKumar.Selvaraj, SenthilKumar.Selvaraj--- via Gcc-patches, richard.guenther Cc: gcc-patches, chertykov On 19 May 2023 07:58:48 CEST, "SenthilKumar.Selvaraj--- via Gcc-patches" <gcc-patches@gcc.gnu.org> wrote: Just a nit: >+static bool >+avr_addr_space_zero_address_valid (addr_space_t as ATTRIBUTE_UNUSED) >+{ >+ return flag_delete_null_pointer_checks == 0; >+} Since we are c++ nowadays, you can omit the parameter name for unused arguments. I.e.: static bool avr_addr_space_zero_address_valid (addr_space_t) { ... ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] avr: Set param_min_pagesize to 0 [PR105523] 2023-05-19 14:02 ` Bernhard Reutner-Fischer @ 2023-05-19 16:51 ` Jeff Law 0 siblings, 0 replies; 12+ messages in thread From: Jeff Law @ 2023-05-19 16:51 UTC (permalink / raw) To: Bernhard Reutner-Fischer, SenthilKumar.Selvaraj, SenthilKumar.Selvaraj--- via Gcc-patches, richard.guenther Cc: chertykov On 5/19/23 08:02, Bernhard Reutner-Fischer via Gcc-patches wrote: > On 19 May 2023 07:58:48 CEST, "SenthilKumar.Selvaraj--- via Gcc-patches" <gcc-patches@gcc.gnu.org> wrote: > > Just a nit: > >> +static bool >> +avr_addr_space_zero_address_valid (addr_space_t as ATTRIBUTE_UNUSED) >> +{ >> + return flag_delete_null_pointer_checks == 0; >> +} > > Since we are c++ nowadays, you can omit the parameter name for unused arguments. I.e.: > > static bool > avr_addr_space_zero_address_valid (addr_space_t) > { Right. And I strongly prefer that over ATTRIBUTE_UNUSED. So OK for the trunk with that change. jeff ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] avr: Set param_min_pagesize to 0 [PR105523] 2023-05-19 5:58 ` SenthilKumar.Selvaraj 2023-05-19 14:02 ` Bernhard Reutner-Fischer @ 2023-05-22 12:05 ` Richard Biener 2023-06-02 7:02 ` SenthilKumar.Selvaraj 1 sibling, 1 reply; 12+ messages in thread From: Richard Biener @ 2023-05-22 12:05 UTC (permalink / raw) To: SenthilKumar.Selvaraj; +Cc: gcc-patches, chertykov On Fri, May 19, 2023 at 7:58 AM <SenthilKumar.Selvaraj@microchip.com> wrote: > > On 26/04/23, 5:51 PM, "Richard Biener" <richard.guenther@gmail.com <mailto:richard.guenther@gmail.com>> wrote: > > On Wed, Apr 26, 2023 at 12:56 PM <SenthilKumar.Selvaraj@microchip.com <mailto:SenthilKumar.Selvaraj@microchip.com>> wrote: > > > > > > On Wed, Apr 26, 2023 at 3:15 PM Richard Biener via Gcc-patches <gcc-patches@gcc.gnu.org <mailto:gcc-patches@gcc.gnu.org>> wrote: > > > > > > > > On Wed, Apr 26, 2023 at 11:42 AM Richard Biener > > > > <richard.guenther@gmail.com <mailto:richard.guenther@gmail.com>> wrote: > > > > > > > > > > On Wed, Apr 26, 2023 at 11:01 AM SenthilKumar.Selvaraj--- via > > > > > Gcc-patches <gcc-patches@gcc.gnu.org <mailto:gcc-patches@gcc.gnu.org>> wrote: > > > > > > > > > > > > Hi, > > > > > > > > > > > > This patch fixes PR 105523 by setting param_min_pagesize to 0 for the > > > > > > avr target. For this target, zero and offsets from zero are perfectly > > > > > > valid addresses, and the default value of param_min_pagesize ends up > > > > > > triggering warnings on valid memory accesses. > > > > > > > > > > I think the proper configuration is to have > > > > > DEFAULT_ADDR_SPACE_ZERO_ADDRESS_VALID > > > > > > > > Err, TARGET_ADDR_SPACE_ZERO_ADDRESS_VALID > > > > > > That worked. Ok for trunk and backporting to 13 and 12 branches > > > (pending regression testing)? > > > > > > OK, but please let Denis time to comment. > > Didn't hear from Denis. When running regression tests with this patch, > I found that some tests with -fdelete-null-pointer-checks were > failing. Commit 19416210b37db0584cd0b3f3b3961324b8973d25 made > -fdelete-null-pointer-checks false by default, while still allowing it > to be overridden from the command line (it was previously > unconditionally false). > > To keep the same behavior, I modified the hook to report zero > addresses as valid only if -fdelete-null-pointer-checks is not set. > With this change, all regression tests pass. > > Ok for trunk and backporting to 13 and 12 branches? I think that's bit backwards - this hook conveys more precise information (it's address-space specific) and it is also more specific. Instead I'd suggest to set the flag to zero in the target like nios2 or msp430 do. In fact we should probably initialize it using this hook (and using the default address space). Richard. > Regards > Senthil > > PR 105523 > > gcc/ChangeLog: > > * config/avr/avr.cc (avr_addr_space_zero_address_valid): > (TARGET_ADDR_SPACE_ZERO_ADDRESS_VALID): Return true if > flag_delete_null_pointer_checks is not set. > > gcc/testsuite/ChangeLog: > > * gcc.target/avr/pr105523.c: New test. > > > diff --git gcc/config/avr/avr.cc gcc/config/avr/avr.cc > index d5af40f..4c9eb84 100644 > --- gcc/config/avr/avr.cc > +++ gcc/config/avr/avr.cc > @@ -9787,6 +9787,18 @@ avr_addr_space_diagnose_usage (addr_space_t as, location_t loc) > (void) avr_addr_space_supported_p (as, loc); > } > > +/* Implement `TARGET_ADDR_SPACE_ZERO_ADDRESS_VALID. Zero is a valid > + address in all address spaces. Even in ADDR_SPACE_FLASH1 etc.., > + a zero address is valid and means 0x<RAMPZ val>0000, where RAMPZ is > + set to the appropriate segment value. > + If the user explicitly passes in -fdelete-null-pointer-checks though, > + assume zero addresses are invalid.*/ > + > +static bool > +avr_addr_space_zero_address_valid (addr_space_t as ATTRIBUTE_UNUSED) > +{ > + return flag_delete_null_pointer_checks == 0; > +} > > /* Look if DECL shall be placed in program memory space by > means of attribute `progmem' or some address-space qualifier. > @@ -14687,6 +14699,9 @@ avr_float_lib_compare_returns_bool (machine_mode mode, enum rtx_code) > #undef TARGET_ADDR_SPACE_DIAGNOSE_USAGE > #define TARGET_ADDR_SPACE_DIAGNOSE_USAGE avr_addr_space_diagnose_usage > > +#undef TARGET_ADDR_SPACE_ZERO_ADDRESS_VALID > +#define TARGET_ADDR_SPACE_ZERO_ADDRESS_VALID avr_addr_space_zero_address_valid > + > #undef TARGET_MODE_DEPENDENT_ADDRESS_P > #define TARGET_MODE_DEPENDENT_ADDRESS_P avr_mode_dependent_address_p > > diff --git gcc/testsuite/gcc.target/avr/pr105523.c gcc/testsuite/gcc.target/avr/pr105523.c > new file mode 100644 > index 0000000..fbbf7bf > --- /dev/null > +++ gcc/testsuite/gcc.target/avr/pr105523.c > @@ -0,0 +1,14 @@ > +/* { dg-do compile } */ > +/* { dg-options "-Os -Wall" } */ > + > +/* Verify no "array subscript 0 is outside array bounds of" is generated > + for accessing memory addresses in the 0-4096 range. */ > + > +typedef __UINT8_TYPE__ uint8_t; > + > +#define SREG (*(volatile uint8_t*) (0x3F + __AVR_SFR_OFFSET__ )) > + > +void bar (void) > +{ > + SREG = 0; > +} > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] avr: Set param_min_pagesize to 0 [PR105523] 2023-05-22 12:05 ` Richard Biener @ 2023-06-02 7:02 ` SenthilKumar.Selvaraj 2023-06-16 10:17 ` [Ping] " SenthilKumar.Selvaraj 0 siblings, 1 reply; 12+ messages in thread From: SenthilKumar.Selvaraj @ 2023-06-02 7:02 UTC (permalink / raw) To: richard.guenther; +Cc: gcc-patches, chertykov On Mon, 2023-05-22 at 14:05 +0200, Richard Biener wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On Fri, May 19, 2023 at 7:58 AM <SenthilKumar.Selvaraj@microchip.com> wrote: > > On 26/04/23, 5:51 PM, "Richard Biener" <richard.guenther@gmail.com <mailto:richard.guenther@gmail.com>> wrote: > > > On Wed, Apr 26, 2023 at 12:56 PM <SenthilKumar.Selvaraj@microchip.com <mailto:SenthilKumar.Selvaraj@microchip.com>> wrote: > > > > On Wed, Apr 26, 2023 at 3:15 PM Richard Biener via Gcc-patches <gcc-patches@gcc.gnu.org <mailto:gcc-patches@gcc.gnu.org>> wrote: > > > > > On Wed, Apr 26, 2023 at 11:42 AM Richard Biener > > > > > <richard.guenther@gmail.com <mailto:richard.guenther@gmail.com>> wrote: > > > > > > On Wed, Apr 26, 2023 at 11:01 AM SenthilKumar.Selvaraj--- via > > > > > > Gcc-patches <gcc-patches@gcc.gnu.org <mailto:gcc-patches@gcc.gnu.org>> wrote: > > > > > > > Hi, > > > > > > > > > > > > > > This patch fixes PR 105523 by setting param_min_pagesize to 0 for the > > > > > > > avr target. For this target, zero and offsets from zero are perfectly > > > > > > > valid addresses, and the default value of param_min_pagesize ends up > > > > > > > triggering warnings on valid memory accesses. > > > > > > > > > > > > I think the proper configuration is to have > > > > > > DEFAULT_ADDR_SPACE_ZERO_ADDRESS_VALID > > > > > > > > > > Err, TARGET_ADDR_SPACE_ZERO_ADDRESS_VALID > > > > > > > > That worked. Ok for trunk and backporting to 13 and 12 branches > > > > (pending regression testing)? > > > > > > OK, but please let Denis time to comment. > > > > Didn't hear from Denis. When running regression tests with this patch, > > I found that some tests with -fdelete-null-pointer-checks were > > failing. Commit 19416210b37db0584cd0b3f3b3961324b8973d25 made > > -fdelete-null-pointer-checks false by default, while still allowing it > > to be overridden from the command line (it was previously > > unconditionally false). > > > > To keep the same behavior, I modified the hook to report zero > > addresses as valid only if -fdelete-null-pointer-checks is not set. > > With this change, all regression tests pass. > > > > Ok for trunk and backporting to 13 and 12 branches? > > I think that's bit backwards - this hook conveys more precise information > (it's address-space specific) and it is also more specific. Instead I'd > suggest to set the flag to zero in the target like nios2 or msp430 do. > In fact we should probably initialize it using this hook (and using the > default address space). Does the below patch work? The hook impl reports that zero address is valid, and flag_delete_null_pointer_checks is set to zero if the hook says zero is a valid address. As flag_delete_null_pointer_checks is now always disabled for avr, I removed the resetting code in avr-common.cc that disables it for OPT_LEVELS_ALL by default, and added avr as a target that always keeps null pointer checks in testsuite/lib/target-supports.exp. I also removed ATTRIBUTE_UNUSED and the parameter name in the target hook to address https://gcc.gnu.org/pipermail/gcc-patches/2023-May/619014.html. PR 105523 gcc/ChangeLog: * common/config/avr/avr-common.cc: Remove setting of OPT_fdelete_null_pointer_checks. * config/avr/avr.cc (avr_option_override): Clear flag_delete_null_pointer_checks if zero_address_valid. (avr_addr_space_zero_address_valid): New function. (TARGET_ADDR_SPACE_ZERO_ADDRESS_VALID): Provide target hook. gcc/testsuite/ChangeLog: * lib/target-supports.exp (check_effective_target_keeps_null_pointer_checks): Add avr. * gcc.target/avr/pr105523.c: New test. diff --git gcc/common/config/avr/avr-common.cc gcc/common/config/avr/avr-common.cc index 2ad0244..2f874c5 100644 --- gcc/common/config/avr/avr-common.cc +++ gcc/common/config/avr/avr-common.cc @@ -29,12 +29,6 @@ /* Implement TARGET_OPTION_OPTIMIZATION_TABLE. */ static const struct default_options avr_option_optimization_table[] = { - // With -fdelete-null-pointer-checks option, the compiler assumes - // that dereferencing of a null pointer would halt the program. - // For AVR this assumption is not true and a program can safely - // dereference null pointers. Changes made by this option may not - // work properly for AVR. So disable this option. - { OPT_LEVELS_ALL, OPT_fdelete_null_pointer_checks, NULL, 0 }, // The only effect of -fcaller-saves might be that it triggers // a frame without need when it tries to be smart around calls. { OPT_LEVELS_ALL, OPT_fcaller_saves, NULL, 0 }, diff --git gcc/config/avr/avr.cc gcc/config/avr/avr.cc index a90cade..b987837 100644 --- gcc/config/avr/avr.cc +++ gcc/config/avr/avr.cc @@ -756,6 +756,10 @@ avr_option_override (void) flag_omit_frame_pointer = 0; } + /* Disable flag_delete_null_pointer_checks if zero is a valid address. */ + if (targetm.addr_space.zero_address_valid (ADDR_SPACE_GENERIC)) + flag_delete_null_pointer_checks = 0; + if (flag_pic == 1) warning (OPT_fpic, "%<-fpic%> is not supported"); if (flag_pic == 2) @@ -9800,6 +9804,16 @@ avr_addr_space_diagnose_usage (addr_space_t as, location_t loc) (void) avr_addr_space_supported_p (as, loc); } +/* Implement `TARGET_ADDR_SPACE_ZERO_ADDRESS_VALID. Zero is a valid + address in all address spaces. Even in ADDR_SPACE_FLASH1 etc.., + a zero address is valid and means 0x<RAMPZ val>0000, where RAMPZ is + set to the appropriate segment value. */ + +static bool +avr_addr_space_zero_address_valid (addr_space_t) +{ + return true; +} /* Look if DECL shall be placed in program memory space by means of attribute `progmem' or some address-space qualifier. @@ -14760,6 +14774,9 @@ avr_float_lib_compare_returns_bool (machine_mode mode, enum rtx_code) #undef TARGET_ADDR_SPACE_DIAGNOSE_USAGE #define TARGET_ADDR_SPACE_DIAGNOSE_USAGE avr_addr_space_diagnose_usage +#undef TARGET_ADDR_SPACE_ZERO_ADDRESS_VALID +#define TARGET_ADDR_SPACE_ZERO_ADDRESS_VALID avr_addr_space_zero_address_valid + #undef TARGET_MODE_DEPENDENT_ADDRESS_P #define TARGET_MODE_DEPENDENT_ADDRESS_P avr_mode_dependent_address_p diff --git gcc/testsuite/gcc.target/avr/pr105523.c gcc/testsuite/gcc.target/avr/pr105523.c new file mode 100644 index 0000000..fbbf7bf --- /dev/null +++ gcc/testsuite/gcc.target/avr/pr105523.c @@ -0,0 +1,14 @@ +/* { dg-do compile } */ +/* { dg-options "-Os -Wall" } */ + +/* Verify no "array subscript 0 is outside array bounds of" is generated + for accessing memory addresses in the 0-4096 range. */ + +typedef __UINT8_TYPE__ uint8_t; + +#define SREG (*(volatile uint8_t*) (0x3F + __AVR_SFR_OFFSET__ )) + +void bar (void) +{ + SREG = 0; +} diff --git gcc/testsuite/lib/target-supports.exp gcc/testsuite/lib/target-supports.exp index 263ef35..56fa73d 100644 --- gcc/testsuite/lib/target-supports.exp +++ gcc/testsuite/lib/target-supports.exp @@ -686,7 +686,8 @@ proc check_effective_target_keeps_null_pointer_checks { } { if [target_info exists keeps_null_pointer_checks] { return 1 } - if { [istarget msp430-*-*] } { + if { [istarget msp430-*-*] + || [istarget avr-*-*] } { return 1; } return 0 ^ permalink raw reply [flat|nested] 12+ messages in thread
* [Ping] Re: [PATCH] avr: Set param_min_pagesize to 0 [PR105523] 2023-06-02 7:02 ` SenthilKumar.Selvaraj @ 2023-06-16 10:17 ` SenthilKumar.Selvaraj 2023-06-17 16:56 ` Jeff Law 0 siblings, 1 reply; 12+ messages in thread From: SenthilKumar.Selvaraj @ 2023-06-16 10:17 UTC (permalink / raw) To: gcc-patches; +Cc: chertykov, richard.guenther On Fri, 2023-06-02 at 12:32 +0530, Senthil Kumar Selvaraj wrote: > On Mon, 2023-05-22 at 14:05 +0200, Richard Biener wrote: > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > > > On Fri, May 19, 2023 at 7:58 AM <SenthilKumar.Selvaraj@microchip.com> wrote: > > > On 26/04/23, 5:51 PM, "Richard Biener" <richard.guenther@gmail.com <mailto:richard.guenther@gmail.com>> wrote: > > > > On Wed, Apr 26, 2023 at 12:56 PM <SenthilKumar.Selvaraj@microchip.com <mailto:SenthilKumar.Selvaraj@microchip.com>> wrote: > > > > > On Wed, Apr 26, 2023 at 3:15 PM Richard Biener via Gcc-patches <gcc-patches@gcc.gnu.org <mailto:gcc-patches@gcc.gnu.org>> wrote: > > > > > > On Wed, Apr 26, 2023 at 11:42 AM Richard Biener > > > > > > <richard.guenther@gmail.com <mailto:richard.guenther@gmail.com>> wrote: > > > > > > > On Wed, Apr 26, 2023 at 11:01 AM SenthilKumar.Selvaraj--- via > > > > > > > Gcc-patches <gcc-patches@gcc.gnu.org <mailto:gcc-patches@gcc.gnu.org>> wrote: > > > > > > > > Hi, > > > > > > > > > > > > > > > > This patch fixes PR 105523 by setting param_min_pagesize to 0 for the > > > > > > > > avr target. For this target, zero and offsets from zero are perfectly > > > > > > > > valid addresses, and the default value of param_min_pagesize ends up > > > > > > > > triggering warnings on valid memory accesses. > > > > > > > > > > > > > > I think the proper configuration is to have > > > > > > > DEFAULT_ADDR_SPACE_ZERO_ADDRESS_VALID > > > > > > > > > > > > Err, TARGET_ADDR_SPACE_ZERO_ADDRESS_VALID > > > > > > > > > > That worked. Ok for trunk and backporting to 13 and 12 branches > > > > > (pending regression testing)? > > > > > > > > OK, but please let Denis time to comment. > > > > > > Didn't hear from Denis. When running regression tests with this patch, > > > I found that some tests with -fdelete-null-pointer-checks were > > > failing. Commit 19416210b37db0584cd0b3f3b3961324b8973d25 made > > > -fdelete-null-pointer-checks false by default, while still allowing it > > > to be overridden from the command line (it was previously > > > unconditionally false). > > > > > > To keep the same behavior, I modified the hook to report zero > > > addresses as valid only if -fdelete-null-pointer-checks is not set. > > > With this change, all regression tests pass. > > > > > > Ok for trunk and backporting to 13 and 12 branches? > > > > I think that's bit backwards - this hook conveys more precise information > > (it's address-space specific) and it is also more specific. Instead I'd > > suggest to set the flag to zero in the target like nios2 or msp430 do. > > In fact we should probably initialize it using this hook (and using the > > default address space). > > Does the below patch work? The hook impl reports that zero address is > valid, and flag_delete_null_pointer_checks is set to zero if the > hook says zero is a valid address. > > As flag_delete_null_pointer_checks is now always disabled for avr, I > removed the resetting code in avr-common.cc that disables it for > OPT_LEVELS_ALL by default, and added avr as a target that always keeps > null pointer checks in testsuite/lib/target-supports.exp. > > I also removed ATTRIBUTE_UNUSED and the parameter name in the target > hook to address https://gcc.gnu.org/pipermail/gcc-patches/2023-May/619014.html. > > PR 105523 > > gcc/ChangeLog: > > * common/config/avr/avr-common.cc: Remove setting > of OPT_fdelete_null_pointer_checks. > * config/avr/avr.cc (avr_option_override): Clear > flag_delete_null_pointer_checks if zero_address_valid. > (avr_addr_space_zero_address_valid): New function. > (TARGET_ADDR_SPACE_ZERO_ADDRESS_VALID): Provide target > hook. > > gcc/testsuite/ChangeLog: > > * lib/target-supports.exp > (check_effective_target_keeps_null_pointer_checks): Add > avr. > * gcc.target/avr/pr105523.c: New test. > > diff --git gcc/common/config/avr/avr-common.cc gcc/common/config/avr/avr-common.cc > index 2ad0244..2f874c5 100644 > --- gcc/common/config/avr/avr-common.cc > +++ gcc/common/config/avr/avr-common.cc > @@ -29,12 +29,6 @@ > /* Implement TARGET_OPTION_OPTIMIZATION_TABLE. */ > static const struct default_options avr_option_optimization_table[] = > { > - // With -fdelete-null-pointer-checks option, the compiler assumes > - // that dereferencing of a null pointer would halt the program. > - // For AVR this assumption is not true and a program can safely > - // dereference null pointers. Changes made by this option may not > - // work properly for AVR. So disable this option. > - { OPT_LEVELS_ALL, OPT_fdelete_null_pointer_checks, NULL, 0 }, > // The only effect of -fcaller-saves might be that it triggers > // a frame without need when it tries to be smart around calls. > { OPT_LEVELS_ALL, OPT_fcaller_saves, NULL, 0 }, > diff --git gcc/config/avr/avr.cc gcc/config/avr/avr.cc > index a90cade..b987837 100644 > --- gcc/config/avr/avr.cc > +++ gcc/config/avr/avr.cc > @@ -756,6 +756,10 @@ avr_option_override (void) > flag_omit_frame_pointer = 0; > } > > + /* Disable flag_delete_null_pointer_checks if zero is a valid address. */ > + if (targetm.addr_space.zero_address_valid (ADDR_SPACE_GENERIC)) > + flag_delete_null_pointer_checks = 0; > + > if (flag_pic == 1) > warning (OPT_fpic, "%<-fpic%> is not supported"); > if (flag_pic == 2) > @@ -9800,6 +9804,16 @@ avr_addr_space_diagnose_usage (addr_space_t as, location_t loc) > (void) avr_addr_space_supported_p (as, loc); > } > > +/* Implement `TARGET_ADDR_SPACE_ZERO_ADDRESS_VALID. Zero is a valid > + address in all address spaces. Even in ADDR_SPACE_FLASH1 etc.., > + a zero address is valid and means 0x<RAMPZ val>0000, where RAMPZ is > + set to the appropriate segment value. */ > + > +static bool > +avr_addr_space_zero_address_valid (addr_space_t) > +{ > + return true; > +} > > /* Look if DECL shall be placed in program memory space by > means of attribute `progmem' or some address-space qualifier. > @@ -14760,6 +14774,9 @@ avr_float_lib_compare_returns_bool (machine_mode mode, enum rtx_code) > #undef TARGET_ADDR_SPACE_DIAGNOSE_USAGE > #define TARGET_ADDR_SPACE_DIAGNOSE_USAGE avr_addr_space_diagnose_usage > > +#undef TARGET_ADDR_SPACE_ZERO_ADDRESS_VALID > +#define TARGET_ADDR_SPACE_ZERO_ADDRESS_VALID avr_addr_space_zero_address_valid > + > #undef TARGET_MODE_DEPENDENT_ADDRESS_P > #define TARGET_MODE_DEPENDENT_ADDRESS_P avr_mode_dependent_address_p > > diff --git gcc/testsuite/gcc.target/avr/pr105523.c gcc/testsuite/gcc.target/avr/pr105523.c > new file mode 100644 > index 0000000..fbbf7bf > --- /dev/null > +++ gcc/testsuite/gcc.target/avr/pr105523.c > @@ -0,0 +1,14 @@ > +/* { dg-do compile } */ > +/* { dg-options "-Os -Wall" } */ > + > +/* Verify no "array subscript 0 is outside array bounds of" is generated > + for accessing memory addresses in the 0-4096 range. */ > + > +typedef __UINT8_TYPE__ uint8_t; > + > +#define SREG (*(volatile uint8_t*) (0x3F + __AVR_SFR_OFFSET__ )) > + > +void bar (void) > +{ > + SREG = 0; > +} > diff --git gcc/testsuite/lib/target-supports.exp gcc/testsuite/lib/target-supports.exp > index 263ef35..56fa73d 100644 > --- gcc/testsuite/lib/target-supports.exp > +++ gcc/testsuite/lib/target-supports.exp > @@ -686,7 +686,8 @@ proc check_effective_target_keeps_null_pointer_checks { } { > if [target_info exists keeps_null_pointer_checks] { > return 1 > } > - if { [istarget msp430-*-*] } { > + if { [istarget msp430-*-*] > + || [istarget avr-*-*] } { > return 1; > } > return 0 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Ping] Re: [PATCH] avr: Set param_min_pagesize to 0 [PR105523] 2023-06-16 10:17 ` [Ping] " SenthilKumar.Selvaraj @ 2023-06-17 16:56 ` Jeff Law 0 siblings, 0 replies; 12+ messages in thread From: Jeff Law @ 2023-06-17 16:56 UTC (permalink / raw) To: SenthilKumar.Selvaraj, gcc-patches; +Cc: chertykov, richard.guenther On 6/16/23 04:17, SenthilKumar.Selvaraj--- via Gcc-patches wrote: > On Fri, 2023-06-02 at 12:32 +0530, Senthil Kumar Selvaraj wrote: >> On Mon, 2023-05-22 at 14:05 +0200, Richard Biener wrote: >>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe >>> >>> On Fri, May 19, 2023 at 7:58 AM <SenthilKumar.Selvaraj@microchip.com> wrote: >>>> On 26/04/23, 5:51 PM, "Richard Biener" <richard.guenther@gmail.com <mailto:richard.guenther@gmail.com>> wrote: >>>>> On Wed, Apr 26, 2023 at 12:56 PM <SenthilKumar.Selvaraj@microchip.com <mailto:SenthilKumar.Selvaraj@microchip.com>> wrote: >>>>>> On Wed, Apr 26, 2023 at 3:15 PM Richard Biener via Gcc-patches <gcc-patches@gcc.gnu.org <mailto:gcc-patches@gcc.gnu.org>> wrote: >>>>>>> On Wed, Apr 26, 2023 at 11:42 AM Richard Biener >>>>>>> <richard.guenther@gmail.com <mailto:richard.guenther@gmail.com>> wrote: >>>>>>>> On Wed, Apr 26, 2023 at 11:01 AM SenthilKumar.Selvaraj--- via >>>>>>>> Gcc-patches <gcc-patches@gcc.gnu.org <mailto:gcc-patches@gcc.gnu.org>> wrote: >>>>>>>>> Hi, >>>>>>>>> >>>>>>>>> This patch fixes PR 105523 by setting param_min_pagesize to 0 for the >>>>>>>>> avr target. For this target, zero and offsets from zero are perfectly >>>>>>>>> valid addresses, and the default value of param_min_pagesize ends up >>>>>>>>> triggering warnings on valid memory accesses. >>>>>>>> >>>>>>>> I think the proper configuration is to have >>>>>>>> DEFAULT_ADDR_SPACE_ZERO_ADDRESS_VALID >>>>>>> >>>>>>> Err, TARGET_ADDR_SPACE_ZERO_ADDRESS_VALID >>>>>> >>>>>> That worked. Ok for trunk and backporting to 13 and 12 branches >>>>>> (pending regression testing)? >>>>> >>>>> OK, but please let Denis time to comment. >>>> >>>> Didn't hear from Denis. When running regression tests with this patch, >>>> I found that some tests with -fdelete-null-pointer-checks were >>>> failing. Commit 19416210b37db0584cd0b3f3b3961324b8973d25 made >>>> -fdelete-null-pointer-checks false by default, while still allowing it >>>> to be overridden from the command line (it was previously >>>> unconditionally false). >>>> >>>> To keep the same behavior, I modified the hook to report zero >>>> addresses as valid only if -fdelete-null-pointer-checks is not set. >>>> With this change, all regression tests pass. >>>> >>>> Ok for trunk and backporting to 13 and 12 branches? >>> >>> I think that's bit backwards - this hook conveys more precise information >>> (it's address-space specific) and it is also more specific. Instead I'd >>> suggest to set the flag to zero in the target like nios2 or msp430 do. >>> In fact we should probably initialize it using this hook (and using the >>> default address space). >> >> Does the below patch work? The hook impl reports that zero address is >> valid, and flag_delete_null_pointer_checks is set to zero if the >> hook says zero is a valid address. >> >> As flag_delete_null_pointer_checks is now always disabled for avr, I >> removed the resetting code in avr-common.cc that disables it for >> OPT_LEVELS_ALL by default, and added avr as a target that always keeps >> null pointer checks in testsuite/lib/target-supports.exp. >> >> I also removed ATTRIBUTE_UNUSED and the parameter name in the target >> hook to address https://gcc.gnu.org/pipermail/gcc-patches/2023-May/619014.html. >> >> PR 105523 >> >> gcc/ChangeLog: >> >> * common/config/avr/avr-common.cc: Remove setting >> of OPT_fdelete_null_pointer_checks. >> * config/avr/avr.cc (avr_option_override): Clear >> flag_delete_null_pointer_checks if zero_address_valid. >> (avr_addr_space_zero_address_valid): New function. >> (TARGET_ADDR_SPACE_ZERO_ADDRESS_VALID): Provide target >> hook. >> >> gcc/testsuite/ChangeLog: >> >> * lib/target-supports.exp >> (check_effective_target_keeps_null_pointer_checks): Add >> avr. >> * gcc.target/avr/pr105523.c: New test. OK for the trunk. I don't think we're likely to hear from Denis. I haven't heard from him in quite a while, nor has Georg. So no need to wait. jeff ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2023-06-17 16:56 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-04-26 9:00 [PATCH] avr: Set param_min_pagesize to 0 [PR105523] SenthilKumar.Selvaraj 2023-04-26 9:42 ` Richard Biener 2023-04-26 9:43 ` Richard Biener 2023-04-26 10:56 ` SenthilKumar.Selvaraj 2023-04-26 12:19 ` Richard Biener 2023-05-19 5:58 ` SenthilKumar.Selvaraj 2023-05-19 14:02 ` Bernhard Reutner-Fischer 2023-05-19 16:51 ` Jeff Law 2023-05-22 12:05 ` Richard Biener 2023-06-02 7:02 ` SenthilKumar.Selvaraj 2023-06-16 10:17 ` [Ping] " SenthilKumar.Selvaraj 2023-06-17 16:56 ` Jeff Law
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).