[PATCH 1/2] MIPS: host_detect_local_cpu, init ret with concat [PR112759]

[PATCH 1/2] MIPS: host_detect_local_cpu, init ret with concat [PR112759]

[YunQiang Su]

The function `reconcat` cannot append string(s) to NULL,
as the concat process will stop at the first NULL.

Let's initialize `ret` with `concat (" ", NULL)`, then
it can be used by reconcat.

gcc/

	PR target/112759
	* config/mips/driver-native.cc (host_detect_local_cpu):
	initialize ret with concat, so that it can be used by
	reconcat later.
---
 gcc/config/mips/driver-native.cc | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gcc/config/mips/driver-native.cc b/gcc/config/mips/driver-native.cc
index afc276f5278..471d1925eff 100644
--- a/gcc/config/mips/driver-native.cc
+++ b/gcc/config/mips/driver-native.cc
@@ -44,7 +44,7 @@ const char *
 host_detect_local_cpu (int argc, const char **argv)
 {
   const char *cpu = NULL;
-  char *ret = NULL;
+  char *ret = concat(" ", NULL);
   char buf[128];
   FILE *f;
   bool arch;
-- 
2.39.2

[PATCH 2/2] libiberty/reconcat: Add note about append string to NULL

[YunQiang Su]

For reconcat, if the `optr` can only be used as the last one
of string list, aka, we cannot append something to it.
Let's add some note into the document.

libiberty:
	* concat.c (reconcat): Add note about append string to NULL
	into document.
---
 libiberty/concat.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/libiberty/concat.c b/libiberty/concat.c
index 4cb1df3baf3..3a6b4ca71e8 100644
--- a/libiberty/concat.c
+++ b/libiberty/concat.c
@@ -169,6 +169,9 @@ loop:
   str = reconcat (str, "pre-", str, NULL);
 @end example
 
+Note: don't try to append string(s) to the a NULL string,
+as the process will stop at the first NULL argument.
+
 @end deftypefn
 
 */
-- 
2.39.2

Re: [PATCH 2/2] libiberty/reconcat: Add note about append string to NULL

[Jakub Jelinek]

On Mon, Dec 18, 2023 at 11:44:22AM +0800, YunQiang Su wrote:
> For reconcat, if the `optr` can only be used as the last one
> of string list, aka, we cannot append something to it.
> Let's add some note into the document.
> 
> libiberty:
> 	* concat.c (reconcat): Add note about append string to NULL
> 	into document.
> ---
>  libiberty/concat.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/libiberty/concat.c b/libiberty/concat.c
> index 4cb1df3baf3..3a6b4ca71e8 100644
> --- a/libiberty/concat.c
> +++ b/libiberty/concat.c
> @@ -169,6 +169,9 @@ loop:
>    str = reconcat (str, "pre-", str, NULL);
>  @end example
>  
> +Note: don't try to append string(s) to the a NULL string,
> +as the process will stop at the first NULL argument.
> +
>  @end deftypefn

I think this is unnecessary and misleading.
The fact that NULL is the variable argument terminator is already clearly
documented, and first argument to reconcat can be NULL just fine,
so one just needs to be careful.

	Jakub

Re: [PATCH 1/2] MIPS: host_detect_local_cpu, init ret with concat [PR112759]

[Jakub Jelinek]

On Mon, Dec 18, 2023 at 11:44:21AM +0800, YunQiang Su wrote:
> The function `reconcat` cannot append string(s) to NULL,
> as the concat process will stop at the first NULL.
> 
> Let's initialize `ret` with `concat (" ", NULL)`, then
> it can be used by reconcat.
> 
> gcc/
> 
> 	PR target/112759
> 	* config/mips/driver-native.cc (host_detect_local_cpu):
> 	initialize ret with concat, so that it can be used by
> 	reconcat later.
> ---
>  gcc/config/mips/driver-native.cc | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/gcc/config/mips/driver-native.cc b/gcc/config/mips/driver-native.cc
> index afc276f5278..471d1925eff 100644
> --- a/gcc/config/mips/driver-native.cc
> +++ b/gcc/config/mips/driver-native.cc
> @@ -44,7 +44,7 @@ const char *
>  host_detect_local_cpu (int argc, const char **argv)
>  {
>    const char *cpu = NULL;
> -  char *ret = NULL;
> +  char *ret = concat(" ", NULL);
>    char buf[128];
>    FILE *f;
>    bool arch;
> -- 
> 2.39.2

The formatting is wrong (no space after concat), but more importantly,
I don't think you want to return " " rather than NULL if there isn't
anything known and it is unnecessary to prefix everything with a space
when
gnu-user.h:  " %{march=native:%<march=native %:local_cpu_detect(arch)}"	\
gnu-user.h:  " %{mtune=native:%<mtune=native %:local_cpu_detect(tune)}"
space is already present.  Given the simple thing the function does,
I think much better would be to avoid any reconcat calls and just do one
concat, i.e.
--- gcc/config/mips/driver-native.cc	2023-01-02 09:32:58.422764590 +0100
+++ gcc/config/mips/driver-native.cc	2023-12-18 09:08:39.547609739 +0100
@@ -44,6 +44,7 @@ const char *
 host_detect_local_cpu (int argc, const char **argv)
 {
   const char *cpu = NULL;
+  const char *nan2008 = "";
   char *ret = NULL;
   char buf[128];
   FILE *f;
@@ -90,7 +91,7 @@ host_detect_local_cpu (int argc, const c
 
 fallback_cpu:
 #if defined (__mips_nan2008)
-  ret = reconcat (ret, " -mnan=2008 ", NULL);
+  nan2008 = "-mnan=2008 ";
 #endif
 
 #ifdef HAVE_GETAUXVAL
@@ -104,7 +105,9 @@ fallback_cpu:
 #endif
 
   if (cpu)
-    ret = reconcat (ret, ret, "-m", argv[0], "=", cpu, NULL);
+    ret = concat (nan2008, "-m", argv[0], "=", cpu, NULL);
+  else if (nan2008)
+    ret = concat (nan2008, NULL);
 
   return ret;
 }


Or, if you really want to use reconcat, drop that space from
before -mnan=2008 and use
    ret = reconcat (ret, ret ? ret : "", "-m", argv[0], "=", cpu, NULL);

	Jakub

Re: [PATCH 1/2] MIPS: host_detect_local_cpu, init ret with concat [PR112759]

[YunQiang Su]

Jakub Jelinek <jakub@redhat.com> 于2023年12月18日周一 16:10写道：
>
> On Mon, Dec 18, 2023 at 11:44:21AM +0800, YunQiang Su wrote:
> > The function `reconcat` cannot append string(s) to NULL,
> > as the concat process will stop at the first NULL.
> >
> > Let's initialize `ret` with `concat (" ", NULL)`, then
> > it can be used by reconcat.
> >
> > gcc/
> >
> >       PR target/112759
> >       * config/mips/driver-native.cc (host_detect_local_cpu):
> >       initialize ret with concat, so that it can be used by
> >       reconcat later.
> > ---
> >  gcc/config/mips/driver-native.cc | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/gcc/config/mips/driver-native.cc b/gcc/config/mips/driver-native.cc
> > index afc276f5278..471d1925eff 100644
> > --- a/gcc/config/mips/driver-native.cc
> > +++ b/gcc/config/mips/driver-native.cc
> > @@ -44,7 +44,7 @@ const char *
> >  host_detect_local_cpu (int argc, const char **argv)
> >  {
> >    const char *cpu = NULL;
> > -  char *ret = NULL;
> > +  char *ret = concat(" ", NULL);
> >    char buf[128];
> >    FILE *f;
> >    bool arch;
> > --
> > 2.39.2
>
> The formatting is wrong (no space after concat), but more importantly,

I will fix the format.

> I don't think you want to return " " rather than NULL if there isn't
> anything known and it is unnecessary to prefix everything with a space
> when
> gnu-user.h:  " %{march=native:%<march=native %:local_cpu_detect(arch)}" \
> gnu-user.h:  " %{mtune=native:%<mtune=native %:local_cpu_detect(tune)}"
> space is already present.  Given the simple thing the function does,
> I think much better would be to avoid any reconcat calls and just do one
> concat, i.e.
> --- gcc/config/mips/driver-native.cc    2023-01-02 09:32:58.422764590 +0100
> +++ gcc/config/mips/driver-native.cc    2023-12-18 09:08:39.547609739 +0100
> @@ -44,6 +44,7 @@ const char *
>  host_detect_local_cpu (int argc, const char **argv)
>  {
>    const char *cpu = NULL;
> +  const char *nan2008 = "";
>    char *ret = NULL;
>    char buf[128];
>    FILE *f;
> @@ -90,7 +91,7 @@ host_detect_local_cpu (int argc, const c
>
>  fallback_cpu:
>  #if defined (__mips_nan2008)
> -  ret = reconcat (ret, " -mnan=2008 ", NULL);
> +  nan2008 = "-mnan=2008 ";
>  #endif
>
>  #ifdef HAVE_GETAUXVAL
> @@ -104,7 +105,9 @@ fallback_cpu:
>  #endif
>
>    if (cpu)
> -    ret = reconcat (ret, ret, "-m", argv[0], "=", cpu, NULL);
> +    ret = concat (nan2008, "-m", argv[0], "=", cpu, NULL);
> +  else if (nan2008)
> +    ret = concat (nan2008, NULL);
>
>    return ret;
>  }
>
>
> Or, if you really want to use reconcat, drop that space from
> before -mnan=2008 and use
>     ret = reconcat (ret, ret ? ret : "", "-m", argv[0], "=", cpu, NULL);
>

Good idea.
Maybe that we can use
ret = reconcat (ret, "-m", argv[0], "=", cpu, ret, NULL);
Then won't worry about ret is NULL here.

One reason I want to use reconcat is that I try to avoid something
wrong in future,
if somebody else add some new detect conditions.
With reconcat, the person will need only copy/paste ;)

It will reduce the possibility for doing something wrong in future.
And let's add a comment here.

>         Jakub
>