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

Fix crash in ApplyLinkProgramExtra #1544

Merged
merged 1 commit into from
Jan 16, 2018

Conversation

y-novikov
Copy link
Contributor

Access to i.ActiveResources.DefaultUniformBlock used to crash
when glLinkProgram failed, because i.ActiveResources was null.

@y-novikov
Copy link
Contributor Author

I'm not sure code flow is even supposed to reach ApplyLinkProgramExtra in the case when glLinkProgram fails.
The check itself should be correct anyway.
Unless you want to change the flow in a way that ApplyLinkProgramExtra is not reached, then it will be better to change the if() to assert().

@ben-clayton
Copy link
Contributor

Hi @y-novikov,

Thank you very much for your PR.

I'm not sure code flow is even supposed to reach ApplyLinkProgramExtra in the case when glLinkProgram fails.

The same API logic is executed for both interception time and for replay, so the command:

cmd void glLinkProgram(ProgramId program) {
  p := GetProgramOrError(program)

  ctx := GetContext()
  ApplyLinkProgramExtra(p, GetLinkProgramExtra(ctx, p, null))
}

Will call GetLinkProgramExtra for interception and replay before passing that into ApplyLinkProgramExtra.
GetLinkProgramExtra however is implemented differently for interception and replay.

  • For interception it is implemented in gles_extras.cpp, which will only fill in the bits of the LinkProgramExtra that it can get back from the driver. In this situation, the link failed so we can't get uniform information, hence the null pointer.
  • For replay, we simply return the data encoded by the interceptor here.

The logic flow has no way to bail in either case. Just because the intercepted command set a GL error state, that doesn't affect the way we attempt to replay the command stream.

I've considered your fix with a few other approaches, and aside from the single review comment, I think this is good. I'm slightly concerned that there is other code knocking around expecting ActiveResources to be non-null, but if this is now working for you, it's a major improvement.

Thank you again,
Ben

Values: values[offset:len(values)],
UniformIndex: uniformIndex
)
if i.ActiveResources != null {
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be slightly more consistent to use i.LinkStatus == GL_TRUE as the conditional here as it then mirrors the logic in the interceptor here.

Feel free to ignore.

Access to i.ActiveResources.DefaultUniformBlock used to crash
when glLinkProgram failed, because i.ActiveResources was null.
@y-novikov
Copy link
Contributor Author

I've changed it to "i.LinkStatus == GL_TRUE".
Could you please merge it?
I don't have write access, I think.

@ben-clayton
Copy link
Contributor

Thank you for the change. I've flagged the PR for automated building. Assuming everything is green (should be!) I'll merge ASAP.

Thank you again.

@ben-clayton ben-clayton merged commit 54c25c0 into google:master Jan 16, 2018
@y-novikov y-novikov deleted the link_program_crash branch January 16, 2018 21:36
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