Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Avoid migrating existing patches. Phabricator shutdown timeline

[WebAssembly] Initial support for reference type funcref in clang

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



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 marked 15 inline comments as done.Jan 13 2023, 4:48 AM
pmatos added inline comments.

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 ?

6855 ↗(On Diff #487700)

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


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


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

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.


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

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

erichkeane added inline comments.Jan 17 2023, 6:48 AM

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.


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.

1060–1063 ↗(On Diff #493251)

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.

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


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.

@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.

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


Right - fixed.


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


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


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.

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


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.

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?


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.


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).

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?

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


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

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.


Seems to be fine now. Test added.

6930 ↗(On Diff #497632)


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.

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!

1056 ↗(On Diff #501081)

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

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.

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