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

src/codegen.cpp: fix segfault on @code_native with eltype (#34434) #34458

Merged
merged 1 commit into from
Feb 3, 2020
Merged

src/codegen.cpp: fix segfault on @code_native with eltype (#34434) #34458

merged 1 commit into from
Feb 3, 2020

Conversation

anaveragehuman
Copy link
Contributor

Fixes #34434

According to git bisect:

8c44566 is the first bad commit

git bisect start                                                                                          
# bad: [c6da87ff4bc7a855e217856757ad3413cf6d1f79] Set VERSION to 1.2.0 (#32964)                           
git bisect bad c6da87ff4bc7a855e217856757ad3413cf6d1f79                                               
# good: [55e36cc308b66d3472990a06b2797f9f9154ea0a] Set VERSION to 1.1.1 (#31969)                      
git bisect good 55e36cc308b66d3472990a06b2797f9f9154ea0a                                                  
# good: [b55b85cd9952ce39246d1bcb6c4b0417ac457531] spawn/IO: supercharge the API (#30278)                 
git bisect good b55b85cd9952ce39246d1bcb6c4b0417ac457531                                                  
# good: [0b38dd0ac1ec977166e71fdee92d2ac5d1ee36fe] More xrefs for the meta docs                           
git bisect good 0b38dd0ac1ec977166e71fdee92d2ac5d1ee36fe                                                  
# bad: [a3997800b0b8d364c628d2a2ec8ae8e246b8f63f] Fix Int64 overrun potential in skip_deleted (#31452)    
git bisect bad a3997800b0b8d364c628d2a2ec8ae8e246b8f63f                                                   
# good: [a168bc8a8c73f6f6b536a40ced32df8c528a7b4b] Merge pull request #31324 from JuliaLang/vc/tuple_props
git bisect good a168bc8a8c73f6f6b536a40ced32df8c528a7b4b
# good: [672bf8bea7b01a1828fc21e6689c8d3fa5f46ea6] realpath buffer is a vector (#31433)
git bisect good 672bf8bea7b01a1828fc21e6689c8d3fa5f46ea6
# good: [c0b40d8530dc62cebc556e026ab5ae5409a7bd29] Merge pull request #31413 from JuliaLang/jn/nfc-1
git bisect good c0b40d8530dc62cebc556e026ab5ae5409a7bd29
# good: [11156024da041ea0060fd267f6acd8d83193b653] Fix #31396 (#31401)
git bisect good 11156024da041ea0060fd267f6acd8d83193b653
# good: [53d7fbfbd67607659e33b779b9c91668dff7ca71] Replaced `Any[...]` with `[...]` in example (#31526)
git bisect good 53d7fbfbd67607659e33b779b9c91668dff7ca71
# good: [9f125f90cdb823e5682868838eded05c394f1944] add test for #28762, at-inferred error (#31430)
git bisect good 9f125f90cdb823e5682868838eded05c394f1944
# bad: [8c445663547791e59f33f9e8d49b276332107614] internals: better representation for code (#31191)
git bisect bad 8c445663547791e59f33f9e8d49b276332107614
# good: [4b1230277ca50c70ee517578db1ec30483cf9ad9] add a nop conversion for some (#31536)
git bisect good 4b1230277ca50c70ee517578db1ec30483cf9ad9
# first bad commit: [8c445663547791e59f33f9e8d49b276332107614] internals: better representation for code (#31191)

@StefanKarpinski StefanKarpinski added kind:bugfix This change fixes an existing bug compiler:codegen Generation of LLVM IR and native code backport 1.4 labels Jan 21, 2020
@StefanKarpinski
Copy link
Sponsor Member

The blamed commit is included in quite a few releases, so I'm wondering if this did really introduce the bug or if the bug was there but not as visible? I've marked for backporting to 1.4 but maybe needs to be backported to 1.3 as well.

@JeffBezanson
Copy link
Sponsor Member

The bug does exist in 1.3 as well.

@anaveragehuman
Copy link
Contributor Author

The blamed commit is included in quite a few releases, so I'm wondering if this did really introduce the bug or if the bug was there but not as visible? I've marked for backporting to 1.4 but maybe needs to be backported to 1.3 as well.

I started bisecting from 1.2.0 because I noticed it was also broken:

/dev/shm$ julia-1.2.0/bin/julia 
               _
   _       _ _(_)_     |  Documentation: https://docs.julialang.org
  (_)     | (_) (_)    |
   _ _   _| |_  __ _   |  Type "?" for help, "]?" for Pkg help.
  | | | | | | |/ _` |  |
  | | |_| | | | (_| |  |  Version 1.2.0 (2019-08-20)
 _/ |\__'_|_|_|\__'_|  |  Official https://julialang.org/ release
|__/                   |

julia> @code_native eltype(1)

signal (11): Segmentation fault
in expression starting at REPL[1]:1
Twine at /buildworker/worker/package_linux64/build/usr/include/llvm/ADT/Twine.h:268 [inlined]
jl_get_llvmf_decl at /buildworker/worker/package_linux64/build/src/codegen.cpp:1637
_dump_function_linfo at /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.2/InteractiveUtils/src/codeview.jl:100
_dump_function at /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.2/InteractiveUtils/src/codeview.jl:84
_dump_function at /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.2/InteractiveUtils/src/codeview.jl:71 [inlined]

I tested each commit listed in the bisect log above manually with these commands, marking bad if it segfaulted and good otherwise:

make -j8
./julia <<< 'Main.@code_native eltype(1)'

So yes, I think it has actually been broken since that commit.

@ararslan
Copy link
Member

Suggested test which kind of sucks but fails without this change and should work with it:

@testset "Issue #34434" begin
    io = IOBuffer()
    code_native(io, eltype, Tuple{Int})
    @test occursin("eltype", String(take!(io)))
end

@ararslan ararslan added the needs tests Unit tests are required for this change label Jan 22, 2020
@KristofferC
Copy link
Sponsor Member

Would it be better to put the test in stdlib/InteractiveUtils/test/runtets.jl?

@ararslan ararslan removed the needs tests Unit tests are required for this change label Jan 24, 2020
@ararslan
Copy link
Member

I think it makes sense to be part of the Base tests in this case, since it isn't testing InteractiveUtils functionality as much as it is testing for a bug in the core of Julia by way of InteractiveUtils for lack of a better reproducer.

@KristofferC
Copy link
Sponsor Member

KristofferC commented Jan 24, 2020

Putting dependencies on standard libraries (more than really needed) in the Base tests seems bad though.

And we already test stuff like #18883 there so why not be consistent?

@ararslan
Copy link
Member

Sure, that makes sense.

@KristofferC KristofferC mentioned this pull request Jan 26, 2020
26 tasks
src/codegen.cpp Outdated Show resolved Hide resolved
@KristofferC KristofferC merged commit 6a96a4d into JuliaLang:master Feb 3, 2020
@sethaxen
Copy link
Contributor

sethaxen commented Feb 3, 2020

Thanks for the fix!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler:codegen Generation of LLVM IR and native code kind:bugfix This change fixes an existing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Segfault calling @code_native on eltype
7 participants