RFC: Adding attribute(nonnull) to things in libc++

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
17 messages Options
Reply | Threaded
Open this post in threaded view
|

RFC: Adding attribute(nonnull) to things in libc++

Marshall Clow-2
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++.

I did some investigation, and found that he was exactly right - there were places (deep inside the vector code, for example) which called std::memcpy(null, null, 0) - which is definitely UB.

In an ideal world, our C library would define ::memcpy with the correct annotations, and libc++ would import that into namespace std, and we'd be golden.  

But we don't have a C library - we use whatever is provided by the system we're running on, so that's not really an option.

For my testing, I changed libc++'s <cstring> header:

-using ::memcpy;
+inline _LIBCPP_INLINE_VISIBILITY 
+void* memcpy(void* __s1, const void* __s2, size_t __n) __attribute__((nonnull(1, 2)))
+{ return ::memcpy(__s1, __s2, __n); }

(similarly for memmove and memcmp), and I found several cases of simple code that now UBSAN fires off on:

such as:    std::vector<int> v;  v.push_back(1);
and :          int *p = NULL;  std::copy(p,p,p);

This seems fairly useful to me.

I would like to hear other people's opinions about:

* Is adding this kind of UB detection something that people want in libc++?

* What do people think about wrapping the C library functions to enable UBSAN to catch them (this is a separate Q from the first Q, because I can see putting these kind of parameter checks into functions that have no counterpart in the C library). Sadly, this would NOT affect calls to ::memcpy (for example), just std::memcpy.

* Is that the best way to annotate the declarations? Is there a more portable, standard way to do this (things that start with double underscores worry me). In any case, I would probably wrap this in a macro to support compilers that don't understand whatever mechanism we end up using.

Thanks

-- Marshall


_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: [cfe-dev] RFC: Adding attribute(nonnull) to things in libc++

Reid Kleckner-2
Why should memset / memcpy be attribute nonnull? Is there standardese that supports that?

On Mon, Jun 1, 2015 at 7:20 AM, Marshall Clow <[hidden email]> 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++.

I did some investigation, and found that he was exactly right - there were places (deep inside the vector code, for example) which called std::memcpy(null, null, 0) - which is definitely UB.

In an ideal world, our C library would define ::memcpy with the correct annotations, and libc++ would import that into namespace std, and we'd be golden.  

But we don't have a C library - we use whatever is provided by the system we're running on, so that's not really an option.

For my testing, I changed libc++'s <cstring> header:

-using ::memcpy;
+inline _LIBCPP_INLINE_VISIBILITY 
+void* memcpy(void* __s1, const void* __s2, size_t __n) __attribute__((nonnull(1, 2)))
+{ return ::memcpy(__s1, __s2, __n); }

(similarly for memmove and memcmp), and I found several cases of simple code that now UBSAN fires off on:

such as:    std::vector<int> v;  v.push_back(1);
and :          int *p = NULL;  std::copy(p,p,p);

This seems fairly useful to me.

I would like to hear other people's opinions about:

* Is adding this kind of UB detection something that people want in libc++?

* What do people think about wrapping the C library functions to enable UBSAN to catch them (this is a separate Q from the first Q, because I can see putting these kind of parameter checks into functions that have no counterpart in the C library). Sadly, this would NOT affect calls to ::memcpy (for example), just std::memcpy.

* Is that the best way to annotate the declarations? Is there a more portable, standard way to do this (things that start with double underscores worry me). In any case, I would probably wrap this in a macro to support compilers that don't understand whatever mechanism we end up using.

Thanks

-- Marshall


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev



_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: [cfe-dev] RFC: Adding attribute(nonnull) to things in libc++

Joerg Sonnenberger
On Mon, Jun 01, 2015 at 09:57:17AM -0700, Reid Kleckner wrote:
> Why should memset / memcpy be attribute nonnull? Is there standardese that
> supports that?

The generic entry text of the standard section. IMO this is a standard
bug and someone should *please* get it fixed. It is ridiculous that zero
sized operations are considered UB.

Joerg
_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: [cfe-dev] RFC: Adding attribute(nonnull) to things in libc++

Marshall Clow-2
On Mon, Jun 1, 2015 at 10:35 AM, Joerg Sonnenberger <[hidden email]> wrote:
On Mon, Jun 01, 2015 at 09:57:17AM -0700, Reid Kleckner wrote:
> Why should memset / memcpy be attribute nonnull? Is there standardese that
> supports that?

The generic entry text of the standard section. IMO this is a standard
bug and someone should *please* get it fixed. It is ridiculous that zero
sized operations are considered UB.

That would require a change to the C standard, and, as far as I know, there are no current plans to issue a revised C standard.

I *suppose* that we could change it in the C++ standard, but I doubt that there would be any support on the committee for making std::memcpy different from ::memcpy.

-- Marshall

P.S.   recent gcc (at least 4.8.x and later) make optimizations based on this UB (i.e, if you pass a pointer to memcpy, then it can't be NULL).



_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: [cfe-dev] RFC: Adding attribute(nonnull) to things in libc++

Joerg Sonnenberger
On Mon, Jun 01, 2015 at 10:52:20AM -0700, Marshall Clow wrote:

> On Mon, Jun 1, 2015 at 10:35 AM, Joerg Sonnenberger <[hidden email]
> > wrote:
>
> > On Mon, Jun 01, 2015 at 09:57:17AM -0700, Reid Kleckner wrote:
> > > Why should memset / memcpy be attribute nonnull? Is there standardese
> > that
> > > supports that?
> >
> > The generic entry text of the standard section. IMO this is a standard
> > bug and someone should *please* get it fixed. It is ridiculous that zero
> > sized operations are considered UB.
> >
>
> That would require a change to the C standard, and, as far as I know, there
> are no current plans to issue a revised C standard.

Well, a good start would be to get a position on whether this
interpretation is intentional. Especially given GCC's aggressive
exploitation. It doesn't make sense to me as there doesn't seem to be a
way that "if len is 0, the pointer must be not-null" can improve an
implementation on any system where memory operations can trap.

Joerg
_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: [cfe-dev] RFC: Adding attribute(nonnull) to things in libc++

Joerg Sonnenberger
In reply to this post by Marshall Clow-2
On Mon, Jun 01, 2015 at 10:52:20AM -0700, Marshall Clow wrote:
> P.S.   recent gcc (at least 4.8.x and later) make optimizations based on
> this UB (i.e, if you pass a pointer to memcpy, then it can't be NULL).

BTW, this seems to be more an issue with glibc adding the tagging and
not behavior of GCC itself.

Joerg
_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: [cfe-dev] RFC: Adding attribute(nonnull) to things in libc++

David Chisnall-5
In reply to this post by Marshall Clow-2
On 1 Jun 2015, at 18:52, Marshall Clow <[hidden email]> wrote:
>
>> The generic entry text of the standard section. IMO this is a standard
>> bug and someone should *please* get it fixed. It is ridiculous that zero
>> sized operations are considered UB.
>
> That would require a change to the C standard, and, as far as I know, there are no current plans to issue a revised C standard.

Not necessarily.  Other standards, such as POSIX, are free to define behaviour that is undefined or implementation defined in C.  POSIX mandates that a char is exactly 8 bits, for example, which is IB in C.  The goal of UB is to give freedom to implementors.  Saying that NULL arguments to memcpy are UB does not mean that we are compelled to disallow them, it just means that:

- We don’t have to accept them.
- We don’t have to be consistent in whether we accept or reject them.
- We can choose to do whatever makes implementation easiest.

If the easiest thing is to permit them as long as the length is zero (it seems to be), then that’s a perfectly valid implementation of undefined behaviour.

It is also undefined behaviour whether pointer comparisons between different objects are stable, but for the most part they are (and a lot of code would break if they weren’t), because implementers have decided that this is the easiest implementation of this particular bit of UB.

David


_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: [cfe-dev] RFC: Adding attribute(nonnull) to things in libc++

Benjamin Kramer
In reply to this post by Joerg Sonnenberger

> On 01.06.2015, at 23:14, Joerg Sonnenberger <[hidden email]> wrote:
>
> On Mon, Jun 01, 2015 at 10:52:20AM -0700, Marshall Clow wrote:
>> P.S.   recent gcc (at least 4.8.x and later) make optimizations based on
>> this UB (i.e, if you pass a pointer to memcpy, then it can't be NULL).
>
> BTW, this seems to be more an issue with glibc adding the tagging and
> not behavior of GCC itself.

GCC also adds nonnull attributes via its builtin functions mechanism. If we want to follow GCC here we should do the same (Builtins.def has a FIXME about nonnull), which is imo cleaner than wrapping memcpy in libc++ and has the advantage of also working with plain C code.

I don't think this has an impact on optimization right now; Clang lowers memcpy calls to the llvm.memcpy intrinsic and we traditionally have allowed llvm.memcpy with a nullptr and zero length (i.e. the optimizer has to prove that the length argument is non-zero to assume the pointer is dereferenceable).

- Ben
_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: [cfe-dev] RFC: Adding attribute(nonnull) to things in libc++

Joerg Sonnenberger
On Tue, Jun 02, 2015 at 12:49:11PM +0200, Benjamin Kramer wrote:

>
> > On 01.06.2015, at 23:14, Joerg Sonnenberger <[hidden email]> wrote:
> >
> > On Mon, Jun 01, 2015 at 10:52:20AM -0700, Marshall Clow wrote:
> >> P.S.   recent gcc (at least 4.8.x and later) make optimizations based on
> >> this UB (i.e, if you pass a pointer to memcpy, then it can't be NULL).
> >
> > BTW, this seems to be more an issue with glibc adding the tagging and
> > not behavior of GCC itself.
>
> GCC also adds nonnull attributes via its builtin functions mechanism.

I don't see it on NetBSD with GCC 4.8.4, so a plain prototype doesn't
seeem to trigger it.

Joerg
_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: [cfe-dev] RFC: Adding attribute(nonnull) to things in libc++

Benjamin Kramer

> On 02.06.2015, at 13:34, Joerg Sonnenberger <[hidden email]> wrote:
>
> On Tue, Jun 02, 2015 at 12:49:11PM +0200, Benjamin Kramer wrote:
>>
>>> On 01.06.2015, at 23:14, Joerg Sonnenberger <[hidden email]> wrote:
>>>
>>> On Mon, Jun 01, 2015 at 10:52:20AM -0700, Marshall Clow wrote:
>>>> P.S.   recent gcc (at least 4.8.x and later) make optimizations based on
>>>> this UB (i.e, if you pass a pointer to memcpy, then it can't be NULL).
>>>
>>> BTW, this seems to be more an issue with glibc adding the tagging and
>>> not behavior of GCC itself.
>>
>> GCC also adds nonnull attributes via its builtin functions mechanism.
>
> I don't see it on NetBSD with GCC 4.8.4, so a plain prototype doesn't
> seeem to trigger it.

Hmm, odd. GCC has done so for a long time, maybe it's disabled on some platforms? I get nonnull warnings with gcc 4.7 and gcc 5.0.

$ gcc -Wall -x c - <<< "int main() { memcpy(0, 0, 0); }"
<stdin>: In function ‘main’:
<stdin>:1:1: warning: implicit declaration of function ‘memcpy’ [-Wimplicit-function-declaration]
<stdin>:1:14: warning: incompatible implicit declaration of built-in function ‘memcpy’ [enabled by default]
<stdin>:1:1: warning: null argument where non-null required (argument 1) [-Wnonnull]
<stdin>:1:1: warning: null argument where non-null required (argument 2) [-Wnonnull]

- Ben
_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: [cfe-dev] RFC: Adding attribute(nonnull) to things in libc++

Marshall Clow-2
In reply to this post by David Chisnall-5


On Tue, Jun 2, 2015 at 2:12 AM, David Chisnall <[hidden email]> wrote:
On 1 Jun 2015, at 18:52, Marshall Clow <[hidden email]> wrote:
>
>> The generic entry text of the standard section. IMO this is a standard
>> bug and someone should *please* get it fixed. It is ridiculous that zero
>> sized operations are considered UB.
>
> That would require a change to the C standard, and, as far as I know, there are no current plans to issue a revised C standard.

Not necessarily.  Other standards, such as POSIX, are free to define behaviour that is undefined or implementation defined in C.  POSIX mandates that a char is exactly 8 bits, for example, which is IB in C.  The goal of UB is to give freedom to implementors.  Saying that NULL arguments to memcpy are UB does not mean that we are compelled to disallow them, it just means that:

- We don’t have to accept them.
- We don’t have to be consistent in whether we accept or reject them.
- We can choose to do whatever makes implementation easiest.

If the easiest thing is to permit them as long as the length is zero (it seems to be), then that’s a perfectly valid implementation of undefined behaviour.

Heh. There are no "invalid implementations" of undefined behavior.

The reason I proposed this change was to allow UBSAN to detect undefined behavior, and report it to users - rather than having their code (potentially) fail in hard-to-diagnose ways when they change *something* (refactor code, change compilers, change optimization level, etc).


-- Marshall


_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: [cfe-dev] RFC: Adding attribute(nonnull) to things in libc++

Nick Lewycky-2
In reply to this post by Marshall Clow-2
On 1 June 2015 at 07:20, Marshall Clow <[hidden email]> 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.

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.

Nick

I did some investigation, and found that he was exactly right - there were places (deep inside the vector code, for example) which called std::memcpy(null, null, 0) - which is definitely UB.

In an ideal world, our C library would define ::memcpy with the correct annotations, and libc++ would import that into namespace std, and we'd be golden.  

But we don't have a C library - we use whatever is provided by the system we're running on, so that's not really an option.

For my testing, I changed libc++'s <cstring> header:

-using ::memcpy;
+inline _LIBCPP_INLINE_VISIBILITY 
+void* memcpy(void* __s1, const void* __s2, size_t __n) __attribute__((nonnull(1, 2)))
+{ return ::memcpy(__s1, __s2, __n); }

(similarly for memmove and memcmp), and I found several cases of simple code that now UBSAN fires off on:

such as:    std::vector<int> v;  v.push_back(1);
and :          int *p = NULL;  std::copy(p,p,p);

This seems fairly useful to me.

I would like to hear other people's opinions about:

* Is adding this kind of UB detection something that people want in libc++?

* What do people think about wrapping the C library functions to enable UBSAN to catch them (this is a separate Q from the first Q, because I can see putting these kind of parameter checks into functions that have no counterpart in the C library). Sadly, this would NOT affect calls to ::memcpy (for example), just std::memcpy.

* Is that the best way to annotate the declarations? Is there a more portable, standard way to do this (things that start with double underscores worry me). In any case, I would probably wrap this in a macro to support compilers that don't understand whatever mechanism we end up using.

Thanks

-- Marshall


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev



_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: [cfe-dev] RFC: Adding attribute(nonnull) to things in libc++

Marshall Clow-2
On Wed, Jun 3, 2015 at 12:21 AM, Nick Lewycky <[hidden email]> wrote:
On 1 June 2015 at 07:20, Marshall Clow <[hidden email]> 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.

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
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: [cfe-dev] RFC: Adding attribute(nonnull) to things in libc++

Chandler Carruth-2
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:

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 <[hidden email]> wrote:
On Wed, Jun 3, 2015 at 12:21 AM, Nick Lewycky <[hidden email]> wrote:
On 1 June 2015 at 07:20, Marshall Clow <[hidden email]> 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.

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
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev

_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: [cfe-dev] RFC: Adding attribute(nonnull) to things in libc++

David Blaikie


On Mon, Aug 3, 2015 at 7:20 PM, Chandler Carruth <[hidden email]> 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:

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 <[hidden email]> wrote:
On Wed, Jun 3, 2015 at 12:21 AM, Nick Lewycky <[hidden email]> wrote:
On 1 June 2015 at 07:20, Marshall Clow <[hidden email]> 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.

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
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev

_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev



_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: [cfe-dev] RFC: Adding attribute(nonnull) to things in libc++

Marshall Clow-2
On Mon, Aug 3, 2015 at 8:07 PM, David Blaikie <[hidden email]> wrote:

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?


I, too like optimizing on these things.
Smaller code that runs faster is always nice.

-- Marshall
 


_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

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

Doerfert, Johannes via llvm-dev
In reply to this post by Chandler Carruth-2
Re-send because the original went to the old lists.
-- Marshall

---------- Forwarded message ----------
From: Marshall Clow <[hidden email]>
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 <[hidden email]>
Cc: Nick Lewycky <[hidden email]>, "[hidden email] Developers" <[hidden email]>, LLVM Developers Mailing List <[hidden email]>


On Mon, Aug 3, 2015 at 7:20 PM, Chandler Carruth <[hidden email]> 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:

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
 



_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev