core: Inline From<AllocErr> for CollectionAllocErr

[alexcrichton]

This shows up in allocations of vectors and such, so no need for it to not be
inlined!

[rust-highfive]

r? @Mark-Simulacrum

(rust_highfive has picked a reviewer for you, use r? to override)

[alexcrichton]

r? @sfackler

[sfackler]

@bors r+

[bors]

📌 Commit 9e73690 has been approved by sfackler

[kennytm]

I suspect this PR causes the failure of #49875 (comment). (The image is dist-x86_64-musl, the test is collections::hash::map::test_map::test_try_reserve which panics with isize::MAX + 1 should trigger an OOM!)

[alexcrichton]

@bors: r-

Yes this is the correct diagnosis. No idea how this would cause that issue, but I'll diagnose locally.

[alexcrichton]

@eddyb do you think you could help me debug this? I've minimized this without many dependencies which fails on today's nightly. If you run in the playground it panics when the program should exit successfully.

I've bisected this historically in nightlies to this range which is most likely pointing to #45225. I'm having trouble pinning down precisely where the error is being introduced in trans or in the IR, and this may also be related somewhat to the funky __rust_alloc ABI (but I don't think so?)

In any case I'm curious if you can see anything "obviously wrong" in the codegen I may be missing

[eddyb]

If you look at the "can this call instruction write to this pointer" code, you can see this special-case:

  // If the CallSite is to malloc or calloc, we can assume that it doesn't
  // modify any IR visible value.  This is only valid because we assume these
  // routines do not read values visible in the IR.  TODO: Consider special
  // casing realloc and strdup routines which access only their arguments as
  // well.  Or alternatively, replace all of this with inaccessiblememonly once
  // that's implemented fully.
  auto *Inst = CS.getInstruction();
  if (isMallocOrCallocLikeFn(Inst, &TLI)) {
    // Be conservative if the accessed pointer may alias the allocation -
    // fallback to the generic handling below.
    if (getBestAAResults().alias(MemoryLocation(Inst), Loc) == NoAlias)
      return ModRefInfo::NoModRef;
  }

What this means in practice is that __rust_alloc can be assumed by LLVM to never write to the AllocErr pointer, and thus inlining anything reading the AllocErr will see undef.

However, in @alexcrichton's examples it's not the AllocErr being corrupted, but a value containing it. Well, that's the connection with #45225: the outer enums reuse AllocErr's tag, and rely on it being either 0 or 1 to indicate a valid AllocErr value, whereas the outer enums use 2 and 3.
When LLVM replaces the load of the tag with undef it ends up branching to the case indicated by the 3 value of the tag (completely arbitrarily AFAICT), whereas the real value is either a 0 or a 1.
So to reproduce before #45225, you would want to match on the AllocErr itself.

[alexcrichton]

Thanks so much for the help debugging this @eddyb!

It sounds like I'll block this on #49669 which will change the signatures here anyway

[bors]

☔ The latest upstream changes (presumably #49669) made this pull request unmergeable. Please resolve the merge conflicts.

[alexcrichton]

@bors: r=sfackler rollup

[bors]

📌 Commit 2e73e76 has been approved by sfackler

[kennytm]

@bors rollup-

This PR might still be too risky to rollup. See #49904 (comment).

[bors]

⌛ Testing commit 2e73e76 with merge bd40cbb...

[bors]

☀️ Test successful - status-appveyor, status-travis
Approved by: sfackler
Pushing bd40cbb to master...