On 30 Oct 2022 00:44, Indu Bhagat via Binutils wrote:
> --- a/libsframe/configure.ac
> +++ b/libsframe/configure.ac
>  
> +# Determine if we are cross compiling

use `dnl` for comments in .ac files

> +AC_CANONICAL_HOST

this only sets up $host, but you're also checking $target.  you should be
using AC_CANONICAL_TARGET.
https://www.gnu.org/savannah-checkouts/gnu/autoconf/manual/autoconf-2.71/html_node/Canonicalizing.html

> --- /dev/null
> +++ b/libsframe/testsuite/lib/sframe-lib.exp
>
> +load_file $srcdir/../../ld/testsuite/lib/ld-lib.exp

this doesn't seem right to me, but i guess if the ld maintainers are
OK with it ...

> --- /dev/null
> +++ b/libsframe/testsuite/libsframe.unwind/solib.exp
>
> +# Run the tests only if we are not cross compiling.

why ?  people need to test cross-compilers too.  some folks will never test
it with a native compiler because they only ever make cross-compilers, which
means these tests rarely (if ever) get run.  especially for embedded arches
that can't self-host (e.g. nommu).

> --- /dev/null
> +++ b/libsframe/testsuite/libsframe.unwind/unwind.exp
>
> +if {[info exists env(LC_ALL)]} {
> +    set old_lc_all $env(LC_ALL)
> +}
> +set env(LC_ALL) "C"

why ?  weird code like this really needs a comment.  this is usually
a read flag that the test/code is broken.
-mike