Skip to content

dynamic tracing compiler json result support #9760

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.

Sign up for GitHub

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

Closed
AdvaitNaik wants to merge 2 commits into from
+31 −10
Closed

dynamic tracing compiler json result support #9760

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9278ee0
dtrace json output support
AdvaitNaik Apr 29, 2026
824576f
address comments
AdvaitNaik Apr 30, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/runtime_src/core/common/aiebu
Open in desktop
Submodule aiebu updated 64 files
+2 −2 .github/workflows/ci.yml
+0 −4 .gitmodules
+1 −2 README.rst
+2 −2 build/build.sh
+4 −5 build/build22.bat
+10 −1 cmake/cpack.cmake
+3 −0 cmake/settings.cmake
+0 −1 lib/aie-rt
+20 −12 src/cpp/CMakeLists.txt
+14 −8 src/cpp/aie-rt/README.rst
+170 −50 src/cpp/analyzer/transform_manager.cpp
+76 −4 src/cpp/analyzer/transform_manager.h
+47 −1 src/cpp/assembler/aiebu_assembler.cpp
+1 −0 src/cpp/assembler/assembler.cpp
+3 −2 src/cpp/common/assembler_state.cpp
+10 −5 src/cpp/common/assembler_state.h
+1 −2 src/cpp/common/regex_wrapper.h
+2 −6 src/cpp/dtrace/CMakeLists.txt
+181 −46 src/cpp/dtrace/action/action_control.h
+18 −7 src/cpp/dtrace/action/break.cpp
+48 −11 src/cpp/dtrace/action/count.cpp
+48 −12 src/cpp/dtrace/action/host_timestamp.cpp
+58 −19 src/cpp/dtrace/action/host_timestamps.cpp
+44 −24 src/cpp/dtrace/action/mask_write_register.cpp
+21 −8 src/cpp/dtrace/action/operation.cpp
+24 −11 src/cpp/dtrace/action/print.cpp
+33 −20 src/cpp/dtrace/action/printa.cpp
+20 −11 src/cpp/dtrace/action/profile.cpp
+52 −15 src/cpp/dtrace/action/read_handshake.cpp
+63 −19 src/cpp/dtrace/action/read_mem.cpp
+49 −12 src/cpp/dtrace/action/read_register.cpp
+20 −9 src/cpp/dtrace/action/sleep.cpp
+48 −12 src/cpp/dtrace/action/timestamp.cpp
+48 −11 src/cpp/dtrace/action/timestamp32.cpp
+56 −18 src/cpp/dtrace/action/timestamps.cpp
+56 −16 src/cpp/dtrace/action/timestamps32.cpp
+46 −18 src/cpp/dtrace/action/write_handshake.cpp
+21 −10 src/cpp/dtrace/action/write_mem.cpp
+22 −10 src/cpp/dtrace/action/write_register.cpp
+80 −44 src/cpp/dtrace/control/control.cpp
+2 −1 src/cpp/dtrace/control/control.h
+25 −6 src/cpp/dtrace/dtrace.cpp
+20 −5 src/cpp/dtrace/dtrace.h
+89 −55 src/cpp/dtrace/utils.h
+1 −1 src/cpp/elf/aie_elf_constants.h
+94 −28 src/cpp/encoder/aie2ps/aie2ps_encoder.cpp
+43 −8 src/cpp/encoder/aie2ps/aie2ps_encoder.h
+4 −2 src/cpp/encoder/aie4/aie4_encoder.h
+4 −0 src/cpp/encoder/encoder.h
+10 −5 src/cpp/ops/ops.cpp
+1 −1 src/cpp/preprocessor/aie2/aie2_blob_preprocessor_input.cpp
+1 −7 src/cpp/preprocessor/aie2ps/aie2ps_preprocessor.h
+1 −1 src/cpp/preprocessor/aie2ps/aie2ps_preprocessor_input.h
+1 −1 src/cpp/preprocessor/aie4/aie4_preprocessor.h
+122 −43 src/cpp/preprocessor/asm/asm_parser.cpp
+74 −45 src/cpp/preprocessor/asm/asm_parser.h
+2 −1 src/cpp/preprocessor/asm/pager.cpp
+13 −2 src/cpp/preprocessor/preprocessor_input.h
+1 −1 test/aie4-ctrlcode/1col_preempt/gold_target.md5
+4 −2 test/aie4-ctrlcode/compile_time/fused_hw_package_subgraph_0/CMakeLists.txt
+1 −1 test/aie4-ctrlcode/compile_time/fused_hw_package_subgraph_0/gold.md5
+3 −2 test/cmake-test/CMakeLists.txt
+15 −38 test/dtrace_test/CMakeLists.txt
+31 −108 test/dtrace_test/dtrace_test.cpp
28 changes: 21 additions & 7 deletions src/runtime_src/core/common/api/xrt_module.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -719,9 +719,19 @@ class module_run_aie_gen2_plus : public module_run
dtrace_util() : dtrace_handle(nullptr, destroy_dtrace_handle) {}

dtrace_util(const std::string& ctrl_file_path, const std::string& map_data,
uint32_t log_level)
: dtrace_handle(create_dtrace_handle(ctrl_file_path.c_str(), map_data.c_str(), log_level),
destroy_dtrace_handle) {}
uint32_t log_level, uint32_t output_fmt)
: dtrace_handle(nullptr, destroy_dtrace_handle)
{
Comment on lines 721 to +724
Copy link

Copilot AI Apr 29, 2026

Choose a reason for hiding this comment

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

dtrace_config stores ctrl_file_path.c_str() / map_data.c_str() pointers, but the arguments passed from create_dtrace_util() are a local std::string path and a temporary m_config.dump_buf.to_string(). After the constructor call/assignment completes those underlying string buffers are destroyed, leaving dtrace_config (and potentially the dtrace handle) with dangling pointers. Please persist the strings inside dtrace_util (e.g., std::string ctrl_path_storage; std::string map_data_storage; and set dtrace_config to point to their c_str()), or otherwise ensure create_dtrace_handle() does not retain the pointers beyond the call.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Collaborator Author

@AdvaitNaik AdvaitNaik Apr 30, 2026

Choose a reason for hiding this comment

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

In this flow, create_dtrace_handle does not retain the dtrace_config pointer. It constructs dtrace::control using those inputs, and control takes std::string/const std::string&, so the data is copied during handle creation. That means there is no dangling-pointer use after the constructor call

Copy link
Copy Markdown
Collaborator

@stsoe stsoe Apr 30, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

The code is not clean, the copilot comment shows why it is not structured correctly, please redo. dtrace_config is a data member and it is definitely holding on to stale pointers after construction.

Copy link
Copy Markdown
Collaborator Author

@AdvaitNaik AdvaitNaik Apr 30, 2026

Choose a reason for hiding this comment

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

done

Copy link
Copy Markdown
Collaborator

@stsoe stsoe Apr 30, 2026

Choose a reason for hiding this comment

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

This code still makes no sense. aiebu/.../control.h control ctor takes std::string, you are moving to c_str() only to go back to std::string.

I don't want a constructor body in xrt_module, you have to fix create_dtrace_handle to accept the types it needs. You are faking it by introducing the dtrace_config struct, but then running into problems with what you store in this struct. You could in fact store std::string in config_t, and apply the proper moves in the right places, or you could have create_dtrace_handle accept the types it needs and drop config_t. Moving from std::string -> c_str -> std::string is just weird.

Let's back up and review the changes you made to AIEBU in Xilinx/aiebu#278. For example, I see that control ctor take script_file by value and map_data by const &, why is the former by value, as far as I can see it is not modified in the function. You should request Sonal's review on AIEBU changes and maybe me too (even as I don't really want review AIEBU).

I will accept the xrt_module changes without the dtrace_config struct hack. I will need much time to review the AIEBU changes, but if Sonal approves these I am okay.

Copy link
Copy Markdown
Collaborator

@xuhz xuhz Apr 30, 2026

Choose a reason for hiding this comment

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

the reason there is std::string -> c_str -> std::string is, we want to provide C API, but the implementation in aiebu is cpp and caller is also cpp.
the reason we want to provide C API is there is potential use case in embedded baremetal
so probably here we don't need the config_t, and pass all required to create_handle?

Copy link
Copy Markdown
Collaborator Author

@AdvaitNaik AdvaitNaik Apr 30, 2026

Choose a reason for hiding this comment

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

config_t struct was also introduced for future extensibility, adding new parameters for dtrace handle creation then doesn't require changing the API signature

Copy link
Copy Markdown
Collaborator

@stsoe stsoe Apr 30, 2026

Choose a reason for hiding this comment

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

I understand your motivation for config_t, but what does it buy you? You add a new data member to the struct, you change upstream code to populate the member; or you change a function signature and change upstream code to use the new signature. Either way two changes. A config_t struct doesn't buy you anything in this case, that doesn't mean it's wrong or bad, except it is bad in its current use.

// Configuration for dtrace handle creation
const dtrace_config_t dtrace_config
{
ctrl_file_path.c_str(),
map_data.c_str(),
log_level,
output_fmt
};
dtrace_handle.reset(create_dtrace_handle(&dtrace_config));
}
};
dtrace_util m_dtrace;

Expand Down Expand Up @@ -749,10 +759,14 @@ class module_run_aie_gen2_plus : public module_run
return false;
}

// log level 0: error, 1: warning, 2: info
auto log_level = static_cast<uint32_t>(xrt_core::config::get_dtrace_log_level());
log_level = (log_level > 3) ? 3U : (log_level < 1) ? 1U : log_level;
log_level = (log_level > 2) ? 2U : log_level;

// output format 0: python, 1: json
uint32_t output_fmt = xrt_core::config::get_dtrace_output_json_format() ? 1U : 0U;

m_dtrace = dtrace_util(path, m_config.dump_buf.to_string(), log_level);
m_dtrace = dtrace_util(path, m_config.dump_buf.to_string(), log_level, output_fmt);
if (!m_dtrace.dtrace_handle.get()) {
xrt_core::message::send(xrt_core::message::severity_level::debug, "xrt_module",
"[dtrace] : Failed to get dtrace handle");
Expand Down Expand Up @@ -1135,11 +1149,11 @@ class module_run_aie_gen2_plus : public module_run

try {
// dtrace output is dumped into current working directory
// output is a python file
// output is a python/json file
std::string result_file_path = std::filesystem::current_path().string()
+ "/dtrace_dump"
+ postfix
+ ".py";
+ (xrt_core::config::get_dtrace_output_json_format() ? ".json" : ".py");

get_dtrace_result_file(m_dtrace.dtrace_handle.get(), result_file_path.c_str());

Expand Down
9 changes: 8 additions & 1 deletion src/runtime_src/core/common/config_reader.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1162,7 +1162,14 @@ get_dtrace_control_file_path()
inline unsigned int
get_dtrace_log_level()
{
static unsigned int value = detail::get_uint_value("Debug.dtrace_log_level", 1);
static unsigned int value = detail::get_uint_value("Debug.dtrace_log_level", 0);
return value;
Comment thread
AdvaitNaik marked this conversation as resolved.
}

inline bool
get_dtrace_output_json_format()
{
static bool value = detail::get_bool_value("Debug.dtrace_output_json_format", false);
return value;
}

Expand Down
2 changes: 1 addition & 1 deletion src/runtime_src/core/common/uc_log_schema.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ uc_log_schema = {
{85, "Page-in last elf to slot %u\n"},
{86, "load elf skip\n"},
{87, "load elf ...\n"},
{88, "fw_state: 0x%x\n"},
{88, "fw_state before hang is detected: 0x%x\n"},
{89, "qh: 0x%x ql: 0x%x\n"},
{90, "mpnpu base time: high 0x%x low 0x%x\n"},
{91, "shim dma mm2s status: addr 0x%x value 0x%x\n"},
Expand Down
Loading