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

[vcpkg-tool] Add -DPIC to pic flags on Linux #25158

Closed
wants to merge 1 commit into from

Conversation

lrineau
Copy link
Contributor

@lrineau lrineau commented Jun 9, 2022

Describe the pull request

Because /usr/bin/libtool has that line of code:

pic_flag=" -fPIC -DPIC"

That macro definition PIC can trigger different assembler code, as for example in GMP assembler code.

*Note: I think the same bug applies to the other toolchains using -fPIC:

scripts/toolchains/android.cmake
scripts/toolchains/freebsd.cmake
scripts/toolchains/ios.cmake
scripts/toolchains/openbsd.cmake
scripts/toolchains/linux.cmake
scripts/toolchains/osx.cmake

but I can test only on Linux. Should I commit a similar change in all those files, and let the CI scripts and others test locally? In my opinion, I hardly see how adding -DPIC can harm.

Because `/usr/bin/libtool` has that line of code:
```shell
pic_flag=" -fPIC -DPIC"
```

That macro definition `PIC` can trigger different assembler code,
as for example in GMP assembler code.
@Neumann-A
Copy link
Contributor

Rebuild gmp with #25009 merged in. An retry the issue in #13827. Currently the linker flags are not correctly passed if i remember correctly.

According to docs. -fPIC defines __PIC__ so upstream should check against that and not a arbitrary define added by libtool.

@lrineau
Copy link
Contributor Author

lrineau commented Jun 9, 2022

Rebuild gmp with #25009 merged in. An retry the issue in #13827. Currently the linker flags are not correctly passed if i remember correctly.

I have tried to merge #25009 in master but I still had the issue #13827. Only the define of -DPIC fixes it.

According to docs. -fPIC defines __PIC__ so upstream should check against that and not a arbitrary define added by libtool.

Do you mean that this PR will not be accepted? The upstream GMP project documents that --with-pic should be added to ./configure. But @BillyONeal said in #13827 (comment) that the enabling of PIC should be from the toolchain, and not from the GMP port file. That is why I have created this PR (#25158).

@JonLiu1993 JonLiu1993 added the category:tool-update The issue is with build tool or build script, which requires update or should be executed correctly label Jun 10, 2022
@JonLiu1993
Copy link
Member

@lrineau, Thanks for your pr, looks ci test failed on x64-linux triplet
This is error log, please take a look:
llvm:
install-x64-linux-dbg-out.log
libmikmod:
install-x64-linux-dbg-out.log

@JonLiu1993 JonLiu1993 changed the title Add -DPIC to pic flags on Linux [vcpkg-tool] Add -DPIC to pic flags on Linux Jun 13, 2022
@lrineau
Copy link
Contributor Author

lrineau commented Jun 13, 2022

@lrineau, Thanks for your pr, looks ci test failed on x64-linux triplet
This is error log, please take a look:
llvm:
install-x64-linux-dbg-out.log
libmikmod:
install-x64-linux-dbg-out.log

For libmikmod, I do not understand the error, and how it might be related to PIC being a macro.

As for llvm, there is an understandable issue: PIC is used as a symbol name in the code:

llvm/include/llvm/MC/MCObjectFileInfo.h:237:52: note: in expansion of macro ‘PIC’
  237 |   void initMCObjectFileInfo(MCContext &MCCtx, bool PIC,
      |                                                    ^~~

That means PIC cannot be defined by the toolchain. I close this PR, and will try again to act on the gmp port itself.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:tool-update The issue is with build tool or build script, which requires update or should be executed correctly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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