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

Wed Apr 14 07:10:26 PDT 2021

jyknight added a comment.

(Disclaimer: I'm also employed by Google).

In D69428#2688409 <https://reviews.llvm.org/D69428#2688409>, @xbolva00 wrote:

>>> I think it's unfair what just happened here. Google had over a year to comment and ended up wasting people's time.
>
> True. Why the LLVM project should suffer because Google is unable to take action for such long time?

It was certainly quite unfortunate that nobody responded to questions posed here. It could've avoided unnecessary strife if this conversation had started sooner. Yet, in the end, everyone's goal is to have a good compiler -- nobody wants the LLVM project to suffer.

> Rather than provide any reasonable action plan (maybe temporarily provide option for google/others to leave this hack on for them), they just reverted it without proper review/discussion to possibly solve this "issue" with a different way  :/ Just OK from google-colleague is not enough.

I believe the revert was consistent with the long-standing LLVM reversion policy (which was recently codified in https://llvm.org/docs/DeveloperPolicy.html#patch-reversion-policy). There were real problems identified with this change -- it breaks both `clang -fsanitize=leak` and external leak-checkers, and a current user of external leak-checking (Google) came forward to say that they were broken by it.

As far as I can tell, the breakage of `-fsanitize=leak` was not known prior to the commit, nor was it known that this change would cause active breakage for Google's current use of an external leak-checker. (It's unfortunate and frustrating that the question was raised, yet not answered in the affirmative until now, but I don't think that changes the ultimate outcome.)

So, now we have two new pieces of information, which were not known at the time of commit. That is reasonable grounds for a revert while discussion takes place as to what the next step ("action plan") should be.

>>> I find it quite worrying/unacceptable that $BIGCORP believes they get to have the final say as to what happens in $PROJ.
>
> +1.

I don't think it's helpful to look at it that way. _Everyone_ gets to point out problems, even after commits have gone in. If there are issues, the first answer to revert while discussion on the proper resolution to the issue takes place. A patch reversion in the LLVM process is not a "veto" or a "final say", it is a step to take during discussion. One possible outcome of the discussion could be consensus that the commit was good after-all and should be reinstated as is, despite the issues identified. If so, it will be a more-educated conclusion, properly taking into account those now known issues.

I hope/expect that further discussion on this review can result in an outcome to everyone's satisfaction without any need for escalation. Yet, if disagreement on a review is not resolving, there are escalation mechanisms in place to help resolve the issue. First, the question can be brought up on llvm-dev for wider input. That's usually sufficient. If that //also// fails to resolve the issue, it can be escalated to the formal decision-making process (https://github.com/llvm/llvm-www/blob/main/proposals/LP0001-LLVMDecisionMaking.md). (I mention that mostly for completeness, I seriously doubt this could get that far.)

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69428