On Tue, Oct 26, 2021 at 05:11:29PM -0400, Jeff King wrote:
> On Tue, Oct 26, 2021 at 04:23:14PM -0400, Taylor Blau wrote:
>
> > So this all feels like a bug in ASan to me. I'm curious if it works on
> > your system, but in the meantime I think the best path forward is to
> > drop the last patch of my original series (the one with the three
> > UNLEAK() calls) and to avoid relying on this patch for the time being.
>
> Bugs aside, I'd much rather see UNLEAK() annotations than external ones,
> for all the reasons we introduced UNLEAK() in the first place:
>
> - it keeps the annotations near the code. Yes, that creates conflicts
> when the code is changed (or the leak is actually fixed), but that's
> a feature. It keeps them from going stale.
I agree completely. I noted as much in my message here:
https://lore.kernel.org/git/YXJAfICQN8s5Gm7s@nand.local/
but Ævar made it sound like his work would be made much easier without
the conflict. Since I'm not in any kind of rush to make t5319 leak-free,
I figured that queueing the parts of that series that wouldn't conflict
with Ævar's ongoing work would be a net-positive.
> - leak-checkers only know where things are allocated, not who is
> supposed to own them. So you're often annotating the wrong thing;
> it's not a strdup() call which is buggy and leaking, but the
> function five layers up the stack which was supposed to take
> ownership and didn't.
TBH, I find this much more compelling. Either way, I don't feel strongly
enough to deviate from v2 much, and I don't want to create more work for
Ævar along the way.
Thanks,
Taylor
