[clang] Ignore GCC 11 [[malloc(x)]] attribute

[aloisklink]

Ignore the [[malloc(x)]] or [[malloc(x, 1)]] function attribute syntax added in GCC 11.

Unlike [[malloc]] with no arguments (which is supported by Clang), GCC uses the one or two argument form to specify a deallocator for GCC's static analyzer.

Code currently compiled with [[malloc(x)]] or __attribute__((malloc(x))) fails with the following error: 'malloc' attribute takes no arguments.

Fixes: #51607
Partial-Bug: #53152 (this PR only ignores __attribute__((malloc(x))) and allows it to compile, so only partially fixes the problem).

---

In the future, we can add this attribute to the AST as well. This will let us improve the clang static analyzer's unix.MismatchedDeallocator checker.

[tbaederr]

Is it really ignored or is it just treated as if it had no arguments?

[aloisklink]

In this PR, it's completely ignored, it's not even added to the Clang AST.

It just reaches the end of this function where nothing happens (let me know if I can improve the comment!):

https://github.com/llvm/llvm-project/blob/f9c914729a5f5ac7f8b61ea2d39509ff0236a228/clang/lib/Sema/SemaDeclAttr.cpp#L2084-L2087

We can't treat it as if has no arguments because in GCC it means two different things:

• __attribute__((malloc)) with no args means the return value is guaranteed not to alias to any other pointer (aka like __declspec(restrict) in MSVC) and that the function will normally return non-NULL, allowing optimizations,
• __attribute__((malloc(deallocator))) instead says the returned pointer should be deallocated by the specified deallocator, allowing a static analyzer to print warnings.

See the malloc section of https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html for more info.

To be honest, I feel like GCC should have given the one/two argument form a different attribute name to make things less confusing, but GCC 11 has already been out for years unfortunately.

[erichkeane]

I think I'd like this to be diagnosed. We shouldn't be silently ignoring the attribute.

[aloisklink]

I slightly disagree, since even if we do add support for this attribute, only the Clang analyzer would use it, but I've added a 'malloc' attribute ignored because Clang does not support the one/two argument form warning in commit a76561f.

[AaronBallman]

Thank you for the fix!

We generally want ignored attributes to be diagnosed as being ignored; otherwise users have a much harder time determining whether the attribute is working or not. I think we should issue an "attribute ignored" warning when we're dropping the attribute in the AST.

Also, the changes should come with a release note so that users know about the bug fix.

[aloisklink]

We generally want ignored attributes to be diagnosed as being ignored; otherwise users have a much harder time determining whether the attribute is working or not. I think we should issue an "attribute ignored" warning when we're dropping the attribute in the AST.

Personally, I think there shouldn't be a warning, because even if we add the attribute to the AST, Clang will do nothing with it; the attribute would only be used by Clang's static analyzer. But I've added a warning anyway in a76561f.

I think a generic 'malloc' attribute ignored warning would have been pretty confusing, (especially because most of the time, people use both the no-argument form and the one/two argument form on the same function, e.g. __attribute__((malloc, malloc(my_cleanup))) void *my_func();) so instead I've made a custom warning that says: 'malloc' attribute ignored because Clang does not support the one/two argument form.

Also, the changes should come with a release note so that users know about the bug fix.

👍 That makes sense! I've added a note in the Bug Fixes to Attribute Support section in a76561f.

[shafik]

Just a nit

[shafik]

Suggested change

	let Args = [IdentifierArgument<"Deallocator", /*opt*/ 1>,
	ParamIdxArgument<"DeallocatorPtrArgIndex", /*opt*/ 1>];
	let Args = [IdentifierArgument<"Deallocator", /*opt=*/1>,
	ParamIdxArgument<"DeallocatorPtrArgIndex", /*opt=*/ 1>];

[shafik]

Nit: Unfortunately we have not been consistent through our codebase in argument comment format but e should strive to be consistent with bugprone-argument-comment

[aloisklink]

👍 Fixed in b7e1258

[AaronBallman]

Suggested change

	(`#51607 <https://github.com/llvm/llvm-project/issues/51607>`).
	(`#51607 <https://github.com/llvm/llvm-project/issues/51607>`_).

[aloisklink]

Sorry! That's my fault for being lazy and skipping build the docs! Fixed in 5f6f893

[AaronBallman]

Suggested change

	def warn_multiarg_malloc_attribute_ignored: Warning<
	"'malloc' attribute ignored because"
	" Clang does not support the one/two argument form">,
	InGroup<IgnoredAttributes>;
	def warn_attribute_form_ignored : Warning<
	"%0 attribute ignored; Clang does not yet support this attribute signature">,
	InGroup<IgnoredAttributes>;

If we generalize the wording a bit, we can reuse this diagnostic for other circumstances where we don't yet support a particular signature.

Also, this diagnostic should be moved to DiagnosticSemaKinds.td (it's only diagnosed from clang/lib/Sema, so no need for it to be in the "common" diagnostics file for cross-lib diagnostics).

[aloisklink]

Fixed in 02a153d, although I've very slightly changed the wording to match other warnings by using because instead of the ;.

I've also added a couple of other error diagnostics in other commit, so please let me know if you think it's worth generalizing them too.

[AaronBallman]

One more test I'd like to see is:

inline __attribute__((malloc(1))) void *func(void) // error: '1' does not name a function

[aloisklink]

Fixed in 0659620, although with a different error diagnostic. I've also added a bunch more error checking (adapting the code from handleCleanupAttr), so now we should even be stricter than GCC on what args we accept:

llvm-project/clang/test/Sema/attr-args.c

Lines 2 to 6 in 0659620

	int a;
	void func_a(void * ptr, int a);
	void func_b(int a);
	void __attribute__((overloadable)) ambigious_func(void *); // expected-note {{candidate function}}
	void __attribute__((overloadable)) ambigious_func(void *, int); // expected-note {{candidate function}}

llvm-project/clang/test/Sema/attr-args.c

Lines 13 to 21 in 0659620

	inline __declspec(restrict(a)) void *f6_a(void); // expected-error {{'restrict' attribute takes no arguments}}
	inline __attribute__((malloc(func_a, 1, a))) void *f6_b(void); // expected-error {{'malloc' attribute takes no more than 2 arguments}}
	inline __attribute__((malloc(func_a, 1))) void *f6_c(void); // expected-warning {{'malloc' attribute ignored because Clang does not yet support this attribute signature}}
	inline __attribute__((malloc(1234))) void *f6_d(void); // expected-error {{'malloc' argument for deallocator is not a function}}
	inline __attribute__((malloc(a))) void *f6_e(void); // expected-error {{'malloc' argument 'a' is not a function}}
	inline __attribute__((malloc(ambigious_func))) void *f6_f(void); // expected-error {{'malloc' argument 'ambigious_func' is not a single function}}
	inline __attribute__((malloc(func_b))) void *f6_g(void); // expected-error {{'malloc' argument 'func_b' must take a pointer type as its first argument}}
	inline __attribute__((malloc(func_a, 3))) void *f6_h(void); // expected-error {{'malloc' attribute parameter 2 is out of bounds}}
	inline __attribute__((malloc(func_a, 2))) void *f6_i(void); // expected-error {{'malloc' argument '2' refers to non-pointer type 'int' of 'func_a'}}

[gustedt]

I see that the fix is almost ready, good. But generally it would also have helped if the __has_c_attribute feature test for this type of borrowed attributes would provide means to distinguish different versions. Here gcc as well as clang only have the value 1. So if the patch would also change the return value for clang to the year whenever the first version of gcc-11 with that feature was released, that would really be helpful.

[AaronBallman]

I see that the fix is almost ready, good. But generally it would also have helped if the __has_c_attribute feature test for this type of borrowed attributes would provide means to distinguish different versions. Here gcc as well as clang only have the value 1. So if the patch would also change the return value for clang to the year whenever the first version of gcc-11 with that feature was released, that would really be helpful.

Yeah, our current default behavior is that we use the value 1 for almost any vendor-specific attribute. It would be a massive undertaking to try to figure out a consistent date-based value for each of our attributes, and it would be especially complex given that we often have subtle (and sometimes intentional) differences between our implementation and the original implementation. I don't think your suggestion is a bad one, but it's not clear it's workable in practice either. That said, it might be nice for us to have some sort of policy about feature testing changes in semantics of attributes more generally (CC @erichkeane for awareness).

[erichkeane]

I see that the fix is almost ready, good. But generally it would also have helped if the __has_c_attribute feature test for this type of borrowed attributes would provide means to distinguish different versions. Here gcc as well as clang only have the value 1. So if the patch would also change the return value for clang to the year whenever the first version of gcc-11 with that feature was released, that would really be helpful.

Yeah, our current default behavior is that we use the value 1 for almost any vendor-specific attribute. It would be a massive undertaking to try to figure out a consistent date-based value for each of our attributes, and it would be especially complex given that we often have subtle (and sometimes intentional) differences between our implementation and the original implementation. I don't think your suggestion is a bad one, but it's not clear it's workable in practice either. That said, it might be nice for us to have some sort of policy about feature testing changes in semantics of attributes more generally (CC @erichkeane for awareness).

I'd actually been thinking about this all morning, and I don't see how we could do it in a way that wouldn't make the world a worse place. The reason feature-test-macros work is because there is an agreed upon 'source' of those values, without Clang and GCC agreeing on a value here (which we do in C++ via SD10), anything we do is going to be no better than just compiler version checks unfortunately.

[gustedt]

Hi, yes the really bad choice here is by gcc to have the same name for basically two different attributes. For the value, they also missed the opportunity to do something sensible when moving to C++ attributes, what a pitty. For a concrete guideline in that jungle, when (= which version) do you expect this patch to hit distribution? Will it be in clang-18 once that is released? Can we expect a similar feature, let's Call it clang::deallocator_function in a future version? Thanks Jens -- Jens Gustedt - INRIA & ICube, Strasbourg, France

[erichkeane]

Hi, yes the really bad choice here is by gcc to have the same name for basically two different attributes. For the value, they also missed the opportunity to do something sensible when moving to C++ attributes, what a pitty. For a concrete guideline in that jungle, when (= which version) do you expect this patch to hit distribution? Will it be in clang-18 once that is released? Can we expect a similar feature, let's Call it clang::deallocator_function in a future version? Thanks Jens -- Jens Gustedt - INRIA & ICube, Strasbourg, France

It does appear that this is on track for clang 18, but it hasn't finished review yet, so I cannot promise that of course. At the moment, nothing like a clang::deallocator_function is proposed, so unless someone does the work to implement and propose it, we don't have a plan to do so.

[aloisklink]

I've fixed reviewer comments! Sorry for the delay! I didn't have much time, and my PC isn't the fastest, so building Clang + regression tests takes a while!

As recommended by #68059 (comment), I added type checking for the attribute arguments so that we should throw an error on any invalid args that GCC throws an error (or even a warning on). Since this is a pretty substantial change, I made it all as fixup! commits to make the changes easier to review.

---

gcc to have the same name for basically two different attributes

I agree, it's not ideal! Having a separate name for this attribute (e.g. [[dealloc(func)]] or [[deallocator(func)]] would be my preference. Although I think it might be worth asking the GCC team to implement it first. After all, this PR doesn't actually add support for the [[malloc(deallocator)]] attribute form in Clang, it just ignores it, instead of throwing an error.