[PATCH] Fix to PR15845 - Clang accepts invalid code
Aaron Ballman
aaron at aaronballman.com
Mon Apr 29 11:01:31 PDT 2013
Correct; since we're going to be modifying the way this extension
works, it would be great if we could make it more compatible for
-fms-compatibility. In this case, MSVC handles void func(xxx) {} when
/TC is passed in, so it would be good for clang to do so as well
(along with the other cases that Richard pointed out).
~Aaron
On Mon, Apr 29, 2013 at 1:56 PM, Serge Pavlov <sepavloff at gmail.com> wrote:
> 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
>> >
>
>
More information about the cfe-commits
mailing list