[4/4] gdb/dap: only include sourceReference if file path does not exist

  According to the DAP specification if the "sourceReference" field is
included in a Source object and is non-zero, then the DAP client _must_
make a "source" request to the debugger to retrieve file contents, even
if the Source object also includes path information.

If the Source's path field is a valid path that the DAP client is able
to read from the filesystem, having to make another request to the
debugger to get the file contents is wasteful and leads to incorrect
results (DAP clients will try to get the contents from the server and
display those contents as a file with the name in "source.path", but
this could conflict with the _acutal_ existing file at "source.path").

Instead, only set "sourceReference" if the source file path does not
exist.
---

It is unclear exactly what the meaning of "sourceReference" is from the
specification, but at least two items support my interpretation here:

1. The wording in the specification for sourceReference mentions that
   the contents of the source must be retrieved through a 'source'
   request, _even if_ a path is specified. This suggests to me that
   sourceReference is meant to be used in cases where the debugger has
   access to source contents that the client does not (e.g. I could see
   something like this being used for "constructed" or "generated"
   sources, like disassembly).

2. Other debuggers (such as lldb-vscode and codelldb) do not set the
   sourceReference field at all for files which exist on disk. If this
   patch is accepted then GDB will match the behavior of those two.

And at least anecdotally, my testing with nvim-dap (my DAP client of
choice) shows that this behavior better matches the expectation of that
DAP client in particular.

 gdb/python/lib/gdb/dap/sources.py | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)
  

>>>>> "Gregory" == Gregory Anders via Gdb-patches <gdb-patches@sourceware.org> writes:

Thank you for the patch.

Gregory> According to the DAP specification if the "sourceReference" field is
Gregory> included in a Source object and is non-zero, then the DAP client _must_
Gregory> make a "source" request to the debugger to retrieve file contents, even
Gregory> if the Source object also includes path information.

On closer reading (this isn't the first time I've gotten the DAP spec
wrong) I think you are right.

The docs for the Source object say:

   * If the value > 0 the contents of the source must be retrieved through the
   * `source` request (even if a path is specified).

... which I think supports your view.

Gregory> +        if not os.path.exists(fullname):
Gregory> +            global _next_source

I suspect we should just remove all sourceReference handling from
sources.py, because if we can't find a full path, gdb isn't going to be
able to come up with the source anyway.

What do you think?

I thought the test suite might need some updates but I don't see any
references to sourceReference there.

Tom
  

On Fri, 01 Sep 2023 13:42 -0600, Tom Tromey wrote:
>I suspect we should just remove all sourceReference handling from
>sources.py, because if we can't find a full path, gdb isn't going to be
>able to come up with the source anyway.
>
>What do you think?

I agree. I avoided doing this originally to keep the change small, but I
think eliminating sourceReference completely (for the reason you
mentioned) is the better approach. It will keep the code simpler.

>I thought the test suite might need some updates but I don't see any
>references to sourceReference there.

There was one test case that was failing with the v1 patch
(sources.exp). I updated it in v2 (sent a few minutes ago).

Greg
  

On Fri, 01 Sep 2023 21:43 +0000, Gregory Anders wrote:
>On Fri, 01 Sep 2023 13:42 -0600, Tom Tromey wrote:
>>I suspect we should just remove all sourceReference handling from
>>sources.py, because if we can't find a full path, gdb isn't going to be
>>able to come up with the source anyway.
>>
>>What do you think?
>
>I agree. I avoided doing this originally to keep the change small, but I
>think eliminating sourceReference completely (for the reason you
>mentioned) is the better approach. It will keep the code simpler.
>
>>I thought the test suite might need some updates but I don't see any
>>references to sourceReference there.
>
>There was one test case that was failing with the v1 patch
>(sources.exp). I updated it in v2 (sent a few minutes ago).
>
>Greg

I checked what lldb-vscode does, and AFAICT they are doing the same
thing proposed in this patch: if the source file exists/is valid, they
do not populate sourceReference at all and do not maintain that entry
in any kind of source map.

They do, however, maintain a source map (with sourceReferences) for
things like function disassembly. So it might be worth keeping that
stuff around in GDB in case that feature is ever implemented in the
future.

Your call.

Greg