Bug 152549 – Add validation of JSC options to catch typos.

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Mark Lam]

Patch coming.

[Mark Lam]

Created attachment 267904 [details]
proposed patch.

[Geoffrey Garen]

Comment on attachment 267904 [details]
proposed patch.

Let's make validation the default behavior rather than an option. Having to  type in an option in order to validate your options begs the original question of typing options correctly.

[Filip Pizlo]

(In reply to comment #2)
> Comment on attachment 267904 [details]
> proposed patch.
> 
> Let's make validation the default behavior rather than an option. Having to 
> type in an option in order to validate your options begs the original
> question of typing options correctly.

There were arguments against doing this when we discussed this last.

I said that I didn't want this because I was worried about end users seeing crashes just because "JSC_" ended up at the start of some environment variable.

Michael said that he didn't want this because it would break his lldb workflow.

[Keith Miller]

(In reply to comment #3)
> (In reply to comment #2)
> > Comment on attachment 267904 [details]
> > proposed patch.
> > 
> > Let's make validation the default behavior rather than an option. Having to 
> > type in an option in order to validate your options begs the original
> > question of typing options correctly.
> 
> There were arguments against doing this when we discussed this last.
> 
> I said that I didn't want this because I was worried about end users seeing
> crashes just because "JSC_" ended up at the start of some environment
> variable.
> 
> Michael said that he didn't want this because it would break his lldb
> workflow.

I disagree that Michael's issue is a valid argument against setting validate as the default as it's trivial to export JSC_validateOptions=false. It's also trivial to export JSC_validateOptions=true, however, so I'm not sure it's necessary to have validation as the default either.

I agree with the first argument, although I think it should be noted that the crashes only really realistically be an issue with command line users of JSC. However, I still think that any frustration for end users is enough of an issue that we should not make validation the default.

[Geoffrey Garen]

Comment on attachment 267904 [details]
proposed patch.

I just now read the email discussion of this feature. It wasn't mentioned in this patch, so I didn't think to look. I guess I have to remove my review since it was based on incomplete information.

Based on the email discussion, it seems like there's consensus that it's valuable to log incorrect options as a warning, and also consensus that it is harmful to crash. Also, as I said above, adding an option to validate options begs the original question.

So, now I think we should always log, and never crash.

What's the affirmative reason to crash?

A portion of the email discussion revealed that we have caused problems by just letting folks do what they feel in this area of code, without strict review, so I am applying stricter review now.

[Mark Lam]

(In reply to comment #5)
> Comment on attachment 267904 [details]
> proposed patch.
> 
> I just now read the email discussion of this feature. It wasn't mentioned in
> this patch, so I didn't think to look. I guess I have to remove my review
> since it was based on incomplete information.
> 
> Based on the email discussion, it seems like there's consensus that it's
> valuable to log incorrect options as a warning, and also consensus that it
> is harmful to crash. Also, as I said above, adding an option to validate
> options begs the original question.
> 
> So, now I think we should always log, and never crash.
> 
> What's the affirmative reason to crash?
> 
> A portion of the email discussion revealed that we have caused problems by
> just letting folks do what they feel in this area of code, without strict
> review, so I am applying stricter review now.

This patch already always logs the bad options.  The reason for wanting a mode that allows crashing is because logging will not always get one's attention.  Logging is not always readily available either.  For example, on iOS, I will have to remember to redirect dataLog and read the dataLog file for the error message in order to notice this.

Granted, there's is still a chance that one can have a typo when setting JSC_validateOptions, but it is much easier to get that right then to have to get all the other options right, especially when those options are the ones we will change from time to time depending on benchmarking / debugging needs.

Here's a use case scenario that will save me from losing a day's work (or more) due to potential typo in options that I need to set and change in the course of debugging hard bugs:

1. In my .profile file, I add an environment var JSC_validateOptions=true.
2. I do debugging like normal.

Note that I only need to check that there's no typo in my .profile change just once.  Once I've set it correctly in my shell profile, I never have to worry about typos in options again.

For those whose workflows prefer checking their typing every time, they can look for the logging output.  For my type of workflow where I prefer to have change options depending on what the data tells me in the course of debugging, it's better to have a hard failure when I'm not using the options I expected.

[Saam Barati]

(In reply to comment #6)
> (In reply to comment #5)
> > Comment on attachment 267904 [details]
> > proposed patch.
> > 
> > I just now read the email discussion of this feature. It wasn't mentioned in
> > this patch, so I didn't think to look. I guess I have to remove my review
> > since it was based on incomplete information.
> > 
> > Based on the email discussion, it seems like there's consensus that it's
> > valuable to log incorrect options as a warning, and also consensus that it
> > is harmful to crash. Also, as I said above, adding an option to validate
> > options begs the original question.
> > 
> > So, now I think we should always log, and never crash.
> > 
> > What's the affirmative reason to crash?
> > 
> > A portion of the email discussion revealed that we have caused problems by
> > just letting folks do what they feel in this area of code, without strict
> > review, so I am applying stricter review now.
> 
> This patch already always logs the bad options.  The reason for wanting a
> mode that allows crashing is because logging will not always get one's
> attention.  Logging is not always readily available either.  For example, on
> iOS, I will have to remember to redirect dataLog and read the dataLog file
> for the error message in order to notice this.
> 
> Granted, there's is still a chance that one can have a typo when setting
> JSC_validateOptions, but it is much easier to get that right then to have to
> get all the other options right, especially when those options are the ones
> we will change from time to time depending on benchmarking / debugging needs.
> 
> Here's a use case scenario that will save me from losing a day's work (or
> more) due to potential typo in options that I need to set and change in the
> course of debugging hard bugs:
> 
> 1. In my .profile file, I add an environment var JSC_validateOptions=true.
> 2. I do debugging like normal.
> 
> Note that I only need to check that there's no typo in my .profile change
> just once.  Once I've set it correctly in my shell profile, I never have to
> worry about typos in options again.
> 
> For those whose workflows prefer checking their typing every time, they can
> look for the logging output.  For my type of workflow where I prefer to have
> change options depending on what the data tells me in the course of
> debugging, it's better to have a hard failure when I'm not using the options
> I expected.

I agree with this argument. When I'm debugging JSC, I always want a crash if I mistype
an option. I think it's also totally fine to have this behavior with the option being
off by default. Or, if we really want to get fancy, we could log mistyped options by
default and only crash when another option is set (maybe JSC_validateOptionsWithCrash)
that is off by default.

[Mark Lam]

Thanks for the review.  Landed in r194590: <http://trac.webkit.org/r194590>.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 152751

[Mark Lam]

Created attachment 268320 [details]
proposed patch for re-landing: no longer restricting where we put -- options on the command line.

[Mark Lam]

Thanks for the review.  The revised patch has been landed in r194606: <http://trac.webkit.org/r194606>.