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

JoystickImpl: memory leak and strange wordings #660

Closed
axalix opened this Issue Jul 13, 2014 · 21 comments

Comments

Projects
None yet
5 participants
@axalix

axalix commented Jul 13, 2014

Hi

I am on Mint / Linux, 64 bit and decided to try the latest code from the repo. Once I switched, I keep getting these ugly messages even for official examples:

Unable to get attribute 'idVendor'. Could not find parent USB device for joystick at index 0
Unable to get attribute 'idProduct'. Could not find parent USB device for joystick at index 0

It looks like it happens after this commit;
29c0f14

In addition to that Valgrind that never complained about SFML code now yells this:

Leak_DefinitelyLost|====|4,756 (448 direct, 4,308 indirect) bytes in 1 blocks are definitely lost in loss record 136 of 139|
||Call stack:|
/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so||0x4C2ABB4: calloc|
/lib/x86_64-linux-gnu/libudev.so.1.2.2||0x5530E5D: udev_device_new_from_syspath|
/lib/x86_64-linux-gnu/libudev.so.1.2.2||0x553144A: udev_device_new_from_subsystem_sysname|
MY_GAME||0x44C3DD: sf::priv::JoystickImpl::getUdevAttributeUint(unsigned int, std::string)|
MY_GAME||0x44CA54: sf::priv::JoystickImpl::open(unsigned int)|
MY_GAME||0x44A3D3: sf::priv::JoystickManager::update()|
MY_GAME||0x443AA0: sf::priv::WindowImpl::WindowImpl()|
MY_GAME||0x448D01: sf::priv::WindowImplX11::WindowImplX11(sf::VideoMode, sf::String const&, unsigned long, sf::ContextSettings const&)|
MY_GAME||0x4435F0: sf::priv::WindowImpl::create(sf::VideoMode, sf::String const&, unsigned int, sf::ContextSettings const&)|
MY_GAME||0x4433BD: sf::Window::create(sf::VideoMode, sf::String const&, unsigned int, sf::ContextSettings const&)|
MY_GAME||0x436E49: sf::RenderWindow::RenderWindow(sf::VideoMode, sf::String const&, unsigned int, sf::ContextSettings const&)|

@axalix axalix changed the title from Memory leak and strange wordings to JoystickImpl: memory leak and strange wordings Jul 13, 2014

@binary1248 binary1248 added bug labels Jul 13, 2014

@axalix

This comment has been minimized.

Show comment
Hide comment
@axalix

axalix Jul 13, 2014

Two things:

  1. I don't have any joysticks
  2. I've seen some details about the same problem here: http://en.sfml-dev.org/forums/index.php?topic=15344.0

axalix commented Jul 13, 2014

Two things:

  1. I don't have any joysticks
  2. I've seen some details about the same problem here: http://en.sfml-dev.org/forums/index.php?topic=15344.0
@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Jul 13, 2014

Member

Well... I think the fix for the leak is quite obvious. udev_device_get_parent_with_subsystem_devtype overwrites dev with the parent device and when calling udev_device_unref the child device isn't properly unreferenced.

Documentation states that udev_device_get_parent_with_subsystem_devtype returns a device that is not referenced since it is attached to the child, hence the unreferencing is happening on the wrong device 😉.

As for why this triggers even when no joysticks are present... I have no idea. But I think the root of the problem lies in the detection of whether joysticks are present.

Can you check if there are any joystick nodes in /dev/input? They look something like /dev/input/js0 for node 0 and so on. SFML tries to open them one by one to check if any joysticks are present.

Member

binary1248 commented Jul 13, 2014

Well... I think the fix for the leak is quite obvious. udev_device_get_parent_with_subsystem_devtype overwrites dev with the parent device and when calling udev_device_unref the child device isn't properly unreferenced.

Documentation states that udev_device_get_parent_with_subsystem_devtype returns a device that is not referenced since it is attached to the child, hence the unreferencing is happening on the wrong device 😉.

As for why this triggers even when no joysticks are present... I have no idea. But I think the root of the problem lies in the detection of whether joysticks are present.

Can you check if there are any joystick nodes in /dev/input? They look something like /dev/input/js0 for node 0 and so on. SFML tries to open them one by one to check if any joysticks are present.

@axalix

This comment has been minimized.

Show comment
Hide comment
@axalix

axalix Jul 13, 2014

It says I have one, but I actually don't. May be just a port on my laptop

crw-r--r-- 1 root root 13, 0 Jul 13 09:08 js0

axalix commented Jul 13, 2014

It says I have one, but I actually don't. May be just a port on my laptop

crw-r--r-- 1 root root 13, 0 Jul 13 09:08 js0

@axalix

This comment has been minimized.

Show comment
Hide comment
@axalix

axalix Jul 13, 2014

If it helps
cat /sys/class/input/js0/device/name
ST LIS3LV02DL Accelerometer
Second answer here: http://superuser.com/questions/149407/why-does-my-hp-pavilion-notebook-have-an-accelerometer

axalix commented Jul 13, 2014

If it helps
cat /sys/class/input/js0/device/name
ST LIS3LV02DL Accelerometer
Second answer here: http://superuser.com/questions/149407/why-does-my-hp-pavilion-notebook-have-an-accelerometer

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Jul 13, 2014

Member

Hmm... I guess these days, accelerometers also count as joysticks... we might have to add a check for this.

Member

binary1248 commented Jul 13, 2014

Hmm... I guess these days, accelerometers also count as joysticks... we might have to add a check for this.

@axalix

This comment has been minimized.

Show comment
Hide comment
@axalix

axalix Jul 13, 2014

Cool - thanks. Once update is done - I will try it right away.

axalix commented Jul 13, 2014

Cool - thanks. Once update is done - I will try it right away.

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Jul 13, 2014

Member

One possibility to distinguish accelerometers from joysticks is to check whether the device provides buttons. See also here.

Member

Bromeon commented Jul 13, 2014

One possibility to distinguish accelerometers from joysticks is to check whether the device provides buttons. See also here.

@axalix

This comment has been minimized.

Show comment
Hide comment
@axalix

axalix Jul 13, 2014

May be this link can help as well: http://cgit.freedesktop.org/systemd/systemd/tree/src/udev/accelerometer/accelerometer.c?id=8fef0ff25c9fd7e5bb99d66f43c6357e4996a3cc

I am not sure about accelerometer for mobile phones.

Anyways, for me I've just made a small "hack" and disabled functionality in SFML if parent udev device is not defined. Also I've made that fix for memory leak that binary1248 referred to in 3rd comment. These 2 things solved all my issues, but I would be much more happier with a real fix from the team.

Last thing is about "getUdevAttributeUint". Hope someone will refactor this method as it doesn't make any sense for me to call it twice for "vendorId" and "productId" for each index and then call "udev_device_*" methods twice as well.

axalix commented Jul 13, 2014

May be this link can help as well: http://cgit.freedesktop.org/systemd/systemd/tree/src/udev/accelerometer/accelerometer.c?id=8fef0ff25c9fd7e5bb99d66f43c6357e4996a3cc

I am not sure about accelerometer for mobile phones.

Anyways, for me I've just made a small "hack" and disabled functionality in SFML if parent udev device is not defined. Also I've made that fix for memory leak that binary1248 referred to in 3rd comment. These 2 things solved all my issues, but I would be much more happier with a real fix from the team.

Last thing is about "getUdevAttributeUint". Hope someone will refactor this method as it doesn't make any sense for me to call it twice for "vendorId" and "productId" for each index and then call "udev_device_*" methods twice as well.

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Jul 14, 2014

Member

Yes, I have already planned to refactor several parts of the joystick code. It has been a mix of various contributions and is rather messy, sometimes it doesn't even conform to the SFML style. Unfortunately I'm quite busy at the moment, but I should have time to work on it in August.

So, there are actually 3 issues:

  • memory leak
  • accelerometer wrongly detected
  • general refactoring
Member

Bromeon commented Jul 14, 2014

Yes, I have already planned to refactor several parts of the joystick code. It has been a mix of various contributions and is rather messy, sometimes it doesn't even conform to the SFML style. Unfortunately I'm quite busy at the moment, but I should have time to work on it in August.

So, there are actually 3 issues:

  • memory leak
  • accelerometer wrongly detected
  • general refactoring
@axalix

This comment has been minimized.

Show comment
Hide comment
@axalix

axalix Jul 15, 2014

Yep, those three. Sounds like a plan :)

axalix commented Jul 15, 2014

Yep, those three. Sounds like a plan :)

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Jul 15, 2014

Member
  • memory leak
  • accelerometer wrongly detected
  • general refactoring

e86915a 😀

Tested on Windows, Linux and FreeBSD. I don't have an Apple machine, so can't test there. Don't have a joystick either, so someone else will have to test the Unix accelerometer/joystick detection code. If anyone has anything else to add to the commit, feel free to do so or mention it in a line comment.

Member

binary1248 commented Jul 15, 2014

  • memory leak
  • accelerometer wrongly detected
  • general refactoring

e86915a 😀

Tested on Windows, Linux and FreeBSD. I don't have an Apple machine, so can't test there. Don't have a joystick either, so someone else will have to test the Unix accelerometer/joystick detection code. If anyone has anything else to add to the commit, feel free to do so or mention it in a line comment.

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila
Member

LaurentGomila commented Jul 15, 2014

\o/

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Jul 15, 2014

Member

How insane! :D
I only found superficial stuff (but I didn't have a deep semantic look at it), in general the code looks much better now. Many thanks, binary!

Member

Bromeon commented Jul 15, 2014

How insane! :D
I only found superficial stuff (but I didn't have a deep semantic look at it), in general the code looks much better now. Many thanks, binary!

@axalix

This comment has been minimized.

Show comment
Hide comment
@axalix

axalix Jul 16, 2014

Thanks a lot for the fix, it looks like all my problems have gone: no memory leaks and no those messages. I like the refactoring as well with a tiny addition that you can see in the code.

The only thing is warnings when I compile:

SFML/Window/Unix/JoystickImpl.cpp: In static member function ‘static bool sf::priv::JoystickImpl::isConnected(unsigned int)’:
SFML/src/SFML/Window/Unix/JoystickImpl.cpp:234:57: warning: ignoring return value of ‘ssize_t read(int, void*, size_t)’, declared with attribute warn_unused_result [-Wunused-result]

Thanks again

P.S.: Nowadays it doesn't happen often when Valgrind logs are entirely clean - I am impressed with quality of SFML. Well done, guys!

P.P.S.: I only tested on Linux (Mint, 64 bit) - same machine that used before.

axalix commented Jul 16, 2014

Thanks a lot for the fix, it looks like all my problems have gone: no memory leaks and no those messages. I like the refactoring as well with a tiny addition that you can see in the code.

The only thing is warnings when I compile:

SFML/Window/Unix/JoystickImpl.cpp: In static member function ‘static bool sf::priv::JoystickImpl::isConnected(unsigned int)’:
SFML/src/SFML/Window/Unix/JoystickImpl.cpp:234:57: warning: ignoring return value of ‘ssize_t read(int, void*, size_t)’, declared with attribute warn_unused_result [-Wunused-result]

Thanks again

P.S.: Nowadays it doesn't happen often when Valgrind logs are entirely clean - I am impressed with quality of SFML. Well done, guys!

P.P.S.: I only tested on Linux (Mint, 64 bit) - same machine that used before.

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Jul 16, 2014

Member

Amended commit can be found at 46cf34f.

Member

binary1248 commented Jul 16, 2014

Amended commit can be found at 46cf34f.

@axalix

This comment has been minimized.

Show comment
Hide comment
@axalix

axalix Jul 16, 2014

Thank you, binary.

I don't see any issues now. Warnings have also gone. Would be cool to merge these changes.

axalix commented Jul 16, 2014

Thank you, binary.

I don't see any issues now. Warnings have also gone. Would be cool to merge these changes.

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Jul 17, 2014

Member

I cannot test this on OS X until Septembre (no joystick with me). It's now on my todo list but if someone want/can test it, please go ahead. :-)

Member

mantognini commented Jul 17, 2014

I cannot test this on OS X until Septembre (no joystick with me). It's now on my todo list but if someone want/can test it, please go ahead. :-)

@axalix

This comment has been minimized.

Show comment
Hide comment
@axalix

axalix Jul 28, 2014

Any chance the fix will be accepted and merged?

axalix commented Jul 28, 2014

Any chance the fix will be accepted and merged?

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Jul 28, 2014

Member

Any chance the fix will be accepted and merged?

It won't be merged to master before it is reviewed and tested. But if you need this functionality now, checkout the branch ;)

Member

Bromeon commented Jul 28, 2014

Any chance the fix will be accepted and merged?

It won't be merged to master before it is reviewed and tested. But if you need this functionality now, checkout the branch ;)

@binary1248 binary1248 self-assigned this Aug 14, 2014

@binary1248 binary1248 added s:accepted and removed s:unassigned labels Aug 14, 2014

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Aug 20, 2014

Member

superseded? (I'm still a bit confused by our new tag system. ^^')

Member

mantognini commented Aug 20, 2014

superseded? (I'm still a bit confused by our new tag system. ^^')

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Sep 12, 2014

Member

Superseded by PR #686.

Member

mantognini commented Sep 12, 2014

Superseded by PR #686.

@mantognini mantognini closed this Sep 12, 2014

@Bromeon Bromeon added this to the 2.2 milestone Oct 28, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment