This is an archive of the discontinued LLVM Phabricator instance.

Add a GDB/LLDB interface for interactive debugging of MLIR Actions
ClosedPublic

Authored by mehdi_amini on Feb 25 2023, 10:19 PM.

Details

Summary

This includes a small runtime acting as callback for the ExecutionEngine
and a C API that makes it possible to control from the debugger.

A python script for LLDB is included that hook a new mlir subcommand
and allows to set breakpoints and inspect the current action, the context
and the stack.

Diff Detail

Event Timeline

mehdi_amini created this revision.Feb 25 2023, 10:19 PM
mehdi_amini requested review of this revision.Feb 25 2023, 10:19 PM
zero9178 added inline comments.Feb 26 2023, 3:36 AM
mlir/lib/Debug/GdbDebugExecutionContextHook.cpp
300 ↗(On Diff #500507)

__attribute__((noinline)) is not a thing when compiling with MSVC. This should use LLVM_ATTRIBUTE_NOINLINE from llvm/Support/Compilers.h

Use LLVM_ATTRIBUTE_NOINLINE

mehdi_amini marked an inline comment as done.Feb 26 2023, 6:15 AM

Thanks for catching this.

Mogball added inline comments.
mlir/include/mlir/Debug/GdbDebugExecutionContextHook.h
1 ↗(On Diff #500574)

this only works for gdb?

mlir/lib/Debug/GdbDebugExecutionContextHook.cpp
305 ↗(On Diff #500574)

isn't there an LLVM_EXPORT thing that can be used in place of this?

mehdi_amini added inline comments.Mar 12 2023, 4:32 PM
mlir/include/mlir/Debug/GdbDebugExecutionContextHook.h
1 ↗(On Diff #500574)

If you saw the demo, you may have noticed it was done with lldb :)

mlir/lib/Debug/GdbDebugExecutionContextHook.cpp
305 ↗(On Diff #500574)

Nothing portable that I know of?

I think for this revision we definitely need some high level usage docs

mlir/include/mlir/Debug/GdbDebugExecutionContextHook.h
1 ↗(On Diff #500574)

Can we avoid using the name GDB? It's somewhat confusing in the modern multi-debugger world.

22–23 ↗(On Diff #500574)

Why lowercase names?

mlir/lib/Debug/GdbDebugExecutionContextHook.cpp
12 ↗(On Diff #500574)

Please drop the commented out code. Also it feels like a lot of these includes could be dropped.

31–36 ↗(On Diff #500574)

Can you document this a bit?

47 ↗(On Diff #500574)

Please drop the newline here.

305 ↗(On Diff #500574)

The LLVM dump methods (and what we use for the mlir visualizers) is: LLVM_ATTRIBUTE_NOINLINE LLVM_ATTRIBUTE_USED

jpienaar added inline comments.Mar 24 2023, 7:00 PM
mlir/include/mlir/Debug/GdbDebugExecutionContextHook.h
1 ↗(On Diff #500574)

(i was even wondering about DAP)

mehdi_amini marked 7 inline comments as done.

Rebase and address comments

mlir/lib/Debug/GdbDebugExecutionContextHook.cpp
305 ↗(On Diff #500574)

I am hesitant to rely on LLVM_ATTRIBUTE_USED which is non-standard (not supported by MSVC?) for something that isn't a "debugging" things like the dump methods but something core to the feature here.

tschuett added inline comments.
mlir/lib/Debug/DebuggerExecutionContextHook.cpp
46

You need LLVM_THREAD_LOCAL.

Use LLVM_THREAD_LOCAL

mlir/lib/Debug/DebuggerExecutionContextHook.cpp
46

Interesting, I didn't know this one (or forgot about it, we don't use thread_local often).

That said I see one use of LLVM_THREAD_LOCAL in MLIR, vs >=15 used of thread_local as a keyword. Seems like right now we need a mass migration of MLIR!

One more LLVM_THREAD_LOCAL

rebase and address comments

mehdi_amini marked an inline comment as done.

I think for this revision we definitely need some high level usage docs

This is now done, in a child revision: https://reviews.llvm.org/D149037

rriddle accepted this revision.Apr 23 2023, 11:05 PM
rriddle added inline comments.
mlir/lib/Debug/DebuggerExecutionContextHook.cpp
46

Is that advice actually up to date? I've not seen LLVM_THREAD_LOCAL recommended ever over thread_local, and only ever see thread_local in new code.

127

Extra newline here.

mlir/lib/Debug/ExecutionContext.cpp
34–38

llvm::interleave?

mlir/utils/lldb-scripts/action_debugging.py
51
This revision is now accepted and ready to land.Apr 23 2023, 11:05 PM
mehdi_amini added inline comments.Apr 23 2023, 11:06 PM
mlir/lib/Debug/DebuggerExecutionContextHook.cpp
46

I would think that the main advantage may be supporting the LLVM config flag to disable threading entirely?

There are actually two keywords for thread local storage and it is about with or without threads.

There are actually two keywords for thread local storage and it is about with or without threads.

Gotcha. I don't think we are consistent with using those at all.

mehdi_amini marked 3 inline comments as done.

Address comments

This revision was landed with ongoing or failed builds.Apr 24 2023, 2:34 PM
This revision was automatically updated to reflect the committed changes.