Bug fix: Emit code for C++ inline function called from another inline function #31035
Conversation
… function. Adds a test that fails without the fix. The fix consists simply of removing a return statement; as a result, we now always emit code for functions referenced from the function we are currently emitting. Previously, this was not done for externally visible functions. This goes all the way back to when `GenClangDecl.cpp` was originally introduced here: swiftlang@ee22004 I speculate that the reason for doing this was that in C and Objective-C, we can assume that an externally visible function will be provided by some `.o` file we're linking against (though I'm not sure if this is in fact true for C99 inline functions). At any rate, this assumption is no longer true for C++ inline functions.
|
@rjmccall I'd like your feedback on whether this fix looks right. |
One larger change is that we're now also running this test in C as well as C++.
lib/IRGen/GenClangDecl.cpp
Outdated
| @@ -41,7 +41,6 @@ void IRGenModule::emitClangDecl(const clang::Decl *decl) { | |||
| if (!valueDecl || valueDecl->isExternallyVisible()) { | |||
| ClangCodeGen->HandleTopLevelDecl( | |||
| clang::DeclGroupRef(const_cast<clang::Decl*>(decl))); | |||
| return; | |||
| } | |||
There was a problem hiding this comment.
This looks correct, but can we preserve the fast path where the declaration does not have a definition?
There was a problem hiding this comment.
I assume you mean the fast path where the declaration isn't a ValueDecl (which I've done)?
There wasn't a fast path before for the case where the declaration doesn't have a definition (and I believe this isn't possible to check in the general case of a ValueDecl?).
There was a problem hiding this comment.
Well, the fast-path was buggy, we've established that. :) I think the point of the isExternallyVisible() check was supposed to be to look for a function declaration like void foo(); that declares something with external linkage, but of course that's not the right way to check for that.
Currently the rest of the function only does special things for FunctionDecls, although really it needs to do them for VarDecls as well. I think having a helper function that tries to find a body Stmt* for a ValueDecl wouldn't be remiss.
There was a problem hiding this comment.
Well, the fast-path was buggy, we've established that. :) I think the point of the
isExternallyVisible()check was supposed to be to look for a function declaration likevoid foo();that declares something with external linkage, but of course that's not the right way to check for that.
Ah -- that makes sense.
Currently the rest of the function only does special things for
FunctionDecls, although really it needs to do them forVarDecls as well.
Oooooh, right! I've verified this case was not being handled correctly either. See the extra test case I've added -- calledTransitivelyFromVarInit() was not being emitted before I made the code changes in my latest commit.
I think having a helper function that tries to find a body
Stmt*for aValueDeclwouldn't be remiss.
A VarDecl doesn't have a Stmt * though...? But I see what you mean, and I've introduced a helper function that returns a redeclaration of the decl containing "executable code" (body for a function, initializer for a variable), if one exists. This decl contains the body (in the case of a function) or initializer (in the case of a variable), so it's the right thing to traverse to find DeclRefs of things we should emit.
Not super-pleased with the naming of the helper function BTW (getDeclWithExecutableCode()) -- let me know if you have a better suggestion.
There was a problem hiding this comment.
A quirk of Clang's AST is that Expr is a subclass of Stmt, so VarDecls do actually have a Stmt*, but this is a fine approach, too.
There was a problem hiding this comment.
Oh right, I had forgotten about that -- makes sense, given that an expression is a valid statement in C++.
Reinstate fast path where declaration is not a ValueDecl.
martinboehme
force-pushed
the
emit-called-func
branch
from
d2626e7 to
e9ca0c7
Compare
|
@swift-ci Please test |
|
Build failed |
|
Build failed |
|
@swift-ci Please test |
|
@swift-ci Please test Windows |
|
Didn't notice that we already have a recent passing Windows build: https://ci-external.swift.org/job/swift-PR-windows/2043/ Merging. |
Bug fix: Emit code for C++ inline function called from another inline function
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
Adds a test that fails without the fix.
The fix consists simply of removing a return statement; as a result, we now always emit code for functions referenced from the function we are currently emitting.
Previously, this was not done for externally visible functions. This goes all the way back to when
GenClangDecl.cppwas originally introduced in ee22004.I speculate that the reason for doing this was that in C and Objective-C, we can assume that an externally visible function will be provided by some
.ofile we're linking against. This is also true forinlinefunctions in C99, where exactly one translation unit needs to declare the functionextern inlineto cause it to get emitted to the.ofile.C++ inline functions follow a different model, however, so we need to make sure we emit code for them.