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.

Sign up for GitHub

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

Jump to bottom

add support for looking up versioned functions re #889 #890

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

add support for looking up versioned functions re #889 #890

wants to merge 4 commits into from

Conversation

lwoggardner
Copy link

No description provided.

@tduehr tduehr self-requested a review June 2, 2021 15:10
tduehr
tduehr requested changes Jun 2, 2021
View reviewed changes
Copy link
Member

@tduehr tduehr left a comment

Choose a reason for hiding this comment

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

Please add tests.

@larskanis
Copy link
Member

I wrote some tests in the meantime. They yet need to be excluded on platforms that don't support versioned symbols.

Moreover I tried to build a test that uses similar symbols as shown in #889 . In case of fuse_new there are distinct function entries (12830 vs. 12710) for fuse_new and fuse_new@@FUSE_2.6 (the default for non versioned dlsym() calls). I was unable to mimic this in our libtest.so, so far. So the tests don't verify this particular issue.

201: 0000000000012830 20 FUNC GLOBAL DEFAULT 15 fuse_new
202: 0000000000012710 12 FUNC GLOBAL DEFAULT 15 fuse_new@@FUSE_2.6

@lwoggardner
Copy link
Author

Moreover I tried to build a test that uses similar symbols as shown in #889 . In case of fuse_new there are distinct function entries (12830 vs. 12710) for fuse_new and fuse_new@@FUSE_2.6 (the default for non versioned dlsym() calls). I was unable to mimic this in our libtest.so, so far. So the tests don't verify this particular issue.

I'm pretty sure use of versions like that is non standard and they fixed it in Fuse 3. I think the test are sufficient for the expected use of versioned symbols via dlvsym and I can confirm that it works for the Fuse case as well.

chenqiongqiong
chenqiongqiong reviewed Sep 8, 2021
View reviewed changes
ext/ffi_c/DynamicLibrary.c
Comment on lines 29 to 39


// ruby.h first to provide _GNU_SOURCE which gives us dlvsym if available
#include <ruby.h>

#include <sys/types.h>
#include <stdio.h>
#include <stdint.h>

#if (defined(_WIN32) || defined(__WIN32__)) && !defined(__CYGWIN__)
# include <winsock2.h>
Copy link

Choose a reason for hiding this comment

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

,m,,,,

Copy link
Author

Choose a reason for hiding this comment

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

Not sure what ^^ means but the alternative to this change in include order is to define _GNU_SOURCE specifically and accept that ruby.h generates a warning about it being redefined.

@lwoggardner lwoggardner mentioned this pull request Sep 14, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chenqiongqiong chenqiongqiong chenqiongqiong left review comments

@tduehr tduehr tduehr requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@lwoggardner @larskanis @tduehr @chenqiongqiong