[PATCH] Fix to PR15845 - Clang accepts invalid code
Richard Smith
richard at metafoo.co.uk
Tue Apr 30 15:45:26 PDT 2013
On Mon, Apr 29, 2013 at 10:31 AM, 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.
>
Thanks, committed as r180822.
>
>
>> 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.mmb/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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130430/69682093/attachment.html>
More information about the cfe-commits
mailing list