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

[Aaron Ballman via Phabricator via cfe-commits]

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


================
Comment at: clang/include/clang/Basic/LangOptions.def:124
 BENIGN_LANGOPT(ImplicitInt, 1, 0, "C89 implicit 'int'")
+LANGOPT(StrictPrototypes  , 1, 0, "require function types to have a prototype")
 LANGOPT(Digraphs          , 1, 0, "digraphs")
----------------
aaron.ballman wrote:
> aaron.ballman wrote:
> > rsmith wrote:
> > > This makes me think we should have some declarative way of specifying dependencies between `LANGOPT`s. 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?
> > > 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?
> >> 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).
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.


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


================
Comment at: clang/lib/StaticAnalyzer/Core/MemRegion.cpp:1034-1042
+          if (T.isNull() || !T->getAs<FunctionType>())
+            // If the type is invalid or is not a function type, we cannot get
+            // a block pointer type for it. This isn't ideal, but it's better
+            // than asserting in getBlockPointerType() or creating a function
+            // without a prototype in a language that has no such concept (like
+            // C++ or C2x).
+            sReg = getUnknownRegion();
----------------
aaron.ballman wrote:
> tahonermann wrote:
> > rsmith wrote:
> > > 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.
> > 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.
> 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?
I changed to create a function with a prototype instead; no tests broke when I did that as well.


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

https://reviews.llvm.org/D123955