[Ping] Re: [PATCH] avr: Set param_min_pagesize to 0 [PR105523]

[<SenthilKumar.Selvaraj@microchip.com>]

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