Bug 71151 - [avr] -fmerge-constants and -fdata-sections/-ffunction-sections results in string constants in .progmem.gcc_sw section
Summary: [avr] -fmerge-constants and -fdata-sections/-ffunction-sections results in st...
Alias: None
Product: gcc
Classification: Unclassified
Component: target (show other bugs)
Version: 6.1.0
: P3 normal
Target Milestone: 6.2
Assignee: Not yet assigned to anyone
Keywords: wrong-code
: 70999 (view as bug list)
Depends on:
Blocks: 81075
  Show dependency treegraph
Reported: 2016-05-16 17:09 UTC by Senthil Kumar Selvaraj
Modified: 2017-06-14 10:33 UTC (History)
6 users (show)

See Also:
Target: avr
Known to work:
Known to fail:
Last reconfirmed: 2016-08-08 00:00:00

Tentative patch for 6.1 (592 bytes, patch)
2016-05-31 16:02 UTC, Senthil Kumar Selvaraj
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Senthil Kumar Selvaraj 2016-05-16 17:09:13 UTC
Using -fdata-sections causes string literals in functions to go into .progmem.sw_gcc, instead of .rodata.str. This is incorrect, since code using them expects data memory addresses, not flash.

$ cat printf.c
extern void bar(const char*);
void foo(void)

$ avr-gcc -mmcu=atmega32u2 printf.c -S -Os -fdata-sections -ffunction-sections
$ cat printf.s
jaguar:~/scratch$ cat printf.s
	.file	"printf.c"
__SP_H__ = 0x3e
__SP_L__ = 0x3d
__SREG__ = 0x3f
__tmp_reg__ = 0
__zero_reg__ = 1
	.section	.progmem.gcc_sw_table.foo.str1.1,"aMS",@progbits,1
	.string	"BBBBBBBBBB"
	.section	.text.foo,"ax",@progbits
.global	foo
	.type	foo, @function
/* prologue: function */
/* frame size = 0 */
/* stack size = 0 */
.L__stack_usage = 0
	ldi r24,lo8(.LC0)
	ldi r25,hi8(.LC0)
	jmp bar
	.size	foo, .-foo
	.ident	"GCC: (GNU) 6.1.0"

As the testcase shows, the string goes into progmem.gcc_sw_table.<fn_name>.*, and this gets placed in flash by the linker script. Reading from the address in r25:24 will obviously not give the correct results - it will read from whatever happens to be mapped at the same address in the data address space.
Comment 1 Senthil Kumar Selvaraj 2016-05-18 03:35:53 UTC
A workaround is to disable constant merging (-fno-merge-constants).
Comment 2 Georg-Johann Lay 2016-05-18 08:17:51 UTC
Cannot reproduce this on 5.x.

The avr BE tries to apply -fdata-sections to data in progmem in a similar way like -fdata-sections acts on data in RAM.  A dedicated option like -mprogmem-sections was not possible because of the terrible section implementation in varasm.c.

Presumably, the problem is TARGET_ASM_FUNCTION_RODATA_SECTION in avr.c.

Maybe we'll have to give up the feature alltogether and drop the .progmem.var subsections and return to one bulk .progmem.data again.
Comment 3 Senthil Kumar Selvaraj 2016-05-18 09:14:55 UTC
Yeah, it's a 6.x thing. There's a commit that changes varasm.c from using targetm.asm_out.mergeable_rodata_prefix to a function call to targetm.asm_out.function_rodata_section hook if the section category is SECCAT_RODATA_MERGE_STR. So avr_asm_function_rodata_section now gets called for string literals inside functions, not just jumptables.

I'm trying out a fix that propagates the section category down to the target hook and have the avr backend use it to distinguish between jumptables and other data.
Comment 4 Georg-Johann Lay 2016-05-21 10:31:21 UTC
*** Bug 70999 has been marked as a duplicate of this bug. ***
Comment 5 Anatol 2016-05-26 15:17:31 UTC
This bug has been reported by Arch Linux users. https://bugs.archlinux.org/task/49284

It is a severe compiler issue that stop avr-gcc 6 from using. Consider changing "Importance" status to blocker.
Comment 6 Georg-Johann Lay 2016-05-27 15:55:54 UTC
(In reply to Anatol from comment #5)
> It is a severe compiler issue that stop avr-gcc 6 from using.
> Consider changing "Importance" status to blocker.

It's definite not a "blocker".  "blocker" would mean the issue is so severe that no toolchain could be released, e.g. because the compiler for a primary or secondary platform cannot be built.

Moreover, as Senthil pointed out above, the problem can be worked around by adding -fno-merge-constants to the command options.

Anyway, twiddling the "importance" won't have any effect w.r.t. bug fixing quality or swiftness...  In the case of avr, that speed is solely determindey by the number of people that are willing to contribute to avr-gcc...

If all goes well I might have a day or two to work on avr-gcc in June.
Comment 7 Senthil Kumar Selvaraj 2016-05-31 16:02:34 UTC
Created attachment 38613 [details]
Tentative patch for 6.1

Looks like the right fix will need to somehow differentiate between jump tables and other per function data when the function_rodata_section target hook is invoked. I'll submit a tentative patch in the next couple of days that does that.

However, I don't think that patch will get into 6.x, given that it changes the target hook interface. The attached patch instead tries to work around the problem, by letting default_elf_select_section run as usual, but then returning a different section with the name reverted to the right prefix (rodata instead of progmem), with the correct flags (MERGE and STRINGS)
Comment 8 Georg-Johann Lay 2016-06-01 12:31:18 UTC
What about avr_asm_function_rodata_section?  Isn't it possible to filter DECL and only transform for addr_vect?
Comment 9 Senthil Kumar Selvaraj 2016-06-01 13:04:04 UTC
For both kinds of invocation (mergeable constants and jump tables), the decl passed is current_function_decl. And that looks right from the documentation for the target hook.

I'll submit another patch for trunk that adds a new SECCAT_RODATA_JUMPTABLE to enum section_category, and then passes the section_category down to the target hook. avr_asm_function_rodata_section can then choose to return the correct section based on the category.

What do you think?
Comment 10 Georg-Johann Lay 2016-06-01 14:23:44 UTC
Well, then we should remove TARGET_ASM_FUNCTION_RODATA_SECTION implementation altogether (it's weird, not only because it patches flag_data_sections), same for ASM_OUTPUT_ADDR_VEC_ELT.

Instead implement ASM_OUTPUT_ADDR_VEC and do the whole addr_vec stuff by hand:

1) switch to correct section: if -ffunction-sections is on, cook up a section like .progmem.gcc_sw_table.<function-asm-name>, otherwise just .progmem.gcc_sw_table

1) Alternatively, just emit .pushsection and .popsection around the jump table.

2) Output alignment .p2align.  The original alignment from ASM_OUTPUT_BEFORE_CASE_LABEL might be too early (wrong section), so that the alignment must be output again.  ASM_OUTPUT_BEFORE_CASE_LABEL is no more needed.

3) Output the jump table, see final.c for how to iterate.

Anyway, we might consider putting the jump table into .text section.  Since PR63223 we can handle jump-tables at any location, there is no need for having it in .progmem (which is supposed to reside in the lowest 64k).  And for Tiny targets, where .rodata is the best place, JUMP_TABLES_IN_TEXT_SECTION can just return 0.
Comment 11 Georg-Johann Lay 2016-06-01 14:58:41 UTC
well, IIRC for Tiny .rodata is still a part of .data and not part of .text?  If this is still the case, then on Tiny the best place for jump tables is also .text.
Comment 12 Senthil Kumar Selvaraj 2016-06-02 11:36:52 UTC
Yes, tiny also has .rodata in .data

I'd totally missed PR 63323, so just removing the target hook and turning on JUMP_TABLES_IN_TEXT_SECTION does the trick, like you said. Wrote a basic test to make sure jumptables get generated and used correctly, and that worked too.
Comment 13 Georg-Johann Lay 2016-06-03 07:39:11 UTC
Disadvantage of havong tables in same text section a code is that code side might increase for the following reason:  branches that cross a switch statement will also have to cross the jump table, hence branch offsets and thus code size might increase.
Comment 14 Georg-Johann Lay 2016-07-01 12:10:24 UTC
Author: gjl
Date: Fri Jul  1 12:09:53 2016
New Revision: 237910

URL: https://gcc.gnu.org/viewcvs?rev=237910&root=gcc&view=rev
	PR target/71151
	* gcc.target/avr/pr71151-common.h (foo): Use macro SECTION_NAME
	instead of ".foo" for its section name.
	* gcc.target/avr/pr71151-2.c (SECTION_NAME): Define appropriately
	depending on MCU's flash size.
	* gcc.target/avr/pr71151-3.c (SECTION_NAME): Dito.
	* gcc.target/avr/pr71151-4.c (SECTION_NAME): Dito.
	* gcc.target/avr/pr71151-5.c (SECTION_NAME): Dito.
	* gcc.target/avr/pr71151-6.c (SECTION_NAME): Dito.
	* gcc.target/avr/pr71151-7.c (SECTION_NAME): Dito.
	* gcc.target/avr/pr71151-8.c (SECTION_NAME): Dito.

Comment 15 Georg-Johann Lay 2016-08-01 09:15:58 UTC
Author: gjl
Date: Mon Aug  1 09:15:24 2016
New Revision: 238935

URL: https://gcc.gnu.org/viewcvs?rev=238935&root=gcc&view=rev
	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): Remove.
	(avr_asm_function_rodata_section): Remove.
	* config/avr/avr.h (JUMP_TABLES_IN_TEXT_SECTION): Define to 1.

	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.

Comment 16 Senthil Kumar Selvaraj 2016-08-08 08:14:52 UTC
Fixed in trunk and gcc-6 branch.
Comment 17 Georg-Johann Lay 2016-08-08 10:21:43 UTC
What about the performance issue re. crossing the jump table with a branch?

An easy fix would be to emit ".subsection 1" before the jump table; you think we should do this?
Comment 18 Urja Rannikko 2016-08-27 13:50:46 UTC
This fix seems to have caused an another problem on AVR, as described on 
arch linux bug tracker https://bugs.archlinux.org/task/49284#comment149872

Also my software (this example being https://github.com/urjaman/frser-m328lpcspi ) fails to build with similar output:
avr-gcc  -mmcu=atmega328p -O3 -Wl,--relax -fno-tree-switch-conversion -Wall -W -pipe -flto -flto-partition=none -fwhole-program -std=gnu99 -Ilibfrser -std=gnu99 -I./ -o frser-m328lpcspi.out main.c uart.c flash.c ciface.c console.c lib.c appdb.c commands.c libfrser/frser.c libfrser/udelay.c libfrser/dxprint.c libfrser/spilib.c libfrser/spihw_avrspi.c libfrser/lpcfwh.c nibble.c
/tmp/ccMVXcOJ.lto.o: In function `spi_spiop_end':
<artificial>:(.text+0x12e6): relocation truncated to fit: R_AVR_7_PCREL against `no symbol'
/tmp/ccMVXcOJ.lto.o: In function `frser_main':
<artificial>:(.text+0x1338): relocation truncated to fit: R_AVR_7_PCREL against `no symbol'
<artificial>:(.text+0x1362): relocation truncated to fit: R_AVR_7_PCREL against `no symbol'
<artificial>:(.text+0x1370): relocation truncated to fit: R_AVR_7_PCREL against `no symbol'
<artificial>:(.text+0x13dc): relocation truncated to fit: R_AVR_7_PCREL against `no symbol'
<artificial>:(.text+0x13e4): relocation truncated to fit: R_AVR_7_PCREL against `no symbol'
<artificial>:(.text+0x13ec): relocation truncated to fit: R_AVR_7_PCREL against `no symbol'
<artificial>:(.text+0x140e): relocation truncated to fit: R_AVR_7_PCREL against `no symbol'
<artificial>:(.text+0x1428): relocation truncated to fit: R_AVR_7_PCREL against `no symbol'
<artificial>:(.text+0x1450): relocation truncated to fit: R_AVR_7_PCREL against `no symbol'

Not all software fails (simpler/smaller stuff seems to work), but this is pretty bad, should we open a seperate bug or what?
Comment 19 Senthil Kumar Selvaraj 2016-08-27 19:10:59 UTC
Can reproduce this on trunk with binutils master. Mostly likely a binutils/linker bug, as turning off relaxation fixes the error. Will debug further and post the conclusions.
Comment 20 Georg-Johann Lay 2016-08-29 11:37:28 UTC
(In reply to Senthil Kumar Selvaraj from comment #19)
> Can reproduce this on trunk

So it's not related to PR72767 then? (Fixed since 2016-08-01)
Comment 21 Senthil Kumar Selvaraj 2016-08-30 17:16:00 UTC
It occurs with "7.0.0 20160824 (experimental) (GCC)". Besides, the errors go away if I remove --relax, so I think it's a linker issue.
Comment 22 Senthil Kumar Selvaraj 2016-08-31 13:48:26 UTC
Confirmed that it's a linker issue related to adjusting reloc addends in the presence of align directives. Found two separate bugs, will post patches later this week.
Comment 23 Senthil Kumar Selvaraj 2016-09-01 05:42:40 UTC
Tracking binutils bug https://sourceware.org/bugzilla/show_bug.cgi?id=20545