[PATCH] D59005: Fix memory leak in CreateSigAltStack

[Pietro Fezzardi via Phabricator via llvm-commits]

fez added a comment.

> if you deliver another signal after unregistering all of them, you get the behavior you observed? I'm not sure whether we care about clang-as-a-library being loaded / unloaded in the same program. If not, we could limit signal registration to be a one-time thing.

That's right. That is exactly what happened when I first stumbled on the problem.

I tried to simplify it to a minimal reproducible example with Valgrind. Here's how I realized that even running `valgrind --leak-check=full opt -o /dev/null /dev/null` reported the leak, whereas with my fix the error report was going away (BTW have you had the chance to try it? It runs in just a few seconds even with Valgrind).

I think there are two things to point out about this situation.

- With the suggested fix the error report from Valgrind goes away _and_ also the memory leak with register/unregister goes away, which means that yes, you could prevent a leak if someone has clang-as-a-library loaded/unloaded in the same program (even if I admit that is not something many people are going to do, even in my case I ended up doing something else).
- Valgrind is reporting a false positive on the memory leak with `valgrind --leak-check=full opt -o /dev/null /dev/null`. I just double-checked, with that command line `RegisterHandlers` is being called only once, which means that there isn't actually any memory leak. Anyway, the fix makes the false positive effectively go away, which I think is a nice thing because in this way you get clean runs with Valgrind.

Both things might not be all that important, but the patch is pretty self-contained and I don't see any downsides.

What do you think?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59005/new/

https://reviews.llvm.org/D59005