On 24.07.2024 02:18, victorm.l...@amd.com wrote:
> --- /dev/null
> +++ b/automation/eclair_analysis/linker_symbols.sh
Nit: In names of new files we prefer - over _.
> @@ -0,0 +1,41 @@
> +#!/bin/bash
Can we rely on bash to be there and at that location? As you using any
bash-isms in the script which cannot be avoided?
> +# Stop immediately if any executed command has exit status different from 0.
> +set -e
> +
> +# Extract linker symbol names (except those starting with ".") from
> assignments.
> +
> +script_name=`basename "$0"`
Personally I consider $(...) more readable. What I'm strictly going to
request is that within a script you at least be consistent, seeing ...
> +script_dir="$(
> + cd "$(dirname "$0")"
> + echo "${PWD}"
> +)"
... e.g. this.
> +src_dir="${script_dir}/../.."
> +
> +usage() {
> + echo "Usage: ${script_name} <ARM64|X86_64>"
Why require arguments that then ...
> +}
> +
> +if [ $# -ne 1 ]; then
> + usage
> + exit 1
> +fi
> +
> +if [ "$1" == "X86_64" ]; then
> + filepaths=(
> + "${src_dir}/xen/arch/x86/xen.lds.S"
> + )
> +elif [ "$1" == "ARM64" ]; then
> + filepaths=(
> + "${src_dir}/xen/arch/arm/xen.lds.S"
> + )
... you need to special case? What's wrong with having the arguments be
"arm" or "x86"?
However, you also cannot use xen.lds.S here, as that may yield incomplete
results. This script needs running _after_ a successful build, on the
generated xen.lds.
> +else
> + usage
> + exit 1
> +fi
> +
> +(
> + for file in "${filepaths[@]}"
> + do
> + sed -n "s/^\s*\([a-zA-Z_][a-zA-Z_0-9.\-]*\)\s*=.*;\s*$/\1/p"
> $filepaths
> + done
> +)
Why the extra parentheses? And why a loop and filepaths being an array,
when there's guaranteed to be only one file?
Jan
