Previously, this code assumed that IndexExpression::Convert had done
range checking and that it was safe to access the base expression at
the passed-in index. The inliner violates this assumption, because it
can replace unknowns (where out-of-range access is undefined but non-
fatal) with knowns (where out-of-range access is forbidden).
We now do range-checking inside IndexExpression::Make and report the
error cleanly, instead of asserting inside of Swizzle::Make due to an
invalid component index.
Change-Id: If0f31b1f694bcc2a875d124f70be311d6634c77b
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/469535
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
The ExpressionStatement currently eliminates dangling references without
reporting them as an error. This happens due to optimization; these
expressions (being meaningless) have no side effects, and so the
optimizer replaces them with Nop. When the optimizer is off, these
programs trigger an assert:
https://osscs.corp.google.com/skia/skia/+/main:src/sksl/SkSLAnalysis.cpp;l=582;drc=e7a953524787e3bd0c437ec52de4e40986689825
A followup CL will fix ExpressionStatements so that they report
incomplete expressions as an error.
Change-Id: Ica49166032e670749fc1b4e7a869fbab03364d4f
Bug: skia:12472
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/469524
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Much like http://review.skia.org/467759, this CL defensively guards
against programs which consume more space than is reasonable. Globals
exist outside of functions, so they wouldn't be caught by the stack size
checks.
Change-Id: I035f27d57bc329508820a729a1e367ecaadfe156
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/467760
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Functions that declare variables totaling more than 100,000 slots will
now generate an error.
This is only a partial mitigation to the problem, as a sophisticated
attack could still chain/nest multiple functions together to consume
extremely large amounts of stack. However, this mitigation is still more
sophisticated than our peers; both WebGL and glslang are susceptible to
similar problems, and in the general case (ES3+ with full flow control)
it's intractable.
Change-Id: I153c75267c017a23f59fe9e59f6e391197ee6101
Bug: oss-fuzz:40304, oss-fuzz:40694
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/467759
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
The fuzzer triggered this error in a strange way that involves parsing a
TK_INVALID token. The fuzzer's original input used \xFF bytes in the
shader text to do this. I replaced these with the ` character since it
behaved the same, but allows our test inputs to remain basic ASCII.
The root problem is that `cast_expression`, part of no-op arithmetic
simplification, can now fail because expressions like `int(4000000000)`
no longer get past Constructor::Convert. Previously we had assumed
`cast_expression` could never return null; now we check its result for
null before using it.
Change-Id: I7335395bab0daf1f788b0c7c154904b2372ae13f
Bug: oss-fuzz:40660
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/467316
Commit-Queue: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
It's possible to write code containing errors that are only apparent
once the inliner runs. For instance, a function which takes a short and
returns its negative it is valid for most inputs, but undefined for
-32768 (because +32768 does not fit in a short). A function which takes
floats and casts them to ints is valid for many inputs, but not valid if
you pass in 5 billion.
This CL restructures our out-of-range integer error detection to report
errors cleanly in these cases instead of asserting. It also refactors
the range checking code to be usable in situations where we don't yet
have a Literal expression.
Change-Id: I98f0be63bf9afbbf1ab90233fa86d380cfae42b4
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/466439
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Change-Id: Ic30c48dce0cb0072f07defcdb0b9e60b94f50818
Bug: oss-fuzz:40479
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/465392
Commit-Queue: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
A recent CL (http://review.skia.org/464121) made it an error to coerce a
literal value to a type that cannot hold the value. The fuzzer found a
case where we assumed type-coercion of a literal would always succeed,
and failed to null-check the result. We now null-check the result.
Change-Id: Id97c6016e56c20ef724028f71bbf4688dde3c064
Bug: oss-fuzz:40428
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/464919
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Yesterday's negation-related changes (http://review.skia.org/464123)
exposed a flaw that the fuzzer was able to exploit. We were previously
able to assume that `simplify_negation` would always return a non-null
expression; in some cases, that is no longer true.
Change-Id: Ia585232b0e35fafe0c642384a59ef94ce743ffd5
Bug: oss-fuzz:40427
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/464916
Commit-Queue: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
At this point, it seems like this was a mis-diagnosis of the underlying
issue around dual-source blending (and its interaction with other blend
state).
Change-Id: I11af0c9b70c32e14c353848db3d6adbfe5f08225
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/462176
Reviewed-by: Greg Daniel <egdaniel@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
At present, we only detect four errors here. We should detect six.
Change-Id: I226854ab930a273695c42cf2f7bdb1d5cd97e50b
Bug: oss-fuzz:39998
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/459882
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
We now detect attribute, varying, precision and invariant as reserved.
Change-Id: I8c90655a70b1bad31bf6143c3fdcb2ce582320b1
Bug: skia:12484
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/459479
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
`samplerCube` is a type which we don't support at all. It has been added
to the reserved-word list.
`textureCube` was in our list of built-in types, but was not actually
used in any way; it wasn't actually added to the root or private symbol
tables, and was totally unreferenced by the code. It's been deleted.
Change-Id: I4f79ce5d40ac6ebdb2a7067fa60cc79e316b01b6
Bug: skia:12484
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/459123
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
This reverts commit eb68973c2f.
Reason for revert: ES2 conformance test checks this
Original change's description:
> Disallow matrix ctors which overflow a column.
>
> The GLSL spec allows matrix constructors containing vectors that would
> split between multiple columns of the matrix. However, in practice, this
> does not actually work well on a lot of GPUs!
>
> - "cast not allowed", "internal error":
> Tegra 3
> Quadro P400
> GTX 660
> GTX 960
> - Compiles, but generates wrong result:
> RadeonR9M470X
> RadeonHD7770
>
> Since this isn't a pattern we expect to see in user code, we now report
> it as an error at compile time. mat2(vec4) is treated as an exceptional
> case and still allowed.
>
> Change-Id: Id6925984a2d1ec948aec4defcc790a197a96cf86
> Bug: skia:12443
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/449518
> Commit-Queue: John Stiles <johnstiles@google.com>
> Auto-Submit: John Stiles <johnstiles@google.com>
> Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Bug: skia:12443
Change-Id: I5a32744c88b9b830ad657488824c8c7dd0b0a652
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/458056
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: Leon Scroggins <scroggo@google.com>
Previously, in public code, private types didn't exist anywhere in the
symbol table chain, and those names were free for the taking. Now, we
register them as invalid types in the public symbol table. This prevents
them from being used as variable names, and gives a more explicit error
if you try to use them as a type.
Change-Id: I9a943bf923639b72cbf36b1acf4b4fbe70982786
Bug: skia:12538
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/459119
Commit-Queue: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
This fixes GLSL ES2 conformance test `array`.
Change-Id: I6ebee9253e1e8c394d9ddb6899e3a0940b7a38ef
Bug: skia:12495
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/458718
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
This should fix a failure in the ES2 conformance suite's "const_in_int".
Change-Id: I8b5487749291ef57712b8fe6c3949dc7c3e76883
Bug: skia:12499
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/455157
Reviewed-by: Brian Salomon <bsalomon@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Commit-Queue: Brian Salomon <bsalomon@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
This reverts commit 5f15c695f9.
Reason for revert: landed http://ag/15959743 to fix Android roll
Original change's description:
> Revert "Mark GLSL reserved names as reserved in SkSL grammar."
>
> This reverts commit 57f3fc4cde.
>
> Reason for revert: breaking Android roll
>
> Original change's description:
> > Mark GLSL reserved names as reserved in SkSL grammar.
> >
> > We now reject every reserved name in the ES2 docs as an unexpected
> > token, except for the rule that all names beginning with `gl_` are
> > reserved. (Unfortunately, sksl_frag bends the rules by directly
> > declaring a builtin variable named `gl_SecondaryFragColorEXT`.)
> >
> > Change-Id: I5dcb40b754720ca97fe3d80e2f9072beaa39fcdb
> > Bug: skia:11115
> > Reviewed-on: https://skia-review.googlesource.com/c/skia/+/454737
> > Auto-Submit: John Stiles <johnstiles@google.com>
> > Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
> > Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
>
> Bug: skia:11115
> Change-Id: Ica56f48dc76ef1e52780acaf59b8ad9143637637
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/454860
> Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
> Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Bug: skia:11115
Change-Id: I012b8d4e03be7f9c888c26d912552412529b4fb6
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/455159
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
This reverts commit 57f3fc4cde.
Reason for revert: breaking Android roll
Original change's description:
> Mark GLSL reserved names as reserved in SkSL grammar.
>
> We now reject every reserved name in the ES2 docs as an unexpected
> token, except for the rule that all names beginning with `gl_` are
> reserved. (Unfortunately, sksl_frag bends the rules by directly
> declaring a builtin variable named `gl_SecondaryFragColorEXT`.)
>
> Change-Id: I5dcb40b754720ca97fe3d80e2f9072beaa39fcdb
> Bug: skia:11115
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/454737
> Auto-Submit: John Stiles <johnstiles@google.com>
> Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
> Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Bug: skia:11115
Change-Id: Ica56f48dc76ef1e52780acaf59b8ad9143637637
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/454860
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
We now reject every reserved name in the ES2 docs as an unexpected
token, except for the rule that all names beginning with `gl_` are
reserved. (Unfortunately, sksl_frag bends the rules by directly
declaring a builtin variable named `gl_SecondaryFragColorEXT`.)
Change-Id: I5dcb40b754720ca97fe3d80e2f9072beaa39fcdb
Bug: skia:11115
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/454737
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
This reverts commit 45e3838006.
Reason for revert: Also need to rewrite them in actual ES2 mode.
Original change's description:
> Rewrite switch statements in GLSL strict-ES2 mode.
>
> Once this lands, switch statements will work everywhere--Metal, SPIR-V,
> GLSL, and SkVM.
>
> Change-Id: I2797d0a872de8be77bb9f7aa6acb93421d571d70
> Bug: skia:12450
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/452356
> Commit-Queue: John Stiles <johnstiles@google.com>
> Auto-Submit: John Stiles <johnstiles@google.com>
> Reviewed-by: Brian Osman <brianosman@google.com>
Bug: skia:12450
Change-Id: I92656ed40289872405c0873f2c56a52b04e35b1d
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/452556
Auto-Submit: Brian Osman <brianosman@google.com>
Commit-Queue: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Once this lands, switch statements will work everywhere--Metal, SPIR-V,
GLSL, and SkVM.
Change-Id: I2797d0a872de8be77bb9f7aa6acb93421d571d70
Bug: skia:12450
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/452356
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
SkVM implements switches as a pseudo-loop; breaks are handled with the
condition mask just like a for loop. Fallthrough is handled via a
scratch Value in a temporary slot. `writeStore` neeeded to be refactored
to support writing into slot(s) without an associated Variable.
At IR generation time, SwitchStatements are now emitted without error
even in strict-ES2 mode. The GLSL code generator currently reports these
as an error in strict-ES2 mode, but this will be fixed in a followup
coming shortly (the switch will be rewritten as ifs inside a one-shot
loop, similar to our IR-rewrite strategy).
Change-Id: I5507257246c42a35d2f46b4b9a89492a5ffeff9b
Bug: skia:12450
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/451421
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
This fails on several platforms in practice, and is of very limited
real-world utility.
Change-Id: Ib476396fc33cb51af6bbcf7fe822d30703ed995d
Bug: skia:12467
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/450993
Commit-Queue: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Change-Id: I17b5e21a28140b8e9313d87af9b1145674214fdb
Bug: skia:12450
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/450989
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Also, removed "switch containing dead code" test. This wasn't testing
anything meaningful. (When we had full CFG analysis, we could have
eliminated some of the assignments inside the switch body, but this is
not something we do anymore.)
Change-Id: Iaeb74ebee41a7f368113ede9a4e30c033b9de8ac
Bug: skia:12450
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/450985
Commit-Queue: John Stiles <johnstiles@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
The Mac Radeon GLSL driver crashes when given a switch statement that
only contains a default case and returns a value. Adding a case works
around the crash, and doesn't affect the meaning of the switch.
Change-Id: Iabbd267e0e31e8df7d3b7e747a7204d50931d0be
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/450977
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
While I was in this code, I realized that the setVariable method of
InterfaceBlock was unused and there was therefore no reason to be
storing a pointer instead of a reference.
Bug: oss-fuzz:39000
Change-Id: If7505ba87f4060370cfd32ca2e30c76648965101
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/450446
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
This is a reland of be056f4f62
The Switch test has been restructured to dodge an iOS bug.
Original change's description:
> Add switch statement support to PipelineStage.
>
> This allows us to write SKSL_TEST_ES3 tests in SkSLTest and have them
> run properly. Previously, such a test would assert inside the pipeline-
> stage generator. In ES2 mode, we will rewrite switches as chained ifs,
> but in ES3 mode we will want to continue emitting them as-is (they will
> be faster than chained ifs on a modern GPU).
>
> `writeSwitchStatement` is adapted from GLSLCodeGenerator.
>
> Change-Id: I532ea5ed49869e7cdffced0cdcd0e353af8d4d79
> Bug: skia:12450
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/450478
> Commit-Queue: John Stiles <johnstiles@google.com>
> Commit-Queue: Brian Osman <brianosman@google.com>
> Auto-Submit: John Stiles <johnstiles@google.com>
> Reviewed-by: Brian Osman <brianosman@google.com>
Bug: skia:12450
Change-Id: I5102081c636ef09cd23f5bc894e6c96e92a4c121
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/450757
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
This reverts commit be056f4f62.
Reason for revert: apparently switch on iOS GLSL is extremely broken
Original change's description:
> Add switch statement support to PipelineStage.
>
> This allows us to write SKSL_TEST_ES3 tests in SkSLTest and have them
> run properly. Previously, such a test would assert inside the pipeline-
> stage generator. In ES2 mode, we will rewrite switches as chained ifs,
> but in ES3 mode we will want to continue emitting them as-is (they will
> be faster than chained ifs on a modern GPU).
>
> `writeSwitchStatement` is adapted from GLSLCodeGenerator.
>
> Change-Id: I532ea5ed49869e7cdffced0cdcd0e353af8d4d79
> Bug: skia:12450
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/450478
> Commit-Queue: John Stiles <johnstiles@google.com>
> Commit-Queue: Brian Osman <brianosman@google.com>
> Auto-Submit: John Stiles <johnstiles@google.com>
> Reviewed-by: Brian Osman <brianosman@google.com>
Bug: skia:12450
Change-Id: If40c90023a64c608181285f6470b3e75303cc3cc
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/450756
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
This allows us to write SKSL_TEST_ES3 tests in SkSLTest and have them
run properly. Previously, such a test would assert inside the pipeline-
stage generator. In ES2 mode, we will rewrite switches as chained ifs,
but in ES3 mode we will want to continue emitting them as-is (they will
be faster than chained ifs on a modern GPU).
`writeSwitchStatement` is adapted from GLSLCodeGenerator.
Change-Id: I532ea5ed49869e7cdffced0cdcd0e353af8d4d79
Bug: skia:12450
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/450478
Commit-Queue: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Missed one more case of Optional<Wrapper<Expression>>. This should be
the last one.
Bug: oss-fuzz:38944
Change-Id: Ic7f790cd99e2a3ee1c3874cc767a4702265d1723
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/450476
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Missed a case when eliminating optional/wrapper in an earlier CL.
Change-Id: If7f80ea6e2172acadf7b0087fe1a05853ccae445
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/449838
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
The GLSL spec allows matrix constructors containing vectors that would
split between multiple columns of the matrix. However, in practice, this
does not actually work well on a lot of GPUs!
- "cast not allowed", "internal error":
Tegra 3
Quadro P400
GTX 660
GTX 960
- Compiles, but generates wrong result:
RadeonR9M470X
RadeonHD7770
Since this isn't a pattern we expect to see in user code, we now report
it as an error at compile time. mat2(vec4) is treated as an exceptional
case and still allowed.
Change-Id: Id6925984a2d1ec948aec4defcc790a197a96cf86
Bug: skia:12443
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/449518
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
The fuzzer discovered that it could overflow the program-size value.
Rewrote the logic to use SkSafeMath everywhere, and to early-exit as
soon as a statement manages to exceed the program size.
Change-Id: I01511b2201173c95ebc1ac602901410ac9d74d73
Bug: oss-fuzz:38697
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/449098
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Adjusted default caps in skslc to be consistent with runtime behavior,
and added optional settings mode to enable the feature. Tests for both
scenarios. (The error test crashed prior to the fix).
Bug: oss-fuzz:38726
Change-Id: I5270d4837ac982085d7baf5abd4b361f7bfb8562
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/449062
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
This is a reland of db38ad7b14
Original change's description:
> Fixed DSL assertion error on source files containing nulls
>
> The assertion was there to make sure we weren't running off the end of
> the source, but naturally fails in the presence of legitimate embedded
> nulls.
>
> Change-Id: I3b80499e9b182c9ea046c479f35d7a965d548401
> Bug: oss-fuzz:38107
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/447182
> Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
> Reviewed-by: John Stiles <johnstiles@google.com>
> Reviewed-by: Brian Osman <brianosman@google.com>
Bug: oss-fuzz:38107
Change-Id: Idb1a6b7c64d2bb954edadae828d6de808158fd3f
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/448660
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
The fuzzer has discovered a bug in our program size-checking logic; for
loops that immediately contain another for loop (with no block) were not
counting the inner loop's iterations. This allowed it to exceed our
maximum program-size threshold (and time out during SkVM compilation).
This test demonstrates the issue. A followup will fix it.
Change-Id: I3b7d4c8a4f0ed04cf0aba3f1a32fdad7d6d784e7
Bug: oss-fuzz:37837
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/449096
Commit-Queue: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
We already had a test case here, but it wasn't actually in operation.
The test has been split into ES2 (square) and ES3 (non-square) halves,
returns the color like a proper runtime effect, and it's now running in
dm.
Also, Metal doesn't natively support matrixCompMult, so it injects a
helper function; I tweaked the helper so it no longer requires an extra
result variable.
Change-Id: Ie79242768966fcbe879ad73461d17b4fb8e55670
Bug: skia:12202
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/448117
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
This exposes a bug in the Metal code generator which will be resolved
in a followup CL.
Change-Id: If073835dbee474ea9a805eb92b42dc1fca2afbd0
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/448378
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
The fuzzer found that the `DetectVarDeclarationWithoutScope` check was
placed too late in the function, and could be skipped over by for-loops
containing multiple variables. This was caught in ForStatement::Make,
which mirrors the Convert postconditions with matching assertions.
Change-Id: I6e9d97c7c9ca969aba65e601bbcd9fe676105838
Bug: oss-fuzz:38560
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/448116
Commit-Queue: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
This reverts commit db38ad7b14.
Reason for revert: breaking g3 roll since it thinks the test case is "binary" not flagged as binary
Original change's description:
> Fixed DSL assertion error on source files containing nulls
>
> The assertion was there to make sure we weren't running off the end of
> the source, but naturally fails in the presence of legitimate embedded
> nulls.
>
> Change-Id: I3b80499e9b182c9ea046c479f35d7a965d548401
> Bug: oss-fuzz:38107
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/447182
> Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
> Reviewed-by: John Stiles <johnstiles@google.com>
> Reviewed-by: Brian Osman <brianosman@google.com>
Bug: oss-fuzz:38107
Change-Id: I650d12d728b5d932bda79e81205b873d8b44771f
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/447936
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Commit-Queue: Michael Ludwig <michaelludwig@google.com>
The assertion was there to make sure we weren't running off the end of
the source, but naturally fails in the presence of legitimate embedded
nulls.
Change-Id: I3b80499e9b182c9ea046c479f35d7a965d548401
Bug: oss-fuzz:38107
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/447182
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Bug: oss-fuzz:38140
Change-Id: I76a1b3ef8289b3089192d043d173677c00741a54
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/445836
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
In response to a non-identifier token after a dot, DSLParser would
attempt to swizzle a zero-length field and fail an assertion.
The same basic code path exists in the old compiler, but the resulting
parse error causes the process to abort before it attempts to process
the zero-length swizzle.
Bug: oss-fuzz:38106
Change-Id: Ifd997ce1d564b5f6ef0a9a785d8d9e254785e600
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/446185
Reviewed-by: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
This includes compile-time optimization and tests.
The unit test is disabled in a followup CL
(http://review.skia.org/447057) because it exposes a Radeon 5300M bug
in OpenGL.
Change-Id: I8b2f0411358aeb68c4edfeb0bd7a2814c4be1f40
Bug: skia:12202
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/447056
Reviewed-by: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
DSL was improperly allowing interface blocks in runtime shaders, which
caused PipelineStageGenerator to get upset.
Bug: oss-fuzz:38131
Change-Id: I593e68f2cab3db9151d606e65e2826ffa9c494e2
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/446324
Commit-Queue: Ravi Mistry <rmistry@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
Intrinsic-call optimization can be triggered during inlining. In this
case, inlining turned `normalize(x)` into `normalize(constant-value)`.
DSL is used to implement optimizations for a handful of intrinsic calls,
including `normalize`, which internally relies on `length`.
The DSL expects that it can use the IRGenerator to handle function
calls. This was not working because we were finished with the initial
compilation pass, and the IRGenerator's symbol table is removed when
finish() was called.
We now temporarily give a symbol table back to the IRGenerator while
the inliner runs. We remove it again as soon as inlining is complete.
Change-Id: I6da98788d93749ffeb008c1f4c3f72b436e8ceeb
Bug: oss-fuzz:37994
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/445956
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Our program-size analysis pass needs to recurse into called functions;
depending on the exact order of functions in the program, this recursion
can hypothetically be as deep as the deepest function-call chain. Set an
upper bound on recursion here, so we don't overflow the stack while
trying to check the program size. In practice, 50 frames is far deeper
than a regular shader should ever go.
Change-Id: I733ee48dad6f8053facdfd9f6d8a2b9b2a4af188
Bug: skia:12396
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/445279
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
The fuzzer is currently learning to make unboundedly-large programs by
nesting medium-size loops repeatedly. SkVM doesn't have a mechanism to
limit the ensuing explosion of code and ends up making unreasonably deep
stacks and/or unreasonably large programs.
SkSL now enforces an upper bound of approximately 100,000 IR nodes on a
fully-flattened, fully-inlined strict-ES2 program. The limit is picked
out of thin air, but this should be enough to prevent SkVM from going
haywire while still being large enough to handle any reasonable program.
We can definitely tune this value if we find that it is too large
(admitting dangerous code) or too small (rejecting good code).
Change-Id: I11735636175721fbc79460b4e194d8e4b42dc47d
Bug: skia:12396, oss-fuzz:37827, oss-fuzz:37837
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/444358
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
The fuzzer discovered that, when we attempt to verify that an array
doesn't contain any literal values that are out-of-range for its base
type, we pay a linear-time cost based on the size of the array. This
happens even when the array value isn't known at compile time; we still
iterate over its slot count and diligently discover that every single
constant-subexpression slot in the expression is "null".
We now have a helper function on Expression,
`allowsConstantSubexpressions`, which only returns true for expression
kinds that can contain constant subexpressions. We use this helper to
skip over this linear-per-subexpression check when the expression
cannot possibly contain a constant subexpression. In particular,
`AnyConstructor::compareConstant` and `Type::checkForOutOfRangeLiteral`
will now early-out for expressions that can't possibly contain a
constant subexpression.
Change-Id: Ia34e422afa67b478a8616acb0a0e9cd211b29698
Bug: oss-fuzz:37900
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/444136
Commit-Queue: John Stiles <johnstiles@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
We had a logic bug when attempting to optimize the following code:
const vecN x = vecN(a, b, c);
-x;
The goal was to replace `-x` with `vecN(-a, -b, -c)` but we accidentally
tried to cast the `x` VariableReference to a Constructor. We
unfortunately didn't cover this in any of our test cases, but the fuzzer
managed to synthesize it by mixing and matching elements from its new
corpus.
This affected several different constructor types: splat, diagonal-
matrix, compound and array.
Change-Id: I10dd2460ab26ba3e820b0cff5db091368fb7e648
Bug: oss-fuzz:37764, oss-fuzz:37861
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/443407
Commit-Queue: John Stiles <johnstiles@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Bug: skia:8451 skia:10827
Change-Id: I5b38a1d72cd4558f8e2a92aaf9b12f05efce0923
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/442683
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Chris Dalton <csmartdalton@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
This is a first step towards replacing `finalizeFunction` with a
`FunctionDefinition::Convert` method living outside of the IRGenerator.
Previously this code would assert that we had no early returns from a
vertex-program main() method; this has been turned into an error.
(The original assertion was also tied to fRTFlip, because the *problem*
with early-returns in main is tied to the lack of RTFlip fixups, but
we fundamentally don't allow early returns, so it makes more sense to
just universally disallow it.)
Change-Id: Iba0742f7ef3cbc83995ea130fec1eb1ef2556c44
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/442691
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
The fuzzer invented a much more elaborate example, but I was able to
winnow it down to a simple otherwise-normal test case. This also fixes
a latent DSL bug; DSL functions were not updating the list of referenced
intrinsics, so the compiler might emit finished programs that called
built-in functions that didn't exist in the code.
Change-Id: I095bb566b9db9f87cbe9460732c300b7973eb112
Bug: oss-fuzz:37659
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/442325
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
No-op arithmetic simplification will convert expressions like `x += 0`
to `x`. When making this simplification, we will also downgrade the ref-
kind of `x` from "write" to "read" since the new expression is no longer
an assignment.
The fuzzer discovered that the ref-kind downgrade was too aggressive,
and would also traverse into nested subexpressions and downgrade them
as well. That is, for `x[y=z] += 0` would convert both `x` and `y`
into "read" references, which is incorrect; `y` is still being written
to.
The fuzzer managed to turn this mistake into an assertion by leveraging
a separate optimization. It added a leading, side-effect-less comma
expression for us to detect as worthless and eliminate. In doing so, we
clone the expression with the busted ref-kind, triggering an assertion.
Change-Id: I42fc31f6932f679ae875e2b49db2ad2f4e89e2cb
Bug: oss-fuzz:37677
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/442536
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Our SPIR-V code generator did not implement support for negating a uint.
However, this is something that GLSL allows (as does the rest of SkSL).
I checked glslang and it uses OpSNegate here. The SPIR-V docs indicate
that OpSNegate allows any type of integer, and the validator lets it
pass, so we now use OpSNegate here as well.
http://screen/33mkq92uxAT5Xu8http://screen/4YBTh3gCWz8eZx7http://screen/388HtXyytcN5vLZ
Change-Id: I8c142018fd5e162dcd051abe1bc5d69a6e034794
Bug: oss-fuzz:37627
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/441880
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
The fuzzer detected a serious parsing error; a struct containing a
vardecl with multiple declarations would interpret arrays incorrectly.
An array would be applied to ALL variables in the decl after its initial
appearance. That is, `int w, x[10], y, z;` would be interpreted as
`int w, x[10], y[10], z[10];`. The fuzzer caught this by putting two
arrayed variables in a row; the second variable was interpreted as a
nested array, which led to an assertion.
This CL contains a simple hand-written test case demonstrating the bug,
with the fix coming in a followup.
Change-Id: I42d7372ba77fa1528ae24eb8c29a2e5903784139
Bug: oss-fuzz:37622
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/441878
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
We weren't coercing the expression because we don't care about its type,
but that allowed intermediate-expressions to pass through without
reporting an error. Now we coerce the expression to its present type,
which will always fail if the type is disallowed and succeed otherwise.
Change-Id: Ic0de0d17f0f5d56360575efe992ce4d74dec2a5a
Bug: oss-fuzz:37620
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/441876
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Much easier to maintain, especially with an upcoming change to the
sampling syntax.
Change-Id: I378811b7be0afcce5b7e68a942e7b46d96568155
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/441518
Commit-Queue: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: Brian Osman <brianosman@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
The inliner contained a type error when attempting to inline a function
that takes an array as input. The scratch copy of the array was created
as `float[123] var;` instead of `float var[123];`. This led to an
assertion in VarDeclaration::Make.
Change-Id: I5128fe71462bb59a015a7b4e59c1a74800828b16
Bug: oss-fuzz:37466
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/441576
Commit-Queue: John Stiles <johnstiles@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
This fixes an assertion failure uncovered by the fuzzer.
Bug: oss-fuzz:37469
Change-Id: I626c003cfa8a0bc65851899df3a7695dbe29200b
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/441311
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
During constant-folding, we baked in an assertion stating that any
const-typed variable reference ought to have an initial value, because
you can't declare a const variable without assigning a value. However,
function parameters are an exception to this rule! They are variable
references and are allowed to be const, but will not have an initial
value. (In this case, `const` just means you can't alter the value.)
In this case, all we needed to do was remove the assertion; we already
treated this case defensively and with the appropriate care.
Change-Id: I61242c6d08c59886c6992898f195771e6334f2b4
Bug: oss-fuzz:37465
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/441239
Commit-Queue: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
The fuzzer noticed insufficient guards in IndexExpression::Convert when
converting an array size from an IntLiteral to a SKSL_INT. We had code
in IRGenerator which did this properly, so I moved our array-size
conversion logic into SkSLType and had IndexExpression share it.
Also, a variety of tests around similar error conditions were added.
Change-Id: I51529dea25f9029f81ae236511610069d66be29f
Bug: oss-fuzz:37462
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/441236
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
We now stop processing a var-declaration if its array-size expression is
invalid. Previously, we'd pass a null array-size expression into
convertVar, which would assert (but would fail cleanly afterwards).
Change-Id: I976f3326e32afbc7045a86d73c0dcb28f418a6f4
Bug: oss-fuzz:37457
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/441079
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
If the passed-in shader references RTFlip (i.e., sk_FragCoord is used),
the settings must contain RTFlip layout info; otherwise, an error
occurs. Originally, the fuzzer detected this as a problem because the
error was being delivered via SK_ABORT, but it's failing more cleanly
now that Ethan's new error handling code is in place (causing the fuzzer
to report that the bug was "fixed"). With this CL, the oss-fuzz shader
will actually compile successfully in SPIR-V instead of leading to an
error.
Change-Id: I3268e84bd8e01c95a25ed0845a37324e98033c4b
Bug: oss-fuzz:35916
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/439779
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Surprisingly, we didn't actually have a preexisting test covering this.
Error reporting is lackluster in this CL but will be improved in the
followup.
Change-Id: I0b1cdb5a82f066af6b9d3fd9c39748080c2e18c0
Bug: skia:12348
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/439996
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
We can now add functions to sksl_public.sksl with an $es3 prefix. These
will be allowed in a Runtime Effect when strict-ES2 mode is disabled.
Note that the CPU backend still doesn't have support for these calls,
and will fail ungracefully (assertion, nonsense result) if these
intrinsics are used.
The testing here is limited, due to an unrelated bug in SPIR-V
(skia:12340)
Change-Id: I9c911bc2b77f5051e80844607e7fd08ad386ee56
Bug: skia:12202, skia:12340
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/439058
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
This modifier is currently allowed on built-in functions only.
The presence of this modifier will be used to indicate intrinsics which
are ES3-specific (and therefore, not allowed in user code under typical
circumstances).
Change-Id: Ice6be8d9d1b2bf0c8f07f2a89f335bb2f90f6681
Bug: skia:12202
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/439057
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Compiling a program with "allow narrowing conversions" actually fixes up
narrowing casts in the program by inserting casts wherever they would be
needed for type-correctness. For instance, compiling the statement
`half h = myFloat;`
inserts an appropriate narrowing cast:
`half h = half(myFloat);`.
The Pipeline stage code generator relies on this behavior, as when it
re-emits a runtime effect into a complete SkSL program, the narrowing-
conversions flag will no longer be set, but that is okay, because the
emitted code now contains typecasts anywhere they would be necessary.
Logically, this implies that anything which supports narrowing
conversions must be castable between high and low precision. In GLSL and
SPIR-V, such a cast is trivial, because the types are the same and the
precision qualifiers are treated as individual hints on each variable.
In Metal, we dodge the issue by only emitting full-precision types. But
we also need to emit raw SkSL from an SkSL program (that is what the
Pipeline stage generator does).
SkSL already supported every typical cast, but GLSL lacked any syntax
for casting an array to a different type. This meant SkSL had no array
casting syntax as well. SkSL now has array-cast syntax, but it is only
allowed for casting low/high-precision arrays to the same base type.
(You can't cast an int array to float, or a signed array to unsigned.)
Change-Id: Ia20933541c3bd4a946c1ea38209f93008acdb9cb
Bug: skia:12248
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/437687
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Most of the code generated by the fuzzer is nonsense, but there is a
method to its madness. The crash is only triggered under specific
conditions:
- The runtime effect has enough helper functions to mostly fill up the
call graph hash-map. It won't rehash until it gets close to capacity.
- There must be several calls to built-in functions, in order to add
elements to the call graph to force a rehash.
The fuzzer-generated code manages to satisfy both these requirements.
Change-Id: I9a1d7535557fedd4e9bfece3930ac86ede291ffe
Bug: oss-fuzz:36655
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/437118
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
The optimization logic for swizzling a constructor assumed that every
argument to the constructor was a scalar or vector. When it was written,
this assumption was true. However, we recently added support for casting
mat2x2 to float4 which violates the assumption.
We now check every argument and do not attempt to optimize if a
non-scalar, non-vector arg is found.
Change-Id: Ia2b297bd62dfdf4af56712164fbc80c29c9611eb
Bug: oss-fuzz:36852
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/437017
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
OSSFuzz discovered a minor variation of oss-fuzz:36770 which tickled a
different bug in SPIR-V RTFlip handling; we did not properly handle the
case where the InterfaceBlock is an array. SPIR-V does not support this
at all, but the IRGenerator allows it, and we don't detect it an an
error until later in the compilation process.
Change-Id: I80bd67a13dad878717dc122462132a2ed675532d
Bug: oss-fuzz:36850
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/437018
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
This CL does not update the DSLParser to honor these precision
qualifiers; that will be done in a followup.
Change-Id: Ib629bc99c0e6c7afb550a381d4e3b6ccc26aa64e
Bug: skia:12248
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/436337
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
These parse into new modifier bits; the IR generator does not yet
support these bits. That's coming in a followup CL.
Change-Id: I362e9227694f9b862eaad100f6afca45a9b62a01
Bug: skia:12248
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/436336
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
We don't currently support this. There's no explicit syntax to cast an
array's type, but it can be implicitly required in some situations, like
`halfArray == floatArray` (when fAllowNarrowingConversions is on).
Change-Id: I00fe0ddd4f2682b2950e828dd78bb941d5f0430e
Bug: skia:12248
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/436560
Commit-Queue: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
This code intentionally mixes half4s and float4s everywhere. Before
http://review.skia.org/435916 landed, this resulted in a compile error.
Change-Id: I852fef6ee99a8b78623e0e9ddeee2ad84a8c0504
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/436058
Commit-Queue: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
SPIR-V code generation synthesizes some extra variables that don't
actually exist in the Program. Checking the ProgramUsage of these
variables would fail; ProgramUsage::get doesn't know about these
variables, so it asserts (and would consider them as dead even if it
didn't assert). We now track our SPIR-V bonus variables in a separate
set, and always report them as live.
Change-Id: If2f681470654025abf7ca4b3ec8126de2eb01297
Bug: oss-fuzz:36770
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/435625
Commit-Queue: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Change-Id: I7b53df1eae83a596c4d1f3620e7f9bd146f68af2
Bug: oss-fuzz:36655
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/434465
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
This relaxes our rules to allow calls to declared (but not yet defined)
functions. With that rule change, we have to specifically detect static
recursion and produce an error.
Bug: skia:12137
Change-Id: I39cc281fcd73fb30014bc7b43043552623727e03
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/431537
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Bug: skia:12137
Change-Id: I609dd2578bf39a30e036ea85281886f8c4554579
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/431038
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
At present, they aren't hooked up to anything. They will be made
functional in followup CLs.
Change-Id: I4bfc25eb4e19fce4c36ea0b55494bf37b2a9ee23
Bug: skia:12248
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/430637
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
SkSL does not support shrinking a vector via casting. Use a swizzle
instead.
Change-Id: Ieba78a05dad9c55f44c765924e28f0c7e1667a67
Bug: skia:12193
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/427198
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Boolean scalar-swizzling is currently not working.
Change-Id: Icd965e4b64a12311d098168f65622110d5fb3437
Bug: skia:12195
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/427038
Commit-Queue: John Stiles <johnstiles@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
The tests now check bool4-mat2 conversions, which fortunately do work,
and the vector-to-matrix tests include int and bool conversions as well.
Change-Id: I971271838a93081b9258deb7c1d13b7732fb2440
Bug: skia:12067
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/426757
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
The fuzzer quickly discovered that the newly introduced mat2-to-vec4
conversion code did not account for integer vectors. We now handle
`ivec4(mat2)` casts properly. This required some non-trivial
restructuring of the logic, but in the vast majority of cases, the types
will match and the end result will be identical.
Change-Id: If07c2fe4b4345bd767384b1802374910f65cd3f0
Bug: oss-fuzz:35998
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/426756
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
GLSL disallows mixing swizzle domains within a single swizzle:
http://screen/93eHNQDbx35hMdk
SkSL now disallows it as well.
Change-Id: Ied2e11ee04285b143a864e28cac30335f01aad0e
Bug: skia:10621
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/426458
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
GLSL supports casting vec4 into mat2 and vice versa, so SkSL should have
equivalent support. This CL allows the Compound constructor to take a
matrix as input, and fixes up backends to do the right thing when a
matrix shows up in the compound-constructor path.
Change-Id: I13289ad0a27ba59bddc3706093820594efebc693
Bug: skia:12067
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/426003
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Bug: skia:11296
Change-Id: I7d41614957d6fa535faadebbeca890b54b6977ac
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/425996
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
This reverts commit a89781215a.
Reason for revert: breakage on Windows
Original change's description:
> Add tests for matrix-vector conversions.
>
> GLSL supports casting vec4 into mat2 and vice versa, so SkSL should have
> equivalent support. Adding tests as a starting point.
>
> Change-Id: If8bcbf99afcec94d948d5da9e6205cb4a232af18
> Bug: skia:12067
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/425837
> Auto-Submit: John Stiles <johnstiles@google.com>
> Commit-Queue: Brian Osman <brianosman@google.com>
> Reviewed-by: Brian Osman <brianosman@google.com>
TBR=brianosman@google.com,ethannicholas@google.com,johnstiles@google.com
Change-Id: I2563041f538b1b20074385f1b61af5fc506ffad5
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: skia:12067
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/426057
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
GLSL supports casting vec4 into mat2 and vice versa, so SkSL should have
equivalent support. Adding tests as a starting point.
Change-Id: If8bcbf99afcec94d948d5da9e6205cb4a232af18
Bug: skia:12067
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/425837
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Change-Id: I4066aafc5b6137bfaf38100ff237fd9833023f34
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/424097
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
