This is an archive of the discontinued LLVM Phabricator instance.

[Dexter] Remove builder from Dexter
ClosedPublic

Authored by StephenTozer on May 25 2023, 9:39 AM.

Details

Summary

See "discussion": https://discourse.llvm.org/t/rfc-dexter-feature-removals/60462

With Dexter's inclusion in LLVM, test discovery and building is no longer required and is an active maintenance burden; this patch removes the builder and all associated features from Dexter. This does also mean removing the clang-opt-bisect feature; this is unfortunate collateral damage, as that tool does add functionality that is not otherwise available from a combination of Dexter and existing LLVM tools, but to my knowledge this tool has not seen much use in recent years and nobody yet has expressed a use-case for it. If it is in use, then it could either be reimplemented to take a build script as an argument (as with tools such as git-bisect and llvm-reduce), or implemented as a separate program with general support for "repeatedly run clang with opt-bisect-limit and run a script at each bisection" with a Dexter option.

For the most part this patch deletes builder-related folders completely, and removes other builder-related code from the rest of Dexter. No further functional changes are expected to appear in this patch. Tests have been updated to directly invoke clang (either with %clang or with %dexter_regression_test_build, which substitutes to clang++ -O0 -glldb or clang-cl /Zi /Od depending on platform) to build a temporary executable file and then run Dexter on that file.

Diff Detail

Event Timeline

StephenTozer created this revision.May 25 2023, 9:39 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 25 2023, 9:39 AM
Herald added a subscriber: ormris. · View Herald Transcript
StephenTozer requested review of this revision.May 25 2023, 9:39 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 25 2023, 9:39 AM
StephenTozer added a comment.EditedMay 25 2023, 9:42 AM

Small comment for reviewers; I ran black on the dexter folder to format code afterwards, and it looks like it removed a few blank lines that were untouched by my changes. I've left the newline removals in this patch, because in theory black should be essentially unconfigurable and therefore I assume the formatting it has done is correct and that the previous invocation of black on dexter either missed committing those changes for some reason or used an older version of black that used a slightly different rule for blank lines. The lines in question were the blank lines following def __init__(...): in the command files (dex/command/commands/Dex*.py).

jmorse accepted this revision.Jul 3 2023, 9:05 AM

I approve of the amount of red in this patch, LGTM. There are some files where there are only whitespace changes (deleted blank lines?), which would more ideally be in a cleanup patch instead.

Focusing Dexter more on what it does well, i.e. driving a debugging session and comparing against expectations, should make all our lives easier.

This revision is now accepted and ready to land.Jul 3 2023, 9:05 AM
This revision was automatically updated to reflect the committed changes.

FYI, looks like debug-info tests started failing on the public x86_64 bot after this patch: https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/59089/console

Exit Code: 1

Command Output (stderr):
--
/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/cross-project-tests/debuginfo-tests/dexter/feature_tests/commands/perfect/limit_steps/limit_steps_check_json_step_count.cpp:8:19: error: CHECK-COUNT: expected string not found in input (1 out of 3)
// CHECK-COUNT-3: json_step_count.cpp",
                  ^
<stdin>:3:12: note: scanning from here
## BEGIN ##
           ^
<stdin>:7:20: note: possible intended match here
 limit_steps_check_json_step_count.cpp:14 [index] ExpectValue [18/21]
                   ^

Input file: <stdin>
Check file: /Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/cross-project-tests/debuginfo-tests/dexter/feature_tests/commands/perfect/limit_steps/limit_steps_check_json_step_count.cpp

Mind taking a look?

FYI, looks like debug-info tests started failing on the public x86_64 bot after this patch: https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/59089/console

Exit Code: 1

Command Output (stderr):
--
/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/cross-project-tests/debuginfo-tests/dexter/feature_tests/commands/perfect/limit_steps/limit_steps_check_json_step_count.cpp:8:19: error: CHECK-COUNT: expected string not found in input (1 out of 3)
// CHECK-COUNT-3: json_step_count.cpp",
                  ^
<stdin>:3:12: note: scanning from here
## BEGIN ##
           ^
<stdin>:7:20: note: possible intended match here
 limit_steps_check_json_step_count.cpp:14 [index] ExpectValue [18/21]
                   ^

Input file: <stdin>
Check file: /Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/cross-project-tests/debuginfo-tests/dexter/feature_tests/commands/perfect/limit_steps/limit_steps_check_json_step_count.cpp

Mind taking a look?

Errors look odd, I'll take a look - if you think this needs a revert I'll take care of that too.

Looking further at the failing tests on the green dragon bot: I can't see any obvious causes for the errors; also all but one of the errors disappear after the first failed build. I can't see any clear reason for that test alone to fail in the way that it did, but it was updated inconsistently with another test that was otherwise changed identically (using %clang instead of %clang++), where the other test isn't failed. For now I've pushed up a patch that brings it in line with the remaining test, and if that doesn't fix it then I'll look a little further and revert if necessary.

End of day for me, so reverting now - still seeing some tool substitution errors on the SIE buildbot that don't reproduce locally, and the optnone-vectors-and-functions failure seems quite opaque - I might have the solution, but it'll wait until tomorrow to go up.

Applying this patch again - if there are green dragon errors, at this point I suspect there may some opaque configuration issue on the buildbot (not necessarily an error, but not present on other systems), as the command line should theoretically be identical. Since this patch is large and touches a lot of files, I'd prefer to mark any subsequently failing tests as XFAILs and investigate post-commit to prevent the patch from repeatedly landing and reverting. Open to any principled or practical objections to this!

aprantl added a subscriber: aprantl.Sep 8 2023, 2:48 PM

@StephenTozer Yes, there are green dragon errors. It looks like -lstdc++ is hardcoded. On Darwin we usually only have libc++. Could you make that configurable?

https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/59897/consoleFull#12109776838254eaf0-7326-4999-85b0-388101f2d404

@StephenTozer Yes, there are green dragon errors. It looks like -lstdc++ is hardcoded. On Darwin we usually only have libc++. Could you make that configurable?

https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/59897/consoleFull#12109776838254eaf0-7326-4999-85b0-388101f2d404

Looking into this, apologies for not catching the failures myself!

I see that one of the tests, aggregate-indirect-arg.cpp, has -lstdc++ hardcoded in the test line; this was present prior to this patch though (specified as an --ldflags argument to Dexter), and none of the other tests should have any new/different flags applied to them. Is that confirmed to be the cause of the errors for the other tests? If so, it shouldn't be too hard to create a lit substitution for the command line that passes the right linker flag, but just want to verify whether there are other errors in place.

cross-project-tests/debuginfo-tests/dexter/feature_tests/commands/perfect/unreachable_on_line.cpp