Bug 92606 - [9/10/11/12 Regression][avr] invalid merge of symbols in progmem and data sections
Summary: [9/10/11/12 Regression][avr] invalid merge of symbols in progmem and data sec...
Status: NEW
Alias: None
Product: gcc
Classification: Unclassified
Component: ipa (show other bugs)
Version: 9.2.0
: P2 normal
Target Milestone: 9.5
Assignee: Not yet assigned to anyone
URL:
Keywords: wrong-code
Depends on: 92932
Blocks:
  Show dependency treegraph
 
Reported: 2019-11-20 23:20 UTC by Janis Hamme
Modified: 2021-06-01 08:15 UTC (History)
0 users

See Also:
Host:
Target: avr
Build:
Known to work:
Known to fail:
Last reconfirmed: 2019-12-13 00:00:00


Attachments
123.c: C test case. (413 bytes, text/plain)
2019-12-13 10:36 UTC, Georg-Johann Lay
Details
123f.c: C test case with address space __flash. (233 bytes, text/plain)
2019-12-13 10:51 UTC, Georg-Johann Lay
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Janis Hamme 2019-11-20 23:20:21 UTC
AVR GCC 9.2.0 has a critical bug with merging identical constant progmem and data section symbols to a single progmem symbol (.text). I discovered it while debugging my 3d printer firmware (print head kept crashing inexplicably).

Due to this invalid optimization, if data is read from the symbol originally defined in data space, only arbitrary data is read (from data space at the address of the symbol in program space). Below is a short example for Arduino.

The example was compiled with GCC options `-Os -g -ffunction-sections -fdata-sections -flto -Wl,--gc-section`. I'm not entirely sure, but the bug appears to be triggered by link time optimization (-flto) in combination with optimization levels -Os or higher.

-------------------
#include "Arduino.h"

static const PROGMEM float xyz_prog[] = { 123, 123, 123 };
float xyz[] = { 123, 123, 123 };

volatile int x = 0;

void setup() {
    Serial.begin(57600);
    Serial.print("X_prog: ");
    Serial.println(pgm_read_float_near(&xyz_prog[0]));
}

void loop() {
    Serial.print("X: ");
    Serial.println(xyz[x]);
}
-------------------

Expected output:

X_prog: 123
X: 123
X: 123
X: 123
...

Actual output (example):

X_prog: 123
X: -0.00
X: 0.00
X: 553676288.00
...

I've uploaded the example to https://github.com/xblax/avr_gcc_bug together with the Arduino core library and a cmake project for easy compilation.
Comment 1 Georg-Johann Lay 2019-12-13 10:36:30 UTC
Created attachment 47484 [details]
123.c: C test case.

Confirmed with the attached test case compiler with

$ avr-gcc -mmcu=atmega128 123.c -flto -Os -save-temps -o 123.elf

Then 123.elf.ltrans0.s reads:

xyz_prog:
	.byte	123
...
	.set	xyz,xyz_prog

Which is obviously wrong.  With -ffat-lto-objects (so that 123.s has asm code) it is correct: xyz and xyz_prog are distinct objects, the first in .data and the latter in .progmem.data.
Comment 2 Georg-Johann Lay 2019-12-13 10:41:05 UTC
Confirmed with v8, v9 and v10.

Set component to "other" for now, dunno if it's a missing target hook or a tree flaw.
Comment 3 Georg-Johann Lay 2019-12-13 10:51:48 UTC
Created attachment 47485 [details]
123f.c: C test case with address space __flash.

...and the code is also wrong with address spaces like __flash (and the same options like in comment #1):

$ avr-gcc -mmcu=atmega128 123f.c -flto -Os -save-temps -o 123f.elf
Comment 4 Georg-Johann Lay 2019-12-13 15:30:01 UTC
The problem is that there isn't even a target hook to disallow such optimizations, files as as PR92932.

In a respective hook, at least the attributes and address spaces of either object must be available.
Comment 5 Georg-Johann Lay 2019-12-16 12:55:15 UTC
For the time being, -fno-ipa-icf-variables might do as a work-around.
Comment 6 Richard Biener 2020-01-08 12:57:26 UTC
I wonder if the same applies to

static const __attribute__((section("foobar"))) float xyz_prog[] = { 123, 123, 123 };
float xyz[] = { 123, 123, 123 };

or whether the backend "forgets" to set DECL_SECTION_NAME or the like.
Similarly address-spaces need to be honored (I bet avr would have one -- is
PROGMEM actually an address-space implementation-wise?)

I see there's a 'progmem' target attribute but its effects might be hidden
completely from the middle-end.
Comment 7 Georg-Johann Lay 2020-01-09 11:41:40 UTC
(In reply to Richard Biener from comment #6)
> or whether the backend "forgets" to set DECL_SECTION_NAME or the like.

That caused problems (don't remember which ones exactly, maybe to make -fdata-sections work).  The section is set in TARGET_ASM_SELECT_SECTION so the middle-end knows the section (in principle).  But presumably the middle-end concept is broken, because telling what section belongs to a decl is not enough to convince the middle-end that the section belongs to the decl...  It's gcc after all.

(Very) old versions did set DECL_SECTION_NAME in insert_attributes, but that's no more the case.  Presumably to fix one of the 100s of FIXMEs in the avr backend.

> Similarly address-spaces need to be honored (I bet avr would have one -- is
> PROGMEM actually an address-space implementation-wise?)

No, progmem is just an attribute and dates back to the times when there was no address-spaces in gcc.  It's still supposed to be supported because old code should still work, and because g++ will never have address-spaces.

> I see there's a 'progmem' target attribute but its effects might be hidden
> completely from the middle-end.

That's why I opened PR92932. As mentioned in comment #3, the bug also appears for address-space __flash.
Comment 8 Georg-Johann Lay 2020-01-10 15:22:08 UTC
Giving up on this.

My work-around patch to disable the malicious data optimization for avr has effectively been rejected.

http://gcc.gnu.org/ml/gcc-patches/2020-01/msg00145.html

So we can enjoy code that saves some scrappy bytes at the expense that it might be incorrect...
Comment 9 Richard Biener 2020-01-17 11:34:19 UTC
x86 has __gs/fs address-spaces so a testcase that is broken there can be constructed as well I guess.

Martin?
Comment 10 Martin Liška 2020-02-06 11:16:27 UTC
(In reply to Richard Biener from comment #6)
> I wonder if the same applies to
> 
> static const __attribute__((section("foobar"))) float xyz_prog[] = { 123,
> 123, 123 };
> float xyz[] = { 123, 123, 123 };
> 
> or whether the backend "forgets" to set DECL_SECTION_NAME or the like.
> Similarly address-spaces need to be honored (I bet avr would have one -- is
> PROGMEM actually an address-space implementation-wise?)
> 

I've tried this one and it really ends as 2 different symbols in the corresponding sections.
Comment 11 Martin Liška 2020-02-06 11:18:54 UTC
(In reply to Richard Biener from comment #9)
> x86 has __gs/fs address-spaces so a testcase that is broken there can be
> constructed as well I guess.
> 
> Martin?

Can you please construct such a test-case?
Comment 12 Jakub Jelinek 2020-03-04 09:39:15 UTC
GCC 8.4.0 has been released, adjusting target milestone.
Comment 13 Jakub Jelinek 2021-05-14 09:52:20 UTC
GCC 8 branch is being closed.
Comment 14 Richard Biener 2021-06-01 08:15:34 UTC
GCC 9.4 is being released, retargeting bugs to GCC 9.5.