Skip to content

MDEV-40081: Remove outdated PIC explictness#5250

Draft
grooverdan wants to merge 1 commit into
MariaDB:mainfrom
grooverdan:MDEV-40081
Draft

MDEV-40081: Remove outdated PIC explictness#5250
grooverdan wants to merge 1 commit into
MariaDB:mainfrom
grooverdan:MDEV-40081

Conversation

@grooverdan

Copy link
Copy Markdown
Member

There where too many cases that forced PIC to be on.

CMake has a POSITION_INDEPENDENT_CODE attribute where platform requirements can be handled in a consistent manner.

ADD_CONVIENCE_FUNCTION sets the cmake property POSITION_INDEPENDENT_CODE for all except DISABLED_SHARED.

All other explict enables of PIC/pie have been removed.

  • 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.
  • Security hardened profiles can set -fPIC/pie directly as they are default options now.
  • WITH_PIC options. "Not of much use," - so its removed.

In MERGE_LIBRARIES, we check the PIC property directly rather than make assumptions based on the CFLAGS.

Bundled PCRE has explicit -DCMAKE_POSITION_INDEPENDENT_CODE=ON.

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

There where too many cases that forced PIC to be on.

CMake has a POSITION_INDEPENDENT_CODE attribute where platform
requirements can be handled in a consistent manner.

ADD_CONVIENCE_FUNCTION sets the cmake property POSITION_INDEPENDENT_CODE
for all except DISABLED_SHARED.

All other explict enables of PIC/pie have been removed.
* 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.
* Security hardened profiles can set -fPIC/pie directly as they are default options now.
* WITH_PIC options. "Not of much use," - so its removed.

In MERGE_LIBRARIES, we check the PIC property directly rather than
make assumptions based on the CFLAGS.

Bundled PCRE has explicit -DCMAKE_POSITION_INDEPENDENT_CODE=ON.

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

@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 refactors the CMake build configuration to replace custom WITH_PIC and _SKIP_PIC logic with CMake's native POSITION_INDEPENDENT_CODE target property. It also updates several plugins and the tpool library to support embedded server builds. The review feedback identifies two critical issues: first, removing the platform-skipping logic in cmake/libutils.cmake will cause build failures on Windows and macOS due to missing POSITION_INDEPENDENT_CODE properties; second, passing the OBJECT keyword to ADD_CONVENIENCE_LIBRARY in tpool/CMakeLists.txt is unsupported and will lead to a compilation error as CMake attempts to find a source file named 'OBJECT'.

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.

Comment thread cmake/libutils.cmake
Comment on lines +259 to +270
FOREACH(LIB ${LIBS})
GET_TARGET_PROPERTY(LTYPE ${LIB} TYPE)
IF(LTYPE STREQUAL "STATIC_LIBRARY")
GET_TARGET_PROPERTY(LIB_PIC ${LIB} POSITION_INDEPENDENT_CODE)
IF(NOT LIB_PIC)
MESSAGE(FATAL_ERROR
"Attempted to link non-PIC static library ${LIB} to shared library ${TARGET}\n"
"Please use ADD_CONVENIENCE_LIBRARY, instead of ADD_LIBRARY for ${LIB}"
)
ENDIF()
ENDFOREACH()
ENDIF()
ENDIF()
ENDFOREACH()

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

On platforms like Windows (where PIC is not used) or macOS (where PIC is always enabled by default), the POSITION_INDEPENDENT_CODE target property is typically not explicitly set.

By removing the platform-skipping logic (previously handled by _SKIP_PIC), this check will run on all platforms. On Windows and macOS, GET_TARGET_PROPERTY(LIB_PIC ...) will return NOTFOUND, causing IF(NOT LIB_PIC) to evaluate to true and trigger a FATAL_ERROR during the build.

To prevent build failures on these platforms, we should skip this check on Windows, Cygwin, macOS, or when shared libraries are disabled.

    IF(NOT WIN32 AND NOT CYGWIN AND NOT APPLE AND NOT DISABLE_SHARED)
      FOREACH(LIB ${LIBS})
        GET_TARGET_PROPERTY(LTYPE ${LIB} TYPE)
        IF(LTYPE STREQUAL "STATIC_LIBRARY")
          GET_TARGET_PROPERTY(LIB_PIC ${LIB} POSITION_INDEPENDENT_CODE)
          IF(NOT LIB_PIC)
            MESSAGE(FATAL_ERROR
            "Attempted to link non-PIC static library ${LIB} to shared library ${TARGET}\n"
            "Please use ADD_CONVENIENCE_LIBRARY, instead of ADD_LIBRARY for ${LIB}"
            )
          ENDIF()
        ENDIF()
      ENDFOREACH()
    ENDIF()

Comment thread tpool/CMakeLists.txt
ADD_LIBRARY(tpool_common OBJECT ${COMMON_SOURCES})
IF(WITH_EMBEDDED_SERVER)
ADD_CONVENIENCE_LIBRARY(tpool_embedded aio_linux.cc aio_simulated.cc)
ADD_CONVENIENCE_LIBRARY(tpool_common_embedded OBJECT ${COMMON_SOURCES})

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

The ADD_CONVENIENCE_LIBRARY macro hardcodes the creation of a STATIC library and does not support the OBJECT keyword.

Passing OBJECT as the second argument to ADD_CONVENIENCE_LIBRARY will cause CMake to treat "OBJECT" as a source file name rather than a library type. This will result in a build failure because CMake will look for a source file named OBJECT (e.g., OBJECT.cc or OBJECT.cpp) which does not exist.

The OBJECT keyword should be removed.

    ADD_CONVENIENCE_LIBRARY(tpool_common_embedded ${COMMON_SOURCES})

@gkodinov gkodinov added the MariaDB Foundation Pull requests created by MariaDB Foundation label Jun 17, 2026
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