search

arduino / arduino-cli

Arduino command line tool
https://arduino.github.io/arduino-cli/latest/
GNU General Public License v3.0
4.31k stars 373 forks source link

Builder: Phase out `--relax` option adding #639

Closed matthijskooijman closed 4 years ago

matthijskooijman commented 4 years ago

When building a sketch, if the MCU is set to atmega2560, ,--relax is added to the linker commandline:

https://github.com/arduino/arduino-cli/blob/322bc6a59248e63e3ff331e8eda6e72d384a1d6e/legacy/builder/phases/linker.go#L80-L85

This is something that really should be taken care of in the platform.txt file, not in the builder, since the current code assumes to much about the platform, such as that you're actually using gcc, and that the last option on the link commandline is actually a -Wl option (otherwise things break).

This seems to stem from way back when the Arduino IDE still generated commandlines explicitly: https://github.com/arduino/Arduino/commit/fa4ab4f6ab07117dd95ad284b2b9afff5c81c376

However, it seems that this option can be just be added to the commandline unconditionally, it is potentially helpful for any board with > 8K flash. Maybe this was different for the compiler versions used in 2011 when this was added, but I actually suspect this option was just misunderstood.

I just tried adding the option on a Uno board (which saves a few bytes on the empty sketch) and an Arduino NG with atmega8, which also accepts the option but as expected it does not make a difference.

I'll provide a PR for the AVR core in a minute.

Once that is done, I think this code should be removed from arduino-cli. How shall we approach this? I can imagine adding a check to see if the option is present in platform.txt and if not show a warning to notify core authors to update their core, except that we do not really have any machinery to toggle such warnings meaningfully (so it might just confuse users).

Since it is just an optimization (without it, some calls might end up 2 bytes longer), we could also consider just removing this?

matthijskooijman commented 4 years ago

Pullrequest for ArduinoCore-AVR is here: https://github.com/arduino/ArduinoCore-avr/pull/327

facchinm commented 4 years ago

Any concern in merging this? It would also incidentally solve https://github.com/arduino/ArduinoCore-avr/issues/339#issuecomment-625602068

matthijskooijman commented 4 years ago

It would indeed "solve" that, at least until https://github.com/arduino/ArduinoCore-avr/pull/327 is merged.

facchinm commented 4 years ago

Correct, but before merging arduino/ArduinoCore-avr#327 we should check if it's still an hard requirement with latest toolchains

cmaglie commented 4 years ago

Since it is just an optimization (without it, some calls might end up 2 bytes longer), we could also consider just removing this?

If removing it doesn't break existing cores why not, do you care pushing a PR for this?

matthijskooijman commented 4 years ago

If removing it doesn't break existing cores why not, do you care pushing a PR for this?

I think it does not break anything (only increases flash space for function calls). I can make a PR for this, but might take a while until I get around to that.