[skip-changelog] Made the Debug* gRPC API implementation in par with the rest (#2672)

* Inlined gRPC methods to implement GetDebugConfig and IsDebugSupported

* Inlined function

* Renamed vars for clarity

* Added Debug gRPC adapter function

* Moved function and removed useless file

* Forward os.interrupt (aka CTRL-C) signal to the gdb process

This a challenging problem because we must wait on both an io.Read(...)
and a channel-read but, unfortunately, go native select can wait only on
channels.
To overcome this limitation I had to resort to a conditional variable and
write some boilerplate code to make everything synchronized.

Author: cmaglie

commands/service_debug.go

@@ -18,59 +18,317 @@ package commands
 import (
 	"context"
 	"errors"
+	"fmt"
+	"io"
 	"os"
+	"path/filepath"
+	"runtime"
+	"sync"
+	"sync/atomic"
+	"time"
 
+	"github.com/arduino/arduino-cli/commands/cmderrors"
+	"github.com/arduino/arduino-cli/commands/internal/instances"
+	"github.com/arduino/arduino-cli/internal/arduino/cores/packagemanager"
 	"github.com/arduino/arduino-cli/internal/i18n"
 	rpc "github.com/arduino/arduino-cli/rpc/cc/arduino/cli/commands/v1"
+	paths "github.com/arduino/go-paths-helper"
+	"github.com/djherbis/buffer"
+	"github.com/djherbis/nio/v3"
+	"github.com/sirupsen/logrus"
+	"google.golang.org/grpc/metadata"
 )
 
-// Debug returns a stream response that can be used to fetch data from the
-// target. The first message passed through the `Debug` request must
-// contain DebugRequest configuration params, not data.
+type debugServer struct {
+	ctx      context.Context
+	req      atomic.Pointer[rpc.GetDebugConfigRequest]
+	in       io.Reader
+	inSignal bool
+	inData   bool
+	inEvent  *sync.Cond
+	inLock   sync.Mutex
+	out      io.Writer
+	resultCB func(*rpc.DebugResponse_Result)
+	done     chan bool
+}
+
+func (s *debugServer) Send(resp *rpc.DebugResponse) error {
+	if len(resp.GetData()) > 0 {
+		if _, err := s.out.Write(resp.GetData()); err != nil {
+			return err
+		}
+	}
+	if res := resp.GetResult(); res != nil {
+		s.resultCB(res)
+		s.close()
+	}
+	return nil
+}
+
+func (s *debugServer) Recv() (r *rpc.DebugRequest, e error) {
+	if conf := s.req.Swap(nil); conf != nil {
+		return &rpc.DebugRequest{DebugRequest: conf}, nil
+	}
+
+	s.inEvent.L.Lock()
+	for !s.inSignal && !s.inData {
+		s.inEvent.Wait()
+	}
+	defer s.inEvent.L.Unlock()
+
+	if s.inSignal {
+		s.inSignal = false
+		return &rpc.DebugRequest{SendInterrupt: true}, nil
+	}
+
+	if s.inData {
+		s.inData = false
+		buff := make([]byte, 4096)
+		n, err := s.in.Read(buff)
+		if err != nil {
+			return nil, err
+		}
+		return &rpc.DebugRequest{Data: buff[:n]}, nil
+	}
+
+	panic("invalid state in debug")
+}
+
+func (s *debugServer) close() {
+	close(s.done)
+}
+
+func (s *debugServer) Context() context.Context     { return s.ctx }
+func (s *debugServer) RecvMsg(m any) error          { return nil }
+func (s *debugServer) SendHeader(metadata.MD) error { return nil }
+func (s *debugServer) SendMsg(m any) error          { return nil }
+func (s *debugServer) SetHeader(metadata.MD) error  { return nil }
+func (s *debugServer) SetTrailer(metadata.MD)       {}
+
+// DebugServerToStreams creates a debug server that proxies the data to the given io streams.
+// The GetDebugConfigRequest is used to configure the debbuger. sig is a channel that can be
+// used to send os.Interrupt to the debug process. resultCB is a callback function that will
+// receive the Debug result and closes the debug server.
+func DebugServerToStreams(
+	ctx context.Context,
+	req *rpc.GetDebugConfigRequest,
+	in io.Reader, out io.Writer,
+	sig chan os.Signal,
+	resultCB func(*rpc.DebugResponse_Result),
+) rpc.ArduinoCoreService_DebugServer {
+	server := &debugServer{
+		ctx:      ctx,
+		in:       in,
+		out:      out,
+		resultCB: resultCB,
+		done:     make(chan bool),
+	}
+	serverIn, clientOut := nio.Pipe(buffer.New(32 * 1024))
+	server.in = serverIn
+	server.inEvent = sync.NewCond(&server.inLock)
+	server.req.Store(req)
+	go func() {
+		for {
+			select {
+			case <-sig:
+				server.inEvent.L.Lock()
+				server.inSignal = true
+				server.inEvent.Broadcast()
+				server.inEvent.L.Unlock()
+			case <-server.done:
+				return
+			}
+		}
+	}()
+	go func() {
+		defer clientOut.Close()
+		buff := make([]byte, 4096)
+		for {
+			n, readErr := in.Read(buff)
+
+			server.inEvent.L.Lock()
+			var writeErr error
+			if readErr == nil {
+				_, writeErr = clientOut.Write(buff[:n])
+			}
+			server.inData = true
+			server.inEvent.Broadcast()
+			server.inEvent.L.Unlock()
+			if readErr != nil || writeErr != nil {
+				// exit on error
+				return
+			}
+		}
+	}()
+	return server
+}
+
+// Debug starts a debugging session. The first message passed through the `Debug` request must
+// contain DebugRequest configuration params and no data.
 func (s *arduinoCoreServerImpl) Debug(stream rpc.ArduinoCoreService_DebugServer) error {
+	// Utility functions
+	syncSend := NewSynchronizedSend(stream.Send)
+	sendResult := func(res *rpc.DebugResponse_Result) error {
+		return syncSend.Send(&rpc.DebugResponse{Message: &rpc.DebugResponse_Result_{Result: res}})
+	}
+	sendData := func(data []byte) {
+		_ = syncSend.Send(&rpc.DebugResponse{Message: &rpc.DebugResponse_Data{Data: data}})
+	}
+
 	// Grab the first message
-	msg, err := stream.Recv()
+	debugConfReqMsg, err := stream.Recv()
 	if err != nil {
 		return err
 	}
 
 	// Ensure it's a config message and not data
-	req := msg.GetDebugRequest()
-	if req == nil {
+	debugConfReq := debugConfReqMsg.GetDebugRequest()
+	if debugConfReq == nil {
 		return errors.New(i18n.Tr("First message must contain debug request, not data"))
 	}
 
 	// Launch debug recipe attaching stdin and out to grpc streaming
 	signalChan := make(chan os.Signal)
 	defer close(signalChan)
-	outStream := feedStreamTo(func(data []byte) {
-		stream.Send(&rpc.DebugResponse{Message: &rpc.DebugResponse_Data{
-			Data: data,
-		}})
-	})
-	resp, debugErr := Debug(stream.Context(), req,
-		consumeStreamFrom(func() ([]byte, error) {
-			command, err := stream.Recv()
-			if command.GetSendInterrupt() {
+	outStream := feedStreamTo(sendData)
+	defer outStream.Close()
+	inStream := consumeStreamFrom(func() ([]byte, error) {
+		for {
+			req, err := stream.Recv()
+			if err != nil {
+				return nil, err
+			}
+			if req.GetSendInterrupt() {
 				signalChan <- os.Interrupt
 			}
-			return command.GetData(), err
-		}),
-		outStream,
-		signalChan)
-	outStream.Close()
-	if debugErr != nil {
-		return debugErr
-	}
-	return stream.Send(resp)
-}
+			if data := req.GetData(); len(data) > 0 {
+				return data, nil
+			}
+		}
+	})
+
+	pme, release, err := instances.GetPackageManagerExplorer(debugConfReq.GetInstance())
+	if err != nil {
+		return err
+	}
+	defer release()
+
+	// Exec debugger
+	commandLine, err := getCommandLine(debugConfReq, pme)
+	if err != nil {
+		return err
+	}
+	entry := logrus.NewEntry(logrus.StandardLogger())
+	for i, param := range commandLine {
+		entry = entry.WithField(fmt.Sprintf("param%d", i), param)
+	}
+	entry.Debug("Executing debugger")
+	cmd, err := paths.NewProcess(pme.GetEnvVarsForSpawnedProcess(), commandLine...)
+	if err != nil {
+		return &cmderrors.FailedDebugError{Message: i18n.Tr("Cannot execute debug tool"), Cause: err}
+	}
+	in, err := cmd.StdinPipe()
+	if err != nil {
+		return sendResult(&rpc.DebugResponse_Result{Error: err.Error()})
+	}
+	defer in.Close()
+	cmd.RedirectStdoutTo(io.Writer(outStream))
+	cmd.RedirectStderrTo(io.Writer(outStream))
+	if err := cmd.Start(); err != nil {
+		return sendResult(&rpc.DebugResponse_Result{Error: err.Error()})
+	}
 
-// GetDebugConfig return metadata about a debug session
-func (s *arduinoCoreServerImpl) GetDebugConfig(ctx context.Context, req *rpc.GetDebugConfigRequest) (*rpc.GetDebugConfigResponse, error) {
-	return GetDebugConfig(ctx, req)
+	go func() {
+		for sig := range signalChan {
+			cmd.Signal(sig)
+		}
+	}()
+	go func() {
+		io.Copy(in, inStream)
+		time.Sleep(time.Second)
+		cmd.Kill()
+	}()
+	if err := cmd.Wait(); err != nil {
+		return sendResult(&rpc.DebugResponse_Result{Error: err.Error()})
+	}
+	return sendResult(&rpc.DebugResponse_Result{})
 }
 
-// IsDebugSupported checks if debugging is supported for a given configuration
-func (s *arduinoCoreServerImpl) IsDebugSupported(ctx context.Context, req *rpc.IsDebugSupportedRequest) (*rpc.IsDebugSupportedResponse, error) {
-	return IsDebugSupported(ctx, req)
+// getCommandLine compose a debug command represented by a core recipe
+func getCommandLine(req *rpc.GetDebugConfigRequest, pme *packagemanager.Explorer) ([]string, error) {
+	debugInfo, err := getDebugProperties(req, pme, false)
+	if err != nil {
+		return nil, err
+	}
+
+	cmdArgs := []string{}
+	add := func(s string) { cmdArgs = append(cmdArgs, s) }
+
+	// Add path to GDB Client to command line
+	var gdbPath *paths.Path
+	switch debugInfo.GetToolchain() {
+	case "gcc":
+		gdbexecutable := debugInfo.GetToolchainPrefix() + "-gdb"
+		if runtime.GOOS == "windows" {
+			gdbexecutable += ".exe"
+		}
+		gdbPath = paths.New(debugInfo.GetToolchainPath()).Join(gdbexecutable)
+	default:
+		return nil, &cmderrors.FailedDebugError{Message: i18n.Tr("Toolchain '%s' is not supported", debugInfo.GetToolchain())}
+	}
+	add(gdbPath.String())
+
+	// Set GDB interpreter (default value should be "console")
+	gdbInterpreter := req.GetInterpreter()
+	if gdbInterpreter == "" {
+		gdbInterpreter = "console"
+	}
+	add("--interpreter=" + gdbInterpreter)
+	if gdbInterpreter != "console" {
+		add("-ex")
+		add("set pagination off")
+	}
+
+	// Add extra GDB execution commands
+	add("-ex")
+	add("set remotetimeout 5")
+
+	// Extract path to GDB Server
+	switch debugInfo.GetServer() {
+	case "openocd":
+		var openocdConf rpc.DebugOpenOCDServerConfiguration
+		if err := debugInfo.GetServerConfiguration().UnmarshalTo(&openocdConf); err != nil {
+			return nil, err
+		}
+
+		serverCmd := fmt.Sprintf(`target extended-remote | "%s"`, debugInfo.GetServerPath())
+
+		if cfg := openocdConf.GetScriptsDir(); cfg != "" {
+			serverCmd += fmt.Sprintf(` -s "%s"`, cfg)
+		}
+
+		for _, script := range openocdConf.GetScripts() {
+			serverCmd += fmt.Sprintf(` --file "%s"`, script)
+		}
+
+		serverCmd += ` -c "gdb_port pipe"`
+		serverCmd += ` -c "telnet_port 0"`
+
+		add("-ex")
+		add(serverCmd)
+
+	default:
+		return nil, &cmderrors.FailedDebugError{Message: i18n.Tr("GDB server '%s' is not supported", debugInfo.GetServer())}
+	}
+
+	// Add executable
+	add(debugInfo.GetExecutable())
+
+	// Transform every path to forward slashes (on Windows some tools further
+	// escapes the command line so the backslash "\" gets in the way).
+	for i, param := range cmdArgs {
+		cmdArgs[i] = filepath.ToSlash(param)
+	}
+
+	return cmdArgs, nil
 }

commands/service_debug_config.go

@@ -38,7 +38,7 @@ import (
 )
 
 // GetDebugConfig returns metadata to start debugging with the specified board
-func GetDebugConfig(ctx context.Context, req *rpc.GetDebugConfigRequest) (*rpc.GetDebugConfigResponse, error) {
+func (s *arduinoCoreServerImpl) GetDebugConfig(ctx context.Context, req *rpc.GetDebugConfigRequest) (*rpc.GetDebugConfigResponse, error) {
 	pme, release, err := instances.GetPackageManagerExplorer(req.GetInstance())
 	if err != nil {
 		return nil, err
@@ -48,7 +48,7 @@ func GetDebugConfig(ctx context.Context, req *rpc.GetDebugConfigRequest) (*rpc.G
 }
 
 // IsDebugSupported checks if the given board/programmer configuration supports debugging.
-func IsDebugSupported(ctx context.Context, req *rpc.IsDebugSupportedRequest) (*rpc.IsDebugSupportedResponse, error) {
+func (s *arduinoCoreServerImpl) IsDebugSupported(ctx context.Context, req *rpc.IsDebugSupportedRequest) (*rpc.IsDebugSupportedResponse, error) {
 	pme, release, err := instances.GetPackageManagerExplorer(req.GetInstance())
 	if err != nil {
 		return nil, err