[edk2-devel] [PATCH] MdePkg/BaseFdtLib: Rename standard functions

[edk2-devel] [PATCH] MdePkg/BaseFdtLib: Rename standard functions

[Jeff Brasen via groups.io]

Rename the standard functions in the LibFdtSupport to remove conflicts
with other libraries that define them.

Signed-off-by: Jeff Brasen <jbrasen@nvidia.com>
---
 MdePkg/Library/BaseFdtLib/LibFdtSupport.h | 16 +++++++++++++++
 MdePkg/Library/BaseFdtLib/LibFdtWrapper.c | 25 ++---------------------
 2 files changed, 18 insertions(+), 23 deletions(-)

diff --git a/MdePkg/Library/BaseFdtLib/LibFdtSupport.h b/MdePkg/Library/BaseFdtLib/LibFdtSupport.h
index 393019324b..47beac9fac 100644
--- a/MdePkg/Library/BaseFdtLib/LibFdtSupport.h
+++ b/MdePkg/Library/BaseFdtLib/LibFdtSupport.h
@@ -68,6 +68,12 @@ strrchr    (
   int
   );
 
+char *
+fdt_strrchr    (
+  const char *,
+  int
+  );
+
 unsigned long
 strtoul     (
   const char *,
@@ -75,6 +81,13 @@ strtoul     (
   int
   );
 
+unsigned long
+fdt_strtoul     (
+  const char *,
+  char **,
+  int
+  );
+
 char *
 strcpy (
   char        *strDest,
@@ -93,7 +106,10 @@ strcpy (
 #define strnlen(str, count)                 (size_t)(AsciiStrnLenS(str, count))
 #define strncpy(strDest, strSource, count)  AsciiStrnCpyS(strDest, MAX_STRING_SIZE, strSource, (UINTN)count)
 #define strcat(strDest, strSource)          AsciiStrCatS(strDest, MAX_STRING_SIZE, strSource)
+#define strchr(str, ch)                     ScanMem8(str, AsciiStrSize (str), (UINT8)ch)
 #define strcmp(string1, string2, count)     (int)(AsciiStrCmp(string1, string2))
 #define strncmp(string1, string2, count)    (int)(AsciiStrnCmp(string1, string2, (UINTN)(count)))
+#define strrchr(str, ch)                    fdt_strrchr(str, ch)
+#define strtoul(ptr, end_ptr, base)         fdt_strtoul(ptr, end_ptr, base)
 
 #endif /* FDT_LIB_SUPPORT_H_ */
diff --git a/MdePkg/Library/BaseFdtLib/LibFdtWrapper.c b/MdePkg/Library/BaseFdtLib/LibFdtWrapper.c
index ef6452914f..1a4cd573fd 100644
--- a/MdePkg/Library/BaseFdtLib/LibFdtWrapper.c
+++ b/MdePkg/Library/BaseFdtLib/LibFdtWrapper.c
@@ -18,28 +18,7 @@
 // so the code gets a bit clunky to handle that case specifically.
 
 char *
-strchr (
-  const char  *Str,
-  int         Char
-  )
-{
-  char  *S;
-
-  S = (char *)Str;
-
-  for ( ; ; S++) {
-    if (*S == Char) {
-      return S;
-    }
-
-    if (*S == '\0') {
-      return NULL;
-    }
-  }
-}
-
-char *
-strrchr (
+fdt_strrchr (
   const char  *Str,
   int         Char
   )
@@ -71,7 +50,7 @@ __isspace (
 }
 
 unsigned long
-strtoul (
+fdt_strtoul (
   const char  *Nptr,
   char        **EndPtr,
   int         Base
-- 
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112308): https://edk2.groups.io/g/devel/message/112308
Mute This Topic: https://groups.io/mt/103110792/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] MdePkg/BaseFdtLib: Rename standard functions

[Pedro Falcato]

On Mon, Dec 11, 2023 at 3:40 PM Jeff Brasen via groups.io
<jbrasen=nvidia.com@groups.io> wrote:
>

Jeff,

You're missing CC's on this patch. Also, you should probably send the
3 patches in a single series, since they're all related.

> Rename the standard functions in the LibFdtSupport to remove conflicts
> with other libraries that define them.

This is a funny problem. What error were you seeing? As far as I can
tell, you can totally define your local C library functions, it
shouldn't result in any linker errors (even if, IIRC, deemed UB by the
C spec).

--
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112314): https://edk2.groups.io/g/devel/message/112314
Mute This Topic: https://groups.io/mt/103110792/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] MdePkg/BaseFdtLib: Rename standard functions

[Jeff Brasen via groups.io]

Hit send before adding the cc on this one. (Would probably be good to get that added to here (https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Development-Process) so copy paste doesn't get folks 😊.

I sent them as different patch series as I thought this one might cause some discussion and wanted to separate it from that. 

This issue I was seeing is if you both include FdtLib and BaseCryptLib they both contain implementations of these standard functions with other functions in the same c files. This results in a link error as the linker won't discard part of a compilation unit if it is used so if you have

Foo.c has functions func1 and func2
Bar.c has functions func3 and func2

If func3 and func1 are both used externally that will cause both objects to be included and the linker to complain that func2 is defined twice.

We could move these to c files with just 1 function each in both libraries but that seemed like a bigger change than this.

-Jeff


> -----Original Message-----
> From: Pedro Falcato <pedro.falcato@gmail.com>
> Sent: Monday, December 11, 2023 9:00 AM
> To: devel@edk2.groups.io; Jeff Brasen <jbrasen@nvidia.com>
> Subject: Re: [edk2-devel] [PATCH] MdePkg/BaseFdtLib: Rename standard
> functions
> 
> External email: Use caution opening links or attachments
> 
> 
> On Mon, Dec 11, 2023 at 3:40 PM Jeff Brasen via groups.io
> <jbrasen=nvidia.com@groups.io> wrote:
> >
> 
> Jeff,
> 
> You're missing CC's on this patch. Also, you should probably send the
> 3 patches in a single series, since they're all related.
> 
> > Rename the standard functions in the LibFdtSupport to remove conflicts
> > with other libraries that define them.
> 
> This is a funny problem. What error were you seeing? As far as I can tell, you
> can totally define your local C library functions, it shouldn't result in any linker
> errors (even if, IIRC, deemed UB by the C spec).
> 
> --
> Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112319): https://edk2.groups.io/g/devel/message/112319
Mute This Topic: https://groups.io/mt/103110792/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] MdePkg/BaseFdtLib: Rename standard functions

[Jeff Brasen via groups.io]

CC the maintainers which I missed in the email on this originally.

> -----Original Message-----
> From: Jeff Brasen <jbrasen@nvidia.com>
> Sent: Monday, December 11, 2023 9:53 AM
> To: Pedro Falcato <pedro.falcato@gmail.com>; devel@edk2.groups.io
> Subject: RE: [edk2-devel] [PATCH] MdePkg/BaseFdtLib: Rename standard
> functions
> 
> Hit send before adding the cc on this one. (Would probably be good to get
> that added to here
> (https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-
> Development-Process) so copy paste doesn't get folks 😊.
> 
> I sent them as different patch series as I thought this one might cause some
> discussion and wanted to separate it from that.
> 
> This issue I was seeing is if you both include FdtLib and BaseCryptLib they
> both contain implementations of these standard functions with other
> functions in the same c files. This results in a link error as the linker won't
> discard part of a compilation unit if it is used so if you have
> 
> Foo.c has functions func1 and func2
> Bar.c has functions func3 and func2
> 
> If func3 and func1 are both used externally that will cause both objects to be
> included and the linker to complain that func2 is defined twice.
> 
> We could move these to c files with just 1 function each in both libraries but
> that seemed like a bigger change than this.
> 
> -Jeff
> 
> 
> > -----Original Message-----
> > From: Pedro Falcato <pedro.falcato@gmail.com>
> > Sent: Monday, December 11, 2023 9:00 AM
> > To: devel@edk2.groups.io; Jeff Brasen <jbrasen@nvidia.com>
> > Subject: Re: [edk2-devel] [PATCH] MdePkg/BaseFdtLib: Rename standard
> > functions
> >
> > External email: Use caution opening links or attachments
> >
> >
> > On Mon, Dec 11, 2023 at 3:40 PM Jeff Brasen via groups.io
> > <jbrasen=nvidia.com@groups.io> wrote:
> > >
> >
> > Jeff,
> >
> > You're missing CC's on this patch. Also, you should probably send the
> > 3 patches in a single series, since they're all related.
> >
> > > Rename the standard functions in the LibFdtSupport to remove
> > > conflicts with other libraries that define them.
> >
> > This is a funny problem. What error were you seeing? As far as I can
> > tell, you can totally define your local C library functions, it
> > shouldn't result in any linker errors (even if, IIRC, deemed UB by the C
> spec).
> >
> > --
> > Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114223): https://edk2.groups.io/g/devel/message/114223
Mute This Topic: https://groups.io/mt/103110792/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] MdePkg/BaseFdtLib: Rename standard functions

[Michael D Kinney]

Hi Jeff,

One comment below.

Mike

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Jeff
> Brasen via groups.io
> Sent: Monday, December 11, 2023 7:40 AM
> To: devel@edk2.groups.io
> Cc: Jeff Brasen <jbrasen@nvidia.com>
> Subject: [edk2-devel] [PATCH] MdePkg/BaseFdtLib: Rename standard
> functions
> 
> Rename the standard functions in the LibFdtSupport to remove conflicts
> with other libraries that define them.
> 
> Signed-off-by: Jeff Brasen <jbrasen@nvidia.com>
> ---
>  MdePkg/Library/BaseFdtLib/LibFdtSupport.h | 16 +++++++++++++++
>  MdePkg/Library/BaseFdtLib/LibFdtWrapper.c | 25 ++---------------------
>  2 files changed, 18 insertions(+), 23 deletions(-)
> 
> diff --git a/MdePkg/Library/BaseFdtLib/LibFdtSupport.h
> b/MdePkg/Library/BaseFdtLib/LibFdtSupport.h
> index 393019324b..47beac9fac 100644
> --- a/MdePkg/Library/BaseFdtLib/LibFdtSupport.h
> +++ b/MdePkg/Library/BaseFdtLib/LibFdtSupport.h
> @@ -68,6 +68,12 @@ strrchr    (
>    int
>    );
> 
> +char *
> +fdt_strrchr    (
> +  const char *,
> +  int
> +  );
> +
>  unsigned long
>  strtoul     (
>    const char *,
> @@ -75,6 +81,13 @@ strtoul     (
>    int
>    );

Since stroul() is defined to something else, is the function prototype required?
Same comment for strrchr()

> 
> +unsigned long
> +fdt_strtoul     (
> +  const char *,
> +  char **,
> +  int
> +  );
> +
>  char *
>  strcpy (
>    char        *strDest,
> @@ -93,7 +106,10 @@ strcpy (
>  #define strnlen(str, count)                 (size_t)(AsciiStrnLenS(str,
> count))
>  #define strncpy(strDest, strSource, count)  AsciiStrnCpyS(strDest,
> MAX_STRING_SIZE, strSource, (UINTN)count)
>  #define strcat(strDest, strSource)          AsciiStrCatS(strDest,
> MAX_STRING_SIZE, strSource)
> +#define strchr(str, ch)                     ScanMem8(str, AsciiStrSize
> (str), (UINT8)ch)
>  #define strcmp(string1, string2, count)     (int)(AsciiStrCmp(string1,
> string2))
>  #define strncmp(string1, string2, count)    (int)(AsciiStrnCmp(string1,
> string2, (UINTN)(count)))
> +#define strrchr(str, ch)                    fdt_strrchr(str, ch)
> +#define strtoul(ptr, end_ptr, base)         fdt_strtoul(ptr, end_ptr,
> base)
> 
>  #endif /* FDT_LIB_SUPPORT_H_ */
> diff --git a/MdePkg/Library/BaseFdtLib/LibFdtWrapper.c
> b/MdePkg/Library/BaseFdtLib/LibFdtWrapper.c
> index ef6452914f..1a4cd573fd 100644
> --- a/MdePkg/Library/BaseFdtLib/LibFdtWrapper.c
> +++ b/MdePkg/Library/BaseFdtLib/LibFdtWrapper.c
> @@ -18,28 +18,7 @@
>  // so the code gets a bit clunky to handle that case specifically.
> 
>  char *
> -strchr (
> -  const char  *Str,
> -  int         Char
> -  )
> -{
> -  char  *S;
> -
> -  S = (char *)Str;
> -
> -  for ( ; ; S++) {
> -    if (*S == Char) {
> -      return S;
> -    }
> -
> -    if (*S == '\0') {
> -      return NULL;
> -    }
> -  }
> -}
> -
> -char *
> -strrchr (
> +fdt_strrchr (
>    const char  *Str,
>    int         Char
>    )
> @@ -71,7 +50,7 @@ __isspace (
>  }
> 
>  unsigned long
> -strtoul (
> +fdt_strtoul (
>    const char  *Nptr,
>    char        **EndPtr,
>    int         Base
> --
> 2.34.1
> 
> 
> 
> 
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114232): https://edk2.groups.io/g/devel/message/114232
Mute This Topic: https://groups.io/mt/103110792/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-