Skip to content
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.

Sign up for GitHub

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

[gmp | mpir] Add --with-pic #25209

Merged
merged 2 commits into from
Jun 15, 2022
Merged

[gmp | mpir] Add --with-pic #25209

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e4411a7
[gmp] [mpir] Add --with-pic
lrineau Jun 13, 2022
0b783a7
./vcpkg x-add-version --all
lrineau Jun 13, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions ports/gmp/portfile.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ vcpkg_configure_make(
OPTIONS
${OPTIONS}
--enable-cxx
--with-pic
BillyONeal marked this conversation as resolved.
Show resolved Hide resolved
)

set(tool_names bases fac fib jacobitab psqr trialdivtab)
Expand Down
2 changes: 1 addition & 1 deletion ports/gmp/vcpkg.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
{
"name": "gmp",
"version": "6.2.1",
"port-version": 9,
"port-version": 10,
"description": "The GNU Multiple Precision Arithmetic Library",
"homepage": "https://gmplib.org",
"supports": "!(windows & (arm | arm64))",
Expand Down
6 changes: 3 additions & 3 deletions ports/mpir/portfile.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,12 @@ if(VCPKG_TARGET_IS_LINUX OR VCPKG_TARGET_IS_OSX)
vcpkg_find_acquire_program(YASM)

if(VCPKG_LIBRARY_LINKAGE STREQUAL "static")
set(SHARED_STATIC "--enable-static --disable-shared")
set(SHARED_STATIC --enable-static --disable-shared --with-pic)
Copy link
Member

Choose a reason for hiding this comment

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

Hmmm... usually it's dynamic libs that need PIC, not static. Are you sure you didn't flip the sense here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For non-Windows systems, dynamic libraries require position independent code (aka PIC). On Linux, vcpkg default toolchain, on x86_64, uses static libraries by default:

vcpkg/triplets/x64-linux.cmake

Lines 1 to 5 in 8e1f46d

set(VCPKG_TARGET_ARCHITECTURE x64)
set(VCPKG_CRT_LINKAGE dynamic)
set(VCPKG_LIBRARY_LINKAGE static)
set(VCPKG_CMAKE_SYSTEM_NAME Linux)

But toolchain adds -fPIC as well:

vcpkg/scripts/toolchains/linux.cmake

Lines 51 to 52 in 8e1f46d

string(APPEND CMAKE_C_FLAGS_INIT " -fPIC ${VCPKG_C_FLAGS} ")
string(APPEND CMAKE_CXX_FLAGS_INIT " -fPIC ${VCPKG_CXX_FLAGS} ")

That means that static libraries will be created, but with position independent code inside. That allows to link those static libraries to shared libraries.

The issue #13827 is a bug with GMP or MPIR, because GMP and MPIR have assembler code, to speed-up critical algorithms. That is why GMP and MPIR needs the macro PIC defined (with -DPIC, or with the option --with-pic to ./configure).

Copy link
Contributor Author

@lrineau lrineau Jun 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

PIC is already the default for shared libraries. We can add the option --with-pic:

  • only to static builds, like in MPIR port file,
  • or to any kind of build, like in GMP port file,

and that will have the same behavior: both static and dynamic builds will have PIC code.

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense 👍. Should we add a comment or something indicating that?

Copy link
Contributor

Choose a reason for hiding this comment

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

I was told that it is a concious decision that vcpkg always uses PIC, even for static triplets, to allow mixing of static and shared libs. It doesn't need to be documented per port. If there is a need to add a comment, it should be in a central location.

Copy link
Member

Choose a reason for hiding this comment

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

@dg0yt I don't mean that overall consistency decision, I mean something like:

# Add --with-pic even in static for consistency with vcpkg's toolchain.
# Note that this is necessary for GMP's `-DPIC` rather than `-fPIC`.

Copy link
Member

Choose a reason for hiding this comment

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

(To be clear, I'm not saying "please do this", I'm saying "have you considered doing this to stop people's eyebrows from being raised" @lrineau )

else()
set(SHARED_STATIC "--disable-static --enable-shared")
set(SHARED_STATIC --disable-static --enable-shared)
endif()

set(OPTIONS "--disable-silent-rules --enable-gmpcompat --enable-cxx ${SHARED_STATIC}")
set(OPTIONS --disable-silent-rules --enable-gmpcompat --enable-cxx ${SHARED_STATIC})

string(APPEND VCPKG_C_FLAGS " -Wno-implicit-function-declaration")
string(APPEND VCPKG_CXX_FLAGS " -Wno-implicit-function-declaration")
Expand Down
1 change: 1 addition & 0 deletions ports/mpir/vcpkg.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
{
"name": "mpir",
"version-date": "2022-03-02",
"port-version": 1,
"description": "Multiple Precision Integers and Rationals",
"homepage": "https://github.com/wbhart/mpir",
"license": "GPL-3.0-only",
Expand Down
4 changes: 2 additions & 2 deletions versions/baseline.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2606,7 +2606,7 @@
},
"gmp": {
"baseline": "6.2.1",
"port-version": 9
"port-version": 10
},
"gmsh": {
"baseline": "4.9.0",
Expand Down Expand Up @@ -4686,7 +4686,7 @@
},
"mpir": {
"baseline": "2022-03-02",
"port-version": 0
"port-version": 1
},
"mpmcqueue": {
"baseline": "2021-12-01",
Expand Down
5 changes: 5 additions & 0 deletions versions/g-/gmp.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,10 @@
{
"versions": [
{
"git-tree": "f4748213535c3fd004de44f6b1f15d123927cce6",
"version": "6.2.1",
"port-version": 10
},
{
"git-tree": "7b9a71843073bf4a86bb64ddf219c9900ebb3dbd",
"version": "6.2.1",
Expand Down
5 changes: 5 additions & 0 deletions versions/m-/mpir.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,10 @@
{
"versions": [
{
"git-tree": "9191f07cfaade82121abb4d37cb652182c0e55f6",
"version-date": "2022-03-02",
"port-version": 1
},
{
"git-tree": "5358d4a724061eab499969ae3b56f8abbdea3347",
"version-date": "2022-03-02",
Expand Down