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

[Chandler Carruth 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 <jyknight at google.com> 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 <chandlerc at google.com>
> 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 <
> cfe-dev at lists.llvm.org> wrote:
>
> On Wed, Jan 4, 2017 at 2:01 PM, James Y Knight <jyknight at google.com>
> wrote:
> >
> >
> > On Wed, Jan 4, 2017 at 12:54 PM, Hal Finkel <hfinkel at anl.gov> 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
> >> <cfe-dev at lists.llvm.org> 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
> cfe-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20170314/e8de8899/attachment.html>