Skip to content

Conversation

topolarity
Copy link
Member

Resolves #50714.

@topolarity topolarity requested a review from vtjnash August 12, 2023 00:24
@topolarity
Copy link
Member Author

topolarity commented Aug 12, 2023

Turns out this is an ODR violation: resolved, I believe

==16515==ERROR: AddressSanitizer: odr-violation (0x7efff4c19f60):
--
  | [1] size=1024 'small_typeof' /cache/build/default-amdci4-0/julialang/julia-master/cli/jl_exports.h:21
  | [2] size=1024 'small_typeof' /cache/build/default-amdci4-0/julialang/julia-master/cli/jl_exports.h:21
  | These globals were registered at these points:
  | [1]:
  | #0 0x7efff4e9a040 in __asan_register_globals.part.17 /workspace/srcdir/llvm-project/compiler-rt/lib/asan/asan_globals.cpp:356
  | #1 0x7efff14468be in asan.module_ctor jltypes.c
  | #2 0x7efff5b4efe1 in call_init elf/dl-init.c:72:3
  |  
  | [2]:
  | #0 0x7efff4e9a040 in __asan_register_globals.part.17 /workspace/srcdir/llvm-project/compiler-rt/lib/asan/asan_globals.cpp:356
  | #1 0x7efff4be0ace in asan.module_ctor loader_lib.c
  | #2 0x7efff5b4efe1 in call_init elf/dl-init.c:72:3
  |  
  | ==16515==HINT: if you don't care about these errors you may set ASAN_OPTIONS=detect_odr_violation=0
  | SUMMARY: AddressSanitizer: odr-violation: global 'small_typeof' at /cache/build/default-amdci4-0/julialang/julia-master/cli/jl_exports.h:21
  | ==16515==ABORTING

@brenhinkeller brenhinkeller added system:windows Affects only Windows embedding Embedding Julia using the C API labels Aug 12, 2023
@KristofferC KristofferC added the backport 1.10 Change should be backported to the 1.10 release label Aug 14, 2023
@topolarity
Copy link
Member Author

Should we also rename this to jl_small_typeof or similar to avoid polluting the global namespace?

Copy link
Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

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

The issue was diagnosed as a wrong linker line they were using, so this makes the problem worse, not better.

@vtjnash vtjnash closed this Aug 14, 2023
@topolarity
Copy link
Member Author

topolarity commented Aug 14, 2023

The issue was diagnosed as a wrong linker line they were using, so this makes the problem worse, not better.

cl.exe main.c /I %JULIA_PATH%\include\julia /TP /link /LIBPATH:%JULIA_PATH%\lib libjulia.dll.a /nologo is an incorrect linker invocation?

That invocation works just fine on 1.9 or if you remove the usage of jl_typeof(...)

I think this might have been part of a scheme to avoid an extra
indirection when this symbol is exposed to the linker. We have a similar
technique used for JIT'd modules, but I'm not sure how it's supposed
to work here between libjulia and libjulia-internal.

On Windows, this is broken for embedding currently, and on Linux this
function was just copying from the `small_typeof` in libjulia to a seem-
ingly unused copy of the symbol in libjulia-internal.

For now, this removes the special machinery and replaces it with a single
copy of the data. If we'd like to optimize this in the future, we might
want to use the existing "ijl_" vs. "jl_" pattern
This allows us to avoid a linker indirection on, e.g., Linux and macOS
where symbol interposition means that an extra load is incurred to
support possible relocation of the symbol to a pre-empting library.
@vtjnash vtjnash requested review from vtjnash and removed request for vtjnash August 31, 2023 17:22
@vtjnash vtjnash added the merge me PR is reviewed. Merge when all tests are passing label Aug 31, 2023
@topolarity topolarity force-pushed the fix-50714 branch 2 times, most recently from 2e2f78f to 0a37250 Compare August 31, 2023 17:56
@vtjnash vtjnash self-requested a review September 1, 2023 18:45
@vtjnash vtjnash merged commit 91b8c9b into JuliaLang:master Sep 1, 2023
@DilumAluthge DilumAluthge removed the merge me PR is reviewed. Merge when all tests are passing label Sep 1, 2023
KristofferC pushed a commit that referenced this pull request Sep 15, 2023
Resolves #50714

(cherry picked from commit 91b8c9b)
KristofferC added a commit that referenced this pull request Oct 2, 2023
Backported PRs:
- [x] #48625 <!-- add replace(io, str, patterns...) -->
- [x] #48387 <!-- Improve documentation of sort-related functions -->
- [x] #48363 <!-- Revise sort.md and docstrings in sort.jl -->
- [x] #48977 <!-- Update SparseArrays.jl stdlib for SuiteSparse 7 -->
- [x] #50719 <!-- fix `CyclePadding(::DataType)` -->
- [x] #50694 <!-- inference: permit recursive type traits -->
- [x] #50860 <!-- Add `Base.get_extension` to docs/API -->
- [x] #50594 <!-- Disallow non-index Integer types in isassigned -->
- [x] #50802 <!-- Makes IntrusiveLinkedListSynchronized mutable to avoid
allocation on poptask -->
- [x] #50858 <!-- Add a `threadpool` parameter to `Channel` constructor
-->
- [x] #50874 <!-- Restrict COFF to a single thread when symbol count is
high -->
- [x] #50822 <!-- Add default method for setmodifiers! -->
- [x] #50730 <!-- Fix integer overflow in `isapprox` -->
- [x] #50850 <!-- Remove weird Rational dispatch and add pi functions to
list -->
- [x] #50809 <!-- Limit type-printing in MethodError -->
- [x] #50915 <!-- Add note the `Task` about sticky bit -->
- [x] #50929 <!-- when widening tuple types in tmerge, only widen the
complex parts -->
- [x] #50928 <!-- Bump JuliaSyntax to 0.4.6 -->
- [x] #50959 <!-- Update libssh2 patches -->
- [x] #50823 <!-- Make ranges more robust with unsigned indexes. -->
- [x] #48542 <!-- Add docs on task-specific buffering using
multithreading -->
- [x] #50912 <!-- Separate foreign threads into a :foreign threadpool
-->
- [x] #51010 <!-- Add ORIGIN to SuiteSparse rpath on Linux/FreeBSD -->
- [x] #50753 <!-- cat: remove unused promote_eltype methods that confuse
inference -->
- [x] #51027 <!-- Implement realloc accounting correctly -->
- [x] #51019 <!-- fix a case of potentially use of undefined variable
when handling error in distributed message processing -->
- [x] #51039 <!-- Revert "Optimize findall(f, ::AbstractArray{Bool})
(#42202)" -->
- [x] #51036 <!-- add missing invoke edge for nospecialize targets -->
- [x] #51042 <!-- inference: fix return_type_tfunc modeling of concrete
functions -->
- [x] #51114 <!-- Workaround upstream FreeBSD issue #272992 -->
- [x] #50892 <!-- Add `JL_DLLIMPORT` to `small_typeof` declaration -->
- [x] #51154 <!-- broadcast: use recursion rather than ntuple to map
over a tuple -->
- [x] #51153 <!-- fix debug typo in "add missing invoke edge for
nospecialize targets (#51036)" -->
- [x] #51222 <!-- Check again if the tty is open inside the IO lock -->
- [x] #51236 <!-- Add lock around uv_unref during init -->
- [x] #51243 <!-- GMP: Gracefully handle more overflows. -->
- [x] #51254 <!-- Ryu: make sure adding zeros does not overwrite
trailing dot -->
- [x] #51175 <!-- shorten stale_age for cachefile lock -->
- [x] #51300 <!-- fix method definition error for bad vararg -->
- [x] #51307 <!-- fix force-throw ctrl-C on Windows -->
- [x] #51303 <!-- ensure revising structs is safe -->
- [x] #51393 
- [x] #51403 

Need manual backport:
- [x] #51009 <!-- fix #50562, regression in `in` of tuple of Symbols -->
- [x] #51053 <!-- Bump Statistics stdlib -->
- [x] #51013 <!-- fix #50709, type bound error due to free typevar in
sparam env -->
- [x] #51305 <!-- reduce test time for rounding and floatfuncs -->

Contains multiple commits, manual intervention needed:
- [ ] #50663 <!-- Fix Expr(:loopinfo) codegen -->
- [ ] #51035 <!-- refactor GC scanning code to reflect jl_binding_t are
now first class -->
- [ ] #51092 <!-- inference: fix bad effects for recursion -->
- [x] #51247 <!-- Check if malloc has succeeded before incrementing gc
stats -->
- [x] #51294 <!-- use LibGit2_jll for LibGit2 library -->

Non-merged PRs with backport label:
- [ ] #51132 <!-- Handle `AbstractQ` in concatenation -->
- [x] #51029 <!-- testdefs: make sure that if a test set changes the
active project, they change it back when they're done -->
- [ ] #50919 <!-- Code loading: do the "skipping mtime check for stdlib"
check regardless of the value of `ispath(f)` -->
- [ ] #50824 <!-- Add some aliasing warnings to docstrings for mutating
functions -->
- [x] #50385 <!-- Precompile pidlocks: add to NEWS and docs -->
- [ ] #49805 <!-- Limit TimeType subtraction to AbstractDateTime -->
@KristofferC KristofferC removed the backport 1.10 Change should be backported to the 1.10 release label Oct 2, 2023
nalimilan pushed a commit that referenced this pull request Nov 5, 2023
Resolves #50714

(cherry picked from commit 91b8c9b)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
embedding Embedding Julia using the C API system:windows Affects only Windows
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unresolved symbol small_typeof when jl_typeof is used on Windows with 1.10.0-beta1
5 participants