Mark OPENSSL_armcap_P .hidden in arm asm

[dg0yt]

Fixes #21583 (comment):

ld: error: relocation R_ARM_REL32 cannot be used against symbol 'OPENSSL_armcap_P'; recompile with -fPIC

when linking a static build of openssl to a shared library for 32 bit arm on Android,
since #21583.

This source change has one observable change in readelf output (e.g. for crypto/sha/libcrypto-lib-sha512-armv4.o):

 Symbol table '.symtab' contains 12 entries:
    Num:    Value  Size Type    Bind   Vis      Ndx Name
      0: 00000000     0 NOTYPE  LOCAL  DEFAULT  UND 
      1: 00000000     0 SECTION LOCAL  DEFAULT    2 
      2: 00000000   640 OBJECT  LOCAL  DEFAULT    2 K512
      3: 00000000     0 NOTYPE  LOCAL  DEFAULT    2 $d.0
      4: 000002a0     0 NOTYPE  LOCAL  DEFAULT    2 $t.1
      5: 0000183a     0 NOTYPE  LOCAL  DEFAULT    2 $d.2
      6: 00000000     0 SECTION LOCAL  DEFAULT    6 
      7: 00000000     0 SECTION LOCAL  DEFAULT    8 
      8: 00000000     0 SECTION LOCAL  DEFAULT   11 
-     9: 00000000     0 NOTYPE  GLOBAL DEFAULT  UND OPENSSL_armcap_P
+     9: 00000000     0 NOTYPE  GLOBAL HIDDEN   UND OPENSSL_armcap_P
     10: 000002a1  1134 FUNC    GLOBAL DEFAULT    2 sha512_block_data_order
     11: 00000711  4394 FUNC    GLOBAL DEFAULT    2 sha512_block_data_order_n
 

This change was automated using

sed -e '/[.]hidden.*OPENSSL_armcap_P/d; /[.]extern.*OPENSSL_armcap_P/ {p; s/extern/hidden/ }' -i -- crypto/*arm*pl crypto/*/asm/*arm*pl

with the filepath patterns derived from #21583.

[dg0yt]

More reading:
Commit message in google/boringssl@16e38b2.
android/ndk#191.

[tom-cosgrove-arm]

Rather than making an identical change in every Perl-asm file, and then requiring this same pairing in every new Perl-asm that gets added in the future, wouldn't it be better to have arm-xlate.pl apply this directly? (At least, that's the way I've been looking at fixing this)

[dg0yt]

I can't say much about openssl's perl asm stuff.
First, it should be validated that .hidden is the right thing here.

[tom-cosgrove-arm]

Note that this isn't what BoringSSL has anyway - they still have .comm rather than .extern. One of our recent-ish changes made that here, but I am now revisiting whether that was the best thing to do, as arm-xlate.pl actually removes the .extern altogether, but .comm keeps it in

[kroeckx]

The .comm vs .extern is not relevant. The reason the .hidden solves the problem is that it now can not be a symbol overridden by another library or executable. There is no reason to support that.

[talregev]

@t8m
Any news?

[tom-cosgrove-arm]

I am looking at this (slowly, sorry)

[BillyONeal]

@tom-cosgrove-arm @t8m Given those tags just added does that mean this solution is acceptable to the OpenSSL maintainers? (I'm trying to confirm that it's safe to merge @dg0yt 's patch of this in vcpkg)

[tom-cosgrove-arm]

@BillyONeal I don't think the final fix will look like this, but not sure exactly what it should be at the moment. This is now top of my OpenSSL list, though

(The reason I don't think the final fix will look like this is at a minimum the duplication, and I think the fix will need something in arm-xlate.pl too)

[BillyONeal]

@tom-cosgrove-arm OK, thanks, I'll drop this from daily checks. Please poke me when this is fixed if you remember :)

[kroeckx]

I see no problem with the patch as is

[BillyONeal]

I see no problem with the patch as is

As in, it passes for the platforms vcpkg ships to but there are others that openssl cares about perl scripts or something like that, so I should merge @dg0yt's PR?

[tom-cosgrove-arm]

As it stands, this patch would require us to add .hidden every time we add a new assembler file - specifically, to remember to do that. That's not good for maintainability - it will just cause this issue to recur every now and them. Moreover, I don't understand why this patch works - declaring the variable .extern should be sufficient. However, the xlate script removes .extern for unknown reasons, and that doesn't seem right either. Hence: doing more investigations to understand the root cause, rather than adding a workaround

[talregev]

@tom-cosgrove-arm We just want to know if the current change is safe to merge in vcpkg side. meaning everytime that someone will use vcpkg openssl with this patch it will work correctly and it will be safe to use.
When you find long term solution and it will be in different PR or / and it will be on the next version, when vcpkg will update the openssl next version it will take the long term solution with the new version / new commit.

Thank you for your time.

[dg0yt]

However, the xlate script removes .extern for unknown reasons, and that doesn't seem right either.

Can you verify this? I don't see in the script, and I don't see the effect in the build dir. E.g.:

buildtrees/openssl/arm-neon-android-dbg/crypto/sha/sha512-armv4.S:.extern       OPENSSL_armcap_P
buildtrees/openssl/arm-neon-android-dbg/crypto/sha/sha512-armv4.S:.hidden       OPENSSL_armcap_P

And IIUC, OPENSSL_armcap_P would be undeclared without .extern.

[kroeckx]

As the linker complains, the code is not position independent code (PIC). With a symbol that's exported, the dynamic linker needs to be able to support replacing the symbol by one from an other library (or executable). To be able to do that, the code needs to be PIC, and you'll get a different relocation type. So there are a few solutions:

• Don't export the symbol. This can be done by marking it hidden. The linker will then know no other library can replace it, and it doesn't need to be PIC.
• Rewrite the code to be PIC.
• Tell the linker it doesn't need to support replacing the symbol, by building the shared library using -Bsymbolic.

[kroeckx]

Note that OpenSSL builds using -Bsymbolic by default, which is why it doesn't show up for most people.

[t8m]

The problem is now independently reported in #22414

[Eimji]

Hello

I tried the fix here, it works, I can compile openssl for my Android application.

Thanks.

[dg0yt]

Is the fix to a present problem still blocked by concerns about files which might be added in the future?
Isn't it possible to verify whether "the xlate script removes .extern"?

[dg0yt]

Still waiting.

[dg0yt]

I am looking at this (slowly, sorry)

Any progress?

[talregev]

@t8m Are you going to respond on this issue?
Not solving this issue is also a respond.

[dg0yt]

@talregev Please don't put oil on the fire.

[tom-cosgrove-arm]

Yes, still looking at it. The issue recently fixed by #22880 meant that some of the changes we made to the handling of OPENSSL_armcap_P could be rolled back, so I am looking at doing that as an option

[dg0yt]

Another six weeks. Why contribute at all.

[t8m]

@tom-cosgrove-arm any progress on this?

[ospfranco]

Facing this issue. Seems there is no possible workaround at the moment. Is there anything that can be done to merge this faster? Is sponsoring possible?

I've also created an issue on the NDK repo in hopes of finding a workaround.

At least in my use case, openSSL is being compiled as a subdependency from Rust, so I cannot apply this changes directly I believe.

[dg0yt]

Spring 2024.

[t8m]

@tom-cosgrove-arm ping?

[tom-cosgrove-arm]

Yes, sorry, won't be this week, but still on my radar

[dg0yt]

May 2024.