From 8eeb4a5c4b84ce46e6f2d0b75b4b25b5724f5d39 Mon Sep 17 00:00:00 2001 From: Tim Hutt Date: Tue, 5 Nov 2024 13:53:11 +0000 Subject: [PATCH] Enable Pylint in CI and fix its errors The main fixes were: * Specify encoding for all file opens. By default it depends on environment variables which is bad. * Use `with` to open files. Otherwise they don't necessarily get closed. There were also a few minor things like using `enumerate`, not using objects as default arguments, etc. In some cases I slightly refactored the code. --- .pre-commit-config.yaml | 9 +-- .pylintrc | 31 ++++++++ c_utils.py | 4 +- chisel_utils.py | 24 +++--- constants.py | 26 ++++--- go_utils.py | 9 +-- latex_utils.py | 163 ++++++++++++++++++++-------------------- parse.py | 2 +- pyrightconfig.json | 2 +- rust_utils.py | 9 +-- shared_utils.py | 57 +++++++------- sverilog_utils.py | 8 +- 12 files changed, 191 insertions(+), 153 deletions(-) create mode 100644 .pylintrc diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index bd41c169..dc25bc10 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -21,11 +21,10 @@ repos: hooks: - id: black - # TODO: Enable this when lints are fixed. - # - repo: https://github.com/PyCQA/pylint - # rev: v3.3.1 - # hooks: - # - id: pylint + - repo: https://github.com/PyCQA/pylint + rev: v3.3.1 + hooks: + - id: pylint - repo: https://github.com/RobertCraigie/pyright-python rev: v1.1.383 diff --git a/.pylintrc b/.pylintrc new file mode 100644 index 00000000..33ae3c30 --- /dev/null +++ b/.pylintrc @@ -0,0 +1,31 @@ +[MAIN] +py-version = 3.9.0 +disable= + # Allow 'TODO:' in code. + fixme, + # Overly zealous duplicate code detection. + duplicate-code, + # These debatable style lints are quite annoying, and often push + # you into mixing up small changes (adding one statement to a function) + # with large refactors (splitting the function up into shorter functions). + too-few-public-methods, + too-many-arguments, + too-many-positional-arguments, + too-many-branches, + too-many-instance-attributes, + too-many-locals, + too-many-return-statements, + too-many-statements, + # Handled by Black. + line-too-long, + # This is technically correct but not that important. + logging-fstring-interpolation, + # TODO: These should be enabled but writing documentation for + # all of the code is not feasible in one go. + missing-module-docstring, + missing-function-docstring, + missing-class-docstring, + +# These names are fine when used sensibly. Without listing them here +# Pylint will complain they are too short. +good-names=c,i,j,k,id,pc diff --git a/c_utils.py b/c_utils.py index 4454d504..8aa5138b 100644 --- a/c_utils.py +++ b/c_utils.py @@ -43,7 +43,7 @@ def make_c(instr_dict: InstrDict): mask = ((1 << (end - begin + 1)) - 1) << begin arg_str += f"#define INSN_FIELD_{sanitized_name.upper()} {hex(mask)}\n" - with open(f"{os.path.dirname(__file__)}/encoding.h", "r") as file: + with open(f"{os.path.dirname(__file__)}/encoding.h", "r", encoding="utf-8") as file: enc_header = file.read() commit = os.popen('git log -1 --format="format:%h"').read() @@ -75,5 +75,5 @@ def make_c(instr_dict: InstrDict): """ # Write the modified output to the file - with open("encoding.out.h", "w") as enc_file: + with open("encoding.out.h", "w", encoding="utf-8") as enc_file: enc_file.write(output_str) diff --git a/chisel_utils.py b/chisel_utils.py index fbe6806c..34f6ac3f 100644 --- a/chisel_utils.py +++ b/chisel_utils.py @@ -21,7 +21,6 @@ def make_chisel(instr_dict: InstrDict, spinal_hdl: bool = False): if not spinal_hdl: extensions = instr_dict_2_extensions(instr_dict) for e in extensions: - e_instrs = filter(lambda i: instr_dict[i]["extension"][0] == e, instr_dict) if "rv64_" in e: e_format = e.replace("rv64_", "").upper() + "64" elif "rv32_" in e: @@ -31,10 +30,11 @@ def make_chisel(instr_dict: InstrDict, spinal_hdl: bool = False): else: e_format = e.upper() chisel_names += f' val {e_format+"Type"} = Map(\n' - for instr in e_instrs: - tmp_instr_name = '"' + instr.upper().replace(".", "_") + '"' - chisel_names += f' {tmp_instr_name:<18s} -> BitPat("b{instr_dict[instr]["encoding"].replace("-","?")}"),\n' - chisel_names += f" )\n" + for instr_name, instr in instr_dict.items(): + if instr["extension"][0] == e: + tmp_instr_name = '"' + instr_name.upper().replace(".", "_") + '"' + chisel_names += f' {tmp_instr_name:<18s} -> BitPat("b{instr["encoding"].replace("-","?")}"),\n' + chisel_names += " )\n" for num, name in causes: cause_names_str += f' val {name.lower().replace(" ","_")} = {hex(num)}\n' @@ -63,12 +63,11 @@ def make_chisel(instr_dict: InstrDict, spinal_hdl: bool = False): csr_names_str += """ res.toArray }""" - if spinal_hdl: - chisel_file = open("inst.spinalhdl", "w") - else: - chisel_file = open("inst.chisel", "w") - chisel_file.write( - f""" + with open( + "inst.spinalhdl" if spinal_hdl else "inst.chisel", "w", encoding="utf-8" + ) as chisel_file: + chisel_file.write( + f""" /* Automatically generated by parse_opcodes */ object Instructions {{ {chisel_names} @@ -80,5 +79,4 @@ def make_chisel(instr_dict: InstrDict, spinal_hdl: bool = False): {csr_names_str} }} """ - ) - chisel_file.close() + ) diff --git a/constants.py b/constants.py index 4b608e35..fa59aa7f 100644 --- a/constants.py +++ b/constants.py @@ -44,7 +44,7 @@ ) -def read_csv(filename: str): +def read_int_map_csv(filename: str) -> "list[tuple[int, str]]": """ Reads a CSV file and returns a list of tuples. Each tuple contains an integer value (from the first column) and a string (from the second column). @@ -55,20 +55,26 @@ def read_csv(filename: str): Returns: list of tuple: A list of (int, str) tuples extracted from the CSV file. """ - with open(filename) as f: + with open(filename, encoding="utf-8") as f: csv_reader = csv.reader(f, skipinitialspace=True) return [(int(row[0], 0), row[1]) for row in csv_reader] -causes = read_csv("causes.csv") -csrs = read_csv("csrs.csv") -csrs32 = read_csv("csrs32.csv") +causes = read_int_map_csv("causes.csv") +csrs = read_int_map_csv("csrs.csv") +csrs32 = read_int_map_csv("csrs32.csv") -# Load the argument lookup table (arg_lut) from a CSV file, mapping argument names to their bit positions -arg_lut = { - row[0]: (int(row[1]), int(row[2])) - for row in csv.reader(open("arg_lut.csv"), skipinitialspace=True) -} + +def read_arg_lut_csv(filename: str) -> "dict[str, tuple[int, int]]": + """ + Load the argument lookup table (arg_lut) from a CSV file, mapping argument names to their bit positions. + """ + with open(filename, encoding="utf-8") as f: + csv_reader = csv.reader(f, skipinitialspace=True) + return {row[0]: (int(row[1]), int(row[2])) for row in csv_reader} + + +arg_lut = read_arg_lut_csv("arg_lut.csv") # for mop arg_lut["mop_r_t_30"] = (30, 30) diff --git a/go_utils.py b/go_utils.py index 9bdc64db..5c1fcbf3 100644 --- a/go_utils.py +++ b/go_utils.py @@ -1,5 +1,6 @@ import logging import pprint +import subprocess import sys from shared_utils import InstrDict, signed @@ -49,14 +50,12 @@ def make_go(instr_dict: InstrDict): return &inst{{ {hex(opcode)}, {hex(funct3)}, {hex(rs1)}, {hex(rs2)}, {signed(csr,12)}, {hex(funct7)} }} """ - with open("inst.go", "w") as file: + with open("inst.go", "w", encoding="utf-8") as file: file.write(prelude) file.write(instr_str) file.write(endoffile) try: - import subprocess - - subprocess.run(["go", "fmt", "inst.go"]) - except: + subprocess.run(["go", "fmt", "inst.go"], check=True) + except: # pylint: disable=bare-except pass diff --git a/latex_utils.py b/latex_utils.py index 3d9a0098..080a1e37 100644 --- a/latex_utils.py +++ b/latex_utils.py @@ -10,7 +10,6 @@ def make_priv_latex_table(): - latex_file = open("priv-instr-table.tex", "w") type_list = ["R-type", "I-type"] system_instr = ["_h", "_s", "_system", "_svinval", "64_h", "_svinval_h"] dataset_list = [(system_instr, "Trap-Return Instructions", ["sret", "mret"], False)] @@ -75,9 +74,8 @@ def make_priv_latex_table(): ) ) caption = "\\caption{RISC-V Privileged Instructions}" - make_ext_latex_table(type_list, dataset_list, latex_file, 32, caption) - - latex_file.close() + with open("priv-instr-table.tex", "w", encoding="utf-8") as latex_file: + make_ext_latex_table(type_list, dataset_list, latex_file, 32, caption) def make_latex_table(): @@ -101,84 +99,89 @@ def make_latex_table(): riscv-isa-manual. """ # open the file and use it as a pointer for all further dumps - latex_file = open("instr-table.tex", "w") - - # create the rv32i table first. Here we set the caption to empty. We use the - # files rv_i and rv32_i to capture instructions relevant for rv32i - # configuration. The dataset is a list of 4-element tuples : - # (list_of_extensions, title, list_of_instructions, include_pseudo_ops). If list_of_instructions - # is empty then it indicates that all instructions of the all the extensions - # in list_of_extensions need to be dumped. If not empty, then only the - # instructions listed in list_of_instructions will be dumped into latex. - caption = "" - type_list = ["R-type", "I-type", "S-type", "B-type", "U-type", "J-type"] - dataset_list: list[tuple[list[str], str, list[str], bool]] = [ - (["_i", "32_i"], "RV32I Base Instruction Set", [], False) - ] - dataset_list.append((["_i"], "", ["fence_tso", "pause"], True)) - make_ext_latex_table(type_list, dataset_list, latex_file, 32, caption) - - type_list = ["R-type", "I-type", "S-type"] - dataset_list = [ - (["64_i"], "RV64I Base Instruction Set (in addition to RV32I)", [], False) - ] - dataset_list.append( - (["_zifencei"], "RV32/RV64 Zifencei Standard Extension", [], False) - ) - dataset_list.append((["_zicsr"], "RV32/RV64 Zicsr Standard Extension", [], False)) - dataset_list.append((["_m", "32_m"], "RV32M Standard Extension", [], False)) - dataset_list.append( - (["64_m"], "RV64M Standard Extension (in addition to RV32M)", [], False) - ) - make_ext_latex_table(type_list, dataset_list, latex_file, 32, caption) - - type_list = ["R-type"] - dataset_list = [(["_a"], "RV32A Standard Extension", [], False)] - dataset_list.append( - (["64_a"], "RV64A Standard Extension (in addition to RV32A)", [], False) - ) - make_ext_latex_table(type_list, dataset_list, latex_file, 32, caption) - - type_list = ["R-type", "R4-type", "I-type", "S-type"] - dataset_list = [(["_f"], "RV32F Standard Extension", [], False)] - dataset_list.append( - (["64_f"], "RV64F Standard Extension (in addition to RV32F)", [], False) - ) - make_ext_latex_table(type_list, dataset_list, latex_file, 32, caption) + with open("instr-table.tex", "w", encoding="utf-8") as latex_file: + + # create the rv32i table first. Here we set the caption to empty. We use the + # files rv_i and rv32_i to capture instructions relevant for rv32i + # configuration. The dataset is a list of 4-element tuples : + # (list_of_extensions, title, list_of_instructions, include_pseudo_ops). If list_of_instructions + # is empty then it indicates that all instructions of the all the extensions + # in list_of_extensions need to be dumped. If not empty, then only the + # instructions listed in list_of_instructions will be dumped into latex. + caption = "" + type_list = ["R-type", "I-type", "S-type", "B-type", "U-type", "J-type"] + dataset_list: list[tuple[list[str], str, list[str], bool]] = [ + (["_i", "32_i"], "RV32I Base Instruction Set", [], False) + ] + dataset_list.append((["_i"], "", ["fence_tso", "pause"], True)) + make_ext_latex_table(type_list, dataset_list, latex_file, 32, caption) + + type_list = ["R-type", "I-type", "S-type"] + dataset_list = [ + (["64_i"], "RV64I Base Instruction Set (in addition to RV32I)", [], False) + ] + dataset_list.append( + (["_zifencei"], "RV32/RV64 Zifencei Standard Extension", [], False) + ) + dataset_list.append( + (["_zicsr"], "RV32/RV64 Zicsr Standard Extension", [], False) + ) + dataset_list.append((["_m", "32_m"], "RV32M Standard Extension", [], False)) + dataset_list.append( + (["64_m"], "RV64M Standard Extension (in addition to RV32M)", [], False) + ) + make_ext_latex_table(type_list, dataset_list, latex_file, 32, caption) - type_list = ["R-type", "R4-type", "I-type", "S-type"] - dataset_list = [(["_d"], "RV32D Standard Extension", [], False)] - dataset_list.append( - (["64_d"], "RV64D Standard Extension (in addition to RV32D)", [], False) - ) - make_ext_latex_table(type_list, dataset_list, latex_file, 32, caption) + type_list = ["R-type"] + dataset_list = [(["_a"], "RV32A Standard Extension", [], False)] + dataset_list.append( + (["64_a"], "RV64A Standard Extension (in addition to RV32A)", [], False) + ) + make_ext_latex_table(type_list, dataset_list, latex_file, 32, caption) - type_list = ["R-type", "R4-type", "I-type", "S-type"] - dataset_list = [(["_q"], "RV32Q Standard Extension", [], False)] - dataset_list.append( - (["64_q"], "RV64Q Standard Extension (in addition to RV32Q)", [], False) - ) - make_ext_latex_table(type_list, dataset_list, latex_file, 32, caption) + type_list = ["R-type", "R4-type", "I-type", "S-type"] + dataset_list = [(["_f"], "RV32F Standard Extension", [], False)] + dataset_list.append( + (["64_f"], "RV64F Standard Extension (in addition to RV32F)", [], False) + ) + make_ext_latex_table(type_list, dataset_list, latex_file, 32, caption) - caption = "\\caption{Instruction listing for RISC-V}" - type_list = ["R-type", "R4-type", "I-type", "S-type"] - dataset_list = [ - (["_zfh", "_d_zfh", "_q_zfh"], "RV32Zfh Standard Extension", [], False) - ] - dataset_list.append( - (["64_zfh"], "RV64Zfh Standard Extension (in addition to RV32Zfh)", [], False) - ) - make_ext_latex_table(type_list, dataset_list, latex_file, 32, caption) + type_list = ["R-type", "R4-type", "I-type", "S-type"] + dataset_list = [(["_d"], "RV32D Standard Extension", [], False)] + dataset_list.append( + (["64_d"], "RV64D Standard Extension (in addition to RV32D)", [], False) + ) + make_ext_latex_table(type_list, dataset_list, latex_file, 32, caption) - ## The following is demo to show that Compressed instructions can also be - # dumped in the same manner as above + type_list = ["R-type", "R4-type", "I-type", "S-type"] + dataset_list = [(["_q"], "RV32Q Standard Extension", [], False)] + dataset_list.append( + (["64_q"], "RV64Q Standard Extension (in addition to RV32Q)", [], False) + ) + make_ext_latex_table(type_list, dataset_list, latex_file, 32, caption) + + caption = "\\caption{Instruction listing for RISC-V}" + type_list = ["R-type", "R4-type", "I-type", "S-type"] + dataset_list = [ + (["_zfh", "_d_zfh", "_q_zfh"], "RV32Zfh Standard Extension", [], False) + ] + dataset_list.append( + ( + ["64_zfh"], + "RV64Zfh Standard Extension (in addition to RV32Zfh)", + [], + False, + ) + ) + make_ext_latex_table(type_list, dataset_list, latex_file, 32, caption) - # type_list = [''] - # dataset_list = [(['_c', '32_c', '32_c_f','_c_d'],'RV32C Standard Extension', [])] - # dataset_list.append((['64_c'],'RV64C Standard Extension (in addition to RV32C)', [])) - # make_ext_latex_table(type_list, dataset_list, latex_file, 16, caption) + ## The following is demo to show that Compressed instructions can also be + # dumped in the same manner as above - latex_file.close() + # type_list = [''] + # dataset_list = [(['_c', '32_c', '32_c_f','_c_d'],'RV32C Standard Extension', [])] + # dataset_list.append((['64_c'],'RV64C Standard Extension (in addition to RV32C)', [])) + # make_ext_latex_table(type_list, dataset_list, latex_file, 16, caption) def make_ext_latex_table( @@ -319,8 +322,7 @@ def make_ext_latex_table( # for each argument/string of 1s or 0s, create a multicolumn latex table # entry entry = "" - for r in range(len(fields)): - (msb, lsb, name) = fields[r] + for r, (msb, lsb, name) in enumerate(fields): if r == len(fields) - 1: entry += ( f"\\multicolumn{{{msb - lsb + 1}}}{{|c|}}{{{name}}} & {t} \\\\\n" @@ -379,7 +381,7 @@ def make_ext_latex_table( encoding = instr_dict[inst]["encoding"] for r in range(0, ilen): x = encoding[r] - if ((msb, ilen - 1 - r + 1)) in latex_fixed_fields: + if (msb, ilen - 1 - r + 1) in latex_fixed_fields: fields.append((msb, ilen - 1 - r + 1, y)) msb = ilen - 1 - r y = "" @@ -397,8 +399,7 @@ def make_ext_latex_table( fields.sort(key=lambda y: y[0], reverse=True) entry = "" - for r in range(len(fields)): - (msb, lsb, name) = fields[r] + for r, (msb, lsb, name) in enumerate(fields): if r == len(fields) - 1: entry += f'\\multicolumn{{{msb - lsb + 1}}}{{|c|}}{{{name}}} & {inst.upper().replace("_",".")} \\\\\n' elif r == 0: diff --git a/parse.py b/parse.py index 4339e81d..5704877c 100755 --- a/parse.py +++ b/parse.py @@ -44,7 +44,7 @@ def main(): instr_dict = create_inst_dict(extensions, include_pseudo) - with open("instr_dict.json", "w") as outfile: + with open("instr_dict.json", "w", encoding="utf-8") as outfile: json.dump(add_segmented_vls_insn(instr_dict), outfile, indent=2) instr_dict = collections.OrderedDict(sorted(instr_dict.items())) diff --git a/pyrightconfig.json b/pyrightconfig.json index 67b99de7..79a6cbf0 100644 --- a/pyrightconfig.json +++ b/pyrightconfig.json @@ -1,4 +1,4 @@ { "typeCheckingMode": "strict", - "pythonVersion": "3.6", + "pythonVersion": "3.9", } diff --git a/rust_utils.py b/rust_utils.py index d2df1537..bc9153bb 100644 --- a/rust_utils.py +++ b/rust_utils.py @@ -19,11 +19,10 @@ def make_rust(instr_dict: InstrDict): mask_match_str += ( f'const CAUSE_{name.upper().replace(" ","_")}: u8 = {hex(num)};\n' ) - rust_file = open("inst.rs", "w") - rust_file.write( - f""" + with open("inst.rs", "w", encoding="utf-8") as rust_file: + rust_file.write( + f""" /* Automatically generated by parse_opcodes */ {mask_match_str} """ - ) - rust_file.close() + ) diff --git a/shared_utils.py b/shared_utils.py index 9e66e729..4b6917cb 100644 --- a/shared_utils.py +++ b/shared_utils.py @@ -6,7 +6,7 @@ import pprint import re from itertools import chain -from typing import Dict, TypedDict +from typing import Dict, Optional, TypedDict from constants import ( arg_lut, @@ -397,7 +397,7 @@ def create_expanded_instruction( # Return a list of relevant lines from the specified file def read_lines(file: str) -> "list[str]": """Reads lines from a file and returns non-blank, non-comment lines.""" - with open(file) as fp: + with open(file, encoding="utf-8") as fp: lines = (line.rstrip() for line in fp) return [line for line in lines if line and not line.startswith("#")] @@ -487,22 +487,23 @@ def process_imported_instructions( continue logging.debug(f"Processing imported line: {line}") import_ext, reg_instr = imported_regex.findall(line)[0] - ext_file = find_extension_file(import_ext, opcodes_dir) - - validate_instruction_in_extension(reg_instr, ext_file, file_name, line) - - for oline in open(ext_file): - if re.findall(f"^\\s*{reg_instr}\\s+", oline): - name, single_dict = process_enc_line(oline, file_name) - if name in instr_dict: - if instr_dict[name]["encoding"] != single_dict["encoding"]: - log_and_exit( - f"Imported instruction {name} from {os.path.basename(file_name)} has different encodings" - ) - instr_dict[name]["extension"].extend(single_dict["extension"]) - else: - instr_dict[name] = single_dict - break + ext_filename = find_extension_file(import_ext, opcodes_dir) + + validate_instruction_in_extension(reg_instr, ext_filename, file_name, line) + + with open(ext_filename, encoding="utf-8") as ext_file: + for oline in ext_file: + if re.findall(f"^\\s*{reg_instr}\\s+", oline): + name, single_dict = process_enc_line(oline, file_name) + if name in instr_dict: + if instr_dict[name]["encoding"] != single_dict["encoding"]: + log_and_exit( + f"Imported instruction {name} from {os.path.basename(file_name)} has different encodings" + ) + instr_dict[name]["extension"].extend(single_dict["extension"]) + else: + instr_dict[name] = single_dict + break # Locate the path of the specified extension file, checking fallback directories @@ -518,14 +519,15 @@ def find_extension_file(ext: str, opcodes_dir: str): # Confirm the presence of an original instruction in the corresponding extension file. def validate_instruction_in_extension( - inst: str, ext_file: str, file_name: str, pseudo_inst: str + inst: str, ext_filename: str, file_name: str, pseudo_inst: str ): """Validates if the original instruction exists in the dependent extension.""" found = False - for oline in open(ext_file): - if re.findall(f"^\\s*{inst}\\s+", oline): - found = True - break + with open(ext_filename, encoding="utf-8") as ext_file: + for oline in ext_file: + if re.findall(f"^\\s*{inst}\\s+", oline): + found = True + break if not found: log_and_exit( f"Original instruction {inst} required by pseudo_op {pseudo_inst} in {file_name} not found in {ext_file}" @@ -536,11 +538,11 @@ def validate_instruction_in_extension( def create_inst_dict( file_filter: "list[str]", include_pseudo: bool = False, - include_pseudo_ops: "list[str]" = [], + include_pseudo_ops: "Optional[list[str]]" = None, ) -> InstrDict: - """Creates a dictionary of instructions based on the provided file filters.""" - """ + Creates a dictionary of instructions based on the provided file filters. + This function return a dictionary containing all instructions associated with an extension defined by the file_filter input. Allowed input extensions: needs to be rv* file name without the 'rv' prefix i.e. '_i', '32_i', etc. @@ -569,6 +571,9 @@ def create_inst_dict( - Adds the pseudo_op to the dictionary if the dependent instruction is not already present; otherwise, it is skipped. """ + if include_pseudo_ops is None: + include_pseudo_ops = [] + opcodes_dir = os.path.dirname(os.path.realpath(__file__)) instr_dict: InstrDict = {} diff --git a/sverilog_utils.py b/sverilog_utils.py index e32ad71c..d2b26b6e 100644 --- a/sverilog_utils.py +++ b/sverilog_utils.py @@ -1,5 +1,6 @@ import logging import pprint +from pathlib import Path from constants import csrs, csrs32 from shared_utils import InstrDict @@ -18,13 +19,12 @@ def make_sverilog(instr_dict: InstrDict): f" localparam logic [11:0] CSR_{name.upper()} = 12'h{hex(num)[2:]};\n" ) - sverilog_file = open("inst.sverilog", "w") - sverilog_file.write( + Path("inst.sverilog").write_text( f""" /* Automatically generated by parse_opcodes */ package riscv_instr; {names_str} endpackage -""" +""", + encoding="utf-8", ) - sverilog_file.close()