User Details
- User Since
- Jun 3 2014, 6:35 AM (485 w, 4 d)
Jul 12 2023
Mar 22 2023
For reference, the dlopen interceptor is:
INTERCEPTOR(void*, dlopen, const char *filename, int flag) { void *ctx; COMMON_INTERCEPTOR_ENTER_NOIGNORE(ctx, dlopen, filename, flag); if (filename) COMMON_INTERCEPTOR_READ_STRING(ctx, filename, 0); void *res = COMMON_INTERCEPTOR_DLOPEN(filename, flag); Symbolizer::GetOrInit()->InvalidateModuleList(); COMMON_INTERCEPTOR_LIBRARY_LOADED(filename, res); return res; }
Mar 17 2023
Mar 6 2023
@kcc Any takes from you on this?
Feb 24 2023
Can we also get a test with llvm.type.checked.load.relative where the call *doesn't* get devirtualized?
AFAICT, this looks reasonable. If we devirtualize, we need to drop the ptrauth bundle. @ab ?
Feb 22 2023
Feb 15 2023
Feb 14 2023
Nov 12 2022
Nov 11 2022
Nov 2 2022
BTW. Repro is very trivial, would you like to add a regression test?
Oct 21 2022
Sep 30 2022
May 18 2022
Very nice!
May 6 2022
LGTM.
Apr 11 2022
+ more Apple folks
Mar 30 2022
Feb 17 2022
Feb 16 2022
Jan 12 2022
Jan 7 2022
LGTM.
Dec 20 2021
Dec 17 2021
Dec 16 2021
Aha, this is just the same treatment that's already done to the _linux.cpp version of this test: https://github.com/llvm/llvm-project/commit/c13524856bb304e6b4f80da7f5c5ecdc021920ee
Dec 15 2021
Dec 1 2021
@nikic Awesome suggestion, thanks! I didn't realize we have this nice facility to ignore dbginfo instructions. The diff is now really simple, and still fixes the problem.
Nov 4 2021
@pcc , @rjmccall , @fhahn See the other phabricator links -- I assume it might take a while to review and iterate on this. Would it be reasonable to merge *this* change in the meantime?
Nov 1 2021
- https://reviews.llvm.org/D112965 [GlobalDCE][IR] Allow and support multiple !vcall_visibility ranges
- https://reviews.llvm.org/D112929 [Clang] For VFE, emit vtable ranges in !vcall_visibility metadata
- https://reviews.llvm.org/D112967 [GlobalDCE] Precisely account for visibility when multiple !vcall_visibility ranges are present on vtables
But I don't think we should pull this into this particular patch. I saw @kubamracek already put up D112929 and I think the rest of the cleanups (removing the special handling for VFE) should be done as follow-up patches, to avoid making the patch at hand even bigger.
Oct 30 2021
I think Peter is saying that if you can make Clang emit correct ranges for v-tables — specifically so that the ranges don't cover the type_info pointer, which we don't emit enough metadata to safely remove — then you can also go ahead and remove the special case where we only eliminate loads of function pointers. That's assuming we don't need to support old bitcode.
I would still like to take this opportunity to drop the special case for Functions. Can we make it so that !vcall_visibility with a single operand means that none of the constant has the given visibility, and teach Clang to emit the correct ranges? I think this will also require supporting more than one !vcall_visibility range per global (this could be done by accommodating more than one !vcall_visibility annotation, or allowing the single annotation to have 2n+1 operands).
Thanks for the feedback! I've changed the approach to teach FunctionComparator to understand metadata in instruction operands/values, and only ignore debug info metadata.
Shouldn't this be fixed in FunctionComparator?
Oct 22 2021
Oct 21 2021
Oct 20 2021
Updated diff, should have all comments addressed, added a test for relative pointer vtable.
Oct 19 2021
Is this callback here really necessary or could it be in FindVirtualFunctionsInVTable? It seems to make it harder to see what's going on when reading FindVirtualFunctionsInVTable in isolation.
It restricts the eligible functions to a single range, which is not as powerful as the new constant approach. Just to double check, will this be sufficient for your use cases?
Oct 18 2021
Oct 15 2021
I have talked with @rjmccall offline, and I'm going to attempt to summarize his thought. @rjmccall, please correct me where I' wrong :)
Oct 14 2021
has this always been broken?
I guess it might be helpful to elaborate why it should be safe to ignore custom sections and/or when it would be safe for frontends to emit the new flag.
Oct 13 2021
Removed the leftover debug print. Added a test case for a circular dependency.
Oct 12 2021
The only way around that that I can see is to break the assumption of a static offset. You will need to tie the load to its loadee more symbolically, like by saying that you are conceptually accessing Foo.bar and annotating the field in the global as only accessed through loads tagged Foo.bar.
Oct 11 2021
Thank you for the detailed response, @rjmccall ! Let me try to explain my ultimate motivation here :)
@rjmccall, please see the history of this review, what you're saying was proposed already, and then declined by @pcc, who IIUC was involved in the original design and implementation of VFE, and who's requesting that the right way to express this in the IR is via a new constant that wraps the function pointers. @pcc provided some rationale for that -- do you disagree with it?
Oct 6 2021
Thanks for the review!
Oct 4 2021
Added IR Verifier for llvm.used.conditional.
@pcc does this approach look good to you now? Can you review? Thanks :)