This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly] Initial support for reference type funcref in clang
ClosedPublic

Authored by pmatos on Jun 23 2022, 6:49 AM.

Details

Summary

This is the funcref counterpart to 104bad5a. We introduce a new attribute
that marks a function pointer as a funcref. It also implements builtin
__builtin_wasm_ref_null_func(), that returns a null funcref value.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
pmatos updated this revision to Diff 446406.Jul 21 2022, 2:58 AM

Rebase on top of main. Add missing code in Sema* to ensure the new funcref builtin is checked for.
Thanks to @asb for helping fix this.

pmatos updated this revision to Diff 446726.Jul 21 2022, 11:53 PM

Update the patch as there were some missing calls from SemaChecking.
Thanks to @asb for spotting it.

pmatos added a comment.Aug 5 2022, 6:28 AM

It would be great to get this landed. Can someone pls take a look at it? It's quite independent from the remainder of the work and self-contained.

maybe @tlively ping?

Oops, sorry about that. Will take a look.

It would be good to add tests for more error conditions like in the externref patch and also additional errors like trying to apply __funcref to types that aren't function pointers.

clang/include/clang/Basic/Attr.td
4053

It would be good to document this!

clang/lib/Sema/SemaChecking.cpp
4522

Are the extra parentheses meaningful here?

6549–6552

How do these parts correspond to the original source code?

aaron.ballman requested changes to this revision.Aug 17 2022, 7:15 AM

Same question here about an RFC for adding this new type to the type system and a design document for how the type behaves as in https://reviews.llvm.org/D122215.

clang/include/clang/Basic/Attr.td
4053

It's a requirement that this be documented. Also, this should probably have a Subjects line that expects a function of some sort? (It doesn't impact the semantics of anything, but it self-documents the expectations for people reading Attr.td.)

clang/include/clang/Basic/DiagnosticSemaKinds.td
7332
  1. What is __clang_webassembly_funcref? Did you mean __funcref?
  2. There's no test coverage for this diagnostic.
clang/include/clang/Basic/TokenKinds.def
666

Why is this a keyword in all language modes?

clang/include/clang/Parse/Parser.h
2843
clang/include/clang/Sema/Sema.h
13103–13104

Formatting is off here.

13169

I think there's a verb missing from this function identifier -- also, I don't think we need to continue the terrible trend of prepending Sema onto the function name which lives in the Sema object.

clang/lib/AST/ASTContext.cpp
954

Where did this value come from?

clang/lib/Parse/ParseDecl.cpp
845–846
851–854
5780

Why that specific ordering as opposed to AR_VendorAttributesParsed?

clang/lib/Sema/SemaChecking.cpp
6554–6558

Why would we need to do this?? And what do we do when there is no FunctionDecl to get back to (because it's a call expression through a function pointer, for example)?

6561

Why are you guaranteed this function returns void?

clang/lib/Sema/SemaType.cpp
7256
7269

This seems like it's not used.

7289–7290

What happens when the user's function already has an explicitly specified address space?

This revision now requires changes to proceed.Aug 17 2022, 7:15 AM
pmatos added inline comments.Sep 15 2022, 5:32 AM
clang/lib/AST/ASTContext.cpp
954

Unsure what you mean here. This is the address space number we also use on the LLVM side.

aaron.ballman added inline comments.Sep 15 2022, 9:26 AM
clang/lib/AST/ASTContext.cpp
954

Unsure what you mean here. This is the address space number we also use on the LLVM side.

That's exactly what I was looking for -- I wanted to make sure this agreed with the LLVM side of things and any public documentation about address spaces for WASM.

pmatos marked 2 inline comments as done.Sep 18 2022, 11:57 PM
pmatos added inline comments.
clang/lib/AST/ASTContext.cpp
954

Unsure what you mean here. This is the address space number we also use on the LLVM side.

That's exactly what I was looking for -- I wanted to make sure this agreed with the LLVM side of things and any public documentation about address spaces for WASM.

Sure! Marking as done.

pmatos marked 3 inline comments as done.Sep 19 2022, 12:52 AM
pmatos added inline comments.
clang/include/clang/Basic/DiagnosticSemaKinds.td
7332
  1. What is __clang_webassembly_funcref? Did you mean __funcref?
  2. There's no test coverage for this diagnostic.

Yes, I meant __funcref. Thanks, will add test coverage for the diagnostic.

clang/include/clang/Basic/TokenKinds.def
666

I might have misread the docs but this keyword should be available for both C and C++. Maybe I want KEYC99 | KEYCXX ?

clang/include/clang/Sema/Sema.h
13103–13104

Don't understand why. It's the same as CheckRISCVBuiltinFunctionCall ?

pmatos updated this revision to Diff 461159.Sep 19 2022, 1:49 AM

Addressed some of the concerns from the comments but not ready for review yet.

pmatos planned changes to this revision.Sep 19 2022, 1:49 AM
pmatos updated this revision to Diff 461544.Sep 20 2022, 5:44 AM

Address most of the comments.

pmatos planned changes to this revision.Sep 20 2022, 5:49 AM
pmatos updated this revision to Diff 470734.Oct 26 2022, 12:52 AM

Updated this revision on top of D122215.

pmatos planned changes to this revision.Oct 26 2022, 12:53 AM
pmatos updated this revision to Diff 487700.Jan 9 2023, 11:39 PM

Rebase on top of main.

@aaron.ballman Hi Aaron, since you requested an RFC for this. I wonder if you have any comments for this or the other related patches: D122215, D139010

aaron.ballman added a subscriber: erichkeane.

@aaron.ballman Hi Aaron, since you requested an RFC for this. I wonder if you have any comments for this or the other related patches: D122215, D139010

I appreciate your patience with the extreme delay on getting you feedback after I asked you to post the RFC; I wanted to give the RFC a chance to bake and then ran into the holiday season as a result. Sorry for that!

Adding @erichkeane as new attributes code owner as well, in case he's got additional thoughts.

clang/include/clang/Basic/TokenKinds.def
666

I was thinking this keyword would only work for a web assembly target, so we'd likely want to add KEYWEBASM or something to the list like we have for KEYALTIVEC. But now I'm second-guessing that instinct and am wondering what behavior we want here.

  1. Parse the keyword in all language modes and for all targets, give an ignored attribute warning if the target isn't web assembly
  2. Parse the keyword in all language modes and for all targets, give a parse time diagnostic (error?) if the target isn't web assembly
  3. Only expose the keyword if the target is web assembly, otherwise it parses as an identifier and you get the usual parse errors

My original thinking was that we wanted #3, but on reflection both #1 and #2 seem reasonable to me. Do you have a preference? I think I prefer either 2 or 3 over 1 because this involves the type system (errors are usually a better approach in that case).

clang/lib/AST/TypePrinter.cpp
1651–1652

I'm not certain that the assert adds much here given that the predicate tests exactly the same thing, perhaps remove it (and drop the curly braces)?

clang/lib/Basic/Targets/WebAssembly.h
44–45

I'm not super familiar with the address space maps, but this sets of my spidey senses. All of the other address space maps end with:

ptr64, hlsl_groupshared, wasm_funcref

but this one is ending with:

ptr64, wasm_externref, wasm_funcref

which makes me think something's amiss here. Thoughts?

clang/lib/Parse/ParseDecl.cpp
846

I think we should assert this condition; we already know what the token is before we call this function anyway.

5780–5783

I don't think we need to care about attribute requirements because this is a keyword attribute (this feature is used to prohibit attributes in certain grammars and I don't think that applies here).

clang/lib/Sema/SemaChecking.cpp
6554–6560

These comments no longer seem to match the code -- nothing creates a FunctionDecl here.

6561

I'm still wondering why this code is needed and what it's trying to do. It worries me that we're changing the type of the call expression like this.

clang/lib/Sema/SemaDecl.cpp
14373
clang/lib/Sema/SemaType.cpp
7266

We're missing Sema test coverage for all the various diagnostics you've added; can you add that coverage?

7269

Still wondering about this

7289–7290

Still wondering about this as well.

erichkeane added inline comments.Jan 11 2023, 6:42 AM
clang/include/clang/Basic/Attr.td
4054

Note that it seems this is likely not going to work correctly on template type aliases! You probably want to make sure that is acceptable to you.

clang/include/clang/Basic/AttrDocs.td
6855 ↗(On Diff #487700)

This says 'function pointer declaration', but you have it's subject as Typedef?

clang/include/clang/Basic/TokenKinds.def
666

I don't think #1 is acceptable for the keyword, we shouldn't ignore keywords like we do attributes. Doing an ignored warning in SemaDeclAttr.td for the ATTRIBUTE is fine, but keyword form needs to be an error.

My preference is #2, an error if we encounter it during parsing and we're not in WebAsm target.

clang/lib/Basic/Targets/DirectX.h
45

Also apply to the rest of the targets. Also, why is there only 1 added here? I thought we had 2 address spaces for Wasm?

clang/lib/Basic/Targets/WebAssembly.h
44–45

That DEFINITELY looks suspicious to me as well, I noted above too, but either we need to specify we're re-using the hlsl_groupshared for wasm_externref address space (at which point, i wonder: why not re-use a different address space?), or correct this.

clang/lib/Parse/ParseDecl.cpp
849
clang/lib/Sema/SemaChecking.cpp
6546

Does this diagnose? Should we be emitting an error here?

6558

All those comments don't really seem relevant here, and are not really correct. The only thing SemaChecking should do is either modify its arguments (though you don't have any), or set its return type.

6561

This appears to be a 'custom type checking' builtin, where the code in Sema has to check the arguments (rather than the automatic behavior working), and set its return type to the 'correct' one. It appears that this accepts no arguments, then changes the result of the builtin.

So this doesn't seem particularly odd, other than the strange comments above.

clang/lib/Sema/SemaType.cpp
7255

This name seems misleading... should you be desugaring 'Type' to store it in 'Desugared'?

Also, I see that this value is never used, so why do the copy init? ALSO, why is it called Desugared, when it never seems to get a Desugared type?

aaron.ballman added inline comments.Jan 11 2023, 8:18 AM
clang/lib/Sema/SemaChecking.cpp
6561

Oh yeah that's right, that is how the custom checking code works, thank you for the reminder!

pmatos updated this revision to Diff 488645.Jan 12 2023, 7:29 AM

@aaron.ballman Added support for externref mangling in Microsoft. This follows the pattern implemented by OpenCL types.

@aaron.ballman Added support for externref mangling in Microsoft. This follows the pattern implemented by OpenCL types.

Wrong revision - sorry.

erichkeane added inline comments.Jan 12 2023, 7:31 AM
clang/lib/AST/MicrosoftMangle.cpp
2481 ↗(On Diff #488645)

I'm not sure what the PA is supposed to mean? I see some uses of mangleArtificialTagType don't do anything with it, so I fear it is something OCL specific.

pmatos updated this revision to Diff 488646.Jan 12 2023, 7:33 AM

Undo last patch. Submitted change to incorrect revision.

pmatos marked 15 inline comments as done.Jan 13 2023, 4:48 AM
pmatos added inline comments.
clang/include/clang/Basic/Attr.td
4054

Documentation says about Subject:

// The things to which an attribute can appertain
SubjectList Subjects;

Given this can be attached to function pointer types, is the subject then just Type ?

clang/include/clang/Basic/AttrDocs.td
6855 ↗(On Diff #487700)

That's a mistake due to how things worked in an earlier revision. Thanks for spotting.

clang/include/clang/Basic/TokenKinds.def
666

Thanks for the comments, I have now updated this with an implementation of option 2 and added a diagnostic test.

clang/lib/Basic/Targets/DirectX.h
45

Externref is a type in clang, doesn't need its own address space in clang, only when it's lowered to a opaque type in llvm it gets its own address space.

erichkeane added inline comments.Jan 13 2023, 6:38 AM
clang/include/clang/Basic/Attr.td
4054

IIRC, the list of subjects are the Decls and Stmt types that are possibly allowed. I don't thnk it would be just 'Type' there.

As you have it, it is only allowed on TypedefNameDecl.

See the SubsetSubject's at the top of this file for some examples on how you could constrain a special matcher to only work on function pointer decls.

clang/lib/Basic/Targets/DirectX.h
45

Please fix the comment in my suggestion.

Also, your explanation doesn't make it clear to me why you added TWO entries to this list for other targets, but only 1 here.

pmatos updated this revision to Diff 489031.Jan 13 2023, 8:50 AM
pmatos marked 2 inline comments as done.

Address a few comments, add new test for funcref keyword diagnostic.

pmatos updated this revision to Diff 489552.Jan 16 2023, 7:25 AM

Add new Attr Subject Function Pointer for __funcref attribute.

pmatos added inline comments.Jan 16 2023, 7:26 AM
clang/include/clang/Basic/Attr.td
4054

Good point @erichkeane . What do you think of FunctionPointer implementation here?

erichkeane added inline comments.Jan 17 2023, 6:48 AM
clang/include/clang/Basic/Attr.td
4054

That seems acceptable to me.

pmatos marked an inline comment as done.Jan 18 2023, 1:20 AM
pmatos updated this revision to Diff 493251.Jan 30 2023, 2:27 AM

Rebase on main.

Some more comments, but there are unaddressed comments from earlier reviews as well.

clang/include/clang/Basic/Attr.td
4054

That makes sense to me as well, but FWIW, type attributes don't currently have much of any tablegen automation (unlike decl and stmt attributes), so the subject list isn't strictly necessary here. But it's still appreciated because 1) it's documentation, and 2) we want to tablegen more and this helps us with that goal.

clang/include/clang/Basic/DiagnosticSemaKinds.td
11686
clang/lib/AST/DeclBase.cpp
1060–1063 ↗(On Diff #493251)
clang/lib/Parse/ParseDecl.cpp
856–857

Be sure to run clang-format over the patch to fix this sort of thing.

pmatos marked 11 inline comments as done.Feb 1 2023, 5:14 AM
pmatos added inline comments.
clang/lib/Basic/Targets/DirectX.h
45

I have now fixed that. We just need one address space.

clang/lib/Basic/Targets/WebAssembly.h
44–45

Fixed this now. We only need the address space for funcref. The address space for externref is now necessary as it's its own type.

pmatos marked an inline comment as done.Feb 1 2023, 5:34 AM
pmatos added inline comments.
clang/include/clang/Basic/Attr.td
4054

@erichkeane Yes, you're right. Something like

using IntIntFuncref = int(*)(int) __funcref;

doesn't work. Why is this not accepted? Is it due to the Subjects listed?

pmatos updated this revision to Diff 493919.Feb 1 2023, 5:43 AM
pmatos marked an inline comment as done.

Address some of the pending comments.

pmatos marked 6 inline comments as done.Feb 13 2023, 5:13 AM
pmatos added inline comments.
clang/lib/Sema/SemaChecking.cpp
6549–6552

If I understand your question correctly, this is just the semantic checks for each of the builtins.

6554–6560

Right - fixed.

6558

Right, these comments can be removed. I fixed these.

6561

I think this question is not relevant anymore given the current code, right? The wasm ref null builtin will always return the null funcref.

clang/lib/Sema/SemaType.cpp
7255

Bad naming due to earlier code. I have simplified this code now.

pmatos updated this revision to Diff 496937.Feb 13 2023, 5:14 AM
pmatos marked 5 inline comments as done.

Addressed a few comments.

pmatos marked 4 inline comments as done.Feb 13 2023, 8:31 AM
pmatos added inline comments.
clang/lib/Sema/SemaChecking.cpp
6546

I think it makes sense. Will add a test as well for the diagnostic.

6554–6558

I have added a test to check this situation.

pmatos updated this revision to Diff 497002.Feb 13 2023, 8:31 AM
pmatos marked 2 inline comments as done.

Further refinement of the patch with more diagnostics tested.

pmatos updated this revision to Diff 497004.Feb 13 2023, 8:33 AM

Remove unnecessary code.

pmatos marked 2 inline comments as done.Feb 13 2023, 8:33 AM
pmatos added inline comments.
clang/lib/Sema/SemaType.cpp
7269

That's correct - removed.

pmatos marked 2 inline comments as done.Feb 14 2023, 7:39 AM

@aaron.ballman I have finished addressing all the concerns on this patch. Do you have any further comments?

clang/lib/Sema/SemaType.cpp
7289–7290

We'll just be overriding the address space in this case. Although, I am not sure something like this can happen in Wasm given there's only one linear memory.

pmatos updated this revision to Diff 497632.Feb 15 2023, 4:20 AM

Rebased on current HEAD.

Generally LGTM with a few comments sprinkled around.

clang/include/clang/Basic/Attr.td
4054

Not due to the subjects -- keyword attributes require manual parsing and type attributes require manual processing (we don't tablegen as much as I'd like for type attributes). But that code should be accepted (you should add a test case for it as well).

clang/include/clang/Basic/AttrDocs.td
6930 ↗(On Diff #497632)

DocCatType, right?

6933 ↗(On Diff #497632)

"Function pointer declaration" reads a bit odd to me, I think you mean "function pointer type", right?

e.g.,
void (*)() __funcref is fine, as is void (*fp)() __funcref -- the attribute appertains to the type, not to the declaration?

clang/lib/Sema/SemaType.cpp
7247

Just to avoid confusing editors that can't tell Type is a variable and not clang::Type the type.

clang/test/Sema/wasm-refs.c
50 ↗(On Diff #497632)

Should we also test two-level application of the qualifier, as in?

using uh_oh = funcref_t __funcref;
pmatos marked 4 inline comments as done.Feb 28 2023, 3:40 AM

Thanks @aaron.ballman . I think I have now address all outstanding comments.

clang/include/clang/Basic/Attr.td
4054

Seems to be fine now. Test added.

clang/include/clang/Basic/AttrDocs.td
6930 ↗(On Diff #497632)

Yes.

clang/test/Sema/wasm-refs.c
50 ↗(On Diff #497632)

Double qualification shouldn't be an issue - adding this to SemaCXX/wasm-funcref.cpp.

pmatos updated this revision to Diff 501075.Feb 28 2023, 3:41 AM
pmatos marked 3 inline comments as done.

Patch with all outstanding comments addressed.

pmatos updated this revision to Diff 501081.Feb 28 2023, 4:02 AM

Add forgotten comment to test.

pmatos added a comment.Mar 8 2023, 1:41 AM

@erichkeane @aaron.ballman any further comments on this?

First, we've been dealing with the 16.0 release process/regressions, which unfortunately got us pretty far behind on reviews. Sorry about that!

I have only 1 concern here, otherwise, this LGTM. I'll leave it to Aaron to make the final 'value judgement' of if we want to keep this, so I'll leave it to him to Approve.

clang/lib/AST/DeclBase.cpp
1056 ↗(On Diff #501081)

probably want to canonicalize here (and above). Else typedefs-to-typedefs/etc might getcha.

aaron.ballman accepted this revision.Mar 14 2023, 7:25 AM

LGTM aside from some small nits that you can fix when landing. Thank you for your patience on this review!

clang/lib/AST/DeclBase.cpp
1056 ↗(On Diff #501081)

Yeah, I'd do that below on the return, as in: return Ty.getCanonicalType()->isFunctionPointerType();

clang/test/SemaCXX/wasm-funcref.cpp
14 ↗(On Diff #501081)

Please add a newline to the end of the file.

This revision is now accepted and ready to land.Mar 14 2023, 7:25 AM
pmatos marked 3 inline comments as done.Mar 17 2023, 9:08 AM

Thanks all for the comments, fixed and will soon land after running a check-all.

This revision was landed with ongoing or failed builds.Mar 17 2023, 10:32 AM
This revision was automatically updated to reflect the committed changes.
MyDeveloperDay added inline comments.
clang/lib/Format/FormatToken.h
576

Is there a unit test in clang format for this change?