[Patch, avr] Fix PR 71151

[Patch, avr] Fix PR 71151

[Senthil Kumar Selvaraj]

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.

  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
 
 #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;
+  }
+	y = y + g;
+}
+
+int main()
+{
+	foo(5);
+  if (y != 37)
+		abort();
+
+	foo(0);
+  if (y != 67)
+		abort();
+
+	foo(7);
+  if (y != 98)
+		abort();
+}
-- 
2.7.4

Re: [Patch, avr] Fix PR 71151

[Georg-Johann Lay]

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();
> +}

Re: [Patch, avr] Fix PR 71151

[Georg-Johann Lay]

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.
> 
>   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;

The progmem_swtable_section is no more needed; the code to set it up can 
also be removed.

Johann

> -}
> -
> -
>  /* 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
>  
>  #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;
> +  }
> +	y = y + g;
> +}
> +
> +int main()
> +{
> +	foo(5);
> +  if (y != 37)
> +		abort();
> +
> +	foo(0);
> +  if (y != 67)
> +		abort();
> +
> +	foo(7);
> +  if (y != 98)
> +		abort();
> +}

Re: [Patch, avr] Fix PR 71151

[Senthil Kumar Selvaraj]

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.

Regards
Senthil

>
>
> Johann
>
>
>> +	y = y + g;
>> +}
>> +
>> +int main()
>> +{
>> +	foo(5);
>> +  if (y != 37)
>> +		abort();
>> +
>> +	foo(0);
>> +  if (y != 67)
>> +		abort();
>> +
>> +	foo(7);
>> +  if (y != 98)
>> +		abort();
>> +}

Re: [Patch, avr] Fix PR 71151

[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  <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.



diff --git gcc/config/avr/avr.c gcc/config/avr/avr.c
index ba5cd91..dd99f42 100644
--- gcc/config/avr/avr.c
+++ gcc/config/avr/avr.c
@@ -203,9 +203,6 @@ static GTY(()) rtx xstring_e;
 /* Current architecture.  */
 const avr_arch_t *avr_arch;
 
-/* Section to put switch tables in.  */
-static GTY(()) section *progmem_swtable_section;
-
 /* Unnamed sections associated to __attribute__((progmem)) aka. PROGMEM
    or to address space __flash* or __memx.  Only used as singletons inside
    avr_asm_select_section, but it must not be local there because of GTY.  */
@@ -9461,24 +9458,6 @@ avr_output_progmem_section_asm_op (const void *data)
 static void
 avr_asm_init_sections (void)
 {
-  /* Set up a section for jump tables.  Alignment is handled by
-     ASM_OUTPUT_BEFORE_CASE_LABEL.  */
-
-  if (AVR_HAVE_JMP_CALL)
-    {
-      progmem_swtable_section
-        = get_unnamed_section (0, output_section_asm_op,
-                               "\t.section\t.progmem.gcc_sw_table"
-                               ",\"a\",@progbits");
-    }
-  else
-    {
-      progmem_swtable_section
-        = get_unnamed_section (SECTION_CODE, output_section_asm_op,
-                               "\t.section\t.progmem.gcc_sw_table"
-                               ",\"ax\",@progbits");
-    }
-
   /* Override section callbacks to keep track of `avr_need_clear_bss_p'
      resp. `avr_need_copy_data_p'.  */
 
@@ -9488,65 +9467,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 +13667,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
 
 #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..e523ce0
--- /dev/null
+++ gcc/testsuite/gcc.target/avr/pr71151-2.c
@@ -0,0 +1,24 @@
+/* { dg-do run } */
+/* { dg-options "-Os -fno-tree-switch-conversion -ffunction-sections -fdata-sections" } */
+
+/* Make sure jumptables work properly if placed below 64 KB i.e. 2 byte
+   flash address for loading jump table entry, 2 byte entry, after
+   removing the special section placement hook. */
+
+#include "exit-abort.h"
+#include "pr71151-common.h"
+
+int main()
+{
+	foo(5);
+	if (y != 37)
+		abort();
+
+	foo(0);
+	if (y != 67)
+		abort();
+
+	foo(7);
+	if (y != 98)
+		abort();
+}
diff --git gcc/testsuite/gcc.target/avr/pr71151-3.c gcc/testsuite/gcc.target/avr/pr71151-3.c
new file mode 100644
index 0000000..ce0ba59
--- /dev/null
+++ gcc/testsuite/gcc.target/avr/pr71151-3.c
@@ -0,0 +1,25 @@
+/* { dg-do run } */
+/* { dg-options "-Os -fno-tree-switch-conversion -ffunction-sections -mno-relax -fdata-sections -Wl,--section-start=.foo=0x10000" } */
+
+/* Make sure jumptables work properly if placed above 64 KB and below 128 KB,
+   i.e. 3 byte flash address for loading jump table entry and 2 byte jump table
+   entry, with relaxation disabled, after removing the special section
+   placement hook. */
+
+#include "exit-abort.h"
+#include "pr71151-common.h"
+
+int main()
+{
+	foo(5);
+	if (y != 37)
+		abort();
+
+	foo(0);
+	if (y != 67)
+		abort();
+
+	foo(7);
+	if (y != 98)
+		abort();
+}
diff --git gcc/testsuite/gcc.target/avr/pr71151-4.c gcc/testsuite/gcc.target/avr/pr71151-4.c
new file mode 100644
index 0000000..51250b0
--- /dev/null
+++ gcc/testsuite/gcc.target/avr/pr71151-4.c
@@ -0,0 +1,25 @@
+/* { dg-do run } */
+/* { dg-options "-Os -fno-tree-switch-conversion -ffunction-sections -fdata-sections -mrelax -Wl,--section-start=.foo=0x10000" } */
+
+/* Make sure jumptables work properly if placed above 64 KB and below 128 KB,
+   i.e. 3 byte flash address for loading jump table entry and 2 byte jump
+   table entry, with relaxation enabled, after removing the special section
+   placement hook. */
+
+#include "exit-abort.h"
+#include "pr71151-common.h"
+
+int main()
+{
+	foo(5);
+	if (y != 37)
+		abort();
+
+	foo(0);
+	if (y != 67)
+		abort();
+
+	foo(7);
+	if (y != 98)
+		abort();
+}
diff --git gcc/testsuite/gcc.target/avr/pr71151-5.c gcc/testsuite/gcc.target/avr/pr71151-5.c
new file mode 100644
index 0000000..40fdb22
--- /dev/null
+++ gcc/testsuite/gcc.target/avr/pr71151-5.c
@@ -0,0 +1,30 @@
+/* { dg-do run } */
+/* { dg-options "-Os -fno-tree-switch-conversion -ffunction-sections -fdata-sections -mno-relax -Wl,--section-start=.foo=0x20000" } */
+
+/* Make sure jumptables work properly if placed above 128 KB, i.e. 3 byte
+   flash address for loading jump table entry and a jump table entry
+   that is a stub, with relaxation disabled, after removing the special
+   section placement hook. */
+
+#include "exit-abort.h"
+#include "pr71151-common.h"
+
+int main()
+{
+	/* Not meant for devices with flash <= 128K */
+#if defined (__AVR_2_BYTE_PC__)
+	exit(0);
+#else
+	foo(5);
+	if (y != 37)
+		abort();
+
+	foo(0);
+	if (y != 67)
+		abort();
+
+	foo(7);
+	if (y != 98)
+		abort();
+#endif
+}
diff --git gcc/testsuite/gcc.target/avr/pr71151-6.c gcc/testsuite/gcc.target/avr/pr71151-6.c
new file mode 100644
index 0000000..f4d2ef9
--- /dev/null
+++ gcc/testsuite/gcc.target/avr/pr71151-6.c
@@ -0,0 +1,30 @@
+/* { dg-do run } */
+/* { dg-options "-Os -fno-tree-switch-conversion -ffunction-sections -fdata-sections -mrelax -Wl,--section-start=.foo=0x20000" } */
+
+/* Make sure jumptables work properly if placed above 128 KB, i.e. 3 byte
+   flash address for loading jump table entry and a jump table entry
+   that is a stub, with relaxation enabled, after removing the special
+   section placement hook. */
+
+#include "exit-abort.h"
+#include "pr71151-common.h"
+
+int main()
+{
+	/* Not meant for devices with flash <= 128K */
+#if defined (__AVR_2_BYTE_PC__)
+	exit(0);
+#else
+	foo(5);
+	if (y != 37)
+		abort();
+
+	foo(0);
+	if (y != 67)
+		abort();
+
+	foo(7);
+	if (y != 98)
+		abort();
+#endif
+}
diff --git gcc/testsuite/gcc.target/avr/pr71151-7.c gcc/testsuite/gcc.target/avr/pr71151-7.c
new file mode 100644
index 0000000..cdc7ea9
--- /dev/null
+++ gcc/testsuite/gcc.target/avr/pr71151-7.c
@@ -0,0 +1,28 @@
+/* { dg-do run } */
+/* { dg-options "-Os -fno-tree-switch-conversion -ffunction-sections -fdata-sections -mno-relax -Wl,--section-start=.foo=0x1fffa" } */
+
+/* Make sure jumptables work properly if placed straddling 128 KB i.e
+   some entries below 128 KB and some above it, with relaxation disabled. */
+
+#include "exit-abort.h"
+#include "pr71151-common.h"
+
+int main()
+{
+	/* Not meant for devices with flash <= 128K */
+#if defined (__AVR_2_BYTE_PC__)
+	exit(0);
+#else
+	foo(5);
+	if (y != 37)
+		abort();
+
+	foo(0);
+	if (y != 67)
+		abort();
+
+	foo(7);
+	if (y != 98)
+		abort();
+#endif
+}
diff --git gcc/testsuite/gcc.target/avr/pr71151-8.c gcc/testsuite/gcc.target/avr/pr71151-8.c
new file mode 100644
index 0000000..0b7bf6a
--- /dev/null
+++ gcc/testsuite/gcc.target/avr/pr71151-8.c
@@ -0,0 +1,28 @@
+/* { dg-do run } */
+/* { dg-options "-Os -fno-tree-switch-conversion -ffunction-sections -fdata-sections -mrelax -Wl,--section-start=.foo=0x1fffa" } */
+
+/* Make sure jumptables work properly if placed straddling 128 KB i.e
+   some entries below 128 KB and some above it, with relaxation disabled. */
+
+#include "exit-abort.h"
+#include "pr71151-common.h"
+
+int main()
+{
+	/* Not meant for devices with flash <= 128K */
+#if defined (__AVR_2_BYTE_PC__)
+	exit(0);
+#else
+	foo(5);
+	if (y != 37)
+		abort();
+
+	foo(0);
+	if (y != 67)
+		abort();
+
+	foo(7);
+	if (y != 98)
+		abort();
+#endif
+}
diff --git gcc/testsuite/gcc.target/avr/pr71151-common.h gcc/testsuite/gcc.target/avr/pr71151-common.h
new file mode 100644
index 0000000..0df1783
--- /dev/null
+++ gcc/testsuite/gcc.target/avr/pr71151-common.h
@@ -0,0 +1,27 @@
+volatile char y;
+volatile char g;
+
+__attribute__((section(".foo")))
+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;
+	}
+	y = y + g;
+}
-- 
2.7.4

Re: [Patch, avr] Fix PR 71151

[Denis Chertykov]

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.

Re: [Patch, avr] Fix PR 71151

[Senthil Kumar Selvaraj]

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

Re: [Patch, avr] Fix PR 71151

[Georg-Johann Lay]

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.
> 
>   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>
> 

Missing PR target/71151

> 	* 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>
> 

Missing PR target/71151

> 	* gcc/testsuite/gcc.target/avr/pr71151-1.c: New.
> 	* gcc/testsuite/gcc.target/avr/pr71151-2.c: New.
> 

With the PR entry in the ChangeLog / commit message it might be easier 
to find the change, and the respective bugzilla PR will get an automatic 
entry pointing to the commit.

Thanks,  Johann

Re: [Patch, avr] Fix PR 71151

[Senthil Kumar Selvaraj]

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.
>> 
>>   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>
>> 
>
> Missing PR target/71151
>
>> 	* 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>
>> 
>
> Missing PR target/71151
>
>> 	* gcc/testsuite/gcc.target/avr/pr71151-1.c: New.
>> 	* gcc/testsuite/gcc.target/avr/pr71151-2.c: New.
>> 

Sorry I missed that. Is it ok to modify the entry alone again?

I just started using the mklog perl script from gcc/contrib
instead of writing the entries by hand - I didn't realize
that script doesn't know about PRs.

Regards
Senthil

Re: [Patch, avr] Fix PR 71151

[Georg-Johann Lay]

Senthil Kumar Selvaraj schrieb:
> Senthil Kumar Selvaraj writes:
> 
>> Georg-Johann Lay writes:
>>
>>> Senthil Kumar Selvaraj schrieb:
>>>> Hi,
>>>>
>>>>   [set JUMP_TABLES_IN_TEXT_SECTION to 1]
>>
>> I added tests that use linker relaxation and discovered a relaxation 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.

Should we mention in the release notes that Binutils >= 2.26 is needed 
for avr-gcc >= 6 ?

Maybe even check during configure whether an appropriate version of 
Binutils is used?

Johann

Re: [Patch, avr] Fix PR 71151

[Senthil Kumar Selvaraj]

Georg-Johann Lay writes:

> Senthil Kumar Selvaraj schrieb:
>> Senthil Kumar Selvaraj writes:
>> 
>>> Georg-Johann Lay writes:
>>>
>>>> Senthil Kumar Selvaraj schrieb:
>>>>> Hi,
>>>>>
>>>>>   [set JUMP_TABLES_IN_TEXT_SECTION to 1]
>>>
>>> I added tests that use linker relaxation and discovered a relaxation 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.
>
> Should we mention in the release notes that Binutils >= 2.26 is needed 
> for avr-gcc >= 6 ?

Yes, we should document it. binutils 2.25 would probably work too, as
the bugs were introduced only in binutils 2.26. I'll check and send a patch.
>
> Maybe even check during configure whether an appropriate version of 
> Binutils is used?

That would be nice, but is it ok to add target specific conditions to
configure.ac?

Regards
Senthil

Re: [Patch, avr] Fix PR 71151

[Georg-Johann Lay]

Senthil Kumar Selvaraj schrieb:
> Georg-Johann Lay writes:
> 
>> Senthil Kumar Selvaraj schrieb:
>>> Senthil Kumar Selvaraj writes:
>>>
>>>> Georg-Johann Lay writes:
>>>>
>>>>> Senthil Kumar Selvaraj schrieb:
>>>>>> Hi,
>>>>>>
>>>>>>   [set JUMP_TABLES_IN_TEXT_SECTION to 1]
>>>> I added tests that use linker relaxation and discovered a relaxation 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.
>> Should we mention in the release notes that Binutils >= 2.26 is needed 
>> for avr-gcc >= 6 ?
> 
> Yes, we should document it. binutils 2.25 would probably work too, as
> the bugs were introduced only in binutils 2.26. I'll check and send a patch.
>> Maybe even check during configure whether an appropriate version of 
>> Binutils is used?
> 
> That would be nice, but is it ok to add target specific conditions to
> configure.ac?

We already have avr-specific tests in gcc/configure.ac which test 
whether -mrmw and --mlink-relax are supported by as or not.  This is 
then used in gen-avr-mmcu-specs.c:

#ifdef HAVE_AS_AVR_MLINK_RELAX_OPTION
   fprintf (f, "*asm_relax:\n\t%s\n\n", ASM_RELAX_SPEC);
#endif // have avr-as --mlink-relax

#ifdef HAVE_AS_AVR_MRMW_OPTION
   fprintf (f, "*asm_rmw:\n%s\n\n", rmw
            ? "\t%{!mno-rmw: -mrmw}"
            : "\t%{mrmw}");
#endif // have avr-as -mrmw


Johann

Re: [Patch, avr] Fix PR 71151

[Mike Stump]

On Jun 23, 2016, at 9:16 AM, Georg-Johann Lay <avr@gjlay.de> wrote:
>>>>> Maybe even check during configure whether an appropriate version of Binutils is used?
>> That would be nice, but is it ok to add target specific conditions to
>> configure.ac?
> 
> We already have avr-specific tests in gcc/configure.ac

Further, the only point of configure is to have target specific tests in it.  :-)

[avr,backported,6] Fix PR 71151

[Georg-Johann Lay]

[-- Attachment #1: Type: text/plain, Size: 1056 bytes --]

Applied the simple fix for PR71151: Set JUMP_TABLES_IN_TEXT_SECTION to 1.

https://gcc.gnu.org/r238935

Johann

gcc/
	Backport from 2016-06-16 trunk r237536.

	2016-06-16  Senthil Kumar Selvaraj  <senthil_kumar.selvaraj@atmel.com>

	PR target/71151
	* 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.

testsuite/
	Backport from 2016-06-16 trunk r237536, r237910.

	2016-06-16  Senthil Kumar Selvaraj  <senthil_kumar.selvaraj@atmel.com>

	PR target/71151
	* 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 file.



[-- Attachment #2: pr71151-v6.diff --]
[-- Type: text/x-patch, Size: 12148 bytes --]

Index: config/avr/avr.c
===================================================================
--- config/avr/avr.c	(revision 238934)
+++ config/avr/avr.c	(working copy)
@@ -203,9 +203,6 @@ static GTY(()) rtx xstring_e;
 /* Current architecture.  */
 const avr_arch_t *avr_arch;
 
-/* Section to put switch tables in.  */
-static GTY(()) section *progmem_swtable_section;
-
 /* Unnamed sections associated to __attribute__((progmem)) aka. PROGMEM
    or to address space __flash* or __memx.  Only used as singletons inside
    avr_asm_select_section, but it must not be local there because of GTY.  */
@@ -9461,24 +9458,6 @@ avr_output_progmem_section_asm_op (const
 static void
 avr_asm_init_sections (void)
 {
-  /* Set up a section for jump tables.  Alignment is handled by
-     ASM_OUTPUT_BEFORE_CASE_LABEL.  */
-
-  if (AVR_HAVE_JMP_CALL)
-    {
-      progmem_swtable_section
-        = get_unnamed_section (0, output_section_asm_op,
-                               "\t.section\t.progmem.gcc_sw_table"
-                               ",\"a\",@progbits");
-    }
-  else
-    {
-      progmem_swtable_section
-        = get_unnamed_section (SECTION_CODE, output_section_asm_op,
-                               "\t.section\t.progmem.gcc_sw_table"
-                               ",\"ax\",@progbits");
-    }
-
   /* Override section callbacks to keep track of `avr_need_clear_bss_p'
      resp. `avr_need_copy_data_p'.  */
 
@@ -9488,65 +9467,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.  */
 
@@ -13749,9 +13669,6 @@ avr_fold_builtin (tree fndecl, int n_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
 
Index: config/avr/avr.h
===================================================================
--- config/avr/avr.h	(revision 238934)
+++ config/avr/avr.h	(working copy)
@@ -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
 
 #define ASM_COMMENT_START " ; "
 
Index: testsuite/gcc.target/avr/pr71151-1.c
===================================================================
--- testsuite/gcc.target/avr/pr71151-1.c	(nonexistent)
+++ testsuite/gcc.target/avr/pr71151-1.c	(working copy)
@@ -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");
+}
Index: testsuite/gcc.target/avr/pr71151-2.c
===================================================================
--- testsuite/gcc.target/avr/pr71151-2.c	(nonexistent)
+++ testsuite/gcc.target/avr/pr71151-2.c	(working copy)
@@ -0,0 +1,26 @@
+/* { dg-do run } */
+/* { dg-options "-Os -fno-tree-switch-conversion -ffunction-sections -fdata-sections" } */
+
+/* Make sure jumptables work properly if placed below 64 KB i.e. 2 byte
+   flash address for loading jump table entry, 2 byte entry, after
+   removing the special section placement hook. */
+
+#define SECTION_NAME ".foo"
+
+#include "exit-abort.h"
+#include "pr71151-common.h"
+
+int main()
+{
+	foo(5);
+	if (y != 37)
+		abort();
+
+	foo(0);
+	if (y != 67)
+		abort();
+
+	foo(7);
+	if (y != 98)
+		abort();
+}
Index: testsuite/gcc.target/avr/pr71151-3.c
===================================================================
--- testsuite/gcc.target/avr/pr71151-3.c	(nonexistent)
+++ testsuite/gcc.target/avr/pr71151-3.c	(working copy)
@@ -0,0 +1,32 @@
+/* { dg-do run } */
+/* { dg-options "-Os -fno-tree-switch-conversion -ffunction-sections -mno-relax -fdata-sections -Wl,--section-start=.foo=0x10000" } */
+
+#ifdef __AVR_HAVE_ELPM__
+/* Make sure jumptables work properly if placed above 64 KB and below 128 KB,
+   i.e. 3 byte flash address for loading jump table entry and 2 byte jump table
+   entry, with relaxation disabled, after removing the special section
+   placement hook. */
+#define SECTION_NAME ".foo"
+#else
+/* No special jump table placement so that avrtest won't abort
+   for, e.g. ATmega64.  */
+#define SECTION_NAME ".text.foo"
+#endif
+
+#include "exit-abort.h"
+#include "pr71151-common.h"
+
+int main()
+{
+	foo(5);
+	if (y != 37)
+		abort();
+
+	foo(0);
+	if (y != 67)
+		abort();
+
+	foo(7);
+	if (y != 98)
+		abort();
+}
Index: testsuite/gcc.target/avr/pr71151-4.c
===================================================================
--- testsuite/gcc.target/avr/pr71151-4.c	(nonexistent)
+++ testsuite/gcc.target/avr/pr71151-4.c	(working copy)
@@ -0,0 +1,32 @@
+/* { dg-do run } */
+/* { dg-options "-Os -fno-tree-switch-conversion -ffunction-sections -fdata-sections -mrelax -Wl,--section-start=.foo=0x10000" } */
+
+#ifdef __AVR_HAVE_ELPM__
+/* Make sure jumptables work properly if placed above 64 KB and below 128 KB,
+   i.e. 3 byte flash address for loading jump table entry and 2 byte jump
+   table entry, with relaxation enabled, after removing the special section
+   placement hook. */
+#define SECTION_NAME ".foo"
+#else
+/* No special jump table placement so that avrtest won't abort
+   for, e.g. ATmega64.  */
+#define SECTION_NAME ".text.foo"
+#endif
+
+#include "exit-abort.h"
+#include "pr71151-common.h"
+
+int main()
+{
+	foo(5);
+	if (y != 37)
+		abort();
+
+	foo(0);
+	if (y != 67)
+		abort();
+
+	foo(7);
+	if (y != 98)
+		abort();
+}
Index: testsuite/gcc.target/avr/pr71151-5.c
===================================================================
--- testsuite/gcc.target/avr/pr71151-5.c	(nonexistent)
+++ testsuite/gcc.target/avr/pr71151-5.c	(working copy)
@@ -0,0 +1,32 @@
+/* { dg-do run } */
+/* { dg-options "-Os -fno-tree-switch-conversion -ffunction-sections -fdata-sections -mno-relax -Wl,--section-start=.foo=0x20000" } */
+
+#ifdef __AVR_3_BYTE_PC__
+/* Make sure jumptables work properly if placed above 128 KB, i.e. 3 byte
+   flash address for loading jump table entry and a jump table entry
+   that is a stub, with relaxation disabled, after removing the special
+   section placement hook. */
+#define SECTION_NAME ".foo"
+#else
+/* No special jump table placement so that avrtest won't abort
+   for, e.g. ATmega128.  */
+#define SECTION_NAME ".text.foo"
+#endif
+
+#include "exit-abort.h"
+#include "pr71151-common.h"
+
+int main()
+{
+	foo(5);
+	if (y != 37)
+		abort();
+
+	foo(0);
+	if (y != 67)
+		abort();
+
+	foo(7);
+	if (y != 98)
+		abort();
+}
Index: testsuite/gcc.target/avr/pr71151-6.c
===================================================================
--- testsuite/gcc.target/avr/pr71151-6.c	(nonexistent)
+++ testsuite/gcc.target/avr/pr71151-6.c	(working copy)
@@ -0,0 +1,32 @@
+/* { dg-do run } */
+/* { dg-options "-Os -fno-tree-switch-conversion -ffunction-sections -fdata-sections -mrelax -Wl,--section-start=.foo=0x20000" } */
+
+#ifdef __AVR_3_BYTE_PC__
+/* Make sure jumptables work properly if placed above 128 KB, i.e. 3 byte
+   flash address for loading jump table entry and a jump table entry
+   that is a stub, with relaxation enabled, after removing the special
+   section placement hook. */
+#define SECTION_NAME ".foo"
+#else
+/* No special jump table placement so that avrtest won't abort
+   for, e.g. ATmega128.  */
+#define SECTION_NAME ".text.foo"
+#endif
+
+#include "exit-abort.h"
+#include "pr71151-common.h"
+
+int main()
+{
+	foo(5);
+	if (y != 37)
+		abort();
+
+	foo(0);
+	if (y != 67)
+		abort();
+
+	foo(7);
+	if (y != 98)
+		abort();
+}
Index: testsuite/gcc.target/avr/pr71151-7.c
===================================================================
--- testsuite/gcc.target/avr/pr71151-7.c	(nonexistent)
+++ testsuite/gcc.target/avr/pr71151-7.c	(working copy)
@@ -0,0 +1,30 @@
+/* { dg-do run } */
+/* { dg-options "-Os -fno-tree-switch-conversion -ffunction-sections -fdata-sections -mno-relax -Wl,--section-start=.foo=0x1fffa" } */
+
+#ifdef __AVR_3_BYTE_PC__
+/* Make sure jumptables work properly if placed straddling 128 KB i.e
+   some entries below 128 KB and some above it, with relaxation disabled. */
+#define SECTION_NAME ".foo"
+#else
+/* No special jump table placement so that avrtest won't abort
+   for, e.g. ATmega128.  */
+#define SECTION_NAME ".text.foo"
+#endif
+
+#include "exit-abort.h"
+#include "pr71151-common.h"
+
+int main()
+{
+	foo(5);
+	if (y != 37)
+		abort();
+
+	foo(0);
+	if (y != 67)
+		abort();
+
+	foo(7);
+	if (y != 98)
+		abort();
+}
Index: testsuite/gcc.target/avr/pr71151-8.c
===================================================================
--- testsuite/gcc.target/avr/pr71151-8.c	(nonexistent)
+++ testsuite/gcc.target/avr/pr71151-8.c	(working copy)
@@ -0,0 +1,29 @@
+/* { dg-do run } */
+/* { dg-options "-Os -fno-tree-switch-conversion -ffunction-sections -fdata-sections -mrelax -Wl,--section-start=.foo=0x1fffa" } */
+
+#ifdef __AVR_3_BYTE_PC__
+/* Make sure jumptables work properly if placed straddling 128 KB i.e
+   some entries below 128 KB and some above it, with relaxation disabled. */
+#define SECTION_NAME ".foo"
+#else
+/* No special jump table placement so that avrtest won't abort.  */
+#define SECTION_NAME ".text.foo"
+#endif
+
+#include "exit-abort.h"
+#include "pr71151-common.h"
+
+int main()
+{
+	foo(5);
+	if (y != 37)
+		abort();
+
+	foo(0);
+	if (y != 67)
+		abort();
+
+	foo(7);
+	if (y != 98)
+		abort();
+}
Index: testsuite/gcc.target/avr/pr71151-common.h
===================================================================
--- testsuite/gcc.target/avr/pr71151-common.h	(nonexistent)
+++ testsuite/gcc.target/avr/pr71151-common.h	(working copy)
@@ -0,0 +1,27 @@
+volatile char y;
+volatile char g;
+
+__attribute__((section(SECTION_NAME)))
+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;
+	}
+	y = y + g;
+}