[avr] Fix PR 71151

Submitted by Senthil Kumar Selvaraj on June 3, 2016, 2:28 p.m.

Details

Message ID 87y46m1h3y.fsf@atmel.com
State New
Headers show

Commit Message

Senthil Kumar Selvaraj June 3, 2016, 2:28 p.m.
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.

  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.

Comments

Georg-Johann Lay June 3, 2016, 4:52 p.m.
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();
> +}
Georg-Johann Lay June 4, 2016, 2:20 p.m.
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.
> 
>   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;

The progmem_swtable_section is no more needed; the code to set it up can 
also be removed.

Johann

> -}
> -
> -
>  /* 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
>  
>  #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;
> +  }
> +	y = y + g;
> +}
> +
> +int main()
> +{
> +	foo(5);
> +  if (y != 37)
> +		abort();
> +
> +	foo(0);
> +  if (y != 67)
> +		abort();
> +
> +	foo(7);
> +  if (y != 98)
> +		abort();
> +}
Senthil Kumar Selvaraj June 7, 2016, 11:57 a.m.
Georg-Johann Lay writes:

> 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...

JUMP_TABLES_IN_TEXT_SECTION was 1 before r37465 - that was when the
progmem.gcc_sw_table section was introduced. But yes, I understand that
the target hook for FUNCTION_RODATA_SECTION was done to get them gc'ed
along with the code.

>
> 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.

Understood.
>
>>   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.

It did generate a jumptable, but I added -fno-tree-switch-conversion
just in case.
>
> 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?

Yes, for ".vectors" and ".jumptables".

I added tests that use linker relaxation and discovered arelaxation bug
in binutils 2.26 (and later) that messes up symbol values in the
presence of alignment directives. I'm working on that right now -
hopefully, it'll get backported to the release branch.

Once that gets upstream, I'll resend the patch - with more tests, and
incorporating your comments.

Regards
Senthil

>
>
> Johann
>
>
>> +	y = y + g;
>> +}
>> +
>> +int main()
>> +{
>> +	foo(5);
>> +  if (y != 37)
>> +		abort();
>> +
>> +	foo(0);
>> +  if (y != 67)
>> +		abort();
>> +
>> +	foo(7);
>> +  if (y != 98)
>> +		abort();
>> +}
Georg-Johann Lay June 17, 2016, 12:59 p.m.
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.
> 
>   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>
> 

Missing PR target/71151

> 	* 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>
> 

Missing PR target/71151

> 	* gcc/testsuite/gcc.target/avr/pr71151-1.c: New.
> 	* gcc/testsuite/gcc.target/avr/pr71151-2.c: New.
> 

With the PR entry in the ChangeLog / commit message it might be easier 
to find the change, and the respective bugzilla PR will get an automatic 
entry pointing to the commit.

Thanks,  Johann
Senthil Kumar Selvaraj June 19, 2016, 6:18 a.m.
Georg-Johann Lay writes:

> 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.
>> 
>>   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>
>> 
>
> Missing PR target/71151
>
>> 	* 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>
>> 
>
> Missing PR target/71151
>
>> 	* gcc/testsuite/gcc.target/avr/pr71151-1.c: New.
>> 	* gcc/testsuite/gcc.target/avr/pr71151-2.c: New.
>> 

Sorry I missed that. Is it ok to modify the entry alone again?

I just started using the mklog perl script from gcc/contrib
instead of writing the entries by hand - I didn't realize
that script doesn't know about PRs.

Regards
Senthil

Patch hide | download patch | download mbox

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
 
 #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;
+  }
+	y = y + g;
+}
+
+int main()
+{
+	foo(5);
+  if (y != 37)
+		abort();
+
+	foo(0);
+  if (y != 67)
+		abort();
+
+	foo(7);
+  if (y != 98)
+		abort();
+}