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.

Sign up for GitHub

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

Jump to bottom

Disallows TypeManager::GetId on non unique types #5691

Open
5 of 15 tasks
s-perron opened this issue May 29, 2024 · 0 comments
Open
5 of 15 tasks

Disallows TypeManager::GetId on non unique types #5691

s-perron opened this issue May 29, 2024 · 0 comments
Assignees
@s-perron
Labels
component:optimization

Comments

@s-perron
Copy link
Collaborator

s-perron commented May 29, 2024 •
edited
Loading

As was found in #5624, the multiple types in the module can hash to the same entry in the type manager. When GetId is called on one of these, the return value is not predictable. This can lead to errors when it returns the id of a type different than what is expected.

To solve this problem, we need to disallow calls to GetId where the type is not guarenteed to be unique in the spec. Before we can add the assert to enforce this restriction, we need to fix up the places in the code that uses GetId on types that are not necessarily unique.

The list of test buckets that fail when the assertion is added is:

  • CopyPropArrayPassTest
  • ElimDeadIOComponentsTest
  • ElimDeadOutputStoresTest
  • FixStorageClassTest
  • FixTypeTest
  • GraphicsRobustAccessTest
  • InlineTest
  • InstDebugPrintfTest
  • InterfaceVariableScalarReplacementTest
  • TypeManager
  • UpgradeMemoryModelTest
  • CompositeConstructFoldingTest
  • CompositeExtractOrInsertMatchingTest
  • AnalyzeLiveInputTest
  • ConvertToSampledImageTest

Not all of these issues will lead to real problems, but they all trigger the assert. We will also want to check with some real users to make sure their code does not trigger the assert before it lands.
@s-perron s-perron self-assigned this May 29, 2024
s-perron added a commit to s-perron/SPIRV-Tools that referenced this issue May 29, 2024
@s-perron
Remove implicit call to GetId in ConvertToSampledImagePass.
63162b3 
We replace getting the id of a poitner type with a specific funciton
call to FindPointerToType. Also, FindPointerToType is updated to not
indirectly call GetId. This leads to a linear search for an existing
type in all cases, but it is necessary.

Note that this function could have a similar problem. There could be two
pointer types with the same pointee and storage class, and the first one
will be returned. I have checked the ~20 uses, and they are all used in
situations where the id is used to create something new, and it does not
have to match an existing type. These will not cause problems.

Part of KhronosGroup#5691
@s-perron s-perron mentioned this issue May 29, 2024
Merged
s-perron added a commit to s-perron/SPIRV-Tools that referenced this issue May 29, 2024
@s-perron
Remove calls to GetId in liveness analysis
cda9d03 
Part of KhronosGroup#5691
@s-perron s-perron mentioned this issue May 29, 2024
Merged
s-perron added a commit to s-perron/SPIRV-Tools that referenced this issue May 30, 2024
@s-perron
Remove implicit call to GetId in ConvertToSampledImagePass.
b4143d3 
We replace getting the id of a poitner type with a specific funciton
call to FindPointerToType. Also, FindPointerToType is updated to not
indirectly call GetId. This leads to a linear search for an existing
type in all cases, but it is necessary.

Note that this function could have a similar problem. There could be two
pointer types with the same pointee and storage class, and the first one
will be returned. I have checked the ~20 uses, and they are all used in
situations where the id is used to create something new, and it does not
have to match an existing type. These will not cause problems.

Part of KhronosGroup#5691
s-perron added a commit to s-perron/SPIRV-Tools that referenced this issue May 30, 2024
@s-perron
Remove calls to GetId in liveness analysis
5c441a9 
Part of KhronosGroup#5691
s-perron added a commit that referenced this issue Jun 3, 2024
@s-perron
Remove implicit call to GetId in ConvertToSampledImagePass. (#5692)
95681dc 
We replace getting the id of a poitner type with a specific funciton
call to FindPointerToType. Also, FindPointerToType is updated to not
indirectly call GetId. This leads to a linear search for an existing
type in all cases, but it is necessary.

Note that this function could have a similar problem. There could be two
pointer types with the same pointee and storage class, and the first one
will be returned. I have checked the ~20 uses, and they are all used in
situations where the id is used to create something new, and it does not
have to match an existing type. These will not cause problems.

Part of #5691
s-perron added a commit that referenced this issue Jun 3, 2024
@s-perron
Remove calls to GetId in liveness analysis (#5693)
fd96922 
Part of #5691
s-perron added a commit to s-perron/SPIRV-Tools that referenced this issue Jul 17, 2024
@s-perron
[opt] Fix uses of type manager in fix storage class
138ac9f 
This removes some uses of the type manager. One use could not be
removed. Instead I had to update GenCopy to not use the type manager,
and be able to copy pointers.

Part of KhronosGroup#5691
s-perron added a commit to s-perron/SPIRV-Tools that referenced this issue Jul 17, 2024
@s-perron
[opt] Fix uses of type manager in fix storage class
9cc3cf0 
This removes some uses of the type manager. One use could not be
removed. Instead I had to update GenCopy to not use the type manager,
and be able to copy pointers.

Part of KhronosGroup#5691
s-perron added a commit to s-perron/SPIRV-Tools that referenced this issue Jul 17, 2024
@s-perron
[opt] Fix uses of type manager in fix storage class
5de46fe 
This removes some uses of the type manager. One use could not be
removed. Instead I had to update GenCopy to not use the type manager,
and be able to copy pointers.

Part of KhronosGroup#5691
s-perron added a commit to s-perron/SPIRV-Tools that referenced this issue Jul 18, 2024
@s-perron
[opt] Fix uses of type manager in fix storage class
666fe2e 
This removes some uses of the type manager. One use could not be
removed. Instead I had to update GenCopy to not use the type manager,
and be able to copy pointers.

Part of KhronosGroup#5691
@s-perron s-perron mentioned this issue Jul 18, 2024
Merged
s-perron added a commit to s-perron/SPIRV-Tools that referenced this issue Jul 18, 2024
@s-perron
[opt] Fix uses of type manager in fix storage class
82672b1 
This removes some uses of the type manager. One use could not be
removed. Instead I had to update GenCopy to not use the type manager,
and be able to copy pointers.

Part of KhronosGroup#5691
s-perron added a commit that referenced this issue Jul 24, 2024
@s-perron
[opt] Fix uses of type manager in fix storage class (#5740)
81a1160 
This removes some uses of the type manager. One use could not be
removed. Instead I had to update GenCopy to not use the type manager,
and be able to copy pointers.

Part of #5691
Keenuts pushed a commit to Keenuts/SPIRV-Tools that referenced this issue Nov 12, 2024
@s-perron @Keenuts
[opt] Fix uses of type manager in fix storage class (KhronosGroup#5740)
7d6ac19 
This removes some uses of the type manager. One use could not be
removed. Instead I had to update GenCopy to not use the type manager,
and be able to copy pointers.

Part of KhronosGroup#5691
mmoult pushed a commit to mmoult/SPIRV-Tools that referenced this issue Nov 16, 2024
@s-perron @mmoult
Remove implicit call to GetId in ConvertToSampledImagePass. (KhronosG…
7fad324 
…roup#5692)

We replace getting the id of a poitner type with a specific funciton
call to FindPointerToType. Also, FindPointerToType is updated to not
indirectly call GetId. This leads to a linear search for an existing
type in all cases, but it is necessary.

Note that this function could have a similar problem. There could be two
pointer types with the same pointee and storage class, and the first one
will be returned. I have checked the ~20 uses, and they are all used in
situations where the id is used to create something new, and it does not
have to match an existing type. These will not cause problems.

Part of KhronosGroup#5691
mmoult pushed a commit to mmoult/SPIRV-Tools that referenced this issue Nov 16, 2024
@s-perron @mmoult
Remove calls to GetId in liveness analysis (KhronosGroup#5693)
c292fba 
Part of KhronosGroup#5691
mmoult pushed a commit to mmoult/SPIRV-Tools that referenced this issue Nov 16, 2024
@s-perron @mmoult
[opt] Fix uses of type manager in fix storage class (KhronosGroup#5740)
4b788c0 
This removes some uses of the type manager. One use could not be
removed. Instead I had to update GenCopy to not use the type manager,
and be able to copy pointers.

Part of KhronosGroup#5691
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@s-perron s-perron

Labels
component:optimization
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@s-perron