This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [Patch, avr] Fix PR 71151
- From: Senthil Kumar Selvaraj <senthil_kumar dot selvaraj at atmel dot com>
- To: Denis Chertykov <chertykov at gmail dot com>
- Cc: Georg-Johann Lay <avr at gjlay dot de>, gcc-patches <gcc-patches at gcc dot gnu dot org>
- Date: Fri, 17 Jun 2016 10:09:14 +0530
- Subject: Re: [Patch, avr] Fix PR 71151
- Authentication-results: sourceware.org; auth=none
- References: <87y46m1h3y dot fsf at atmel dot com> <5751B5EA dot 5000108 at gjlay dot de> <877fe1nrdy dot fsf at atmel dot com> <87lh258ufn dot fsf at atmel dot com> <CADOs=zYF5-VsM6yR+cTKkEAsNKScJHRZf8xFwnusA0ZkPtDiiQ at mail dot gmail dot com>
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