Bug 154223 – [JSC] Add some extra validation for the first basic block in B3 and Air

• Status: NEW
• Component: New Bugs

[Benjamin Poulain]

[JSC] Add some extra validation for the first basic block in B3 and Air

[Benjamin Poulain]

Created attachment 271301 [details]
Patch

[Csaba Osztrogonác]

Comment on attachment 271301 [details]
Patch

r- due to build failures.

[Benjamin Poulain]

Created attachment 271487 [details]
Patch

[Build Bot]

Comment on attachment 271487 [details]
Patch

Attachment 271487 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/842033

New failing tests:
js/regress/Float32Array-to-Float64Array-set.html

[Build Bot]

Created attachment 271496 [details]
Archive of layout-test-results from ews116 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Benjamin Poulain]

Created attachment 271528 [details]
Patch

[Filip Pizlo]

Comment on attachment 271528 [details]
Patch

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

> Source/JavaScriptCore/b3/B3Validate.cpp:119
> +        if (m_procedure.size()) {

We should also validate that m_procedure.size() > 0.

> Source/JavaScriptCore/b3/B3Validate.cpp:121
> +            VALIDATE(!allPredecessors.contains(entryPoint), ("At ", *entryPoint));

You could just use entryPoint->predecessors, since we validate that it's equivalent to allPredecessors.

[Filip Pizlo]

Comment on attachment 271528 [details]
Patch

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

I think you could improve this a lot. Turning empty procedures/codes into a validation failure is probably the most important.

> Source/JavaScriptCore/b3/air/AirLiveness.h:194
> +#if !ASSERT_DISABLED

You could use "if (!ASSERT_DISABLED)". You could also condiyionalize this on validation being enabled.

> Source/JavaScriptCore/b3/air/AirLiveness.h:198
> +                ASSERT(Adapter::isValidOnCodeEntry(index));

... And make this a release assert

> Source/JavaScriptCore/b3/air/AirValidate.cpp:97
> +        if (m_code.size()) {

You should validate that m_code.size() > 0.

[Benjamin Poulain]

Created attachment 271594 [details]
Patch

[Geoffrey Garen]

Comment on attachment 271594 [details]
Patch

r=me

[Filip Pizlo]

Comment on attachment 271594 [details]
Patch

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

I think that the m_procedure.size() check in B3Validation should be replaced with a validation rule that m_procedure is not empty.

> Source/JavaScriptCore/b3/B3Validate.cpp:119
> +        if (m_procedure.size()) {

This should assert that m_procedure.size() is not zero.  An empty procedure is not valid.