token-vesting icon indicating copy to clipboard operation
token-vesting copied to clipboard

8k flash 1-series parts, when set to use an external clock, CI (only) fails to compile the barometric pressure example

Open SpenceKonde opened this issue 2 years ago • 10 comments

It's a PCREL error (translation: two pieces of code that need to be the ends of the jump are more than 8k apart, in other words the binary is too big. But compiling for larger or smaller parts gives a 5.5k size. dafuk?

SpenceKonde avatar Sep 19 '22 23:09 SpenceKonde

Does not reproduce on actual system.....

SpenceKonde avatar Sep 21 '22 05:09 SpenceKonde

Can anyone reproduce this failure other than the CI script? :-(

SpenceKonde avatar Sep 21 '22 06:09 SpenceKonde

BarometricPressureSensor compiles fine for me with target set to 816 with external 20MHz clock:

Sketch uses 5590 bytes (68%) of program storage space. Maximum is 8192 bytes.
Global variables use 100 bytes (19%) of dynamic memory, leaving 412 bytes for local variables. Maximum is 512 bytes.

Arduino 1.8.19 megaTinyCore 2.5.11 Windows

grandaspanna avatar Sep 21 '22 06:09 grandaspanna

I was able to reproduce this, and suspect it may have something to do with the -mrelax flag.

By changing the CI script to have verbose: true, I was able to determine the exact flags used during compilation. I then altered my custom Makefile to use (mostly) the same flags:

DEFINES = -DF_CPU=$(F_CPU) -DCLOCK_SOURCE=0 -DCLOCK_SOURCE=2 -DTWI_MORS -DMILLIS_USE_TIMERD0 -DCORE_ATTACH_OLD -DARDUINO=10607 -DARDUINO_ARCH_MEGAAVR
CPPFLAGS = -Wall -Werror=vla -g -Os -std=gnu++17 -fpermissive -Wno-sized-deallocation -fno-exceptions -ffunction-sections -fdata-sections -fno-threadsafe-statics -MMD -flto -mrelax -mmcu=$(MMCU) $(DEFINES)
CFLAGS = -Wall -Werror=vla -g -Os -std=gnu11 -ffunction-sections -fdata-sections -MMD -flto -fno-fat-lto-objects -mrelax -mmcu=$(MMCU) $(DEFINES)
ASSEMBLERFLAGS = -g -x assembler-with-cpp -flto -MMD -mmcu=$(MMCU) $(DEFINES)
LDFLAGS = -Wall -g -Os -flto -fuse-linker-plugin -Wl,--gc-sections -Wl,--section-start=.text=0x0 -mrelax -mmcu=$(MMCU)

Building for attiny816 results in:

/Users/jvasileff/lib/avr-azduino6/bin/../lib/gcc/avr/7.3.0/avrxmega3/short-calls/libgcc.a(_ctors.o): In function `__do_global_ctors':
/home/admin/Arduino/toolchain-avr-special/gcc-build/avr/avrxmega3/short-calls/libgcc/../../../../../gcc/libgcc/config/avr/lib1funcs.S:2481:(.init6+0xc): relocation truncated to fit: R_AVR_13_PCREL against symbol `__tablejump2__' defined in .text.libgcc section in /Users/jvasileff/lib/avr-azduino6/bin/../lib/gcc/avr/7.3.0/avrxmega3/short-calls/libgcc.a(_tablejump2.o)
collect2: error: ld returned 1 exit status
make: *** [build/attiny-scratch.elf] Error 1

Now... why do I suspect -mrelax?

In my work to build alternate toolchains, I noticed that GCC 8.5.0 builds fail with:

/Users/jvasileff/Library/Arduino15/packages/MANUAL_INSTALL/tools/avr-gcc/7.3.0-atmel3.6.1-azduino4b/bin/../lib/gcc/avr/8.5.0/../../../../avr/bin/ld: --relax and -r may not be used together
collect2: error: ld returned 1 exit status

From what little I understand --relax from quick Google research, it seems like the combo mentioned in the error above may cause strange errors like the one in this issue.

I tried three additional compiles with my Makefile:

  1. Build using a GCC 11.3.0 toolchain, which doesn't complain about "--relax and -r" (perhaps they made changes to avoid the problem?) - compilation succeeds using the same flags that fail using the azduino6 toolchain.
  2. Build using azduino6 without -mrelax in LDFLAGS works.
  3. Duplicating Serial.print(realTemp); in the sketch 10 times, with the goal of shifting addresses around, works.

In summary, particularly with 3 in mind, the error appears to be a very randomly appearing bug in the toolchain, and therefore make me suspect the bug is that the version of gcc incorrectly allows --relax and -r which newer versions of gcc addresses.

jvasileff avatar Sep 21 '22 14:09 jvasileff

But where is -r and -mrelax being used together? o_o am I just blind?

Search of platform.txt for "-r " shows no matches. I'm also not sure if mrelax even does anything on parts with 8k or less flash. does prevent flash size from spiking when you exceed that though (it tells the linker to replace 2-word 3 cycle jmps with 1 word 2 cycle rjmps.

SpenceKonde avatar Sep 21 '22 19:09 SpenceKonde

I think it is likely significant that the issue occurs on 8k chips, (and apparently only there). 8k chips do not have jmp, only rjmp, because with 4096 instruction words, the -2048-+2047 word jump provided by rjmp precisely matches the maximum flash size.

SpenceKonde avatar Sep 21 '22 19:09 SpenceKonde

Boosting priority, as this the evidence you present implies a serious compiler configuration defect.

SpenceKonde avatar Sep 21 '22 19:09 SpenceKonde

One thing that I note about that sketch is that by deleting three characters and adding under a dozen, you can shrink the compiles size of that example by a considerable amount - but we don't have a switch that makes the compiler reach out of the computer to hit people with a baseball bat when the try to concatenate Strings. It's line 94.

Original, 5354b: Serial.println("\tPressure [Pa]="+String(pressure));

Changing that to this saves a whopping 1596 bytes of flash! all from malloc and fee. 3758b Serial.print("\tPressure [Pa]="); Serial.println(pressure);

Obviously this is not the cause of the problem, but my god that example is a piece of shit code.

It also leftshifts a 16-bit integer 16 places before casting it to a 32-bit one eyeroll

SpenceKonde avatar Sep 21 '22 19:09 SpenceKonde

But where is -r and -mrelax being used together? o_o am I just blind?

Yeah, no idea.

I'm also not sure if mrelax even does anything on parts with 8k or less flash.

Well, it does do something. On my test number 3, I can successfully compile with and without -mrelax. The following:

        @echo "Program Space: $$($(AVRSIZE) -A $@ | grep -e '\.text\|\.data\|\.rodata\|\.bootloader' | awk '{s+=$$2} END {print s}') bytes," \
              "Dynamic Memory: $$($(AVRSIZE) -A $@ | grep -e '\.data\|\.bss\|\.noinit' | awk '{s+=$$2} END {print s}') bytes"; \

with the arg, reports:

Program Space: 5508 bytes, Dynamic Memory: 125 bytes

and without:

Program Space: 5580 bytes, Dynamic Memory: 125 bytes

jvasileff avatar Sep 21 '22 20:09 jvasileff

Hmmm, if only there was a way to automatically test that for a bunch of sketches over all sub-8k processors...... Oh, right...

(the close and reopen was github getting ahead of itself because I mentioned the issue in the commit. But let's wait for the CI to run before we pronounce it fixed.

SpenceKonde avatar Sep 22 '22 03:09 SpenceKonde

Closed.

This compiler bug no longer manifests if -mrelax is not used (nor does it appear to manifest on parts with more than 8k of flash) I suspect this was from the linker being confused by the unified address space and failing to "wrap around" an rjmp to the other end of the flash. Since -mrelax does not have any utility on parts with less than 16k of flash (on parts with 16k or more of flash, it can provide substantial flash savings), boards.txt has been amended to not use -mrelax unless the part has more than 8k of flash.

SpenceKonde avatar Sep 24 '22 11:09 SpenceKonde