[test_runner] Convert boolean use-elf to an enum gen-snapshot-format.

sstrickl · Commit Queue · commit af5221f44216 · 2025-05-26T08:53:03.000-07:00
Currently, the configuration code only has a boolean option, 'use-elf' for specifying the output format for gen_snapshot. If false, then the output format is assumed to be assembly. Add a new GenSnapshotFormat enum and replace the old 'use-elf' option with a new 'gen-snapshot-format' option. The new enum has two getters: * snapshotType returns the appropriate string to pass to gen_snapshot via the '--snapshot-type' option. * fileOption returns the name of the option used to specify the output path for the given format. In addition, make it so the Configuration only has a non-null genSnapshotFormat field if the compiler is Compiler.dartkp to avoid spurious differences in Configurations that are created with different GenSnapshotFormat values when the configurations in question don't actually call gen_snapshot. TEST=pkg/smith/test/configuration_test.dart (and the CI in general) Change-Id: I7f17dc8d3a1bb6d4bf57750bc5ef4a16b8a78c11 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/429980 Reviewed-by: Ryan Macnak <rmacnak@google.com> Commit-Queue: Tess Strickland <sstrickl@google.com>
diff --git a/pkg/smith/lib/configuration.dart b/pkg/smith/lib/configuration.dart
@@ -238,12 +238,15 @@ class Configuration {
     var system = enumOption("system", System.names, System.find);
     var nnbdMode = enumOption("nnbd", NnbdMode.names, NnbdMode.find);
     var sanitizer = enumOption("sanitizer", Sanitizer.names, Sanitizer.find);
+    var genSnapshotFormat = enumOption(
+        "gen-snapshot-format", GenSnapshotFormat.names, GenSnapshotFormat.find);
 
     // Fill in any missing values using defaults when possible.
     architecture ??= Architecture.host;
     system ??= System.host;
     nnbdMode ??= NnbdMode.strong;
     sanitizer ??= Sanitizer.none;
+    genSnapshotFormat ??= GenSnapshotFormat.assembly;
 
     // Infer runtime from executable if we don't know runtime and compiler.
     if (runtime == null && compiler == null && words.contains("custom")) {
@@ -295,9 +298,9 @@ class Configuration {
         isCsp: boolOption("csp"),
         enableHostAsserts: boolOption("host-asserts"),
         isMinified: boolOption("minified"),
+        genSnapshotFormat: genSnapshotFormat,
         useAnalyzerCfe: boolOption("use-cfe"),
         useAnalyzerFastaParser: boolOption("analyzer-use-fasta-parser"),
-        useElf: boolOption("use-elf"),
         useHotReload: boolOption("hot-reload"),
         useHotReloadRollback: boolOption("hot-reload-rollback"),
         useSdk: boolOption("use-sdk"),
@@ -369,7 +372,7 @@ class Configuration {
   final bool useAnalyzerCfe;
   final bool useAnalyzerFastaParser;
 
-  final bool useElf;
+  final GenSnapshotFormat? genSnapshotFormat;
 
   final bool useHotReload;
   final bool useHotReloadRollback;
@@ -395,9 +398,9 @@ class Configuration {
       bool? isCsp,
       bool? enableHostAsserts,
       bool? isMinified,
+      GenSnapshotFormat? genSnapshotFormat,
       bool? useAnalyzerCfe,
       bool? useAnalyzerFastaParser,
-      bool? useElf,
       bool? useHotReload,
       bool? useHotReloadRollback,
       bool? useSdk,
@@ -417,9 +420,11 @@ class Configuration {
         isCsp = isCsp ?? false,
         enableHostAsserts = enableHostAsserts ?? false,
         isMinified = isMinified ?? false,
+        // Use a null output for non-precompiler targets.
+        genSnapshotFormat =
+            compiler == Compiler.dartkp ? genSnapshotFormat : null,
         useAnalyzerCfe = useAnalyzerCfe ?? false,
         useAnalyzerFastaParser = useAnalyzerFastaParser ?? false,
-        useElf = useElf ?? false,
         useHotReload = useHotReload ?? false,
         useHotReloadRollback = useHotReloadRollback ?? false,
         useSdk = useSdk ?? false,
@@ -456,9 +461,9 @@ class Configuration {
     required this.isCsp,
     required this.enableHostAsserts,
     required this.isMinified,
+    required this.genSnapshotFormat,
     required this.useAnalyzerCfe,
     required this.useAnalyzerFastaParser,
-    required this.useElf,
     required this.useHotReload,
     required this.useHotReloadRollback,
     required this.useSdk,
@@ -493,9 +498,9 @@ class Configuration {
         isCsp: source.isCsp,
         enableHostAsserts: source.enableHostAsserts,
         isMinified: source.isMinified,
+        genSnapshotFormat: source.genSnapshotFormat,
         useAnalyzerCfe: source.useAnalyzerCfe,
         useAnalyzerFastaParser: source.useAnalyzerFastaParser,
-        useElf: source.useElf,
         useHotReload: source.useHotReload,
         useHotReloadRollback: source.useHotReloadRollback,
         useSdk: source.useSdk,
@@ -531,7 +536,7 @@ class Configuration {
       isMinified == other.isMinified &&
       useAnalyzerCfe == other.useAnalyzerCfe &&
       useAnalyzerFastaParser == other.useAnalyzerFastaParser &&
-      useElf == other.useElf &&
+      genSnapshotFormat == other.genSnapshotFormat &&
       useHotReload == other.useHotReload &&
       useHotReloadRollback == other.useHotReloadRollback &&
       useSdk == other.useSdk &&
@@ -566,6 +571,7 @@ class Configuration {
       mode.hashCode ^
       runtime.hashCode ^
       system.hashCode ^
+      genSnapshotFormat.hashCode ^
       nnbdMode.hashCode ^
       builderTag.hashCode ^
       genKernelOptions.join(" & ").hashCode ^
@@ -583,7 +589,6 @@ class Configuration {
         isMinified,
         useAnalyzerCfe,
         useAnalyzerFastaParser,
-        useElf,
         useHotReload,
         useHotReloadRollback,
         useSdk,
@@ -623,6 +628,11 @@ class Configuration {
     if (isCsp) fields.add("csp");
     if (enableHostAsserts) fields.add("host-asserts");
     if (isMinified) fields.add("minified");
+    // Only output the requested gen_snapshot format if using gen_snapshot
+    // or the output is unexpectedly non-null.
+    if (compiler == Compiler.dartkp || genSnapshotFormat != null) {
+      fields.add("gen-snapshot-format: $genSnapshotFormat");
+    }
     if (useAnalyzerCfe) fields.add("use-cfe");
     if (useAnalyzerFastaParser) fields.add("analyzer-use-fasta-parser");
     if (useHotReload) fields.add("hot-reload");
@@ -681,6 +691,15 @@ class Configuration {
     boolField("csp", isCsp, other.isCsp);
     boolField("host-asserts", enableHostAsserts, other.enableHostAsserts);
     boolField("minified", isMinified, other.isMinified);
+    // Only output the requested gen_snapshot format if using gen_snapshot
+    // or the output is unexpectedly non-null.
+    if (compiler == Compiler.dartkp ||
+        other.compiler == Compiler.dartkp ||
+        genSnapshotFormat != null ||
+        other.genSnapshotFormat != null) {
+      fields.add(
+          "gen-snapshot-format: $genSnapshotFormat ${other.genSnapshotFormat}");
+    }
     boolField("use-cfe", useAnalyzerCfe, other.useAnalyzerCfe);
     boolField("analyzer-use-fasta-parser", useAnalyzerFastaParser,
         other.useAnalyzerFastaParser);
@@ -816,6 +835,33 @@ class Architecture extends NamedEnum {
   }
 }
 
+/// Specifies the output format used by gen_snapshot to create AOT snapshots.
+class GenSnapshotFormat extends NamedEnum {
+  static const assembly = GenSnapshotFormat._('assembly', 'assembly');
+  static const elf = GenSnapshotFormat._('elf', 'elf');
+  static const machODylib = GenSnapshotFormat._('macho-dylib', 'macho');
+
+  static final List<String> names = _all.keys.toList();
+
+  static final _all = Map<String, GenSnapshotFormat>.fromIterable([
+    assembly,
+    elf,
+    machODylib,
+  ], key: (format) => (format as GenSnapshotFormat).name);
+
+  static GenSnapshotFormat find(String name) {
+    var format = _all[name];
+    if (format != null) return format;
+
+    throw ArgumentError('Unknown gen_snapshot format "$name".');
+  }
+
+  String get snapshotType => 'app-aot-$name';
+
+  final String fileOption;
+  const GenSnapshotFormat._(super.name, this.fileOption);
+}
+
 class Compiler extends NamedEnum {
   static const dart2js = Compiler._('dart2js');
   static const dart2analyzer = Compiler._('dart2analyzer');
diff --git a/pkg/smith/test/configuration_test.dart b/pkg/smith/test/configuration_test.dart
@@ -390,7 +390,7 @@ void main() {
         isMinified: true,
         useAnalyzerCfe: true,
         useAnalyzerFastaParser: true,
-        useElf: true,
+        genSnapshotFormat: GenSnapshotFormat.elf,
         useHotReload: true,
         useHotReloadRollback: true,
         useSdk: true,
diff --git a/pkg/test_runner/lib/src/compiler_configuration.dart b/pkg/test_runner/lib/src/compiler_configuration.dart
@@ -884,7 +884,7 @@ class PrecompilerCompilerConfiguration extends CompilerConfiguration
           tempDir, arguments, environmentOverrides));
     }
 
-    if (!_configuration.useElf) {
+    if (_configuration.genSnapshotFormat == GenSnapshotFormat.assembly) {
       commands.add(
           computeAssembleCommand(tempDir, arguments, environmentOverrides));
       if (!_configuration.keepGeneratedFiles) {
@@ -893,7 +893,8 @@ class PrecompilerCompilerConfiguration extends CompilerConfiguration
       }
     }
 
-    if (_configuration.useElf && _isAndroid) {
+    if (_isAndroid &&
+        _configuration.genSnapshotFormat == GenSnapshotFormat.elf) {
       // On Android, run the NDK's "strip" tool with "--strip-unneeded" to copy
       // Flutter's workflow. Skip this step on tests for DWARF (which may get
       // stripped).
@@ -970,17 +971,16 @@ class PrecompilerCompilerConfiguration extends CompilerConfiguration
       }
     }
 
+    var format = _configuration.genSnapshotFormat!;
+    var basename = (format == GenSnapshotFormat.assembly) ? 'out.S' : 'out.aotsnapshot';
+    // Whether or not loading units are used. Mach-O doesn't currently support
+    // this, and this isn't done for assembly output to avoid having to handle
+    // the assembly of multiple assembly output files.
+    var split = format == GenSnapshotFormat.elf;
     var args = [
-      if (_configuration.useElf) ...[
-        "--snapshot-kind=app-aot-elf",
-        "--elf=$tempDir/out.aotsnapshot",
-        // Only splitting with a ELF to avoid having to setup compilation of
-        // multiple assembly files in the test harness.
-        "--loading-unit-manifest=$tempDir/ignored.json",
-      ] else ...[
-        "--snapshot-kind=app-aot-assembly",
-        "--assembly=$tempDir/out.S",
-      ],
+      "--snapshot-kind=${format.snapshotType}",
+      "--${format.fileOption}=$tempDir/$basename",
+      if (split) "--loading-unit-manifest=$tempDir/ignored.json",
       if (_isAndroid && (_isArm || _isArmX64)) ...[
         '--no-sim-use-hardfp',
       ],
diff --git a/pkg/test_runner/lib/src/configuration.dart b/pkg/test_runner/lib/src/configuration.dart
@@ -122,7 +122,7 @@ class TestConfiguration {
   bool get isSimulator => architecture.isSimulator;
   bool get useAnalyzerCfe => configuration.useAnalyzerCfe;
   bool get useAnalyzerFastaParser => configuration.useAnalyzerFastaParser;
-  bool get useElf => configuration.useElf;
+  GenSnapshotFormat? get genSnapshotFormat => configuration.genSnapshotFormat;
   bool get useSdk => configuration.useSdk;
   bool get enableAsserts => configuration.enableAsserts;
   bool get useQemu => configuration.useQemu;
@@ -475,6 +475,11 @@ class TestConfiguration {
       isValid = false;
     }
 
+    if (compiler == Compiler.dartkp && genSnapshotFormat == null) {
+      print("Error: gen_snapshot output format must be specified in AOT mode.");
+      isValid = false;
+    }
+
     if (shard < 1 || shard > shardCount) {
       print("Error: shard index is $shard out of $shardCount shards");
       isValid = false;
diff --git a/pkg/test_runner/lib/src/options.dart b/pkg/test_runner/lib/src/options.dart
@@ -116,6 +116,11 @@ riscv32, riscv64, simriscv32, simriscv64''')
         defaultsTo: Platform.operatingSystem,
         hide: true,
         help: 'The operating system to run tests on.')
+    ..addMultiOption('gen-snapshot-format',
+        allowed: ['all', ...GenSnapshotFormat.names],
+        defaultsTo: [GenSnapshotFormat.assembly.name],
+        hide: true,
+        help: 'The output format used by gen_snapshot.')
     ..addMultiOption('sanitizer',
         allowed: ['all', ...Sanitizer.names],
         defaultsTo: [Sanitizer.none.name],
@@ -165,12 +170,6 @@ test options, specifying how tests should be run.''')
         aliases: ['use_blobs'],
         hide: true,
         help: 'Use mmap instead of shared libraries for precompilation.')
-    ..addFlag(
-      'use-elf',
-      aliases: ['use_elf'],
-      hide: true,
-      help: 'Directly generate an ELF shared libraries for precompilation.',
-    )
     ..addFlag('use-qemu',
         aliases: ['use_qemu'],
         hide: true,
@@ -417,7 +416,7 @@ has been specified on the command line.''')
     'enable-asserts',
     'use-cfe',
     'analyzer-use-fasta-parser',
-    'use-elf',
+    'gen-snapshot-format',
     'use-sdk',
     'hot-reload',
     'hot-reload-rollback',
@@ -612,6 +611,10 @@ has been specified on the command line.''')
     var system = System.find(systemName);
     var runtimes = [...(data["runtime"] as List<String>).map(Runtime.find)];
     var compilers = [...(data["compiler"] as List<String>).map(Compiler.find)];
+    var formats = [
+      ...(data["gen-snapshot-format"] as List<String>)
+          .map(GenSnapshotFormat.find)
+    ];
 
     // Pick default compilers or runtimes if only one or the other is provided.
     if (runtimes.isEmpty) {
@@ -772,37 +775,40 @@ has been specified on the command line.''')
             }
             for (var sanitizerName in sanitizers) {
               var sanitizer = Sanitizer.find(sanitizerName);
-              var timeout = data["timeout"] != null
-                  ? int.parse(data["timeout"] as String)
-                  : null;
-              var configuration = Configuration(
-                  "custom-configuration-${configurationNumber++}",
-                  architecture,
-                  compiler,
-                  mode,
-                  runtime,
-                  system,
-                  nnbdMode: nnbdMode,
-                  sanitizer: sanitizer,
-                  timeout: timeout,
-                  enableAsserts: data['enable-asserts'] as bool,
-                  useAnalyzerCfe: data["use-cfe"] as bool,
-                  useAnalyzerFastaParser:
-                      data["analyzer-use-fasta-parser"] as bool,
-                  useElf: data["use-elf"] as bool,
-                  useSdk: data["use-sdk"] as bool,
-                  useHotReload: data["hot-reload"] as bool,
-                  useHotReloadRollback: data["hot-reload-rollback"] as bool,
-                  enableHostAsserts: data["host-asserts"] as bool,
-                  isCsp: data["csp"] as bool,
-                  isMinified: data["minified"] as bool,
-                  vmOptions: vmOptions,
-                  dart2jsOptions: dart2jsOptions,
-                  ddcOptions: ddcOptions,
-                  experiments: experiments,
-                  builderTag: data["builder-tag"] as String?,
-                  useQemu: data["use-qemu"] as bool);
-              addConfiguration(configuration);
+              // Expand formats.
+              for (final format in formats) {
+                var timeout = data["timeout"] != null
+                    ? int.parse(data["timeout"] as String)
+                    : null;
+                var configuration = Configuration(
+                    "custom-configuration-${configurationNumber++}",
+                    architecture,
+                    compiler,
+                    mode,
+                    runtime,
+                    system,
+                    nnbdMode: nnbdMode,
+                    sanitizer: sanitizer,
+                    timeout: timeout,
+                    enableAsserts: data['enable-asserts'] as bool,
+                    useAnalyzerCfe: data["use-cfe"] as bool,
+                    useAnalyzerFastaParser:
+                        data["analyzer-use-fasta-parser"] as bool,
+                    useSdk: data["use-sdk"] as bool,
+                    useHotReload: data["hot-reload"] as bool,
+                    useHotReloadRollback: data["hot-reload-rollback"] as bool,
+                    enableHostAsserts: data["host-asserts"] as bool,
+                    isCsp: data["csp"] as bool,
+                    isMinified: data["minified"] as bool,
+                    genSnapshotFormat: format,
+                    vmOptions: vmOptions,
+                    dart2jsOptions: dart2jsOptions,
+                    ddcOptions: ddcOptions,
+                    experiments: experiments,
+                    builderTag: data["builder-tag"] as String?,
+                    useQemu: data["use-qemu"] as bool);
+                addConfiguration(configuration);
+              }
             }
           }
         }
diff --git a/pkg/test_runner/lib/src/runtime_configuration.dart b/pkg/test_runner/lib/src/runtime_configuration.dart
@@ -51,13 +51,13 @@ abstract class RuntimeConfiguration {
       case Runtime.dartPrecompiled:
         if (configuration.system == System.android) {
           return DartPrecompiledAdbRuntimeConfiguration(
-            configuration.useElf,
+            configuration.genSnapshotFormat == GenSnapshotFormat.elf,
           );
         } else if (configuration.system == System.fuchsia) {
           return DartkFuchsiaEmulatorRuntimeConfiguration(true);
         }
         return DartPrecompiledRuntimeConfiguration(
-            configuration.useElf,
+            configuration.genSnapshotFormat == GenSnapshotFormat.elf,
           );
     }
     throw "unreachable";
diff --git a/tools/bots/test_matrix.json b/tools/bots/test_matrix.json
