[RFC] Only compute realpath when basenames_may_differ is set

[RFC] Only compute realpath when basenames_may_differ is set

[Tom Tromey]

A user noted that, when sources are symlinked, gdb annotations will
print the real path, rather than the name of the symlink.

It seems to me that it is better to print the name of the file that
was actually used in the build, unless there is some reason not to.

This patch implements this, with the caveat that it will not work when
basenames-may-differ is enabled.  The way this mode is currently
implemented, returning the symbolic (not real) path is not possible.

While I think it would be good to redo the source file name cache and
perhaps integrate it with class source_cache, I haven't done so here.

Regression tested on x86-64 Fedora 29.

gdb/ChangeLog
2019-05-30  Tom Tromey  <tromey@adacore.com>

	* source.c (find_and_open_source): Respect basenames_may_differ.

gdb/testsuite/ChangeLog
2019-05-30  Tom Tromey  <tromey@adacore.com>

	* gdb.base/annotate-symlink.exp: New file.
---
 gdb/ChangeLog                               |  4 ++
 gdb/source.c                                | 14 ++++--
 gdb/testsuite/ChangeLog                     |  4 ++
 gdb/testsuite/gdb.base/annotate-symlink.exp | 51 +++++++++++++++++++++
 4 files changed, 68 insertions(+), 5 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/annotate-symlink.exp

diff --git a/gdb/source.c b/gdb/source.c
index 9a30209880b..9e4b8e4fafb 100644
--- a/gdb/source.c
+++ b/gdb/source.c
@@ -987,7 +987,10 @@ find_and_open_source (const char *filename,
       result = gdb_open_cloexec (fullname->get (), OPEN_MODE, 0);
       if (result >= 0)
 	{
-	  *fullname = gdb_realpath (fullname->get ());
+	  if (basenames_may_differ)
+	    *fullname = gdb_realpath (fullname->get ());
+	  else
+	    *fullname = gdb_abspath (fullname->get ());
 	  return scoped_fd (result);
 	}
 
@@ -1036,15 +1039,16 @@ find_and_open_source (const char *filename,
 	filename = rewritten_filename.get ();
     }
 
-  result = openp (path, OPF_SEARCH_IN_PATH | OPF_RETURN_REALPATH, filename,
-		  OPEN_MODE, fullname);
+  openp_flags flags = OPF_SEARCH_IN_PATH;
+  if (basenames_may_differ)
+    flags |= OPF_RETURN_REALPATH;
+  result = openp (path, flags, filename, OPEN_MODE, fullname);
   if (result < 0)
     {
       /* Didn't work.  Try using just the basename.  */
       p = lbasename (filename);
       if (p != filename)
-	result = openp (path, OPF_SEARCH_IN_PATH | OPF_RETURN_REALPATH, p,
-			OPEN_MODE, fullname);
+	result = openp (path, flags, p, OPEN_MODE, fullname);
     }
 
   return scoped_fd (result);
diff --git a/gdb/testsuite/gdb.base/annotate-symlink.exp b/gdb/testsuite/gdb.base/annotate-symlink.exp
new file mode 100644
index 00000000000..6dba2fe823c
--- /dev/null
+++ b/gdb/testsuite/gdb.base/annotate-symlink.exp
@@ -0,0 +1,51 @@
+# Copyright (C) 2019 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+standard_testfile realname-expand.c realname-expand-real.c
+
+if [is_remote host] {
+    unsupported "compiling on a remote host does not support a filename with directory."
+    return 0
+}
+
+set srcdirabs [file join [pwd] $srcdir]
+set srcfilelink [standard_output_file realname-expand-link.c]
+
+remote_exec build "ln -sf ${srcdirabs}/${subdir}/${srcfile2} $srcfilelink"
+
+if { [file type $srcfilelink] != "link" } {
+    unsupported "target directory cannot have symbolic links"
+    return -1
+}
+
+if { [gdb_compile "${srcdir}/${subdir}/${srcfile} ${srcfilelink}" "${binfile}" \
+		  executable {debug}] != "" } {
+    untested "failed to compile"
+    return -1
+}
+
+clean_restart ${testfile}
+
+if {![runto_main]} {
+    unsupported "failed to run to main"
+    return -1
+}
+
+gdb_breakpoint func message
+
+gdb_test_no_output "set annotate 1"
+
+gdb_test "continue" \
+    "Breakpoint .* func .*realname-expand-link.c:$decimal\r\n\032\032.*realname-expand-link.c:.*"
-- 
2.20.1

Re: [RFC] Only compute realpath when basenames_may_differ is set

[Tom Tromey]

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

Tom> A user noted that, when sources are symlinked, gdb annotations will
Tom> print the real path, rather than the name of the symlink.

Tom> It seems to me that it is better to print the name of the file that
Tom> was actually used in the build, unless there is some reason not to.

Tom> This patch implements this, with the caveat that it will not work when
Tom> basenames-may-differ is enabled.  The way this mode is currently
Tom> implemented, returning the symbolic (not real) path is not possible.

Tom> While I think it would be good to redo the source file name cache and
Tom> perhaps integrate it with class source_cache, I haven't done so here.

Tom> Regression tested on x86-64 Fedora 29.

Tom> gdb/ChangeLog
Tom> 2019-05-30  Tom Tromey  <tromey@adacore.com>

Tom> 	* source.c (find_and_open_source): Respect basenames_may_differ.

I'm checking this in now.

Tom

Re: [RFC] Only compute realpath when basenames_may_differ is set

[Tom de Vries]

On 14-06-19 16:30, Tom Tromey wrote:
>>>>>> "Tom" == Tom Tromey <tromey@adacore.com> writes:
> 
> Tom> A user noted that, when sources are symlinked, gdb annotations will
> Tom> print the real path, rather than the name of the symlink.
> 
> Tom> It seems to me that it is better to print the name of the file that
> Tom> was actually used in the build, unless there is some reason not to.
> 
> Tom> This patch implements this, with the caveat that it will not work when
> Tom> basenames-may-differ is enabled.  The way this mode is currently
> Tom> implemented, returning the symbolic (not real) path is not possible.
> 
> Tom> While I think it would be good to redo the source file name cache and
> Tom> perhaps integrate it with class source_cache, I haven't done so here.
> 
> Tom> Regression tested on x86-64 Fedora 29.
> 
> Tom> gdb/ChangeLog
> Tom> 2019-05-30  Tom Tromey  <tromey@adacore.com>
> 
> Tom> 	* source.c (find_and_open_source): Respect basenames_may_differ.
> 
> I'm checking this in now.
> 

This caused PR24687 - "FAIL: gdb.base/fullname.exp: set breakpoint by
full path before/after loading symbols - built relative".

Thanks,
- Tom

Re: [RFC] Only compute realpath when basenames_may_differ is set

[Tom Tromey]

Tom> This caused PR24687 - "FAIL: gdb.base/fullname.exp: set breakpoint by
Tom> full path before/after loading symbols - built relative".

Thanks for the report.  I replied in the bug -- I couldn't reproduce
this.

Tom

Re: [RFC] Only compute realpath when basenames_may_differ is set

[Jonah Graham]

On Fri, 14 Jun 2019 at 10:31, Tom Tromey <tromey@adacore.com> wrote:

> I'm checking this in now.
>
> Tom
>

Hi Tom,

This causes an error with Eclipse CDT's default use case. In CDT the
compile lines are basically "gcc -g -c -o file.o ../file.c" with the pwd
being a directory under the source:

root/
  file.c
  Debug/
    Makefile
    file.o
    program.elf

I have modified the fullname.exp test with a testcase that shows the
failure, see the patch below. Here is also a short trace of a session
showing the problem:

/tmp $ mkdir test && cd test
/tmp/test $ echo 'int main() {}' > testcase.c
/tmp/test $ mkdir build && cd build
/tmp/test/build $ gcc -g ../testcase.c -o testcase
/tmp/test/build $ gdb -q testcase
Reading symbols from testcase...done.
(gdb) b /tmp/test/testcase.c:1
Breakpoint 1 at 0x603: file ../testcase.c, line 1.
(gdb) quit
/tmp/test/build $ /scratch/gdb/binutils-gdb/gdb/gdb -q testcase
Reading symbols from testcase...
(gdb) b /tmp/test/testcase.c:1
No source file named /tmp/test/testcase.c.
Make breakpoint pending on future shared library load? (y or [n]) n
(gdb) quit
/tmp/test/build $ gdb --version | head -1
GNU gdb (Ubuntu 8.1-0ubuntu3) 8.1.0.20180409-git
/tmp/test/build $ /scratch/gdb/binutils-gdb/gdb/gdb --version | head -1
GNU gdb (GDB) 8.3.50.20190819-git

Thanks,
Jonah

---
 gdb/testsuite/gdb.base/fullname.exp | 30 +++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/gdb/testsuite/gdb.base/fullname.exp
b/gdb/testsuite/gdb.base/fullname.exp
index 07d6245d15..e306400b2b 100644
--- a/gdb/testsuite/gdb.base/fullname.exp
+++ b/gdb/testsuite/gdb.base/fullname.exp
@@ -96,6 +96,36 @@ if { [gdb_breakpoint [standard_output_file
tmp-${srcfile}]:${line} {no-message}]
     fail $msg
 }

+# Build the test executable using a relative path with /../ .
+file mkdir [standard_output_file ""]/build
+with_cwd [standard_output_file ""]/build {
+    if  { [gdb_compile ../tmp-${srcfile} \
+        "${binfile}" executable {debug}] != "" } {
+        return -1
+    }
+
+    gdb_exit
+    gdb_start
+    gdb_load ${binfile}
+
+    set msg "set breakpoint by full path before loading symbols - built
relative with .."
+    if { [gdb_breakpoint [standard_output_file tmp-${srcfile}]:${line}
{no-message}] != 0 } {
+        pass $msg
+    } else {
+        fail $msg
+    }
+
+    gdb_test "break main" \
+        "Breakpoint.*at.*line.*" "set breakpoint at main - built relative
with .."
+
+    set msg "set breakpoint by full path after loading symbols - built
relative with .."
+    if { [gdb_breakpoint [standard_output_file tmp-${srcfile}]:${line}
{no-message}] != 0 } {
+        pass $msg
+    } else {
+        fail $msg
+    }
+}
+
 # Build the test executable using relative paths not relative to the
directory
 # we'll run GDB from.

-- 
2.22.0

Re: [RFC] Only compute realpath when basenames_may_differ is set

[Jonah Graham]

bump.


On Mon, 19 Aug 2019 at 15:25, Jonah Graham <jonah@kichwacoders.com> wrote:

>
> On Fri, 14 Jun 2019 at 10:31, Tom Tromey <tromey@adacore.com> wrote:
>
>> I'm checking this in now.
>>
>> Tom
>>
>
> Hi Tom,
>
> This causes an error with Eclipse CDT's default use case. In CDT the
> compile lines are basically "gcc -g -c -o file.o ../file.c" with the pwd
> being a directory under the source:
>
> root/
>   file.c
>   Debug/
>     Makefile
>     file.o
>     program.elf
>
> I have modified the fullname.exp test with a testcase that shows the
> failure, see the patch below. Here is also a short trace of a session
> showing the problem:
>
> /tmp $ mkdir test && cd test
> /tmp/test $ echo 'int main() {}' > testcase.c
> /tmp/test $ mkdir build && cd build
> /tmp/test/build $ gcc -g ../testcase.c -o testcase
> /tmp/test/build $ gdb -q testcase
> Reading symbols from testcase...done.
> (gdb) b /tmp/test/testcase.c:1
> Breakpoint 1 at 0x603: file ../testcase.c, line 1.
> (gdb) quit
> /tmp/test/build $ /scratch/gdb/binutils-gdb/gdb/gdb -q testcase
> Reading symbols from testcase...
> (gdb) b /tmp/test/testcase.c:1
> No source file named /tmp/test/testcase.c.
> Make breakpoint pending on future shared library load? (y or [n]) n
> (gdb) quit
> /tmp/test/build $ gdb --version | head -1
> GNU gdb (Ubuntu 8.1-0ubuntu3) 8.1.0.20180409-git
> /tmp/test/build $ /scratch/gdb/binutils-gdb/gdb/gdb --version | head -1
> GNU gdb (GDB) 8.3.50.20190819-git
>
> Thanks,
> Jonah
>
> ---
>  gdb/testsuite/gdb.base/fullname.exp | 30 +++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
>
> diff --git a/gdb/testsuite/gdb.base/fullname.exp
> b/gdb/testsuite/gdb.base/fullname.exp
> index 07d6245d15..e306400b2b 100644
> --- a/gdb/testsuite/gdb.base/fullname.exp
> +++ b/gdb/testsuite/gdb.base/fullname.exp
> @@ -96,6 +96,36 @@ if { [gdb_breakpoint [standard_output_file
> tmp-${srcfile}]:${line} {no-message}]
>      fail $msg
>  }
>
> +# Build the test executable using a relative path with /../ .
> +file mkdir [standard_output_file ""]/build
> +with_cwd [standard_output_file ""]/build {
> +    if  { [gdb_compile ../tmp-${srcfile} \
> +        "${binfile}" executable {debug}] != "" } {
> +        return -1
> +    }
> +
> +    gdb_exit
> +    gdb_start
> +    gdb_load ${binfile}
> +
> +    set msg "set breakpoint by full path before loading symbols - built
> relative with .."
> +    if { [gdb_breakpoint [standard_output_file tmp-${srcfile}]:${line}
> {no-message}] != 0 } {
> +        pass $msg
> +    } else {
> +        fail $msg
> +    }
> +
> +    gdb_test "break main" \
> +        "Breakpoint.*at.*line.*" "set breakpoint at main - built relative
> with .."
> +
> +    set msg "set breakpoint by full path after loading symbols - built
> relative with .."
> +    if { [gdb_breakpoint [standard_output_file tmp-${srcfile}]:${line}
> {no-message}] != 0 } {
> +        pass $msg
> +    } else {
> +        fail $msg
> +    }
> +}
> +
>  # Build the test executable using relative paths not relative to the
> directory
>  # we'll run GDB from.
>
> --
> 2.22.0
>

Re: [RFC] Only compute realpath when basenames_may_differ is set

[Jonah Graham]

On Thu, 12 Sep 2019 at 13:47, Jonah Graham <jonah@kichwacoders.com> wrote:

> bump.
>
>
> On Mon, 19 Aug 2019 at 15:25, Jonah Graham <jonah@kichwacoders.com> wrote:
>
>>
>> On Fri, 14 Jun 2019 at 10:31, Tom Tromey <tromey@adacore.com> wrote:
>>
>>> I'm checking this in now.
>>>
>>> Tom
>>>
>>
>> Hi Tom,
>>
>> This causes an error with Eclipse CDT's default use case. In CDT the
>> compile lines are basically "gcc -g -c -o file.o ../file.c" with the pwd
>> being a directory under the source:
>>
>> root/
>>   file.c
>>   Debug/
>>     Makefile
>>     file.o
>>     program.elf
>>
>> I have modified the fullname.exp test with a testcase that shows the
>> failure, see the patch below. Here is also a short trace of a session
>> showing the problem:
>>
>> /tmp $ mkdir test && cd test
>> /tmp/test $ echo 'int main() {}' > testcase.c
>> /tmp/test $ mkdir build && cd build
>> /tmp/test/build $ gcc -g ../testcase.c -o testcase
>> /tmp/test/build $ gdb -q testcase
>> Reading symbols from testcase...done.
>> (gdb) b /tmp/test/testcase.c:1
>> Breakpoint 1 at 0x603: file ../testcase.c, line 1.
>> (gdb) quit
>> /tmp/test/build $ /scratch/gdb/binutils-gdb/gdb/gdb -q testcase
>> Reading symbols from testcase...
>> (gdb) b /tmp/test/testcase.c:1
>> No source file named /tmp/test/testcase.c.
>> Make breakpoint pending on future shared library load? (y or [n]) n
>> (gdb) quit
>> /tmp/test/build $ gdb --version | head -1
>> GNU gdb (Ubuntu 8.1-0ubuntu3) 8.1.0.20180409-git
>> /tmp/test/build $ /scratch/gdb/binutils-gdb/gdb/gdb --version | head -1
>> GNU gdb (GDB) 8.3.50.20190819-git
>>
>> Thanks,
>> Jonah
>>
>> ---
>>  gdb/testsuite/gdb.base/fullname.exp | 30 +++++++++++++++++++++++++++++
>>  1 file changed, 30 insertions(+)
>>
>> diff --git a/gdb/testsuite/gdb.base/fullname.exp
>> b/gdb/testsuite/gdb.base/fullname.exp
>> index 07d6245d15..e306400b2b 100644
>> --- a/gdb/testsuite/gdb.base/fullname.exp
>> +++ b/gdb/testsuite/gdb.base/fullname.exp
>> @@ -96,6 +96,36 @@ if { [gdb_breakpoint [standard_output_file
>> tmp-${srcfile}]:${line} {no-message}]
>>      fail $msg
>>  }
>>
>> +# Build the test executable using a relative path with /../ .
>> +file mkdir [standard_output_file ""]/build
>> +with_cwd [standard_output_file ""]/build {
>> +    if  { [gdb_compile ../tmp-${srcfile} \
>> +        "${binfile}" executable {debug}] != "" } {
>> +        return -1
>> +    }
>> +
>> +    gdb_exit
>> +    gdb_start
>> +    gdb_load ${binfile}
>> +
>> +    set msg "set breakpoint by full path before loading symbols - built
>> relative with .."
>> +    if { [gdb_breakpoint [standard_output_file tmp-${srcfile}]:${line}
>> {no-message}] != 0 } {
>> +        pass $msg
>> +    } else {
>> +        fail $msg
>> +    }
>> +
>> +    gdb_test "break main" \
>> +        "Breakpoint.*at.*line.*" "set breakpoint at main - built
>> relative with .."
>> +
>> +    set msg "set breakpoint by full path after loading symbols - built
>> relative with .."
>> +    if { [gdb_breakpoint [standard_output_file tmp-${srcfile}]:${line}
>> {no-message}] != 0 } {
>> +        pass $msg
>> +    } else {
>> +        fail $msg
>> +    }
>> +}
>> +
>>  # Build the test executable using relative paths not relative to the
>> directory
>>  # we'll run GDB from.
>>
>> --
>> 2.22.0
>>
>
Tom de Vries has fixed the behaviour as part of PR24687. Can the testcase I
wrote for this case (that best represents CDT's use case) be added to GDB
testsuite. The original email with the testcase that can be added to
gdb.base/fullname.exp is
https://sourceware.org/ml/gdb-patches/2019-08/msg00421.html

Thank you,
Jonah