avr: Set param_min_pagesize to 0 [PR105523]
Checks
Commit Message
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.
Comments
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;
> +}
>
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;
> > +}
> >
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;
+}
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;
> +}
>
>
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;
+}
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)
{
...
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
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;
> +}
>
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
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
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
@@ -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;
new file mode 100644
@@ -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;
+}