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

Change TIdSocketListVCLPosix to use poll instead of select #473

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

msccto
Copy link

@msccto msccto commented Apr 26, 2023

fix for #445

I replaced TIdSocketListVCLPosix with the code you suggested in the #445

@msccto
Copy link
Author

msccto commented Oct 5, 2023

Hi,
is there any chance this PR is going to be merged?

thx
Matt

@rlebeau rlebeau self-assigned this Jan 11, 2024
@rlebeau rlebeau added Type: Enhancement Issue is proposing a new feature/enhancement Element: Socket Stacks Issues related to OS socket APIs, TIdStack and TIdSocketList descedants, etc Element: Operating System Issues related to specific Operating Systems labels Jan 11, 2024
Copy link

Choose a reason for hiding this comment

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

nfds is defined as nfds_t, which is unsigned long int, and that's UInt64, not integer

Copy link
Member

Choose a reason for hiding this comment

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

Technically, nfds_t is simply defined as an "unsigned integer". Some platforms do use unsigned long, but there are others that use unsigned int instead.

Copy link

Choose a reason for hiding this comment

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

Just as i said t's defined in glibc as 'typedef unsigned long int nfds_t', but i do agree that there may be other definitions on different platforms, so i'd suggest using nativeuint here instead of integer

Copy link
Member

Choose a reason for hiding this comment

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

NativeUInt probably would not be appropriate either, because of the platforms that use unsigned int which doesn't change size between 32bit/64bit builds. There really needs to be separate platform-appropriate nfds_t definitions.

Copy link

@zTorden zTorden Jan 25, 2024

Choose a reason for hiding this comment

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

you're right, and this is actually the case with glibc where it is always unsigned long int both on 32 and 64 bit. still maybe unsigned long int itself differs on 32bit, can't really tell. but on ubuntu 22.04 which is always 64bit it's UInt64, i've checked that. but who knows, maybe it's only valid for gcc and not some other compiler, or is dependent on some 'compatibility' compiler options.

Copy link

Choose a reason for hiding this comment

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

as for the de-facto standard System V AMD64 calling convention implemented on most (if not all) *nixes it's safe to be defined as integer (at least when there are less then 2^32 fds allocated), because even when it is passed via EDI register, the higher bits of RDI are being zeroed rendering it a valid 64bit value. e.g. after these operations

mov rdi, $ffffffffffffffff
mov edi, 0

the value of rdi would be 0 and not $ffffffff00000000

Copy link

Choose a reason for hiding this comment

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

but this is only valid for register-passed parameters (i.e. first 6 function parameters), if it's passed via stack (7+ parameter), or as a part of structure, or referenced by pointer it will cause trouble

Copy link
Member

Choose a reason for hiding this comment

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

In general, yes. But that should not be a problem in THIS case since poll() has only 3 parameters and they are all simply types (1 pointer and 2 integers). The 1st parameter is a pointer to a struct, but that does not affect the nfds_t parameter in question.

@rlebeau rlebeau added the Status: In Progress Issue is being worked on label Oct 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Element: Operating System Issues related to specific Operating Systems Element: Socket Stacks Issues related to OS socket APIs, TIdStack and TIdSocketList descedants, etc Status: In Progress Issue is being worked on Type: Enhancement Issue is proposing a new feature/enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants