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

[Nick Lewycky]

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.

Nick

 > Also, there's a comment that these should be errors. That turned out to
> not
> > be entirely trivial due to some codegen tests, so it's left out of this
> > patch.
> >
> > Nick
> >
> >
> > _______________________________________________
> > cfe-commits mailing list
> > cfe-commits at cs.uiuc.edu
> > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130131/50c2e7e6/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: undefined-but-used-2.patch
Type: application/octet-stream
Size: 19826 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130131/50c2e7e6/attachment.obj>