Date: Wed, 20 Jul 2022 16:39:09 -0600 From: Warner Losh <imp@bsdimp.com> To: John Baldwin <jhb@freebsd.org> Cc: Toomas Soome <tsoome@me.com>, Dmitry Chagin <dchagin@heemeyer.club>, Warner Losh <imp@freebsd.org>, src-committers <src-committers@freebsd.org>, "<dev-commits-src-all@freebsd.org>" <dev-commits-src-all@freebsd.org>, dev-commits-src-main@freebsd.org Subject: Re: git: 84bf2bbbecc3 - main - stand: constrain zlib/gzip CFLAGS better Message-ID: <CANCZdfpKndvQ%2BCPY5Q4vj2MiBhysOaxPY56xRWzft9%2BruVq6GQ@mail.gmail.com> In-Reply-To: <3cea890a-3441-4ced-e449-b54494f42a64@FreeBSD.org> References: <202207081750.268Ho5kZ066824@gitrepo.freebsd.org> <Ytg6Noc7T8R6sLNy@heemeyer.club> <244CD526-C7D0-4D42-9DAB-6EA690DFD3A7@me.com> <CANCZdfpdKV0%2BaAKUbdVmmCaOjvKPccE1LwUGc8VDjzYVeV-sew@mail.gmail.com> <CANCZdfo%2BzQuyr3M0uuBGXEE%2BMp4hiChyzqSAF8icxQwdpeWQyw@mail.gmail.com> <CANCZdfpc14U4sA2eX-SYSUC6zY7-0_nSF6AvLOH%2BLiEg08KD-w@mail.gmail.com> <3cea890a-3441-4ced-e449-b54494f42a64@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
[-- Attachment #1 --] Hey John, On Wed, Jul 20, 2022 at 4:13 PM John Baldwin <jhb@freebsd.org> wrote: > On 7/20/22 11:20 AM, Warner Losh wrote: > > On Wed, Jul 20, 2022 at 12:12 PM Warner Losh <imp@bsdimp.com> wrote: > > > >> > >> > >> On Wed, Jul 20, 2022 at 12:06 PM Warner Losh <imp@bsdimp.com> wrote: > >> > >>> > >>> > >>> On Wed, Jul 20, 2022 at 11:44 AM Toomas Soome <tsoome@me.com> wrote: > >>> > >>>> > >>>> > >>>>> On 20. Jul 2022, at 20:24, Dmitry Chagin <dchagin@heemeyer.club> > >>>> wrote: > >>>>> > >>>>> On Fri, Jul 08, 2022 at 05:50:05PM +0000, Warner Losh wrote: > >>>>>> The branch main has been updated by imp: > >>>>>> > >>>>>> URL: > >>>> > https://cgit.FreeBSD.org/src/commit/?id=84bf2bbbecc369cea6095bed7a738674b27f8d13 > >>>>>> > >>>>>> commit 84bf2bbbecc369cea6095bed7a738674b27f8d13 > >>>>>> Author: Warner Losh <imp@FreeBSD.org> > >>>>>> AuthorDate: 2022-07-08 16:29:25 +0000 > >>>>>> Commit: Warner Losh <imp@FreeBSD.org> > >>>>>> CommitDate: 2022-07-08 17:47:37 +0000 > >>>>>> > >>>>>> stand: constrain zlib/gzip CFLAGS better > >>>>>> > >>>>>> Define ZLIB_CFLAGS and use it only for the sources that are in > >>>> ZLIB or > >>>>>> that include it. > >>>>>> > >>>>>> Sponsored by: Netflix > >>>>>> --- > >>>>>> stand/libsa/Makefile | 13 +++++++------ > >>>>>> 1 file changed, 7 insertions(+), 6 deletions(-) > >>>>>> > >>>>>> diff --git a/stand/libsa/Makefile b/stand/libsa/Makefile > >>>>>> index b5d800c26295..09637bd5e9d4 100644 > >>>>>> --- a/stand/libsa/Makefile > >>>>>> +++ b/stand/libsa/Makefile > >>>>>> @@ -96,9 +96,11 @@ SRCS+=${i} > >>>>>> > >>>>>> # decompression functionality from zlib > >>>>>> .PATH: ${SRCTOP}/sys/contrib/zlib > >>>>>> -CFLAGS+=-DHAVE_MEMCPY -I${SRCTOP}/sys/contrib/zlib > >>>>>> -SRCS+= adler32.c crc32.c > >>>>>> -SRCS+= infback.c inffast.c inflate.c inftrees.c zutil.c > >>>>>> +ZLIB_CFLAGS=-DHAVE_MEMCPY -I${SRCTOP}/sys/contrib/zlib > >>>>>> +.for i in adler32.c crc32.c infback.c inffast.c inflate.c > inftrees.c > >>>> zutil.c > >>>>>> +CFLAGS.${i}+=${ZLIB_CFLAGS} > >>>>>> +SRCS+= ${i} > >>>>>> +.endfor > >>>>>> > >>>>>> # lz4 decompression functionality > >>>>>> .PATH: ${SRCTOP}/sys/cddl/contrib/opensolaris/common/lz4 > >>>>>> @@ -168,9 +170,8 @@ SRCS+= time.c > >>>>>> .PATH: ${SRCTOP}/sys/ufs/ffs > >>>>>> SRCS+=ffs_subr.c ffs_tables.c > >>>>>> > >>>>>> -CFLAGS.dosfs.c+= -I${LDRSRC} > >>>>>> -CFLAGS.tftp.c+= -I${LDRSRC} > >>>>>> -CFLAGS.ufs.c+= -I${LDRSRC} > >>>>> ^^^^^^^^^^^^ is this correct? at least it breaks builds with > >>>>> WITHOUT_LOADER_ZFS and WITHOUT_BOOT probably, see PR/260083 > >>>>> > >>>>> > >>>> > >>>> No, it is not correct. > >>>> > >>> > >>> My change is correct, theoretically. However, there's a layering > >>> violation that means they are needed so it was premature. > >>> > >>> I'll fix a bandaide and do it better when I return from vacation. > >>> > >> > >> Doh! I don't have the right keys loaded in my ssh-agent, so I can't push > >> the change because the port forwarding on my router is broken and I > can't > >> remotely login :( > >> > >> If someone could commit the change I suggested in > >> https://reviews.freebsd.org/D35860 that would be great! > >> > > > > The changes were needed, btw, to limit the scope of CFLAGS for the > OpenZFS > > blake3.c support. However, this one scope limiting shouldn't break that. > > The thing that doesn't make sense to me at all though is that what happens > in practice is that ZFS unconditionally adds -ILDRSRC to the global CFLAGS. > Yes. That's wrong. I missed that. I have another round planned to fix that. I'd rather add one more global kludge, marked as such that I can more easily remove in the next round once the issues are cleared up. > Given that the goal is to limit the proliferation of global options in > CFLAGS, it seems odd to reject changes that add -ILDRSRC to specific files. > Yea, these shouldn't be added to these files, at all. Only due to the layering violations are they needed at all. > My second review for GELI does replace a global CFLAGS change you removed > with instead adding it to specific files which seems to be a step in the > right direction. > Yea. > Further fixing ZFS to not set any global CFLAGS would seem to be a further > improvement that would expose this current breakage even when ZFS is > enabled. However, I don't really see why we shouldn't add the needed > include just to specific files for now rather than the hack in defs.mk. > I'm fine either way. I can undo more stuff later. I just expressed my preference since it would be easy to undo later. > If we later fix libsa to not need headers from the loader (which I agree > with, and might mean that we just need to move some headers or their > contents over to libsa) can remove targeted CFLAGS.foo.c bits for -ILDRSRC > as part of that commit. > True, but that's more work later and more work now. Since I'm out of pocket for a while, go ahead and commit them... I'd have already committed the kludge-for-now fix... Warner [-- Attachment #2 --] <div dir="ltr"><div dir="ltr">Hey John,<br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Jul 20, 2022 at 4:13 PM John Baldwin <<a href="mailto:jhb@freebsd.org">jhb@freebsd.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On 7/20/22 11:20 AM, Warner Losh wrote:<br> > On Wed, Jul 20, 2022 at 12:12 PM Warner Losh <<a href="mailto:imp@bsdimp.com" target="_blank">imp@bsdimp.com</a>> wrote:<br> > <br> >><br> >><br> >> On Wed, Jul 20, 2022 at 12:06 PM Warner Losh <<a href="mailto:imp@bsdimp.com" target="_blank">imp@bsdimp.com</a>> wrote:<br> >><br> >>><br> >>><br> >>> On Wed, Jul 20, 2022 at 11:44 AM Toomas Soome <<a href="mailto:tsoome@me.com" target="_blank">tsoome@me.com</a>> wrote:<br> >>><br> >>>><br> >>>><br> >>>>> On 20. Jul 2022, at 20:24, Dmitry Chagin <dchagin@heemeyer.club><br> >>>> wrote:<br> >>>>><br> >>>>> On Fri, Jul 08, 2022 at 05:50:05PM +0000, Warner Losh wrote:<br> >>>>>> The branch main has been updated by imp:<br> >>>>>><br> >>>>>> URL:<br> >>>> <a href="https://cgit.FreeBSD.org/src/commit/?id=84bf2bbbecc369cea6095bed7a738674b27f8d13" rel="noreferrer" target="_blank">https://cgit.FreeBSD.org/src/commit/?id=84bf2bbbecc369cea6095bed7a738674b27f8d13</a><br>; >>>>>><br> >>>>>> commit 84bf2bbbecc369cea6095bed7a738674b27f8d13<br> >>>>>> Author: Warner Losh <imp@FreeBSD.org><br> >>>>>> AuthorDate: 2022-07-08 16:29:25 +0000<br> >>>>>> Commit: Warner Losh <imp@FreeBSD.org><br> >>>>>> CommitDate: 2022-07-08 17:47:37 +0000<br> >>>>>><br> >>>>>> stand: constrain zlib/gzip CFLAGS better<br> >>>>>><br> >>>>>> Define ZLIB_CFLAGS and use it only for the sources that are in<br> >>>> ZLIB or<br> >>>>>> that include it.<br> >>>>>><br> >>>>>> Sponsored by: Netflix<br> >>>>>> ---<br> >>>>>> stand/libsa/Makefile | 13 +++++++------<br> >>>>>> 1 file changed, 7 insertions(+), 6 deletions(-)<br> >>>>>><br> >>>>>> diff --git a/stand/libsa/Makefile b/stand/libsa/Makefile<br> >>>>>> index b5d800c26295..09637bd5e9d4 100644<br> >>>>>> --- a/stand/libsa/Makefile<br> >>>>>> +++ b/stand/libsa/Makefile<br> >>>>>> @@ -96,9 +96,11 @@ SRCS+=${i}<br> >>>>>><br> >>>>>> # decompression functionality from zlib<br> >>>>>> .PATH: ${SRCTOP}/sys/contrib/zlib<br> >>>>>> -CFLAGS+=-DHAVE_MEMCPY -I${SRCTOP}/sys/contrib/zlib<br> >>>>>> -SRCS+= adler32.c crc32.c<br> >>>>>> -SRCS+= infback.c inffast.c inflate.c inftrees.c zutil.c<br> >>>>>> +ZLIB_CFLAGS=-DHAVE_MEMCPY -I${SRCTOP}/sys/contrib/zlib<br> >>>>>> +.for i in adler32.c crc32.c infback.c inffast.c inflate.c inftrees.c<br> >>>> zutil.c<br> >>>>>> +CFLAGS.${i}+=${ZLIB_CFLAGS}<br> >>>>>> +SRCS+= ${i}<br> >>>>>> +.endfor<br> >>>>>><br> >>>>>> # lz4 decompression functionality<br> >>>>>> .PATH: ${SRCTOP}/sys/cddl/contrib/opensolaris/common/lz4<br> >>>>>> @@ -168,9 +170,8 @@ SRCS+= time.c<br> >>>>>> .PATH: ${SRCTOP}/sys/ufs/ffs<br> >>>>>> SRCS+=ffs_subr.c ffs_tables.c<br> >>>>>><br> >>>>>> -CFLAGS.dosfs.c+= -I${LDRSRC}<br> >>>>>> -CFLAGS.tftp.c+= -I${LDRSRC}<br> >>>>>> -CFLAGS.ufs.c+= -I${LDRSRC}<br> >>>>> ^^^^^^^^^^^^ is this correct? at least it breaks builds with<br> >>>>> WITHOUT_LOADER_ZFS and WITHOUT_BOOT probably, see PR/260083<br> >>>>><br> >>>>><br> >>>><br> >>>> No, it is not correct.<br> >>>><br> >>><br> >>> My change is correct, theoretically. However, there's a layering<br> >>> violation that means they are needed so it was premature.<br> >>><br> >>> I'll fix a bandaide and do it better when I return from vacation.<br> >>><br> >><br> >> Doh! I don't have the right keys loaded in my ssh-agent, so I can't push<br> >> the change because the port forwarding on my router is broken and I can't<br> >> remotely login :(<br> >><br> >> If someone could commit the change I suggested in<br> >> <a href="https://reviews.freebsd.org/D35860" rel="noreferrer" target="_blank">https://reviews.freebsd.org/D35860</a>; that would be great!<br> >><br> > <br> > The changes were needed, btw, to limit the scope of CFLAGS for the OpenZFS<br> > blake3.c support. However, this one scope limiting shouldn't break that.<br> <br> The thing that doesn't make sense to me at all though is that what happens<br> in practice is that ZFS unconditionally adds -ILDRSRC to the global CFLAGS.<br></blockquote><div><br></div><div>Yes. That's wrong. I missed that. I have another round planned to fix that.</div><div><br></div><div>I'd rather add one more global kludge, marked as such that I can more easily</div><div>remove in the next round once the issues are cleared up.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> Given that the goal is to limit the proliferation of global options in<br> CFLAGS, it seems odd to reject changes that add -ILDRSRC to specific files.<br></blockquote><div><br></div><div>Yea, these shouldn't be added to these files, at all. Only due to the layering</div><div>violations are they needed at all.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> My second review for GELI does replace a global CFLAGS change you removed<br> with instead adding it to specific files which seems to be a step in the<br> right direction.<br></blockquote><div>Yea.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> Further fixing ZFS to not set any global CFLAGS would seem to be a further<br> improvement that would expose this current breakage even when ZFS is<br> enabled. However, I don't really see why we shouldn't add the needed<br> include just to specific files for now rather than the hack in <a href="http://defs.mk" rel="noreferrer" target="_blank">defs.mk</a>.<br></blockquote><div><br></div><div>I'm fine either way. I can undo more stuff later. I just expressed my preference</div><div>since it would be easy to undo later.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> If we later fix libsa to not need headers from the loader (which I agree<br> with, and might mean that we just need to move some headers or their<br> contents over to libsa) can remove targeted CFLAGS.foo.c bits for -ILDRSRC<br> as part of that commit.<br></blockquote><div><br></div><div>True, but that's more work later and more work now. Since I'm out of pocket</div><div>for a while, go ahead and commit them... I'd have already committed the kludge-for-now</div><div>fix...</div><div><br></div><div>Warner </div></div></div>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CANCZdfpKndvQ%2BCPY5Q4vj2MiBhysOaxPY56xRWzft9%2BruVq6GQ>