diff mbox

Fix file find utils and add unit tests (PR driver/81829).

Message ID b744f5fd-4b71-11d4-1747-77745a424131@suse.cz
State New
Headers show

Commit Message

Martin Liška Aug. 18, 2017, 10:17 a.m. UTC
On 08/15/2017 02:45 PM, Martin Liška wrote:
> Hi.
> 
> As shown in the PR, remove_prefix function is written wrongly. It does not distinguish
> in between a node in linked list and pprefix->plist. So I decide to rewrite it.
> Apart from that, I identified discrepancy in between do_add_prefix and prefix_from_string
> where the later one appends always DIR_SEPARATOR (if missing). So I also the former function.
> And I'm adding unit tests for all functions in file-find.c.
> 
> Patch can bootstrap on ppc64le-redhat-linux and survives regression tests.
> 
> Ready to be installed?
> Martin
> 
> gcc/ChangeLog:
> 
> 2017-08-14  Martin Liska  <mliska@suse.cz>
> 
> 	PR driver/81829
> 	* file-find.c (do_add_prefix): Always append DIR_SEPARATOR
> 	at the end of a prefix.
> 	(remove_prefix): Properly remove elements and accept also
> 	path without a trailing DIR_SEPARATOR.
> 	(purge): New function.
> 	(file_find_verify_prefix_creation): Likewise.
> 	(file_find_verify_prefix_add): Likewise.
> 	(file_find_verify_prefix_removal): Likewise.
> 	(file_find_c_tests): Likewise.
> 	* selftest-run-tests.c (selftest::run_tests): Add new
> 	file_find_c_tests.
> 	* selftest.h (file_find_c_tests): Likewise.
> ---
>  gcc/file-find.c          | 182 ++++++++++++++++++++++++++++++++++++++++++-----
>  gcc/selftest-run-tests.c |   1 +
>  gcc/selftest.h           |   1 +
>  3 files changed, 167 insertions(+), 17 deletions(-)
> 
> 

Hi.

As doko pointed out, the first version was not correct. Let me describe 2 scenarios which should
be supported:

1) my original motivation where I configure gcc w/ --prefix=/my_bin and I manually create in /my_bin/bin:

$ ln -s gcc-ar ar
$ ln -s gcc-nm nm
$ ln -s gcc-ranlib ranlib

and then is set PATH=/my_bin/bin:... and LD_LIBRARY_PATH and I don't have to care about NM=gcc-nm and so on.
That's how I usually test a newly built GCC.

2) using NM=gcc-nm and so on will force usage of LTO plugin. That's what was broken in first version of the patch.

In order to support both cases we need to identify real name of GCC wrapper (like /my_bin/bin/gcc-ar) and when
find_a_file (&path, real_exe_name, X_OK) will point to the same file, the directory should be ignored from prefix.

Can you please test attached patch in your scenario?
Thanks,
Martin

Comments

Martin Sebor Aug. 25, 2017, 4:41 p.m. UTC | #1
On 08/18/2017 04:17 AM, Martin Liška wrote:
> On 08/15/2017 02:45 PM, Martin Liška wrote:
>> Hi.
>>
>> As shown in the PR, remove_prefix function is written wrongly. It does not distinguish
>> in between a node in linked list and pprefix->plist. So I decide to rewrite it.
>> Apart from that, I identified discrepancy in between do_add_prefix and prefix_from_string
>> where the later one appends always DIR_SEPARATOR (if missing). So I also the former function.
>> And I'm adding unit tests for all functions in file-find.c.

I know only very little about this API but from a quick glance at
the change it looks to me like it introduces an implicit assumption
that prefix points to a non-empty string.  If that is in fact one
of the function's preconditions I would suggest to a) assert it
before relying on it, and b) document it.  Otherwise, if the prefix
is allowed to be empty then the code below is undefined in that
case.

Martin

@@ -126,11 +127,22 @@ do_add_prefix (struct path_prefix *pprefix, const 
char *prefix, bool first)
    /* Keep track of the longest prefix.  */

    len = strlen (prefix);
+  bool append_separator = !IS_DIR_SEPARATOR (prefix[len - 1]);
+  if (append_separator)
+    len++;
+
    if (len > pprefix->max_le
Martin Liška Aug. 30, 2017, 10:44 a.m. UTC | #2
On 08/25/2017 06:41 PM, Martin Sebor wrote:
> On 08/18/2017 04:17 AM, Martin Liška wrote:
>> On 08/15/2017 02:45 PM, Martin Liška wrote:
>>> Hi.
>>>
>>> As shown in the PR, remove_prefix function is written wrongly. It does not distinguish
>>> in between a node in linked list and pprefix->plist. So I decide to rewrite it.
>>> Apart from that, I identified discrepancy in between do_add_prefix and prefix_from_string
>>> where the later one appends always DIR_SEPARATOR (if missing). So I also the former function.
>>> And I'm adding unit tests for all functions in file-find.c.
> 
> I know only very little about this API but from a quick glance at
> the change it looks to me like it introduces an implicit assumption
> that prefix points to a non-empty string.  If that is in fact one
> of the function's preconditions I would suggest to a) assert it
> before relying on it, and b) document it.  Otherwise, if the prefix
> is allowed to be empty then the code below is undefined in that
> case.
> 
> Martin
> 
> @@ -126,11 +127,22 @@ do_add_prefix (struct path_prefix *pprefix, const char *prefix, bool first)
>    /* Keep track of the longest prefix.  */
> 
>    len = strlen (prefix);
> +  bool append_separator = !IS_DIR_SEPARATOR (prefix[len - 1]);
> +  if (append_separator)
> +    len++;
> +
>    if (len > pprefix->max_le

Thanks Martin.

I'm tending to simplify the functionality, let's see what will be discussed in the PR.

Martin
diff mbox

Patch

From f8029ed6d3dd444ee2608146118f2189cf9ef0d8 Mon Sep 17 00:00:00 2001
From: marxin <mliska@suse.cz>
Date: Mon, 14 Aug 2017 13:56:32 +0200
Subject: [PATCH] Fix file find utils and add unit tests (PR driver/81829).

gcc/ChangeLog:

2017-08-14  Martin Liska  <mliska@suse.cz>

	PR driver/81829
	* file-find.c (do_add_prefix): Always append DIR_SEPARATOR
	at the end of a prefix.
	(remove_prefix): Properly remove elements and accept also
	path without a trailing DIR_SEPARATOR.
	(purge): New function.
	(file_find_verify_prefix_creation): Likewise.
	(file_find_verify_prefix_add): Likewise.
	(file_find_verify_prefix_removal): Likewise.
	(file_find_c_tests): Likewise.
	* selftest-run-tests.c (selftest::run_tests): Add new
	file_find_c_tests.
	* selftest.h (file_find_c_tests): Likewise.
---
 gcc/file-find.c          | 182 ++++++++++++++++++++++++++++++++++++++++++-----
 gcc/gcc-ar.c             |  19 +++--
 gcc/selftest-run-tests.c |   1 +
 gcc/selftest.h           |   1 +
 4 files changed, 179 insertions(+), 24 deletions(-)

diff --git a/gcc/file-find.c b/gcc/file-find.c
index b072a4993d7..23a883042a2 100644
--- a/gcc/file-find.c
+++ b/gcc/file-find.c
@@ -21,6 +21,7 @@  along with GCC; see the file COPYING3.  If not see
 #include "system.h"
 #include "filenames.h"
 #include "file-find.h"
+#include "selftest.h"
 
 static bool debug = false;
 
@@ -126,11 +127,22 @@  do_add_prefix (struct path_prefix *pprefix, const char *prefix, bool first)
   /* Keep track of the longest prefix.  */
 
   len = strlen (prefix);
+  bool append_separator = !IS_DIR_SEPARATOR (prefix[len - 1]);
+  if (append_separator)
+    len++;
+
   if (len > pprefix->max_len)
     pprefix->max_len = len;
 
   pl = XNEW (struct prefix_list);
-  pl->prefix = xstrdup (prefix);
+  char *dup = XCNEWVEC (char, len + 1);
+  memcpy (dup, prefix, append_separator ? len - 1 : len);
+  if (append_separator)
+    {
+      dup[len - 1] = DIR_SEPARATOR;
+      dup[len] = '\0';
+    }
+  pl->prefix = dup;
 
   if (*prev)
     pl->next = *prev;
@@ -212,34 +224,170 @@  prefix_from_string (const char *p, struct path_prefix *pprefix)
 void
 remove_prefix (const char *prefix, struct path_prefix *pprefix)
 {
-  struct prefix_list *remove, **prev, **remove_prev = NULL;
+  char *dup = NULL;
   int max_len = 0;
+  size_t len = strlen (prefix);
+  if (prefix[len - 1] != DIR_SEPARATOR)
+    {
+      char *dup = XNEWVEC (char, len + 2);
+      memcpy (dup, prefix, len);
+      dup[len] = DIR_SEPARATOR;
+      dup[len + 1] = '\0';
+      prefix = dup;
+    }
 
   if (pprefix->plist)
     {
-      prev = &pprefix->plist;
-      for (struct prefix_list *pl = pprefix->plist; pl->next; pl = pl->next)
+      prefix_list *prev = NULL;
+      for (struct prefix_list *pl = pprefix->plist; pl;)
 	{
 	  if (strcmp (prefix, pl->prefix) == 0)
 	    {
-	      remove = pl;
-	      remove_prev = prev;
-	      continue;
+	      if (prev == NULL)
+		pprefix->plist = pl->next;
+	      else
+		prev->next = pl->next;
+
+	      prefix_list *remove = pl;
+	      free (remove);
+	      pl = pl->next;
 	    }
+	  else
+	    {
+	      prev = pl;
 
-	  int l = strlen (pl->prefix);
-	  if (l > max_len)
-	    max_len = l;
-
-	  prev = &pl;
-	}
+	      int l = strlen (pl->prefix);
+	      if (l > max_len)
+		max_len = l;
 
-      if (remove_prev)
-	{
-	  *remove_prev = remove->next;
-	  free (remove);
+	      pl = pl->next;
+	    }
 	}
 
       pprefix->max_len = max_len;
     }
+
+  if (dup)
+    free (dup);
+}
+
+#if CHECKING_P
+
+namespace selftest {
+
+/* Encode '#' and '_' to path and dir separators in order to test portability
+   of the test-cases.  */
+
+static char *
+purge (const char *input)
+{
+  char *s = xstrdup (input);
+  for (char *c = s; *c != '\0'; c++)
+    switch (*c)
+      {
+      case '/':
+      case ':':
+	*c = 'a'; /* Poison default string values.  */
+	break;
+      case '_':
+	*c = PATH_SEPARATOR;
+	break;
+      case '#':
+	*c = DIR_SEPARATOR;
+	break;
+      default:
+	break;
+      }
+
+  return s;
+}
+
+const char *env1 = purge ("#home#user#bin_#home#user#bin_#bin_#usr#bin");
+const char *env2 = purge ("#root_#root_#root");
+
+/* Verify creation of prefix.  */
+
+static void
+file_find_verify_prefix_creation (void)
+{
+  path_prefix prefix;
+  memset (&prefix, 0, sizeof (prefix));
+  prefix_from_string (env1, &prefix);
+
+  ASSERT_EQ (15, prefix.max_len);
+
+  /* All prefixes end with DIR_SEPARATOR.  */
+  ASSERT_STREQ (purge ("#home#user#bin#"), prefix.plist->prefix);
+  ASSERT_STREQ (purge ("#home#user#bin#"), prefix.plist->next->prefix);
+  ASSERT_STREQ (purge ("#bin#"), prefix.plist->next->next->prefix);
+  ASSERT_STREQ (purge ("#usr#bin#"), prefix.plist->next->next->next->prefix);
+  ASSERT_EQ (NULL, prefix.plist->next->next->next->next);
+}
+
+/* Verify adding a prefix.  */
+
+static void
+file_find_verify_prefix_add (void)
+{
+  path_prefix prefix;
+  memset (&prefix, 0, sizeof (prefix));
+  prefix_from_string (env1, &prefix);
+
+  add_prefix (&prefix, purge ("#root"));
+  ASSERT_STREQ (purge ("#home#user#bin#"), prefix.plist->prefix);
+  ASSERT_STREQ (purge ("#root#"),
+		prefix.plist->next->next->next->next->prefix);
+
+  add_prefix_begin (&prefix, purge ("#var"));
+  ASSERT_STREQ (purge ("#var#"), prefix.plist->prefix);
+}
+
+/* Verify adding a prefix.  */
+
+static void
+file_find_verify_prefix_removal (void)
+{
+  path_prefix prefix;
+  memset (&prefix, 0, sizeof (prefix));
+  prefix_from_string (env1, &prefix);
+
+  /* All occurences of a prefix should be removed.  */
+  remove_prefix (purge ("#home#user#bin"), &prefix);
+
+  ASSERT_EQ (9, prefix.max_len);
+  ASSERT_STREQ (purge ("#bin#"), prefix.plist->prefix);
+  ASSERT_STREQ (purge ("#usr#bin#"), prefix.plist->next->prefix);
+  ASSERT_EQ (NULL, prefix.plist->next->next);
+
+  remove_prefix (purge ("#usr#bin#"), &prefix);
+  ASSERT_EQ (5, prefix.max_len);
+  ASSERT_STREQ (purge ("#bin#"), prefix.plist->prefix);
+  ASSERT_EQ (NULL, prefix.plist->next);
+
+  remove_prefix (purge ("#dev#random#"), &prefix);
+  remove_prefix (purge ("#bi#"), &prefix);
+
+  remove_prefix (purge ("#bin#"), &prefix);
+  ASSERT_EQ (NULL, prefix.plist);
+  ASSERT_EQ (0, prefix.max_len);
+
+  memset (&prefix, 0, sizeof (prefix));
+  prefix_from_string (env2, &prefix);
+  ASSERT_EQ (6, prefix.max_len);
+
+  remove_prefix (purge ("#root#"), &prefix);
+  ASSERT_EQ (NULL, prefix.plist);
+  ASSERT_EQ (0, prefix.max_len);
+}
+
+/* Run all of the selftests within this file.  */
+
+void file_find_c_tests ()
+{
+  file_find_verify_prefix_creation ();
+  file_find_verify_prefix_add ();
+  file_find_verify_prefix_removal ();
 }
+
+} // namespace selftest
+#endif /* CHECKING_P */
diff --git a/gcc/gcc-ar.c b/gcc/gcc-ar.c
index 78d2fc1ad30..1155ba83e35 100644
--- a/gcc/gcc-ar.c
+++ b/gcc/gcc-ar.c
@@ -194,15 +194,20 @@  main (int ac, char **av)
 #ifdef CROSS_DIRECTORY_STRUCTURE
       real_exe_name = concat (target_machine, "-", PERSONALITY, NULL);
 #endif
-      /* Do not search original location in the same folder.  */
-      char *exe_folder = lrealpath (av[0]);
-      exe_folder[strlen (exe_folder) - strlen (lbasename (exe_folder))] = '\0';
-      char *location = concat (exe_folder, PERSONALITY, NULL);
+      char *wrapper_file = lrealpath (av[0]);
+      exe_name = lrealpath (find_a_file (&path, real_exe_name, X_OK));
 
-      if (access (location, X_OK) == 0)
-	remove_prefix (exe_folder, &path);
+      /* If the exe_name points to the wrapper, remove folder of the wrapper
+	 from prefix and try search again.  */
+      if (strcmp (exe_name, wrapper_file) == 0)
+	{
+	  char *exe_folder = wrapper_file;
+	  exe_folder[strlen (exe_folder) - strlen (lbasename (exe_folder))] = '\0';
+	  remove_prefix (exe_folder, &path);
+
+	  exe_name = find_a_file (&path, real_exe_name, X_OK);
+	}
 
-      exe_name = find_a_file (&path, real_exe_name, X_OK);
       if (!exe_name)
 	{
 	  fprintf (stderr, "%s: Cannot find binary '%s'\n", av[0],
diff --git a/gcc/selftest-run-tests.c b/gcc/selftest-run-tests.c
index 30e476d14c5..dadc71a7c2a 100644
--- a/gcc/selftest-run-tests.c
+++ b/gcc/selftest-run-tests.c
@@ -66,6 +66,7 @@  selftest::run_tests ()
   sreal_c_tests ();
   fibonacci_heap_c_tests ();
   typed_splay_tree_c_tests ();
+  file_find_c_tests ();
 
   /* Mid-level data structures.  */
   input_c_tests ();
diff --git a/gcc/selftest.h b/gcc/selftest.h
index 0572fefd281..cfa3a82bc60 100644
--- a/gcc/selftest.h
+++ b/gcc/selftest.h
@@ -197,6 +197,7 @@  extern void tree_cfg_c_tests ();
 extern void vec_c_tests ();
 extern void wide_int_cc_tests ();
 extern void predict_c_tests ();
+extern void file_find_c_tests ();
 
 extern int num_passes;
 
-- 
2.13.3