patch: new warning on ODR-use of undefined inline functions

[Nick Lewycky]

On 31 January 2013 19:28, Richard Smith <richard at metafoo.co.uk> wrote:

> On Thu, Jan 31, 2013 at 6:34 PM, Nick Lewycky <nlewycky at google.com> wrote:
> > On 30 January 2013 22:43, Richard Smith <richard at metafoo.co.uk> wrote:
> >>
> >> SortUndefinedButUsed is not a strict weak ordering. Consider:
> >>
> >>   A = {1, invalid}
> >>   B = {2, 1}
> >>   C = {0, 2}
> >>
> >> We have A < B (comparing first because A.second is invalid), B < C
> >> (comparing second), and C < A (comparing first because A.second is
> >> invalid).
> >
> >
> > Fixed. Thanks for catching that.
> >
> >> @@ -8428,12 +8436,12 @@
> >>    if (FD) {
> >>      FD->setBody(Body);
> >>
> >> -    // The only way to be included in UndefinedInternals is if there is
> >> an
> >> +    // The only way to be included in UndefinedButUsed is if there is
> an
> >>      // ODR-use before the definition. Avoid the expensive map lookup if
> >> this
> >>      // is the first declaration.
> >> -    if (FD->getPreviousDecl() != 0 && FD->getPreviousDecl()->isUsed()
> &&
> >> -        FD->getLinkage() != ExternalLinkage)
> >> -      UndefinedInternals.erase(FD);
> >> +    if (FD->getPreviousDecl() != 0 && FD->getPreviousDecl()->isUsed())
> >> +      if (FD->getLinkage() != ExternalLinkage && !FD->isInlined())
> >> +        UndefinedButUsed.erase(FD);
> >>
> >> Shouldn't this be (FD->getLinkage() != ExternalLinkage ||
> >> FD->getPreviousDecl()->isInlined()) ?
> >
> >
> > Right.
> >
> >> +namespace test7 {
> >> +  // It is invalid to redeclare a function inline after its definition,
> >> but we
> >> +  // do not diagnose this yet.
> >> +  void f();  // expected-warning{{inline function 'test7::f' is not
> >> defined}}
> >> +  void test() { f(); } // no used-here note.
> >> +  inline void f();
> >> +}
> >>
> >> This test doesn't redeclare a function inline after its definition,
> >> just after its use.
> >
> >
> > Erp, of course. Removed the stale comment.
> >
> >> On Wed, Jan 30, 2013 at 10:19 PM, Nick Lewycky <nlewycky at google.com>
> >> wrote:
> >> > This patch extends the tracking for used but undefined functions and
> >> > variables with internal linkage to also apply to inline functions.
> >> > Sema::UndefinedInternals is renamed Sema::UndefinedButUsed.
> >> >
> >> > Fixes PR14993. Please review! Please pay particular attention to the
> >> > behaviour of the error under C99 and GNU89 modes.
> >>
> >> I've not looked at this from the perspective of C yet, but it looks
> >> correct for C++ (modulo the above comments).
> >
> >
> > C99 has a similar rule:
> >
> >   "For a function with external linkage, the following restrictions
> apply:
> > If a function is declared with an inline function specifier, then it shall
> > also be defined in the same translation unit."
> >   - C99 6.7.4p6
> >
> > Of course, a function with internal linkage and isn't covered by the
> above,
> > but it doesn't matter since that's invalid for other reasons. Good news,
> in
> > C99 mode GCC warns on declaration without definition of  'inline',
> 'extern
> > inline' functions, and used but undefined 'static inline' functions.
> >
> > Bad news, GNU89 mode seems to permit it. 'static inline' functions get
> the
> > same warning as in C99 mode, but in practice you can define a function of
> > the same name in another TU, without 'static inline', and it links fine.
> > With 'extern inline' I think it's even deliberate.
> >
> > I've changed the patch to simply not warn on used but not defined
> > gnu89-inline functions.
>
> OK, fair enough.
>
>
> @@ -366,13 +366,17 @@
>  }
>
>  namespace {
> -  struct SortUndefinedInternal {
> +  struct SortUndefinedButUsed {
>      const SourceManager &SM;
> -    explicit SortUndefinedInternal(SourceManager &SM) : SM(SM) {}
> +    explicit SortUndefinedButUsed(SourceManager &SM) : SM(SM) {}
>
>      bool operator()(const std::pair<NamedDecl *, SourceLocation> &l,
>                      const std::pair<NamedDecl *, SourceLocation> &r)
> const {
> -      if (l.second != r.second)
> +      if (l.second.isValid() && !r.second.isValid())
> +        return true;
> +      if (!l.second.isValid() && r.second.isValid())
> +        return false;
> +      if (l.second != r.second && l.second.isValid() &&
> r.second.isValid())
>
> I think you can drop the && l.second.isValid() && r.second.isValid()
> here -- invalid source locations compare equal.
>

Done.

+++ lib/Sema/SemaDecl.cpp       (working copy)
> @@ -2215,6 +2215,14 @@
>      New->setType(QualType(NewType, 0));
>      NewQType = Context.getCanonicalType(New->getType());
>    }
> +
> +  // If this redeclaration makes the function inline, we may need to add
> it to
> +  // UndefinedButUsed.
> +  if (!Old->isInlined() && New->isInlined() &&
> +      Old->isUsed(false) &&
> +      !Old->isDefined() && !New->isThisDeclarationADefinition())
> +    UndefinedButUsed.insert(std::make_pair(Old->getCanonicalDecl(),
> +                                           SourceLocation()));
>
> Do you need a GNU inline check here?
>

Yep.

Also, what happens if a redeclaration adds the gnu_inline attribute to
> a function which is already declared inline? In g++ that's an error,
> but Clang appears to allow it.
>

Yep again. See the new tests!

Nick
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130131/6f6a9920/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: undefined-but-used-3.patch
Type: application/octet-stream
Size: 20261 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130131/6f6a9920/attachment.obj>