On Wed, Mar 5, 2014 at 3:49 PM, Andrew Eikum aeikum@codeweavers.com wrote:
On Tue, Mar 04, 2014 at 08:19:16PM +0100, Elias Vanderstuyft wrote:
a) When unloading conditional effects in joy.cpl, the program crashes: When unloading the conditional effect array entries, Wine tries to access an invalid (high) address because dieffect.lpvTypeSpecificParams was not set at the time the effect was created. So declare a valid 'standard' (maxed out) conditional effect. For other unknown effects: - The same can happen when CustomForce effects are available, but because Wine's FF dinput->linux translation does not support this yet, we simply print a FIXME to indicate a possible cause if a crash would happen. - The same for other (unsupported) effects (this is probably not safe?), but then we print a WARN.
For unhandled types, it's probably best to print a FIXME and return DIENUM_CONTINUE, rather than continue to rely on uninitialized data.
Alright. Should I also write the extra if-test for the CustomForce type, or do I have to cover it with one if-test as an unhandled type?
Additionally, our IDirectInputEffect implementations could check for NULL lpvTypeSpecificParams and fail appropriately instead of crashing, if this is what Windows does.
Well, I checked the SetParameters() function, and it does correctly check on NULL lpvTypeSpecificParams and fail appropriately. The problem seems again to be related with joy.cpl/main.c: there is no check performed on the result of IDirectInputDevice2_CreateEffect(), so change joy.cpl/main.c:784 : hr = IDirectInputDevice2_CreateEffect( joystick->device, &pdei->guid, &dieffect, &joystick->effects[joystick->cur_effect].effect, NULL);
joystick->effects[joystick->cur_effect].params = dieffect; joystick->effects[joystick->cur_effect].info = *pdei; joystick->cur_effect += 1;
return DIENUM_CONTINUE; to : hr = IDirectInputDevice2_CreateEffect( joystick->device, &pdei->guid, &dieffect, &joystick->effects[joystick->cur_effect].effect, NULL); if (FAILED(hr)) return DIENUM_CONTINUE;
joystick->effects[joystick->cur_effect].params = dieffect; joystick->effects[joystick->cur_effect].info = *pdei; joystick->cur_effect += 1;
return DIENUM_CONTINUE;
Before I knew the joy.cpl app was to be blamed, and was searching in the IDirectInputEffect implementions, I saw something that is not conform MSDN: JoystickAImpl_EnumEffects and JoystickWImpl_EnumEffects should check on DIENUM_CONTINUE to continue the enumeration, or DIENUM_STOP to stop it: http://msdn.microsoft.com/en-us/library/windows/desktop/microsoft.directx_sd... For example, in JoystickWImpl_EnumEffects(), each occurrence of "(*lpCallback)(&dei, pvRef);" should check whether the return value is DIENUM_CONTINUE or DIENUM_STOP. When the latter is the case, should we then first add a "goto" statement to jump to the last "if (xfd == -1) IDirectInputDevice8_Unacquire(iface);" statement, or directly return? And what should it return in that case, "DIERR_NOTINITIALIZED" (does stopping enumeration mean failure? that's not clear in MSDN), or "DI_OK"? And even worse, what if the return value of "(*lpCallback)(&dei, pvRef);" is not equal to DIENUM_CONTINUE or DIENUM_STOP? Should JoystickWImpl_EnumEffects() interpret it as the former or as the latter? (also not mentioned in MSDN)
b) Weirdly, I had to add these commands that evaluates the DIEFFECT pointers at the end of each if-case, otherwise the pointers seemed to point to other address or data on that address was getting overwritten: TRACE("&rforce=0x%x\n", &rforce); TRACE("&cforce=0x%x\n", &cforce); TRACE("&pforce=0x%x\n", &pforce); TRACE("&cxyforce[0]=0x%x; &cxyforce[1]=0x%x\n", &(cxyforce[0]), &(cxyforce[1]));
This is an out of scope problem. pforce, rforce, etc are local to the body of the if-blocks and fall out of scope when the if-blocks close. They should be moved to the top of the function.
Ah, interesting, thanks! I'll move their declarations to the top of the function in the corresponding patch that I will send. To clarify for other people: Change joy.cpl/main.c:741 : dieffect.cAxes = 2; dieffect.rgdwAxes = axes; dieffect.rglDirection = direction;
if (IsEqualGUID(&pdei->guid, &GUID_RampForce)) to : dieffect.cAxes = 2; dieffect.rgdwAxes = axes; dieffect.rglDirection = direction;
DIRAMPFORCE rforce; DICONSTANTFORCE cforce; DIPERIODIC pforce; DICONDITION cxyforce[2];
if (IsEqualGUID(&pdei->guid, &GUID_RampForce))
By the way, when you begin to send patches to wine-patches, please only send up to 2-4 at a time, in logical groups. That way they can be reasonably reviewed & resubmitted as necessary.
OK, no problem ;)
Elias