This is an archive of the discontinued LLVM Phabricator instance.

Place implictly declared functions at block scope
ClosedPublic

Authored by chill on May 30 2017, 7:32 AM.

Details

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

Diff Detail

Repository
rL LLVM

Event Timeline

chill created this revision.May 30 2017, 7:32 AM

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

rogfer01 edited edge metadata.May 30 2017, 8:04 AM

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 added a comment.May 30 2017, 8:54 AM

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 added a comment.EditedMay 30 2017, 9:02 AM

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 updated this revision to Diff 101522.EditedJun 6 2017, 1:34 AM

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.

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 ↗(On Diff #101522)

Is there a way not to lose the note?

chill updated this revision to Diff 102802.EditedJun 16 2017, 4:05 AM

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

chill added inline comments.Jun 16 2017, 4:56 AM
test/Sema/implicit-decl.c
12 ↗(On Diff #101522)

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

Thanks Momchil, this looks sensible to me.

What do you think @aaron.ballman @rsmith ?

rsmith added inline comments.Jul 7 2017, 11:43 AM
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 added inline comments.Jul 11 2017, 9:44 AM
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 updated this revision to Diff 106056.Jul 11 2017, 9:56 AM

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).

aaron.ballman added inline comments.Aug 2 2017, 7:23 AM
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 added inline comments.Aug 2 2017, 8:15 AM
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 accepted this revision.Aug 2 2017, 9:24 AM

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

This revision is now accepted and ready to land.Aug 2 2017, 9:24 AM
chill updated this revision to Diff 109366.Aug 2 2017, 9:41 AM

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

chill added a comment.Aug 2 2017, 9:42 AM

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

This revision was automatically updated to reflect the committed changes.