[Lldb-commits] [PATCH] D123020: increase timeouts if running on valgrind

Mon Apr 4 10:03:13 PDT 2022

llunak added a comment.

In D123020#3426252 <https://reviews.llvm.org/D123020#3426252>, @labath wrote:

>> BTW, does it make sense to get even things like this reviewed, or is it ok if I push these directly if I'm reasonably certain I know what I'm doing? I feel like I'm spamming you by now.
>
> Generally, I would say yes. I'm not even sure that some of your other patches should have been submitted without a pre-commit review (*all* llvm patches are expected to be reviewed).

I see. I haven't contributed anything for a while and git history shows a number of commits to do not refer to reviews.llvm.org, so I assumed simple patches were fine. I'll get even the simple ones reviewed if that's expected.

> I would say that the entire ScopedTimeout class should be multiplier-based. Instead of passing a fix value, the user would say "I know this packed is slow, so I want to use X-times the usual timeout value". Then, we could support valgrind (and various *SANs) by just setting the initial timeout value to a reasonably high value, and the multipliers would take care of the rest.

For the Valgrind case it doesn't really matter what the timeout is, as long as it's long enough to not time out on normal calls. Even multiplying is just taking a guess at it, Valgrind doesn't slow things down linearly.

In D123020#3426572 <https://reviews.llvm.org/D123020#3426572>, @JDevlieghere wrote:

> Is this really something we want to hardcode at all? It seems like this should be a setting that is configured by the test harness.

This is not about tests. I had a double-free abort, so I ran lldb normally in valgrind and it aborted attach because of the 6-second timeout in GDBRemoteCommunicationClient::QueryNoAckModeSupported(). That's too short for anything in Valgrind, and I thought this change would be useful in general, as otherwise lldb is presumably unusable for Valgrind runs. But if you think this requires getting complicated as such rewritting an entire class for it, then I'm abandoning the patch.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D123020/new/

https://reviews.llvm.org/D123020