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 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
