[edk2-devel] [edk2-libc Patch 1/1] ek2-libc: Enhance StdLib for supporting Aarch64 and ARM

[Jayaprakash, N]

Thanks Pedro.

These are pretty old PRs and some of them have valuable contributions to edk2-libc. 
Not every PR owner is interested in submitting the email patch for the PRs. 
Mike and Rebecca are aware of this. We had a discussion couple of Months ago on processing these PRs. 
I have volunteered to send email patches on behalf of these old PRs raised on edk2-libc over last few years.

Regarding the patch currently under review, I have extracted the patch from PR https://github.com/tianocore/edk2-libc/pull/3) and submitted it for review without any changes of mine added to it to retain the sanctity of the original changes.
 
All the changes are owned by Tyler Erickson <tyler.erickson at seagate.com> and he has added his consent in the PR https://github.com/tianocore/edk2-libc/pull/3 
I agree this could have been submitted as multiple patches but since I am not the original author I don't wanted to make changes to this patch. 
Since the original author of the changes has already given consent through Signed-off-by clause in the PR, It doesn't require my signature.
Also while merging the change the commit log message would be edited to reflect the author correctly. 

Adding Tyler Erickson <tyler.erickson at seagate.com> to this review discussions so that he can take appropriate action on the comments. 
@Tyler Erickson Would you want to take a look at these comments and make changes accordingly.

Regards,
JP
-----Original Message-----
From: Pedro Falcato <pedro.falcato at gmail.com> 
Sent: Saturday, October 21, 2023 1:57 AM
To: devel at edk2.groups.io; Jayaprakash, N <n.jayaprakash at intel.com>
Cc: Rebecca Cran <rebecca at bsdio.com>; Kinney, Michael D <michael.d.kinney at intel.com>; Tyler Erickson <tyler.erickson at seagate.com>; Ard Biesheuvel <ardb at kernel.org>; Leif Lindholm <llindhol at qti.qualcomm.com>
Subject: Re: [edk2-devel] [edk2-libc Patch 1/1] ek2-libc: Enhance StdLib for supporting Aarch64 and ARM

On Fri, Oct 20, 2023 at 3:04 PM Jayaprakash, N <n.jayaprakash at intel.com> wrote:
>
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4570
>
> This commit is for processing the below PR on edk2-libc repo
> https://github.com/tianocore/edk2-libc/pull/3
> These are the changes introduced to StdLib to build an application for 
> the UEFI shell.
> Added format macros for int types to Aarch64, ARM, and Ia32.
> Also modified the X64 macros so that everything would build when they 
> are used.
> Added some macros that can be used for compatibility that define when 
> socklen_t has been defined.
> Added getopt_long parser from OpenBSD to provide long and short option 
> parsing capability with getopt.

This patch is unreviewable. You'd think (from the subject) that it's an ARM centric change, but it ends up being a whole squashed up sequence of unrelated changes.
Please separate this into one commit per change.
>
> Cc: Rebecca Cran <rebecca at bsdio.com>
> Cc: Michael D Kinney <michael.d.kinney at intel.com>
> Cc: Jayaprakash N <n.jayaprakash at intel.com>
> Signed-off-by: Tyler Erickson <tyler.erickson at seagate.com>

AIUI, if this is Tyler's commit, you need a From: Tyler Erickson <tyler.erickson at seagate.com> (since this is his commit?). Also probably your own Signed-off-by, I'm not sure.

Lastly, you can't just take NetBSD's headers like this for one simple
reason: UNIX systems have used LP64 for ages, Windows systems use LLP64. What does this mean?
UNIX (and thus, code compiled using gcc or clang
linux/netbsd/whatever) has sizeof(long) = 8 for 64-bit systems, whereas in MSVC sizeof(long) = 4.

So macros like:
> #define PRIdPTR     "ld"    /* intptr_t     */
> #define PRIuPTR     "lu"    /* uintptr_t     */

etc, are not correct on MSVC.

Tip: since it's pretty safe, you can probably have two headers: 32-bit architectures (ILP32, should not change between MSVC and GCC/clang) and 64-bit architectures (LP64 on GCC/clang, LLP64 on MSVC). The same goes for the other type-related headers (I have *no* idea if those are correct).

--
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109882): https://edk2.groups.io/g/devel/message/109882
Mute This Topic: https://groups.io/mt/102081650/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-