Quantcast

RFC: do not optimize on basis of __attribute__((nonnull)) in glibc headers

classic Classic list List threaded Threaded
26 messages Options
12
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFC: do not optimize on basis of __attribute__((nonnull)) in glibc headers

Ivan Garramona via cfe-dev
Just to reiterate...

I would really like to see us communicate this decision, with an appropriately persuasive argument, both to the glibc authors and to the standards committees, and treat this as a *transitional* hack, which is needed until glibc (and any other libc if there are any?) can remove the erroneous nonnull declarations from their headers, and everyone has upgraded.

On Thu, Mar 9, 2017 at 11:04 PM, Chandler Carruth <[hidden email]> wrote:
I've sent a patch to implement this here: https://reviews.llvm.org/D30806

It turns out to be both easier and I hope less controversial than I had imagined. Neither Clang nor LLVM ever actually got nonnull onto these declarations even when they were suitable annotated. We didn't carry that annotation across to the declaration. Oops.

So I've fixed that, but *also* disabled it for the libc functions that we have library builtin recognition for. We can extend this to cover any set of library functions folks want, just let me know. The use of it will even be correctly controlled by -fno-builtin and friends.

The net result is that this should not regress optimizations for *anyone*, and actually enable a bunch of optimizations outside of libc functions, while preserving safety on those libc functions.

We still have the nullability attributes which can be used to annotate more interfaces in a way that will provide warnings and static analysis aid, but not optimize on and so not run the risk of changing behavior of existing code. Those would be good candidates (IMO) for libc++ to use on any relevant interfaces which match the criteria outlined in this thread: pointers paired with a size should allow null+zero.

Hopefully this makes everyone happy, please feel free to chime in on the review thread if more discussion is needed.

On Wed, Jan 4, 2017 at 11:24 AM Aaron Ballman via cfe-dev <[hidden email]> wrote:
On Wed, Jan 4, 2017 at 2:01 PM, James Y Knight <[hidden email]> wrote:
>
>
> On Wed, Jan 4, 2017 at 12:54 PM, Hal Finkel <[hidden email]> wrote:
>>
>>
>> On 01/04/2017 11:43 AM, James Y Knight via cfe-dev wrote:
>>
>> On Wed, Jan 4, 2017 at 11:12 AM, Aaron Ballman via cfe-dev
>> <[hidden email]> wrote:
>>>
>>> So I would be opposed to ignoring those attributes in
>>>
>>> Sema (I think we should still warn when users do nonportable things),
>>> but in favor of not changing the optimizer to capitalize on this
>>> "opportunity."
>>
>>
>> I'd be opposed to ignoring the attributes only in some places and not in
>> others. It should be ignored totally, as if it wasn't present on those
>> functions. Doing anything else sends the wrong message -- that libc authors
>> should continue to use nonnull on these functions because they might be
>> helpful, and won't do anything bad.
>>
>>
>> I think that we have a responsibility to our users to continue to warn
>> (statically, in ubsan, etc.) on non-portable behavior, which this is and
>> will continue to be in practice for at least a decade or two, regardless of
>> the message we'd like to send libc authors. We cannot undo history here and
>> this will be relevant to production systems for at least a decade. We can
>> talk to libc developers directly -- they're a much smaller set -- and we can
>> pursue change at the standards level while still providing the most useful
>> set of tools to our users in the mean time.
>
>
> But, this is an entirely different question.
>
> - Should clang warn about non-portable usage of passing NULL to memcpy/etc?
>
> Sounds like a fine warning to add.
>
> - Should that warning be dependent on the libc headers having nonnull
> annotations on these functions, which will be used only for warnings, and
> ignored for semantics, on this given list of hardcoded functions?
>
> No.
>
> Firstly: I'd note that nearly all libc implementations don't use these
> attributes today. In some cases, because they've simply not thought about
> it, but in others because they explicitly decided to NOT break their users'
> code by introducing this problem! Glibc is the outlier, here.
>
> So: what portability do you want to warn for? Portability assuming the same
> libc, but a different compiler which might fail to ignore the nonnull
> attribute? Or portability to other libc? If the latter, depending on the
> nonnull attribute being present doesn't and can't work.

My preference is for portability for both, but you bring up a good
point about other libc implementations not being annotated. So long as
we retain the ability to tell users their code is not portable *and*
we get rid of the dangerous optimizations, I'm happy. I just don't
want to lose the diagnostics because of the optimizations.

~Aaron

> Secondly: if we already have a hardcoded list of functions to special case,
> that could just as well be used to generate a nonportable-stringfunc-null
> warning, as well.
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFC: do not optimize on basis of __attribute__((nonnull)) in glibc headers

Ivan Garramona via cfe-dev
I'm happy to handle at least the standards side of this; I don't really have great connections on the libc front, but happy to help folks do that.

On Tue, Mar 14, 2017 at 2:35 PM James Y Knight <[hidden email]> wrote:
Just to reiterate...

I would really like to see us communicate this decision, with an appropriately persuasive argument, both to the glibc authors and to the standards committees, and treat this as a *transitional* hack, which is needed until glibc (and any other libc if there are any?) can remove the erroneous nonnull declarations from their headers, and everyone has upgraded.

On Thu, Mar 9, 2017 at 11:04 PM, Chandler Carruth <[hidden email]> wrote:
I've sent a patch to implement this here: https://reviews.llvm.org/D30806

It turns out to be both easier and I hope less controversial than I had imagined. Neither Clang nor LLVM ever actually got nonnull onto these declarations even when they were suitable annotated. We didn't carry that annotation across to the declaration. Oops.

So I've fixed that, but *also* disabled it for the libc functions that we have library builtin recognition for. We can extend this to cover any set of library functions folks want, just let me know. The use of it will even be correctly controlled by -fno-builtin and friends.

The net result is that this should not regress optimizations for *anyone*, and actually enable a bunch of optimizations outside of libc functions, while preserving safety on those libc functions.

We still have the nullability attributes which can be used to annotate more interfaces in a way that will provide warnings and static analysis aid, but not optimize on and so not run the risk of changing behavior of existing code. Those would be good candidates (IMO) for libc++ to use on any relevant interfaces which match the criteria outlined in this thread: pointers paired with a size should allow null+zero.

Hopefully this makes everyone happy, please feel free to chime in on the review thread if more discussion is needed.

On Wed, Jan 4, 2017 at 11:24 AM Aaron Ballman via cfe-dev <[hidden email]> wrote:
On Wed, Jan 4, 2017 at 2:01 PM, James Y Knight <[hidden email]> wrote:
>
>
> On Wed, Jan 4, 2017 at 12:54 PM, Hal Finkel <[hidden email]> wrote:
>>
>>
>> On 01/04/2017 11:43 AM, James Y Knight via cfe-dev wrote:
>>
>> On Wed, Jan 4, 2017 at 11:12 AM, Aaron Ballman via cfe-dev
>> <[hidden email]> wrote:
>>>
>>> So I would be opposed to ignoring those attributes in
>>>
>>> Sema (I think we should still warn when users do nonportable things),
>>> but in favor of not changing the optimizer to capitalize on this
>>> "opportunity."
>>
>>
>> I'd be opposed to ignoring the attributes only in some places and not in
>> others. It should be ignored totally, as if it wasn't present on those
>> functions. Doing anything else sends the wrong message -- that libc authors
>> should continue to use nonnull on these functions because they might be
>> helpful, and won't do anything bad.
>>
>>
>> I think that we have a responsibility to our users to continue to warn
>> (statically, in ubsan, etc.) on non-portable behavior, which this is and
>> will continue to be in practice for at least a decade or two, regardless of
>> the message we'd like to send libc authors. We cannot undo history here and
>> this will be relevant to production systems for at least a decade. We can
>> talk to libc developers directly -- they're a much smaller set -- and we can
>> pursue change at the standards level while still providing the most useful
>> set of tools to our users in the mean time.
>
>
> But, this is an entirely different question.
>
> - Should clang warn about non-portable usage of passing NULL to memcpy/etc?
>
> Sounds like a fine warning to add.
>
> - Should that warning be dependent on the libc headers having nonnull
> annotations on these functions, which will be used only for warnings, and
> ignored for semantics, on this given list of hardcoded functions?
>
> No.
>
> Firstly: I'd note that nearly all libc implementations don't use these
> attributes today. In some cases, because they've simply not thought about
> it, but in others because they explicitly decided to NOT break their users'
> code by introducing this problem! Glibc is the outlier, here.
>
> So: what portability do you want to warn for? Portability assuming the same
> libc, but a different compiler which might fail to ignore the nonnull
> attribute? Or portability to other libc? If the latter, depending on the
> nonnull attribute being present doesn't and can't work.

My preference is for portability for both, but you bring up a good
point about other libc implementations not being annotated. So long as
we retain the ability to tell users their code is not portable *and*
we get rid of the dangerous optimizations, I'm happy. I just don't
want to lose the diagnostics because of the optimizations.

~Aaron

> Secondly: if we already have a hardcoded list of functions to special case,
> that could just as well be used to generate a nonportable-stringfunc-null
> warning, as well.
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFC: do not optimize on basis of __attribute__((nonnull)) in glibc headers

Ivan Garramona via cfe-dev
Are there blockers to landing https://reviews.llvm.org/D30806 ?

I'm committed to following through with the respective standards bodies and anyone else we feel is prudent, but I would somewhat like to unblock the optimization work in this area in parallel as I suspect that the communication and establishment of a rational plan will require a lot of time.

On Tue, Mar 14, 2017 at 2:54 PM Chandler Carruth via cfe-dev <[hidden email]> wrote:
I'm happy to handle at least the standards side of this; I don't really have great connections on the libc front, but happy to help folks do that.

On Tue, Mar 14, 2017 at 2:35 PM James Y Knight <[hidden email]> wrote:
Just to reiterate...

I would really like to see us communicate this decision, with an appropriately persuasive argument, both to the glibc authors and to the standards committees, and treat this as a *transitional* hack, which is needed until glibc (and any other libc if there are any?) can remove the erroneous nonnull declarations from their headers, and everyone has upgraded.

On Thu, Mar 9, 2017 at 11:04 PM, Chandler Carruth <[hidden email]> wrote:
I've sent a patch to implement this here: https://reviews.llvm.org/D30806

It turns out to be both easier and I hope less controversial than I had imagined. Neither Clang nor LLVM ever actually got nonnull onto these declarations even when they were suitable annotated. We didn't carry that annotation across to the declaration. Oops.

So I've fixed that, but *also* disabled it for the libc functions that we have library builtin recognition for. We can extend this to cover any set of library functions folks want, just let me know. The use of it will even be correctly controlled by -fno-builtin and friends.

The net result is that this should not regress optimizations for *anyone*, and actually enable a bunch of optimizations outside of libc functions, while preserving safety on those libc functions.

We still have the nullability attributes which can be used to annotate more interfaces in a way that will provide warnings and static analysis aid, but not optimize on and so not run the risk of changing behavior of existing code. Those would be good candidates (IMO) for libc++ to use on any relevant interfaces which match the criteria outlined in this thread: pointers paired with a size should allow null+zero.

Hopefully this makes everyone happy, please feel free to chime in on the review thread if more discussion is needed.

On Wed, Jan 4, 2017 at 11:24 AM Aaron Ballman via cfe-dev <[hidden email]> wrote:
On Wed, Jan 4, 2017 at 2:01 PM, James Y Knight <[hidden email]> wrote:
>
>
> On Wed, Jan 4, 2017 at 12:54 PM, Hal Finkel <[hidden email]> wrote:
>>
>>
>> On 01/04/2017 11:43 AM, James Y Knight via cfe-dev wrote:
>>
>> On Wed, Jan 4, 2017 at 11:12 AM, Aaron Ballman via cfe-dev
>> <[hidden email]> wrote:
>>>
>>> So I would be opposed to ignoring those attributes in
>>>
>>> Sema (I think we should still warn when users do nonportable things),
>>> but in favor of not changing the optimizer to capitalize on this
>>> "opportunity."
>>
>>
>> I'd be opposed to ignoring the attributes only in some places and not in
>> others. It should be ignored totally, as if it wasn't present on those
>> functions. Doing anything else sends the wrong message -- that libc authors
>> should continue to use nonnull on these functions because they might be
>> helpful, and won't do anything bad.
>>
>>
>> I think that we have a responsibility to our users to continue to warn
>> (statically, in ubsan, etc.) on non-portable behavior, which this is and
>> will continue to be in practice for at least a decade or two, regardless of
>> the message we'd like to send libc authors. We cannot undo history here and
>> this will be relevant to production systems for at least a decade. We can
>> talk to libc developers directly -- they're a much smaller set -- and we can
>> pursue change at the standards level while still providing the most useful
>> set of tools to our users in the mean time.
>
>
> But, this is an entirely different question.
>
> - Should clang warn about non-portable usage of passing NULL to memcpy/etc?
>
> Sounds like a fine warning to add.
>
> - Should that warning be dependent on the libc headers having nonnull
> annotations on these functions, which will be used only for warnings, and
> ignored for semantics, on this given list of hardcoded functions?
>
> No.
>
> Firstly: I'd note that nearly all libc implementations don't use these
> attributes today. In some cases, because they've simply not thought about
> it, but in others because they explicitly decided to NOT break their users'
> code by introducing this problem! Glibc is the outlier, here.
>
> So: what portability do you want to warn for? Portability assuming the same
> libc, but a different compiler which might fail to ignore the nonnull
> attribute? Or portability to other libc? If the latter, depending on the
> nonnull attribute being present doesn't and can't work.

My preference is for portability for both, but you bring up a good
point about other libc implementations not being annotated. So long as
we retain the ability to tell users their code is not portable *and*
we get rid of the dangerous optimizations, I'm happy. I just don't
want to lose the diagnostics because of the optimizations.

~Aaron

> Secondly: if we already have a hardcoded list of functions to special case,
> that could just as well be used to generate a nonportable-stringfunc-null
> warning, as well.
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFC: do not optimize on basis of __attribute__((nonnull)) in glibc headers

Ivan Garramona via cfe-dev
I'm in support of unblocking the LLVM side optimization changes.  I'm definitely not qualified to speak towards the direction of the clang changes though.  :)

Philip

On 03/16/2017 03:39 PM, Chandler Carruth via cfe-dev wrote:
Are there blockers to landing https://reviews.llvm.org/D30806 ?

I'm committed to following through with the respective standards bodies and anyone else we feel is prudent, but I would somewhat like to unblock the optimization work in this area in parallel as I suspect that the communication and establishment of a rational plan will require a lot of time.

On Tue, Mar 14, 2017 at 2:54 PM Chandler Carruth via cfe-dev <[hidden email]> wrote:
I'm happy to handle at least the standards side of this; I don't really have great connections on the libc front, but happy to help folks do that.

On Tue, Mar 14, 2017 at 2:35 PM James Y Knight <[hidden email]> wrote:
Just to reiterate...

I would really like to see us communicate this decision, with an appropriately persuasive argument, both to the glibc authors and to the standards committees, and treat this as a *transitional* hack, which is needed until glibc (and any other libc if there are any?) can remove the erroneous nonnull declarations from their headers, and everyone has upgraded.

On Thu, Mar 9, 2017 at 11:04 PM, Chandler Carruth <[hidden email]> wrote:
I've sent a patch to implement this here: https://reviews.llvm.org/D30806

It turns out to be both easier and I hope less controversial than I had imagined. Neither Clang nor LLVM ever actually got nonnull onto these declarations even when they were suitable annotated. We didn't carry that annotation across to the declaration. Oops.

So I've fixed that, but *also* disabled it for the libc functions that we have library builtin recognition for. We can extend this to cover any set of library functions folks want, just let me know. The use of it will even be correctly controlled by -fno-builtin and friends.

The net result is that this should not regress optimizations for *anyone*, and actually enable a bunch of optimizations outside of libc functions, while preserving safety on those libc functions.

We still have the nullability attributes which can be used to annotate more interfaces in a way that will provide warnings and static analysis aid, but not optimize on and so not run the risk of changing behavior of existing code. Those would be good candidates (IMO) for libc++ to use on any relevant interfaces which match the criteria outlined in this thread: pointers paired with a size should allow null+zero.

Hopefully this makes everyone happy, please feel free to chime in on the review thread if more discussion is needed.

On Wed, Jan 4, 2017 at 11:24 AM Aaron Ballman via cfe-dev <[hidden email]> wrote:
On Wed, Jan 4, 2017 at 2:01 PM, James Y Knight <[hidden email]> wrote:
>
>
> On Wed, Jan 4, 2017 at 12:54 PM, Hal Finkel <[hidden email]> wrote:
>>
>>
>> On 01/04/2017 11:43 AM, James Y Knight via cfe-dev wrote:
>>
>> On Wed, Jan 4, 2017 at 11:12 AM, Aaron Ballman via cfe-dev
>> <[hidden email]> wrote:
>>>
>>> So I would be opposed to ignoring those attributes in
>>>
>>> Sema (I think we should still warn when users do nonportable things),
>>> but in favor of not changing the optimizer to capitalize on this
>>> "opportunity."
>>
>>
>> I'd be opposed to ignoring the attributes only in some places and not in
>> others. It should be ignored totally, as if it wasn't present on those
>> functions. Doing anything else sends the wrong message -- that libc authors
>> should continue to use nonnull on these functions because they might be
>> helpful, and won't do anything bad.
>>
>>
>> I think that we have a responsibility to our users to continue to warn
>> (statically, in ubsan, etc.) on non-portable behavior, which this is and
>> will continue to be in practice for at least a decade or two, regardless of
>> the message we'd like to send libc authors. We cannot undo history here and
>> this will be relevant to production systems for at least a decade. We can
>> talk to libc developers directly -- they're a much smaller set -- and we can
>> pursue change at the standards level while still providing the most useful
>> set of tools to our users in the mean time.
>
>
> But, this is an entirely different question.
>
> - Should clang warn about non-portable usage of passing NULL to memcpy/etc?
>
> Sounds like a fine warning to add.
>
> - Should that warning be dependent on the libc headers having nonnull
> annotations on these functions, which will be used only for warnings, and
> ignored for semantics, on this given list of hardcoded functions?
>
> No.
>
> Firstly: I'd note that nearly all libc implementations don't use these
> attributes today. In some cases, because they've simply not thought about
> it, but in others because they explicitly decided to NOT break their users'
> code by introducing this problem! Glibc is the outlier, here.
>
> So: what portability do you want to warn for? Portability assuming the same
> libc, but a different compiler which might fail to ignore the nonnull
> attribute? Or portability to other libc? If the latter, depending on the
> nonnull attribute being present doesn't and can't work.

My preference is for portability for both, but you bring up a good
point about other libc implementations not being annotated. So long as
we retain the ability to tell users their code is not portable *and*
we get rid of the dangerous optimizations, I'm happy. I just don't
want to lose the diagnostics because of the optimizations.

~Aaron

> Secondly: if we already have a hardcoded list of functions to special case,
> that could just as well be used to generate a nonportable-stringfunc-null
> warning, as well.
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev



_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFC: do not optimize on basis of __attribute__((nonnull)) in glibc headers

Ivan Garramona via cfe-dev
FYI, this is landed in r298491; thanks for the reviews.

I'm hoping to have papers for standards bodies (and also useful for libc maintainers) by next week.

On Sat, Mar 18, 2017 at 2:36 AM Philip Reames <[hidden email]> wrote:
I'm in support of unblocking the LLVM side optimization changes.  I'm definitely not qualified to speak towards the direction of the clang changes though.  :)


Philip


On 03/16/2017 03:39 PM, Chandler Carruth via cfe-dev wrote:
Are there blockers to landing https://reviews.llvm.org/D30806 ?

I'm committed to following through with the respective standards bodies and anyone else we feel is prudent, but I would somewhat like to unblock the optimization work in this area in parallel as I suspect that the communication and establishment of a rational plan will require a lot of time.

On Tue, Mar 14, 2017 at 2:54 PM Chandler Carruth via cfe-dev <[hidden email]> wrote:
I'm happy to handle at least the standards side of this; I don't really have great connections on the libc front, but happy to help folks do that.

On Tue, Mar 14, 2017 at 2:35 PM James Y Knight <[hidden email]> wrote:
Just to reiterate...

I would really like to see us communicate this decision, with an appropriately persuasive argument, both to the glibc authors and to the standards committees, and treat this as a *transitional* hack, which is needed until glibc (and any other libc if there are any?) can remove the erroneous nonnull declarations from their headers, and everyone has upgraded.

On Thu, Mar 9, 2017 at 11:04 PM, Chandler Carruth <[hidden email]> wrote:
I've sent a patch to implement this here: https://reviews.llvm.org/D30806

It turns out to be both easier and I hope less controversial than I had imagined. Neither Clang nor LLVM ever actually got nonnull onto these declarations even when they were suitable annotated. We didn't carry that annotation across to the declaration. Oops.

So I've fixed that, but *also* disabled it for the libc functions that we have library builtin recognition for. We can extend this to cover any set of library functions folks want, just let me know. The use of it will even be correctly controlled by -fno-builtin and friends.

The net result is that this should not regress optimizations for *anyone*, and actually enable a bunch of optimizations outside of libc functions, while preserving safety on those libc functions.

We still have the nullability attributes which can be used to annotate more interfaces in a way that will provide warnings and static analysis aid, but not optimize on and so not run the risk of changing behavior of existing code. Those would be good candidates (IMO) for libc++ to use on any relevant interfaces which match the criteria outlined in this thread: pointers paired with a size should allow null+zero.

Hopefully this makes everyone happy, please feel free to chime in on the review thread if more discussion is needed.

On Wed, Jan 4, 2017 at 11:24 AM Aaron Ballman via cfe-dev <[hidden email]> wrote:
On Wed, Jan 4, 2017 at 2:01 PM, James Y Knight <[hidden email]> wrote:
>
>
> On Wed, Jan 4, 2017 at 12:54 PM, Hal Finkel <[hidden email]> wrote:
>>
>>
>> On 01/04/2017 11:43 AM, James Y Knight via cfe-dev wrote:
>>
>> On Wed, Jan 4, 2017 at 11:12 AM, Aaron Ballman via cfe-dev
>> <[hidden email]> wrote:
>>>
>>> So I would be opposed to ignoring those attributes in
>>>
>>> Sema (I think we should still warn when users do nonportable things),
>>> but in favor of not changing the optimizer to capitalize on this
>>> "opportunity."
>>
>>
>> I'd be opposed to ignoring the attributes only in some places and not in
>> others. It should be ignored totally, as if it wasn't present on those
>> functions. Doing anything else sends the wrong message -- that libc authors
>> should continue to use nonnull on these functions because they might be
>> helpful, and won't do anything bad.
>>
>>
>> I think that we have a responsibility to our users to continue to warn
>> (statically, in ubsan, etc.) on non-portable behavior, which this is and
>> will continue to be in practice for at least a decade or two, regardless of
>> the message we'd like to send libc authors. We cannot undo history here and
>> this will be relevant to production systems for at least a decade. We can
>> talk to libc developers directly -- they're a much smaller set -- and we can
>> pursue change at the standards level while still providing the most useful
>> set of tools to our users in the mean time.
>
>
> But, this is an entirely different question.
>
> - Should clang warn about non-portable usage of passing NULL to memcpy/etc?
>
> Sounds like a fine warning to add.
>
> - Should that warning be dependent on the libc headers having nonnull
> annotations on these functions, which will be used only for warnings, and
> ignored for semantics, on this given list of hardcoded functions?
>
> No.
>
> Firstly: I'd note that nearly all libc implementations don't use these
> attributes today. In some cases, because they've simply not thought about
> it, but in others because they explicitly decided to NOT break their users'
> code by introducing this problem! Glibc is the outlier, here.
>
> So: what portability do you want to warn for? Portability assuming the same
> libc, but a different compiler which might fail to ignore the nonnull
> attribute? Or portability to other libc? If the latter, depending on the
> nonnull attribute being present doesn't and can't work.

My preference is for portability for both, but you bring up a good
point about other libc implementations not being annotated. So long as
we retain the ability to tell users their code is not portable *and*
we get rid of the dangerous optimizations, I'm happy. I just don't
want to lose the diagnostics because of the optimizations.

~Aaron

> Secondly: if we already have a hardcoded list of functions to special case,
> that could just as well be used to generate a nonportable-stringfunc-null
> warning, as well.
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev



_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFC: do not optimize on basis of __attribute__((nonnull)) in glibc headers

Ivan Garramona via cfe-dev
Sounds great!

On Mar 22, 2017 5:22 AM, "Chandler Carruth" <[hidden email]> wrote:
FYI, this is landed in r298491; thanks for the reviews.

I'm hoping to have papers for standards bodies (and also useful for libc maintainers) by next week.

On Sat, Mar 18, 2017 at 2:36 AM Philip Reames <[hidden email]> wrote:
I'm in support of unblocking the LLVM side optimization changes.  I'm definitely not qualified to speak towards the direction of the clang changes though.  :)


Philip


On 03/16/2017 03:39 PM, Chandler Carruth via cfe-dev wrote:
Are there blockers to landing https://reviews.llvm.org/D30806 ?

I'm committed to following through with the respective standards bodies and anyone else we feel is prudent, but I would somewhat like to unblock the optimization work in this area in parallel as I suspect that the communication and establishment of a rational plan will require a lot of time.

On Tue, Mar 14, 2017 at 2:54 PM Chandler Carruth via cfe-dev <[hidden email]> wrote:
I'm happy to handle at least the standards side of this; I don't really have great connections on the libc front, but happy to help folks do that.

On Tue, Mar 14, 2017 at 2:35 PM James Y Knight <[hidden email]> wrote:
Just to reiterate...

I would really like to see us communicate this decision, with an appropriately persuasive argument, both to the glibc authors and to the standards committees, and treat this as a *transitional* hack, which is needed until glibc (and any other libc if there are any?) can remove the erroneous nonnull declarations from their headers, and everyone has upgraded.

On Thu, Mar 9, 2017 at 11:04 PM, Chandler Carruth <[hidden email]> wrote:
I've sent a patch to implement this here: https://reviews.llvm.org/D30806

It turns out to be both easier and I hope less controversial than I had imagined. Neither Clang nor LLVM ever actually got nonnull onto these declarations even when they were suitable annotated. We didn't carry that annotation across to the declaration. Oops.

So I've fixed that, but *also* disabled it for the libc functions that we have library builtin recognition for. We can extend this to cover any set of library functions folks want, just let me know. The use of it will even be correctly controlled by -fno-builtin and friends.

The net result is that this should not regress optimizations for *anyone*, and actually enable a bunch of optimizations outside of libc functions, while preserving safety on those libc functions.

We still have the nullability attributes which can be used to annotate more interfaces in a way that will provide warnings and static analysis aid, but not optimize on and so not run the risk of changing behavior of existing code. Those would be good candidates (IMO) for libc++ to use on any relevant interfaces which match the criteria outlined in this thread: pointers paired with a size should allow null+zero.

Hopefully this makes everyone happy, please feel free to chime in on the review thread if more discussion is needed.

On Wed, Jan 4, 2017 at 11:24 AM Aaron Ballman via cfe-dev <[hidden email]> wrote:
On Wed, Jan 4, 2017 at 2:01 PM, James Y Knight <[hidden email]> wrote:
>
>
> On Wed, Jan 4, 2017 at 12:54 PM, Hal Finkel <[hidden email]> wrote:
>>
>>
>> On 01/04/2017 11:43 AM, James Y Knight via cfe-dev wrote:
>>
>> On Wed, Jan 4, 2017 at 11:12 AM, Aaron Ballman via cfe-dev
>> <[hidden email]> wrote:
>>>
>>> So I would be opposed to ignoring those attributes in
>>>
>>> Sema (I think we should still warn when users do nonportable things),
>>> but in favor of not changing the optimizer to capitalize on this
>>> "opportunity."
>>
>>
>> I'd be opposed to ignoring the attributes only in some places and not in
>> others. It should be ignored totally, as if it wasn't present on those
>> functions. Doing anything else sends the wrong message -- that libc authors
>> should continue to use nonnull on these functions because they might be
>> helpful, and won't do anything bad.
>>
>>
>> I think that we have a responsibility to our users to continue to warn
>> (statically, in ubsan, etc.) on non-portable behavior, which this is and
>> will continue to be in practice for at least a decade or two, regardless of
>> the message we'd like to send libc authors. We cannot undo history here and
>> this will be relevant to production systems for at least a decade. We can
>> talk to libc developers directly -- they're a much smaller set -- and we can
>> pursue change at the standards level while still providing the most useful
>> set of tools to our users in the mean time.
>
>
> But, this is an entirely different question.
>
> - Should clang warn about non-portable usage of passing NULL to memcpy/etc?
>
> Sounds like a fine warning to add.
>
> - Should that warning be dependent on the libc headers having nonnull
> annotations on these functions, which will be used only for warnings, and
> ignored for semantics, on this given list of hardcoded functions?
>
> No.
>
> Firstly: I'd note that nearly all libc implementations don't use these
> attributes today. In some cases, because they've simply not thought about
> it, but in others because they explicitly decided to NOT break their users'
> code by introducing this problem! Glibc is the outlier, here.
>
> So: what portability do you want to warn for? Portability assuming the same
> libc, but a different compiler which might fail to ignore the nonnull
> attribute? Or portability to other libc? If the latter, depending on the
> nonnull attribute being present doesn't and can't work.

My preference is for portability for both, but you bring up a good
point about other libc implementations not being annotated. So long as
we retain the ability to tell users their code is not portable *and*
we get rid of the dangerous optimizations, I'm happy. I just don't
want to lose the diagnostics because of the optimizations.

~Aaron

> Secondly: if we already have a hardcoded list of functions to special case,
> that could just as well be used to generate a nonportable-stringfunc-null
> warning, as well.
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev




_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
12
Loading...