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

hiredis-client: use -fvisibility=hidden instead of -exported_symbols_list #82

Merged
merged 1 commit into from
Jan 20, 2023

Conversation

casperisfine
Copy link
Collaborator

Rather than to use an exported symbols list, we compile with -fvisibility=hidden and mark the init function with RUBY_FUNC_EXPORTED.

This was suggested by @nobu.

Ref: #58 (comment)

Copy link
Contributor

@eregon eregon left a comment

Choose a reason for hiding this comment

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

This looks much cleaner :)

@casperisfine
Copy link
Collaborator Author

Yeah, but CI is broken for some reason, not sure if related or not.

@casperisfine
Copy link
Collaborator Author

Somehow the hiredis test suite pass on my machine, but on CI it's consistently broken for 3.1 and 3.2. 😫

@eregon
Copy link
Contributor

eregon commented Jan 20, 2023

Is it also broken on master? Maybe trigger a run from master's commit to see if it's due to these changes or not.

@casperisfine
Copy link
Collaborator Author

Is it also broken on master?

No it passes fine: https://github.com/redis-rb/redis-client/actions/runs/3968428783/jobs/6801585027

the pure-ruby part of the test suite also pass fine, only the hiredis (binding) part is failing with a weird EAGAIN issue.

I probably won't have time to debug this much before a week or two though :/

@casperisfine
Copy link
Collaborator Author

interesting, if I try to repro locally inside the ruby:3.2 docker image, I get a ENOTSOCK error on connect, that's even wierder.

@eregon given that you are on Linux if I understand correctly, I wonder if you get the same issue?

@casperisfine
Copy link
Collaborator Author

So the issue seem to be that on Linux the visibility=hidden doesn't apply to the symbols coming from the libhiredis.a, I tried a few solution I found on the web for no luck so far.

@eregon
Copy link
Contributor

eregon commented Jan 20, 2023

Even though libhiredis.a is compiled with visibility=hidden?

@casperisfine
Copy link
Collaborator Author

Well, somehow on 3.2 it is not passed:

cc -std=c99 -pedantic -c -g -fPIC  -DHIREDIS_TEST_SSL -Wall -W -Wstrict-prototypes -Wwrite-strings -Wno-missing-field-initializers -g -ggdb alloc.c
cc -std=c99 -pedantic -c -g -fPIC  -DHIREDIS_TEST_SSL -Wall -W -Wstrict-prototypes -Wwrite-strings -Wno-missing-field-initializers -g -ggdb net.c
cc -std=c99 -pedantic -c -g -fPIC  -DHIREDIS_TEST_SSL -Wall -W -Wstrict-prototypes -Wwrite-strings -Wno-missing-field-initializers -g -ggdb hiredis.c
cc -std=c99 -pedantic -c -g -fPIC  -DHIREDIS_TEST_SSL -Wall -W -Wstrict-prototypes -Wwrite-strings -Wno-missing-field-initializers -g -ggdb sds.c
cc -std=c99 -pedantic -c -g -fPIC  -DHIREDIS_TEST_SSL -Wall -W -Wstrict-prototypes -Wwrite-strings -Wno-missing-field-initializers -g -ggdb async.c
cc -std=c99 -pedantic -c -g -fPIC  -DHIREDIS_TEST_SSL -Wall -W -Wstrict-prototypes -Wwrite-strings -Wno-missing-field-initializers -g -ggdb read.c
cc -std=c99 -pedantic -c -g -fPIC  -DHIREDIS_TEST_SSL -Wall -W -Wstrict-prototypes -Wwrite-strings -Wno-missing-field-initializers -g -ggdb sockcompat.c
ar rcs libhiredis.a alloc.o net.o hiredis.o sds.o async.o read.o sockcompat.o
cc -std=c99 -pedantic -c -g -fPIC  -DHIREDIS_TEST_SSL -Wall -W -Wstrict-prototypes -Wwrite-strings -Wno-missing-field-initializers -g -ggdb ssl.c

But on 3.0 it is:

cc -std=c99 -pedantic -c -g -fPIC  "-I/opt/hostedtoolcache/Ruby/3.0.5/x64/openssl/include" "-fvisibility=hidden" -Wall -W -Wstrict-prototypes -Wwrite-strings -Wno-missing-field-initializers -g -ggdb alloc.c
cc -std=c99 -pedantic -c -g -fPIC  "-I/opt/hostedtoolcache/Ruby/3.0.5/x64/openssl/include" "-fvisibility=hidden" -Wall -W -Wstrict-prototypes -Wwrite-strings -Wno-missing-field-initializers -g -ggdb net.c
cc -std=c99 -pedantic -c -g -fPIC  "-I/opt/hostedtoolcache/Ruby/3.0.5/x64/openssl/include" "-fvisibility=hidden" -Wall -W -Wstrict-prototypes -Wwrite-strings -Wno-missing-field-initializers -g -ggdb hiredis.c
cc -std=c99 -pedantic -c -g -fPIC  "-I/opt/hostedtoolcache/Ruby/3.0.5/x64/openssl/include" "-fvisibility=hidden" -Wall -W -Wstrict-prototypes -Wwrite-strings -Wno-missing-field-initializers -g -ggdb sds.c
cc -std=c99 -pedantic -c -g -fPIC  "-I/opt/hostedtoolcache/Ruby/3.0.5/x64/openssl/include" "-fvisibility=hidden" -Wall -W -Wstrict-prototypes -Wwrite-strings -Wno-missing-field-initializers -g -ggdb async.c
cc -std=c99 -pedantic -c -g -fPIC  "-I/opt/hostedtoolcache/Ruby/3.0.5/x64/openssl/include" "-fvisibility=hidden" -Wall -W -Wstrict-prototypes -Wwrite-strings -Wno-missing-field-initializers -g -ggdb read.c
cc -std=c99 -pedantic -c -g -fPIC  "-I/opt/hostedtoolcache/Ruby/3.0.5/x64/openssl/include" "-fvisibility=hidden" -Wall -W -Wstrict-prototypes -Wwrite-strings -Wno-missing-field-initializers -g -ggdb sockcompat.c
ar rcs libhiredis.a alloc.o net.o hiredis.o sds.o async.o read.o sockcompat.o
cc -std=c99 -pedantic -c -g -fPIC  "-I/opt/hostedtoolcache/Ruby/3.0.5/x64/openssl/include" "-fvisibility=hidden" -Wall -W -Wstrict-prototypes -Wwrite-strings -Wno-missing-field-initializers -g -ggdb ssl.c

So that's likely the source of the issue yeah.

@casperisfine casperisfine force-pushed the export-func branch 2 times, most recently from 720732a to 1905d8c Compare January 20, 2023 15:27
@casperisfine casperisfine force-pushed the export-func branch 2 times, most recently from 6b5fcc7 to f7f1063 Compare January 20, 2023 15:43
@casperisfine
Copy link
Collaborator Author

Ok, I finally figured it out.

@flavorjones is working on a cleanup of the extconf.rb though, so I'll wait for his PR first.

@eregon
Copy link
Contributor

eregon commented Jan 20, 2023

@flavorjones is working on a cleanup of the extconf.rb though, so I'll wait for his PR first.

Sounds like heaven to me :) (I've come across many extconf.rb, and most could afford a good cleanup, it tends to be the least readable Ruby scripts of all)

@casperisfine casperisfine force-pushed the export-func branch 2 times, most recently from a9d8ac0 to 0fbefe0 Compare January 20, 2023 16:06
Rather than to use an exported symbols list, we compile with -fvisibility=hidden
and mark the init function with RUBY_FUNC_EXPORTED.

This was suggested by @nobu.

Ref: #58 (comment)
@casperisfine casperisfine changed the title hiredis-client: refactor compilation flags hiredis-client: use -fvisibility=hidden instead of -exported_symbols_list Jan 20, 2023
@casperisfine casperisfine merged commit f25f38e into master Jan 20, 2023
@casperisfine casperisfine deleted the export-func branch January 20, 2023 16:15
@casperisfine
Copy link
Collaborator Author

Ok, all green. Thank you all! It's much cleaner now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants