Skip to content

Revert "Revert "Merge pull request #15860 from JuliaLang/kf/tracing"" #15890

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

Merged
merged 1 commit into from
Apr 16, 2016

Conversation

Keno
Copy link
Member

@Keno Keno commented Apr 16, 2016

This was silently reverted by #15779.

@Keno Keno merged commit c176eac into master Apr 16, 2016
@vtjnash
Copy link
Member

vtjnash commented Apr 16, 2016

i'm concerned about the usage of this now that i can see JuliaDebug/Gallium.jl@a14a67b. the one thing you must not do with the returned lambda object is look at its llvm object (there are a few code paths where that can assert). so i don't see how it can be safely used for debugging.

also you need to update jltypes.c now

also, 810e53f was unrelated, but I'm not sure it's a good idea to assume without testing that jl_tparam0 in jl_get_llvmf is a datatype.

@Keno
Copy link
Member Author

Keno commented Apr 16, 2016

i'm concerned about the usage of this now that i can see JuliaDebug/Gallium.jl@a14a67b. the one thing you must not do with the returned lambda object is look at its llvm object (there are a few code paths where that can assert). so i don't see how it can be safely used for debugging.

Returned lambda object where?

also you need to update jltypes.c now

in what way?

also, 810e53f was unrelated, but I'm not sure it's a good idea to assume without testing that jl_tparam0 in jl_get_llvmf is a datatype.

Adding an assert to this extent seems reasonable.

@yuyichao yuyichao deleted the kf/tracing2 branch April 16, 2016 14:03
@vtjnash
Copy link
Member

vtjnash commented Apr 16, 2016

from cache_method / jl_linfo_tracer

in what way?

there's an extra field in jl_method_t, it needs to be added to the description of the type

@Keno
Copy link
Member Author

Keno commented Apr 16, 2016

Which code path is it that would assert. Would it be reasonable to add an extra tracing flag to the lambda info that gets triggered when that lambda info gets compiled? That flags could be set from jl_linfo_tracer, and only set the breakpoint after the thing is actually compiled?

I'm not sure I want this flag to be accessible from julia.

@vtjnash
Copy link
Member

vtjnash commented Apr 16, 2016

you can give it a name of "" to make it harder to access

that's sounds like a possibility. we might be able to attach NotifyObjectEmitted to Julia? that's a tricky time too though, since calling jl_get_llvmf in there will hit llvm assertions (recursive compilation and double-finalizing the RTDyld)

codegen may decide to assert if inference hasn't decided to finish yet (for example, if cache_method is called from within inference), or if the function is being compiled recursively and forms part of a cycle (because it would force resolution of the function before the rest of the cycle was ready)

@Keno
Copy link
Member Author

Keno commented Apr 16, 2016

We could record all traced linfos in NotifyObjectEmitted and delay calling the callback until we're out of the JIT again, no?

@vtjnash
Copy link
Member

vtjnash commented Apr 16, 2016

that sounds plausible. i don't know off-hand how many entry points we have into the JIT, but I don't believe it could be that many (c-function, c-alias, and fptr?)

@Keno
Copy link
Member Author

Keno commented Apr 16, 2016

Don't they all go through getAddressForFunction?

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.

2 participants