diff mbox series

[1/2] gcc/file-prefix-map: Allow remapping of relative paths

Message ID 20220817121534.1825108-1-richard.purdie@linuxfoundation.org
State New, archived
Headers
Series [1/2] gcc/file-prefix-map: Allow remapping of relative paths |

Commit Message

Richard Purdie Aug. 17, 2022, 12:15 p.m. UTC 
  Relative paths currently aren't remapped by -ffile-prefix-map and friends.
When cross compiling with separate 'source' and 'build' directories, the same
relative paths between directories may not be available on target as compared
to build time.

In order to be able to remap these relative build paths to paths that would
work on target, resolve paths within the file-prefix-map function using
realpath().

This does cause a change of behaviour if users were previously relying upon
symlinks or absolute paths not being resolved.

Use basename to ensure plain filenames don't have paths added.

gcc/ChangeLog:

    * file-prefix-map.cc (remap_filename): Allow remapping of relative paths

Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
---
 gcc/file-prefix-map.cc | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

Comments

Jeff Law Nov. 1, 2022, 7:46 p.m. UTC | #1 
On 8/17/22 06:15, Richard Purdie via Gcc-patches wrote:
> Relative paths currently aren't remapped by -ffile-prefix-map and friends.
> When cross compiling with separate 'source' and 'build' directories, the same
> relative paths between directories may not be available on target as compared
> to build time.
>
> In order to be able to remap these relative build paths to paths that would
> work on target, resolve paths within the file-prefix-map function using
> realpath().

Understood.


>
> 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.


>
> Use basename to ensure plain filenames don't have paths added.
>
> gcc/ChangeLog:
>
>      * file-prefix-map.cc (remap_filename): Allow remapping of relative paths

Basically OK.  Just formatting nit:



>
> Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
> ---
>   gcc/file-prefix-map.cc | 15 ++++++++++++---
>   1 file changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/gcc/file-prefix-map.cc b/gcc/file-prefix-map.cc
> index 24733f831d6..50d5d724a8f 100644
> --- a/gcc/file-prefix-map.cc
> +++ b/gcc/file-prefix-map.cc
> @@ -70,19 +70,28 @@ remap_filename (file_prefix_map *maps, const char *filename)
>     file_prefix_map *map;
>     char *s;
>     const char *name;
> +  char *realname;
>     size_t name_len;
>   
> +  if (lbasename (filename) == filename)
> +    return filename;
> +
> +  realname = lrealpath (filename);
> +
>     for (map = maps; map; map = map->next)
> -    if (filename_ncmp (filename, map->old_prefix, map->old_len) == 0)
> +    if (filename_ncmp (realname, map->old_prefix, map->old_len) == 0)
>         break;
> -  if (!map)
> +  if (!map) {
> +    free (realname);
>       return filename;
> -  name = filename + map->old_len;
> +  }


Put the the curley braces go on their own lines, indented two 
positions.  The code inside the curleys is indented two more 
positions.   I fixed that and pushed this change to the trunk.


THanks,

jeff
Eric Botcazou Nov. 4, 2022, 9:12 a.m. UTC | #2 
> gcc/ChangeLog:
> 
>     * file-prefix-map.cc (remap_filename): Allow remapping of relative paths

Small regression in Ada though, probably a missing guard somewhere:

                === gnat tests ===


Running target unix
FAIL: gnat.dg/specs/coverage1.ads (test for excess errors)

In order to reproduce, configure the compiler with Ada enabled, build it, and 
copy $[srcdir)/gcc/testsuite/gnat.dg/specs/coverage1.ads into the build 
directory, then just issue:

gcc/gnat1 -quiet coverage1.ads -ftest-coverage -Igcc/ada/rts

raised STORAGE_ERROR : stack overflow or erroneous memory access
Richard Purdie Nov. 4, 2022, 5:21 p.m. UTC | #3 
On Fri, 2022-11-04 at 10:12 +0100, Eric Botcazou wrote:
> > gcc/ChangeLog:
> > 
> >     * file-prefix-map.cc (remap_filename): Allow remapping of
> > relative paths
> 
> Small regression in Ada though, probably a missing guard somewhere:
> 
>                 === gnat tests ===
> 
> 
> Running target unix
> FAIL: gnat.dg/specs/coverage1.ads (test for excess errors)
> 
> In order to reproduce, configure the compiler with Ada enabled, build
> it, and 
> copy $[srcdir)/gcc/testsuite/gnat.dg/specs/coverage1.ads into the
> build 
> directory, then just issue:
> 
> gcc/gnat1 -quiet coverage1.ads -ftest-coverage -Igcc/ada/rts
> 
> raised STORAGE_ERROR : stack overflow or erroneous memory access

It took me a while to work out how to get ada to build. When I did I
found it was faulting due to a NULL filename being passed to lbasename:

Program received signal SIGSEGV, Segmentation fault.
lbasename (name=0x0) at gcc/libiberty/lbasename.c:82
82	  return unix_lbasename (name);
(gdb) bt
#0  lbasename (name=0x0) at gcc/libiberty/lbasename.c:82
#1  0x0000000000f3d566 in remap_filename (maps=0x0, filename=filename@entry=0x0) at gcc/gcc/file-prefix-map.cc:76
#2  0x0000000000f3d6df in remap_profile_filename (filename=filename@entry=0x0) at gcc/gcc/file-prefix-map.cc:158
#3  0x0000000000e59f59 in coverage_begin_function (lineno_checksum=lineno_checksum@entry=595732889, cfg_checksum=cfg_checksum@entry=2754642872)
    at gcc/gcc/coverage.cc:650
#4  0x00000000012263c0 in branch_prob (thunk=thunk@entry=false) at gcc/gcc/profile.cc:1400
#5  0x00000000013b1205 in tree_profiling () at gcc/gcc/tree-profile.cc:782
#6  (anonymous namespace)::pass_ipa_tree_profile::execute (this=<optimised out>) at gcc/gcc/tree-profile.cc:888
#7  0x00000000011ef30b in execute_one_pass (pass=0x36ebea0) at gcc/gcc/passes.cc:2644
#8  0x00000000011f0697 in execute_ipa_pass_list (pass=0x36ebea0) at gcc/gcc/passes.cc:3093
#9  0x0000000000e4c28d in ipa_passes () at gcc/gcc/cgraphunit.cc:2170
#10 symbol_table::compile (this=0x7ffff718f000) at gcc/gcc/cgraphunit.cc:2291
#11 0x0000000000e4ec08 in symbol_table::compile (this=0x7ffff718f000) at gcc/gcc/cgraphunit.cc:2271
#12 symbol_table::finalize_compilation_unit (this=0x7ffff718f000) at gcc/gcc/cgraphunit.cc:2543
#13 0x00000000012cfea0 in compile_file () at gcc/gcc/toplev.cc:471
#14 0x00000000009a727c in do_compile (no_backend=false) at gcc/gcc/toplev.cc:2125
#15 toplev::main (this=this@entry=0x7fffffffe21e, argc=<optimised out>, argc@entry=4, argv=<optimised out>, argv@entry=0x7fffffffe348) at gcc/gcc/toplev.cc:2277
#16 0x00000000009a8a8b in main (argc=4, argv=0x7fffffffe348) at gcc/gcc/main.cc:39

I can send the obvious patch to make it work as before and ignore the
NULL which fixes this. I'm not sure if it should really be passing NULL
around in the first place or not which is why I'm sharing the
backtrace.

Cheers,

Richard
Eric Botcazou Nov. 7, 2022, 8:01 a.m. UTC | #4 
> I can send the obvious patch to make it work as before and ignore the
> NULL which fixes this. I'm not sure if it should really be passing NULL
> around in the first place or not which is why I'm sharing the backtrace.

Thanks for the investigation.  That's because the DECL_SOURCE_LOCATION of the 
function is UNKNOWN_LOCATION since:

2021-08-11  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	PR debug/101598
	* gcc-interface/trans.c (Subprogram_Body_to_gnu): Set the
	DECL_SOURCE_LOCATION of DECL_IGNORED_P gnu_subprog_decl to
	UNKNOWN_LOCATION.

That's indeed quite irregular but we have to live with it now.
Jeff Law Nov. 7, 2022, 4:21 p.m. UTC | #5 
On 11/7/22 01:01, Eric Botcazou via Gcc-patches wrote:
>> I can send the obvious patch to make it work as before and ignore the
>> NULL which fixes this. I'm not sure if it should really be passing NULL
>> around in the first place or not which is why I'm sharing the backtrace.
> Thanks for the investigation.  That's because the DECL_SOURCE_LOCATION of the
> function is UNKNOWN_LOCATION since:
>
> 2021-08-11  Bernd Edlinger  <bernd.edlinger@hotmail.de>
>
> 	PR debug/101598
> 	* gcc-interface/trans.c (Subprogram_Body_to_gnu): Set the
> 	DECL_SOURCE_LOCATION of DECL_IGNORED_P gnu_subprog_decl to
> 	UNKNOWN_LOCATION.
>
> That's indeed quite irregular but we have to live with it now.

Eric, can you push the approved fix for this issue (look for a message 
from Richard P day or two back, approved by Richi)?  I'm dealing with a 
medical issue and heading offline again momentarily.


jeff

>
Eric Botcazou Nov. 7, 2022, 4:36 p.m. UTC | #6 
> Eric, can you push the approved fix for this issue (look for a message
> from Richard P day or two back, approved by Richi)?  I'm dealing with a
> medical issue and heading offline again momentarily.

Sure, done.
Jakub Jelinek Jan. 19, 2023, 1:06 p.m. UTC | #7 
On Tue, Nov 01, 2022 at 01:46:20PM -0600, Jeff Law via Gcc-patches wrote:
> 
> On 8/17/22 06:15, Richard Purdie via Gcc-patches wrote:
> > Relative paths currently aren't remapped by -ffile-prefix-map and friends.
> > When cross compiling with separate 'source' and 'build' directories, the same
> > relative paths between directories may not be available on target as compared
> > to build time.
> > 
> > In order to be able to remap these relative build paths to paths that would
> > work on target, resolve paths within the file-prefix-map function using
> > realpath().
> 
> Understood.
> 
> 
> > 
> > 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.

This breaks ccache testsuite and -fdebug-prefix-map behavior in directories
which are symlinks, see PR108464/  I can't see how the new behavior would be
correct in that case, user is asking to remap say /home/jakub/foobar2 to
some other path, but exactly /home/jakub/foobar2 appears in the debug info,
rather than the other path.

	Jakub
diff mbox series

Patch

diff --git a/gcc/file-prefix-map.cc b/gcc/file-prefix-map.cc
index 24733f831d6..50d5d724a8f 100644
--- a/gcc/file-prefix-map.cc
+++ b/gcc/file-prefix-map.cc
@@ -70,19 +70,28 @@  remap_filename (file_prefix_map *maps, const char *filename)
   file_prefix_map *map;
   char *s;
   const char *name;
+  char *realname;
   size_t name_len;
 
+  if (lbasename (filename) == filename)
+    return filename;
+
+  realname = lrealpath (filename);
+
   for (map = maps; map; map = map->next)
-    if (filename_ncmp (filename, map->old_prefix, map->old_len) == 0)
+    if (filename_ncmp (realname, map->old_prefix, map->old_len) == 0)
       break;
-  if (!map)
+  if (!map) {
+    free (realname);
     return filename;
-  name = filename + map->old_len;
+  }
+  name = realname + map->old_len;
   name_len = strlen (name) + 1;
 
   s = (char *) ggc_alloc_atomic (name_len + map->new_len);
   memcpy (s, map->new_prefix, map->new_len);
   memcpy (s + map->new_len, name, name_len);
+  free (realname);
   return s;
 }