[IRGen] Use the lexical decl context for VarDecls. #33306

3405691582 · 2020-08-05T01:36:10Z

In emitClangDecl, we spend some time ensuring that we only pass file or top-level decls to LLVM. This is done by examining the decl context (via Decl::getDeclContext) and determining whether isFileContext is true. On the LLVM side, CodeGenModule::EmitGlobal asserts if the decl eventually passed to it is not a "file scoped" via VarDecl::isFileVarDecl.

Generally, these concepts match in all cases but one. Local extern variables have a decl context which has isFileContext being true, just like global extern variables, but isFileVarDecl does not return true -- it is a local variable, after all.

Indeed, isFileVarDecl is implemented by examining the context with Decl::getLexicalDeclContext instead of Decl::getDeclContext. Thus, we should probably be using getLexicalDeclContext instead of getDeclContext in emitClangDecl, so we properly match the expectations on the LLVM side.

This indirectly addresses #28968, since that contains the only part of the Swift stdlib that uses a local extern variable, but any header that is used with Swift that contains a local extern variable will cause the compiler to assert when built with assertions enabled.

//

Note to the prospective reviewer: I'm fairly confident this is correct, but it would be great for someone more familiar with this corner of the code to double-check my reasoning and whether this change is valid.

3405691582 · 2020-08-19T00:37:34Z

Ping. Let me know if you need further information.

3405691582 · 2020-08-31T18:48:45Z

Ping.

As additional context, this doesn't just happen specifically when building stdlib (and the specific case described in #28968), this happens when using any C header with a local extern variable and with LLVM built with assertions enabled.

For example, this is a reproduction case from a Linux environment using a development snapshot:

$ cat y.h
int _foo() { extern int i; return i; }
$ cat y.swift
public var foo: Int32 {
  return _foo()
}

print("\(foo)")
$ ./swift-DEVELOPMENT-SNAPSHOT-2020-07-22-a-ubuntu16.04/usr/bin/swiftc -import-objc-header y.h y.swift
swift-frontend: /home/buildnode/jenkins/workspace/oss-swift-package-linux-ubuntu-16_04/llvm-
project/clang/lib/CodeGen/CodeGenModule.cpp:2513: void clang::CodeGen::CodeGenModule::EmitGlobal(clang::GlobalDecl): 
Assertion `VD->isFileVarDecl() && "Cannot emit local var decl as global."' failed.
<snip>

CodaFi · 2020-08-31T20:20:01Z

Please commit that as a regression test. I'll try to get eyes on this.

3405691582 · 2020-09-01T00:39:25Z

Thanks. I've gone ahead and added a test to this commit.

I was a little unsure at first whether to add a test for this since it only occurs when LLVM is built with assertions enabled, but I've done so now anyway.

rjmccall · 2020-09-01T18:53:14Z

test/IRGen/Inputs/local_extern.h

@@ -0,0 +1 @@
+static inline int _foo() { extern int i; return i; }


You should test the case where a function uses an extern declaration that's actually defined within the header. And the test needs to verify that we actually emit the variable.

You also need to test extern functions.

Added additional cases where a local extern declaration has no prior definitions of variables and functions, as well for local externs with prior definitions of variables and functions.

Added checks for the extern declaration name in the emitted IR.

Actually, these cases as-is probably aren't quite sufficient to ensure correctness...

These cases should be correct now.

rjmccall · 2020-09-01T19:00:25Z

lib/IRGen/GenClangDecl.cpp

@@ -80,7 +80,7 @@ void IRGenModule::emitClangDecl(const clang::Decl *decl) {
    // If it's a member of a file-level decl, like a C++ static member variable,
    // we want to add the entire file-level declaration because Clang doesn't
    // expect to see members directly here.
-    for (auto *DC = D->getDeclContext();; DC = DC->getParent()) {
+    for (auto *DC = D->getLexicalDeclContext();; DC = DC->getParent()) {


I'm not sure this is right; your logic makes us ignore local externs, which works in your test case only because we don't actually need to emit the variable/function. We need to look for a non-local-extern declaration.

Can you expand a little more about that? I'm not sure I follow 100%. In practice, there are cases where the resolution of the local extern may not happen until link time.

Sure, and in those cases we aren't responsible for emitting the declaration; it's the programmer's responsibility to ensure that the symbol is defined somewhere. But if there is a definition in the header, it might still be our obligation to trigger its emission, e.g.

int get_x() { extern const int x; return x; } const int x = 5;

And you can get a similar effect with functions with inline.

I see the problem: as it stands here, we're not emitting any references to symbols from local externs, even when we are able to resolve them immediately. I'll look in to an alternative way of handling this.

emitClangDecl interacts with clang and LLVM to achieve C interop. On the LLVM side, CodeGenModule::EmitGlobal asserts if the decl eventually passed to it is not a "file scoped" via VarDecl::isFileVarDecl. LLVM currently asserts on local extern variables in C headers passed to Swift when the definition exists outside that header. To fix this, we need to ensure that we are only passing Decls that do not trip the assertion but not unduly limit local extern variables when the corresponding definition exists inside that header. We can do that fairly simply by checking for isFileVarDecl just before we hand-off to clang. When the definition for the local extern variable exists inside the header, isFileVarDecl is true, and if it exists elsewhere, it is false. This matches up with the assert expectation on the LLVM side exactly. This indirectly addresses swiftlang#28968, since that contains the only part of the Swift stdlib that uses a local extern variable, but any header that is used with Swift that contains a local extern variable will cause the compiler to assert when built with assertions enabled.

3405691582 · 2020-09-02T13:25:21Z

Okay, PTAL.

Unit test covers the case of calling extern functions as well as variables and functions defined prior in the header. Code change is now to filter out VarDecls that are not isFileVarDecl right before passing them to clang. This change has that unit test pass and also matches the assert in LLVM.

rjmccall

Thanks, LGTM.

rjmccall · 2020-09-04T04:54:48Z

@swift-ci Please test.

swift-ci · 2020-09-04T04:57:40Z

Build failed
Swift Test Linux Platform
Git Sha - 555109c915bbe1bfa467e3ae7f1ac5a091f2e2ed

swift-ci · 2020-09-04T04:58:52Z

Build failed
Swift Test OS X Platform
Git Sha - 555109c915bbe1bfa467e3ae7f1ac5a091f2e2ed

rjmccall · 2020-09-04T20:09:06Z

Thank you!

compnerd · 2020-09-05T00:19:16Z

@shahmishal - this didn't trigger the windows CI?

compnerd · 2020-09-05T00:19:45Z

This seems to have caused a regression on Windows that @xedin is hitting (https://ci-external.swift.org/job/swift-PR-windows/6918/console)

Explanation: LLVM currently asserts on local extern variables in C headers passed to Swift when the definition exists outside that header. Scope: This never worked. Risk: Low. Testing: Regression test added. Reviewed-by: John McCall Original PR: swiftlang#33306 This fix also required the cherry-pick of: swiftlang#31035 and swiftlang#31272 rdar://67951491

theblixguy requested a review from rjmccall August 5, 2020 01:58

3405691582 force-pushed the FixAssertingLocalExtern branch from 555109c to 11a2c52 Compare September 1, 2020 00:33

rjmccall requested changes Sep 1, 2020

View reviewed changes

rjmccall mentioned this pull request Sep 1, 2020

IRGen: Ignore local variable declaration in inline c functions #33746

Closed

3405691582 force-pushed the FixAssertingLocalExtern branch 5 times, most recently from 6541bc4 to 20f1493 Compare September 2, 2020 04:40

3405691582 force-pushed the FixAssertingLocalExtern branch from 20f1493 to 38dd7d8 Compare September 2, 2020 04:50

rjmccall approved these changes Sep 4, 2020

View reviewed changes

rjmccall merged commit ddb43e3 into swiftlang:master Sep 4, 2020

compnerd mentioned this pull request Sep 5, 2020

Revert "[IRGen] Use the lexical decl context for VarDecls." #33813

Closed

xedin mentioned this pull request Sep 5, 2020

[ConstraintSystem] Record generic fix if destination of a pointer con… #33809

Merged

aschwaighofer mentioned this pull request Sep 9, 2020

[5.3] IRGen: Fix asserting local extern declarations. #33881

Merged

aschwaighofer mentioned this pull request Oct 13, 2020

[SR-12539] Linker error: undefined symbols for architecture x86_64 #54983

Closed

		@@ -0,0 +1 @@
		static inline int _foo() { extern int i; return i; }

[IRGen] Use the lexical decl context for VarDecls. #33306

[IRGen] Use the lexical decl context for VarDecls. #33306

Uh oh!

Conversation

3405691582 commented Aug 5, 2020

Uh oh!

3405691582 commented Aug 19, 2020

Uh oh!

3405691582 commented Aug 31, 2020

Uh oh!

CodaFi commented Aug 31, 2020

Uh oh!

3405691582 commented Sep 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rjmccall Sep 1, 2020

Choose a reason for hiding this comment

Uh oh!

3405691582 Sep 1, 2020

Choose a reason for hiding this comment

Uh oh!

3405691582 Sep 1, 2020

Choose a reason for hiding this comment

Uh oh!

3405691582 Sep 1, 2020

Choose a reason for hiding this comment

Uh oh!

rjmccall Sep 1, 2020

Choose a reason for hiding this comment

Uh oh!

3405691582 Sep 1, 2020

Choose a reason for hiding this comment

Uh oh!

rjmccall Sep 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

3405691582 Sep 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

3405691582 commented Sep 2, 2020

Uh oh!

rjmccall left a comment

Choose a reason for hiding this comment

Uh oh!

rjmccall commented Sep 4, 2020

Uh oh!

swift-ci commented Sep 4, 2020

Uh oh!

swift-ci commented Sep 4, 2020

Uh oh!

rjmccall commented Sep 4, 2020

Uh oh!

compnerd commented Sep 5, 2020

Uh oh!

compnerd commented Sep 5, 2020

Uh oh!

Uh oh!

3405691582 commented Sep 1, 2020 •

edited

Loading

rjmccall Sep 1, 2020 •

edited

Loading

3405691582 Sep 1, 2020 •

edited

Loading