Code Size: Add the TFM Medium Profile config files and the option to track code size changes #7426
Conversation
aditya-deshpande-arm
force-pushed
the
code-size-tracking
branch
from
1c53fbf to
5ba96bd
Compare
|
@adeaarm please confirm you're happy for us to update our copies of the three files updated by |
would it be possible to have the relicensing only in its own PR? |
Sure, I'll remove the re-licensing commit from this PR and open a separate PR as soon as this one is merged. |
aditya-deshpande-arm
force-pushed
the
code-size-tracking
branch
4 times, most recently
from
7132f5c to
dc057df
Compare
aditya-deshpande-arm
added
needs-review
aditya-deshpande-arm
force-pushed
the
code-size-tracking
branch
from
dc057df to
0073566
Compare
|
@aditya-deshpande-arm Is this ready for review? CI still reports a coding style error. It looks easy to fix. |
|
@yanrayw I have already begun my review because I believe it is ready but it would be nice to get the CI to go green to be sure |
|
@aditya-deshpande-arm |
There was a problem hiding this comment.
Generally speaking this PR does look good. The python is nice and it does work; I have tested it and used it. I have requested changes for the following reasons (note I have left comments on most of these things but will summarise here):
- When bringing the configs across I see a slightly different version of
tfm_mbedcrypto_config_profile_medium.hwhich does not include the config optionMBEDTLS_PSA_CRYPTO_KEY_ID_ENCODE_OWNER. I would like to know if this has been added in manually and if so it should probably be mentioned why. - Several commit messages contain the word "also" which shows the commit is doing more than one thing. We try to stick to granular commits that do one thing, and one thing only, where possible. I think these commits should be broken up.
- In the larger commits, for example the 3rd commit, if it is not possible or practical to split them up a bit more then a more detailed commit message would be appreciated.
- Commits that apply corrections in response to CI failures should really have those changes applied in their original commits as this leads to a cleaner commit history.
- There are some small gramatical nits (see comments) that are not a big deal on their own but would make sense to clean them up since other changes are being made.
Please write on here for any clarifications and I will help you get this over the line.
| @@ -290,6 +303,11 @@ def main(): | |||
| help="new revision for comparison, default is the current work \ | |||
| directory, including uncommitted changes." | |||
| ) | |||
| parser.add_argument( | |||
| "-c", "--config", type=str, default="default", | |||
| help="optional configuration for Mbed TLS. Default uses current \ | |||
There was a problem hiding this comment.
nit: Capital "O" on optional.
There was a problem hiding this comment.
The existing help messages in the file start with lowercase letters too so I've just replicated that style for consistency. If we change this to a capital O then we should change the messages for all options..
| @@ -262,13 +266,15 @@ def _gen_code_size_csv(self, revision, git_worktree_path): | |||
| sizes_dict = self._gen_code_size_report(revision, git_worktree_path) | |||
|
|
|||
| def write_dict_to_csv(d): | |||
| for (f,s) in d.items(): | |||
| csv_file.write(f'{f}, {s.text}, {s.data}, {s.bss}, {s.total}\n') | |||
| for (f, s) in d.items(): | |||
There was a problem hiding this comment.
Since the reviews had not started at the time these changes were made, it would be better to fix them via an interactive rebase so that the commit history is cleaner, i.e. fixing this in the original commit rather than applying a commit at the end which fixes the problem.
| x509_text = result.decode() | ||
| # Size for libmbedtls.a | ||
| try: | ||
| result = subprocess.check_output( | ||
| ["size -t library/libmbedtls.a"], cwd=git_worktree_path, shell=True | ||
| ) | ||
| except subprocess.CalledProcessError as e: | ||
| self._handle_CalledProcessError(e,git_worktree_path) | ||
| self._handle_called_process_error(e, git_worktree_path) |
There was a problem hiding this comment.
Fixing pylint should ideally be done in a separate commit. This commit was primarily about removing f-strings so really it should only do this one thing.
As is the case with my comment around the f-strings, it would be better to fix the pylint related errors in their original commit, via an interactive rebase, rather than applying a fix-up commit to the end since this will generate a cleaner git history.
There was a problem hiding this comment.
Yes, ideally that would be in a separate commit, but at this point, let's not use Aditya's limited time rewriting the commit history
|
I looked into this - it looks like in between me cloning TFM and your review this option has been removed from the config. I'll rebase and take the latest config from TFM. |
The TF-M configs are at https://git.trustedfirmware.org/TF-M/trusted-firmware-m.git/tree/lib/ext/mbedcrypto/mbedcrypto_config . All of them enable |
aditya-deshpande-arm
added
needs-work
and removed
needs-review
| * you wish to support. | ||
| * | ||
| * Note that this option is meant for internal use only and may be removed | ||
| * without notice. |
There was a problem hiding this comment.
Maybe make a comment here that TF-M does it in CMakeLists.txt?
There was a problem hiding this comment.
Just to add some more on this point, the problem in TF-M is that we want to use the same config file without duplicating the file between the client view of the API (NS or other partitions) and the server view of the API (the crypto service and the crypto_platform_keys components). For this reason we have removed that define from the config so that the file can be shared, but we enforce it on the crypto service and crypto_platform_keys components via a CMakelists.txt option. This is basically the consequence of TF-M keeping a different view of the key encodings between clients and server, i.e. the service.
There was a problem hiding this comment.
I've added a comment here explaining that TF-M enables this option using CMake instead of in the config. I've also pointed to this discussion for reference.
aditya-deshpande-arm
force-pushed
the
code-size-tracking
branch
3 times, most recently
from
800dc05 to
67b1258
Compare
tom-daubney-arm
added
needs-review
yanrayw
added
the
needs-review
daverodgman
added
needs-reviewer
There was a problem hiding this comment.
Thanks for the changes @yanrayw. I have checked all the different options for the script and they are working now. I think this is ready to go.
LGTM.
|
I've just tested this on #7578 with |
That's interesting, I have just seen this error myself. I was not using this script but just compiling something for TF-M on another PR I am reviewing. I am wondering if the TF-M configs that were brought across don't work for all cases because I was running a near idential command to what the script produces, and this error appeared for me. I can chat with @yanrayw and see if we can get to the bottom of this. |
|
@yanrayw in order to get around the error that @daverodgman reported, I had to turn off the following config options from This is a bit confusing because I did not encounter this error when I was playing around with testing the PR earlier today, but I did get this same build error when testing another PR which I was building with the TF-M configs. Have you got any ideas as to why this might be happening? I can chat with you about this tomorrow. |
|
@tom-daubney-arm The problem is that chains of dependencies between |
Thank you, @gilles-peskine-arm! |
|
Look into the build error, I think this error could be fixed by tweaking the location which we include |
Thanks @yanrayw. I think these are both reasonable options. Can I ask @daverodgman to weigh in since Tom C is away at the moment. Which solution do you want to see? |
davidhorstmann-arm
requested review from
davidhorstmann-arm
and removed request for
tom-cosgrove-arm
|
Know Limitations:
Those limitations will be added in a follow-up PR. (It's also up to what kind of code size measurement script we want) We can add extra functionalities if needed. |
|
Following up on a test I have done which is around point the script at commits which are not in its current branch. I confirm that the script cannot handle this. My steps to preproduce were as follows:
So we are seeing an error here around not finding the config file. |
| return 'make -j lib' | ||
|
|
||
| if self.config == 'default': | ||
| if self.arch == 'aarch32': |
There was a problem hiding this comment.
Nit: aarch32 is an execution state of Armv8-A, whereas this is compiling for M-class (M33). Changing aarch32 to armv8-m would correct this.
| self.text = text | ||
| self.data = data | ||
| self.bss = bss | ||
| self.total = total |
There was a problem hiding this comment.
Non-blocker: Is total just text + data + bss? If so, why not calculate it on the fly by replacing it with a method so that we can never have an inconsistent object?
Or does alignment/padding mean that total might be different from the sum of the other 3?
| 'make -j lib CC=armclang \ | ||
| CFLAGS=\'--target=arm-arm-none-eabi -mcpu=cortex-m33 -Os \ | ||
| -DMBEDTLS_CONFIG_FILE=\\\"../configs/tfm_mbedcrypto_config_profile_medium.h\\\" \ | ||
| -DMBEDTLS_PSA_CRYPTO_CONFIG_FILE=\\\"../configs/crypto_config_profile_medium.h\\\" \' ' |
There was a problem hiding this comment.
Nit: Indentation here is a bit funky.
|
|
||
| def _gen_code_size_csv(self, revision, git_worktree_path): | ||
| """Generate code size csv file.""" | ||
|
|
||
| csv_fname = revision + ".csv" | ||
| csv_fname = revision + "-" + self.config + ".csv" |
There was a problem hiding this comment.
Should we use self.arch here to avoid a name clash if used on the same revisions over different architectures?
| # Tell the user what went wrong | ||
| print("The following command: {} failed and exited with code {}"\ | ||
| .format(e.cmd, e.returncode)) | ||
| print("Process output:\n {}".format(e.output)) |
There was a problem hiding this comment.
Formatting nit: The output is a bytes b'xyz' object so is printed as this not as a string. This means that for example newlines are escaped not actually printed. This should be converted to a string before outputting to make it easier to read.
| print("Process output:\n {}".format(e.output)) | |
| print("Process output:\n {}".format(str(e.output, "utf-8"))) |
|
Thanks @tom-daubney-arm for providing feedback. In #7426 (comment), there are logs about To compare two commits by this script, firstly it creates a worktree for that commit. The issue here is, regarding 399fa48, we haven't created |
Thanks @yanrayw. I agree with this analysis here; I thought this is what was happening. I personally think it is ok to have this as a limitation of the script. In my understanding, the main use case of this script is for measuing the impact that a new piece of work has (whether that is adding or removing code from the lib) on the code size of the library. In order to do this, a developer would make their measurement after adding some commits and then call the script, pointing it at the current HEAD and the last commit that existed when they started their work. The script in its current form does indeed handle this use case without error and as such I think it can be merged (subject to the review comments from @davidhorstmann-arm being addressed). I know there are some additional features that might be nice to have in the script, such as architecture detection etc, however I feel like these could be brought in with a follow up PR. What do people think? @daverodgman in particular as I know you have some issues you want followed up on but also @yanrayw, @davidhorstmann-arm |
|
This PR is not in a good shape. I'll split this PR into smaller pieces and upload smaller PRs. |
|
This PR is superseded by #7650 and its follow-up PRs. |
Description
This PR:
configs/.code_size_compare.pyso that it separates the sizes ofcrypto,x509, andtlsin the csv report and allows for the use of alternative configurations (like the TFM Baremetal one).Should resolve #7360. Will also lay the foundations for addressing #7361 and #7359.
Gatekeeper checklist