[oe] [meta-oe][PATCH] libwebsockets: Fix the build with -Os

[Adrian Bunk]

On Thu, Aug 29, 2019 at 01:51:05PM -0700, Khem Raj wrote:
> On Thu, Aug 29, 2019 at 1:25 PM Adrian Bunk <bunk at stusta.de> wrote:
> >
> > On Thu, Aug 29, 2019 at 12:54:16PM -0700, Khem Raj wrote:
> > > On Thu, Aug 29, 2019 at 12:14 PM Adrian Bunk <bunk at stusta.de> wrote:
> > > >
> > > > On Thu, Aug 29, 2019 at 11:46:55AM -0700, Khem Raj wrote:
> > > > > On Thu, Aug 29, 2019 at 3:39 AM Adrian Bunk <bunk at stusta.de> wrote:
> > > > > >
> > > > > > lib/event-libs/libuv/libuv.c: In function 'elops_destroy_context1_uv':
> > > > > > lib/event-libs/libuv/libuv.c:519:7: error: 'm' may be used uninitialized in this function [-Werror=maybe-uninitialized]
> > > > > >     if (m)
> > > > > >        ^
> > > > > >
> > > > > > Signed-off-by: Adrian Bunk <bunk at stusta.de>
> > > > > > ---
> > > > > >  .../recipes-connectivity/libwebsockets/libwebsockets_3.1.0.bb   | 2 ++
> > > > > >  1 file changed, 2 insertions(+)
> > > > > >
> > > > > > diff --git a/meta-oe/recipes-connectivity/libwebsockets/libwebsockets_3.1.0.bb b/meta-oe/recipes-connectivity/libwebsockets/libwebsockets_3.1.0.bb
> > > > > > index 50620d99e..fcabeb902 100644
> > > > > > --- a/meta-oe/recipes-connectivity/libwebsockets/libwebsockets_3.1.0.bb
> > > > > > +++ b/meta-oe/recipes-connectivity/libwebsockets/libwebsockets_3.1.0.bb
> > > > > > @@ -28,3 +28,5 @@ EXTRA_OECMAKE += " \
> > > > > >  PACKAGES =+ "${PN}-testapps"
> > > > > >
> > > > > >  FILES_${PN}-testapps += "${datadir}/libwebsockets-test-server/*"
> > > > > > +
> > > > > > +CFLAGS_append = " -Wno-error"
> > > > >
> > > > > is it possible to fix the underlying problem? since Os is not default
> > > > > it definitely could be a bug in upstream but
> > > > > by disabling warnings for all kind of builds we are  painting with broad brush
> > > >
> > > > The underlying problem is that some gcc warnings are not reliable with -Os,
> > > > there are bugs open in the gcc bugzilla for that.
> > > >
> > > I am aware of that for maybe-* warnings heuristics may go wrong, but
> > > then its better to just disable that
> > > one warning from being treated as error if thats possible to add
> > > easily something like
> > > -Wno-error=maybe-uninitialized could do it.
> >
> > And then the package fails to build due to a different warning after the
> > next gcc upgrade.
> >
> 
> which is fine, since we can report it upstream to either gcc if its
> gcc's fault or to package itself

This is often a waste of time since the code in OE is often much older 
than upstream master.

> and better still with a patch.

But please do not add such patches to OE.

Patches from people who don't know the code well are often quite buggy,
and fixing warnings then tends to add more bugs than it fixes.

Google "Debian OpenSSL disaster" for how the Debian maintainer "fixing" 
a Valgrind warning in the Debian OpenSSL package made private keys used 
for ssh authentication in Debian/Ubuntu predictable (AKA everyone on the 
internet could log into the affected machines).

> Ideally we should use package defaults
> or maybe be more strict
>  If everyone stop using these warnings then why is compiler adding
> them in the first place
> here I realize lies the difference of opinion.

When you are developing software these warnings are very useful.

A distribution is not developing software, it is distributing 
software developed by other people.

> > The few packages that manually set -Werror are causing so much trouble,
> > and seeing warnings as errors in this code from 2018 that is currently
> > 298 commits behind upstream master won't bring actual benefits even
> > when the warnings are not gcc bugs.
> >
> > If you are interested in warnings you shouldn't have -Werror in very few
> > packages, but check for the most problematic warnings in all packages.
> >
> > E.g. -Wimplicit-function-declaration warnings are often the cause for
> > runime crashes, and a quick grep through my build logs shows that your
> > gettid patches to snort/lttng-tools/lttng-ust are not correct.[1]
> 
> maybe you should explain a bit more why they not correct
> these patches are to define local gettid function if system libc does
> not provide it
> there is a new syscall wrapper in glibc 2.30, older glibcs wont have it.

...
../../../../lttng-tools-2.10.7/src/common/error.h:154:27: warning: implicit declaration of function 'getpid'; did you mean 'getpt'? [-Wimplicit-function-declaration]
../../../../lttng-tools-2.10.7/src/common/error.h:154:44: warning: implicit declaration of function 'gettid'; did you mean 'getcpu'? [-Wimplicit-function-declaration]
...

You ifdef'ed away the #include <unistd.h> that provides the prototypes 
for these functions in glibc 2.30.

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed