2016-06-16 10:27 GMT+03:00 Senthil Kumar Selvaraj
<senthil_kumar.selva...@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.selva...@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.selva...@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.selva...@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.selva...@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.
