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

We should forbid pointers to handles #1832

Closed
alan-baker opened this issue Jun 10, 2021 · 4 comments · Fixed by #1983
Closed

We should forbid pointers to handles #1832

alan-baker opened this issue Jun 10, 2021 · 4 comments · Fixed by #1983
Assignees
Labels
wgsl WebGPU Shading Language Issues
Projects
Milestone

Comments

@alan-baker
Copy link
Contributor

Currently the spec forbids naming the handle address space directly in text and pointers to handles are disallowed as function parameters. In order to encourage a consistent usage of samplers and textures by value, I would suggest banning let constants and the address of operator for types containing handles.

The only way to get a pointer to a handle right now would be to use type inference in a let declaration.

Banning let constants isn't strictly necessary, but it doesn't provide much value if we want to disallow pointers to handles. It would really just be an aliased name.

@alan-baker alan-baker added the wgsl WebGPU Shading Language Issues label Jun 10, 2021
@alan-baker alan-baker added this to the MVP milestone Jun 10, 2021
@alan-baker alan-baker added this to Needs Discussion in WGSL Jun 10, 2021
@dneto0
Copy link
Contributor

dneto0 commented Jul 14, 2021

You can also chain together & with *:

    [[group(0),binding(0)]] var t: texture_2d;
    [[group(0),binding(0)]] var s: sampler;

   fn foo {
      let v = textureSample(  *&*&*&*&t, s );  // Yes, this is silly.
   }

I agree we don't need the ability to form a pointer to a handle, and it only adds complexity.

I think the (only) spec change is to forbid the address-of operator from operating on a reference-to-handle.

@dneto0
Copy link
Contributor

dneto0 commented Jul 14, 2021

In a future where WGSL has arrays of resources, we can relax the rule.

@kdashg kdashg changed the title Pointers to handles We should forbid pointers to handles Jul 20, 2021
@alan-baker alan-baker self-assigned this Jul 20, 2021
@kdashg kdashg moved this from Needs Discussion to Resolved: Needs Specification Work in WGSL Jul 20, 2021
@kdashg
Copy link
Contributor

kdashg commented Jul 20, 2021

WGSL meeting minutes 2021-07-20
  • (Proposal: Ban address-of on reference-to-handle)

  • MM: The issue wasn’t clear about the motivation. What would this stop authors from writing?

  • AB: Currently function restrictions prevent you from creating a pointer to the handle storage class. Maybe in the future when we have arrays of samplers, right now these have no real purpose. No builtins take them. So let’s keep things simple and forbid them. We want handles to be passed by value to functions, so taking away the option to pass them by pointers makes sense. Since all the builtins work this way, it makes user functions more consistent with the builtins

  • MM: So I can still say var foo: texture = my_texture?

  • AB: If you were to do let foo = my_texture, that’s just a local name for that. But you wouldn’t be able to apply the & operator to my_texture

  • AB: This would be permitted:

      Var x : sampler
    
    
      Let y = x;
    
    
      Let y : sampler  = x;
    
  • AB: This is forbidden:

      Let z = &x;
    
  • AB: You can’t pass z, none of the builtins accept it, all you can do is dereference it.

  • MM: Can you provide an explicit type for y, with a colon?

  • AB: Yes. We should always work with handles consistently as values

  • MM: I think this makes sense. DN raised the point that && works in C.

  • JG: It just feels like a handle is a pointer by another name. And we forbid taking pointers to pointers…

  • AB: Right, at the hardware level, the handle is really a pointer to some stuff in memory with metadata

  • JG: And traditionally, var creates some memory and gives you a pointer to it.

  • MM: It’s probably fine to disallow the && case for now. If people ask for it, we can add it back in

  • AB: The time to reexamine this is when we re-introduce bindless. It’s good to encourage people to work with resources in a consistent way, it’s a benefit to readers. Rather than some people using pointers and others using values, everyone will just use values.

  • JG: Yeah, vars are pointers with sugar on top

  • MM: Is anyone arguing against this

  • JG: I am. It’s not onerous to have it in the language, and it’s consistent, so why not just let it work.

  • DM: The handle storage class is different.

  • JG: It’s weird to say, you can get a pointer from anything if it’s a var, because it’s a var, but pointer handles are special.

  • AB: We may have gotten the var/let split wrong, but I don’t want to revisit it. Ideally, handles would not be permitted in vars.

  • MM: I don’t have a position on this. But one argument that might be relevant: If we want to have a larger language feature like bindless, at the point that we want to specify that, we might want to change how pointers to resources work. Forbidding them now gives us the flexibility to set the rules later freely when we need them

  • BC: The rules around what we can pass in parameters are already difficult enough to validate

  • JG: Isn’t that orthogonal to this change?

  • BC: Validation is complicated by chasing derefs and refs

  • JG: Doesn’t the function’s signature make that clear?

  • BC: If you’ve got a let which is the address of a texture, and then you pass that variable to a texture, you need to chase that down. It was an implementation difficulty.

  • JP: It complicated the identification of originating variables.

  • MM: But you do have the chain to back through.

  • AB: Yes, but there are fewer cases to handle

  • MM: I’m surprised one is easier to implement.

  • DM: Do people take pointers to textures in MSL? HLSL doesn’t allow it. Let’s just forbid it and see what we need later

  • JG: We already have restrictions on passing pointers to textures to functions. This is a different way to enforce that, at the expense of consistency in what you can do with vars. “This is a var, but because it’s a texture there are additional restrictions”

  • MM: sympathetic to that. Adding unexpected restrictions is irritating to developers.

  • AB: I don’t think it’s valuable to have two ways to do the same thing. Handles are already pointers. There’s precedent for treating handles this way. It would be fine if we used pointers everywhere when you work with handles, but we’ve already gone with the implicit handle approach, and we should stick with it

  • JG: I just don’t like the thing we did with vars for handles

  • AB: Please file an issue - I’m happy to have the discussion, I just thought it wouldn’t be palatable

  • MM: David had strong opinions about what an allocation is, what an rvalue is. Let’s revisit that when he’s here

  • Resolution: needs specification

alan-baker added a commit to alan-baker/gpuweb that referenced this issue Jul 23, 2021
@dneto0
Copy link
Contributor

dneto0 commented Jul 23, 2021

I wanted to fix/comment-on a couple of items discussed in the 7-20 meeting, to make sure we're on the same page:

So I can still say var foo: texture = my_texture?

No, for two cases:

  • at module scope, only private variables can have initializers. So texture/sampler variables can't have initializers
  • in function scope, a variable's store type must be constructible. texture/samplers values are not constructible.

Var x : sampler
Let y = x; Let y : sampler = x;

This isn't permitted because the type of a let-declaration must be constructible, or a pointer type.

It's that last case (pointer type) that is the only change needed.

WGSL automation moved this from Resolved: Needs Specification Work to Done Jul 27, 2021
dneto0 pushed a commit that referenced this issue Jul 27, 2021
* Disallow taking the address of a handle

Fixes #1832

* simplify text
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wgsl WebGPU Shading Language Issues
Projects
WGSL
Done
Development

Successfully merging a pull request may close this issue.

3 participants