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

[Khem Raj]

On Thu, Aug 29, 2019 at 2:23 PM Adrian Bunk <bunk at stusta.de> wrote:
>
> 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.
>

which is partly because we dont have conducive workflow and tooling
for devs to upstream the patches or
consider building master branch of upstream repos easily. Maybe some
thought for future enhancements where
we can have a develop mode for packages which we can use for doing
this kind of work.

> > 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).
>

right I remember that, but then I also know first-hand cases where the
compiler was telling you all the way and it was
ignored which ended up in field bugs, so there is no right answer.

> > 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.
>

we also provide development environment along with distro building
tools so its in interest for us to enable
not only packagers but also package developers

> > > 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
>