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.

Sign up for GitHub

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

Jump to bottom

Fix chacha-armv4.pl with clang -fno-integrated-as. #3694

Closed
wants to merge 1 commit into from
Closed

Fix chacha-armv4.pl with clang -fno-integrated-as. #3694

wants to merge 1 commit into from

Conversation

davidben
Copy link
Contributor

@dot-asm, does this look right? I'm not fully up to speed with all the details around ARM assembly syntax variants and am piecing things together as I go.

The __clang__-guarded #defines cause gas to complain if clang is passed
-fno-integrated-as. Emitting .syntax unified when those are used fixes
this. This matches the change made to ghash-armv4.pl in
6cf412c.

@davidben
Fix chacha-armv4.pl with clang -fno-integrated-as.
f6378e2 
The __clang__-guarded #defines cause gas to complain if clang is passed
-fno-integrated-as. Emitting .syntax unified when those are used fixes
this. This matches the change made to ghash-armv4.pl in
6cf412c.
@davidben
Copy link
Contributor Author

davidben commented Jun 16, 2017 •
edited

By the way, what's the reason not to just use .syntax unified uniformly? I'm not sure how old of assemblers OpenSSL supports. Looks like support in gas dates to binutils 2.17 which was released back in 2006.

@dot-asm
Copy link
Contributor

dot-asm commented Jun 17, 2017

what's the reason not to just use .syntax unified uniformly?

You mean unconditionally? I.e. with no #ifdef __thumb2__? Well... It's an option, but I'd still say it's a bit daring. I mean I don't have the feeling that we can't run into some elderly cross-compiler for some embedded system. Those things tend to be more conservative than one would normally expect... And as long as it's not very big burden (keeping it working even with older tools), I don't really see conditional .syntax unified as problem. As counter-example all those thumb2-conditional itt* things are much bigger annoyance. I mean if assembler didn't issue warnings[!!!] "you've missed itt* thing", who in sane state of mind would add them? BTW, how do I add them? I just look at what assembler generates and copy them. I mean assembler does generate them, just complains about doing its job...

@dot-asm
Copy link
Contributor

dot-asm commented Jun 17, 2017

Concern is extra #if condition few lines below. I mean I need to cross-check this a bit more, especially with iOS...

@davidben
Copy link
Contributor Author

You mean the #define ldrhsb ldrbhs block? That's the motivation for this. Without .syntax unified, clang with -fno-integrated-as (i.e. it uses the system one) rejects ldrbhs. 6cf412c has some similar macros.

agl pushed a commit to google/boringssl that referenced this pull request Jun 28, 2017
@davidben
Fix chacha-armv4.pl with clang -fno-integrated-as.
3c97292 
The __clang__-guarded #defines cause gas to complain if clang is passed
-fno-integrated-as. Emitting .syntax unified when those are used fixes
this. This matches the change made to ghash-armv4.pl in upstream's
6cf412c473d8145562b76219ce3da73b201b3255.

See also openssl/openssl#3694. This fixes the
build with the latest Android NDK (use the NDK-supplied toolchain file)
with the armeabi ABI.

Bug: chromium:732066
Change-Id: Ic6ca633a58edbe8ae8c7d501bd9515c2476fd7c2
Reviewed-on: https://boringssl-review.googlesource.com/17404
Commit-Queue: Steven Valdez <svaldez@google.com>
Reviewed-by: Steven Valdez <svaldez@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
@davidben
Copy link
Contributor Author

FWIW, this seems to build fine for us on iOS. This file does not have a __thumb2__ undef, so I think it's a no-op.

@kaduk
Copy link
Contributor

kaduk commented Sep 12, 2017

@dot-asm is this still on your radar?

dot-asm
dot-asm approved these changes Nov 5, 2017
View reviewed changes
Copy link
Contributor

@dot-asm dot-asm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I apologize that it effectively slipped. But I recalled this [and double-checked] when looking at recent problems triggered by latest binutils :-)

@dot-asm dot-asm added the approval: review pending This pull request needs review by a committer label Nov 5, 2017
@kroeckx kroeckx added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Nov 26, 2017
@dot-asm
Copy link
Contributor

dot-asm commented Nov 28, 2017

Merged. Thanks!

@dot-asm dot-asm closed this Nov 28, 2017
levitte pushed a commit that referenced this pull request Nov 28, 2017
@davidben
Fix chacha-armv4.pl with clang -fno-integrated-as.
3f6a831 
The __clang__-guarded #defines cause gas to complain if clang is passed
-fno-integrated-as. Emitting .syntax unified when those are used fixes
this. This matches the change made to ghash-armv4.pl in
6cf412c.

Reviewed-by: Andy Polyakov <appro@openssl.org>
Reviewed-by: Kurt Roeckx <kurt@roeckx.be>
(Merged from #3694)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kroeckx kroeckx kroeckx approved these changes

@dot-asm dot-asm dot-asm approved these changes

Assignees
No one assigned
Labels
approval: done This pull request has the required number of approvals
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@davidben @dot-asm @kaduk @kroeckx