Bug 177944 – Avoid integer overflow in DFGStrengthReduction.cpp

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Robin Morisset]

The check that we won't cause integer overflow is itself a signed integer overflow! I believe this is undefined behavior, and a future compiler could remove the check altogether.
Instead, make it an explicit check that value != INT32_MIN.

[Robin Morisset]

Created attachment 322838 [details]
Patch

[Saam Barati]

Comment on attachment 322838 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGStrengthReductionPhase.cpp:158
> +                if (value != INT32_MIN) {

Why not use Checked here?

[Robin Morisset]

(In reply to Saam Barati from comment #2)
> Comment on attachment 322838 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=322838&action=review
> 
> > Source/JavaScriptCore/dfg/DFGStrengthReductionPhase.cpp:158
> > +                if (value != INT32_MIN) {
> 
> Why not use Checked here?

I just did not know about the existence of Checked.

I could use it here, but I am not sure it would be a win: the check here is trivial, and using Checked would be a bit more bothersome:
  Checked<int32_t, RecordOnOverflow> checkedNewValue = 0;
  checkedNewValue -= value;
  int32_t newValue;
  if (CheckedState::DidNotOverflow == checkedNewValue.safeGet(newValue)) {
    ... (newValue)...
  }
Instead of
  if (value != INT32_MIN) {
    ... (-value) ...
  }

If you think it would be better, I can do the change and use it, it is as you want.

[Saam Barati]

(In reply to Robin Morisset from comment #3)
> (In reply to Saam Barati from comment #2)
> > Comment on attachment 322838 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=322838&action=review
> > 
> > > Source/JavaScriptCore/dfg/DFGStrengthReductionPhase.cpp:158
> > > +                if (value != INT32_MIN) {
> > 
> > Why not use Checked here?
> 
> I just did not know about the existence of Checked.
> 
> I could use it here, but I am not sure it would be a win: the check here is
> trivial, and using Checked would be a bit more bothersome:
>   Checked<int32_t, RecordOnOverflow> checkedNewValue = 0;
>   checkedNewValue -= value;
>   int32_t newValue;
>   if (CheckedState::DidNotOverflow == checkedNewValue.safeGet(newValue)) {
>     ... (newValue)...
>   }
> Instead of
>   if (value != INT32_MIN) {
>     ... (-value) ...
>   }
> 
> If you think it would be better, I can do the change and use it, it is as
> you want.

Keep it as you have it. r=me still

[WebKit Commit Bot]

Comment on attachment 322838 [details]
Patch

Clearing flags on attachment: 322838

Committed r222981: <http://trac.webkit.org/changeset/222981>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/34857276>