[WIP] Arm Best Practices Refactor

[samwalls-arm]

Overview

Here's the beginnings of some features we've discussed. This is still work-in-progress, but I welcome feedback and criticism: I may be missing some assumptions in the codebase, and there's probably still a few things to discuss in regards to these additions.

The main aim of these changes are to make the best practices system easier to maintain, and extend. This is mainly achieved through making it easier to logically separate individual checks from each other. However, there are additionally changes made which allow vendors to agree with specific checks. This has the advantage of handling cases where: checks provided from vendor A may be beneficial to endorse for vendor B, too, but perhaps not vendor C.

Here's some diagrams explaining the desired "before and after".

Before

After

Still TODO

At the moment, I've moved all of the checks that existed into a class class MyExampleBestPractices : public BestPracticeBase. However, this is only to move the refactored framework to a state where all the best-practice tests pass, to demonstrate its operation. In the final iteration, we would want to split this class up.

• Code generation for best practice harness code.
• It looks like the APICallHookInterface interface I abstracted out will not be sufficient. The interface for best practices should include all functions with state_data parameters. This may require another code generator script?
• Re-enable ManualPostCallRecordX functions
• Unit tests for any framework changes. (almost complete)
• The question still remains how we want to use the system, but it now allows for automatic IDs based on the check's specified template parameter. How best to split current checks + assign IDs? Should there be such a thing as a "class of checks" which can be agreed with, or should checks always be individually referenced?
• Undertake the work to split the checks up into separate spaces.

[samwalls-arm]

Hey @mark-lunarg, I'm wondering about the best way to approach using the code-generating scripts in order to implement the functions of the BestPractices class, as seen in the "After" diagram.

I want all functions, PreCallValidateX, PreCallRecordX, and PostCallRecordX, to have the following code:

foreachPractice([=](APICallHookInterface& practice) { practice.FN_NAME(...); });

where FN_NAME corresponds to the API hook's function name, and all the parameter names are passed down where we see ....

It seems like the generator scripts for best_practices.h/best_practices.cpp currently only generate functions for PostCallRecordX. Do you have any tips on how to enable generation of all functions?

[samwalls-arm]

Update: I managed to get everything to the point where there's a new generator script which outputs the foreachPractice calls, as the previous comment talks about.

After a lot of trial and error, everything builds. However, it looks like the changes mean that none of the validation layers are executed... I'm not sure why at the moment. It looks like, for example, when a test involving vkCreateInstance is run, CreateInstance in chassis.cpp is never reached.

I'll continue looking into that.

[samwalls-arm]

Hey, @mark-lunarg, I think I managed to isolate the issues with windows builds. Let me know if that works.

[samwalls-arm]

All the best practices tests are passing now on windows + linux!

Next steps on this would be to further refine what we want out of the vendor agreement system. Then, we can write tests against that definition.

[samwalls-arm]

At commit 74881fa everything passed except for a single test:

[----------] 1 test from VkDebugPrintfTest
[ RUN      ] VkDebugPrintfTest.GpuDebugPrintf
unknown file: Failure
C++ exception with description "std::bad_alloc" thrown in the test body.
[  FAILED  ] VkDebugPrintfTest.GpuDebugPrintf (18 ms)
[----------] 1 test from VkDebugPrintfTest (18 ms total)

I'm not sure what caused this, or what part of my changes could be causing this, if any.

EDIT: I tried out this test on the exact commit locally. It passes on my linux setup.

[samwalls-arm]

I've added work-in-progress unit tests for behaviour in the check system itself, just so I can illustrate something I'm struggling with.

For some reason, I can't link code defined in best_practices_utils.cpp into vklayertests_best_practices.cpp - cmake/make says:

> make
[  8%] Built target VkLayer_utils
Scanning dependencies of target VkLayer_khronos_validation
[ 10%] Building CXX object layers/CMakeFiles/VkLayer_khronos_validation.dir/best_practices_utils.cpp.o
[ 12%] Linking CXX shared library libVkLayer_khronos_validation.so
[ 53%] Built target VkLayer_khronos_validation
[ 56%] Built target gtest
[ 60%] Built target gtest_main
[ 60%] Built target VkLayer_khronos_validation-json
[ 62%] Linking CXX executable vk_layer_validation_tests
/usr/bin/ld: CMakeFiles/vk_layer_validation_tests.dir/vklayertests_best_practices.cpp.o: in function `BestPracticesChassisTest_NoChecks_Test::TestBody()':
/home/sam/w/arm/Vulkan-ValidationLayers/tests/vklayertests_best_practices.cpp:51: undefined reference to `BestPracticesTracker::initReverseLookup()'

@mark-lunarg Would it be easy/feasible to include symbols from VkLayer_khronos_validation in the test executable? I tried playing around with the CMakeLists.txt for the tests target, but I don't have a good idea of what's going on. This is resolved.

[samwalls-arm]

Would it be easy/feasible to include symbols from VkLayer_khronos_validation in the test executable? I tried playing around with the CMakeLists.txt for the tests target, but I don't have a good idea of what's going on.

I've fixed those build issues now (the fix involved repeatedly re-compiling, and adding cpp files containing the missing definitions to the vk_layer_validation_tests target in cmake). Still looking into the behaviour of the failing tests. What I'm trying to test might not be possible at the moment.

Apart from this, see the unit tests in the BestPracticesChassisTest fixture - they show a basic example of a self-contained "check" class, and how to register it with the best-practices system.

[mark-lunarg]

@samwalls-arm, sorry, just saw your two last comments. We'd like to avoid adding any of the chassis framework stuff to the tests. There are a few layer files included, but they are pretty much standalone utility files. Maybe a static routine defined in a header file? I'll take a look at the unit tests...

[samwalls-arm]

@mark-lunarg Yes, I wasn't sure if this was a good solution. It also looks like I won't be able to appropriately mock the layer framework in the tests themselves, at the moment. I think I should remove the tests for now. We may figure out a way of using them properly later.

[mark-lunarg]

Adding a link to this PR to the Best Practices Tracking issue #24, in case these changes become necessary in the future.