[Fix] (Bench, Trace, CI): let only the profile answer for itself, name an unreadable profile, and describe the CI step by the mechanism the tree has

- The verified-profile guard read the process environment as well as the profile: the test ran
  after the source, so PROFILE_VERIFIED=1 exported in an operator's shell re-opened the fail-open
  hole for every profile that says nothing. Both scripts now set PROFILE_VERIFIED=0 immediately
  before sourcing, so the file is the only thing that can answer.
- A --device path that cannot be sourced was diagnosed as an unverified profile, because both
  scripts cd to their own directory first and neither checked readability. The path is now also
  tried relative to the directory the script was invoked from (which is what a repo-root-relative
  --device means), and an unreadable one is reported as unreadable, naming both places tried.
- Verified: exported PROFILE_VERIFIED=1 + an unverified profile -> rc 2; exported 1 + a profile
  with no key -> rc 2; a repo-root-relative path -> resolved, then refused for its own reason;
  a missing file -> "cannot read the device profile"; odinlite.env -> past the guard;
  --allow-unverified-profile -> the three warnings, then proceeds.
- test.yml's new step described a mechanism the tree does not have. G6's and G10's entries are
  registered in the pull build too - they must be, for G2's name-for-name comparison - and skip
  inside their bodies. The step's value is unchanged and its comment now says the true thing: the
  `test` job runs those names as a column of skips, and this is the first CI job that unpacks a
  build which compiled the assertions.
- trace_benchmark takes the wall baseline before the CPU baseline, the order OnFrameBoundary
  already reads them in, so frame 0 stops reporting a CPU delta biased upward against its own
  wall delta; and it includes <time.h> rather than <ctime> for the POSIX names it uses.
This commit is contained in:
2026-09-07 23:18:10 -04:00
parent e5603f9a46
commit b1c37699b1
5 changed files with 64 additions and 10 deletions
+9 -6
View File
@@ -551,12 +551,15 @@ jobs:
# The push-only unit tests, on the verify runtime.
#
# WHY HERE AND NOT IN `test`. The `test` job builds the PULL library, and G6's chunk-table
# walk and G10's residual assertions live in MG_Test/Pipe, compiled only under
# MOBILEGL_PIPE_PUSH (MGPipeRenderStateSpans.cpp and PipeApply.cpp are appended to
# SOURCE_FILES inside the `if (MOBILEGL_PIPE_PUSH)` block, which is exactly how the pull
# build stays symbol-identical). So before P2 those tests ran in no CI job at all: they
# existed, they were green locally, and CI never executed one of them.
# WHY HERE AND NOT IN `test`. The entries themselves are registered in EVERY build - they
# have to be, or `ctest -N` would stop matching name-for-name between the pull and the push
# build (gate G2). What is push-only is what they assert about: MGPipeRenderStateSpans.cpp
# and PipeApply.cpp are appended to SOURCE_FILES inside the `if (MOBILEGL_PIPE_PUSH)` block,
# which is exactly how the pull build stays symbol-identical, so in a pull build each case
# opens with `#if !MOBILEGL_PIPE_PUSH GTEST_SKIP() << "push not compiled in"`. The `test`
# job therefore runs G6's chunk-table walk and G10's residual assertions as a column of
# skips: CI executes the NAMES and never one of the assertions. This job unpacks a build
# that compiled them, so it is the first place in CI where they actually run.
#
# This artifact already carries them - the packaging step above tars
# ${BUILD_DIR}/MobileGL/MG_Test whole - so the whole cost is the run, which is ~14 s for
@@ -8,8 +8,14 @@
// CLOCK_THREAD_CPUTIME_ID is POSIX and present on Linux and on every Android API this replays
// on; the guard exists so the desktop CLI still builds where it is not, and so that "no CPU
// series" is a compile-time fact rather than a silently-zero column.
//
// <time.h>, not <ctime>: clock_gettime, CLOCK_THREAD_CPUTIME_ID and struct timespec are POSIX
// names, and only <time.h> is required to put them at global scope - <ctime> guarantees the C++
// subset in namespace std and leaves the rest to the implementation. glibc and bionic both happen
// to provide them either way; this file is built for both by two different toolchains, so it asks
// for the header that actually promises what it uses.
#if defined(__unix__) || defined(__linux__) || defined(__ANDROID__) || defined(__APPLE__)
#include <ctime>
#include <time.h>
#define MOBILEGL_TRACE_HAVE_THREAD_CPU_CLOCK 1
#else
#define MOBILEGL_TRACE_HAVE_THREAD_CPU_CLOCK 0
@@ -78,12 +84,17 @@ void Begin(bool finishEachFrame) {
gFrameMs.reserve(kFrameReserve);
gFrameCpuMs.clear();
gFrameCpuMs.reserve(kFrameReserve);
gLastBoundaryCpuMs = ThreadCpuMs();
gFinishEachFrame = finishEachFrame;
gResolvedGlFinish = false;
gGlFinish = nullptr;
// Wall baseline FIRST, CPU baseline second - the same order OnFrameBoundary reads them in,
// and for the same reason. Frame 0's CPU interval then sits strictly inside its wall interval,
// so whatever this function costs between the two readings lands in the wall number where it
// can be seen, instead of inflating the CPU number where it cannot. Taken the other way round
// (as this was), frame 0 alone reported a CPU delta biased upward against its own wall delta.
gStart = Clock::now();
gLastBoundary = gStart;
gLastBoundaryCpuMs = ThreadCpuMs();
gEnabled = true;
}
+4
View File
@@ -37,6 +37,10 @@ frequency-pin integrity.
`--allow-unverified-profile` is passed, which labels the run unpinned in the warning.
**A profile that omits the key entirely is refused the same way** - the guard defaults to
unverified, so copying a verified profile and editing the serial cannot inherit its verdict.
Only the file can answer: both scripts reset `PROFILE_VERIFIED=0` immediately before sourcing
it, so `PROFILE_VERIFIED=1` exported in your shell does not re-open the hole. Nor does a
profile path that cannot be read get mistaken for an unverified one - it is reported as
unreadable, and a path relative to the directory you ran the script from is resolved.
(`profile.sh` pins nothing - it records a simpleperf profile - so it carries no such guard.)
That refusal exists because the pin path is silent when it is wrong: the harness writes
Executable → Regular
+21 -1
View File
@@ -22,6 +22,11 @@
# Screenshots (pre/post measurement) land in results/<timestamp>-<label>/.
set -u -o pipefail
# Remembered BEFORE the cd, so a --device path written relative to the caller's directory (the
# repo root, most of the time) still resolves. Without it, `tools/device_bench/bench.sh --device
# tools/device_bench/devices/odinlite.env` from the repo root sourced nothing and then blamed the
# profile for not being verified - a true refusal for a false reason.
INVOKED_FROM=$PWD
cd "$(dirname "$0")"
# Git Bash: stop MSYS from rewriting /sys/... arguments into C:/Program Files/...
export MSYS_NO_PATHCONV=1 MSYS2_ARG_CONV_EXCL='*'
@@ -55,6 +60,20 @@ while [ $# -gt 0 ]; do
done
[ -n "$DEVICE_ENV" ] && [ -n "$BACKEND" ] || { echo "need --device and --backend" >&2; exit 2; }
case "$DEVICE_ENV" in
/*) ;;
*) [ -r "$DEVICE_ENV" ] || [ ! -r "$INVOKED_FROM/$DEVICE_ENV" ] || DEVICE_ENV="$INVOKED_FROM/$DEVICE_ENV" ;;
esac
[ -r "$DEVICE_ENV" ] || {
echo "cannot read the device profile: $DEVICE_ENV" >&2
echo "(tried it relative to $(pwd) and to $INVOKED_FROM)" >&2
exit 2
}
# The profile, and ONLY the profile, gets to say whether it has been verified. This is set to the
# refusing value BEFORE the source, so a PROFILE_VERIFIED=1 left exported in the operator's shell
# cannot answer for a profile that says nothing - which would be the same fail-open hole the
# guard below closes, entered through the environment instead of through the file.
PROFILE_VERIFIED=0
# shellcheck disable=SC1090
. "$DEVICE_ENV"
@@ -72,7 +91,8 @@ require_verified_profile() {
# The default is UNVERIFIED. A profile that simply omits the key is a profile nobody has
# confirmed against its device, and defaulting it to "verified" would hand exactly the
# fail-open behaviour this guard exists to prevent to the most likely way a new profile is
# written - by copying an existing one and editing the serial.
# written - by copying an existing one and editing the serial. The variable is reset to 0
# immediately before the profile is sourced, so this test reads the FILE and not the shell.
if [ "${PROFILE_VERIFIED:-0}" = "1" ]; then return 0; fi
if [ "$ALLOW_UNVERIFIED_PROFILE" = "1" ]; then
echo "[warn] $DEVICE_ENV does not carry PROFILE_VERIFIED=1 and --allow-unverified-profile was passed:" >&2
Executable → Regular
+17 -1
View File
@@ -16,6 +16,9 @@
# cleanup unpins).
set -u -o pipefail
# Remembered BEFORE the cd, so a --device path written relative to the caller's directory still
# resolves instead of being reported as an unverified profile (see bench.sh).
INVOKED_FROM=$PWD
cd "$(dirname "$0")"
export MSYS_NO_PATHCONV=1 MSYS2_ARG_CONV_EXCL='*'
@@ -38,6 +41,18 @@ while [ $# -gt 0 ]; do
esac
done
[ -n "$DEVICE_ENV" ] || { echo "need --device" >&2; exit 2; }
case "$DEVICE_ENV" in
/*) ;;
*) [ -r "$DEVICE_ENV" ] || [ ! -r "$INVOKED_FROM/$DEVICE_ENV" ] || DEVICE_ENV="$INVOKED_FROM/$DEVICE_ENV" ;;
esac
[ -r "$DEVICE_ENV" ] || {
echo "cannot read the device profile: $DEVICE_ENV" >&2
echo "(tried it relative to $(pwd) and to $INVOKED_FROM)" >&2
exit 2
}
# The profile, and ONLY the profile, gets to say whether it has been verified: reset before the
# source, so an exported PROFILE_VERIFIED=1 cannot answer for a profile that says nothing.
PROFILE_VERIFIED=0
# shellcheck disable=SC1090
. "$DEVICE_ENV"
@@ -55,7 +70,8 @@ require_verified_profile() {
# The default is UNVERIFIED. A profile that simply omits the key is a profile nobody has
# confirmed against its device, and defaulting it to "verified" would hand exactly the
# fail-open behaviour this guard exists to prevent to the most likely way a new profile is
# written - by copying an existing one and editing the serial.
# written - by copying an existing one and editing the serial. The variable is reset to 0
# immediately before the profile is sourced, so this test reads the FILE and not the shell.
if [ "${PROFILE_VERIFIED:-0}" = "1" ]; then return 0; fi
if [ "$ALLOW_UNVERIFIED_PROFILE" = "1" ]; then
echo "[warn] $DEVICE_ENV does not carry PROFILE_VERIFIED=1 and --allow-unverified-profile was passed:" >&2