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

glslang should report a error for Feature: last case/default label not followed by statements'. #2712

Merged
merged 1 commit into from
Nov 1, 2021

Conversation

ZhiqianXia
Copy link
Contributor

@ZhiqianXia ZhiqianXia commented Jul 29, 2021

the ol430 spec says:
Fall through labels are allowed, but it is a compile-time error to have no statement between a label and the end of the switch statement.

So if the glsl version <=430 , glslang should report a error for Feature: last case/default label not followed by statements'.

shader:

#version 430
precision mediump float;
in highp vec4 dEQP_Position;

void main ()
{
	switch (1)
	{
		case 0: break;
		case 1:
	}
	gl_Position = dEQP_Position;
}

specs:

[1] https://www.khronos.org/registry/OpenGL/specs/gl/GLSLangSpec.4.30.pdf
[2] https://www.khronos.org/registry/OpenGL/specs/gl/GLSLangSpec.4.40.pdf

@greg-lunarg
Copy link
Contributor

Looks like you will need to fix test spv.switch.frag.

@greg-lunarg greg-lunarg added the kokoro:run Trigger Google bot runs label Jul 30, 2021
@kokoro-team kokoro-team removed the kokoro:run Trigger Google bot runs label Jul 30, 2021
@greg-lunarg
Copy link
Contributor

greg-lunarg commented Jul 30, 2021

Here is the current situation as I can see:

If no statement after last label {
    If ESProfile and (version <= 300 or version >= 320)
        error
    else if !ESProfile and (version <= 430 or version >= 460)
        error
    else
        warn
}

As far as I can tell, generation of SPIR-V does not have an effect.

Is this consistent with your reading of the specs?

If so, please change the logic here to fit this understanding of the specs.

Thanks!

@ZhiqianXia
Copy link
Contributor Author

Spec4.6 says: Fall through labels are allowed.
Spec4.3 says: Fall through labels are allowed, but it is a compile-time error to have no statement between a label and the end of the switch statement.

So , I think there is a warn for 4.6 , not an error.

@greg-lunarg
Copy link
Contributor

This is from the 4.60.7 GLSL spec:

Fall through labels are allowed, but it is a compile-time error to have no statement between a label and the end of the switch statement.

@greg-lunarg
Copy link
Contributor

Please change your code to match the logic as I have outlined above.

@ZhiqianXia
Copy link
Contributor Author

@greg-lunarg thanks for your suggestion, I have changed the code.

@greg-lunarg greg-lunarg self-requested a review October 22, 2021 16:00
@greg-lunarg greg-lunarg added the kokoro:run Trigger Google bot runs label Oct 22, 2021
@kokoro-team kokoro-team removed kokoro:run Trigger Google bot runs labels Oct 22, 2021
Copy link
Contributor

@greg-lunarg greg-lunarg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please fix formatting where noted. Otherwise good. Thanks!

error(loc, "last case/default label not followed by statements", "switch", "");
else if (!isEsProfile() && (version <= 430 || version >= 460))
error(loc, "last case/default label not followed by statements", "switch", "");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please fix indentation here? Should align with error above.

else
warn(loc, "last case/default label not followed by statements", "switch", "");
warn(loc, "last case/default label not followed by statements", "switch", "");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please fix indentation here? Should align with errors above.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for that , i have fixed it.

@greg-lunarg greg-lunarg added the kokoro:run Trigger Google bot runs label Nov 1, 2021
@kokoro-team kokoro-team removed the kokoro:run Trigger Google bot runs label Nov 1, 2021
@greg-lunarg greg-lunarg merged commit 77d680e into KhronosGroup:master Nov 1, 2021
@ZhiqianXia ZhiqianXia deleted the switch_error branch November 2, 2021 06:34
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

Successfully merging this pull request may close these issues.

3 participants