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

2941 - [Backport] Fixing FFIConstantHandleType externalType wrong in 64 bit #2945

Merged

Conversation

@VincentBlondeau
Copy link
Contributor

commented Mar 20, 2019

Getting inspired by the nice code that was written in squeak monticello repository.

I.e.:

FFI-Kernel-eem.57
Time: 6 November 2018, 6:39:32 pm

Provide the longLong to complement the set of other integer conveniences.

FFI-Kernel-eem.55
Time: 23 August 2018, 2:12:03 pm

Add an errorCode to ExternalFunction>>invokeWithArguments:. At least for now the design for the catching of exceptions during FFI calls has failing the callout on exception enabled by the callout having an error code.

FFI-Kernel-nice.52
Time: 29 April 2018, 6:39:52 pm

Fix pointerSpec for 64bits FFI
A pointer has a Smalltalk wordSize byte-length, rather than hardcoded 4 byte length.

FFI-Kernel-nice.49
Time: 13 April 2018, 3:56:28 pm

Give a chance to recompile the structure compiledSpec when resuming on a different platform. See ExternalStructure class>>#install.

FFI-Kernel-nice.48
Time: 13 April 2018, 8:00:08 am

Correct a bug for 64bits pointer arithmetic: unsignedLongAt: and unsignedLongAt:put: are not machine dependent, they allways fetch/store an uint32_t

No need to invoke self class wordSize, when self size already encodes the same information.

Getting inspired by the nice code that was written in squeak monticel…
…lo repository.

I.e.:

FFI-Kernel-eem.57
Time: 6 November 2018, 6:39:32 pm

Provide the longLong to complement the set of other integer conveniences.

FFI-Kernel-eem.55
Time: 23 August 2018, 2:12:03 pm

Add an errorCode to ExternalFunction>>invokeWithArguments:.  At least for now the design for the catching of exceptions during FFI calls has failing the callout on exception enabled by the callout having an error code.

FFI-Kernel-nice.52
Time: 29 April 2018, 6:39:52 pm

Fix pointerSpec for 64bits FFI
A pointer has a Smalltalk wordSize byte-length, rather than hardcoded 4 byte length.

FFI-Kernel-nice.49
Time: 13 April 2018, 3:56:28 pm

Give a chance to recompile the structure compiledSpec when resuming on a different platform. See ExternalStructure class>>#install.

FFI-Kernel-nice.48
Time: 13 April 2018, 8:00:08 am

Correct a bug for 64bits pointer arithmetic: unsignedLongAt: and unsignedLongAt:put: are not machine dependent, they allways fetch/store an uint32_t

No need to invoke self class wordSize, when self size already encodes the same information.

@Ducasse Ducasse requested review from tesonep and guillep Mar 21, 2019

@Ducasse

This comment has been minimized.

Copy link
Member

commented Mar 21, 2019

Thanks a lot Vincent. I know that guille and pablo got some fixes for UFFI.

@guillep
Copy link
Member

left a comment

Overall, harvesting the changes is good, but still I do not understand how they actually fix the original bug with FFIConstantHandleType. I am reading now

FFIConstantHandleType >> externalType
	^ ExternalType ulong

And I wonder: in windows HANDLEs are just pointers, why not redefine that as

FFIConstantHandleType >> externalType
	^ ExternalType void asPointer

?

That would make much much clearer the intention.
In any case, making that change without harvesting the changes seems futile because this seems buggy in the current version. In 64 bits I get:

ExternalType void asPointerType byteSize => 4

!!!

bytes unsignedLongAt: 1 put: (bytes unsignedLongAt: 1) + offset.
self size = 4
ifTrue: [bytes unsignedLongAt: 1 put: (bytes unsignedLongAt: 1) + offset]
ifFalse: [bytes unsignedLongLongAt: 1 put: (bytes unsignedLongLongAt: 1) + offset].

This comment has been minimized.

Copy link
@guillep

guillep Mar 22, 2019

Member

Why writing this by hand like that and not reusing pointerAt:put:?

This comment has been minimized.

Copy link
@guillep

guillep Mar 22, 2019

Member

It's not clear to us that size will return the actual size of a word for an address...
Alternatively, there is also platformLongAt:put: that uses the platform size...

@estebanlm

This comment has been minimized.

Copy link
Member

commented Mar 25, 2019

I commented here: #2940 (review)
In the same sense of @guillep: correct fix is to make FFIConstantHandleType be a pointer instead a number.

@estebanlm

This comment has been minimized.

Copy link
Member

commented Mar 25, 2019

but, now that I remember there was a reason why it was not just a pointer, something about how windows works with this: even if it is a pointer, it is represented as a number.
It still needs to be represented as a number.
@astares I implemented ConstantHandle because you needed it, do you remember why it needs to be a number?

@StephanEggermont

This comment has been minimized.

Copy link
Contributor

commented Mar 26, 2019

This is the back port. Seems to be the wrong place to comment

@VincentBlondeau VincentBlondeau changed the title 2941 - [Backport] Fixing FFIConstantHandleType externalType wrong in 64 bit [WaitingForIntegrationOnPharo8.0]2941 - [Backport] Fixing FFIConstantHandleType externalType wrong in 64 bit Mar 26, 2019

@stale

This comment has been minimized.

Copy link

commented Apr 15, 2019

It is hard to review old PRs so this PR has been marked as stale since there has been no activity the last 20 days. It will be closed in 10 days if no further activity occurs. A simple comment will reactivate the PR, but please also consider updating the PR to the latest SNAPSHOT build to make it easier for reviewers.

@stale stale bot added the stale label Apr 15, 2019

@VincentBlondeau

This comment has been minimized.

Copy link
Contributor Author

commented Apr 15, 2019

not in stale

@stale stale bot removed the stale label Apr 15, 2019

@StephanEggermont

This comment has been minimized.

Copy link
Contributor

commented Apr 15, 2019

I presume the pointer is compared with 0 or -1 or so

@astares

This comment has been minimized.

Copy link
Member

commented Apr 15, 2019

but, now that I remember there was a reason why it was not just a pointer, something about how windows works with this: even if it is a pointer, it is represented as a number.
It still needs to be represented as a number.
@astares I implemented ConstantHandle because you needed it, do you remember why it needs to be a number?

@estebanlm , @guillep

I required it to wrap windows registry functionality in "WinRegistryKey" (a subclass of WinHandle) as there are predefined fixed constants handles like HKEY_LOCAL_MACHINE, HKEY_CURRENT_USER, HKEY_CURRENT_CONFIG, ...

Just load "OSWindows" from Catalog and browse users of FFIConstantHandleType.

asExternalTypeOn: generator
	^ FFIConstantHandleType objectClass: self

The constants itself are initialized in a shared pool (class WinRegistryConstants):

initRegistryConstants

	HKEY_CLASSES_ROOT := 16r80000000.
	HKEY_CURRENT_USER := 16r80000001.
	HKEY_LOCAL_MACHINE := 16r80000002.
	HKEY_USERS := 16r80000003.
        ...

The WinRegistryKey class is then used to define API types like "HKEY" or "PHKEY" which are common in Windows for some API definitions:

initRegistryTypes
        HKEY := #WinRegistryKey.
	PHKEY := 'HKEY*'

to use/wrap Win32 APIs like:

regOpenKeyExA: hKey with: lpSubKey with: ulOptions with: samDesired with: phkResult
 
	^ self ffiCall: #(
		long RegOpenKeyExA (
			HKEY hKey, 
			LPTSTR lpSubKey, 
			long ulOptions, 
			long samDesired, 
			PHKEY phkResult)) module: #advapi32 

Note that my "OSWindows" code is covered with many unit tests (to also help Esteban implement UFFI and see when it breaks). So whatever you change - try to check that "OSWindows" works afterwards.

@stale

This comment has been minimized.

Copy link

commented May 12, 2019

It is hard to review old PRs so this PR has been marked as stale since there has been no activity the last 20 days. It will be closed in 10 days if no further activity occurs. A simple comment will reactivate the PR, but please also consider updating the PR to the latest SNAPSHOT build to make it easier for reviewers.

@stale stale bot added the stale label May 12, 2019

@VincentBlondeau

This comment has been minimized.

Copy link
Contributor Author

commented May 12, 2019

pong

@stale stale bot removed the stale label May 12, 2019

@VincentBlondeau VincentBlondeau changed the title [WaitingForIntegrationOnPharo8.0]2941 - [Backport] Fixing FFIConstantHandleType externalType wrong in 64 bit 2941 - [Backport] Fixing FFIConstantHandleType externalType wrong in 64 bit May 13, 2019

Vincent Blondeau
@VincentBlondeau

This comment has been minimized.

Copy link
Contributor Author

commented May 13, 2019

Changes are sync with the Pharo8 version

@estebanlm estebanlm closed this May 23, 2019

@estebanlm estebanlm reopened this May 23, 2019

@estebanlm

This comment has been minimized.

Copy link
Member

commented May 23, 2019

let's see if we can merge :)

@stale

This comment has been minimized.

Copy link

commented Jun 12, 2019

It is hard to review old PRs so this PR has been marked as stale since there has been no activity the last 20 days. It will be closed in 10 days if no further activity occurs. A simple comment will reactivate the PR, but please also consider updating the PR to the latest SNAPSHOT build to make it easier for reviewers.

@stale stale bot added the stale label Jun 12, 2019

@VincentBlondeau

This comment has been minimized.

Copy link
Contributor Author

commented Jun 12, 2019

Can we?

@stale stale bot removed the stale label Jun 12, 2019

@estebanlm estebanlm merged commit b84a1e1 into pharo-project:Pharo7.0 Jun 14, 2019

1 of 3 checks passed

continuous-integration/jenkins/pr-merge This commit has test failures
Details
probot/minimum-reviews Pending review approvals
WIP Ready for review
Details

@VincentBlondeau VincentBlondeau deleted the VincentBlondeau:29418-ffi-patch-backport branch Jul 2, 2019

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