Hi Rasmus,
On 2026-05-22T21:27:48, Rasmus Villemoes <[email protected]> wrote:
> tools: add linker-lists.py parser/checker script
>
> Add a python script which will make use of the special symbols emitted
> by the linker list macros, and perform various sanity checks. By
> default, it ends with printing a list of all the defined linker lists,
> including their start/end addresses, the size and aligment of
> individual items and the number of items.
>
> With --check, it only does the sanity checking and its exit code
> reflects whether any problems were found. That is eventually intended
> to be done as part of the build.
>
> With --dump, it not only prints the lists and their overall
> properties, but also the names/addresses of each item belong to the
> list.
>
> Signed-off-by: Rasmus Villemoes <[email protected]>
>
> tools/linker-lists.py | 234 ++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 234 insertions(+)
Perhaps check-linker-lists.py ? BTW I did write a checker script which
works just with the existing linker lists, but I don't think it is as
powerful as the mechanism you have here. I'll send it as an RFC just
for comparison. That is how I found the alignment problems.
Please use single quotes by default for strings (unless you need a
single quote in the string, iwc the outer would should be double
quotes)
> diff --git a/tools/linker-lists.py b/tools/linker-lists.py
> new file mode 100755
> index 00000000000..419879ee31a
> --- /dev/null
> +++ b/tools/linker-lists.py
> @@ -0,0 +1,234 @@
> +#!/usr/bin/python3
> +# SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +
> +import sys
> +from argparse import ArgumentParser
> +import subprocess
Please switch to '#!/usr/bin/env python3' as the absolute path is not
portable. Also please add a short module docstring
> diff --git a/tools/linker-lists.py b/tools/linker-lists.py
> @@ -0,0 +1,234 @@
> +class unique_dict(dict):
> + def __init__(self, name, format_spec, *arg, **kw):
> + super(unique_dict, self).__init__(*arg, **kw)
Can this just be super().__init__(...) ?
> + self.name = name
> + self.format_spec = format_spec
> +
> + def __setitem__(self, key, value):
> + if key in self:
> + old = self[key]
> + if value != old:
> + warn(key,
> + f"Inconsistent {self.name} for list '{key}': Old value
> {self.format_spec}, new value {self.format_spec}" %
> + (old, value))
> + return
> + super(unique_dict, self).__setitem__(key, value)
Mixing an f-string with trailing %-formatting is hard to follow - how
about something like::
warn(key, f"Inconsistent {self.name} for list '{key}': "
f"Old value {old:{self.format_spec}}, new value
{value:{self.format_spec}}")
with format_spec stored as 'd' / '#010x' rather than C-style
> diff --git a/tools/linker-lists.py b/tools/linker-lists.py
> @@ -0,0 +1,234 @@
> +def handle_symbol(symbol, address, size):
> + if not symbol.startswith('_u_boot_list_'):
> + return
> +
> + symbol = symbol[13:]
> + if symbol.endswith('_1_start'):
> + assert(size == 0)
> + start_address[symbol[0:-8]] = address
> + return
> +
> + if symbol.endswith('_3_end'):
> + assert(size == 0)
> + end_address[symbol[0:-6]] = address
> + return
> +
> + if symbol.endswith('_0_item_align'):
> + item_alignment[symbol[0:-13]] = size
> + return
> +
> + if symbol.endswith('_0_item_size'):
> + item_size[symbol[0:-12]] = size
> + return
The magic slice offsets ([13:] etc.) are fiddly to verify by eye and
easy to break if any suffix changes. Please use
str.removeprefix()/str.removesuffix() - they are self-documenting and
the offsets disappear.
Also, the two assert(size == 0) calls are the wrong tool - if the ELF
ever contains a malformed start/end marker the script crashes rather
than reports. Since the whole point is sanity checking, better to call
warn() (and carry on) instead?
> diff --git a/tools/linker-lists.py b/tools/linker-lists.py
> @@ -0,0 +1,234 @@
> +subp = subprocess.Popen(cmd, stdout=subprocess.PIPE, text=True)
> +
> +for line in subp.stdout:
> + if '_u_boot_list_' not in line:
> + continue
> + parser(line)
There is no subp.wait() and no return-code check. If the ELF is
missing, or nm/readelf is not installed, the tool writes its complaint
to stderr and silently reports zero lists - and with --check still
exits 0, which is exactly the failure mode this is meant to catch in
CI. Please check subp.returncode after the loop and fail with a
message. Using 'with subprocess.Popen(...) as subp:' also ensures the
pipe is cleaned up.
> diff --git a/tools/linker-lists.py b/tools/linker-lists.py
> @@ -0,0 +1,234 @@
> +ap = ArgumentParser(description='Linker lists sanity checker')
> +
> +ap.add_argument('--parser', '-p', default='nm', choices=['nm', 'readelf'],
> + help='Program to use to parse the ELF file (nm or readelf)')
The whole body of the script runs at module scope. Please wrap it in a
main() function with the standard
if __name__ == '__main__':
sys.exit(main())
guard. That matches the other tools/ scripts and lets the parsing
helpers be imported by a future test without side effects.
> Add a python script which will make use of the special symbols emitted
> by the linker list macros, and perform various sanity checks. By
> default, it ends with printing a list of all the defined linker lists,
> including their start/end addresses, the size and aligment of
> individual items and the number of items.
Typos: 'alignment', and later 'each item belong to the list' should
read 'belonging to the list'.
Regards,
Simon
