Report on structure fields #5234
Conversation
Report the size, alignment and offset of fields of structures defined in Mbed TLS headers. Usage example on Cortex-M0: ``` cd mbedtls/struct-optimization scripts/config.py baremetal ../types-report/scripts/types_report.py -t armv6m-unknown-eabi -I ~/Packages/ARMCompiler6.6/include ``` Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
This script is focusing on fields. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
This script is focusing on fields. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Report how many places in the code access each field. Known bug: the code in this commit doesn't match type names at the point of declaration with type names at the point of use properly. I think the problem is that field definitions are recorded against the name of the struct when it has one, but field uses are recorded against the typedef. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
The source code said this was done, but the code did the opposite. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
The previous code did not correctly associate typedefs with structs when the two had different names. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
This is a lot easier for testing. Production usage is now ``` scripts/field_report.py include/*/*.h library/*.[hc] ``` Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
No behavior change. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Don't skip structure definitions that have no name, but for which there is a named typedef. Do skip unions and anonymous structs that don't have a type name (which is common for structs embedded in another struct or union). Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
The score is the access cost on Cortex-M0+ thumb builds, expressed in instructions. This commit does not have any provisions to calculate different scores based on the target architecture, so it's only meaningful when running this script with `-t armv6m-unknown-eabi`. Go with a simple model: direct access is possible for the first 128 fields of a structure, direct access costs 1 instruction, indirect access costs 3 instructions. The score calculation assumes that each lexical use in the source code corresponds to one access. This does not take into account unused code, inlining, uses from application code, etc. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
get_canonical() sees through typedefs, but it doesn't dereference pointers. So we need to continue digging after reaching a canonical type. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Remove the remaining Mbed TLS-specific bits of field_report.py. Create a wrapper script that calls field_report.py with options for Mbed TLS specifically. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Add a sanity check to detect the most common problem, which is a missing include directory leading to compilation errors (which this script is unable to report due to a limitation of the clang python bindings). Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
gilles-peskine-arm
added
needs-review
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
|
This looks good. I did find it a bit difficult to get a working python3 clang library, and ended up using a docker image of Ubuntu. The pypi package seems to like to reference a not-quite-correct spelling of libclang.so. Regarding this, I tested with all of the versions of python3-clang available on Ubuntu 21.10. python3-clang-9, python3-clang-11, python3-clang-12 all worked, however it gets a compilation error with python3-clang-13. Probably a useful addition (if reasonable to do) would be to figure out if there is a way to print out compilation errors. |
I think the problem is that the clang bindings need to be made for a specific clang version, and installing it with pip doesn't necessarily figure it out. If you have multiple versions of clang installed, you may need to pass
Unfortunately it seems to be a long-standing defect of the clang python bindings that they suppress errors. I haven't found a solution. |
|
|
||
| def run_analysis(self, files: List[str], | ||
| sanity_checks: bool = True) -> None: | ||
| self.read_field_definitions(files) |
There was a problem hiding this comment.
Missing method description comment/docstring.
| self.index = clang.cindex.Index.create() | ||
| self.files = {} #type: Dict[str, TranslationUnit] | ||
| # fields[TYPE_OR_STRUCT_NAME][FIELD_NAME] | ||
| self.fields = {} #type: Dict[str, Dict[str, FieldInfo]] |
There was a problem hiding this comment.
The way this data is accessed makes it feel like it should be a DataClass() ref
We have the data stored, and could contain methods to perfom the following function
- Getter method to return dictionary sorted by field_name or by entry order.
- Boolean method to check for SanityCheck
- An iterative function to printout the data like report_field, pprint or str , repr overrides.
There was a problem hiding this comment.
dataclasses was added in Python 3.7. We still officially support 3.6 and run CI on 3.5.
| if not hasattr(location.file, 'name'): | ||
| # Some artificial nodes have associated no file name. | ||
| # Let's hope they're not important. | ||
| return False |
There was a problem hiding this comment.
Given that we are not sure about the files that miss this attribute, it could be worth it to capture the list somewhere. Or add an optional verbosity level on the parser that would print them to the terminal.
There was a problem hiding this comment.
When a node has no location, it's difficult to analyze what it's for. I don't think we could print useful information in this log.
| """ | ||
| if hasattr(typ, 'get_canonical'): | ||
| lower = typ.get_canonical() | ||
| if lower != typ: |
There was a problem hiding this comment.
if lower == typ is that meant to return None? I wonder if we could get away with a try/except approach.
There was a problem hiding this comment.
The following cases cover lower == typ with a return other than None. I don't know what exception you're thinking of catching here.
The data structures I'm traversing here are not documented. I arrived at this code by trial and error.
gilles-peskine-arm
added
needs-work
and removed
needs-review
| import sys | ||
| from typing import Dict, List, Optional | ||
|
|
||
| import clang.cindex #type: ignore |
There was a problem hiding this comment.
To merge this PR, we need to install the clang python bindings on the CI. This may be more difficult than just adding clang to requirements.txt because I think the version of the python bindings has to match the version of libclang.
daverodgman
added
the
priority-low
tom-daubney-arm
added
the
historical-reviewed
|
As part of our review of historical PRs we have made the decision to convert older PRs that have not been updated in 3 months into drafts until they are worked on again. |
Report the size and offsets of structure fields. This is informative for deciding how to order them in order to reduce code size:
p[n]is cheaper whenn==0, i.e. it helps if the first field is the one that's accessed the most often.p[n]is cheaper whenn < 128, where the offset can be in units of 1, 2 or 4 bytes. So byte fields at an offset <128, 16-bit fields at an offset <256 and 32-bit or 64-bit fields at an offset <512 are cheaper.Status: this script is usable, but not great. Code size depends a lot of factors. Even focusing on the thumb 128-element window, blindly minimizing the out-of-bounds fields isn't always optimal. Some known limitations of the current version:
p->s.xis likely to be compiled to a single indirection, but my script only knowsoffsetof(p_t, s)andoffsetof(s_t, x), not the offset ofxinp.p->xonly ever appears in astatic inline int get_x(foo_t *p) { return p->x; }, this may be used in hundreds of locations but the script countsxas being used only once.Due to these limitations, I think the right way to use this script is for a human to review its output and fiddle with field order in likely places. I don't think the numbers are useful enough for any kind of automated analysis. Hence I do not think this script should run on the CI. I do think it's good to have it in the code base so we can use it when designing new structures or adding fields to already large structures.
The script can easily run on a different worktree, so there's no point in backporting it.
The script takes about a minute to run on my machine. I may have done something grossly inefficient somewhere.
I used this script to find likely candidates for optimization in #5189.