Skip to content

MDEV-35462: fix - compiler flags for RocksDB still need PIC#5217

Open
grooverdan wants to merge 1 commit into
MariaDB:13.0from
grooverdan:MDEV-35462_fix
Open

MDEV-35462: fix - compiler flags for RocksDB still need PIC#5217
grooverdan wants to merge 1 commit into
MariaDB:13.0from
grooverdan:MDEV-35462_fix

Conversation

@grooverdan

Copy link
Copy Markdown
Member

-fPIC was incorrectly removed in 526f076.

As the rocksdb static library is dynamicly linked to the rocksdb storage engine, position independent code is required.

@grooverdan grooverdan added the MariaDB Foundation Pull requests created by MariaDB Foundation label Jun 10, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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 vaintroub left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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 grooverdan force-pushed the MDEV-35462_fix branch 4 times, most recently from e00a6f8 to bf360e1 Compare June 12, 2026 03:42
@grooverdan grooverdan changed the title MDEV-35462: rocksdb required PIC build MDEV-35462: Cleanup PIC Jun 12, 2026
@grooverdan grooverdan requested a review from vaintroub June 12, 2026 04:28

@vaintroub vaintroub left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

…Lists.txt (fix)

-fPIC was erronously removed in 526f076. Replaced with the
POSITION_INDEPENDENT_CODE target property.
@grooverdan grooverdan changed the title MDEV-35462: Cleanup PIC MDEV-35462: fix - compiler flags for RocksDB still need PIC Jun 17, 2026
@grooverdan

Copy link
Copy Markdown
Member Author

Leaving the full cleanup of PIC on #5250.

For now, lets just get the build working again (aocc worker highlighted the problem)

@vaintroub vaintroub self-requested a review June 17, 2026 08:27

@vaintroub vaintroub left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

For the purpose of fixing the build, OK

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

MariaDB Foundation Pull requests created by MariaDB Foundation

Development

Successfully merging this pull request may close these issues.

2 participants