public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
From: Bernd Edlinger <bernd.edlinger@hotmail.de>
To: Nathan Sidwell <nathan@acm.org>, Joseph Myers <joseph@codesourcery.com>
Cc: "gcc-patches@gcc.gnu.org" <gcc-patches@gcc.gnu.org>,
	Jan Hubicka	<hubicka@ucw.cz>
Subject: Re: [PING**3] [PATCH] Force use of absolute path names for gcov
Date: Mon, 05 Jun 2017 16:31:00 -0000	[thread overview]
Message-ID: <AM4PR0701MB2162C57E13155CDC39E5D841E4CA0@AM4PR0701MB2162.eurprd07.prod.outlook.com> (raw)
In-Reply-To: <8480b83a-221a-9dbc-691f-cbecbb0d08d4@acm.org>

[-- Attachment #1: Type: text/plain, Size: 2249 bytes --]

On 06/05/17 13:11, Nathan Sidwell wrote:
> +Compile the source files additionally with @option{-fprofile-abs-path}
> +to create absolute path names in the @file{.gcno} files.  This allows
> +@command{gcov} to find the correct sources in projects with multiple
> +directories.
> 
> I think the second sentence could be better.  It's not that the sources 
> are in different directories, it's that the compiler is invoked with 
> different working directories.  How about
>    'This allows @command{gcov} to find the correct sources in projects
>     where compilations occur with different working directories.'
> 
> modify as you see fit.
> 

done.

> gcov_write_filename (const char *filename)
> +{
> +  char buf[1024];
> +  size_t len;
> 
> Ew.  (a) bad fixed length (b) bad stack frame bloat.  Please use malloc 
> & MAX_PATH_LEN (or whatever the right #define is).
> 

MAX_PATH is just a portability nightmare and may or may not be
defined or sometimes is called PATH_MAX etc.

But I see getcwd (NULL, 0) is used in gcov-tools already,
and although that is a posix extension, it seems to be safe to use,
because libiberty provides a fall-back.

So I would go for getcwd(NULL, 0) and xrealloc the result buffer
as needed.

> +  if (profile_abs_path_flag && filename && filename[0]
> 
> Can filename ever be null here?  Can it ever be the empty string?
> 

I am unable to prove that, if the call from coverage.c where
filename comes directly from expand_location and there are
numerous places like:

  if (xloc.file)
    chksum = coverage_checksum_string (chksum, xloc.file);

which tells me that xloc.file might be NULL, typically in
case of UNKNOWN_LOCATION.

I am anxious about empty string here because of the code in
#ifdef HAVE_DOS_BASED_FILE_SYSTEM does not work with empty string,
while the original gcov_write_string works fine with NULL string
and empty string.


> +      if (getcwd (buf, sizeof (buf) - len - 1) != NULL)
> 
> This is going to getcwd on every file (most likely).  Isn't this already 
> available somewhere?
> 
> 

No, at least grep does not see any other place, except in gcov-tool.c :(


So how about this new patch?
Is it OK for trunk?


Bernd.

[-- Attachment #2: changelog-gcov-abs-path.txt --]
[-- Type: text/plain, Size: 498 bytes --]

gcc:
2017-04-21  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	* doc/invoke.texi: Document the -fprofile-abs-path option.
	* common.opt (fprofile-abs-path): New option.
	* gcov-io.h (gcov_write_filename): Declare.
	* gcov-io.c (gcov_write_filename): New function.
	* coverage.c (coverage_begin_function): Use gcov_write_filename.
	* profile.c (output_location): Likewise.

gcc/testsuite:
2017-04-21  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	* gcc.misc-tests/gcov-1a.c: New test.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: patch-gcov-abs-path.diff --]
[-- Type: text/x-patch; name="patch-gcov-abs-path.diff", Size: 5549 bytes --]

Index: gcc/common.opt
===================================================================
--- gcc/common.opt	(revision 248852)
+++ gcc/common.opt	(working copy)
@@ -1969,6 +1969,10 @@ fprofile
 Common Report Var(profile_flag)
 Enable basic program profiling code.
 
+fprofile-abs-path
+Common Report Var(profile_abs_path_flag)
+Generate absolute source path names for gcov.
+
 fprofile-arcs
 Common Report Var(profile_arc_flag)
 Insert arc-based program profiling code.
Index: gcc/coverage.c
===================================================================
--- gcc/coverage.c	(revision 248852)
+++ gcc/coverage.c	(working copy)
@@ -663,7 +663,7 @@ coverage_begin_function (unsigned lineno_checksum,
   gcov_write_unsigned (cfg_checksum);
   gcov_write_string (IDENTIFIER_POINTER
 		     (DECL_ASSEMBLER_NAME (current_function_decl)));
-  gcov_write_string (xloc.file);
+  gcov_write_filename (xloc.file);
   gcov_write_unsigned (xloc.line);
   gcov_write_length (offset);
 
Index: gcc/doc/invoke.texi
===================================================================
--- gcc/doc/invoke.texi	(revision 248852)
+++ gcc/doc/invoke.texi	(working copy)
@@ -443,6 +443,7 @@ Objective-C and Objective-C++ Dialects}.
 @item Program Instrumentation Options
 @xref{Instrumentation Options,,Program Instrumentation Options}.
 @gccoptlist{-p  -pg  -fprofile-arcs  --coverage  -ftest-coverage @gol
+-fprofile-abs-path @gol
 -fprofile-dir=@var{path}  -fprofile-generate  -fprofile-generate=@var{path} @gol
 -fsanitize=@var{style}  -fsanitize-recover  -fsanitize-recover=@var{style} @gol
 -fasan-shadow-offset=@var{number}  -fsanitize-sections=@var{s1},@var{s2},... @gol
@@ -10694,6 +10695,12 @@ additional @option{-ftest-coverage} option.  You d
 every source file in a program.
 
 @item
+Compile the source files additionally with @option{-fprofile-abs-path}
+to create absolute path names in the @file{.gcno} files.  This allows
+@command{gcov} to find the correct sources in projects where compilations
+occur with different working directories.
+
+@item
 Link your object files with @option{-lgcov} or @option{-fprofile-arcs}
 (the latter implies the former).
 
@@ -10737,6 +10744,13 @@ above for a description of @var{auxname} and instr
 generate test coverage data.  Coverage data matches the source files
 more closely if you do not optimize.
 
+@item -fprofile-abs-path
+@opindex fprofile-abs-path
+Automatically convert relative source file names to absolute path names
+in the @file{.gcno} files.  This allows @command{gcov} to find the correct
+sources in projects where compilations occur with different working
+directories.
+
 @item -fprofile-dir=@var{path}
 @opindex fprofile-dir
 
Index: gcc/gcov-io.c
===================================================================
--- gcc/gcov-io.c	(revision 248852)
+++ gcc/gcov-io.c	(working copy)
@@ -357,6 +357,38 @@ gcov_write_string (const char *string)
 #endif
 
 #if !IN_LIBGCOV
+/* Write FILENAME to coverage file.  Sets error flag on file
+   error, overflow flag on overflow */
+
+GCOV_LINKAGE void
+gcov_write_filename (const char *filename)
+{
+  if (profile_abs_path_flag && filename && filename[0]
+      && !(IS_DIR_SEPARATOR (filename[0])
+#if HAVE_DOS_BASED_FILE_SYSTEM
+	   || filename[1] == ':'
+#endif
+	  ))
+    {
+      char *buf = getcwd (NULL, 0);
+      if (buf != NULL && buf[0])
+	{
+	  size_t len = strlen (buf);
+	  buf = (char*)xrealloc (buf, len + strlen (filename) + 2);
+	  if (!IS_DIR_SEPARATOR (buf[len - 1]))
+	    strcat (buf, "/");
+	  strcat (buf, filename);
+	  gcov_write_string (buf);
+	  free (buf);
+	  return;
+	}
+    }
+
+  gcov_write_string (filename);
+}
+#endif
+
+#if !IN_LIBGCOV
 /* Write a tag TAG and reserve space for the record length. Return a
    value to be used for gcov_write_length.  */
 
Index: gcc/gcov-io.h
===================================================================
--- gcc/gcov-io.h	(revision 248852)
+++ gcc/gcov-io.h	(working copy)
@@ -387,6 +387,7 @@ GCOV_LINKAGE void gcov_write_unsigned (gcov_unsign
 /* Available only in compiler */
 GCOV_LINKAGE unsigned gcov_histo_index (gcov_type value);
 GCOV_LINKAGE void gcov_write_string (const char *);
+GCOV_LINKAGE void gcov_write_filename (const char *);
 GCOV_LINKAGE gcov_position_t gcov_write_tag (gcov_unsigned_t);
 GCOV_LINKAGE void gcov_write_length (gcov_position_t /*position*/);
 #endif
Index: gcc/profile.c
===================================================================
--- gcc/profile.c	(revision 248852)
+++ gcc/profile.c	(working copy)
@@ -954,7 +954,7 @@ output_location (char const *file_name, int line,
     {
       prev_file_name = file_name;
       gcov_write_unsigned (0);
-      gcov_write_string (prev_file_name);
+      gcov_write_filename (prev_file_name);
     }
   if (line_differs)
     {
Index: gcc/testsuite/gcc.misc-tests/gcov-1a.c
===================================================================
--- gcc/testsuite/gcc.misc-tests/gcov-1a.c	(revision 0)
+++ gcc/testsuite/gcc.misc-tests/gcov-1a.c	(working copy)
@@ -0,0 +1,20 @@
+/* Test Gcov basics.  */
+
+/* { dg-options "-fprofile-arcs -ftest-coverage -fprofile-abs-path" } */
+/* { dg-do run { target native } } */
+
+void noop ()
+{
+}
+
+int main ()
+{
+  int i;
+
+  for (i = 0; i < 10; i++)	/* count(11) */
+    noop ();			/* count(10) */
+
+  return 0;			/* count(1) */
+}
+
+/* { dg-final { run-gcov gcov-1a.c } } */

  reply	other threads:[~2017-06-05 16:31 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-04-21 18:51 Bernd Edlinger
2017-04-21 20:27 ` Joseph Myers
2017-04-21 20:57   ` Bernd Edlinger
2017-04-28 18:14     ` [PING] " Bernd Edlinger
2017-05-12 16:48       ` [PING**2] " Bernd Edlinger
2017-06-01 15:59         ` [PING**3] " Bernd Edlinger
2017-06-01 17:52           ` Nathan Sidwell
2017-06-01 19:24             ` Bernd Edlinger
2017-06-02 11:35               ` Nathan Sidwell
2017-06-02 14:43                 ` Bernd Edlinger
2017-06-05 11:11                   ` Nathan Sidwell
2017-06-05 16:31                     ` Bernd Edlinger [this message]
2017-06-05 19:00                       ` Nathan Sidwell

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=AM4PR0701MB2162C57E13155CDC39E5D841E4CA0@AM4PR0701MB2162.eurprd07.prod.outlook.com \
    --to=bernd.edlinger@hotmail.de \
    --cc=gcc-patches@gcc.gnu.org \
    --cc=hubicka@ucw.cz \
    --cc=joseph@codesourcery.com \
    --cc=nathan@acm.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).