-
Notifications
You must be signed in to change notification settings - Fork 43
Add support for TF-PSA-Crypto test driver #243
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
0d9ae6a
936943b
415201c
d95c135
50bd6aa
ca68ad8
9d7330c
5fecb0e
3b39e68
08b152f
7bada02
c5384fe
a09ed59
e8dcf71
ed9c4a3
9ce24b3
c806e68
450a7c9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,293 @@ | ||||||||||||||
| """Library for building a TF-PSA-Crypto test driver from the built-in driver | ||||||||||||||
| """ | ||||||||||||||
|
|
||||||||||||||
| # Copyright The Mbed TLS Contributors | ||||||||||||||
| # SPDX-License-Identifier: Apache-2.0 OR GPL-2.0-or-later | ||||||||||||||
| # | ||||||||||||||
|
|
||||||||||||||
| import argparse | ||||||||||||||
| import re | ||||||||||||||
| import shutil | ||||||||||||||
| import subprocess | ||||||||||||||
|
|
||||||||||||||
| from fnmatch import fnmatch | ||||||||||||||
| from pathlib import Path | ||||||||||||||
| from typing import Iterable, List, Match, Optional, Set | ||||||||||||||
|
|
||||||||||||||
| def get_parsearg_base() -> argparse.ArgumentParser: | ||||||||||||||
| """ Get base arguments for scripts building a TF-PSA-Crypto test driver """ | ||||||||||||||
| parser = argparse.ArgumentParser(description="""\ | ||||||||||||||
| Clone the built-in driver tree, rewrite header inclusions and prefix | ||||||||||||||
| exposed C identifiers. | ||||||||||||||
| """) | ||||||||||||||
|
|
||||||||||||||
| parser.add_argument("dst_dir", metavar="DST_DIR", | ||||||||||||||
| help="Destination directory (relative to repository root)") | ||||||||||||||
| parser.add_argument("--driver", default="libtestdriver1", metavar="DRIVER", | ||||||||||||||
| help="Test driver name (default: %(default)s).") | ||||||||||||||
| parser.add_argument('--list-vars-for-cmake', nargs="?", \ | ||||||||||||||
| const="__AUTO__", metavar="FILE", | ||||||||||||||
| help=""" | ||||||||||||||
| Generate a file to be included from a CMakeLists.txt and exit. The file | ||||||||||||||
| defines CMake list variables with the script's inputs/outputs files. If | ||||||||||||||
| FILE is omitted, the output name defaults to '<DRIVER>-list-vars.cmake'. | ||||||||||||||
| """) | ||||||||||||||
| return parser | ||||||||||||||
|
|
||||||||||||||
| class TestDriverGenerator: | ||||||||||||||
| """A TF-PSA-Crypto test driver generator""" | ||||||||||||||
| def __init__(self, src_dir: Path, dst_dir: Path, driver: str, \ | ||||||||||||||
| exclude_files: Optional[Iterable[str]] = None) -> None: | ||||||||||||||
| """ | ||||||||||||||
| Initialize a test driver generator. | ||||||||||||||
| Args: | ||||||||||||||
| src_dir (Path): | ||||||||||||||
| Path to the source directory that contains the built-in driver. | ||||||||||||||
| If this path is relative, it should be relative to the repository | ||||||||||||||
| root so that the source paths returned by `write_list_vars_for_cmake` | ||||||||||||||
| are correct. | ||||||||||||||
| The source directory is expected to contain: | ||||||||||||||
| - an `include` directory | ||||||||||||||
| - an `src` directory | ||||||||||||||
| - the `include/` directory contains exactly one subdirectory | ||||||||||||||
| dst_dir (Path): | ||||||||||||||
| Path to the destination directory where the rewritten tree will | ||||||||||||||
| be created. | ||||||||||||||
| driver (str): | ||||||||||||||
| Name of the driver. This is used as a prefix when rewritting | ||||||||||||||
| the tree. | ||||||||||||||
| exclude_files (Optional[Set[str]]): | ||||||||||||||
| Glob patterns for the basename of the files to be excluded from | ||||||||||||||
| the source directory. | ||||||||||||||
| """ | ||||||||||||||
| self.src_dir = src_dir | ||||||||||||||
| self.dst_dir = dst_dir | ||||||||||||||
| self.driver = driver | ||||||||||||||
| self.exclude_files: Iterable[str] = () | ||||||||||||||
| if exclude_files is not None: | ||||||||||||||
| self.exclude_files = exclude_files | ||||||||||||||
|
Comment on lines
+71
to
+73
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. An iterable can be an “active” thing. For example, if you pass a generator then the code in the generator will be executed each time the resulting object iterates over |
||||||||||||||
|
|
||||||||||||||
| if not (src_dir / "include").is_dir(): | ||||||||||||||
| raise RuntimeError(f'"include" directory in {src_dir} not found') | ||||||||||||||
|
|
||||||||||||||
| if not (src_dir / "src").is_dir(): | ||||||||||||||
| raise RuntimeError(f'"src" directory in {src_dir} not found') | ||||||||||||||
|
|
||||||||||||||
| def write_list_vars_for_cmake(self, fname: str) -> None: | ||||||||||||||
| src_relpaths = self.__get_src_code_files() | ||||||||||||||
| with open(self.dst_dir / fname, "w") as f: | ||||||||||||||
| f.write(f"set({self.driver}_input_files " + \ | ||||||||||||||
| "\n".join(str(path) for path in src_relpaths) + ")\n\n") | ||||||||||||||
|
Comment on lines
+84
to
+85
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor: would be nicer with indentation and quoting, something like |
||||||||||||||
| f.write(f"set({self.driver}_files " + \ | ||||||||||||||
| "\n".join(str(self.__get_dst_relpath(path.relative_to(self.src_dir))) \ | ||||||||||||||
| for path in src_relpaths) + ")\n\n") | ||||||||||||||
| f.write(f"set({self.driver}_src_files " + \ | ||||||||||||||
| "\n".join(str(self.__get_dst_relpath(path.relative_to(self.src_dir))) \ | ||||||||||||||
| for path in src_relpaths if path.suffix == ".c") + ")\n") | ||||||||||||||
|
|
||||||||||||||
| def get_identifiers_to_prefix(self, prefixes: Set[str]) -> Set[str]: | ||||||||||||||
| """ | ||||||||||||||
| Get the set of identifiers that will be prefixed in the test driver code. | ||||||||||||||
| This method is intended to be amended by subclasses in consuming branches. | ||||||||||||||
| The default implementation returns the complete set of identifiers from | ||||||||||||||
| the built-in driver whose names begin with any of the `prefixes`. These | ||||||||||||||
| are the identifiers that could be renamed in the test driver before | ||||||||||||||
| adaptation. | ||||||||||||||
| Subclasses need to filter, transform, or otherwise adjust the set of | ||||||||||||||
| identifiers that should be renamed when generating the test driver. | ||||||||||||||
| Args: | ||||||||||||||
| prefixes (Set[str]): | ||||||||||||||
| The set of identifier prefixes used by the built-in driver | ||||||||||||||
| for the symbols it exposes to the other parts of the crypto | ||||||||||||||
| library. All identifiers beginning with any of these | ||||||||||||||
| prefixes are candidates for renaming in the test driver to | ||||||||||||||
| avoid symbol clashes. | ||||||||||||||
| Returns: | ||||||||||||||
| Set[str]: The default set of identifiers to rename. | ||||||||||||||
| """ | ||||||||||||||
| identifiers = set() | ||||||||||||||
| for file in self.__get_src_code_files(): | ||||||||||||||
| identifiers.update(self.get_c_identifiers(file)) | ||||||||||||||
|
|
||||||||||||||
| identifiers_with_prefixes = set() | ||||||||||||||
| for identifier in identifiers: | ||||||||||||||
| if any(identifier.startswith(prefix) for prefix in prefixes): | ||||||||||||||
| identifiers_with_prefixes.add(identifier) | ||||||||||||||
| return identifiers_with_prefixes | ||||||||||||||
|
|
||||||||||||||
| def create_test_driver_tree(self, prefixes: Set[str]) -> None: | ||||||||||||||
| """ | ||||||||||||||
| Build a test driver tree from `self.src_dir` into `self.dst_dir`. | ||||||||||||||
| Only the `include/` and `src/` subdirectories of the source tree are | ||||||||||||||
| used, and their internal directory structure is preserved. | ||||||||||||||
| Only "*.h" and "*.c" files are copied. Files whose basenames match any | ||||||||||||||
| of the glob patterns in `self.exclude_files` are excluded. | ||||||||||||||
| The basename of all files is prefixed with `{self.driver}-`. The | ||||||||||||||
| header inclusions referencing the renamed headers are rewritten | ||||||||||||||
| accordingly. | ||||||||||||||
| Symbol identifiers exposed by the built-in driver are renamed by | ||||||||||||||
| prefixing them with `{self.driver}_` to avoid collisions when linking the | ||||||||||||||
| built-in driver and the test driver together in the crypto library. | ||||||||||||||
| Args: | ||||||||||||||
| prefixes (Set[str]): | ||||||||||||||
| The set of identifier prefixes used by the built-in driver | ||||||||||||||
| for the symbols it exposes to the other parts of the crypto | ||||||||||||||
| library. All identifiers beginning with any of these | ||||||||||||||
| prefixes are candidates for renaming in the test driver to | ||||||||||||||
| avoid symbol clashes. | ||||||||||||||
| """ | ||||||||||||||
| if (self.dst_dir / "include").exists(): | ||||||||||||||
| shutil.rmtree(self.dst_dir / "include") | ||||||||||||||
|
|
||||||||||||||
| if (self.dst_dir / "src").exists(): | ||||||||||||||
| shutil.rmtree(self.dst_dir / "src") | ||||||||||||||
|
|
||||||||||||||
| headers = { | ||||||||||||||
| f.name \ | ||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's an edge case: suppose that (for example) both The code currently just changes all places that include any header whose basename is |
||||||||||||||
| for f in self.__get_src_code_files() if f.suffix == ".h" | ||||||||||||||
| } | ||||||||||||||
| identifiers_to_prefix = self.get_identifiers_to_prefix(prefixes) | ||||||||||||||
|
|
||||||||||||||
| # Create the test driver tree | ||||||||||||||
| for file in self.__get_src_code_files(): | ||||||||||||||
| dst = self.dst_dir / \ | ||||||||||||||
| self.__get_dst_relpath(file.relative_to(self.src_dir)) | ||||||||||||||
| dst.parent.mkdir(parents=True, exist_ok=True) | ||||||||||||||
| self.__write_test_driver_file(file, dst, headers,\ | ||||||||||||||
| identifiers_to_prefix, self.driver) | ||||||||||||||
|
|
||||||||||||||
| @staticmethod | ||||||||||||||
| def __get_code_files(root: Path) -> List[Path]: | ||||||||||||||
| """ | ||||||||||||||
| Return all "*.c" and "*.h" files found recursively under the | ||||||||||||||
| `include` and `src` subdirectories of `root`. | ||||||||||||||
| """ | ||||||||||||||
| return sorted(path | ||||||||||||||
| for directory in ('include', 'src') | ||||||||||||||
| for path in (root / directory).rglob('*.[hc]')) | ||||||||||||||
|
|
||||||||||||||
| def __get_src_code_files(self) -> List[Path]: | ||||||||||||||
| """ | ||||||||||||||
| Return all "*.c" and "*.h" files found recursively under the | ||||||||||||||
| `include` and `src` subdirectories of the source directory `self.src_dir` | ||||||||||||||
| excluding the files whose basename match any of the patterns in | ||||||||||||||
| `self.exclude_files`. | ||||||||||||||
| """ | ||||||||||||||
| out = [] | ||||||||||||||
| for file in self.__get_code_files(self.src_dir): | ||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is not failing but it it tricky. While it is not in any critical path, we shouldn't invoking to staticmethods using self. but rather
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Calling |
||||||||||||||
| if not any(fnmatch(file.name, pattern) for pattern in self.exclude_files): | ||||||||||||||
| out.append(file) | ||||||||||||||
| return out | ||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not a blocker: Any reasons we don't do
Comment on lines
+191
to
+195
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor: I think this would be more readable as a filter pattern: (or maybe invert the logic with a function |
||||||||||||||
|
|
||||||||||||||
| def __get_dst_relpath(self, src_relpath: Path) -> Path: | ||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not a blocker but this could also be a staticmethod, and |
||||||||||||||
| """ | ||||||||||||||
| Return the path relative to `dst_dir` of the file that corresponds to the | ||||||||||||||
| file with relative path `src_relpath` in the source tree. | ||||||||||||||
| Same as `src_relpath` but the basename prefixed with `self.driver` | ||||||||||||||
| """ | ||||||||||||||
| assert not src_relpath.is_absolute(), "src_relpath must be relative" | ||||||||||||||
|
|
||||||||||||||
| parts = src_relpath.parts | ||||||||||||||
| if len(parts) > 1: | ||||||||||||||
| return Path(*parts[:-1], f"{self.driver}-{parts[-1]}") | ||||||||||||||
| else: | ||||||||||||||
| return Path(f"{self.driver}-{parts[-1]}") | ||||||||||||||
|
Comment on lines
+206
to
+210
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor: why not simply
Suggested change
|
||||||||||||||
|
|
||||||||||||||
| @staticmethod | ||||||||||||||
| def get_c_identifiers(file: Path) -> Set[str]: | ||||||||||||||
| """ | ||||||||||||||
| Extract the C identifiers in `file` using ctags. | ||||||||||||||
| Identifiers of the following types are returned (with their corresponding | ||||||||||||||
| ctags c-kinds flag in parentheses): | ||||||||||||||
| - macro definitions (d) | ||||||||||||||
| - enum values (e) | ||||||||||||||
| - functions (f) | ||||||||||||||
| - enum tags (g) | ||||||||||||||
| - function prototypes (p) | ||||||||||||||
| - struct tags (s) | ||||||||||||||
| - typedefs (t) | ||||||||||||||
| - union tags (u) | ||||||||||||||
| - global variables (v) | ||||||||||||||
| This method requires `Universal Ctags`. The command used here has been | ||||||||||||||
| validated with Universal Ctags 5.9.0 (the default `ctags` on Ubuntu | ||||||||||||||
| 22.04 and 24.04). `Exuberant Ctags` is not compatible: it does not | ||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How was that tested? should we do a quick check that ctags is what we expect it to be? :
Given that this script will be called on most build, it will be hard for users to discover that depedenency hidden in a comment unless we print something back to them |
||||||||||||||
| support all `--c-kinds` flags used here, and will either fail with an | ||||||||||||||
| error or produce incomplete results. | ||||||||||||||
|
Comment on lines
+230
to
+234
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's the problem with Exuberant? I tried running tests/scripts/generate_test_driver.py in Mbed-TLS/TF-PSA-Crypto#586 as of 2b70a5e35e5b5b92aa24295cfbaf166db61160e8 and got identical results with Exuberant Ctags 5.8 (compiled manually), Exuberant Ctags 5.9~svn20110310 (Ubuntu 24.04), Universal Ctags 5.9.0 (Ubuntu 24.04) and Universal Ctags 6.2.0 (Ubuntu 26.04). BSD and Emacs ctags don't work, but they fail to parse the command line so you get an error trace showing that the ctags invocation failed. If it's possible to get incomplete results, the script should check the ctags implementation and error out. Possibly by calling ctags-universal, which is the command for Universal Ctags on Debian and derivatives, but not on other platforms (e.g. Brew on macOS just installs ctags and you can only have one of Exuberant or Universal, and you might have to fiddle with $PATH to put Brew ahead of the system ctags which would error out on invocation). |
||||||||||||||
| """ | ||||||||||||||
| output = subprocess.check_output( | ||||||||||||||
| ["ctags", "-x", "--language-force=C", "--c-kinds=defgpstuv", str(file)], | ||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add ctags to |
||||||||||||||
| stderr=subprocess.STDOUT, | ||||||||||||||
| universal_newlines=True, | ||||||||||||||
| ) | ||||||||||||||
| identifiers = set() | ||||||||||||||
| for line in output.splitlines(): | ||||||||||||||
| identifiers.add(line.split()[0]) | ||||||||||||||
|
|
||||||||||||||
| return identifiers | ||||||||||||||
|
|
||||||||||||||
| @staticmethod | ||||||||||||||
| def __write_test_driver_file(src: Path, dst: Path, | ||||||||||||||
| headers: Set[str], | ||||||||||||||
| identifiers_to_prefix: Set[str], | ||||||||||||||
| driver: str) -> None: | ||||||||||||||
| """ | ||||||||||||||
| Rewrite a test driver file: | ||||||||||||||
| 1) Rewrite `#include` directives that include one of the header in | ||||||||||||||
| headers. The basename of the header is prefixed by `{driver}-`. | ||||||||||||||
| For example: | ||||||||||||||
| #include "mbedtls/private/aes.h" | ||||||||||||||
| becomes: | ||||||||||||||
| #include "mbedtls/private/libtestdriver1-aes.h" | ||||||||||||||
| 2) Prefix each identifier in `identifiers` with the uppercase | ||||||||||||||
| form of `driver` if the identifier is uppercase, or with the lowercase | ||||||||||||||
| form of `driver` otherwise. | ||||||||||||||
| """ | ||||||||||||||
| text = src.read_text(encoding="utf-8") | ||||||||||||||
|
|
||||||||||||||
| include_line_re = re.compile( | ||||||||||||||
| fr'^\s*#\s*include\s*([<"])([^>"]+)([>"])', | ||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor: it would be nicer and simple to preserve the spacing.
Suggested change
and adapt the replacement accordingly. |
||||||||||||||
| re.MULTILINE | ||||||||||||||
| ) | ||||||||||||||
| def repl_header_inclusion(m: Match) -> str: | ||||||||||||||
| parts = m.group(2).split("/") | ||||||||||||||
| if parts[-1] in headers: | ||||||||||||||
| path = "/".join(parts[:-1] + [driver + "-" + parts[-1]]) | ||||||||||||||
| return f'#include {m.group(1)}{path}{m.group(3)}' | ||||||||||||||
| return m.group(0) | ||||||||||||||
| intermediate_text = include_line_re.sub(repl_header_inclusion, text) | ||||||||||||||
|
|
||||||||||||||
| c_identifier_re = re.compile(r"\b[A-Za-z_][A-Za-z0-9_]*\b") | ||||||||||||||
| prefix_uppercased = driver.upper() | ||||||||||||||
| prefix_lowercased = driver.lower() | ||||||||||||||
|
|
||||||||||||||
| def repl(m: Match) -> str: | ||||||||||||||
| identifier = m.group(0) | ||||||||||||||
| if identifier in identifiers_to_prefix: | ||||||||||||||
| if identifier[0].isupper(): | ||||||||||||||
| return f"{prefix_uppercased}_{identifier}" | ||||||||||||||
| else: | ||||||||||||||
| return f"{prefix_lowercased}_{identifier}" | ||||||||||||||
| return identifier | ||||||||||||||
|
|
||||||||||||||
| new_text = c_identifier_re.sub(repl, intermediate_text) | ||||||||||||||
| dst.write_text(new_text, encoding="utf-8") | ||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.