From 8e2dc38f9f156b83b85810a5db5cf8d30053e053 Mon Sep 17 00:00:00 2001 From: Bruce Forstall Date: Fri, 11 Dec 2020 15:13:41 -0800 Subject: [PATCH] Fix superpmi.py argument parsing 1. Hoist out argument parsing/verification of target_os/target_arch/mch_arch so they apply to more commands that require them, e.g., "list-collections". 2. Rename `-jit` argument to `-jit_name` to be more explicit, and differentiate it from the pre-existing `-jit_path` argument. 3. Hoist verification of `-jit_ee_version` to reduce code duplication. Fixes #45981 --- src/coreclr/scripts/superpmi.py | 158 ++++++++++++++------------------ 1 file changed, 71 insertions(+), 87 deletions(-) diff --git a/src/coreclr/scripts/superpmi.py b/src/coreclr/scripts/superpmi.py index ad1e05c3bdd4d..f3c58b606fb5f 100755 --- a/src/coreclr/scripts/superpmi.py +++ b/src/coreclr/scripts/superpmi.py @@ -214,7 +214,15 @@ core_root_parser.add_argument("-log_file", help=log_file_help) core_root_parser.add_argument("-spmi_location", help=spmi_location_help) -# Create a set of argument common to all commands that run SuperPMI. +# Create a set of arguments common to target specification. Used for replay, upload, download, list-collections. + +target_parser = argparse.ArgumentParser(add_help=False) + +target_parser.add_argument("-target_arch", help=target_arch_help) +target_parser.add_argument("-target_os", help=target_os_help) +target_parser.add_argument("-mch_arch", help=mch_arch_help) + +# Create a set of arguments common to all commands that run SuperPMI. superpmi_common_parser = argparse.ArgumentParser(add_help=False) @@ -223,14 +231,11 @@ superpmi_common_parser.add_argument("--skip_cleanup", action="store_true", help=skip_cleanup_help) superpmi_common_parser.add_argument("--sequential", action="store_true", help="Run SuperPMI in sequential mode. Default is to run in parallel for faster runs.") superpmi_common_parser.add_argument("-spmi_log_file", help=spmi_log_file_help) -superpmi_common_parser.add_argument("-jit", help="Specify the filename of the jit to use, e.g., 'clrjit_win_arm64_x64.dll'. Default is clrjit.dll/libclrjit.so") +superpmi_common_parser.add_argument("-jit_name", help="Specify the filename of the jit to use, e.g., 'clrjit_win_arm64_x64.dll'. Default is clrjit.dll/libclrjit.so") superpmi_common_parser.add_argument("--altjit", action="store_true", help="Set the altjit variables on replay.") -superpmi_common_parser.add_argument("-target_arch", help=target_arch_help) -superpmi_common_parser.add_argument("-target_os", help=target_os_help) -superpmi_common_parser.add_argument("-mch_arch", help=mch_arch_help) # subparser for collect -collect_parser = subparsers.add_parser("collect", description=collect_description, parents=[core_root_parser, superpmi_common_parser]) +collect_parser = subparsers.add_parser("collect", description=collect_description, parents=[core_root_parser, target_parser, superpmi_common_parser]) # Add required arguments collect_parser.add_argument("collection_command", nargs='?', help=superpmi_collect_help) @@ -251,9 +256,9 @@ collect_parser.add_argument("--skip_clean_and_verify_step", action="store_true", help="Do not run the collection cleaning, TOC creation, and verifying step.") collect_parser.add_argument("--skip_collect_mc_files", action="store_true", help="Do not collect .MC files") -# Create a set of argument common to all SuperPMI replay commands, namely basic replay and ASM diffs. +# Create a set of arguments common to all SuperPMI replay commands, namely basic replay and ASM diffs. # Note that SuperPMI collection also runs a replay to verify the final MCH file, so many arguments -# common to replay are also applicable that that replay as well. +# common to replay are also applicable to that replay as well. replay_common_parser = argparse.ArgumentParser(add_help=False) @@ -264,13 +269,13 @@ replay_common_parser.add_argument("-jit_ee_version", help=jit_ee_version_help) # subparser for replay -replay_parser = subparsers.add_parser("replay", description=replay_description, parents=[core_root_parser, superpmi_common_parser, replay_common_parser]) +replay_parser = subparsers.add_parser("replay", description=replay_description, parents=[core_root_parser, target_parser, superpmi_common_parser, replay_common_parser]) # Add required arguments replay_parser.add_argument("-jit_path", help="Path to clrjit. Defaults to Core_Root JIT.") -# subparser for asmDiffs -asm_diff_parser = subparsers.add_parser("asmdiffs", description=asm_diff_description, parents=[core_root_parser, superpmi_common_parser, replay_common_parser]) +# subparser for asmdiffs +asm_diff_parser = subparsers.add_parser("asmdiffs", description=asm_diff_description, parents=[core_root_parser, target_parser, superpmi_common_parser, replay_common_parser]) # Add required arguments asm_diff_parser.add_argument("-base_jit_path", help="Path to baseline clrjit. Defaults to baseline JIT from rolling build, by computing baseline git hash.") @@ -285,7 +290,7 @@ asm_diff_parser.add_argument("-temp_dir", help="Specify a temporary directory used for a previous ASM diffs run (for which --skip_cleanup was used) to view the results. The replay command is skipped.") # subparser for upload -upload_parser = subparsers.add_parser("upload", description=upload_description, parents=[core_root_parser]) +upload_parser = subparsers.add_parser("upload", description=upload_description, parents=[core_root_parser, target_parser]) upload_parser.add_argument("-mch_files", metavar="MCH_FILE", required=True, nargs='+', help=upload_mch_files_help) upload_parser.add_argument("-az_storage_key", help="Key for the clrjit Azure Storage location. Default: use the value of the CLRJIT_AZ_KEY environment variable.") @@ -294,7 +299,7 @@ upload_parser.add_argument("--skip_cleanup", action="store_true", help=skip_cleanup_help) # subparser for download -download_parser = subparsers.add_parser("download", description=download_description, parents=[core_root_parser]) +download_parser = subparsers.add_parser("download", description=download_description, parents=[core_root_parser, target_parser]) download_parser.add_argument("-filter", nargs='+', help=filter_help) download_parser.add_argument("-jit_ee_version", help=jit_ee_version_help) @@ -303,7 +308,7 @@ download_parser.add_argument("-mch_files", metavar="MCH_FILE", nargs='+', help=replay_mch_files_help) # subparser for list-collections -list_collections_parser = subparsers.add_parser("list-collections", description=list_collections_description, parents=[core_root_parser]) +list_collections_parser = subparsers.add_parser("list-collections", description=list_collections_description, parents=[core_root_parser, target_parser]) list_collections_parser.add_argument("-jit_ee_version", help=jit_ee_version_help) list_collections_parser.add_argument("--all", action="store_true", help="Show all MCH files, not just those for the specified (or default) JIT-EE version, OS, and architecture") @@ -1707,20 +1712,20 @@ def determine_pmi_location(coreclr_args): def determine_jit_name(coreclr_args): - """ Determine the jit based on the OS. If "-jit" is specified, then use the specified jit. - This function is called for cases where the "-jit" flag is not used, so be careful not - to depend on the "jit" attribute existing. + """ Determine the jit based on the OS. If "-jit_name" is specified, then use the specified jit. + This function is called for cases where the "-jit_name" flag is not used, so be careful not + to depend on the "jit_name" attribute existing. Args: coreclr_args (CoreclrArguments): parsed args Return: - jit_name(str) : name of the jit for this OS + (str) : name of the jit for this OS """ - # If `-jit` is used, it must be given a full filename, not just a "base name", so use it without additional processing. - if hasattr(coreclr_args, "jit") and coreclr_args.jit is not None: - return coreclr_args.jit + # If `-jit_name` is used, it must be given a full filename, not just a "base name", so use it without additional processing. + if hasattr(coreclr_args, "jit_name") and coreclr_args.jit_name is not None: + return coreclr_args.jit_name jit_base_name = "clrjit" if coreclr_args.host_os == "OSX": @@ -2647,6 +2652,34 @@ def setup_jit_path_arg(jit_path): return os.path.abspath(jit_path) return find_tool(coreclr_args, determine_jit_name(coreclr_args), search_path=False) # It doesn't make sense to search PATH for the JIT dll. + def verify_jit_ee_version_arg(): + + coreclr_args.verify(args, + "jit_ee_version", + lambda unused: True, + "Invalid JIT-EE Version.", + modify_arg=setup_jit_ee_version_arg) + + def verify_target_args(): + + coreclr_args.verify(args, + "target_os", + lambda target_os: check_target_os(coreclr_args, target_os), + lambda target_os: "Unknown target_os {}\nSupported OS: {}".format(target_os, (", ".join(coreclr_args.valid_host_os))), + modify_arg=lambda target_os: target_os if target_os is not None else coreclr_args.host_os) # Default to `host_os` + + coreclr_args.verify(args, + "target_arch", + lambda target_arch: check_target_arch(coreclr_args, target_arch), + lambda target_arch: "Unknown target_arch {}\nSupported architectures: {}".format(target_arch, (", ".join(coreclr_args.valid_arches))), + modify_arg=lambda target_arch: target_arch if target_arch is not None else coreclr_args.arch) # Default to `arch` + + coreclr_args.verify(args, + "mch_arch", + lambda mch_arch: check_mch_arch(coreclr_args, mch_arch), + lambda mch_arch: "Unknown mch_arch {}\nSupported architectures: {}".format(mch_arch, (", ".join(coreclr_args.valid_arches))), + modify_arg=lambda mch_arch: mch_arch if mch_arch is not None else coreclr_args.target_arch) # Default to `target_arch` + def verify_superpmi_common_args(): coreclr_args.verify(args, @@ -2680,45 +2713,23 @@ def verify_superpmi_common_args(): def verify_replay_common_args(): + verify_jit_ee_version_arg() + coreclr_args.verify(args, "force_download", lambda unused: True, "Unable to set force_download") coreclr_args.verify(args, - "jit", + "jit_name", lambda unused: True, - "Unable to set jit.") + "Unable to set jit_name.") coreclr_args.verify(args, "altjit", # Must be set before `jit_path` (determine_jit_name() depends on it) lambda unused: True, "Unable to set altjit.") - coreclr_args.verify(args, - "target_os", - lambda target_os: check_target_os(coreclr_args, target_os), - lambda target_os: "Unknown target_os {}\nSupported OS: {}".format(target_os, (", ".join(coreclr_args.valid_host_os))), - modify_arg=lambda target_os: target_os if target_os is not None else coreclr_args.host_os) # Default to `host_os` - - coreclr_args.verify(args, - "target_arch", - lambda target_arch: check_target_arch(coreclr_args, target_arch), - lambda target_arch: "Unknown target_arch {}\nSupported architectures: {}".format(target_arch, (", ".join(coreclr_args.valid_arches))), - modify_arg=lambda target_arch: target_arch if target_arch is not None else coreclr_args.arch) # Default to `arch` - - coreclr_args.verify(args, - "mch_arch", - lambda mch_arch: check_mch_arch(coreclr_args, mch_arch), - lambda mch_arch: "Unknown mch_arch {}\nSupported architectures: {}".format(mch_arch, (", ".join(coreclr_args.valid_arches))), - modify_arg=lambda mch_arch: mch_arch if mch_arch is not None else coreclr_args.target_arch) # Default to `target_arch` - - coreclr_args.verify(args, - "jit_ee_version", - lambda unused: True, - "Invalid JIT-EE Version.", - modify_arg=setup_jit_ee_version_arg) - coreclr_args.verify(args, "filter", lambda unused: True, @@ -2731,39 +2742,19 @@ def verify_replay_common_args(): if coreclr_args.mode == "collect": + verify_target_args() verify_superpmi_common_args() coreclr_args.verify(args, - "jit", # The replay code checks this, so make sure it's set + "jit_name", # The replay code checks this, so make sure it's set lambda unused: True, - "Unable to set jit.") + "Unable to set jit_name.") coreclr_args.verify(args, "altjit", # The replay code checks this, so make sure it's set lambda unused: True, "Unable to set altjit.") - # For collection replays, target_os is expected to just default to host_os - coreclr_args.verify(args, - "target_os", - lambda target_os: check_target_os(coreclr_args, target_os), - lambda target_os: "Unknown target_os {}\nSupported OS: {}".format(target_os, (", ".join(coreclr_args.valid_host_os))), - modify_arg=lambda target_os: target_os if target_os is not None else coreclr_args.host_os) # Default to `host_os` - - # For collection replays, target_arch is expected to just default to the host arch. - coreclr_args.verify(args, - "target_arch", - lambda target_arch: check_target_arch(coreclr_args, target_arch), - lambda target_arch: "Unknown target_arch {}\nSupported architectures: {}".format(target_arch, (", ".join(coreclr_args.valid_arches))), - modify_arg=lambda target_arch: target_arch if target_arch is not None else coreclr_args.arch) # Default to `arch` - - # For collection replays, mch_arch is expected to just default to the host arch. - coreclr_args.verify(args, - "mch_arch", - lambda mch_arch: check_mch_arch(coreclr_args, mch_arch), - lambda mch_arch: "Unknown mch_arch {}\nSupported architectures: {}".format(mch_arch, (", ".join(coreclr_args.valid_arches))), - modify_arg=lambda mch_arch: mch_arch if mch_arch is not None else coreclr_args.target_arch) # Default to `target_arch` - coreclr_args.verify(args, "collection_command", lambda command: command is None or os.path.isfile(command), @@ -2848,6 +2839,7 @@ def verify_replay_common_args(): elif coreclr_args.mode == "replay": + verify_target_args() verify_superpmi_common_args() verify_replay_common_args() @@ -2898,6 +2890,7 @@ def verify_replay_common_args(): elif coreclr_args.mode == "asmdiffs": + verify_target_args() verify_superpmi_common_args() verify_replay_common_args() @@ -3010,6 +3003,9 @@ def verify_replay_common_args(): elif coreclr_args.mode == "upload": + verify_target_args() + verify_jit_ee_version_arg() + coreclr_args.verify(args, "az_storage_key", lambda item: item is not None, @@ -3021,12 +3017,6 @@ def verify_replay_common_args(): lambda unused: True, "Unable to set jit_location.") - coreclr_args.verify(args, - "jit_ee_version", - lambda unused: True, - "Invalid JIT-EE Version.", - modify_arg=setup_jit_ee_version_arg) - coreclr_args.verify(args, "mch_files", lambda unused: True, @@ -3034,17 +3024,14 @@ def verify_replay_common_args(): elif coreclr_args.mode == "download": + verify_target_args() + verify_jit_ee_version_arg() + coreclr_args.verify(args, "force_download", lambda unused: True, "Unable to set force_download") - coreclr_args.verify(args, - "jit_ee_version", - lambda unused: True, - "Invalid JIT-EE Version.", - modify_arg=setup_jit_ee_version_arg) - coreclr_args.verify(args, "filter", lambda unused: True, @@ -3057,11 +3044,8 @@ def verify_replay_common_args(): elif coreclr_args.mode == "list-collections": - coreclr_args.verify(args, - "jit_ee_version", - lambda unused: True, - "Invalid JIT-EE Version.", - modify_arg=setup_jit_ee_version_arg) + verify_target_args() + verify_jit_ee_version_arg() coreclr_args.verify(args, "all",