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

[Richard Smith]

LGTM

On Thu, Jan 31, 2013 at 11:39 PM, Nick Lewycky <nlewycky at google.com> wrote:
> 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
>
>