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

vim not able to build with Perl >=5.37 #21164

Closed
pheiduck opened this issue Jun 21, 2023 · 14 comments
Closed

vim not able to build with Perl >=5.37 #21164

pheiduck opened this issue Jun 21, 2023 · 14 comments

Comments

@pheiduck
Copy link

pheiduck commented Jun 21, 2023

Description
Not possible to build vim with perl >= 5.37

Complete log coulg be found on <https://jplesnik.fedorapeople.org/5.38/vim-build.log>
Examples from log: 

if_perl.xs: In function 'Perl_SvTRUE_common': 
/usr/lib64/perl5/CORE/embed.h:109:49: warning: implicit declaration of function 'Perl_SvPVXtrue'; did you mean 'Perl_sv_true'? [-Wimplicit-function-declaration]
  109 | # define SvPVXtrue(a)                           Perl_SvPVXtrue(aTHX_ a)
      |                                                 ^~~~~~~~~~~~~~
if_perl.xs:724:16: note: in expansion of macro 'SvPVXtrue'
  724 |         return SvPVXtrue(sv);
      |                ^~~~~~~~~
if_perl.xs: In function 'Perl_SvTRUE':
<...>
/usr/bin/ld: /tmp/ccxXLZZi.ltrans63.ltrans.o: in function `ex_perl':
/builddir/build/BUILD/vim90/src/if_perl.xs:745: undefined reference to `Perl_SvGETMAGIC'
/usr/bin/ld: /builddir/build/BUILD/vim90/src/if_perl.xs:1108: undefined reference to `Perl_SvPV_helper'
/usr/bin/ld: /builddir/build/BUILD/vim90/src/if_perl.xs:724: undefined reference to `Perl_SvPVXtrue'
/usr/bin/ld: /tmp/ccxXLZZi.ltrans63.ltrans.o: in function `perl_to_vim.lto_priv.0':
/builddir/build/BUILD/vim90/src/if_perl.xs:1210: undefined reference to `Perl_SvPV_helper'
/usr/bin/ld: /builddir/build/BUILD/vim90/src/if_perl.xs:1323: undefined reference to `Perl_SvPV_helper'
/usr/bin/ld: /builddir/build/BUILD/vim90/src/if_perl.xs:1199: undefined reference to `Perl_SvNV'
/usr/bin/ld: /builddir/build/BUILD/vim90/src/if_perl.xs:1203: undefined reference to `Perl_SvIV'
/usr/bin/ld: /tmp/ccxXLZZi.ltrans64.ltrans.o: in function `XS_VIM_DoCommand':
/builddir/build/BUILD/vim90/src/if_perl.c:1789: undefined reference to `Perl_SvPV_helper'
/usr/bin/ld: /tmp/ccxXLZZi.ltrans64.ltrans.o: in function `ex_perldo':
/builddir/build/BUILD/vim90/src/if_perl.xs:1433: undefined reference to `Perl_SvPV_helper'

Steps to Reproduce

wget -O perl.tar.gz https://cpan.metacpan.org/authors/id/S/SH/SHAY/perl-5.37.11.tar.gz
tar -xaf perl.tar.gz && cd perl-5.37.11
<install make and gcc>
sh Configure
make && sudo make install

git clone https://github.com/vim/vim
cd vim
./configure --enable-gui=no --with-features=Huge --enable-perlinterp=dynamic
make

Expected behavior
no build errors by Perl

Perl configuration

# perl -V 5.37.11
@jkeenan
Copy link
Contributor

jkeenan commented Jun 21, 2023

In the posted log file, the points in Vim's build where built-time warnings related to perl code (their if_perl.xs) appear are related to these commits in the Perl core distribution during the current development cycle:

2356f8b

commit 2356f8bb5077e29623007d815dab58f43473a56d
Author:     Richard Leach <rich+perl@hyphen-dash-hyphen.info>
AuthorDate: Sat May 28 13:47:10 2022 -0600
Commit:     Karl Williamson <khw@cpan.org>
CommitDate: Sat May 28 13:51:10 2022 -0600

    Move sv.h functions from inline.h into sv_inline.h,

a5dfa56

commit a5dfa5631f10e72f300025bdd0649c0f05fe52e8
Author:     Yves Orton <demerphq@gmail.com>
AuthorDate: Sat Dec 17 16:39:54 2022 +0100
Commit:     Yves Orton <demerphq@gmail.com>
CommitDate: Sat Dec 24 01:17:22 2022 +0100

    embed.h - make regen after recent changes

While I suspect that the fix should be done on the Vim side, it would be good for our devs to take a look at this. @demerphq @richardleach

This was reported on the vim-dev list at:

https://groups.google.com/g/vim_dev/c/4zPpgHznm8k

... and on Vim's issue tracker here:

vim/vim#12543

Thank you very much.
Jim Keenan

@xenu
Copy link
Member

xenu commented Jun 21, 2023

That if_perl.xs file looks extremely fragile, I don't understand why it redefines so many Perl functions.

Anyway, the root cause is that we have a new header, sv_inline.h, but if_perl.xs defines PERL_NO_INLINE_FUNCTIONS which prevents it from being included. It also suppresses inline.h, but apparently if_perl.xs works around that by copying functions from that header.

@pheiduck
Copy link
Author

Just to note, I am worked on a Patch on the vim side so for now its working from 5.36 until 5.38.
Without the dynamic option. 🥳
Best regards,
Philip Heiduck

@jkeenan
Copy link
Contributor

jkeenan commented Jul 11, 2023

Just to note, I am worked on a Patch on the vim side so for now its working from 5.36 until 5.38. Without the dynamic option. partying_face Best regards, Philip Heiduck

This looks promising. Please keep us informed of the developments in that vim pull request.

@pheiduck
Copy link
Author

pheiduck commented Jul 11, 2023

This looks promising. Please keep us informed of the developments in that vim pull request.

The developments are done on this PR, I wait until the maintainer will pull it into the main branch.
At least I got some feedback from fedora they have tested it successfully. :) Arch Linux will apply the Patch when they have pulled the latested Perl version, which take some time because of the packages which depend on them.

@pheiduck
Copy link
Author

more improvements are suggested by other vim developers, so it's not really done :D

@hyder365
Copy link

hyder365 commented Aug 9, 2023

vim/vim@1d7caa5

@pheiduck pheiduck closed this as completed Aug 9, 2023
@ychin
Copy link

ychin commented Aug 24, 2023

That if_perl.xs file looks extremely fragile, I don't understand why it redefines so many Perl functions.

I have been looking at this issue as I think the Vim fixes didn't account for all Perl versions (e.g. it doesn't work on Perl 5.30). Looking at this history though, I think the reason why the if_perl.xs is so fragile is because of a lack of a better way to do things.

Vim supports embedding Perl either via static or dynamic linking (meaning that the binary has no dependency on Perl at all and only loads it in dynamically at runtime via dlopen as needed). I think with the way the perl.h header is structured, there is no clean way to do it without such a big hack.

If you just include perl.h as-is, Vim's code uses S_SvREFCNT_inc from the inline.h file which ends up linking against Perl_sv_free2(), which doesn't exist because we are explicitly trying to not link against the Perl lib statically. A previous solution was to define PERL_NO_INLINE_FUNCTIONS, and then include inline.h later, after we manually define our own Perl_sv_free2 (which is a function pointer to a dynamically loaded function); but that caused issues when Perl 5.22.2 came out because a lot of assert macros are now only defined in proto.h when not using PERL_NO_INLINE_FUNCTIONS, and as a result you can't just include inline.h later as those needed macros are now undefined, see vim/vim#790. This led to the current solution of manually copy-pasting a bunch of Perl inline functions.

I think one solution would be to first define Perl_sv_free2 before including perl.h itself, but it's somewhat difficult to do because it currenty looks like this in Vim's if_perl.xs file:

/* Perl-5.18 has a different Perl_sv_free2 signature. */
#  if (PERL_REVISION == 5) && (PERL_VERSION >= 18)
static void (*Perl_sv_free2)(pTHX_ SV*, const U32);
#  else
static void (*Perl_sv_free2)(pTHX_ SV*);
#  endif

We need PERL_VERSION, and the definitions of "pTHX_ SV" to be able to define this properly. Those are only available after including perl.h, so you have a chick-and-egg problem here.

@xenu Is there a better way to do this? Looking at the code history I think this was the only way that actually works if you want to link against Perl completely dynamically. Vim needs this functionality because we need to build Vim in a way that allows for Perl functionality as needed, but doesn't force users to install Perl if they don't have it installed.

@ant0sha
Copy link

ant0sha commented Aug 25, 2023

silly question - will that work: create intermediate .so, statically linked to perl, which is dynamically loaded from vim?

@ychin
Copy link

ychin commented Aug 25, 2023

Actually, I think I found a simpler way. Basically the issue here is that inline.h is trying to link against random functions like Perl_sv_free2 that won't exist if we are dynamically loading a library (since we won't be linking to it during build time). We can just create stubs like this in our code base like this and avoids having to define PERL_NO_INLINE_FUNCTIONS (dll_Perl_sv_free2 is a function pointer to the dynamically loaded function):

void Perl_sv_free2(pTHX_ SV* sv, const U32 refcnt)
{
    (*dll_Perl_sv_free2)(aTHX_ sv, refcnt);
}

@Leont
Copy link
Contributor

Leont commented Aug 25, 2023

silly question - will that work: create intermediate .so, statically linked to perl, which is dynamically loaded from vim?

Yeah, that is the more conventional approach.

@ychin
Copy link

ychin commented Aug 25, 2023

silly question - will that work: create intermediate .so, statically linked to perl, which is dynamically loaded from vim?

I think the issue is we want people to just install their version of Perl (matched with the version compiled against Vim) and then just load in the lib dynamically. The user won't have to compile anything like a custom .so file. But I think the solution I said would work (by just manually stubbing functions like Perl_sv_free2 to help inline functions work).

@ant0sha
Copy link

ant0sha commented Aug 30, 2023 via email

@ychin
Copy link

ychin commented Sep 4, 2023

Why user? If at all - intermediate_perl_for_vim.so has to be build by vim project and distributed with vim. That way vim can dlopen("intermediate_perl_for_vim.so") instead of dlopen("libperl.so") directly...

Maybe stepping back, the issue here is really that if you include perl.h, a lot of utility functions uses a lot of inline functions from inline.h who reference functions like Perl_sv_free2 which the linker won't find, unless you are linking against a known Perl library. It kind of doesn't matter if you are using an intermediate library and whatnot. This will be an issue as long as you are using the dlopen() approach to load the library because the headers aren't designed to be worked that way as they want to link against known functions.

In the end the solution wasn't too hard (we removed the original method of copy-pasting inline functions from Perl headers which was really fragile): just stub the necessary functions and call the DLL ones (vim/vim#12914). It's just a necessary step. There's a caveat that when a new Perl version comes out sometimes we need to stub more functions since the inline implementation got changed to refer to more functions but it's a few lines of changes usually.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants