From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mout02.posteo.de (mout02.posteo.de [185.67.36.66]) by mx.groups.io with SMTP id smtpd.web11.146.1633628624090037506 for ; Thu, 07 Oct 2021 10:43:45 -0700 Authentication-Results: mx.groups.io; dkim=fail reason="body hash did not verify" header.i=@posteo.de header.s=2017 header.b=WTzK3FA8; spf=pass (domain: posteo.de, ip: 185.67.36.66, mailfrom: mhaeuser@posteo.de) Received: from submission (posteo.de [89.146.220.130]) by mout02.posteo.de (Postfix) with ESMTPS id 26AA3240105 for ; Thu, 7 Oct 2021 19:43:40 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=posteo.de; s=2017; t=1633628621; bh=EGB6WghWHdmoTM3o4BHn5us8O8M/u82OXPHJJcY6Ikk=; h=Date:Subject:To:Cc:From:From; b=WTzK3FA88jwwdrn2pMxZ3alALvUgtPis9Jy2BLim7t64BMO3Z5lwGE8cVQpmNXEIF 4idkHrdrmS+jujXEwHK2TUEaDaBXJQrCHoQ13DAk897OnTPOJB5li+FdMmEACgdBeC pM1QDQenvdO4ozdlWrQgSSdl6Zw80EWHrr1Dyji3IQONKom+9jb9cSb6ElzN3VqS7v Yfz0tCac1t8kSsZ5962Gsw+Gcm/NlU1u1P0iyNTwESEzaI6w595nPjpBh4fp22AJJj mUGuZ9gBTmsYVZAMineNW1HoFLckb9L3qJRuXOU5sVsHYkdUyON+XiXlGNFC3xl4Xo 902D4uwnrlfrw== Received: from customer (localhost [127.0.0.1]) by submission (posteo.de) with ESMTPSA id 4HQJZz3jpXz9rxm; Thu, 7 Oct 2021 19:43:39 +0200 (CEST) Message-ID: Date: Thu, 7 Oct 2021 17:43:39 +0000 MIME-Version: 1.0 Subject: Re: [edk2-devel] Progress on getting Uncrustify working for EDK2? To: Andrew Fish , edk2-devel-groups-io , Mike Kinney Cc: Leif Lindholm , "mikuback@linux.microsoft.com" , "rebecca@nuviainc.com" , Michael Kubacki , Bret Barkelew References: <07a6ecff-f7bf-083a-f24d-246ca6c7988b@nuviainc.com> <2679bfa3-b4ec-d8e9-7e56-54ebe42d9001@posteo.de> <9fe0f984-db9d-9aec-0b44-5d30791a2855@linux.microsoft.com> <20211007104813.wa4rmfsqgcpvnzwt@leviathan> <07d5c8bc-40b2-4e99-3b3d-4c8ac4e14220@posteo.de> <438B4D66-2CFB-45E3-AF75-42342F0B1E67@apple.com> From: =?UTF-8?B?TWFydmluIEjDpHVzZXI=?= In-Reply-To: <438B4D66-2CFB-45E3-AF75-42342F0B1E67@apple.com> Content-Language: en-GB Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: quoted-printable Hey Mike, Hey Andrew, I'll just reply to both mails at once :) On 07/10/2021 19:36, Andrew Fish wrote: > > >> On Oct 7, 2021, at 1:19 PM, Michael D Kinney=20 >> wrote: >> >> Hi Marvin, >> >> Some comments below. >> >> Mike >> >>> -----Original Message----- >>> From:devel@edk2.groups.io On Behalf Of Marvin=20 >>> H=C3=A4user >>> Sent: Thursday, October 7, 2021 11:31 AM >>> To: Leif Lindholm=20 >>> ;devel@edk2.groups.io;mikuback@linux.microsoft.com >>> Cc:rebecca@nuviainc.com; Michael Kubacki=20 >>> ; Bret Barkelew=20 >>> ; >>> Kinney, Michael D >>> Subject: Re: [edk2-devel] Progress on getting Uncrustify working for=20 >>> EDK2? >>> >>> Good day, >>> >>> +1, but while you're at it, can we have arguments not align to the >>> function name... >>> >>> =C2=A0=C2=A0Status =3D Test ( >>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0a >>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0); >>> >>> ... but to the next natural indentation level? >>> >>> =C2=A0=C2=A0Status =3D Test ( >>> =C2=A0=C2=A0=C2=A0=C2=A0a >>> =C2=A0=C2=A0=C2=A0=C2=A0); >>> >>> Basically no IDE I have seen supports EDK II's style, and I wouldn't be >>> keen on writing known-broken style to then rely on Uncrustify to fix it= . >>> >>> I also have heard some controversy regarding casts off-list, where some >>> prefer no spaces after casts to stress the evaluation order, and some >>> prefer spaces to have clearer visuals (as a cast *ideally* would be >>> something rare that requires good justification). Just throwing that ou= t >>> there. >>> >>> >>> For things unrelated to autoformat (so semi-offtopic) but still relevan= t >>> to the coding spec: >>> >>> 1. Allow STATIC functions (if the debugging concerns are still relevant= , >>> there could be another level of indirection, like RELEASE_STATIC)? >> >> Debugging concerns are no longer relevant. =C2=A0The suggestion in the B= Z=20 >> below >> is to remove the STATIC macro and allow EDK II sources to add 'static' >> to any functions it makes sense to use on. >> >> https://bugzilla.tianocore.org/show_bug.cgi?id=3D1766 Thanks! I'd keep STATIC actually just for the sake of not doing no-op=20 changes that do not really do anything and for consistency with CONST,=20 but whatever works really. >> >>> >>> 2. Allow variable assignments on definition (basically non-static CONST >>> variables are banned...)? >> >> Are referring to use of pre-initialized CONST variables declared within >> a function? =C2=A0I think Bret brought this topic up when implementing s= ome >> unit tests and the suggestion to pass ECCC was to promote them to >> pre-initialized CONST global variables. Yes. >> >> The challenges we have seen in the past with pre-initialized=20 >> variables within >> a function is that they can cause compilers to inject use of memcpy()=20 >> calls, >> especially if the variable being initialized on the stack is a structure= . >> These cause build breaks today. > > This issue is independent of CONST. I=E2=80=99m not sure a coding style t= ool=20 > is smart enough to catch this generically? You need an understanding=20 > of C types to know if the local variable assignment is going to=20 > trigger a memcpy(). > > What I=E2=80=99ve seen in the real world is the firmware compiles with -O= s or=20 > LTO to fit int he ROM for DEBUG and RELEASE, and the optimizer=20 > optimizes away the call to memcpy. Then if you try to build NOOPT (or=20 > over ride the compiler flags on an individual driver/lib) you fail to=20 > link as only the NOOPT build injects the memcpy. +1 > > Thus I think the best way to enforce this rule is to compile a project=20 > NOOPT. I=E2=80=99m trying to remember are there flags to built to tell it= to=20 > compile and skip the FD construction? Maybe we should advocate=20 > platforms add a NOOPT build target that just compiles the code, but=20 > does not create the FD? I know there were stability concerns with intrinsics in the past, but=20 memcpy() is in the standard, and the rest remained stable to my=20 knowledge. Maybe it's time to fix the issues at the root? Works for us: https://github.com/acidanthera/OpenCorePkg/tree/master/Library/OcCompilerIn= trinsicsLib Best regards, Marvin > > Thanks, > > Andrew Fish > >> >>> >>> 3. Allow variable declarations at any scope (I had some nasty shadowing >>> bugs before, probably prohibit shadowing with warnings)? >> >> By shadowing do you mean the declaration of the same variable name in >> multiple scoped within the same function? >> >>> >>> 4. Require that exactly all function declarations and all function >>> definitions with no prior declaration must be documented (first >>> direction is enforcing docs, second is prohibiting doc duplication, I'v= e >>> seen them go out-of-sync plenty of times)? >> >> I agree that this can reduce duplication and sync issues. =C2=A0The uncr= ustify >> tool being discussed here could not help clean this up or enforce this >> type of rule. =C2=A0It is a good topic, but may need to be split out int= o its >> own thread. >> >>> >>> The latter bunch would not require any autoformat rules or reformatatio= n >>> of existing code, but would be target only new submissions in my >>> opinion. Thoughts? >>> >>> >>> Thanks for your efforts! >>> >>> Best regards, >>> Marvin >>> >>> >>> Am 07.10.2021 um 12:48 schrieb Leif Lindholm: >>>> Hi Michael, >>>> >>>> Apologies, I've owed you a response (promised off-list) for a while >>>> now. >>>> >>>> First, let me say I hugely appreciate this effort. Apart from aligning >>>> the codebase(s), this will reduce manual reviewing effort >>>> substantially, as well as cutting down on number of rework cycles for >>>> developers. >>>> >>>> Looking at the changes to (well, the comments in) uncrustify, this >>>> seems to be constrained to: >>>> - Newline after '(' for multi-line function calls. >>>> - Dealing with "(("/"))" for DEBUG macros. >>>> - Function pointer typedefs: >>>> =C2=A0=C2=A0- typedef\nEFIAPI >>>> =C2=A0=C2=A0- closing parentheses indentation >>>> >>>> I don't think I've made any secret over the years that I am not a >>>> massive fan of the EDK2 coding style in general. So I think for any >>>> of its quirks that are substantial enough that they require not just >>>> custom configuration but actual new function added to existing code >>>> conformance tools, this would be an excellent point to sanitise the >>>> coding style instead. >>>> >>>> Taking these in order: >>>> >>>> Newline after '(' >>>> ----------------- >>>> I think we already reached a level of flexibility around this, where >>>> we don't actually enforce this (or single argument per >>>> line). Personally, I'd be happy to update the coding style as >>>> required instead. >>>> >>>> DEBUG macro parentheses >>>> ----------------------- >>>> How does uncrustify treat DEBUG macros without this modification? >>>> Do we start getting everything turned into multi-level indented >>>> multi-line statements without this change? >>>> >>>> Function pointer typedefs: >>>> -------------------------- >>>> I don't see that function pointer typedefs need to rigidly follow the >>>> same pattern as the declaration of functions implementing them. Could >>>> we update the coding style (if needed) instead? >>>> >>>> Best Regards, >>>> >>>> Leif >>>> >>>> On Mon, Aug 16, 2021 at 16:00:38 -0400, Michael Kubacki wrote: >>>>> The edk2 branch was created here: >>>>> https://github.com/makubacki/edk2/tree/uncrustify_poc_2 >>>>> >>>>> We have a Project Mu fork with custom changes to the Uncrustify=20 >>>>> tool to help >>>>> comply with EDK II formatting here: >>>>> https://dev.azure.com/projectmu/_git/Uncrustify >>>>> >>>>> The latest information about the status and how to experiment with th= e >>>>> configuration file and the tool are in that fork: >>> https://dev.azure.com/projectmu/Uncrustify/_wiki/wikis/Uncrustify.wiki/= 1/Project-Mu-(EDK-II)-Fork-Readme >>>>> >>>>> That said, I have also finished a CI plugin to run Uncrustify that=20 >>>>> should be >>>>> ready soon to initially deploy in Project Mu. Before doing so, I=20 >>>>> am trying >>>>> to settle on an initial configuration file that less strictly but mor= e >>>>> reliably formats the code than in the examples in those branches. For >>>>> example, remove heuristics that when run against the same set of code >>>>> multiple times can produce different results. An example would be=20 >>>>> a rule >>>>> that reformats code because it exceeds a specified column width on=20 >>>>> one run >>>>> but on the next run that reformatted code triggers a different rule t= o >>>>> further align the code and so on. At least initially, some rules=20 >>>>> might be >>>>> tweaked in a more conservative approach that can be tightened in=20 >>>>> the future. >>>>> Once this configuration file is ready, we will baseline Project Mu=20 >>>>> code as >>>>> an example and turn on the plugin. The CI plugin runs Uncrustify=20 >>>>> against >>>>> modified files and if there's any changes, indicating a formatting >>>>> deviation, the diff chunks are saved in a log so they can be=20 >>>>> viewed as a >>>>> build artifact. >>>>> >>>>> I am making progress on the updated config file and I should be=20 >>>>> able to post >>>>> a "uncrustify_poc_3" branch soon with the results. >>>>> >>>>> Regarding indentation, Marvin is right that Uncrustify cannot=20 >>>>> support edk2 >>>>> indentation style out-of-box. Some changes are made in that fork=20 >>>>> to handle >>>>> the formatting. At this point, it can handle the indentation in=20 >>>>> the cases >>>>> I've seen. Uncrustify does potentially give us the ability to=20 >>>>> massively >>>>> deploy changes across the codebase in case a decision were made to=20 >>>>> change >>>>> the style. >>>>> >>>>> Thanks, >>>>> Michael >>>>> >>>>> On 8/16/2021 3:39 PM, Marvin H=C3=A4user wrote: >>>>>> Hey Rebecca, >>>>>> >>>>>> I think even Uncrustify has issues with the EDK II indentation style= . >>>>>> You might want to check the UEFI Talkbox Discord server, I had a=20 >>>>>> brief >>>>>> chat with Michael about it there. I don't think realistically any=20 >>>>>> tool >>>>>> supports EDK II's indentation style however, so I'd propose it is >>>>>> changed. This could be for new submissions only, or actually the=20 >>>>>> entire >>>>>> codebase could be reformatted at once with a good tool setup.=20 >>>>>> While this >>>>>> screws with git blame, the (to my understanding) decided on CRLF=20 >>>>>> -> LF >>>>>> change does that anyway, so at least two evils could be dealt with i= n >>>>>> one go really. >>>>>> >>>>>> Best regards, >>>>>> Marvin >>>>>> >>>>>> On 16/08/2021 21:34, Rebecca Cran wrote: >>>>>>> >>>>>>> cc devel@ . >>>>>>> >>>>>>> On 8/16/21 1:33 PM, Rebecca Cran wrote: >>>>>>>> >>>>>>>> I noticed a message on Twitter about an idea of using Uncrustify >>>>>>>> for EDK2 instead of the ECC tool, and came across https://www.mail= - >>> archive.com/search?l=3Ddevel@edk2.groups.io&q=3Dsubject:%22Re%5C%3A+%5C= %5Bedk2%5C- >>> devel%5C%5D+TianoCore+Community+Meeting+Minutes+%5C-+2%5C%2F4%22&o=3Dne= west&f=3D1 >>>>>>>> . >>>>>>>> >>>>>>>> I was wondering if there's been any progress on it that I could >>>>>>>> check out? >>>>>>>> >>>>>>>> >>>>>>>> Michael Kubacki: in that message, you said: >>>>>>>> >>>>>>>> "I'm planning to put up a branch that we can use as a reference >>>>>>>> for a conversation around uncrustify in the next couple of >>>>>>>> weeks." >>>>>>>> >>>>>>>> >>>>>>>> Did you end up creating that branch, and if so could you provide >>>>>>>> a link to it please? >>>>>>>> >>>>>>>> >>>>>>>> -- >>>>>>>> Rebecca Cran >>>>>>>> >>>>>>> >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> >>>>> >>>>> >>>>> >>>>> >>>>> >>>>> >>> >>> >>> >>> >> >> >> >>=20 >