Change ValueObjectPrinter to use references to m_orig_valobj, and ensure m_valobj can't be null #8192
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…rincipled (NFC) (llvm#81314) I get a small but fairly steady stream of crash reports which I can only explain by ValueObjectPrinter trying to access its m_valobj field, and finding it NULL. I have never been able to reproduce any of these, and the reports show a state too long after the fact to know what went wrong. I've read through this section of lldb a bunch of times trying to figure out how this could happen, but haven't ever found anything actually wrong that could cause this. OTOH, ValueObjectPrinter is somewhat sloppy about how it handles the ValueObject it is printing. a) lldb allows you to make a ValueObjectPrinter with a Null incoming ValueObject. However, there's no affordance to set the ValueObject in the Printer after the fact, and it doesn't really make sense to do that. So I change the ValueObjectPrinter API's to take a ValueObject reference, rather than a pointer. All the places that make ValueObjectPrinters already check the non-null status of their ValueObject's before making the ValueObjectPrinter, so sadly, I didn't find the bug, but this will enforce the intent. b) The next step in printing the ValueObject is deciding which of the associated DynamicValue/SyntheticValue we are actually printing (based on the use_dynamic and use_synthetic settings in the original ValueObject. This was put in a pointer by GetMostSpecializedValue, but most of the printer code just accessed the pointer, and it was hard to reason out whether we were guaranteed to always call this before using m_valobj. So far as I could see we always do (sigh, didn't find the bug there either) but this was way too hard to reason about. In fact, we figure out once which ValueObject we're going to print and don't change that through the life of the printer. So I changed this to both set the "most specialized value" in the constructor, and then to always access it through GetMostSpecializedValue(). That makes it easier to reason about the use of this ValueObject as well. This is an NFC change, all it does is make the code easier to reason about. (cherry picked from commit c2b01c8)
I changed llvm.org to not pass as pointers values that cannot be null, but there were a couple swift specific usages that also need fixing.
|
@swift-ci please test |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a cherry-pick of c2b01c8 plus one swift specific change.