[PATCH] Fix to PR15845 - Clang accepts invalid code
Serge Pavlov
sepavloff at gmail.com
Mon Apr 29 10:31:30 PDT 2013
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.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/54d14921/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: Fix-to-PR15845-Clang-accepts-invalid-code.patch
Type: application/octet-stream
Size: 3302 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130430/54d14921/attachment.obj>
More information about the cfe-commits
mailing list