Reject a TTD launch with no trace specified (Fixes #910) - #1145
Conversation
The TTD adapter replays a recorded trace, but nothing checked that one was actually configured, so launching without a trace path failed deep inside the engine with an opaque "OpenDumpFile failed: 0x..." HRESULT. Validate the trace path in AdapterSettingsDialog::apply() and keep the dialog open with an explanation instead of accepting it. The settings view writes each value as it is edited, so the current value can simply be read back; no changes to SettingsView are needed. The check is limited to the launch group with the DBGENG_TTD adapter selected so the other flows are unaffected. Also check the trace path in DbgEngTTDAdapter::ExecuteWithArgsInternal before starting the engine, which covers the headless and scripted paths as well as the launches where the settings dialog is skipped (#1133). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
fuzyll
left a comment
fuzyll
left a comment
There was a problem hiding this comment.
Looks good on static review, but unless something earlier rejects directories that I missed, I think just using std::filesystem::exists is not quite enough in two places.
| } | ||
|
|
||
| std::error_code ec; | ||
| if (!std::filesystem::exists(tracePath, ec)) |
There was a problem hiding this comment.
Need to check that this is a file (e.g. std::filesystem::is_regular_file) as well, I think?
There was a problem hiding this comment.
Yeah we should check for regular files. I will fix it
| // HRESULT, so check it up front. The UI blocks this in the adapter settings dialog; this covers the | ||
| // headless and scripted paths, plus the launches where that dialog is skipped. | ||
| std::error_code ec; | ||
| if (tracePath.empty() || !std::filesystem::exists(tracePath, ec)) |
There was a problem hiding this comment.
Need to check that this is a file (e.g. std::filesystem::is_regular_file) as well, I think?
std::filesystem::exists accepts directories, so point the check at is_regular_file instead and report "is not a file" separately from "does not exist" so the message still fits what the user set. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Merged with approval. I noticed that we can make it one step further by checking the given trace file is not a valid trace file, e.g., by checking the magic header of the trace against the known good value, or at least reject it when a PE is selected. A common foot-gun is the user would incorrectly select the PE file instead of the trace file before launching. But we need to be sure to account for the possibility that MS would change the trace file magic without us knowing, so we would also need to have a setting to skip this check. So much said, we would better leave it as a future enhancement |
2 participants
Fixes #910.
This PR fails the TTD launch attempt and gives a meaningful error message (rather than the cryptic OpenDumpFile failed with xxxxx)
The check are both added in the UI and the debug adapter