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

[Richard Smith]

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.


+++ 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?

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.