[Patch, avr] Fix PR 71151

Senthil Kumar Selvaraj senthil_kumar.selvaraj@atmel.com
Tue Jun 7 11:57:00 GMT 2016


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



More information about the Gcc-patches mailing list