Re: [libcpp] maybe canonicalize system paths in line-map

[Dodji Seketeli <dodji at seketeli dot org>]

Jason Merrill <jason@redhat.com> a Ãcrit:

> It seems like we'll do this for every line in the header, which could
> lead to a lot of leaked memory.  Instead, we should canonicalize when
> setting ORDINARY_MAP_FILE_NAME.

[...]

Manuel LÃpez-IbÃÃez <lopezibanez@gmail.com> a Ãcrit:

> On 21 April 2012 14:56, Jason Merrill <jason@redhat.com> wrote:
>> It seems like we'll do this for every line in the header, which could lead
>> to a lot of leaked memory. ÂInstead, we should canonicalize when setting
>> ORDINARY_MAP_FILE_NAME.
>
> Hum, my understanding of the code is that this is exactly what I
> implemented. That is, at most we leak every time
> ORDINARY_MAP_FILE_NAME is set to a system header file (if the realpath
> is shorter than the original path). This is a bit inefficient because
> this happens two times per #include (when entering and when leaving).
> But I honestly don't know how to avoid this.

My understanding is that by the time we are about to deal with the
ORDINARY_MAP_FILE_NAME property of a given map, it's too late; we have
lost the "memory leak game" because that property just points to the
path carried by the instance _cpp_file::path that is current when the
map is built.  So the lifetime of that property is tied to the lifetime
of the relevant instance of _cpp_file.

So maybe it'd be better to canonicalize the _cpp_file::path when it's
first build?  One drawback of that approach would be that
_cpp_file::path will then permanently loose the information about the
current directory, that is indirectly encoded into the way the file path
is originally built.  But do we really need that information?

If you agree with that approach, the patch below might do the trick.  I
have only lightly tested it and am about to bootstrap it to double
check.

On the test case below:

     1	#include <algorithm>
     2	
     3	void
     4	f ()
     5	{
     6	    int a = 0;
     7	    std::sort (a);
     8	}

It yields an output that looks like:

---------->8<----------------
test.cc: In function âvoid f()â:
test.cc:7:17: erreur: no matching function for call to âsort(int&)â
     std::sort (a);
                 ^
test.cc:7:17: note: candidates are:
     std::sort (a);
                 ^
In file included from /home/dodji/devel/git/gcc/tmp/libstdc++-v3/include/std/algorithm:63:0,
                 from test.cc:1:
/home/dodji/devel/git/gcc/tmp/libstdc++-v3/include/bits/stl_algo.h:5463:5: note: template<class _RAIter> void std::sort(_RAIter, _RAIter)
     sort(_RandomAccessIterator __first, _RandomAccessIterator __last)
     ^
/home/dodji/devel/git/gcc/tmp/libstdc++-v3/include/bits/stl_algo.h:5463:5: note:   template argument deduction/substitution failed:
     sort(_RandomAccessIterator __first, _RandomAccessIterator __last)
     ^
test.cc:7:17: note:   candidate expects 2 arguments, 1 provided
     std::sort (a);
                 ^
In file included from /home/dodji/devel/git/gcc/tmp/libstdc++-v3/include/std/algorithm:63:0,
                 from test.cc:1:
/home/dodji/devel/git/gcc/tmp/libstdc++-v3/include/bits/stl_algo.h:5499:5: note: template<class _RAIter, class _Compare> void std::sort(_RAIter, _RAIter, _Compare)
     sort(_RandomAccessIterator __first, _RandomAccessIterator __last,
     ^
/home/dodji/devel/git/gcc/tmp/libstdc++-v3/include/bits/stl_algo.h:5499:5: note:   template argument deduction/substitution failed:
     sort(_RandomAccessIterator __first, _RandomAccessIterator __last,
     ^
test.cc:7:17: note:   candidate expects 3 arguments, 1 provided
     std::sort (a);
                 ^
---------->8<----------------



>
> If any one has suggestions on a better implementation, I am happy to
> hear about it.

So here is the patch; it just borrows your implementation of
maybe_shorter_path and uses it in file_file_in_dir.  There should not be
any leak, IIUC.

diff --git a/libcpp/files.c b/libcpp/files.c
index 29ccf3b..147963f 100644
--- a/libcpp/files.c
+++ b/libcpp/files.c
@@ -341,6 +341,27 @@ pch_open_file (cpp_reader *pfile, _cpp_file *file, bool *invalid_pch)
   return valid;
 }
 
+/* Canonicalize the path to FILE. Return the canonical form if it is
+   shorter, otherwise return the original.  This function may free the
+   memory pointed by FILE.  */
+
+static char *
+maybe_shorter_path (const char * file)
+{
+  const char * file2 = lrealpath (file);
+  if (file2 && strlen (file2) < strlen (file))
+    {
+      /* Unfortunately, it is not safe to delete file, so we may leak
+	 some memory.  */
+      return file2;
+    }
+  else 
+    {
+      XDELETEVEC (file2);
+      return file;
+    }
+}
+
 /* Try to open the path FILE->name appended to FILE->dir.  This is
    where remap and PCH intercept the file lookup process.  Return true
    if the file was found, whether or not the open was successful.
@@ -349,7 +370,7 @@ pch_open_file (cpp_reader *pfile, _cpp_file *file, bool *invalid_pch)
 static bool
 find_file_in_dir (cpp_reader *pfile, _cpp_file *file, bool *invalid_pch)
 {
-  char *path;
+  char *path, *canonical_path;
 
   if (CPP_OPTION (pfile, remap) && (path = remap_filename (pfile, file)))
     ;
@@ -359,6 +380,15 @@ find_file_in_dir (cpp_reader *pfile, _cpp_file *file, bool *invalid_pch)
     else
       path = append_file_to_dir (file->name, file->dir);
 
+  canonical_path = maybe_shorter_path (path);
+  if (canonical_path != NULL && canonical_path != path)
+    {
+      /* The canonical path was newly allocated.  Let's free the
+	 non-canonical one.  */
+      free (path);
+      path = canonical_path;
+    }
+
   if (path)
     {
       hashval_t hv = htab_hash_string (path);

-- 
		Dodji