- User Since
- Oct 20 2017, 4:34 PM (182 w, 3 d)
Mar 19 2021
The comment at the top says this is the master but this def seems right. @MaskRay did the compiler-rt change so I'll just tag him in.
Mar 17 2021
Fix check in test
Address Fangrui's comments
Address Fangrui's comments
I'd completely rewrite the description. Consider this:
Yea, I figured that this was likely a fragile ordering issue. I was thinking about an approach but didn't know enough about lld outside of the standard LTO code path, thus figured I should ask here first. Thanks for the feedback!
Add test case
Mar 16 2021
The isDefined() requirement is to make version-script-weak.s work: a STB_WEAK lazy symbol, if not fetched, should be treated similar to an undefined weak symbol. An undefined symbol should not have non-VER_NDX_GLOBAL versionId values.
The summary says !isLocal but the code says !isLazy. I'm assuming the code is correct.
Mar 9 2021
An older version of the patch had the vtables marked "hidden" in the LLVM assembly. My point was that clang would have given them a non-public vcall_visibility in that case.
Mar 8 2021
Well right now they are marked with public visibility, which is the same thing as not having any vcall_visibility. Since they are marked in the IR as having hidden visibility, clang presumably should have marked them with a more restricted vcall_visibility (linkage unit level).
Do you use a local: version node in a version script to make vtable symbols local in a -shared link? LTO does not know the effective binding has become local in that case and can lose devirtualization opportunities.
Thanks Fangrui! A cherry-pick and test and local run makes it seem like it works.
I see - so just to confirm, when compiling it isn't clear that these symbols are have internal or hidden visibility, but only during linking? Because otherwise clang should already have applied an appropriate vcall_visibility that allows WPD.
To me this is WAI. Why is "config->shared" true for your bitcode module? This should only affect when using the linker flags that assert you have whole program visibility during the link, which isn't true for a shared library and its symbols.
Mar 7 2021
@tejohnson I'm not sure this change is working correctly -- either that or my builds are messed up.
Feb 5 2021
Thanks, Jonas! This generates bindings but they aren't entirely functional as it generates type annotations like "lldb::SBDebugger" which Python LSPs can't figure out. I ended up adding -py3 and then running some sed post-processing to convert "lldb::\(.*\)" to \1 etc: https://github.com/lanza/lldbpybind. That lldb.py works pretty well with pyright getting type info. Figuring out how to teach the build system + swig to generate proper type annotations might require a similar post processing step.
Feb 4 2021
LGTM, I had to do this to fix it locally and can confirm it works.
Jan 23 2021
Jan 22 2021
TBH, a little while after I fixed the UB that was causing miscompiles for us, I was pulled into other things and never got back to this. I'm certainly not working on it right now, although I'm happy to review things in this area (and maybe I'll dive back in a some point if no one else does...).
Jan 13 2021
Jan 12 2021
This goes right before codegen in ThinLTOCodeGenerator. TM.addPassesToEmitFile is the entry point to the llvm backend. So the legacy implementation has this pass ran immediately after the front-end is finished and right before the backend starts. I imagine doing the same here makes the most sense.
Jan 7 2021
Great, that does indeed seem to work properly. Thanks, Jonas!
I guess if the intention of that is maintain the debugger instance around it should work, but at the moment it segfaults pretty quick with Xcode's lldb using:
On a similar note, I noticed elsewhere after fixing this that you mentioned in a commit message that you intended to remove iplist and ilist and rename as owning_ilist (or something of that sorts). Is that still on the table? Either way, the docs should be updated all around to account for either path name as there are still references to the pre-2016 implementation (e.g. https://llvm.org/docs/ProgrammersManual.html#dss-iplist).
What's the boundaries of the stable API, then? This was a public API that was removed and broke a plugin I used for vim. The author used threading.Thread(someFunc, (debugger,)) to listen on a socket for fetch requests from lldb outside of the prompt. Not the most beautiful of implementations, but it worked for years on top of a promised public stable API.
Dec 30 2020
Dec 29 2020
did it backwards
Dec 22 2020
Hey @ahatanak, I've ran into this problem in my companies projects and was wondering what the status of this patch is?
Dec 9 2020
Yup, this worked. Thank you, Louis! I can also verify that the test suite passed on centOS, Darwin and Windows with a pretty standard build.
Sorry, this was landed already but I forgot to put the Differential Revision: in! Closing it now. As far as the Windows changes you guys are talking about I'll leave up to you -- I'm not a Windows dev and was just maintaining a windows CI oncall last week.
Dec 4 2020
Dec 3 2020
Seems pretty trivially that this just requires LIBCXX_INCLUDE_TESTS and LIBCXX_ENABLE_SHARED for Windows. The error is:
Dec 2 2020
Nov 30 2020
Nov 20 2020
Nov 6 2020
Nov 5 2020
Oct 3 2020
Oct 2 2020
Clean clang-tidy warnings before landing
Oct 1 2020
Update with John's suggestions
Sep 24 2020
Fixed and added a test under the REDUNDANCY prefix.
Fix duplicate inheritance issue
Sep 23 2020
Sep 16 2020
Hmm, I thought we actually just generated a bogus definition for the protocol when it was forward-declared; really, this is better behavior that I expected. Regardless, I don't think it's worthwhile to diagnose this more strongly than a warning because of the history of not doing so.
Sep 8 2020
I don't think it'll actually error out at link time: protocol objects get emitted eagerly on use, cross-module linking is just a code-size optimization. This actually has caused longstanding problems.
A concern that has come up while rewriting this for the listed concerns is forward declared protocols that are defined as non_runtime.
Sep 2 2020
Other than the one comment LGTM.
Aug 28 2020
Looks like it, I did this around the same time the patch landed. Thanks!
Aug 20 2020
Aug 19 2020
This change provides a codegen options flag to clang -fobjc-export-direct-method-wrappers to generate the wrapper functions that begin with the prefix objc_direct_wrapper and are marked as attribute__((alwaysinline)). This way within a link unit the wrapper functions should be inlined away at their call sites, but across a dylib boundary the wrapper call is used.
Aug 11 2020
Adding jmolloy, he seems to be the original author.
Aug 4 2020
No problem! Thank you, John!
Aug 3 2020
Sounds good, just sent out a message to the mailing list.
ping @rjmccall. Any update on a timeline for this review process? Thanks!
Jul 21 2020
Apr 30 2020
Sorry for bump to this old diff, but I agree with both Jim and Greg -- we shouldn't be importing your ~/.lldbinit, but tests shouldn't depend on there never being another br s. This change should land as breakpoint set.
@rjmccall Hey John, I sent the proposal to the addresses I was pointed to but haven't heard back in multiple weeks. Any update on this?