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;
}

@kcc Any takes from you on this?

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 ?

In D143959#4128779, @dvyukov wrote:

kubamracek added a reviewer: dvyukov.

Do you want me to look at something in particular? Frankly, I don't understand the details here and can't meaningfully review.
If you think it's a good change and tests pass, I would say go on and merge.

BTW. Repro is very trivial, would you like to add a regression test?

Very nice!

LGTM.

+ more Apple folks

LGTM.

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

@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.

@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?

• 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.

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?

Updated diff, should have all comments addressed, added a test for relative pointer vtable.

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?

I have talked with @rjmccall offline, and I'm going to attempt to summarize his thought. @rjmccall, please correct me where I' wrong :)

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.

Removed the leftover debug print. Added a test case for a circular dependency.

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.

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?

@MaskRay, @pcc friendly ping on the review request :)

In D111096#3046948, @MaskRay wrote:

Can you demonstrate the intended usage by marking that patch as a child revision (Edit Related Revision)?

Thanks for the review!

Added IR Verifier for llvm.used.conditional.

@pcc does this approach look good to you now? Can you review? Thanks :)