spirv-fuzz: add FuzzerPassAddCompositeInserts by antonikarp · Pull Request #3606 · KhronosGroup/SPIRV-Tools

antonikarp · 2020-07-28T14:10:26Z

Adds FuzzerPassAddCompositeInserts, which randomly adds new
OpCompositeInsert instructions. Each OpCompositeInsert instruction
yields a copy of an original composite with one subcomponent replaced
with an existing or newly added object. Synonym facts are added for the
unchanged components in the original and added composite, and for the
replaced subcomponent and the object, if possible.

Fixes #2859

paulthomson

This looks like a good first attempt! The idea is correct. Please see comments.

source/fuzz/fuzzer_pass_add_composite_inserts.cpp

test/fuzz/fuzzer_pass_add_composite_inserts_test.cpp

Vasniktel

PR is looking very nice! I left some comments on various tricky corner cases of the implementation.

source/fuzz/fuzzer_pass_add_composite_inserts.cpp

source/fuzz/transformation_composite_insert.h

source/fuzz/fuzzer_pass_add_composite_inserts.cpp

test/fuzz/transformation_composite_insert_test.cpp

source/fuzz/fuzzer_pass_add_composite_inserts.cpp

paulthomson · 2020-08-06T15:39:18Z

source/fuzz/transformation_composite_insert.cpp

+  auto object_instruction =
+      ir_context->get_def_use_mgr()->GetDef(message_.object_id());
+  if (object_instruction == nullptr) {
+    return false;
+  }
+
+  // We ignore pointers for now. Consider adding support for pointer types.
+  auto object_instruction_type =
+      ir_context->get_type_mgr()->GetType(object_instruction->type_id());
+  // No unused variables in release mode (to keep compilers happy).
+  (void)object_instruction_type;
+  assert(object_instruction_type->kind() != opt::analysis::Type::kPointer &&
+         "The object to be inserted cannot be a pointer.");


I believe we currently like to imagine that transformations could be applied to ANY module, and they should never crash. This means the IsApplicable method should not crash, but may return false (and so the Apply method will never be called). Theoretically, if Apply was somehow called even though IsApplicable would return false, we are allowed to crash (assert false).

Thus, the instruction found from message_.object_id might end up not having a type id. So you should do if (!object_instruction->type_id()) return false;.

Similarly, I think the assert should not be an assert, and we should instead just return false if object_instruction_type happens to be a pointer type. Also, I think we prefer to use ->AsPointer() (or ->AsInteger(), ->AsFloat(), etc.) to check the type; we will get nullptr if the type is not a pointer (integer, float, etc.).

Actually, I guess you technically don't need to check that object_instruction->type_id() is non-zero because it is compared with component_to_be_replaced_type_id, which is non-zero. But in general, it does not hurt to check this.

Indeed. I agree. It was me who requested that we add an assertion here (sorry, @antnkarp).

But now that I think about it, it might indeed be better to use an if statement here. I guess the rule of thumb here is:

if some condition is valid but not supported (like the pointer case above) - use ifs; otherwise (if the condition is not possible in the current context) - use asserts.

The last part might seem like a waste of time but it helps the reader to understand writer's assumptions about the code.

Vasniktel

Looks very good! I've requested some minor changes to the comments as well as noticed some unhandled corner cases.

source/fuzz/fuzzer_pass_add_composite_inserts.cpp

source/fuzz/transformation_composite_insert.cpp

Vasniktel · 2020-08-06T17:06:19Z

source/fuzz/transformation_composite_insert.cpp

+  auto object_instruction =
+      ir_context->get_def_use_mgr()->GetDef(message_.object_id());
+  if (object_instruction == nullptr) {
+    return false;
+  }
+
+  // We ignore pointers for now. Consider adding support for pointer types.
+  auto object_instruction_type =
+      ir_context->get_type_mgr()->GetType(object_instruction->type_id());
+  // No unused variables in release mode (to keep compilers happy).
+  (void)object_instruction_type;
+  assert(object_instruction_type->kind() != opt::analysis::Type::kPointer &&
+         "The object to be inserted cannot be a pointer.");


Indeed. I agree. It was me who requested that we add an assertion here (sorry, @antnkarp).

But now that I think about it, it might indeed be better to use an if statement here. I guess the rule of thumb here is:

if some condition is valid but not supported (like the pointer case above) - use ifs; otherwise (if the condition is not possible in the current context) - use asserts.

The last part might seem like a waste of time but it helps the reader to understand writer's assumptions about the code.

source/fuzz/transformation_composite_insert.cpp

Vasniktel

I haven't done a complete review iteration but I've noticed some issues.

source/fuzz/fuzzer_context.h

source/fuzz/fuzzer_pass_add_composite_inserts.cpp

Vasniktel

The changes you made look good! I've found a potential bug, though. I also haven't checked the tests thoroughly yet.

source/fuzz/fuzzer_pass_add_composite_inserts.cpp

source/fuzz/transformation_composite_insert.cpp

test/fuzz/transformation_composite_insert_test.cpp

Vasniktel · 2020-08-07T11:54:57Z

I've just noticed that you haven't added your fuzzer pass to fuzzer.cpp file. Without it, your transformation will never be applied.

This mistake is easy to make so be sure that your other merged transformations don't have it.

Vasniktel

Looks very good! I've added some minor suggestions. Thanks for your hard work on this one 👍

source/fuzz/fuzzer_context.h

source/fuzz/fuzzer_pass_replace_adds_subs_muls_with_carrying_extended.cpp

source/fuzz/fuzzer_pass_replace_copy_objects_with_stores_loads.cpp

source/fuzz/fuzzer_pass_add_composite_inserts.cpp

test/fuzz/transformation_composite_insert_test.cpp

Vasniktel

LGTM. Thanks for your hard work again!

paulthomson

This looks really good! And thanks for the excellent tests! I am sorry to ask you to refactor them at this stage; perhaps I could have spotted this sooner. But I think this is something Ally would also insist on changing.

source/fuzz/fuzzer_pass_add_composite_inserts.cpp

source/fuzz/transformation_composite_insert.cpp

test/fuzz/transformation_composite_insert_test.cpp

paulthomson

Excellent! This is really close. Just one main style violation. And I had previously missed the unrelated changes; I think you should revert those before we merge.

Also, to clarify one of my previous comments; the word "valid" (and invalid) is OK in many cases. I just want us to avoid saying things like "message.id() must be valid"; this comment is incorrect (in my opinion). Because "message.id()" is just a number. It cannot be valid or invalid. What we actually mean is: the id |message.id()| must be found in current module, must exist in the current module, must be defined in the current module, or similar. That is the only case I was concerned with, and I know that there are existing comments in other transformations that say this (and we should change them at some point). Apologies for not being clearer. But the other cases you removed look mostly fine. I have highlighted one case where I think it is a good idea to say that our changes to the module have "invalidated most analyzes".

source/fuzz/fuzzer_pass_add_composite_inserts.cpp

source/fuzz/fuzzer_pass_replace_adds_subs_muls_with_carrying_extended.cpp

source/fuzz/fuzzer_pass_replace_copy_objects_with_stores_loads.cpp

source/fuzz/transformation_composite_insert.cpp

paulthomson

Nice! Just a few final suggestions.

source/fuzz/fuzzer_pass_add_composite_inserts.cpp

source/fuzz/fuzzer_pass_replace_adds_subs_muls_with_carrying_extended.cpp

source/fuzz/fuzzer_pass_replace_copy_objects_with_stores_loads.cpp

Adds FuzzerPassAddCompositeInserts, which randomly adds new OpCompositeInsert instructions. Each OpCompositeInsert instruction yields a copy of an original composite with one subcomponent replaced with an existing or newly added object. Synonym facts are added for the unchanged components in the original and added composite, and for the replaced subcomponent and the object, if possible. Fixes KhronosGroup#2859

Roll third_party/glslang/ f257e0e..983698b (2 commits) KhronosGroup/glslang@f257e0e...983698b $ git log f257e0e..983698b --date=short --no-merges --format='%ad %ae %s' 2020-08-23 john Revert "Merge pull request KhronosGroup#2371 from RafaelMarinheiro/master" 2020-08-21 julius.ikkala Obey ENABLE_PCH CMake option Created with: roll-dep third_party/glslang Roll third_party/googletest/ adeef1929..1e315c5b1 (14 commits) google/googletest@adeef19...1e315c5 $ git log adeef1929..1e315c5b1 --date=short --no-merges --format='%ad %ae %s' 2020-08-20 absl-team Googletest export 2020-08-17 absl-team Googletest export 2020-08-12 robert.earhart Export LICENSE 2020-08-03 amatanhead Remove ThrowsMessageHasSubstr and fix some nits after review 2020-07-13 amatanhead Cleanup a bulky expression, document implementation details 2020-07-07 amatanhead Fix build under msvc 2020-07-07 amatanhead Update tests after changing an error message 2020-07-07 amatanhead Fix build under msvc 2020-07-07 amatanhead Add a test to ensure that the `Throws` matcher only invokes its argument once. 2020-07-07 amatanhead Add a test for duplicate catch clauses in throw matchers, fix a couple of nitpicks. 2020-07-03 amatanhead Add missing documentation piece 2020-06-20 amatanhead Small improvements: code style and property name 2020-06-20 amatanhead Add matchers for testing exception properties 2018-05-01 lantw44 Avoid using environ on FreeBSD Created with: roll-dep third_party/googletest Roll third_party/spirv-cross/ 4c7944bb4..685f86471 (6 commits) KhronosGroup/SPIRV-Cross@4c7944b...685f864 $ git log 4c7944bb4..685f86471 --date=short --no-merges --format='%ad %ae %s' 2020-08-24 post Run format_all.sh. 2020-08-24 post Work around annoying warning on GCC 10.2. 2020-08-21 post Overhaul how we deal with reserved identifiers. 2020-08-20 post HLSL: Fix FragCoord.w. 2020-08-20 post HLSL: Deal with partially filled 16-byte word in cbuffers. 2020-08-20 post HLSL: Fix bug in is_packing_standard for cbuffer. Created with: roll-dep third_party/spirv-cross Roll third_party/spirv-tools/ b8de4f5..4dd1223 (9 commits) KhronosGroup/SPIRV-Tools@b8de4f5...4dd1223 $ git log b8de4f5..4dd1223 --date=short --no-merges --format='%ad %ae %s' 2020-08-21 andreperezmaselco.developer spirv-fuzz: Add words instead of logical operands (KhronosGroup#3728) 2020-08-20 dnovillo CCP should mark IR changed if it created new constants. (KhronosGroup#3732) 2020-08-19 antonikarp spirv-fuzz: add FuzzerPassAddCompositeInserts (KhronosGroup#3606) 2020-08-19 antonikarp spirv-fuzz: Support pointer types in FuzzerPassAddParameters (KhronosGroup#3627) 2020-08-18 jaebaek Let ADCE pass check DebugScope (KhronosGroup#3703) 2020-08-18 andreperezmaselco.developer spirv-opt: Implement opt::Function::HasEarlyReturn function (KhronosGroup#3711) 2020-08-17 andreperezmaselco.developer spirv-fuzz: Check termination instructions when donating modules (KhronosGroup#3710) 2020-08-17 jackoalan Fix -Wrange-loop-analysis warning (KhronosGroup#3712) 2020-08-17 andreperezmaselco.developer spirv-fuzz: Check header dominance when adding dead block (KhronosGroup#3694) Created with: roll-dep third_party/spirv-tools

paulthomson reviewed Jul 28, 2020

View reviewed changes

antonikarp force-pushed the 2859_add_op_composite_insert branch from 0fbb4f0 to 90445bd Compare July 31, 2020 05:57

paulthomson added the kokoro:run label Aug 3, 2020

kokoro-team removed the kokoro:run label Aug 3, 2020

antonikarp force-pushed the 2859_add_op_composite_insert branch from 90445bd to d8116e8 Compare August 4, 2020 10:21

paulthomson added the kokoro:run label Aug 4, 2020

kokoro-team removed the kokoro:run label Aug 4, 2020

Vasniktel suggested changes Aug 5, 2020

View reviewed changes

paulthomson reviewed Aug 5, 2020

View reviewed changes

source/fuzz/fuzzer_pass_add_composite_inserts.cpp Outdated Show resolved Hide resolved

paulthomson reviewed Aug 5, 2020

View reviewed changes

source/fuzz/fuzzer_pass_add_composite_inserts.cpp Outdated Show resolved Hide resolved

antonikarp force-pushed the 2859_add_op_composite_insert branch from 6642346 to 5269fb3 Compare August 6, 2020 15:21

paulthomson reviewed Aug 6, 2020

View reviewed changes

Vasniktel suggested changes Aug 6, 2020

View reviewed changes

Vasniktel reviewed Aug 6, 2020

View reviewed changes

source/fuzz/transformation_composite_insert.cpp Outdated Show resolved Hide resolved

antonikarp force-pushed the 2859_add_op_composite_insert branch from 5269fb3 to 255de71 Compare August 7, 2020 09:56

Vasniktel suggested changes Aug 7, 2020

View reviewed changes

source/fuzz/fuzzer_context.h Outdated Show resolved Hide resolved

source/fuzz/fuzzer_pass_add_composite_inserts.cpp Outdated Show resolved Hide resolved

Vasniktel suggested changes Aug 7, 2020

View reviewed changes

Vasniktel suggested changes Aug 10, 2020

View reviewed changes

Vasniktel approved these changes Aug 10, 2020

View reviewed changes

antonikarp force-pushed the 2859_add_op_composite_insert branch from ed0fc93 to 59b43ef Compare August 11, 2020 09:28

paulthomson reviewed Aug 11, 2020

View reviewed changes

paulthomson reviewed Aug 12, 2020

View reviewed changes

paulthomson reviewed Aug 17, 2020

View reviewed changes

Antoni Karpiński added 6 commits August 19, 2020 10:35

Initial structure.

caf7292

Implemented fuzzer pass.

a1df498

Minor changes.

f4c2ec9

Added corresponding transformation.

193f66c

Changed name to TransformationCompositeInsert.

01f8c49

Added insertion in nested composites.

fadaf20

Antoni Karpiński added 21 commits August 19, 2020 10:35

Added synonym facts.

889b4f0

Added tests.

eaa539a

Fixes comments.

ccc69f4

Fix unused parameter.

fa5a869

Changes according to comments (excluding tests).

17dfeaf

Added tests.

5aa0c68

Fixes return after assert.

eafea17

Fix unused variable.

9962dfb

Fixes empty line

27c3bbb

Fixed assert_death in release mode.

eff66ee

Fixed unsupported character in debug message.

a1e1a21

Fixes according to the comments.

1ff0f3a

Fix check for empty component.

83a38ec

Fixes number of elements bug. Adds my fuzzer passes.

fd97dab

Fix range for loop.

82c1eba

Fixes according to comments.

eee6369

Changes in tests.

9554f15

Fixes according to the comments. Extended code coverage.

b6073fb

Changes according to comments. Copy vector of instructions in the pass.

33a8b45

Fixes according to comments.

39d1725

Moved CanFindOrCreateZeroConstant.

f1ef91d

antonikarp force-pushed the 2859_add_op_composite_insert branch from 1fa0c31 to f1ef91d Compare August 19, 2020 11:24

Fix resolution scope.

f193253

paulthomson changed the title ~~spirv-fuzz: adds FuzzerPassAddCompositeInserts~~ spirv-fuzz: add FuzzerPassAddCompositeInserts Aug 19, 2020

paulthomson merged commit a711c59 into KhronosGroup:master Aug 19, 2020

Conversation

antonikarp commented Jul 28, 2020 • edited by paulthomson Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

paulthomson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Vasniktel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

paulthomson Aug 6, 2020

Choose a reason for hiding this comment

Uh oh!

paulthomson Aug 6, 2020

Choose a reason for hiding this comment

Uh oh!

Vasniktel Aug 6, 2020

Choose a reason for hiding this comment

Uh oh!

Vasniktel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Vasniktel Aug 6, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Vasniktel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Vasniktel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Vasniktel commented Aug 7, 2020

Uh oh!

Vasniktel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Vasniktel left a comment

antonikarp commented Jul 28, 2020 •

edited by paulthomson

Loading