[PATCH v2] MIPS: Put the ret to the end of args of reconcat [PR112759]

[PATCH v2] MIPS: Put the ret to the end of args of reconcat [PR112759]

[YunQiang Su]

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

Let's always put the `ret` to the end, as it may be NULL.
We keep use reconcat here, due to that reconcat can make it
easier if we add more hardware features detecting, for example
by hwcap.

gcc/

        PR target/112759
        * config/mips/driver-native.cc (host_detect_local_cpu):
	Put the ret to the end of args of reconcat.
---
 gcc/config/mips/driver-native.cc | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/gcc/config/mips/driver-native.cc b/gcc/config/mips/driver-native.cc
index afc276f5278..9a224b3f401 100644
--- a/gcc/config/mips/driver-native.cc
+++ b/gcc/config/mips/driver-native.cc
@@ -44,6 +44,8 @@ const char *
 host_detect_local_cpu (int argc, const char **argv)
 {
   const char *cpu = NULL;
+  /* Don't assigne any static string to ret.  If you need to do so,
+     use concat.  */
   char *ret = NULL;
   char buf[128];
   FILE *f;
@@ -90,7 +92,8 @@ host_detect_local_cpu (int argc, const char **argv)
 
 fallback_cpu:
 #if defined (__mips_nan2008)
-  ret = reconcat (ret, " -mnan=2008 ", NULL);
+  /* Put the ret to the end of list, since it maybe NULL.  */
+  ret = reconcat (ret, "-mnan=2008", ret, NULL);
 #endif
 
 #ifdef HAVE_GETAUXVAL
@@ -104,7 +107,7 @@ fallback_cpu:
 #endif
 
   if (cpu)
-    ret = reconcat (ret, ret, "-m", argv[0], "=", cpu, NULL);
+    ret = reconcat (ret, "-m", argv[0], "=", cpu, ret, NULL);
 
   return ret;
 }
-- 
2.39.2

Re: [PATCH v2] MIPS: Put the ret to the end of args of reconcat [PR112759]

[Jakub Jelinek]

On Tue, Dec 19, 2023 at 09:30:49AM +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 always put the `ret` to the end, as it may be NULL.
> We keep use reconcat here, due to that reconcat can make it
> easier if we add more hardware features detecting, for example
> by hwcap.
> 
> gcc/
> 
>         PR target/112759
>         * config/mips/driver-native.cc (host_detect_local_cpu):
> 	Put the ret to the end of args of reconcat.
> ---
>  gcc/config/mips/driver-native.cc | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/gcc/config/mips/driver-native.cc b/gcc/config/mips/driver-native.cc
> index afc276f5278..9a224b3f401 100644
> --- a/gcc/config/mips/driver-native.cc
> +++ b/gcc/config/mips/driver-native.cc
> @@ -44,6 +44,8 @@ const char *
>  host_detect_local_cpu (int argc, const char **argv)
>  {
>    const char *cpu = NULL;
> +  /* Don't assigne any static string to ret.  If you need to do so,
> +     use concat.  */
>    char *ret = NULL;
>    char buf[128];
>    FILE *f;
> @@ -90,7 +92,8 @@ host_detect_local_cpu (int argc, const char **argv)
>  
>  fallback_cpu:
>  #if defined (__mips_nan2008)
> -  ret = reconcat (ret, " -mnan=2008 ", NULL);
> +  /* Put the ret to the end of list, since it maybe NULL.  */
> +  ret = reconcat (ret, "-mnan=2008", ret, NULL);
>  #endif
>  
>  #ifdef HAVE_GETAUXVAL
> @@ -104,7 +107,7 @@ fallback_cpu:
>  #endif
>  
>    if (cpu)
> -    ret = reconcat (ret, ret, "-m", argv[0], "=", cpu, NULL);
> +    ret = reconcat (ret, "-m", argv[0], "=", cpu, ret, NULL);

I think if you don't put any spaces, the above could return
-march=loongson3a-mnan=2008
which will not work.
If you want to emit no spurious spaces around but emit them when needed,
one way is to put there the space when needed, so
ret = reconcat (ret, "-mnan=2008", ret ? " " : "", ret, NULL);
or
ret = reconcat (ret, "-m", argv[0], "=", cpu, ret ? " " : "", ret, NULL);
would do it.

I must say I'm also surprised by determining whether to use -mnan=2008 or
not by how has the host compiler been configured, shouldn't that be
querying properties of the hardware (say, perform some floating point
operation that should result in a quiet NaN and see if it has the mantissa
MSB set or clear)?  And, do you really want to add that -mnan=2008 twice
for -march=native -mtune=native, or just for one of those (I assume
-mnan=2008 is an ABI option, so shouldn't be about tuning but about
-march=).

That said, don't really know anything about MIPS, so these are just random
comments.

	Jakub

Re: [PATCH v2] MIPS: Put the ret to the end of args of reconcat [PR112759]

[YunQiang Su]

Jakub Jelinek <jakub@redhat.com> 于2023年12月19日周二 16:40写道：
>
> On Tue, Dec 19, 2023 at 09:30:49AM +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 always put the `ret` to the end, as it may be NULL.
> > We keep use reconcat here, due to that reconcat can make it
> > easier if we add more hardware features detecting, for example
> > by hwcap.
> >
> > gcc/
> >
> >         PR target/112759
> >         * config/mips/driver-native.cc (host_detect_local_cpu):
> >       Put the ret to the end of args of reconcat.
> > ---
> >  gcc/config/mips/driver-native.cc | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/gcc/config/mips/driver-native.cc b/gcc/config/mips/driver-native.cc
> > index afc276f5278..9a224b3f401 100644
> > --- a/gcc/config/mips/driver-native.cc
> > +++ b/gcc/config/mips/driver-native.cc
> > @@ -44,6 +44,8 @@ const char *
> >  host_detect_local_cpu (int argc, const char **argv)
> >  {
> >    const char *cpu = NULL;
> > +  /* Don't assigne any static string to ret.  If you need to do so,
> > +     use concat.  */
> >    char *ret = NULL;
> >    char buf[128];
> >    FILE *f;
> > @@ -90,7 +92,8 @@ host_detect_local_cpu (int argc, const char **argv)
> >
> >  fallback_cpu:
> >  #if defined (__mips_nan2008)
> > -  ret = reconcat (ret, " -mnan=2008 ", NULL);
> > +  /* Put the ret to the end of list, since it maybe NULL.  */
> > +  ret = reconcat (ret, "-mnan=2008", ret, NULL);
> >  #endif
> >
> >  #ifdef HAVE_GETAUXVAL
> > @@ -104,7 +107,7 @@ fallback_cpu:
> >  #endif
> >
> >    if (cpu)
> > -    ret = reconcat (ret, ret, "-m", argv[0], "=", cpu, NULL);
> > +    ret = reconcat (ret, "-m", argv[0], "=", cpu, ret, NULL);
>
> I think if you don't put any spaces, the above could return
> -march=loongson3a-mnan=2008
> which will not work.

Thanks.

> If you want to emit no spurious spaces around but emit them when needed,
> one way is to put there the space when needed, so
> ret = reconcat (ret, "-mnan=2008", ret ? " " : "", ret, NULL);
> or
> ret = reconcat (ret, "-m", argv[0], "=", cpu, ret ? " " : "", ret, NULL);
> would do it.
>
> I must say I'm also surprised by determining whether to use -mnan=2008 or
> not by how has the host compiler been configured, shouldn't that be
> querying properties of the hardware (say, perform some floating point
> operation that should result in a quiet NaN and see if it has the mantissa
> MSB set or clear)?  And, do you really want to add that -mnan=2008 twice

In fact, we cannot. Since the operating system env can have only one value.
Currently, the OS env can be NAN-legacy, and it can be used on NAN2008 hardware
if an extra kernel option is set. And vice versa.

If we detect the hardware, user will meet some problem when linking, such as
      linking -mnan=2008 module with previous -mnan=legacy modules

And the binaries for NaN2008, and NaN-legacy use different dynamic linkers.
If we don't pass -mnan=2008 here, if the gcc is configured --with-nan=2008,
the NaN-legacy dynamic linkers will be tried, and then failed.

Maybe it needs a better fix.
Let's keep it for now, and try to find a better solution.

> for -march=native -mtune=native, or just for one of those (I assume
> -mnan=2008 is an ABI option, so shouldn't be about tuning but about
> -march=).
>

Thank you. Your suggestion is correct.
I will skip -mtune, as a user may use it separately just for tuning.

> That said, don't really know anything about MIPS, so these are just random
> comments.
>
>         Jakub
>