ord: implement whitelist-based Tcl command logging#10686
Conversation
Signed-off-by: Sparsh Karna <sparsh2005karna@gmail.com>
There was a problem hiding this comment.
Code Review
This pull request implements Tcl command tracing to log newly registered OpenRoad commands while filtering out standard Tcl built-ins, along with a corresponding integration test. The feedback highlights two important improvements: first, passing the TCL_ALLOW_INLINE_COMPILATION flag to Tcl_CreateObjTrace to avoid severe performance degradation caused by disabling bytecode compilation; second, stripping the leading :: namespace prefix from command names to ensure fully qualified commands are correctly matched against the whitelist.
| Tcl_CreateObjTrace( | ||
| interp, 0, 0, logCommandCallback, trace_data, deleteCommandTrace); |
There was a problem hiding this comment.
Without the TCL_ALLOW_INLINE_COMPILATION flag, Tcl disables bytecode compilation for all commands executed in the interpreter while the trace is active. This can cause a severe performance degradation (often 10x or more) for Tcl scripts, especially those containing loops or heavy computations.
Since the callback only uses objc and objv (which are fully populated and valid even with inline compilation), enabling TCL_ALLOW_INLINE_COMPILATION is completely safe and preserves Tcl's execution performance.
Tcl_CreateObjTrace(
interp, 0, TCL_ALLOW_INLINE_COMPILATION, logCommandCallback, trace_data, deleteCommandTrace);| const char* cmd_name = Tcl_GetString(objv[0]); | ||
| if (cmd_name == nullptr || data->whitelist.count(cmd_name) == 0) { | ||
| return TCL_OK; | ||
| } |
There was a problem hiding this comment.
If a user invokes an OpenROAD command using a leading namespace qualifier (for example, ::read_lef instead of read_lef), Tcl_GetString(objv[0]) will return the fully qualified name with the leading colons. Since the whitelist contains the unqualified command names returned by info commands, the lookup data->whitelist.count(cmd_name) will fail, and the command will not be logged.
We can make this robust by stripping the leading :: prefix from the command name before performing the whitelist lookup.
const char* cmd_name = Tcl_GetString(objv[0]);
if (cmd_name == nullptr) {
return TCL_OK;
}
if (std::strncmp(cmd_name, "::", 2) == 0) {
cmd_name += 2;
}
if (data->whitelist.count(cmd_name) == 0) {
return TCL_OK;
}Signed-off-by: Sparsh Karna <sparsh2005karna@gmail.com>
Signed-off-by: Sparsh Karna <sparsh2005karna@gmail.com>
Signed-off-by: Sparsh Karna <sparsh2005karna@gmail.com>
Signed-off-by: Sparsh Karna <sparsh2005karna@gmail.com>
Signed-off-by: Sparsh Karna <sparsh2005karna@gmail.com>
Fixes a strict type mismatch in the macOS CI where Tcl_CreateObjTrace requires the callback's objc parameter to be an int rather than a Tcl_Size. Signed-off-by: Sparsh Karna <sparsh2005karna@gmail.com>
|
hey @maliberty could you please review this |
Description
This PR implements Tcl command logging (Issue #3539) by logging every registered OpenROAD command executed by the user to the file specified by the
-logflag, ascmd: <command>with its variables expanded.To avoid the noise of internal implementation details (such as
set,if, and internal proc bodies) flooding the log, this implementation utilizes a whitelist approach.Key Changes
info commandsbefore and afterord::initOpenRoadis called. The difference forms a robust whitelist of OpenROAD-registered commands, explicitly excluding Tcl builtins.Tcl_CreateObjTraceto intercept commands before they are executed. If the command name is present in the whitelist, the full command string is reconstructed usingTcl_GetStringon the already-expandedobjvarguments and logged vialogger->report().-logparameter is provided (log_filename != nullptr). This ensures that standard CTest regression runs (which capture stdout via redirection rather than the-logflag) do not fail theircompare_logfilechecks due to unexpectedcmd:lines.cmd_log_trace) that spawns an OpenROAD subprocess with-log, executes a mix of whitelisted commands and internal Tcl builtins, and automatically reads back its own log to verify that only the correct commands are traced.Prerequisites
git commit -s).Related Issues