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

Segfault when accessing interface extension #5900

Closed
tunabrain opened this issue Dec 17, 2024 · 9 comments
Closed

Segfault when accessing interface extension #5900

tunabrain opened this issue Dec 17, 2024 · 9 comments
Assignees
Labels
goal:client support Feature or fix needed for a current slang user.

Comments

@tunabrain
Copy link

The following snippet causes a compiler segfault:

interface IFoo
{
    float get();
}

extension<FooType : IFoo> FooType {
    float load()
    {
        return get();
    }
}

struct Foo : IFoo
{
    StructuredBuffer<float> buffer;
    int dummy;

    float get() { return buffer[0]; }
}

float bugTest(IFoo t)
{
    return t.load();
}

uniform Foo input;
uniform RWStructuredBuffer<float> output;

[shader("compute")]
[numthreads(32, 1, 1)]
void main(uint3 dispatchThreadID: SV_DispatchThreadID)
{
    output[0] = bugTest(input);
}

Variations of it (e.g. commenting out dummy) generates invalid HLSL instead.

@bmillsNV bmillsNV assigned bmillsNV and kaizhangNV and unassigned bmillsNV Dec 19, 2024
@bmillsNV bmillsNV added the goal:client support Feature or fix needed for a current slang user. label Dec 19, 2024
@bmillsNV bmillsNV added this to the Q4 2024 (Fall) milestone Dec 19, 2024
@bmillsNV
Copy link
Collaborator

@kaizhangNV can you take a look at this in the current sprint?

@kaizhangNV
Copy link
Contributor

Yes

@kaizhangNV
Copy link
Contributor

kaizhangNV commented Dec 25, 2024

You need to change

float bugTest(IFoo t)

to

float bugTest<T: IFoo>(T t)

for now.

I'm not sure whether float bugTest(IFoo t) is a correct syntax here in this scenario.

@kaizhangNV
Copy link
Contributor

kaizhangNV commented Dec 25, 2024

From what I understand,

extension<FooType : IFoo> FooType {

is not actually extending IFoo itself, it extends every type that conforms to IFoo, so it uses generic extension here.
I don't know the fundamental reason why slang doesn't support interface extension.

So when you try to use load method, you have to use generic as well, because load is pretty much equivalent to a generic function, similar (but not exactly) to this:

float load<T: IFoo> (T this) { ... }

@kaizhangNV
Copy link
Contributor

@csyonghe

I think what we should really fix is to report error at:

float bugTest(IFoo t)
{
    return t.load();    // -> report error, because t doesn't have load member function.
}

@csyonghe
Copy link
Collaborator

@kaizhangNV Actually this call is allowed in Slang.

When defining a variable or a parameter whose type is an interface, it means an "existential type". The function will then be specialized by the actual type of the argument, if the type is statically known, or fallback to dynamic dispatch if the type isn't statically known.

@kaizhangNV
Copy link
Contributor

@kaizhangNV Actually this call is allowed in Slang.

When defining a variable or a parameter whose type is an interface, it means an "existential type". The function will then be specialized by the actual type of the argument, if the type is statically known, or fallback to dynamic dispatch if the type isn't statically known.

Hmm, good to know, then I think the actual issue is the specialization. As when I look at the IR code, it looks like the specialization can't specialize the actual type. In this code, the type is statically known.

@tunabrain
Copy link
Author

You need to change

float bugTest(IFoo t)

to

float bugTest<T: IFoo>(T t)

for now.

I'm not sure whether float bugTest(IFoo t) is a correct syntax here in this scenario.

I did notice that changing it into the latter makes it work. This was surprising to me, since following the documentation I expected them to be treated the same. On this page:
https://shader-slang.com/slang/user-guide/interfaces-generics.html
it shows both forms and then mentions "From the Slang compiler’s view, apply and apply_simple will be compiled to the same target code."

Unfortunately the second form does not work for us in this case due to reflection issues for generic functions (that's a separate problem), but it was my understanding the two would be equivalent.

@kaizhangNV
Copy link
Contributor

close the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
goal:client support Feature or fix needed for a current slang user.
Projects
None yet
Development

No branches or pull requests

4 participants