proc,service: pretty print time.Time variables

[aarzilli]

Fixes #999

[polinasok]

Meta question: this PR manually reconstructs things from the low-level fields. I am assuming somewhere in the standard library this is done as well when time is converted to string. That logic is probably hidden somewhere inside of unexported methods and can't be reused, but but can they at least be referenced beyond only a pointer to time.go? That could be of help in sanity checking the logic.

[polinasok]

Why do you need this?

[aarzilli]

Leftover, removed.

[polinasok]

All structs?

[aarzilli]

I don't think there's a reason to be more specific.

[aarzilli]

Meta question: this PR manually reconstructs things from the low-level fields. I am assuming somewhere in the standard library this is done as well when time is converted to string. That logic is probably hidden somewhere inside of unexported methods and can't be reused, but but can they at least be referenced beyond only a pointer to time.go? That could be of help in sanity checking the logic.

The work here is done by copying a time.Time object read from the target into a time.Time object allocated by delve, time.go doesn't do this anywhere. The relevant comment is the one inside type Time struct.
We could format directly but we'd have to copy considerable more code.

[mark-pictor-csec]

@polinasok wrote

Meta question: this PR manually reconstructs things from the low-level fields. I am assuming somewhere in the standard library this is done as well when time is converted to string. That logic is probably hidden somewhere inside of unexported methods and can't be reused, but but can they at least be referenced beyond only a pointer to time.go? That could be of help in sanity checking the logic.

Kinda tangential: I'd like to see pretty printers as starlark scripts, contributed to (and ultimately maintained in parallel with) the packages they serve. Ideally with tests to ensure they stay in sync with the go code.

This thinking insipired by gdb pretty-printer scripts being embedded in the distributed library, for example libc. I don't know where those scripts live within the libc source, but for go it'd certainly make sense to bundle them inside the package - perhaps under time/_dlv/ or time/_prettyprint/ or so. The underscore is just a hint for humans: no go code under here - though if the standard library already has another mechanism to indicate that, we should use it instead. As for actually embedding the script within binaries, that seems less necessary for go than it is for languages like C.

[aarzilli]

I'd like to see pretty printers as starlark scripts, contributed to (and ultimately maintained in parallel with) the packages they serve

At the moment all starlark scripts are executed client side, meaning that if we did this all the pretty printing would only work on the command line client, which isn't that different from defining a print-time command and using it.

I don't know where those scripts live within the libc source, but for go it'd certainly make sense to bundle them inside the package - perhaps under time/_dlv/ or time/_prettyprint/ or so

If we moved starlark evaluation to the backend we wouldn't necessarily be able to access those directories as delve's backend only has access to the executable it's debugging. Also directories that only contain non-go files are stripped from module archives, which means that you wouldn't have access to those files for any package except the packages of the main module and the standard library.

[mark-pictor-csec]

I'd like to see pretty printers as starlark scripts, contributed to (and ultimately maintained in parallel with) the packages they serve

At the moment all starlark scripts are executed client side, meaning that if we did this all the pretty printing would only work on the command line client, which isn't that different from defining a print-time command and using it.

I don't know where those scripts live within the libc source, but for go it'd certainly make sense to bundle them inside the package - perhaps under time/_dlv/ or time/_prettyprint/ or so

If we moved starlark evaluation to the backend we wouldn't necessarily be able to access those directories as delve's backend only has access to the executable it's debugging. Also directories that only contain non-go files are stripped from module archives, which means that you wouldn't have access to those files for any package except the packages of the main module and the standard library.

oof. This sounds like far more work than I'd hoped for.

• moving starlark to the backend sounds like a lot of work
  • we'd still have the problem of feeding scripts to it
    • allow backend to read starlark files (security implications??)
    • send script from frontend? requires additional message type(s)
• making it work in the frontend means adding scripting support to additional frontends such as dlv-dap
  • probable duplication of code between frontends
• the problem of where to store/find the scripts remains
  • maybe path/to/pkg/_dlv.star
• regardless of where starlark is interpreted, additional message types may be needed in dap

Any alternatives I'm missing?

The reason I prefer scripts over hard-coded functions is that I expect it'd be easier to add additional pretty-printers, and wouldn't require re-building delve. The most immediate use for me would be with ast.Node and related structures, and running the debugger through vscode. Iterating on a script to print those seems far easier/faster than iterating on hard-coded functionality.

[derekparker]

I'm catching up on the discussion still a bit here however I do like the approach of using the scripting interface for the pretty printing... I know it's a lot of work and I'm not suggesting it right now but I really think it's worth a conversation and consideration to take more advantage of the scripting support.

[aarzilli]

Moving starlark evaluation to the backend is possible, there's going to be some pain points with making dlv_command, read_file/write_file and command registration work. Having starlark running in the server could also be used to create more complex breakpoint conditions.
Loading prettyprinters automatically from modules is something I'm not convinced is useful enough given the complications (also we could think about this later).
Before I port starlark to run server-side however I'd like to know three things:

1. what happens to the time.Time pretty printer? Does it become a hardcoded script?
2. are we going to add some starlark init file that gets loaded at startup by headless servers (at the moment the init file is not starlark and is only loaded by the frontend)
3. how does this get integrated with DAP?

[hyangah]

Let's discuss the general scripting interface in a separate issue.

As @aarzilli pointed out, the implication with DAP is unclear. Starlark scripting is too powerful (It's delve RPC-based so you can do more than just pretty printing) for DAP to embrace it without careful design IMO. It's unclear to me, who's responsible for time.Time pretty printing support, and how trivial it is to write in a script (unexperienced users would attempt to utilize dlv call command-like features which may not be desirable at this moment).

For gdb support, go embeds the location of a very very basic script (from GOROOT) to the .debug_gdb_scripts section in the binary, but I am not sure if that approach can generally work for other packages without Go tool's support.

[aarzilli]

unexperienced users would attempt to utilize dlv call command-like features which may not be desirable at this moment

I wouldn't allow call and other commands that resume execution in this context.

[derekparker]

Let's discuss the general scripting interface in a separate issue.

Agreed, this will be a large undertaking if we happen to go for it and would need upfront design, discussion and consideration.

[derekparker]

There's still a few outstanding questions from @polinasok, let's move the discussion of using the scripting interface to it's own issue and review this PR as is.

[derekparker]

@aarzilli would you mind creating the issue for the scripting changes?

[aarzilli]

There's still a few outstanding questions from @polinasok

Oops I didn't see them.

[aarzilli]

@aarzilli would you mind creating the issue for the scripting changes?

Done: #2887

[derekparker]

Needs rebase. Also @polinasok can you do another review pass?

[aarzilli]

Rebased.

[polinasok]

Thank you for adding the location logic.

[derekparker]

LGTM