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

integral_constant is exposed to the global namespace in DXC (trunk) #6646

Closed
KStocky opened this issue May 23, 2024 · 3 comments · Fixed by #6700
Closed

integral_constant is exposed to the global namespace in DXC (trunk) #6646

KStocky opened this issue May 23, 2024 · 3 comments · Fixed by #6700
Assignees
Labels
bug Bug, regression, crash

Comments

@KStocky
Copy link

KStocky commented May 23, 2024

Description
integral_constant is defined in the global namespace in DXC(1.8.2405.17). The primary template appears to be declared as follows:

template<typename T>
struct integral_constant;

I don't think this type should be defined in the global namespace.

Steps to Reproduce

This should not compile: https://godbolt.org/z/hKxdKhW7e.
DXC 1.8.2403 does not compile this code: https://godbolt.org/z/vha9GennT

Environment

  • DXC version: 1.8.2405.17
  • Host Operating System N/A
@KStocky KStocky added bug Bug, regression, crash needs-triage Awaiting triage labels May 23, 2024
@damyanp damyanp moved this to For Google in HLSL Triage May 28, 2024
@damyanp
Copy link
Member

damyanp commented May 28, 2024

@cassiebeckley - we think that this is the result of something you added recently that should be in the vk namespace.

@KStocky
Copy link
Author

KStocky commented Jun 2, 2024

Just updating this to say that this bug is now in 1.8.2405.17. It is no longer just an issue with DXC trunk on godbolt

@s-perron s-perron self-assigned this Jun 17, 2024
@s-perron s-perron removed the needs-triage Awaiting triage label Jun 17, 2024
@s-perron s-perron moved this from For Google to Triaged in HLSL Triage Jun 17, 2024
@s-perron
Copy link
Collaborator

In the code that adds the builtin types, the pattern used for adding types in the vk namespace is

} else if (kind == AR_OBJECT_VK_INTEGRAL_CONSTANT && m_vkNSDecl) {

Note that the second condition is to check if the vk namespace has been defined. If not, it continues to the next condition, and eventually falls into the default case, which add the function in the default namespace:

} else {
DXASSERT(templateArgCount == 1 || templateArgCount == 2,
"otherwise a new case has been added");
TypeSourceInfo *typeDefault =
TemplateHasDefaultType(kind) ? float4TypeSourceInfo : nullptr;
recordDecl = DeclareTemplateTypeWithHandle(
*m_context, typeName, templateArgCount, typeDefault);
}
m_objectTypeDecls[i] = recordDecl;
m_objectTypeDeclsMap[i] = std::make_pair(recordDecl, i);
}

We need to change this first condition, so that it continues if the vk namespace is not defined.

@damyanp damyanp added this to the Next 2024 Release milestone Jun 17, 2024
s-perron added a commit to s-perron/DirectXShaderCompiler that referenced this issue Jun 18, 2024
Some of the types that have been added to the vk namespace were being
added to the default namespace when compiling for DXIL. The if
conditions were such that they would fall through to a default case.

The solution is to explicitly add code that we should skip adding those
builtin types when the vk namespace is not defined.

Fixes microsoft#6646.
s-perron added a commit that referenced this issue Jun 21, 2024
Some of the types that have been added to the vk namespace were being
added to the default namespace when compiling for DXIL. The if
conditions were such that they would fall through to a default case.

The solution is to explicitly add code that we should skip adding those
builtin types when the vk namespace is not defined.

Fixes #6646.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bug, regression, crash
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants