llvm / llvm-project

The LLVM Project is a collection of modular and reusable compiler and toolchain technologies.
http://llvm.org
Other
26.77k stars 10.97k forks source link

`returns_nonnull` cannot be applied to function pointers #81350

Open yshui opened 4 months ago

yshui commented 4 months ago

Example:

https://godbolt.org/z/azrsxE7o8

It works fine on function declarations.

llvmbot commented 4 months ago

@llvm/issue-subscribers-clang-frontend

Author: Yuxuan Shui (yshui)

Example: https://godbolt.org/z/azrsxE7o8 It works fine on function declarations.
shafik commented 4 months ago

It is not clear to me where this diagnostic is being generated from, I see generated AttrParsedAttrImpl.inc but I can't really trace it back to something else.

So I can't really see the git history.

CC @AaronBallman @erichkeane

AaronBallman commented 4 months ago

The diagnostic comes from https://github.com/llvm/llvm-project/blob/8373ceef8f2ee377d6daf884e2f3ea11408a7fe2/clang/include/clang/Basic/Attr.td#L2121 because the attribute is a declaration attribute, not a type attribute.

GCC doesn't seem to treat it as a type attribute either: https://godbolt.org/z/s9Kesrzbv, those are the semantics I would expect out of a declaration attribute.

It's a bit unclear whether this is a bug in GCC or by design. My guess is that it's by design, but I'm not certain we want to confuse appertainment in the same way.

yshui commented 4 months ago

i guess it's because it can also be used on a function, which is a declaration?

if returns_nonnull is also a type attribute, won't it become ambiguous whether it's annotating the function pointer, or the return type?

AaronBallman commented 4 months ago

i guess it's because it can also be used on a function, which is a declaration?

if returns_nonnull is also a type attribute, won't it become ambiguous whether it's annotating the function pointer, or the return type?

GNU-style attributes "slide" around to whatever makes the most sense for them to apply to, so it's ambiguous there but we have latitude to apply it to whatever we want. The [[]]-style attribute has very strict rules about what it applies to based on the syntactic position of the attribute. Vendor-specified attributes can behave however they want in terms of their type effects, so GCC is conforming, but I think it's also a very confused design because users expect type attributes to apply to the type, so things like type mismatches would be diagnosed. It's possible GCC is missing a diagnostic there, but it sure seems like this isn't a real type attribute because it doesn't behave like one: https://godbolt.org/z/hjzK9qKoe

erichkeane commented 4 months ago

Yeah, based on Aaron's analysis, I think that this is more of a case of GCC missing the diagnostic, rather than us diagnosing when they shouldn't. I couldn't come up with any examples where the attribute on a function pointer means anything here. I lean towards this being 'not a defect'.

yshui commented 4 months ago

@erichkeane no, this is not the case. as this attribute on function pointer changes optimization: https://godbolt.org/z/r6fooon13

IIUC, I think Aaron's complaint here is that gcc is treating this attribute as a declaration attribute, so you can assign a returns_nonnull fp to another fp which is not returns_nonnull. and I think his argument is that when applied to a fp, returns_nonnull should be a type attribute.

erichkeane commented 4 months ago

@erichkeane no, this is not the case. as this attribute on function pointer changes optimization: https://godbolt.org/z/r6fooon13

IIUC, I think Aaron's complaint here is that gcc is treating this attribute as a declaration attribute, so you can assign a returns_nonnull fp to another fp which is not returns_nonnull. and I think his argument is that when applied to a fp, returns_nonnull should be a type attribute.

That Opt behavior isn't because of the returns_nonnull on the function pointer, you can see that the functions are identical if you remove the assignment to 'foo' (that is, there is something about the assignment that is impacting opt). But that is a byproduct of it being a declaration attribute, it is not part of the 'function type' signature, so the pointer doesn't really have ability to propagate that information.

See: https://godbolt.org/z/ecfKTeqo1

yshui commented 4 months ago

@erichkeane huh, this is indeed very weird. but i am leaning towards gcc being inconsistent. because as you can see here: https://godbolt.org/z/q587G5dE7 all other things being equal, the code gen is different in at least some cases. which i think is ample proof that this attribute definitely has semantics on function pointers.

the type attribute vs declaration attribute question i think is a separate issue. i agree that this information not propagating is weird, so i don't have an opinion which it should be.

but the bottom line is, when i put a returns_nonnull on a function pointer, i think it's pretty clear what my intent is. i think gcc understands that intent, maybe not consistently. and i think it's useful for clang to support that as well.

erichkeane commented 4 months ago

@erichkeane huh, this is indeed very weird. but i am leaning towards gcc being inconsistent. because as you can see here: https://godbolt.org/z/q587G5dE7 all other things being equal, the code gen is different in at least some cases. which i think is ample proof that this attribute definitely has semantics on function pointers.

the type attribute vs declaration attribute question i think is a separate issue. i agree that this information not propagating is weird, so i don't have an opinion which it should be.

but the bottom line is, when i put a returns_nonnull on a function pointer, i think it's pretty clear what my intent is. i think gcc understands that intent, maybe not consistently. and i think it's useful for clang to support that as well.

Yeah, i am having a hard time understanding why the assignment matters there.

I'll disagree that GCC DOES understand the intent, but just sorta gets lucky some times!

I WILL agree that I think we can do better about this case, and would welcome a patch that gave sane behavior to it. I would think that even allowing it to stay as a 'decl' attribute and having it slide would be fine, then just calls/accesses through that function pointer declaration would gain the behavior (and thus, staying out of the type system). We'd likely end up with some pretty ugly assignment behavior, but that would better reflect what GCC is doing anyway.