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:
Yet another attempt. This update includes:
  - use IS_DIR_SEPARATOR instead of DIR_SEPARATOR{,_2}
  - optimized disk traffic regarding to dir creation
  - optimized memory usage in create_file_directory
  - simplified string length comparison
  - avoiding info structure modification
  - user documentation updated with macro dependencies
Some notes below...


This usage model is logically correct but seems to be a bit complicated.
I'd suggest user to specify exact path to place data files on the target
system, more likely using absolute path prefix. Otherwise relocation is
meaningless. Implicitly allowing non-absolute paths we'll reach troubles
with application changing working directory back and forth. There is more general question: should we allow relative GCOV_PREFIX at all?

yeah, I don't know :) Let's just document that it must be an absolute path.



  static int
! create_file_directory (char *filename)
Please add a comment saying what the return value means, and that
filename must be writeable, and what this attempts to do.  I see
some other existing functions don't have such comments, which is
bad.  If you want to add them feel free, otherwise I'll take care
of it later.

! {
! char *s;
! ! if (!filename || *filename == '\0')
! return -2;
Can this situation occur?  I don't think so.  If you're paranoid
replace this with a gcc_assert.

+ /* Get file name relocation prefix. */
+ gcov_prefix = getenv("GCOV_PREFIX");
+ if (gcov_prefix && *gcov_prefix != '\0')
+ {
+ size_t prefix_length = strlen(gcov_prefix);
+ + /* Check if the level of dirs to strip off specified. */
+ char *tmp = getenv("GCOV_PREFIX_STRIP");
+ if (tmp)
doesn't this need to be 'tmp && *tmp'?


+       /* Store and mormalize filename prefix. */
+       strcpy (gi_filename, gcov_prefix);
+       gi_filename_up = gi_filename + prefix_length;
+       if (gi_filename_up > gi_filename && IS_DIR_SEPARATOR(*(gi_filename_up - 1)))
+         gi_filename_up--;
We know prefix_length is non-zero, because of the previous check
'gcov_prefix && *gcov_prefix', so this need only have the second operand of the &&.


Index: gcc/doc/gcov.texi

+ @itemize @bullet
+ @item
+ GCOV_PREFIX contains the prefix to add to the absolute paths + in the object file. The default is no prefix.
Add something to emphasize that this must be an absolute path.

If you'd care to make those changes, you may commit.  Thanks for iterating
on this.

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]