diff mbox series

[v2,1/2] driver: Use <triple>-as/ld/objcopy as final fallback instead of native ones for cross

Message ID 20240522095404.1825269-1-syq@gcc.gnu.org
State New
Headers show
Series [v2,1/2] driver: Use <triple>-as/ld/objcopy as final fallback instead of native ones for cross | expand

Commit Message

YunQiang Su May 22, 2024, 9:54 a.m. UTC 
If `find_a_program` cannot find `as/ld/objcopy` and we are a cross toolchain,
the final fallback is `as/ld` of system.  In fact, we can have a try with
<triple>-as/ld/objcopy before fallback to native as/ld/objcopy.

This patch is derivatived from Debian's patch:
  gcc-search-prefixed-as-ld.diff

gcc
	* gcc.cc(execute): Looks for <triple>-as/ld/objcopy before fallback
	to native as/ld/objcopy.
---
 gcc/gcc.cc | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

Comments

YunQiang Su May 28, 2024, 6:32 p.m. UTC | #1 
YunQiang Su <syq@gcc.gnu.org> 于2024年5月22日周三 17:54写道：
>
> If `find_a_program` cannot find `as/ld/objcopy` and we are a cross toolchain,
> the final fallback is `as/ld` of system.  In fact, we can have a try with
> <triple>-as/ld/objcopy before fallback to native as/ld/objcopy.
>
> This patch is derivatived from Debian's patch:
>   gcc-search-prefixed-as-ld.diff
>
> gcc
>         * gcc.cc(execute): Looks for <triple>-as/ld/objcopy before fallback
>         to native as/ld/objcopy.

ping. OK for the trunk?

> ---
>  gcc/gcc.cc | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
>
> diff --git a/gcc/gcc.cc b/gcc/gcc.cc
> index 830a4700a87..3dc6348d761 100644
> --- a/gcc/gcc.cc
> +++ b/gcc/gcc.cc
> @@ -3293,6 +3293,26 @@ execute (void)
>        string = find_a_program(commands[0].prog);
>        if (string)
>         commands[0].argv[0] = string;
> +      else if (*cross_compile != '0'
> +               && !strcmp (commands[0].argv[0], commands[0].prog)
> +               && (!strcmp (commands[0].prog, "as")
> +                   || !strcmp (commands[0].prog, "ld")
> +                   || !strcmp (commands[0].prog, "objcopy")))
> +       {
> +         string = concat (DEFAULT_REAL_TARGET_MACHINE, "-",
> +                               commands[0].prog, NULL);
> +         const char *string_args[] = {string, "--version", NULL};
> +         int exit_status = 0;
> +         int err = 0;
> +         const char *errmsg = pex_one (PEX_SEARCH, string,
> +                         CONST_CAST (char **, string_args), string,
> +                         NULL, NULL, &exit_status, &err);
> +         if (errmsg == NULL && exit_status == 0 && err == 0)
> +           {
> +             commands[0].argv[0] = string;
> +             commands[0].prog = string;
> +           }
> +       }
>      }
>
>    for (n_commands = 1, i = 0; argbuf.iterate (i, &arg); i++)
> --
> 2.39.2
>
Richard Sandiford May 28, 2024, 9:27 p.m. UTC | #2 
YunQiang Su <syq@gcc.gnu.org> writes:
> If `find_a_program` cannot find `as/ld/objcopy` and we are a cross toolchain,
> the final fallback is `as/ld` of system.  In fact, we can have a try with
> <triple>-as/ld/objcopy before fallback to native as/ld/objcopy.
>
> This patch is derivatived from Debian's patch:
>   gcc-search-prefixed-as-ld.diff

I'm probably making you repeat a previous discussion, sorry, but could
you describe the use case in more detail?  The current approach to
handling cross toolchains has been used for many years.  Presumably
this patch is supporting a different way of organising things,
but I wasn't sure from the description what it was.

AIUI, we currently assume that cross as, ld and objcopy will be
installed under those names in $prefix/$target_alias/bin (aka $tooldir/bin).
E.g.:

   bin/aarch64-elf-as = aarch64-elf/bin/as

GCC should then find as in aarch64-elf/bin.

Is that not true in your case?

To be clear, I'm not saying the patch is wrong.  I'm just trying to
understand why the patch is needed.

Thanks,
Richard

>
> gcc
> 	* gcc.cc(execute): Looks for <triple>-as/ld/objcopy before fallback
> 	to native as/ld/objcopy.
> ---
>  gcc/gcc.cc | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
>
> diff --git a/gcc/gcc.cc b/gcc/gcc.cc
> index 830a4700a87..3dc6348d761 100644
> --- a/gcc/gcc.cc
> +++ b/gcc/gcc.cc
> @@ -3293,6 +3293,26 @@ execute (void)
>        string = find_a_program(commands[0].prog);
>        if (string)
>  	commands[0].argv[0] = string;
> +      else if (*cross_compile != '0'
> +		&& !strcmp (commands[0].argv[0], commands[0].prog)
> +		&& (!strcmp (commands[0].prog, "as")
> +		    || !strcmp (commands[0].prog, "ld")
> +		    || !strcmp (commands[0].prog, "objcopy")))
> +	{
> +	  string = concat (DEFAULT_REAL_TARGET_MACHINE, "-",
> +				commands[0].prog, NULL);
> +	  const char *string_args[] = {string, "--version", NULL};
> +	  int exit_status = 0;
> +	  int err = 0;
> +	  const char *errmsg = pex_one (PEX_SEARCH, string,
> +			  CONST_CAST (char **, string_args), string,
> +			  NULL, NULL, &exit_status, &err);
> +	  if (errmsg == NULL && exit_status == 0 && err == 0)
> +	    {
> +	      commands[0].argv[0] = string;
> +	      commands[0].prog = string;
> +	    }
> +	}
>      }
>  
>    for (n_commands = 1, i = 0; argbuf.iterate (i, &arg); i++)
YunQiang Su May 29, 2024, 2:02 a.m. UTC | #3 
Richard Sandiford <richard.sandiford@arm.com> 于2024年5月29日周三 05:28写道：
>
> YunQiang Su <syq@gcc.gnu.org> writes:
> > If `find_a_program` cannot find `as/ld/objcopy` and we are a cross toolchain,
> > the final fallback is `as/ld` of system.  In fact, we can have a try with
> > <triple>-as/ld/objcopy before fallback to native as/ld/objcopy.
> >
> > This patch is derivatived from Debian's patch:
> >   gcc-search-prefixed-as-ld.diff
>
> I'm probably making you repeat a previous discussion, sorry, but could
> you describe the use case in more detail?  The current approach to
> handling cross toolchains has been used for many years.  Presumably
> this patch is supporting a different way of organising things,
> but I wasn't sure from the description what it was.
>
> AIUI, we currently assume that cross as, ld and objcopy will be
> installed under those names in $prefix/$target_alias/bin (aka $tooldir/bin).
> E.g.:
>
>    bin/aarch64-elf-as = aarch64-elf/bin/as
>
> GCC should then find as in aarch64-elf/bin.
>
> Is that not true in your case?
>

Yes. This patch is only about the final fallback. I mean aarch64-elf/bin/as
still has higher priority than bin/aarch64-elf-as.

In the current code, we find gas with:
    /prefix/aarch64-elf/bin/as > $PATH/as

And this patch a new one between them:
    /prefix/aarch64-elf/bin/as > $PATH/aarch64-elf-as > $PATH/as

> To be clear, I'm not saying the patch is wrong.  I'm just trying to
> understand why the patch is needed.
>

Yes. If gcc is configured correctly, it is not so useful.
In some case for some lazy user, it may be useful,
for example, the binutils installed into different prefix with libc etc.

For example, binutils is installed into /usr/aarch64-elf/bin, while
libc is installed into /usr/local/aarch64-elf/.

> Thanks,
> Richard
>
> >
> > gcc
> >       * gcc.cc(execute): Looks for <triple>-as/ld/objcopy before fallback
> >       to native as/ld/objcopy.
> > ---
> >  gcc/gcc.cc | 20 ++++++++++++++++++++
> >  1 file changed, 20 insertions(+)
> >
> > diff --git a/gcc/gcc.cc b/gcc/gcc.cc
> > index 830a4700a87..3dc6348d761 100644
> > --- a/gcc/gcc.cc
> > +++ b/gcc/gcc.cc
> > @@ -3293,6 +3293,26 @@ execute (void)
> >        string = find_a_program(commands[0].prog);
> >        if (string)
> >       commands[0].argv[0] = string;
> > +      else if (*cross_compile != '0'
> > +             && !strcmp (commands[0].argv[0], commands[0].prog)
> > +             && (!strcmp (commands[0].prog, "as")
> > +                 || !strcmp (commands[0].prog, "ld")
> > +                 || !strcmp (commands[0].prog, "objcopy")))
> > +     {
> > +       string = concat (DEFAULT_REAL_TARGET_MACHINE, "-",
> > +                             commands[0].prog, NULL);
> > +       const char *string_args[] = {string, "--version", NULL};
> > +       int exit_status = 0;
> > +       int err = 0;
> > +       const char *errmsg = pex_one (PEX_SEARCH, string,
> > +                       CONST_CAST (char **, string_args), string,
> > +                       NULL, NULL, &exit_status, &err);
> > +       if (errmsg == NULL && exit_status == 0 && err == 0)
> > +         {
> > +           commands[0].argv[0] = string;
> > +           commands[0].prog = string;
> > +         }
> > +     }
> >      }
> >
> >    for (n_commands = 1, i = 0; argbuf.iterate (i, &arg); i++)
YunQiang Su June 6, 2024, 2:41 a.m. UTC | #4 
YunQiang Su <syq@gcc.gnu.org> 于2024年5月29日周三 10:02写道：
>
> Richard Sandiford <richard.sandiford@arm.com> 于2024年5月29日周三 05:28写道：
> >
> > YunQiang Su <syq@gcc.gnu.org> writes:
> > > If `find_a_program` cannot find `as/ld/objcopy` and we are a cross toolchain,
> > > the final fallback is `as/ld` of system.  In fact, we can have a try with
> > > <triple>-as/ld/objcopy before fallback to native as/ld/objcopy.
> > >
> > > This patch is derivatived from Debian's patch:
> > >   gcc-search-prefixed-as-ld.diff
> >
> > I'm probably making you repeat a previous discussion, sorry, but could
> > you describe the use case in more detail?  The current approach to
> > handling cross toolchains has been used for many years.  Presumably
> > this patch is supporting a different way of organising things,
> > but I wasn't sure from the description what it was.
> >
> > AIUI, we currently assume that cross as, ld and objcopy will be
> > installed under those names in $prefix/$target_alias/bin (aka $tooldir/bin).
> > E.g.:
> >
> >    bin/aarch64-elf-as = aarch64-elf/bin/as
> >
> > GCC should then find as in aarch64-elf/bin.
> >
> > Is that not true in your case?
> >
>
> Yes. This patch is only about the final fallback. I mean aarch64-elf/bin/as
> still has higher priority than bin/aarch64-elf-as.
>
> In the current code, we find gas with:
>     /prefix/aarch64-elf/bin/as > $PATH/as
>
> And this patch a new one between them:
>     /prefix/aarch64-elf/bin/as > $PATH/aarch64-elf-as > $PATH/as
>
> > To be clear, I'm not saying the patch is wrong.  I'm just trying to
> > understand why the patch is needed.
> >
>
> Yes. If gcc is configured correctly, it is not so useful.
> In some case for some lazy user, it may be useful,
> for example, the binutils installed into different prefix with libc etc.
>
> For example, binutils is installed into /usr/aarch64-elf/bin, while
> libc is installed into /usr/local/aarch64-elf/.
>

Any idea about it? Is it a use case making sense?
Richard Sandiford June 6, 2024, 9:54 a.m. UTC | #5 
YunQiang Su <syq@gcc.gnu.org> writes:
> YunQiang Su <syq@gcc.gnu.org> 于2024年5月29日周三 10:02写道：
>>
>> Richard Sandiford <richard.sandiford@arm.com> 于2024年5月29日周三 05:28写道：
>> >
>> > YunQiang Su <syq@gcc.gnu.org> writes:
>> > > If `find_a_program` cannot find `as/ld/objcopy` and we are a cross toolchain,
>> > > the final fallback is `as/ld` of system.  In fact, we can have a try with
>> > > <triple>-as/ld/objcopy before fallback to native as/ld/objcopy.
>> > >
>> > > This patch is derivatived from Debian's patch:
>> > >   gcc-search-prefixed-as-ld.diff
>> >
>> > I'm probably making you repeat a previous discussion, sorry, but could
>> > you describe the use case in more detail?  The current approach to
>> > handling cross toolchains has been used for many years.  Presumably
>> > this patch is supporting a different way of organising things,
>> > but I wasn't sure from the description what it was.
>> >
>> > AIUI, we currently assume that cross as, ld and objcopy will be
>> > installed under those names in $prefix/$target_alias/bin (aka $tooldir/bin).
>> > E.g.:
>> >
>> >    bin/aarch64-elf-as = aarch64-elf/bin/as
>> >
>> > GCC should then find as in aarch64-elf/bin.
>> >
>> > Is that not true in your case?
>> >
>>
>> Yes. This patch is only about the final fallback. I mean aarch64-elf/bin/as
>> still has higher priority than bin/aarch64-elf-as.
>>
>> In the current code, we find gas with:
>>     /prefix/aarch64-elf/bin/as > $PATH/as
>>
>> And this patch a new one between them:
>>     /prefix/aarch64-elf/bin/as > $PATH/aarch64-elf-as > $PATH/as
>>
>> > To be clear, I'm not saying the patch is wrong.  I'm just trying to
>> > understand why the patch is needed.
>> >
>>
>> Yes. If gcc is configured correctly, it is not so useful.
>> In some case for some lazy user, it may be useful,
>> for example, the binutils installed into different prefix with libc etc.
>>
>> For example, binutils is installed into /usr/aarch64-elf/bin, while
>> libc is installed into /usr/local/aarch64-elf/.
>>
>
> Any idea about it? Is it a use case making sense?

Yeah, I think it makes sense.  GCC and binutils are separate packages.
Users could cherry-pick a GCC installation and a separate binutils
installation rather than bundling them together into a single
toolchain.  And not everyone will have permission to change $tooldir.

So I agree we should support searching the user's path for an
as/ld/etc. based on the tool prefix.  Unfortunately, I don't think
I understand the code & constraints well enough to do a review.

In particular, it seems unfortunate that we need to do a trial
subcommand invocation before committing to the prefixed name.
And, if we continue to search for "as" in the user's path as a fallback,
it's not 100% obvious that "${triple}-as" later in the path should trump
"as" earlier in the path.

In some ways, it seems more consistent to do the replacement without
first doing a trial invocation.  But I don't know whether that would
break existing use cases.  (To be clear, I wouldn't feel comfortable
approving a patch to do that without buy-in from other maintainers.)

Thanks,
Richard
YunQiang Su June 10, 2024, 10:08 a.m. UTC | #6 
Richard Sandiford <richard.sandiford@arm.com> 于2024年6月6日周四 17:54写道：
>
> YunQiang Su <syq@gcc.gnu.org> writes:
> > YunQiang Su <syq@gcc.gnu.org> 于2024年5月29日周三 10:02写道：
> >>
> >> Richard Sandiford <richard.sandiford@arm.com> 于2024年5月29日周三 05:28写道：
> >> >
> >> > YunQiang Su <syq@gcc.gnu.org> writes:
> >> > > If `find_a_program` cannot find `as/ld/objcopy` and we are a cross toolchain,
> >> > > the final fallback is `as/ld` of system.  In fact, we can have a try with
> >> > > <triple>-as/ld/objcopy before fallback to native as/ld/objcopy.
> >> > >
> >> > > This patch is derivatived from Debian's patch:
> >> > >   gcc-search-prefixed-as-ld.diff
> >> >
> >> > I'm probably making you repeat a previous discussion, sorry, but could
> >> > you describe the use case in more detail?  The current approach to
> >> > handling cross toolchains has been used for many years.  Presumably
> >> > this patch is supporting a different way of organising things,
> >> > but I wasn't sure from the description what it was.
> >> >
> >> > AIUI, we currently assume that cross as, ld and objcopy will be
> >> > installed under those names in $prefix/$target_alias/bin (aka $tooldir/bin).
> >> > E.g.:
> >> >
> >> >    bin/aarch64-elf-as = aarch64-elf/bin/as
> >> >
> >> > GCC should then find as in aarch64-elf/bin.
> >> >
> >> > Is that not true in your case?
> >> >
> >>
> >> Yes. This patch is only about the final fallback. I mean aarch64-elf/bin/as
> >> still has higher priority than bin/aarch64-elf-as.
> >>
> >> In the current code, we find gas with:
> >>     /prefix/aarch64-elf/bin/as > $PATH/as
> >>
> >> And this patch a new one between them:
> >>     /prefix/aarch64-elf/bin/as > $PATH/aarch64-elf-as > $PATH/as
> >>
> >> > To be clear, I'm not saying the patch is wrong.  I'm just trying to
> >> > understand why the patch is needed.
> >> >
> >>
> >> Yes. If gcc is configured correctly, it is not so useful.
> >> In some case for some lazy user, it may be useful,
> >> for example, the binutils installed into different prefix with libc etc.
> >>
> >> For example, binutils is installed into /usr/aarch64-elf/bin, while
> >> libc is installed into /usr/local/aarch64-elf/.
> >>
> >
> > Any idea about it? Is it a use case making sense?
>
> Yeah, I think it makes sense.  GCC and binutils are separate packages.
> Users could cherry-pick a GCC installation and a separate binutils
> installation rather than bundling them together into a single
> toolchain.  And not everyone will have permission to change $tooldir.
>
> So I agree we should support searching the user's path for an
> as/ld/etc. based on the tool prefix.  Unfortunately, I don't think
> I understand the code & constraints well enough to do a review.
>
> In particular, it seems unfortunate that we need to do a trial
> subcommand invocation before committing to the prefixed name.
> And, if we continue to search for "as" in the user's path as a fallback,
> it's not 100% obvious that "${triple}-as" later in the path should trump
> "as" earlier in the path.
>
> In some ways, it seems more consistent to do the replacement without
> first doing a trial invocation.  But I don't know whether that would
> break existing use cases.  (To be clear, I wouldn't feel comfortable

Yes. This is also my worry as some users may set $PATH manually
to a path which contains target `as`, such as
   export PATH="/usr/aarch64-linux-gnu/bin:$PATH"

> approving a patch to do that without buy-in from other maintainers.)
>
> Thanks,
> Richard
diff mbox series

Patch

diff --git a/gcc/gcc.cc b/gcc/gcc.cc
index 830a4700a87..3dc6348d761 100644
--- a/gcc/gcc.cc
+++ b/gcc/gcc.cc
@@ -3293,6 +3293,26 @@  execute (void)
       string = find_a_program(commands[0].prog);
       if (string)
 	commands[0].argv[0] = string;
+      else if (*cross_compile != '0'
+		&& !strcmp (commands[0].argv[0], commands[0].prog)
+		&& (!strcmp (commands[0].prog, "as")
+		    || !strcmp (commands[0].prog, "ld")
+		    || !strcmp (commands[0].prog, "objcopy")))
+	{
+	  string = concat (DEFAULT_REAL_TARGET_MACHINE, "-",
+				commands[0].prog, NULL);
+	  const char *string_args[] = {string, "--version", NULL};
+	  int exit_status = 0;
+	  int err = 0;
+	  const char *errmsg = pex_one (PEX_SEARCH, string,
+			  CONST_CAST (char **, string_args), string,
+			  NULL, NULL, &exit_status, &err);
+	  if (errmsg == NULL && exit_status == 0 && err == 0)
+	    {
+	      commands[0].argv[0] = string;
+	      commands[0].prog = string;
+	    }
+	}
     }
 
   for (n_commands = 1, i = 0; argbuf.iterate (i, &arg); i++)