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
--0000000000009f6dd205e44445a2 Content-Type: text/plain; charset="UTF-8" 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 --0000000000009f6dd205e44445a2 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable <div dir=3D"ltr"><div dir=3D"ltr">Hey John,<br></div><br><div class=3D"gmai= l_quote"><div dir=3D"ltr" class=3D"gmail_attr">On Wed, Jul 20, 2022 at 4:13= PM John Baldwin <<a href=3D"mailto:jhb@freebsd.org">jhb@freebsd.org</a>= > wrote:<br></div><blockquote class=3D"gmail_quote" style=3D"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=3D"mailto:imp= @bsdimp.com" target=3D"_blank">imp@bsdimp.com</a>> wrote:<br> > <br> >><br> >><br> >> On Wed, Jul 20, 2022 at 12:06 PM Warner Losh <<a href=3D"mailto= :imp@bsdimp.com" target=3D"_blank">imp@bsdimp.com</a>> wrote:<br> >><br> >>><br> >>><br> >>> On Wed, Jul 20, 2022 at 11:44 AM Toomas Soome <<a href=3D"m= ailto:tsoome@me.com" target=3D"_blank">tsoome@me.com</a>> wrote:<br> >>><br> >>>><br> >>>><br> >>>>> On 20. Jul 2022, at 20:24, Dmitry Chagin <dchagin@h= eemeyer.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=3D"https://cgit.FreeBSD.org/src/commit/?id=3D84bf2= bbbecc369cea6095bed7a738674b27f8d13" rel=3D"noreferrer" target=3D"_blank">h= ttps://cgit.FreeBSD.org/src/commit/?id=3D84bf2bbbecc369cea6095bed7a738674b2= 7f8d13</a><br> >>>>>><br> >>>>>> commit 84bf2bbbecc369cea6095bed7a738674b27f8d13<br= > >>>>>> Author:=C2=A0 =C2=A0 =C2=A0Warner Losh <imp@Fre= eBSD.org><br> >>>>>> AuthorDate: 2022-07-08 16:29:25 +0000<br> >>>>>> Commit:=C2=A0 =C2=A0 =C2=A0Warner Losh <imp@Fre= eBSD.org><br> >>>>>> CommitDate: 2022-07-08 17:47:37 +0000<br> >>>>>><br> >>>>>>=C2=A0 =C2=A0 =C2=A0stand: constrain zlib/gzip CFLA= GS better<br> >>>>>><br> >>>>>>=C2=A0 =C2=A0 =C2=A0Define ZLIB_CFLAGS and use it o= nly for the sources that are in<br> >>>> ZLIB or<br> >>>>>>=C2=A0 =C2=A0 =C2=A0that include it.<br> >>>>>><br> >>>>>>=C2=A0 =C2=A0 =C2=A0Sponsored by:=C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0Netflix<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/Ma= kefile<br> >>>>>> index b5d800c26295..09637bd5e9d4 100644<br> >>>>>> --- a/stand/libsa/Makefile<br> >>>>>> +++ b/stand/libsa/Makefile<br> >>>>>> @@ -96,9 +96,11 @@ SRCS+=3D${i}<br> >>>>>><br> >>>>>> # decompression functionality from zlib<br> >>>>>> .PATH: ${SRCTOP}/sys/contrib/zlib<br> >>>>>> -CFLAGS+=3D-DHAVE_MEMCPY -I${SRCTOP}/sys/contrib/z= lib<br> >>>>>> -SRCS+=3D=C2=A0 =C2=A0 =C2=A0 adler32.c crc32.c<br= > >>>>>> -SRCS+=3D=C2=A0 =C2=A0 =C2=A0 infback.c inffast.c = inflate.c inftrees.c zutil.c<br> >>>>>> +ZLIB_CFLAGS=3D-DHAVE_MEMCPY -I${SRCTOP}/sys/contr= ib/zlib<br> >>>>>> +.for i in adler32.c crc32.c infback.c inffast.c i= nflate.c inftrees.c<br> >>>> zutil.c<br> >>>>>> +CFLAGS.${i}+=3D${ZLIB_CFLAGS}<br> >>>>>> +SRCS+=3D=C2=A0 =C2=A0 =C2=A0 ${i}<br> >>>>>> +.endfor<br> >>>>>><br> >>>>>> # lz4 decompression functionality<br> >>>>>> .PATH: ${SRCTOP}/sys/cddl/contrib/opensolaris/comm= on/lz4<br> >>>>>> @@ -168,9 +170,8 @@ SRCS+=3D=C2=A0 =C2=A0time.c<br= > >>>>>> .PATH: ${SRCTOP}/sys/ufs/ffs<br> >>>>>> SRCS+=3Dffs_subr.c ffs_tables.c<br> >>>>>><br> >>>>>> -CFLAGS.dosfs.c+=3D -I${LDRSRC}<br> >>>>>> -CFLAGS.tftp.c+=3D -I${LDRSRC}<br> >>>>>> -CFLAGS.ufs.c+=3D -I${LDRSRC}<br> >>>>> ^^^^^^^^^^^^ is this correct? at least it breaks build= s with<br> >>>>> WITHOUT_LOADER_ZFS and WITHOUT_BOOT probably, see PR/2= 60083<br> >>>>><br> >>>>><br> >>>><br> >>>> No, it is not correct.<br> >>>><br> >>><br> >>> My change is correct, theoretically. However, there's a la= yering<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 va= cation.<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=3D"https://reviews.freebsd.org/D35860" rel=3D"noreferrer" = target=3D"_blank">https://reviews.freebsd.org/D35860</a>; that would be grea= t!<br> >><br> > <br> > The changes were needed, btw, to limit the scope of CFLAGS for the Ope= nZFS<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 happ= ens<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</d= iv><div>remove in the next round once the issues are cleared up.</div><div>= =C2=A0</div><blockquote class=3D"gmail_quote" style=3D"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 t= hese files, at all. Only due to the layering</div><div>violations are they = needed at all.</div><div>=C2=A0</div><blockquote class=3D"gmail_quote" styl= e=3D"margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);paddin= g-left:1ex"> My second review for GELI does replace a global CFLAGS change you removed<b= r> with instead adding it to specific files which seems to be a step in the<br= > right direction.<br></blockquote><div>Yea.</div><div>=C2=A0</div><blockquot= e class=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-left:1px s= olid 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.=C2=A0 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=3D"h= ttp://defs.mk" rel=3D"noreferrer" target=3D"_blank">defs.mk</a>.<br></block= quote><div><br></div><div>I'm fine either way. I can undo more stuff la= ter. I just expressed my preference</div><div>since it would be easy to und= o later.</div><div>=C2=A0</div><blockquote class=3D"gmail_quote" style=3D"m= argin: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&= #39;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=C2=A0<= /div></div></div> --0000000000009f6dd205e44445a2--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CANCZdfpKndvQ%2BCPY5Q4vj2MiBhysOaxPY56xRWzft9%2BruVq6GQ>