Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Require openssl-no-asm option explicitly if older assembler version is found. #19944

Closed
shigeki opened this issue Apr 11, 2018 · 13 comments
Closed

Comments

@shigeki
Copy link
Contributor

shigeki commented Apr 11, 2018

Currently openssl asm support requires assembler versions as described in BUILDING.md and automatically falls back to no-asm build with warning.
Should we stop building then require and opt-in to have --openssl-no-asm option in that case?

CC @nodejs/build

#### OpenSSL asm support

OpenSSL-1.1.0 requires the following asssembler version for use of asm
support.

* gas (GNU assembler) version 2.23 or higher
* xcode version 5.0 or higher
* llvm version 3.3 or higher
* nasm version 2.10 or higher in Windows

Otherwise, `--openssl-no-asm` is added with warning in configure.

*Note:* The forthcoming OpenSSL-1.1.1 will require higher
 version. Please refer
 https://www.openssl.org/docs/man1.1.1/man3/OPENSSL_ia32cap.html for
 details.

Ref #19918

  • Version: 10.0.0-pre
  • Platform: All
  • Subsystem: build, deps(openssl)
@rvagg
Copy link
Member

rvagg commented Apr 11, 2018

+1 consistent with #19943 if we accept that one too. I'm going with @bnoordhuis on this one that you should opt in.

Your notes about versions had me wondering if our infra even supports this and our Linux release builders didn't until today! So nightlies, and the last 10.0.0 test build for Linux (@ https://nodejs.org/download/test/v10.0.0-test20180410edd9cc466a/) would have been built without asm support if it was falling back to no-asm silently. as was 2.20.

I also still need to make sure our macOS machines are fully updated with xcode too.

It would have been nice to get build failures because as was too old instead of silently building suboptimal binaries. Hence, +1 on this from me.

@shigeki
Copy link
Contributor Author

shigeki commented Apr 12, 2018

When we make opt-in, we should consider the future upgrade to OpenSSL-1.1.1 which needs higher version so as not to be semver-major update.

Following to the requirements in 1.1.1, asm support is

#### OpenSSL asm support

OpenSSL requires the following asssembler version for use of asm
support.

* gas (GNU assembler) version 2.26 or higher
* xcode version 5.0 or higher
* llvm version 3.3 or higher
* nasm version 2.11.8 or higher in Windows

Otherwise, build with `--openssl-no-asm` in configure.

*Note:* This requirements come from the forthcoming OpenSSL-1.1.1. See
 https://www.openssl.org/docs/man1.1.1/man3/OPENSSL_ia32cap.html for
 details.

At the current ,the following CI would be failed due to the older assembler version.

debian8-64: 'gas_version': '2.25',
debian8-x86: 'gas_version': '2.25',
ubuntu1404-32: 'gas_version': '2.24',
ubuntu1404-64: 'gas_version': '2.24',
ppcle-ubuntu1404: 'gas_version': '2.24',
smartos15-64: 'gas_version': '2.25',
rhel72-s390x: 'gas_version': '2.25',
cc-armv6:'gas_version': '2.25',
cc-armv7:'gas_version': '2.25',

aix61-ppc64: defined no_asm?

@mhdawson
Copy link
Member

ppcle, s390x how use higher versions for Node 10.x and higher.

aix we'll have to check why it reports as no_asm.

@mhdawson
Copy link
Member

as is on the machine but reports:

^C-bash-4.3$ as --version
Assembler:
line 1: 1252-168 --version is not a recognized flag.
line 1: 1252-170 Usage: as [-a{32|64}] -l[ListFile] -s[ListFile] -n Name
-o ObjectFile [-w|-W] -x[XCrossFile] -u
-m ModeName [-i] [-p{off|on}] [-E{off|on}] [-v] [InputFile]

@mhdawson
Copy link
Member

@gireeshpunathil can you take a look at the requirements for as in openssl for AIX to figure out if we need an additional or newer version of the as tool or if the detection logic needs a tweek.

@gireeshpunathil
Copy link
Member

gireeshpunathil commented Apr 20, 2018

sure

@richardlau
Copy link
Member

as is on the machine but reports:

^C-bash-4.3$ as --version
Assembler:
line 1: 1252-168 --version is not a recognized flag.
line 1: 1252-170 Usage: as [-a{32|64}] -l[ListFile] -s[ListFile] -n Name
-o ObjectFile [-w|-W] -x[XCrossFile] -u
-m ModeName [-i] [-p{off|on}] [-E{off|on}] [-v] [InputFile]

That's almost certainly not the GNU assembler and looks like it is the AIX assembler.

Node.js' configure explicitly looks for GNU assembler:

node/configure

Lines 679 to 701 in 0857790

def get_gas_version(cc):
try:
proc = subprocess.Popen(shlex.split(cc) + ['-Wa,-v', '-c', '-o',
'/dev/null', '-x',
'assembler', '/dev/null'],
stdin=subprocess.PIPE, stderr=subprocess.PIPE,
stdout=subprocess.PIPE)
except OSError:
print('''Node.js configure error: No acceptable C compiler found!
Please make sure you have a C compiler installed on your system and/or
consider adjusting the CC environment variable if you installed
it in a non-standard prefix.
''')
sys.exit()
match = re.match(r"GNU assembler version ([2-9]\.[0-9]+)",
proc.communicate()[1])
if match:
return match.group(1)
else:
return 0

on one of IBM's internal AIX systems, the equivalent gcc command produces:

-bash-4.4$ gcc -Wa,-v -c -o /dev/null -x assembler /dev/null
as V6.1
-bash-4.4$

which doesn't match the regexp so we end up with 'gas_version': '0'.

Does openssl support AIX's assembler?

cc @nodejs/platform-aix

@shigeki
Copy link
Contributor Author

shigeki commented Apr 20, 2018

I found that OpenSSL built on s390 does not check assembler version.
https://github.com/nodejs/node/blob/master/deps/openssl/openssl/crypto/sha/asm/sha512-s390x.pl
It should be fixed but I never had an experience to build openssl on s390.
In order to check generated assembler exactly, can I login and build OpenSSL on it?
Otherwise, please build openssl-1.1.0h on s390 with

config no-comp no-shared no-afalgeng; make

and then give me build sources in tar ball.

@shigeki
Copy link
Contributor Author

shigeki commented Apr 20, 2018

And also please give me a stdout logs of config and make.

@richardlau
Copy link
Member

cc @nodejs/platform-s390 @nodejs/build

@mhdawson
Copy link
Member

@jBarz @mmalecki can you help @shigeki get what he needs? @shigeki we can also get you access to one of the linuxOne machines in the community CI as well. I've opened the issue to request access for you here: nodejs/build#1246

@joransiu
Copy link
Contributor

I found that OpenSSL built on s390 does not check assembler version.

What does "check assembler version" mean here? Are you suggesting the sha512-s390x.pl check for as version too? Not sure if I see similar checks in other platform's OpenSSL ASM perl scripts.

@shigeki
Copy link
Contributor Author

shigeki commented Apr 23, 2018

What does "check assembler version" mean here?

Other architectures have checks assembler version such as

if (`$ENV{CC} -Wa,-v -c -o /dev/null -x assembler /dev/null 2>&1`
=~ /GNU assembler version ([2-9]\.[0-9]+)/) {
$avx = ($1>=2.19) + ($1>=2.22);
}
if (!$avx && $win64 && ($flavour =~ /nasm/ || $ENV{ASM} =~ /nasm/) &&
`nasm -v 2>&1` =~ /NASM version ([2-9]\.[0-9]+)/) {
$avx = ($1>=2.09) + ($1>=2.10);
}
if (!$avx && $win64 && ($flavour =~ /masm/ || $ENV{ASM} =~ /ml64/) &&
`ml64 2>&1` =~ /Version ([0-9]+)\./) {
$avx = ($1>=10) + ($1>=12);
}
if (!$avx && `$ENV{CC} -v 2>&1` =~ /((?:^clang|LLVM) version|.*based on LLVM) ([3-9]\.[0-9]+)/) {
$avx = ($2>=3.0) + ($2>3.0);
}

jasnell pushed a commit that referenced this issue Apr 23, 2018
PR-URL: #20226
Fixes: #19944
Refs: #20217
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ali Ijaz Sheikh <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
jasnell pushed a commit that referenced this issue Apr 23, 2018
The current openssl checks assembler version only x86_64 or ia32
target arch for use of AES-NI, AVX and AVX2.
This requires --openssl-no-asm option during configure when an older
assembler version is found only on x86_64 or ia32.

PR-URL: #20226
Fixes: #19944
Refs: #20217
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ali Ijaz Sheikh <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
jasnell pushed a commit that referenced this issue Apr 23, 2018
PR-URL: #20226
Fixes: #19944
Refs: #20217
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ali Ijaz Sheikh <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
jasnell pushed a commit that referenced this issue Apr 23, 2018
PR-URL: #20226
Fixes: #19944
Refs: #20217
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ali Ijaz Sheikh <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
jasnell pushed a commit that referenced this issue Apr 23, 2018
The current openssl checks assembler version only x86_64 or ia32
target arch for use of AES-NI, AVX and AVX2.
This requires --openssl-no-asm option during configure when an older
assembler version is found only on x86_64 or ia32.

PR-URL: #20226
Fixes: #19944
Refs: #20217
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ali Ijaz Sheikh <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants