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

Update 64 bit types #5

Merged
merged 2 commits into from Oct 21, 2014

Conversation

Projects
None yet
2 participants
@elsabio
Copy link
Contributor

commented Oct 21, 2014

The minimum changes to correct the SAMCLI.DLL crashes are now committed to this branch.

elsabio added some commits Oct 21, 2014

Use DWORD_PTR for resumeHandle
Match declared type for resumeHandle parameter to fix crashes on 64-bit
systems in SAMCLI.DLL.

@jandubois jandubois merged commit a3a9884 into perl-libwin32:master Oct 21, 2014

@jandubois

This comment has been minimized.

Copy link
Member

commented Oct 21, 2014

I'm not sure why you only changed some of the resumeHandle variables to the DWORD_PTR type; I believe they all need to be changed. I've done this in a followup commit. Please let me know if you think this is not correct!

@elsabio

This comment has been minimized.

Copy link
Contributor Author

commented Oct 22, 2014

I believe that also, but I only tested the changes I proposed, expecting to have this very discussion on the merits of changing the rest :-). If you have test cases that exercise them all on a 64 bit Perl (and for regression, 32 bit, and also prior to 5.14) and it passes with all the variables changed in this way, then my confidence would be improved!

The MS published Win32 API, and the lmaccess.h header from the Perl distribution(s) are not 100% consistent in the parameter naming (not relevant except in that searching is less certain) and the types used (which might be).

@jandubois

This comment has been minimized.

Copy link
Member

commented Oct 22, 2014

On Tue, Oct 21, 2014 at 5:19 PM, elsabio notifications@github.com wrote:

I believe that also, but I only tested the changes I proposed, expecting to have this very discussion on the merits of changing the rest :-). If you have test cases that exercise them all on a 64 bit Perl (and for regression, 32 bit, and also prior to 5.14) and it passes with all the variables changed in this way, then my confidence would be improved!

I don't have any test cases, and no time to create any. :(

However, changing a DWORD to a DWORD_PTR just increases the amount of
memory allocated to them on Win64. Since they are always passed by
pointer, the worst that can happen is that only the first 4 bytes will
be used and the rest ignored. So the code should be correct either
way.

I see that there is also one instance of a DWORD resumeHandle left in
Win32API::Net. I will change that one as well, and re-release it.

Cheers,
-Jan

@elsabio

This comment has been minimized.

Copy link
Contributor Author

commented Oct 22, 2014

Knowing that the stack is only ever contaning pointers to space for parameters - a fact I was unsure of - makes this a safe change, I agree.

Thank you for your time and attention responding to this issue and reviewing my suggested fix.

@elsabio elsabio deleted the elsabio:update-64-bit-types branch Oct 22, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.