From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=pass header.i=@apple.com header.s=20180706 header.b=XxYPXOSQ; spf=pass (domain: apple.com, ip: 17.151.62.68, mailfrom: afish@apple.com) Received: from nwk-aaemail-lapp03.apple.com (nwk-aaemail-lapp03.apple.com [17.151.62.68]) by groups.io with SMTP; Fri, 16 Aug 2019 15:59:02 -0700 Received: from pps.filterd (nwk-aaemail-lapp03.apple.com [127.0.0.1]) by nwk-aaemail-lapp03.apple.com (8.16.0.27/8.16.0.27) with SMTP id x7GMuwOV064852; Fri, 16 Aug 2019 15:59:00 -0700 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=apple.com; h=sender : from : message-id : content-type : mime-version : subject : date : in-reply-to : cc : to : references; s=20180706; bh=f0A6JybAn2ysZbiXDpgN3COsojqktupjgYXkmakeAsk=; b=XxYPXOSQVbpo6JgPv/0l7z39u2oSLCNCtdd2KewZzPDlkb7E1IFgz2uwhofDTxSCs5rc Gn0JNPMBEjkGjQTT5cyc09hvukw9iBkIjBta0bHN1ZaHUci0z0STZJLeAHV/dvJ+yojI TUxdPxdDzPzqNPNNJ2oaba5j2euys2VZfMVW7Tjh8y6BSYuk/rv1JRNPaMhOhJyr+qU5 00/Mz7AjLj1KvPLPQDgvWsb9bEbtLm/lM4udqUye1xb3v13hvUokBXIbsV3A800p8VTz 1bhF8ioP/pdj8GJeXbH9bXvMmDnD8IMMJVT8YZ4Et8QSpjavQriNHfh4IJCTcul2zIiL Cw== Received: from mr2-mtap-s01.rno.apple.com (mr2-mtap-s01.rno.apple.com [17.179.226.133]) by nwk-aaemail-lapp03.apple.com with ESMTP id 2uae5ntsph-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128 verify=NO); Fri, 16 Aug 2019 15:58:59 -0700 Received: from nwk-mmpp-sz09.apple.com (nwk-mmpp-sz09.apple.com [17.128.115.80]) by mr2-mtap-s01.rno.apple.com (Oracle Communications Messaging Server 8.0.2.4.20190507 64bit (built May 7 2019)) with ESMTPS id <0PWC006TTQIBT970@mr2-mtap-s01.rno.apple.com>; Fri, 16 Aug 2019 15:58:59 -0700 (PDT) Received: from process_milters-daemon.nwk-mmpp-sz09.apple.com by nwk-mmpp-sz09.apple.com (Oracle Communications Messaging Server 8.0.2.4.20190507 64bit (built May 7 2019)) id <0PWC00C00QCXTO00@nwk-mmpp-sz09.apple.com>; Fri, 16 Aug 2019 15:58:59 -0700 (PDT) X-Va-A: X-Va-T-CD: e65e81296e009a2c770ee052678c11ce X-Va-E-CD: 41f1a2311feb45054f42613e6105d552 X-Va-R-CD: 539ae9ddee3f082d32a832ea31d0c7f2 X-Va-CD: 0 X-Va-ID: 2de15d51-419c-4191-909d-f7be83459650 X-V-A: X-V-T-CD: e65e81296e009a2c770ee052678c11ce X-V-E-CD: 41f1a2311feb45054f42613e6105d552 X-V-R-CD: 539ae9ddee3f082d32a832ea31d0c7f2 X-V-CD: 0 X-V-ID: 00a983f0-1555-4f37-b9cf-dffd1e072406 X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:,, definitions=2019-08-16_10:,, signatures=0 Received: from [17.235.18.156] (unknown [17.235.18.156]) by nwk-mmpp-sz09.apple.com (Oracle Communications Messaging Server 8.0.2.4.20190507 64bit (built May 7 2019)) with ESMTPSA id <0PWC00FZZQI9JGB0@nwk-mmpp-sz09.apple.com>; Fri, 16 Aug 2019 15:58:59 -0700 (PDT) Sender: afish@apple.com From: "Andrew Fish" Message-id: <308CCC50-2275-4E0F-B468-464F814C20A4@apple.com> MIME-version: 1.0 (Mac OS X Mail 12.4 \(3445.104.11\)) Subject: Re: [edk2-devel] [PATCH v2 1/1] MdePkg: Add STATIC_ASSERT macro Date: Fri, 16 Aug 2019 15:58:57 -0700 In-reply-to: Cc: Mike Kinney , "lersek@redhat.com" , "leif.lindholm@linaro.org" To: devel@edk2.groups.io, vit9696@protonmail.com References: <20190813081644.53963-1-vit9696@protonmail.com> <65C2153C-7D59-490C-8DD2-A48FF0EEA8DE@protonmail.com> <74D8A39837DF1E4DA445A8C0B3885C503F75D776@shsmsx102.ccr.corp.intel.com> <4A89E2EF3DFEDB4C8BFDE51014F606A14E4D1378@SHSMSX104.ccr.corp.intel.com> <8e766773-0b4e-e9bd-31a2-a858c7b476c9@redhat.com> X-Mailer: Apple Mail (2.3445.104.11) X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:,, definitions=2019-08-16_10:,, signatures=0 Content-type: multipart/alternative; boundary="Apple-Mail=_5BA412C7-BA35-451E-87E7-C207DA533A7A" --Apple-Mail=_5BA412C7-BA35-451E-87E7-C207DA533A7A Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 > On Aug 16, 2019, at 2:40 PM, Vitaly Cheptsov via Groups.Io wrote: >=20 > Mike, >=20 > I missed your message while writing mine, but I am afraid I disagree wit= h the functional macro usage for this feature. >=20 > I explicitly quoted C standard static_assert definition in one of my pre= vious messages, and I want EDK II to be as close to standard C as possible. >=20 > This will avoid a lot of confusion for newcomers and will let us later a= dopt a more flexible single and double argument interface when it gets stan= dardised. >=20 > For these reasons altogether, I am not positive the macro could get a do= xygen documentation as it is not designed to have any arguments in the firs= t place. >=20 Vitaly, I don't understand your logic? It is always possible to write a comment in= C code? In terms of the C standard and the EFI type system we kind of have a long = history of how the code ended up like it is. The (U)EFI spec defined its ow= n type system (and ABI) as a way of specifying interoperability so the code= got built on top of that. 20 years ago we shied away from having and EFI c= ode base produce definitions of standard C things as we wanted to make it e= asier to import chunks of code in OS loaders that needed to get ported to E= FI from lots of different vendors. Also one of the early compilers that we = standardized on was VC2003 and that does not even fully support C99. For so= me reason it seems VC2008 was also a popular target for some time. I don't = think VC++ got around to C99 until 2013/2015. So that is kind how the edk2 = ended up with its own type system.=20 I'm all for modernization of the C code as long we are thoughtful about co= mpatibility. For example I still see that VS2008 is a supported BaseTools/C= onf/tools_def.template. Thanks, Andrew Fish > Best wishes, > Vitaly >=20 > On =D1=81=D0=B1, =D0=B0=D0=B2=D0=B3. 17, 2019 at 00:04, Kinney, Michael = D > wrote: >>=20 >> Laszlo, >>=20 >> I agree that better comments/documentation of STATIC_ASSERT() >> for EDK II usages is required. For example, EDK II defines >> the ASSERT() macro which is based on the standard C function >> assert(), but we still document it fully for EDK II usage. >>=20 >> /** >> Macro that calls DebugAssert() if an expression evaluates to FALSE. >>=20 >> If MDEPKG_NDEBUG is not defined and the DEBUG_PROPERTY_DEBUG_ASSERT_ENA= BLED >> bit of PcdDebugProperyMask is set, then this macro evaluates the Boolea= n >> expression specified by Expression. If Expression evaluates to FALSE, t= hen >> DebugAssert() is called passing in the source filename, source line num= ber, >> and Expression. >>=20 >> @param Expression Boolean expression. >>=20 >> **/ >> #if !defined(MDEPKG_NDEBUG) >> #define ASSERT(Expression) \ >> do { \ >> if (DebugAssertEnabled ()) { \ >> if (!(Expression)) { \ >> _ASSERT (Expression); \ >> ANALYZER_UNREACHABLE (); \ >> } \ >> } \ >> } while (FALSE) >> #else >> #define ASSERT(Expression) >> #endif >>=20 >> I would like to see the macro documentation for >> STATIC_ASSERT() with the Doxygen style description of the >> parameters. The fact I asked if STATIC_ASSERT() supported >> the 2nd message parameter should have been a trigger >> for me to ask for the more complete macro comment block. >> The fact that this macro can be directly mapped to >> built in compiler name makes the implementation simple, >> but other implementations are possible for compilers >> that do not support this feature directly. This is why >> the complete description of the EDK II version is required. >>=20 >> I would like to see a V3 with the complete description. >>=20 >> In general, I agree it is better if there is code that >> uses a new feature in the code base, so the feature can >> be tested. A second option we can consider going forward >> is for unit test code to be submitted with a new feature, >> so even if there are no consumers from the EDK II repos, >> the feature can still be tested as the EDK II repos are >> maintained. A third option is for community members to >> provide Tested-by responses to the feature along with >> statements in the Bugzilla that clearly documents how the >> the feature was tested. >>=20 >> Best regards, >>=20 >> Mike >>=20 >> > -----Original Message----- >> > From: devel@edk2.groups.io >> > [mailto:devel@edk2.groups.io] On Behalf Of Laszlo Ersek >> > Sent: Friday, August 16, 2019 12:39 PM >> > To: vit9696@protonmail.com >> > Cc: devel@edk2.groups.io; leif.lindholm@linaro.org; >> > afish@apple.com >> > Subject: Re: [edk2-devel] [PATCH v2 1/1] MdePkg: Add >> > STATIC_ASSERT macro >> > >> > On 08/16/19 19:23, vit9696@protonmail.com wrote: >> > > Laszlo, >> > > >> > > I have already mentioned that the documentation is >> > sufficient as >> > > _Static_assert is C standard >> > >> > Yes, in a release of the ISO C standard that edk2 does >> > not target. >> > >> > In addition, edk2 already has several restrictions in >> > place against standards-conformant code. (Such as bit- >> > shifting of UINT64 values, initialization of structures >> > with automatic storage duration, structure assignment, >> > maybe more.) >> > >> > > so I do not plan to make a V3 for this patch. >> > >> > I find that regrettable. >> > >> > > The patch is merge ready. >> > >> > Such statements are usually made when people that >> > comment on a patch arrive at a consensus. The patch may >> > be merge-ready from your perspective and from Mike's. >> > It is not merge-ready from my perspective. >> > I hope I'm allowed to comment (constructively) on >> > patches that aren't strictly aimed at the subsystems I >> > co-maintain. >> > >> > > As for usage examples I have an opposing opinion to >> > yours and believe >> > > it is based on very good reasons. Not using >> > STATIC_ASSERT in the >> > > current release will make the feature optionally >> > available and let >> > > people test it in their setups. >> > >> > Not using STATIC_ASSERT in the current stable release >> > makes the STATIC_ASSERT macro definition *dead code* in >> > edk2 proper. I understand that edk2 is a "kit", and >> > quite explicitly caters to out-of-tree platforms. >> > That's not a positive trait of edk2 however; it's a >> > negative one, in my judgement. Whatever we add to the >> > core of edk2, we should exercise as diligently as we >> > can *inside* of edk2. >> > >> > > In case they notice it does not work for them they >> > will have 3 months >> > > grace period to report it to us and consider making a >> > change. >> > >> > That is what the feature freezes are for. The feature >> > is reviewed before the soft feature freeze, merged (at >> > the latest) during the soft feature freeze, and bugs >> > can still be fixed during the hard feature freeze. The >> > community is expected to test diligently during the >> > hard feature freeze. >> > Perhaps we should extend the hard feature freeze. >> > >> > My problem is not that the change is not "in your >> > face". I'm all for avoiding regressions. My problem is >> > that the code is dead and untestable without platform >> > changes, even though it could be put to great use in >> > core code at once. If you think that's too risky, this >> > close to the stable tag, then one solution is to >> > resubmit at the beginning of the next development cycle >> > (again with additional patches that utilize the >> > STATIC_ASSERT macro at once). Developers will then have >> > close to three months to report and fix issues. >> > >> > Another solution would be to conditionally keep >> > VERIFY_SIZE_OF, vs. >> > using STATIC_ASSERT, for expressing the build-time >> > invariants. The default would be STATIC_ASSERT. Should >> > it break, people could immediately switch back to >> > VERIFY_SIZE_OF, without disruption to their workflows. >> > >> > We've done similar things in OvmfPkg in the past. For >> > example: >> > - USE_LEGACY_ISA_STACK (commit a06810229618 / commit >> > 562688707145), >> > - USE_OLD_BDS (commit 79c098b6d25d / commit >> > dd43486577b3), >> > - USE_OLD_PCI_HOST (commit 4014885ffdfa / commit >> > cef83a3050e5). >> > >> > > This will also give them 3 months grace period of >> > VERIFY_SIZE_OF macro >> > > removal in favour of STATIC_ASSERT. Making the change >> > now will let >> > > people do seamless transition to the new feature and >> > will avoid >> > > obstacles you are currently trying to create. >> > >> > Please stop making claims in bad faith. I'm not trying >> > to "create obstacles". I'm a fan of STATIC_ASSERT. I'm >> > not a fan of dead code. >> > >> > > Thus STATIC_ASSERT usage and VERIFY_SIZE_OF removal >> > must both be >> > > separate patchsets with potentially separate BZs. >> > > >> > > Thanks for understanding, >> > >> > Why are you presenting this as a done deal? The v2 >> > patch was submitted three days ago, IIUC. >> > >> > Also, I wish we could have this discussion without >> > condescension. >> > >> > Thanks, >> > Laszlo >> > >> >=20 >>=20 >=20 >=20 >=20 --Apple-Mail=_5BA412C7-BA35-451E-87E7-C207DA533A7A Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8

On Aug 16, 2= 019, at 2:40 PM, Vitaly Cheptsov via Groups.Io <vit9696=3Dprotonmail.com@groups.io= > wrote:

Mike,

I missed your message while writing mine, but I am afraid I disagree= with the functional macro usage for this feature.

I explicitly quoted C standard static_assert definition in one of my= previous messages, and I want EDK II to be as close to standard C as possi= ble.

This will avoid a lot of confusion for = newcomers and will let us later adopt a more flexible single and double arg= ument interface when it gets standardised.

F= or these reasons altogether, I am not positive the macro could get a doxyge= n documentation as it is not designed to have any arguments in the first pl= ace.


Vitaly,

I don't understand your logic? It is always possible to w= rite a comment in C code?

In terms of t= he C standard and the EFI type system we kind of have a long history of how= the code ended up like it is. The (U)EFI spec defined its own type system = (and ABI) as a way of specifying interoperability so the code got built on = top of that. 20 years ago we shied away from having and EFI code base produ= ce definitions of standard C things as we wanted to make it easier to impor= t chunks of code in OS loaders that needed to get ported to EFI from lots o= f different vendors. Also one of the early compilers that we standardized o= n was VC2003 and that does not even fully support C99. For some reason it s= eems VC2008 was also a popular target for some time. I don't think VC++ got= around to C99 until 2013/2015. So that is kind how the edk2 ended up with = its own type system. 

I'm all for = modernization of the C code as long we are thoughtful about compatibility. = For example I still see that VS2008 is a supported BaseTools/Conf/tool= s_def.template.

Thanks,

Andrew Fish


Best wishes,
Vitaly

On =D1=81=D0=B1, =D0=B0=D0=B2=D0=B3.= 17, 2019 at 00:04, Kinney, Michael D <michael.d= .kinney@intel.com> wrote:
Laszlo,

I agree that better comments/do= cumentation of STATIC_ASSERT()
for EDK II usages is required.= For example, EDK II defines
the ASSERT() macro which is base= d on the standard C function
assert(), but we still document = it fully for EDK II usage.

/**
M= acro that calls DebugAssert() if an expression evaluates to FALSE.

If MDEPKG_NDEBUG is not defined and the DEBUG_PROPERT= Y_DEBUG_ASSERT_ENABLED
bit of PcdDebugProperyMask is set, the= n this macro evaluates the Boolean
expression specified by Ex= pression. If Expression evaluates to FALSE, then
DebugAssert(= ) is called passing in the source filename, source line number,
and Expression.

@param Expression Boolean e= xpression.

**/
#if !defined(MDEP= KG_NDEBUG)
#define ASSERT(Expression) \
do { \<= br class=3D"">if (DebugAssertEnabled ()) { \
if (!(Expression= )) { \
_ASSERT (Expression); \
ANALYZER_UNREACH= ABLE (); \
} \
} \
} while (FALSE= )
#else
#define ASSERT(Expression)
#endif

I would like to see the macro documen= tation for
STATIC_ASSERT() with the Doxygen style description= of the
parameters. The fact I asked if STATIC_ASSERT() suppo= rted
the 2nd message parameter should have been a trigger
for me to ask for the more complete macro comment block.
The fact that this macro can be directly mapped to
bui= lt in compiler name makes the implementation simple,
but othe= r implementations are possible for compilers
that do not supp= ort this feature directly. This is why
the complete descripti= on of the EDK II version is required.

I would = like to see a V3 with the complete description.

In general, I agree it is better if there is code that
uses= a new feature in the code base, so the feature can
be tested= . A second option we can consider going forward
is for unit t= est code to be submitted with a new feature,
so even if there= are no consumers from the EDK II repos,
the feature can stil= l be tested as the EDK II repos are
maintained. A third optio= n is for community members to
provide Tested-by responses to = the feature along with
statements in the Bugzilla that clearl= y documents how the
the feature was tested.
Best regards,

Mike
<= br class=3D"">> -----Original Message-----
> From: devel@edk2.groups.io
> [mailto:= devel@edk2.groups.io] On Behalf Of Laszlo Ersek
> Sent= : Friday, August 16, 2019 12:39 PM
> To: vit9696@protonmail.com
> Cc: devel@edk2.gr= oups.io; leif.li= ndholm@linaro.org;
> afish@apple.com
> Subject: Re: [edk2-deve= l] [PATCH v2 1/1] MdePkg: Add
> STATIC_ASSERT macro
>
> On 08/16/19 19:23, vit9696@protonmail.com wrote:
> > Laszlo,
> >
> > I have = already mentioned that the documentation is
> sufficient a= s
> > _Static_assert is C standard
>> Yes, in a release of the ISO C standard that edk2 does> not target.
>
> In addit= ion, edk2 already has several restrictions in
> place agai= nst standards-conformant code. (Such as bit-
> shifting of= UINT64 values, initialization of structures
> with automa= tic storage duration, structure assignment,
> maybe more.)=
>
> > so I do not plan to make a V3 f= or this patch.
>
> I find that regrettabl= e.
>
> > The patch is merge ready.
>
> Such statements are usually made when pe= ople that
> comment on a patch arrive at a consensus. The = patch may
> be merge-ready from your perspective and from = Mike's.
> It is not merge-ready from my perspective.
> I hope I'm allowed to comment (constructively) on
> patches that aren't strictly aimed at the subsystems I
> co-maintain.
>
> > As for usage= examples I have an opposing opinion to
> yours and believ= e
> > it is based on very good reasons. Not using
> STATIC_ASSERT in the
> > current release = will make the feature optionally
> available and let
> > people test it in their setups.
>
> Not using STATIC_ASSERT in the current stable release
> makes the STATIC_ASSERT macro definition *dead code* in
> edk2 proper. I understand that edk2 is a "kit", and
> quite explicitly caters to out-of-tree platforms.
>= ; That's not a positive trait of edk2 however; it's a
> ne= gative one, in my judgement. Whatever we add to the
> core= of edk2, we should exercise as diligently as we
> can *in= side* of edk2.
>
> > In case they noti= ce it does not work for them they
> will have 3 months
> > grace period to report it to us and consider making a<= br class=3D"">> change.
>
> That is wh= at the feature freezes are for. The feature
> is reviewed = before the soft feature freeze, merged (at
> the latest) d= uring the soft feature freeze, and bugs
> can still be fix= ed during the hard feature freeze. The
> community is expe= cted to test diligently during the
> hard feature freeze.<= br class=3D"">> Perhaps we should extend the hard feature freeze.
>
> My problem is not that the change is not "i= n your
> face". I'm all for avoiding regressions. My probl= em is
> that the code is dead and untestable without platf= orm
> changes, even though it could be put to great use in=
> core code at once. If you think that's too risky, this<= br class=3D"">> close to the stable tag, then one solution is to
> resubmit at the beginning of the next development cycle
> (again with additional patches that utilize the
&= gt; STATIC_ASSERT macro at once). Developers will then have
&= gt; close to three months to report and fix issues.
>
> Another solution would be to conditionally keep
> VERIFY_SIZE_OF, vs.
> using STATIC_ASSERT, for expr= essing the build-time
> invariants. The default would be S= TATIC_ASSERT. Should
> it break, people could immediately = switch back to
> VERIFY_SIZE_OF, without disruption to the= ir workflows.
>
> We've done similar thin= gs in OvmfPkg in the past. For
> example:
&g= t; - USE_LEGACY_ISA_STACK (commit a06810229618 / commit
> = 562688707145),
> - USE_OLD_BDS (commit 79c098b6d25d / comm= it
> dd43486577b3),
> - USE_OLD_PCI_HOST = (commit 4014885ffdfa / commit
> cef83a3050e5).
>
> > This will also give them 3 months grace per= iod of
> VERIFY_SIZE_OF macro
> > remo= val in favour of STATIC_ASSERT. Making the change
> now wi= ll let
> > people do seamless transition to the new fea= ture and
> will avoid
> > obstacles yo= u are currently trying to create.
>
> Ple= ase stop making claims in bad faith. I'm not trying
> to "= create obstacles". I'm a fan of STATIC_ASSERT. I'm
> not a= fan of dead code.
>
> > Thus STATIC_A= SSERT usage and VERIFY_SIZE_OF removal
> must both be
> > separate patchsets with potentially separate BZs.
> >
> > Thanks for understanding,
>
> Why are you presenting this as a done deal?= The v2
> patch was submitted three days ago, IIUC.
>
> Also, I wish we could have this discussion = without
> condescension.
>
= > Thanks,
> Laszlo
>
>= ; 


=


--Apple-Mail=_5BA412C7-BA35-451E-87E7-C207DA533A7A--