Add validation info to gapis (google#992)

Tanat Boozayaangool · web-flow · commit 92fafcea7fbe · 2022-02-24T15:20:52.000Z
* Add validation info to gapis
- Always download perfetto trace from validation
- Include path to trace in result proto
diff --git a/cmd/gapit/validate_gpu_profiling.go b/cmd/gapit/validate_gpu_profiling.go
@@ -51,9 +51,13 @@ func (verb *validateGpuProfilingVerb) Run(ctx context.Context, flags flag.FlagSe
 	someDeviceFailed := false
 	for i, p := range devices {
 		fmt.Fprintf(stdout, "-- Device %v: %v --\n", i, p.ID.ID())
-		err = client.ValidateDevice(ctx, p)
+		res, err := client.ValidateDevice(ctx, p)
 		if err != nil {
-			fmt.Fprintf(stdout, "%v\n", log.Err(ctx, err, "Failed to validate device"))
+			fmt.Fprintf(stdout, "%v\n", log.Errf(ctx, err, "Failed to start device validation: %s", err))
+			someDeviceFailed = true
+			continue
+		} else if len(res.ValidationFailureMsg) > 0 {
+			fmt.Fprintf(stdout, "%v\n", log.Errf(ctx, nil, "Device validation failed: %s, trace file: %s", res.ValidationFailureMsg, res.TracePath))
 			someDeviceFailed = true
 			continue
 		}
diff --git a/core/os/file/path.go b/core/os/file/path.go
@@ -16,6 +16,7 @@ package file
 
 import (
 	"encoding/json"
+	"fmt"
 	"io/ioutil"
 	"net/url"
 	"os"
@@ -56,6 +57,16 @@ func Temp() (Path, error) {
 	return Abs(p.Name()), nil
 }
 
+// TempWithExt creates a new temp file with the given name and extension and returns its path.
+func TempWithExt(name string, ext string) (Path, error) {
+	p, err := ioutil.TempFile("", fmt.Sprintf("%s*.%s", name, ext))
+	if err != nil {
+		return Path{}, err
+	}
+	p.Close()
+	return Abs(p.Name()), nil
+}
+
 // ExecutablePath returns the path to the running executable.
 func ExecutablePath() Path {
 	path := Abs(os.Args[0])
diff --git a/gapic/src/main/com/google/gapid/models/Devices.java b/gapic/src/main/com/google/gapid/models/Devices.java
@@ -446,7 +446,7 @@ public DeviceValidationResult(Service.Error error, boolean passed, boolean skipp
     }
 
     public DeviceValidationResult(Service.ValidateDeviceResponse r) {
-      this(r.getError(), !r.hasError(), false);
+      this(r.getError(), !r.hasError() && r.getResult().getValidationFailureMsg().length() == 0, false);
     }
 
     @Override
diff --git a/gapis/client/client.go b/gapis/client/client.go
@@ -577,17 +577,17 @@ func (c *client) PerfettoQuery(ctx context.Context, capture *path.Capture, query
 	return res.GetResult(), nil
 }
 
-func (c *client) ValidateDevice(ctx context.Context, device *path.Device) error {
+func (c *client) ValidateDevice(ctx context.Context, device *path.Device) (*service.DeviceValidationResult, error) {
 	res, err := c.client.ValidateDevice(ctx, &service.ValidateDeviceRequest{
 		Device: device,
 	})
 	if err != nil {
-		return err
+		return nil, err
 	}
 	if err := res.GetError(); err != nil {
-		return err.Get()
+		return nil, err.Get()
 	}
-	return nil
+	return res.GetResult(), nil
 }
 
 func (c *client) InstallApp(ctx context.Context, d *path.Device, app string) error {
diff --git a/gapis/config/config.go b/gapis/config/config.go
@@ -38,9 +38,6 @@ const (
 	// DumpReplayProfile dumps the perfetto trace of a replay profile.
 	DumpReplayProfile = false
 
-	// DumpValidationTrace dumps the perfetto trace of a validation profile.
-	DumpValidationTrace = false
-
 	// AllInitialCommandsLive forces all initial commands to be considered as
 	// live when computing dead code elimination.
 	AllInitialCommandsLive = false
diff --git a/gapis/server/grpc.go b/gapis/server/grpc.go
@@ -688,11 +688,11 @@ func (s *grpcServer) PerfettoQuery(ctx xctx.Context, req *service.PerfettoQueryR
 
 func (s *grpcServer) ValidateDevice(ctx xctx.Context, req *service.ValidateDeviceRequest) (*service.ValidateDeviceResponse, error) {
 	defer s.inRPC()()
-	err := s.handler.ValidateDevice(s.bindCtx(ctx), req.Device)
+	res, err := s.handler.ValidateDevice(s.bindCtx(ctx), req.Device)
 	if err := service.NewError(err); err != nil {
-		return &service.ValidateDeviceResponse{Error: err}, nil
+		return &service.ValidateDeviceResponse{Res: &service.ValidateDeviceResponse_Error{Error: err}}, nil
 	}
-	return &service.ValidateDeviceResponse{}, nil
+	return &service.ValidateDeviceResponse{Res: &service.ValidateDeviceResponse_Result{Result: res}}, nil
 }
 
 func (s *grpcServer) InstallApp(ctx xctx.Context, req *service.InstallAppRequest) (*service.InstallAppResponse, error) {
diff --git a/gapis/server/server.go b/gapis/server/server.go
@@ -930,11 +930,11 @@ func (s *server) PerfettoQuery(ctx context.Context, c *path.Capture, query strin
 	return res, nil
 }
 
-func (s *server) ValidateDevice(ctx context.Context, d *path.Device) error {
+func (s *server) ValidateDevice(ctx context.Context, d *path.Device) (*service.DeviceValidationResult, error) {
 	ctx = status.Start(ctx, "RPC ValidateDevice")
 	defer status.Finish(ctx)
 	ctx = log.Enter(ctx, "ValidateDevice")
-	return trace.Validate(ctx, d)
+	return trace.Validate(ctx, d, s.enableLocalFiles)
 }
 
 func (s *server) InstallApp(ctx context.Context, d *path.Device, app string) error {
diff --git a/gapis/service/service.go b/gapis/service/service.go
@@ -190,7 +190,7 @@ type Service interface {
 
 	// ValidateDevice validates the GPU profiling capabilities of the given device and returns
 	// an error if validation failed or the GPU profiling data is invalid.
-	ValidateDevice(ctx context.Context, d *path.Device) error
+	ValidateDevice(ctx context.Context, d *path.Device) (*DeviceValidationResult, error)
 
 	// InstallApp installs an application on the given device.
 	InstallApp(ctx context.Context, d *path.Device, app string) error
diff --git a/gapis/service/service.proto b/gapis/service/service.proto
@@ -1482,7 +1482,23 @@ message ValidateDeviceRequest {
 }
 
 message ValidateDeviceResponse {
-  Error error = 1;
+  reserved 1;
+  oneof res {
+    DeviceValidationResult result = 2;
+
+    // Internal error from being unable to start validation
+    // (doesn't include failure from the validation itself).
+    Error error = 3;
+  }
+}
+
+message DeviceValidationResult {
+  // Message with details of how the device failed validation (doesn't
+  // include internal errors where we somehow failed to start validation).
+  string validation_failure_msg = 1;
+
+  // Path to the perfetto trace file to help debug validation issues.
+  string trace_path = 2;
 }
 
 message FuchsiaTraceConfig {
diff --git a/gapis/trace/android/BUILD.bazel b/gapis/trace/android/BUILD.bazel
@@ -30,12 +30,12 @@ go_library(
         "//core/os/android/apk:go_default_library",
         "//core/os/device:go_default_library",
         "//core/os/device/bind:go_default_library",
+        "//core/os/file:go_default_library",
         "//gapidapk:go_default_library",
         "//gapidapk/pkginfo:go_default_library",
         "//gapii/client:go_default_library",
         "//gapis/api:go_default_library",
         "//gapis/api/sync:go_default_library",
-        "//gapis/config:go_default_library",
         "//gapis/perfetto:go_default_library",
         "//gapis/perfetto/android:go_default_library",
         "//gapis/service:go_default_library",
diff --git a/gapis/trace/android/trace.go b/gapis/trace/android/trace.go
@@ -20,6 +20,7 @@ import (
 	"context"
 	"fmt"
 	"io"
+	"io/fs"
 	"io/ioutil"
 	"os"
 	"path/filepath"
@@ -30,10 +31,8 @@ import (
 
 	perfetto_pb "protos/perfetto/config"
 
-	"github.com/google/gapid/gapis/api/sync"
-	"github.com/google/gapid/gapis/service/path"
-
 	"github.com/golang/protobuf/proto"
+
 	"github.com/google/gapid/core/app"
 	"github.com/google/gapid/core/app/crash"
 	"github.com/google/gapid/core/app/status"
@@ -44,14 +43,16 @@ import (
 	"github.com/google/gapid/core/os/android/apk"
 	"github.com/google/gapid/core/os/device"
 	"github.com/google/gapid/core/os/device/bind"
+	"github.com/google/gapid/core/os/file"
 	"github.com/google/gapid/gapidapk"
 	"github.com/google/gapid/gapidapk/pkginfo"
 	gapii "github.com/google/gapid/gapii/client"
 	"github.com/google/gapid/gapis/api"
-	"github.com/google/gapid/gapis/config"
+	"github.com/google/gapid/gapis/api/sync"
 	"github.com/google/gapid/gapis/perfetto"
 	perfetto_android "github.com/google/gapid/gapis/perfetto/android"
 	"github.com/google/gapid/gapis/service"
+	"github.com/google/gapid/gapis/service/path"
 	"github.com/google/gapid/gapis/trace/android/adreno"
 	"github.com/google/gapid/gapis/trace/android/mali"
 	"github.com/google/gapid/gapis/trace/android/profile"
@@ -175,45 +176,52 @@ func (t *androidTracer) ProcessProfilingData(ctx context.Context, buffer *bytes.
 	}, nil
 }
 
-func (t *androidTracer) Validate(ctx context.Context) error {
+func (t *androidTracer) Validate(ctx context.Context, enableLocalFiles bool) (*service.DeviceValidationResult, error) {
 	ctx = status.Start(ctx, "Android Device Validation")
 	defer status.Finish(ctx)
+
 	if t.v == nil {
-		return log.Errf(ctx, nil, "No validator found for device %d", t.b.Instance().ID.ID())
+		return nil, log.Errf(ctx, nil, "No validator found for device %d", t.b.Instance().ID.ID())
 	}
 	d := t.b.(adb.Device)
 	osConfiguration := d.Instance().GetConfiguration()
 	if osConfiguration.GetOS().GetAPIVersion() < minimumSupportedApiLevel {
-		return log.Errf(ctx, nil, "Unsupported OS version on device %d", d.Instance().ID.ID())
+		return &service.DeviceValidationResult{
+			ValidationFailureMsg: fmt.Sprintf("Device OS (%d) is below the minimum supported version (API level) (%d)", osConfiguration.GetOS().GetAPIVersion(), minimumSupportedApiLevel),
+		}, nil
 	}
 	if osConfiguration.GetPerfettoCapability() == nil {
-		return log.Errf(ctx, nil, "No Perfetto Capability found on device %d", d.Instance().ID.ID())
+		return &service.DeviceValidationResult{
+			ValidationFailureMsg: fmt.Sprintf("No Perfetto Capability found on device %d", d.Instance().ID.ID()),
+		}, nil
 	}
 	if gpuProfiling := osConfiguration.GetPerfettoCapability().GetGpuProfiling(); gpuProfiling == nil || gpuProfiling.GetGpuCounterDescriptor() == nil {
-		return log.Errf(ctx, nil, "No GPU profiling capabilities found on device %d", d.Instance().ID.ID())
+		return &service.DeviceValidationResult{
+			ValidationFailureMsg: fmt.Sprintf("Device (%d) does not support GPU profiling or profiling support was not detected", d.Instance().ID.ID()),
+		}, nil
 	}
 
 	// Get ActivityAction
 	gapidPackage := gapidapk.PackageName(osConfiguration.PreferredABI(nil))
 	packages, _ := d.InstalledPackages(ctx)
 	pkg := packages.FindByName(gapidPackage)
 	if pkg == nil {
-		return log.Errf(ctx, nil, "Package %v not found", gapidPackage)
+		return nil, log.Errf(ctx, nil, "Package %v not found", gapidPackage)
 	}
 	activityAction := pkg.ActivityActions.FindByName(intentAction, activityName)
 	if activityAction == nil {
-		return log.Errf(ctx, nil, "Activity %v not found in %v", activityName, gapidPackage)
+		return nil, log.Errf(ctx, nil, "Activity %v not found in %v", activityName, gapidPackage)
 	}
 
 	// Construct trace config
 	traceOpts, err := deviceValidationTraceOptions(ctx, t.v)
 	if err != nil {
-		return log.Err(ctx, err, "Could not get the trace configuration")
+		return nil, log.Err(ctx, err, "Could not get the trace configuration")
 	}
 	process, cleanup, err := perfetto_android.Start(ctx, d, activityAction, traceOpts, nil, []string{})
 	if err != nil {
 		cleanup.Invoke(ctx)
-		return log.Err(ctx, err, "Error when start Perfetto tracing.")
+		return nil, log.Err(ctx, err, "Error when start Perfetto tracing.")
 	}
 	defer cleanup.Invoke(ctx)
 
@@ -223,6 +231,7 @@ func (t *androidTracer) Validate(ctx context.Context) error {
 
 	var buf bytes.Buffer
 	var written int64
+
 	// Start to capture.
 	status.Do(ctx, "Tracing", func(ctx context.Context) {
 		startSignal, startFunc := task.NewSignal()
@@ -240,32 +249,62 @@ func (t *androidTracer) Validate(ctx context.Context) error {
 
 		startFunc(ctx)
 		if !doneSignal.Wait(ctx) {
-			err = log.Err(ctx, err, "Fail to wait for done signal from Perfetto.")
+			err = log.Err(ctx, err, "Failed to wait for done signal from Perfetto.")
 		}
 		log.I(ctx, "Perfetto trace size %v bytes", written)
 	})
 	if err != nil {
-		return err
+		return nil, err
+	}
+
+	// Attempt to download perfetto trace.
+	var temp file.Path
+	if enableLocalFiles {
+		temp, err = file.TempWithExt("validation", "perfetto")
+		if err != nil {
+			return nil, log.Err(ctx, err, "Failed to create a temp file for trace")
+		}
 	}
 
 	var processor *perfetto.Processor
+	var traceLoadingErr error
 	status.Do(ctx, "Trace loading", func(ctx context.Context) {
-		// Load Perfetto trace and create trace processor.
-		if config.DumpValidationTrace {
-			if out, err := os.Create("validate.perfetto"); err == nil {
-				out.Write(buf.Bytes())
-				out.Close()
-			}
-		}
 		processor, err = perfetto.NewProcessor(ctx, buf.Bytes())
+		if err != nil {
+			traceLoadingErr = log.Err(ctx, err, "Failed to initialize the perfetto processor")
+			return
+		}
+		defer processor.Close()
+
+		file, err := os.OpenFile(temp.System(), os.O_APPEND|os.O_WRONLY, fs.ModeAppend)
+		if err != nil {
+			traceLoadingErr = log.Err(ctx, err, "Failed to open temp file")
+			return
+		}
+		defer file.Close()
+
+		numWritten, err := file.Write(buf.Bytes())
+		if err != nil {
+			traceLoadingErr = log.Err(ctx, err, "Failed to write trace to temp file")
+			return
+		}
+		log.I(ctx, "Writing trace size %v bytes to %v", numWritten, file.Name())
 	})
-	if err != nil {
-		return err
+	if traceLoadingErr != nil {
+		return nil, traceLoadingErr
+	}
+	res := &service.DeviceValidationResult{
+		TracePath: temp.System(),
 	}
-	defer processor.Close()
 	ctx = status.Start(ctx, "Validation")
 	defer status.Finish(ctx)
-	return t.v.Validate(ctx, processor)
+
+	err = t.v.Validate(ctx, processor)
+	if err != nil {
+		res.ValidationFailureMsg = err.Error()
+	}
+
+	return res, nil
 }
 
 func (t *androidTracer) GetPackages(ctx context.Context, isRoot bool, iconDensityScale float32) (*pkginfo.PackageList, error) {
diff --git a/gapis/trace/desktop/trace.go b/gapis/trace/desktop/trace.go
@@ -65,8 +65,8 @@ func (t *DesktopTracer) ProcessProfilingData(ctx context.Context, buffer *bytes.
 	return nil, log.Err(ctx, nil, "Desktop replay profiling is unsupported.")
 }
 
-func (t *DesktopTracer) Validate(ctx context.Context) error {
-	return nil
+func (t *DesktopTracer) Validate(ctx context.Context, enableLocalFiles bool) (*service.DeviceValidationResult, error) {
+	return &service.DeviceValidationResult{}, nil
 }
 
 // TraceConfiguration returns the device's supported trace configuration.
diff --git a/gapis/trace/fuchsia/trace.go b/gapis/trace/fuchsia/trace.go
@@ -149,8 +149,8 @@ func (t *fuchsiaTracer) ProcessProfilingData(ctx context.Context, buffer *bytes.
 
 // Validate validates the GPU profiling capabilities of the given device and returns
 // an error if validation failed or the GPU profiling data is invalid.
-func (t *fuchsiaTracer) Validate(ctx context.Context) error {
-	return nil
+func (t *fuchsiaTracer) Validate(ctx context.Context, enableLocalFiles bool) (*service.DeviceValidationResult, error) {
+	return &service.DeviceValidationResult{}, nil
 }
 
 func NewTracer(d bind.Device) tracer.Tracer {
diff --git a/gapis/trace/trace.go b/gapis/trace/trace.go
@@ -114,12 +114,13 @@ func ProcessProfilingData(ctx context.Context, device *path.Device, capture *pat
 	return t.ProcessProfilingData(ctx, buffer, capture, staticAnalysisResult, handleMapping, syncData)
 }
 
-func Validate(ctx context.Context, device *path.Device) error {
+func Validate(ctx context.Context, device *path.Device, enableLocalFiles bool) (*service.DeviceValidationResult, error) {
 	t, err := GetTracer(ctx, device)
 	if err != nil {
-		return err
+		return nil, err
 	}
-	return t.Validate(ctx)
+
+	return t.Validate(ctx, enableLocalFiles)
 }
 
 func GetTracer(ctx context.Context, device *path.Device) (tracer.Tracer, error) {
diff --git a/gapis/trace/tracer/tracer.go b/gapis/trace/tracer/tracer.go
@@ -79,7 +79,7 @@ type Tracer interface {
 	ProcessProfilingData(ctx context.Context, buffer *bytes.Buffer, capture *path.Capture, staticAnalysisResult chan *api.StaticAnalysisProfileData, handleMapping map[uint64][]service.VulkanHandleMappingItem, syncData *sync.Data) (*service.ProfilingData, error)
 	// Validate validates the GPU profiling capabilities of the given device and returns
 	// an error if validation failed or the GPU profiling data is invalid.
-	Validate(ctx context.Context) error
+	Validate(ctx context.Context, enableLocalFiles bool) (*service.DeviceValidationResult, error)
 }
 
 // LayersFromOptions Parses the perfetto options, and returns the required layers

Original file line number	Diff line number	Diff line change
`@@ -446,7 +446,7 @@ public DeviceValidationResult(Service.Error error, boolean passed, boolean skipp`
`446`	`446`	`}`
`447`	`447`
`448`	`448`	`public DeviceValidationResult(Service.ValidateDeviceResponse r) {`
`449`		`- this(r.getError(), !r.hasError(), false);`
	`449`	`+ this(r.getError(), !r.hasError() && r.getResult().getValidationFailureMsg().length() == 0, false);`
`450`	`450`	`}`
`451`	`451`
`452`	`452`	`@Override`
Original file line number	Diff line number	Diff line change
`@@ -577,17 +577,17 @@ func (c client) PerfettoQuery(ctx context.Context, capture path.Capture, query`
`577`	`577`	`return res.GetResult(), nil`
`578`	`578`	`}`
`579`	`579`
`580`		`-func (c client) ValidateDevice(ctx context.Context, device path.Device) error {`
	`580`	`+func (c client) ValidateDevice(ctx context.Context, device path.Device) (*service.DeviceValidationResult, error) {`
`581`	`581`	`res, err := c.client.ValidateDevice(ctx, &service.ValidateDeviceRequest{`
`582`	`582`	`Device: device,`
`583`	`583`	`})`
`584`	`584`	`if err != nil {`
`585`		`- return err`
	`585`	`+ return nil, err`
`586`	`586`	`}`
`587`	`587`	`if err := res.GetError(); err != nil {`
`588`		`- return err.Get()`
	`588`	`+ return nil, err.Get()`
`589`	`589`	`}`
`590`		`- return nil`
	`590`	`+ return res.GetResult(), nil`
`591`	`591`	`}`
`592`	`592`
`593`	`593`	`func (c client) InstallApp(ctx context.Context, d path.Device, app string) error {`
Original file line number	Diff line number	Diff line change
`@@ -688,11 +688,11 @@ func (s grpcServer) PerfettoQuery(ctx xctx.Context, req service.PerfettoQueryR`
`688`	`688`
`689`	`689`	`func (s grpcServer) ValidateDevice(ctx xctx.Context, req service.ValidateDeviceRequest) (*service.ValidateDeviceResponse, error) {`
`690`	`690`	`defer s.inRPC()()`
`691`		`- err := s.handler.ValidateDevice(s.bindCtx(ctx), req.Device)`
	`691`	`+ res, err := s.handler.ValidateDevice(s.bindCtx(ctx), req.Device)`
`692`	`692`	`if err := service.NewError(err); err != nil {`
`693`		`- return &service.ValidateDeviceResponse{Error: err}, nil`
	`693`	`+ return &service.ValidateDeviceResponse{Res: &service.ValidateDeviceResponse_Error{Error: err}}, nil`
`694`	`694`	`}`
`695`		`- return &service.ValidateDeviceResponse{}, nil`
	`695`	`+ return &service.ValidateDeviceResponse{Res: &service.ValidateDeviceResponse_Result{Result: res}}, nil`
`696`	`696`	`}`
`697`	`697`
`698`	`698`	`func (s grpcServer) InstallApp(ctx xctx.Context, req service.InstallAppRequest) (*service.InstallAppResponse, error) {`
Original file line number	Diff line number	Diff line change
`@@ -930,11 +930,11 @@ func (s server) PerfettoQuery(ctx context.Context, c path.Capture, query strin`
`930`	`930`	`return res, nil`
`931`	`931`	`}`
`932`	`932`
`933`		`-func (s server) ValidateDevice(ctx context.Context, d path.Device) error {`
	`933`	`+func (s server) ValidateDevice(ctx context.Context, d path.Device) (*service.DeviceValidationResult, error) {`
`934`	`934`	`ctx = status.Start(ctx, "RPC ValidateDevice")`
`935`	`935`	`defer status.Finish(ctx)`
`936`	`936`	`ctx = log.Enter(ctx, "ValidateDevice")`
`937`		`- return trace.Validate(ctx, d)`
	`937`	`+ return trace.Validate(ctx, d, s.enableLocalFiles)`
`938`	`938`	`}`
`939`	`939`
`940`	`940`	`func (s server) InstallApp(ctx context.Context, d path.Device, app string) error {`