[PATCH 0/3] aarch64: enable PAC and BTI

[Bill Roberts]

<snip>

> 
> I also have a question on copyright. I take it you are doing this work
> on behalf of your employer? 

Yes

Do you know if ARM Limited (is that the
> proper company name?)

That is the proper name AFAICT

  has a copyright assignment agreement in place with
> the FSF, that covers GMP, of if we need to do some paperwork?

I will find out.

> 
> Concrete suggestions:
> 
> Please drop the sed, grep and egrep configure checks, I think the job
> can be done with portable mainstream features of sed and grep (and in

So, usually in a configure script you check that the tools needed 
further down the road are available now rather than failing further down 
because of a "command not found" error. For example, one checks that a C 
compiler is available, it is assigned to variable CC and invoking it 
would always use the CC variable. Likewise, Autoconf provides the 
AC_PROG_SED and GREP checks as a standard way of doing this, and to 
ensure interoperability across systems. They're even dedicated macros, 
not even an auxiliary checks using something like AC_CHECK_PROG or friends.

So existing GMP code, without these patches, could really benefit from 
using that autoconf standard mechanism. So I don't know why we would 
drop that. I am more than happy to, but its fixing a bug, which is why I 
broke it out into a separate patch.

> addition, your use egrep looks suspicious, as I read it, is '(' used
> both for grouping and as a literal character to match). See
> https://www.gnu.org/savannah-checkouts/gnu/autoconf/manual/autoconf-2.72/autoconf.html#Limitations-of-Usual-Tools
> for some advice on which features of sed and grep to avoid.
I'll take a closer look at this. But a quick peruse, I thought I needed 
egrep for | but it looks like you just escape it \|. So that should be 
easy enough to change to using regular grep.

> 
> Please drop configure check for readelf, just let the test script exit
> with a log message and exit code 77 if readelf is unavalable.

I can, but I think the issue here is that when readelf isn't there, and 
I am running the test in an environment where I want a hard failure, ie 
the binary is built with support, I don't want to skip if readelf is 
missing as something like a CI will still pass. I think if readelf is 
missing, we can make this a hard failure?

It's interesting to me the push back on configure time checks for tools 
needed under certain conditions and thus pushing the error further out 
instead of providing a clear message to the user. I'm just curious what 
the rationale is?

> 
> I would prefer to also drop the dependency on bash, are bash features
> really essential for this test? From a quick look, I see no use of fancy
> things like bash arrays. (I do see a mix of [ ... ] and bash-specific [[
> ... ]], which is a bit inconsistent).

local, pipefail and declare -g offhand. However, pipefail and local can 
go away and declare -g goes back to eval. I had an eval there before and 
someone talked me into declare -g. But yeah, we can remove this 
requirement and do it POSIX shell.

> 
> Regards,
> /Niels

Thanks Niels, appreciate the review.Bill