clayborg added a comment. In D123020#3426867 <https://reviews.llvm.org/D123020#3426867>, @JDevlieghere wrote:
> 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. The GDB remote packet timeout is customizable: (lldb) settings set plugin.process.gdb-remote.packet-timeout 10000 So maybe we don't need this patch? 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
