[dunfell,15/15] lz4: specify gnu17 in CFLAGS to fix reproducibility issues

We are seeing reproducibility issues where gcc-cross sometimes defaults
to gnu11 and other times to gnu17.

Specify std=gnu17 rather than leave this to chance.

Signed-off-by: Steve Sakoman <steve@sakoman.com>
Signed-off-by: Khem Raj <raj.khem@gmail.com>
---
 meta/recipes-support/lz4/lz4_1.9.2.bb | 1 +
 1 file changed, 1 insertion(+)

On Sat, 2023-11-11 at 05:03 -1000, Steve Sakoman wrote:
> We are seeing reproducibility issues where gcc-cross sometimes defaults
> to gnu11 and other times to gnu17.

Why is gcc-cross doing that? It is supposed to be deterministic so we
really need to get to the bottom of this :/.

Cheers,

Richard

On Sat, Nov 11, 2023 at 9:36 AM Richard Purdie
<richard.purdie@linuxfoundation.org> wrote:
>
> On Sat, 2023-11-11 at 05:03 -1000, Steve Sakoman wrote:
> > We are seeing reproducibility issues where gcc-cross sometimes defaults
> > to gnu11 and other times to gnu17.
>
> Why is gcc-cross doing that? It is supposed to be deterministic so we
> really need to get to the bottom of this :/.

I think the previous patch to enforce OE cflags is exposing this
problem. Now, it could be a
bug in how gcc is invoked or in gcc itself,  but there is one file
that shows up these c11 tags
in debug info segments.

>
> Cheers,
>
> Richard
>
>
> -=-=-=-=-=-=-=-=-=-=-=-
> Links: You receive all messages sent to this group.
> View/Reply Online (#190434): https://lists.openembedded.org/g/openembedded-core/message/190434
> Mute This Topic: https://lists.openembedded.org/mt/102526884/1997914
> Group Owner: openembedded-core+owner@lists.openembedded.org
> Unsubscribe: https://lists.openembedded.org/g/openembedded-core/unsub [raj.khem@gmail.com]
> -=-=-=-=-=-=-=-=-=-=-=-
>

On Sat, Nov 11, 2023 at 8:08 AM Khem Raj <raj.khem@gmail.com> wrote:
>
> On Sat, Nov 11, 2023 at 9:36 AM Richard Purdie
> <richard.purdie@linuxfoundation.org> wrote:
> >
> > On Sat, 2023-11-11 at 05:03 -1000, Steve Sakoman wrote:
> > > We are seeing reproducibility issues where gcc-cross sometimes defaults
> > > to gnu11 and other times to gnu17.
> >
> > Why is gcc-cross doing that? It is supposed to be deterministic so we
> > really need to get to the bottom of this :/.
>
> I think the previous patch to enforce OE cflags is exposing this
> problem.

No, the problem also exists without the previous patch.

Richard suggested the backport as a potential solution, since it fixed
a repro issue in master.

It turned out to not be a complete fix in dunfell, hence my second patch.

I agree that it would be good to get to the bottom of the gcc
reproducibility problem.  But until we do, this second patch will fix
the issue now and will do no harm once gcc is fixed since it is just
explicitly specifying what should be the gcc default.

Steve

> Now, it could be a
> bug in how gcc is invoked or in gcc itself,  but there is one file
> that shows up these c11 tags
> in debug info segments.
>
> >
> > Cheers,
> >
> > Richard
> >
> >
> > -=-=-=-=-=-=-=-=-=-=-=-
> > Links: You receive all messages sent to this group.
> > View/Reply Online (#190434): https://lists.openembedded.org/g/openembedded-core/message/190434
> > Mute This Topic: https://lists.openembedded.org/mt/102526884/1997914
> > Group Owner: openembedded-core+owner@lists.openembedded.org
> > Unsubscribe: https://lists.openembedded.org/g/openembedded-core/unsub [raj.khem@gmail.com]
> > -=-=-=-=-=-=-=-=-=-=-=-
> >

On Sat, 2023-11-11 at 08:20 -1000, Steve Sakoman wrote:
> On Sat, Nov 11, 2023 at 8:08 AM Khem Raj <raj.khem@gmail.com> wrote:
> > 
> > On Sat, Nov 11, 2023 at 9:36 AM Richard Purdie
> > <richard.purdie@linuxfoundation.org> wrote:
> > > 
> > > On Sat, 2023-11-11 at 05:03 -1000, Steve Sakoman wrote:
> > > > We are seeing reproducibility issues where gcc-cross sometimes defaults
> > > > to gnu11 and other times to gnu17.
> > > 
> > > Why is gcc-cross doing that? It is supposed to be deterministic so we
> > > really need to get to the bottom of this :/.
> > 
> > I think the previous patch to enforce OE cflags is exposing this
> > problem.
> 
> No, the problem also exists without the previous patch.
> 
> Richard suggested the backport as a potential solution, since it fixed
> a repro issue in master.
> 
> It turned out to not be a complete fix in dunfell, hence my second patch.
> 
> I agree that it would be good to get to the bottom of the gcc
> reproducibility problem.  But until we do, this second patch will fix
> the issue now and will do no harm once gcc is fixed since it is just
> explicitly specifying what should be the gcc default.

I worry that if we merge that, we'll not spend the time needed to get
to the bottom of the gcc issue :(. We don't know if the issue is there
on master or not :/.

Cheers,

Richard

On Sat, Nov 11, 2023 at 8:47 AM Richard Purdie
<richard.purdie@linuxfoundation.org> wrote:
>
> On Sat, 2023-11-11 at 08:20 -1000, Steve Sakoman wrote:
> > On Sat, Nov 11, 2023 at 8:08 AM Khem Raj <raj.khem@gmail.com> wrote:
> > >
> > > On Sat, Nov 11, 2023 at 9:36 AM Richard Purdie
> > > <richard.purdie@linuxfoundation.org> wrote:
> > > >
> > > > On Sat, 2023-11-11 at 05:03 -1000, Steve Sakoman wrote:
> > > > > We are seeing reproducibility issues where gcc-cross sometimes defaults
> > > > > to gnu11 and other times to gnu17.
> > > >
> > > > Why is gcc-cross doing that? It is supposed to be deterministic so we
> > > > really need to get to the bottom of this :/.
> > >
> > > I think the previous patch to enforce OE cflags is exposing this
> > > problem.
> >
> > No, the problem also exists without the previous patch.
> >
> > Richard suggested the backport as a potential solution, since it fixed
> > a repro issue in master.
> >
> > It turned out to not be a complete fix in dunfell, hence my second patch.
> >
> > I agree that it would be good to get to the bottom of the gcc
> > reproducibility problem.  But until we do, this second patch will fix
> > the issue now and will do no harm once gcc is fixed since it is just
> > explicitly specifying what should be the gcc default.
>
> I worry that if we merge that, we'll not spend the time needed to get
> to the bottom of the gcc issue :(. We don't know if the issue is there
> on master or not :/.

Given that my knowledge of gcc is pretty much non-existent, I don't
see myself getting to the bottom of this while still maintaining the
three stable branches. So help from experts is required.

If you are OK with ongoing lz4 repro errors in dunfell, I'm happy to
drop the patches and wait for someone knowledgeable to fix gcc.

Steve

On Sat, 2023-11-11 at 05:03 -1000, Steve Sakoman wrote:
> We are seeing reproducibility issues where gcc-cross sometimes defaults
> to gnu11 and other times to gnu17.
> 
> Specify std=gnu17 rather than leave this to chance.
> 
> Signed-off-by: Steve Sakoman <steve@sakoman.com>
> Signed-off-by: Khem Raj <raj.khem@gmail.com>
> ---
>  meta/recipes-support/lz4/lz4_1.9.2.bb | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/meta/recipes-support/lz4/lz4_1.9.2.bb b/meta/recipes-support/lz4/lz4_1.9.2.bb
> index c2e24b518c..8cf71fdc04 100644
> --- a/meta/recipes-support/lz4/lz4_1.9.2.bb
> +++ b/meta/recipes-support/lz4/lz4_1.9.2.bb
> @@ -23,6 +23,7 @@ S = "${WORKDIR}/git"
>  # Fixed in r118, which is larger than the current version.
>  CVE_CHECK_WHITELIST += "CVE-2014-4715"
>  
> +CFLAGS_append = " -std=gnu17"
>  EXTRA_OEMAKE = "PREFIX=${prefix} CC='${CC}' CFLAGS='${CFLAGS}' DESTDIR=${D} LIBDIR=${libdir} INCLUDEDIR=${includedir} BUILD_STATIC=no"
>  
>  do_install() {

I've looked into this a lot more and I'm now of the opinion this is
totally wrong and definitely should not merge.

An example failure is:

http://autobuilder.yocto.io/pub/repro-fail/oe-reproducible-20231109-ch73ktqp/packages/diff-html/

First, it is worth noting the huge size difference. It appears debug
line information is being missed out. The difference in cflags is a
false lead, it is because crti.S is compiled with c17 and the producer
string is being mis-compared, the data is missing on one side.

So what we're looking for really is a cflags issue where -g is going
missing.

I did check older failures which look similar:

http://autobuilder.yocto.io/pub/repro-fail/oe-reproducible-20231105-_xpb575y/packages/diff-html/

Since we're looking for cflags issues:

https://github.com/lz4/lz4/commit/7d8d1075e696d585238e6aa6a4f43dc759da250e

looks interesting and leads to:

https://savannah.gnu.org/bugs/?func=detailitem&item_id=59230

If the makefile had intermittently picked up CFLAGS from the
environment, the output would be identical to the previous cflags patch
in the series, which would cross link the hash equivalence
inputs/outputs and could generally confuse things.

The real question is which autobuilder worker is compiling without the
-g option and is it a make bug? Perhaps it doesn't matter and we just
need to force the input and output hashes to change to stop the errors
once the previous patch is applied.

Cheers,

Richard

On Mon, Nov 13, 2023 at 3:42 AM Richard Purdie
<richard.purdie@linuxfoundation.org> wrote:
>
> On Sat, 2023-11-11 at 05:03 -1000, Steve Sakoman wrote:
> > We are seeing reproducibility issues where gcc-cross sometimes defaults
> > to gnu11 and other times to gnu17.
> >
> > Specify std=gnu17 rather than leave this to chance.
> >
> > Signed-off-by: Steve Sakoman <steve@sakoman.com>
> > Signed-off-by: Khem Raj <raj.khem@gmail.com>
> > ---
> >  meta/recipes-support/lz4/lz4_1.9.2.bb | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/meta/recipes-support/lz4/lz4_1.9.2.bb b/meta/recipes-support/lz4/lz4_1.9.2.bb
> > index c2e24b518c..8cf71fdc04 100644
> > --- a/meta/recipes-support/lz4/lz4_1.9.2.bb
> > +++ b/meta/recipes-support/lz4/lz4_1.9.2.bb
> > @@ -23,6 +23,7 @@ S = "${WORKDIR}/git"
> >  # Fixed in r118, which is larger than the current version.
> >  CVE_CHECK_WHITELIST += "CVE-2014-4715"
> >
> > +CFLAGS_append = " -std=gnu17"
> >  EXTRA_OEMAKE = "PREFIX=${prefix} CC='${CC}' CFLAGS='${CFLAGS}' DESTDIR=${D} LIBDIR=${libdir} INCLUDEDIR=${includedir} BUILD_STATIC=no"
> >
> >  do_install() {
>
> I've looked into this a lot more and I'm now of the opinion this is
> totally wrong and definitely should not merge.
>
> An example failure is:
>
> http://autobuilder.yocto.io/pub/repro-fail/oe-reproducible-20231109-ch73ktqp/packages/diff-html/
>
> First, it is worth noting the huge size difference. It appears debug
> line information is being missed out. The difference in cflags is a
> false lead, it is because crti.S is compiled with c17 and the producer
> string is being mis-compared, the data is missing on one side.
>
> So what we're looking for really is a cflags issue where -g is going
> missing.
>
> I did check older failures which look similar:
>
> http://autobuilder.yocto.io/pub/repro-fail/oe-reproducible-20231105-_xpb575y/packages/diff-html/
>
> Since we're looking for cflags issues:
>
> https://github.com/lz4/lz4/commit/7d8d1075e696d585238e6aa6a4f43dc759da250e
>
> looks interesting and leads to:
>
> https://savannah.gnu.org/bugs/?func=detailitem&item_id=59230
>
> If the makefile had intermittently picked up CFLAGS from the
> environment, the output would be identical to the previous cflags patch
> in the series, which would cross link the hash equivalence
> inputs/outputs and could generally confuse things.

Thanks for looking into this!  I will remove the patch from the series.

> The real question is which autobuilder worker is compiling without the
> -g option and is it a make bug? Perhaps it doesn't matter and we just
> need to force the input and output hashes to change to stop the errors
> once the previous patch is applied.

What is the proper way to force the input and output hashes to change?

Thanks again!

Steve

On Mon, 2023-11-13 at 04:14 -1000, Steve Sakoman wrote:
> On Mon, Nov 13, 2023 at 3:42 AM Richard Purdie
> <richard.purdie@linuxfoundation.org> wrote:
> > 
> > On Sat, 2023-11-11 at 05:03 -1000, Steve Sakoman wrote:
> > > We are seeing reproducibility issues where gcc-cross sometimes defaults
> > > to gnu11 and other times to gnu17.
> > > 
> > > Specify std=gnu17 rather than leave this to chance.
> > > 
> > > Signed-off-by: Steve Sakoman <steve@sakoman.com>
> > > Signed-off-by: Khem Raj <raj.khem@gmail.com>
> > > ---
> > >  meta/recipes-support/lz4/lz4_1.9.2.bb | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/meta/recipes-support/lz4/lz4_1.9.2.bb b/meta/recipes-support/lz4/lz4_1.9.2.bb
> > > index c2e24b518c..8cf71fdc04 100644
> > > --- a/meta/recipes-support/lz4/lz4_1.9.2.bb
> > > +++ b/meta/recipes-support/lz4/lz4_1.9.2.bb
> > > @@ -23,6 +23,7 @@ S = "${WORKDIR}/git"
> > >  # Fixed in r118, which is larger than the current version.
> > >  CVE_CHECK_WHITELIST += "CVE-2014-4715"
> > > 
> > > +CFLAGS_append = " -std=gnu17"
> > >  EXTRA_OEMAKE = "PREFIX=${prefix} CC='${CC}' CFLAGS='${CFLAGS}' DESTDIR=${D} LIBDIR=${libdir} INCLUDEDIR=${includedir} BUILD_STATIC=no"
> > > 
> > >  do_install() {
> > 
> > I've looked into this a lot more and I'm now of the opinion this is
> > totally wrong and definitely should not merge.
> > 
> > An example failure is:
> > 
> > http://autobuilder.yocto.io/pub/repro-fail/oe-reproducible-20231109-ch73ktqp/packages/diff-html/
> > 
> > First, it is worth noting the huge size difference. It appears debug
> > line information is being missed out. The difference in cflags is a
> > false lead, it is because crti.S is compiled with c17 and the producer
> > string is being mis-compared, the data is missing on one side.
> > 
> > So what we're looking for really is a cflags issue where -g is going
> > missing.
> > 
> > I did check older failures which look similar:
> > 
> > http://autobuilder.yocto.io/pub/repro-fail/oe-reproducible-20231105-_xpb575y/packages/diff-html/
> > 
> > Since we're looking for cflags issues:
> > 
> > https://github.com/lz4/lz4/commit/7d8d1075e696d585238e6aa6a4f43dc759da250e
> > 
> > looks interesting and leads to:
> > 
> > https://savannah.gnu.org/bugs/?func=detailitem&item_id=59230
> > 
> > If the makefile had intermittently picked up CFLAGS from the
> > environment, the output would be identical to the previous cflags patch
> > in the series, which would cross link the hash equivalence
> > inputs/outputs and could generally confuse things.
> 
> Thanks for looking into this!  I will remove the patch from the series.
> 
> > The real question is which autobuilder worker is compiling without the
> > -g option and is it a make bug? Perhaps it doesn't matter and we just
> > need to force the input and output hashes to change to stop the errors
> > once the previous patch is applied.
> 
> What is the proper way to force the input and output hashes to change?
> 
> Thanks again!

https://git.yoctoproject.org/poky/commit/?id=ad3dfac07f856917434deb073fe9c8a788d4c21d

is an example.

Cheers,

Richard

On Mon, Nov 13, 2023 at 4:35 AM Richard Purdie
<richard.purdie@linuxfoundation.org> wrote:
>
> On Mon, 2023-11-13 at 04:14 -1000, Steve Sakoman wrote:
> > On Mon, Nov 13, 2023 at 3:42 AM Richard Purdie
> > <richard.purdie@linuxfoundation.org> wrote:
> > >
> > > On Sat, 2023-11-11 at 05:03 -1000, Steve Sakoman wrote:
> > > > We are seeing reproducibility issues where gcc-cross sometimes defaults
> > > > to gnu11 and other times to gnu17.
> > > >
> > > > Specify std=gnu17 rather than leave this to chance.
> > > >
> > > > Signed-off-by: Steve Sakoman <steve@sakoman.com>
> > > > Signed-off-by: Khem Raj <raj.khem@gmail.com>
> > > > ---
> > > >  meta/recipes-support/lz4/lz4_1.9.2.bb | 1 +
> > > >  1 file changed, 1 insertion(+)
> > > >
> > > > diff --git a/meta/recipes-support/lz4/lz4_1.9.2.bb b/meta/recipes-support/lz4/lz4_1.9.2.bb
> > > > index c2e24b518c..8cf71fdc04 100644
> > > > --- a/meta/recipes-support/lz4/lz4_1.9.2.bb
> > > > +++ b/meta/recipes-support/lz4/lz4_1.9.2.bb
> > > > @@ -23,6 +23,7 @@ S = "${WORKDIR}/git"
> > > >  # Fixed in r118, which is larger than the current version.
> > > >  CVE_CHECK_WHITELIST += "CVE-2014-4715"
> > > >
> > > > +CFLAGS_append = " -std=gnu17"
> > > >  EXTRA_OEMAKE = "PREFIX=${prefix} CC='${CC}' CFLAGS='${CFLAGS}' DESTDIR=${D} LIBDIR=${libdir} INCLUDEDIR=${includedir} BUILD_STATIC=no"
> > > >
> > > >  do_install() {
> > >
> > > I've looked into this a lot more and I'm now of the opinion this is
> > > totally wrong and definitely should not merge.
> > >
> > > An example failure is:
> > >
> > > http://autobuilder.yocto.io/pub/repro-fail/oe-reproducible-20231109-ch73ktqp/packages/diff-html/
> > >
> > > First, it is worth noting the huge size difference. It appears debug
> > > line information is being missed out. The difference in cflags is a
> > > false lead, it is because crti.S is compiled with c17 and the producer
> > > string is being mis-compared, the data is missing on one side.
> > >
> > > So what we're looking for really is a cflags issue where -g is going
> > > missing.
> > >
> > > I did check older failures which look similar:
> > >
> > > http://autobuilder.yocto.io/pub/repro-fail/oe-reproducible-20231105-_xpb575y/packages/diff-html/
> > >
> > > Since we're looking for cflags issues:
> > >
> > > https://github.com/lz4/lz4/commit/7d8d1075e696d585238e6aa6a4f43dc759da250e
> > >
> > > looks interesting and leads to:
> > >
> > > https://savannah.gnu.org/bugs/?func=detailitem&item_id=59230
> > >
> > > If the makefile had intermittently picked up CFLAGS from the
> > > environment, the output would be identical to the previous cflags patch
> > > in the series, which would cross link the hash equivalence
> > > inputs/outputs and could generally confuse things.
> >
> > Thanks for looking into this!  I will remove the patch from the series.
> >
> > > The real question is which autobuilder worker is compiling without the
> > > -g option and is it a make bug? Perhaps it doesn't matter and we just
> > > need to force the input and output hashes to change to stop the errors
> > > once the previous patch is applied.
> >
> > What is the proper way to force the input and output hashes to change?
> >
> > Thanks again!
>
> https://git.yoctoproject.org/poky/commit/?id=ad3dfac07f856917434deb073fe9c8a788d4c21d
>
> is an example.

Thanks Richard!  I'll test with this and resubmit the series.

Steve