| Message ID | 87lh258ufn.fsf@atmel.com |
|---|---|
| State | New |
| Headers | show |
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.
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
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
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
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
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. :-)
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; +}