[cfe-dev] RFC: do not optimize on basis of __attribute__((nonnull)) in glibc headers

[James Y Knight via cfe-dev]

On Tue, Jan 3, 2017 at 6:19 PM, Richard Smith via cfe-dev <
cfe-dev at lists.llvm.org> wrote:

> On 3 January 2017 at 14:46, Friedman, Eli <efriedma at codeaurora.org> wrote:
>
>> On 1/3/2017 2:06 PM, Richard Smith via cfe-dev wrote:
>>
>>> Via https://reviews.llvm.org/D27855, LLVM is likely to gain the ability
>>> to delete null checks in callers based on __attribute__((nonnull)) on the
>>> callee. This interacts badly with glibc's choice to mark the pointer
>>> parameters of memcpy, memmove, etc. as __attribute__((nonnull)) -- it is
>>> relatively common for programs to pass a null pointer and a zero size to
>>> these functions, and in practice libc implementations accept such usage.
>>> Indeed, LLVM's lowering of @llvm.memcpy intrinsics relies on these calls
>>> working.
>>>
>>> Deleting a null pointer check on p after a memcpy(p, q, 0) call seems
>>> extremely user-hostile, and very unlikely to result in a valuable
>>> improvement to the program, so I propose that we stop lowering
>>> __attribute__((nonnull)) on these builtin library functions to the llvm
>>> nonnull attribute.
>>>
>>> (Chandler is working on a paper for the C++ committee proposing to give
>>> these functions defined behavior when given a null pointer and a zero size,
>>> but optimizing on the basis of these particular nonnull attributes seems
>>> like a bad idea regardless of the C or C++ committees' decisions.)
>>>
>>> Thoughts?
>>>
>>
>> "memcpy, memmove, etc." seems to be hiding some details here; do you have
>> a complete list of functions you want to special-case?  There are a lot of
>> functions which could potentially go on that list, and some of them are
>> POSIX or GNU extensions.
>>
>
> The following glibc functions in <string.h> take both a pointer and a size
> providing an upper limit on the number of bytes that may be accessed
> through that pointer, and mark the pointer as __attribute__((nonnull)):
>
> ISO C:
>   memcpy, memmove, memset, memcmp, memchr, memrchr
>   strncpy [param 1 only], strncat [param 1 only], strncmp
>
> Extensions:
>   strndup, memccpy, mempcpy, strnlen, bcopy, bzero, bcmp, memfrob,
>   strncasecmp, strncasecmp_l [not parameter 4], stpncpy
>
> I would be interested in covering at least the ISO C functions. I don't
> have strong opinions about the extensions (any code built using those is
> likely also compiled with GCC, and so presumably has to fix this
> regardless).
>

IMO, ignoring nonnull on all of these would be a fine idea -- but if we go
down this path, please do open a bug against glibc asking for them to be
removed, pointing out that,
a. it is harmful to mark them nonnull because <evidence>.
b. clang is going to start ignoring the attributes on those functions even
if glibc doesn't remove them, because of a.
c. there will be a standards proposal to stop requiring nonnull, too.

It's worth at least an attempt to persuade them it's wrong to have those
attributes.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20170103/64217e75/attachment.html>