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

Mon Apr 4 10:22:53 PDT 2022

JDevlieghere added a comment.

In D123020#3426839 <https://reviews.llvm.org/D123020#3426839>, @llunak wrote:

> 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.

FWIW the official policy is outlined here: https://llvm.org/docs/CodeReview.html

>> 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.

My suggestion was that this timeout should be configurable through a setting. If it is, then you can increase it when running under Valgrind (or anywhere else that slows down lldb, like the sanitizers). I wouldn't mind having a bit of code that checks `llvm::sys::RunningOnValgrind()` and increases the default timeouts so that you don't even have to change your setting. But what I don't think is a good idea is to sprinkle checks for whether we're running under Vanguard throughout the code.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123020