Place implictly declared functions at block scope

[chill]

Summary

Such implicitly declared functions behave as if the enclosing block contained the declaration
extern int name() (C90, 6.3.3.2 Function calls) , thus their names should have block scope
(C90, 6.1.2.1 Scope of identifiers) .

This patch fixes https://bugs.llvm.org/show_bug.cgi?id=33224

[chill]

This issue https://bugs.llvm.org//show_bug.cgi?id=2266 does not appear to trigger anymore, with this patch applied.

[rogfer01]

I wonder what should we do in C99 for cases like

void foo(void) {

  {
    extern void g(int(*a)[call_to_undeclared()]);

    int (*p_ok)() = call_to_undeclared;
  }
  int (*p_err)() = call_to_undeclared;
}

I understand that the scope S in this case is a (function) prototype scope so it would not be the innermost block scope, would it? That said, GCC does not accept p_ok above so probably this behaviour is sensible, after all in C99 this is an extension.

[chill]

In D33676#767746, @rogfer01 wrote:

I understand that the scope S in this case is a (function) prototype scope so it would not be the innermost block scope, would it? That said, GCC does not accept p_ok above so probably this behaviour is sensible, after all in C99 this is an extension.

I actually missed the possibility of a function call expression to appear in such a context. I also think that GCC (and Clang) should accept the p_ok initializer, as they do if one adds extern int call_to_undeclared(); at the beginning of the block.

[rogfer01]

I think what happens is that the implicit declaration is done in the prototype scope which is not visible later.

Doing the same as GCC in the handling of this extension is probably the best: note that in C90 a function call should not happen inside an array declaration because VLAs do not exist there. So all the expressions inside an array declarator are constant-expressions, which in C90 (at least) do not allow function-calls.

[chill]

Update 1 notes.

The scopes for compound statements are explicitly marked as such, so then Sema::ImplicitylDefineFunction can ascend to the nearest enclosing scope. The function scopes (ones with Scope::FnScope) do not necessarily have the Scope::CompoundStmtScope flag set, that's why function scope is tested separately. I have considered, for consistency, setting the compound statement flag to all kinds of compound statement scopes, including function scopes, try/catch/SEH, block scopes, etc., but , given that this makes difference just for C90, I decided that such sweeping changes are unwarranted.

[rogfer01]

Given that we are already considering extensions, maybe you want to add a test for compound statements in expressions.

void foo(void)
{
  ({ bar(); });
  int (*p)() = bar; /* expected-error {{use of undeclared identifier 'bar'}} */
}

This already works with your patch: just to make clear how we interact with it.

test/Sema/implicit-decl.c

12

Is there a way not to lose the note?

[chill]

Update 2: added a testcase involving expression statements statement expressions.

[chill]

test/Sema/implicit-decl.c

12

The note is a part of the conflicting types diagnostic, that's no longer issued.

[rogfer01]

Thanks Momchil, this looks sensible to me.

What do you think @aaron.ballman @rsmith ?

[chill]

Ping?

[chill]

Ping.

[rsmith]

lib/Parse/ParseStmt.cpp

843–845 ↗

(On Diff #102802)

This seems to miss quite a lot of places that introduce compound statement scopes. (Search for callers of ParseCompundStatementBody and callers of the 2-argument form of ParseCompoundStatement for those.)

[chill]

lib/Parse/ParseStmt.cpp

843–845 ↗

(On Diff #102802)

Yes, all the other cases are not applicable to C90, they are C++, ObjC, etc and the new flag is tested only in one place and only for C90.
However, for the sake of consistency, I've updated the patch to set the compound statement flag on all compound statements, including function bodies, try/catch/finally/blocks and whatnot.

[chill]

Set the compound statement flag on all compound statement scopes (previous version used to set the flag on just enough scopes
as to be sufficient for the purpose of inserting C90 implicit function declarations in said scopes).

[chill]

Ping.

[chill]

Ping.

[chill]

Ping.

[aaron.ballman]

lib/Parse/ParseCXXInlineMethods.cpp

521–522 ↗

(On Diff #106056)

Did clang-format produce this? The lack of spaces between the | operators is surprising. Same comment applies elsewhere, so you might want to run clang-format over the patch if you've not already done so.

[chill]

lib/Parse/ParseCXXInlineMethods.cpp

521–522 ↗

(On Diff #106056)

No, I wrote it manually like this to match the already existing style. Will be glad to run clang-format on all the touched files, though.

[aaron.ballman]

This looks reasonable to me, once it's been formatted. Let's give @rsmith a few days to comment before committing, though.

[chill]

Updated with only whitespace changes after formating each changed line with clang-format.

[chill]

Thanks for the review, I'll wait a few days.

lib/Sema/SemaDecl.cpp

D.AddTypeInfo(DeclaratorChunk::getFunction(/*HasProto=*/false,
                                              /*NoexceptExpr=*/nullptr,
                                              /*ExceptionSpecTokens=*/nullptr,
                                              /*DeclsInPrototype=*/None,
                                              Loc, Loc, D),
                 DS.getAttributes(),
                 SourceLocation());
   D.SetIdentifier(&II, Loc);
 
-  // Insert this function into translation-unit scope.
+  // Insert this function into the current scope.
 
   DeclContext *PrevDC = CurContext;
   CurContext = Context.getTranslationUnitDecl();
 
-  FunctionDecl *FD = cast<FunctionDecl>(ActOnDeclarator(TUScope, D));
+  FunctionDecl *FD = cast<FunctionDecl>(ActOnDeclarator(S, D));
   FD->setImplicit();
 
   CurContext = PrevDC;
 
   AddKnownFunctionAttributes(FD);
 
   return FD;
 }

test/Sema/implicit-decl-c90.c

+// RUN: %clang_cc1 %s -std=c90 -verify -fsyntax-only
+int f() {
+  return g() + 1;
+}
+
+int (*p)() = g; /* expected-error {{use of undeclared identifier 'g'}} */
+
+float g(); /* not expecting conflicting types diagnostics here */

test/Sema/implicit-decl.c

 // RUN: %clang_cc1 %s -verify -fsyntax-only -Werror
 
 typedef int int32_t;
 typedef unsigned char Boolean;
 
 extern int printf(__const char *__restrict __format, ...); // expected-note{{'printf' declared here}}
 
 void func() {
    int32_t *vector[16];
    const char compDesc[16 + 1];
    int32_t compCount = 0;
-   if (_CFCalendarDecomposeAbsoluteTimeV(compDesc, vector, compCount)) { // expected-note {{previous implicit declaration is here}} \
-         expected-error {{implicit declaration of function '_CFCalendarDecomposeAbsoluteTimeV' is invalid in C99}}
+   if (_CFCalendarDecomposeAbsoluteTimeV(compDesc, vector, compCount)) { // expected-error {{implicit declaration of function '_CFCalendarDecomposeAbsoluteTimeV' is invalid in C99}}
    }
 
    printg("Hello, World!\n"); // expected-error{{implicit declaration of function 'printg' is invalid in C99}} \
                               // expected-note{{did you mean 'printf'?}}
 
   __builtin_is_les(1, 3); // expected-error{{use of unknown builtin '__builtin_is_les'}}
 }
-Boolean _CFCalendarDecomposeAbsoluteTimeV(const char *componentDesc, int32_t **vector, int32_t count) { // expected-error{{conflicting types for '_CFCalendarDecomposeAbsoluteTimeV'}}
+Boolean _CFCalendarDecomposeAbsoluteTimeV(const char *componentDesc, int32_t **vector, int32_t count) {
  return 0;
 }
 
 
 // Test the typo-correction callback in Sema::ImplicitlyDefineFunction
 extern int sformatf(char *str, __const char *__restrict __format, ...); // expected-note{{'sformatf' declared here}}
 void test_implicit() {
   int formats = 0;
   formatd("Hello, World!\n"); // expected-error{{implicit declaration of function 'formatd' is invalid in C99}} \
                               // expected-note{{did you mean 'sformatf'?}}
 }