<div dir="ltr">On Sun, Apr 28, 2013 at 6:37 AM, Serge Pavlov <span dir="ltr"><<a href="mailto:sepavloff@gmail.com" target="_blank">sepavloff@gmail.com</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div>Hi all,</div><div><br></div>This patch fixes PR15845 - Clang accepts invalid code, <a href="http://llvm.org/bugs/show_bug.cgi?id=15845" target="_blank">http://llvm.org/bugs/show_bug.cgi?id=15845</a>.<div>
Could someone please review this patch?</div>
<div>Thank you.<br></div><div>--Serge</div><div><br>Description:<br>The problem is that clang in C++ mode accepts the code:<br> int foo(xxx);<br>Clang intentionally accepts this code due to a check in Parser::ParseImplicitInt, which appeared in r156854.<br>
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.</div></div>
</blockquote><div><br></div><div style>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...</div>
<div style><br></div><div style>const n = 0; // ok?</div><div style>static f() { // ok?</div><div style> extern m; // ok?</div><div style> return m;</div><div style>}</div><div><br></div><div style>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.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div>
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.<br></div></div></blockquote><div>
<br></div><div>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.</div>
<div><br></div><div style>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.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div dir="ltr"><div>Files:<br> include/clang/Basic/DiagnosticSemaKinds.td<br>
lib/Parse/ParseDecl.cpp<br> lib/Sema/SemaType.cpp<br> test/Sema/MicrosoftCompatibility.cpp<br> test/Sema/alloc_size.c<br> test/Sema/function.c<br> test/Sema/invalid-decl.c<br><br>-----------------------------------------------------------------------------------------------------<br>
diff --git a/include/clang/Basic/DiagnosticSemaKinds.td b/include/clang/Basic/DiagnosticSemaKinds.td<br>index 1461716..166dbab 100644<br>--- a/include/clang/Basic/DiagnosticSemaKinds.td<br>+++ b/include/clang/Basic/DiagnosticSemaKinds.td<br>
@@ -2314,8 +2314,9 @@ def err_void_only_param : Error<<br> "'void' must be the first and only parameter if specified">;<br> def err_void_param_qualified : Error<<br> "'void' as parameter must not have type qualifiers">;<br>
-def err_ident_list_in_fn_declaration : Error<<br>- "a parameter list without types is only allowed in a function definition">;<br>+def warn_ident_list_in_fn_declaration : Warning<<br>+ "a parameter list without types is only allowed in a function definition">,<br>
+ InGroup<C99Compat>;<br></div></div></blockquote><div><br></div><div>This should be an ExtWarn, not a Warning, since this is a required diagnostic per the various C language standards. Also, C99Compat seems wrong.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div dir="ltr"><div> def ext_param_not_declared : Extension<<br> "parameter %0 was not declared, defaulting to type 'int'">;<br> def err_param_typedef_of_void : Error<<br>diff --git a/lib/Parse/ParseDecl.cpp b/lib/Parse/ParseDecl.cpp<br>
index d786ce2..2f0c1a3 100644<br>--- a/lib/Parse/ParseDecl.cpp<br>+++ b/lib/Parse/ParseDecl.cpp<br>@@ -2038,10 +2038,9 @@ bool Parser::ParseImplicitInt(DeclSpec &DS, CXXScopeSpec *SS,<br> // error, do lookahead to try to do better recovery. This never applies<br>
// within a type specifier. Outside of C++, we allow this even if the<br> // language doesn't "officially" support implicit int -- we support<br>- // implicit int as an extension in C99 and C11. Allegedly, MS also<br>
- // supports implicit int in C++ mode.<br>+ // implicit int as an extension in C99 and C11.<br> if (DSC != DSC_type_specifier && DSC != DSC_trailing &&<br>- (!getLangOpts().CPlusPlus || getLangOpts().MicrosoftExt) &&<br>
+ !getLangOpts().CPlusPlus &&<br></div></div></blockquote><div><br></div><div style>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.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div> isValidAfterIdentifierInDeclarator(NextToken())) {<br> // If this token is valid for implicit int, e.g. "static x = 4", then<br>
// we just avoid eating the identifier, so it will be parsed as the<br>
diff --git a/lib/Sema/SemaType.cpp b/lib/Sema/SemaType.cpp<br>index 8bf5143..243b772 100644<br>--- a/lib/Sema/SemaType.cpp<br>+++ b/lib/Sema/SemaType.cpp<br>@@ -2742,7 +2742,7 @@ static TypeSourceInfo *GetFullTypeForDeclarator(TypeProcessingState &state,<br>
if (FTI.NumArgs && FTI.ArgInfo[0].Param == 0) {<br> // C99 6.7.5.3p3: Reject int(x,y,z) when it's not a function<br> // definition.<br>- S.Diag(FTI.ArgInfo[0].IdentLoc, diag::err_ident_list_in_fn_declaration);<br>
+ S.Diag(FTI.ArgInfo[0].IdentLoc, diag::warn_ident_list_in_fn_declaration);<br> D.setInvalidType(true);<br></div></div></blockquote><div><br></div><div style>If you're not issuing an error, you must build a correct AST -- you can't set things invalid.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div> // Recover by creating a K&R-style function type.<br> T = Context.getFunctionNoProtoType(T);<br>
diff --git a/test/Sema/MicrosoftCompatibility.cpp b/test/Sema/MicrosoftCompatibility.cpp<br>new file mode 100644<br>index 0000000..15c2558<br>--- /dev/null<br>+++ b/test/Sema/MicrosoftCompatibility.cpp<br>@@ -0,0 +1,4 @@<br>
+// RUN: %clang_cc1 %s -fsyntax-only -Wno-unused-value -Wmicrosoft -verify -fms-compatibility<br>+<br>+// PR15845<br>+int foo(xxx); // expected-error{{unknown type name}}<br>diff --git a/test/Sema/alloc_size.c b/test/Sema/alloc_size.c<br>
index 84f3932..0067bea 100644<br>--- a/test/Sema/alloc_size.c<br>+++ b/test/Sema/alloc_size.c<br>@@ -22,6 +22,6 @@ void *fn8(int, int) __attribute__((alloc_size(1, 1))); // OK<br> 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}}<br>
<br> void* fn10(size_t, size_t) __attribute__((alloc_size(1,2))); // expected-error{{redefinition of parameter}} \<br>- // expected-error{{a parameter list without types is only allowed in a function definition}} \<br>
+ // expected-warning{{a parameter list without types is only allowed in a function definition}} \<br> // expected-error{{attribute parameter 1 is out of bounds}}<br>
void* fn11() __attribute__((alloc_size(1))); // expected-error{{attribute parameter 1 is out of bounds}}<br>diff --git a/test/Sema/function.c b/test/Sema/function.c<br>index bbf81a5..c0125ed 100644<br>--- a/test/Sema/function.c<br>
+++ b/test/Sema/function.c<br>@@ -17,7 +17,7 @@ void h(); // expected-note {{previous declaration is here}}<br> void h (const char *fmt, ...) {} // expected-error{{conflicting types for 'h'}}<br> <br> // PR1965<br>
-int t5(b); // expected-error {{parameter list without types}}<br>+int t5(b); // expected-warning {{parameter list without types}}<br> int t6(int x, g); // expected-warning {{type specifier missing, defaults to 'int'}}<br>
<br> int t7(, ); // expected-error {{expected parameter declarator}} expected-error {{expected parameter declarator}}<br>diff --git a/test/Sema/invalid-decl.c b/test/Sema/invalid-decl.c<br>index 950d51d..30b7657 100644<br>
--- a/test/Sema/invalid-decl.c<br>+++ b/test/Sema/invalid-decl.c<br>@@ -40,7 +40,7 @@ void foo() {<br> }<br> <br> void test2();<br>-void test2(undef); // expected-error {{a parameter list without types is only allowed in a function definition}}<br>
+void test2(undef); // expected-warning {{a parameter list without types is only allowed in a function definition}}<br> void test2() { }<br> <br> void test3();<br><br><br></div></div>
<br>_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
<br></blockquote></div><br></div></div>