[PATCH] D69428: [GlobalOpt] Remove valgrind specific hacks (revert r160529)

James Y Knight via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Apr 14 09:35:18 PDT 2021


jyknight added a comment.

In D69428#2689038 <https://reviews.llvm.org/D69428#2689038>, @evgeny777 wrote:

>> As far as I can tell, the breakage of -fsanitize=leak was not known prior to the commit
>
> What kind of breakage? According to @MaskRay (I've just verified his code) LSAN detects memory leak when valgrind hack is reversed, so there is at least some improvement, not breakage.

MaskRay's example does not show an improvement in LSAN behavior, but rather a new false positive. Memory which is still accessible at process termination is not a leak, so the source -- as written -- had no leak, and should ideally not be reported as such.

> Even if LSAN needs to keep pointer globals for some reason then it can do so from instrumentation pass, because LSAN is part of LLVM.

Perhaps that's the best answer in the end.

> I see no reason why hacks should reside in LLVM trunk to appease some third party tools.

Well, there is a reason. It's pretty generic: if (some) users expect to be able to be able to use such tools, then breaking them -- with no possible recourse -- hurts such users. Of course, even knowing that you're breaking users, sometimes it can turn out that other factors are ultimately deemed more important. For example, the change might help many other users, or reduce complexity in the compiler significantly. Those factors could be judged to outweigh the breakage.

Yet, even when you judge other considerations to have a higher value, it is useful to recognize that this is a judgement on relative values, and not an absolute truth: the thing being broken had //some// non-zero value. So, it could be more accurate and less confrontational to say something like "I don't think it's worth reducing the optimization potential of LLVM to appease third-party tools like Valgrind or Google's leak checker, when LSAN can do the same job these days", instead of saying there is "no reason". (Assuming LSAN is fixed, of course.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69428



More information about the llvm-commits mailing list