This is an archive of the discontinued LLVM Phabricator instance.

[Dexter] Add option to pass a Visual Studio solution instead of a binary
ClosedPublic

Authored by StephenTozer on Sep 21 2021, 6:39 AM.

Details

Summary

This patch allows a visual studio solution file to be passed directly into Dexter, instead of using a pre-built binary and a small internal solution file with template arguments. This is primarily to allow launching an application that has specific launch configuration requirements, without needing all the details of this configuration to be built directly into Dexter or adding a config file that simply duplicates existing settings in the VS solution.

Diff Detail

Event Timeline

StephenTozer requested review of this revision.Sep 21 2021, 6:39 AM
StephenTozer created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptSep 21 2021, 6:39 AM

LGTM, just one inline question.

cross-project-tests/debuginfo-tests/dexter/dex/debugger/DebuggerControllers/DebuggerControllerBase.py
2–1

Is this (and the necessity for the new init_success attribute) a bug-fix?

StephenTozer added inline comments.Oct 7 2021, 4:38 AM
cross-project-tests/debuginfo-tests/dexter/dex/debugger/DebuggerControllers/DebuggerControllerBase.py
2–1

The current error handling approach seems somewhat unmaintained - it's hard to say whether this qualifies as a "bug fix" or just implementing error handling for the behaviour in this specific patch.

Orlando added inline comments.Oct 7 2021, 5:24 AM
cross-project-tests/debuginfo-tests/dexter/dex/debugger/DebuggerControllers/DebuggerControllerBase.py
2–1

What happens without this patch if the vs solution cannot be created (i.e. when _custom_init raises LoadDebuggerException)? I'm just trying to understand the impact of this part of the patch.

The previous version of the patch made use of the existing _loading_error field in a way that was inconsistent with existing code; existing uses have been fixed up so that the field is used consistently.

In short summary, the field was assigned inconsistent value types - on initialization it is assigned an exception object directly, in all other assignments it is either None or the value of sys.exc_info(). This patch assigned an exception directly, consistent with the initial value, but it appears as though the initial assignment was never actually used (and would cause an error if it was). Also, while we would assign self._loading_error = sys.exc_info() when we encountered an error when initializing or entering the debugger, we only used the loading error during initialization, not afterwards (making the second assignment meaningless as well).

The fix for this is to set the value to None initially instead of a never-used exception object, and use the loading_error property consistently between initializing and entering the debugger.

Do we need both _loading_error and init_success? I might be missing something but it looks like the property is_available (which uses _loading_error) currently covers the use case of init_success.

Do we need both _loading_error and init_success? I might be missing something but it looks like the property is_available (which uses _loading_error) currently covers the use case of init_success.

Probably not - it basically amounts to keeping state in the Debugger that records where we failed rather than just what the error was. With that said, I don't think we really use that at all right now, since error handling is just "Init -> Check for failure -> Setup -> Check for failure", so we could probably do fine without it.

Remove init_success field.

Orlando accepted this revision.Oct 8 2021, 9:17 AM

LGTM

This revision is now accepted and ready to land.Oct 8 2021, 9:17 AM
This revision was landed with ongoing or failed builds.Oct 8 2021, 9:43 AM
This revision was automatically updated to reflect the committed changes.