[PATCH] Fix to PR15845 - Clang accepts invalid code
Serge Pavlov
sepavloff at gmail.com
Mon Apr 29 10:56:32 PDT 2013
Clang now allows implicit int in C mode (-x c) in some cases:
abc = 1;
static foo() { return 1; }
But in function declaration it is now forbidden unlike to GCC and MSVC:
void func(xxx);
--Serge
2013/4/30 Aaron Ballman <aaron at aaronballman.com>
> To be clear, MSVC does accept that code so long as you pass in the /TC
> flag. So, for instance, the test cases with a .c or .m extension
> should allow the implicit int (by default .c files are compiled with
> /TC in MSVC), but the .cpp/.mm files should disallow it (unless we
> force compilation as C with -x c, which would be the clang version of
> /TC).
>
> ~Aaron
>
> On Mon, Apr 29, 2013 at 1:31 PM, Serge Pavlov <sepavloff at gmail.com> wrote:
> > Hi Richard,
> >
> >
> > 2013/4/29 Richard Smith <richard at metafoo.co.uk>
> > [...]
> >>
> >> The problem is that clang in C++ mode accepts the code:
> >>>
> >>> int foo(xxx);
> >>> Clang intentionally accepts this code due to a check in
> >>> Parser::ParseImplicitInt, which appeared in r156854.
> >>> The comment in the inserted code states that MS supports implicit int
> in
> >>> C++ mode, however it looks like none of VS2005, VS2008, VS2010 or
> VS2012
> >>> does it. So removing the check for MS extension solves the problem.
> >>
> >>
> >> If it is indeed the case that MSVC does not allow implicit int in C++,
> >> then we should absolutely remove that "extension". However, someone
> >> presumably added it for a reason, so I'd like to be sure that we've
> checked
> >> this thoroughly before proceeding. Does MSVC allow implicit int in any
> other
> >> contexts? For instance...
> >
> >
> > MSVC doesn't allow implicit int in any context if in C++ mode, details
> are
> > in bugzilla.
> >
> >>
> >>
> >> const n = 0; // ok?
> >> static f() { // ok?
> >> extern m; // ok?
> >> return m;
> >> }
> >
> >
> > None of these cases are accepted by MSVC.
> >
> >>
> >> If MSVC does allow these, then the fix is different: the
> >> decl-specifier-seq (or, in C, the declaration-specifiers) for a
> parameter
> >> cannot be empty, so 'int foo(xxx);' would not have implicit int applied,
> >> whereas 'int foo(const xxx);' would, and we should make the parser
> handle
> >> that correctly.
> >>
> >>>
> >>> Another problem - the same code compiled in C mode produces an error,
> >>> while both GCC and MSC accept it. To fix it the message
> >>> err_ident_list_in_fn_declaration was converted into warning.
> >>
> >>
> >> Have you checked whether they treat it as an implicit int, or whether
> they
> >> treat it as an (ignored, presumably) identifier list?
> >
> >
> > They are ignored. For instance, both MSVC and GCC successfully compile
> the
> > following code:
> >
> > void abc(xxx);
> > void abc(int x, char*y) {}
> >
> >>
> >> Also, do you actually have code which relies upon this extension? If
> not,
> >> let's not add it gratuitously.
> >
> >
> > I know nothing about such, the intent was to make behavior more
> compatible.
> > Probably it doesn't worth implementation.
> >
> >> Please split this into its two constituent changes (removing implicit
> int
> >> in microsoft mode, and accepting an identifier-list in a non-defining
> >> function declaration). They're basically unrelated, and make more sense
> to
> >> review separately.
> >
> >
> > OK. This patch only removes implicit int in MS-compatibility mode for
> C++.
> > Fix to accepting an identifier-list in a non-defining function
> declaration
> > is dropped.
> >
> >>>
> >>> Files:
> >>> include/clang/Basic/DiagnosticSemaKinds.td
> >>> lib/Parse/ParseDecl.cpp
> >>> lib/Sema/SemaType.cpp
> >>> test/Sema/MicrosoftCompatibility.cpp
> >>> test/Sema/alloc_size.c
> >>> test/Sema/function.c
> >>> test/Sema/invalid-decl.c
> >>>
> >>>
> >>>
> -----------------------------------------------------------------------------------------------------
> >>> diff --git a/include/clang/Basic/DiagnosticSemaKinds.td
> >>> b/include/clang/Basic/DiagnosticSemaKinds.td
> >>> index 1461716..166dbab 100644
> >>> --- a/include/clang/Basic/DiagnosticSemaKinds.td
> >>> +++ b/include/clang/Basic/DiagnosticSemaKinds.td
> >>> @@ -2314,8 +2314,9 @@ def err_void_only_param : Error<
> >>> "'void' must be the first and only parameter if specified">;
> >>> def err_void_param_qualified : Error<
> >>> "'void' as parameter must not have type qualifiers">;
> >>> -def err_ident_list_in_fn_declaration : Error<
> >>> - "a parameter list without types is only allowed in a function
> >>> definition">;
> >>> +def warn_ident_list_in_fn_declaration : Warning<
> >>> + "a parameter list without types is only allowed in a function
> >>> definition">,
> >>> + InGroup<C99Compat>;
> >>
> >>
> >> This should be an ExtWarn, not a Warning, since this is a required
> >> diagnostic per the various C language standards. Also, C99Compat seems
> >> wrong.
> >
> >
> > Thank you for the explanation.
> >
> >>
> >>>
> >>> def ext_param_not_declared : Extension<
> >>> "parameter %0 was not declared, defaulting to type 'int'">;
> >>> def err_param_typedef_of_void : Error<
> >>> diff --git a/lib/Parse/ParseDecl.cpp b/lib/Parse/ParseDecl.cpp
> >>> index d786ce2..2f0c1a3 100644
> >>> --- a/lib/Parse/ParseDecl.cpp
> >>> +++ b/lib/Parse/ParseDecl.cpp
> >>> @@ -2038,10 +2038,9 @@ bool Parser::ParseImplicitInt(DeclSpec &DS,
> >>> CXXScopeSpec *SS,
> >>> // error, do lookahead to try to do better recovery. This never
> >>> applies
> >>> // within a type specifier. Outside of C++, we allow this even if
> the
> >>> // language doesn't "officially" support implicit int -- we support
> >>> - // implicit int as an extension in C99 and C11. Allegedly, MS also
> >>> - // supports implicit int in C++ mode.
> >>> + // implicit int as an extension in C99 and C11.
> >>> if (DSC != DSC_type_specifier && DSC != DSC_trailing &&
> >>> - (!getLangOpts().CPlusPlus || getLangOpts().MicrosoftExt) &&
> >>> + !getLangOpts().CPlusPlus &&
> >>
> >>
> >> There is a matching check in lib/Sema/DeclSpec.cpp, and possibly
> >> elsewhere. If we're not enabling implicit int in -fms-extensions mode,
> we
> >> need to do that consistently throughout the compiler.
> >
> >
> > Indeed, SemaType.cpp also contains similar check.
> >
> >>>
> >>> isValidAfterIdentifierInDeclarator(NextToken())) {
> >>> // If this token is valid for implicit int, e.g. "static x = 4",
> >>> then
> >>> // we just avoid eating the identifier, so it will be parsed as
> the
> >>> diff --git a/lib/Sema/SemaType.cpp b/lib/Sema/SemaType.cpp
> >>> index 8bf5143..243b772 100644
> >>> --- a/lib/Sema/SemaType.cpp
> >>> +++ b/lib/Sema/SemaType.cpp
> >>> @@ -2742,7 +2742,7 @@ static TypeSourceInfo
> >>> *GetFullTypeForDeclarator(TypeProcessingState &state,
> >>> if (FTI.NumArgs && FTI.ArgInfo[0].Param == 0) {
> >>> // C99 6.7.5.3p3: Reject int(x,y,z) when it's not a function
> >>> // definition.
> >>> - S.Diag(FTI.ArgInfo[0].IdentLoc,
> >>> diag::err_ident_list_in_fn_declaration);
> >>> + S.Diag(FTI.ArgInfo[0].IdentLoc,
> >>> diag::warn_ident_list_in_fn_declaration);
> >>> D.setInvalidType(true);
> >>
> >>
> >> If you're not issuing an error, you must build a correct AST -- you
> can't
> >> set things invalid.
> >>
> >
> > My fault...
> > [...]
> >
> >
> > Updated patch:
> >
> > Files:
> > lib/Parse/ParseDecl.cpp
> > lib/Sema/DeclSpec.cpp
> > lib/Sema/SemaType.cpp
> > test/Rewriter/rewrite-byref-in-nested-blocks.mm
> > test/Sema/MicrosoftCompatibility.cpp
> >
> >
> > diff --git a/lib/Parse/ParseDecl.cpp b/lib/Parse/ParseDecl.cpp
> > index d786ce2..2f0c1a3 100644
> > --- a/lib/Parse/ParseDecl.cpp
> > +++ b/lib/Parse/ParseDecl.cpp
> > @@ -2038,10 +2038,9 @@ bool Parser::ParseImplicitInt(DeclSpec &DS,
> > CXXScopeSpec *SS,
> > // error, do lookahead to try to do better recovery. This never
> applies
> > // within a type specifier. Outside of C++, we allow this even if the
> > // language doesn't "officially" support implicit int -- we support
> > - // implicit int as an extension in C99 and C11. Allegedly, MS also
> > - // supports implicit int in C++ mode.
> > + // implicit int as an extension in C99 and C11.
> > if (DSC != DSC_type_specifier && DSC != DSC_trailing &&
> > - (!getLangOpts().CPlusPlus || getLangOpts().MicrosoftExt) &&
> > + !getLangOpts().CPlusPlus &&
> > isValidAfterIdentifierInDeclarator(NextToken())) {
> > // If this token is valid for implicit int, e.g. "static x = 4",
> then
> > // we just avoid eating the identifier, so it will be parsed as the
> > diff --git a/lib/Sema/DeclSpec.cpp b/lib/Sema/DeclSpec.cpp
> > index 124f50c..3b3ab2c 100644
> > --- a/lib/Sema/DeclSpec.cpp
> > +++ b/lib/Sema/DeclSpec.cpp
> > @@ -1003,8 +1003,7 @@ void DeclSpec::Finish(DiagnosticsEngine &D,
> > Preprocessor &PP) {
> > // the type specifier is not optional, but we got 'auto' as a storage
> > // class specifier, then assume this is an attempt to use C++0x's
> 'auto'
> > // type specifier.
> > - // FIXME: Does Microsoft really support implicit int in C++?
> > - if (PP.getLangOpts().CPlusPlus && !PP.getLangOpts().MicrosoftExt &&
> > + if (PP.getLangOpts().CPlusPlus &&
> > TypeSpecType == TST_unspecified && StorageClassSpec == SCS_auto) {
> > TypeSpecType = TST_auto;
> > StorageClassSpec = SCS_unspecified;
> > diff --git a/lib/Sema/SemaType.cpp b/lib/Sema/SemaType.cpp
> > index 8bf5143..2038f12 100644
> > --- a/lib/Sema/SemaType.cpp
> > +++ b/lib/Sema/SemaType.cpp
> > @@ -793,9 +793,7 @@ static QualType
> > ConvertDeclSpecToType(TypeProcessingState &state) {
> > // "At least one type specifier shall be given in the declaration
> > // specifiers in each declaration, and in the specifier-qualifier
> > list in
> > // each struct declaration and type name."
> > - // FIXME: Does Microsoft really have the implicit int extension in
> > C++?
> > - if (S.getLangOpts().CPlusPlus &&
> > - !S.getLangOpts().MicrosoftExt) {
> > + if (S.getLangOpts().CPlusPlus) {
> > S.Diag(DeclLoc, diag::err_missing_type_specifier)
> > << DS.getSourceRange();
> >
> > diff --git a/test/Rewriter/rewrite-byref-in-nested-blocks.mm
> > b/test/Rewriter/rewrite-byref-in-nested-blocks.mm
> > index 022bb5f..f416b66 100644
> > --- a/test/Rewriter/rewrite-byref-in-nested-blocks.mm
> > +++ b/test/Rewriter/rewrite-byref-in-nested-blocks.mm
> > @@ -19,7 +19,7 @@ void f(void (^block)(void));
> > - (void)foo {
> > __block int kerfluffle;
> > // radar 7692183
> > - __block x;
> > + __block int x;
> > f(^{
> > f(^{
> > y = 42;
> >
> > diff --git a/test/Sema/MicrosoftCompatibility.cpp
> > b/test/Sema/MicrosoftCompatibility.cpp
> > new file mode 100644
> > index 0000000..15c2558
> > --- /dev/null
> > +++ b/test/Sema/MicrosoftCompatibility.cpp
> > @@ -0,0 +1,4 @@
> > +// RUN: %clang_cc1 %s -fsyntax-only -Wno-unused-value -Wmicrosoft
> -verify
> > -fms-compatibility
> > +
> > +// PR15845
> > +int foo(xxx); // expected-error{{unknown type name}}
> >
> >
> > --
> > Thanks,
> > --Serge
> >
> > _______________________________________________
> > cfe-commits mailing list
> > cfe-commits at cs.uiuc.edu
> > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130430/5c49cfa6/attachment.html>
More information about the cfe-commits
mailing list