Builder: Phase out --relax option adding

[matthijskooijman]

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

arduino-cli/legacy/builder/phases/linker.go

Lines 80 to 85 in 322bc6a

	func addRelaxTrickIfATMEGA2560(buildProperties *properties.Map) string {
	if buildProperties.Get(constants.BUILD_PROPERTIES_BUILD_MCU) == "atmega2560" {
	return ",--relax"
	}
	return constants.EMPTY_STRING
	}

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: arduino/Arduino@fa4ab4f

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]

Pullrequest for ArduinoCore-AVR is here: arduino/ArduinoCore-avr#327

[facchinm]

Any concern in merging this? It would also incidentally solve arduino/ArduinoCore-avr#339 (comment)

[matthijskooijman]

It would indeed "solve" that, at least until arduino/ArduinoCore-avr#327 is merged.

[facchinm]

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

[cmaglie]

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]

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.