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

ExtUtils-CBuilder: Remove image-base generation on Win32/gcc #18494

Merged

Conversation

zmughal
Copy link
Contributor

@zmughal zmughal commented Jan 19, 2021

Switches from generating an image-base address using the basename of the
output file to using GCC's built-in --enable-auto-image-base linker
option.

This aligns the linking behaviour for Win32/gcc with that of
ExtUtils-MakeMaker which removed image-base generation in commit
Perl-Toolchain-Gang/ExtUtils-MakeMaker@343d21a.

CC: @ambs, @Leont, @plicease

Connects with: PerlAlien/Alien-Build#243 (comment)

@xenu
Copy link
Member

xenu commented Jan 19, 2021

This is a reasonable change, +1.

In the future we should switch to --high-entropy-va to make ASLR work, but that would be a bigger patch. We would have to detect whether the linker supports the flag and then put in $Config{lddconfig} or somewhere.

@zmughal zmughal force-pushed the extutils-cbuilder-win32-gcc-auto-image-base branch from 6300e79 to 7528e36 Compare January 19, 2021 08:06
@zmughal
Copy link
Contributor Author

zmughal commented Jan 20, 2021

I just wanted to note that under MSYS2/MINGW64, the default build options use ASLR in /etc/makepkg_mingw64.conf:

LDFLAGS="-pipe -Wl,--dynamicbase,--high-entropy-va,--nxcompat,--default-image-base-high"

More discussion on this for MINGW at msys2/MINGW-packages#6674. However the mingw-w64-x86_64-perl package's Config.pm does not have these flags.

@ambs
Copy link
Contributor

ambs commented Jan 26, 2021

Please let me know when/if it gets merged, so I can prepare the CPAN release of the module.

@jkeenan jkeenan added distro-mswin32 Win32 This is a meta-ticket to tag issues in the perl core which need attention on Win32. See #11925 labels Jan 30, 2021
@jkeenan
Copy link
Contributor

jkeenan commented Jan 30, 2021

To facilitate evaluation of this pull request, I have created the following smoke-me branch:
smoke-me/jkeenan/zmughal-extutils-cbuilder-win32-gcc-auto-image-base

Could we get people with access to Win32 to test this, preferably with gcc and other compilers?

Thank you very much.
Jim Keenan

Switches from generating an image-base address using the basename of the
output file to using GCC's built-in `--enable-auto-image-base` linker
option.

This aligns the linking behaviour for Win32/gcc with that of
ExtUtils-MakeMaker which removed image-base generation in commit
<Perl-Toolchain-Gang/ExtUtils-MakeMaker@343d21a>.
@xenu xenu force-pushed the extutils-cbuilder-win32-gcc-auto-image-base branch from 7528e36 to e4c6153 Compare February 12, 2021 21:51
@xenu xenu merged commit 4371f64 into Perl:blead Feb 12, 2021
@xenu
Copy link
Member

xenu commented Feb 12, 2021

@ambs it is merged now.

@ambs
Copy link
Contributor

ambs commented Feb 12, 2021

Thank you. Will release EU::CB tomorrow.

@ambs
Copy link
Contributor

ambs commented Feb 13, 2021

Done. Released with the same version it is on Blead. Hope that isn't a problem. I can't recall if that can cause problems.

@zmughal zmughal deleted the extutils-cbuilder-win32-gcc-auto-image-base branch November 21, 2021 06:26
@Leont
Copy link
Contributor

Leont commented Sep 2, 2023

I'm not sure this change was complete, really. We're currently doing this bizarre dlltool;ld; dlltool; ld dance that should not be necessary anymore, AFAIK we can just do one ld now that we have --enable-auto-image-base.

And it appears that not doing the simple thing has a unfortunate interaction with antivirus scanners Leont/magic-check#1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
distro-mswin32 Win32 This is a meta-ticket to tag issues in the perl core which need attention on Win32. See #11925
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants