[Bug gcov-profile/94570] New: -fprofile-dir is broken on Cygwin

[Bug gcov-profile/94570] New: -fprofile-dir is broken on Cygwin

[john at selbie dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94570

            Bug ID: 94570
           Summary: -fprofile-dir is broken on Cygwin
           Product: gcc
           Version: 9.3.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: gcov-profile
          Assignee: unassigned at gcc dot gnu.org
          Reporter: john at selbie dot com
                CC: marxin at gcc dot gnu.org
  Target Milestone: ---

The following bug is unique to gcc and g++ running on Cygwin.  I can repro this
on both gcc 9.2 and 9.3.

Using almost any source file, execute the following to start the first phase of
a profile guided optimization on Cygwin with a target directory to store .gcda
files:

$ g++ anyprogram.cpp -o anyprogram -fprofile-generate -fprofile-dir=profiledata

Then run the compiled code to start the program:

$ ./anyprogram.exe

After the program completes, the following output is revealed to show that the
coverage code couldn't save the gcda file:

profiling:profiledata/#home#jselbie\anyprogram.gcda:Skip

The .gcda file is not crated.

You can even see the mangled string it in the resulting binary:

$ strings anyprogram.exe | grep jselbie
profiledata/#home#jselbie\anyprogram.gcda


I see two possible issues that's causing this.

First, the anyprogram.gcda string is getting appended with a back slash instead
of a forward slash

A quick cursory glace of GCC sources would suggest the issue is in
\gcc\coverage.c. Looking at the code for coverage_init inside gcc/converage.c
reveals the following:

          if (profile_data_prefix)
        {
#if HAVE_DOS_BASED_FILE_SYSTEM
     const char *separator = "\\";
    #else
     const char *separator = "/";
    #endif
     filename = concat (getpwd (), separator, filename, NULL);
     filename = mangle_path (filename);
     len = strlen (filename);
    }


Another cursory search of gcc sources suggest HAVE_DOS_BASED_FILE_SYSTEM is
defined by this preprocessor stuff:

#if defined(__MSDOS__) || defined(_WIN32) || defined(__OS2__) || defined
(__CYGWIN__)
#  ifndef HAVE_DOS_BASED_FILE_SYSTEM
#    define HAVE_DOS_BASED_FILE_SYSTEM 1
#  endif

Because HAVE_DOS_BASED_FILE_SYSTEM is getting defined for Cygwin, then
coverage_init is going to use a backslash separator as well. Whether CYGWIN
should be considered a DOS based file system or a special exception needs to be
made in coverage_init, well, I'm not sure.

The second issue is with the way the path gets mangled with # chars.  That's
not consistent with the Linux build (at least with g++ 7.5 that I have going).


The workaround for now is to skip using the -fprofile-dir flag and allow
default save behavior for the gcda flag.

[Bug gcov-profile/94570] -fprofile-dir is broken on Cygwin

[marxin at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94570

Martin Liška <marxin at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Assignee|unassigned at gcc dot gnu.org      |marxin at gcc dot gnu.org
   Last reconfirmed|                            |2020-04-14
     Ever confirmed|0                           |1
             Status|UNCONFIRMED                 |ASSIGNED

--- Comment #1 from Martin Liška <marxin at gcc dot gnu.org> ---
Mine.

[Bug gcov-profile/94570] -fprofile-dir is broken on Cygwin

[marxin at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94570

--- Comment #2 from Martin Liška <marxin at gcc dot gnu.org> ---
(In reply to John Selbie from comment #0)
> The following bug is unique to gcc and g++ running on Cygwin.  I can repro
> this on both gcc 9.2 and 9.3.
> 
> Using almost any source file, execute the following to start the first phase
> of a profile guided optimization on Cygwin with a target directory to store
> .gcda files:
> 
> $ g++ anyprogram.cpp -o anyprogram -fprofile-generate
> -fprofile-dir=profiledata
> 
> Then run the compiled code to start the program:
> 
> $ ./anyprogram.exe
> 
> After the program completes, the following output is revealed to show that
> the coverage code couldn't save the gcda file:
> 
> profiling:profiledata/#home#jselbie\anyprogram.gcda:Skip
> 
> The .gcda file is not crated.

Thank you for the report and the analysis.
I see, we mix '/' and *separator (in this case '\'). So a possible fix could
be:

diff --git a/gcc/coverage.c b/gcc/coverage.c
index 30ae84df90f..aa1d1b0d120 100644
--- a/gcc/coverage.c
+++ b/gcc/coverage.c
@@ -1199,6 +1199,13 @@ coverage_obj_finish (vec<constructor_elt, va_gc> *ctor)
 void
 coverage_init (const char *filename)
 {
+#if HAVE_DOS_BASED_FILE_SYSTEM
+  const char *dir_separator = "\\";
+#else
+  const char *dir_separator = "/";
+#endif
+
+
   int len = strlen (filename);
   int prefix_len = 0;

@@ -1215,12 +1222,7 @@ coverage_init (const char *filename)
         of filename in order to prevent file path clashing.  */
       if (profile_data_prefix)
        {
-#if HAVE_DOS_BASED_FILE_SYSTEM
-         const char *separator = "\\";
-#else
-         const char *separator = "/";
-#endif
-         filename = concat (getpwd (), separator, filename, NULL);
+         filename = concat (getpwd (), dir_separator, filename, NULL);
          filename = mangle_path (filename);
          len = strlen (filename);
        }
@@ -1238,7 +1240,7 @@ coverage_init (const char *filename)
   if (profile_data_prefix)
     {
       memcpy (da_file_name, profile_data_prefix, prefix_len);
-      da_file_name[prefix_len++] = '/';
+      da_file_name[prefix_len++] = *dir_separator;
     }
   memcpy (da_file_name + prefix_len, filename, len);
   strcpy (da_file_name + prefix_len + len, GCOV_DATA_SUFFIX);

> 
> You can even see the mangled string it in the resulting binary:
> 
> $ strings anyprogram.exe | grep jselbie
> profiledata/#home#jselbie\anyprogram.gcda
> 
> 
> I see two possible issues that's causing this.
> 
> First, the anyprogram.gcda string is getting appended with a back slash
> instead of a forward slash
> 
> A quick cursory glace of GCC sources would suggest the issue is in
> \gcc\coverage.c. Looking at the code for coverage_init inside
> gcc/converage.c reveals the following:
> 
>           if (profile_data_prefix)
>         {
> #if HAVE_DOS_BASED_FILE_SYSTEM
>      const char *separator = "\\";
>     #else
>      const char *separator = "/";
>     #endif
>      filename = concat (getpwd (), separator, filename, NULL);
>      filename = mangle_path (filename);
>      len = strlen (filename);
>     }
> 
> 
> Another cursory search of gcc sources suggest HAVE_DOS_BASED_FILE_SYSTEM is
> defined by this preprocessor stuff:
> 
> #if defined(__MSDOS__) || defined(_WIN32) || defined(__OS2__) || defined
> (__CYGWIN__)
> #  ifndef HAVE_DOS_BASED_FILE_SYSTEM
> #    define HAVE_DOS_BASED_FILE_SYSTEM 1
> #  endif
> 
> Because HAVE_DOS_BASED_FILE_SYSTEM is getting defined for Cygwin, then
> coverage_init is going to use a backslash separator as well. Whether CYGWIN
> should be considered a DOS based file system or a special exception needs to
> be made in coverage_init, well, I'm not sure.

I guess we should use backslashes, or?

> 
> The second issue is with the way the path gets mangled with # chars.  That's
> not consistent with the Linux build (at least with g++ 7.5 that I have
> going).

Yes, that's expected and it's the answer for PR85759.

> 
> 
> The workaround for now is to skip using the -fprofile-dir flag and allow
> default save behavior for the gcda flag.

[Bug gcov-profile/94570] -fprofile-dir is broken on Cygwin

[john at selbie dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94570

--- Comment #3 from John Selbie <john at selbie dot com> ---
I am not sure I agree.  But I do defer to your expertise.

The problem is that Cygwin is not really a DOS_BASED_FILE_SYSTEM.  It's
emulating Unix - including using forward-slashes everywhere.  So I don't know
why it's trying to embed backslashes in the code.

Wouldn't the fix be this:

@@ -1215,7 +1215,7 @@ coverage_init (const char *filename)
         of filename in order to prevent file path clashing.  */
       if (profile_data_prefix)
        {
-#if HAVE_DOS_BASED_FILE_SYSTEM
+#if HAVE_DOS_BASED_FILE_SYSTEM && !CYGWIN
          const char *separator = "\\";
 #else
          const char *separator = "/";


There's so reason to use backslashes at all, even for native Win32. Windows and
DOS can't handle forward slashes at the command line, but absolutely can handle
paths with forward slashes in code.  So an even simpler fix:

@@ -1215,12 +1215,7 @@ coverage_init (const char *filename)
         of filename in order to prevent file path clashing.  */
       if (profile_data_prefix)
        {
-#if HAVE_DOS_BASED_FILE_SYSTEM
-         const char *separator = "\\";
-#else
-         const char *separator = "/";
-#endif
-         filename = concat (getpwd (), separator, filename, NULL);
+         filename = concat (getpwd (), "/", filename, NULL);
          filename = mangle_path (filename);
          len = strlen (filename);
        }


But even then I'm skeptical. I had "hacked the binary" with a hex editor to
change the backslash to a forward slash.  Same error, even though the modified
path appears on the screen:

   profiling:profile/#home#jselbie#bench/anyprogram.gcda:Skip

I even hacked it the other way to use forward slashes more consistently.  Same
thing:

    profiling:profile\#home#jselbie#bench\anyprogram.gcda:Skip

So while I think forward-slash consistency is a key.  It's not the only bug
preventing the -fprofile-dir flag to work.

[Bug gcov-profile/94570] -fprofile-dir is broken on Cygwin

[marxin at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94570

Martin Liška <marxin at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |10walls at gmail dot com

--- Comment #4 from Martin Liška <marxin at gcc dot gnu.org> ---
(In reply to John Selbie from comment #3)
> I am not sure I agree.  But I do defer to your expertise.
> 
> The problem is that Cygwin is not really a DOS_BASED_FILE_SYSTEM.  It's
> emulating Unix - including using forward-slashes everywhere.  So I don't
> know why it's trying to embed backslashes in the code.

All right, so it's normal Unix file system. Can you please show me output
of the 'pwd' command?

> 
> Wouldn't the fix be this:
> 
> @@ -1215,7 +1215,7 @@ coverage_init (const char *filename)
>          of filename in order to prevent file path clashing.  */
>        if (profile_data_prefix)
>         {
> -#if HAVE_DOS_BASED_FILE_SYSTEM
> +#if HAVE_DOS_BASED_FILE_SYSTEM && !CYGWIN
>           const char *separator = "\\";
>  #else
>           const char *separator = "/";
> 
> 
> There's so reason to use backslashes at all, even for native Win32. Windows
> and DOS can't handle forward slashes at the command line, but absolutely can
> handle paths with forward slashes in code.  So an even simpler fix:

But I bet they don't use them as directory separator character. And that's what
we want for concatenation of -profile-dir and an object name.

> 
> @@ -1215,12 +1215,7 @@ coverage_init (const char *filename)
>          of filename in order to prevent file path clashing.  */
>        if (profile_data_prefix)
>         {
> -#if HAVE_DOS_BASED_FILE_SYSTEM
> -         const char *separator = "\\";
> -#else
> -         const char *separator = "/";
> -#endif
> -         filename = concat (getpwd (), separator, filename, NULL);
> +         filename = concat (getpwd (), "/", filename, NULL);
>           filename = mangle_path (filename);
>           len = strlen (filename);
>         }
> 
> 
> But even then I'm skeptical. I had "hacked the binary" with a hex editor to
> change the backslash to a forward slash.  Same error, even though the
> modified path appears on the screen:
> 
>    profiling:profile/#home#jselbie#bench/anyprogram.gcda:Skip

It goes through:

libgcc/libgcov-driver-system.c:
create_file_directory:

static int
create_file_directory (char *filename)
{
#if !defined(TARGET_POSIX_IO) && !defined(_WIN32)
  (void) filename;
  return -1;
#else
  char *s;

  s = filename;

  if (HAS_DRIVE_SPEC(s))
    s += 2;
  if (IS_DIR_SEPARATOR(*s))
    ++s;
  for (; *s != '\0'; s++)
    if (IS_DIR_SEPARATOR(*s))
      {
        char sep = *s;
        *s  = '\0';
...

so again, it uses IS_DIR_SEPARATOR which is:

#if defined(__MSDOS__) || defined(_WIN32) || defined(__OS2__) || defined
(__CYGWIN__)
...
#  define HAS_DRIVE_SPEC(f) HAS_DOS_DRIVE_SPEC (f)
#  define IS_DIR_SEPARATOR(c) IS_DOS_DIR_SEPARATOR (c)
#  define IS_ABSOLUTE_PATH(f) IS_DOS_ABSOLUTE_PATH (f)

That's why you can't only replace strings in the instrumented binary.

You can test the following patch:

diff --git a/include/filenames.h b/include/filenames.h
index 1ed441221ac..773ba4865b3 100644
--- a/include/filenames.h
+++ b/include/filenames.h
@@ -32,7 +32,7 @@ Foundation, Inc., 51 Franklin Street - Fifth Floor, Boston,
MA 02110-1301, USA.
 extern "C" {
 #endif

-#if defined(__MSDOS__) || defined(_WIN32) || defined(__OS2__) || defined
(__CYGWIN__)
+#if defined(__MSDOS__) || defined(_WIN32) || defined(__OS2__)
 #  ifndef HAVE_DOS_BASED_FILE_SYSTEM
 #    define HAVE_DOS_BASED_FILE_SYSTEM 1
 #  endif

I'm adding Cygwin port maintainer, he can help us with the path separators.

> 
> I even hacked it the other way to use forward slashes more consistently. 
> Same thing:
> 
>     profiling:profile\#home#jselbie#bench\anyprogram.gcda:Skip
> 
> So while I think forward-slash consistency is a key.  It's not the only bug
> preventing the -fprofile-dir flag to work.

[Bug gcov-profile/94570] -fprofile-dir is broken on Cygwin

[john at selbie dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94570

--- Comment #5 from John Selbie <john at selbie dot com> ---
> All right, so it's normal Unix file system. Can you please show me output
of the 'pwd' command?

jselbie@IRONMAIDEN ~/bench
$ pwd
/home/jselbie/bench


I'm happy to test your patches, but I've actually never built gcc from scratch
before.  I'd have to read the docs on how to do that.  Is it has as simple as
running "./configure" and "make" ?

[Bug gcov-profile/94570] -fprofile-dir is broken on Cygwin

[marxin at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94570

--- Comment #6 from Martin Liška <marxin at gcc dot gnu.org> ---
(In reply to John Selbie from comment #5)
> > All right, so it's normal Unix file system. Can you please show me output
> of the 'pwd' command?
> 
> jselbie@IRONMAIDEN ~/bench
> $ pwd
> /home/jselbie/bench

Which is a normal UNIX filesystem.

> 
> 
> I'm happy to test your patches, but I've actually never built gcc from
> scratch before.  I'd have to read the docs on how to do that.  Is it has as
> simple as running "./configure" and "make" ?

Please take a look here:
https://gcc.gnu.org/install/index.html

[Bug gcov-profile/94570] -fprofile-dir is broken on Cygwin

[10walls at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94570

--- Comment #7 from jon_y <10walls at gmail dot com> ---
Created attachment 48281
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=48281&action=edit
Alternate patch

I'm not sure if ltmain.sh is shared in the cygnus tree, but this patch should
work too.

[Bug gcov-profile/94570] -fprofile-dir is broken on Cygwin

[10walls at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94570

--- Comment #8 from jon_y <10walls at gmail dot com> ---
I forgot to mention that while Cygwin runs on Windows, applications should
always use UNIX style paths.

[Bug gcov-profile/94570] -fprofile-dir is broken on Cygwin

[10walls at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94570

--- Comment #9 from jon_y <10walls at gmail dot com> ---
if defined(__MSDOS__) || (defined(_WIN32) && ! defined(__CYGWIN__) ||
defined(__OS2__)

I've tested the above and confirmed that coverage.ii does have the correct
separator after updating filenames.h.

Cygwin may define _WIN32 if win32api headers are included, but mingw will never
define __CYGWIN__.

[Bug gcov-profile/94570] -fprofile-dir is broken on Cygwin

[10walls at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94570

jon_y <10walls at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #48281|0                           |1
        is obsolete|                            |

--- Comment #10 from jon_y <10walls at gmail dot com> ---
Created attachment 48282
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=48282&action=edit
Combined patch

Combined patch attached.

[Bug gcov-profile/94570] -fprofile-dir is broken on Cygwin

[marxin at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94570

--- Comment #11 from Martin Liška <marxin at gcc dot gnu.org> ---
Thank you jon.
I'm testing the patch and will send it soon to the GCC patches mailing list.

[Bug gcov-profile/94570] -fprofile-dir is broken on Cygwin

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94570

--- Comment #12 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Martin Liska <marxin@gcc.gnu.org>:

https://gcc.gnu.org/g:e9f799d25973fc38022c5ea71ed5a2bca58a847f

commit r10-7767-ge9f799d25973fc38022c5ea71ed5a2bca58a847f
Author: Martin Liska <mliska@suse.cz>
Date:   Fri Apr 17 09:22:51 2020 +0200

    Do not use HAVE_DOS_BASED_FILE_SYSTEM for Cygwin.

            PR gcov-profile/94570
            * ltmain.sh: Do not define HAVE_DOS_BASED_FILE_SYSTEM
            for CYGWIN.

            PR gcov-profile/94570
            * coverage.c (coverage_init): Use separator properly.

            PR gcov-profile/94570
            * filenames.h (defined): Do not define HAVE_DOS_BASED_FILE_SYSTEM
            for CYGWIN.

    Co-Authored-By: Jonathan Yong <10walls@gmail.com>

[Bug gcov-profile/94570] -fprofile-dir is broken on Cygwin

[marxin at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94570

Martin Liška <marxin at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
      Known to fail|                            |9.3.0
      Known to work|                            |10.0

--- Comment #13 from Martin Liška <marxin at gcc dot gnu.org> ---
Fixed on master so far.

[Bug gcov-profile/94570] -fprofile-dir is broken on Cygwin

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94570

--- Comment #14 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-9 branch has been updated by Martin Liska
<marxin@gcc.gnu.org>:

https://gcc.gnu.org/g:5c09a1b71bb96d1fd6488d040b65001807fd16c2

commit r9-8513-g5c09a1b71bb96d1fd6488d040b65001807fd16c2
Author: Martin Liska <mliska@suse.cz>
Date:   Mon Apr 20 11:26:58 2020 +0200

    Backport e9f799d25973fc38022c5ea71ed5a2bca58a847f

            Backport from mainline
            2020-04-17  Martin Liska  <mliska@suse.cz>
                        Jonathan Yong <10walls@gmail.com>

            PR gcov-profile/94570
            * ltmain.sh: Do not define HAVE_DOS_BASED_FILE_SYSTEM
            for CYGWIN.
            Backport from mainline
            2020-04-17  Martin Liska  <mliska@suse.cz>
                        Jonathan Yong <10walls@gmail.com>

            PR gcov-profile/94570
            * coverage.c (coverage_init): Use separator properly.
            Backport from mainline
            2020-04-17  Martin Liska  <mliska@suse.cz>
                        Jonathan Yong <10walls@gmail.com>

            PR gcov-profile/94570
            * filenames.h (defined): Do not define HAVE_DOS_BASED_FILE_SYSTEM
            for CYGWIN.

    Co-Authored-By: Jonathan Yong <10walls@gmail.com>

[Bug gcov-profile/94570] -fprofile-dir is broken on Cygwin

[marxin at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94570

Martin Liška <marxin at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
      Known to work|                            |9.3.1
             Status|ASSIGNED                    |RESOLVED
      Known to fail|9.3.0                       |

--- Comment #15 from Martin Liška <marxin at gcc dot gnu.org> ---
Fixed now.