added support for compiling source with -flto option disabled. #7089

mtei · 2019-10-19T18:39:17Z

Description

Some source code (eg helix-serial.c) is sensitive to execution timing and will not work as expected when compiled with the -flto option.

This commit adds the ability to specify the -fno-lto option for specific source code in tmk_core/rules.mk.

Usage:

file: keybaord, keymap, others rules.mk

SRC += timing_sensitive_code.c

change to

SRC += timing_sensitive_code.c/NO-LTO

~~This commit does not change the build results.~~

Postscript

Currently there are keyboards that use LIB_SRC or QUANTUM_LIB_SRC to specify -fno-lto. But this is not correct.

LIB_SRC and QUANTUM_LIB_SRC are ways to specify that objects are stored in their respective libraries.
LIB_SRC and QUANTUM_LIB_SRC ARE NOT a way to specify -fno-lto for compilation.

Currently, the file specified in LIB_SRC and QUANTUM_LIB_SRC is compiled with -fno-lto, but this is only a workaround for the 'ar' command not supporting lto object. If the 'ar' command supports lto objects, this action is gone.

With this PR change, LIB_SRC, QUANTUM_LIB_SRC no longer means adding -fno-lto.

Types of Changes

Checklist

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
I have tested the changes and verified that they work and don't break anything (as well as I can manage).

Some source code (eg helix-serial.c) is sensitive to execution timing and will not work as expected when compiled with the -flto option. This commit adds the ability to specify the -fno-lto option for specific source code in tmk_core/rules.mk. Usage: file: keybaord,keymap,others rules.mk ``` SRC += timing_sensitive_code.c ``` change to ``` SRC += timing_sensitive_code.c/NO-LTO ``` There is no change in the build result.

When LINK_TIME_OPTIMIZATION_ENABLE=no, there is no change in the build result.

There is no change in the build result.

…ier versions

zvecr · 2019-10-20T15:05:08Z

@mtei as we currently use QUANTUM_LIB_SRC to avoid the issues you have highlighted, can you give some insight to the value add of this PR. It would be good to have that context for reviewing the changes.

mtei · 2019-10-20T16:34:27Z

Of course, I know QUANTUM_LIB_SRC and LIB_SRC. Because it was my contribution. (#4522)

LIB_SRC is for automatically selecting whether a link is required. Certainly, the source specified by LIB_SRC is compiled with -fno-lto. But that's just a side effect. It is not the main purpose of LIB_SRC.

I believe there must be a way to explicitly specify compiling with -fno-lto.

For example, when LINK_TIME_OPTIMIZATION_ENABLE=no, the following two statements have exactly the same result. So if you are sure that it already works, you can safely add /NO-LTO.

SRC += a.c b.c c.c

SRC += a.c b.c/NO-LTO c.c

drashna · 2019-10-20T19:15:54Z

LIB_SRC is for automatically selecting whether a link is required. Certainly, the source specified by LIB_SRC is compiled with -fno-lto. But that's just a side effect. It is not the main purpose of LIB_SRC.

I believe there must be a way to explicitly specify compiling with -fno-lto.

Is there a specific/technical reason that LIB_SRC won't work for this?

I mean, if LIB_SRC is doing the same thing, and some extra stuff that doesn't cause issues, I don't see why we can't use that over adding another setting here. Because adding more options like this feels like it may be adding complexity for edge cases where other options would not only work, but work well. Which will add confusion down the line.

And to clarify, I'm not trying to be mean, shoot the contribution down, or anything like that. I want to understand why, since I don't have a clear understanding of some of this, and would like to be able to understand what is going on, so I can explain it to others, myself (if/when the need arises).

mtei · 2019-10-20T20:36:30Z

Currently, the source specified by LIB_SRC is compiled with -fno-lto because 'tmk_core/rules.mk#L37' forces it.

qmk_firmware/tmk_core/rules.mk

Line 37 in f64d9b0

NO_LTO_OBJ := $(filter %.a,$(OBJ))

If we comment out this, it will look like this:

{qmk_firmware} % make LINK_TIME_OPTIMIZATION_ENABLE=yes  lets_split:default
QMK Firmware 0.7.40
Making lets_split/rev2 with keymap default

avr-gcc (GCC) 8.3.0
Copyright (C) 2018 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

Compiling: keyboards/lets_split/lets_split.c                                [OK]
......
Compiling: quantum/split_common/serial.c                                    [OK]
Archiving: .build/obj_lets_split_rev2_default/quantum/split_common/serial.o [WARNINGS]
 | 
 | avr-ar: .build/obj_lets_split_rev2_default/quantum/split_common/serial.o: plugin needed to handle lto object
 | 
Compiling: drivers/avr/i2c_master.c                                         [OK]
Archiving: .build/obj_lets_split_rev2_default/i2c_master.o                  [WARNINGS]
 | 
 | avr-ar: .build/obj_lets_split_rev2_default/i2c_master.o: plugin needed to handle lto object
 | 
Compiling: drivers/avr/i2c_slave.c                                          [OK]
Archiving: .build/obj_lets_split_rev2_default/i2c_slave.o                   [WARNINGS]
 | 
 | avr-ar: .build/obj_lets_split_rev2_default/i2c_slave.o: plugin needed to handle lto object
 | 
Compiling: tmk_core/common/host.c                                           [OK]
......
Linking: .build/lets_split_rev2_default.elf              [ERRORS]

As you can see in the message above, avr-ar does not currently support lto objects.

In the future, if avr-ar will support lto objects as standard, it will not be necessary to add -fno-lto in 'tmk_core/rules.mk'.
In other words, using LIB_SRC does not guarantee to use -fno-lto.

This is why we need a way to explicitly specify -fno-lto.

There is no change in the build result.

When LINK_TIME_OPTIMIZATION_ENABLE=no, there is no change in the build result.

…-lto" This reverts commit 19dc347.

drashna · 2019-11-03T22:57:31Z

Okay, to see if I understand this correctly...

The LIB_SRC and QUANTUM_LIB_SRC, add the files after SRC and QUANTUM_SRC (I think). Additionally, if you have the same file added multiple times to the LIB version, it parses that correctly, so it doesn't generate a warning about multiple inclusions.

This makes the compilation cleaner, and more intelligent.

RIght now, those also assume no lto, but that's more of an unintentional side effect, rather than intended functionality.

Ideally, stuff like the i2c_master.c should be called by using LIB_SRC += i2c_master.c/NO-LTO.

If that's correct, awesome, and I think this would be a good change. I was just having a hard time understanding why this was a good change, or why it was needed.

Thank you for taking the time and effort to explain this!

mtei · 2019-11-04T11:02:54Z

I'm glad you understood.

Okay, to see if I understand this correctly...

The LIB_SRC and QUANTUM_LIB_SRC, add the files after SRC and QUANTUM_SRC (I think). Additionally, if you have the same file added multiple times to the LIB version, it parses that correctly, so it doesn't generate a warning about multiple inclusions.

That is correct.

In addition, in the following example, lib_d.a is linked if any of a.o, b.o or c.o calls a function in lib_d.a. Otherwise, lib_d.a will not be linked.

# file: rules.mk
LIB_SRC += lib_d.c
SRC += a.c
SRC += b.c
SRC += c.c

The link order is as follows.

...  a.o b.o c.o  ...  lib_d.a  ...

mtei · 2019-11-04T11:16:34Z

Ideally, stuff like the i2c_master.c should be called by using LIB_SRC += i2c_master.c/NO-LTO.

/NO-LTO can be specified for both SRC and LIB_SRC files, as shown in the following example.

common_features.mk#L387-L389

      QUANTUM_LIB_SRC += $(QUANTUM_DIR)/split_common/serial.c/NO-LTO \
                         i2c_master.c \
                         i2c_slave.c

keyboards/helix/rev2/rules.mk#L3-L6

SRC += local_drivers/i2c.c
SRC += local_drivers/serial.c/NO-LTO
SRC += local_drivers/ssd1306.c
KEYBOARD_PATHS += $(HELIX_TOP_DIR)/local_drivers

more detail see https://gcc.gnu.org/onlinedocs/gcc/Optimize-Options.html#index-ffat-lto-objects

This reverts commit 75da167.

mtei · 2019-11-05T16:10:26Z

Changed to use gcc-ar instead of ar command for library creation. Objects compiled with LTO enabled can now be stored in a library. As a result, the workaround in tmk_core/rules.mk is no longer needed and has been removed.

Currently, LIB_SRC and QUANTUM_LIB_SRC no longer mean adding -fno-lto.

https://gcc.gnu.org/onlinedocs/gcc/Optimize-Options.html#index-ffat-lto-objects

noroadsleft · 2020-04-28T00:12:19Z

This has merge conflicts.

drashna · 2020-04-28T06:12:45Z

Now that I've looked at this closer

With this PR change, LIB_SRC, QUANTUM_LIB_SRC no longer means adding -fno-lto.

If this is the case, then the places where QUANTUM_LIB_SRC += i2c_master.c is called should be moved.

But it sounds like we don't have a LIB option, for when multiple things include the same file (i2c_master.c namely). If that's the case, it will generate warnings for anything that may trigger this (such as using OLEDs and rgb matrix with an ISSI controller, or the ergodox ez glow).

If that's the case, that may be a blocker to getting this merged

mtei · 2020-04-28T06:55:41Z

I'd like to break this PR down into a few PRs. This should make it easier to verify each one.

I'm going to do it when I have more time.

drashna · 2020-04-29T11:14:53Z

Sounds good! Smaller PRs are easier to review, especially for how complex MAKE stuff is!

Erovia · 2020-06-20T22:11:33Z

@mtei, would you like to leave this PR open or can it be closed?

mtei · 2020-07-04T18:36:03Z

@mtei, would you like to leave this PR open or can it be closed?

Now I'm getting ready to split up this PR and reopen a new one.

Hold on a moment.

tzarc · 2020-11-10T04:47:58Z

Converted to draft so that someone doesn't decide to have a bright idea and attempt to merge.

mtei added 5 commits October 20, 2019 03:10

keyboards/helix/{rev1|rev2|pico} serial.c compile with -fno-lto

d433b65

When LINK_TIME_OPTIMIZATION_ENABLE=no, there is no change in the build result.

keyboards/fortitude60/serial.c compile with -fno-lto

9c5232f

When LINK_TIME_OPTIMIZATION_ENABLE=no, there is no change in the build result.

Changed LIB_SRC and QUANTUM_LIB_SRC to allow /NO-LTO.

3fea2ae

There is no change in the build result.

Reduce helix/pico/back:default size for compiling with gcc 5 and earl…

6bf75f5

…ier versions

drashna requested review from a team October 20, 2019 19:11

drashna added core enhancement labels Oct 20, 2019

mtei added 17 commits October 21, 2019 20:58

Improved /NO-LTO handling for LIB_SRC and QUANTUM_LIB_SRC

ec95f28

There is no change in the build result.

comment update in tmk_core/rules.mk

968fcd9

keyboards/ai03/orbit/serial.c compile with -fno-lto

d055a61

When LINK_TIME_OPTIMIZATION_ENABLE=no, there is no change in the build result.

keyboards/claw44/serial.c compile with -fno-lto

1eed939

When LINK_TIME_OPTIMIZATION_ENABLE=no, there is no change in the build result.

keyboards/crkbd/serial.c compile with -fno-lto

8bb66fa

When LINK_TIME_OPTIMIZATION_ENABLE=no, there is no change in the build result.

keyboards/deltasplit75/serial.c compile with -fno-lto

5f6586d

When LINK_TIME_OPTIMIZATION_ENABLE=no, there is no change in the build result.

keyboards/ergo42/serial.c compile with -fno-lto

d1e166f

When LINK_TIME_OPTIMIZATION_ENABLE=no, there is no change in the build result.

keyboards/ergoinu/serial.c compile with -fno-lto

c7c6ab6

When LINK_TIME_OPTIMIZATION_ENABLE=no, there is no change in the build result.

keyboards/handwired/not_so_minidox/serial.c compile with -fno-lto

19dc347

When LINK_TIME_OPTIMIZATION_ENABLE=no, there is no change in the build result.

keyboards/lily58/serial.c compile with -fno-lto

60633b5

When LINK_TIME_OPTIMIZATION_ENABLE=no, there is no change in the build result.

keyboards/minidox/serial.c compile with -fno-lto

72ea461

When LINK_TIME_OPTIMIZATION_ENABLE=no, there is no change in the build result.

keyboards/mint60/serial.c compile with -fno-lto

2bd03f0

When LINK_TIME_OPTIMIZATION_ENABLE=no, there is no change in the build result.

keyboards/naked60/serial.c compile with -fno-lto

0f10d56

When LINK_TIME_OPTIMIZATION_ENABLE=no, there is no change in the build result.

keyboards/treadstone48/serial.c compile with -fno-lto

ece786c

When LINK_TIME_OPTIMIZATION_ENABLE=no, there is no change in the build result.

keyboards/yosino58/serial.c compile with -fno-lto

67d07f3

When LINK_TIME_OPTIMIZATION_ENABLE=no, there is no change in the build result.

keyboards/zinc/serial.c compile with -fno-lto

19dae64

When LINK_TIME_OPTIMIZATION_ENABLE=no, there is no change in the build result.

Revert "keyboards/handwired/not_so_minidox/serial.c compile with -fno…

0545539

…-lto" This reverts commit 19dc347.

mtei added 4 commits October 24, 2019 06:49

Merge tag '0.7.44' into no_lto_compile_option

e76e6f9

Added description of /NO-LTO to config_options.md.

ec6e619

Merge commit 'e96cac081' into no_lto_compile_option

01b72ee

Merge commit 'b47c10bf6' into no_lto_compile_option

6f69add

mtei added 5 commits November 5, 2019 18:24

Merge tag '0.7.59' into no_lto_compile_option

3b40c92

use xxx-gcc-ar instead of xxx-ar when LINK_TIME_OPTIMIZATION_ENABLE=yes

d2ddab9

more detail see https://gcc.gnu.org/onlinedocs/gcc/Optimize-Options.html#index-ffat-lto-objects

remove workaround for ar command when LTO enable

020a29e

add temporary test shell-spript

75da167

test end remove test script. Revert "add temporary test shell-spript"

7511cb8

This reverts commit 75da167.

drashna added the breaking_change Changes that need to wait for a version increment label Nov 9, 2019

drashna approved these changes Nov 9, 2019

View reviewed changes

mtei mentioned this pull request Dec 7, 2019

Added workaround for serial.c issue when LTO_ENABLE=yes for the Helix. #7558

Merged

13 tasks

noroadsleft added the future_2019q3 label Jan 10, 2020

noroadsleft removed the future_2020q1 label Feb 2, 2020

drashna self-requested a review April 28, 2020 06:12

drashna requested a review from a team April 28, 2020 06:12

noroadsleft added the on hold label May 4, 2020

tzarc marked this pull request as draft November 10, 2020 04:47

tzarc closed this Oct 28, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added support for compiling source with -flto option disabled. #7089

added support for compiling source with -flto option disabled. #7089

mtei commented Oct 19, 2019 •

edited

Loading

zvecr commented Oct 20, 2019

mtei commented Oct 20, 2019 •

edited

Loading

drashna commented Oct 20, 2019 •

edited

Loading

mtei commented Oct 20, 2019 •

edited

Loading

drashna commented Nov 3, 2019

mtei commented Nov 4, 2019

mtei commented Nov 4, 2019

mtei commented Nov 5, 2019

noroadsleft commented Apr 28, 2020

drashna commented Apr 28, 2020

mtei commented Apr 28, 2020

drashna commented Apr 29, 2020

Erovia commented Jun 20, 2020

mtei commented Jul 4, 2020

tzarc commented Nov 10, 2020

added support for compiling source with -flto option disabled. #7089

added support for compiling source with -flto option disabled. #7089

Conversation

mtei commented Oct 19, 2019 • edited Loading

Description

Usage:

Postscript

Types of Changes

Checklist

zvecr commented Oct 20, 2019

mtei commented Oct 20, 2019 • edited Loading

drashna commented Oct 20, 2019 • edited Loading

mtei commented Oct 20, 2019 • edited Loading

drashna commented Nov 3, 2019

mtei commented Nov 4, 2019

mtei commented Nov 4, 2019

mtei commented Nov 5, 2019

noroadsleft commented Apr 28, 2020

drashna commented Apr 28, 2020

mtei commented Apr 28, 2020

drashna commented Apr 29, 2020

Erovia commented Jun 20, 2020

mtei commented Jul 4, 2020

tzarc commented Nov 10, 2020

mtei commented Oct 19, 2019 •

edited

Loading

mtei commented Oct 20, 2019 •

edited

Loading

drashna commented Oct 20, 2019 •

edited

Loading

mtei commented Oct 20, 2019 •

edited

Loading