From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 122961 invoked by alias); 17 Jun 2016 04:39:48 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 122952 invoked by uid 89); 17 Jun 2016 04:39:47 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-3.3 required=5.0 tests=BAYES_00,RP_MATCHES_RCVD,SPF_PASS autolearn=ham version=3.3.2 spammy=denis X-HELO: eusmtp01.atmel.com Received: from eusmtp01.atmel.com (HELO eusmtp01.atmel.com) (212.144.249.242) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Fri, 17 Jun 2016 04:39:37 +0000 Received: from HNOCHT01.corp.atmel.com (10.161.30.161) by eusmtp01.atmel.com (10.161.101.30) with Microsoft SMTP Server (TLS) id 14.3.235.1; Fri, 17 Jun 2016 06:39:26 +0200 Received: from jaguar.atmel.com (10.161.30.18) by HNOCHT01.corp.atmel.com (10.161.30.161) with Microsoft SMTP Server (TLS) id 14.3.235.1; Fri, 17 Jun 2016 06:39:30 +0200 References: <87y46m1h3y.fsf@atmel.com> <5751B5EA.5000108@gjlay.de> <877fe1nrdy.fsf@atmel.com> <87lh258ufn.fsf@atmel.com> User-agent: mu4e 0.9.17; emacs 24.5.1 From: Senthil Kumar Selvaraj To: Denis Chertykov CC: Georg-Johann Lay , gcc-patches Subject: Re: [Patch, avr] Fix PR 71151 In-Reply-To: Date: Fri, 17 Jun 2016 04:39:00 -0000 Message-ID: <87inx88m4t.fsf@atmel.com> MIME-Version: 1.0 Content-Type: text/plain X-IsSubscribed: yes X-SW-Source: 2016-06/txt/msg01287.txt.bz2 Denis Chertykov writes: > 2016-06-16 10:27 GMT+03:00 Senthil Kumar Selvaraj > : >> >> 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 >>>>> >>>>> * 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 >>>>> >>>>> * 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 >> >> * 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 >> >> * 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