[stdlib] Fix a LLVM assert on *BSD. #28968
Conversation
3405691582
force-pushed
the
environ_llvm_assert
branch
from
6ee4cad to
0d1f556
Compare
3405691582
changed the title
|
Hmm, this creates a declaration which is now exposed to swift, which is not ideal. Is there no way to limit the scope? Do you know if this occurs on the current top-of-tree LLVM? We will be branching soon for 5.2, and I wonder if we can get wait until then (though, we could also backport the LLVM fix to deal with the issue if we know it is fixed already). I really would prefer that we fix LLVM here. |
|
AIUI yes; this looks like a relatively fresh checkout: llvm-project/clang has remote https://github.com/apple/llvm-project.git, HEAD is at c3e5ae3 (swift/master). Let me know if you need me to retest against a different branch. Off the top of my head, and without going spelunking into the LLVM codegen library, there doesn't appear to be a simple way around this. |
3405691582
force-pushed
the
environ_llvm_assert
branch
from
0d1f556 to
f4de9cb
Compare
|
I agree that fixing this in LLVM would be preferable. Depending on how long that takes, we may still want to work around it here (with a note to put it back once LLVM is fixed). |
|
@3405691582 - swift/master is not near top of tree, master-next would be closer than that, but, still not necessarily very close. |
|
OK: I'll switch branches on LLVM, rebuild, and report back when it's all done. |
|
@3405691582 make sure that you switch branches on swift when you do that - there are API changes that need to be accounted for. |
|
Indeed, as master-next doesn't build cleanly out of the box (even on Linux), checking this branch might take a little time and effort. |
|
Thats fair; we are going to have to do that work soon anyway to bump the clang/LLVM version at which point it will become master. |
However, when building Glibc with assertions enabled, LLVM asserts in CodeGenModule::EmitGlobal with "Cannot emit local var decl as global". This assert is _probably_ wrong in LLVM because the local extern reference isn't being handled properly and needs to be addressed there. We could move the declaration to global scope, but that is not ideal because it makes the pointer declaration visible to Swift. OpenBSD needs to implement _swift_stdlib_getEnviron regardless, so let's do so.
3405691582
force-pushed
the
environ_llvm_assert
branch
from
f4de9cb to
a6b0e92
Compare
|
Hello again; bringing this back to life. I've sniffed around the process whereby this assert gets tripped, and I've tentatively concluded that the assert in LLVM is probably wrong and the scope of the assert needs expanding. However, OpenBSD still needs to see _swift_stdlib_getEnviron, so I've changed the scope of this PR to just expand the scope of the #ifdef, and I'll chase this up with the LLVM project separately. Can we take another look? |
|
Well, this looks like a simple and straightforward change to me! |
|
Ping. Anything left to get this committed? |
|
Ping. The actual assertion work is addressed in the aforementioned PR now, this is now just ensuring that |
|
Whoops, sorry about that! I'll kick off a test and merge. |
|
@swift-ci please test and merge |
|
Strange, it looks like that didn't seem to take effect? (edit. Or maybe it did now since I touched the PR? 😂 ) |
|
I guess you unstuck it. I think we had some CI infrastructure issues that this may have run into. Anyway, seems to be going now. Please feel free to ping me if you see anything else go wrong. |
|
Hmm, is this stuck again? The tests have passed but the merge doesn't seem to have occurred. |
|
Don’t know why it didn’t merge automatically, but I put in the great effort of tapping the button. |
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.
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.
3 participants
When building Glibc.o, LLVM asserts in CodeGenModule::EmitGlobal
with "Cannot emit local var decl as global.". This appears to be
because of the way _swift_stdlib_getEnviron is written for the BSDs
as an extern local, since moving the extern declaration to global scope
seems to avoid the problem.
(Note: this is tested on a WIP OpenBSD port, but not on FreeBSD proper; nevertheless, I think the FreeBSD port is probably broken and this will likely fix the problem on both platforms. However, I'm happy to go back and create distinct ifdefs for FreeBSD and OpenBSD separately if there's a concern.)