[PATCH] D123955: [C2x] Disallow functions without prototypes/functions with identifier lists

Wed Apr 20 07:43:33 PDT 2022

aaron.ballman marked 2 inline comments as done.
aaron.ballman added inline comments.

================
Comment at: clang/lib/Parse/ParseDecl.cpp:6664-6666
+    // OpenCL disallows variadic functions, so it also disallows a function
+    // without a prototype. However, it doesn't enforce strict prototypes
+    // because it allows function definitions with an identifier list.
----------------
rsmith wrote:
> aaron.ballman wrote:
> > aaron.ballman wrote:
> > > rsmith wrote:
> > > > I don't follow this comment: functions without a prototype are not variadic (they're compatible with any *non-variadic* prototype), so OpenCL disallowing variadic functions seems irrelevant here.
> > > Heh, this comment came from feedback I got from someone on IRC when I was asking what OpenCL actually supports. As best I found, OpenCL allows `void f();` to mean `void f(void);` as in C++, but also allows `void f(a, b) int a, b; {}` (despite having no way to actually declare this function).
> > > 
> > > I'll take a pass at fixing up the comment to be more clear, thanks!
> > I updated both comments.
> Fascinating! There's a common confusion and misapprehension that `void f()` declares a variadic function in C, and that confusion has made its way into the OpenCL specification. The relevant rule there is 6.11/g:
> 
> > e. Variadic functions are not supported, with the exception of printf and enqueue_kernel.
> > g. If a list of parameters in a function declaration is empty, the function takes no arguments. **This is due to the above restriction on variadic functions.**
> 
> (Emphasis mine.) So I think this implementation is correct, and your earlier comment correctly reflected the confusion in the OpenCL spec :-)
> 
> A comment with a direct reference to this part of the [OpenCL C 3.0 specification](https://www.khronos.org/registry/OpenCL/specs/3.0-unified/pdf/OpenCL_C.pdf) would be nice.
> Fascinating! There's a common confusion and misapprehension that void f() declares a variadic function in C, and that confusion has made its way into the OpenCL specification. 

I think it's understandable confusion. `void f(...);` in C++ and `void f();` in C89 say the "same" thing: this function can be called with zero or more arguments, YOLO. The fact that the standards made `...` mean "variadic" but functions without a prototype mean "magic" is mostly lost on users, I think.

> A comment with a direct reference to this part of the OpenCL C 3.0 specification would be nice.

I'll add that link in, good call!

================
Comment at: clang/lib/Parse/ParseDecl.cpp:6667
+    // have an identifier list.
+    HasProto = ParamInfo.size() || getLangOpts().requiresStrictPrototypes() ||
+               getLangOpts().OpenCL;
----------------
rsmith wrote:
> Hm, so `-fstrict-prototypes` causes us to treat `void f()` as `void f(void)`? That's not normally how our `-fstrict-` flags work: normally they mean "strictly enforce this language rule, even though that may result in programs having UB that we could define away with a more permissive rule". (For example, `-fstrict-aliasing`, `-fstrict-float-cast-overflow`, `-fstrict-enums`, `-fstrict-overflow`, `-fstrict-vtable-pointers`, `-fstrict-return` all work like that.) I wonder if a different flag name would work better, eg `-fno-unprototyped-functions`. Is `-fstrict-prototypes` a GCC flag that we're trying to be compatible with, or our own invention?
> 
> If you can't find a better name, I'm not dead set against the current one, but it does seem a little inconsistent.
> Hm, so -fstrict-prototypes causes us to treat void f() as void f(void)?

Yup, the idea is that it is strictly enforcing that all functions have a prototype.

>  Is -fstrict-prototypes a GCC flag that we're trying to be compatible with, or our own invention?

It's our invention, and I'm not strongly tied to the name. It's the one that came up during the RFC and I suspect it's influenced by the name of the warning group `-Wstrict-prototypes`.

I think `-fno-` would be a bit of a weird way to spell the flag as that usually disables something rather than enables it. I'll switch to `-fforce-prototypes` because that's basically what this does. WDYT?

================
Comment at: clang/test/Driver/strict-prototypes.c:5
+// RUN: not %clang -fno-strict-prototypes -x c %s 2>&1 | FileCheck %s
+// RUN: not %clang -fno-strict-prototypes -std=c89 -x c %s 2>&1 | FileCheck %s
+
----------------
rsmith wrote:
> I would expect this case (`-std=c89 -fno-strict-prototypes`) to work: we usually allow ` -fno-X` flag to override an earlier `-fX` flag in the same command line. Not a big deal, though.
I think it's better to disallow the user from ever uttering `-fno-strict-prototypes` even when it's behavior would be benign. But I'm happy to revisit later if it becomes an issue.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D123955/new/

https://reviews.llvm.org/D123955