[PATCH] Fix to PR15845 - Clang accepts invalid code
Richard Smith
richard at metafoo.co.uk
Sun Apr 28 13:59:39 PDT 2013
On Sun, Apr 28, 2013 at 6:37 AM, Serge Pavlov <sepavloff at gmail.com> wrote:
> Hi all,
>
> This patch fixes PR15845 - Clang accepts invalid code,
> http://llvm.org/bugs/show_bug.cgi?id=15845.
> Could someone please review this patch?
> Thank you.
> --Serge
>
> Description:
> 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...
const n = 0; // ok?
static f() { // ok?
extern m; // ok?
return m;
}
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? Also, do you actually
have code which relies upon this extension? If not, let's not add it
gratuitously.
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.
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.
> 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.
> 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.
> // Recover by creating a K&R-style function type.
> T = Context.getFunctionNoProtoType(T);
> 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}}
> diff --git a/test/Sema/alloc_size.c b/test/Sema/alloc_size.c
> index 84f3932..0067bea 100644
> --- a/test/Sema/alloc_size.c
> +++ b/test/Sema/alloc_size.c
> @@ -22,6 +22,6 @@ void *fn8(int, int) __attribute__((alloc_size(1, 1)));
> // OK
> void* fn9(unsigned) __attribute__((alloc_size(12345678901234567890123)));
> // expected-warning {{integer constant is too large for its type}} //
> expected-error {{attribute parameter 1 is out of bounds}}
>
> void* fn10(size_t, size_t) __attribute__((alloc_size(1,2))); //
> expected-error{{redefinition of parameter}} \
> - //
> expected-error{{a parameter list without types is only allowed in a
> function definition}} \
> + //
> expected-warning{{a parameter list without types is only allowed in a
> function definition}} \
> //
> expected-error{{attribute parameter 1 is out of bounds}}
> void* fn11() __attribute__((alloc_size(1))); // expected-error{{attribute
> parameter 1 is out of bounds}}
> diff --git a/test/Sema/function.c b/test/Sema/function.c
> index bbf81a5..c0125ed 100644
> --- a/test/Sema/function.c
> +++ b/test/Sema/function.c
> @@ -17,7 +17,7 @@ void h(); // expected-note {{previous declaration is
> here}}
> void h (const char *fmt, ...) {} // expected-error{{conflicting types for
> 'h'}}
>
> // PR1965
> -int t5(b); // expected-error {{parameter list without types}}
> +int t5(b); // expected-warning {{parameter list without types}}
> int t6(int x, g); // expected-warning {{type specifier missing,
> defaults to 'int'}}
>
> int t7(, ); // expected-error {{expected parameter declarator}}
> expected-error {{expected parameter declarator}}
> diff --git a/test/Sema/invalid-decl.c b/test/Sema/invalid-decl.c
> index 950d51d..30b7657 100644
> --- a/test/Sema/invalid-decl.c
> +++ b/test/Sema/invalid-decl.c
> @@ -40,7 +40,7 @@ void foo() {
> }
>
> void test2();
> -void test2(undef); // expected-error {{a parameter list without types is
> only allowed in a function definition}}
> +void test2(undef); // expected-warning {{a parameter list without types
> is only allowed in a function definition}}
> void test2() { }
>
> void test3();
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130428/bc7ecc01/attachment.html>
More information about the llvm-commits
mailing list