[PATCH] D120305: [Driver] Default CLANG_DEFAULT_PIE_ON_LINUX to ON

Fri Feb 25 22:11:01 PST 2022

MaskRay added a comment.

In D120305#3347058 <https://reviews.llvm.org/D120305#3347058>, @nemanjai wrote:

> 

> Statements like this seem to be at odds with this community's culture (or at least the way I understand it).
> As long as I have been a member of this community, the guidance for patches that break bots is to fix it immediately if the fix is obvious/trivial and if it isn't, to pull the patch until a solution can be found. I am not aware of any changes to this policy. I would also like to add that this approach serves most other members of the community quite well and at least I personally don't see much opposition to this. Frankly, the only person I have ever received a response that amounts to "I would rather not" when asking them to pull a patch that breaks bots is yourself.

Hi @nemanjai, when I mentioned "as I watch sanitizer failures quite closely and ppc64 often tends to be the outlier thing", I was talking about the build bots.
However, you seem to take it very personally, conflate it with a judgement to the PowerPC target, and try to defend for the PowerPC target.
I think perhaps you become too emotional from this reply. Hope we can clam down.

> So I'll try to respond to the individual statements you have made here.
>
> No access to a PowerPC machine - we have given you access to one before and are happy to do so again at any time.

Ack. Thanks for that.

> "bluntly requesting to pull the patch" - This is perhaps the part of your statement that I find most surprising. In case you haven't come across this, I encourage you to have a read through it. If you feel this needs to change, I encourage you to bring it up for discussion with the wider community. Of particular interest is this sentence: We strongly encourage “revert to green” as opposed to “fixing forward”. We encourage reverting first, investigating offline, and then reapplying the fixed patch - possibly after another round of review if warranted.

Sure, I know this policy. See below.

> "there is a better approach" - I don't think I have to spend a lot of time explaining how subjective this statement is.
> "there is something that a bot maintainer can do" - I can't quite decipher whether this is a disingenuous statement pretending that this is a situation where the bot maintainer (effectively myself in this case) isn't willing to help. Or perhaps it is a statement you made in the heat of the moment when I asked you to pull the patch and you missed the part where I offered help to resolve the issues when normal business resumes. I will give you the benefit of assuming the latter as I truly don't believe you have ill intentions here. I would just like to add that, as you surely realize, bot maintainers are not sitting around waiting for someone to break their bot so they can jump in immediately and work on resolving the issue. As contributors to this project, it is our responsibility to keep the tip of trunk green and to work with bot maintainers on their own time (within reason) to resolve issues we cause on their bots.

The current issue is that one ppc64 bot (clang-ppc64le-rhel) was complaing about some sanitizer tests while other ppc64 bots testing sanitizers (e.g. clang-ppc64le-linux-multistage sanitizer-ppc64be-linux sanitizer-ppc64le-linux) were apparently happy.
This is an important factor making me believe this is a build bot stability issue, not the patch's fault.

>From the failure log, the diagnostic looks like:

  <stdin>:1:11: note: possible intended match here
  error: Segmentation fault (core dumped)

This is weird. This was not the first time I saw this "Segmentation fault".
In the past, I saw GNU ld crashes or similar weird segfaults (on perhaps this bot).

-fPIE -pie is a very common configuration tested extensively by users and a bunch of ppc64 distributions.
In the downstream distributors typically patch Clang to behave the similar way as GCC, as this CMake patch tried to do to reduce downstream patch/customization burden.

Making use of my expertise and experience, I put up this statement fairly responsibly: this really looks like the problem with this bot, or sanitizer tests got enabled while some inherent stability issues havn't been addressed.

Perhaps sanitizers are not so compatible with the bot's environment (say glibc).
If so, the issue will pop up from time to time and a bot maintainer may jump out to every patch which tickles the problem.

This is just going to waste developer time.
There is a very high probability that a patch author may shortgun and revert the wrong patch, causing unneeded churn.

I understand that a bot maintainer does not bandwidth to watch every change.
But this is the very time that we would appreciate that a bot maintainer did a little work just to make everyone happy by drop testing the flaky tests.
I realized that I would not suggest adding UNSUPPORTED to test files since that would add churn to the history.
The right call is to disable such tests for the particular bot, or let it use CLANG_DEFAULT_PIE_ON_LINUX=off for now, which I did in https://github.com/llvm/llvm-zorg/commit/b6ddf02ce3a54da2df29e7e599b1838167e0e3ad

> The rather surprising and discouraging statement you made under (c) - while I realize that PowerPC may not be a target on which you develop, it is really not fair to make a blanket statement that PowerPC is not stable wrt. sanitizers. If you feel there is a stability issue with a specific bot or a specific target, I encourage you to collect data about false failures and bring it up with us - we would be happy to look into it as I am sure would any other target/bot maintainer. Statements like this sow the seeds of resentment towards specific targets - you are effectively saying that PowerPC is an unstable target (at least wrt. to sanitizers) but we insist on burdening the community with our unstable target by running sanitizer tests in our bots. I am afraid that unlike number 4. above, I don't find any ambiguity in your statement here. Your statement seems to be clearly and unambiguously hostile towards PowerPC and as such, I find it at odds with the spirit of this community. Regardless of all of that, I would once again like to reiterate that I would be very happy to look into false failures you are encountering with sanitizers on PowerPC.

Everyone can prove the issue on my behalf.

Open https://lab.llvm.org/buildbot/#/builders/57 , click "Success Rate" tab, the rate is lower compared to other bots recently, even lower than some other ppc64 bots.
Perhaps other ppc64 sanitizer bots are quite good and this is just a problem at least in this bot.

Perhaps open https://lab.llvm.org/buildbot/#/builders/57?numbuilds=1000 : it failed quite frequently in the past.

> Fangrui, I would really like you to understand that I very much value your contribution to various LLVM projects. You are an exceptional developer and seem to be laser focused on advancing the state of LLVM and its subprojects. This is not at all lost on me. However, you need to understand that this community has all kinds of participants, many of which also care about older or somewhat esoteric targets, operating systems, toolchains, etc. As such, there will be situations such as this where it is definitely the case that most != all and there may have to be some nuance in applying changes. At the very least, such situations may require slowing down, pulling a patch or two, re-evaluating and finding the best way forward. And it is important to recognize that sometimes the best way forward may not be something that all parties prefer, but at least all affected parties need to be part of the discussion.
>
> It is unfortunate that a discussion in a patch review devolved into a discussion on the philosophy of community participation, but I felt that you made strong statements that I needed to address. While I won't continue this discussion in this forum, I would be happy to meet with you virtually and discuss this.

Thank you for the praise:) But please see my first paragraph in the reply. You perhaps become too emotional from this reply.
I do not see how my previous reply implied anything disrespect to the PowerPC target.
I am going to say I do not necessarily agree with your implicit claim that I do not care about PowerPC: I have made ~191 changes mentioning PowerPC or PPC.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120305