From: Georg-Johann Lay <avr@gjlay.de>
To: Senthil Kumar Selvaraj <senthil_kumar.selvaraj@atmel.com>
Cc: gcc-patches@gcc.gnu.org, Denis Chertykov <chertykov@gmail.com>
Subject: Re: [Patch, avr] Fix PR 71151
Date: Fri, 03 Jun 2016 16:52:00 -0000 [thread overview]
Message-ID: <5751B5EA.5000108@gjlay.de> (raw)
In-Reply-To: <87y46m1h3y.fsf@atmel.com>
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 <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.
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();
> +}
next prev parent reply other threads:[~2016-06-03 16:52 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-06-03 14:29 Senthil Kumar Selvaraj
2016-06-03 16:52 ` Georg-Johann Lay [this message]
2016-06-07 11:57 ` Senthil Kumar Selvaraj
2016-06-16 7:28 ` Senthil Kumar Selvaraj
2016-06-16 16:51 ` Denis Chertykov
2016-06-17 4:39 ` Senthil Kumar Selvaraj
2016-08-01 9:42 ` [avr,backported,6] " Georg-Johann Lay
2016-06-22 18:23 ` [Patch, avr] " Georg-Johann Lay
2016-06-23 6:45 ` Senthil Kumar Selvaraj
2016-06-23 16:16 ` Georg-Johann Lay
2016-06-23 17:51 ` Mike Stump
2016-06-04 14:19 ` Georg-Johann Lay
2016-06-17 12:59 ` Georg-Johann Lay
2016-06-19 6:24 ` Senthil Kumar Selvaraj
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5751B5EA.5000108@gjlay.de \
--to=avr@gjlay.de \
--cc=chertykov@gmail.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=senthil_kumar.selvaraj@atmel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).