[test/Shell] Add stepper test for Benchmark_Onone #2059
Conversation
vedantk
requested review from
kastiglione,
JDevlieghere,
Teemperor and
adrian-prantl
|
@swift-ci test |
| ############################################################################### | ||
|
|
||
| # Only visit unoptimized frames. | ||
| kOnlyUnoptimized = True |
There was a problem hiding this comment.
just curious: why do these names start with k?
There was a problem hiding this comment.
If these are meant to be constants they should be all uppercase (https://www.python.org/dev/peps/pep-0008/#constants)
There was a problem hiding this comment.
Will fix. @adrian-prantl heh, we have some history here: https://reviews.llvm.org/D80684#inline-741730
| @@ -0,0 +1,214 @@ | |||
| from __future__ import print_function | |||
There was a problem hiding this comment.
why is this in Inputs? Should it be in helper/?
|
@swift-ci test |
lldb/test/Shell/helper/stepper.py
Outdated
| SKIP_N_FRAMES_BETWEEN_INSPECTIONS | ||
| opts = os.getenv('STEPPER_OPTIONS', '').split(';') | ||
| for o in opts: | ||
| m = re.match('(\w+)=([^;]+)', o) |
There was a problem hiding this comment.
([^;]+) seems like it could be (.+) since the split above will ensure there are no ; characters. This could even be split('=').
lldb/test/Shell/helper/stepper.py
Outdated
| thread.StepOutOfFrame(frame) | ||
| new_frame = thread.GetSelectedFrame() | ||
| new_name = new_frame.GetFunctionName() | ||
| print(':: Transitioned from {0} -> {1}.'.format(old_name, new_name)) |
There was a problem hiding this comment.
the format numbers could be left out, ex {} -> {}
| get_statics, get_in_scope_only) | ||
| for var in variables: | ||
| name = var.GetName() | ||
| if not var.GetLocation(): |
There was a problem hiding this comment.
maybe a comment on why a variable is skipped when it has no location?
lldb/test/Shell/helper/stepper.py
Outdated
|
|
||
| def doquit(dbg): | ||
| "Quit the stepper script interpreter." | ||
| os._exit(0) |
There was a problem hiding this comment.
If there's a reason to not call os.exit(), maybe a comment why?
There was a problem hiding this comment.
No reason, just exit(0) works fine. Will fix.
lldb/test/Shell/helper/stepper.py
Outdated
| else: | ||
| option, val = m.groups() | ||
| print('Setting', option, '=', val) | ||
| globals()[option] = eval(val) |
There was a problem hiding this comment.
Probably safer to use ast.literal_eval() over eval().
There was a problem hiding this comment.
TIL! Agreed, but in this case imo it's not worth making the code more defensive, since eval-abuse is unlikely to get through code review.
| process = target.GetProcess() | ||
| visited_pc_counts = defaultdict(int) | ||
| while True: | ||
| gen += 1 |
There was a problem hiding this comment.
would it be useful to also track how many times do_inspection ends up being true, and including that count along with the generation count?
There was a problem hiding this comment.
Yep, we want to configure tests to minimize the difference between #inspections and #generations, otherwise we're just wasting cycles without gaining test coverage.
|
@swift-ci test |
|
|
||
| # Skip optimized frames if asked to do so. | ||
| if do_inspection and ONLY_VISIT_UNOPTIMIZED and \ | ||
| str(frame).endswith(' [opt]'): |
There was a problem hiding this comment.
TIL SBFrame has IsArtificial and IsInlined but not IsOptimized. Should we add this?
There was a problem hiding this comment.
Probably, yeah. I'll put a patch together.
There was a problem hiding this comment.
Ah, maybe SBFunction::GetIsOptimized is sufficient? I can see the logic behind that. Whether or not a function is inlined or tail-called depends on the frame. But if a function is optimized, that's true independent of which frame you pick.
Add a new "buildbot_incremental,lldb_asan_ubsan" preset. This differs from the existing preset used to do sanitized lldb testing on ci.swift.org, "buildbot_incremental_asan_ubsan,tools=RDA,stdlib=RDA", in the following ways: - It builds the swift benchmarks, with an unoptimized benchmark driver. This is to enable stepper testing on CI. For more context, see: swiftlang/llvm-project#2059. - It does not build a sanitized clang or swift. This can save a large amount of time, and make reports from oss-lldb-asan bots more useful. This is still WIP pending more testing. Note: A dry run for the existing buildbot_incremental_asan_ubsan preset does not apply '--skip-build-benchmarks': ``` ./swift/utils/build-script -n --preset "buildbot_incremental_asan_ubsan,tools=RDA,stdlib=RDA" ``` So I'm not yet sure where that comes from. Perhaps the oss-lldb-asan-osx bot is using a different preset? rdar://69589638
This PR wires up the stepper testing script to run alongside normal test/Shell testing.
This isn't the only (or necessarily the best) way to do stepper testing in CI. For example, we could modify some CI jobs to check out and run the stepper script.
Some advantages of doing stepper testing within test/Shell: