Michael137 wrote: > > Side-note, I'm also a bit wary of all the cycle-detection stuff. Might be a > > remnant of protection against old libcxx bugs? We don't really do this > > stuff for other libcxx containers. > > We don't do that for other containers, but it's not really as acute there. > For example, std::map has an explicit "size" field, so even if we spin in > circles, we'd stop when reach that number. Of course, if the size field is > corrupted _and_ the binary tree has a loop, then we're doomed, but ... > > It's also not so much about the libcxx bugs as it is about bugs in the > program using it. If the user program has a bug, then there's no way of > telling what state its data structures will be in -- just because the data > structure is defined in the standard library, it doesn't mean the user can't > corrupt it. A looping list might just as well be the cause of the crash, so > ideally we don't it want to bring down the debugger as well.
Yea I agree, it is definitely a nice-to-have. I haven't actually tried on a real program what the cycle-detection looks like. We don't actually point out the cycle right? We just avoid printing in a loop? I think it would be cool if we somehow told the user "this is a corrupted std::list". Maybe we are already doing that, I haven't read the implementation too carefully. If not, it would be a nice enhancement in my opinion. > > Should we keep it? > > That's a complicated question. > > Because std::forward_list does not have an explicit size member, the only way > to compute its size is to traverse it. As you can imagine, this can be quite > slow, particularly if you need to go over a network to fetch each element. > This makes it a very bad match for lldb's SB API, which _really_ wants you to > call `GetNumChildren` on each value, which means that doing just about > anything with this type can be very slow. And of course, if the list loops, > then you get a broken debugger if you simply have this somewhere in your > stack frame. > > We currently have two mechanisms to prevent this. Both of them were added > before my time, and the history is unclear as to their order. One is the loop > detection thingy. The other is `m_list_capping_size`, which prevents you from > going over `target.max-children-count` children. > > I don't think we need both of these. However, I also don't think the currents > state of things is particularly reasonable. My beef with > `m_list_capping_size` is that it gives you no indication that the list > actually contains more elements. It will lie to your face about the size of > the list. It's particularly ridiculous now that the setting value is 24 by > default. I guess the only reason this didn't come up yet is because people > don't use this container very often. Yikes, yea that's not great. > So, while removing the loop detector is one option, if this was up to me > (which it isn't), I would remove m_list_capping_size instead. The reason is > that we now have better mechanisms to deal with the children count issue. > Some [10 years ago](http://reviews.llvm.org/D13778) we added the > GetNumChildren(max) overload, which lets you cap the number of children on a > per-call basis. So like, if all you want to know is whether the list has more > then 100 elements (because your UI doesn't show more by default), you can > call `GetNumChildren(100)` and it will not attempt to go over that. However, > this requires discipline to not call the "exact" overload willy-nilly (which, > for synthetic values basically means -- never). I think the current lldb code > is in a pretty good state (we've made sure of that as we have data formatters > where enumerating all children is fairly expensive, even though they're not > linked lists), but there's no way to tell what other SB users are doing. Happy with just keeping the cycle detection! > That leaves us with the summary string, which wants to print the `size=%d` > thingy, but there I think the solution is to just not do that. There's no law > that states that a container summary has to include its (exact) size, so I'd > just change the summary to display an approximate value as well. E.g. we > could say that if the size is smaller some value (maybe the value of > `target.max-children-count` -- I think this is the only legitimate use of > that setting in a formatter), we print the exact size. Otherwise, we just say > "larger than N". I vaguely remember a thread somewhere else about this where you recommended this too. I think that's a good idea --- ah found it: https://github.com/llvm/llvm-project/pull/139805#issuecomment-2879353718 https://github.com/llvm/llvm-project/pull/148285 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
