-
Notifications
You must be signed in to change notification settings - Fork 37.7k
devtools: Correctly extract symbol versions in symbol-check #22244
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
Conversation
I misunderstood the ELF specification for version symbols (verneed): The `vn_aux` pointer is relative to the main verneed record, not the start of the section. This caused many symbols to not be versioned properly in the return value of `elf.dyn_symbols`. This was discovered in bitcoin#21454. Fix it by correcting the offset computation.
xkb versions symbols (using the prefix `V`), as this library is used by bitcoin-qt, add it to the valid versions in `symbol-check.py`.
|
Is there a reference document that explains what the offset is? I found a doc that mentioned it, but my reading of that also came to the original conclusion that you made. |
I couldn't find anything. But this matches the All the other offsets ( |
Found it:
Source: Oracle, of all places… |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approach ACK c9b19ed, tested including comparison with master + reverted 634f6ec from #20434 (non-pixie).
* xkb versions symbols (using the prefix `V`), as this library is used by bitcoin-qt...
The libxkbcommon
package was added in #21376.
This PR works flawlessly. Need more time to reason about the last commit to final ACK.
FWW, the last commit is a continuation of #17538 from 2019, which bumped the minimum glibc to 2.17. See also your comment here you were rightly confused: #17538 (comment) Maybe it would be better to rewrite the code so that the minimum GLIBC version isn't split over two places? |
…bol-check.py The (ancient) versions specified here were deceptive. Entries older than MAX_VERSIONS['GLIBC'], which is 2.17, are ignored here. So reorganize the code to avoid confusion for other people reading this code.
Okay, pushed a new last commit that does this. Instead of hardcoding a separate table for GLIBC, it optionally allows entries in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK e8cd370
Okay, pushed a new last commit that does this. Instead of hardcoding a separate table for GLIBC, it optionally allows entries in
MAX_VERSIONS
to be a per-architecture dictionary. I hope this is clearer.
Many thanks for that!
On master, with applied #22281, #22287 and this PR:
For |
…ymbol-check e8cd370 devtools: Integrate ARCH_MIN_GLIBC_VER table into MAX_VERSIONS in symbol-check.py (W. J. van der Laan) a33381a devtools: Add xkb version to symbol-check (W. J. van der Laan) 19e598b devtools: Fix verneed section parsing in pixie (W. J. van der Laan) Pull request description: I misunderstood the ELF specification for version symbols (verneed): The `vn_aux` pointer is relative to the main verneed record, not the start of the section. This caused many symbols to not be versioned properly in the return value of `elf.dyn_symbols`. This was discovered in bitcoin#21454. Fix it by correcting the offset computation. - xkb versions symbols (using the prefix `V`), as this library is used by bitcoin-qt, add it to the valid versions in `symbol-check.py` This unfortunately brings to light some symbols that have been introduced since and weren't caught (from a gitian compile of master): ``` bitcoin-cli: symbol getrandom from unsupported version GLIBC_2.25 bitcoin-cli: failed IMPORTED_SYMBOLS bitcoind: symbol getrandom from unsupported version GLIBC_2.25 bitcoind: symbol log from unsupported version GLIBC_2.29 bitcoind: symbol fcntl64 from unsupported version GLIBC_2.28 bitcoind: symbol pow from unsupported version GLIBC_2.29 bitcoind: symbol exp from unsupported version GLIBC_2.29 bitcoind: failed IMPORTED_SYMBOLS bitcoin-qt: symbol exp from unsupported version GLIBC_2.29 bitcoin-qt: symbol fcntl64 from unsupported version GLIBC_2.28 bitcoin-qt: symbol log from unsupported version GLIBC_2.29 bitcoin-qt: symbol pow from unsupported version GLIBC_2.29 bitcoin-qt: symbol statx from unsupported version GLIBC_2.28 bitcoin-qt: symbol getrandom from unsupported version GLIBC_2.25 bitcoin-qt: symbol renameat2 from unsupported version GLIBC_2.28 bitcoin-qt: symbol getentropy from unsupported version GLIBC_2.25 bitcoin-qt: failed IMPORTED_SYMBOLS bitcoin-wallet: symbol exp from unsupported version GLIBC_2.29 bitcoin-wallet: symbol log from unsupported version GLIBC_2.29 bitcoin-wallet: symbol fcntl64 from unsupported version GLIBC_2.28 bitcoin-wallet: failed IMPORTED_SYMBOLS test_bitcoin: symbol getrandom from unsupported version GLIBC_2.25 test_bitcoin: symbol log from unsupported version GLIBC_2.29 test_bitcoin: symbol fcntl64 from unsupported version GLIBC_2.28 test_bitcoin: symbol pow from unsupported version GLIBC_2.29 test_bitcoin: symbol exp from unsupported version GLIBC_2.29 test_bitcoin: failed IMPORTED_SYMBOLS ``` ACKs for top commit: hebasto: ACK e8cd370 Tree-SHA512: 8c15e3478eb642f01a1ddaadef03f80583f088f9fa8e3bf171ce16b0ec05ffb4675ec147d7ffc6a4360637ed47fca517c6ca2bac7bb30d794c03783cfb964b79
I misunderstood the ELF specification for version symbols (verneed): The
vn_aux
pointer is relative to the main verneed record, not the start of the section.This caused many symbols to not be versioned properly in the return value of
elf.dyn_symbols
. This was discovered in #21454.Fix it by correcting the offset computation.
V
), as this library is used by bitcoin-qt, add it to the valid versions insymbol-check.py
This unfortunately brings to light some symbols that have been introduced since and weren't caught (from a gitian compile of master):