Add malloc-specific attributes to our standard headers

[pfg]

Summary

Introduce two gcc attributes (also recognized by clang) to
stdlib.h:

warn_unused_result obviously good for checking the code is correct.
alloc_size GCC uses this information to improve the

			correctness of __builtin_object_size.

Both are supported by modern clang.

The specific attributes were inspired by bionic's libc (which uses
dlmalloc).

This should have some performance positive impact but it is very
difficult to quantify and should also help the static checker.

A proof of concept patch to add the same flags to jemalloc's build
has been submitted upstream and is likely to be accepted.

Test Plan

make tinderbox (on going - so far so good).
Run it for a while with some ports

[pfg]

Hmm.. this breaks the build in xlint :( :
...
stdlib.h(90): syntax error [249]
stdlib.h(97): syntax error [249]
stdlib.h(104): syntax error [249]
llib-lposix(84): redeclaration of calloc [27]
llib-lposix(190): redeclaration of malloc [27]
llib-lposix(221): redeclaration of realloc [27]

[ed]

Although __warn_unused_result is a long attribute name, I'd rather not abbreviate it to __wur. It's not obvious what it means. The name itself is also largely inconsistent. Right now all of the annotations we have are named in such a way that the describe the semantics, as opposed to describing what the compiler is allowed to do.

"This function is pure" instead of "Optimize away unneeded calls".
"This function behaves like an allocator" instead of "Assume return value never aliases".
"This function requires a lock to be held" instead of "Raise a compiler warning if not unlocked".

It would be nice if we had a name that used a similar convention. We need something like __return_value_should_be_processed, but then with a saner name. Thoughts?

[pfg]

In D2107#5, @ed wrote:

Although __warn_unused_result is a long attribute name, I'd rather not abbreviate it to __wur. It's not obvious what it means. The name itself is also largely inconsistent. Right now all of the annotations we have are named in such a way that the describe the semantics, as opposed to describing what the compiler is allowed to do.

The __wur thing came from google's bionic. Perhaps just __warn_unused ?

I spend some time updating xlint (from NetBSD) and I'll come back to this.

[op]

The __warn_unused has different meaning, in my reading that means, that every function which marked with __warn_unused must be used somewhere.
What do you think about __w_unused_res?

[pfg]

In D2107#7, @opntr wrote:

The __warn_unused has different meaning, in my reading that means, that every function which marked with __warn_unused must be used somewhere.
What do you think about __w_unused_res?

I dislike such contractions __warn_result ?

[ed]

In D2107#8, @pfg wrote:

In D2107#7, @opntr wrote:

The __warn_unused has different meaning, in my reading that means, that every function which marked with __warn_unused must be used somewhere.
What do you think about __w_unused_res?

I dislike such contractions __warn_result ?

As I mentioned previously, I dislike such naming, as it is too much focused on the end result.

What about __result_mandatory?

[pfg]

In D2107#9, @ed wrote:

In D2107#8, @pfg wrote:

In D2107#7, @opntr wrote:

The __warn_unused has different meaning, in my reading that means, that every function which marked with __warn_unused must be used somewhere.
What do you think about __w_unused_res?

I dislike such contractions __warn_result ?

As I mentioned previously, I dislike such naming, as it is too much focused on the end result.

What about __result_mandatory?

That would indicate an error, while's it's only a warning.
Next color for bikeshed __w_unused_result . ;)

[op]

In D2107#10, @pfg wrote:

In D2107#9, @ed wrote:

In D2107#8, @pfg wrote:

In D2107#7, @opntr wrote:

The __warn_unused has different meaning, in my reading that means, that every function which marked with __warn_unused must be used somewhere.
What do you think about __w_unused_res?

I dislike such contractions __warn_result ?

As I mentioned previously, I dislike such naming, as it is too much focused on the end result.

What about __result_mandatory?

That would indicate an error, while's it's only a warning.
Next color for bikeshed __w_unused_result . ;)

Or use the best parts from both: __w_result_mandatory

[pfg]

In D2107#11, @opntr wrote:

In D2107#10, @pfg wrote:

In D2107#9, @ed wrote:

In D2107#8, @pfg wrote:

In D2107#7, @opntr wrote:

The __warn_unused has different meaning, in my reading that means, that every function which marked with __warn_unused must be used somewhere.
What do you think about __w_unused_res?

I dislike such contractions __warn_result ?

As I mentioned previously, I dislike such naming, as it is too much focused on the end result.

What about __result_mandatory?

That would indicate an error, while's it's only a warning.
Next color for bikeshed __w_unused_result . ;)

Or use the best parts from both: __w_result_mandatory

That is longer than the original __warn_unused_result

[pfg]

This should work. (Going through buildworld ATM)

[ed]

In D2107#10, @pfg wrote:

What about __result_mandatory?

That would indicate an error, while's it's only a warning.

No, it wouldn't. There are other attributes in <sys/cdefs.h> that use a similar naming scheme that only raise a compiler warning by default.

[pfg]

In D2107#15, @ed wrote:

In D2107#10, @pfg wrote:

What about __result_mandatory?

That would indicate an error, while's it's only a warning.

No, it wouldn't. There are other attributes in <sys/cdefs.h> that use a similar naming scheme that only raise a compiler warning by default.

Hmm ... none of them has the "mandatory" word on it. I also find it counter-intuitive to change the name so much. FWIW, googling at other projects, in llvm they use __has_attribute(warn_unused_result) , on glibc they use __wur.

FWIW, I am still having issues with xlint so I am trying another patch.

[pfg]

The alloc_size attribute was somewhat tricky for xlint.
This version survives buildworld.

I will wait a while before committing this to sort out
the remaining naming issue (which is not too important IMHO).

[ed]

In D2107#16, @pfg wrote:

In D2107#15, @ed wrote:

In D2107#10, @pfg wrote:

What about __result_mandatory?

That would indicate an error, while's it's only a warning.

No, it wouldn't. There are other attributes in <sys/cdefs.h> that use a similar naming scheme that only raise a compiler warning by default.

Hmm ... none of them has the "mandatory" word on it.

But at least one of them has the word 'requires' in it, even though it only throws a compiler warning by default.

Still, my remark was not about the words or synonyms of words that are used. It's about the grammatical style. All of the existing keywords that are used as annotations can be used in a sentence that uses 'this function', 'this structure', 'this variable', etc. as the subject:

• This function is pure.
• This function requires lock $x.
• This function is malloc-like.
• This function is printf-like.
• This function is a strong reference to $x.
• This structure is packed.
• This variable is placed in section $x.
• This variable is used (or unused).

Adding 'warn' to the name implies that it's the function that somehow 'generates a warning', even though it's the compiler that does that -- not the function.

Ask yourself the question: why do we want to throw a warning if we don't use the result of this function? Well, it's actually more related to __pure2 than you think. Even though functions like malloc() are not pure in the sense that multiple calls can be folded into one or the compiler can even call it more often than requested, it has no side-effects in the sense that it does not cause a state transformation that is visible to the process. It is just a very weak form of __pure2.

The same holds for a function like arc4random(). It is clearly not a pure function, as individual calls cannot be merged. Also, every invocation alters the internal state of the randomizer. Still, state that is visible towards the caller is unaffected, as you wouldn't know what arc4random() would return anyway. That is the reason why you should capture the return value.

Maybe it should be called something in the direction of __no_observable_side_effects.

What's interesting about this is that in addition to allowing more diagnostics, there are actually some interesting optimizations we could make if we know which functions have no observable side effects. There is no need for the compiler to discard any data cached in registers after invoking such a function. GCC and Clang may not support this yet, but by the time it gets added, we only need to change update this #define.

I also find it counter-intuitive to change the name so much. FWIW, googling at other projects, in llvm they use __has_attribute(warn_unused_result) , on glibc they use __wur.

The reason why LLVM uses __has_attribute(warn_unused_result) is because __has_attribute() literally tests whether a certain __attribute__(()) is supported. Of course they just sticked to GCC's naming scheme. I'd rather use glibc as an example how not to do things. Naming things different from the GCC attributes is not that uncommon either.

That said, I personally do not care strongly about which name gets used eventually, as long as it gets determined through reasoning that is not too narrowly focused.

[pfg]

In D2107#19, @ed wrote:

In D2107#16, @pfg wrote:

In D2107#15, @ed wrote:

In D2107#10, @pfg wrote:

What about __result_mandatory?

That would indicate an error, while's it's only a warning.

No, it wouldn't. There are other attributes in <sys/cdefs.h> that use a similar naming scheme that only raise a compiler warning by default.

Hmm ... none of them has the "mandatory" word on it.

But at least one of them has the word 'requires' in it, even though it only throws a compiler warning by default.

Still, my remark was not about the words or synonyms of words that are used. It's about the grammatical style. All of the existing keywords that are used as annotations can be used in a sentence that uses 'this function', 'this structure', 'this variable', etc. as the subject:

Point taken: an attribute is usually an adjective or a noun. To warn is an action and it is difficult to convey both.

<snip>

That said, I personally do not care strongly about which name gets used eventually, as long as it gets determined through reasoning that is not too narrowly focused.

You last proposal was __result_mandatory ; how about __result_use_check .

"check" is conveniently both a noun and a verb. It's still not nice but "use check" is a noun and doesn't involve something fatal.

[pfg]

Here is a more complete version, which covers

posix_memalign() and
reallocf().

(It is experimental)

[pfg]

FWIW, it appears that gcc48 clones a copy of cdefs.h and uses it instead of the one in base so I have to commit the cdefs.h changes some weeks in advance of using the new attributes to give gcc users time to catch up.

If there are no objections to __result_use_check I will commit the cdefs.h part tomorrow, otherwise suggest a better name ... at this time I'll take just about anything ;).

[op]

The __result_use_check is fine.

[pfg]

bde objected to the use of variadic macros as it breaks pre-C99 compilers
and the compatibility modes, so I had to update the changes for the
updated macros. The result is not nice but it is not too ugly either.

Fortunately multiple arguments are not too common.

The patch compiles buildworld.

[ed]

In D2107#27, @pfg wrote:

bde objected to the use of variadic macros as it breaks pre-C99 compilers and the compatibility modes

Which is a requirement that has been pulled out of thin air.

C99 has been out for 16 years. Our compiler defaults to C99. The next version of Clang that is going to be imported will default to C11. All sane compilers that default to C89 still allow VA_ARGS to be used.

A problem is not a problem if nobody is affected by it.

[pfg]

In D2107#30, @ed wrote:

In D2107#27, @pfg wrote:

bde objected to the use of variadic macros as it breaks pre-C99 compilers and the compatibility modes

Which is a requirement that has been pulled out of thin air.

C99 has been out for 16 years. Our compiler defaults to C99. The next version of Clang that is going to be imported will default to C11. All sane compilers that default to C89 still allow VA_ARGS to be used.

A problem is not a problem if nobody is affected by it.

I think so too, but it was not terribly difficult to accommodate for now. I do think we should clean out all the garbage in our headers that accommodate for gcc 2.8 (or older) and the intel compiler but that was not my objective at this time.

[pfg]

Closed by commit rS281130 (authored by @pfg).

[op]

Btw, in LLVM exists an equivalent of -fno-delete-null-pointer-checks compiler flag?
http://marc.info/?l=llvm-bugs&m=129810929109244&w=2

http://lwn.net/Articles/342330/
http://lwn.net/Articles/341773/

[dim]

In D2107#35, @opntr wrote:

Btw, in LLVM exists an equivalent of -fno-delete-null-pointer-checks compiler flag?
http://marc.info/?l=llvm-bugs&m=129810929109244&w=2

http://lwn.net/Articles/342330/
http://lwn.net/Articles/341773/

Please, let's not try to emulate the Linux kernel's bad programming practices. :-)

[pfg]

In D2107#35, @opntr wrote:

Btw, in LLVM exists an equivalent of -fno-delete-null-pointer-checks compiler flag?
http://marc.info/?l=llvm-bugs&m=129810929109244&w=2

No, the bug report is still open. I also don't want to introduce more attributes unless Android tests them thoroughly for us ;). I will be bringing some allocation attribute uses to libkern though.

FWIW, I also looked at the type safety annotations for ioctl() and fcntl() but I couldn't find use cases for them:
http://hpc-ua.org/hpc-ua-12/files/proceedings/3.pdf