Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Redirect if internal console #3108

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pkg/proc/native/proc.go
Original file line number Diff line number Diff line change
Expand Up @@ -326,7 +326,7 @@ func openRedirects(redirects [3]string, foreground bool) (stdin, stdout, stderr
return dflt
}
var f *os.File
f, err = os.Create(path)
f, err = os.OpenFile(path, os.O_WRONLY|os.O_CREATE, 0o666)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a backwards incompatible change because you are not passsing O_TRUNC.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will add it - I still need to check whether O_TRUNC has any bad effect when opening a fifo.

if f != nil {
toclose = append(toclose, f)
}
Expand Down
76 changes: 62 additions & 14 deletions service/dap/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -273,11 +273,9 @@ const (
maxStringLenInCallRetVars = 1 << 10 // 1024
)

var (
// Max number of goroutines that we will return.
// This is a var for testing
maxGoroutines = 1 << 10
)
// Max number of goroutines that we will return.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid unrelated reformats, they pollute blames and make merge conflicts unnecessarily harder.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My editor auto-formats using gofumpt, so it formatted some extra editted files. Will revert such changes.

// This is a var for testing
var maxGoroutines = 1 << 10

// NewServer creates a new DAP Server. It takes an opened Listener
// via config and assumes its ownership. config.DisconnectChan has to be set;
Expand Down Expand Up @@ -805,7 +803,24 @@ func (s *Session) logToConsole(msg string) {
Body: dap.OutputEventBody{
Output: msg + "\n",
Category: "console",
}})
},
})
}

type debugConsoleLogger struct {
session *Session
category string
}

func (l *debugConsoleLogger) Write(p []byte) (int, error) {
l.session.send(&dap.OutputEvent{
Event: *newEvent("output"),
Body: dap.OutputEventBody{
Output: string(p),
Category: l.category,
},
})
return len(p), nil
}

func (s *Session) onInitializeRequest(request *dap.InitializeRequest) {
Expand Down Expand Up @@ -881,7 +896,7 @@ func (s *Session) onLaunchRequest(request *dap.LaunchRequest) {
return
}

var args = defaultLaunchConfig // narrow copy for initializing non-zero default values
args := defaultLaunchConfig // narrow copy for initializing non-zero default values
if err := unmarshalLaunchAttachArgs(request.Arguments, &args); err != nil {
s.sendShowUserErrorResponse(request.Request,
FailedToLaunch, "Failed to launch", fmt.Sprintf("invalid debug configuration - %v", err))
Expand Down Expand Up @@ -989,7 +1004,8 @@ func (s *Session) onLaunchRequest(request *dap.LaunchRequest) {
Body: dap.OutputEventBody{
Output: fmt.Sprintf("Build Error: %s\n%s (%s)\n", cmd, strings.TrimSpace(string(out)), err.Error()),
Category: "stderr",
}})
},
})
// Users are used to checking the Debug Console for build errors.
// No need to bother them with a visible pop-up.
s.sendErrorResponse(request.Request, FailedToLaunch, "Failed to launch",
Expand Down Expand Up @@ -1028,6 +1044,12 @@ func (s *Session) onLaunchRequest(request *dap.LaunchRequest) {
if args.NoDebug {
s.mu.Lock()
cmd, err := s.newNoDebugProcess(debugbinary, args.Args, s.config.Debugger.WorkingDir)
if args.Console == "internalConsole" {
cmd.Stdout, cmd.Stderr = &debugConsoleLogger{session: s, category: "stdout"}, &debugConsoleLogger{session: s, category: "stderr"}
}
if err == nil {
err = cmd.Start()
}
s.mu.Unlock()
if err != nil {
s.sendShowUserErrorResponse(request.Request, FailedToLaunch, "Failed to launch", err.Error())
Expand All @@ -1049,6 +1071,32 @@ func (s *Session) onLaunchRequest(request *dap.LaunchRequest) {
return
}

func() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Followed a similar pattern I've seen later on of wrapping some logic in an inline function to ease breaking, not anything special - I can export it to a different method.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you refer to this file, that pattern is to execute a defer, not to group anything.

if args.Console == "internalConsole" && runtime.GOOS == "linux" && (args.Backend == "default" || args.Backend == "native") {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's weird that using internalConsole on an unsupported configuration fails silently. It's also weird that all values of console that aren't internalConsole are accepted as valid.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current behavior is that console is not checked, so it can also be completely invalid and dlv dap would accept it, so I'm not sure we necessarily have to validate the console argument.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suppose someone misspells internlaConsole, how do they notice?

Copy link
Contributor Author

@gfszr gfszr Aug 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Usually, such syntax can be checked in VSCode itself - it can present an error such as: "internalConsole" is not a valid "console" value".

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not everyone uses vscode with dap. I thought that was the whole point of this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, true. I am not familiar with how every DAP client handles this, but adding an allowed list of declared DAP consoles (I think it is standardized) should be easy enough.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it should just be the consoles that we support.

redirects, err := generateStdioTempPipes()
if err != nil {
return
}
s.config.Debugger.Redirects[1] = redirects[0]
s.config.Debugger.Redirects[2] = redirects[1]

stdoutWriter := &debugConsoleLogger{session: s, category: "stdout"}
stderrWriter := &debugConsoleLogger{session: s, category: "stderr"}
f := func(pipePath string, w io.Writer) {
// Blocking until read side of the pipe succeeds, and then delete file
pipe, err := os.Open(pipePath)
os.Remove(pipePath)
if err != nil {
return
}
defer pipe.Close()
io.Copy(w, pipe)
}
go f(redirects[0], stdoutWriter)
go f(redirects[1], stderrWriter)
}
}()

func() {
s.mu.Lock()
defer s.mu.Unlock() // Make sure to unlock in case of panic that will become internal error
Expand Down Expand Up @@ -1088,9 +1136,6 @@ func (s *Session) newNoDebugProcess(program string, targetArgs []string, wd stri
}
cmd := exec.Command(program, targetArgs...)
cmd.Stdout, cmd.Stderr, cmd.Stdin, cmd.Dir = os.Stdout, os.Stderr, os.Stdin, wd
if err := cmd.Start(); err != nil {
return nil, err
}
s.noDebugProcess = &process{Cmd: cmd, exited: make(chan struct{})}
return cmd, nil
}
Expand Down Expand Up @@ -1571,7 +1616,8 @@ func (s *Session) onConfigurationDoneRequest(request *dap.ConfigurationDoneReque
func (s *Session) onContinueRequest(request *dap.ContinueRequest, allowNextStateChange chan struct{}) {
s.send(&dap.ContinueResponse{
Response: *newResponse(request.Request),
Body: dap.ContinueResponseBody{AllThreadsContinued: true}})
Body: dap.ContinueResponseBody{AllThreadsContinued: true},
})
s.runUntilStopAndNotify(api.Continue, allowNextStateChange)
}

Expand Down Expand Up @@ -1635,7 +1681,8 @@ func (s *Session) onThreadsRequest(request *dap.ThreadsRequest) {
Body: dap.OutputEventBody{
Output: fmt.Sprintf("Unable to retrieve goroutines: %s\n", err.Error()),
Category: "stderr",
}})
},
})
}
threads = []dap.Thread{{Id: 1, Name: "Dummy"}}
} else if len(gs) == 0 {
Expand Down Expand Up @@ -2463,7 +2510,7 @@ func (s *Session) convertVariableWithOpts(v *proc.Variable, qualifiedNameOrExpr

// Some of the types might be fully or partially not loaded based on LoadConfig.
// Those that are fully missing (e.g. due to hitting MaxVariableRecurse), can be reloaded in place.
var reloadVariable = func(v *proc.Variable, qualifiedNameOrExpr string) (value string) {
reloadVariable := func(v *proc.Variable, qualifiedNameOrExpr string) (value string) {
// We might be loading variables from the frame that's not topmost, so use
// frame-independent address-based expression, not fully-qualified name as per
// https://github.com/go-delve/delve/blob/master/Documentation/api/ClientHowto.md#looking-into-variables.
Expand Down Expand Up @@ -3393,6 +3440,7 @@ func newEvent(event string) *dap.Event {

const BetterBadAccessError = `invalid memory address or nil pointer dereference [signal SIGSEGV: segmentation violation]
Unable to propagate EXC_BAD_ACCESS signal to target process and panic (see https://github.com/go-delve/delve/issues/852)`

const BetterNextWhileNextingError = `Unable to step while the previous step is interrupted by a breakpoint.
Use 'Continue' to resume the original step command.`

Expand Down
13 changes: 13 additions & 0 deletions service/dap/stdio_pipe.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
//go:build !linux
// +build !linux

package dap

import (
"errors"
)

func generateStdioTempPipes() (res [2]string, err error) {
err = errors.New("Unimplemented")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Merging this with just a linux implementation would be weird. This is enough of a prominent feature that having it implemented for one operating system implies that we have to implement it for at least the more important ones. I think it needs to support Windows and macOS.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implementing it to all platforms should be relatively easier if we can change the Redirects to not only be paths to stdout/stderr, but io.Writers. I'll implement complete support for native, which should be relatively easy to do soin that backend.

return res, err
}
33 changes: 33 additions & 0 deletions service/dap/stdio_pipe_linux.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
//go:build linux
// +build linux

package dap

import (
"crypto/rand"
"encoding/hex"
"os"
"path/filepath"
"syscall"
)

func generateStdioTempPipes() (res [2]string, err error) {
r := make([]byte, 4)
if _, err := rand.Read(r); err != nil {
return res, err
}
prefix := filepath.Join(os.TempDir(), hex.EncodeToString(r))
stdoutPath := prefix + "stdout"
stderrPath := prefix + "stderr"
if err := syscall.Mkfifo(stdoutPath, 0o600); err != nil {
return res, err
}
if err := syscall.Mkfifo(stderrPath, 0o600); err != nil {
os.Remove(stdoutPath)
return res, err
}

res[0] = stdoutPath
res[1] = stderrPath
return res, nil
}
6 changes: 6 additions & 0 deletions service/dap/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,12 @@ type LaunchConfig struct {
// reference to other environment variables is not supported.
Env map[string]*string `json:"env,omitempty"`

// Console specifies the console the user desired to execute with.
// If the user requested `internalConsole` as its terminal, then we
// wish to redirect its stdout/stderr as DAP OutputEvents.
// By default, we do not redirect if it doesn't exist.
Console string `json:"console,omitempty"`

LaunchAttachCommonConfig
}

Expand Down
25 changes: 21 additions & 4 deletions service/debugger/debugger.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"fmt"
"go/parser"
"go/token"
"io"
"os"
"os/exec"
"path/filepath"
Expand Down Expand Up @@ -139,6 +140,10 @@ type Config struct {
// Redirects specifies redirect rules for stdin, stdout and stderr
Redirects [3]string

// Writers written which are used instead of Redirects, in case the caller desires to do so
CaptureStdout io.Writer
CaptureStderr io.Writer

// DisableASLR disables ASLR
DisableASLR bool
}
Expand Down Expand Up @@ -727,7 +732,6 @@ func (d *Debugger) CreateBreakpoint(requestedBp *api.Breakpoint, locExpr string,
}
}
createdBp, err := createLogicalBreakpoint(d, requestedBp, &setbp)

if err != nil {
return nil, err
}
Expand Down Expand Up @@ -1879,6 +1883,13 @@ func (d *Debugger) FindLocation(goid int64, frame, deferredCall int, locStr stri
d.targetMutex.Lock()
defer d.targetMutex.Unlock()

if len(d.target.Targets()) != 1 {
// TODO(aarzilli): if there is more than one target process all must be
// searched and the addresses returned need to specify which target process
// they belong to.
panic("multiple targets not implemented")
}

if _, err := d.target.Valid(); err != nil {
return nil, err
}
Expand All @@ -1899,6 +1910,13 @@ func (d *Debugger) FindLocationSpec(goid int64, frame, deferredCall int, locStr
d.targetMutex.Lock()
defer d.targetMutex.Unlock()

if len(d.target.Targets()) != 1 {
// TODO(aarzilli): if there is more than one target process all must be
// searched and the addresses returned need to specify which target process
// they belong to.
panic("multiple targets not implemented")
}

if _, err := d.target.Valid(); err != nil {
return nil, err
}
Expand Down Expand Up @@ -2024,7 +2042,6 @@ func (d *Debugger) ListDynamicLibraries() []*proc.Image {
d.targetMutex.Lock()
defer d.targetMutex.Unlock()
return d.target.Selected.BinInfo().Images[1:] // skips the first image because it's the executable file

}

// ExamineMemory returns the raw memory stored at the given address.
Expand Down Expand Up @@ -2118,7 +2135,7 @@ func (d *Debugger) DumpStart(dest string) error {
d.targetMutex.Lock()
// targetMutex will only be unlocked when the dump is done

//TODO(aarzilli): what do we do if the user switches to a different target after starting a dump but before it's finished?
// TODO(aarzilli): what do we do if the user switches to a different target after starting a dump but before it's finished?

if !d.target.Selected.CanDump {
d.targetMutex.Unlock()
Expand Down Expand Up @@ -2270,7 +2287,7 @@ func verifyBinaryFormat(exePath string) error {
if err != nil {
return err
}
if (fi.Mode() & 0111) == 0 {
if (fi.Mode() & 0o111) == 0 {
return api.ErrNotExecutable
}
}
Expand Down