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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
