[PATCH] Apply substitute-path to relative filenames as well

[PATCH] Apply substitute-path to relative filenames as well

[LRN]

[-- Attachment #1.1.1: Type: text/plain, Size: 2417 bytes --]

The patch is attached.

The change itself is similar to
https://www.sourceware.org/ml/gdb-patches/2017-02/msg00693.html , but the code
is slightly cleaner (at the cost of making the patch larger).

Also, as the patch is trivial, so i would expect it to not to require copyright
assignment (i do have one, but David Grayson might not have).

Here's the copy of the commit message:

When source file path is relative to the build directory (which
is considered a good practice and is enforced in certain buildsystems,
such as meson), gdb only applies substitute-path to the build directory
path. Then gdb appends the source file path to the rewritten build
directory path, and tries to access that.

This fails if either two of the following conditions are true:
a) The user didn't specify substitute-path for the build directory.
   This is highly likely, since path substitution for build directories
   is not documented anywhere, and since gdb does not tell[0] the user
   the path to the build directory, just the source file path.
b) The source file path changed.
   This can also easily happen, since a source path that is relative
   to the build directory can include any number of directory names
   that are not part of the program source tree (starting with the
   name of the root directory of the source tree). Gdb will not apply
   substitute-path to that relative path, thus there is no way for
   the user to tell gdb about these changes.

This commit changes the code to apply substitute-path to all filenames,
both relative and absolute. This way it is possible to do things like:

set substitute-path ../foobar-1.0 /src/my/foobar-1.0

which is completely in line with the user expectations.

This might break unusual cases where build directory path is also
relative (is that even possible?) and happens to match the path
to the source directory (i.e. happens to match a substitution rule).

[0]: There's a "maintenance info symtabs" command that does show the names
     of the build directories, but normal users are not required to
     know or use that.
---
 gdb/source.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

gdb/ChangeLog:

2019-02-28 Руслан Ижбулатов  <lrn1986@gmail.com>

       * source.c (find_and_open_source): Apply substitute-path to all
       filenames, both absolute and relative



[-- Attachment #1.1.2: 0001-Apply-substitute-path-to-relative-filenames-as-well.patch --]
[-- Type: text/plain, Size: 2876 bytes --]

From 28431cdf98ec6c606373fd02c36c5642c4f196df Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=D0=A0=D1=83=D1=81=D0=BB=D0=B0=D0=BD=20=D0=98=D0=B6=D0=B1?=
 =?UTF-8?q?=D1=83=D0=BB=D0=B0=D1=82=D0=BE=D0=B2?= <lrn1986@gmail.com>
Date: Thu, 28 Feb 2019 10:25:41 +0000
Subject: [PATCH] Apply substitute-path to relative filenames as well

When source file path is relative to the build directory (which
is considered a good practice and is enforced in certain buildsystems,
such as meson), gdb only applies substitute-path to the build directory
path. Then gdb appends the source file path to the rewritten build
directory path, and tries to access that.

This fails if either two of the following conditions are true:
a) The user didn't specify substitute-path for the build directory.
   This is highly likely, since path substitution for build directories
   is not documented anywhere, and since gdb does not tell[0] the user
   the path to the build directory, just the source file path.
b) The source file path changed.
   This can also easily happen, since a source path that is relative
   to the build directory can include any number of directory names
   that are not part of the program source tree (starting with the
   name of the root directory of the source tree). Gdb will not apply
   substitute-path to that relative path, thus there is no way for
   the user to tell gdb about these changes.

This commit changes the code to apply substitute-path to all filenames,
both relative and absolute. This way it is possible to do things like:

set substitute-path ../foobar-1.0 /src/my/foobar-1.0

which is completely in line with the user expectations.

This might break unusual cases where build directory path is also
relative (is that even possible?) and happens to match the path
to the source directory (i.e. happens to match a substitution rule).

[0]: There's a "maintenance info symtabs" command that does show the names
     of the build directories, but normal users are not required to
     know or use that.
---
 gdb/source.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/gdb/source.c b/gdb/source.c
index f99215f..673ebc3 100644
--- a/gdb/source.c
+++ b/gdb/source.c
@@ -1028,15 +1028,11 @@ find_and_open_source (const char *filename,
     }
 
   gdb::unique_xmalloc_ptr<char> rewritten_filename;
-  if (IS_ABSOLUTE_PATH (filename))
-    {
-      /* If filename is absolute path, try the source path
-	 substitution on it.  */
-      rewritten_filename = rewrite_source_path (filename);
 
-      if (rewritten_filename != NULL)
-	filename = rewritten_filename.get ();
-    }
+  rewritten_filename = rewrite_source_path (filename);
+
+  if (rewritten_filename != NULL)
+    filename = rewritten_filename.get ();
 
   result = openp (path, OPF_SEARCH_IN_PATH | OPF_RETURN_REALPATH, filename,
 		  OPEN_MODE, fullname);
-- 
2.4.0


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] Apply substitute-path to relative filenames as well

[Tom Tromey]

>>>>> "LRN" == LRN  <lrn1986@gmail.com> writes:

LRN> The patch is attached.
LRN> The change itself is similar to
LRN> https://www.sourceware.org/ml/gdb-patches/2017-02/msg00693.html , but the code
LRN> is slightly cleaner (at the cost of making the patch larger).

Thank you for the patch.

LRN> Also, as the patch is trivial, so i would expect it to not to require copyright
LRN> assignment (i do have one, but David Grayson might not have).

I agree.

LRN> 2019-02-28 Руслан Ижбулатов  <lrn1986@gmail.com>

LRN>        * source.c (find_and_open_source): Apply substitute-path to all
LRN>        filenames, both absolute and relative

Is it possible to get a test case for the patch?

LRN>    gdb::unique_xmalloc_ptr<char> rewritten_filename;
[...]
LRN> +  rewritten_filename = rewrite_source_path (filename);

I think these two lines can now be joined; no need to have a separate
assignment.

thanks,
Tom

Re: [PATCH] Apply substitute-path to relative filenames as well

[LRN]

[-- Attachment #1.1.1: Type: text/plain, Size: 1323 bytes --]

On 06.03.2019 1:10, Tom Tromey wrote:
> LRN writes:
>> 2019-02-28 Руслан Ижбулатов  <lrn1986@gmail.com>
>> 
>>        * source.c (find_and_open_source): Apply substitute-path to all
>>        filenames, both absolute and relative
> 
> Is it possible to get a test case for the patch?

No. I couldn't find a gdb testcase for the substitute-path feature (there's one
that checks issuing the `set substitute-path` command itself, but it doesn't
seem to be testing its *runtime effect*, i.e. how it is applied when gdb needs
to read a source file), so i can't provide a test case for this change by
modifying an existing one. That means i'd have to write one from scratch, and i
wouldn't know how to even begin to do that.

Also, running gdb testsuite requires dejagnu, which i don't have (and by "i
don't have" i mean "i've never built it from the source", as this is how i get
90% of all my dev software). It should technically be possible for me to build
it, but that'll take time that i would rather spend on other things.

>>    gdb::unique_xmalloc_ptr<char> rewritten_filename;
> [...]
>> +  rewritten_filename = rewrite_source_path (filename);
> 
> I think these two lines can now be joined; no need to have a separate
> assignment.
> 

New version of the patch is attached.

[-- Attachment #1.1.2: 0001-Apply-substitute-path-to-relative-filenames-as-well.patch --]
[-- Type: text/plain, Size: 2908 bytes --]

From 74ec81cb519c331d8b10d419eefa2a599fac2f4e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=D0=A0=D1=83=D1=81=D0=BB=D0=B0=D0=BD=20=D0=98=D0=B6=D0=B1?=
 =?UTF-8?q?=D1=83=D0=BB=D0=B0=D1=82=D0=BE=D0=B2?= <lrn1986@gmail.com>
Date: Thu, 28 Feb 2019 10:25:41 +0000
Subject: [PATCH] Apply substitute-path to relative filenames as well

When source file path is relative to the build directory (which
is considered a good practice and is enforced in certain buildsystems,
such as meson), gdb only applies substitute-path to the build directory
path. Then gdb appends the source file path to the rewritten build
directory path, and tries to access that.

This fails if either two of the following conditions are true:
a) The user didn't specify substitute-path for the build directory.
   This is highly likely, since path substitution for build directories
   is not documented anywhere, and since gdb does not tell[0] the user
   the path to the build directory, just the source file path.
b) The source file path changed.
   This can also easily happen, since a source path that is relative
   to the build directory can include any number of directory names
   that are not part of the program source tree (starting with the
   name of the root directory of the source tree). Gdb will not apply
   substitute-path to that relative path, thus there is no way for
   the user to tell gdb about these changes.

This commit changes the code to apply substitute-path to all filenames,
both relative and absolute. This way it is possible to do things like:

set substitute-path ../foobar-1.0 /src/my/foobar-1.0

which is completely in line with the user expectations.

This might break unusual cases where build directory path is also
relative (is that even possible?) and happens to match the path
to the source directory (i.e. happens to match a substitution rule).

[0]: There's a "maintenance info symtabs" command that does show the names
     of the build directories, but normal users are not required to
     know or use that.
---
 gdb/source.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/gdb/source.c b/gdb/source.c
index f99215f..3fc7717 100644
--- a/gdb/source.c
+++ b/gdb/source.c
@@ -1027,16 +1027,10 @@ find_and_open_source (const char *filename,
 	}
     }
 
-  gdb::unique_xmalloc_ptr<char> rewritten_filename;
-  if (IS_ABSOLUTE_PATH (filename))
-    {
-      /* If filename is absolute path, try the source path
-	 substitution on it.  */
-      rewritten_filename = rewrite_source_path (filename);
+  gdb::unique_xmalloc_ptr<char> rewritten_filename = rewrite_source_path (filename);
 
-      if (rewritten_filename != NULL)
-	filename = rewritten_filename.get ();
-    }
+  if (rewritten_filename != NULL)
+    filename = rewritten_filename.get ();
 
   result = openp (path, OPF_SEARCH_IN_PATH | OPF_RETURN_REALPATH, filename,
 		  OPEN_MODE, fullname);
-- 
2.4.0


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] Apply substitute-path to relative filenames as well

[LRN]

[-- Attachment #1.1: Type: text/plain, Size: 182 bytes --]

On 06.03.2019 1:10, Tom Tromey wrote:
> Thank you for the patch.
> 

Hey, it's been a week. Do i need to do something to move this along? (Please,
don't say "test case" :( )


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] Apply substitute-path to relative filenames as well

[LRN]

[-- Attachment #1.1: Type: text/plain, Size: 340 bytes --]

On 13.03.2019 15:47, LRN wrote:
> On 06.03.2019 1:10, Tom Tromey wrote:
>> Thank you for the patch.
>>
> 
> Hey, it's been a week. Do i need to do something to move this along? (Please,
> don't say "test case" :( )
> 
Hey, it's been two weeks. Do i need to do something to move this along?
(Please, don't say "test case" :( )



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] Apply substitute-path to relative filenames as well

[Tom Tromey]

>>>>> "LRN" == LRN  <lrn1986@gmail.com> writes:

>> Thank you for the patch.

LRN> Hey, it's been a week. Do i need to do something to move this along? (Please,
LRN> don't say "test case" :( )

Is it possible to write one?  I didn't think about it in detail.

The norm is that a patch should come with a test case.  Exceptions are
for refactoring patches, where the intent is to preserve semantics; and
for the relatively rare cases where a test is very difficult or
impossible to write.

I see there are already tests in gdb.base/subst.exp.  Perhaps it could
be extended?

Also the review had a little nit that could be addressed.

thanks,
Tom

Re: [PATCH] Apply substitute-path to relative filenames as well

[LRN]

[-- Attachment #1.1: Type: text/plain, Size: 1614 bytes --]

On 28.03.2019 0:36, Tom Tromey wrote:
> LRN writes:
> 
>> Hey, it's been a week. Do i need to do something to move this along? (Please,
>> don't say "test case" :( )
> 
> Is it possible to write one?  I didn't think about it in detail.
> 
> I see there are already tests in gdb.base/subst.exp.  Perhaps it could
> be extended?

These tests only test the interactive output of gdb with regards to subst
commands. I.e. it issues a command, such as "show substitute-path", and then
matches the output to the one it expects.

My patch influences the output only when gdb is actually debugging something -
which means that the test must compile some program (using relative path to the
source file!), run it under gdb, set up a breakpoint, verify that the source
path printed at the breakpoint hit is relative (and unresolved - the test would
probably need to move the source file before running?), then use subst, then
probably restart and verify that the path is non-relative and resolved when the
same breakpoint hits.

Okay, maybe it can be done without breakpoints - just run and list
../../path/to/sourcefile.c:0 or something.

Anyway, that still sounds an order of magnitude more complicated that
subst.exp, so i would rather not try to implement all that using completely
unfamiliar testing framework that i do not have.

> 
> Also the review had a little nit that could be addressed.
> 

I did address it. The first follow-up[0] message had the new version of the
patch attached, with the nit fixed.

[0]: https://www.sourceware.org/ml/gdb-patches/2019-03/msg00082.html


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] Apply substitute-path to relative filenames as well

[Tom Tromey]

>>>>> "LRN" == LRN  <lrn1986@gmail.com> writes:

LRN> Anyway, that still sounds an order of magnitude more complicated that
LRN> subst.exp, so i would rather not try to implement all that using completely
LRN> unfamiliar testing framework that i do not have.

Alright, I will see if I can write one.

LRN> I did address it. The first follow-up[0] message had the new version of the
LRN> patch attached, with the nit fixed.

Thanks, I must have missed that.

Tom

Re: [PATCH] Apply substitute-path to relative filenames as well

[Tom Tromey]

>>>>> "Tom" == Tom Tromey <tom@tromey.com> writes:

>>>>> "LRN" == LRN  <lrn1986@gmail.com> writes:
LRN> Anyway, that still sounds an order of magnitude more complicated that
LRN> subst.exp, so i would rather not try to implement all that using completely
LRN> unfamiliar testing framework that i do not have.

Tom> Alright, I will see if I can write one.

LRN> I did address it. The first follow-up[0] message had the new version of the
LRN> patch attached, with the nit fixed.

Tom> Thanks, I must have missed that.

I'm checking this in.  I finally looked, and writing a test did seem
like a pain.

Tom