Message ID | 1419087669-2365-1-git-send-email-syntheticpp@gmx.net |
---|---|
State | Superseded |
Headers | show |
Dear Peter Kümmel, We reviewed your patch during the Buildroot meeting, here is some feedback. For such a non-trivial feature, having an empty commit log is a bit weird. Can you write down some more details in the commit log? On Sat, 20 Dec 2014 16:01:09 +0100, Peter Kümmel wrote: > diff --git a/package/binutils/Config.in b/package/binutils/Config.in > index 64d0a09..a5d2cc9 100644 > --- a/package/binutils/Config.in > +++ b/package/binutils/Config.in > @@ -19,6 +19,11 @@ config BR2_PACKAGE_BINUTILS_TARGET > > http://www.gnu.org/software/binutils/ > > +config BR2_PACKAGE_BINUTILS_ENABLE_LTO > + bool "Enable support for link-time-optimization" > + help > + Enable lto support. Allows passing a lto plugin to ar and ranlib. > + We believe this is not needed, as we don't support development on the target. Even though we have a target binutils package for binutils libraries (used by oprofile or other things), or because sometimes using objdump on the target might be useful, we generally don't support development on the target, so having the option of enabling LTO in the target binutils is not very useful. > +ifeq ($(BR2_PACKAGE_BINUTILS_ENABLE_LTO),y) > +BINUTILS_CONF_OPTS += --enable-plugins --enable-lto > +endif So this chunk would no longer be needed. > + > +ifeq ($(BR2_GCC_ENABLE_LTO),y) > +HOST_BINUTILS_CONF_OPTS += --enable-plugins --enable-lto > +endif This looks fine. > + > $(eval $(autotools-package)) > $(eval $(host-autotools-package)) > diff --git a/package/gcc/Config.in.host b/package/gcc/Config.in.host > index 0750807..4e0c2ae 100644 > --- a/package/gcc/Config.in.host > +++ b/package/gcc/Config.in.host > @@ -128,6 +128,12 @@ config BR2_GCC_ENABLE_TLS > Enable the compiler to generate code for accessing > thread local storage variables > > +config BR2_GCC_ENABLE_LTO > + bool "Enable compiler link-time-optimization support" > + help > + Since version 4.5 GCC supports lto. Build GCC with lto support enabled. > + Needed when -flto should be used. Since it's only supported since GCC 4.5, then maybe make it 'depends on !BR2_GCC_VERSION_4_2_2_AVR32_2_1_5'. > +ifeq ($(BR2_GCC_ENABLE_LTO),y) > +HOST_GCC_COMMON_CONF_OPTS += --enable-plugins --enable-lto > +endif Looks fine. > + > ifeq ($(BR2_GCC_ENABLE_LIBMUDFLAP),y) > HOST_GCC_COMMON_CONF_OPTS += --enable-libmudflap > else > diff --git a/toolchain/toolchain-external/toolchain-external.mk b/toolchain/toolchain-external/toolchain-external.mk > index b07b16c..8baf561 100644 > --- a/toolchain/toolchain-external/toolchain-external.mk > +++ b/toolchain/toolchain-external/toolchain-external.mk > @@ -643,6 +643,9 @@ define TOOLCHAIN_EXTERNAL_INSTALL_WRAPPER > for i in $(TOOLCHAIN_EXTERNAL_CROSS)*; do \ > base=$${i##*/}; \ > case "$$base" in \ > + *-ar|*-ranlib|*-nm) \ > + ln -sf $$(echo $$i | sed 's%^$(HOST_DIR)%../..%') .; \ > + ;; \ Please add a comment here in the code to explain why this is happening. During the review, we had to find a previous version of your patch with a more detailed commit log to understand what was going on. I'll mark your patch as 'Changes Requested' in patchwork, so please resend an updated version otherwise we will forget about it. Thanks a lot! Thomas
diff --git a/package/binutils/Config.in b/package/binutils/Config.in index 64d0a09..a5d2cc9 100644 --- a/package/binutils/Config.in +++ b/package/binutils/Config.in @@ -19,6 +19,11 @@ config BR2_PACKAGE_BINUTILS_TARGET http://www.gnu.org/software/binutils/ +config BR2_PACKAGE_BINUTILS_ENABLE_LTO + bool "Enable support for link-time-optimization" + help + Enable lto support. Allows passing a lto plugin to ar and ranlib. + endif comment "binutils needs a toolchain w/ wchar" diff --git a/package/binutils/binutils.mk b/package/binutils/binutils.mk index 9a9bb94..3916cad 100644 --- a/package/binutils/binutils.mk +++ b/package/binutils/binutils.mk @@ -96,5 +96,13 @@ BINUTILS_PRE_PATCH_HOOKS += BINUTILS_XTENSA_PRE_PATCH HOST_BINUTILS_PRE_PATCH_HOOKS += BINUTILS_XTENSA_PRE_PATCH endif +ifeq ($(BR2_PACKAGE_BINUTILS_ENABLE_LTO),y) +BINUTILS_CONF_OPTS += --enable-plugins --enable-lto +endif + +ifeq ($(BR2_GCC_ENABLE_LTO),y) +HOST_BINUTILS_CONF_OPTS += --enable-plugins --enable-lto +endif + $(eval $(autotools-package)) $(eval $(host-autotools-package)) diff --git a/package/gcc/Config.in.host b/package/gcc/Config.in.host index 0750807..4e0c2ae 100644 --- a/package/gcc/Config.in.host +++ b/package/gcc/Config.in.host @@ -128,6 +128,12 @@ config BR2_GCC_ENABLE_TLS Enable the compiler to generate code for accessing thread local storage variables +config BR2_GCC_ENABLE_LTO + bool "Enable compiler link-time-optimization support" + help + Since version 4.5 GCC supports lto. Build GCC with lto support enabled. + Needed when -flto should be used. + config BR2_GCC_ENABLE_OPENMP bool "Enable compiler OpenMP support" depends on !BR2_PTHREADS_NONE && !BR2_avr32 && !BR2_arc && !BR2_microblaze diff --git a/package/gcc/gcc.mk b/package/gcc/gcc.mk index cdd71aa..8eaa91d 100644 --- a/package/gcc/gcc.mk +++ b/package/gcc/gcc.mk @@ -127,6 +127,10 @@ else HOST_GCC_COMMON_CONF_OPTS += --disable-tls endif +ifeq ($(BR2_GCC_ENABLE_LTO),y) +HOST_GCC_COMMON_CONF_OPTS += --enable-plugins --enable-lto +endif + ifeq ($(BR2_GCC_ENABLE_LIBMUDFLAP),y) HOST_GCC_COMMON_CONF_OPTS += --enable-libmudflap else diff --git a/toolchain/toolchain-external/toolchain-external.mk b/toolchain/toolchain-external/toolchain-external.mk index b07b16c..8baf561 100644 --- a/toolchain/toolchain-external/toolchain-external.mk +++ b/toolchain/toolchain-external/toolchain-external.mk @@ -643,6 +643,9 @@ define TOOLCHAIN_EXTERNAL_INSTALL_WRAPPER for i in $(TOOLCHAIN_EXTERNAL_CROSS)*; do \ base=$${i##*/}; \ case "$$base" in \ + *-ar|*-ranlib|*-nm) \ + ln -sf $$(echo $$i | sed 's%^$(HOST_DIR)%../..%') .; \ + ;; \ *cc|*cc-*|*++|*++-*|*cpp) \ ln -sf ext-toolchain-wrapper $$base; \ ;; \
Signed-off-by: Peter Kümmel <syntheticpp@gmx.net> --- package/binutils/Config.in | 5 +++++ package/binutils/binutils.mk | 8 ++++++++ package/gcc/Config.in.host | 6 ++++++ package/gcc/gcc.mk | 4 ++++ toolchain/toolchain-external/toolchain-external.mk | 3 +++ 5 files changed, 26 insertions(+)