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

ccall: Add probecall for user-defined probe points #45185

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

jpsamaroo
Copy link
Member

@jpsamaroo jpsamaroo commented May 4, 2022

Adds a new probecall calling convention to ccall, which, when compiled, generates a new USDT-compatible probe slot (and associated semaphore), which, when executed via ccall, calls the loaded probe with the provided arguments.

Todo:

@jpsamaroo jpsamaroo added this to the 1.9 milestone May 4, 2022
@jpsamaroo jpsamaroo added needs tests Unit tests are required for this change needs docs Documentation for this change is required labels May 4, 2022
@jpsamaroo jpsamaroo changed the title ccall: Add probecall for user-defined probes ccall: Add probecall for user-defined probe points May 4, 2022
base/compiler/tfuncs.jl Outdated Show resolved Hide resolved
src/intrinsics.h Outdated
@@ -101,6 +101,7 @@
/* c interface */ \
ADD_I(cglobal, 2) \
ALIAS(llvmcall, llvmcall) \
ALIAS(probecall, probecall) \
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
ALIAS(probecall, probecall) \

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we still need this to define Base.probecall?

Copy link
Member

Choose a reason for hiding this comment

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

but we don't need to define it? It's ccall("name", probecall). Base.llvmcall !== ccall("name", llvmcall)

Copy link
Member

Choose a reason for hiding this comment

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

Making this a new special function form hijacking onto ccall seems a terrible idea 😬

Copy link
Member

Choose a reason for hiding this comment

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

But turning it into a call ABI should be fine right? The reason is that for BpF we need to make sure the args are in the right registers

Copy link
Member

Choose a reason for hiding this comment

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

Making it a builtin function would likely be better. Regardless, need to figure out what this does in the interpreter also

Copy link
Member Author

Choose a reason for hiding this comment

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

Makes sense to me; I'll get it all properly working with ccall first, and then switch to a builtin.

For the interpreter, we can always dynamically lookup the probe and semaphore values from the runtime, so that should be pretty straightforward.

base/compiler/tfuncs.jl Outdated Show resolved Hide resolved
@KristofferC KristofferC removed this from the 1.9 milestone Nov 15, 2022
@vchuravy
Copy link
Member

On the profiling call today @gbaraldi and I were discussing that on could potentially use LLVM patchpoints

declare void @llvm.experimental.patchpoint.void(i64, i32, ptr, i32, ...)

define i32 @square(i32) local_unnamed_addr #0 {
    %2 = mul nsw i32 %0, %0
    call anyregcc void (i64, i32, i8*, i32, ...) @llvm.experimental.patchpoint.void(i64 78, i32 16, ptr null, i32 0)
    ret i32 %2
}

This will generate an elf section of _LLVM.Stackmaps that we would need to translate into staps format. Or we add an intrinsic in that style to LLVM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs docs Documentation for this change is required needs tests Unit tests are required for this change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

USDT probes are missing on master
4 participants