[PATCH] D81319: [Dexter] Add --source-dir-root flag

[Tom Weaver via Phabricator via llvm-commits]

TWeaver added a comment.

Hey and thanks for this!

If I understand this correctly, this patch is trying to solve the problem of source files being moved from the path/dir that they were originally in to a new dir after the binary we pass via --binary has been compiled. This problem occurs because the file in which we store our dexter commands is also the file path associated with the commands. If we move the source file, we change the expectations and these may no longer align with the pre-compiled binary.

Whilst I have no misgivings about the way in which this problem is solved, using a --source-dir option, I am, however, hesitant to condone the implementation being in DebuggerBase. DebuggerBase and the subsequent debuggers' jobs are concerned with exposing debugger features, not with contending with the test environment in which dexter is run.

As such, my preference would be a solution that handles this case lower in the stack, preferably in the Test tool, which is concerned with handling the test environment.

You could potentially do the external_to_debug mapping before passing context to the debugger controller. run the debugger and then remap the debug_to_external locations in the test tool before passing the watch data off to the heuristic scorer.

Please let me know if there's any issues or if you have any concerns yourself.

Thanks again,



================
Comment at: debuginfo-tests/dexter-tests/source-root-dir.cpp:1
+// REQUIRES: lldb
+// UNSUPPORTED: system-windows
----------------
the dexter-tests folder is for debug-info tests that make use of dexter as a tool.

from the looks of it, this test is checking the new feature works?

if that's the case, this test should be in the dexter/feature-tests folder, in an appropriate place.


================
Comment at: debuginfo-tests/dexter/dex/debugger/DebuggerBase.py:183
+    def _external_to_debug_path(self, path):
+        root_dir = self.context.options.source_root_dir
+        if not root_dir or not path:
----------------
One of our goals with dexter is to eradicate the over passing and over use of context throughout the code base.

There's a lot of work to do in this area and it may never happen but, one of the things we can do to ease the burden for any work we do in the future around this area is to do the following:

unpack anything you intend to use from context into a newly defined self.<thing_from_context> in the init method of the class that uses it.

use the new self.<thing_from_context> that you unpacked instead of self.context.<thing_from_context>.

This will hopefully reduce the burden in parsing long member look up chains as seen on this line.

It also means that, in future, when we look to remove context, we can know which bits of the context are required in the class without having to scout through the whole code base to find all the uses of it.


================
Comment at: debuginfo-tests/dexter/dex/debugger/DebuggerBase.py:187
+        assert path.startswith(root_dir)
+        return path[len(root_dir):].lstrip('/')
+
----------------
what will be effect here on windows platforms? have you considered the case where windows paths have '\' instead?


================
Comment at: debuginfo-tests/dexter/dex/debugger/DebuggerBase.py:190
+    def _debug_to_external_path(self, path):
+        if not path or not self.context.options.source_root_dir:
+            return path
----------------
same comment about context.


================
Comment at: debuginfo-tests/dexter/dex/debugger/DebuggerBase.py:192
+            return path
+        for file in self.context.options.source_files:
+            if path.endswith(self._external_to_debug_path(file)):
----------------
same comment about context


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81319/new/

https://reviews.llvm.org/D81319