MDEV-35462: Cleanup PIC#5217
Conversation
grooverdan
added
the
MariaDB Foundation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the CMake build configuration for RocksDB by enabling position-independent code (POSITION_INDEPENDENT_CODE ON) for the rocksdblib target. There are no review comments, and I have no additional feedback to provide.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
vaintroub
left a comment
vaintroub
left a comment
There was a problem hiding this comment.
Actually, fPIC should have been handled by ADD_CONVENIENCE_LIBRARY, this thing was there alone for that purpose, to compile static libraries with shared libraries flags. and predates corresponding CMake property. but worked ok so far, and this is why we do not recompile mysys or strings for embedded shared library.
grooverdan
force-pushed
the
MDEV-35462_fix
branch
3 times, most recently
from
60cddb6 to
e00a6f8
Compare
-fPIC was incorrectly removed in 526f076 causing the rocksdb library to fail to link to a shared ha_rocksdb.so. As the rocksdb static library is dynamicly linked to the rocksdb storage engine, position independent code is required. This is the reponsibility of the ADD_CONVIENCE_FUNCTION. ADD_CONVIENCE_FUNCTION sets the cmake property POSITION_INDEPENDENT_CODE for all except DISABLED_SHARED. In MERGE_LIBRARIES, we check the PIC property directly rather than make assumptions based on the CFLAGS. ASAN didn't need the -fPIC so removed. gprof hasn't need -fPIC or -no-pie in recent history (gcc-10+). Remove the explictness around -fPIC everywhere using the PIC property. Bundled PCRE has explicit -DCMAKE_POSITION_INDEPENDENT_CODE=ON. Security hardened profiles can set -fPIC/pie directly as they are default options now. WITH_PIC options. "Not of much use," - so its removed. Create tpool_embedded as convience library for embedded server. CSV, Sequence and userstat plugins as manditory plugins need to be recompile for embedded. Remove TOKUDB only MYSQL_PROJECT_NAME_DOCSTRING
grooverdan
force-pushed
the
MDEV-35462_fix
branch
from
e00a6f8 to
bf360e1
Compare
grooverdan
changed the title
There was a problem hiding this comment.
I would not do tpool_embedded, but rather compile it with PIC. I'd even prefer SET_TARGET_PROPERTIES(POSITION_INDEPENDENT_CODE) to the dated ADD_CONVENIENCE_LIBRARY contraption. Either always (my preference), or if WITH_EMBEDDED_SERVER is set or Innodb is compiled as dynamic plugin (it links directly to tpool, so it does need PIC in this case)
I do not think tpool with PIC will have register pressure or much impact of GOT redirection, and this won't have any noticeable impact on performance, it is rather heavy on system and library calls, so PIC here won't have much impact, even on systems where PIC means overhead.
Historically, "embedded" convention, and "recompile for embedded" is that : only libraries that depend on EMBEDDED_LIBRARY preprocessor symbol get their "_embedded" counterparts. Because then the ABI is different, sizeof(THD) varies and this is the main reason. There is no THD in tpool, it is kept clean of such things, by design :) Other code, that could be used in shared libraries, was compiled with PIC
2 participants
-fPIC was incorrectly removed in 526f076.
As the rocksdb static library is dynamicly linked to the rocksdb storage engine, position independent code is required.