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.

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

Conversation

lrineau
Copy link
Contributor

@lrineau lrineau commented Jun 13, 2022

Describe the pull request

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

You have modified or added at least one vcpkg.json where you should check the license field.

If you feel able to do so, please consider adding a "license" field to the following files:

  • ports/gmp/vcpkg.json

Valid values for the license field can be found in the documentation

@BillyONeal
Copy link
Member

I added a line to the description describing why this isn't the typical "the toolchain should handle that" case :).

@@ -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:

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:

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

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 )

@JonLiu1993 JonLiu1993 added the category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist label Jun 14, 2022
@JonLiu1993 JonLiu1993 changed the title [gmp] [mpir] Add --with-pic [gmp | mpir] Add --with-pic Jun 14, 2022
@JonLiu1993 JonLiu1993 added the info:reviewed Pull Request changes follow basic guidelines label Jun 15, 2022
@vicroms vicroms merged commit d4cd43e into microsoft:master Jun 15, 2022
@lrineau lrineau deleted the pic-for-gmp-and-mpir branch June 16, 2022 08:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[MPIR:x64-linux] build failure (static linking)
5 participants