From nobody Wed Jul 20 22:39:09 2022 X-Original-To: dev-commits-src-main@mlmmj.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id 4Lp9cB1ns3z4X7ym for ; Wed, 20 Jul 2022 22:39:22 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Received: from mail-vs1-xe36.google.com (mail-vs1-xe36.google.com [IPv6:2607:f8b0:4864:20::e36]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (2048 bits) client-digest SHA256) (Client CN "smtp.gmail.com", Issuer "GTS CA 1D4" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4Lp9c92pB4z3kJW for ; Wed, 20 Jul 2022 22:39:21 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Received: by mail-vs1-xe36.google.com with SMTP id j65so17671931vsc.3 for ; Wed, 20 Jul 2022 15:39:21 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bsdimp-com.20210112.gappssmtp.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=PGhiFhpNYeanxcPqcf3/5IU7GIPHaClCuSy6ThN6axA=; b=UhAQ2tlPgdCn52OEc4D9et9sJ361EPwmPWLs16LmyuD36FodBk2wY6s7fvh3lzk6hp KA4+MqSEHe9p5wweqQIkp/sbxszS65UepPun07sfGnKHhtWcM5Jk7ZdOdUvhCy9mF6mo MOhhjFYFkGDymP6maz2N+H4yQve6y/7bVXfkJ43QTUynWT0rCYNuPs8M3wi53N/3mfot zuKuFJ+9WQH1roPqj1KteM2PxONfxLb9ZpYX2Qgkm7/qJq64K6aiLnFaw616vmS5/QEl wP7SdCs2aWr1jW3gCIf38iB3Sz/AW6USZmWFJvAVM69IA65Rat9pMoq/cOlRLMC3OAg+ o2qg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=PGhiFhpNYeanxcPqcf3/5IU7GIPHaClCuSy6ThN6axA=; b=nt+9dkluMxB8nTuHkdtZIsHCFP2mCViqBHbpmFzha3B31w2ewC/DzX3CewE3vBOTSv Yqrw2QkJtVd8nFewhLtNJRKnoCvP7lkxQC6YKUKl9xV5MIhe90kShcoInTRUYpZxnIB9 sGuGmJUFnnM1+Q/jo1kgV045hsO5+Amvg14GbaMkQQZObtAyG0RP2UWAnZWoWHYn5hgI oL8B9CJZmlaFYdRvelA1icKnB73bX/ryRDKw/YA7VLQYE1uTUOV0fkJzpOidObBHB6WO 26Q88nrlvNChilRWDIA5SAjbWfRsJmz7OM/I8ShDzNG7oQW9bTzpL7FE68caT20sLy+K vKEg== X-Gm-Message-State: AJIora/bhxx8S94KMc4GL1YKPK2LzmqTfNnNQyMXcDmXoAJbwzuSLHDl u6cWDhJfrxO7oX/lgJYXLPDi0w2sFq0uqRSKmpXYVA== X-Google-Smtp-Source: AGRyM1uwzuuw+dLxMZTaf0ZAD6TV5z/taZXo194MNs+xEpzdBuEDE8Rsw/aepydEDAFZCgdlu0wNMDAXbJ8res0tUTk= X-Received: by 2002:a67:e9cf:0:b0:357:6d9e:d2fb with SMTP id q15-20020a67e9cf000000b003576d9ed2fbmr13699414vso.41.1658356760533; Wed, 20 Jul 2022 15:39:20 -0700 (PDT) List-Id: Commit messages for the main branch of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-main List-Help: List-Post: List-Subscribe: List-Unsubscribe: Sender: owner-dev-commits-src-main@freebsd.org X-BeenThere: dev-commits-src-main@freebsd.org MIME-Version: 1.0 References: <202207081750.268Ho5kZ066824@gitrepo.freebsd.org> <244CD526-C7D0-4D42-9DAB-6EA690DFD3A7@me.com> <3cea890a-3441-4ced-e449-b54494f42a64@FreeBSD.org> In-Reply-To: <3cea890a-3441-4ced-e449-b54494f42a64@FreeBSD.org> From: Warner Losh Date: Wed, 20 Jul 2022 16:39:09 -0600 Message-ID: Subject: Re: git: 84bf2bbbecc3 - main - stand: constrain zlib/gzip CFLAGS better To: John Baldwin Cc: Toomas Soome , Dmitry Chagin , Warner Losh , src-committers , "" , dev-commits-src-main@freebsd.org Content-Type: multipart/alternative; boundary="0000000000009f6dd205e44445a2" X-Rspamd-Queue-Id: 4Lp9c92pB4z3kJW X-Spamd-Bar: -- Authentication-Results: mx1.freebsd.org; dkim=pass header.d=bsdimp-com.20210112.gappssmtp.com header.s=20210112 header.b=UhAQ2tlP; dmarc=none; spf=none (mx1.freebsd.org: domain of wlosh@bsdimp.com has no SPF policy when checking 2607:f8b0:4864:20::e36) smtp.mailfrom=wlosh@bsdimp.com X-Spamd-Result: default: False [-3.00 / 15.00]; NEURAL_HAM_MEDIUM(-1.00)[-1.000]; NEURAL_HAM_LONG(-1.00)[-1.000]; NEURAL_HAM_SHORT(-1.00)[-1.000]; FORGED_SENDER(0.30)[imp@bsdimp.com,wlosh@bsdimp.com]; R_DKIM_ALLOW(-0.20)[bsdimp-com.20210112.gappssmtp.com:s=20210112]; MIME_GOOD(-0.10)[multipart/alternative,text/plain]; MLMMJ_DEST(0.00)[dev-commits-src-main@freebsd.org]; RCVD_IN_DNSWL_NONE(0.00)[2607:f8b0:4864:20::e36:from]; R_SPF_NA(0.00)[no SPF record]; ARC_NA(0.00)[]; RCVD_TLS_LAST(0.00)[]; MIME_TRACE(0.00)[0:+,1:+,2:~]; ASN(0.00)[asn:15169, ipnet:2607:f8b0::/32, country:US]; RCPT_COUNT_SEVEN(0.00)[7]; DKIM_TRACE(0.00)[bsdimp-com.20210112.gappssmtp.com:+]; TO_MATCH_ENVRCPT_SOME(0.00)[]; FROM_HAS_DN(0.00)[]; RCVD_COUNT_TWO(0.00)[2]; FROM_NEQ_ENVFROM(0.00)[imp@bsdimp.com,wlosh@bsdimp.com]; TO_DN_SOME(0.00)[]; DMARC_NA(0.00)[bsdimp.com]; PREVIOUSLY_DELIVERED(0.00)[dev-commits-src-main@freebsd.org]; FREEMAIL_CC(0.00)[me.com,heemeyer.club,freebsd.org] X-ThisMailContainsUnwantedMimeParts: N --0000000000009f6dd205e44445a2 Content-Type: text/plain; charset="UTF-8" Hey John, On Wed, Jul 20, 2022 at 4:13 PM John Baldwin wrote: > On 7/20/22 11:20 AM, Warner Losh wrote: > > On Wed, Jul 20, 2022 at 12:12 PM Warner Losh wrote: > > > >> > >> > >> On Wed, Jul 20, 2022 at 12:06 PM Warner Losh wrote: > >> > >>> > >>> > >>> On Wed, Jul 20, 2022 at 11:44 AM Toomas Soome wrote: > >>> > >>>> > >>>> > >>>>> On 20. Jul 2022, at 20:24, Dmitry Chagin > >>>> 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 > >>>>>> AuthorDate: 2022-07-08 16:29:25 +0000 > >>>>>> Commit: Warner Losh > >>>>>> 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
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@h= eemeyer.club>
>>>> wrote:
>>>>>
>>>>> On Fri, Jul 08, 2022 at 05:50:05PM +0000, Warner Losh = wrote:
>>>>>> The branch main has been updated by imp:
>>>>>>
>>>>>> URL:
>>>> h= ttps://cgit.FreeBSD.org/src/commit/?id=3D84bf2bbbecc369cea6095bed7a738674b2= 7f8d13
>>>>>>
>>>>>> commit 84bf2bbbecc369cea6095bed7a738674b27f8d13 >>>>>> Author:=C2=A0 =C2=A0 =C2=A0Warner Losh <imp@Fre= eBSD.org>
>>>>>> AuthorDate: 2022-07-08 16:29:25 +0000
>>>>>> Commit:=C2=A0 =C2=A0 =C2=A0Warner Losh <imp@Fre= eBSD.org>
>>>>>> CommitDate: 2022-07-08 17:47:37 +0000
>>>>>>
>>>>>>=C2=A0 =C2=A0 =C2=A0stand: constrain zlib/gzip CFLA= GS better
>>>>>>
>>>>>>=C2=A0 =C2=A0 =C2=A0Define ZLIB_CFLAGS and use it o= nly for the sources that are in
>>>> ZLIB or
>>>>>>=C2=A0 =C2=A0 =C2=A0that include it.
>>>>>>
>>>>>>=C2=A0 =C2=A0 =C2=A0Sponsored by:=C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0Netflix
>>>>>> ---
>>>>>> stand/libsa/Makefile | 13 +++++++------
>>>>>> 1 file changed, 7 insertions(+), 6 deletions(-) >>>>>>
>>>>>> diff --git a/stand/libsa/Makefile b/stand/libsa/Ma= kefile
>>>>>> index b5d800c26295..09637bd5e9d4 100644
>>>>>> --- a/stand/libsa/Makefile
>>>>>> +++ b/stand/libsa/Makefile
>>>>>> @@ -96,9 +96,11 @@ SRCS+=3D${i}
>>>>>>
>>>>>> # decompression functionality from zlib
>>>>>> .PATH: ${SRCTOP}/sys/contrib/zlib
>>>>>> -CFLAGS+=3D-DHAVE_MEMCPY -I${SRCTOP}/sys/contrib/z= lib
>>>>>> -SRCS+=3D=C2=A0 =C2=A0 =C2=A0 adler32.c crc32.c >>>>>> -SRCS+=3D=C2=A0 =C2=A0 =C2=A0 infback.c inffast.c = inflate.c inftrees.c zutil.c
>>>>>> +ZLIB_CFLAGS=3D-DHAVE_MEMCPY -I${SRCTOP}/sys/contr= ib/zlib
>>>>>> +.for i in adler32.c crc32.c infback.c inffast.c i= nflate.c inftrees.c
>>>> zutil.c
>>>>>> +CFLAGS.${i}+=3D${ZLIB_CFLAGS}
>>>>>> +SRCS+=3D=C2=A0 =C2=A0 =C2=A0 ${i}
>>>>>> +.endfor
>>>>>>
>>>>>> # lz4 decompression functionality
>>>>>> .PATH: ${SRCTOP}/sys/cddl/contrib/opensolaris/comm= on/lz4
>>>>>> @@ -168,9 +170,8 @@ SRCS+=3D=C2=A0 =C2=A0time.c >>>>>> .PATH: ${SRCTOP}/sys/ufs/ffs
>>>>>> SRCS+=3Dffs_subr.c ffs_tables.c
>>>>>>
>>>>>> -CFLAGS.dosfs.c+=3D -I${LDRSRC}
>>>>>> -CFLAGS.tftp.c+=3D -I${LDRSRC}
>>>>>> -CFLAGS.ufs.c+=3D -I${LDRSRC}
>>>>> ^^^^^^^^^^^^ is this correct? at least it breaks build= s with
>>>>> WITHOUT_LOADER_ZFS and WITHOUT_BOOT probably, see PR/2= 60083
>>>>>
>>>>>
>>>>
>>>> No, it is not correct.
>>>>
>>>
>>> My change is correct, theoretically. However, there's a la= yering
>>> violation that means they are needed so it was premature.
>>>
>>> I'll fix a bandaide and do it better when I return from va= cation.
>>>
>>
>> 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 grea= t!
>>
>
> The changes were needed, btw, to limit the scope of CFLAGS for the Ope= nZFS
> 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 happ= ens
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.
= =C2=A0
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 t= hese files, at all. Only due to the layering
violations are they = needed at all.
=C2=A0
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.
=C2=A0
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
enabled.=C2=A0 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 la= ter. I just expressed my preference
since it would be easy to und= o later.
=C2=A0
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<= br> as part of that commit.

True, but that&= #39;s more work later and more work now. Since I'm out of pocket
<= div>for a while, go ahead and commit them... I'd have already committed= the kludge-for-now
fix...

Warner=C2=A0<= /div>
--0000000000009f6dd205e44445a2--