llvm / llvm-project

The LLVM Project is a collection of modular and reusable compiler and toolchain technologies.
http://llvm.org
Other
28.48k stars 11.77k forks source link

Overzealous -Wstrict-prototypes warnings on function definitions #90596

Open jmarshall opened 5 months ago

jmarshall commented 5 months ago

Consider the following, foo.c:

int foo() {
    return 42;
}

Current clang (19.0.0git c3598b161a4d868b1cd10a7ee7ac37d68e5a36fe) with -pedantic or -Wstrict-prototypes produces a warning for this code:

clang -c -Wstrict-prototypes foo.c 
foo.c:1:8: warning: a function declaration without a prototype is deprecated in all versions of C [-Wstrict-prototypes]
    1 | int foo() {
      |        ^
      |         void

The discussion in [RFC] Enabling -Wstrict-prototypes by default in C largely focusses on encouraging users to write declarations as int foo(void); but I am interested in the application of this warning to function definitions.

The diagnostic message above is somewhat confusing as this is a function definition, not just a function declaration. More importantly, I believe there is no confusion about a function definition with an empty parameter list. In particular C17 N2176 §6.7.6.3/14 says [similar text appears in earlier versions; emphasis added]

An identifier list declares only the identifiers of the parameters of the function. An empty list in a function declarator that is part of a definition of that function specifies that the function has no parameters. 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. [Footnote 147: See “future language directions” (6.11.6).]

I would take this to mean that a function definition with an empty parameter list effectively provides a prototype, and therefore that the warning is spurious in this case. It seems unfortunate to encourage users to hypercorrect their code by writing such functions as int foo(void) { return 42; } when the void doesn't add anything in a function definition.

I also tried to find the chapter and verse of the standard saying that this scenario is “deprecated in all versions of C”. I guess this is based on §6.11.6, which says

The use of function declarators with empty parentheses (not prototype-format parameter type declarators) is an obsolescent feature.

Based on the two “…empty list…” sentences in the paragraph quoted above whose footnote refers to §6.11.6 and the fact that the obsolescent feature we're trying to get rid of in §6.11.6 is non-prototype declarations, I think this sentence intends only to refer to the latter and should perhaps read

The use of function declarators (that are not part of definitions of the respective functions) with empty parentheses (not prototype-format parameter type declarators) is an obsolescent feature.

in which case it would not provide motivation for emitting this warning on function definitions. (I did not see a defect report to this effect or otherwise relating to §6.11.6.)

llvmbot commented 5 months ago

@llvm/issue-subscribers-c

Author: John Marshall (jmarshall)

Consider the following, _foo.c_: ```c int foo() { return 42; } ``` Current clang (19.0.0git c3598b161a4d868b1cd10a7ee7ac37d68e5a36fe) with `-pedantic` or `-Wstrict-prototypes` produces a warning for this code: ``` clang -c -Wstrict-prototypes foo.c foo.c:1:8: warning: a function declaration without a prototype is deprecated in all versions of C [-Wstrict-prototypes] 1 | int foo() { | ^ | void ``` The discussion in [[RFC] Enabling -Wstrict-prototypes by default in C](https://discourse.llvm.org/t/rfc-enabling-wstrict-prototypes-by-default-in-c/60521) largely focusses on encouraging users to write declarations as `int foo(void);` but I am interested in the application of this warning to function **definitions**. The diagnostic message above is somewhat confusing as this is a function definition, not just a function declaration. More importantly, I believe there is no confusion about a function definition with an empty parameter list. In particular C17 N2176 §6.7.6.3/14 says [similar text appears in earlier versions; emphasis added] > An identifier list declares only the identifiers of the parameters of the function. **An empty list in a function declarator that is part of a definition of that function specifies that the function has no parameters.** 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. [Footnote 147: See “future language directions” (6.11.6).] I would take this to mean that a function **definition** with an empty parameter list effectively provides a prototype, and therefore that the warning is spurious in this case. It seems unfortunate to encourage users to hypercorrect their code to define such functions as `int foo(void) { return 42; }` when the `void` doesn't add anything in a function **definition**. I also tried to find the chapter and verse of the standard saying that this scenario is “deprecated in all versions of C”. I guess this is based on §6.11.6, which says > The use of function declarators with empty parentheses (not prototype-format parameter type declarators) is an obsolescent feature. Based on the two “…empty list…” sentences in the paragraph quoted above whose footnote refers to §6.11.6 and the fact that the obsolescent feature we're trying to get rid of in §6.11.6 is non-prototype declarations, I think this sentence intends only to refer to the latter and should perhaps read > The use of function declarators **(that are not part of definitions of the respective functions)** with empty parentheses (not prototype-format parameter type declarators) is an obsolescent feature. in which case it would not provide motivation for emitting this warning on function **definitions**. (I did not see a defect report to this effect or otherwise relating to §6.11.6.)
shafik commented 5 months ago

CC @AaronBallman @uecker

MitalAshok commented 5 months ago
int foo() {
    return 42;
}
int bar(void) {
    return 42;
}

These still declare functions with two different types - int() and int(void).

It's still desireable for there to be a declaration with int(void) to prevent it from being compatible with every other function type, i.e.:

int foo() { return 42; }
int bar(void) { return 42; }
int f(int(int));
int main(void) {
    f(foo);  // Compatible
    // f(bar);  // Error
}

And §6.5.2.2 function call semantics are defined in terms of the type of the function. §6.5.2.2/8 specifically mentions function definitions without a prototype.

int main(void) {
    foo(1);  // Compiles but UB
    // bar(1);  // Error
}

So the (void) does add something.

jmarshall commented 5 months ago

Thank you, that's very informative.

(Though now I barely understand what the “An empty list in a function declarator that is part of a definition of that function specifies that the function has no parameters” sentence is there for…)

AaronBallman commented 5 months ago

There are several cases to consider.

void foo() {}

does not provide a prototype for a function because it does not provide a parameter-type-list, but instead uses the optional identifier-list grammar production. See C17 6.7.6p1 and 6.9.1p7. However, by virtue of it being a definition, it does specify how many arguments may be passed to the function (zero).

void foo(); // #1
// ...
void foo() {} // #2

Neither of these declarations provide a prototype (per the same citations as above). Code between the declaration at (1) and the definition at (2) can call foo() with any number of arguments without diagnosing, but this is undefined behavior because the definition at (2) defines a function that accepts no arguments. Any calls providing arguments after the definition at (2) are diagnosed because the compiler now has complete information about the definition: https://godbolt.org/z/18vWso7d5

Finally, this gem:

void foo(void); // #1
// ...
void foo() {} // #2

The declaration at (1) provides a prototype, the definition at (2) does not provide a prototype, but gets one anyway because of composite type rules (see C17 6.2.7p3). Unlike the previous example, calls to foo() between (1) and (2) which pass an argument will get diagnosed because of the function prototype. Clang issues a -Wstrict-prototypes diagnostic at (2) because we form the prototype-less type before doing declaration merging (which is when we'd give the declaration a prototype) and we diagnose forming that type: https://godbolt.org/z/eq69rP1Wj

It's technically a false positive to diagnose in that case, but I don't think we'll ever fix that bug because of the difficulties it presents to do so. We want to still catch cases like (void (*)()) as a type cast because that forms a function type without a prototype (and similar for typedefs, etc). This is why we diagnose when forming the type itself. There's not a trivial way to retroactively say "nevermind, don't issue that diagnostic about the type because we happened to merge with a function type that does have a prototype"; we'd have to delay issuing diagnostics (potentially until the end of the TU) which can then mess with the order in which diagnostics are issued in a confusing way (e.g., you get warnings about those functions well after you would get other diagnostics related to those functions), and because this is an off-by-default diagnostic in Clang, it seems like the false positive in that case is better than the alternatives.

brenns10 commented 4 months ago

Hello,

Finally, this gem:

void foo(void); // #1
// ...
void foo() {} // #2

I actually hit that exact issue here, and while the wording was initially confusing, I did get to the bottom of it. A nicer worded error would be nice though, since it led me to believe that clang wasn't seeing the initial declaration (which could have been possible due to #ifdef hell in the include files).

Just wanted to provide a data point on encountering this issue in the wild. The explanations here are very helpful.