sdesmalen added inline comments.
================
Comment at: clang/test/Sema/aarch64-sme-func-attrs.c:181
+void non_za_definition(void) {
+ sme_arm_new_za(); // OK
----------------
rsandifo-arm wrote:
> sdesmalen wrote:
> > rsandifo-arm wrote:
> > > Would be good to have some tests for indirect function calls too (via
> > > function pointers), to make sure that the diagnostic still works when no
> > > decl is available.
> > >
> > > I suppose this applies to D157269 too.
> > I'm not sure that's necessary because D127762 already added tests to ensure
> > the attributes propagate on pointer types, which then sets the ExtProtoInfo
> > for those values. This patch merely checks the SME attribute fields from
> > ExtProtoInfo. i.e. there is already nothing depending on a declaration
> > being available.
> But `Sema::checkCall` does have some tests that depend on the decl rather
> than the type. So the purpose of the test wouldn't be “does the attribute
> stick when applied to indirect function types?” (which I agree is already
> covered), but “does the new code correctly process the attribute on the
> target of a function pointer type?”
The declaration is only relevant for the call-site, not the callee.
if (ExtInfo.AArch64SMEAttributes & FunctionType::SME_PStateZASharedMask) {
The above line checks __arm_shared_za attribute of the callee (could be a decl,
or a function pointer, but in either case is a prototyped function with the
propagated attributes)
if (auto *CallerFD = dyn_cast<FunctionDecl>(CurContext)) {
The above line checks if the call-site context is a FunctionDecl (or definition
for that matter). If the call is not part of a declaration (e.g. it's part of
some global initialiser), we know it cannot have any live ZA state (which I now
realise is missing a test-case).
So I think that a test like this:
__arm_new_za void foo(void (*f)() __arm_shared_za) { f(); }
is not testing anything that isn't already tested. But perhaps I'm still
misunderstanding your point. If so, could you give an example of a test you
have in mind?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D157270/new/
https://reviews.llvm.org/D157270
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
