| Message ID | 20240507053320.1566814-7-raj.khem@gmail.com |
|---|---|
| State | Accepted, archived |
| Commit | 584940c2f3234bfaf579429e162e683934e86538 |
| Headers | show |
| Series | [v2,01/11] rng-tools: ignore incompatible-pointer-types errors for now | expand |
On Tue, 7 May 2024 at 07:33, Khem Raj via lists.openembedded.org
<raj.khem=gmail.com@lists.openembedded.org> wrote:
> +CXXFLAGS:remove = "-fvisibility-inlines-hidden"
Same comment as v1: this seems like the wrong way around. Why do we
have it as a global glibc/musl setting in the first place? Shouldn't
we just remove it from there? I did some digging and this line was
imported from oe-classic in 2010 with eglibc definition, so most
likely whatever reasons there used to be are long obsolete.
Alex
On Tue, May 7, 2024 at 3:15 AM Alexander Kanavin <alex.kanavin@gmail.com> wrote: > > On Tue, 7 May 2024 at 07:33, Khem Raj via lists.openembedded.org > <raj.khem=gmail.com@lists.openembedded.org> wrote: > > > +CXXFLAGS:remove = "-fvisibility-inlines-hidden" > > Same comment as v1: this seems like the wrong way around. Why do we > have it as a global glibc/musl setting in the first place? Shouldn't > we just remove it from there? I did some digging and this line was > imported from oe-classic in 2010 with eglibc definition, so most > likely whatever reasons there used to be are long obsolete. Firstly I am inclined towards removing it if we can, since I think it should be packages to decide to use it, then they can maintain it better from testing POV, However, there is something to consider w.r.t. to this option. This does help in optimizing loading DSOs by reducing the number of PLTs and improves loading time for DSOs which might be quite meaningful for embedded devices that we target. However, we only enable hidden visibility only for inlines so it won't be full benefits but we do have better default compatibility. Since we have had this option for a long time and it still seems relevant, I would think it would make sense to do some benchmarking on a sizeable C++ program perhaps chromium or webkit, and see the difference in DSOs sizes with and without this option and also benchmark the loading time of the resulting chromium package. maybe we will find that the gains are not as significant, in that case we might drop it from defaults. > > Alex
On Tue, 7 May 2024 at 18:13, Khem Raj <raj.khem@gmail.com> wrote: > Firstly I am inclined towards removing it if we can, since I think it should be > packages to decide to use it, then they can maintain it better from testing POV, > However, there is something > to consider w.r.t. to this option. This does help in optimizing loading DSOs > by reducing the number of PLTs and improves loading time for DSOs which might > be quite meaningful for embedded devices that we target. However, we only > enable hidden visibility only for inlines so it won't be full benefits but we do > have better default compatibility. > > Since we have had this option for a long time and it still seems > relevant, I would > think it would make sense to do some benchmarking on a sizeable C++ program > perhaps chromium or webkit, and see the difference in DSOs sizes with > and without > this option and also benchmark the loading time of the resulting > chromium package. > > maybe we will find that the gains are not as significant, in that case > we might drop > it from defaults. I digged deeper and this is where I think it originated: https://git.openembedded.org/openembedded/commit/?id=ddd6d1fd26416a3766d754eeda4237a680a65520 Ah the simpler times of oe-classic, when one could bundle four unrelated changes into one commit and explain none of them. Benchmarking I have no time for right now, just wanted to point out that adding the flag was never justified in commit log. Can you experiment with webkit? Alex
On Tue, May 7, 2024 at 12:56 PM Alexander Kanavin <alex.kanavin@gmail.com> wrote: > > On Tue, 7 May 2024 at 18:13, Khem Raj <raj.khem@gmail.com> wrote: > > Firstly I am inclined towards removing it if we can, since I think it should be > > packages to decide to use it, then they can maintain it better from testing POV, > > However, there is something > > to consider w.r.t. to this option. This does help in optimizing loading DSOs > > by reducing the number of PLTs and improves loading time for DSOs which might > > be quite meaningful for embedded devices that we target. However, we only > > enable hidden visibility only for inlines so it won't be full benefits but we do > > have better default compatibility. > > > > Since we have had this option for a long time and it still seems > > relevant, I would > > think it would make sense to do some benchmarking on a sizeable C++ program > > perhaps chromium or webkit, and see the difference in DSOs sizes with > > and without > > this option and also benchmark the loading time of the resulting > > chromium package. > > > > maybe we will find that the gains are not as significant, in that case > > we might drop > > it from defaults. > > I digged deeper and this is where I think it originated: > https://git.openembedded.org/openembedded/commit/?id=ddd6d1fd26416a3766d754eeda4237a680a65520 > > Ah the simpler times of oe-classic, when one could bundle four > unrelated changes into one commit and explain none of them. > > Benchmarking I have no time for right now, just wanted to point out > that adding the flag was never justified in commit log. Can you > experiment with webkit? > Yes, I have added it to my backlog. > Alex
On 2024-05-07 3:56 p.m., Alexander Kanavin via lists.openembedded.org wrote: > On Tue, 7 May 2024 at 18:13, Khem Raj<raj.khem@gmail.com> wrote: >> Firstly I am inclined towards removing it if we can, since I think it should be >> packages to decide to use it, then they can maintain it better from testing POV, >> However, there is something >> to consider w.r.t. to this option. This does help in optimizing loading DSOs >> by reducing the number of PLTs and improves loading time for DSOs which might >> be quite meaningful for embedded devices that we target. However, we only >> enable hidden visibility only for inlines so it won't be full benefits but we do >> have better default compatibility. >> >> Since we have had this option for a long time and it still seems >> relevant, I would >> think it would make sense to do some benchmarking on a sizeable C++ program >> perhaps chromium or webkit, and see the difference in DSOs sizes with >> and without >> this option and also benchmark the loading time of the resulting >> chromium package. >> >> maybe we will find that the gains are not as significant, in that case >> we might drop >> it from defaults. > I digged deeper and this is where I think it originated: > https://git.openembedded.org/openembedded/commit/?id=ddd6d1fd26416a3766d754eeda4237a680a65520 > > Ah the simpler times of oe-classic, when one could bundle four > unrelated changes into one commit and explain none of them. > > Benchmarking I have no time for right now, just wanted to point out > that adding the flag was never justified in commit log. Can you > experiment with webkit? I tried to build chromium with and without this flag but ran into build problems - sigh. I pulled out the next most-bloated single executable that I could think of: nodejs The bottom line is that the flag reduces the nodejs executable size by ~6%. Actual sizes: >>> 37333922 - 35245092 2088830 so 2 MB out of 37 MB ! Chromium has been updated over the weekend so I'll try that next. What else would people like to see to assess the utility of this compiler option? ../Randy Original build: ❯ size ../chromium-poky-m-aab1335523/tmp/work/core2-64-poky-linux/nodejs/20.12.2/packages-split/nodejs/usr/bin/node text data bss dec hex filename 35245092 628548 154792 36028432 225c010 ../chromium-poky-m-aab1335523/tmp/work/core2-64-poky-linux/nodejs/20.12.2/packages-split/nodejs/usr/bin/node With this patch: ❯ git diff diff --git a/meta-oe/recipes-devtools/nodejs/nodejs_20.12.2.bb b/meta-oe/recipes-devtools/nodejs/nodejs_20.12.2.bb index d86c38f2f..50cff533f 100644 --- a/meta-oe/recipes-devtools/nodejs/nodejs_20.12.2.bb +++ b/meta-oe/recipes-devtools/nodejs/nodejs_20.12.2.bb @@ -42,6 +42,8 @@ S = "${WORKDIR}/node-v${PV}" CVE_PRODUCT += "node.js" +CXXFLAGS:remove = "-fvisibility-inlines-hidden" + # v8 errors out if you have set CCACHE CCACHE = "" ❯ size tmp/work/core2-64-poky-linux/nodejs/20.12.2/packages-split/nodejs/usr/bin/node text data bss dec hex filename 37333922 628548 154792 38117262 2459f8e tmp/work/core2-64-poky-linux/nodejs/20.12.2/packages-split/nodejs/usr/bin/node ❯ python3 Python 3.10.12 (main, Nov 20 2023, 15:14:05) [GCC 11.4.0] on linux Type "help", "copyright", "credits" or "license" for more information. >>> 35245092.0/37333922.0*100.0 94.40500786389386 > > Alex > > -=-=-=-=-=-=-=-=-=-=-=- > Links: You receive all messages sent to this group. > View/Reply Online (#199104):https://lists.openembedded.org/g/openembedded-core/message/199104 > Mute This Topic:https://lists.openembedded.org/mt/105955497/3616765 > Group Owner:openembedded-core+owner@lists.openembedded.org > Unsubscribe:https://lists.openembedded.org/g/openembedded-core/unsub [randy.macleod@windriver.com] > -=-=-=-=-=-=-=-=-=-=-=- >
On Tue, May 14, 2024 at 8:25 AM Randy MacLeod <randy.macleod@windriver.com> wrote: > > On 2024-05-07 3:56 p.m., Alexander Kanavin via lists.openembedded.org wrote: > > On Tue, 7 May 2024 at 18:13, Khem Raj <raj.khem@gmail.com> wrote: > > Firstly I am inclined towards removing it if we can, since I think it should be > packages to decide to use it, then they can maintain it better from testing POV, > However, there is something > to consider w.r.t. to this option. This does help in optimizing loading DSOs > by reducing the number of PLTs and improves loading time for DSOs which might > be quite meaningful for embedded devices that we target. However, we only > enable hidden visibility only for inlines so it won't be full benefits but we do > have better default compatibility. > > Since we have had this option for a long time and it still seems > relevant, I would > think it would make sense to do some benchmarking on a sizeable C++ program > perhaps chromium or webkit, and see the difference in DSOs sizes with > and without > this option and also benchmark the loading time of the resulting > chromium package. > > maybe we will find that the gains are not as significant, in that case > we might drop > it from defaults. > > I digged deeper and this is where I think it originated: > https://git.openembedded.org/openembedded/commit/?id=ddd6d1fd26416a3766d754eeda4237a680a65520 > > Ah the simpler times of oe-classic, when one could bundle four > unrelated changes into one commit and explain none of them. > > Benchmarking I have no time for right now, just wanted to point out > that adding the flag was never justified in commit log. Can you > experiment with webkit? > > > I tried to build chromium with and without this flag but ran into build problems - sigh. > > I pulled out the next most-bloated single executable that I could think of: nodejs > > The bottom line is that the flag reduces the nodejs executable size by ~6%. > Actual sizes: > > >>> 37333922 - 35245092 > 2088830 > > so 2 MB out of 37 MB ! > > Chromium has been updated over the weekend so I'll try that next. > > What else would people like to see to assess the utility of this compiler option? > Thanks for taking this up. I think chromium and nodejs are large enough packages to give us a good idea of its benefits. We should add a comment to config files where this option is added for future reference. > > ../Randy > > Original build: > > ❯ size ../chromium-poky-m-aab1335523/tmp/work/core2-64-poky-linux/nodejs/20.12.2/packages-split/nodejs/usr/bin/node > text data bss dec hex filename > 35245092 628548 154792 36028432 225c010 ../chromium-poky-m-aab1335523/tmp/work/core2-64-poky-linux/nodejs/20.12.2/packages-split/nodejs/usr/bin/node > > With this patch: > > ❯ git diff > diff --git a/meta-oe/recipes-devtools/nodejs/nodejs_20.12.2.bb b/meta-oe/recipes-devtools/nodejs/nodejs_20.12.2.bb > index d86c38f2f..50cff533f 100644 > --- a/meta-oe/recipes-devtools/nodejs/nodejs_20.12.2.bb > +++ b/meta-oe/recipes-devtools/nodejs/nodejs_20.12.2.bb > @@ -42,6 +42,8 @@ S = "${WORKDIR}/node-v${PV}" > > CVE_PRODUCT += "node.js" > > +CXXFLAGS:remove = "-fvisibility-inlines-hidden" > + > # v8 errors out if you have set CCACHE > CCACHE = "" > > > > ❯ size tmp/work/core2-64-poky-linux/nodejs/20.12.2/packages-split/nodejs/usr/bin/node > text data bss dec hex filename > 37333922 628548 154792 38117262 2459f8e tmp/work/core2-64-poky-linux/nodejs/20.12.2/packages-split/nodejs/usr/bin/node > > ❯ python3 > Python 3.10.12 (main, Nov 20 2023, 15:14:05) [GCC 11.4.0] on linux > Type "help", "copyright", "credits" or "license" for more information. > >>> 35245092.0/37333922.0*100.0 > 94.40500786389386 > > > > > Alex > > > -=-=-=-=-=-=-=-=-=-=-=- > Links: You receive all messages sent to this group. > View/Reply Online (#199104): https://lists.openembedded.org/g/openembedded-core/message/199104 > Mute This Topic: https://lists.openembedded.org/mt/105955497/3616765 > Group Owner: openembedded-core+owner@lists.openembedded.org > Unsubscribe: https://lists.openembedded.org/g/openembedded-core/unsub [randy.macleod@windriver.com] > -=-=-=-=-=-=-=-=-=-=-=- > > > -- > # Randy MacLeod > # Wind River Linux
Can you also try webkitgtk please? It's directly in core, no extra layer or special setup needed. Alex On Tue, 14 May 2024 at 17:43, Khem Raj <raj.khem@gmail.com> wrote: > > On Tue, May 14, 2024 at 8:25 AM Randy MacLeod > <randy.macleod@windriver.com> wrote: > > > > On 2024-05-07 3:56 p.m., Alexander Kanavin via lists.openembedded.org wrote: > > > > On Tue, 7 May 2024 at 18:13, Khem Raj <raj.khem@gmail.com> wrote: > > > > Firstly I am inclined towards removing it if we can, since I think it should be > > packages to decide to use it, then they can maintain it better from testing POV, > > However, there is something > > to consider w.r.t. to this option. This does help in optimizing loading DSOs > > by reducing the number of PLTs and improves loading time for DSOs which might > > be quite meaningful for embedded devices that we target. However, we only > > enable hidden visibility only for inlines so it won't be full benefits but we do > > have better default compatibility. > > > > Since we have had this option for a long time and it still seems > > relevant, I would > > think it would make sense to do some benchmarking on a sizeable C++ program > > perhaps chromium or webkit, and see the difference in DSOs sizes with > > and without > > this option and also benchmark the loading time of the resulting > > chromium package. > > > > maybe we will find that the gains are not as significant, in that case > > we might drop > > it from defaults. > > > > I digged deeper and this is where I think it originated: > > https://git.openembedded.org/openembedded/commit/?id=ddd6d1fd26416a3766d754eeda4237a680a65520 > > > > Ah the simpler times of oe-classic, when one could bundle four > > unrelated changes into one commit and explain none of them. > > > > Benchmarking I have no time for right now, just wanted to point out > > that adding the flag was never justified in commit log. Can you > > experiment with webkit? > > > > > > I tried to build chromium with and without this flag but ran into build problems - sigh. > > > > I pulled out the next most-bloated single executable that I could think of: nodejs > > > > The bottom line is that the flag reduces the nodejs executable size by ~6%. > > Actual sizes: > > > > >>> 37333922 - 35245092 > > 2088830 > > > > so 2 MB out of 37 MB ! > > > > Chromium has been updated over the weekend so I'll try that next. > > > > What else would people like to see to assess the utility of this compiler option? > > > Thanks for taking this up. > I think chromium and nodejs are large enough packages to give us a > good idea of its benefits. > We should add a comment to config files where this option is added for > future reference. > > > > > ../Randy > > > > Original build: > > > > ❯ size ../chromium-poky-m-aab1335523/tmp/work/core2-64-poky-linux/nodejs/20.12.2/packages-split/nodejs/usr/bin/node > > text data bss dec hex filename > > 35245092 628548 154792 36028432 225c010 ../chromium-poky-m-aab1335523/tmp/work/core2-64-poky-linux/nodejs/20.12.2/packages-split/nodejs/usr/bin/node > > > > With this patch: > > > > ❯ git diff > > diff --git a/meta-oe/recipes-devtools/nodejs/nodejs_20.12.2.bb b/meta-oe/recipes-devtools/nodejs/nodejs_20.12.2.bb > > index d86c38f2f..50cff533f 100644 > > --- a/meta-oe/recipes-devtools/nodejs/nodejs_20.12.2.bb > > +++ b/meta-oe/recipes-devtools/nodejs/nodejs_20.12.2.bb > > @@ -42,6 +42,8 @@ S = "${WORKDIR}/node-v${PV}" > > > > CVE_PRODUCT += "node.js" > > > > +CXXFLAGS:remove = "-fvisibility-inlines-hidden" > > + > > # v8 errors out if you have set CCACHE > > CCACHE = "" > > > > > > > > ❯ size tmp/work/core2-64-poky-linux/nodejs/20.12.2/packages-split/nodejs/usr/bin/node > > text data bss dec hex filename > > 37333922 628548 154792 38117262 2459f8e tmp/work/core2-64-poky-linux/nodejs/20.12.2/packages-split/nodejs/usr/bin/node > > > > ❯ python3 > > Python 3.10.12 (main, Nov 20 2023, 15:14:05) [GCC 11.4.0] on linux > > Type "help", "copyright", "credits" or "license" for more information. > > >>> 35245092.0/37333922.0*100.0 > > 94.40500786389386 > > > > > > > > > > Alex > > > > > > -=-=-=-=-=-=-=-=-=-=-=- > > Links: You receive all messages sent to this group. > > View/Reply Online (#199104): https://lists.openembedded.org/g/openembedded-core/message/199104 > > Mute This Topic: https://lists.openembedded.org/mt/105955497/3616765 > > Group Owner: openembedded-core+owner@lists.openembedded.org > > Unsubscribe: https://lists.openembedded.org/g/openembedded-core/unsub [randy.macleod@windriver.com] > > -=-=-=-=-=-=-=-=-=-=-=- > > > > > > -- > > # Randy MacLeod > > # Wind River Linux
diff --git a/meta/recipes-connectivity/kea/kea_2.4.1.bb b/meta/recipes-connectivity/kea/kea_2.4.1.bb index 6c1e457938a..19309ce3144 100644 --- a/meta/recipes-connectivity/kea/kea_2.4.1.bb +++ b/meta/recipes-connectivity/kea/kea_2.4.1.bb @@ -38,6 +38,7 @@ DEBUG_OPTIMIZATION:append:mipsel = " -O" BUILD_OPTIMIZATION:remove:mipsel = " -Og" BUILD_OPTIMIZATION:append:mipsel = " -O" +CXXFLAGS:remove = "-fvisibility-inlines-hidden" EXTRA_OECONF = "--with-boost-libs=-lboost_system \ --with-log4cplus=${STAGING_DIR_TARGET}${prefix} \ --with-openssl=${STAGING_DIR_TARGET}${prefix}"
This fixes build with gcc-14, where default visibility is extended to inline functions and getAll() function now falls into this category and functions are marked hidden resulting in linking errors Fixes /OE/build/oe-core/tmp-glibc/work/core2-64-oe-linux/kea/2.5.8/recipe-sysroot-native/usr/bin/x86_64-oe-linux/../../libexec/x86_64-oe-linux/gcc/x86_64-oe-linux/14.0.1/ld: ./.libs/libdhcp4.a(dhcp4_srv.o): in function `isc::dhcp::Dhcpv4Srv::appendRequestedVendorOptions(isc::dhcp::Dhcpv4Exchange&)': /usr/src/debug/kea/2.5.8/src/bin/dhcp4/dhcp4_srv.cc:2356:(.text+0xaac2): undefined reference to `isc::dhcp::CfgOption::getAll(unsigned int) const' /OE/build/oe-core/tmp-glibc/work/core2-64-oe-linux/kea/2.5.8/recipe-sysroot-native/usr/bin/x86_64-oe-linux/../../libexec/x86_64-oe-linux/gcc/x86_64-oe-linux/14.0.1/ld: ./.libs/libdhcp4.a(dhcp4_srv.o): in function `isc::dhcp::OptionDescriptor isc::dhcp::CfgOption::get<unsigned int>(unsigned int const&, unsigned short) const': /usr/src/debug/kea/2.5.8/src/lib/dhcpsrv/cfg_option.h:609:(.text+0xb288): undefined reference to `isc::dhcp::CfgOption::getAll(unsigned int) const' /OE/build/oe-core/tmp-glibc/work/core2-64-oe-linux/kea/2.5.8/recipe-sysroot-native/usr/bin/x86_64-oe-linux/../../libexec/x86_64-oe-linux/gcc/x86_64-oe-linux/14.0.1/ld: ./.libs/libdhcp4.a(dhcp4_srv.o): in function `isc::dhcp::Dhcpv4Srv::appendRequestedOptions(isc::dhcp::Dhcpv4Exchange&)': /usr/src/debug/kea/2.5.8/src/bin/dhcp4/dhcp4_srv.cc:2128:(.text+0xc556): undefined reference to `isc::dhcp::CfgOption::getAll(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) const' /OE/build/oe-core/tmp-glibc/work/core2-64-oe-linux/kea/2.5.8/recipe-sysroot-native/usr/bin/x86_64-oe-linux/../../libexec/x86_64-oe-linux/gcc/x86_64-oe-linux/14.0.1/ld: ./.libs/libdhcp4.a(dhcp4_srv.o): in function `std::vector<isc::dhcp::OptionDescriptor, std::allocator<isc::dhcp::OptionDescriptor> > isc::dhcp::CfgOption::getList<char [6]>(char const (&) [6], unsigned short) const': /usr/src/debug/kea/2.5.8/src/lib/dhcpsrv/cfg_option.h:641:(.text._ZNK3isc4dhcp9CfgOption7getListIA6_cEESt6vectorINS0_16OptionDescriptorESaIS5_EERKT_t[_ZNK3isc4dhcp9CfgOption7getListIA6_cEESt6vectorINS0_16OptionDescriptorESaIS5_EERKT_t]+0x86): undefined reference to `isc::dhcp::CfgOption::getAll(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) const' /OE/build/oe-core/tmp-glibc/work/core2-64-oe-linux/kea/2.5.8/recipe-sysroot-native/usr/bin/x86_64-oe-linux/../../libexec/x86_64-oe-linux/gcc/x86_64-oe-linux/14.0.1/ld: ./.libs/libdhcp4.a(dhcp4_srv.o): in function `isc::dhcp::OptionDescriptor isc::dhcp::CfgOption::get<char [6]>(char const (&) [6], unsigned short) const': /usr/src/debug/kea/2.5.8/src/lib/dhcpsrv/cfg_option.h:609:(.text._ZNK3isc4dhcp9CfgOption3getIA6_cEENS0_16OptionDescriptorERKT_t[_ZNK3isc4dhcp9CfgOption3getIA6_cEENS0_16OptionDescriptorERKT_t]+0x77): undefined reference to `isc::dhcp::CfgOption::getAll(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) const' collect2: error: ld returned 1 exit status make[5]: *** [Makefile:651: kea-dhcp4] Error 1 Signed-off-by: Khem Raj <raj.khem@gmail.com> Cc: Martin Jansa <martin.jansa@gmail.com> --- meta/recipes-connectivity/kea/kea_2.4.1.bb | 1 + 1 file changed, 1 insertion(+)