Bug 54687 – Invalid label can be added into backtrack when building with armcc

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

[Yong Li]

Yarr::GenerationState::BacktrackDestination::hasDataLabel() doesn't work when it is built with armcc, because JmpDst::isSet compares m_offset against -1, and m_offset is declared with "int m_offset : 31". According to standards, it depends on the compiler implementation that "int" bit field is signed or unsigned. After changing it to "signed int m_offset : 31", it works well.

[Yong Li]

Created attachment 82858 [details]
the fix

[Yong Li]

There are other cases where we use "int" to declare signed bit field. We should also change them to "signed int". That should be low priority and I prefer to do that in another bug

[Alexey Proskuryakov]

Is this covered by existing regression tests?

[Yong Li]

(In reply to comment #3)
> Is this covered by existing regression tests?

I'll give a try. It is hard for me to write a test case for this because I know very little about how YarrJIT works. The change is harmless. I also plan to make such change to other places including those in WebCore even they haven't caused any known problem. Probably we should have this rule in coding guidelines: "Always explicitly use "signed"/"unsigned" to declare integral bit fields"

[Eric Seidel (no email)]

Comment on attachment 82858 [details]
the fix

We've had related trouble with bitfields using MSVC as well.  There the problem was that enum is a "signed int" and when you make it a bitfield it uses one of the bits as a sign when you don't expect it to. :)

This change seems fine.  Seems you might want a comment here to explain why you need the "signed" part.

[Yong Li]

(In reply to comment #5)
> (From update of attachment 82858 [details])
> We've had related trouble with bitfields using MSVC as well.  There the problem was that enum is a "signed int" and when you make it a bitfield it uses one of the bits as a sign when you don't expect it to. :)
> 
> This change seems fine.  Seems you might want a comment here to explain why you need the "signed" part.

Thanks. I plan to add "sign" to all similar places to make the code independent from the implementation differences of compilers (seems Qt always uses "signed" to declare signed bitfields). After that is done, the change in ARMAssember.h will not be special.

[Yong Li]

Bug 54807 created for the rest places we use "int"/"short" to declare bitfields

[WebKit Commit Bot]

Comment on attachment 82858 [details]
the fix

Clearing flags on attachment: 82858

Committed r79124: <http://trac.webkit.org/changeset/79124>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

This patch makes no sense to me. "int" is always signed in C++.

Eric, I don't understand why this has been given green light. There is no explanation of what specific problem this patch fixes, and according to comment 4, even regression tests haven't been run!

[Eric Seidel (no email)]

(In reply to comment #10)
> This patch makes no sense to me. "int" is always signed in C++.
> 
> Eric, I don't understand why this has been given green light. There is no explanation of what specific problem this patch fixes, and according to comment 4, even regression tests haven't been run!

The patch seemed reasonable to me.  Yes, ideally there should have been a link to the armcc bug (as I requested in the other bug he filed on this subject).

He states in comment 0 that according to his (I assume yong is a he?) reading of the c++ spec ints in bitfields are unspecified as to their sign.  I've not confirmed this.  But I could believe that armcc might have such a bug.

The cq obviously ran the regression tests. :)

Feel free to roll out the change.

[Alexey Proskuryakov]

> The cq obviously ran the regression tests. :)

I meant, the tests haven't been run on the affected platform.

Since this isn't breaking anything, there is no rush to roll out, and I'll give some time for everyone to comment.

[Alexey Proskuryakov]

Daniel Bates explained in bug 54807 that I was wrong - bitfields are an exception where "signed int" is different from "int".

I think that this should have used "signed", not "signed int" though, just like we never write "unsigned int" in WebKit code. And it doesn't look like we had a good explanation of why there is no regression test.

[Yong Li]

(In reply to comment #13)
> Daniel Bates explained in bug 54807 that I was wrong - bitfields are an exception where "signed int" is different from "int".
> 
> I think that this should have used "signed", not "signed int" though, just like we never write "unsigned int" in WebKit code. And it doesn't look like we had a good explanation of why there is no regression test.

I just tried a few existing JS tests, and I haven't been able to find one that doesn't crash without the fix. (For example, fast/js/const.html, fast/js/array-foreach.html, ...)