llvm dev - Aug 2015 - [LLVMdev] [cfe-dev] RFC: Adding attribute(nonnull) to things in libc++

On Wed, Jun 3, 2015 at 12:21 AM, Nick Lewycky <nlewycky at google.com>
wrote:
> On 1 June 2015 at 07:20, Marshall Clow <mclow.lists at gmail.com>
wrote:
>
>> This weekend, I got an email from Nuno Lopes informing me that UBSAN
now
>> paid attention to attribute(nonnull), and he was having some problems
with
>> it going off when using libc++.
>>
>
> FYI, I also looked into turning this on, but with libstdc++, and found
> that they annotated basic_string<T>::assign(pointer, len) with
attribute
> nonnull. That's a problem, because it's valid to call
> basic_string<T>::assign(nullptr, 0), but the reasoning why it's
valid makes
> me want to ask the committee whether this is what they intended.
>
> The language std text claims that the pointer must point to an array of
> 'n' (second argument) length, but earlier in the text it also
states that
> in the library, whenever it says "array" it means any pointer
upon which
> address computations and accesses to objects (that would be valid if the
> pointer did point to the first element of such an array). Thus, nullptr is
> valid if 'n' is zero.
>
> This was changed in DR2235:
> http://cplusplus.github.io/LWG/lwg-defects.html#2235
> The text and discussion of DR2235 sound like they intend to make the
> behaviour of assign match that of the constructor that takes the same
> arguments. What they actually did was change the constructor to match the
> behaviour of assign, and it doesn't look like removing the requirement
of a
> nonnull pointer was considered and intended.
>
> At this point I made a note that somebody should ask the committee when
> they get the chance, and never got back around to it.
>
>
I think that the reference in the discussion to [res.on.arguments] (the
relevant section there is paragraph 1.2) make it clear that the change was
deliberate - to allow (null, 0) as a set of parameters.

I do agree that this is different behavior than memcpy, memmove, etc.
That should make Joerg happy :-)

-- Marshall
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20150603/b4ba5e32/attachment.html>

I just want to revive this thread.

I'm not really endorsing this particular use of non-null in API design (I
agree with the committee that we should permit 'nullptr, 0' arguments).
However, I think that we should add these attributes for memcpy and
friends. Why? Because glibc has already shipped with these attributes which
means that portable code will never be able to pass a null pointer here
without running the risk of miscompile. Even if the standard were to
retract the dubious wording, we would have a very large number of glibc
installations in the world with the attributes and a large number of
compilers that will miscompile code by optimizing based on them. We should,
IMO, annotate it everywhere so that warnings and tools like UBSan can find
all of these bugs waiting to happen.

To get an idea of how widespread this is on at least the Linux platform,
they went into glibc over a decade ago:
https://sourceware.org/git/?p=glibc.git;a=commit;f=string/string.h;h=be27d08c05911a658949ba7b84f4321a65a2dbf4

I just spent a pile of time cleaning up LLVM and Clang. It would be great
to get more help keeping us in this clean state.

On Wed, Jun 3, 2015 at 6:37 AM Marshall Clow <mclow.lists at gmail.com>
wrote:
> On Wed, Jun 3, 2015 at 12:21 AM, Nick Lewycky <nlewycky at
google.com> wrote:
>
>> On 1 June 2015 at 07:20, Marshall Clow <mclow.lists at gmail.com>
wrote:
>>
>>> This weekend, I got an email from Nuno Lopes informing me that
UBSAN now
>>> paid attention to attribute(nonnull), and he was having some
problems with
>>> it going off when using libc++.
>>>
>>
>> FYI, I also looked into turning this on, but with libstdc++, and found
>> that they annotated basic_string<T>::assign(pointer, len) with
attribute
>> nonnull. That's a problem, because it's valid to call
>> basic_string<T>::assign(nullptr, 0), but the reasoning why
it's valid makes
>> me want to ask the committee whether this is what they intended.
>>
>> The language std text claims that the pointer must point to an array of
>> 'n' (second argument) length, but earlier in the text it also
states
>> that in the library, whenever it says "array" it means any
pointer upon
>> which address computations and accesses to objects (that would be valid
if
>> the pointer did point to the first element of such an array). Thus,
nullptr
>> is valid if 'n' is zero.
>>
>> This was changed in DR2235:
>> http://cplusplus.github.io/LWG/lwg-defects.html#2235
>>
<https://urldefense.proofpoint.com/v2/url?u=http-3A__cplusplus.github.io_LWG_lwg-2Ddefects.html-232235&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=Mfk2qtn1LTDThVkh6-oGglNfMADXfJdty4_bhmuhMHA&m=qWWobq8tJTAgytR0zHtK8nTZ1DcWszk2BFMbTnwZRTI&s=L1aJ06GiBGnAq3UajsCB29a-yiQx-HeurymfyzUm6tE&e=>
>> The text and discussion of DR2235 sound like they intend to make the
>> behaviour of assign match that of the constructor that takes the same
>> arguments. What they actually did was change the constructor to match
the
>> behaviour of assign, and it doesn't look like removing the
requirement of a
>> nonnull pointer was considered and intended.
>>
>> At this point I made a note that somebody should ask the committee when
>> they get the chance, and never got back around to it.
>>
>>
> I think that the reference in the discussion to [res.on.arguments] (the
> relevant section there is paragraph 1.2) make it clear that the change was
> deliberate - to allow (null, 0) as a set of parameters.
>
> I do agree that this is different behavior than memcpy, memmove, etc.
> That should make Joerg happy :-)
>
> -- Marshall
> _______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20150804/ea0db18e/attachment.html>

On Mon, Aug 3, 2015 at 7:20 PM, Chandler Carruth <chandlerc at google.com>
wrote:
> I just want to revive this thread.
>
> I'm not really endorsing this particular use of non-null in API design
(I
> agree with the committee that we should permit 'nullptr, 0'
arguments).
> However, I think that we should add these attributes for memcpy and
> friends. Why? Because glibc has already shipped with these attributes which
> means that portable code will never be able to pass a null pointer here
> without running the risk of miscompile. Even if the standard were to
> retract the dubious wording, we would have a very large number of glibc
> installations in the world with the attributes and a large number of
> compilers that will miscompile code by optimizing based on them. We should,
> IMO, annotate it everywhere so that warnings and tools like UBSan can find
> all of these bugs waiting to happen.
>
I rather like optimizing on these things - but if you don't & you just
want
to provide defense for those users writing portable code without punishing
them on our own compiler, what about using Doug's new nullability
attributes that are documented to not impact optimizatiosn/codegen?

>
> To get an idea of how widespread this is on at least the Linux platform,
> they went into glibc over a decade ago:
>
>
https://sourceware.org/git/?p=glibc.git;a=commit;f=string/string.h;h=be27d08c05911a658949ba7b84f4321a65a2dbf4
>
<https://urldefense.proofpoint.com/v2/url?u=https-3A__sourceware.org_git_-3Fp-3Dglibc.git-3Ba-3Dcommit-3Bf-3Dstring_string.h-3Bh-3Dbe27d08c05911a658949ba7b84f4321a65a2dbf4&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=Mfk2qtn1LTDThVkh6-oGglNfMADXfJdty4_bhmuhMHA&m=fNitvUpc-pwC1YORlsu2qkj139P7_ylGlo2DjB0C9Tc&s=r3S3bCJjUtn5rhXEdSqw21lWya86yF3bwVmT84yKDrY&e=>
>
> I just spent a pile of time cleaning up LLVM and Clang. It would be great
> to get more help keeping us in this clean state.
>
> On Wed, Jun 3, 2015 at 6:37 AM Marshall Clow <mclow.lists at
gmail.com>
> wrote:
>
>> On Wed, Jun 3, 2015 at 12:21 AM, Nick Lewycky <nlewycky at
google.com>
>> wrote:
>>
>>> On 1 June 2015 at 07:20, Marshall Clow <mclow.lists at
gmail.com> wrote:
>>>
>>>> This weekend, I got an email from Nuno Lopes informing me that
UBSAN
>>>> now paid attention to attribute(nonnull), and he was having
some problems
>>>> with it going off when using libc++.
>>>>
>>>
>>> FYI, I also looked into turning this on, but with libstdc++, and
found
>>> that they annotated basic_string<T>::assign(pointer, len)
with attribute
>>> nonnull. That's a problem, because it's valid to call
>>> basic_string<T>::assign(nullptr, 0), but the reasoning why
it's valid makes
>>> me want to ask the committee whether this is what they intended.
>>>
>>> The language std text claims that the pointer must point to an
array of
>>> 'n' (second argument) length, but earlier in the text it
also states
>>> that in the library, whenever it says "array" it means
any pointer upon
>>> which address computations and accesses to objects (that would be
valid if
>>> the pointer did point to the first element of such an array). Thus,
nullptr
>>> is valid if 'n' is zero.
>>>
>>> This was changed in DR2235:
>>> http://cplusplus.github.io/LWG/lwg-defects.html#2235
>>>
<https://urldefense.proofpoint.com/v2/url?u=http-3A__cplusplus.github.io_LWG_lwg-2Ddefects.html-232235&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=Mfk2qtn1LTDThVkh6-oGglNfMADXfJdty4_bhmuhMHA&m=qWWobq8tJTAgytR0zHtK8nTZ1DcWszk2BFMbTnwZRTI&s=L1aJ06GiBGnAq3UajsCB29a-yiQx-HeurymfyzUm6tE&e=>
>>> The text and discussion of DR2235 sound like they intend to make
the
>>> behaviour of assign match that of the constructor that takes the
same
>>> arguments. What they actually did was change the constructor to
match the
>>> behaviour of assign, and it doesn't look like removing the
requirement of a
>>> nonnull pointer was considered and intended.
>>>
>>> At this point I made a note that somebody should ask the committee
when
>>> they get the chance, and never got back around to it.
>>>
>>>
>> I think that the reference in the discussion to [res.on.arguments] (the
>> relevant section there is paragraph 1.2) make it clear that the change
was
>> deliberate - to allow (null, 0) as a set of parameters.
>>
>> I do agree that this is different behavior than memcpy, memmove, etc.
>> That should make Joerg happy :-)
>>
>> -- Marshall
>> _______________________________________________
>> LLVM Developers mailing list
>> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
>>
>
> _______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20150803/1e343e97/attachment.html>

Re-send because the original went to the old lists.
-- Marshall

---------- Forwarded message ----------
From: Marshall Clow <mclow.lists at gmail.com>
Date: Tue, Aug 11, 2015 at 10:50 AM
Subject: Re: [LLVMdev] [cfe-dev] RFC: Adding attribute(nonnull) to things
in libc++
To: Chandler Carruth <chandlerc at google.com>
Cc: Nick Lewycky <nlewycky at google.com>, "cfe-dev at cs.uiuc.edu
Developers" <
cfe-dev at cs.uiuc.edu>, LLVM Developers Mailing List <llvmdev at
cs.uiuc.edu>


On Mon, Aug 3, 2015 at 7:20 PM, Chandler Carruth <chandlerc at google.com>
wrote:
> I just want to revive this thread.
>
> I'm not really endorsing this particular use of non-null in API design
(I
> agree with the committee that we should permit 'nullptr, 0'
arguments).
> However, I think that we should add these attributes for memcpy and
> friends. Why? Because glibc has already shipped with these attributes which
> means that portable code will never be able to pass a null pointer here
> without running the risk of miscompile. Even if the standard were to
> retract the dubious wording, we would have a very large number of glibc
> installations in the world with the attributes and a large number of
> compilers that will miscompile code by optimizing based on them. We should,
> IMO, annotate it everywhere so that warnings and tools like UBSan can find
> all of these bugs waiting to happen.
>
> To get an idea of how widespread this is on at least the Linux platform,
> they went into glibc over a decade ago:
>
>
https://sourceware.org/git/?p=glibc.git;a=commit;f=string/string.h;h=be27d08c05911a658949ba7b84f4321a65a2dbf4
>
> I just spent a pile of time cleaning up LLVM and Clang. It would be great
> to get more help keeping us in this clean state.
>
>
I put up http://reviews.llvm.org/D11948 for review.

-- Marshall
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20150811/50ab5314/attachment.html>