Add warning to detect when calls passing arguments are made to functions without prototypes.

[delcypher]

Summary

The warning is currently disabled by default but can be enabled with
-Wstrict-calls-without-prototype. Enabling it by default will be
handled by future patches.

The warning is intended to catch C code like this:

int f(); // No prototype, accepts any number of arguments of any type.

int call(void) {
  return f(1, 2, 3);
}

While code like this is legal it can very easily invoke undefined behavior
by passing the wrong number of arguments or wrong types.

The warning only fires when arguments are passed to make it less noisy, i.e.
so that the warning does not fire on code like this:

int g();
int call(void) {
    return g();
}

While the above code could invoke undefined behavior, if the definition
of g() takes no arguments then the lack of a prototype is benign.
Writing function declarations that are not prototypes is also quite
common when the intention is for it to be a zero argument function
because it's easy to forget to add the void.

This warning while similar to -Wstrict-prototypes is distinct because
it warns at call sites rather at declarations.

This patch currently warns on calls to K&R style function declarations where a
previous declaration has a prototype. It is currently not clear if this is the correct behavior
as noted in the included test case.

rdar://87118271

[delcypher]

Add another test case for function definitions without prototypes.

[delcypher]

clang/test/Sema/warn-calls-without-prototype.c

39

@NoQ Any ideas about this? It seems kind of weird that when merging not_a_prototype3 prototype with the K&R style definition of not_a_prototype3 that the resulting FunctionDecl we see at the call site in call_to_function_without_prototype3 is marked as not having a prototype.

If I flip the order (see not_a_prototype6) then the merged declaration is marked as having a prototype.

I'm not sure if this is a bug in Sema::MergeFunctionDecl or if this just a peculiarity of K&R style function definitions.

[delcypher]

Patch https://reviews.llvm.org/D116636 demonstrates the changes needed if the warning was enabled by default:

[delcypher]

clang/test/Sema/warn-calls-without-prototype.c

39

I suspect the problem might be here in Sema::MergeFunctionDecl.

 // 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.

isa<FunctionNoProtoType>(NewFuncType) is false in this particular case, however New doesn't have a prototype (i.e. New->hasPrototype() is false). One fix might be to replace isa<FunctionNoProtoType>(NewFuncType) with

(isa<FunctionNoProtoType>(NewFuncType) || !New->hasPrototype())

However, I don't really know this code well enough to know if that's the right fix.

[fcloutier]

I think that this is a good warning and I'll defer to the experts for what has to happen when prototypes merge with K&R definitions :)

clang/include/clang/Basic/DiagnosticSemaKinds.td

5530

sp: I think this should be called "strict-call-without-prototype" (singular call) as most warning flag names seem to prefer singulars.

[delcypher]

clang/include/clang/Basic/DiagnosticSemaKinds.td

5530

@fcloutier
I'm open to renaming the warning. I originally made the warning plural because there were warnings near by that used it. E.g.

• missing-variable-declarations
• strict-prototypes
• missing-prototypes

If no one else chimes in I'll make it singular.

Also, do you think we should drop the "strict" prefix? I.e. call-without-prototype instead of strict-call-without-prototype. The reason I'm thinking that using strict might be a bad idea is:

1. The warning isn't actually strict. A strict version of the warning would warn on all calls to functions without prototypes. We've decided to only warn on calls with arguments to avoid noisy warnings that result from people frequently forgetting to explicitly mark functions as taking no arguments.

2. Calling this warning strict-call-without-prototype precludes from adding a stricter version of warning in the future (i.e. we'd be force to name it something like strict-strict-call-without-prototype which is super confusing).

3. The intention in a future patch is to enable the warning by default. It seems odd to have a "strict" warning on by default given that there is precedence for doing the opposite (e.g. -Wstrict-prototypes is off by default).

[delcypher]

clang/test/Sema/warn-calls-without-prototype.c

39

Okay. I think the above would actually be the wrong location for a fix because in this case we don't need to go down the path that synthesizes the parameters because we already know them for both old and new in this situation.

Instead I think the change would have to be in Sema::MergeCompatibleFunctionDecls to do something like.

// If New is a K&R function definition it will be marked
// as not having a prototype. If `Old` has a prototype
// then to "merge" we should mark the K&R function as having a prototype.
if (!getLangOpts().CPlusPlus && Old->hasPrototype() && !New->hasPrototype())
  New->setHasInheritedPrototype();

What I'm not sure about is if this is semantically the right thing to do. Thoughts?

[NoQ]

clang/test/Sema/warn-calls-without-prototype.c

39

Ok dunno but I definitely find this whole thing surprising. I'd expect this example to be the entirely normal situation for this code, where it sees that the new declaration has no prototype so it "inherits" it from the old declaration. But you're saying that

isa<FunctionNoProtoType>(NewFuncType) is false in this particular case

Where does that proto-type come from then? I only see this code affecting the type

3523   if (RequiresAdjustment) {
3524     const FunctionType *AdjustedType = New->getType()->getAs<FunctionType>();
3525     AdjustedType = Context.adjustFunctionType(AdjustedType, NewTypeInfo);
3526     New->setType(QualType(AdjustedType, 0));
3527     NewQType = Context.getCanonicalType(New->getType());
3528   }

which doesn't seem to touch no-proto-types:

3094 const FunctionType *ASTContext::adjustFunctionType(const FunctionType *T,
3095                                                    FunctionType::ExtInfo Info) {
3096   if (T->getExtInfo() == Info)
3097     return T;
3098
3099   QualType Result;
3100   if (const auto *FNPT = dyn_cast<FunctionNoProtoType>(T)) {
3101     Result = getFunctionNoProtoType(FNPT->getReturnType(), Info);
...
3107   }
3108
3109   return cast<FunctionType>(Result.getTypePtr());
3110 }

So it sounds like New->getType() was already with prototype from the start? Maybe whatever code has set that type should also have set the HasInheritedPrototype flag?

[aaron.ballman]

clang/include/clang/Basic/DiagnosticSemaKinds.td

5529

This diagnostic doesn't tell me what's wrong with the code (and in fact, there's very possibly nothing wrong with the code whatsoever). Further, why does calling a function *with no arguments* matter when it has no prototype? I would imagine this should flag any call to a function without a prototype given that the function without a prototype may still expect arguments. e.g.,

// Header.h
int f();

// Source.c
int f(a) int a; { ... }

// Caller.c
#include "Header.h"

int main() {
  return f();
}

I think the diagnostic text should be updated to something more like cannot verify %0 is being called with the correct number or %plural{1:type|:types}1 of arguments because it was declared without a prototype (or something along those lines that explains what's wrong with the code).

clang/lib/Sema/SemaExpr.cpp

6391

clang/test/Sema/warn-calls-without-prototype.c

39

@NoQ Any ideas about this? It seems kind of weird that when merging not_a_prototype3 prototype with the K&R style definition of not_a_prototype3 that the resulting FunctionDecl we see at the call site in call_to_function_without_prototype3 is marked as not having a prototype.

I am reasonably certain this is not a bug, but C being weird: https://godbolt.org/z/sGj5aejrY.

I'd have to double-check what C89 says, but IIRC, the only thing that was specified is how to tell whether the two types are compatible, and it's left implicit that the definition is the final arbiter of the function type.

152

Please add a newline to the end of this file.

[delcypher]

clang/test/Sema/warn-calls-without-prototype.c

39

Ok dunno but I definitely find this whole thing surprising. I'd expect this example to be the entirely normal situation for this code, where it sees that the new declaration has no prototype so it "inherits" it from the old declaration. But you're saying that

isa<FunctionNoProtoType>(NewFuncType) is false in this particular case

Yep.

(lldb) p NewFuncType->dump()
FunctionProtoType 0x128829430 'int (int, int)' cdecl
|-BuiltinType 0x12782bf10 'int'
|-BuiltinType 0x12782bf10 'int'
`-BuiltinType 0x12782bf10 'int'
(lldb) p OldFuncType->dump()
FunctionProtoType 0x128829430 'int (int, int)' cdecl
|-BuiltinType 0x12782bf10 'int'
|-BuiltinType 0x12782bf10 'int'
`-BuiltinType 0x12782bf10 'int'

I think we might be confusing the FunctionNoProtoType type and what the FunctionDecl::hasPrototype() method does.

FunctionNoProtoType

I'm not 100% sure about this but I suspect FunctionNoProtoType exists only for functions without any prototype that are written like this.

int function();

which would make some sense given what this code tries to do.

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.
  ...

i.e. I think it's supposed to handle code like this

int foo(int, int, int); // prototype
int foo(); // has no prototype, but when we merge decls it inherits the prototype from the previous decl.

FunctionDecl::hasPrototype()

This method is implemented as

bool hasPrototype() const {
  return hasWrittenPrototype() || hasInheritedPrototype();
}

and those implementations get the data from data in the class without examining its FunctionType. So it seems a FunctionDecl's property of having a prototype is independent of that FunctionDecl's FunctionType.

K&R function declarations

So it sounds like New->getType() was already with prototype from the start? Maybe whatever code has set that type should also have set the HasInheritedPrototype flag?

The nasty thing about K&R function declarations is they are treated as not having a prototype according to the C11 standard (6.9.1.13) .

extern int max(int a, int b)
{
  return a > b ? a : b;
}

extern int max(a, b)
      int a, b;
{
  return a > b ? a : b;
}

Here int a, b; is the declaration list for the parameters. The difference between these two definitions is that the first form acts as a prototype declaration that forces conversion of the arguments of subsequent calls to the function, whereas the second form does not.

As much as I hate this, given the above it makes to make that in Sema::MergeFunctionDecl New->hasPrototype() returns false. So in my example

unsigned not_a_prototype3(int a, int b, int c); // "Old" has a prototype
unsigned not_a_prototype3(a, b, c ) // "New" has no prototype
  int a;
  int b;
  int c;
{
  return a + b +c;
}

The question is, what should we do when trying to merge these two FunctionDecls?

[delcypher]

clang/include/clang/Basic/DiagnosticSemaKinds.td

5529

@aaron.ballman Thanks for the helpful feedback.

This diagnostic doesn't tell me what's wrong with the code (and in fact, there's very possibly nothing wrong with the code whatsoever).

That's a fair criticism. I think the diagnostic message you suggest is a lot more helpful so I'll go for something like that.

Further, why does calling a function *with no arguments* matter when it has no prototype?

The reason was to avoid the warning being noisy. E.g. I didn't the warning to fire in this situation.

// Header.h
int f(); // The user forgot to put `void` between parentheses

// Source.c
int f(void) { ... }

// Caller.c
#include "Header.h"

int main() {
  return f();
}

Forgetting to put void in the declaration of f() is a pretty common thing to do because a lot of people read int f() as declaring a function that takes no arguments (it does in C++ but not in C).

I don't want the warning to be noisy because I was planning on switching it on by default in open source and in a downstream use-case make it an error.

How about this as a compromise? Split the warning into two separate warnings

• strict-call-without-prototype - Warns on calls to functions without a prototype when no arguments are passed. Not enabled by default
• call-without-prototype -Warns on calls to functions without a prototype when arguments are passed. Enable this by default.

Alternatively we could enable both by default. That would still allow me to make call-without-prototype an error and strict-call-without-prototype not be an error for my downstream use-case.

Thoughts?

[aaron.ballman]

clang/include/clang/Basic/DiagnosticSemaKinds.td

5529

Forgetting to put void in the declaration of f() is a pretty common thing to do because a lot of people read int f() as declaring a function that takes no arguments (it does in C++ but not in C).

Yup, and this is exactly why I think we *should* be warning. That is a function without a prototype, so the code is very brittle and dangerous at the call site. The fact that the call site *currently* is correct doesn't mean it's *intentionally* correct. e.g.,

// Header.h
int f(); // No prototype

// Source.c
int f(int a, int b) { return 0; } // Has a prototype, no diagnostic

// OtherSource.c
#include "Header.h"

int main() {
  return f(); // No diagnostic with this patch, but still have the UB.
}

I don't want the warning to be noisy because I was planning on switching it on by default in open source and in a downstream use-case make it an error.

Hmmm. Thinking out loud here.

Functions without prototypes were standardized in C89 as a deprecated feature (C89 3.9.4, 3.9.5). I'd like to get to the point where any code that doesn't pass -ansi is given a diagnostic (at least in pedantic mode outside of system headers) about this deprecation, though I could probably be persuaded to keep not diagnose in c89 mode if that's a massive pain point. But if in C99 or later, I definitely see no reason not to diagnose the declarations as deprecated by default.

However, calling a function without a prototype declaration is not itself indicative of a programming mistake and is also not deprecated (it just stops being a problem once all functions are required to have a prototype), so I'm not certain it's well-motivated to enable the new diagnostic by default. This is a bit more like use of VLAs, in that it's a common situation to accidentally declare a function without a prototype. So having a "congrats, you're using this feature" warning (like we did for -Wvla) for people who don't want to accidentally use it seems reasonable. But "use" is a bit weird here -- this flags call sites but the issue is with the declaration of the function, not with its callers.

So I'm more of the opinion that we should be strengthening the diagnostics here rather than weakening them, and I sort of think we should be focusing on the declarations and not the call expressions. As a WG14 member for the community, I'm definitely motivated to see us be more aggressive here because of proposals like: http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2841.htm. The committee is trying to remove support for function declarations without prototypes, and the empty paren case is basically the final sticking point. Diagnosing it more appropriately to our users would help avoid nasty surprises.

Have you considered whether you could stomach strengthening -Wstrict-prototypes by enabling it by default outside of -ansi (or perhaps -std=c89)? I know this does not match GCC's behavior, but IIRC, GCC's behavior came about because they implemented -Wstrict-prototypes in around 1990, aka, just as prototypes were being deprecated in C (so it would have been incredibly disruptive to enable it at that point).

Alternatively we could enable both by default. That would still allow me to make call-without-prototype an error and strict-call-without-prototype not be an error for my downstream use-case.

We could definitely split the diagnostic into two if we're convinced that diagnosing call sites is the appropriate action to take.

[delcypher]

clang/include/clang/Basic/DiagnosticSemaKinds.td

5529

However, calling a function without a prototype declaration is not itself indicative of a programming mistake and is also not deprecated (it just stops being a problem once all functions are required to have a prototype), so I'm not certain it's well-motivated to enable the new diagnostic by default

True. It's not necessarily a mistake but calling functions without a prototype is very error prone due the lack of argument count and type checking. This is why I think it might be worth flagging the potential problem by default. I'm happy to not have it on by default if that is the general consensus.

But "use" is a bit weird here -- this flags call sites but the issue is with the declaration of the function, not with its callers.

You're right that the underlying issue is at the declaration and not at the call sites. However, if I wanted to warn about all the declarations I would just use -Wstrict-prototypes which is already implemented in Clang. I don't consider that warning very pragmatic because it'll warn about functions that I'm not calling which could make it extremely noisy. Instead I wanted a more pragmatic (less noisy) warning. I consider what I'm proposing to be more pragmatic because if a function is missing a prototype, it only matters when it is called (i.e. this is where the lack of a prototype will cause problems if the arguments/types aren't "just right"). If I'm not calling a function I don't want to be told its missing a prototype because it does not matter for the current compilation unit.

Have you considered whether you could stomach strengthening -Wstrict-prototypes by enabling it by default outside of -ansi (or perhaps -std=c89)?

As I said above I don't think -Wstrict-prototypes is very pragmatic. It's probably good if you're trying to audit a header file, but noisy if you're actually trying to compile code.

Thinking about it I guess what I've proposed is a complement to -Wstrict-prototypes. I don't see why clang couldn't have both warnings.

[delcypher]

clang/test/Sema/warn-calls-without-prototype.c

39

@aaron.ballman Thanks for chiming in. I'll update the test cases to assume this is the intended behavior.

[aaron.ballman]

clang/include/clang/Basic/DiagnosticSemaKinds.td

5529

Thinking about it I guess what I've proposed is a complement to -Wstrict-prototypes. I don't see why clang couldn't have both warnings.

To me, it's about what action the user can take to respond to the diagnostic. I don't see the complement adding value in that regard. The user is calling a function that's declared somewhere without a prototype, but unless the user has control over the declaration, there is *nothing* they can do at the call site about it. Well, almost. They could add their own redeclaration with a prototype, but that's actually *worse* because now the redeclaration might get out of sync with the actual definition, but the users won't get any warnings about it.

Basically, I don't think it's a problem worth diagnosing to *call* a function without a prototype, but I definitely agree it's more dangerous to *have* functions without prototypes which are called. So I'm absolutely sold on warning users about the dangers here, but I don't think a new warning is the right approach when we could strengthen the existing warning. The current diagnostic is effectively -Wstrict-prototypes-but-only-when-called.

[delcypher]

clang/include/clang/Basic/DiagnosticSemaKinds.td

5529

@aaron.ballman Sorry for the slow response here, I was out sick.

To me, it's about what action the user can take to respond to the diagnostic. I don't see the complement adding value in that regard. The user is calling a function that's declared somewhere without a prototype, but unless the user has control over the declaration, there is *nothing* they can do at the call site about it. Well, almost. They could add their own redeclaration with a prototype, but that's actually *worse* because now the redeclaration might get out of sync with the actual definition, but the users won't get any warnings about it.

While this is all true I think:

• The existing -Wstrict-prototypes also has this exact same problem (if you can't change the declaration then it's hard to work around). This problem did not stop us creating the -Wstrict-prototypes warning in clang and I don't think that problem should stop us creating a new warning either.
• If the user can't adjust the header with the bad declaration they also have the option of filing a bug against the vendor of the header file. If the warning really causes them problems they can switch it off. If they really want to switch it off only at the call site they could use pragmas.

Basically, I don't think it's a problem worth diagnosing to *call* a function without a prototype, but I definitely agree it's more dangerous to *have* functions without prototypes which are called.

I'm a bit confused. If you agree it's more dangerous then why do you think it's not worth diagnosing?

The point of only looking at calls to functions without prototypes is that it is effectively a quieter version of -Wstrict-prototypes. That "quieter" nature means it could conceivably be switched on by default in the future, whereas I would never want to switch -Wstrict-prototypes on by default.

We can also do a better job of warning when we know what the call sites look like.
For example, when warning about calls to functions without a prototype and the call site doesn't pass args (not implemented in the patch yet) we can deduce the function declaration is very likely missing a void between parentheses. So we can warn about that specially.

So I'm absolutely sold on warning users about the dangers here, but I don't think a new warning is the right approach when we could strengthen the existing warning.

What do you mean by "strengthen" here? The existing warning isn't very sophisticated and I don't really see how we can make it better.

[aaron.ballman]

clang/include/clang/Basic/DiagnosticSemaKinds.td

5529

Sorry for the slow response here, I was out sick.

No worries! I hope you've made a full recovery. Sorry for my own delays -- I was in C standards meetings last week, so still catching up on review backlog.

I'm a bit confused. If you agree it's more dangerous then why do you think it's not worth diagnosing?

Because the call site is not where the issue is; the declaration is the issue, the call is not. Now, if you issued the warning against the *declaration* based on its usage at the call site, that could perhaps be useful. But I'm not certain it's worth the effort still (see below).

whereas I would never want to switch -Wstrict-prototypes on by default.

I'd like to hear more about this because that's precisely what I'm working towards doing. Functions without prototypes have never been a recommended practice in any standardized version of C (they were introduced into C89 as an already-deprecated feature). Functions without prototypes have been removed entirely from C23. That we don't warn about the deprecation by default is a historical accident, and the longer we wait to fix it, the worse the issues become. Note, the C committee votes next Monday on whether we're going to redefine the meaning of void f(); to be void f(void); in C23 to match the declaration behavior of C++, which increases the urgency of needing to diagnose this by default.

What do you mean by "strengthen" here? The existing warning isn't very sophisticated and I don't really see how we can make it better.

Strengthen as in enable it by default. Given that functions without prototypes have been removed from C23, we know we'll be introducing an error for that standard version. But this functionality is deprecated in all C language modes Clang supports, so diagnosing the declarations in some form for all language modes is what I expect will happen (in this release cycle most likely). Based on that, I'm not certain that warning on the call sites is going to add much value.

[delcypher]

clang/include/clang/Basic/DiagnosticSemaKinds.td

5529

whereas I would never want to switch -Wstrict-prototypes on by default.

I'd like to hear more about this because that's precisely what I'm working towards doing. Functions without prototypes have never been a recommended practice in any standardized version of C (they were introduced into C89 as an already-deprecated feature)

In hindsight I think should have said "whereas I would never want to switch -Wstrict-prototypes on by default without significant community consensus that we should do that". -Wstrict-prototypes is much louder warning that what was proposed in this PR so doing that is probably going to need an RFC.

I've noticed you've actually done this so I think it's best I abandon this patch for now and focus on reaching consensus on the direction that we want to take.

Given that functions without prototypes is actually going to be removed in an upcoming C standard starting to warn about these seems like the right direction to go in.

[aaron.ballman]

FWIW, now that https://discourse.llvm.org/t/rfc-enabling-wstrict-prototypes-by-default-in-c has reached a conclusion, I think this patch may be worth resurrecting, if you're interested.

https://discourse.llvm.org/t/rfc-enabling-wstrict-prototypes-by-default-in-c/60521/38 has most of the conclusion which includes a diagnostic option roughly similar to what you have in this patch, though there was a request for the diagnostic in this patch to be named -Wstrict-uses-without-prototype so that it can encompass other cases like function pointer assignment.