[avr] Fix PR 71151

Submitted by Senthil Kumar Selvaraj on June 16, 2016, 7:27 a.m.

Details

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

Commit Message

Senthil Kumar Selvaraj June 16, 2016, 7:27 a.m.
Senthil Kumar Selvaraj writes:

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

There were two binutils bugs (PR ld/20221 and ld/20254) that were
blocking this patch - on enabling, relaxation, jumptables were
getting corrupted. Both of the issues are now fixed, and the fixes
are in master and 2.26 branch.

This is pretty much the same patch as before, but with a lot more
testcases as suggested by Johann, testing jumptables above 64K,
straddling 128K, and above 128K, with and without relaxation.

All the tests pass with atxmega128. For atmega128, the tests that place
code above 128K fail (because the avrtest rightly ABORTs on excess flash
size), the other tests pass. Perhaps we need a dg-require for large
flash devices?

If this is ok, could someone commit please? I don't have commit access.

Regards
Senthil


gcc/ChangeLog:

2016-06-16  Senthil Kumar Selvaraj  <senthil_kumar.selvaraj@atmel.com>

	* config/avr/avr.c (avr_asm_init_sections): Remove setup of
  progmem_swtable_section.
	(progmem_swtable_section): Remove.
	(avr_asm_function_rodata_section): Remove.
	(TARGET_ASM_FUNCTION_RODATA_SECTION): Remove.
	* config/avr/avr.h (JUMP_TABLES_IN_TEXT_SECTION: Define
  to 1.


gcc/testsuite/ChangeLog:

2016-06-16  Senthil Kumar Selvaraj  <senthil_kumar.selvaraj@atmel.com>

	* gcc.target/avr/pr71151-1.c: New test.
	* gcc.target/avr/pr71151-2.c: New test.
	* gcc.target/avr/pr71151-3.c: New test.
	* gcc.target/avr/pr71151-4.c: New test.
	* gcc.target/avr/pr71151-5.c: New test.
	* gcc.target/avr/pr71151-6.c: New test.
	* gcc.target/avr/pr71151-7.c: New test.
	* gcc.target/avr/pr71151-8.c: New test.
	* gcc.target/avr/pr71151-common.h: New test.

Comments

Denis Chertykov June 16, 2016, 4:51 p.m.
2016-06-16 10:27 GMT+03:00 Senthil Kumar Selvaraj
<senthil_kumar.selvaraj@atmel.com>:
>
> Senthil Kumar Selvaraj writes:
>
>> 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.
>>
>
> There were two binutils bugs (PR ld/20221 and ld/20254) that were
> blocking this patch - on enabling, relaxation, jumptables were
> getting corrupted. Both of the issues are now fixed, and the fixes
> are in master and 2.26 branch.
>
> This is pretty much the same patch as before, but with a lot more
> testcases as suggested by Johann, testing jumptables above 64K,
> straddling 128K, and above 128K, with and without relaxation.
>
> All the tests pass with atxmega128. For atmega128, the tests that place
> code above 128K fail (because the avrtest rightly ABORTs on excess flash
> size), the other tests pass. Perhaps we need a dg-require for large
> flash devices?
>
> If this is ok, could someone commit please? I don't have commit access.
>
> Regards
> Senthil
>
>
> gcc/ChangeLog:
>
> 2016-06-16  Senthil Kumar Selvaraj  <senthil_kumar.selvaraj@atmel.com>
>
>         * config/avr/avr.c (avr_asm_init_sections): Remove setup of
>   progmem_swtable_section.
>         (progmem_swtable_section): Remove.
>         (avr_asm_function_rodata_section): Remove.
>         (TARGET_ASM_FUNCTION_RODATA_SECTION): Remove.
>         * config/avr/avr.h (JUMP_TABLES_IN_TEXT_SECTION: Define
>   to 1.
>
>
> gcc/testsuite/ChangeLog:
>
> 2016-06-16  Senthil Kumar Selvaraj  <senthil_kumar.selvaraj@atmel.com>
>
>         * gcc.target/avr/pr71151-1.c: New test.
>         * gcc.target/avr/pr71151-2.c: New test.
>         * gcc.target/avr/pr71151-3.c: New test.
>         * gcc.target/avr/pr71151-4.c: New test.
>         * gcc.target/avr/pr71151-5.c: New test.
>         * gcc.target/avr/pr71151-6.c: New test.
>         * gcc.target/avr/pr71151-7.c: New test.
>         * gcc.target/avr/pr71151-8.c: New test.
>         * gcc.target/avr/pr71151-common.h: New test.
>
>
>

Committed.
Senthil Kumar Selvaraj June 17, 2016, 4:39 a.m.
Denis Chertykov writes:

> 2016-06-16 10:27 GMT+03:00 Senthil Kumar Selvaraj
> <senthil_kumar.selvaraj@atmel.com>:
>>
>> Senthil Kumar Selvaraj writes:
>>
>>> 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.
>>>
>>
>> There were two binutils bugs (PR ld/20221 and ld/20254) that were
>> blocking this patch - on enabling, relaxation, jumptables were
>> getting corrupted. Both of the issues are now fixed, and the fixes
>> are in master and 2.26 branch.
>>
>> This is pretty much the same patch as before, but with a lot more
>> testcases as suggested by Johann, testing jumptables above 64K,
>> straddling 128K, and above 128K, with and without relaxation.
>>
>> All the tests pass with atxmega128. For atmega128, the tests that place
>> code above 128K fail (because the avrtest rightly ABORTs on excess flash
>> size), the other tests pass. Perhaps we need a dg-require for large
>> flash devices?
>>
>> If this is ok, could someone commit please? I don't have commit access.
>>
>> Regards
>> Senthil
>>
>>
>> gcc/ChangeLog:
>>
>> 2016-06-16  Senthil Kumar Selvaraj  <senthil_kumar.selvaraj@atmel.com>
>>
>>         * config/avr/avr.c (avr_asm_init_sections): Remove setup of
>>   progmem_swtable_section.
>>         (progmem_swtable_section): Remove.
>>         (avr_asm_function_rodata_section): Remove.
>>         (TARGET_ASM_FUNCTION_RODATA_SECTION): Remove.
>>         * config/avr/avr.h (JUMP_TABLES_IN_TEXT_SECTION: Define
>>   to 1.
>>
>>
>> gcc/testsuite/ChangeLog:
>>
>> 2016-06-16  Senthil Kumar Selvaraj  <senthil_kumar.selvaraj@atmel.com>
>>
>>         * gcc.target/avr/pr71151-1.c: New test.
>>         * gcc.target/avr/pr71151-2.c: New test.
>>         * gcc.target/avr/pr71151-3.c: New test.
>>         * gcc.target/avr/pr71151-4.c: New test.
>>         * gcc.target/avr/pr71151-5.c: New test.
>>         * gcc.target/avr/pr71151-6.c: New test.
>>         * gcc.target/avr/pr71151-7.c: New test.
>>         * gcc.target/avr/pr71151-8.c: New test.
>>         * gcc.target/avr/pr71151-common.h: New test.
>>
>>
>>
>
> Committed.

Could you please commit this to the 6.x branch as well?

Regards
Senthil
Georg-Johann Lay June 22, 2016, 6:23 p.m.
Senthil Kumar Selvaraj schrieb:
> Senthil Kumar Selvaraj writes:
> 
>> Georg-Johann Lay writes:
>>
>>> Senthil Kumar Selvaraj schrieb:
>>>> Hi,
>>>>
>>>>   [set JUMP_TABLES_IN_TEXT_SECTION to 1]
>>
>> I added tests that use linker relaxation and discovered a relaxation 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.
>>
> 
> There were two binutils bugs (PR ld/20221 and ld/20254) that were
> blocking this patch - on enabling, relaxation, jumptables were
> getting corrupted. Both of the issues are now fixed, and the fixes
> are in master and 2.26 branch.

Should we mention in the release notes that Binutils >= 2.26 is needed 
for avr-gcc >= 6 ?

Maybe even check during configure whether an appropriate version of 
Binutils is used?

Johann
Senthil Kumar Selvaraj June 23, 2016, 6:45 a.m.
Georg-Johann Lay writes:

> Senthil Kumar Selvaraj schrieb:
>> Senthil Kumar Selvaraj writes:
>> 
>>> Georg-Johann Lay writes:
>>>
>>>> Senthil Kumar Selvaraj schrieb:
>>>>> Hi,
>>>>>
>>>>>   [set JUMP_TABLES_IN_TEXT_SECTION to 1]
>>>
>>> I added tests that use linker relaxation and discovered a relaxation 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.
>>>
>> 
>> There were two binutils bugs (PR ld/20221 and ld/20254) that were
>> blocking this patch - on enabling, relaxation, jumptables were
>> getting corrupted. Both of the issues are now fixed, and the fixes
>> are in master and 2.26 branch.
>
> Should we mention in the release notes that Binutils >= 2.26 is needed 
> for avr-gcc >= 6 ?

Yes, we should document it. binutils 2.25 would probably work too, as
the bugs were introduced only in binutils 2.26. I'll check and send a patch.
>
> Maybe even check during configure whether an appropriate version of 
> Binutils is used?

That would be nice, but is it ok to add target specific conditions to
configure.ac?

Regards
Senthil
Georg-Johann Lay June 23, 2016, 4:16 p.m.
Senthil Kumar Selvaraj schrieb:
> Georg-Johann Lay writes:
> 
>> Senthil Kumar Selvaraj schrieb:
>>> Senthil Kumar Selvaraj writes:
>>>
>>>> Georg-Johann Lay writes:
>>>>
>>>>> Senthil Kumar Selvaraj schrieb:
>>>>>> Hi,
>>>>>>
>>>>>>   [set JUMP_TABLES_IN_TEXT_SECTION to 1]
>>>> I added tests that use linker relaxation and discovered a relaxation 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.
>>>>
>>> There were two binutils bugs (PR ld/20221 and ld/20254) that were
>>> blocking this patch - on enabling, relaxation, jumptables were
>>> getting corrupted. Both of the issues are now fixed, and the fixes
>>> are in master and 2.26 branch.
>> Should we mention in the release notes that Binutils >= 2.26 is needed 
>> for avr-gcc >= 6 ?
> 
> Yes, we should document it. binutils 2.25 would probably work too, as
> the bugs were introduced only in binutils 2.26. I'll check and send a patch.
>> Maybe even check during configure whether an appropriate version of 
>> Binutils is used?
> 
> That would be nice, but is it ok to add target specific conditions to
> configure.ac?

We already have avr-specific tests in gcc/configure.ac which test 
whether -mrmw and --mlink-relax are supported by as or not.  This is 
then used in gen-avr-mmcu-specs.c:

#ifdef HAVE_AS_AVR_MLINK_RELAX_OPTION
   fprintf (f, "*asm_relax:\n\t%s\n\n", ASM_RELAX_SPEC);
#endif // have avr-as --mlink-relax

#ifdef HAVE_AS_AVR_MRMW_OPTION
   fprintf (f, "*asm_rmw:\n%s\n\n", rmw
            ? "\t%{!mno-rmw: -mrmw}"
            : "\t%{mrmw}");
#endif // have avr-as -mrmw


Johann
Mike Stump June 23, 2016, 5:50 p.m.
On Jun 23, 2016, at 9:16 AM, Georg-Johann Lay <avr@gjlay.de> wrote:
>>>>> Maybe even check during configure whether an appropriate version of Binutils is used?
>> That would be nice, but is it ok to add target specific conditions to
>> configure.ac?
> 
> We already have avr-specific tests in gcc/configure.ac

Further, the only point of configure is to have target specific tests in it.  :-)

Patch hide | download patch | download mbox

diff --git gcc/config/avr/avr.c gcc/config/avr/avr.c
index ba5cd91..dd99f42 100644
--- gcc/config/avr/avr.c
+++ gcc/config/avr/avr.c
@@ -203,9 +203,6 @@  static GTY(()) rtx xstring_e;
 /* Current architecture.  */
 const avr_arch_t *avr_arch;
 
-/* Section to put switch tables in.  */
-static GTY(()) section *progmem_swtable_section;
-
 /* Unnamed sections associated to __attribute__((progmem)) aka. PROGMEM
    or to address space __flash* or __memx.  Only used as singletons inside
    avr_asm_select_section, but it must not be local there because of GTY.  */
@@ -9461,24 +9458,6 @@  avr_output_progmem_section_asm_op (const void *data)
 static void
 avr_asm_init_sections (void)
 {
-  /* Set up a section for jump tables.  Alignment is handled by
-     ASM_OUTPUT_BEFORE_CASE_LABEL.  */
-
-  if (AVR_HAVE_JMP_CALL)
-    {
-      progmem_swtable_section
-        = get_unnamed_section (0, output_section_asm_op,
-                               "\t.section\t.progmem.gcc_sw_table"
-                               ",\"a\",@progbits");
-    }
-  else
-    {
-      progmem_swtable_section
-        = get_unnamed_section (SECTION_CODE, output_section_asm_op,
-                               "\t.section\t.progmem.gcc_sw_table"
-                               ",\"ax\",@progbits");
-    }
-
   /* Override section callbacks to keep track of `avr_need_clear_bss_p'
      resp. `avr_need_copy_data_p'.  */
 
@@ -9488,65 +9467,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 +13667,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..e523ce0
--- /dev/null
+++ gcc/testsuite/gcc.target/avr/pr71151-2.c
@@ -0,0 +1,24 @@ 
+/* { dg-do run } */
+/* { dg-options "-Os -fno-tree-switch-conversion -ffunction-sections -fdata-sections" } */
+
+/* Make sure jumptables work properly if placed below 64 KB i.e. 2 byte
+   flash address for loading jump table entry, 2 byte entry, after
+   removing the special section placement hook. */
+
+#include "exit-abort.h"
+#include "pr71151-common.h"
+
+int main()
+{
+	foo(5);
+	if (y != 37)
+		abort();
+
+	foo(0);
+	if (y != 67)
+		abort();
+
+	foo(7);
+	if (y != 98)
+		abort();
+}
diff --git gcc/testsuite/gcc.target/avr/pr71151-3.c gcc/testsuite/gcc.target/avr/pr71151-3.c
new file mode 100644
index 0000000..ce0ba59
--- /dev/null
+++ gcc/testsuite/gcc.target/avr/pr71151-3.c
@@ -0,0 +1,25 @@ 
+/* { dg-do run } */
+/* { dg-options "-Os -fno-tree-switch-conversion -ffunction-sections -mno-relax -fdata-sections -Wl,--section-start=.foo=0x10000" } */
+
+/* Make sure jumptables work properly if placed above 64 KB and below 128 KB,
+   i.e. 3 byte flash address for loading jump table entry and 2 byte jump table
+   entry, with relaxation disabled, after removing the special section
+   placement hook. */
+
+#include "exit-abort.h"
+#include "pr71151-common.h"
+
+int main()
+{
+	foo(5);
+	if (y != 37)
+		abort();
+
+	foo(0);
+	if (y != 67)
+		abort();
+
+	foo(7);
+	if (y != 98)
+		abort();
+}
diff --git gcc/testsuite/gcc.target/avr/pr71151-4.c gcc/testsuite/gcc.target/avr/pr71151-4.c
new file mode 100644
index 0000000..51250b0
--- /dev/null
+++ gcc/testsuite/gcc.target/avr/pr71151-4.c
@@ -0,0 +1,25 @@ 
+/* { dg-do run } */
+/* { dg-options "-Os -fno-tree-switch-conversion -ffunction-sections -fdata-sections -mrelax -Wl,--section-start=.foo=0x10000" } */
+
+/* Make sure jumptables work properly if placed above 64 KB and below 128 KB,
+   i.e. 3 byte flash address for loading jump table entry and 2 byte jump
+   table entry, with relaxation enabled, after removing the special section
+   placement hook. */
+
+#include "exit-abort.h"
+#include "pr71151-common.h"
+
+int main()
+{
+	foo(5);
+	if (y != 37)
+		abort();
+
+	foo(0);
+	if (y != 67)
+		abort();
+
+	foo(7);
+	if (y != 98)
+		abort();
+}
diff --git gcc/testsuite/gcc.target/avr/pr71151-5.c gcc/testsuite/gcc.target/avr/pr71151-5.c
new file mode 100644
index 0000000..40fdb22
--- /dev/null
+++ gcc/testsuite/gcc.target/avr/pr71151-5.c
@@ -0,0 +1,30 @@ 
+/* { dg-do run } */
+/* { dg-options "-Os -fno-tree-switch-conversion -ffunction-sections -fdata-sections -mno-relax -Wl,--section-start=.foo=0x20000" } */
+
+/* Make sure jumptables work properly if placed above 128 KB, i.e. 3 byte
+   flash address for loading jump table entry and a jump table entry
+   that is a stub, with relaxation disabled, after removing the special
+   section placement hook. */
+
+#include "exit-abort.h"
+#include "pr71151-common.h"
+
+int main()
+{
+	/* Not meant for devices with flash <= 128K */
+#if defined (__AVR_2_BYTE_PC__)
+	exit(0);
+#else
+	foo(5);
+	if (y != 37)
+		abort();
+
+	foo(0);
+	if (y != 67)
+		abort();
+
+	foo(7);
+	if (y != 98)
+		abort();
+#endif
+}
diff --git gcc/testsuite/gcc.target/avr/pr71151-6.c gcc/testsuite/gcc.target/avr/pr71151-6.c
new file mode 100644
index 0000000..f4d2ef9
--- /dev/null
+++ gcc/testsuite/gcc.target/avr/pr71151-6.c
@@ -0,0 +1,30 @@ 
+/* { dg-do run } */
+/* { dg-options "-Os -fno-tree-switch-conversion -ffunction-sections -fdata-sections -mrelax -Wl,--section-start=.foo=0x20000" } */
+
+/* Make sure jumptables work properly if placed above 128 KB, i.e. 3 byte
+   flash address for loading jump table entry and a jump table entry
+   that is a stub, with relaxation enabled, after removing the special
+   section placement hook. */
+
+#include "exit-abort.h"
+#include "pr71151-common.h"
+
+int main()
+{
+	/* Not meant for devices with flash <= 128K */
+#if defined (__AVR_2_BYTE_PC__)
+	exit(0);
+#else
+	foo(5);
+	if (y != 37)
+		abort();
+
+	foo(0);
+	if (y != 67)
+		abort();
+
+	foo(7);
+	if (y != 98)
+		abort();
+#endif
+}
diff --git gcc/testsuite/gcc.target/avr/pr71151-7.c gcc/testsuite/gcc.target/avr/pr71151-7.c
new file mode 100644
index 0000000..cdc7ea9
--- /dev/null
+++ gcc/testsuite/gcc.target/avr/pr71151-7.c
@@ -0,0 +1,28 @@ 
+/* { dg-do run } */
+/* { dg-options "-Os -fno-tree-switch-conversion -ffunction-sections -fdata-sections -mno-relax -Wl,--section-start=.foo=0x1fffa" } */
+
+/* Make sure jumptables work properly if placed straddling 128 KB i.e
+   some entries below 128 KB and some above it, with relaxation disabled. */
+
+#include "exit-abort.h"
+#include "pr71151-common.h"
+
+int main()
+{
+	/* Not meant for devices with flash <= 128K */
+#if defined (__AVR_2_BYTE_PC__)
+	exit(0);
+#else
+	foo(5);
+	if (y != 37)
+		abort();
+
+	foo(0);
+	if (y != 67)
+		abort();
+
+	foo(7);
+	if (y != 98)
+		abort();
+#endif
+}
diff --git gcc/testsuite/gcc.target/avr/pr71151-8.c gcc/testsuite/gcc.target/avr/pr71151-8.c
new file mode 100644
index 0000000..0b7bf6a
--- /dev/null
+++ gcc/testsuite/gcc.target/avr/pr71151-8.c
@@ -0,0 +1,28 @@ 
+/* { dg-do run } */
+/* { dg-options "-Os -fno-tree-switch-conversion -ffunction-sections -fdata-sections -mrelax -Wl,--section-start=.foo=0x1fffa" } */
+
+/* Make sure jumptables work properly if placed straddling 128 KB i.e
+   some entries below 128 KB and some above it, with relaxation disabled. */
+
+#include "exit-abort.h"
+#include "pr71151-common.h"
+
+int main()
+{
+	/* Not meant for devices with flash <= 128K */
+#if defined (__AVR_2_BYTE_PC__)
+	exit(0);
+#else
+	foo(5);
+	if (y != 37)
+		abort();
+
+	foo(0);
+	if (y != 67)
+		abort();
+
+	foo(7);
+	if (y != 98)
+		abort();
+#endif
+}
diff --git gcc/testsuite/gcc.target/avr/pr71151-common.h gcc/testsuite/gcc.target/avr/pr71151-common.h
new file mode 100644
index 0000000..0df1783
--- /dev/null
+++ gcc/testsuite/gcc.target/avr/pr71151-common.h
@@ -0,0 +1,27 @@ 
+volatile char y;
+volatile char g;
+
+__attribute__((section(".foo")))
+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;
+}