[WebAssembly] Initial support for reference type funcref in clang

[pmatos]

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.

[pmatos]

clang/include/clang/Basic/Attr.td

4043

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 ↗

(On Diff #446726)

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

37

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]

clang/include/clang/Basic/Attr.td

4043

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

37

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]

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

[pmatos]

Add new Attr Subject Function Pointer for __funcref attribute.

[pmatos]

clang/include/clang/Basic/Attr.td

4043

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

[erichkeane]

clang/include/clang/Basic/Attr.td

4043

That seems acceptable to me.

[pmatos]

Rebase on main.

[aaron.ballman]

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

clang/include/clang/Basic/Attr.td

4043

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

11664

clang/lib/AST/DeclBase.cpp

1060–1063 ↗

(On Diff #493251)

clang/lib/Parse/ParseDecl.cpp

854–855 ↗

(On Diff #493251)

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

[pmatos]

clang/lib/Basic/Targets/DirectX.h

37

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

clang/lib/Basic/Targets/WebAssembly.h

44–45 ↗

(On Diff #487700)

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]

clang/include/clang/Basic/Attr.td

4043

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

Address some of the pending comments.

[pmatos]

clang/lib/Sema/SemaChecking.cpp

6494–6497

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

6499–6505

Right - fixed.

6503

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

6506

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

7239

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

[pmatos]

Addressed a few comments.

[pmatos]

clang/lib/Sema/SemaChecking.cpp

6491

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

6499–6503

I have added a test to check this situation.

[pmatos]

Further refinement of the patch with more diagnostics tested.

[pmatos]

Remove unnecessary code.

[pmatos]

clang/lib/Sema/SemaType.cpp

7253

That's correct - removed.

[pmatos]

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

clang/lib/Sema/SemaType.cpp

7273–7274

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]

Rebased on current HEAD.

[aaron.ballman]

Generally LGTM with a few comments sprinkled around.

clang/include/clang/Basic/Attr.td

4043

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

7231

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]

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

clang/include/clang/Basic/Attr.td

4043

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]

Patch with all outstanding comments addressed.

[pmatos]

Add forgotten comment to test.

[pmatos]

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

[erichkeane]

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]

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.

[pmatos]

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

[MyDeveloperDay]

clang/lib/Format/FormatToken.h

615 ↗

(On Diff #506134)

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