This is an archive of the discontinued LLVM Phabricator instance.

[lsan][Darwin] Scan libdispatch and Foundation memory regions
ClosedPublic

Authored by lgrey on Jul 8 2022, 10:48 AM.

Details

Summary

libdispatch uses its own heap (_dispatch_main_heap) for some allocations, including the dispatch_continuation_t that holds a dispatch source's event handler.
Objective-C block trampolines (creating methods at runtime with a block as the implementations) use the VM_MEMORY_FOUNDATION region (see https://github.com/apple-oss-distributions/objc4/blob/8701d5672d3fd3cd817aeb84db1077aafe1a1604/runtime/objc-block-trampolines.mm#L371).

This change scans both regions to fix false positives. See tests for details; unfortunately I was unable to reduce the trampoline example with imp_implementationWithBlock on a new class, so I'm resorting to something close to the bug as seen in the wild.

Diff Detail

Event Timeline

lgrey created this revision.Jul 8 2022, 10:48 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 8 2022, 10:48 AM
Herald added a subscriber: Enna1. · View Herald Transcript
lgrey requested review of this revision.Jul 8 2022, 10:48 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 8 2022, 10:48 AM
Herald added a subscriber: Restricted Project. · View Herald Transcript
fjricci resigned from this revision.Jul 8 2022, 10:52 AM

Hey sorry, it’s been several years since I’ve done anything LLVM-related, I don’t think I’m still the right reviewer for this code.

I actually don’t even write C++ anymore.

lgrey edited reviewers, added: yln; removed: fjricci.Jul 8 2022, 11:09 AM
yln added a comment.Jul 8 2022, 11:40 AM

Hi Leonard!

Does this fix the check-lsan and check-asan targets (currently many tests are failing there on macOS Beta 13)?

If this fixes a specific problem not already exposed by existing tests, then can we try to add a test for it?
If this fixes existing tests, can you mention in the commit messages which ones they are?

Thanks!

yln added a comment.Jul 8 2022, 11:41 AM
This comment was removed by yln.
lgrey added a comment.Jul 8 2022, 12:43 PM

Does this fix the check-lsan and check-asan targets (currently many tests are failing there on macOS Beta 13)?
Thanks!

To be perfectly honest, I'm not sure since the objc runtime issues are masking everything else! I'm going to send a separate change to suppress those and get back to you :) (For context, I'm working on enabling LSAN for Mac Chromium, so there's probably a few more of these in the pipeline).

Are there any public builders running these suites on 13? My 13 machine doesn't have a dev environment so I'd be curious to see if there's anything happening beyond what I'm seeing on 11/12.

lgrey added a comment.Jul 14 2022, 8:59 AM

To be perfectly honest, I'm not sure since the objc runtime issues are masking everything else! I'm going to send a separate change to suppress those and get back to you :)

On second thought: What I'm realizing is that suppressing ObjC runtime errors is not great, since they're so common. The stack still needs to be symbolized to do the suppression, so everything is very slow.

Instead of suppressing, WDYT about just ignoring the relevant regions? Details on why this happens here: https://docs.google.com/document/d/11h3N06tN-_n7nbwVK8qTRnWz3paICI_HlrVM8BvmZ9s

yln added a comment.Jul 15 2022, 2:50 PM

I'm working on enabling LSAN for Mac Chromium

Very cool and good to know! :)

Are there any public builders running these suites on 13? My 13 machine doesn't have a dev environment so I'd be curious to see if there's anything happening beyond what I'm seeing on 11/12.

Apple doesn't support LSan because we ship the leaks tool in our toolchain already that serves this purpose. I don't think we are running check-lsan anywhere.
However, I think that this mostly worked on macOS until recently (before macOS 13?) so I initially thought that your are fixing that problem.

Instead of suppressing, WDYT about just ignoring the relevant regions? Details on why this happens here: https://docs.google.com/document/d/11h3N06tN-_n7nbwVK8qTRnWz3paICI_HlrVM8BvmZ9s

Yes, I think that sounds like a better approach.

Now, that I better understand what this revision is trying to do my general feedback would be: if we fix a false positive, it would be nice to have a test for it!

lgrey updated this revision to Diff 458864.Sep 8 2022, 2:28 PM
lgrey retitled this revision from [lsan][Darwin] Scan libdispatch heap to [lsan][Darwin] Scan libdispatch and Foundation memory regions.
lgrey edited the summary of this revision. (Show Details)

Updated with test now that https://reviews.llvm.org/D133126 has landed.

I originally wanted the Foundation part to be a different patch, but it makes more sense to do it at once, since the scanning logic has to change anyway.

yln added inline comments.Sep 8 2022, 5:56 PM
compiler-rt/lib/lsan/lsan_common_mac.cpp
29–30

Can we get rid of this #ifdef here? I think we can drop support for compiling with SDK versions <= macOS 10.9 by now.
Maybe even inline the usage of the macros altogether?

38

Slight preference to make this an enum class and drop the k prefix:

enum class RegionKind {
  None = 0,
....

scan_state.seen_regions |= RegionKind::AllocOnce;
lgrey updated this revision to Diff 459465.Sep 12 2022, 8:20 AM
lgrey marked an inline comment as done.

enum -> enum class, remove memory tag aliases, clang-format

compiler-rt/lib/lsan/lsan_common_mac.cpp
38

Done (but a little uglier now since it needs casts if that makes a difference).

yln added inline comments.Sep 12 2022, 10:53 AM
compiler-rt/lib/lsan/lsan_common_mac.cpp
46

Would making this a SeenRegion field get rid of the casts?

yln accepted this revision.EditedSep 12 2022, 10:55 AM

LGTM, after my final nit. Let's give others a few days to chime in.

This revision is now accepted and ready to land.Sep 12 2022, 10:55 AM
lgrey added inline comments.Sep 12 2022, 11:27 AM
compiler-rt/lib/lsan/lsan_common_mac.cpp
46

Only if we implement operator| on it and do the casts in there. Given the number of uses it seems like a wash to me, happy to go with whichever you prefer.

yln added inline comments.Sep 12 2022, 11:49 AM
compiler-rt/lib/lsan/lsan_common_mac.cpp
46

Ahh, sorry I didn't realize this complication when suggesting using the enum class. TIL that enum classes and bitwise flags don't mesh well out of the box.

Since we are already halfway there, something like this should work, right?

inline AnimalFlags operator|(AnimalFlags a, AnimalFlags b)
{
    return static_cast<AnimalFlags>(static_cast<int>(a) | static_cast<int>(b));
}

Thank you and apologies for the detour.

lgrey updated this revision to Diff 459557.Sep 12 2022, 2:21 PM

Add operator| and operator|=, remove casting

yln accepted this revision.Sep 12 2022, 3:53 PM
lgrey added a comment.Sep 14 2022, 9:11 AM

I'll merge this later today if nobody has objections.