This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [4.1 patch] relocate profile data file


Grigory Zagorodnev wrote:
Hi,
This is updated libgcov relocation patch. It includes:
 - info pages rewording
 - getenv moved to gcov_exit
 - full filename generating in gcov_exit not in callees
 - single alloca with respect to longest file name
 - create_file_directory is still there (see reasons below)

We try to keep libgcov error messages clear, distinguishing between directory creation failure and file open failure. It appears that routine gcov_open is not suitable to emit messages because it is originally silent. At the same time gcov_open's return value is not suitable to handle two failures in the caller because only one value reserved for error condition. Thus it is impossible to put directory creation into gcov_open without significant interface changes.

thanks for the explanation, unfortunately there are still some problems with the patch. Some are minor, but there are a couple of biggies ...

+ static int
+ create_file_directory (const char *filename, char sep)
You can't just use 'sep' here, filesystem that allow two
separator characters can mix them in a single path -- and, yes
this happens in real life.  Use the DIR_SEPARATOR{,_2} #defines.

As that'll stop you using strrchr, perhaps the best way of dealing
with it is for create_directory to scan forwards creating each
one in the path.  This will create unneccessary disk traffic, if
the directory exists, to prevent that, how about arranging the
caller to look like
	fd = gcov_open (...);
	if (!fd)
		creat_directory (..)
		gcov_open (..)
		if (!fd) error
?


+   /* Get a copy of the name so we can modify it. */
+   r = alloca(strlen(filename) + 1);
There's no real need to copy filename, as it is modifiable
(gcov_exit passes in a pointer to the alloced area), so you
don't really need to const qualify it.

+   /* Skip consecutive separators.  */
+   for (dname = r; *dname && *dname == sep; ++dname);
This seems unnecessary, what harm will result (other than
more disk accesses -- a suitable punishment for user stupidity :)

+   const char *gcov_prefix;
+   int gcov_prefix_strip = 0;
Now these are no longer global, there's really no need for the
gcov_ prefix

+ char *gi_filename, *gi_filename_up;
+ char dir_separator, *s;
memset (&all, 0, sizeof (all));
/* Find the totals for this execution. */
*************** gcov_exit (void)
*** 151,156 ****
--- 202,244 ----
}
}
+ /* Get file name relocation prefix. */
+ gcov_prefix = getenv("GCOV_PREFIX");
+ if (gcov_prefix)
+ {
+ /* Check if the level of dirs to strip off specified. */
+ char *tmp = getenv("GCOV_PREFIX_STRIP");
+ if (tmp)
+ {
+ gcov_prefix_strip = atoi (tmp);
+ /* Do not consider negative values. */
+ if (gcov_prefix_strip < 0)
+ gcov_prefix_strip = 0;
you only look at GCOV_PREFIX_STRIP if GCOV_PREFIX is non-empty.
This is not documented.  Maybe you should look at both independently,
and if GCOV_PREFIX is empty, but GCOV_PREFIX_STRIP is not set (but might
or might not be zero), then set the prefix to '.'.  I think that
would match user expectation in that situation.

+ else + gcov_prefix = "";
Why not just use NULL, and then test for that later, rather
than dereference it?  You'll have to insert a check above
to see if GCOV_PREFIX is empty.

+ /* It seems to be safe to update info block with the pointer + to stack area - each block is used once within the loop
+ and will be unloaded after gcov_exit return. */
+ gi_ptr->filename = gi_filename;
this is not true.  gcov_exit can be called multiple times, if the
program so chooses, or calls fork or exec


+       /* Refresh the longest file name information */
+       ptr = info->filename + strlen(info->filename);
+       if (gcov_max_filename < (size_t)(ptr - info->filename))
+         gcov_max_filename = ptr - info->filename;
This is a really funky way of doing string length comparison.  Please
simplify it.

nathan

--
Nathan Sidwell    ::   http://www.codesourcery.com   ::     CodeSourcery LLC
nathan@codesourcery.com    ::     http://www.planetfall.pwp.blueyonder.co.uk


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]