diff mbox series

Factor out `find_a_program` helper around `find_a_file`

Message ID 20210804182100.1505813-1-git@JohnEricson.me
State New
Headers show
Series Factor out `find_a_program` helper around `find_a_file` | expand

Commit Message

John Ericson Aug. 4, 2021, 6:21 p.m. UTC 
The helper is for `--print-prog-name` and similar things. Since all
executable finding goes through it, we can move the default overrides
into that path too. This also ensures that if some is looking for a
*non*-program that called `as`, `ld`, etc., weird things don't happen.
---
 gcc/gcc.c | 59 ++++++++++++++++++++++++++++++++-----------------------
 1 file changed, 34 insertions(+), 25 deletions(-)

Comments

Jeff Law Sept. 19, 2021, 3:10 p.m. UTC | #1 
On 8/4/2021 12:21 PM, John Ericson wrote:
> The helper is for `--print-prog-name` and similar things. Since all
> executable finding goes through it, we can move the default overrides
> into that path too. This also ensures that if some is looking for a
> *non*-program that called `as`, `ld`, etc., weird things don't happen.
> ---
>   gcc/gcc.c | 59 ++++++++++++++++++++++++++++++++-----------------------
>   1 file changed, 34 insertions(+), 25 deletions(-)
Thanks.  I added a ChangeLog entry and pushed this to the trunk.

Jeff
Iain Sandoe Sept. 19, 2021, 10:41 p.m. UTC | #2 
> On 19 Sep 2021, at 16:10, Jeff Law via Gcc-patches <gcc-patches@gcc.gnu.org> wrote:
> 
> 
> 
> On 8/4/2021 12:21 PM, John Ericson wrote:
>> The helper is for `--print-prog-name` and similar things. Since all
>> executable finding goes through it, we can move the default overrides
>> into that path too. This also ensures that if some is looking for a
>> *non*-program that called `as`, `ld`, etc., weird things don't happen.
>> ---
>>  gcc/gcc.c | 59 ++++++++++++++++++++++++++++++++-----------------------
>>  1 file changed, 34 insertions(+), 25 deletions(-)
> Thanks.  I added a ChangeLog entry and pushed this to the trunk.

This doesn’t appear to have been tested with —with-{as,ld,dsymutil}= where
bootstrap fails since ‘mode' is undefined in the reorganised code.

I’m testing the patch below and will apply it if all goes OK,
thanks
Iain

diff --git a/gcc/gcc.c b/gcc/gcc.c
index 1a74bf92f7a..506c2acc282 100644
--- a/gcc/gcc.c
+++ b/gcc/gcc.c
@@ -3083,17 +3083,17 @@ find_a_program (const char *name)
   /* Do not search if default matches query. */
 
 #ifdef DEFAULT_ASSEMBLER
-  if (! strcmp (name, "as") && access (DEFAULT_ASSEMBLER, mode) == 0)
+  if (! strcmp (name, "as") && access (DEFAULT_ASSEMBLER, X_OK) == 0)
     return xstrdup (DEFAULT_ASSEMBLER);
 #endif
 
 #ifdef DEFAULT_LINKER
-  if (! strcmp (name, "ld") && access (DEFAULT_LINKER, mode) == 0)
+  if (! strcmp (name, "ld") && access (DEFAULT_LINKER, X_OK) == 0)
     return xstrdup (DEFAULT_LINKER);
 #endif
 
 #ifdef DEFAULT_DSYMUTIL
-  if (! strcmp (name, "dsymutil") && access (DEFAULT_DSYMUTIL, mode) == 0)
+  if (! strcmp (name, "dsymutil") && access (DEFAULT_DSYMUTIL, X_OK) == 0)
     return xstrdup (DEFAULT_DSYMUTIL);
 #endif
Iain Sandoe Sept. 20, 2021, 6:51 a.m. UTC | #3 
Hi Folks

> On 19 Sep 2021, at 23:41, Iain Sandoe <idsandoe@googlemail.com> wrote:
> 
>> On 19 Sep 2021, at 16:10, Jeff Law via Gcc-patches <gcc-patches@gcc.gnu.org> wrote:
>> 
>> On 8/4/2021 12:21 PM, John Ericson wrote:
>>> The helper is for `--print-prog-name` and similar things. Since all
>>> executable finding goes through it, we can move the default overrides
>>> into that path too. This also ensures that if some is looking for a
>>> *non*-program that called `as`, `ld`, etc., weird things don't happen.
>>> ---
>>> gcc/gcc.c | 59 ++++++++++++++++++++++++++++++++-----------------------
>>> 1 file changed, 34 insertions(+), 25 deletions(-)
>> Thanks.  I added a ChangeLog entry and pushed this to the trunk.
> 
> This doesn’t appear to have been tested with —with-{as,ld,dsymutil}= where
> bootstrap fails since ‘mode' is undefined in the reorganised code.
> 
> I’m testing the patch below and will apply it if all goes OK,

https://gcc.gnu.org/pipermail/gcc-cvs/2021-September/353756.html
Iain
Gerald Pfeifer Sept. 20, 2021, 6:25 p.m. UTC | #4 
On Sun, 19 Sep 2021, Jeff Law via Gcc-patches wrote:
> On 8/4/2021 12:21 PM, John Ericson wrote:
>> The helper is for `--print-prog-name` and similar things. Since all
>> executable finding goes through it, we can move the default overrides
>> into that path too. This also ensures that if some is looking for a
>> *non*-program that called `as`, `ld`, etc., weird things don't happen.
>> ---
>>   gcc/gcc.c | 59 ++++++++++++++++++++++++++++++++-----------------------
>>   1 file changed, 34 insertions(+), 25 deletions(-)
> Thanks.  I added a ChangeLog entry and pushed this to the trunk.

|    commit 5fee8a0a9223d030c66d53c104fb0a431369248f
|    Author: John Ericson <git@JohnEricson.me>
|    Date:   Sun Sep 19 11:08:32 2021 -0400
|
|    [PATCH] Factor out `find_a_program` helper around `find_a_file`
|    
|    gcc/
|            * gcc.c (find_a_program): New function, factored out of...
|            (find_a_file): Here.
|            (execute): Use find_a_program when looking for programs rather
|            than find_a_file.

I'm afraid this broke x86_64-unknown-freebsd12*:

.../GCC-HEAD/gcc/gcc.c: In function 'char* find_a_program(const char*)':
/scratch/tmp/gerald/GCC-HEAD/gcc/gcc.c:3086:59: error: 'mode' was not declared in this scope; did you mean 'pmode'?
 3086 |   if (! strcmp (name, "as") && access (DEFAULT_ASSEMBLER, mode) == 0)
      |                                                           ^~~~
      |                                                           pmode
/scratch/tmp/gerald/GCC-HEAD/gcc/gcc.c:3091:56: error: 'mode' was not declared in this scope; did you mean 'pmode'?
 3091 |   if (! strcmp (name, "ld") && access (DEFAULT_LINKER, mode) == 0)
      |                                                        ^~~~
      |                                                        pmode

Looking at the code in question I see

  static char*
  find_a_program (const char *name)
  {
    /* Do not search if default matches query. */

  #ifdef DEFAULT_ASSEMBLER
    if (! strcmp (name, "as") && access (DEFAULT_ASSEMBLER, mode) == 0)
      return xstrdup (DEFAULT_ASSEMBLER);
  #endif

where mode is not defined in that function.

Looks like the factoring out did not take the non-default case into
consideration?


I was just going to commit a fix I tested last night, when that patch 
simply "disappeared". Looks like Ian beat me to it with exactly the same 
patch. :-)

Gerald
diff mbox series

Patch

diff --git a/gcc/gcc.c b/gcc/gcc.c
index 3e98bc7973e..1a74bf92f7a 100644
--- a/gcc/gcc.c
+++ b/gcc/gcc.c
@@ -367,6 +367,7 @@  static void putenv_from_prefixes (const struct path_prefix *, const char *,
 				  bool);
 static int access_check (const char *, int);
 static char *find_a_file (const struct path_prefix *, const char *, int, bool);
+static char *find_a_program (const char *);
 static void add_prefix (struct path_prefix *, const char *, const char *,
 			int, int, int);
 static void add_sysrooted_prefix (struct path_prefix *, const char *,
@@ -3052,22 +3053,7 @@  find_a_file (const struct path_prefix *pprefix, const char *name, int mode,
 {
   struct file_at_path_info info;
 
-#ifdef DEFAULT_ASSEMBLER
-  if (! strcmp (name, "as") && access (DEFAULT_ASSEMBLER, mode) == 0)
-    return xstrdup (DEFAULT_ASSEMBLER);
-#endif
-
-#ifdef DEFAULT_LINKER
-  if (! strcmp (name, "ld") && access (DEFAULT_LINKER, mode) == 0)
-    return xstrdup (DEFAULT_LINKER);
-#endif
-
-#ifdef DEFAULT_DSYMUTIL
-  if (! strcmp (name, "dsymutil") && access (DEFAULT_DSYMUTIL, mode) == 0)
-    return xstrdup (DEFAULT_DSYMUTIL);
-#endif
-
-  /* Determine the filename to execute (special case for absolute paths).  */
+  /* Find the filename in question (special case for absolute paths).  */
 
   if (IS_ABSOLUTE_PATH (name))
     {
@@ -3088,6 +3074,32 @@  find_a_file (const struct path_prefix *pprefix, const char *name, int mode,
 				file_at_path, &info);
 }
 
+/* Specialization of find_a_file for programs that also takes into account
+   configure-specified default programs. */
+
+static char*
+find_a_program (const char *name)
+{
+  /* Do not search if default matches query. */
+
+#ifdef DEFAULT_ASSEMBLER
+  if (! strcmp (name, "as") && access (DEFAULT_ASSEMBLER, mode) == 0)
+    return xstrdup (DEFAULT_ASSEMBLER);
+#endif
+
+#ifdef DEFAULT_LINKER
+  if (! strcmp (name, "ld") && access (DEFAULT_LINKER, mode) == 0)
+    return xstrdup (DEFAULT_LINKER);
+#endif
+
+#ifdef DEFAULT_DSYMUTIL
+  if (! strcmp (name, "dsymutil") && access (DEFAULT_DSYMUTIL, mode) == 0)
+    return xstrdup (DEFAULT_DSYMUTIL);
+#endif
+
+  return find_a_file (&exec_prefixes, name, X_OK, false);
+}
+
 /* Ranking of prefixes in the sort list. -B prefixes are put before
    all others.  */
 
@@ -3243,8 +3255,7 @@  execute (void)
 
   if (wrapper_string)
     {
-      string = find_a_file (&exec_prefixes,
-			    argbuf[0], X_OK, false);
+      string = find_a_program (argbuf[0]);
       if (string)
 	argbuf[0] = string;
       insert_wrapper (wrapper_string);
@@ -3269,7 +3280,7 @@  execute (void)
 
   if (!wrapper_string)
     {
-      string = find_a_file (&exec_prefixes, commands[0].prog, X_OK, false);
+      string = find_a_program(commands[0].prog);
       if (string)
 	commands[0].argv[0] = string;
     }
@@ -3284,8 +3295,7 @@  execute (void)
 	commands[n_commands].prog = argbuf[i + 1];
 	commands[n_commands].argv
 	  = &(argbuf.address ())[i + 1];
-	string = find_a_file (&exec_prefixes, commands[n_commands].prog,
-			      X_OK, false);
+	string = find_a_program(commands[n_commands].prog);
 	if (string)
 	  commands[n_commands].argv[0] = string;
 	n_commands++;
@@ -8556,8 +8566,7 @@  driver::maybe_putenv_COLLECT_LTO_WRAPPER () const
   if (have_c)
     lto_wrapper_file = NULL;
   else
-    lto_wrapper_file = find_a_file (&exec_prefixes, "lto-wrapper",
-				    X_OK, false);
+    lto_wrapper_file = find_a_program ("lto-wrapper");
   if (lto_wrapper_file)
     {
       lto_wrapper_file = convert_white_space (lto_wrapper_file);
@@ -8671,7 +8680,7 @@  driver::maybe_print_and_exit () const
 #endif
 	  print_prog_name = concat (print_prog_name, use_ld, NULL);
 	}
-      char *newname = find_a_file (&exec_prefixes, print_prog_name, X_OK, 0);
+      char *newname = find_a_program (print_prog_name);
       printf ("%s\n", (newname ? newname : print_prog_name));
       return (0);
     }
@@ -9070,7 +9079,7 @@  driver::maybe_run_linker (const char *argv0) const
 	  /* We'll use ld if we can't find collect2.  */
 	  if (! strcmp (linker_name_spec, "collect2"))
 	    {
-	      char *s = find_a_file (&exec_prefixes, "collect2", X_OK, false);
+	      char *s = find_a_program ("collect2");
 	      if (s == NULL)
 		set_static_spec_shared (&linker_name_spec, "ld");
 	    }