From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from ma1-aaemail-dr-lapp02.apple.com (ma1-aaemail-dr-lapp02.apple.com [17.171.2.68]) by mx.groups.io with SMTP id smtpd.web10.2905.1595524519472183079 for ; Thu, 23 Jul 2020 10:15:19 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@apple.com header.s=20180706 header.b=Z3wRbi2L; spf=pass (domain: apple.com, ip: 17.171.2.68, mailfrom: afish@apple.com) Received: from pps.filterd (ma1-aaemail-dr-lapp02.apple.com [127.0.0.1]) by ma1-aaemail-dr-lapp02.apple.com (8.16.0.42/8.16.0.42) with SMTP id 06NHDKlK020267; Thu, 23 Jul 2020 10:15:15 -0700 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=apple.com; h=from : message-id : content-type : mime-version : subject : date : in-reply-to : cc : to : references; s=20180706; bh=H+DJs8yEfdYtF+9lVnnDBJQutwYFIioqPKc6HaD2vQo=; b=Z3wRbi2L2rd/RDQf/yR1EZWrgV0GPA+B5c3Z9cysCGUaVThwcGXdbyql0LbaRVwX1zNa f4gmT9/h4Jh350o5SuAUBoSYn77/FXXNA2izHeDuFZXA/NUR9SA6GpSoxxnfrYeE+jb5 2MzX4t2y8EHlQqreABqKmlNhLZkEOvJ0yc5WEHlNfWouLjdaqS44z2j6KioMEwZpt3tl 2J5sgPhWl/g53C3kqz8hD1v34HZyb1tRn96gPlJVmm4WB5XuHEo/COHF7btIGLGlMbxv eP0KzApQxIhQLUMCBjeiq6s+rD8tmyb/QNTMOyMD9iIi1O/ignZuJgoMnv9Vb58jNzgk ww== Received: from rn-mailsvcp-mta-lapp03.rno.apple.com (rn-mailsvcp-mta-lapp03.rno.apple.com [10.225.203.151]) by ma1-aaemail-dr-lapp02.apple.com with ESMTP id 32bwrtw4et-9 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128 verify=NO); Thu, 23 Jul 2020 10:15:15 -0700 Received: from rn-mailsvcp-mmp-lapp01.rno.apple.com (rn-mailsvcp-mmp-lapp01.rno.apple.com [17.179.253.14]) by rn-mailsvcp-mta-lapp03.rno.apple.com (Oracle Communications Messaging Server 8.1.0.5.20200312 64bit (built Mar 12 2020)) with ESMTPS id <0QDX007IHMLDH7O0@rn-mailsvcp-mta-lapp03.rno.apple.com>; Thu, 23 Jul 2020 10:15:13 -0700 (PDT) Received: from process_milters-daemon.rn-mailsvcp-mmp-lapp01.rno.apple.com by rn-mailsvcp-mmp-lapp01.rno.apple.com (Oracle Communications Messaging Server 8.1.0.5.20200312 64bit (built Mar 12 2020)) id <0QDX00700M8ROH00@rn-mailsvcp-mmp-lapp01.rno.apple.com>; Thu, 23 Jul 2020 10:15:13 -0700 (PDT) X-Va-A: X-Va-T-CD: 678bf7de5df0d9ff994f556fd1b44182 X-Va-E-CD: 2965961a75318aaa9070c24eb58ea84c X-Va-R-CD: a511239d35d2c8e34a6a6c58917a436e X-Va-CD: 0 X-Va-ID: 0f8f328a-7353-4146-91d6-bdd159a56992 X-V-A: X-V-T-CD: 678bf7de5df0d9ff994f556fd1b44182 X-V-E-CD: 2965961a75318aaa9070c24eb58ea84c X-V-R-CD: a511239d35d2c8e34a6a6c58917a436e X-V-CD: 0 X-V-ID: d0285eca-e0f5-4060-9980-60aaf42ac3a2 X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.235,18.0.687 definitions=2020-07-23_09:2020-07-23,2020-07-23 signatures=0 Received: from [17.235.23.115] (unknown [17.235.23.115]) by rn-mailsvcp-mmp-lapp01.rno.apple.com (Oracle Communications Messaging Server 8.1.0.5.20200312 64bit (built Mar 12 2020)) with ESMTPSA id <0QDX00RSLML95800@rn-mailsvcp-mmp-lapp01.rno.apple.com>; Thu, 23 Jul 2020 10:15:11 -0700 (PDT) From: "Andrew Fish" Message-id: MIME-version: 1.0 (Mac OS X Mail 13.4 \(3608.80.23.2.2\)) Subject: Re: [edk2-devel] [edk2-platforms][PATCH 1/1] Platforms/RaspberryPi: Fix BIOS Release Date and System Manufacturer Date: Thu, 23 Jul 2020 10:15:09 -0700 In-reply-to: <20200723154735.GO1337@vanye> Cc: Pete Batard , ard.biesheuvel@arm.com, awarkentin@vmware.com, samer.el-haj-mahmoud@arm.com To: devel@edk2.groups.io, leif@nuviainc.com References: <20200720161507.14352-1-pete@akeo.ie> <20200723110826.GM1337@vanye> <20200723154735.GO1337@vanye> X-Mailer: Apple Mail (2.3608.80.23.2.2) X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.235,18.0.687 definitions=2020-07-23_09:2020-07-23,2020-07-23 signatures=0 Content-type: multipart/alternative; boundary="Apple-Mail=_DEF48B35-C1AD-4802-B143-FD67323E07BB" --Apple-Mail=_DEF48B35-C1AD-4802-B143-FD67323E07BB Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 > On Jul 23, 2020, at 8:47 AM, Leif Lindholm wrote: >=20 > Hi Pete, >=20 > On Thu, Jul 23, 2020 at 15:38:55 +0100, Pete Batard wrote: >>>> @@ -626,6 +627,28 @@ BIOSInfoUpdateSmbiosType0 ( >>>> INTN i; >>>> INTN State =3D 0; >>>> INTN Value[2]; >>>> + INTN Year =3D (__DATE__[7] =3D=3D '?' ? 1900 \ >>>> + : (((__DATE__[7] - '0') * 1000 ) \ >>>> + + (__DATE__[8] - '0') * 100 \ >>>> + + (__DATE__[9] - '0') * 10 \ >>>> + + __DATE__[10] - '0')); >>>> + INTN Month =3D ( __DATE__ [2] =3D=3D '?' ? 1 \ >>>> + : __DATE__ [2] =3D=3D 'n' ? ( \ >>>> + __DATE__ [1] =3D=3D 'a' ? 1 : 6) \ >>>> + : __DATE__ [2] =3D=3D 'b' ? 2 \ >>>> + : __DATE__ [2] =3D=3D 'r' ? ( \ >>>> + __DATE__ [0] =3D=3D 'M' ? 3 : 4) \ >>>> + : __DATE__ [2] =3D=3D 'y' ? 5 \ >>>> + : __DATE__ [2] =3D=3D 'l' ? 7 \ >>>> + : __DATE__ [2] =3D=3D 'g' ? 8 \ >>>> + : __DATE__ [2] =3D=3D 'p' ? 9 \ >>>> + : __DATE__ [2] =3D=3D 't' ? 10 \ >>>> + : __DATE__ [2] =3D=3D 'v' ? 11 \ >>>> + : 12); >>>> + INTN Day =3D ( __DATE__[4] =3D=3D '?' ? 1 \ >>>> + : ((__DATE__[4] =3D=3D ' ' ? 0 : \ >>>> + ((__DATE__[4] - '0') * 10)) \ >>>> + + __DATE__[5] - '0')); >>>=20 >>> So, this hunk is very neat, but nigh-on unreviewable. >>> I.e. we should defintely have it - but only once. >>>=20 >>> Could you break this up into some macros to go into some generic >>> helper lib? (I don't have a better idea than EmbeddedPkg TimeBaseLib, >>> but then that is already included in this module.) >>=20 >> So you would like to have a set of macros like: >> TIME_GET_BUILD_YEAR, TIME_GET_BUILD_MONTH, TIME_GET_BUILD_DAY in >> TimeBaseLib.h that perform the above? >>=20 >>> Would you be OK to break that snippet out separate? >>=20 >> I think that's a good idea, especially as there is a potential underlyi= ng >> issue with the __DATE__ format being specific to each compiler, so we >> probably want to handle compiler detection somewhere, preferably global= ly. >> For instance, the Intel compiler's __DATE__ format would not work with = the >> above, so I'll add some "vetted compiler" detection for the macros. >=20 > Yes, thanks. > A friendly "your compiler probably won't know what to do with this" > #error would be friendlier than whatever non-GCC/clang would generate > on encountering this. >=20 Leif, I was thinking the "Mmm dd yyyy=E2=80=9D form of __DATE__ was standard. Ma= ybe all we need is a compile time assert that checks the format? 6.10.8.1 Mandatory macros 1 The following macro names shall be de=EF=AC=81= ned by the implementation: _ _DATE_ _ The date of translation of the prepro= cessing translation unit: a character string literal of the form "Mmm dd yy= yy", where the names of the months are the same as those generated by the a= sctime function, and the =EF=AC=81rst character of dd is a space character = if the value is less than 10. If the date of translation is not available, = an implementation-de=EF=AC=81ned valid date shall be supplied=20 It seems likely some compilers had an alternate version and chose not to c= hange it and break existing code when the format got standardized. You woul= d hope those compilers had some kind of optional strict ANSI flag or some s= uch=E2=80=A6. Thanks, Andrew Fish >> The one thing I am not planning to look into is that, of course, as lon= g as >> you don't explicitly force the compiler to rebuild the sources where th= ese >> macros are used, then you may end up with a very obsolete build date >> compared to the actual date of your last re-build. >=20 > Yeah, that's fine. >=20 >> But that's mostly because >> I don't think there exists a generic solution we can ise to force >> recompilation of a file that uses a specific macro and also because our= main >> goal with these is to ensure that the Pi firmwares, that we produce thr= ough >> AppVeyor, have a proper build date, and AppVeyor builds are are always >> clean. >=20 > Yeah, I really don't care about that. > Or the flipside, where someone modifies only (say) the Smbios > populating code a month after they built the rest of the tree. >=20 > If someone wants to put a *specific* date in, that should be done > manually, and if someone wants information on what was actually built, > they should put in the hashes of the top commits of all repositories > on PACKAGES_PATH as well as whether the state of each was clean. >=20 > Managing these bits through a rigid build process, like you are, is > very much valid. >=20 >> I'll send an EDK2 patch for the macros, and then a revised patch for th= is >> when I get a chance. >=20 > Thanks! >=20 > Regards, >=20 > Leif >=20 >> Regards, >>=20 >> /Pete >>=20 >>>=20 >>> / >>> Leif >>>=20 >>>> // Populate the Firmware major and minor. >>>> Status =3D mFwProtocol->GetFirmwareRevision (&EpochSeconds); >>>> @@ -648,6 +671,10 @@ BIOSInfoUpdateSmbiosType0 ( >>>> mBiosVendor, sizeof (mBiosVendor)); >>>> UnicodeStrToAsciiStrS ((CHAR16*)PcdGetPtr (PcdFirmwareVersionStrin= g), >>>> mBiosVersion, sizeof (mBiosVersion)); >>>> + ASSERT (Year >=3D 0 && Year <=3D 9999); >>>> + ASSERT (Month >=3D 1 && Month <=3D 12); >>>> + ASSERT (Day >=3D 1 && Day <=3D 31); >>>> + AsciiSPrint (mBiosDate, sizeof (mBiosDate), "%02d/%02d/%04d", Mont= h, Day, Year); >>>> // Look for a "x.y" numeric string anywhere in mBiosVersion and >>>> // try to parse it to populate the BIOS major and minor. >>>> --=20 >>>> 2.21.0.windows.1 >>>>=20 >>=20 >=20 >=20 >=20 --Apple-Mail=_DEF48B35-C1AD-4802-B143-FD67323E07BB Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8

On Jul 23, 2= 020, at 8:47 AM, Leif Lindholm <leif@nuviainc.com> wrote:

Hi Pete,

On Thu, Jul 23, 2020 at 15:38:55 +0100, Pete Batard wrote:
@@ -626,6 +627,28 @@ BIOSInfoUp= dateSmbiosType0 (
   INTN   i;
   INTN   State =3D 0;
&= nbsp;  INTN   Value[2];
+  INTN &nbs= p; Year =3D (__DATE__[7] =3D=3D '?' ? 1900  \
+ &nb= sp;         : (((__DATE__[7] -= '0') * 1000 ) \
+        =    + (__DATE__[8] - '0') * 100      = ;\
+          &n= bsp;+ (__DATE__[9] - '0') * 10       \
+           + __D= ATE__[10] - '0'));
+  INTN   Month =3D ( __DAT= E__ [2] =3D=3D '?' ? 1  \
+     &nbs= p;     : __DATE__ [2] =3D=3D 'n' ? (   &= nbsp;    \
+     &nbs= p;       __DATE__ [1] =3D=3D 'a' ? 1 : 6= )   \
+        &= nbsp;  : __DATE__ [2] =3D=3D 'b' ? 2     &nbs= p;  \
+        &= nbsp;  : __DATE__ [2] =3D=3D 'r' ? (     &nbs= p;  \
+        &= nbsp;    __DATE__ [0] =3D=3D 'M' ? 3 : 4)   \=
+          &nbs= p;: __DATE__ [2] =3D=3D 'y' ? 5        \=
+          &nbs= p;: __DATE__ [2] =3D=3D 'l' ? 7        \=
+          &nbs= p;: __DATE__ [2] =3D=3D 'g' ? 8        \=
+          &nbs= p;: __DATE__ [2] =3D=3D 'p' ? 9        \=
+          &nbs= p;: __DATE__ [2] =3D=3D 't' ? 10       \
+           : _= _DATE__ [2] =3D=3D 'v' ? 11       \
+           : 12);=
+  INTN   Day =3D ( __DATE__[4] =3D=3D '?' ? = 1     \
+      &= nbsp;    : ((__DATE__[4] =3D=3D ' ' ? 0 :   &= nbsp; \
+        &nbs= p;    ((__DATE__[4] - '0') * 10))    \+           = + __DATE__[5] - '0'));

So, this h= unk is very neat, but nigh-on unreviewable.
I.e. we should de= fintely have it - but only once.

Could you bre= ak this up into some macros to go into some generic
helper li= b? (I don't have a better idea than EmbeddedPkg TimeBaseLib,
= but then that is already included in this module.)

So you would like to have a set of macros like:
TIME_GET_BUILD_YEAR, TIME_GET_BUILD_MONTH, TIME_GET_BUILD_DAY in
TimeBaseLib.h that perform the above?

Would you be OK to break that snipp= et out separate?

I think that's a= good idea, especially as there is a potential underlying
iss= ue with the __DATE__ format being specific to each compiler, so we
probably want to handle compiler detection somewhere, preferably glo= bally.
For instance, the Intel compiler's __DATE__ format wou= ld not work with the
above, so I'll add some "vetted compiler= " detection for the macros.

Yes, = thanks.
A friendly "your compiler probably won't know what to= do with this"
#error would be friendlier than whatever non-G= CC/clang would generate
on encountering this.
<= br class=3D"">

Leif,=

I was thinking the "Mmm dd yyyy=E2=80= =9D form of __DATE__ was standard. Maybe all we need is a compile time ass= ert that checks the format?

6= .10.8.1 Mandatory macros 1 The following macro names shall be de=EF=AC=81ne= d by the implementation: _ _DATE_ _ The date of translation of the preproce= ssing translation unit: a character string literal of the form "Mmm dd yyyy= ", where the names of the months are the same as those generated by the asc= time function, and the =EF=AC=81rst character of dd is a space character if= the value is less than 10. If the date of translation is not available, an= implementation-de=EF=AC=81ned valid date shall be supplied 
<= div>
It seems likely some compilers had an alterna= te version and chose not to change it and break existing code when the form= at got standardized. You would hope those compilers had some kind of option= al strict ANSI flag or some such=E2=80=A6.

<= div>Thanks,

Andrew Fish

The one thing I= am not planning to look into is that, of course, as long as
= you don't explicitly force the compiler to rebuild the sources where these<= br class=3D"">macros are used, then you may end up with a very obsolete bui= ld date
compared to the actual date of your last re-build.

Yeah, that's fine.
But that's mostly because=
I don't think there exists a generic solution we can ise to = force
recompilation of a file that uses a specific macro and = also because our main
goal with these is to ensure that the P= i firmwares, that we produce through
AppVeyor, have a proper = build date, and AppVeyor builds are are always
clean.

Yeah, I really don't care about that.Or the flipside, where someone modifies only (say) the Smbiospopulating code a month after they built the rest of the tree.=

If someone wants to put a *specific* date in,= that should be done
manually, and if someone wants informati= on on what was actually built,
they should put in the hashes = of the top commits of all repositories
on PACKAGES_PATH as we= ll as whether the state of each was clean.

Man= aging these bits through a rigid build process, like you are, is
very much valid.

I'll send an EDK2 patch for the macros, and then a revised p= atch for this
when I get a chance.

Thanks!

Regards,

Leif

Regards,

/Pete


/
    Leif

   // Populate the Firmware major a= nd minor.
   Status =3D mFwProtocol->GetFi= rmwareRevision (&EpochSeconds);
@@ -648,6 +671,10 @@ BIOS= InfoUpdateSmbiosType0 (
     mBiosV= endor, sizeof (mBiosVendor));
   UnicodeStrTo= AsciiStrS ((CHAR16*)PcdGetPtr (PcdFirmwareVersionString),
&n= bsp;    mBiosVersion, sizeof (mBiosVersion));
+  ASSERT (Year >=3D 0 && Year <=3D 9999);
+  ASSERT (Month >=3D 1 && Month <=3D 12);
+  ASSERT (Day >=3D 1 && Day <=3D 31);
+  AsciiSPrint (mBiosDate, sizeof (mBiosDate), "%02d/%02d/%04d"= , Month, Day, Year);
   // Look for a "x.y" n= umeric string anywhere in mBiosVersion and
   = ;// try to parse it to populate the BIOS major and minor.
-- =
2.21.0.windows.1

<= /blockquote>




--Apple-Mail=_DEF48B35-C1AD-4802-B143-FD67323E07BB--