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

[CLI] error on not finding symbol #38994

Merged
merged 1 commit into from
Apr 16, 2021
Merged

[CLI] error on not finding symbol #38994

merged 1 commit into from
Apr 16, 2021

Conversation

vchuravy
Copy link
Member

From #38980, adds a verification when the CLI can't find a symbol.

Currently doesn't work since:

src/processor_x86.cpp
7:extern "C" JL_DLLEXPORT void jl_cpuid(int32_t CPUInfo[4], int32_t InfoType)
30:extern "C" JL_DLLEXPORT void jl_cpuidex(int32_t CPUInfo[4], int32_t InfoType, int32_t subInfoType)

is only exported on x86.

@vchuravy vchuravy mentioned this pull request Dec 25, 2020
Merged
@staticfloat
Copy link
Member

I've added some preprocessor trickery to export jl_cpuid only on x86_64 and i686 processors, but it is not at all clear to me whether this is actually desirable:

It would appear that jl_cpuid is not listed in julia.h. This could be an oversight, (I believe this was exported to be used from Julia code for detecting the currently-running microarchitecture for march-specific BB binaries) but I think since the only real usecase we have for this right now is to use it from Julia code, I'm leaning more toward just removing it from src/jl_exported_funcs.inc and letting it be reachable from Julia code only and not from C code at all. I would feel differently if we had a unified interface across all architectures, but sadly we don't.

@vtjnash what do you think?

vtjnash
vtjnash approved these changes Apr 3, 2021
View reviewed changes
src/jl_exported_funcs.inc Outdated Show resolved Hide resolved
src/jl_exported_funcs.inc Outdated Show resolved Hide resolved
@vtjnash vtjnash marked this pull request as ready for review April 3, 2021 17:09
@vtjnash vtjnash added merge me PR is reviewed. Merge when all tests are passing and removed merge me PR is reviewed. Merge when all tests are passing labels Apr 3, 2021
@vtjnash vtjnash added the merge me PR is reviewed. Merge when all tests are passing label Apr 6, 2021
@vtjnash vtjnash force-pushed the vc/verify_cli branch 2 times, most recently from bacad09 to c31713b Compare April 8, 2021 07:14
@vtjnash vtjnash added forget me not PRs that one wants to make sure aren't forgotten and removed merge me PR is reviewed. Merge when all tests are passing labels Apr 9, 2021
vtjnash
vtjnash reviewed Apr 9, 2021
View reviewed changes
Make.inc Outdated Show resolved Hide resolved
@vchuravy @vtjnash
[CLI] error on not finding symbol
b680ef1 
Also removes some excess exports that are not in julia.h, including
intrinsics and the unused `jl_cpuid` function (only defined on Intel
processors).
@vtjnash vtjnash added merge me PR is reviewed. Merge when all tests are passing and removed forget me not PRs that one wants to make sure aren't forgotten labels Apr 15, 2021
@vtjnash vtjnash merged commit 67f29d8 into master Apr 16, 2021
@vtjnash vtjnash deleted the vc/verify_cli branch April 16, 2021 16:42
@simeonschaub simeonschaub removed the merge me PR is reviewed. Merge when all tests are passing label Apr 28, 2021
ElOceanografo pushed a commit to ElOceanografo/julia that referenced this pull request May 4, 2021
@vchuravy @ElOceanografo
[CLI] error on not finding symbol (JuliaLang#38994)
1608c1b 
Also removes some excess exports that are not in julia.h, including
intrinsics and the unused `jl_cpuid` function (only defined on Intel
processors).
vtjnash added a commit that referenced this pull request May 7, 2021
@vtjnash
[CLI] ensure error on not finding symbol
e9029e1 
This sanity check was attempted in #38994, but dlsym is often also okay
with finding the trampoline, so we want to also check that it does not
use that to resolve the trampoline ad infinitum.
@vtjnash vtjnash mentioned this pull request May 7, 2021
Merged
antoine-levitt pushed a commit to antoine-levitt/julia that referenced this pull request May 9, 2021
@vchuravy @antoine-levitt
[CLI] error on not finding symbol (JuliaLang#38994)
d67ec1f 
Also removes some excess exports that are not in julia.h, including
intrinsics and the unused `jl_cpuid` function (only defined on Intel
processors).
vtjnash added a commit that referenced this pull request May 10, 2021
@vtjnash
[CLI] ensure error on not finding symbol (#40748)
e4f79b7 
This sanity check was attempted in #38994, but dlsym is often also okay
with finding the trampoline, so we want to also check that it does not
use that to resolve the trampoline ad infinitum.
shirodkara pushed a commit to shirodkara/julia that referenced this pull request Jun 9, 2021
@vtjnash
[CLI] ensure error on not finding symbol (JuliaLang#40748)
72e5aff 
This sanity check was attempted in JuliaLang#38994, but dlsym is often also okay
with finding the trampoline, so we want to also check that it does not
use that to resolve the trampoline ad infinitum.
johanmon pushed a commit to johanmon/julia that referenced this pull request Jul 5, 2021
@vchuravy @johanmon
[CLI] error on not finding symbol (JuliaLang#38994)
bb58742 
Also removes some excess exports that are not in julia.h, including
intrinsics and the unused `jl_cpuid` function (only defined on Intel
processors).
johanmon pushed a commit to johanmon/julia that referenced this pull request Jul 5, 2021
@vtjnash @johanmon
[CLI] ensure error on not finding symbol (JuliaLang#40748)
d7286b7 
This sanity check was attempted in JuliaLang#38994, but dlsym is often also okay
with finding the trampoline, so we want to also check that it does not
use that to resolve the trampoline ad infinitum.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vtjnash vtjnash vtjnash approved these changes

Assignees

@staticfloat staticfloat

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@vchuravy @staticfloat @vtjnash @simeonschaub