[Bug other/108464] New: [13 Regression] Broken -fdebug-prefix-map since r13-3599

[Bug other/108464] New: [13 Regression] Broken -fdebug-prefix-map since r13-3599

[jakub at gcc dot gnu.org]

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

            Bug ID: 108464
           Summary: [13 Regression] Broken -fdebug-prefix-map since
                    r13-3599
           Product: gcc
           Version: 13.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: other
          Assignee: unassigned at gcc dot gnu.org
          Reporter: jakub at gcc dot gnu.org
  Target Milestone: ---

Starting with r13-3599-ge5c15eb183f17e806ad -fdebug-prefix-map stopped working
in some cases (breaks ccache testsuite among other things).

Reproducer:
mkdir -p foobar1/include foobar1/src
echo 'int test;' > foobar1/include/test.h
echo '#include <test.h>' > foobar1/src/test.c
ln -sf foobar1 foobar2
cd foobar2
gcc -S -I include/ -fdebug-prefix-map=`pwd`=XXXX -g -dA src/test.c -o test.s;
grep 'XXXX\|foobar' test.s

Before r13-3599, all the way back from r0-82686-gc8aea42ce2c691e4e8 when
-fdebug-prefix-map= has been introduced, this prints XXXX (once or multiple
times depending on exact version) and doesn't print any foobar strings.
Even r13-3598 prints
        .long   .LASF1  # DW_AT_comp_dir: "XXXX"
        .long   .LASF1  # Directory Entry: 0: "XXXX"
        .string "XXXX"
but starting with r13-3599 we print
        .long   .LASF1  # DW_AT_comp_dir: "/home/jakub/foobar2"
        .long   .LASF1  # Directory Entry: 0: "/home/jakub/foobar2"
        .string "/home/jakub/foobar2"
instead, which is wrong because user asked for remapping of /home/jakub/foobar2
and it wasn't done.

[Bug other/108464] [13 Regression] Broken -fdebug-prefix-map since r13-3599

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
           Priority|P3                          |P1
   Target Milestone|---                         |13.0
                 CC|                            |law at gcc dot gnu.org,
                   |                            |richard.purdie@linuxfoundat
                   |                            |ion.org
     Ever confirmed|0                           |1
   Last reconfirmed|                            |2023-01-19

[Bug other/108464] [13 Regression] Broken -fdebug-prefix-map since r13-3599

[richard.purdie at linuxfoundation dot org]

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

--- Comment #1 from Richard Purdie <richard.purdie at linuxfoundation dot org> ---
I can see why that would change behaviour with the patch :(.

It is a tough dilemma since without the patch, files might have prefixes
removed some of the time but not all the time, depending on whether they were
accessed via a symlink (or similar) or not. It was effectively impossible to
remap some prefix values meaning in some cases output files couldn't be build
deterministically.

One possible way to fix this issue might be to call realpath on DW_AT_comp_dir
so that we're always working on canonicalized paths? I'm not sure if that would
be acceptable or not, the challenge right now is that we're making comparisons
against things which aren't always definitive.

[Bug other/108464] [13 Regression] Broken -fdebug-prefix-map since r13-3599

[jakub at gcc dot gnu.org]

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

--- Comment #2 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Dunno about other uses of remap_filename, but for remap_debug_filename what
that commit changed doesn't seem to be ever appropriate.  Users pass often pass
what they get from pwd to -fdebug-prefix-map=, and that is usually not the
canonicalized path.  And DW_AT_comp_dir was that same path as well and IMHO it
would be a very bad idea to change that.  Debug info contains relative paths in
many cases, that also shouldn't be canonicalized, should be whatever user used
in -I etc. options or #include paths etc.
If one is mixing absolute paths and some are canonical and others aren't, one
can use multiple -fdebug-prefix-map= options.  But ignoring what user asked for
is just wrong.

[Bug other/108464] [13 Regression] Broken -fdebug-prefix-map since r13-3599

[richard.purdie at linuxfoundation dot org]

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

--- Comment #3 from Richard Purdie <richard.purdie at linuxfoundation dot org> ---
(In reply to Jakub Jelinek from comment #2)
> Dunno about other uses of remap_filename, but for remap_debug_filename what
> that commit changed doesn't seem to be ever appropriate.

I think it depends on your use case. For example if via debug-prefix-map you
say you want:

/somepath/somesources translated to /somewhere

and then some code references ../somesources and ../../somesources from it's
build tree, would you want the user to have to add mappings for those and any
other combination some build system might end up generating due to it's source
layout?

>  Users pass often
> pass what they get from pwd to -fdebug-prefix-map=, and that is usually not
> the canonicalized path.  And DW_AT_comp_dir was that same path as well and
> IMHO it would be a very bad idea to change that.  Debug info contains
> relative paths in many cases, that also shouldn't be canonicalized, should
> be whatever user used in -I etc. options or #include paths etc.
> If one is mixing absolute paths and some are canonical and others aren't,
> one can use multiple -fdebug-prefix-map= options.  But ignoring what user
> asked for is just wrong.

It can be argued that if you ask for my above example and it doesn't remap the
relative accesses, it is also just wrong.

I can say that with all the cross compiling we do (in Yocto Project), making
this handle the corner cases improved our binary reproducibility quite
significantly. gcc can revert the change but we'd just have to patch something
in to handle this.

Another possible option is to resolve both sides of the equation before
deciding to remap or not (i.e. the directory in debug-prefix-map and the
potential target)?

[Bug other/108464] [13 Regression] Broken -fdebug-prefix-map since r13-3599

[jakub at gcc dot gnu.org]

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

--- Comment #4 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Though, even for the other options the change doesn't seem to be a good idea as
is.
I think instead the syntax of those remapping options should be extended, so
that for each of the pair one can choose the behavior which is wanted.  Using
the OLD=NEW syntax
with the previous behavior, and some other to mark pairs where you want new
behavior
(dunno whether to mark those using say extra = at the end after NEW or whatever
else).
Then when populating file_prefix_map entries, one would record this
non-canonicalizing vs. canonicalizing flag and in the latter case canonicalize
the old_prefix path in the structure.
remap_filename then would ideally lazily lrealpath when seeing first map entry
with the bool flag indicating canonicalization.

[Bug other/108464] [13 Regression] Broken -fdebug-prefix-map since r13-3599

[richard.purdie at linuxfoundation dot org]

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

--- Comment #5 from Richard Purdie <richard.purdie at linuxfoundation dot org> ---
(In reply to Jakub Jelinek from comment #4)
> Though, even for the other options the change doesn't seem to be a good idea
> as is.
> I think instead the syntax of those remapping options should be extended, so
> that for each of the pair one can choose the behavior which is wanted. 
> Using the OLD=NEW syntax
> with the previous behavior, and some other to mark pairs where you want new
> behavior
> (dunno whether to mark those using say extra = at the end after NEW or
> whatever else).
> Then when populating file_prefix_map entries, one would record this
> non-canonicalizing vs. canonicalizing flag and in the latter case
> canonicalize the old_prefix path in the structure.
> remap_filename then would ideally lazily lrealpath when seeing first map
> entry with the bool flag indicating canonicalization.

Having some way to enable the behaviour would be very valuable to the places we
use prefix mapping so we could make that work FWIW.

[Bug other/108464] [13 Regression] Broken -fdebug-prefix-map since r13-3599

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                URL|                            |https://gcc.gnu.org/piperma
                   |                            |il/gcc-patches/2023-January
                   |                            |/610285.html
             Status|NEW                         |ASSIGNED
           Assignee|unassigned at gcc dot gnu.org      |jakub at gcc dot gnu.org

[Bug other/108464] [13 Regression] Broken -fdebug-prefix-map since r13-3599

[orion at cora dot nwra.com]

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

--- Comment #6 from Orion Poplawski <orion at cora dot nwra.com> ---
So, there was quite the initial flurry of activity here and then nothing.  Any
chance we could get some more movement here?  It's breaking a ccache test for
one.  Thank you.

[Bug other/108464] [13 Regression] Broken -fdebug-prefix-map since r13-3599

[jakub at gcc dot gnu.org]

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

--- Comment #7 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
The patches (3 variants) have been posted and are awaiting review.  I've pinged
them twice.

[Bug other/108464] [13 Regression] Broken -fdebug-prefix-map since r13-3599

[cvs-commit at gcc dot gnu.org]

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

--- Comment #8 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Jakub Jelinek <jakub@gcc.gnu.org>:

https://gcc.gnu.org/g:2eb0191aa104badf3cab127f7f87d371c0fef92b

commit r13-6576-g2eb0191aa104badf3cab127f7f87d371c0fef92b
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Fri Mar 10 10:21:23 2023 +0100

    file-prefix-map: Fix up -f*-prefix-map= [PR108464]

    On Tue, Nov 01, 2022 at 01:46:20PM -0600, Jeff Law via Gcc-patches wrote:
    > > This does cause a change of behaviour if users were previously relying
upon
    > > symlinks or absolute paths not being resolved.
    >
    > I'm not too worried about this scenario.

    As mentioned in the PR, this patch breaks e.g. ccache testsuite.

    I strongly doubt most of the users want such a behavior, because it
    makes all filenames absolute when -f*-prefix-map= options remap one
    absolute path to another one.
    Say if I'm in /tmp and /tmp is the canonical path and there is
    src/test.c file, with -fdebug-prefix-map=/tmp=/blah
    previously there would be DW_AT_comp_dir "/blah" and it is still there,
    but DW_AT_name which was previouly "src/test.c" (relative against
    DW_AT_comp_dir) is now "/blah/src/test.c" instead.

    Even worse, the canonicalization is only done on the remap_filename
    argument, but not on the old_prefix side.  That is e.g. what breaks
    ccache.  If there is
    /tmp/foobar1 directory and
    ln -sf foobar1 /tmp/foobar2
    cd /tmp/foobar2
    then -fdebug-prefix-map=`pwd`:/blah will just not work, while
    src/test.c will be canonicalized to /tmp/foobar1/src/test.c,
    old_prefix is still what the user provided which is /tmp/foobar2.
    User would need to change their uses to use -fdebug-prefix-map=`realpath
$(pwd)`=/blah

    I've created 3 patches for this.

    The first patch just reverts the patch (and its follow-up patch).

    The second introduces a new option, -f{,no}-canon-prefix-map which affects
    the behavior of -f{file,macro,debug,profile}-prefix-map=, if on it
    canonicalizes the old path of the prefix map option and compares that
    against the canonicalized filename for absolute paths but not relative.

    And last is like the second, but does that also for relative paths except
    for filenames with no / (or / or \ on DOS based fs).  So, the third patch
    gets an optional behavior of what has been on the trunk lately with the
    difference that the old_prefix is canonicalized by the compiler.

    Initially I've thought I'd just add some magic syntax to the OLD=NEW
    argument of those options (because there are 4 of them), but as noted
    in the comments, = is valid char in OLD (just not new), so it would
    be hard to figure out some syntax.  So instead a new option, which one
    can turn on and off for different -f*-prefix-map= options if needed.

    -fdebug-prefix-map=/path1=/mypath1 -fcanon-prefix-map \
    -fdebug-prefix-map=/path2=/mypath2 -fno-canon-prefix-map \
    -fdebug-prefix-map=/path3=/mypath3

    will use the old behavior for the /path1 and /path3 handling and
    the new one only for /path2 handling.

    This commit is the third patch described above.

    2023-03-10  Jakub Jelinek  <jakub@redhat.com>

            PR other/108464
            * common.opt (fcanon-prefix-map): New option.
            * opts.cc: Include file-prefix-map.h.
            (flag_canon_prefix_map): New variable.
            (common_handle_option): Handle OPT_fcanon_prefix_map.
            (gen_command_line_string): Ignore OPT_fcanon_prefix_map.
            * file-prefix-map.h (flag_canon_prefix_map): Declare.
            * file-prefix-map.cc (struct file_prefix_map): Add canonicalize
            member.
            (add_prefix_map): Initialize canonicalize member from
            flag_canon_prefix_map, and if true canonicalize it using lrealpath.
            (remap_filename): Revert 2022-11-01 and 2022-11-07 changes,
            use lrealpath result only for map->canonicalize map entries.
            * lto-opts.cc (lto_write_options): Ignore OPT_fcanon_prefix_map.
            * opts-global.cc (handle_common_deferred_options): Clear
            flag_canon_prefix_map at the start and handle
OPT_fcanon_prefix_map.
            * doc/invoke.texi (-fcanon-prefix-map): Document.
            (-ffile-prefix-map, -fdebug-prefix-map, -fprofile-prefix-map): Add
            see also for -fcanon-prefix-map.
            * doc/cppopts.texi (-fmacro-prefix-map): Likewise.

[Bug other/108464] [13 Regression] Broken -fdebug-prefix-map since r13-3599

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
             Status|ASSIGNED                    |RESOLVED

--- Comment #9 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Fixed now.