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

PPC vector register break #16995

Closed
paulidale opened this issue Nov 9, 2021 · 9 comments
Closed

PPC vector register break #16995

paulidale opened this issue Nov 9, 2021 · 9 comments
Labels
branch: master Merge to master branch branch: 3.0 Merge to openssl-3.0 branch help wanted triaged: bug The issue/pr is/fixes a bug

Comments

@paulidale
Copy link
Contributor

Reported by @evanmiller as a comment on the commit:

This commit generates broken PPC code, such as

vsel 0,0,0,0

It appears that vsr2vr1 is being passed a full register name e.g. "v7" but the logic is expecting only the integer portion e.g. "7".

Downstream bug report: https://trac.macports.org/ticket/63847

@paulidale paulidale added branch: master Merge to master branch help wanted triaged: bug The issue/pr is/fixes a bug branch: 3.0 Merge to openssl-3.0 branch labels Nov 9, 2021
@evanmiller
Copy link
Contributor

Just to clarify, the flavor in question is osx32. Reproduce with

perl5 crypto/aes/asm/aesp8-ppc.pl "osx32" | grep vsel

@t8m
Copy link
Member

t8m commented Nov 9, 2021

@martin-schwenke Could you please help?

@martin-schwenke
Copy link
Contributor

Oh great! Hopefully the last of the PPC platforms broken with that handful of commits... 😃

I've pushed potential fix to https://github.com/martin-schwenke/openssl/tree/fix-osx.

I used the following script to test that it only changes output on OS X platforms:

#!/bin/sh

set -e

dir="$1"
mkdir -p "$dir"

flavours="aix32 aix64 linux32 linux64 linux64le linux64v2 osx32 osx64"
scripts=$(git grep -l ppc-xlate)

for f in $flavours ; do
	for s in $scripts ; do
		# Do not run scripts for 64-bit flavours against 32-bit flavours
		case "$f" in
		*32*)
			case "$s" in
			*64*)
				continue
			esac
		esac
		b=$(basename "$s")
		out="${dir}/${b}-${f}.out"
		perl "$s" "$f" "$out"
	done
done

Run with master branch with argument (say) before and then on this branch with argument (say) after. Then diff -qru before after shows only changes for OS X flavours. Dropping the q from the diff command appears to show useful changes for the OS X flavours.

@evanmiller Can you please verify that my branch fixes the build on osx32?

I'm no longer at IBM, so:

  • Perhaps @amitay can please take a look from a linux64le perspective and confirm that I'm not doing anything stupid.
  • If this turns into a PR then this will be under personal rather than corporate CLA - I'll try to remember to note that in the PR if we get to that point. 😉

@t8m
Copy link
Member

t8m commented Nov 9, 2021

The fix looks good to me - I did not verify it though.

@evanmiller
Copy link
Contributor

I do not know PPC assembly, but the comment over the original function (line 153) makes it sound like the function will change the prefix from vs to v.

Building now on a PPC32 machine.

@martin-schwenke
Copy link
Contributor

I do not know PPC assembly, but the comment over the original function (line 153) makes it sound like the function will change the prefix from vs to v.

The comment is a little confusing but, at the time, I couldn't figure out a way to keep it both useful and short. It is really referring to the 2 different ways that registers are numbered in the ISA. I blame the confusion on whoever decided the vector registers should be numbered in 2 different ways.

@evanmiller
Copy link
Contributor

@martin-schwenke Thanks for the clarification.

The generated assembly now builds successfully with the patch. Will report back when the complete build and test suite are finished.

@evanmiller
Copy link
Contributor

Builds and test suite passes locally 👍

@paulidale
Copy link
Contributor Author

Our CIs do a linux64le cross compile build. We've no other PPC builds otherwise.

martin-schwenke added a commit to martin-schwenke/openssl that referenced this issue Nov 14, 2021
vsr2vr1() fails on OS X because the main loop doesn't strip the
non-numeric register prefixes for OS X.

Strip any non-numeric prefix (likely just "v") from registers before
doing numeric calculation, then put the prefix back on the result.

Fixes: openssl#16995

Signed-off-by: Martin Schwenke <martin@meltin.net>
openssl-machine pushed a commit that referenced this issue Nov 18, 2021
vsr2vr1() fails on OS X because the main loop doesn't strip the
non-numeric register prefixes for OS X.

Strip any non-numeric prefix (likely just "v") from registers before
doing numeric calculation, then put the prefix back on the result.

Fixes: #16995

Signed-off-by: Martin Schwenke <martin@meltin.net>

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #17026)

(cherry picked from commit e67edf6)
ramikhaldi pushed a commit to ramikhaldi/openssl that referenced this issue Nov 21, 2021
vsr2vr1() fails on OS X because the main loop doesn't strip the
non-numeric register prefixes for OS X.

Strip any non-numeric prefix (likely just "v") from registers before
doing numeric calculation, then put the prefix back on the result.

Fixes: openssl#16995

Signed-off-by: Martin Schwenke <martin@meltin.net>

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from openssl#17026)
evanmiller added a commit to evanmiller/macports-ports that referenced this issue Dec 20, 2021
neverpanic pushed a commit to macports/macports-ports that referenced this issue Dec 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
branch: master Merge to master branch branch: 3.0 Merge to openssl-3.0 branch help wanted triaged: bug The issue/pr is/fixes a bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants