Skip to content

dist/IO.xs remove a dTHX call #23208

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

Open
wants to merge 1 commit into
base: blead
Choose a base branch
from
Open

Conversation

bulk88
Copy link
Contributor

@bulk88 bulk88 commented Apr 19, 2025

  • threaded perls without CPerlHost/iperlsys.h (ie all threaded unix perls) dont need a my_perl ptr, and don't assume all CCes have perfect LTO/LTCG and all OSes have a perfect designed bin image loader, to optimize away an unused my_perl var. This symbol crosses 2 separate TUs. ELF interposition, on paper, doesn't allow shifting over and dropping out args, but in real life, things are probably different. Write it correctly than assume optimizations will happen.

  • This set of changes requires a perldelta entry, and it is included.
  • This set of changes requires a perldelta entry, and I need help writing it.
  • This set of changes does not require a perldelta entry.

@@ -29,7 +29,14 @@
#ifdef poll
# undef poll
#endif
#define poll Perl_my_poll

#if defined(PERL_IMPLICIT_SYS)
Copy link
Contributor

Choose a reason for hiding this comment

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

These declarations should probably come after the definition of struct pollfd

@Leont
Copy link
Contributor

Leont commented Apr 19, 2025

This code should not even run on Mac. poll(2) seems to have been explicitly disabled because MacOS 10.4 broke poll(). 20 years ago, and no one ever looked at it again.

@mohawk2
Copy link
Contributor

mohawk2 commented Apr 19, 2025

It's not compiling on MacOS:

In file included from poll.c:17:
./poll.h:37:28: warning: declaration of 'struct pollfd' will not be visible outside of this function [-Wvisibility]
   int Perl_my_poll(struct pollfd *, unsigned long, int);
                           ^
poll.c:48:1: error: conflicting types for 'Perl_my_poll'
Perl_my_poll(struct pollfd *fds, unsigned long nfds, int timeout)
^
./poll.h:37:8: note: previous declaration is here
   int Perl_my_poll(struct pollfd *, unsigned long, int);
       ^

@bulk88
Copy link
Contributor Author

bulk88 commented Apr 20, 2025

I'm gonna revise this commit and repush it and see if I can fix it for OSX in the CI cloud w/o personal access to an OSX SDK. I will note, in current blead version of the code, there is no static tag (needed for good citizen reasons). And that is intentional since there are 2 TUs and its out of scope for me to do that plastic surgery to get rid of TU # 2.

@bulk88 bulk88 force-pushed the dist_IO_rmv_dthx branch from 6f1b059 to 31bb022 Compare April 20, 2025 20:01
@bulk88
Copy link
Contributor Author

bulk88 commented Apr 20, 2025

pushed rev # 2

@tonycoz
Copy link
Contributor

tonycoz commented Apr 21, 2025

This is incomplete, poll.c doesn't define PERL_NO_GET_CONTEXT, so the call to select() compiles to:

    err = ((*((((PerlInterpreter *)Perl_get_context())->ISock)))->pSelect)((((PerlInterpreter *)Perl_get_context())->ISock), n+1, (char*)&rfd, (char*)&wfd, (char*)&efd, tbuf);

which I doubt is the result you wanted.

@bulk88 bulk88 force-pushed the dist_IO_rmv_dthx branch from 31bb022 to 654b66e Compare April 22, 2025 01:11
- threaded perls without CPerlHost/iperlsys.h (ie all threaded unix perls)
  dont need a my_perl ptr, and don't assume all CCes have perfect LTO/LTCG
  and all OSes have a perfect designed bin image loader, to optimize away
  an unused my_perl var. This symbol crosses 2 separate TUs.
  ELF interposition, on paper, doesn't allow shifting over and dropping
  out args, but in real life, things are probably different. Write it
  correctly than assume optimizations will happen.
@bulk88 bulk88 force-pushed the dist_IO_rmv_dthx branch from 654b66e to cf383b2 Compare April 22, 2025 01:14
@bulk88
Copy link
Contributor Author

bulk88 commented Apr 22, 2025

This is incomplete, poll.c doesn't define PERL_NO_GET_CONTEXT

Good catch. That was a cherry pick or rebase git accident on my side at some point. The very 1st attempt (draft) had the PERL_NO_GET_CONTEXT line. The hunk from poll.c disappeared when the early WIP version of this commit got moved from a generic branch to this purpose-named branch for polishing and final submit.

On purpose, I am not going to overthink #if defined(PERL_IMPLICIT_SYS) vs any other possible substitute CPP macro test. I know Win32 is the only legit user of PERL_IMPLICIT_SYS and the only legit platform that will want the pTHX, but officially per the Perl C XS API, the feature test is #if defined(PERL_IMPLICIT_SYS) and nothing else. dist/IO/*.c.xs.h isn't the place to re-engineer any such build logic and it is a dual-life CPAN XS module on paper that must support ancient stable perls.

I am aware this PR opened a can of worms, of why on earth, Perl on OSX 15, needs to be emulating a basic POSIX syscall, but that topic is beyond scope for this PR. It will be someone elses PR and code, if they want to solve that topic. Just mentioning that OSX thing here in this PR for archive reasons if someone is researching or git blaming in the future.

pushed as rev # 3

@tonycoz
Copy link
Contributor

tonycoz commented Apr 22, 2025

I am aware this PR opened a can of worms, of why on earth, Perl on OSX 15, needs to be emulating a basic POSIX syscall, but that topic is beyond scope for this PR.

There's also WSAPoll(), but apparently that's also badly broken.

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 this pull request may close these issues.

4 participants