This is an archive of the discontinued LLVM Phabricator instance.

[Dexter] Implement DexDeclareFile, a new Dexter command
ClosedPublic

Authored by TWeaver on Mar 31 2021, 4:37 AM.

Details

Summary

DexDeclareFile allows test producers to write test files with .dex extensions that
contain pure dexter commands.

.dex file commands do not need to be commented out like they do when written
inline within test source files.

DexDeclareFile commands are declarative in behaviour, they state that any Dexter
command seen from this point on will have its path attribute set to the path
declared in the DexDeclareFile command.

Provided in this patch is in the implementation of DexDeclareFile and 4 tests.

Diff Detail

Event Timeline

TWeaver requested review of this revision.Mar 31 2021, 4:37 AM
TWeaver created this revision.

LGTM with a few tiny nits, but please wait for a +1 from someone else too.

debuginfo-tests/dexter/Commands.md
229

Changes -> Change (or maybe Set) fits the existing style of this document better imo, but this isn't very important.

debuginfo-tests/dexter/dex/command/ParseCommand.py
264

Should cmd_path get normalized with os.path.normcase here since the user can spell the path name in a few different ways on windows? Unhelpfully I can't remember exactly why it was, but when tinkering with this patch, at one point I did have to make that change.

Does everything work if the DexDeclareFile commands use\\annOying/PathSpelling (i.e. a mix of case and slashes that doesn't match the "canonical" spelling)? Maybe this deserves a test.

debuginfo-tests/dexter/dex/tools/TestToolBase.py
103

Tiny nit: comments should probably follow the llvm style (comments added here don't start with capitals but should).

jmorse accepted this revision.Apr 6 2021, 9:45 AM

LGTM, with a couple of nits, plus Orlandos comments should be addressed.

The precompiled binary test is great, that should get us going in the right direction.

debuginfo-tests/dexter/dex/tools/test/Tool.py
144–145

Mega nit: self.context.options.source_files.extend(list(new_source_files)) is slightly more concise.

debuginfo-tests/dexter/feature_tests/commands/penalty/dex_declare_file.cpp
17

Major nit, but I think this should be a filename that absolutely screams that it doesn't exist; thus something like "this_file_should_not_exist.cpp". that avoids the reader spending a second wondering whether the "not" prefix is significant.

(This might be because my default name for anything, when moved, is not_$originalname).

This revision is now accepted and ready to land.Apr 6 2021, 9:45 AM
jmorse requested changes to this revision.Apr 6 2021, 2:54 PM

I ended up taking this for a test drive, and found some difficulty when using the LLDB bindings -- _add_conditional_breakpoint ends up being passed a PurePath object for the file_, which is then passed to the LLDB bindings, which croak. This is easily fixed by calling str(file_) to convert the PurePath object to a string, but this might affect other debugger drivers & need some assertions adding.

This revision now requires changes to proceed.Apr 6 2021, 2:54 PM
Orlando added inline comments.Apr 8 2021, 9:41 AM
debuginfo-tests/dexter/feature_tests/commands/perfect/dex_declare_file/dex_and_source/test.cpp
10

This lit test fails for me locally - dexter doesn't run the test. Adding a test.cfg file to this directory fixes it.

TWeaver marked 5 inline comments as done.Apr 19 2021, 9:11 AM
TWeaver added inline comments.
debuginfo-tests/dexter/feature_tests/commands/perfect/dex_declare_file/dex_and_source/test.cpp
10

slightly perplexing, this patch contains the test.cfg for the directory in question...

Orlando added inline comments.Apr 20 2021, 1:14 AM
debuginfo-tests/dexter/feature_tests/commands/perfect/dex_declare_file/dex_and_source/test.cpp
10

Ah yes I see it, I must've messed up applying the patch and not noticed. Sorry!

TWeaver updated this revision to Diff 346194.May 18 2021, 9:38 AM
TWeaver marked 3 inline comments as done.

Addressed feedback and fixed a test failure in the clang-opt-bisect tool test.

TWeaver updated this revision to Diff 346388.May 19 2021, 3:37 AM

re-uploaded new diff with more context

Orlando added a comment.EditedMay 19 2021, 4:10 AM

It looks like you've addressed both of our comments (except my suggestion for a non-canonical path spelling test for windows exclusively, which would still be welcomed but isn't critical) so LGTM. I'll leave it to @jmorse to Accept this if he's happy.

jmorse accepted this revision.May 20 2021, 4:25 AM

LGTM

This revision is now accepted and ready to land.May 20 2021, 4:25 AM

Hi, before this lands you will need to update your dexter tests to use the new label reference syntax added in D101147. You can do so manually, or alternatively use this python script by running python make_label_refs.py debuginfo-tests/feature_tests (please note it will modify files in place). I'm happy for you to amend your commit with that update without bringing it back up here for review.

Hi, before this lands you will need to update your dexter tests to use the new label reference syntax added in D101147. You can do so manually, or alternatively use this python script by running python make_label_refs.py debuginfo-tests/feature_tests (please note it will modify files in place). I'm happy for you to amend your commit with that update without bringing it back up here for review.

Of course, you don't need to do this if you don't use DexLabels in your tests, which you haven't. Please ignore that, sorry!

TWeaver added a comment.EditedMay 24 2021, 7:41 AM

Apologies for the missing test, here it is! :)

Will be pushing this shortly, thanks a bunch.

# Purpose:
#    Check that non canonical paths resolve correctly on windows.
#
# REQUIRES: system-windows
#
# RUN: %clang "%S/source/test file.cpp" -O0 -g -o %t
# RUN: %dexter_regression_test --binary %t %s | FileCheck %s
# CHECK: test.dex
#
# ./source/test file.cpp
# 1 int main(const int argc, const char * argv[]) {
# 2 int result = argc;
# 3 return result;
# 4 }

DexDeclareFile('./sOuRce\\test filE.cpp')
DexExpectWatchValue('result', 1, on_line=3)

Apologies for the missing test, here it is! :)

Will be pushing this shortly, thanks a bunch.

# Purpose:
#    Check that non canonical paths resolve correctly on windows.
#
# REQUIRES: system-windows
#
# RUN: %clang "%S/source/test file.cpp" -O0 -g -o %t
# RUN: %dexter_regression_test --binary %t %s | FileCheck %s
# CHECK: test.dex
#
# ./source/test file.cpp
# 1 int main(const int argc, const char * argv[]) {
# 2 int result = argc;
# 3 return result;
# 4 }

DexDeclareFile('./sOuRce\\test filE.cpp')
DexExpectWatchValue('result', 1, on_line=3)

Thanks! Looks good.

This revision was landed with ongoing or failed builds.May 25 2021, 4:47 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMay 25 2021, 4:47 AM