-
-
Notifications
You must be signed in to change notification settings - Fork 267
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Emulate @weak functions on Windows and don't emit COMDATs for ELF anymore #3424
Conversation
I've looked into this because it might be a solution for #3158, at least for LDC - only emitting weak |
103a0ac
to
a5507cf
Compare
Is there any point in emitting Comdats for ELF? It prevents |
tests/PGO/irbased_indirect_calls.d
Outdated
@@ -27,7 +27,7 @@ extern (C) | |||
|
|||
void function() foo; | |||
|
|||
@weak // disable reasoning about this function | |||
pragma(inline, false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does not do the same thing. What we want is the function to be opaque. Disabling inlining still allows IPO to learn about what is written to foo
. I think the same holds for optStrategy(none)
.
Edit: but applied to the hot
function it does not matter here because there indeed we don't want inlining.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just wanted to avoid additional complexity for string matching; for hot
, disabling inlining wasn't enough, as the call was still elided. What worked was pragma(inline, false); llvm_sideeffect();
in the hot
body, but optStrategy
seemed nicer and had the same effect.
For select_func
here, I thought disabling inlining would be enough (for string matching in the main body wrt. the foo
function ptr); re-adding @weak
wouldn't affect any strings though, and might even fix the regression with LLVM 4.0.
@rainers: Any suggestions wrt. how to 'best' adapt the mangled name (for backtrace/debugger/...)? |
If you want it to be demangled with some modifications, prepending a package Your use case in #3158 probably needs C++ symbols, though. VC mangling doesn't encode identifier length, so patching the first identifier might work, but it can be back referenced so it appears multiple times in the demangled name. |
Cleaned up & put a little more effort into the renamed mangle. clang emits COMDATs for templates and inline functions only (with |
You could ask on the forums about the COMDATs on Linux, but I doubt you'll get useful feedback. |
Yeah I wanted to know if you guys know more about this, otherwise I'd propose to include this in the upcoming beta and see how it goes. |
Interestingly, the produced static libraries are now noticeably smaller for the Azure Linux x64 job (compared against latest master run):
The shared libs are minimally larger:
|
if (lwc.second) | ||
obj->setComdat(gIR->module.getOrInsertComdat(obj->getName())); | ||
obj->setComdat(lwc.second ? gIR->module.getOrInsertComdat(obj->getName()) | ||
: nullptr); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, I've also tried setting the COMDAT's selection kind to 'exact match' (instead of 'any'), but that only led back to the multiple definition errors.
…more Emulation for @weak global variables is still left out but should be analogous. The test adaptations are mostly a revert of 3893840. The testcase has shown that @weak hasn't worked properly for ELF (linker apparently prefers the version in the 1st object file, independent of whether it's weak or not), because the functions are emitted in COMDATs. clang emits COMDATs for templates and inline functions only, not for regular functions.
I just found out that the ELF COMDAT change is causing trouble at Weka. It prevents merging of symbols, leaving excess data around in a section where Weka's tooling wants there to be no garbage (or well, it just exceeds the amount of data that the system can deal with (
I'll work on a small testcase, so you can see what the problem is. |
Opened an issue for it with testcase: #3589 |
LLVM 11 release notes: @kinke Does that remove the need for the "emulation" of this PR ? |
I've read that, found it a nice conincidence, but haven't tested whether it works similar to this manual emulation here. Anyway, it's not restricted to LLVM11+, and we always emit comdats on Windows. |
The test adaptations are mostly a revert of 3893840.
@weak
global variables are still left out but could be emulated exactly the same way.