From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 26350 invoked by alias); 3 Jun 2016 16:52:56 -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 26329 invoked by uid 89); 3 Jun 2016 16:52:55 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.1 required=5.0 tests=BAYES_00,RCVD_IN_DNSWL_NONE,RCVD_IN_SORBS_WEB autolearn=no version=3.3.2 spammy=H*c:ISO-8859-15, TEXT, HTo:D*atmel.com X-HELO: mo4-p00-ob.smtp.rzone.de Received: from mo4-p00-ob.smtp.rzone.de (HELO mo4-p00-ob.smtp.rzone.de) (81.169.146.221) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Fri, 03 Jun 2016 16:52:45 +0000 X-RZG-AUTH: :LXoWVUeid/7A29J/hMvvT2k715jHQaJercGOZE+TiTS5oyq/h41Png== X-RZG-CLASS-ID: mo00 Received: from [192.168.2.100] (dslb-084-058-192-030.084.058.pools.vodafone-ip.de [84.58.192.30]) by smtp.strato.de (RZmta 38.2 DYNA|AUTH) with ESMTPSA id Q0506cs53Gqb2MD (using TLSv1 with cipher ECDHE-RSA-AES256-SHA (curve secp521r1 with 521 ECDH bits, eq. 15360 bits RSA)) (Client did not present a certificate); Fri, 3 Jun 2016 18:52:37 +0200 (CEST) Message-ID: <5751B5EA.5000108@gjlay.de> Date: Fri, 03 Jun 2016 16:52:00 -0000 From: Georg-Johann Lay User-Agent: Thunderbird 2.0.0.24 (Windows/20100228) MIME-Version: 1.0 To: Senthil Kumar Selvaraj CC: gcc-patches@gcc.gnu.org, Denis Chertykov Subject: Re: [Patch, avr] Fix PR 71151 References: <87y46m1h3y.fsf@atmel.com> In-Reply-To: <87y46m1h3y.fsf@atmel.com> Content-Type: text/plain; charset=ISO-8859-15; format=flowed Content-Transfer-Encoding: 7bit X-SW-Source: 2016-06/txt/msg00291.txt.bz2 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... 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. > 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. 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? Johann > + y = y + g; > +} > + > +int main() > +{ > + foo(5); > + if (y != 37) > + abort(); > + > + foo(0); > + if (y != 67) > + abort(); > + > + foo(7); > + if (y != 98) > + abort(); > +}