Skip to content

Commit

Permalink
[C89/C2x] Improve diagnostics around strict prototypes in C
Browse files Browse the repository at this point in the history 
Functions without prototypes in C (also known as K&R C functions) were
introduced into C89 as a deprecated feature and C2x is now reclaiming
that syntax space with different semantics. However, Clang's
-Wstrict-prototypes diagnostic is off-by-default (even in pedantic
mode) and does not suffice to warn users about issues in their code.

This patch changes the behavior of -Wstrict-prototypes to only diagnose
declarations and definitions which are not going to change behavior in
C2x mode, and enables the diagnostic in -pedantic mode. The diagnostic
is now specifically about the fact that the feature is deprecated.

It also adds -Wdeprecated-non-prototype, which is grouped under
-Wstrict-prototypes and diagnoses declarations or definitions which
will change behavior in C2x mode. This diagnostic is enabled by default
because the risk is higher for the user to continue to use the
deprecated feature.

Differential Revision: https://reviews.llvm.org/D122895
  • Loading branch information
@AaronBallman
AaronBallman committed Apr 8, 2022
1 parent 4aaf25b commit 11da1b5
Show file tree
Hide file tree
Showing 24 changed files with 363 additions and 128 deletions.
9 changes: 9 additions & 0 deletions clang/docs/ReleaseNotes.rst
View file
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,15 @@ Improvements to Clang's diagnostics
- ``-Wunused-variable`` no longer warn for references extending the lifetime
of temporaries with side effects. This fixes `Issue 54489
<https://github.com/llvm/llvm-project/issues/54489>`_.
- Modified the behavior of ``-Wstrict-prototypes`` and added a new, related
diagnostic ``-Wdeprecated-non-prototype``. The strict prototypes warning will
now only diagnose deprecated declarations and definitions of functions
without a prototype where the behavior in C2x will remain correct. This
diagnostic remains off by default but is now enabled via ``-pedantic`` due to
it being a deprecation warning. ``-Wdeprecated-non-prototype`` will diagnose
cases where the deprecated declarations or definitions of a function without
a prototype will change behavior in C2x. This diagnostic is grouped under the
``-Wstrict-prototypes`` warning group, but is enabled by default.

Non-comprehensive list of changes in this release
-------------------------------------------------
Expand Down
2 changes: 2 additions & 0 deletions clang/include/clang/Basic/DiagnosticGroups.td
View file
Original file line number Diff line number Diff line change
Expand Up @@ -444,6 +444,8 @@ def InlineNamespaceReopenedNoninline
def InvalidNoreturn : DiagGroup<"invalid-noreturn">;
def InvalidSourceEncoding : DiagGroup<"invalid-source-encoding">;
def KNRPromotedParameter : DiagGroup<"knr-promoted-parameter">;
def DeprecatedNonPrototype : DiagGroup<"deprecated-non-prototype">;
def StrictPrototypes : DiagGroup<"strict-prototypes", [DeprecatedNonPrototype]>;
def : DiagGroup<"init-self">;
def : DiagGroup<"inline">;
def : DiagGroup<"invalid-pch">;
Expand Down
14 changes: 10 additions & 4 deletions clang/include/clang/Basic/DiagnosticSemaKinds.td
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5560,10 +5560,16 @@ def warn_missing_prototype : Warning<
def note_declaration_not_a_prototype : Note<
"this declaration is not a prototype; add %select{'void'|parameter declarations}0 "
"to make it %select{a prototype for a zero-parameter function|one}0">;
def warn_strict_prototypes : Warning<
"this %select{function declaration is not|block declaration is not|"
"old-style function definition is not preceded by}0 a prototype">,
InGroup<DiagGroup<"strict-prototypes">>, DefaultIgnore;
// This is not actually an extension, but we only want it to be enabled in
// -pedantic mode and this is the most direct way of accomplishing that.
def warn_strict_prototypes : Extension<
"a %select{function|block}0 declaration without a prototype is deprecated "
"%select{in all versions of C|}0">, InGroup<StrictPrototypes>;
def warn_non_prototype_changes_behavior : Warning<
"a function declaration without a prototype is deprecated in all versions of "
"C and is not supported in C2x">, InGroup<DeprecatedNonPrototype>;
def note_func_decl_changes_behavior : Note<
"a function declaration without a prototype is not supported in C2x">;
def warn_missing_variable_declarations : Warning<
"no previous extern declaration for non-static variable %0">,
InGroup<DiagGroup<"missing-variable-declarations">>, DefaultIgnore;
Expand Down
202 changes: 163 additions & 39 deletions clang/lib/Sema/SemaDecl.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3879,39 +3879,118 @@ bool Sema::MergeFunctionDecl(FunctionDecl *New, NamedDecl *&OldD,

// C: Function types need to be compatible, not identical. This handles
// duplicate function decls like "void f(int); void f(enum X);" properly.
if (!getLangOpts().CPlusPlus &&
Context.typesAreCompatible(OldQType, NewQType)) {
const FunctionType *OldFuncType = OldQType->getAs<FunctionType>();
const FunctionType *NewFuncType = NewQType->getAs<FunctionType>();
const FunctionProtoType *OldProto = nullptr;
if (MergeTypeWithOld && isa<FunctionNoProtoType>(NewFuncType) &&
(OldProto = dyn_cast<FunctionProtoType>(OldFuncType))) {
// The old declaration provided a function prototype, but the
// new declaration does not. Merge in the prototype.
assert(!OldProto->hasExceptionSpec() && "Exception spec in C");
SmallVector<QualType, 16> ParamTypes(OldProto->param_types());
NewQType =
Context.getFunctionType(NewFuncType->getReturnType(), ParamTypes,
OldProto->getExtProtoInfo());
New->setType(NewQType);
New->setHasInheritedPrototype();

// Synthesize parameters with the same types.
SmallVector<ParmVarDecl*, 16> Params;
for (const auto &ParamType : OldProto->param_types()) {
ParmVarDecl *Param = ParmVarDecl::Create(Context, New, SourceLocation(),
SourceLocation(), nullptr,
ParamType, /*TInfo=*/nullptr,
SC_None, nullptr);
Param->setScopeInfo(0, Params.size());
Param->setImplicit();
Params.push_back(Param);
if (!getLangOpts().CPlusPlus) {
// If we are merging two functions where only one of them has a prototype,
// we may have enough information to decide to issue a diagnostic that the
// function without a protoype will change behavior in C2x. This handles
// cases like:
// void i(); void i(int j);
// void i(int j); void i();
// void i(); void i(int j) {}
// See ActOnFinishFunctionBody() for other cases of the behavior change
// diagnostic. See GetFullTypeForDeclarator() for handling of a function
// type without a prototype.
if (New->hasWrittenPrototype() != Old->hasWrittenPrototype() &&
!New->isImplicit() && !Old->isImplicit()) {
const FunctionDecl *WithProto, *WithoutProto;
if (New->hasWrittenPrototype()) {
WithProto = New;
WithoutProto = Old;
} else {
WithProto = Old;
WithoutProto = New;
}

New->setParams(Params);
if (WithProto->getNumParams() != 0) {
// The function definition has parameters, so this will change
// behavior in C2x.
//
// If we already warned about about the function without a prototype
// being deprecated, add a note that it also changes behavior. If we
// didn't warn about it being deprecated (because the diagnostic is
// not enabled), warn now that it is deprecated and changes behavior.
bool AddNote = false;
if (Diags.isIgnored(diag::warn_strict_prototypes,
WithoutProto->getLocation())) {
if (WithoutProto->getBuiltinID() == 0 &&
!WithoutProto->isImplicit() &&
SourceMgr.isBeforeInTranslationUnit(WithoutProto->getLocation(),
WithProto->getLocation())) {
PartialDiagnostic PD =
PDiag(diag::warn_non_prototype_changes_behavior);
if (TypeSourceInfo *TSI = WithoutProto->getTypeSourceInfo()) {
if (auto FTL = TSI->getTypeLoc().getAs<FunctionNoProtoTypeLoc>())
PD << FixItHint::CreateInsertion(FTL.getRParenLoc(), "void");
}
Diag(WithoutProto->getLocation(), PD);
}
} else {
AddNote = true;
}

// Because the function with a prototype has parameters but a previous
// declaration had none, the function with the prototype will also
// change behavior in C2x.
if (WithProto->getBuiltinID() == 0 && !WithProto->isImplicit()) {
if (SourceMgr.isBeforeInTranslationUnit(
WithProto->getLocation(), WithoutProto->getLocation())) {
// If the function with the prototype comes before the function
// without the prototype, we only want to diagnose the one without
// the prototype.
Diag(WithoutProto->getLocation(),
diag::warn_non_prototype_changes_behavior);
} else {
// Otherwise, diagnose the one with the prototype, and potentially
// attach a note to the one without a prototype if needed.
Diag(WithProto->getLocation(),
diag::warn_non_prototype_changes_behavior);
if (AddNote && WithoutProto->getBuiltinID() == 0)
Diag(WithoutProto->getLocation(),
diag::note_func_decl_changes_behavior);
}
} else if (AddNote && WithoutProto->getBuiltinID() == 0 &&
!WithoutProto->isImplicit()) {
// If we were supposed to add a note but the function with a
// prototype is a builtin or was implicitly declared, which means we
// have nothing to attach the note to, so we issue a warning instead.
Diag(WithoutProto->getLocation(),
diag::warn_non_prototype_changes_behavior);
}
}
}

return MergeCompatibleFunctionDecls(New, Old, S, MergeTypeWithOld);
if (Context.typesAreCompatible(OldQType, NewQType)) {
const FunctionType *OldFuncType = OldQType->getAs<FunctionType>();
const FunctionType *NewFuncType = NewQType->getAs<FunctionType>();
const FunctionProtoType *OldProto = nullptr;
if (MergeTypeWithOld && isa<FunctionNoProtoType>(NewFuncType) &&
(OldProto = dyn_cast<FunctionProtoType>(OldFuncType))) {
// The old declaration provided a function prototype, but the
// new declaration does not. Merge in the prototype.
assert(!OldProto->hasExceptionSpec() && "Exception spec in C");
SmallVector<QualType, 16> ParamTypes(OldProto->param_types());
NewQType =
Context.getFunctionType(NewFuncType->getReturnType(), ParamTypes,
OldProto->getExtProtoInfo());
New->setType(NewQType);
New->setHasInheritedPrototype();

// Synthesize parameters with the same types.
SmallVector<ParmVarDecl *, 16> Params;
for (const auto &ParamType : OldProto->param_types()) {
ParmVarDecl *Param = ParmVarDecl::Create(
Context, New, SourceLocation(), SourceLocation(), nullptr,
ParamType, /*TInfo=*/nullptr, SC_None, nullptr);
Param->setScopeInfo(0, Params.size());
Param->setImplicit();
Params.push_back(Param);
}

New->setParams(Params);
}

return MergeCompatibleFunctionDecls(New, Old, S, MergeTypeWithOld);
}
}

// Check if the function types are compatible when pointer size address
Expand Down Expand Up @@ -14838,18 +14917,63 @@ Decl *Sema::ActOnFinishFunctionBody(Decl *dcl, Stmt *Body,
? FixItHint::CreateInsertion(findBeginLoc(), "static ")
: FixItHint{});
}
}

// GNU warning -Wstrict-prototypes
// Warn if K&R function is defined without a previous declaration.
// This warning is issued only if the definition itself does not
// provide a prototype. Only K&R definitions do not provide a
// prototype.
if (!FD->hasWrittenPrototype()) {
TypeSourceInfo *TI = FD->getTypeSourceInfo();
TypeLoc TL = TI->getTypeLoc();
FunctionTypeLoc FTL = TL.getAsAdjusted<FunctionTypeLoc>();
Diag(FTL.getLParenLoc(), diag::warn_strict_prototypes) << 2;
// If the function being defined does not have a prototype, then we may
// need to diagnose it as changing behavior in C2x because we now know
// whether the function accepts arguments or not. This only handles the
// case where the definition has no prototype but does have parameters
// and either there is no previous potential prototype, or the previous
// potential prototype also has no actual prototype. This handles cases
// like:
// void f(); void f(a) int a; {}
// void g(a) int a; {}
// See MergeFunctionDecl() for other cases of the behavior change
// diagnostic. See GetFullTypeForDeclarator() for handling of a function
// type without a prototype.
if (!FD->hasWrittenPrototype() && FD->getNumParams() != 0 &&
(!PossiblePrototype || (!PossiblePrototype->hasWrittenPrototype() &&
!PossiblePrototype->isImplicit()))) {
// The function definition has parameters, so this will change behavior
// in C2x. If there is a possible prototype, it comes before the
// function definition.
// FIXME: The declaration may have already been diagnosed as being
// deprecated in GetFullTypeForDeclarator() if it had no arguments, but
// there's no way to test for the "changes behavior" condition in
// SemaType.cpp when forming the declaration's function type. So, we do
// this awkward dance instead.
//
// If we have a possible prototype and it declares a function with a
// prototype, we don't want to diagnose it; if we have a possible
// prototype and it has no prototype, it may have already been
// diagnosed in SemaType.cpp as deprecated depending on whether
// -Wstrict-prototypes is enabled. If we already warned about it being
// deprecated, add a note that it also changes behavior. If we didn't
// warn about it being deprecated (because the diagnostic is not
// enabled), warn now that it is deprecated and changes behavior.
bool AddNote = false;
if (PossiblePrototype) {
if (Diags.isIgnored(diag::warn_strict_prototypes,
PossiblePrototype->getLocation())) {

PartialDiagnostic PD =
PDiag(diag::warn_non_prototype_changes_behavior);
if (TypeSourceInfo *TSI = PossiblePrototype->getTypeSourceInfo()) {
if (auto FTL = TSI->getTypeLoc().getAs<FunctionNoProtoTypeLoc>())
PD << FixItHint::CreateInsertion(FTL.getRParenLoc(), "void");
}
Diag(PossiblePrototype->getLocation(), PD);
} else {
AddNote = true;
}
}

// Because this function definition has no prototype and it has
// parameters, it will definitely change behavior in C2x.
Diag(FD->getLocation(), diag::warn_non_prototype_changes_behavior);
if (AddNote)
Diag(PossiblePrototype->getLocation(),
diag::note_func_decl_changes_behavior);
}

// Warn on CPUDispatch with an actual body.
Expand Down
7 changes: 4 additions & 3 deletions clang/lib/Sema/SemaType.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5548,15 +5548,16 @@ static TypeSourceInfo *GetFullTypeForDeclarator(TypeProcessingState &state,
diag::warn_noderef_on_non_pointer_or_array);

// GNU warning -Wstrict-prototypes
// Warn if a function declaration is without a prototype.
// Warn if a function declaration or definition is without a prototype.
// This warning is issued for all kinds of unprototyped function
// declarations (i.e. function type typedef, function pointer etc.)
// C99 6.7.5.3p14:
// The empty list in a function declarator that is not part of a definition
// of that function specifies that no information about the number or types
// of the parameters is supplied.
if (!LangOpts.CPlusPlus &&
D.getFunctionDefinitionKind() == FunctionDefinitionKind::Declaration) {
// See ActOnFinishFunctionBody() and MergeFunctionDecl() for handling of
// function declarations whose behavior changes in C2x.
if (!LangOpts.CPlusPlus) {
bool IsBlock = false;
for (const DeclaratorChunk &DeclType : D.type_objects()) {
switch (DeclType.Kind) {
Expand Down
5 changes: 2 additions & 3 deletions clang/test/CodeGen/2009-06-01-addrofknr.c
View file
Original file line number Diff line number Diff line change
@@ -1,12 +1,11 @@
// RUN: %clang_cc1 %s -o %t -emit-llvm -verify
// expected-no-diagnostics
// RUN: %clang_cc1 %s -o %t -emit-llvm -verify -std=c89
// PR4289

struct funcptr {
int (*func)();
};

static int func(f)
static int func(f) // expected-warning {{a function declaration without a prototype is deprecated in all versions of C and is not supported in C2x}}
void *f;
{
return 0;
Expand Down
4 changes: 2 additions & 2 deletions clang/test/FixIt/fixit.c
View file
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
// RUN: %clang_cc1 -pedantic -Wunused-label -verify -x c %s
// RUN: %clang_cc1 -pedantic -Wunused-label -Wno-deprecated-non-prototype -verify -x c %s
// RUN: cp %s %t
// RUN: not %clang_cc1 -pedantic -Wunused-label -fixit -x c %t
// RUN: %clang_cc1 -pedantic -Wunused-label -Werror -x c %t
// RUN: %clang_cc1 -pedantic -Wunused-label -Wno-deprecated-non-prototype -Werror -x c %t
// RUN: grep -v CHECK %t | FileCheck %t

/* This is a test of the various code modification hints that are
Expand Down

4 comments on commit 11da1b5

@XrXr
Copy link

@XrXr XrXr commented on 11da1b5 Apr 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I seem to have found a weird interaction between -Wdeprecated-non-prototype and __attribute__((__noreturn__)).
The following issues the warning but it doesn't seem like it should:

typedef unsigned long VALUE;
typedef unsigned long ID;

typedef void rb_gvar_setter_t(VALUE val, ID id, VALUE *data);

__attribute__((__noreturn__))
rb_gvar_setter_t rb_gvar_readonly_setter;

void rb_gvar_readonly_setter(VALUE v, ID id, VALUE *_);

Removing the attribute avoids the warning.
Compiler Explorer link: https://godbolt.org/z/6WEWv3na8

@AaronBallman
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well that's certainly a unique diagnostic to get there. I'll look into it, thank you for bringing it to my attention!

@AaronBallman
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@XrXr -- That issue should now be fixed with 8fd3b5d, thanks for letting me know!

@XrXr
Copy link

@XrXr XrXr commented on 11da1b5 Apr 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AaronBallman Thank you for being so responsive and addressing this so quickly!

Please sign in to comment.