Re: [Patch, avr] Fix PR 71151

[Georg-Johann Lay <avr at gjlay dot de>]

Senthil Kumar Selvaraj schrieb:
> Hi,
>
>   This patch fixes PR 71151 by eliminating the
>   TARGET_ASM_FUNCTION_RODATA_SECTION hook and setting
>   JUMP_TABLES_IN_TEXT_SECTION to 1.
>
>   As described in the bugzilla entry, this hook assumed it will get
>   called only for jumptable rodata for functions. This was true until
>   6.1, when a commit in varasm.c started calling the hook for mergeable
>   string/constant data as well.
>
>   This resulted in string constants ending up in a section intended for
>   jumptables (flash), and broke code using those constants, which
>   expects them to be present in rodata (SRAM).
>
>   Given that the original reason for placing jumptables in a section was
>   fixed by Johann in PR 63323, this patch restores the original
>   behavior. Reg testing on both gcc-6-branch and trunk showed no regressions.

Just for the record:

The intention for jump-tables in function-rodata-section was to get 
fine-grained section for the tables so that --gc-sections and 
-ffunction-sections not only gc unused functions but also unused 
jump-tables.  As these tables had to reside in the lowest 64KiB of flash 
(.progmem section) neither .rodata nor .text was a correct placement, 
hence the hacking in TARGET_ASM_FUNCTION_RODATA_SECTION.

Before using TARGET_ASM_FUNCTION_RODATA_SECTION, all jump tables were 
put into .progmem.gcc_sw_table by ASM_OUTPUT_BEFORE_CASE_LABEL switching 
to that section.

We actually never had fump-tables in .text before...

The purpose of PR63323 was to have more generic jump-table 
implementation that also works if the table does NOT reside in the lower 
64KiB.  This happens when moving whole whole TEXT section around like 
for a bootloader.

>   As pointed out by Johann, this may end up increasing code
>   size if there are lots of branches that cross the jump tables. I
>   intend to propose a separate patch that gives additional information
>   to the target hook (SECCAT_RODATA_{STRING,JUMPTABLE}) so it can know
>   what type of function rodata is coming on. Johann also suggested
>   handling jump table generation ourselves - I'll experiment with that
>   some more.
>
>   If ok, could someone commit please? Could you also backport to
>   gcc-6-branch?
>
> Regards
> Senthil
>
> gcc/ChangeLog
>
> 2016-06-03  Senthil Kumar Selvaraj  <senthil_kumar.selvaraj@atmel.com>
>
> 	* config/avr/avr.c (avr_asm_function_rodata_section): Remove.
> 	* config/avr/avr.c (TARGET_ASM_FUNCTION_RODATA_SECTION): Remove.
>
> gcc/testsuite/ChangeLog
>
> 2016-06-03  Senthil Kumar Selvaraj  <senthil_kumar.selvaraj@atmel.com>
>
> 	* gcc/testsuite/gcc.target/avr/pr71151-1.c: New.
> 	* gcc/testsuite/gcc.target/avr/pr71151-2.c: New.
>
> diff --git gcc/config/avr/avr.c gcc/config/avr/avr.c
> index ba5cd91..3cb8cb7 100644
> --- gcc/config/avr/avr.c
> +++ gcc/config/avr/avr.c
> @@ -9488,65 +9488,6 @@ avr_asm_init_sections (void)
>  }
>  
>  
> -/* Implement `TARGET_ASM_FUNCTION_RODATA_SECTION'.  */
> -
> -static section*
> -avr_asm_function_rodata_section (tree decl)
> -{
> -  /* If a function is unused and optimized out by -ffunction-sections
> -     and --gc-sections, ensure that the same will happen for its jump
> -     tables by putting them into individual sections.  */
> -
> -  unsigned int flags;
> -  section * frodata;
> -
> -  /* Get the frodata section from the default function in varasm.c
> -     but treat function-associated data-like jump tables as code
> -     rather than as user defined data.  AVR has no constant pools.  */
> -  {
> -    int fdata = flag_data_sections;
> -
> -    flag_data_sections = flag_function_sections;
> -    frodata = default_function_rodata_section (decl);
> -    flag_data_sections = fdata;
> -    flags = frodata->common.flags;
> -  }
> -
> -  if (frodata != readonly_data_section
> -      && flags & SECTION_NAMED)
> -    {
> -      /* Adjust section flags and replace section name prefix.  */
> -
> -      unsigned int i;
> -
> -      static const char* const prefix[] =
> -        {
> -          ".rodata",          ".progmem.gcc_sw_table",
> -          ".gnu.linkonce.r.", ".gnu.linkonce.t."
> -        };
> -
> -      for (i = 0; i < sizeof (prefix) / sizeof (*prefix); i += 2)
> -        {
> -          const char * old_prefix = prefix[i];
> -          const char * new_prefix = prefix[i+1];
> -          const char * name = frodata->named.name;
> -
> -          if (STR_PREFIX_P (name, old_prefix))
> -            {
> -              const char *rname = ACONCAT ((new_prefix,
> -                                            name + strlen (old_prefix), NULL));
> -              flags &= ~SECTION_CODE;
> -              flags |= AVR_HAVE_JMP_CALL ? 0 : SECTION_CODE;
> -
> -              return get_section (rname, flags, frodata->named.decl);
> -            }
> -        }
> -    }
> -
> -  return progmem_swtable_section;
> -}
> -
> -
>  /* Implement `TARGET_ASM_NAMED_SECTION'.  */
>  /* Track need of __do_clear_bss, __do_copy_data for named sections.  */
>  
> @@ -13747,9 +13688,6 @@ avr_fold_builtin (tree fndecl, int n_args ATTRIBUTE_UNUSED, tree *arg,
>  #undef  TARGET_FOLD_BUILTIN
>  #define TARGET_FOLD_BUILTIN avr_fold_builtin
>  
> -#undef  TARGET_ASM_FUNCTION_RODATA_SECTION
> -#define TARGET_ASM_FUNCTION_RODATA_SECTION avr_asm_function_rodata_section
> -
>  #undef  TARGET_SCALAR_MODE_SUPPORTED_P
>  #define TARGET_SCALAR_MODE_SUPPORTED_P avr_scalar_mode_supported_p
>  
> diff --git gcc/config/avr/avr.h gcc/config/avr/avr.h
> index 01da708..ab5e465 100644
> --- gcc/config/avr/avr.h
> +++ gcc/config/avr/avr.h
> @@ -391,7 +391,7 @@ typedef struct avr_args
>  
>  #define SUPPORTS_INIT_PRIORITY 0
>  
> -#define JUMP_TABLES_IN_TEXT_SECTION 0
> +#define JUMP_TABLES_IN_TEXT_SECTION 1

IMO the simplest approach as a quick fix.

>  
>  #define ASM_COMMENT_START " ; "
>  
> diff --git gcc/testsuite/gcc.target/avr/pr71151-1.c gcc/testsuite/gcc.target/avr/pr71151-1.c
> new file mode 100644
> index 0000000..615dce8
> --- /dev/null
> +++ gcc/testsuite/gcc.target/avr/pr71151-1.c
> @@ -0,0 +1,12 @@
> +/* { dg-do compile } */
> +/* { dg-options "-Os -ffunction-sections -fdata-sections" } */
> +
> +/* { dg-final { scan-assembler-not ".section	.progmem.gcc_sw_table.foo.str1.1" } } */
> +/* { dg-final { scan-assembler ".section	.rodata.foo.str1.1,\"aMS\"" } } */
> +
> +
> +extern void bar(const char*);
> +void foo(void)
> +{
> +  bar("BBBBBBBBBB");
> +}
> diff --git gcc/testsuite/gcc.target/avr/pr71151-2.c gcc/testsuite/gcc.target/avr/pr71151-2.c
> new file mode 100644
> index 0000000..0041858
> --- /dev/null
> +++ gcc/testsuite/gcc.target/avr/pr71151-2.c
> @@ -0,0 +1,49 @@
> +/* { dg-do run } */
> +/* { dg-options "-Os -ffunction-sections -fdata-sections" } */
> +
> +/* Make sure jumptables work properly, after removing the
> +	 special section placement hook. */
> +
> +#include "exit-abort.h"
> +
> +volatile char y;
> +volatile char g;
> +
> +void foo(char x)
> +{
> +	switch (x)
> +  {
> +	case 0:
> +		y = 67; break;
> +	case 1:
> +		y = 20; break;
> +	case 2:
> +		y = 109; break;
> +	case 3:
> +		y = 33; break;
> +	case 4:
> +		y = 44; break;
> +	case 5:
> +		y = 37; break;
> +	case 6:
> +		y = 10; break;
> +	case 7:
> +		y = 98; break;
> +  }

Not sure if this actually generates a jump-table and not a look-up from 
.rodata by means of tree-switch-conversion.  Hence consider 
-fno-tree-switch-conversion.

The interesting cases are:

- jump-table does not reside in the lower 64KiB

- jump-table crosses a 64KiB boundary (RAMPZ increments)

- jump-table needs linker stub generation, i.e. resides in > 128KiB

IICR there is special code in the linker that avoids relaxing for some 
sections by means of magic name section names?


Johann


> +	y = y + g;
> +}
> +
> +int main()
> +{
> +	foo(5);
> +  if (y != 37)
> +		abort();
> +
> +	foo(0);
> +  if (y != 67)
> +		abort();
> +
> +	foo(7);
> +  if (y != 98)
> +		abort();
> +}