This is an archive of the discontinued LLVM Phabricator instance.

[Dexter] Add --source-dir-root flag
ClosedPublic

Authored by tbosch on Jun 5 2020, 4:39 PM.

Details

Summary

This allows to run dexter tests with separately compiled
binaries that are specified via --binary if the source file
location changed between compilation and dexter test run.

Diff Detail

Event Timeline

tbosch created this revision.Jun 5 2020, 4:39 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 5 2020, 4:39 PM
tbosch added a subscriber: cmtice.
tbosch added a project: debug-info.
tbosch added a subscriber: debug-info.

This looks great, thanks in particular for upping the testing game. As far as I understand it, this is adding a facade in front of the debugger API calls that normalises file paths according to the source root. Ping @TWeaver as he's worked on this most recently; otherwise LGTM.

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,

debuginfo-tests/dexter-tests/source-root-dir.cpp
1 ↗(On Diff #268963)

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.

debuginfo-tests/dexter/dex/debugger/DebuggerBase.py
189

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.

193

what will be effect here on windows platforms? have you considered the case where windows paths have '\' instead?

196

same comment about context.

198

same comment about context

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,

First off, thanks for the feedback!

I also wondered where the best place would be for this path mapping. However, I still think DebuggerBase is a good spot as the debuggers currently rely on knowing context.source_files. So whichever tool wants to use the debuggers will also have to fill context.source_files, and therefore also run in the problem of needing to remap paths to the ones used in the compilation unit. Moving this mapping into the test tool would prevent other tools from reusing it.

Also, moving this mapping into the test tool would require to make the source_files be relative paths (to the source_root_dir) before calling the debuggers.
This leads to a future me now needing to remember what kind of path is used where: An absolute file system path (test tool), a path relative to source_root_dir (DebuggerController), or a debug info path (Debugger).

WDYT?

TWeaver added a comment.EditedJun 11 2020, 10:12 AM

First off, thanks for the feedback!

I also wondered where the best place would be for this path mapping. However, I still think DebuggerBase is a good spot as the debuggers currently rely on knowing context.source_files. So whichever tool wants to use the debuggers will also have to fill context.source_files, and therefore also run in the problem of needing to remap paths to the ones used in the compilation unit. Moving this mapping into the test tool would prevent other tools from reusing it.

Also, moving this mapping into the test tool would require to make the source_files be relative paths (to the source_root_dir) before calling the debuggers.
This leads to a future me now needing to remember what kind of path is used where: An absolute file system path (test tool), a path relative to source_root_dir (DebuggerController), or a debug info path (Debugger).

WDYT?

After musing on it I do agree that having this at the point of use is probably better for reuse of this code and does remove the need for propagating this behaviour around other parts of Dexter too. Happy for this to go ahead from a 'this goes in debuggerbase' point of view now.

thanks for the explanation.

tbosch updated this revision to Diff 270191.Jun 11 2020, 11:34 AM
tbosch marked 9 inline comments as done.

Applied first round of comments.

debuginfo-tests/dexter-tests/source-root-dir.cpp
1 ↗(On Diff #268963)

Ah, good point. Moved.

debuginfo-tests/dexter/dex/debugger/DebuggerBase.py
189

I am reading the options now in init. I can't read the option values yet as the debugger is used for creating options for the potential debuggers.

193

I don't have a windows machine to test on :-(.
Changed this to use os.path.separator instead though.

Ok, tried to apply your comments. Could you take another look?

@TWeaver @jmorse Is this good to go now from your side?

@TWeaver @jmorse It looks like both of you gave an implicit LGTM, is that correct?

I will wait one more day then submit this change so that it doesn't bit rot.

jmorse accepted this revision.Jun 18 2020, 8:54 AM

(Ah yeah, Tom is away for a bit),

LGTM, and thanks again for the patch!

This revision is now accepted and ready to land.Jun 18 2020, 8:54 AM
This revision was automatically updated to reflect the committed changes.

@TWeaver @jmorse It looks like both of you gave an implicit LGTM, is that correct?

I will wait one more day then submit this change so that it doesn't bit rot.

(aside: Generally once something's been sent for review it should not be committed until approved (otherwise it muddies the waters a bit - it was sent for review because the author felt it needed review, but then committed without it?))

@TWeaver @jmorse It looks like both of you gave an implicit LGTM, is that correct?

I will wait one more day then submit this change so that it doesn't bit rot.

(aside: Generally once something's been sent for review it should not be committed until approved (otherwise it muddies the waters a bit - it was sent for review because the author felt it needed review, but then committed without it?))

Acknowledged