Code bloat caused by lack of nounwind attr on `drop_in_place`

compiler
1

As far as I can tell, rustc doesn't mark functions as NoUnwind/NEVER_UNWIND, apart from a handful of broad generalizations like panic=abort or extern "not *-unwind". I saw some scan of function bodies for coroutines, but not for regular functions.

The problem is that when a Drop::drop can't be inlined, it ends up being a potentially-unwinding call, and because unwinding calls can cause more drops, and panics during unwinding are not allowed, every drop then also generates an unwinding landing pad with and a call to panicking::panic_in_cleanup. This basically doubles the amount of drop-related code, and LLVM won't move nor unify calls to unwinding functions.

This affects Arc. It intentionally has a non-inlined drop_slow, which triggers the unwinding bloat:

6 Likes
2

I am surprised that LLVM doesn't seem to deduce "nounwind" itself. Maybe it's because drop_slow has external linkage and gets unwind tables generated, so LLVM can't trust it won't be replaced by the linker with a different function that unwinds? but I don't see LTO making a difference. It's recursive Drop + unwinding IR emitted by rustc.

It should be beneficial to have a MIR opt pass that marks functions as "nounwind" if they don't call any potentially-unwinding functions themselves.

2 Likes
3

In my case it's been made worse by struct + Drop + ? creating quadratically growing code bloat.

4 Likes
4

There are proposals to abort when drop panics, at which point all drop functions could be marked as non unwinding.

5

It usually does, so I would suggest poking at this more. There might just be a bug somewhere that could be fixed -- especially if even LTO doesn't solve it.

1 Like
6

I think it's because rustc emits invoke in the IR, which implies unwinding. LLVM's pass for adding nounwind uses:

/// Helper for NoUnwind inference predicate InstrBreaksAttribute.
static bool InstrBreaksNonThrowing(Instruction &I, const SCCNodeSet &SCCNodes) {
  if (!I.mayThrow(/* IncludePhaseOneUnwind */ true))
    return false;
  if (const auto *CI = dyn_cast<CallInst>(&I)) {
    if (Function *Callee = CI->getCalledFunction()) {
      // I is a may-throw call to a function inside our SCC. This doesn't
      // invalidate our current working assumption that the SCC is no-throw; we
      // just have to scan that other function.
      if (SCCNodes.contains(Callee))
        return false;
    }
  }
  return true;
}

which only allows call (CallInst).

This check is relevant for recursive functions. I happen to have Arc that is a recursive type, so Arc::drop_slow calls drop_in_place which may call Arc::drop_slow again. In this case rustc assuming that drop_slow may unwind makes it emit unwind-requiring invoke instruction when called recursively, which then cements its status as an unwinding function.

I don't know if LLVM checking for CallInst is intentional, or was it meant to be CallBase (which would include InstInvoke)

2 Likes
7

Hang on, is panicking in Drop actually guaranteed not to unwind? Panicking during unwinding results in an abort, which means panic-in-Drop is ill-advised, but I'm not sure the language has actually ruled it out.

Nothing in

even mentions that panicking while already unwinding results in an abort, let alone that panicking in Drop is always forbidden. Thus, as far as I can tell, panicking in Drop is legal, as long as it never happens in practice while another panic is being processed. (But hopefully I've just missed the appropriate reference!)

EDIT: oops, failed reading comprehension

8

Yes, that is the status quo. There have been suggestions to make panicking in drop abort, as @CodesInChaos linked above, but so far that has been considered too much of a breaking change for existing code (at least, if implemented unconditionally).

4 Likes