Commit Graph

384 Commits

Author SHA1 Message Date
John Stiles
c3d8062555 Fix up SkSL test on Wembley.
Test "InlinerHonorsGLSLOutParamSemantics" was failing on Wembley devices
and is now disabled on that GPU.

Also, it turns out that the inliner has ignored functions with out
params for a long time now, but our test names haven't been updated to
account for this. So, did some additional cleanup:
- "InlinerHonorsGLSLOutParamSemantics" (the test in question) has been
  moved to shared/ and renamed to "OutParamsAreDistinct."
- Removed test "OutParamsNoInline" as it is functionally the same as
  "OutParams".

Change-Id: I1431ed197b9216cb482eee4f5e4eb2579a5303f7
Bug: skia:12858
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/502303
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Kevin Lubick <kjlubick@google.com>
Commit-Queue: Kevin Lubick <kjlubick@google.com>
2022-01-31 21:17:40 +00:00
John Stiles
343258fa0c Fix fuzzer-discovered error with sk_SecondaryFragColor in SPIR-V.
sk_SecondaryFragColor corresponds to an ES2-only concept
(gl_SecondaryFragColorEXT) and does not have any SPIR-V equivalent.

Two fixes were needed:
- sk_SecondaryFragColor shouldn't be in SPIR-V code at all. Report it as
  an error when it appears.
- We don't stop compilation when this error is reported, so we need to
  fix up the assertion that the fuzzer initially discovered.
  Specifically, the fuzzer found that the `sk_SecondaryFragColor`
  variable never got a SPIR-V ID assigned to it in fVariableMap, so the
  compiler would assert when assembling an expression containing that
  variable. Now, we make sure to populate fVariableMap with an (unused)
  ID in `writeGlobalVar` to avoid this crash.

Change-Id: Ib86919dfc9a325b2b82a7f4b2054b747dad7c32f
Bug: oss-fuzz:44096
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/501976
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
2022-01-31 16:21:45 +00:00
John Stiles
f4103618ff Report an error if an out param is never written to.
GLSL ES2 behavior is explicitly undefined if an out-param is never
written to: "If a function does not write to an out parameter, the value
of the actual parameter is undefined when the function returns."

We do see divergence here in practice: SkVM's behavior (the parameter is
left alone) differs from my GPU's behavior (the parameter is zeroed
out).

SkSL will now report an error if an out parameter is never assigned-to.
There is no control flow analysis performed, so we will not report
cases where the out parameter is assigned-to on some paths but not
others. (Technically the return-on-all-paths logic could be adapted
for this, but it would be a fair amount of work.)

Structs are currently exempt from the rule because custom mesh
specifications require an `out` parameter for a Varyings struct, even if
your mesh program doesn't need Varyings.

Bug: skia:12867
Change-Id: Ie828d3ce91c2c67e008ae304fdb163ffa88d744c
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/500440
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
2022-01-26 21:42:13 +00:00
John Stiles
493a9c0cbc Fix up test SkSLInlineWithInoutArgument.
The test has been moved to shared/, since it's a valid test, but it is
no longer related to inlining, as the inliner no longer attempts to
inline functions with inouts at all.

Also, one function here (outParameterIgnore) actually invoked undefined
behavior and has been removed. According to the GLSL ES2 docs: "If a
function does not write to an out parameter, the value of the actual
parameter is undefined when the function returns." SkVM leaves the value
unchanged, so SKSL_TEST_CPU would pass, but a GPU might clear it (and in
fact, my GPU does).

Change-Id: I77c77ed1354bc980344ec5c406992bd62015f5e5
Bug: skia:11919
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/499752
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
2022-01-25 21:33:45 +00:00
John Stiles
3b7fd14ea8 Move matrix-scalar splat tests into MatrixFolding.
Previously, matrix-scalar operations did not actually fold, so the tests
didn't live in folding/. In a followup CL, these will fold.

Bug: skia:12819
Change-Id: I6fdacf89088920719e7666d6c9b05ddffaf6cb6d
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/497742
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
2022-01-25 17:32:38 +00:00
John Stiles
cc473cd92f Fix fuzzer-discovered error with swizzles.
Some paths through swizzle optimization would replace a swizzles with a
constructor--e.g. `float3(1, 2, 3).y` would be replaced with `float(2)`.
(Constructor::Convert was responsible for replacing this trivial
constructor with the literal `2.0`.)

The optimization code asserted that this replacement would succeed, but
the fuzzer managed to construct a counterexample where the constructor
rejected the value. Specifically, by nesting casts between int3 and
float3, it found a case where Constructor::Convert returned null because
the literal value was out of range for `int` types.

This assertion didn't really add value so removing it was harmless.
Constructor::Convert already reports an error when it fails, and null
returns are handled properly throughout.

Change-Id: I575d441ed90d6b696f6399941c3f6d84698794bc
Bug: oss-fuzz:44045
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/499382
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
2022-01-25 16:25:09 +00:00
John Stiles
b8f05d6f63 Revert "[skslc] Generate .hlsl test output files"
This reverts commit a97a6769b5.

Reason for revert: breaking bot Housekeeper-PerCommit-CheckGeneratedFiles -- see http://screen/5FN75fF9tvcQFKR

Based on the diff, I think this just needs to be synced up to latest code and a rebuild should fix it.


Original change's description:
> [skslc] Generate .hlsl test output files
>
> - The build now generates HLSL output when `skia_compile_sksl_tests` is
> enabled.
> - The "blend" and "shared" tests have been enabled for HLSL with the
> exception of 6 tests that exercise intrinsic inverse hyperbolic
> functions, which don't have HLSL equivalents.
>
> Bug: skia:12691, skia:12352
> Change-Id: Ia970f878f75ff58e8e3d47249c2dc2f756c165b4
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/482778
> Reviewed-by: John Stiles <johnstiles@google.com>
> Reviewed-by: Brian Osman <brianosman@google.com>
> Auto-Submit: Arman Uguray <armansito@google.com>
> Commit-Queue: Arman Uguray <armansito@google.com>
> Commit-Queue: Brian Osman <brianosman@google.com>

Bug: skia:12691, skia:12352
Change-Id: Iaad607d48edd136eee2b60e48c0643b6e90179e9
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/499216
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>
2022-01-25 04:57:15 +00:00
Arman Uguray
a97a6769b5 [skslc] Generate .hlsl test output files
- The build now generates HLSL output when `skia_compile_sksl_tests` is
enabled.
- The "blend" and "shared" tests have been enabled for HLSL with the
exception of 6 tests that exercise intrinsic inverse hyperbolic
functions, which don't have HLSL equivalents.

Bug: skia:12691, skia:12352
Change-Id: Ia970f878f75ff58e8e3d47249c2dc2f756c165b4
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/482778
Reviewed-by: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Auto-Submit: Arman Uguray <armansito@google.com>
Commit-Queue: Arman Uguray <armansito@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
2022-01-24 21:50:30 +00:00
John Stiles
9f681e6df8 Reject $ in variable names for non-builtin code.
These identifiers are reserved for SkSL internal use (and can't be
exposed to GLSL or Metal anyway).

Change-Id: Id554cbf21ed2fb66785e77700ff79424ecdf66db
Bug: skia:12854
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/498036
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
2022-01-24 14:17:36 +00:00
Julia Lavrova
55c215cfb6 Reland "Better Matrix/Scalar testing"
This reverts commit 455e580b9c.

Reason for revert: Fixing the build break

Original change's description:
> Revert "Better Matrix/Scalar testing"
>
> This reverts commit abb611550e.
>
> Reason for revert: Build break
> Original change's description:
> > Better Matrix/Scalar testing
> >
> > Adding tests for matrix math and comparison
> > bug: skia:12681
> >
> > Change-Id: Ia1537ee2e411383749456fd6ff938b7c9a2e1061
> > Reviewed-on: https://skia-review.googlesource.com/c/skia/+/493416
> > Reviewed-by: John Stiles <johnstiles@google.com>
> > Commit-Queue: Julia Lavrova <jlavrova@google.com>
>
> Change-Id: I70871b4b75c1f10e870dc5e884a42405a80fc0f9
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/494816
> Reviewed-by: John Stiles <johnstiles@google.com>
> Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
> Commit-Queue: Julia Lavrova <jlavrova@google.com>

Change-Id: Idef8dbcd6f5a5bbe84d3fd86888e3eab0f0521ee
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/494817
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: Julia Lavrova <jlavrova@google.com>
2022-01-14 19:12:00 +00:00
Julia Lavrova
455e580b9c Revert "Better Matrix/Scalar testing"
This reverts commit abb611550e.

Reason for revert: Build break
Original change's description:
> Better Matrix/Scalar testing
>
> Adding tests for matrix math and comparison
> bug: skia:12681
>
> Change-Id: Ia1537ee2e411383749456fd6ff938b7c9a2e1061
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/493416
> Reviewed-by: John Stiles <johnstiles@google.com>
> Commit-Queue: Julia Lavrova <jlavrova@google.com>

Change-Id: I70871b4b75c1f10e870dc5e884a42405a80fc0f9
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/494816
Reviewed-by: John Stiles <johnstiles@google.com>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Commit-Queue: Julia Lavrova <jlavrova@google.com>
2022-01-13 18:52:04 +00:00
Julia Lavrova
abb611550e Better Matrix/Scalar testing
Adding tests for matrix math and comparison
bug: skia:12681

Change-Id: Ia1537ee2e411383749456fd6ff938b7c9a2e1061
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/493416
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: Julia Lavrova <jlavrova@google.com>
2022-01-13 15:35:12 +00:00
John Stiles
8522bf910a Add test for switch-case folding.
Change-Id: I3dabd77890a73ea054bb57d466a6ed8273eae3e8
Bug: skia:12811
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/494196
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
2022-01-12 22:24:55 +00:00
Julia Lavrova
ae5f86ea1a Test: Interface blocks allow duplicate fields
bug: skia:12793
Change-Id: Ie47b1df381779ed24cab8b05b1ee6947bda82516
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/492396
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: Julia Lavrova <jlavrova@google.com>
2022-01-07 15:55:58 +00:00
Julia Lavrova
ce49ff6520 Test: duplicate fields in the same struct allowed
As @johnstiles suggested I add the test first and the fix after.
bug: skia:12712

Change-Id: I9316cf40f71e756fc1730ee630bc0d0377f200d6
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/491936
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: Julia Lavrova <jlavrova@google.com>
2022-01-06 19:40:50 +00:00
Ethan Nicholas
f393067c37 Made SkSL type aliases into first-class objects
Previously, type aliases ('vec2') were just an additional name which
could be used to refer to a type ('float2'). This was simple and worked,
except that error messages would be wrong - any type-related error
message would refer to the type as 'float2' rather than the 'vec2' that
the user actually typed.

This CL adds an AliasType class so that we can track which name was
used to refer to an aliased type and report messages using the correct
type name.

Bug: skia:12737

Change-Id: I40e234239ab47557033e0695e4fbbd5f01da354e
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/490256
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
2022-01-05 16:20:19 +00:00
Brian Osman
735ff421bb Reject #extension in runtime-effect mode
Bug: oss-fuzz:43062
Change-Id: I10d8fa40c81c5b1595d30221d89c84f5cc3478fd
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/490857
Commit-Queue: Brian Osman <brianosman@google.com>
Auto-Submit: Brian Osman <brianosman@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
2022-01-04 16:12:50 +00:00
John Stiles
aa09c782a8 Add test for structs/interface-blocks with invalid member names.
Structs already handled this appropriately, but interface blocks did not
guard against naming their member variables built-in type names like
"float" or "bool".

Change-Id: I12ec054b3f158b83e35031449cf2a088ff8d0dc2
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/489596
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
2021-12-28 20:55:32 +00:00
John Stiles
1c7d442b52 Add test containing anonymous function parameters.
SkSL will reject ES2-compatible code because function parameters always
require a name in SkSL. (A followup CL relaxes this restriction and
allows anonymous parameters in SkSL.)

Change-Id: Ifdcf0fcbe0f52d16007c018b545631ca4033a8c4
Bug: skia:12769
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/489537
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
2021-12-28 20:08:56 +00:00
John Stiles
48432e133e Add test demonstrating struct/interface-block name conflict.
Structs and interface blocks allow a trailing identifier which is added
to the symbol table. This identifier should be prohibited from
overlapping built-in types; at present, this is not checked. Add a test
demonstrating the issue.

Change-Id: I99aa915c1715c468cc369c97b7f12e031b86ea4a
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/489496
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
2021-12-28 19:26:17 +00:00
Julia Lavrova
94f726ae62 Adding test files demonstrating type confusion for arrays/structs.
Change-Id: Iacd924471971c3c551648f75c1ecf0f1bfd3ed07
Bug: skia:12642
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/479061
Commit-Queue: Julia Lavrova <jlavrova@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
2021-12-03 20:54:36 +00:00
Brian Osman
00aa1a9259 Prevent 'binding' and 'set' on struct/interface block fields
I should have realized the fuzzer would find this assert when I added
it. Now the front-end rejects these layout qualifiers on both struct
fields and interface block fields. LayoutInInterfaceBlock.sksl is a
reformatted version of the fuzzer input. LayoutInStruct is hand-crafted
to trigger the same failure on a different code path. Both would
previously assert in the SPIRV generator. Now, neither one gets that
far.

Bug: oss-fuzz:41347
Change-Id: Iff69d8f5482da7b772e9331c4fd2d58e89813c46
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/476396
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
2021-11-29 15:53:57 +00:00
John Stiles
626dbe195a Add test for commutative operations in SkVM.
SkVM should be able to optimize away these equivalent expressions:

	(intA & intB)   <->  (intB & intA)
	(intA ^ intB)   <->  (intB ^ intA)
	(intA | intB)   <->  (intB | intA)
	(intA + intB)   <->  (intB + intA)
	(intA * intB)   <->  (intB * intA)
	(intA == intB)  <->  (intB == intA)
	(intA != intB)  <->  (intB != intA)

These should be guaranteed by IEEE754 as well:

	(floatA + floatB)   <->  (floatB + floatA)
	(floatA * floatB)   <->  (floatB * floatA)

I've added a test to demonstrate existing behavior, which leaves these
optimizations on the table.

Change-Id: I01ce1d6f1cfadb3d77db405a83752c9dd52c99bd
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/473238
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
2021-11-18 21:16:48 +00:00
John Stiles
bcd6d4901d Add test for construction of void.
This fails exactly as it should, but we had no test for it.

Change-Id: I0aa3307c444f2c9bc3512ff43b784a56a7c09856
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/472449
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
2021-11-17 16:04:35 +00:00
John Stiles
504d57e9bd Add test for void type in struct.
Mysteriously, I had written a test which put arrays of void inside a
struct, but had neglected to include the non-array case. It causes an
okay-not-great error (referring to void as an "opaque type").

Change-Id: Id20a9d3512d29aecea81d46877dce708b7b2f973
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/472450
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
2021-11-17 15:21:41 +00:00
John Stiles
fe3cfc1d4f Add test for variables of type void.
We should, of course, detect this and report an error.

Change-Id: I42b3be6e714a1f367d3251842506a384f2afe019
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/472447
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
2021-11-17 14:51:52 +00:00
John Stiles
e10839f998 Add basic unit test for octal parsing support.
This is required by the ES2 standard: http://screen/Qysv4fPW5r5LA9e
This actually already worked fine because `strtoull` natively recognizes
octal values without any work on our part. However, we lacked a test.

Change-Id: I3033de899918abe99c63a9b7b79bd4c3374ee315
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/471716
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
2021-11-16 13:49:41 +00:00
John Stiles
14a487fd54 Replace getConstantSubexpression with getConstantValue.
The only type of expressions that getConstantSubexpression could ever
return are Literal and nullptr. getConstantValue now returns an
optional<double>; nullopt indicates a non-constant value in the slot.
This simplifies most use cases, and allows us to get rid of some extra
"zero" and "one" Literal objects in some of our Constructor classes.

This change fixes a recent fuzzer issue. The fuzzer had discovered that
calling `getConstantSubexpression` on a ConstructorCompoundCast that
contained a compile-time-constant value would return literals of the
wrong type (the cast was not applied). By nesting repeated matrix casts,
this type confusion could be turned into an assertion.

Change-Id: Icee69219e6db2822ffdfab4e5ccdaff54584a4b6
Bug: oss-fuzz:41000
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/471376
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
2021-11-15 14:46:21 +00:00
John Stiles
107ea94139 Add support for clamp($genUType, ...) in SkSL.
We never used it internally, but the shaders used by Filament rely on
it. It doesn't exist in ES2 so this doesn't affect Runtime Effects.

Change-Id: Idb2afb15ff160b950ad02101bf6381a5d5c56468
Bug: skia:12635, skia:11209
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/470156
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
2021-11-11 18:43:42 +00:00
John Stiles
d92f9c2f8d Add tests for matrix constant-expressions.
We should support constant-expressions involving matrices (GLSL ES2
does, WebGL does). We currently don't. We do properly report out-of-
range indexing, but we don't optimize away valid matrix index
expressions or allow matrices to be indexed in a constant-expression
context.

Change-Id: If58aa4c5f15abef421a412957072f3617b4176df
Bug: skia:12472
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/469818
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
2021-11-10 20:59:36 +00:00
John Stiles
0b84159e3b Improve array-indexing tests.
Previously, we didn't have tests which leveraged constant-evaluation of
array indexing (because we didn't support it), and our test files
commingled constant-indexing into vectors with constant-indexing into
arrays.

The test files now separate vector- and array-handling into separate
tests, and a ton of new cases have been added to ArrayFolding. The
ArrayFolding tests now require constant-evaluation of array indexing,
so they fail in this CL, but will be fixed in the followup CL.

Change-Id: I3b663e743d97d6db80627bc9b7808f88c99917a7
Bug: skia:12472
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/469528
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
2021-11-10 18:31:28 +00:00
John Stiles
6e6ae1b762 Fix inlined out-of-range vector access.
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>
2021-11-10 14:44:38 +00:00
John Stiles
ee525493ea Add test for incomplete expressions.
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>
2021-11-09 22:09:49 +00:00
John Stiles
e7a9535247 Enforce basic limits on global size in SkSL.
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>
2021-11-04 18:34:19 +00:00
John Stiles
7cde28909f Enforce basic limits on function stack size in SkSL.
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>
2021-11-04 18:17:44 +00:00
John Stiles
390edeb88d Fix fuzzer-discovered error with no-op arithmetic.
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>
2021-11-03 14:57:48 +00:00
John Stiles
aa369d2b8e Fix error with inlined literals overflowing their types.
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>
2021-11-02 14:26:44 +00:00
John Stiles
ba9d5362e4 Detect and properly handle swizzles of out-of-range literal values.
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>
2021-10-29 19:39:33 +00:00
John Stiles
4363cdb5ea Fix for fuzzer-discovered error with bitwise-not.
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>
2021-10-28 16:44:09 +00:00
John Stiles
f2d016f12e Fix for fuzzer-discovered error with negation.
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>
2021-10-28 16:14:42 +00:00
Brian Osman
2d1207acb9 Remove the "in blend modes randomly fail for all zero vec" workaround
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>
2021-10-21 16:54:51 +00:00
John Stiles
319d75d6f6 Add error test for unary-negating and unary-plussing arrays.
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>
2021-10-18 14:30:36 +00:00
John Stiles
c8a96076b1 Added four more reserved words to SkSL.
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>
2021-10-14 16:29:32 +00:00
John Stiles
f7d2673643 Fix up samplerCube/textureCube for ES2 conformance.
`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>
2021-10-14 15:21:31 +00:00
John Stiles
21fe518fbb Revert "Disallow matrix ctors which overflow a column."
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>
2021-10-14 01:30:08 +00:00
John Stiles
64c907c052 Mark private types as invalid in the public symbol table.
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>
2021-10-13 20:41:09 +00:00
John Stiles
980169a5b5 Allow non-trivial constant expressions for array sizes.
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>
2021-10-12 21:04:33 +00:00
John Stiles
906e9eb538 Emit qualifiers in the GLSL ES-required order.
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>
2021-10-01 19:09:43 +00:00
John Stiles
7e947ab4b6 Reland "Mark GLSL reserved names as reserved in SkSL grammar."
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>
2021-10-01 18:31:50 +00:00
Ethan Nicholas
5f15c695f9 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>
2021-10-01 14:53:12 +00:00