Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG] Using bool field type in buffer causes stack overflow #70

Closed
thargy opened this issue May 19, 2018 · 1 comment
Closed

[BUG] Using bool field type in buffer causes stack overflow #70

thargy opened this issue May 19, 2018 · 1 comment

Comments

@thargy
Copy link
Contributor

thargy commented May 19, 2018

When using the bool field type in a struct that is itself used in a buffer, ShaderGen crashes due to a stack overflow. This can be traced to TypeSizeCache.GetCSharpSize not having a known size for System.Boolean, to fix that issue requires reference to the conversation in #14, as the size is different depending on the backend in question.

This bug then causes it to try to determine the size manually, there are only two fields in a bool, and both are static strings (FalseString and TrueString). As they are statics they should be skipped as they don't contribute to the size.

However, this bug causes TypeSizeCahce.GetCSharpAlignment to be called for the symbol System.String. This two is not a known type, and also has a single field, which is also a static string (Empty), so TypeSizeCahce.GetCSharpAlignment is called again for the symbol System.String. And we get recursion.

The fixes therefore are:

  1. Add a size of 4 bytes for bool, which should be correct for everything but Metal (see Fix usage of boolean uniform variables, with test #14). May need to add support to specify a size of 1 for Metal.
  2. Skip consideration of static fields in structs when calculating Size.
  3. Change recursive methods to use stack based recursion and detect repeats.
  4. Consider adding support for System.String by treating as a byte[] or similar.
thargy added a commit to thargy/ShaderGen that referenced this issue May 19, 2018
…culates alignments and sizes in one pass and it adds alignment info objects as it goes, preventing duplicate analysis. It remains thread safe and should be considerably faster and safer.

It also correctly skips static fields now, and the code is no longer duplicated.
@thargy
Copy link
Contributor Author

thargy commented May 19, 2018

I've completed a fix for this an tested that the new analyser returns the same info as the old algorithm.

thargy added a commit to thargy/ShaderGen that referenced this issue May 20, 2018
…ould probably not support it at all in shader code. To that end I've added a check that will auto-detect the majority of non-blittable types and error cleanly.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant