Bug 223284 – Fixed undefined behavior bug in Const32Value::checkNegConstant().

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Lam]

This was causing a failure in testb3 on a release build.

[Mark Lam]

Created attachment 423402 [details]
proposed patch.

[Darin Adler]

Comment on attachment 423402 [details]
proposed patch.

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

> Source/JavaScriptCore/b3/B3Const32Value.cpp:109
> -    if (m_value == -m_value)
> +    // We're using volatile temps here to force Clang to actually compute these
> +    // bit values and compare them. Otherwise, Clang can just assume that they
> +    // are not equal since the result of (m_value == -m_value) is undefined
> +    // behavior.
> +    volatile uint32_t value = m_value;
> +    volatile uint32_t negatedValue = -m_value;
> +    if (value == negatedValue)

If we’re having trouble because we’re depending on undefined behavior, I suggest we select a solution that does not involve undefined behavior. Using volatile seems like the wrong workaround, since it’s depending even more on undefined behavior, other types. The way we are using here is not what volatile is for. I suggest this solution, which should work reliably:

    if (m_value == std::numeric_limits<int32_t>::min())
        return nullptr;

> Source/JavaScriptCore/b3/B3Const64Value.cpp:110
> -    if (m_value == -m_value)
> +    // We're using volatile temps here to force Clang to actually compute these
> +    // bit values and compare them. Otherwise, Clang can just assume that they
> +    // are not equal since the result of (m_value == -m_value) is undefined
> +    // behavior.
> +    volatile uint64_t value = m_value;
> +    volatile uint64_t negatedValue = -m_value;
> +    if (value == negatedValue)
>          return nullptr;

Same:

    if (m_value == std::numeric_limits<int64_t>::min())
        return nullptr;

[Mark Lam]

Comment on attachment 423402 [details]
proposed patch.

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

>> Source/JavaScriptCore/b3/B3Const32Value.cpp:109
>> +    if (value == negatedValue)
> 
> If we’re having trouble because we’re depending on undefined behavior, I suggest we select a solution that does not involve undefined behavior. Using volatile seems like the wrong workaround, since it’s depending even more on undefined behavior, other types. The way we are using here is not what volatile is for. I suggest this solution, which should work reliably:
> 
>     if (m_value == std::numeric_limits<int32_t>::min())
>         return nullptr;

That's a great solution.  I should have thought of that.  Will give it a try.

[Mark Lam]

Created attachment 423411 [details]
proposed patch.

[Mark Lam]

Comment on attachment 423411 [details]
proposed patch.

Thanks for the review.  Landing now.

[EWS]

Committed r274538: <https://commits.webkit.org/r274538>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 423411 [details].

[Radar WebKit Bug Importer]

<rdar://problem/75502173>