This is an archive of the discontinued LLVM Phabricator instance.

[C2x] Disallow functions without prototypes/functions with identifier lists
ClosedPublic

Authored by aaron.ballman on Apr 18 2022, 12:16 PM.

Details

Summary

WG14 has elected to remove support for K&R C functions in C2x. The feature was introduced into C89 already deprecated, so after this long of a deprecation period, the committee has made an empty parameter list mean the same thing in C as it means in C++: the function accepts no arguments exactly as if the function were written with (void) as the parameter list.

This patch implements WG14 N2841 No function declarators without prototypes (http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2841.htm) and WG14 N2432 Remove support for function definitions with identifier lists (http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2432.pdf).

Diff Detail

Event Timeline

aaron.ballman created this revision.Apr 18 2022, 12:16 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 18 2022, 12:16 PM
aaron.ballman requested review of this revision.Apr 18 2022, 12:16 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 18 2022, 12:16 PM
erichkeane added inline comments.Apr 18 2022, 12:25 PM
clang/lib/AST/ASTContext.cpp
11243

How tough would it be to change this to represent always as a C++-esque foo(...) prototype, just always?

clang/lib/Sema/SemaDecl.cpp
8821

This assert is quite strange, particularly with the string part of it only in the 'else' of the conditional? It seems you're trying to ensure that:
"if strictProtoTypes, assert hasProtoType"?

In that case, I'd likely suggest:

assert((HasProtoType || !SemeaRef.getLangOpts().StrictPrototypes) && "Strict prototypes are required");

clang/test/Parser/c2x-attributes.c
64

This is an unfortunate change in diagnostic quality.

aaron.ballman marked an inline comment as done.Apr 18 2022, 12:31 PM
aaron.ballman added inline comments.
clang/lib/AST/ASTContext.cpp
11243

C doesn't allow that (but we do if the user specifies the overloadable attribute), so it could be a bit tricky.

clang/lib/Sema/SemaDecl.cpp
8821

You decoded what I was going for, but I like your approach better. Thanks!

clang/test/Parser/c2x-attributes.c
64

Agreed, but thankfully this situation should be exceedingly rare (trying to put [[]] attributes after a function definition with an identifier list).

aaron.ballman marked an inline comment as done.

Updated based on review feedback

erichkeane added inline comments.Apr 18 2022, 12:33 PM
clang/lib/AST/ASTContext.cpp
11243

Hmm... I guess that is fair. Thinking further, we likely 'assume' that cannot be the case else where :/

clang/test/Parser/c2x-attributes.c
64

Ah! less concerned then.

erichkeane accepted this revision.Apr 18 2022, 12:34 PM
This revision is now accepted and ready to land.Apr 18 2022, 12:34 PM

Thanks for working on this, Just a few comments

clang/lib/Basic/LangOptions.cpp
119

Stupid question, we probably don't want -fno-strict-prototype` to have an effect in C++. is there something to deal with that? I'm not familiar with the option system

clang/lib/Sema/SemaDecl.cpp
8821–8823

That would make more sense to me, not that it affects the logic

assert((SemaRef.getLangOpts().StrictPrototypes
               ? HasPrototype
               : true) && "Strict prototypes are required");
clang/lib/Sema/SemaType.cpp
5299

can you explain the Context.IntTy here ?

aaron.ballman marked 3 inline comments as done.Apr 18 2022, 12:53 PM
aaron.ballman added inline comments.
clang/lib/Basic/LangOptions.cpp
119

I don't currently expose a user-facing option for this yet. I was thinking about it, but wasn't certain if -Werror=deprecated-non-prototype was sufficient or not.

That said, if I did produce a user-facing option, I would not expose an -fno-strict-prototype option at all. Users can opt into strict prototypes, but they cannot opt out of them.

clang/lib/Sema/SemaType.cpp
5299

It's our default fallback type -- we use the same thing on line 5120 as an example. It shouldn't matter overly much as we tell the declarator that it has an invalid type.

rsmith added a subscriber: rsmith.Apr 18 2022, 1:01 PM
rsmith added inline comments.
clang/include/clang/Basic/LangOptions.def
124

This makes me think we should have some declarative way of specifying dependencies between LANGOPTs. It's presumably sufficiently obvious to a library user that they shouldn't enable (eg) CPlusPlus20 unless they enable all the previous CPlusPlusXY modes and CPlusPlus, but I doubt it's obvious that enabling CPlusPlus requires also enabling StrictPrototypes.

In fact, after this change, I think a lot of existing library users of Clang that invent their own LangOptions will silently start getting this wrong. That's concerning. Maybe we should consider prototypes to be required if either StrictPrototypes or CPlusPlus is enabled?

clang/lib/Parse/ParseDecl.cpp
6664–6666

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.

clang/lib/Sema/SemaType.cpp
5273–5275

Same as before, OpenCL disallowing variadics doesn't seem relevant here.

clang/lib/StaticAnalyzer/Core/MemRegion.cpp
1034–1042

I find it really surprising that the "signature is present but is not a function type" case is reachable -- the static analyzer should only run on valid code, and in valid code I'd expect the signature of a block would always be a function type. Is that case actually reached in our test suite?

I worry that the "block has no explicit signature" case here is common, and that we're losing substantial coverage in that case. Per https://clang.llvm.org/docs/BlockLanguageSpec.html#block-literal-expressions, ^ { ... } is equivalent to ^ (void) { ... }, so it seems the original code here was just wrong and we should always have been creating a FunctionProtoType in this case.

aaron.ballman marked 3 inline comments as done.Apr 18 2022, 1:22 PM
aaron.ballman added a subscriber: NoQ.
aaron.ballman added inline comments.
clang/include/clang/Basic/LangOptions.def
124

This makes me think we should have some declarative way of specifying dependencies between LANGOPTs.

Tee hee: https://github.com/llvm/llvm-project/commit/be9371659380388a693ec99624e1f3d02f07047f

I was caught by the same issues that I ended up fixing in that commit -- I tried making StrictPrototypes dependent and was very surprised when it doesn't work. The issue was Invocation->getLangOpts()->resetNonModularOptions(); in compileModuleImpl() IIRC -- it would clear all of the language options (including ones like digraphs and wchar support).

In fact, after this change, I think a lot of existing library users of Clang that invent their own LangOptions will silently start getting this wrong. That's concerning. Maybe we should consider prototypes to be required if either StrictPrototypes or CPlusPlus is enabled?

@cor3ntin also shared this same concern -- my goal with this language option was:

  • Make it easy to reenable functions without prototypes in C2x mode if we find some major breakage in the wild (hopefully we don't)
  • Make it easy to add -fstrict-prototypes as a language flag to opt *into* strict prototypes (there would be no option to opt *out* of strict prototypes).
  • Stop repeating CPlusPlus || C2x in a bunch of places and give it a named option.

So I'm not keen on adding StrictPrototypes || CPlusPlus everywhere -- C++ requires strict prototypes. However, the fears are still valid -- what if I found someplace nice to add an assert that StrictPrototypes cannot be false when CPlusPlus is true?

clang/lib/Parse/ParseDecl.cpp
6664–6666

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!

clang/lib/Sema/SemaType.cpp
5273–5275

Same here as above.

clang/lib/StaticAnalyzer/Core/MemRegion.cpp
1034–1042

I tried raising someone from the static analyzer team on IRC and failed, so this was my best guess.

Without this change, we get two assertions (when I enabled the asserts in getFunctionNoProtoType()): test/Analysis/blocks.m and test/Analysis/templates.cpp. So we definitely reach this spot. Given the FIXME comment above my changes, it looks like the analyzer folks knew they were doing something wrong here. With these changes, I get no test failures, so I'm not certain we're losing substantial coverage or not.

I think you might be correct about creating a function with a prototype. @NoQ -- do you agree with that fix?

tahonermann added inline comments.
clang/lib/StaticAnalyzer/Core/MemRegion.cpp
1034–1042

I haven't studied the surrounding code much, so perhaps this comment isn't applicable, but I would expect FunctionProtoType to be applicable when an empty parameter list is explicitly specified as in ^ () { ... } prior to C2x.

aaron.ballman marked an inline comment as not done.Apr 19 2022, 5:05 AM
aaron.ballman added inline comments.
clang/include/clang/Basic/LangOptions.def
124

This makes me think we should have some declarative way of specifying dependencies between LANGOPTs.

Tee hee: https://github.com/llvm/llvm-project/commit/be9371659380388a693ec99624e1f3d02f07047f

I was caught by the same issues that I ended up fixing in that commit -- I tried making StrictPrototypes dependent and was very surprised when it doesn't work. The issue was Invocation->getLangOpts()->resetNonModularOptions(); in compileModuleImpl() IIRC -- it would clear all of the language options (including ones like digraphs and wchar support).

There's actually a secondary issue, which is that users can set language options directly -- there's not a setter function for them. This means that places where we create a LangOptions object and manually set it up (like our unit tests) have no way to automatically set StrictPrototypes when setting other language options.

However, this speaks to the importance of having an assert to ensure that by the time the compiler instance is invoked, we have language options that are correct (on the assumption that later stages will not be modifying the language options particularly often).

aaron.ballman marked an inline comment as not done.

Updated based on review feedback.

  • Added -fstrict-prototypes command line option (but there is NO -fno-strict-prototypes support)
  • Changed the way the language option works to be a little less fragile
  • Fixed the static analyzer to create a function with a prototype instead
aaron.ballman marked 5 inline comments as done.Apr 19 2022, 8:22 AM
aaron.ballman added inline comments.
clang/include/clang/Basic/LangOptions.def
124

I ended up reworking this so that the language option is only for the command line extension, and there's a helper method to ask whether strict prototypes is enabled (based on the values of the language options). It's still a little fragile (someone could use the language option instead of the helper method), but I think that's mitigated by the name of the language option.

clang/lib/Parse/ParseDecl.cpp
6664–6666

I updated both comments.

clang/lib/StaticAnalyzer/Core/MemRegion.cpp
1034–1042

I changed to create a function with a prototype instead; no tests broke when I did that as well.

rsmith accepted this revision.Apr 19 2022, 12:37 PM

LGTM. Some comments for potential improvements, but I'd be OK with this landing as-is if you don't find any of them sufficiently motivating :)

clang/lib/Parse/ParseDecl.cpp
6664–6666

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 would be nice.

6667

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.

clang/test/Driver/strict-prototypes.c
5 ↗(On Diff #423629)

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.

aaron.ballman marked 2 inline comments as done.Apr 20 2022, 7:43 AM
aaron.ballman added inline comments.
clang/lib/Parse/ParseDecl.cpp
6664–6666

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!

6667

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?

clang/test/Driver/strict-prototypes.c
5 ↗(On Diff #423629)

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.

aaron.ballman marked 2 inline comments as done.Apr 20 2022, 9:09 AM
aaron.ballman added inline comments.
clang/lib/Parse/ParseDecl.cpp
6667

After a bunch of off-list discussion with @tahonermann and @erichkeane, we landed on -fno-knr-functions as the proposed new name for the flag.

This revision was landed with ongoing or failed builds.Apr 20 2022, 10:28 AM
This revision was automatically updated to reflect the committed changes.