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

Revert inlined method signature stacktrace lookup code #52754

Merged
merged 2 commits into from
Jan 14, 2024

Conversation

BioTurboNick
Copy link
Contributor

The fallback code that was written for #41099 is causing unintended issues with some inlined stack frames (one previous #51405, new #52709), since the main piece, linetable storage and lookup, was removed in #50546. Probably better to strip it all back to how it was previously, until it can all be revisited more fully.

Should be backported to 1.10.

Copy link
Member

@aviatesk aviatesk left a comment

Choose a reason for hiding this comment

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

LGTM. Would it be difficult to add tests?

@BioTurboNick
Copy link
Contributor Author

using LinearAlgebra
f(M::AbstractMatrix) = f!(copy(M))
function f!(M::AbstractMatrix)
    m, n = size(M)
    for j in 1:min(n, m)
        for i in max(1, j - 1):m
            g(M)
        end
    end
    M
end
g(::AbstractMatrix) = error()
g(::Matrix) = nothing

f(zeros(5, 5))
f(Diagonal(1:5))
#=
ERROR:
Stacktrace:
 [1] error()
   @ Base .\error.jl:44
 [2] g(::Diagonal{Int64, Vector{Int64}})
   @ Main .\REPL[4]:1
 [3] f!(M::Matrix{Float64}) # <------ wrong type
   @ Main .\REPL[11]:5 [inlined]
 [4] f(M::Diagonal{Int64, UnitRange{Int64}})
   @ Main .\REPL[2]:1
 [5] top-level scope
   @ REPL[13]:1
=#

I simplified a triggering condition as much as I could. It seems to actually be a pretty narrow set of conditions. I'll make a test for it shortly.

@BioTurboNick
Copy link
Contributor Author

Trying to see if there's a simpler way to create the effect than this. I notice the function, when it has a problem, has different effects than when it doesn't:

Problem: (!c,!e,!n,!t,!s,!m,+i)′
No problem: (+c,+e,!n,+t,+s,+m,+i)

If so, I'm wondering if I can force Julia to apply the former to a simpler function... @assume_effects doesn't seem to do it.

@vtjnash
Copy link
Member

vtjnash commented Jan 12, 2024

This hack should also be fully unnecessary once someone merges #52415 since that preserves the information natively (indeed, I think it already replaces it there with a corrected version)

@BioTurboNick
Copy link
Contributor Author

Oh sweet!

@BioTurboNick
Copy link
Contributor Author

In that case, maybe a test just isn't needed - it's sort of an edge case that only exists as an edge case because of a quirk of this code. It's extremely unlikely to resurface now that the approach I was previously trying to use won't be necessary.

@aviatesk
Copy link
Member

Ok, then let's move this forward as is.

@aviatesk aviatesk merged commit 46e6f23 into JuliaLang:master Jan 14, 2024
7 checks passed
aviatesk pushed a commit that referenced this pull request Jan 14, 2024
The fallback code that was written for #41099 is causing unintended
issues with some inlined stack frames (one previous #51405, new #52709),
since the main piece, linetable storage and lookup, was removed in
#50546. Probably better to strip it all back to how it was previously,
until it can all be revisited more fully.

Should be backported to 1.10.
@aviatesk aviatesk mentioned this pull request Jan 14, 2024
33 tasks
KristofferC added a commit that referenced this pull request Feb 6, 2024
Backported PRs:
- [x] #51095 <!-- Fix edge cases where inexact conversions to UInt don't
throw -->
- [x] #52583 <!-- Don't access parent of triangular matrix in powm -->
- [x] #52645 <!-- update --gcthreads section in command line options -->
- [x] #52423 <!-- update nthreads info in versioninfo -->
- [x] #52721 <!-- inference: Guard TypeVar special case against vararg
-->
- [x] #52637 <!-- fix finding bundled stdlibs even if they are e.g.
devved in an environment higher in the load path -->
- [x] #52752 <!-- staticdata: handle cycles in datatypes -->
- [x] #52758 <!-- use a Dict instead of an IdDict for caching of the
`cwstring` for Windows env variables -->
- [x] #51375 <!-- Insert hardcoded backlinks to stdlib doc pages -->
- [x] #52994 <!-- place work-stealing queue indices on different cache
lines to avoid false-sharing -->
- [x] #53015 <!-- Add type assertion in iterate for logicalindex -->
- [x] #53032 <!-- Fix a list in GC devdocs -->
- [x] #52748 
- [x] #52856 
- [x] #52878
- [x] #52754 
- [x] #52228
- [x] #52924
- [x] #52569 <!-- Fix GC rooting during rehashing of iddict -->
- [x] #52605 <!-- Default uplo in symmetric/hermitian -->
- [x] #52618 <!-- heap snapshot: add gc roots and gc finalist roots to
fix unrooted nodes -->
- [x] #52781 <!-- fix type-stability bugs in Ryu code -->
- [x] #53055 <!-- Profile: use full terminal cols to show function name
-->
- [x] #53096 
- [x] #53076 
- [x] #52841 <!-- Extensions: make loading of extensions independent of
what packages are in the sysimage -->
- [x] #52078 <!-- Replace `&hArr;` by `&harr;` in documentation -->
- [x] #53035 <!-- use proper cache-line size variable in work-stealing
queue -->
- [x] #53066 <!-- doc: replace harr HTML entity by unicode -->
- [x] #52996 <!-- Apple silicon has 128 byte alignment so fix our
defines to match -->
- [x] #53121 

Non-merged PRs with backport label:
- [ ] #52694 <!-- Reinstate similar for AbstractQ for backward
compatibility -->
- [ ] #51479 <!-- prevent code loading from lookin in the versioned
environment when building Julia -->
Drvi pushed a commit to RelationalAI/julia that referenced this pull request Jun 7, 2024
The fallback code that was written for JuliaLang#41099 is causing unintended
issues with some inlined stack frames (one previous JuliaLang#51405, new JuliaLang#52709),
since the main piece, linetable storage and lookup, was removed in
JuliaLang#50546. Probably better to strip it all back to how it was previously,
until it can all be revisited more fully.

Should be backported to 1.10.
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