This is illegal in older versions of GLSL and in Metal. We now fail at
SkSL compilation time and properly report the error.
Change-Id: I6ddaeabff5386a1ed6ca3eb8703a6035476ec77a
Bug: skia:11021
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/339298
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
GLSL only supports arrays of samplers in very limited ways; they aren't
supported at all by SkSL. We now detect arrays of opaque objects and
reject the code.
We have several paths through the IR generator that create and process
array types; the unit test covers global and local variables, and array
on the type versus array on the variable.
Change-Id: I5b45e88e31cf4005723c3bf35561622d65321f7b
Bug: skia:11008
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/339317
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Previously, GLSL and Metal code generators would emit a struct wherever
the type was first used in the code, regardless of where it was
originally defined or what scope the type needs to live in. This CL adds
a ProgramElement for struct definitions, so that structs will now appear
at the top-level as they were originally defined. In the case of Metal,
some special handling is also needed to handle the Globals struct
properly.
Not yet fully supported:
- No special handling for structs declared inside functions yet
- No support for structs in separate scopes with overlapping names
The severity of the remaining issues depends mostly on whether we want
to support structs inside functions in Runtime Effects.
Change-Id: Ia95d4529506cb3fa6da63f5cb548199a93e1c0c5
Bug: skia:10922, skia:10923, skia:10925, skia:10926
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/338600
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Previously, we allowed unary minus on numbers and vectors (of any type).
Now, we allow them on numbers and vectors of numbers.
Also updated the Boolean arithmetic error test to cover scalars as well
as vectors.
Change-Id: Ie74d1f3bfc1e9353e04c6f8e468fa20e0cbba16f
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/338396
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
GLSL does not allow most binary operations on bvec types; we can now
detect these and properly flag them as errors.
Note that `determine_binary_type` was also refactored. It originally
started with an enormous omni-switch over every possible Token type,
used to set various bools describing the type of binary expression at
hand. Instead of one big switch, this has been refactored into several
small switches in standalone functions that simply switch on the op and
immediately return true or false. Conceptually this seems like more
work (checking the op multiple times), but these tiny switches actually
boil down to little branchless shift-and-mask functions, so in practice
they should be quite efficient compared to the original omni-switch.
Change-Id: I81b473d98c65da1edd136f35fc8f656261f8930d
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/338346
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Previously, we assumed that if a vector in `is_constant` was not made of
floats, it must be made of integers. This ignores that boolean vectors
also exist. The original code would abort when `getIVecComponent` was
called on a bool vector.
There is another bug here--arithmetic operators on bool types should be
disallowed entirely. That will be addressed in later CLs.
Change-Id: I78781d839abde9376917fd92f2fe6311a1a58b02
Bug: oss-fuzz:27808
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/338055
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Previously, the code which calculated Constructor constant values
assumed that a constant-value PrefixExpression would always have an
operand of Constructor. It turns out that another valid case is multiple
PrefixExpressions nested within each other (representing repeated
negation). Updated the code to work regardless of the type of the prefix
operand.
Change-Id: Ic9bf54725ae59330ac817bc4ec7a64def384ab54
Bug: oss-fuzz:27663
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/337177
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
The fix submitted at http://review.skia.org/335868 did not support
casts. The fuzzer discovered this shortcoming right away.
Change-Id: I2f5166528cee41367348564d4e664476fd5704ff
Bug: oss-fuzz:27650
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/336656
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
This was slightly complicated by the fact that this syntax indicates an
array with a known size:
float[] x = float[](1, 2, 3, 4);
Of course, the size is 4; it's just never explicitly stated in the
code. (The SkSL parser never actually deduces the size, but it doesn't
apparently have a need to; we don't do much in the way of optimization
for arrays.) However, this prevents us from simply failing whenever we
parse "[]" in non-builtin code; we need to keep scanning and see if the
variable is initialized. We already check this in the
ArrayConstructors.sksl test file.
Change-Id: I5b86958e81bd9bf5edf28a617cecf95c1875583e
Bug: skia:10957
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/335240
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Previously, the Type's fOffset was set to -1 during parsing, so any
errors related to the Type would be reported on line 1.
Change-Id: I9834f733bc763c5946b3ff81d8aef4807cdc13d1
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/335584
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 is a followup to http://review.skia.org/335196. This detects opaque
types (samplers and textures) at parsing or IR generation time and
reports an error regardless of backend. This check occurs before Metal
or SPIR-V would have a chance to detect the error, so it changes their
output to a slightly more focused error message. The Metal/SPIR-V fix in
the prior CL is still a nice broad catch-all for preventing spurious
ABORTs, though.
Change-Id: I4cce92a8767d72b5d3d7277a8afde8ce5ce86db2
Bug: skia:10956
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/335217
Commit-Queue: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
This fixes a fuzzer crash in Metal.
Private types aren't meant to be used directly; we can't generate a
valid MemoryLayout for them. We will now detect them during IR
generation and report an error. (Note that unreferenced structs
currently don't have any IR representation at all, so structs have to be
used somewhere in the code to trigger the error.)
Bug: oss-fuzz:27288
Change-Id: I432f0a69fbb54cd33ff5b90a9f3d4757a9370117
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/334830
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
At present, we do not report any error; the values wrap silently.
Change-Id: I8c435cfdd81f6c2e5fd87e9c39c708138bf4ec82
Bug: skia:10932
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/333676
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
This addresses a sanitizer issue discovered in
https://oss-fuzz.com/testcase-detail/4908118777266176 (it has not been
assigned an oss-fuzz bug number yet; coming soon)
This puts an upper bound on struct nesting, again to prevent memory-
layout and other recursive type-handling code from overflowing the
stack. Coincidentally, while researching GLSL behavior around this bug,
I learned that WebGL has a similar limitation but caps nested structs to
4 deep. (I could not find any documented GLSL upper bound.)
Note that both the GLSL and Metal outputs for StructMaxDepth are badly
malformed. (Structs cannot be embedded within another struct in GLSL;
structs SA7 and below are never declared in GLSL; the array list for SA7
is backwards in GLSL; Metal is missing structs SA1 through SA8; Metal
puts the array list on the type instead of the variable name.)
These issues will be addressed in separate CLs.
Change-Id: I0f1059b6faa400cd0647dd7010ec839f73779a36
Bug: skia:10922, skia:10923, skia:10925, skia:10926
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/333316
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
This addresses a sanitizer issue discovered in
https://oss-fuzz.com/testcase-detail/4908118777266176 (it has not been
assigned an oss-fuzz bug number yet; coming soon)
We need to set some sort of limit here to avoid stack overflow. Eight
array dimensions seems like more than enough for any sort of code that
we might realistically need, but the limit is definitely flexible if we
wanted to increase it. (The fuzzer needed to generate a several-
hundred-dimensional array before encountering a crash.)
Change-Id: I3630ab40e47cc58a2280ba200b485e1958371fdc
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/333160
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
This addresses a sanitizer issue discovered in
https://oss-fuzz.com/testcase-detail/4908118777266176 (it has not been
assigned an oss-fuzz bug number yet; coming soon)
A followup CL will limit array dimensionality to 8. This is an arbitrary
choice which is hopefully larger than any reasonable program will need.
Change-Id: I4cf05f40ec92c1c3444c71c45f759bb30d7da3c9
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/333135
Commit-Queue: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
This resolves the fuzzer error, as the program will fail compilation
before reaching the SPIR-V translation stage at all.
Change-Id: Ia73af497b1f57314a29878f2d2a29dc80186e630
Bug: oss-fuzz:27300
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/333130
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
`in` vars shouldn't support initializer expressions at all. The fuzzer
noticed that dead-stripping interacts poorly with `in` var initializer
expressions, which makes sense because it's an unsupported and untested
path. In a followup CL, lines 1 and 3 will both become errors.
Change-Id: Ibb64ca319a046b040eea976acb6798a1402451de
Bug: oss-fuzz:27300
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/333128
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>
Previously, when a prototype was parsed, this added a function
declaration to the symbol table, but the prototype itself was not
re-emitted during code generation. This meant that the final code might
not be valid, since the absence of prototypes meant that the code might
attempt to invoke a function before its declaration. Now, prototypes are
stored in the ProgramElement list and re-emitted during code generation
for GLSL/Metal/CPP. (SPIR-V doesn't name its functions at all.)
Change-Id: I76446c796000eb0b56f964d82457122182c28b87
Bug: skia:10872
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/331136
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
- Prototypes for never-declared functions
- Prototype before use
- Prototype after use
- A variety of inputs and outputs on the prototyped functions.
- Calling declared-but-undefined functions
Currently, the prototypes are not actually emitted in the generated GLSL
or Metal output at all. This CL is demonstrates our baseline before
proper prototype support is added.
Change-Id: I6112e0a89ab9bbecefccaca9fba985bb8011fff1
Bug: skia:10872
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/331376
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
This error was caused by an unbalanced symbol table push. This could
occur when an interface block encountered an error while parsing its
var-decls.
Change-Id: I910a980ac92fac7c0786c48b8dc3003ee3e75e5b
Bug: oss-fuzz:26700
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/330896
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
There's no functional change here; it's just using a slightly higher-
level abstraction to pass the same payload.
Change-Id: Ife7efa038db5d6dbde5decae2be79ad9db877aba
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/329782
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
GLSL requires that functions are declared before first use. In most
cases, we avoid the requirement for explicit prototypes by this by
strategically ordering our functions within the emitted code, but an FP
file might reference its helper functions in any order or have helper
functions that cross-invoke each other, so prototypes should be emitted.
Change-Id: I3b9e9c9ec4bd5be90b4f71f8165af45364facf30
Bug: skia:10872
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/329781
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
This is necessary to support function calls in FP files properly; in
some cases, functions can be referenced before they have been emitted,
and we need to be able to name them.
This CL resolves the remaining errors in GrRecursion.cpp. There are
still additional errors in GrNestedCall.cpp that will be fixed in
followup CLs.
Change-Id: Iec98ef02ea6a98a9945a4e0e3cfa3537dff01305
Bug: skia:10684, skia:10872
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/329676
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Previously, we'd just emit functions with `%s` in their function bodies.
Also, the fFormatArgs wouldn't get cleaned up, so subsequent code would
fill in the wrong format arguments in each place.
There are still problems with function calls (`sample` is broken;
function names are accessed before they've been declared or initialized)
but this is a step in the right direction.
Bug: skia:10684
Change-Id: I7399a71d44ebba5049703484ec9933dffbe8e2b9
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/329417
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
The generated code does not assign to sk_OutColor correctly; it assigns
into the `factorial` function name instead, which doesn't make sense.
Change-Id: Ibad1d47f2f9c4fbb410b5277cea6e1022daf8b9d
Bug: skia:10684
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/329360
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>
These don't compile in GLSL, so they shouldn't compile in SkSL either--
and fortunately, they do not.
(In C++, and consequently in Metal, these expressions are considered
legal by the grammar and do compile, but generate garbage output.)
Change-Id: I6c7bea70b3d91677ccd8fcbad1eba123d655e856
Bug: skia:10694
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/329359
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>
As a prelude to going back to sharing global data (safely), we want to
eliminate as much mutation of shared state as possible. The special
cases for global variable declaration were unnecessary, so just remove
them. The editing of main's parameters immediately after they were
created is also unnecessary - just hoist the logic up so we create the
variables correctly in the first place.
There is still one use, related to invocation ID. That's more
complicated (?), so leaving it as a separate CL.
Change-Id: Ia3dad78dd5a634273b2e2239368be7adaff65f38
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/325661
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
Auto-Submit: Brian Osman <brianosman@google.com>
These aren't allowed in GLSL, and typically don't make sense.
Change-Id: I0afca0df638590466922a809e91ef0be35b13ca8
Bug: skia:10765
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/324816
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
GLSL does not support assigning to ternaries, and will fail to compile
and/or generate non-functional shaders if we pass in a shader that tries
to assign into a ternary expression.
If SkSL is able to completely eliminate the ternary (e.g. if it boils
down to a simple `true ? x : y` or `false ? x : y`), SkSL can strip out
the ternary entirely and generate valid GLSL. This case is harmless and
so it is still allowed.
Change-Id: I960f119fb9934f998697634e6c4e519cd77d3780
Bug: skia:10767
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/319679
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
This will allow the inliner to use IsAssignable.
Change-Id: Ic94f71002779b53d0b3dc97f37fbe4bb98b026d8
Bug: skia:10756
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/319414
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Many calls to `setRefKind` failed to check the return value; if it's
false, an error has occurred and the program is in a bad state.
Specifically, there is an assignment to a variable that's not marked as
"written-to." If we continue processing the program, we're likely to
assert.
Change-Id: I2dd5d1f41aa5ca0d30f8d638f05fe2e838216d78
Bug: skia:10753
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/319116
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
These cover:
- Properly configured out-params
- Invalid/non-lvalue out-params, which currently cause an SkSL crash
- Interactions between the inliner and variable swizzles
Change-Id: I4874101236084f273e704d8717149b431d813883
Bug: skia:10753
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/319036
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Change-Id: Ic8f4730d035981c32b4ddb48e5e919b0396b6d93
Bug: skia:10694
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/317578
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
We were allowing these ops on floating point types. The resulting GLSL
would fail to compile. We also allowed >>= and <<= on vectors (but not
any of the other bitwise ops).
The newly added unit tests were failing (eg, not catching errors). New
results are correct.
Bug: skia:10707
Change-Id: I97b769f1ce59261361109a71061b42dc8ef3c74b
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/317393
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Change-Id: I83a38f2c953a560fea3483e95e31df532b90773e
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/317456
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
GN lists both the .cpp and the .h as generated outputs, so if they don't
exist, Ninja assumes we need to rebuild the tests every time we compile.
Change-Id: I37b8b3d9e7aef1b0cb8d5c70530c2542a6c0087a
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/317108
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: I56e19153597e2c4393c5821314b82937828c0d50
Bug: skia:10694
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/316569
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
