Bug 189070 - lldb-webkit: KeyError thrown for OptionSet with invalid value
Summary: lldb-webkit: KeyError thrown for OptionSet with invalid value
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Daniel Bates
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-08-28 17:03 PDT by Daniel Bates
Modified: 2018-08-31 09:45 PDT (History)
4 users (show)

See Also:


Attachments
Patch (2.67 KB, patch)
2018-08-28 17:17 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch (2.79 KB, patch)
2018-08-29 15:12 PDT, Daniel Bates
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Bates 2018-08-28 17:03:45 PDT
An OptionSet variable can be in scope and uninitialized as a variable is not initialized until execution has flowed over its assignment. We need to take care to handle this case and not try to compute the set of enumerators in an uninitialized OptionSet.
Comment 1 Daniel Bates 2018-08-28 17:17:32 PDT
Created attachment 348363 [details]
Patch
Comment 2 Daniel Bates 2018-08-28 17:22:24 PDT
Comment on attachment 348363 [details]
Patch

This is wrong.
Comment 3 Daniel Bates 2018-08-29 15:12:02 PDT
Created attachment 348436 [details]
Patch
Comment 4 Daniel Bates 2018-08-29 15:20:28 PDT
Committed r235482: <https://trac.webkit.org/changeset/235482>
Comment 5 Radar WebKit Bug Importer 2018-08-29 15:21:17 PDT
<rdar://problem/43860433>
Comment 6 Darin Adler 2018-08-29 18:13:33 PDT
Comment on attachment 348436 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=348436&action=review

> Tools/lldb/lldb_webkit.py:627
> +            return  # self.valobj is uninitialized memory

This comment and the bug title is too specific and potentially incorrect.

This code path can occur any time the OptionSet has an invalid value.

An uninitialized variable is one case when something might have an invalid value, but of course something uninitialized might also have a valid value. But there are many others such as a variable that has been overwritten by a buffer overrun, a variable been overwritten by something writing through a stale pointer, trying to display a value after a local variable memory region is reused for another purpose because we are past the point of use of the local variable in the code, a value from following a pointer to an object that has already been deleted, and many more.

On the other hand, I suspect the *behavior* of the code you wrote is probably good; it writes out something that lets you see the value of the memory and makes it clear that the OptionSet has an invalid value.
Comment 7 Daniel Bates 2018-08-30 09:40:51 PDT
(In reply to Darin Adler from comment #6)
> Comment on attachment 348436 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=348436&action=review
> 
> > Tools/lldb/lldb_webkit.py:627
> > +            return  # self.valobj is uninitialized memory
> 
> This comment and the bug title is too specific and potentially incorrect.
> 
> This code path can occur any time the OptionSet has an invalid value.
> 
> An uninitialized variable is one case when something might have an invalid
> value, but of course something uninitialized might also have a valid value.
> But there are many others such as a variable that has been overwritten by a
> buffer overrun, a variable been overwritten by something writing through a
> stale pointer, trying to display a value after a local variable memory
> region is reused for another purpose because we are past the point of use of
> the local variable in the code, a value from following a pointer to an
> object that has already been deleted, and many more.

You're right!

We could either remove the comment or I could update the comment to explain that self.valobj has an invalid value,  maybe:

# self.valobj has an invalid value.

What do you suggest?
Comment 8 Darin Adler 2018-08-30 18:25:44 PDT
Either removing the comment or updating it is fine with me.
Comment 9 Darin Adler 2018-08-30 18:26:34 PDT
An even more useful comment would make it clear why just returning early is the right thing to do for an invalid value.

    # Since this is an invalid value, return so the raw hex form is written out.

Or a comment that is correct.
Comment 10 Daniel Bates 2018-08-31 09:45:48 PDT
(In reply to Darin Adler from comment #9)
> An even more useful comment would make it clear why just returning early is
> the right thing to do for an invalid value.
> 
>     # Since this is an invalid value, return so the raw hex form is written
> out.
> 
> Or a comment that is correct.

Updated comment as well as the change log entry for r235482 to reflect the updated title for this bug and better describe the purpose of the change. Committed this in <https://trac.webkit.org/changeset/235555>.