Add Validation for VK_KHR_create_renderpass2

[Tobski]

To achieve decent coverage for VK_KHR_create_renderpass2, I basically ended up implementing several dozen checks for existing renderpass validation holes as well, since there was (unsurprisingly!) enormous overlap. Due to the enormity of this task, I made sure there were tests in place for as many VUIDs as I could manage (though there's a couple I've still not looked at). Unsurprisingly, this is a rather large change to review, so I appreciate it might take some doing.

There is some churn in the tests, as I ended up renaming + regrouping several tests so that I could keep all the similar tests together in order to avoid going crazy whilst modifying them (sometimes several at a time needed a similar edit). Hopefully this is ok, I've confirmed the ones I moved all still test the things they were previously testing, and there are several new ones as well.

There's definitely scope for slightly more cleanliness in a few areas here, and I'm happy to take such feedback as it's pointed out.

Should fix #313

[CLAassistant]

All committers have signed the CLA.

[Tobski]

Re: The last commit. I depend on the vk_safe_structs.h header for upgrading VkRenderPassCreateInfo into safe_VkRenderPassCreateInfo2KHR in both the layer itself AND the tests for the layer. Doing it without the safe version isn't really an option as it would leave pointers allocated, requiring painful user clean up.

However this header is currently only generated for the layers, not the tests. So I've made the tests build after the layers in the top level CMake, but this doesn't really express the dependency properly so I suspect there's a better approach here, but I don't know what it is. Any guidance would be welcome.

[Tobski]

...And that didn't work anyway. Help? 😅

[jzulauf-lunarg]

First pass at review, generally good with some specific requests/comments -- will get to more tomorrow. Some general comment on PR's for VVL

• Break layer and test changes into separate commits when possible with commit titles starting "layers:" and "tests:" respectively.
• Code should be bisectable (compile and pass CI at each commit) including formatting changes
• Fixup/cleanup changes should be "fixup" rebase -i into the code they fix.
• Google C++ Style Guide as possible with new code

[jzulauf-lunarg]

@mikew-lunarg -- can you look at the CMake issues (and build failures) @Tobski is seeing?

[Tobski]

First pass at review, generally good with some specific requests/comments -- will get to more tomorrow. Some general comment on PR's for VVL

Thanks! I imagine this isn't a super easy thing to review :)

• Break layer and test changes into separate commits when possible with commit titles starting "layers:" and "tests:" respectively.

Ok, should I just do that moving forward, or do you think I should go back and edit the commit history?

• Code should be bisectable (compile and pass CI at each commit) including formatting changes

That would have been smart, same question as above - should I go back and fix it, or just squash this one and do that moving forward?

Also I've been developing in Visual Studio, where my local commits all compile - but VS doesn't trigger anywhere near as many warnings as Clang/GCC which, other than the CMake issues, has been the main thing breaking CI AFAICT since you have -werror on, not sure. I also don't get the CMake issue locally, so I'd have a hard time making CI work before actually submitting the commits :(

• Fixup/cleanup changes should be "fixup" rebase -i into the code they fix.

Ah ok, missed that - will do in future. Does "fixup" include things that are minor bug fixes, or just code factoring? Mostly thinking the sort of "have to fix VU check and test in one commit to make something workable" issue here.

• Google C++ Style Guide as possible with new code

Yea sorry about that - a bunch of this (notably the lower case functions in the new file!) was a straight copy/move of code that I wrote a while back, before this style guide was used consistently - I'll stick with it in future.

[Tobski]

@mikew-lunarg -- can you look at the CMake issues (and build failures) @Tobski is seeing?

FYI I think looking at it, the CI failures I'm seeing build the tests first, before the layers, and the file the tests are depending on comes from the layers build (the vk_safe_structs.cpp file). I don't have a preference for how this is fixed, but that file is sort of invaluable in writing these tests unfortunately.

[Tobski]

@jzulauf-lunarg One thing I haven't broached yet because I need advice (and I'd rather tackle all the other stuff first) is that parameter_validation_utils.cpp intercepts with its own version of vkCreateRenderPass for the sake of tracking various pipeline state effectively - if this is intended to work with CRP2, I assume I have to hook vkCreateRenderPass2KHR in the same way, which probably means I want vk_safe_struct.cpp available to this layer build as well 😅

[jzulauf-lunarg]

• Break layer and test changes into separate commits when possible with commit titles starting "layers:" and "tests:" respectively.

Ok, should I just do that moving forward, or do you think I should go back and edit the commit history?

• Code should be bisectable (compile and pass CI at each commit) including formatting changes

That would have been smart, same question as above - should I go back and fix it, or just squash this one and do that moving forward?

In both these cases, it would be best to rebase -i and clean up the code into correct, bisectable, and correctly grouped changes -- after addressing the current set of comments.

I don't mind rewriting history on the PR (in fact I prefer it to an ugly squash), but as it will strand all the review comments made to date, best to get them out of the way first , no?

[Tobski]

In both these cases, it would be best to rebase -i and clean up the code into correct, bisectable, and correctly grouped changes -- after addressing the current set of comments.

I don't mind rewriting history on the PR (in fact I prefer it to an ugly squash), but as it will strand all the review comments made to date, best to get them out of the way first , no?

Sounds fine, I just wanted to check the best approach first - I'll make it all work and then rewrite the history afterwards then :)

[jzulauf-lunarg]

Re: the parameter_validation for CRP2 -- not sure you have to consolidate the two the way you did with core_validation. If you can refactor out the common code from pv_vkCreateRenderPass and vkCreateRenderPass in parameter_validation_utils.cpp to common functions, that will likely be DRY enough.

[Tobski]

Re: the parameter_validation for CRP2 -- not sure you have to consolidate the two the way you did with core_validation. If you can refactor out the common code from pv_vkCreateRenderPass and vkCreateRenderPass in parameter_validation_utils.cpp to common functions, that will likely be DRY enough.

Ok but I do have to do something. I'll take a look at it closer once the rest of this is fixed.

[mikew-lunarg]

In repro-ing the CMake error, Ubuntu 16.04 gcc 5.4.0 has some messages.

[karl-lunarg]

I wanted to report both/all the handles to be honest - I thought there was a path to do that now with debug_utils but I couldn't figure it out...?

I've been told that there was some code proposed to support this, but it didn't pass review and didn't get any attention after that. It does seem that this was a motivation for debug_utils in the first place. I know that we'll need this soon and hopefully will take another crack at it. But for now, try to add any additional handle info in the free-form text string.

[Tobski]

@mikew-lunarg @jzulauf-lunarg Ok so I eventually got this building on Linux locally, and found that I had to add some sort of indication to cmake in the tests CMakeLists.txt that the safe_structs files are indeed generated. The easiest and safest way I could find to do that was to literally copy and paste the generation command into that CMakeLists.txt.

I guess the command in the top level CMakeLists.txt isn't sufficient for CMake to make this connection, which sounds about right given what I remember of how CMake works. Still feels a little odd though, so not sure if this is really the right approach but... not sure what else to do at the moment.

[karl-lunarg]

The easiest and safest way I could find to do that was to literally copy and paste the generation command into that CMakeLists.txt.

@Tobski I added a comment to your commit where you did this with some (hopefully better) CMake code.

[Tobski]

@jzulauf-lunarg I've rebased the entire thing and separated it somewhat. Basically core validation vs. parameter validation vs. tests for all of this. The core validation set (first commit) is rather enormous though and could, with some effort, be bisected further - but I don't have a good handle on whether that's necessary or at what granularity it might be split up. If you have any advice here it would be appreciated.

[jzulauf-lunarg]

@Tobski Further bisection for a specific feature is usually limited to

"layers: refactored this thing to make new feature easier to implement atop of"
"layers: implement new validation"
"tests: added needed features to test framework to enable new tests"
"tests: new tests for the layers stuff I just added"

Repeat for logically grouped features.

I'll give it a detailed look today (unless it spills over to more than one)

[jzulauf-lunarg]

Finished 2nd pass through the end of buffer_validation.h... continued later

[Tobski]

Alright, let's try this again - it's now been rebased on master (again!) and fixed up, so there's now only three commits. Should be good to go.

[Tobski]

Hum. Looks like one of the Android builds hung randomly, but otherwise checks are clean (requesting a rerun doesn't seem to do anything...). Do we need any further review or... is this ready to go?

[shannon-lunarg]

Just a note, that same android build has been hanging on most jobs the past day or so. Rerunning it usually results in success (or another hang).

[Tobski]

Ok looks like my rerun worked the last time I tried and it passed, so we're all good I think. @jzulauf-lunarg @mikes-lunarg anything left to do here? Or just rebase and merge? Few people waiting on this one so would like to get it in ASAP if it's ready.

[jzulauf-lunarg]

@Tobski -- I'm just doing a quick read-over before I "push the button".

[Tobski]

@jzulauf-lunarg 😬

[jzulauf-lunarg]

There's still undesired whitespace changes. Also, at least two tests look like they've disappeared. The diff is mangled and some are no longer aligning correctly as well. Dunno if the whitespace/missing functions are contributing to that.

[Tobski]

I really need to just setup a hook to make git clang format run when I commit :|

[Tobski]

(Should be good to go again now)

[jzulauf-lunarg]

The diff is still a mess, but the unintentional WS changes appear to be cleaned up.

[Tobski]

The diff is still a mess, but the unintentional WS changes appear to be cleaned up.

Yep, it is. I tried, but not really much I can do about a lot of it - code movement distance is just too large with some things trading positions. I did verify the changes all made sense locally twice over though, if that gives you any less indigestion 😅

[Tobski]

I'm starting to get paranoid that git is secretly reformatting my files since the last build threw formatting errors that didn't get thrown previously despite me not changing that code between runs... 🤔

[jzulauf-lunarg]

Yes, I saw that exact change in an earlier push. It's a trivial amend to the last commit, yes?

[Tobski]

@jzulauf-lunarg Yea it just fixed the formatting issues that showed up. All good now.

[jzulauf-lunarg]

HUZZAH!

[Tobski]

Thanks again for sticking with this 👍

[tobine]

Heroic efforts all around, thanks @Tobski and reviewers!