| 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 |
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 >
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++)
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 <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?
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
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 --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++)