From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 21861 invoked by alias); 16 Jun 2016 16:51:58 -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 21846 invoked by uid 89); 16 Jun 2016 16:51:57 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.4 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,RCVD_IN_DNSWL_LOW,SPF_PASS autolearn=ham version=3.3.2 spammy= X-HELO: mail-wm0-f49.google.com Received: from mail-wm0-f49.google.com (HELO mail-wm0-f49.google.com) (74.125.82.49) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Thu, 16 Jun 2016 16:51:44 +0000 Received: by mail-wm0-f49.google.com with SMTP id f126so58582241wma.1 for ; Thu, 16 Jun 2016 09:51:43 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=Hdcn6FnZYPU1eiTHR6e8MZgo5IKQeI7qIb8+u6pV1MY=; b=mH7U6sGOLfH3PgRUrLmSev2VOZbryILVQFMgLap6eD6jEuvL8eo4y6qZuULq53wGjZ spBFKBibOzrNGkAm9uyZnJeVRF2tjZ99vJaLE5x2kO5sLSY1FjbRc1P3bG8F3kqoR4+y s1G0G3lYkP7ctdMK/SdfdbriN+3XkqukYacKLVQFlHX9+EwaZYM/yhcA7UXzLHdL52/7 EmxY/Sz4hJVPWO8Oqf15xb2plzXhassbPM1KZ63xtdqVTBYHtzon69UxbbkeGloDYPYP cDVSYrnJ17vlEM7iJ12+pXqp9YJeQvjxTysCuPy1EDlxU0eS5uXuJN7hZMsoGoBbwzNx hEig== X-Gm-Message-State: ALyK8tJH3qvigM+V9Q22FeNjM+ph+KRUs1LVcJgB750tfaKMsSlcfOpaWjC+5roCJ5IAL1/kDNWD4fpxqnZqkg== X-Received: by 10.194.134.230 with SMTP id pn6mr550318wjb.165.1466095900932; Thu, 16 Jun 2016 09:51:40 -0700 (PDT) MIME-Version: 1.0 Received: by 10.28.169.2 with HTTP; Thu, 16 Jun 2016 09:51:21 -0700 (PDT) In-Reply-To: <87lh258ufn.fsf@atmel.com> References: <87y46m1h3y.fsf@atmel.com> <5751B5EA.5000108@gjlay.de> <877fe1nrdy.fsf@atmel.com> <87lh258ufn.fsf@atmel.com> From: Denis Chertykov Date: Thu, 16 Jun 2016 16:51:00 -0000 Message-ID: Subject: Re: [Patch, avr] Fix PR 71151 To: Senthil Kumar Selvaraj Cc: Georg-Johann Lay , gcc-patches Content-Type: text/plain; charset=UTF-8 X-SW-Source: 2016-06/txt/msg01265.txt.bz2 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.