https://github.com/labath commented:
> 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. > 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. 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. 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". 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
