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

[Jakub Jelinek <jakub@redhat.com>]

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