This is an archive of the discontinued LLVM Phabricator instance.

[LLDB] Allow expression evaluators to set arbitrary timeouts
ClosedPublic

Authored by wallace on Aug 11 2023, 3:32 PM.

Details

Summary

https://github.com/llvm/llvm-project/commit/59237bb52c9483fce395428bfab5996eabe54ed0 changed the behavior of the SetTimeout and GetTimeout methods of EvaluateExpressionOptions, forbidding the use of infinite timeouts, which are specified with Timeout<std::micro>(std::nullopt).
This broke the Mojo REPL and related services (https://docs.modular.com/mojo/) because it relies on having infinite timeouts. That's a necessity because developers often use the REPL for executing extremely long-running numeric jobs.
Having said that, EvaluateExpressionOptions shouldn't be that opinionated on this matter anyway. Instead, it should be the responsibility of the evaluator to define which timeout to use for each specific case.

Diff Detail

Event Timeline

wallace created this revision.Aug 11 2023, 3:32 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 11 2023, 3:32 PM
wallace requested review of this revision.Aug 11 2023, 3:32 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 11 2023, 3:32 PM
wallace edited the summary of this revision. (Show Details)Aug 11 2023, 3:34 PM
wallace added reviewers: JDevlieghere, rriddle.
Mogball accepted this revision.Aug 11 2023, 4:11 PM
This revision is now accepted and ready to land.Aug 11 2023, 4:11 PM

This makes sense to me, but again will defer to area owners for what the expectation is here.

Btw, the SBAPI expects the old behavior.

// Set the timeout for the expression, 0 means wait forever.
void SetTimeoutInMicroSeconds(uint32_t timeout = 0);
JDevlieghere accepted this revision.Aug 12 2023, 9:11 AM

The motivation behind this change was that the IRInterpreter wasn't interruptible, which means that if you interpret an infinite loop, you'd be stuck forever. I fixed that in 61af957aeaa3 so I think that's less of a concern today. I'm fine with going back to allowing an infinite timeout as long as there's no code path in LLDB that sets it to zero by default.

@wallace this is breaking TestIRInterpreter.py on macOS now; https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/ has been failing since it went in (there was another llvm issue causing failures right before this went in, so it was easy to miss). On my desktop this change results in TestIRInterpreter.py running without end; I think the bots kill tests that run for more than 5 minutes.

@jasonmolenda , yep. I have already reverted this patch. I'll figure out how to make that work nicely with these changes.

@jasonmolenda , yep. I have already reverted this patch. I'll figure out how to make that work nicely with these changes.

oh lol sorry took me a little while to confirm and I missed that. :)

@jasonmolenda , yep. I have already reverted this patch. I'll figure out how to make that work nicely with these changes.

My recommendation would be to change the timeout in the test from 0 to 500ms to maintain the previous behavior.

@JDevlieghere , that worked. Thanks!!