-
Notifications
You must be signed in to change notification settings - Fork 746
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
Altivec code in crypto/ocb_internal.cc
fails to compile, needs to use __vector
instead of vector
#1321
Comments
crypto/ocb_internal.cc
fails to compilecrypto/ocb_internal.cc
fails to compile
Please provide details about your system. This works in our CI and in all distribution packages. |
@achernya Thank you for responding! It could be that either code is correct but uses VSX and not Altivec, in which case fixing a macro will work (i.e., use If it is supposed to be ISA 2.03-compliant, then a macro for |
With includes by the way configure does not check for |
A lazy fix will be
or if it is known that it works on G4/G5 but not on MacOS, then
However it may be just some error, and the code could work. In which case we would not want to simply disable vectorization. |
macOS 10.6 is no longer in support and ocb_internal is vendered code from https://www.cs.ucdavis.edu/~rogaway/ocb/news/code/ocb.c so I'm hesitant to touch it in a meaningful way other than deletions. I suggest you try enabling the openssl-based ocb implementation instead. (That would have been our default for mosh-1.4.0 if it hasn't been for CVE-2022-2097) |
@achernya Well, if no one knows how to fix it to work on macOS (I do not know Altivec syntax), then just disabling is fine. It will not affect anything else, obviously, since |
@barracuda156 please demonstrate that the build is broken on a modern, supported macOS -- otherwise we are not interested in this patchset. |
@achernya The patch specifically disables it where it is broken. It does not affect other systems where the code is presumed to work. I also fix the failing case here. |
@barracuda156 as I mentioned in my comment above: I am not willing to take any changes to this file other than deletions. If you would like to get mosh to work, please enable the openssl-based ocb build instead. If you would like to submit a PR to delete all the altivec code instead, I would be willing to merge that. |
This comment was marked as off-topic.
This comment was marked as off-topic.
@achernya I guess I did not understand correctly your reasoning. If you do not want to modify the code because it is vendored, I am not a legal expert, but I can accept that. I am not sure if it is a right thing to remove the code, since while it will fix the bug for one platform, it may result in suboptimal performance on another platform, and I have no way to verify if the code builds on BSD and Linux or not. |
@barracuda156 there's nothing about legality or not of modifying it. It's our inability to have CI for this configuration, therefore a desire to not support it. |
@achernya What you do with your project is up to you, of course, but to be honest I cannot see how my fix could have any conceivable adverse effect on any platform whatsoever. The only effect of it was to disable usage of that Altivec code on macOS. It was as non-invasive as possible. I am not asking you to support PowerPC proactively nor expect anyone to do it. But no one is better off if the broken code is left unfixed, when the cost of the fix is a single line change with no effect on other systems. |
Your fix creates an ongoing expectation of support by the maintainers of this project. I've already expressed my desires here that we reduce that support burden since we cannot easily test for altivec on ppc. As a result, I can't take your contribution as is. Continuing to discuss won't cause us to accept the single-line fix since it doesn't change our ability to test it in an ongoing basis. |
Just for the record if anyone ever bumps into the issue: the correct fix is to change My initial suggestion to disable it was suboptimal. The code is fine, but type used is inaccurate. Related chunk in |
crypto/ocb_internal.cc
fails to compilecrypto/ocb_internal.cc
fails to compile, needs to use __vector
instead of vector
Code in
ocb_internal.cc
is broken and fails to compile:Same failure with gcc-4.2 and gcc-13.2.1.
The text was updated successfully, but these errors were encountered: