Array was 4 words; vendor asm .comm requests 10, so linker warns
and the reserved capability bits are never seen by the C side.
Details
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
No Lint Coverage - Unit
No Test Coverage - Build Status
Buildable 75563 Build 72446: arc lint + arc unit
Event Timeline
https://cgit.freebsd.org/src/tree/sys/crypto/openssl/i386/x86cpuid.S << this is vendored.
Maybe i should write : upstream ? Or it fine ?
Indeed, the assembly in crypto/openssl/crypto/x86_64cpuid.pl has:
.hidden OPENSSL_ia32cap_P .comm OPENSSL_ia32cap_P,40,4 # <--Should match with internal/cryptlib.h OPENSSL_IA32CAP_P_MAX_INDEXES .text
and crypto/openssl/include/internal/cryptlib.h has:
#if defined(__i386) || defined(__i386__) || defined(_M_IX86) || defined(__x86_64) || defined(__x86_64__) || defined(_M_AMD64) || defined(_M_X64) #define OPENSSL_IA32CAP_P_MAX_INDEXES 10 extern unsigned int OPENSSL_ia32cap_P[]; #endif
It's unfortunate we can't just use the OPENSSL_IA32CAP_P_MAX_INDEXES define directly in sys/crypto/openssl/ossl_x86.c, but fixing the value directly is probably better than leaving it too small.
Alternatively, since it's a common symbol, we might also get away with declaring it as weak, and not specifying the size at all? But for kernel land, I'm unsure whether this works.
The code in sys/crypto/openssl/ossl_x86.c got moved around by @mhorne in rGfd86ae6800383dabe050e22176783857895800e3, maybe Mitchell has opinions about this too.
Ok, the original ossl.c with this OPENSSL_ia32cap_P definition was added by @jhb in rGba610be90a7cb6d851e0e0e6d7612769352a3c0c. This was in 2020, so maybe OPENSSL_ia32cap_P was indeed 4. The code in ossl_cpuid() only accesses OPENSSL_ia32cap_P[0] through OPENSSL_ia32cap_P[3], not anything beyond it.
The original code was correct at that time. I only noticed this warning after compiling FreeBSD for the Xbox.
| sys/crypto/openssl/ossl_x86.c | ||
|---|---|---|
| 44 | Note this, and see the comments below. If we are going to expand the array, it's important that we populate the new leaves with the correct values as documented in the updated manpage for newer versions of OpenSSL. The current manpage (https://man.freebsd.org/cgi/man.cgi?query=OPENSSL_ia32cap&apropos=0&sektion=0&manpath=FreeBSD+16.0-CURRENT&format=html) for OpenSSL in the tree only documents one additional level (LV4). Presumably the other leaves are just zero-initialized. If the assembly is setting the size by treating it as a common symbol, then perhaps we can remove this definition and instead just depend on it being declared extern as an array. That I think is a modification of what dim@ suggested and would let us use OpenSSL's size always (which is more future proof). I guess for now this commit can just remove the declaration below (and replace it with an extern if needed). Implementing the LV4 leaf could be a separate commit, but is worth doing. | |