Thanks for the explanations and your patience. This patch LGTM, modulo resolution of @efriedma's concerns (so I'll leave it to him to mark accepted).

In D105207#2924918, @chrisjackson wrote:

Apologies for thre slow response, I currently have covid so I needed some time off. Thanks for reporting the failure I will attempt to address the assertion failure today. If I fix that then I am keen for this to be backported.

In D105207#2922863, @JosephTremoulet wrote:

We've got a regression (assertion failure) with this change that didn't get fixed with 2537120c870c. I've opened PR 51329 for it. If this change gets backported to release/13.x, we'll need the fix for this assertion failure to also get backported.

Please see D107348 for a simple patch that provides a fix.

Thanks for the quick fix!

We've got a regression (assertion failure) with this change that didn't get fixed with 2537120c870c. I've opened PR 51329 for it. If this change gets backported to release/13.x, we'll need the fix for this assertion failure to also get backported.

LGTM assuming you clang-format the patch.

In D101541#2726412, @nikic wrote:

LGTM, but I'd suggest waiting a bit in case there are further comments -- it's a tricky area.

In D101541#2732474, @nlopes wrote:

There's this comment in isNonEscapingLocalObject:

// If this is an argument that corresponds to a byval or noalias argument,
// then it has not escaped before entering the function.  Check if it escapes
// inside the function.

Which supports this patch. Though this claim doesn't seem very clear to me. e.g. we mark memcpy's arguments with noalias. It's not guaranteed case that the input pointers haven't escaped before. So the exact meaning of noalias is not clear to me.
I've read the C11 standard re restrict, and it is omissive w.r.t. to int2ptr casts. Though I think this patch respects the spirit of the standard. We just need to nail the exact semantics of noalias at some point..
TL;DR: LGTM.

In D101541#2727752, @aqjune wrote:

isNonEscapingLocalObject returns true if the pointer is a noalias argument, and it's worth clarifying why this is okay somewhere.

The noalias argument case is slightly different from other cases because its address could have been ptrtoint-ed already. For example:

%i = ptrtoint i8* %ptr0 to i64
call void @f(i8* %ptr0, i64 %i)

define void @f(i8* noalias %ptr, i64 %i) {
  ; access %ptr <- isNonEscapingLocalObject(%ptr) is true, but
  ; access inttoptr(%i) <- this can exactly touch %ptr.
}

It isn't clear whether the second access is successfully done according to the definition of based-on in LangRef.
Should we clarify that noalias ptr (%ptr) is not based on the original pointer (%ptr0)?

In D101541#2725915, @nikic wrote:

I believe this is correct, and also not affected by open ptrtoint/inttoptr issues.

• Include negative tests and alloca as well as noalias parameter

I've verified this doesn't break anything in check-all or in test-suite with SpecInt2017 included. I'm happy for suggestions if there are other testing paces this should be put through. Thanks.

In D96626#2675166, @clayborg wrote:

Since no one else is responding, lets try this out and see how things go.

Ping

Ping. I'd like to figure out how to make forward progress with this. A couple points:

In D96626#2640715, @labath wrote:

I haven't checked the manpage or the linux kernel behavior, but intuitively, I would expect that a shared mapping should not trigger an OOM, because at any point the kernel can decide to flush the dirty pages to disk (that's actually one technique for creating a per-process swap file).

Thanks for taking a look!

In D96626#2600946, @clayborg wrote:

I would mark as accepted but I am not the code owner for llvm/lib/Support/Unix.

ping

Ping

In D96626#2561573, @krytarowski wrote:

MAP_NORESERVE is dead on NetBSD as this is the default mode always.

What led me here was failure in LLDB to open large core dumps because the mmap call was failing with the default memory overcommit policy. It's easy to override the policy as a workaround, but it would be nice not to have to. I considered making this a configurable attribute of mapped_file_region (as in a constructor parameter specifying whether OOM failure should be prompt or delayed), which could then be plumbed through so specifically opening core files in LLDB requests it, but I figured this should be consistent across the "Unix" and "Windows" implementations, and AFAICT there's not a good way to have a Linux-style eager-commit/failure on the Windows side, and since delaying the failure is the Windows behavior already, this change does make things consistent. So this is what I ended up with.

LGTM (and thanks for the fix and apologies for the break) )but I think @tstellar should have a look given it's fixing a build break in the release/11.x branch caused by my porting of changes from master for https://bugs.llvm.org/show_bug.cgi?id=48053

Oh, I see. I made this change in the master branch and then ported it to the release/11.x branch. It changed the signature of a virtual method, which had a couple more overrides in the release/11.x branch, and when I ported the change I didn't realize those other overrides existed. So now the release/11.x builds that include those overrides (btw, what build flags are needed to include those?) are broken. I'm not sure what the options are now that 11.0.1 has been released and 11.1.0 is at rc2 -- @tstellar, thoughts?

In D89156#2530353, @Abpostelnicu wrote:

Shouldn't this also update PlatformiOSSimulator.h?

Could you also add the switch test case from PR48812?

• Add testcase from PR48812

• Match comment to code

• Cast the terminator, not the block

This is meant to address a regression from rG7f69860243e8, which moved a check for whether dynamic cast to BranchInst succeeds in UnrollLoop from before to after the call to peelLoop. Adding a check to canPeel seemed like the most logical option to me.

• Fix comment

OK, I believe this is correct now.

• Ignore old modules not in target list

• Expand logging as suggested in D89157

But then you get to a point where you shouldn't really have multiple modules replacing a single one so you aren't really sure what to do about it. That part makes me a little uneasy.

Thanks for the review!

I'd been hoping to get this bugfix into 11.0.1, but it (and D89156, which is just some cleanup that occurred to me while working on this code) have been stalled for a few weeks, and I don't have any insight into the reason. If there's anything i can do to help either one along, please let me know. One idea I had is that I could rebase this on master so it doesn't depend on the other, would that help?

Ping

In D89156#2328772, @jingham wrote:

IIUC, the problem with ReplaceModule will get fixed in a dependent patch, right? So the last bit of this patch will get cleaned up by that change. If that's right, then this LGTM.

• Comment new parameter

Thanks for the speedy review! I've updated it to include a testcase that shows the problem you described, and include the fix you suggested.

• Handle finding wrong version before right one

Yeah, the potential for quadratic explosion is why I didn't hook this flag up to expensive checks when I added it. It occurs to me now that if we want, we could presumably have some cutoff counter so that after inserting 300 instructions or whatever limit is reasonable, we start hashing normally. We'd have to empty the table and repopulate it with the normal hash when we cut over, but that should be doable I think (it's similar to what the table does internally when it grows).

To clarify, the failure did not reproduce for me in the SUSE 15.02 container. I don't know what I'm doing differently. To create the environment, I just did docker run ... opensuse/leap:15.2, and then zypper in gcc-c++ python3-devel inside the container.

FWIW, I tried running the test in an SUSE 15.2 docker container (on an Ubuntu 18.04 host), and here's what I'm seeing. raise and abort indeed do not indicate that they had the S set in the augmentation:

eh_frame augmented UnwindPlan:
...
This UnwindPlan is for a trap handler function: yes.

And its parent 'raise' is set so as well because of 'GetNextFrame()->m_frame_type == eTrapHandlerFrame '.

LGTM, thanks.